Merge pull request #2296 from weaveworks/issues/2284-die-another-way

Move kill-on-failure responsibility to caller of weave script

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, false); err != nil {
+		if err := proxy.attach(id); 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,9 +383,26 @@ func (proxy *Proxy) listen(protoAndAddr string) (net.Listener, string, error) {
 
 // weavedocker.ContainerObserver interface
 func (proxy *Proxy) ContainerStarted(ident string) {
-	// 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)
+	err := proxy.attach(ident)
+	if err != nil {
+		var e error
+		// attach failed: 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.
+		if proxy.waitChan(ident) != nil {
+			e = proxy.client.KillContainer(docker.KillContainerOptions{ID: ident})
+		} else {
+			var c *docker.Container
+			if c, e = proxy.client.InspectContainer(ident); e == nil {
+				var process *os.Process
+				if process, e = os.FindProcess(c.State.Pid); e == nil {
+					e = process.Kill()
+				}
+			}
+		}
+		if e != nil {
+			Log.Warningf("Error killing %s: %s", ident, e)
+		}
+	}
 	proxy.notifyWaiters(ident, err)
 }
 
@@ -466,7 +483,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, killProcess bool) error {
+func (proxy *Proxy) attach(containerID string) error {
 	container, err := proxy.client.InspectContainer(containerID)
 	if err != nil {
 		if _, ok := err.(*docker.NoSuchContainer); !ok {
@@ -502,13 +519,6 @@ func (proxy *Proxy) attach(containerID string, orDie, killProcess bool) error {
 	if proxy.NoMulticastRoute {
 		args = append(args, "--no-multicast-route")
 	}
-	if orDie {
-		if killProcess {
-			args = append(args, "--or-kill")
-		} else {
-			args = append(args, "--or-die")
-		}
-	}
 	args = append(args, container.ID)
 	return callWeaveAttach(container, args)
 }

weave

@@ -315,6 +315,10 @@ extra_hosts_args() {
     done
 }
 
+kill_container() {
+    docker kill $1 >/dev/null 2>&1 || true
+}
+
 
 check_docker_version
 
@@ -373,7 +377,10 @@ elif [ "$1" = "run" -a -z "$IS_LOCAL" ] ; then
     collect_cidr_args "$@"
     shift $CIDR_ARG_COUNT
     CONTAINER=$(docker $DOCKER_CLIENT_ARGS run -e WEAVE_CIDR=none $DNS_ARGS -d "$@")
-    exec_remote attach $CIDR_ARGS --or-die $DNS_EXTRA_HOSTS_ARGS $ATTACH_ARGS $CONTAINER >/dev/null
+    if ! exec_remote attach $CIDR_ARGS $DNS_EXTRA_HOSTS_ARGS $ATTACH_ARGS $CONTAINER >/dev/null ; then
+        kill_container $CONTAINER
+        exit 1
+    fi
     echo $CONTAINER
     exit 0
 fi
@@ -828,14 +835,6 @@ with_container_netns() {
     return $STATUS
 }
 
-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
@@ -843,13 +842,6 @@ 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 "$@"
@@ -1476,13 +1468,6 @@ 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
@@ -2165,12 +2150,6 @@ EOF
         shift $CIDR_ARG_COUNT
         while [ $# -gt 0 ]; do
             case "$1" in
-                --or-die)
-                    ATTACH_TYPE="_or_die"
-                    ;;
-                --or-kill)
-                    ATTACH_TYPE="_or_kill"
-                    ;;
                 --rewrite-hosts)
                     REWRITE_HOSTS=1
                     ;;
@@ -2193,9 +2172,9 @@ EOF
         [ $# -eq 1 ] || usage
         CONTAINER=$(container_id $1)
         create_bridge
-        ipam_cidrs$ATTACH_TYPE allocate $CONTAINER $CIDR_ARGS
+        ipam_cidrs allocate $CONTAINER $CIDR_ARGS
         [ -n "$REWRITE_HOSTS" ] && rewrite_etc_hosts $DNS_EXTRA_HOSTS
-        with_container_netns$ATTACH_TYPE $CONTAINER attach $ALL_CIDRS >/dev/null
+        with_container_netns $CONTAINER attach $ALL_CIDRS >/dev/null
         when_weave_running with_container_fqdn $CONTAINER put_dns_fqdn $ALL_CIDRS
         show_addrs $ALL_CIDRS
         ;;