Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove inappropriate allocator import #2855

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove inappropriate allocator import #2855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Remove inappropriate allocator import 
Removes inappropriate import of manager/allocator/cnmallocator into
node/node.go. The consequence of this is that in upstream moby/moby, we
can remove that import as well. manager/allocator/cnmallocator is an
implementation detail, and importing it so high in the stack creates an
unacceptably high degree of coupling.

Signed-off-by: Drew Erny <drew.erny@docker.com>
  • Loading branch information
@dperny
dperny committed Feb 17, 2023
commit dceb99741fd45349b32d489a3a8c6169e192f8a5
49 changes: 28 additions & 21 deletions manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,15 @@ type Config struct {
// FIPS setting.
FIPS bool

// NetworkConfig stores network related config for the cluster
NetworkConfig *cnmallocator.NetworkConfig
// DefaultAddrPool specifies default subnet pool for global scope networks
DefaultAddrPool []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, a pointer was used; are there situations where these could be nil (and for a reason?)

Actually wondering; should we just copy the NetworkConfig type here, and use that instead of defining these fields both here, and in node/node.go ?


// SubnetSize specifies the subnet size of the networks created from
// the default subnet pool
SubnetSize uint32

// VXLANUDPPort specifies the UDP port number for VXLAN traffic
VXLANUDPPort uint32
}

// Manager is the cluster manager for Swarm.
Expand Down Expand Up @@ -961,15 +968,13 @@ func (m *Manager) becomeLeader(ctx context.Context) {

// If defaultAddrPool is valid we update cluster object with new value
// If VXLANUDPPort is not 0 then we call update cluster object with new value
if m.config.NetworkConfig != nil {
if m.config.NetworkConfig.DefaultAddrPool != nil {
clusterObj.DefaultAddressPool = m.config.NetworkConfig.DefaultAddrPool
clusterObj.SubnetSize = m.config.NetworkConfig.SubnetSize
}
if m.config.DefaultAddrPool != nil {
clusterObj.DefaultAddressPool = m.config.DefaultAddrPool
clusterObj.SubnetSize = m.config.SubnetSize
}

if m.config.NetworkConfig.VXLANUDPPort != 0 {
clusterObj.VXLANUDPPort = m.config.NetworkConfig.VXLANUDPPort
}
if m.config.VXLANUDPPort != 0 {
clusterObj.VXLANUDPPort = m.config.VXLANUDPPort
}
err := store.CreateCluster(tx, clusterObj)

Expand Down Expand Up @@ -1021,27 +1026,29 @@ 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 VXLANUDPPort is 0, read it from the store - cluster object
if m.config.NetworkConfig == nil || m.config.NetworkConfig.DefaultAddrPool == nil || m.config.NetworkConfig.VXLANUDPPort == 0 {
if m.config.DefaultAddrPool == nil || m.config.VXLANUDPPort == 0 {
var cluster *api.Cluster
s.View(func(tx store.ReadTx) {
cluster = store.GetCluster(tx, clusterID)
})
if cluster.DefaultAddressPool != nil {
if m.config.NetworkConfig == nil {
m.config.NetworkConfig = &cnmallocator.NetworkConfig{}
}
m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, cluster.DefaultAddressPool...)
m.config.NetworkConfig.SubnetSize = cluster.SubnetSize
m.config.DefaultAddrPool = append(m.config.DefaultAddrPool, cluster.DefaultAddressPool...)
m.config.SubnetSize = cluster.SubnetSize
}
if cluster.VXLANUDPPort != 0 {
if m.config.NetworkConfig == nil {
m.config.NetworkConfig = &cnmallocator.NetworkConfig{}
}
m.config.NetworkConfig.VXLANUDPPort = cluster.VXLANUDPPort
m.config.VXLANUDPPort = cluster.VXLANUDPPort
}
}

m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.NetworkConfig)
// Toss all this info into a netCfg, so that the allocator can do its
// thing.
netCfg := &cnmallocator.NetworkConfig{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you creating a new config instead of using m.config.NetworkConfig?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ @dperny good point; especially in light of;

The long-pending long-awaited allocator rewrite on the new-allocator branch removes cnmallocator, and doing this refactoring in an earlier commit will simplify that operation greatly.

DefaultAddrPool: m.config.DefaultAddrPool,
SubnetSize: m.config.SubnetSize,
VXLANUDPPort: m.config.VXLANUDPPort,
}

m.allocator, err = allocator.New(s, m.config.PluginGetter, netCfg)
if err != nil {
log.G(ctx).WithError(err).Error("failed to create allocator")
// TODO(stevvooe): It doesn't seem correct here to fail
Expand Down
20 changes: 16 additions & 4 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/moby/swarmkit/v2/ioutils"
"github.com/moby/swarmkit/v2/log"
"github.com/moby/swarmkit/v2/manager"
"github.com/moby/swarmkit/v2/manager/allocator/cnmallocator"
"github.com/moby/swarmkit/v2/manager/encryption"
"github.com/moby/swarmkit/v2/remotes"
"github.com/moby/swarmkit/v2/xnet"
Expand Down Expand Up @@ -106,8 +105,19 @@ type Config struct {
// for connections to the remote API (including the raft service).
AdvertiseRemoteAPI string

// NetworkConfig stores network related config for the cluster
NetworkConfig *cnmallocator.NetworkConfig
// DefaultAddrPool specifies default subnet pool for global scope networks
// This duplicates information found in a type deeper in the stack
// (manager/allocation/cnmallocator.NetworkConfig) but is brought up to
// the top level in this config file to avoid creating a dependency from
// this node package to any deeper levels.
DefaultAddrPool []string

// SubnetSize specifies the subnet size of the networks created from
// the default subnet pool
SubnetSize uint32

// VXLANUDPPort specifies the UDP port number for VXLAN traffic
VXLANUDPPort uint32

// Executor specifies the executor to use for the agent.
Executor exec.Executor
Expand Down Expand Up @@ -1014,7 +1024,9 @@ func (n *Node) runManager(ctx context.Context, securityConfig *ca.SecurityConfig
PluginGetter: n.config.PluginGetter,
RootCAPaths: rootPaths,
FIPS: n.config.FIPS,
NetworkConfig: n.config.NetworkConfig,
DefaultAddrPool: n.config.DefaultAddrPool,
SubnetSize: n.config.SubnetSize,
VXLANUDPPort: n.config.VXLANUDPPort,
})
if err != nil {
return false, err
Expand Down