Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some functions: add_server(),add_peer(),remove_server(),remove_peer() and it's support round_robin or ip_hash. #13

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

wangfakang
Copy link

Hi, agentzh :
I did the following there points Pls review the code, and thx.
First: Add functions: add_server(), add_peer(), remove_server() and remove_peer() and they support load balance algorithm: ip_hash or round_robin.
Second: In order to support UPSTREAM_CHECK_MODULE, I add the ngx_http_lua_upstream_module.h head file. In this file, there are save some structs/variables which are used in health check module.
Third: Update README.md and add some test for add_server() / add_peer() / remove_server() / remove_peer().

…peat add same server.

Second: add api add_peer and it's support ip_hash or round_robin.
Third:  in order to support UPSTREAM_CHECK_MODULE and add a ngx_http_lua_upstream_module.h head file(save some variables on health check module)
        and add api: remove_server remove_peer and it's support round_robin or ip_hash.
Fourth: update README.md and add some test for add_server add_peer remove_server remove_peer(thanks for chunshengster@gmail.com for guide).
@chunshengster
Copy link

Hi,@agentzh , @wangfakang is the member of my team, Plz review the code, if anything wrong, plz contact me or @wangfakang .

}

if (uscf->servers == NULL || uscf->servers->nelts == 0) {
//TODO: 对于 默认的空upstream来讲,nginx当前会不允许其启动,可以考虑调整策略,允许此种情况下nginx启动
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No Chinese comments please.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: And no C++ style line comments. This is required by the nginx coding style.

@agentzh
Copy link
Member

agentzh commented Aug 11, 2015

Hi guys. Thanks for contributing. I've found so many coding style issues in this patch. So many that I'm tired of commenting in the patch. Please fix them before I can review your logic. Thanks a lot!

@wangfakang
Copy link
Author

Hi @agentzh, Thanks for your code review. I'll update code as soon as possible.

@agentzh
Copy link
Member

agentzh commented Aug 12, 2015

Since adding add/remove peers is inevitably hacky, I wonder if the upcoming balancer_by_lua directive in ngx_lua will fulfil your requirements. Basically we will allow using Lua to write nginx upstream balancer directly where you can specify a dynamic set of peers to try for each request (if you wish) and define dynamic failover policies without hassle. This is much more flexible than the add/remove peer APIs you propose in this PR and its implementation is also much cleaner and simpler. What do you think?

…ow support load balance algorithm: ip_hash/round_robin/least_conn.
@wangfakang
Copy link
Author

@agentzh Oh, its a good idea, Im looking forward to try it, I have some questions that through balancer_by_lua directive to realize dynamic upstream can work with all the existing balancer modules like least_conn, ip_hash ? Id like to know the implementation principle of balancer_by_lua, could you give me some tips and I'm glad to learn from your idea thanks :D.

@agentzh
Copy link
Member

agentzh commented Aug 13, 2015

@wangfakang least_conn and ip_hash assume the round-robin upstream module at the bottom. Since balancer_by_lua can replace round-robin altogether, I need to think more about getting existing upstream modules work with balancer_by_lua at the same time. Or we can simply re-implement those least_conn and ip_hash logic in pure Lua. Their logic is very simple anyway. And this way can be way easier (and also more flexible).

@wangfakang
Copy link
Author

@agentzh Thank you, I've learned. It's really flexible, so that we can achieve some business logic in the upstream module using balancer_by_lua is more simpler :).

@agentzh
Copy link
Member

agentzh commented Aug 14, 2015

@wangfakang Yeah, that was the goal of balancer_by_lua. We needed for per-request load balancing policies at CloudFlare. It would be out in next 2 months or so.

@wangfakang
Copy link
Author

@agentzh thanks for CR, Im update code style and some logic, now they support load balance algorithm: ip_hash, round_robin or least_conn.

@aledbf
Copy link

aledbf commented Mar 16, 2016

@wangfakang any update on this? Thanks

@agentzh
Copy link
Member

agentzh commented Mar 16, 2016

@aledbf Now it's recommended to use balancer_by_lua* for such kind of things.

@wangfakang
Copy link
Author

@aledbf As agentzh put it. dynamic manage upstream clould use balancer_by_lua* and it's very flexible.
Now our internal using lua-upstream-nginx-module provide atomic rest http about dynamic manage upstream. Using consul/etcd or redis as service discovery to automation add or remove upstream server.

wfarr added a commit to Shopify/lua-upstream-nginx-module that referenced this pull request Jul 14, 2016
This is based heavily on the work in openresty#13
but involves slightly fewer changes to the module as a whole.
wfarr added a commit to Shopify/lua-upstream-nginx-module that referenced this pull request Jul 15, 2016
This is based heavily on the work in openresty#13
but involves slightly fewer changes to the module as a whole.
@woodgear
Copy link

woodgear commented Nov 9, 2021

. Or we can simply re-implement those least_conn and ip_hash logic in pure Lu

@agentzh ip_hash is easy, we have all information we need, but how could i get the current connection number in all upstream?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants