From a7e0f036474c9c7da1329ab861c7d2c9dadea8a9 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Sep 2015 17:02:35 +0100 Subject: [PATCH 1/2] Use explicit index into operations slice in cancelOpsFor() to avoid mis-indexing when more than one op is removed --- ipam/allocator.go | 6 ++++-- ipam/allocator_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 28f2e449..4f692f84 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -126,12 +126,14 @@ func (alloc *Allocator) cancelOps(ops *[]operation) { // if we found any. func (alloc *Allocator) cancelOpsFor(ops *[]operation, ident string) bool { var found bool - for i, op := range *ops { - if op.ForContainer(ident) { + for i := 0; i < len(*ops); { + if op := (*ops)[i]; op.ForContainer(ident) { found = true op.Cancel() *ops = append((*ops)[:i], (*ops)[i+1:]...) + continue } + i++ } return found } diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index fd444190..309de81c 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -240,6 +240,42 @@ func TestCancel(t *testing.T) { } } +func TestCancelOnDied(t *testing.T) { + const ( + CIDR = "10.0.1.7/26" + container1 = "abcdef" + ) + + router := gossip.NewTestRouter(0.0) + + alloc1, subnet := makeAllocator("01:00:00:02:00:00", CIDR, 2) + alloc1.SetInterfaces(router.Connect(alloc1.ourName, alloc1)) + + alloc1.Start() + + doneChan := make(chan bool) + go func() { + _, ok := alloc1.Allocate(container1, subnet, nil) + doneChan <- ok == nil + }() + + // Do another allocation in parallel, to check that is handled correctly + go func() { + _, ok := alloc1.Allocate(container1, subnet, nil) + doneChan <- ok == nil + }() + + AssertNothingSent(t, doneChan) + time.Sleep(100 * time.Millisecond) + AssertNothingSent(t, doneChan) + + alloc1.ContainerDied(container1) + + if <-doneChan || <-doneChan { + require.FailNow(t, "Error: got result from Allocate") + } +} + func TestGossipShutdown(t *testing.T) { const ( container1 = "abcdef" From d55c68c248583be225f49bf7c02ea439821f266c Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 15 Sep 2015 16:18:37 +0100 Subject: [PATCH 2/2] Review feedback --- ipam/allocator.go | 4 ++-- ipam/allocator_test.go | 18 +++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 4f692f84..c7258ce6 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -131,9 +131,9 @@ func (alloc *Allocator) cancelOpsFor(ops *[]operation, ident string) bool { found = true op.Cancel() *ops = append((*ops)[:i], (*ops)[i+1:]...) - continue + } else { + i++ } - i++ } return found } diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 309de81c..220ee176 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -229,7 +229,6 @@ func TestCancel(t *testing.T) { doneChan <- ok == nil }() - AssertNothingSent(t, doneChan) time.Sleep(100 * time.Millisecond) AssertNothingSent(t, doneChan) @@ -247,30 +246,27 @@ func TestCancelOnDied(t *testing.T) { ) router := gossip.NewTestRouter(0.0) - alloc1, subnet := makeAllocator("01:00:00:02:00:00", CIDR, 2) alloc1.SetInterfaces(router.Connect(alloc1.ourName, alloc1)) - alloc1.Start() doneChan := make(chan bool) - go func() { + f := func() { _, ok := alloc1.Allocate(container1, subnet, nil) doneChan <- ok == nil - }() + } - // Do another allocation in parallel, to check that is handled correctly - go func() { - _, ok := alloc1.Allocate(container1, subnet, nil) - doneChan <- ok == nil - }() + // Attempt two allocations in parallel, to check that this is handled correctly + go f() + go f() - AssertNothingSent(t, doneChan) + // Nothing should happen, because we declared the quorum as 2 time.Sleep(100 * time.Millisecond) AssertNothingSent(t, doneChan) alloc1.ContainerDied(container1) + // Check that the two allocations both exit with an error if <-doneChan || <-doneChan { require.FailNow(t, "Error: got result from Allocate") }