manager/allocator/cnmallocator: remove uses of deprecated Init() funcs #3138
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3138 +/- ##
==========================================
+ Coverage 61.58% 61.60% +0.02%
==========================================
Files 154 155 +1
Lines 31106 31116 +10
==========================================
+ Hits 19156 19169 +13
+ Misses 10402 10397 -5
- Partials 1548 1550 +2 |
thaJeztah
force-pushed
the
refactor_initializers
branch
4 times, most recently
from
96b9c3d
to
e59e2fe
Compare
This function was not using the DriverCallback interface, and only required the Registerer interface. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
refactor_initializers
branch
from
e59e2fe
to
4834c8e
Compare
There was a problem hiding this comment.
drvregistry.New is also deprecated and its use should be removed at the same time as the uses of the deprecated Init functions which depend on it.
Given that the majority of the drivers being registered are managers which are only referenced by Swarmkit, and that Swarmkit always passes nil for the config argument, I propose we change the signatures of the managers' Register functions to
func Register(driverapi.Registerer) errorand deal with host.Register by either special-casing it in Swarmkit, using an adapter, or changing its signature in libnetwork to the above (it takes no configuration) and dealing with the change on the libnetwork side.
| if err := initializeDrivers(reg); err != nil { | ||
| return nil, err | ||
| for _, init := range initializers { | ||
| if err := init(reg, nil); err != nil { |
There was a problem hiding this comment.
The driver config is always nil?! And the majority of initializers are for "manager" drivers which are exclusively used by swarmkit... I wish I had checked before refactoring libnetwork and used a different signature for those Register functions. It's not too late yet to fix my past mistake!
There was a problem hiding this comment.
There was a problem hiding this comment.
The driver config is always nil?!
Hm... good one, I have to double check if there's a driver I missed that still consumes a config, but at least most of them were not using it, hence me opening;
I wish I had checked before refactoring libnetwork and used a different signature for those Register functions. It's not too late yet to fix my past mistake!
Don't blame yourself! There's so much indirection, and so much "oh, let's add some options, just in case, someday, maybe we'll use them".
I started to chip away more bits in moby/moby#45870, and some of those changes may become obsolete if we carve out larger bits, but it was in an attempt to get clearer what's actually used.
| 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()) | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah! Gotcha (I think) I'll have another look tomorrow
There was a problem hiding this comment.
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.
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
DOH! I think it is now yes; originally I had to adjust the DriverCallback, but that's no longer true, so yup.
There was a problem hiding this comment.
- Oh; looking at it locally now; yes it's unused, but it was implemented in SwarmKit to (as I understand it) provide functionality to be used in libnetwork: Add managers for Windows drivers moby#44983 (comment)
I see that PR was closed though, but I wasn't sure if it was still needed (to be revived); do you know?
There was a problem hiding this comment.
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.
Refactor the initializers slice to be a map (as driver-names should be unique, and they were not dynamically set here either way). Also inlining the code from initializeDrivers(), as it was only used in a single location, and the code lived far from where it was used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These functions were deprecated in moby/moby@28edc8e in favor of the Register functions. Unfortunately, not all Register functions have the same signature, so some (temporary) adaptor code was needed for these. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make it clearer that the config argument is not used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
refactor_initializers
branch
from
4834c8e
to
cecae4a
Compare
|
superseded by #3143 |
cnmallocator: RegisterManager does not require DriverCallback interface
This function was not using the DriverCallback interface, and only
required the Registerer interface.
manager/allocator/cnmallocator: use a map for initializers
Refactor the initializers slice to be a map (as driver-names should be
unique, and they were not dynamically set here either way).
Also inlining the code from initializeDrivers(), as it was only used
in a single location, and the code lived far from where it was used.
manager/allocator/cnmallocator: remove uses of deprecated Init() funcs
These functions were deprecated in moby/moby@28edc8e (moby/moby#44875)
in favor of the Register functions.
Unfortunately, not all Register functions have the same signature, so
some (temporary) adaptor code was needed for these.
manager/allocator/cnmallocator: StubManagerInit: config arg is unused
Make it clearer that the config argument is not used.
- What I did
- How I did it
- How to test it
- Description for the changelog