From a509244057c689eacada0b5f8c75aeb434b99730 Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Fri, 22 Jan 2016 10:46:05 -0800 Subject: [PATCH] Fix predefined pool reservation - The pool request code does not behave properly in case of concurrent requests when client does not specify a preferred pool. It may dispense the same predefined pool to different networks. - The issue is common for local and global address spaces Signed-off-by: Alessandro Boch --- libnetwork/Makefile | 2 +- libnetwork/ipam/allocator.go | 27 +++++---- libnetwork/ipam/allocator_test.go | 98 +++++++++++++++++++++++++++++++ libnetwork/ipam/structures.go | 5 +- 4 files changed, 119 insertions(+), 13 deletions(-) diff --git a/libnetwork/Makefile b/libnetwork/Makefile index 0741af2f62..2031826c7d 100644 --- a/libnetwork/Makefile +++ b/libnetwork/Makefile @@ -63,7 +63,7 @@ run-tests: if ls $$dir/*.go &> /dev/null; then \ pushd . &> /dev/null ; \ cd $$dir ; \ - $(shell which godep) go test ${INSIDECONTAINER} -test.parallel 3 -test.v -covermode=count -coverprofile=./profile.tmp ; \ + $(shell which godep) go test ${INSIDECONTAINER} -test.parallel 5 -test.v -covermode=count -coverprofile=./profile.tmp ; \ ret=$$? ;\ if [ $$ret -ne 0 ]; then exit $$ret; fi ;\ popd &> /dev/null; \ diff --git a/libnetwork/ipam/allocator.go b/libnetwork/ipam/allocator.go index ce404e2ad2..4fa7e1e972 100644 --- a/libnetwork/ipam/allocator.go +++ b/libnetwork/ipam/allocator.go @@ -145,12 +145,12 @@ func (a *Allocator) GetDefaultAddressSpaces() (string, string, error) { // RequestPool returns an address pool along with its unique id. func (a *Allocator) RequestPool(addressSpace, pool, subPool string, options map[string]string, v6 bool) (string, *net.IPNet, map[string]string, error) { log.Debugf("RequestPool(%s, %s, %s, %v, %t)", addressSpace, pool, subPool, options, v6) - k, nw, ipr, err := a.parsePoolRequest(addressSpace, pool, subPool, v6) +retry: + k, nw, ipr, pdf, err := a.parsePoolRequest(addressSpace, pool, subPool, v6) if err != nil { return "", nil, nil, types.InternalErrorf("failed to parse pool request for address space %q pool %q subpool %q: %v", addressSpace, pool, subPool, err) } -retry: if err := a.refresh(addressSpace); err != nil { return "", nil, nil, err } @@ -160,8 +160,12 @@ retry: return "", nil, nil, err } - insert, err := aSpace.updatePoolDBOnAdd(*k, nw, ipr) + insert, err := aSpace.updatePoolDBOnAdd(*k, nw, ipr, pdf) if err != nil { + if _, ok := err.(types.MaskableError); ok { + log.Debugf("Retrying predefined pool search: %v", err) + goto retry + } return "", nil, nil, err } @@ -221,38 +225,39 @@ func (a *Allocator) getAddrSpace(as string) (*addrSpace, error) { return aSpace, nil } -func (a *Allocator) parsePoolRequest(addressSpace, pool, subPool string, v6 bool) (*SubnetKey, *net.IPNet, *AddressRange, error) { +func (a *Allocator) parsePoolRequest(addressSpace, pool, subPool string, v6 bool) (*SubnetKey, *net.IPNet, *AddressRange, bool, error) { var ( nw *net.IPNet ipr *AddressRange err error + pdf = false ) if addressSpace == "" { - return nil, nil, nil, ipamapi.ErrInvalidAddressSpace + return nil, nil, nil, false, ipamapi.ErrInvalidAddressSpace } if pool == "" && subPool != "" { - return nil, nil, nil, ipamapi.ErrInvalidSubPool + return nil, nil, nil, false, ipamapi.ErrInvalidSubPool } if pool != "" { if _, nw, err = net.ParseCIDR(pool); err != nil { - return nil, nil, nil, ipamapi.ErrInvalidPool + return nil, nil, nil, false, ipamapi.ErrInvalidPool } if subPool != "" { if ipr, err = getAddressRange(subPool, nw); err != nil { - return nil, nil, nil, err + return nil, nil, nil, false, err } } } else { if nw, err = a.getPredefinedPool(addressSpace, v6); err != nil { - return nil, nil, nil, err + return nil, nil, nil, false, err } - + pdf = true } - return &SubnetKey{AddressSpace: addressSpace, Subnet: nw.String(), ChildSubnet: subPool}, nw, ipr, nil + return &SubnetKey{AddressSpace: addressSpace, Subnet: nw.String(), ChildSubnet: subPool}, nw, ipr, pdf, nil } func (a *Allocator) insertBitMask(key SubnetKey, pool *net.IPNet) error { diff --git a/libnetwork/ipam/allocator_test.go b/libnetwork/ipam/allocator_test.go index d2bbdca56e..96dc2a9ab0 100644 --- a/libnetwork/ipam/allocator_test.go +++ b/libnetwork/ipam/allocator_test.go @@ -2,10 +2,12 @@ package ipam import ( "encoding/json" + "flag" "fmt" "io/ioutil" "math/rand" "net" + "strconv" "testing" "time" @@ -1168,3 +1170,99 @@ func checkDBEquality(a1, a2 *Allocator, t *testing.T) { } } } + +const ( + numInstances = 5 + first = 0 + last = numInstances - 1 +) + +var ( + allocator *Allocator + start = make(chan struct{}) + done = make(chan chan struct{}, numInstances-1) + pools = make([]*net.IPNet, numInstances) +) + +func runParallelTests(t *testing.T, instance int) { + var err error + + t.Parallel() + + pTest := flag.Lookup("test.parallel") + if pTest == nil { + t.Skip("Skipped because test.parallel flag not set;") + } + numParallel, err := strconv.Atoi(pTest.Value.String()) + if err != nil { + t.Fatal(err) + } + if numParallel < numInstances { + t.Skip("Skipped because t.parallel was less than ", numInstances) + } + + // The first instance creates the allocator, gives the start + // and finally checks the pools each instance was assigned + if instance == first { + allocator, err = getAllocator() + if err != nil { + t.Fatal(err) + } + close(start) + } + + if instance != first { + select { + case <-start: + } + + instDone := make(chan struct{}) + done <- instDone + defer close(instDone) + + if instance == last { + defer close(done) + } + } + + _, pools[instance], _, err = allocator.RequestPool(localAddressSpace, "", "", nil, false) + if err != nil { + t.Fatal(err) + } + + if instance == first { + for instDone := range done { + select { + case <-instDone: + } + } + // Now check each instance got a different pool + for i := 0; i < numInstances; i++ { + for j := i + 1; j < numInstances; j++ { + if types.CompareIPNet(pools[i], pools[j]) { + t.Fatalf("Instance %d and %d were given the same predefined pool: %v", i, j, pools) + } + } + } + } +} + +func TestParallelPredefinedRequest1(t *testing.T) { + runParallelTests(t, 0) +} + +func TestParallelPredefinedRequest2(t *testing.T) { + runParallelTests(t, 1) +} + +func TestParallelPredefinedRequest3(t *testing.T) { + runParallelTests(t, 2) +} + +func TestParallelPredefinedRequest4(t *testing.T) { + runParallelTests(t, 3) +} + +func TestParallelPredefinedRequest5(t *testing.T) { + runParallelTests(t, 4) +} diff --git a/libnetwork/ipam/structures.go b/libnetwork/ipam/structures.go index cd0593ceff..601eda4fba 100644 --- a/libnetwork/ipam/structures.go +++ b/libnetwork/ipam/structures.go @@ -257,12 +257,15 @@ func (aSpace *addrSpace) New() datastore.KVObject { } } -func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *AddressRange) (func() error, error) { +func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *AddressRange, pdf bool) (func() error, error) { aSpace.Lock() defer aSpace.Unlock() // Check if already allocated if p, ok := aSpace.subnets[k]; ok { + if pdf { + return nil, types.InternalMaskableErrorf("predefined pool %s is already reserved", nw) + } aSpace.incRefCount(p, 1) return func() error { return nil }, nil }