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

Create an option to stop ignoring weights. By default weights will still be ignored. #174

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ This section is its own hash, which should contain the following keys:
* `listen`: these lines will be parsed and placed in the correct `frontend`/`backend` section as applicable; you can put lines which are the same for the frontend and backend here.
* `backend_order`: optional: how backends should be ordered in the `backend` stanza. (default is shuffling). Setting to `asc` means sorting backends in ascending alphabetical order before generating stanza. `desc` means descending alphabetical order. `no_shuffle` means no shuffling or sorting.
* `shared_frontend`: optional: haproxy configuration directives for a shared http frontend (see below)
* `ignore_weights`: optional: stops haproxy backend 'weight' options being generated, even if the Nerve registrations contain this information. This will cause all backend servers to be treated equally by haproxy. This defaults to true so weights will *NOT* be used by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have strong feelings about this being an option? I'd prefer for folks to just not register weights unless they want them reflected in HAProxy.

Yea I know the other PR did it, but I think it's maybe not the best option...

Copy link
Collaborator

Choose a reason for hiding this comment

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

i am also not a big fan of this option

Copy link
Author

Choose a reason for hiding this comment

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

This is mainly for safety so people can rollout a new version without worrying about breaking something. I'm also not a fan of this option but it's kind of necessary unless you roll this into a major release.

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore_weights doesn't seem very useful as a setting and it is confusing that the default is shipped in a disabled state. There isn't a similar disable switch for haproxy_server_options and that's way more powerful than the weights setting (rhetorical question: why not just set haproxy_server_options to weight 10?).

If the main concern is a breaking API change, then I would suggest bumping to a new major release for the next version. What do you all think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@minkovich I'm fine with us cutting a new minor (aka major in 0.X parlance) version if that's the concern. I think if folks are registering a weight they should expect to have it reflected in the load balancers.

@scarletmeow Weight is separate for a few reasons. The first is because it was merged before I came up with haproxy_server_options back when the plan was to merge all the things as top level keys. The second is because it can be dynamically updated over the stats socket making it much more useful for dynamically balancing load while most options that occur in haproxy_server_options (e.g. healthcheck, backup status. etc ...) cannot. The third is because it is frontend agnostic, weight is a common concept in all load balancers that I'm aware of.

Copy link
Author

Choose a reason for hiding this comment

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

@jolynch @igor47 @scarletmeow I'm going to wipe out the option to ignore weights. It's going to simplify the code a lot. I'd just bold it in the announcement when you cut the next release. :)


<a name="haproxy"/>
### Configuring HAProxy ###
Expand Down
9 changes: 9 additions & 0 deletions lib/synapse/haproxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,7 @@ def initialize(opts)
@opts['do_writes'] = true unless @opts.key?('do_writes')
@opts['do_socket'] = true unless @opts.key?('do_socket')
@opts['do_reloads'] = true unless @opts.key?('do_reloads')
@opts['ignore_weights'] = true unless @opts.key?('ignore_weights')

# how to restart haproxy
@restart_interval = @opts.fetch('restart_interval', 2).to_i
Expand Down Expand Up @@ -742,6 +743,14 @@ def generate_backend_stanza(watcher, config)
b = "\tserver #{backend_name} #{backend['host']}:#{backend['port']}"
b = "#{b} cookie #{backend_name}" unless config.include?('mode tcp')
b = "#{b} #{watcher.haproxy['server_options']}" if watcher.haproxy['server_options']
if !@opts['ignore_weights'] && backend.has_key?('weight')
# Check if server_options already contains weight, is so log a warning
if watcher.haproxy['server_options'].include? 'weight'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the backend (server) provided haproxy_server_options data is a more concerning conflict than server_options (a weight in the server_options would be like a default for servers or something).

Copy link
Author

Choose a reason for hiding this comment

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

@jolynch, that's a good point. To reduce the number of iteration for this PR, could you suggest what message you'd like to see when haproxy_server_options also includes weight?

log.info "synapse: weight is defined by server_options and by nerve"
Copy link
Contributor

Choose a reason for hiding this comment

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

log.warn (nit)

end
weight = backend['weight'].to_i
b = "#{b} weight #{weight}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I believe that HAProxy will use the last weight provided. If that's true I think this should probably slot this in between the HAProxy backend server_options and the server haproxy_server_options level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

or maybe we should bail of server options also contains weight? i don't have a strong feeling as to what the precedence order should be... at least printing a warning to logs when there are contradictory options seems like a good idea.

this is probably indicating that it's time to refactor this function, it's getting pretty big.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, I noted a similar concern about complexity in #171. I'm not sure if these PRs are the right place for that though or if we should refactor it once they're merged.

I agree printing a warning is useful, but I don't think we should bail out as I can understand use cases where someone might want to set some combination as a fallback scheme. weight and haproxy_server_options come from the registration side and server_options come from the discovery side. I can understand a situation where one side might have information the other does not.

Copy link
Author

Choose a reason for hiding this comment

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

@igor47 and @jolynch, let me move these lines and add a warning when a conflict is detected.

end
b = "#{b} #{backend['haproxy_server_options']}" if backend['haproxy_server_options']
b = "#{b} disabled" unless backend['enabled']
b }
Expand Down
27 changes: 25 additions & 2 deletions spec/lib/synapse/haproxy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,18 @@ class MockWatcher; end;
describe Synapse::Haproxy do
subject { Synapse::Haproxy.new(config['haproxy']) }

let(:mockwatcher) do
def createmockwatcher(backends)
mockWatcher = double(Synapse::ServiceWatcher)
allow(mockWatcher).to receive(:name).and_return('example_service')
backends = [{ 'host' => 'somehost', 'port' => 5555}]
allow(mockWatcher).to receive(:backends).and_return(backends)
allow(mockWatcher).to receive(:haproxy).and_return({'server_options' => "check inter 2000 rise 3 fall 2"})
mockWatcher
end

let(:mockwatcher) do
createmockwatcher [{ 'host' => 'somehost', 'port' => '5555'}]
end

let(:mockwatcher_with_server_options) do
mockWatcher = double(Synapse::ServiceWatcher)
allow(mockWatcher).to receive(:name).and_return('example_service')
Expand Down Expand Up @@ -96,4 +99,24 @@ class MockWatcher; end;
expect(subject.generate_frontend_stanza(mockwatcher_frontend_with_bind_address, mockConfig)).to eql(["\nfrontend example_service", [], "\tbind 127.0.0.3:2200", "\tdefault_backend example_service"])
end

it 'generates backend stanza with weight' do
mockConfig = []
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 check inter 2000 rise 3 fall 2 weight 1"]])
end

it 'generates backend stanza with bad weight = 0' do
mockConfig = []
expect(subject.generate_backend_stanza(createmockwatcher([{ 'weight' => 'hi', 'host' => 'somehost', 'port' => '5555'}]), mockConfig)).to eql(["\nbackend example_service", [], ["\tserver somehost:5555 somehost:5555 cookie somehost:5555 check inter 2000 rise 3 fall 2 weight 0"]])
end

it 'generates backend stanza with nil weight = 0' do
mockConfig = []
expect(subject.generate_backend_stanza(createmockwatcher([{ 'weight' => nil, 'host' => 'somehost', 'port' => '5555'}]), mockConfig)).to eql(["\nbackend example_service", [], ["\tserver somehost:5555 somehost:5555 cookie somehost:5555 check inter 2000 rise 3 fall 2 weight 0"]])
end

it 'generates backend stanza without weight' do
mockConfig = []
expect(subject.generate_backend_stanza(createmockwatcher([{ 'host' => 'somehost', 'port' => '5555'}]), mockConfig)).to eql(["\nbackend example_service", [], ["\tserver somehost:5555 somehost:5555 cookie somehost:5555 check inter 2000 rise 3 fall 2"]])
end

end
32 changes: 32 additions & 0 deletions spec/lib/synapse/service_watcher_base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,37 @@ def remove_arg(name)
expect(subject.backends).to eq(matching_labeled_backends)
end
end

context 'with ignore_weights set to false' do
let(:backends_with_weight) { [
{ 'name' => 'server1', 'host' => 'server1', 'port' => 1111, 'weight' => 11 },
{ 'name' => 'server2', 'host' => 'server2', 'port' => 2222, 'weight' => 22 },
] }
let(:non_matching_weight_backends) { [
{ 'name' => 'server1', 'host' => 'server1', 'port' => 1111, 'weight' => 33 },
{ 'name' => 'server2', 'host' => 'server2', 'port' => 2222, 'weight' => 22 },
] }
let(:backends_with_non_matching_weight_duplicates) { [
{ 'name' => 'server1', 'host' => 'server1', 'port' => 1111, 'weight' => 11 },
{ 'name' => 'server1', 'host' => 'server1', 'port' => 1111, 'weight' => 33 },
{ 'name' => 'server2', 'host' => 'server2', 'port' => 2222, 'weight' => 22 },
] }
it 'updates backends only when weights change' do
expect(subject).to receive(:'reconfigure!').exactly(:twice)
expect(subject.send(:set_backends, backends_with_weight)).to equal(true)
expect(subject.backends).to eq(backends_with_weight)
expect(subject.send(:set_backends, non_matching_weight_backends)).to equal(true)
expect(subject.backends).to eq(non_matching_weight_backends)
expect(subject.send(:set_backends, non_matching_weight_backends)).to equal(false)
expect(subject.backends).to eq(non_matching_weight_backends)
end
it 'ignores duplicates even with non-matching weights' do
expect(subject).to receive(:'reconfigure!').exactly(:once)
expect(subject.send(:set_backends, backends_with_weight)).to equal(true)
expect(subject.backends).to eq(backends_with_weight)
expect(subject.send(:set_backends, backends_with_non_matching_weight_duplicates)).to equal(false)
expect(subject.backends).to eq(backends_with_weight)
end
end
end
end
1 change: 1 addition & 0 deletions spec/support/minimum.conf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ haproxy:
config_file_path: "/etc/haproxy/haproxy.cfg"
do_writes: false
do_reloads: false
ignore_weights: false
global:
- global_test_option

Expand Down