libnetwork: clean up vestigial datastore-related code by corhere · Pull Request #44875 · moby/moby

corhere · 2023-01-26T22:55:45Z

- What I did
I cleaned up code related to datastores and their configuration across libnetwork, pruning dead code and refactoring what remained. Highlights:

Refactored package drvregistry to no longer be an ad-hoc Dependency Injection container for network and IPAM drivers, and created separate types for network driver registries and IPAM driver registries
- The PluginGetter dependency is now injected directly into the remote drivers
- Removed the vestiges of datastore dependency injection
Removed DatastoreConfig from discoverapi as dynamic datastore discovery is unreachable
Threw out much of the dead persistence code from package ipam

- How I did it
Prove that code is unreachable. Delete the dead code. Repeat.

- How to verify it
CI.

- Description for the changelog
N/A

- A picture of a cute animal (not mandatory but encouraged)

...as it is unused. Signed-off-by: Cory Snider <csnider@mirantis.com>

Without (*Controller).ReloadConfiguration, the only way to configure datastore scopes would be by passing config.Options to libnetwork.New. The only options defined which relate to datastore scopes are limited to configuring the local-scope datastore. Furthermore, the default datastore config only defines configuration for the local-scope datastore. The local-scope datastore is therefore the only datastore scope possible in libnetwork. Start removing code which is only needed to support multiple datastore scopes. Signed-off-by: Cory Snider <csnider@mirantis.com>

Signed-off-by: Cory Snider <csnider@mirantis.com>

cpuguy83 · 2023-01-26T23:26:49Z

libnetwork/driverapi/driverapi.go

+
 // DriverCallback provides a Callback interface for Drivers into LibNetwork
 type DriverCallback interface {
+	Registerer


lol, this type reminds of https://www.youtube.com/watch?v=6kZBJs527-k
"The Rural Juror"

libnetwork/ipams/remote/remote.go

neersighted

I noticed one missing comma 😆

Otherwise, everything seems well-explained, rational, and well-divided into commits. Awesome work, and it's nice to see that (dead) persistence code just fall away.

libnetwork/drvregistry/ipams.go

tianon

"Registerer" is a little bit awkward and there are a few trailing newlines at EOfunc, but nothing that I'd personally block this for 😅

libnetwork/drvregistry/networks_test.go

libnetwork/drivers/ipvlan/ivmanager/ivmanager.go

The datastore arguments to the IPAM driver Init() functions are always nil, even in Swarmkit. The only IPAM driver which consumed the datastores was builtin; all others (null, remote, windowsipam) simply ignored it. As the signatures of the IPAM driver init functions cannot be changed without breaking the Swarmkit build, they have to be left with the same signatures for the time being. Assert that nil datastores are always passed into the builtin IPAM driver's init function so that there is no ambiguity the datastores are no longer respected. Add new Register functions for the IPAM drivers which are free from the legacy baggage of the Init functions. (The legacy Init functions can be removed once Swarmkit is migrated to using the Register functions.) As the remote IPAM driver is the only one which depends on a PluginGetter, pass it in explicitly as an argument to Register. The other IPAM drivers should not be forced to depend on a GetPluginGetter() method they do not use (Interface Segregation Principle). Signed-off-by: Cory Snider <csnider@mirantis.com>

There is no benefit to having a single registry for both IPAM drivers and network drivers. IPAM drivers are registered in a separate namespace from network drivers, have separate registration methods, separate accessor methods and do not interact with network drivers within a DrvRegistry in any way. The only point of commonality is interface { GetPluginGetter() plugingetter.PluginGetter } which is only used by the respective remote drivers and therefore should be outside of the scope of a driver registry. Create new, separate registry types for network drivers and IPAM drivers, respectively. These types are "legacy-free". Neither type has GetPluginGetter methods. The IPAMs registry does not have an IPAMDefaultAddressSpaces method as that information can be queried directly from the driver using its GetDefaultAddressSpaces method. The Networks registry does not have an AddDriver method as that method is a trivial wrapper around calling one of its arguments with its other arguments. Refactor DrvRegistry in terms of the new IPAMs and Networks registries so that existing code in libnetwork and Swarmkit will continue to work. Signed-off-by: Cory Snider <csnider@mirantis.com>

Per the Interface Segregation Principle, network drivers should not have to depend on GetPluginGetter methods they do not use. The remote network driver is the only one which needs a PluginGetter, and it is already special-cased in Controller so there is no sense warping the interfaces to achieve a foolish consistency. Replace all other network drivers' Init functions with Register functions which take a driverapi.Registerer argument instead of a driverapi.DriverCallback. Add back in Init wrapper functions for only the drivers which Swarmkit references so that Swarmkit can continue to build. Refactor the libnetwork Controller to use the new drvregistry.Networks and drvregistry.IPAMs driver registries in place of the legacy ones. Signed-off-by: Cory Snider <csnider@mirantis.com>

The DatastoreConfig discovery type is unused. Remove the constant and any resulting dead code. Today's biggest loser is the IPAM Allocator: DatastoreConfig was the only type of discovery event it was listening for, and there was no other place where a non-nil datastore could be passed into the allocator. Strip out all the dead persistence code from Allocator, leaving it as purely an in-memory implementation. There is no more need to check the consistency of the allocator's bit-sequences as there is no persistent storage for inconsistent bit sequences to be loaded from. Signed-off-by: Cory Snider <csnider@mirantis.com>

That method was only referenced by ipam.Allocator, but as it no longer stores any state persistently there is no possibility for it to load an inconsistent bit-sequence from Docker 1.9.x. Signed-off-by: Cory Snider <csnider@mirantis.com>

The (*bitseq.Handle).Destroy() method deletes the persisted KVObject from the datastore. This is a no-op on all the bitseq handles in package ipam as they are not persisted in any datastore. Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere · 2023-01-27T17:20:10Z

"Registerer" is a little bit awkward

Nobody's going to confuse that for a concrete type! Also:

- LocalKVProvider, LocalKVProviderConfig, LocalKVProvider, GlobalKVProviderConfig are all unused since moby/libnetwork#908. - MakeKVProvider, MakeKVProviderURL, MakeKVProviderConfig are unused since moby#44683. - MakeKVClient is unused since moby#44875 Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The overlay driver was creating a global store whenever netlabel.GlobalKVClient was specified in its config argument. This specific label is not used anymore since 142b522 (moby#44875).

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since 781e666a and c11c2a16. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since moby/moby@96cfb076 (moby#44683). - MakeKVClient is unused since moby/moby@142b5229 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>