-
Notifications
You must be signed in to change notification settings - Fork 251
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
Weights in generated config #131
base: master
Are you sure you want to change the base?
Conversation
lib/synapse/haproxy.rb
Outdated
@@ -719,6 +720,7 @@ def generate_backend_stanza(watcher, config) | |||
backend = backends[backend_name] | |||
b = "\tserver #{backend_name} #{backend['host']}:#{backend['port']}" | |||
b = "#{b} cookie #{backend_name}" unless config.include?('mode tcp') | |||
b = "#{b} weight #{backend['weight']}" if backend['weight'] && backend['weight'].is_a?(Fixnum) and backend['weight'] > 0 and !@opts['ignore_weights'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to ignore 0 weight backends because that seems like a reasonable use case? (I believe that nerve supplies nil if there is no weight).
What do you think?
Looks reasonable, can you check that this works with the state file (we should/will add tests of that soon)? Also this doesn't have code for doing updates over the socket, but we presumably want that. Is that going to come in a follow up patch? |
spec/lib/synapse/haproxy_spec.rb
Outdated
expect(subject.generate_backend_stanza(createmockwatcher([{ 'weight' => 1, 'host' => 'somehost', 'port' => '5555'}]), mockConfig)).to eql(["\nbackend example_service", [], ["\tserver somehost:5555 somehost:5555 cookie somehost:5555 weight 1 check inter 2000 rise 3 fall 2"]]) | ||
end | ||
|
||
it 'generates backend stanza withour bad weight' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo here.
@jolynch ack - thanks for the feedback. I'll add tests around the state file, and I had totally forgotten the weight updates over the socket. I'd however prefer to do that as a separate patch to keep things relatively simple. Does that work for you? |
My only issue with doing the socket update in a subsequent change is that something else will have to trigger a HAProxy reload to pick up any change in weights (since this patch neither informs the haproxy module that it needs to reload nor changes the weights over the stats socket). At the very least I think it should inform haproxy to restart? |
#140 shows a technique for noticing things have changed via the state file. Perhaps we should just make the state file mandatory ... |
be ignored. This commit is based on: airbnb#131 but does the following things differently: 1. By default weights are ignored. This is to maintain current behavior for safety. 2. HAProxy will reconfigure if weights changes.
be ignored. This commit is based on: airbnb#131 but does the following things differently: 1. By default weights are ignored. This is to maintain current behavior for safety. 2. HAProxy will reconfigure if weights changes.
cc @vulpine
Rebased against airbnb/synapse master