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

Weights in generated config #131

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

Conversation

bobtfish
Copy link
Contributor

cc @vulpine

Rebased against airbnb/synapse master

@@ -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']
Copy link
Collaborator

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?

@jolynch
Copy link
Collaborator

jolynch commented Jul 28, 2015

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?

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
Copy link

Choose a reason for hiding this comment

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

Minor typo here.

@bobtfish
Copy link
Contributor Author

bobtfish commented Aug 3, 2015

@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?

@jolynch
Copy link
Collaborator

jolynch commented Aug 3, 2015

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?

@jolynch jolynch mentioned this pull request Aug 27, 2015
@jolynch
Copy link
Collaborator

jolynch commented Sep 7, 2015

@bobtfish #100 seems rather similar to this (adding metadata to backends that HAProxy uses), can we sync up about how we want to be able to add metadata to nodes? I'm not against adding a bunch of top level keys, but I think we should at least have a plan.

@jolynch
Copy link
Collaborator

jolynch commented Oct 3, 2015

#140 shows a technique for noticing things have changed via the state file. Perhaps we should just make the state file mandatory ...

minkovich added a commit to minkovich/synapse that referenced this pull request Feb 19, 2016
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.
scarletmeow pushed a commit to scarletmeow/synapse that referenced this pull request May 27, 2016
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.
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.

3 participants