Default Address Pool Code Refactor

 We have two params that have been passed around as part of swarm init
config. As part of refactoring the code, we created NetworkConfig struct
and added address pool and subnet mask length as part of the new struct
Older PR review comment has been addressed in this commit.
Signed-off-by: selansen <elango.siva@docker.com>

manager/allocator/allocator.go

@@ -1,7 +1,6 @@
 package allocator
 
 import (
-	"net"
 	"sync"
 
 	"github.com/docker/docker/pkg/plugingetter"
@@ -34,12 +33,8 @@ type Allocator struct {
 	// pluginGetter provides access to docker's plugin inventory.
 	pluginGetter plugingetter.PluginGetter
 
-	// DefaultAddrPool specifies default subnet pool for global scope networks
-	defaultAddrPool []*net.IPNet
-
-	// SubnetSize specifies the subnet size of the networks created from
-	// the default subnet pool
-	subnetSize int
+	// networkConfig stores network related config for the cluster
+	networkConfig *NetworkConfig
 }
 
 // taskBallot controls how the voting for task allocation is
@@ -73,17 +68,16 @@ type allocActor struct {
 
 // New returns a new instance of Allocator for use during allocation
 // stage of the manager.
-func New(store *store.MemoryStore, pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize int) (*Allocator, error) {
+func New(store *store.MemoryStore, pg plugingetter.PluginGetter, netConfig *NetworkConfig) (*Allocator, error) {
 	a := &Allocator{
 		store: store,
 		taskBallot: &taskBallot{
 			votes: make(map[string][]string),
 		},
-		stopChan:        make(chan struct{}),
-		doneChan:        make(chan struct{}),
-		pluginGetter:    pg,
-		defaultAddrPool: defaultAddrPool,
-		subnetSize:      subnetSize,
+		stopChan:      make(chan struct{}),
+		doneChan:      make(chan struct{}),
+		pluginGetter:  pg,
+		networkConfig: netConfig,
 	}
 
 	return a, nil

manager/allocator/allocator_linux_test.go

@@ -16,7 +16,7 @@ func TestIPAMNotNil(t *testing.T) {
 	assert.NotNil(t, s)
 	defer s.Close()
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 

manager/allocator/allocator_test.go

@@ -27,7 +27,7 @@ func TestAllocator(t *testing.T) {
 	assert.NotNil(t, s)
 	defer s.Close()
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 
@@ -668,7 +668,7 @@ func TestNoDuplicateIPs(t *testing.T) {
 
 			return nil
 		}))
-		a, err := New(s, nil, nil, 0)
+		a, err := New(s, nil, nil)
 		assert.NoError(t, err)
 		assert.NotNil(t, a)
 
@@ -800,7 +800,7 @@ func TestAllocatorRestoreForDuplicateIPs(t *testing.T) {
 		return true
 	}
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 	// Start allocator
@@ -950,7 +950,7 @@ func TestAllocatorRestartNoEndpointSpec(t *testing.T) {
 		return true
 	}
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 	// Start allocator
@@ -1154,7 +1154,7 @@ func TestAllocatorRestoreForUnallocatedNetwork(t *testing.T) {
 		return true
 	}
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 	// Start allocator
@@ -1181,7 +1181,7 @@ func TestNodeAllocator(t *testing.T) {
 	assert.NotNil(t, s)
 	defer s.Close()
 
-	a, err := New(s, nil, nil, 0)
+	a, err := New(s, nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, a)
 

manager/allocator/cnmallocator/drivers_ipam.go

@@ -1,7 +1,6 @@
 package cnmallocator
 
 import (
-	"net"
 	"strconv"
 	"strings"
 
@@ -14,24 +13,24 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-func initIPAMDrivers(r *drvregistry.DrvRegistry, defaultAddrPool []*net.IPNet, subnetSize int) error {
+func initIPAMDrivers(r *drvregistry.DrvRegistry, netConfig *NetworkConfig) error {
 	var addressPool []*ipamutils.NetworkToSplit
 	var str strings.Builder
 	str.WriteString("Subnetlist - ")
 	// Extract defaultAddrPool param info and construct ipamutils.NetworkToSplit
 	// from the info. We will be using it to call Libnetwork API
 	// We also need to log new address pool info whenever swarm init
 	// happens with default address pool option
-	if defaultAddrPool != nil {
-		for _, p := range defaultAddrPool {
+	if netConfig != nil {
+		for _, p := range netConfig.DefaultAddrPool {
 			addressPool = append(addressPool, &ipamutils.NetworkToSplit{
-				Base: p.String(),
-				Size: subnetSize,
+				Base: p,
+				Size: int(netConfig.SubnetSize),
 			})
-			str.WriteString(p.String() + ",")
+			str.WriteString(p + ",")
 		}
 		str.WriteString(": Size ")
-		str.WriteString(strconv.Itoa(subnetSize))
+		str.WriteString(strconv.Itoa(int(netConfig.SubnetSize)))
 	}
 	if err := ipamutils.ConfigGlobalScopeDefaultNetworks(addressPool); err != nil {
 		return err

manager/allocator/cnmallocator/networkallocator.go

@@ -86,8 +86,18 @@ type initializer struct {
 	ntype string
 }
 
+// NetworkConfig is used to store network related cluster config in the Manager.
+type NetworkConfig struct {
+	// DefaultAddrPool specifies default subnet pool for global scope networks
+	DefaultAddrPool []string
+
+	// SubnetSize specifies the subnet size of the networks created from
+	// the default subnet pool
+	SubnetSize uint32
+}
+
 // New returns a new NetworkAllocator handle
-func New(pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize int) (networkallocator.NetworkAllocator, error) {
+func New(pg plugingetter.PluginGetter, netConfig *NetworkConfig) (networkallocator.NetworkAllocator, error) {
 	na := &cnmNetworkAllocator{
 		networks: make(map[string]*network),
 		services: make(map[string]struct{}),
@@ -106,7 +116,7 @@ func New(pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize
 		return nil, err
 	}
 
-	if err = initIPAMDrivers(reg, defaultAddrPool, subnetSize); err != nil {
+	if err = initIPAMDrivers(reg, netConfig); err != nil {
 		return nil, err
 	}
 

manager/allocator/cnmallocator/networkallocator_test.go

@@ -13,7 +13,7 @@ import (
 )
 
 func newNetworkAllocator(t *testing.T) networkallocator.NetworkAllocator {
-	na, err := New(nil, nil, 0)
+	na, err := New(nil, nil)
 	assert.NoError(t, err)
 	assert.NotNil(t, na)
 	return na

manager/allocator/network.go

@@ -67,8 +67,22 @@ type networkContext struct {
 	somethingWasDeallocated bool
 }
 
+// NetworkConfig is used to store network related cluster config in the Manager.
+type NetworkConfig struct {
+	// DefaultAddrPool specifies default subnet pool for global scope networks
+	DefaultAddrPool []string
+
+	// SubnetSize specifies the subnet size of the networks created from
+	// the default subnet pool
+	SubnetSize uint32
+}
+
 func (a *Allocator) doNetworkInit(ctx context.Context) (err error) {
-	na, err := cnmallocator.New(a.pluginGetter, a.defaultAddrPool, a.subnetSize)
+	netConfig := &cnmallocator.NetworkConfig{
+		DefaultAddrPool: a.networkConfig.DefaultAddrPool,
+		SubnetSize:      a.networkConfig.SubnetSize,
+	}
+	na, err := cnmallocator.New(a.pluginGetter, netConfig)
 	if err != nil {
 		return err
 	}

manager/manager.go

@@ -127,12 +127,8 @@ type Config struct {
 	// FIPS setting.
 	FIPS bool
 
-	// DefaultAddrPool specifies default subnet pool for global scope networks
-	DefaultAddrPool []*net.IPNet
-
-	// SubnetSize specifies the subnet size of the networks created from
-	// the default subnet pool
-	SubnetSize int
+	// NetworkConfig stores network related config for the cluster
+	NetworkConfig *allocator.NetworkConfig
 }
 
 // Manager is the cluster manager for Swarm.
@@ -947,14 +943,10 @@ func (m *Manager) becomeLeader(ctx context.Context) {
 			nil,
 			0)
 
-		var defaultAddrPool []string
-		for _, p := range m.config.DefaultAddrPool {
-			defaultAddrPool = append(defaultAddrPool, p.String())
-		}
 		// If defaultAddrPool is valid we update cluster object with new value
-		if defaultAddrPool != nil {
-			clusterObj.DefaultAddressPool = defaultAddrPool
-			clusterObj.SubnetSize = uint32(m.config.SubnetSize)
+		if m.config.NetworkConfig != nil && m.config.NetworkConfig.DefaultAddrPool != nil {
+			clusterObj.DefaultAddressPool = m.config.NetworkConfig.DefaultAddrPool
+			clusterObj.SubnetSize = m.config.NetworkConfig.SubnetSize
 		}
 
 		err := store.CreateCluster(tx, clusterObj)
@@ -1003,24 +995,20 @@ func (m *Manager) becomeLeader(ctx context.Context) {
 
 	// If DefaultAddrPool is null, Read from store and check if
 	// DefaultAddrPool info is stored in cluster object
-	if m.config.DefaultAddrPool == nil {
+	if m.config.NetworkConfig == nil {
 		var cluster *api.Cluster
 		s.View(func(tx store.ReadTx) {
 			cluster = store.GetCluster(tx, clusterID)
 		})
 		if cluster.DefaultAddressPool != nil {
 			for _, address := range cluster.DefaultAddressPool {
-				_, b, err := net.ParseCIDR(address)
-				if err != nil {
-					log.G(ctx).WithError(err).Error("Default Address Pool reading failed for cluster object  %s", address)
-				}
-				m.config.DefaultAddrPool = append(m.config.DefaultAddrPool, b)
+				m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, address)
 			}
+			m.config.NetworkConfig.SubnetSize = cluster.SubnetSize
 		}
-		m.config.SubnetSize = int(cluster.SubnetSize)
 	}
 
-	m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.DefaultAddrPool, m.config.SubnetSize)
+	m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.NetworkConfig)
 	if err != nil {
 		log.G(ctx).WithError(err).Error("failed to create allocator")
 		// TODO(stevvooe): It doesn't seem correct here to fail
@@ -1144,7 +1132,7 @@ func defaultClusterObject(
 	rootCA *ca.RootCA,
 	fips bool,
 	defaultAddressPool []string,
-	subnetSize int) *api.Cluster {
+	subnetSize uint32) *api.Cluster {
 	var caKey []byte
 	if rcaSigner, err := rootCA.Signer(); err == nil {
 		caKey = rcaSigner.Key
@@ -1178,7 +1166,7 @@ func defaultClusterObject(
 		UnlockKeys:         initialUnlockKeys,
 		FIPS:               fips,
 		DefaultAddressPool: defaultAddressPool,
-		SubnetSize:         uint32(subnetSize),
+		SubnetSize:         subnetSize,
 	}
 }
 

node/node.go

@@ -28,6 +28,7 @@ import (
 	"github.com/docker/swarmkit/ioutils"
 	"github.com/docker/swarmkit/log"
 	"github.com/docker/swarmkit/manager"
+	"github.com/docker/swarmkit/manager/allocator"
 	"github.com/docker/swarmkit/manager/encryption"
 	"github.com/docker/swarmkit/remotes"
 	"github.com/docker/swarmkit/xnet"
@@ -105,12 +106,8 @@ type Config struct {
 	// for connections to the remote API (including the raft service).
 	AdvertiseRemoteAPI string
 
-	// DefaultAddrPool specifies default subnet pool for global scope networks
-	DefaultAddrPool []*net.IPNet
-
-	// SubnetSize specifies the subnet size of the networks created from
-	// the default subnet pool
-	SubnetSize int
+	// NetworkConfig stores network related config for the cluster
+	NetworkConfig *allocator.NetworkConfig
 
 	// Executor specifies the executor to use for the agent.
 	Executor exec.Executor
@@ -1002,8 +999,7 @@ func (n *Node) runManager(ctx context.Context, securityConfig *ca.SecurityConfig
 		PluginGetter:     n.config.PluginGetter,
 		RootCAPaths:      rootPaths,
 		FIPS:             n.config.FIPS,
-		DefaultAddrPool:  n.config.DefaultAddrPool,
-		SubnetSize:       n.config.SubnetSize,
+		NetworkConfig:    n.config.NetworkConfig,
 	})
 	if err != nil {
 		return false, err