From 5993b3e846b42012b457cb646404a5ea84d0bb84 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 6 May 2016 12:45:03 +0100 Subject: [PATCH 1/4] Refactor: extract function to look up waiting channel for container --- proxy/proxy.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/proxy/proxy.go b/proxy/proxy.go index 74eccfdf..6291c091 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -438,18 +438,20 @@ func (proxy *Proxy) waitForStart(r *http.Request) error { return nil } -// If some other operation is waiting for a container to start, join in the wait -func (proxy *Proxy) waitForStartByIdent(ident string) error { - var ch chan error +func (proxy *Proxy) waitChan(ident string) chan error { proxy.Lock() + defer proxy.Unlock() for _, wait := range proxy.waiters { if ident == wait.ident && !wait.done { - ch = wait.ch - break + return wait.ch } } - proxy.Unlock() - if ch != nil { + return nil +} + +// If some other operation is waiting for a container to start, join in the wait +func (proxy *Proxy) waitForStartByIdent(ident string) error { + if ch := proxy.waitChan(ident); ch != nil { Log.Debugf("Wait for start of container %s", ident) return <-ch } From 01e3d1d81b7e5c26a7ed91ac6a7fb650b61df367 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 6 May 2016 15:45:54 +0100 Subject: [PATCH 2/4] Refactor: extract kill_container function --- weave | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/weave b/weave index 3662412f..3edb0d57 100755 --- a/weave +++ b/weave @@ -828,9 +828,13 @@ with_container_netns() { return $STATUS } +kill_container() { + docker kill $1 >/dev/null 2>&1 || true +} + with_container_netns_or_die() { if ! with_container_netns "$@" >/dev/null ; then - docker kill $1 >/dev/null 2>&1 || true + kill_container $1 exit 1 fi } @@ -1129,7 +1133,7 @@ wait_for_status() { while true ; do "$@" GET /status >/dev/null 2>&1 && return 0 if ! check_running $container >/dev/null 2>&1 ; then - docker kill $container >/dev/null 2>&1 || true # stop it restarting + kill_container $container # stop it restarting echo $(death_msg $container) >&2 return 1 fi @@ -1451,7 +1455,7 @@ ipam_cidrs() { ipam_cidrs_or_die() { if ! ipam_cidrs "$@" ; then - docker kill $2 >/dev/null 2>&1 || true + kill_container $2 exit 1 fi } From 8d71467802effbefc2d1953a9c9232b9a98af08e Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 6 May 2016 15:47:20 +0100 Subject: [PATCH 3/4] Kill process instead of container, on container restart. When Docker restarts a container and something goes wrong attaching the network, kill the process inside the container instead of killing the container, so that Docker will restart it again --- proxy/proxy.go | 15 +++++++++++---- weave | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/proxy/proxy.go b/proxy/proxy.go index 6291c091..54bedc65 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -103,7 +103,7 @@ func (proxy *Proxy) attachWithRetry(id string) { j := &attachJob{id: id, tryInterval: initialInterval} j.timer = time.AfterFunc(time.Duration(0), func() { - if err := proxy.attach(id, false); err != nil { + if err := proxy.attach(id, false, false); err != nil { // The delay at the nth retry is a random value in the range // [i-i/2,i+i/2], where i = initialInterval * 1.5^(n-1). j.timer.Reset(j.tryInterval) @@ -383,7 +383,10 @@ func (proxy *Proxy) listen(protoAndAddr string) (net.Listener, string, error) { // weavedocker.ContainerObserver interface func (proxy *Proxy) ContainerStarted(ident string) { - proxy.notifyWaiters(ident, proxy.attach(ident, true)) + // In case of attach failure, if we have a request waiting on the start, kill the container, + // otherwise assume it is a Docker-initated restart and kill the process inside. + err := proxy.attach(ident, true, proxy.waitChan(ident) == nil) + proxy.notifyWaiters(ident, err) } func containerShouldAttach(container *docker.Container) bool { @@ -463,7 +466,7 @@ func (proxy *Proxy) ContainerDestroyed(ident string) {} // Check if this container needs to be attached, if so then attach it, // and return nil on success or not needed. -func (proxy *Proxy) attach(containerID string, orDie bool) error { +func (proxy *Proxy) attach(containerID string, orDie, killProcess bool) error { container, err := proxy.client.InspectContainer(containerID) if err != nil { if _, ok := err.(*docker.NoSuchContainer); !ok { @@ -500,7 +503,11 @@ func (proxy *Proxy) attach(containerID string, orDie bool) error { args = append(args, "--no-multicast-route") } if orDie { - args = append(args, "--or-die") + if killProcess { + args = append(args, "--or-kill") + } else { + args = append(args, "--or-die") + } } args = append(args, container.ID) return callWeaveAttach(container, args) diff --git a/weave b/weave index 3edb0d57..da6764af 100755 --- a/weave +++ b/weave @@ -832,6 +832,10 @@ kill_container() { docker kill $1 >/dev/null 2>&1 || true } +kill_container_process() { + kill -9 $(docker inspect --format='{{.State.Pid}}' $1) >/dev/null 2>&1 || true +} + with_container_netns_or_die() { if ! with_container_netns "$@" >/dev/null ; then kill_container $1 @@ -839,6 +843,13 @@ with_container_netns_or_die() { fi } +with_container_netns_or_kill() { + if ! with_container_netns "$@" >/dev/null ; then + kill_container_process $1 + exit 1 + fi +} + # Execute arguments as a command within the $CONTAINER_NETNS network namespace netnsenter() { nsenter --net=$CONTAINER_NETNS "$@" @@ -1460,6 +1471,13 @@ ipam_cidrs_or_die() { fi } +ipam_cidrs_or_kill() { + if ! ipam_cidrs "$@" ; then + kill_container_process $2 + exit 1 + fi +} + show_addrs() { addrs= for cidr in "$@" ; do @@ -2136,6 +2154,9 @@ EOF --or-die) ATTACH_TYPE="_or_die" ;; + --or-kill) + ATTACH_TYPE="_or_kill" + ;; --rewrite-hosts) REWRITE_HOSTS=1 ;; From 469d456c8bb5eb30dab7b8c45ea607123aae5e33 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 9 May 2016 11:19:19 +0100 Subject: [PATCH 4/4] Smoke-test for container restart with proxy attach failure --- test/640_proxy_restart_reattaches_test.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/640_proxy_restart_reattaches_test.sh b/test/640_proxy_restart_reattaches_test.sh index 799a9542..4ea8e04b 100755 --- a/test/640_proxy_restart_reattaches_test.sh +++ b/test/640_proxy_restart_reattaches_test.sh @@ -60,8 +60,16 @@ check_attached assert_raises "timeout 90 cat <( run_many )" +# Start a container that needs IPAM, then restart it when the router is stopped +proxy_start_container $HOST1 -di --name=c3 --restart=always +assert_raises "exec_on $HOST1 c3 $CHECK_ETHWE_UP" +weave_on $HOST1 stop-router +docker_on $HOST1 restart c3 +weave_on $HOST1 launch-router +sleep 5 +assert_raises "exec_on $HOST1 c3 $CHECK_ETHWE_UP" + # Restarting proxy shouldn't kill unattachable containers -proxy_start_container $HOST1 -di --name=c3 --restart=always # Use ipam, so it won't be attachable w/o weave weave_on $HOST1 stop weave_on $HOST1 launch-proxy assert_raises "exec_on $HOST1 c3 $CHECK_ETHWE_UP"