Conversation
6e93710 to
a5c1542
Compare
a5c1542 to
86ab11b
Compare
corhere
left a comment
There was a problem hiding this comment.
Adding methods to an interface that importers provide implementations of is a breaking change. Find a solution which does not necessitate bumping the major version of Swarmkit.
api/types.proto
Outdated
|
|
||
| // IPAMState specifies the current state of IP Address Management. | ||
| message IPAMState { | ||
| // Subnet defines a network as a CIDR address (ie network and mask | ||
| // 192.168.0.1/24). | ||
| string subnet = 1; | ||
|
|
||
| // Range defines the portion of the subnet to allocate to tasks. This is | ||
| // defined as a subnet within the primary subnet. | ||
| string range = 2; | ||
|
|
||
| // Counter of allocated IPs in the subnet | ||
| uint64 subnet_allocated = 3; | ||
| // Counter of allocated IPs in the allocation pool | ||
| uint64 range_allocated = 4; | ||
| } | ||
|
|
||
| // NetworkState specifies the current state of a network. | ||
| message NetworkState { | ||
| // IPAMState specifies the current state of IP Address Management of the network. | ||
| repeated IPAMState ipam = 1 [(gogoproto.customname) = "IPAM"]; | ||
| } |
There was a problem hiding this comment.
Too many aspects of Moby that Swarmkit does not need to know about have already leaked into Swarmkit. Please try to avoid adding more Moby-isms to Swarmkit when possible.
Network messages are not directly exposed to Moby's Engine API clients. Moby consumes these messages internally and maps them to the Moby API types. Given that Moby code would be filling in these message fields, and Moby code would be consuming these message fields, there is no need for Swarmkit to have any visibility into the structure or content of this data. All Moby needs is a conduit by which some opaque-to-Swarmkit data about a network can be passed through Swarmkit and back to Moby code. There are already a couple places in the Swarmkit API which afford for conveying opaque data on behalf of the application, at least one of which is currently used by Moby: api.TaskSpec.Runtime.Generic, for Swarm plugin tasks.
There was a problem hiding this comment.
Thanks! I like this idea, I'll try this
| PredefinedNetworks() []PredefinedNetworkData | ||
| SetDefaultVXLANUDPPort(uint32) error | ||
| NewAllocator(*Config) (NetworkAllocator, error) | ||
| NetworkStateUpdater() NetworkStateUpdater |
There was a problem hiding this comment.
I know how these interfaces are implemented on the Moby side. cnmallocator.cnmNetworkAllocator holds the IPAM state, is not a singleton, and is not an implementation of Provider. This method could only be implemented on cnmallocator.Provider using horrible hacks. There has to be a better way.
There was a problem hiding this comment.
You are right!
I've extened Manager with UpdateNetworkState method satisfy the NetworkStateUpdater interface and pass it the Cluster API Server, since it contains the actual allocator instance with IPAM state.
| // UpdateNetworkState updates the network state | ||
| UpdateNetworkState(network *api.Network) error |
There was a problem hiding this comment.
Why do you need to add this method to two separate interfaces?
There was a problem hiding this comment.
The first one extends the NetworkAllocator interface, which includes IPAM, to provide all related details.
The second one introduces the netStateUpdater interface, which is responsible for passing this functionality to the Cluster API Server
There was a problem hiding this comment.
Adding new methods to the NetworkAllocator interface is a breaking change: the CNMAllocator in github.com/moby/moby@v28.3.3 would not satisfy this interface despite satisfying the interface in github.com/moby/swarmkit/v2@v2.0.0. As I requested in my first review, please find a way to extend Swarmkit without introducing a breaking change that would necessitate bumping Swarmit's major version to 3.
78dbe21 to
7d06b7c
Compare
fde2274 to
2769947
Compare
Adds state field to the Network protobuf message to store network state data. Signed-off-by: Andrey Epifanov <aepifanov@mirantis.com>
2769947 to
ffc54a3
Compare
…AM state for network introspection Introduces NetworkStateUpdater interface and implements it in the allocator and Manager. Pass Manager as NetworkStateUpdater to the Swarm API Server to update network state info when retrieving network information. Signed-off-by: Andrey Epifanov <aepifanov@mirantis.com>
ffc54a3 to
452acdf
Compare
- What I did
This PR introduces tracking of IP address allocation state within networks by extending the API, network allocator, and control API to report IPAM metrics.
- How I did it
StateAnyfield inNetwork messagesto include network state details, such as IPAM data with address counters. TheStateobject is passed to theAPI serveras a serialized blob, avoiding changes to the Swarm API and maintaining backward compatibility in future releases.NetworkStateUpdaterinterface with theUpdateNetworkState()method.UpdateNetworkState()in the clusterManagerto satisfy theNetworkStateUpdaterinterface.NetworkStateUpdaterinstance to theCluster API Serverto provide real-time IPAM state for network introspection.NetworkStateUpdater.UpdateNetworkState()intoGetNetworkandListNetworkscalls in the control API server.- How to test it
Run
docker network inspect <network>for overlay network and verify the newState.IPAMsection reflects real-time allocation counts for each subnet configuration.Depends on:
Design doc:
Design IP-utilization
- Description for the changelog