Verify network allocation on creating network

Current network allocation is run asynchronously with network (object) creation, when a user specifies an overlapped subnet, the network creation succeeds but this network is useless with empty IPAM config.

This commit makes network creation use a channel to wait for the result of network allocation from Allocator before return.
Because of this change, some _test.go files are modified.

Also added TestCreateNetworkOverlapIP to network_test.go.

Remove duplicated line in "make coverage" of direct.mk.

Signed-off-by: Xinfeng Liu <xinfeng.liu@gmail.com>

Author: xinfengliu

direct.mk

@@ -124,7 +124,6 @@ coverage: ## generate coverprofiles from the unit tests
 	@echo "🐳 $@"
 	@( for pkg in $(filter-out ${INTEGRATION_PACKAGE},${PACKAGES}); do \
 		go test ${RACE} -tags "${DOCKER_BUILDTAGS}" -test.short -coverprofile="../../../$$pkg/coverage.txt" -covermode=atomic $$pkg || exit; \
-		go test ${RACE} -tags "${DOCKER_BUILDTAGS}" -test.short -coverprofile="../../../$$pkg/coverage.txt" -covermode=atomic $$pkg || exit; \
 	done )
 
 .PHONY: coverage-integration

manager/allocator/network.go

@@ -3,6 +3,7 @@ package allocator
 import (
 	"context"
 	"fmt"
+	"sync"
 	"time"
 
 	"github.com/docker/go-events"
@@ -28,6 +29,9 @@ var (
 	errNoChanges = errors.New("task unchanged")
 
 	retryInterval = 5 * time.Minute
+
+	networkAllocationErrMap = make(map[string]chan error)
+	mu                      sync.Mutex
 )
 
 // Network context information which is used throughout the network allocation code.
@@ -67,6 +71,27 @@ type networkContext struct {
 	somethingWasDeallocated bool
 }
 
+// GetNetworkAllocationErrChan retrieves correspondent chan error given a network ID
+func GetNetworkAllocationErrChan(id string) chan error {
+	mu.Lock()
+	defer mu.Unlock()
+	v, ok := networkAllocationErrMap[id]
+	if !ok {
+		v = make(chan error, 1) // don't want to block the sender
+		networkAllocationErrMap[id] = v
+	}
+	return v
+}
+
+// RemoveNetworkAllocationErrChan removes correspondent chan error given a network ID
+func RemoveNetworkAllocationErrChan(id string) {
+	mu.Lock()
+	defer mu.Unlock()
+	if _, ok := networkAllocationErrMap[id]; ok {
+		delete(networkAllocationErrMap, id)
+	}
+}
+
 func (a *Allocator) doNetworkInit(ctx context.Context) (err error) {
 	var netConfig *cnmallocator.NetworkConfig
 	// There are two ways user can invoke swarm init
@@ -179,28 +204,35 @@ func (a *Allocator) doNetworkAlloc(ctx context.Context, ev events.Event) {
 	switch v := ev.(type) {
 	case api.EventCreateNetwork:
 		n := v.Network.Copy()
+		errCh := GetNetworkAllocationErrChan(n.ID)
 		if nc.nwkAllocator.IsAllocated(n) {
+			errCh <- nil
 			break
 		}
 		if IsIngressNetwork(n) && nc.ingressNetwork != nil {
 			log.G(ctx).Errorf("Cannot allocate ingress network %s (%s) because another ingress network is already present: %s (%s)",
 				n.ID, n.Spec.Annotations.Name, nc.ingressNetwork.ID, nc.ingressNetwork.Spec.Annotations.Name)
+			errCh <- errors.Errorf("another ingress network is already present: %s", nc.ingressNetwork.Spec.Annotations.Name)
 			break
 		}
 
 		if err := a.allocateNetwork(ctx, n); err != nil {
 			log.G(ctx).WithError(err).Errorf("Failed allocation for network %s", n.ID)
+			errCh <- err
 			break
 		}
 
 		if err := a.store.Batch(func(batch *store.Batch) error {
 			return a.commitAllocatedNetwork(ctx, batch, n)
 		}); err != nil {
 			log.G(ctx).WithError(err).Errorf("Failed to commit allocation for network %s", n.ID)
+			errCh <- err
+			break
 		}
 		if IsIngressNetwork(n) {
 			nc.ingressNetwork = n
 		}
+		errCh <- nil
 	case api.EventDeleteNetwork:
 		n := v.Network.Copy()
 

manager/controlapi/network.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/libnetwork/ipamapi"
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/identity"
+	"github.com/docker/swarmkit/log"
 	"github.com/docker/swarmkit/manager/allocator"
 	"github.com/docker/swarmkit/manager/allocator/networkallocator"
 	"github.com/docker/swarmkit/manager/state/store"
@@ -128,6 +129,17 @@ func (s *Server) CreateNetwork(ctx context.Context, request *api.CreateNetworkRe
 		return nil, err
 	}
 
+	// Waiting for network allocation result, remove the network in store if failure
+	errCh := allocator.GetNetworkAllocationErrChan(n.ID)
+	err = <-errCh
+	allocator.RemoveNetworkAllocationErrChan(n.ID)
+	if err != nil {
+		if err1 := s.removeNetwork(n.ID); err1 != nil {
+			log.G(ctx).Error(err1)
+		}
+		return nil, err
+	}
+
 	return &api.CreateNetworkResponse{
 		Network: n,
 	}, nil

manager/controlapi/network_test.go

@@ -22,6 +22,21 @@ func createNetworkSpec(name string) *api.NetworkSpec {
 	}
 }
 
+func createOverlayNetworkSpecWithSubnet(name, subnet string) *api.NetworkSpec {
+	spec := createNetworkSpec(name)
+	spec.DriverConfig = &api.Driver{
+		Name: "overlay",
+	}
+	spec.IPAM = &api.IPAMOptions{
+		Configs: []*api.IPAMConfig{
+			&api.IPAMConfig{
+				Subnet: subnet,
+			},
+		},
+	}
+	return spec
+}
+
 // createInternalNetwork creates an internal network for testing. it is the same
 // as Server.CreateNetwork except without the label check.
 func (s *Server) createInternalNetwork(ctx context.Context, request *api.CreateNetworkRequest) (*api.CreateNetworkResponse, error) {
@@ -209,6 +224,25 @@ func TestCreateNetworkInvalidDriver(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestCreateNetworkOverlapIP(t *testing.T) {
+	ts := newTestServer(t)
+	defer ts.Stop()
+
+	//create first net
+	spec := createOverlayNetworkSpecWithSubnet("overlaynet", "10.2.0.0/24")
+	_, err := ts.Client.CreateNetwork(context.Background(), &api.CreateNetworkRequest{
+		Spec: spec,
+	})
+	assert.NoError(t, err)
+
+	//create second net with overlap subnet
+	spec2 := createOverlayNetworkSpecWithSubnet("overlaynet2", "10.2.0.0/24")
+	_, err = ts.Client.CreateNetwork(context.Background(), &api.CreateNetworkRequest{
+		Spec: spec2,
+	})
+	assert.Error(t, err)
+}
+
 func TestListNetworks(t *testing.T) {
 	ts := newTestServer(t)
 	defer ts.Stop()

manager/controlapi/node_test.go

@@ -292,8 +292,10 @@ func TestListNodesWithLabelFilter(t *testing.T) {
 	// should only return the first 2 nodes
 	assert.NoError(t, err)
 	assert.Len(t, r.Nodes, 2)
-	assert.Contains(t, r.Nodes, nodes[0])
-	assert.Contains(t, r.Nodes, nodes[1])
+
+	// r.Nodes contains NetworkAttachment, so only compare ID here
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[0].ID)
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[1].ID)
 
 	t.Log("list nodes with enginelabel1=shouldmatch engine filter")
 	r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{
@@ -304,8 +306,8 @@ func TestListNodesWithLabelFilter(t *testing.T) {
 	// should only return the first 2 nodes
 	assert.NoError(t, err)
 	assert.Len(t, r.Nodes, 2)
-	assert.Contains(t, r.Nodes, nodes[0])
-	assert.Contains(t, r.Nodes, nodes[1])
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[0].ID)
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[1].ID)
 
 	t.Log("list nodes with node two engine filters")
 	r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{
@@ -319,7 +321,7 @@ func TestListNodesWithLabelFilter(t *testing.T) {
 	// should only return the first node
 	assert.NoError(t, err)
 	assert.Len(t, r.Nodes, 1)
-	assert.Contains(t, r.Nodes, nodes[0])
+	assert.Equal(t, r.Nodes[0].ID, nodes[0].ID)
 
 	t.Log("list nodes with node two node filters")
 	r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{
@@ -333,7 +335,7 @@ func TestListNodesWithLabelFilter(t *testing.T) {
 	// should only return the first node
 	assert.NoError(t, err)
 	assert.Len(t, r.Nodes, 1)
-	assert.Contains(t, r.Nodes, nodes[0])
+	assert.Equal(t, r.Nodes[0].ID, nodes[0].ID)
 
 	t.Log("list nodes with both engine and node filters")
 	r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{
@@ -350,8 +352,8 @@ func TestListNodesWithLabelFilter(t *testing.T) {
 	})
 	assert.NoError(t, err)
 	assert.Len(t, r.Nodes, 2)
-	assert.Contains(t, r.Nodes, nodes[0])
-	assert.Contains(t, r.Nodes, nodes[2])
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[0].ID)
+	assert.Contains(t, []string{r.Nodes[0].ID, r.Nodes[1].ID}, nodes[2].ID)
 }
 
 func TestRemoveNodes(t *testing.T) {

manager/controlapi/server_test.go

@@ -13,15 +13,17 @@ import (
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/ca"
 	cautils "github.com/docker/swarmkit/ca/testutils"
+	"github.com/docker/swarmkit/manager/allocator"
 	"github.com/docker/swarmkit/manager/state/store"
 	stateutils "github.com/docker/swarmkit/manager/state/testutils"
 	"github.com/stretchr/testify/assert"
 )
 
 type testServer struct {
-	Server *Server
-	Client api.ControlClient
-	Store  *store.MemoryStore
+	Server    *Server
+	Client    api.ControlClient
+	Store     *store.MemoryStore
+	Allocator *allocator.Allocator
 
 	grpcServer *grpc.Server
 	clientConn *grpc.ClientConn
@@ -33,6 +35,7 @@ func (ts *testServer) Stop() {
 	ts.clientConn.Close()
 	ts.grpcServer.Stop()
 	ts.Store.Close()
+	ts.Allocator.Stop()
 	os.RemoveAll(ts.tempUnixSocket)
 }
 
@@ -48,6 +51,13 @@ func newTestServer(t *testing.T) *testServer {
 	ts.Store = store.NewMemoryStore(&stateutils.MockProposer{})
 	assert.NotNil(t, ts.Store)
 
+	ts.Allocator, err = allocator.New(ts.Store, nil, nil)
+	assert.NoError(t, err)
+	assert.NotNil(t, ts.Allocator)
+	go func() {
+		assert.NoError(t, ts.Allocator.Run(context.Background()))
+	}()
+
 	ts.Server = NewServer(ts.Store, nil, securityConfig, nil, nil)
 	assert.NotNil(t, ts.Server)