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

Jump to bottom

manager/allocator/cnmallocator: remove uses of deprecated Init() funcs #3138

Closed
wants to merge 4 commits into from
Closed

manager/allocator/cnmallocator: remove uses of deprecated Init() funcs #3138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
470f0b5
cnmallocator: RegisterManager does not require DriverCallback interface
thaJeztah Jun 30, 2023
d98f63b
manager/allocator/cnmallocator: use a map for initializers
thaJeztah Jun 30, 2023
df35053
manager/allocator/cnmallocator: remove uses of deprecated Init() funcs
thaJeztah Jun 30, 2023
cecae4a
manager/allocator/cnmallocator: StubManagerInit: config arg is unused
thaJeztah Jun 30, 2023
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
25 changes: 25 additions & 0 deletions manager/allocator/cnmallocator/drivers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package cnmallocator

import (
"fmt"

"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/drivers/remote"
)

type driverRegisterFn func(r driverapi.Registerer, config map[string]interface{}) error

func registerRemote(r driverapi.Registerer, _ map[string]interface{}) error {
dc, ok := r.(driverapi.DriverCallback)
if !ok {
return fmt.Errorf(`failed to register "remote" driver: driver does not implement driverapi.DriverCallback`)
}
return remote.Register(dc, dc.GetPluginGetter())
}
Comment on lines +12 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

Please please please just special-case the registration of the remote driver so that Swarmkit can stop depending on the deprecated drvregistry.DrvRegistry. And I'd really like to get rid of the DriverCallback interface in libnetwork as well (which I overlooked annotating as deprecated, whoops)

Copy link
Member Author

Choose a reason for hiding this comment

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

so that Swarmkit can stop depending on the deprecated drvregistry.DrvRegistry.

I need to check that last one; while working on #3139 (reviews welcome on that one as well 😅), I found that the plugingetter field is not exported, but looks to be used, so the only way to set it is through the (deprecated) drvregistry.New

I may need to have a deeper look "how" it's used, and if there's other ways to pass it around though.

Copy link
Contributor

Choose a reason for hiding this comment

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

plugingetter field? cnmallocator.New takes the PluginGetter as an argument, constructs a DrvRegistry using it, and passes it to each the initializers in the slice/map. Simply cut out the middleman by passing the PluginGetter directly into a call to remote.Register from cnmallocator.New. That's what I meant by special-casing the registration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Gotcha (I think) I'll have another look tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that the DrvRegistry.pluginGetter field is used. It serves one purpose: implementing the GetPluginGetter() method, which is only used to indirectly pass the PluginGetter into remote.Init(). As I explained in the commit message of moby/moby@5595311, I very deliberately omitted any PluginGetter stuff from the replacement types.


//nolint:unused // is currently only used on Windows, but keeping these adaptors together in one file.
func registerNetworkType(networkType string) func(dc driverapi.Registerer, config map[string]interface{}) error {
return func(r driverapi.Registerer, _ map[string]interface{}) error {
return RegisterManager(r, networkType)
}
}
7 changes: 3 additions & 4 deletions manager/allocator/cnmallocator/drivers_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ package cnmallocator

import (
"github.com/docker/docker/libnetwork/drivers/overlay/ovmanager"
"github.com/docker/docker/libnetwork/drivers/remote"
"github.com/moby/swarmkit/v2/manager/allocator/networkallocator"
)

var initializers = []initializer{
{remote.Init, "remote"},
{ovmanager.Init, "overlay"},
var initializers = map[string]driverRegisterFn{
"remote": registerRemote,
"overlay": ovmanager.Register,
}

// PredefinedNetworks returns the list of predefined network structures
Expand Down
15 changes: 7 additions & 8 deletions manager/allocator/cnmallocator/drivers_network_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,16 @@ import (
"github.com/docker/docker/libnetwork/drivers/ipvlan/ivmanager"
"github.com/docker/docker/libnetwork/drivers/macvlan/mvmanager"
"github.com/docker/docker/libnetwork/drivers/overlay/ovmanager"
"github.com/docker/docker/libnetwork/drivers/remote"
"github.com/moby/swarmkit/v2/manager/allocator/networkallocator"
)

var initializers = []initializer{
{remote.Init, "remote"},
{ovmanager.Init, "overlay"},
{mvmanager.Init, "macvlan"},
{brmanager.Init, "bridge"},
{ivmanager.Init, "ipvlan"},
{host.Init, "host"},
var initializers = map[string]driverRegisterFn{
"remote": registerRemote,
"overlay": ovmanager.Register,
"macvlan": mvmanager.Register,
"bridge": brmanager.Register,
"ipvlan": ivmanager.Register,
"host": host.Register,
}

// PredefinedNetworks returns the list of predefined network structures
Expand Down
13 changes: 6 additions & 7 deletions manager/allocator/cnmallocator/drivers_network_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@ package cnmallocator

import (
"github.com/docker/docker/libnetwork/drivers/overlay/ovmanager"
"github.com/docker/docker/libnetwork/drivers/remote"
"github.com/moby/swarmkit/v2/manager/allocator/networkallocator"
)

var initializers = []initializer{
{remote.Init, "remote"},
{ovmanager.Init, "overlay"},
{StubManagerInit("internal"), "internal"},
{StubManagerInit("l2bridge"), "l2bridge"},
{StubManagerInit("nat"), "nat"},
var initializers = map[string]driverRegisterFn{
"remote": registerRemote,
"overlay": ovmanager.Register,
"internal": registerNetworkType("internal"),
"l2bridge": registerNetworkType("l2bridge"),
"nat": registerNetworkType("nat"),
}

// PredefinedNetworks returns the list of predefined network structures
Expand Down
13 changes: 6 additions & 7 deletions manager/allocator/cnmallocator/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,18 @@ type manager struct {
networkType string
}

func StubManagerInit(networkType string) func(dc driverapi.DriverCallback, config map[string]interface{}) error {
return func(dc driverapi.DriverCallback, config map[string]interface{}) error {
func StubManagerInit(networkType string) func(dc driverapi.DriverCallback, _ map[string]interface{}) error {
return func(dc driverapi.DriverCallback, _ map[string]interface{}) error {
return RegisterManager(dc, networkType)
}
}
Comment on lines +14 to 18
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this now dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

DOH! I think it is now yes; originally I had to adjust the DriverCallback, but that's no longer true, so yup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that PR was closed though, but I wasn't sure if it was still needed (to be revived); do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that PR was closed though, but I wasn't sure if it was still needed (to be revived); do you know?

It is not needed.


// Register registers a new instance of the manager driver for networkType with r.
func RegisterManager(r driverapi.DriverCallback, networkType string) error {
c := driverapi.Capability{
// RegisterManager registers a new instance of the manager driver for networkType with r.
func RegisterManager(r driverapi.Registerer, networkType string) error {
return r.RegisterDriver(networkType, &manager{networkType: networkType}, driverapi.Capability{
DataScope: datastore.LocalScope,
ConnectivityScope: datastore.LocalScope,
}
return r.RegisterDriver(networkType, &manager{networkType: networkType}, c)
})
}

func (d *manager) NetworkAllocate(id string, option map[string]string, ipV4Data, ipV6Data []driverapi.IPAMData) (map[string]string, error) {
Expand Down
29 changes: 6 additions & 23 deletions manager/allocator/cnmallocator/networkallocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,6 @@ type networkDriver struct {
capability *driverapi.Capability
}

type initializer struct {
fn drvregistry.InitFunc
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
Expand Down Expand Up @@ -115,8 +110,10 @@ func New(pg plugingetter.PluginGetter, netConfig *NetworkConfig) (networkallocat
return nil, err
}

if err := initializeDrivers(reg); err != nil {
return nil, err
for name, register := range initializers {
if err := register(reg, nil); err != nil {
return nil, fmt.Errorf("failed to register %q driver: %w", name, err)
}
}

if err = initIPAMDrivers(reg, netConfig); err != nil {
Expand Down Expand Up @@ -978,15 +975,6 @@ func (na *cnmNetworkAllocator) allocatePools(n *api.Network) (map[string]string,
return pools, nil
}

func initializeDrivers(reg *drvregistry.DrvRegistry) error {
for _, i := range initializers {
if err := reg.AddDriver(i.ntype, i.fn, nil); err != nil {
return err
}
}
return nil
}

func serviceNetworks(s *api.Service) []*api.NetworkAttachmentConfig {
// Always prefer NetworkAttachmentConfig in the TaskSpec
if len(s.Spec.Task.Networks) == 0 && len(s.Spec.Networks) != 0 {
Expand All @@ -1010,13 +998,8 @@ func (na *cnmNetworkAllocator) IsVIPOnIngressNetwork(vip *api.Endpoint_VirtualIP

// IsBuiltInDriver returns whether the passed driver is an internal network driver
func IsBuiltInDriver(name string) bool {
n := strings.ToLower(name)
for _, d := range initializers {
if n == d.ntype {
return true
}
}
return false
_, ok := initializers[strings.ToLower(name)]
return ok
}

// setIPAMSerialAlloc sets the ipam allocation method to serial
Expand Down
Loading