From 64dc7a1f69613eb21a8de5a02a0507d78eefcd01 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 11 May 2016 12:05:45 +0100 Subject: [PATCH 1/4] Donate power-of-2-aligned ranges instead of just slicing at the mid-point. Also fix up space tests to match. --- ipam/allocator_test.go | 2 +- ipam/space/space.go | 16 +++++++--- ipam/space/space_test.go | 63 +++++++++++++++++++------------------ net/address/address.go | 29 +++++++++++++++++ net/address/address_test.go | 26 +++++++++++++++ 5 files changed, 99 insertions(+), 37 deletions(-) create mode 100644 net/address/address_test.go diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index af05530a..40401052 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -343,7 +343,7 @@ func TestNoFrag(t *testing.T) { allocs[2].actionChan <- func() { resultChan <- len(allocs[2].ring.Entries) } - require.True(t, <-resultChan < 5, "excessive ring fragmentation") + require.True(t, <-resultChan < 7, "excessive ring fragmentation") stopNetworkOfAllocators(allocs, router) } } diff --git a/ipam/space/space.go b/ipam/space/space.go index 80d99c97..41594898 100644 --- a/ipam/space/space.go +++ b/ipam/space/space.go @@ -122,8 +122,11 @@ func (s *Space) biggestFreeRange(r address.Range) (biggest address.Range) { biggestSize := address.Count(0) s.walkFree(r, func(chunk address.Range) bool { if size := chunk.Size(); size >= biggestSize { - biggest = chunk - biggestSize = size + chunk = chunk.BiggestPow2AlignedRange() + if size = chunk.Size(); size >= biggestSize { + biggest = chunk + biggestSize = size + } } return false }) @@ -137,9 +140,12 @@ func (s *Space) Donate(r address.Range) (address.Range, bool) { return address.Range{}, false } - // Donate half of that biggest free range. Note size/2 rounds down, so - // the resulting donation size rounds up, and in particular can't be empty. - biggest.Start = address.Add(biggest.Start, address.Offset(biggest.Size()/2)) + // Donate no more than half what we have available + if biggest.Size() > s.NumFreeAddressesInRange(r)/2 { + // Note size/2 rounds down, so the resulting donation size + // rounds up, and in particular can't be empty. + biggest.Start = address.Add(biggest.Start, address.Offset(biggest.Size()/2)) + } s.ours = subtract(s.ours, biggest.Start, biggest.End) s.free = subtract(s.free, biggest.Start, biggest.End) diff --git a/ipam/space/space_test.go b/ipam/space/space_test.go index 295cfd57..486410f5 100644 --- a/ipam/space/space_test.go +++ b/ipam/space/space_test.go @@ -68,7 +68,7 @@ func TestLowlevel(t *testing.T) { wt.AssertErrorInterface(t, (*error)(nil), s.Free(100), "double free") r, ok := s.Donate(address.NewRange(0, 1000)) - require.True(t, ok && r.Start == 125 && r.Size() == 25, "donate") + require.Equal(t, address.NewRange(0xa0, 0x20), r, "donate") // test Donate when addresses are scarce s = New() @@ -78,7 +78,7 @@ func TestLowlevel(t *testing.T) { require.NoError(t, s.Claim(0)) require.NoError(t, s.Claim(2)) r, ok = s.Donate(address.NewRange(0, 1000)) - require.True(t, ok && r.Start == 1 && r.End == 2, "donate") + require.Equal(t, address.NewRange(1, 1), r, "donate") r, ok = s.Donate(address.NewRange(0, 1000)) require.True(t, !ok, "donate should fail") } @@ -124,20 +124,21 @@ func TestSpaceAllocate(t *testing.T) { func TestSpaceFree(t *testing.T) { const ( - testAddr1 = "10.0.3.4" + testAddr1 = "10.0.3.16" testAddrx = "10.0.3.19" testAddry = "10.0.9.19" containerID = "deadbeef" + size = 16 ) - entireRange := address.NewRange(ip(testAddr1), 20) - space := makeSpace(ip(testAddr1), 20) + entireRange := address.NewRange(ip(testAddr1), size) + space := makeSpace(ip(testAddr1), size) // Check we are prepared to give up the entire space r := space.biggestFreeRange(entireRange) - require.True(t, r.Start == ip(testAddr1) && r.Size() == 20, "Wrong space") + require.Equal(t, address.NewRange(ip(testAddr1), size), r, "Wrong space") - for i := 0; i < 20; i++ { + for i := 0; i < size; i++ { ok, _ := space.Allocate(entireRange) require.True(t, ok, "Failed to get address") } @@ -149,44 +150,44 @@ func TestSpaceFree(t *testing.T) { require.True(t, r.Size() == 0, "Wrong space") // Free in the middle - require.NoError(t, space.Free(ip("10.0.3.13"))) - r = space.biggestFreeRange(entireRange) - require.True(t, r.Start == ip("10.0.3.13") && r.Size() == 1, "Wrong space") - - // Free one at the end require.NoError(t, space.Free(ip("10.0.3.23"))) r = space.biggestFreeRange(entireRange) require.True(t, r.Start == ip("10.0.3.23") && r.Size() == 1, "Wrong space") + // Free one at the end + require.NoError(t, space.Free(ip("10.0.3.31"))) + r = space.biggestFreeRange(entireRange) + require.True(t, r.Start == ip("10.0.3.31") && r.Size() == 1, "Wrong space") + // Now free a few at the end - require.NoError(t, space.Free(ip("10.0.3.22"))) - require.NoError(t, space.Free(ip("10.0.3.21"))) + require.NoError(t, space.Free(ip("10.0.3.30"))) + require.NoError(t, space.Free(ip("10.0.3.29"))) require.Equal(t, address.Count(4), space.NumFreeAddresses()) - // Now get the biggest free space; should be 3.21 + // Now get the biggest free space; should be 3.30 r = space.biggestFreeRange(entireRange) - require.True(t, r.Start == ip("10.0.3.21") && r.Size() == 3, "Wrong space") + require.Equal(t, address.NewRange(ip("10.0.3.30"), 2), r, "Wrong space") // Now free a few in the middle - require.NoError(t, space.Free(ip("10.0.3.12"))) - require.NoError(t, space.Free(ip("10.0.3.11"))) - require.NoError(t, space.Free(ip("10.0.3.10"))) + require.NoError(t, space.Free(ip("10.0.3.24"))) + require.NoError(t, space.Free(ip("10.0.3.22"))) + require.NoError(t, space.Free(ip("10.0.3.21"))) require.Equal(t, address.Count(7), space.NumFreeAddresses()) - // Now get the biggest free space; should be 3.21 + // Now get the biggest free space; should be 3.30 r = space.biggestFreeRange(entireRange) - require.True(t, r.Start == ip("10.0.3.10") && r.Size() == 4, "Wrong space") + require.Equal(t, address.NewRange(ip("10.0.3.30"), 2), r, "Wrong space") - require.Equal(t, []address.Range{{Start: ip("10.0.3.4"), End: ip("10.0.3.24")}}, space.OwnedRanges()) + require.Equal(t, []address.Range{{Start: ip("10.0.3.16"), End: ip("10.0.3.32")}}, space.OwnedRanges()) } func TestDonateSimple(t *testing.T) { const ( testAddr1 = "10.0.1.0" testAddr2 = "10.0.1.32" - size = 48 + size = 64 ) var ( @@ -199,7 +200,7 @@ func TestDonateSimple(t *testing.T) { r, ok := ps1.Donate(address.NewRange(ip(testAddr1), size)) numGivenUp := r.Size() require.True(t, ok, "Donate result") - require.Equal(t, "10.0.1.24", r.Start.String(), "Invalid start") + require.Equal(t, "10.0.1.32", r.Start.String(), "Invalid start") require.Equal(t, address.Count(size/2), numGivenUp) require.Equal(t, address.Count(size/2), ps1.NumFreeAddresses()) @@ -241,13 +242,13 @@ func TestDonateHard(t *testing.T) { // Now split newRange, ok := spaceset.Donate(address.NewRange(start, size)) require.True(t, ok, "GiveUpSpace result") - require.Equal(t, ip("10.0.1.23"), newRange.Start) - require.Equal(t, address.Count(24), newRange.Size()) - require.Equal(t, address.Count(23), spaceset.NumFreeAddresses()) + require.Equal(t, address.NewRange(ip("10.0.1.16"), 16), newRange, "Wrong space") + require.Equal(t, address.Count(31), spaceset.NumFreeAddresses()) - //Space set should now have 2 spaces - expected := New() - expected.Add(start, 23) - expected.ours = add(nil, ip("10.0.1.47"), ip("10.0.1.48")) + //Space set should now have 3 spaces + expected := &Space{ + ours: add(nil, ip("10.0.1.47"), ip("10.0.1.48")), + free: add(add(nil, ip("10.0.1.0"), ip("10.0.1.16")), ip("10.0.1.32"), ip("10.0.1.47")), + } require.Equal(t, expected, spaceset) } diff --git a/net/address/address.go b/net/address/address.go index f3a90025..f3f54926 100644 --- a/net/address/address.go +++ b/net/address/address.go @@ -35,6 +35,35 @@ func (r Range) AsCIDRString() string { return CIDR{Addr: r.Start, PrefixLen: prefixLen}.String() } +// return the highest bit set in a +// algorithm from http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 +func msb(v Count) Count { + v |= v >> 1 + v |= v >> 2 + v |= v >> 4 + v |= v >> 8 + v |= v >> 16 + // Cast to 64-bit to avoid overflow when original v >= 0x80000000 + return Count((uint64(v) + 1) / 2) +} + +func (r Range) BiggestPow2AlignedRange() Range { + sizeMsb := Offset(msb(r.Size())) + maskedSize := Offset(r.Size()) & (sizeMsb - 1) + maskedStart := Offset(r.Start) & (sizeMsb - 1) + if maskedStart == 0 { + return NewRange(r.Start, sizeMsb) + } + if sizeMsb-maskedStart <= maskedSize { + return NewRange(Add(r.Start, sizeMsb-maskedStart), sizeMsb) + } + size := sizeMsb / 2 + if size >= maskedStart { + return NewRange(Add(r.Start, size-maskedStart), size) + } + return NewRange(Add(r.Start, sizeMsb-maskedStart), size) +} + func MakeCIDR(subnet CIDR, addr Address) CIDR { return CIDR{Addr: addr, PrefixLen: subnet.PrefixLen} } diff --git a/net/address/address_test.go b/net/address/address_test.go new file mode 100644 index 00000000..78cdceef --- /dev/null +++ b/net/address/address_test.go @@ -0,0 +1,26 @@ +package address + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestBiggestPow2AlignedRange(t *testing.T) { + require.Equal(t, NewRange(0, 1), NewRange(0, 1).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 2), NewRange(0, 2).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 2), NewRange(0, 3).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 4), NewRange(0, 7).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(1, 1), NewRange(1, 1).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(1, 1), NewRange(1, 2).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(2, 2), NewRange(1, 3).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(2, 2), NewRange(1, 4).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(4, 4), NewRange(1, 8).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(4, 4), NewRange(2, 8).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(4, 4), NewRange(4, 8).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(8, 4), NewRange(8, 7).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(8, 8), NewRange(8, 8).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 0x4000), NewRange(0, 0x7fff).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0x2000, 2), NewRange(0x2000, 3).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0xffff, 1), NewRange(0xffff, 1).BiggestPow2AlignedRange()) +} From d1e2aa6d36bcd5b601f87b1d2674ec9dc37cf78b Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 11 May 2016 17:34:23 +0100 Subject: [PATCH 2/4] Use quick.Check instead of many specific tests --- net/address/address_test.go | 41 +++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/net/address/address_test.go b/net/address/address_test.go index 78cdceef..42045ca4 100644 --- a/net/address/address_test.go +++ b/net/address/address_test.go @@ -2,25 +2,40 @@ package address import ( "testing" + "testing/quick" "github.com/stretchr/testify/require" ) +func isPower2(x Count) bool { + if x == 0 { + return false + } + for ; x > 1; x /= 2 { + if x&1 != 0 { + return false + } + } + return true +} + func TestBiggestPow2AlignedRange(t *testing.T) { require.Equal(t, NewRange(0, 1), NewRange(0, 1).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0, 2), NewRange(0, 2).BiggestPow2AlignedRange()) require.Equal(t, NewRange(0, 2), NewRange(0, 3).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0, 4), NewRange(0, 7).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(1, 1), NewRange(1, 1).BiggestPow2AlignedRange()) require.Equal(t, NewRange(1, 1), NewRange(1, 2).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(2, 2), NewRange(1, 3).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(2, 2), NewRange(1, 4).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(4, 4), NewRange(1, 8).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(4, 4), NewRange(2, 8).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(4, 4), NewRange(4, 8).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(8, 4), NewRange(8, 7).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(8, 8), NewRange(8, 8).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0, 0x4000), NewRange(0, 0x7fff).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0x2000, 2), NewRange(0x2000, 3).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0xffff, 1), NewRange(0xffff, 1).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 0x40000000), NewRange(0, 0x7fffffff).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0xfffffffe, 1), NewRange(0xfffffffe, 1).BiggestPow2AlignedRange()) + prop := func(start Address, size Offset) bool { + if size > Offset(0xffffffff)-Offset(start) { // out of range + return true + } + r := NewRange(start, size) + result := r.BiggestPow2AlignedRange() + return r.Contains(result.Start) && + r.Contains(result.End-1) && + isPower2(result.Size()) && + result.Size() > r.Size()/4 && + Count(result.Start)%result.Size() == 0 + } + require.NoError(t, quick.Check(prop, &quick.Config{MaxCount: 1000000})) } From 1ec2c0fb3fa98efe01a9894daa18987f671ed1c6 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 11 May 2016 17:35:17 +0100 Subject: [PATCH 3/4] Use a CIDR-aligned CIDR in unit tests --- ipam/allocator_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 40401052..ce7c3759 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -220,7 +220,7 @@ func (alloc *Allocator) pause() func() { } func TestCancel(t *testing.T) { - const cidr = "10.0.1.7/26" + const cidr = "10.0.4.0/26" router := gossip.NewTestRouter(0.0) alloc1, subnet := makeAllocator("01:00:00:02:00:00", cidr, 2) @@ -281,7 +281,7 @@ func TestCancel(t *testing.T) { func TestCancelOnDied(t *testing.T) { const ( - cidr = "10.0.1.7/26" + cidr = "10.0.4.0/26" container1 = "abcdef" ) @@ -334,7 +334,7 @@ func TestGossipShutdown(t *testing.T) { } func TestNoFrag(t *testing.T) { - const cidr = "10.0.1.7/22" + const cidr = "10.0.4.0/22" resultChan := make(chan int) for i := 0; i < 100; i++ { allocs, router, subnet := makeNetworkOfAllocators(3, cidr) @@ -343,13 +343,13 @@ func TestNoFrag(t *testing.T) { allocs[2].actionChan <- func() { resultChan <- len(allocs[2].ring.Entries) } - require.True(t, <-resultChan < 7, "excessive ring fragmentation") + require.True(t, <-resultChan < 6, "excessive ring fragmentation") stopNetworkOfAllocators(allocs, router) } } func TestTransfer(t *testing.T) { - const cidr = "10.0.1.7/22" + const cidr = "10.0.4.0/22" allocs, router, subnet := makeNetworkOfAllocators(3, cidr) defer stopNetworkOfAllocators(allocs, router) alloc0 := allocs[0] @@ -389,7 +389,7 @@ func TestTransfer(t *testing.T) { } func TestFakeRouterSimple(t *testing.T) { - const cidr = "10.0.1.7/22" + const cidr = "10.0.4.0/22" allocs, router, subnet := makeNetworkOfAllocators(2, cidr) defer stopNetworkOfAllocators(allocs, router) @@ -407,7 +407,7 @@ func TestAllocatorFuzz(t *testing.T) { nodes = 5 maxAddresses = 1000 concurrency = 5 - cidr = "10.0.1.7/22" + cidr = "10.0.4.0/22" ) allocs, router, subnet := makeNetworkOfAllocators(nodes, cidr) defer stopNetworkOfAllocators(allocs, router) From d551b6b0a8afa0f2827435afb0fa35f06fb82afe Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 13 May 2016 16:26:34 +0100 Subject: [PATCH 4/4] Review feedback --- ipam/space/space.go | 2 +- net/address/address.go | 4 ++-- net/address/address_test.go | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/ipam/space/space.go b/ipam/space/space.go index 41594898..f0acb0db 100644 --- a/ipam/space/space.go +++ b/ipam/space/space.go @@ -122,7 +122,7 @@ func (s *Space) biggestFreeRange(r address.Range) (biggest address.Range) { biggestSize := address.Count(0) s.walkFree(r, func(chunk address.Range) bool { if size := chunk.Size(); size >= biggestSize { - chunk = chunk.BiggestPow2AlignedRange() + chunk = chunk.BiggestCIDRRange() if size = chunk.Size(); size >= biggestSize { biggest = chunk biggestSize = size diff --git a/net/address/address.go b/net/address/address.go index f3f54926..b6e992ba 100644 --- a/net/address/address.go +++ b/net/address/address.go @@ -35,7 +35,7 @@ func (r Range) AsCIDRString() string { return CIDR{Addr: r.Start, PrefixLen: prefixLen}.String() } -// return the highest bit set in a +// return the highest bit set in v // algorithm from http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 func msb(v Count) Count { v |= v >> 1 @@ -47,7 +47,7 @@ func msb(v Count) Count { return Count((uint64(v) + 1) / 2) } -func (r Range) BiggestPow2AlignedRange() Range { +func (r Range) BiggestCIDRRange() Range { sizeMsb := Offset(msb(r.Size())) maskedSize := Offset(r.Size()) & (sizeMsb - 1) maskedStart := Offset(r.Start) & (sizeMsb - 1) diff --git a/net/address/address_test.go b/net/address/address_test.go index 42045ca4..46f699b4 100644 --- a/net/address/address_test.go +++ b/net/address/address_test.go @@ -20,17 +20,17 @@ func isPower2(x Count) bool { } func TestBiggestPow2AlignedRange(t *testing.T) { - require.Equal(t, NewRange(0, 1), NewRange(0, 1).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0, 2), NewRange(0, 3).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(1, 1), NewRange(1, 2).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0, 0x40000000), NewRange(0, 0x7fffffff).BiggestPow2AlignedRange()) - require.Equal(t, NewRange(0xfffffffe, 1), NewRange(0xfffffffe, 1).BiggestPow2AlignedRange()) + require.Equal(t, NewRange(0, 1), NewRange(0, 1).BiggestCIDRRange()) + require.Equal(t, NewRange(1, 1), NewRange(1, 2).BiggestCIDRRange()) + require.Equal(t, NewRange(2, 2), NewRange(1, 3).BiggestCIDRRange()) + require.Equal(t, NewRange(0, 0x40000000), NewRange(0, 0x7fffffff).BiggestCIDRRange()) + require.Equal(t, NewRange(0xfffffffe, 1), NewRange(0xfffffffe, 1).BiggestCIDRRange()) prop := func(start Address, size Offset) bool { if size > Offset(0xffffffff)-Offset(start) { // out of range return true } r := NewRange(start, size) - result := r.BiggestPow2AlignedRange() + result := r.BiggestCIDRRange() return r.Contains(result.Start) && r.Contains(result.End-1) && isPower2(result.Size()) &&