Merge pull request #2149 from weaveworks/stop-test-router

stop ipam tests from leaking routers

ipam/allocator.go

@@ -46,6 +46,7 @@ type operation interface {
 // are used around data structures.
 type Allocator struct {
 	actionChan        chan<- func()
+	stopChan          chan<- struct{}
 	ourName           mesh.PeerName
 	seed              []mesh.PeerName           // optional user supplied ring seed
 	universe          address.Range             // superset of all ranges
@@ -108,16 +109,20 @@ func NewAllocator(config Config) *Allocator {
 func (alloc *Allocator) Start() {
 	alloc.loadPersistedData()
 	actionChan := make(chan func(), mesh.ChannelSize)
+	stopChan := make(chan struct{})
 	alloc.actionChan = actionChan
+	alloc.stopChan = stopChan
 	alloc.ticker = time.NewTicker(tickInterval)
-	go alloc.actorLoop(actionChan)
+	go alloc.actorLoop(actionChan, stopChan)
 }
 
 // Stop makes the actor routine exit, for test purposes ONLY because any
 // calls after this is processed will hang. Async.
 func (alloc *Allocator) Stop() {
-	alloc.ticker.Stop()
-	alloc.actionChan <- nil
+	select {
+	case alloc.stopChan <- struct{}{}:
+	default:
+	}
 }
 
 // Operation life cycle
@@ -582,14 +587,14 @@ func (alloc *Allocator) SetInterfaces(gossip mesh.Gossip) {
 
 // ACTOR server
 
-func (alloc *Allocator) actorLoop(actionChan <-chan func()) {
+func (alloc *Allocator) actorLoop(actionChan <-chan func(), stopChan <-chan struct{}) {
+	defer alloc.ticker.Stop()
 	for {
 		select {
 		case action := <-actionChan:
-			if action == nil {
-				return
-			}
 			action()
+		case <-stopChan:
+			return
 		case <-alloc.ticker.C:
 			if alloc.awaitingConsensus {
 				alloc.propose()

ipam/allocator_test.go

@@ -164,9 +164,9 @@ func TestAllocatorClaim(t *testing.T) {
 		testAddr2  = "10.0.4.2/24"
 	)
 
-	allocs, _, subnet := makeNetworkOfAllocators(2, universe)
+	allocs, router, subnet := makeNetworkOfAllocators(2, universe)
+	defer stopNetworkOfAllocators(allocs, router)
 	alloc := allocs[1]
-	defer alloc.Stop()
 	addr1, _ := address.ParseCIDR(testAddr1)
 
 	// First claim should trigger "dunno, I'm going to wait"
@@ -333,6 +333,7 @@ func TestTransfer(t *testing.T) {
 		cidr = "10.0.1.7/22"
 	)
 	allocs, router, subnet := makeNetworkOfAllocators(3, cidr)
+	defer stopNetworkOfAllocators(allocs, router)
 	alloc1 := allocs[0]
 	alloc2 := allocs[1]
 	alloc3 := allocs[2] // This will be 'master' and get the first range
@@ -372,8 +373,8 @@ func TestFakeRouterSimple(t *testing.T) {
 	const (
 		cidr = "10.0.1.7/22"
 	)
-	allocs, _, subnet := makeNetworkOfAllocators(2, cidr)
-	defer stopNetworkOfAllocators(allocs)
+	allocs, router, subnet := makeNetworkOfAllocators(2, cidr)
+	defer stopNetworkOfAllocators(allocs, router)
 
 	alloc1 := allocs[0]
 	//alloc2 := allocs[1]
@@ -391,8 +392,8 @@ func TestAllocatorFuzz(t *testing.T) {
 		concurrency  = 5
 		cidr         = "10.0.1.7/22"
 	)
-	allocs, _, subnet := makeNetworkOfAllocators(nodes, cidr)
-	defer stopNetworkOfAllocators(allocs)
+	allocs, router, subnet := makeNetworkOfAllocators(nodes, cidr)
+	defer stopNetworkOfAllocators(allocs, router)
 
 	// Test state
 	// For each IP issued we store the allocator
@@ -599,9 +600,9 @@ func TestSpaceRequest(t *testing.T) {
 		container2 = "cont-2"
 		universe   = "10.32.0.0/12"
 	)
-	allocs, gossipRouter, subnet := makeNetworkOfAllocators(1, universe)
+	allocs, router, subnet := makeNetworkOfAllocators(1, universe)
+	defer stopNetworkOfAllocators(allocs, router)
 	alloc1 := allocs[0]
-	defer alloc1.Stop()
 
 	addr, err := alloc1.Allocate(container1, subnet, returnFalse)
 	require.Nil(t, err, "")
@@ -612,7 +613,7 @@ func TestSpaceRequest(t *testing.T) {
 
 	// Start a new peer
 	alloc2, _ := makeAllocator("02:00:00:02:00:00", universe, 2)
-	alloc2.SetInterfaces(gossipRouter.Connect(alloc2.ourName, alloc2))
+	alloc2.SetInterfaces(router.Connect(alloc2.ourName, alloc2))
 	alloc2.Start()
 	defer alloc2.Stop()
 	alloc2.Allocate(container2, subnet, returnFalse)

ipam/testutils_test.go

@@ -232,7 +232,10 @@ func makeNetworkOfAllocators(size int, cidr string) ([]*Allocator, *gossip.TestR
 	return allocs, gossipRouter, subnet
 }
 
-func stopNetworkOfAllocators(allocs []*Allocator) {
+func stopNetworkOfAllocators(allocs []*Allocator, gossipRouter *gossip.TestRouter) {
+	// NB: We must stop the router first since ipam gossip makes
+	// synchronous calls into the allocator.
+	gossipRouter.Stop()
 	for _, alloc := range allocs {
 		alloc.Stop()
 	}