Create an option to stop ignoring weights. By default weights will still be ignored. #174
Create an option to stop ignoring weights. By default weights will still be ignored. #174minkovich wants to merge 3 commits intoairbnb:masterfrom
Conversation
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.
| b = "#{b} cookie #{backend_name}" unless config.include?('mode tcp') | ||
| if !@opts['ignore_weights'] && backend.has_key?('weight') | ||
| weight = backend['weight'].to_i | ||
| b = "#{b} weight #{weight}" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…s from taking traffic just because they have different weights, add more tests.
lib/synapse/haproxy.rb
Outdated
| 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' | ||
| log.info "synapse: weight is defined by server_options and by nerve" |
…hange log level for duplicate weights.
| b = "#{b} #{watcher.haproxy['server_options']}" if watcher.haproxy['server_options'] | ||
| if backend.has_key?('weight') | ||
| # Check if server_options already contains weight, is so log a warning | ||
| if watcher.haproxy['server_options'].include? 'weight' |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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?
|
I'm not clear on how we know to restart without something like line 715? |
Doesn't the new unit test in spec/lib/synapse/service_watcher_base_spec.rb show that reconfigure will be called when weights change? Doesn't that imply that HAProxy will be reconfigured? Am I missing something? |
|
@minkovich Sorry I should have been more clear. A watcher calling set_backends is necessary but not sufficient to determine a restart is needed (I believe your unit tests test that set_backend/reconfigure are called). Restarting has to be done via notification (by setting to True) on the The typical update ordering is Basically Synapse is juggling three state stores and trying to keep them consistent. It's trying to watch the registry (zk), reflect that state internally (synapse) and then ensure that this state is consistent with the routing component (haproxy). I think your change reflects the changes from zk -> synapse, but I don't see how it reflects them from synapse to haproxy. |
4 participants
This commit is based on the following by @bobtfish
#131
but does the following things differently: