Merge the forks #130

jolynch · 2015-07-22T00:20:12Z

I believe that this takes your feedback from #115, adds some documentation of the features we added.

Major changes in this fork:

Service watchers that use the same zookeeper hosts will share one underlying zookeeper connection
Allow output of registrations to json files on the filesystem instead of just HAProxy
Allow rate limiting HAProxy restarts more aggressively than socket reconfigures
Add a cache of recently seen backends that are put in "disabled" and can then be enabled over the socket without a restart.

* This option defaults to true: if there are no default servers and a watcher reports no backends, then use the previous backends that we already know about. * Factor out code that updates '@backends' into the watcher base class.

If the configuration specifies a file_output key, synapse will manifest and update registrations on the filesystem in an atomic way. This is useful for applications that do not wish to communicate with service backends via haproxy

Note that at this time only the Yelp fork of haproxy has support for redispatching on every retry.

This reverts commit 136d1fe.

We can allow socket updates to go through any time that the config was updated, but restarts must be rate limited for stability

We are experiencing some very slow updates in production and I think it may be due to the connection pooling, try :per_callback threading to see if that helps. Also fixes default values for do_writes, do_reloads, do_socket

This fixes a bug where we could have a session expiry but not fail pings because the pooling code would not actually tear down the connection

igor47 · 2015-07-22T18:57:19Z

README.md

@@ -198,6 +198,19 @@ If you do not list any default servers, no proxy will be created.  The
 `default_servers` will also be used in addition to discovered servers if the
 `keep_default_servers` option is set.

+If you do not list any `default_servers`, and all backends for a service
+disappear then the previous known backends will be used.  Disable this behavior
+by unsetting `use_previous_backends`.


oh thank god, finally!

igor47 · 2015-07-22T19:39:17Z

looks pretty good overall. i left a few questions in a few places. i also think the test coverage is a little light, especially around zk connection pooling and all of the new haproxy functionality.

i think a few additional airbnb peeps from our production infrastructure team would like to take a look. ping @schleyfox @jtai

thanks for doing this, i'm excited about all of the great new functionality (especially connection pooling, the disabling of the keep-backends misfeature, and the haproxy state file so you can continue to see the disabled backends)

jolynch · 2015-07-23T01:14:51Z

Yea, the connection pooling and haproxy cache code was sorta done under P0 pressure (site was actively degraded kind of P0), so we didn't write too many tests heh. Do you think the lack of test coverage blocks merge or is it something that I can work on in separate PRs against this repo after the fork is gone?

schleyfox · 2015-07-23T03:58:31Z

I gave it a skim and it seems pretty reasonable to me. I'm excited.

Adds a comment about when we write out the file output and restored random backend ordering.

jtai · 2015-07-23T16:54:09Z

lib/synapse/haproxy.rb

@@ -43,6 +45,7 @@ class Haproxy
        "option abortonclose",
        "option accept-invalid-http-response",
        "option allbackups",
+        "option allredisp",


Will this cause problems for standard builds of haproxy?

no this is just a list of valid options so we can validate the in-synapse haproxy config. we already have a few options here which won't work with haproxy < 1.5; the symptoms if you use unsupported options is that synapse will start, but it's every attempt to restart haproxy will fail

Both my patches were accepted into 1.6, and Willy just released 1.6-dev3 so we're almost off our fork! Once we upgrade to 1.6 I can go through and re-sync this list with what haproxy accepts. There might be some backwards incompatible changes in 1.6 from 1.5 (only one I have ran into was listen no longer accepts ports in the listen line) so that should be a fun exercise.

jtai · 2015-07-23T17:37:36Z

These improvements are awesome! Thank you!

Added documentation of the state_file cache options as well as a few comments. Also fixed some error messages to be more clear.

igor47 · 2015-07-23T20:50:05Z

okay i think this is ready to merge. once that's done, i'll bump the version and release a build, and we can start trying to run it in airbnbland

Merge the forks

jolynch · 2015-07-23T22:35:11Z

We're going to start dogfooding 3e36e88 and then when you release we'll switch to that.

igor47 · 2015-07-24T00:29:19Z

ok i released v0.12.1; i'm going to start trying it here as well

igor47 · 2015-07-28T17:05:53Z

my smoke testing went well, and now i'm allowing synapse v0.12.1 to propagate through our infrastructure; i'm going to tackle airbnb/nerve#71 now

jnb and others added 15 commits July 21, 2015 17:16

Add 'use_previous_backends' option.

7d51288

* This option defaults to true: if there are no default servers and a watcher reports no backends, then use the previous backends that we already know about. * Factor out code that updates '@backends' into the watcher base class.

Allow registrations to be manifested on the file system

499c54d

If the configuration specifies a file_output key, synapse will manifest and update registrations on the filesystem in an atomic way. This is useful for applications that do not wish to communicate with service backends via haproxy

Explicitly deduplicate registrations

bcd8bcd

Allow the option allredisp option to haproxy.

507af82

Note that at this time only the Yelp fork of haproxy has support for redispatching on every retry.

Add rate limiter.

915193e

Revert "Add rate limiter."

14262e7

This reverts commit 136d1fe.

Increase HAProxy restart interval.

6c5516e

ZooKeeper connection pooling.

8277556

Rate limit restarts but not stats socket updates

73f48bf

We can allow socket updates to go through any time that the config was updated, but restarts must be rate limited for stability

Add state file.

4476f0e

Fix bug in caching logic.

5066638

Add support for the weight key added in nerve

e059316

Try out :per_callback threads and get more debug information

4633f82

We are experiencing some very slow updates in production and I think it may be due to the connection pooling, try :per_callback threading to see if that helps. Also fixes default values for do_writes, do_reloads, do_socket

Turns out it's important to handle session disconnects correctly

1eac133

This fixes a bug where we could have a session expiry but not fail pings because the pooling code would not actually tear down the connection

Fixups for merge to airbnb/master

b3c0fb7

jolynch mentioned this pull request Jul 22, 2015

Allow registrations to be manifested on the file system and add 'use_previous_backends' option #115

Closed

igor47 reviewed Jul 22, 2015
View reviewed changes

Address feedback from igor and schleyfox

cd2ea63

Adds a comment about when we write out the file output and restored random backend ordering.

jolynch force-pushed the merge_the_forks branch from dcfe77c to cd2ea63 Compare July 23, 2015 05:16

jtai reviewed Jul 23, 2015
View reviewed changes

Fixed wording of some documentation, added missing options

6f11a82

Added documentation of the state_file cache options as well as a few comments. Also fixed some error messages to be more clear.

jolynch force-pushed the merge_the_forks branch from 437171a to 6f11a82 Compare July 23, 2015 18:29

jnb added a commit that referenced this pull request Jul 23, 2015

Merge pull request #130 from Yelp/merge_the_forks

3e36e88

Merge the forks

jnb merged commit 3e36e88 into airbnb:master Jul 23, 2015

jolynch mentioned this pull request Jul 23, 2015

Sorting backends #128

Closed

This was referenced Sep 7, 2015

Added global zookeeper for service discovery #72

Closed

Allow service-specific bind_addresses #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge the forks #130

Merge the forks #130

jolynch commented Jul 22, 2015

igor47 Jul 22, 2015

igor47 commented Jul 22, 2015

jolynch commented Jul 23, 2015

schleyfox commented Jul 23, 2015

jtai Jul 23, 2015

igor47 Jul 23, 2015

jolynch Jul 23, 2015

jtai commented Jul 23, 2015

igor47 commented Jul 23, 2015

jolynch commented Jul 23, 2015

igor47 commented Jul 24, 2015

igor47 commented Jul 28, 2015

Merge the forks #130

Merge the forks #130

Conversation

jolynch commented Jul 22, 2015

igor47 Jul 22, 2015

Choose a reason for hiding this comment

igor47 commented Jul 22, 2015

jolynch commented Jul 23, 2015

schleyfox commented Jul 23, 2015

jtai Jul 23, 2015

Choose a reason for hiding this comment

igor47 Jul 23, 2015

Choose a reason for hiding this comment

jolynch Jul 23, 2015

Choose a reason for hiding this comment

jtai commented Jul 23, 2015

igor47 commented Jul 23, 2015

jolynch commented Jul 23, 2015

igor47 commented Jul 24, 2015

igor47 commented Jul 28, 2015