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

Import libnetwork fix for rolling updates #36638

Merged
merged 2 commits into from Mar 29, 2018

Conversation

Projects
None yet
@ctelfer
Contributor

ctelfer commented Mar 19, 2018

- What I did

This patch allows endpoints to complete servicing connections while being removed from a service. The fix is mostly within libnetwork. There is a small change to the moby code to remove a now-superfluous call to sandbox.DisableService(). However, moby would function correctly (just sub-optimally) without said removal. (see the changelog for for the commits further details)

This fix addresses issue raised in docker/libnetwork#2074 and is related to #30321. The libnetwork PR for this fix is here docker/libnetwork#2112. This PR does not include any other libnetwork PRs. The removal of the call to DisableService() from the releaseNetworks() function reverts the change introduced in #35960.

- How I did it

The fix works by initially down-weighting a container endpoint in the load balancer to 0 while keeping the endpoint present in the load balancer. This allows traffic to continue to flow to the endpoint while preventing new connections from going to the endpoint. This allows the container to complete requests during the "stop_grace_period" and then exit when finished without interruption of service.

This change requires propagating the status of disabled service endpoints via the networkDB. Accordingly, the patch includes both code to generate and handle service update messages. It also augments the service structure with a ServiceDisabled boolean to convey whether an endpoint should ultimately be removed or just disabled. This, naturally, required a rebuild of the protocol buffer code.

The protocol buffer encoding is designed to support additions of fields to messages in a backwards-compatible manner. Protocol buffer unmarshalling code automatically skips past any fields that it isn't
aware of. As a result, an older moby daemon without this fix can receive and will process correctly networkDB messages from newer moby daemons with this patch.

As it turns out, the additional field is simply a bool that is otherwise irrelevent on networkDB create and delete events. So its absence in older moby daemon processing has no impact. However, the fix leverages the "update" networkDB message which was previously unused in libnetwork. Although older libnetwork implementations parse the message cleanly, they will see the message as unexpected and as such issue a log at error level indicating the receipt of such.

Other than this there should be no other negative impact for use of this patch in mixed environments. (Although older mobys won't be able to gracefully downgrade connections on their nodes of course.)

Signed-off-by: Chris Telfer ctelfer@docker.com

- How to verify it

One can verify this in a swarm environment by the following steps:

  • git clone https://github.com/ctelfer/docker-rolling-upgrade-test.git
  • cd docker-rolling-upgrade-test
  • OPTIONAL: edit Makefile and docker-compose.yml to change your tag for the image you want to build
  • make build -- do this on the swarm manager or with your env variables pointing to it
  • docker stack deploy -c docker-compose.yml t -- likewise on the manager or w/ docker env pointing to it
  • open an ssh connection to one of the workers in the swarm
  • curl https://LOCALIPADDR/?stream=1 > /dev/null -- open a streaming http connection. Make sure LOCALIPADDR is one of the IP addresses of the worker, but not localhost
  • verify from curl stderr output that the stream is transferring data
  • on the manager: docker service update t_webservice --force -- force an update
  • wait for the update to converge
  • on the worker: verify that curl is still passing traffic. In the current version of moby, this will hang. In the updated version, traffic will continue until the grace period (1 hour in my config) ends.
  • open another ssh connection to the same worker doing the curl
  • nsenter --net=/var/run/docker/netns/ingress_sbox ipvsadm -L
  • verify that there are 4 entries: 3 with weight 1, and 1 with weight 0
  • kill the curl stream with CTRL-C
  • use nsenter ... ipvsadm -L on the worker again and verify that it goes back to 3 entries each with weight 1

- Description for the changelog

Import libnetwork fix for rolling updates

- A picture of a cute animal (not mandatory but encouraged)

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 19, 2018

Sorry, I should have been more clear. This patch only addresses Linux. I'm not yet familiar enough with HCS / HNS to know whether/how equivalent down-weighting could occur there. Having said that, the libnetwork calls into the service_windows.go functions do provide all the information necessary to perform said operation. The patch, as written, should not change the behavior of Windows nodes either positively or negatively in this regard.

@codecov

This comment has been minimized.

codecov bot commented Mar 20, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@3858865). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36638   +/-   ##
=========================================
  Coverage          ?   34.95%           
=========================================
  Files             ?      613           
  Lines             ?    45581           
  Branches          ?        0           
=========================================
  Hits              ?    15934           
  Misses            ?    27560           
  Partials          ?     2087
@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 20, 2018

Updated commit to remove now-redundant libnetwork sandbox.DisableService() call. See summary and commit logs for detail. Meant to include this originally.

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 21, 2018

The experimental failed check regarded TestSwarmLockUnlockCluster and s390x failed check TestPostContainersAttach. Am 99% certain from looking at both tests that neither is affected by this change.

@fcrisciani

LGTM

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 21, 2018

ping @YarekTyshchenko ... Since you raised libnetwork #2074, would you mind trying to build with this PR and verify that it works for you?

@YarekTyshchenko

This comment has been minimized.

YarekTyshchenko commented Mar 21, 2018

Fantastic! We will take a look

@YarekTyshchenko

This comment has been minimized.

YarekTyshchenko commented Mar 22, 2018

@ctelfer Tested this build with our use case and I'm happy to report that it works beautifully. Streaming connections are kept connected honouring the grace timeout parameter. This is the last thing that is required to have proper zero downtime deployments with Swarm.

@fcrisciani

This comment has been minimized.

Contributor

fcrisciani commented Mar 22, 2018

@YarekTyshchenko tnx for testing it on your side too! Much appreciated

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 22, 2018

@YarekTyshchenko Excellent! Very glad to hear it. Thanks for your help!

@abhi

abhi approved these changes Mar 27, 2018

LGTM

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 27, 2018

@vieux and/or @cpuguy83 -- Would either (or both) of you be able/willing to review this?

@vieux

vieux approved these changes Mar 27, 2018

the code LGTM, I restarted the failing CI

@vdemeester

LGTM 🐯

@boaz1337

LGTM 👍

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 28, 2018

Thanks to all the reviewers for taking the time!

Import libnetwork fix for rolling updates
This patch allows endpoints to complete servicing connections while
being removed from a service.  The fix is entirely within libnetwork
and requires no changes to the moby codebase proper.  It operates
by initially down-weighting a container endpoint in the load balancer
to 0 while keeping the endpoint present in the load balancer.  This
allows traffic to continue to flow to the endpoint while preventing new
connections from going to the endpoint.  This allows the container
to complete requests during the "stop_grace_period" and then exit when
finished without interruption of service.

This change requires propagating the status of disabled service
endpoints via the networkDB.  Accordingly, the patch includes both code
to generate and handle service update messages.  It also augments the
service structure with a ServiceDisabled boolean to convey whether an
endpoint should ultimately be removed or just disabled.  This,
naturally, required a rebuild of the protocol buffer code.

The protocol buffer encoding is designed to support additions of fields
to messages in a backwards-compatible manner.  Protocol buffer
unmarshalling code automatically skips past any fields that it isn't
aware of.  As a result, an older moby daemon without this fix can
receive and will process correctly networkDB messages from newer moby
daemons with this patch.

As it turns out, the additional field is simply a bool that is otherwise
irrelevent on networkDB create and delete events.  So its absence in
older moby daemon processing has no impact.  However, the fix leverages
the "update" networkDB message which was previously unused in
libnetwork.  Although older libnetwork implementations parse the message
cleanly, they will see the message as unexpected and as such issue a log
at error level indicating the receipt of such.

Other than this there should be no other negative impact for use of this
patch in mixed environments. (Although older mobys won't be able to
gracefully downgrade connections on their nodes of course.)

Signed-off-by: Chris Telfer <ctelfer@docker.com>
Remove (now) extra call to sb.DisableService()
This call was added as part of commit a042e5a and at the time was
useful.  sandbox.DisableService() basically calls
endpoint.deleteServiceInfoFromCluster() for every endpoint in the
sandbox.  However, with the libnetwork change, endpoint.sbLeave()
invokes endpoint.deleteServiceInfoFromCluster(). The releaseNetwork()
call invokes sandbox.Delete() immediately after
sandbox.DisableService().  The sandbox.Delete() in turn ultimately
invokes endpoint.sbLeave() for every endpoint in the sandbox which thus
removes the endpoint's load balancing entry via
endpoint.deleteServiceInfoFromCluster().  So the call to
sandbox.DisableService() is now redundant.

It is noteworthy that, while redundant, the presence of the call would
not cause errors.  It would just be sub-optimal.  The DisableService()
call would cause libnetwork to down-weight the load balancing entries
while the call to sandbox.Delete() would cause it to remove the entries
immediately afterwards.  Aside from the wasted computation, the extra
call would also propagate an extra state change in the networkDB gossip
messages.  So, overall, it is much better to just avoid the extra
overhead.

Signed-off-by: Chris Telfer <ctelfer@docker.com>

@ctelfer ctelfer force-pushed the ctelfer:rolling-update-libnetwork-import branch from 3a3c3ec to c27417a Mar 28, 2018

@ctelfer

This comment has been minimized.

Contributor

ctelfer commented Mar 28, 2018

Rebased to tip of master to take advantage of updated integration tests.

@anusha-ragunathan anusha-ragunathan merged commit f125748 into moby:master Mar 29, 2018

9 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 40031 has succeeded
Details
janky Jenkins build Docker-PRs 48754 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 9210 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4185 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 20223 has succeeded
Details
z Jenkins build Docker-PRs-s390x 9169 has succeeded
Details

@ctelfer ctelfer deleted the ctelfer:rolling-update-libnetwork-import branch Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment