From 4d352d6eb1ff8287601e8c08274824397b10078c Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 10 Apr 2016 11:04:30 +0100 Subject: [PATCH 1/2] stop router created by makeNetworkOfAllocators so that unit tests don't leave garbage behind --- ipam/allocator_test.go | 19 ++++++++++--------- ipam/testutils_test.go | 5 ++++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 300a147e..4ec4a223 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -153,9 +153,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" @@ -322,6 +322,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 @@ -361,8 +362,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] @@ -380,8 +381,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 @@ -588,9 +589,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, "") @@ -601,7 +602,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) diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index 6e6b374a..b665771b 100644 --- a/ipam/testutils_test.go +++ b/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() } From 695babf7bafd2118c5167f7db5b92b3e7e79c8e6 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 10 Apr 2016 12:00:09 +0100 Subject: [PATCH 2/2] make Allocator.Stop() idempotent and non-blocking So that tests don't have to be careful about ensuring that allocators are stopped exactly once, or rely on the magic mesh.ChannelSize for non-blocking behaviour. --- ipam/allocator.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 20c78bd5..374cfa5e 100644 --- a/ipam/allocator.go +++ b/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()