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 <aboch@docker.com>

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; \

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 {

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)
+}

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
 	}