Inject network allocator into Node #3167
Merged
Conversation
This file contains 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
Replicate the port allocation-related tests from the CNM network allocator's test corpus in preparation for lifting port allocation into allocator.Allocator. Signed-off-by: Cory Snider <csnider@mirantis.com>
The portAllocator constructor function newPortAllocator() takes no arguments; all its initial state is taken from hardcoded constants in the source code. Any failure to construct a port allocator must therefore be the result of a bug in the code, which can only be fixed with a code change. As there is nothing the caller can do at runtime to cause the failure, there is little value in returning the error to the caller. Change the constructor to panic on failure instead. Signed-off-by: Cory Snider <csnider@mirantis.com>
The port allocation logic does not depend on the network allocator implementation in any meaningful way. It has no knowledge of the CNM network allocator's state, and it does not need to change if the network allocator changes. Allocating node ports is fundamentally a seaparate concern from allocating network resources for services and tasks. Therefore the low-level network allocator should not be responsible for allocating both. Lift the port allocator into the Allocator's network context as a sibling of the low-level network allocator. Signed-off-by: Cory Snider <csnider@mirantis.com>
The network allocator configuration is all sourced from the Cluster object and persisted in Raft. Any change to the configuration therefore requires modifications to the Swarmkit API objects and manager logic. Move the cnmallocator.NetworkConfig struct to networkallocator.Config to reflect that Swarmkit, not the network allocator implementations, dictates which configuration values are passed into the allocator. Signed-off-by: Cory Snider <csnider@mirantis.com>
Lift the construction of the low-level network allocator into the Allocator constructor so the cnmallocator constructor arguments do not need to be persisted in the Allocator struct. Signed-off-by: Cory Snider <csnider@mirantis.com>
corhere
force-pushed
the
inject-allocator
branch
5 times, most recently
from
94b740b
to
b02d6f6
Compare
allocator.Allocator is ostensibly loosely coupled with the low-level network allocator implementation by virtue of depending on an abstraction: the networkallocator.NetworkAllocator interface. However, in practice it also depends directly on the cnmallocator implementation. The Allocator constructs a CNM network allocator directly, and code in the allocator package directly calls functions in the cnmallocator package. A similar issue exists within controlapi. While it does not directly depend on cnmallocator, its validation logic contains deep knowledge of what is and is not valid for the CNM network allocator to consume. Decouple manager.Manager and allocator.Allocator from the cnmallocator implementation of the low-level network allocator by refactoring them to depend on a new Provider interface instead of the cnmallocator functions. Decouple controlapi.Server from the cnmallocator implementation by refactoring it to depend on a new Validator interface implemented by the low-level network allocators and lowering the validation logic into the cnmallocator implementation. Construct the cnmallocator Provider in the node.Node and inject it into the manager.Manager. Signed-off-by: Cory Snider <csnider@mirantis.com>
Make whichever main package that instantiates the Node responsible for constructing and injecting the appropriate low-level network allocator Provider for the application. Signed-off-by: Cory Snider <csnider@mirantis.com>
Remove the last direct libnetwork call outside of cnmallocator from Swarmkit. Signed-off-by: Cory Snider <csnider@mirantis.com>
corhere
force-pushed
the
inject-allocator
branch
from
b02d6f6
to
e1c4a7d
Compare
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Refactored the node and manager code to no longer depend on github.com/docker/docker/libnetwork/... packages in preparation for breaking the dependency loop with github.com/docker/docker and moving the cnmallocator to docker/docker. The port allocator has been moved out of package github.com/moby/swarmkit/v2/manager/allocator/cnmallocator and nearly all imports of libnetwork packages have been centralized in the cnmallocator package. (The one exception is the github.com/docker/docker/libnetwork/bitmap package, imported by package github.com/moby/swarmkit/internal/idm which is used by the port allocator. This import will be replaced in a follow-up PR.) All imports of the cnmallocator package have been excised from the swarmkit module. The low-level network allocator implementation is now injected into the node constructor.
- How I did it
Direct libnetwork calls in node, manager and controlapi were replaced with method calls on a new
networkallocator.Providerinterface value. Direct calls to the cnmallocator constructor were replaced with dependency injection of anetworkallocator.NetworkAllocatorvalue. If no provider or allocator is injected, an inert implementation is used which does not allocate any network resources.The allocator test corpus has been refactored to be an integration test suite parameterized on
networkallocator.Providerso that any provider implementation, in-tree or out of tree, can (in theory) be tested for compatibility with the allocator. The test corpus still contains some hardcoded assumptions that the network allocator under test is the CNM allocator such as driver names, so in practice the suite is not yet fit for integration-testing other implementations.- How to test it
CI
- Description for the changelog