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

Support weighting #7

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

Support weighting #7

wants to merge 7 commits into from

Conversation

hulining
Copy link
Contributor

@hulining hulining commented Apr 10, 2023

  • Use table instead of shared memory to store "service" objects to support weights.
  • Use while not ngx.worker.exiting() do instead of while true do to fix the problem that "nginx worker: process is shutting down" process of nginx cannot shutdown when reload nginx.

@xytis
Copy link
Owner

xytis commented Apr 10, 2023

Sadly this is a breaking change for the existing library.

It explicitly moves away from shared memory and does switch to a better round robin implementation.

I would consider merging this PR in two different pieces:

  1. The worker shutdown fix can merged as is, just as a separate PR.
  2. The move from existing round robin implementation should be somehow isolated to prevent breaking something among existing users. I would recommend making a new file, maybe named 'local_consul_balancer.lua' and extract new features there. For added benefit, you could take out common code and move it to 'common.lua' in the same directory.

Also, you should then update readme file, pointing out that your superior implementation is available from another include, meanwhile leaving the old implementation as backward compatible option.

@xytis xytis self-requested a review April 10, 2023 17:38
local address = s["Address"] ~= "" and s["Address"] or na
local port = s["Port"]
-- If the weight parameter fails to be obtained or is passed incorrectly, set it to 1
local weight = s["Weights"]["Passing"] or 1
Copy link
Owner

Choose a reason for hiding this comment

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

This lookup may unwind the stack if response structure does not contain 'Weight' key.

I could not determine which consul version added this field into the API response.
If this key aways exist, then the error handling part of the following code is unnecessary.
If this key exists only after some Consul version, then all clients using earlier version will stop functioning permanently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to solve the consul issues #1088 and 4198, the Weights field was added in cosnul 1.2.3 version,See consul-1.2.3 AgentWeights and #4468.

Also, When I tested with Consul v1.4.0, the response will always have the Weights field by default, whether or not I passed in the Weights field when I signed up for the service.

Copy link
Owner

Choose a reason for hiding this comment

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

Then just as a precaution, slap in Readme that this new code only works with Consul >1.2.3 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add dependencies later.

_persist(service_name, service)
local ok, err = balancer.set_current_peer(upstream["address"], upstream["port"])
local server = rr_up:find()
local ok, err = balancer.set_current_peer(server)
Copy link
Owner

Choose a reason for hiding this comment

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

I could not find firm proof that this function behaves correctly if passed a string containing ':' separated host/port pair.

I checked here: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.md#set_current_peer
and here: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.lua#L109

Please verify this by testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The find() method does return a string of ip:port, and as you can see from the lua-resty-balancer example, the ip:port string can be passed directly to the set_current_peer() method as an argument.

After testing, it is feasible, it is possible that the set_current_peer() method is internally compatible.

@hulining
Copy link
Contributor Author

Sadly this is a breaking change for the existing library.

It explicitly moves away from shared memory and does switch to a better round robin implementation.

I would consider merging this PR in two different pieces:

  1. The worker shutdown fix can merged as is, just as a separate PR.
  2. The move from existing round robin implementation should be somehow isolated to prevent breaking something among existing users. I would recommend making a new file, maybe named 'local_consul_balancer.lua' and extract new features there. For added benefit, you could take out common code and move it to 'common.lua' in the same directory.

Also, you should then update readme file, pointing out that your superior implementation is available from another include, meanwhile leaving the old implementation as backward compatible option.

Sorry, I didn't take these into consideration. I will resubmit PR later.
PR1. Fix the worker shutdown problem.
PR2. Implement weight support on the basis of saving the original implementation, update the README file and add dependencies.

@hulining
Copy link
Contributor Author

  1. The worker shutdown fix can merged as is, just as a separate PR.

A new independent PR has been submitted to address the above issues. See #8 .

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.

2 participants