-
Notifications
You must be signed in to change notification settings - Fork 18.7k
Global Default Address Pool feature support #37558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37558 +/- ##
=========================================
Coverage ? 36.12%
=========================================
Files ? 610
Lines ? 45064
Branches ? 0
=========================================
Hits ? 16280
Misses ? 26538
Partials ? 2246 |
7fb5924
to
c85336d
Compare
ca67176
to
13da5c1
Compare
Please sign your commits following these rules: $ git clone -b "master" git@github.com:selansen/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
890f88d
to
d1be4ae
Compare
Below are the PRs, need to be picked up along with this PR. Libnetwork: moby/libnetwork#2241 CLI : docker/cli#1233 SwarmKit : moby/swarmkit#2714 |
Below failure doesn't look like related to my changes. AIL: docker_cli_swarm_test.go:1376: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey |
2cf041f
to
fef644c
Compare
Added extra integration test :) |
@thaJeztah , PTAL and let me know if you still have any comments. |
483cc86
to
2d7160c
Compare
api/swagger.yaml
Outdated
This flag specifies default subnet pools for global scope networks. If subnet size is not | ||
specified then default value 24 will be used. | ||
If unspecified, Docker will use the predefined subnets as it works on older releases. | ||
type: "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't match the actual API, which uses an array of net.IPNet
for DefaultAddrPool
, and a separate SubnetSize
field for the size.
api/swagger.yaml
Outdated
@@ -8765,6 +8765,12 @@ paths: | |||
nodes in order to reach the containers running on this node. Using this parameter it is possible to | |||
separate the container data traffic from the management traffic of the cluster. | |||
type: "string" | |||
DefaultAddrPool: | |||
description: | | |||
This flag specifies default subnet pools for global scope networks. If subnet size is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swagger describes the API specification, so should not mention "flag".
"subnet size" is a separate option; you can mention it in the description here, but the default should probably be described as part of the description for SubnetSize
.
Would be good to have an example value in the swagger spec as well, so that it's clear how what format is expected.
daemon/cluster/convert/swarm.go
Outdated
@@ -12,6 +13,14 @@ import ( | |||
|
|||
// SwarmFromGRPC converts a grpc Cluster to a Swarm. | |||
func SwarmFromGRPC(c swarmapi.Cluster) types.Swarm { | |||
var defaultAddrPool []*net.IPNet | |||
for _, address := range c.DefaultAddressPool { | |||
_, b, err := net.ParseCIDR(address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change I proposed wouldn't change that logic; the only difference is that it would scope the b
variable to the if
(and it being a more idiomatic code-style); it's just a nit though, so not a blocker
api/types/swarm/swarm.go
Outdated
@@ -153,6 +158,8 @@ type InitRequest struct { | |||
Spec Spec | |||
AutoLockManagers bool | |||
Availability NodeAvailability | |||
DefaultAddrPool []*net.IPNet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I suggested to use IPNet for as internal type (mainly for the CLI-side); for the over-the-wire format for this is rather awkward; the string representation (without converting individual entries to a string when marshalling);
For example, the request now needs to look like;
{"DefaultAddrPool":[{"IP":"192.0.2.0","Mask":"////AA=="}]}
Whereas individually encoding IPNet to a string would look like
{"DefaultAddrPool":["192.0.2.0/24"]}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree with @thaJeztah, we shouldn't leak implementation detail in our API and have our own type here (if we need something complex). Here it seems just a slice of string would work.
api/types/swarm/swarm.go
Outdated
@@ -153,6 +158,8 @@ type InitRequest struct { | |||
Spec Spec | |||
AutoLockManagers bool | |||
Availability NodeAvailability | |||
DefaultAddrPool []*net.IPNet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree with @thaJeztah, we shouldn't leak implementation detail in our API and have our own type here (if we need something complex). Here it seems just a slice of string would work.
daemon/cluster/convert/swarm.go
Outdated
@@ -12,6 +13,14 @@ import ( | |||
|
|||
// SwarmFromGRPC converts a grpc Cluster to a Swarm. | |||
func SwarmFromGRPC(c swarmapi.Cluster) types.Swarm { | |||
var defaultAddrPool []*net.IPNet | |||
for _, address := range c.DefaultAddressPool { | |||
_, b, err := net.ParseCIDR(address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are adding into slice only if b is not null.
Wrong, you're adding it to the slice only if err == nil
, there is no check on b
. I do agree with @thaJeztah and the proposed change 👼
@@ -59,6 +59,18 @@ func NewSwarm(t *testing.T, testEnv *environment.Execution, ops ...func(*daemon. | |||
return d | |||
} | |||
|
|||
// NewSwarmWithOption creates a swarm daemon for testing with init option we specify | |||
func NewSwarmWithOption(t *testing.T, initReq swarmtypes.InitRequest, testEnv *environment.Execution, ops ...func(*daemon.Daemon)) *daemon.Daemon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is required… We should update the NewSwarm
function instead, with a WithInitOption
operation.
internal/test/daemon/swarm.go
Outdated
@@ -28,6 +28,18 @@ func (d *Daemon) StartAndSwarmInit(t testingT) { | |||
d.SwarmInit(t, swarm.InitRequest{}) | |||
} | |||
|
|||
// StartAndSwarmInitWithOption starts the daemon (with busybox) and init the swarm with params | |||
func (d *Daemon) StartAndSwarmInitWithOption(t testingT, initReq swarm.InitRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we could update StartAndSwarmInit
to take *operationswith one for
InitRequest`
func (d *Daemon) StartAndSwarmInit(t testingT, ops ...func(*swarm.InitRequest{})) {
initRequest := &swarm.InitRequest{}
for _, op := range ops { op(initRequest) }
// …
d.SwarmInit(t, *initRequest)
}
@thaJeztah @vdemeester PTAL. Modified API param from IPNet to String slice. |
7887345
to
0e185a2
Compare
vendor.conf
Outdated
@@ -125,7 +125,7 @@ github.com/containerd/ttrpc 94dde388801693c54f88a6596f713b51a8b30b2d | |||
github.com/gogo/googleapis 08a7655d27152912db7aaf4f983275eaf8d128ef | |||
|
|||
# cluster | |||
github.com/docker/swarmkit 8852e8840e30d69db0b39a4a3d6447362e17c64f | |||
github.com/docker/swarmkit 324a450ca695fae75b7bd00d6b83debddea18462 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor issues in swagger.yml (and some suggestions for follow-ups), but otherwise looks good to me.
api/swagger.yaml
Outdated
DefaultAddrPool: | ||
description: | | ||
Default Address Pool specifies default subnet pools for global scope networks. | ||
type: "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "array"
with "string"
as type for the items, e.g. similar to
Lines 414 to 417 in 896d1b1
type: "array" | |
items: | |
type: "string" | |
example: "c 13:* rwm" |
api/swagger.yaml
Outdated
Spec: | ||
$ref: "#/definitions/SwarmSpec" | ||
example: | ||
ListenAddr: "0.0.0.0:2377" | ||
AdvertiseAddr: "192.168.1.1:2377" | ||
DefaultAddrPool: "20.20.0.0/16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultAddrPool
is an array, so this should be something like;
DefaultAddrPool: ["10.10.0.0/16", "20.20.0.0/16"]
// Hostname is not set here. Instead, it is obtained from | ||
// the node description that is reported periodically | ||
swarmnodeConfig := swarmnode.Config{ | ||
ForceNewCluster: conf.forceNewCluster, | ||
ListenControlAPI: control, | ||
ListenRemoteAPI: conf.ListenAddr, | ||
AdvertiseRemoteAPI: conf.AdvertiseAddr, | ||
DefaultAddrPool: defaultAddrPool, | ||
SubnetSize: int(conf.SubnetSize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we'd fix the int
/ uint32
on swarmkit side as well before GA (for a follow-up before GA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I am planning to refactor swarmkit code as Anshul wanted to move around code a bit
@@ -110,13 +115,21 @@ func (n *nodeRunner) start(conf nodeStartConfig) error { | |||
joinAddr = conf.RemoteAddr | |||
} | |||
|
|||
var defaultAddrPool []*net.IPNet | |||
for _, address := range conf.DefaultAddressPool { | |||
if _, b, err := net.ParseCIDR(address); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done in a follow-up, but; I see other validation steps are done in newNodeRunner()
; we should consider moving those all to a single place, not partly here, and partly there. We can also return parsing errors there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@selansen vendor check is failing; can you check?
|
This feature allows user to specify list of subnets for global default address pool. User can configure subnet list using 'swarm init' command. Daemon passes the information to swarmkit. We validate the information in swarmkit, then store it in cluster object. when IPAM init is called, we pass subnet list to IPAM driver. Signed-off-by: selansen <elango.siva@docker.com>
PowerPC failure is unrelated; #34988 |
windowsRS1 failure : this doesnt look related to my change |
This feature allows user to specify list of subnets for global
default address pool. User can configure subnet list using
'swarm init' command. Daemon passes the information to swarmkit.
We validate the information in swarmkit, then store it in cluster
object. when IPAM init is called, we pass subnet list to IPAM driver.
Signed-off-by: selansen elango.siva@docker.com
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)