Leave container allocations for a time after death

so they will be reclaimed if the container is restarted.
Still cancel pending operations immediately a container dies

ipam/allocate.go

@@ -26,6 +26,8 @@ func (g *allocate) Try(alloc *Allocator) bool {
 	}
 
 	if addr, found := alloc.lookupOwned(g.ident, g.r); found {
+		// If we had heard that this container died, resurrect it
+		delete(alloc.dead, g.ident) // delete is no-op if key not in map
 		g.resultChan <- allocateResult{addr, nil}
 		return true
 	}

ipam/allocator.go

@@ -21,8 +21,9 @@ const (
 	msgRingUpdate
 	msgSpaceRequestDenied
 
-	tickInterval  = time.Second * 5
-	MinSubnetSize = 4 // first and last addresses are excluded, so 2 would be too small
+	tickInterval         = time.Second * 5
+	MinSubnetSize        = 4 // first and last addresses are excluded, so 2 would be too small
+	containerDiedTimeout = time.Second * 5
 )
 
 // operation represents something which Allocator wants to do, but
@@ -51,6 +52,7 @@ type Allocator struct {
 	nicknames        map[mesh.PeerName]string     // so we can map nicknames for rmpeer
 	pendingAllocates []operation                  // held until we get some free space
 	pendingClaims    []operation                  // held until we know who owns the space
+	dead             map[string]time.Time         // containers we heard were dead, and when
 	gossip           mesh.Gossip                  // our link to the outside world for sending messages
 	paxos            *paxos.Node
 	paxosActive      bool
@@ -70,6 +72,7 @@ func NewAllocator(ourName mesh.PeerName, ourUID mesh.PeerUID, ourNickname string
 		paxos:       paxos.NewNode(ourName, ourUID, quorum),
 		nicknames:   map[mesh.PeerName]string{ourName: ourNickname},
 		isKnownPeer: isKnownPeer,
+		dead:        make(map[string]time.Time),
 		now:         time.Now,
 	}
 }
@@ -223,10 +226,28 @@ func (alloc *Allocator) Claim(ident string, addr address.Address, noErrorOnUnkno
 	return <-resultChan
 }
 
-// ContainerDied is provided to satisfy the updater interface; does a 'Delete' underneath.  Sync.
+// ContainerDied called from the updater interface.  Async.
 func (alloc *Allocator) ContainerDied(ident string) {
-	if err := alloc.Delete(ident); err == nil {
-		alloc.debugln("Container", ident, "died; released addresses")
+	alloc.actionChan <- func() {
+		if _, found := alloc.lookupOwned(ident, alloc.universe); found {
+			alloc.debugln("Container", ident, "died; noting to remove later")
+			alloc.dead[ident] = alloc.now()
+		}
+		// Also remove any pending ops
+		alloc.cancelOpsFor(&alloc.pendingAllocates, ident)
+		alloc.cancelOpsFor(&alloc.pendingClaims, ident)
+	}
+}
+
+func (alloc *Allocator) removeDeadContainers() {
+	cutoff := alloc.now().Add(-containerDiedTimeout)
+	for ident, timeOfDeath := range alloc.dead {
+		if timeOfDeath.Before(cutoff) {
+			if err := alloc.delete(ident); err == nil {
+				alloc.debugln("Removed addresses for container", ident)
+			}
+			delete(alloc.dead, ident)
+		}
 	}
 }
 
@@ -236,25 +257,24 @@ func (alloc *Allocator) ContainerStarted(ident string) {}
 func (alloc *Allocator) Delete(ident string) error {
 	errChan := make(chan error)
 	alloc.actionChan <- func() {
-		addrs, found := alloc.owned[ident]
-		for _, addr := range addrs {
-			alloc.space.Free(addr)
-		}
-		delete(alloc.owned, ident)
-
-		// Also remove any pending ops
-		found = alloc.cancelOpsFor(&alloc.pendingAllocates, ident) || found
-		found = alloc.cancelOpsFor(&alloc.pendingClaims, ident) || found
-
-		if !found {
-			errChan <- fmt.Errorf("Delete: no addresses for %s", ident)
-			return
-		}
-		errChan <- nil
+		errChan <- alloc.delete(ident)
 	}
 	return <-errChan
 }
 
+func (alloc *Allocator) delete(ident string) error {
+	addrs, found := alloc.owned[ident]
+	for _, addr := range addrs {
+		alloc.space.Free(addr)
+	}
+	delete(alloc.owned, ident)
+
+	if !found {
+		return fmt.Errorf("Delete: no addresses for %s", ident)
+	}
+	return nil
+}
+
 // Free (Sync) - release single IP address for container
 func (alloc *Allocator) Free(ident string, addrToFree address.Address) error {
 	errChan := make(chan error)
@@ -514,6 +534,7 @@ func (alloc *Allocator) actorLoop(actionChan <-chan func()) {
 			if alloc.paxosActive {
 				alloc.propose()
 			}
+			alloc.removeDeadContainers()
 			alloc.tryPendingOps()
 		}
 

ipam/allocator_test.go

@@ -69,8 +69,18 @@ func TestAllocFree(t *testing.T) {
 	require.Equal(t, testAddr2, addr4.String(), "address")
 
 	alloc.ContainerDied(container2)
+
+	// Resurrect
+	addr1c, err := alloc.Allocate(container2, cidr1.HostRange(), returnFalse)
+	require.NoError(t, err)
+	require.Equal(t, addr1b, addr1c, "address")
+
 	alloc.ContainerDied(container3)
-	require.Equal(t, address.Offset(spaceSize), alloc.NumFreeAddresses(subnet))
+	alloc.Encode() // sync up
+	// Move the clock forward and clear out the dead container
+	alloc.actionChan <- func() { alloc.now = func() time.Time { return time.Now().Add(containerDiedTimeout * 2) } }
+	alloc.actionChan <- func() { alloc.removeDeadContainers() }
+	require.Equal(t, address.Offset(spaceSize-1), alloc.NumFreeAddresses(subnet))
 }
 
 func TestBootstrap(t *testing.T) {

site/features.md

@@ -156,6 +156,19 @@ connect to. The individual IP addresses given to containers must, of
 course, be unique - if you pick an address that the automatic
 allocator has already assigned you will receive a warning.
 
+If you restart a container, it will retain the same IP addresses on
+the weave network:
+
+    host1$ docker run --name a1 -tdi ubuntu
+    f76b09a9fcfee04551dbb8d951d9a83e7e7d55126b02fd9f44f9f8a5f07d7c96
+    host1$ weave ps a1
+    a1 1e:dc:2a:db:ef:ff 10.32.0.3/12
+    host1$ docker restart a1
+    host1$ weave ps a1
+    a1 16:c0:6f:5d:c5:73 10.32.0.3/12
+
+(note that the IP address is held for a limited time - currently five seconds)
+
 ### <a name="naming-and-discovery"></a>Naming and discovery
 
 Named containers are automatically registered in

test/640_proxy_restart_reattaches_test.sh

@@ -2,8 +2,6 @@
 
 . ./config.sh
 
-C1=10.2.0.78
-C2=10.2.0.34
 NAME=seetwo.weave.local
 
 check_attached() {
@@ -26,8 +24,9 @@ wait_for_proxy() {
 start_suite "Proxy restart reattaches networking to containers"
 
 WEAVE_DOCKER_ARGS=--restart=always WEAVEPROXY_DOCKER_ARGS=--restart=always weave_on $HOST1 launch
-proxy_start_container          $HOST1 -e WEAVE_CIDR=$C2/24 -di --name=c2 --restart=always -h $NAME
-proxy_start_container_with_dns $HOST1 -e WEAVE_CIDR=$C1/24 -di --name=c1 --restart=always
+proxy_start_container          $HOST1 -di --name=c2 --restart=always -h $NAME
+proxy_start_container_with_dns $HOST1 -di --name=c1 --restart=always
+C2=$(container_ip $HOST1 c2)
 
 proxy docker_on $HOST1 restart -t=1 c2
 check_attached
@@ -42,11 +41,13 @@ run_on $HOST1 sudo kill -KILL $(docker_on $HOST1 inspect --format='{{.State.Pid}
 sleep 1
 check_attached
 
-# Disabled because Docker 1.8 often fails to restart weave or weaveproxy
 # Restart docker itself, using different commands for systemd- and upstart-managed.
-#run_on $HOST1 sh -c "command -v systemctl >/dev/null && sudo systemctl restart docker || sudo service docker restart"
-#wait_for_proxy $HOST1
-#check_attached
+run_on $HOST1 sh -c "command -v systemctl >/dev/null && sudo systemctl restart docker || sudo service docker restart"
+wait_for_proxy $HOST1
+sleep 5 # allow for re-tries of attach
+# Re-fetch the IP since it is not retained on docker restart
+C2=$(container_ip $HOST1 c2) || (echo "container c2 has no IP address" 2>&1; exit 1)
+check_attached
 
 # 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