-
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
Allow registrations to be manifested on the file system and add 'use_previous_backends' option #115
Changes from 9 commits
d5d061a
b855bff
35f1f8f
c3d862c
01826a0
1fdd57e
6eb5850
e5411b0
c6c37fa
136d1fe
e057dbf
e6d03c2
a3e84d2
d0c1161
5928c2b
e96b2a9
4eab7e0
7de6a59
030ccfe
235fa85
36f2a94
e90cf85
86d7f9d
e49afdf
a6a48e3
29f3197
420440f
429a20a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
require 'synapse/log' | ||
require 'fileutils' | ||
require 'tempfile' | ||
|
||
module Synapse | ||
class FileOutput | ||
include Logging | ||
attr_reader :opts, :name | ||
|
||
def initialize(opts) | ||
super() | ||
|
||
unless opts.has_key?("output_directory") | ||
raise ArgumentError, "flat file generation requires an output_directory key" | ||
end | ||
|
||
begin | ||
FileUtils.mkdir_p(opts['output_directory']) | ||
rescue SystemCallError => err | ||
raise ArgumentError, "provided output directory #{opts['output_directory']} is not present or creatable" | ||
end | ||
|
||
@opts = opts | ||
@name = 'file_output' | ||
end | ||
|
||
def update_config(watchers) | ||
watchers.each do |watcher| | ||
write_backends_to_file(watcher.name, watcher.backends) | ||
end | ||
end | ||
|
||
def write_backends_to_file(service_name, new_backends) | ||
data_path = File.join(@opts['output_directory'], "#{service_name}.json") | ||
begin | ||
old_backends = JSON.load(File.read(data_path)) | ||
rescue Errno::ENOENT | ||
old_backends = nil | ||
end | ||
|
||
if old_backends == new_backends | ||
return false | ||
else | ||
# Atomically write new sevice configuration file | ||
temp_path = File.join(@opts['output_directory'], | ||
".#{service_name}.json.tmp") | ||
File.open(temp_path, 'w', 0644) {|f| f.write(new_backends.to_json)} | ||
FileUtils.mv(temp_path, data_path) | ||
return true | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
module Synapse | ||
class Haproxy | ||
include Logging | ||
attr_reader :opts | ||
attr_reader :opts, :name | ||
|
||
# these come from the documentation for haproxy 1.5 | ||
# http://haproxy.1wt.eu/download/1.5/doc/configuration.txt | ||
|
@@ -43,6 +43,7 @@ class Haproxy | |
"option abortonclose", | ||
"option accept-invalid-http-response", | ||
"option allbackups", | ||
"option allredisp", | ||
"option checkcache", | ||
"option forceclose", | ||
"option forwardfor", | ||
|
@@ -173,6 +174,7 @@ class Haproxy | |
"option accept-invalid-http-request", | ||
"option accept-invalid-http-response", | ||
"option allbackups", | ||
"option allredisp", | ||
"option checkcache", | ||
"option clitcpka", | ||
"option contstats", | ||
|
@@ -386,6 +388,7 @@ class Haproxy | |
"option accept-invalid-http-request", | ||
"option accept-invalid-http-response", | ||
"option allbackups", | ||
"option allredisp", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we try to re-sync these lists of options with latest haproxy? i wonder what other options have been added since i put these lists in here... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, this isn't even the option that I ended up getting accepted. http://marc.info/?l=haproxy&m=143161966505891&w=2 basically added a parameter to "option redispatch" which allows this behavior. Until that is released in 1.6 do you mind if we keep this in here (we use is pretty heavily at Yelp) |
||
"option checkcache", | ||
"option clitcpka", | ||
"option contstats", | ||
|
@@ -523,6 +526,7 @@ def initialize(opts) | |
end | ||
|
||
@opts = opts | ||
@name = 'haproxy' | ||
|
||
# how to restart haproxy | ||
@restart_interval = 2 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
require 'set' | ||
require 'synapse/log' | ||
|
||
module Synapse | ||
|
@@ -42,6 +43,10 @@ def initialize(opts={}, synapse) | |
|
||
@keep_default_servers = opts['keep_default_servers'] || false | ||
|
||
# If there are no default servers and a watcher reports no backends, then | ||
# use the previous backends that we already know about. | ||
@use_previous_backends = opts.fetch('use_previous_backends', true) | ||
|
||
# set a flag used to tell the watchers to exit | ||
# this is not used in every watcher | ||
@should_exit = false | ||
|
@@ -95,13 +100,44 @@ def validate_discovery_opts | |
end | ||
|
||
def set_backends(new_backends) | ||
if @keep_default_servers | ||
@backends = @default_servers + new_backends | ||
# Aggregate and deduplicate all potential backend service instances. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you really concerned about duplicates in the default servers list? if that's the case, maybe we should de-dup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned with duplicates in the set of backends we write out to HAProxy in general. Like, we had a few cases where duplicate backends got written out and then synapse would only down a single one when we failed healthchecks. If we dedup the default_servers separately doesn't that mean that we might end up with duplicates between the defaults and new_backends and could end up with the bad situation I mention above? |
||
new_backends = (new_backends + (@keep_default_servers ? @default_servers : [])).uniq {|b| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh i see. i think this would read a little cleaner if it was not all on one line. i would say: new_backends += @default_servers if @keep_default_servers
new_backends.uniq!{|b| [b['host'], b['port'], b.fetch('name', '')]} |
||
[b['host'], b['port'], b.fetch('name', '')] | ||
} | ||
|
||
if new_backends.to_set == @backends.to_set | ||
return false | ||
end | ||
|
||
if new_backends.empty? | ||
if @default_servers.empty? | ||
if @use_previous_backends | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for addressing this original "mis-feature" |
||
# Discard this update | ||
log.warn "synapse: no default servers for service #{@name};" \ | ||
" using previous backends: #{@backends.inspect}" | ||
return false | ||
else | ||
log.warn "synapse: no default servers for service #{@name} and" \ | ||
" 'use_previous_backends' is disabled; dropping all backends" | ||
@backends.clear | ||
end | ||
else | ||
log.warn "synapse: no backends for service #{@name};" \ | ||
" using default servers: #{@default_servers.inspect}" | ||
@backends = @default_servers | ||
end | ||
else | ||
log.info "synapse: discovered #{new_backends.length} backends for service #{@name}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on an unrelated note, one thing i always thought might be necessary eventually is to specify the minimum number of backends to serve traffic. suppose a service goes down. when functionality is restored, a single backend is naturally going to be the first to come up. that backend might then be pounded with all traffic which is normally meant for many multiple backends, and as a result might go down because of excess traffic. this would then happen to the second backend, etc.. etc.. it might be nice here to avoid setting any backends unless the number exceeds some limit which we know can handle the traffic. just a brain-dump, not even necessarily a todo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree this is a needed feature. I've been hacking on a "latency sensitive farm failover" method as suggested in http://blog.haproxy.com/2013/12/23/failover-and-worst-case-management-with-haproxy/ and determining how many backends are needed is core to that. The idea is that you have multiple HAProxy backends powering a single frontend, with each backend having more nodes (e.g. because it encompasses multiple datacenters). |
||
@backends = new_backends | ||
end | ||
|
||
reconfigure! | ||
|
||
return true | ||
end | ||
|
||
# Subclasses should not invoke this directly; it's only exposed so that it | ||
# can be overridden in subclasses. | ||
def reconfigure! | ||
@synapse.reconfigure! | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,21 +89,7 @@ def configure_backends(servers) | |
end | ||
end | ||
|
||
if new_backends.empty? | ||
if @default_servers.empty? | ||
log.warn "synapse: no backends and no default servers for service #{@name};" \ | ||
" using previous backends: #{@backends.inspect}" | ||
else | ||
log.warn "synapse: no backends for service #{@name};" \ | ||
" using default servers: #{@default_servers.inspect}" | ||
@backends = @default_servers | ||
end | ||
else | ||
log.info "synapse: discovered #{new_backends.length} backends for service #{@name}" | ||
set_backends(new_backends) | ||
end | ||
|
||
reconfigure! | ||
set_backends(new_backends) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great! |
||
end | ||
end | ||
end |
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.
not sure what this is calling out to, this class does not inherit from anything
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.
Indeed it does not, I'll fix that up