libnetwork: reduce usage of shared mutable state in ipamutils#44827
Merged
corhere merged 4 commits intomoby:masterfrom Jan 26, 2023
Merged
libnetwork: reduce usage of shared mutable state in ipamutils#44827corhere merged 4 commits intomoby:masterfrom
corhere merged 4 commits intomoby:masterfrom
Conversation
corhere
force-pushed
the
libnw/drop-electinterfaceaddresses
branch
2 times, most recently
from
54ca5b5 to
ed0ab79
Compare
corhere
requested review from
cpuguy83,
neersighted,
samuelkarp and
thaJeztah
and removed request for
cpuguy83
corhere
added
status/2-code-review
area/networking
thaJeztah
reviewed
Jan 17, 2023
|
|
||
| // NewAllocator returns an instance of libnetwork ipam | ||
| func NewAllocator(lcDs, glDs datastore.DataStore) (*Allocator, error) { | ||
| func NewAllocator(lcDs, glDs datastore.DataStore, lcAs, glAs []*net.IPNet) (*Allocator, error) { |
Member
There was a problem hiding this comment.
As we're changing this signature; would it make sense to have a struct to pass these options? It feels a bit error-prone with multiple arguments of the same type
Contributor
Author
There was a problem hiding this comment.
I'll be changing the signature again in a follow-up. (lcDs and glDs are always nil and therefore can go away.) I find the compile errors when changing the signature helpful to my refactoring efforts.
cpuguy83
approved these changes
Jan 26, 2023
tianon
approved these changes
Jan 26, 2023
neersighted
approved these changes
Jan 26, 2023
The netutils.ElectInterfaceAddresses function is only used in one place outside of tests: in the daemon, to configure the default bridge network. The function is also messy to reason about as it references the shared mutable state of ipamutils.PredefinedLocalScopeDefaultNetworks. It uses the list of predefined default networks to always return an IPv4 address even if the named interface does not exist or does not have any IPv4 addresses. This list happens to be the same as the one used to initialize the address pool of the 'builtin' IPAM driver, though that is far from obvious. (Start with "./libnetwork".initIPAMDrivers and trace the dataflow of the addressPool value. Surprise! Global state is being mutated using the value of other global mutable state.) The daemon does not need the fallback behaviour of ElectInterfaceAddresses. In fact, the daemon does not have to configure an address pool for the network at all! libnetwork will acquire one of the available address ranges from the network's IPAM driver when the preferred-pool configuration is unset. It will do so using the same list of address ranges and the exact same logic (netutils.FindAvailableNetworks) as ElectInterfaceAddresses. So unless the daemon needs to force the network to use a specific address range because the bridge interface already exists, it can leave the details up to libnetwork. Signed-off-by: Cory Snider <[email protected]>
The function references global shared, mutable state and is no longer needed. Deleting it brings us one step closer to getting rid of that pesky shared state. Signed-off-by: Cory Snider <[email protected]>
ipam.Allocator is not a singleton, but it references mutable singleton state. Address that deficiency by refactoring it to instead take the predefined address spaces as constructor arguments. Unfortunately some work is needed on the Swarmkit side before the mutable singleton state can be completely eliminated from the IPAMs. Signed-off-by: Cory Snider <[email protected]>
ConfigLocalScopeDefaultNetworks is now dead code, thank goodness! Make sure it stays dead by deleting the function. Refactor package ipamutils to simplify things given its newly-reduced (ahem) scope. Signed-off-by: Cory Snider <[email protected]>
corhere
force-pushed
the
libnw/drop-electinterfaceaddresses
branch
from
ed0ab79 to
e8011d7
Compare
Member
|
Fixes #44220. |
This was referenced Mar 31, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
Reduced the scope of the shared mutable state in package
"./libnetwork/ipamutils"down to only the bits which require Swarmkit changes to modify.- How I did it
I deleted the function
"./libnetwork/netutils".ElectInterfaceAddresseswhich references the mutable state in ipamutils. I determined that the one non-test reference to that function did not actually need the behaviour which is implemented using the shared mutable state. When the interface doesn't exist, the function would pick an available IP network fromipamutils.PredefinedLocalScopeDefaultNetworks. This is the same list of networks which thebuiltinIPAM driver allocates from, and by extension, the list of networks which libnetwork allocates from when creating a new libnetwork network without the preferred-subnet option set in its IPAM config.I eliminated the implicit configuration of
ipam.Allocatorfrom theipamutilsstate by refactoring it to require the predefined address spaces be passed into the constructor. The IPAM driver initializer still has to pull the global address space from theipamutilsstate to support Swarmkit, but it's still an improvement over the status quo. This refactor allows the local-scope default address space list inipamutilsto be made immutable!- How to verify it
CI?
- Description for the changelog
N/A; refactoring without any end-user visible changes (hopefully)
- A picture of a cute animal (not mandatory but encouraged)