Global Default Address Pool feature support #37558

selansen · 2018-07-30T15:30:37Z

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)

codecov · 2018-07-30T20:57:07Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9916827). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37558   +/-   ##
=========================================
  Coverage          ?   36.12%           
=========================================
  Files             ?      610           
  Lines             ?    45064           
  Branches          ?        0           
=========================================
  Hits              ?    16280           
  Misses            ?    26538           
  Partials          ?     2246

GordonTheTurtle · 2018-08-10T18:45:57Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ 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.

selansen · 2018-08-11T20:24:53Z

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

selansen · 2018-08-16T03:04:17Z

Below failure doesn't look like related to my changes. AIL: docker_cli_swarm_test.go:1376: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey
22:04:47
22:04:47 [d8a8c429e1029] waiting for daemon to start
22:04:47 [d8a8c429e1029] daemon started
22:04:47
22:04:47 [d5a76b606666e] waiting for daemon to start
22:04:47 [d5a76b606666e] daemon started
22:04:47
22:04:47 [d82090c38b68e] waiting for daemon to start
22:04:47 [d82090c38b68e] daemon started
22:04:47
22:04:47 [d5a76b606666e] exiting daemon
22:04:47 [d5a76b606666e] waiting for daemon to start
22:04:47 [d5a76b606666e] daemon started
22:04:47
22:04:47 [d82090c38b68e] exiting daemon
22:04:47 [d82090c38b68e] waiting for daemon to start
22:04:47 [d82090c38b68e] daemon started
22:04:47
22:04:47 [d5a76b606666e] exiting daemon
22:04:47 [d5a76b606666e] waiting for daemon to start
22:04:47 [d5a76b606666e] daemon started
22:04:47
22:04:47 [d82090c38b68e] exiting daemon
22:04:47 [d82090c38b68e] waiting for daemon to start
22:04:47 [d82090c38b68e] daemon started
22:04:47
22:04:47 docker_cli_swarm_test.go:1454:
22:04:47 c.Assert(err, checker.IsNil)
22:04:47 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc421e899c0), Stderr:[]uint8(nil)} ("exit status 1")
22:04:47
22:04:47 [d8a8c429e1029] exiting daemon
22:04:47 [d5a76b606666e] exiting daemon
22:04:47 [d82090c38b68e] exiting daemon

selansen · 2018-08-20T01:18:02Z

Added extra integration test :)

selansen · 2018-08-20T01:18:47Z

@thaJeztah , PTAL and let me know if you still have any comments.

thaJeztah · 2018-08-20T11:46:05Z

api/swagger.yaml

+                  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"


This doesn't match the actual API, which uses an array of net.IPNet for DefaultAddrPool, and a separate SubnetSize field for the size.

thaJeztah · 2018-08-20T11:48:59Z

api/swagger.yaml

@@ -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


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.

thaJeztah · 2018-08-20T11:59:01Z

daemon/cluster/convert/swarm.go

@@ -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)


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

thaJeztah · 2018-08-20T12:34:07Z

api/types/swarm/swarm.go

@@ -153,6 +158,8 @@ type InitRequest struct {
 	Spec             Spec
 	AutoLockManagers bool
 	Availability     NodeAvailability
+	DefaultAddrPool  []*net.IPNet


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"]}

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.

vdemeester · 2018-08-20T12:50:01Z

api/types/swarm/swarm.go

@@ -153,6 +158,8 @@ type InitRequest struct {
 	Spec             Spec
 	AutoLockManagers bool
 	Availability     NodeAvailability
+	DefaultAddrPool  []*net.IPNet


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.

vdemeester · 2018-08-20T12:51:34Z

daemon/cluster/convert/swarm.go

@@ -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)


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 👼

vdemeester · 2018-08-20T12:55:52Z

integration/internal/swarm/service.go

@@ -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 {


I don't think this is required… We should update the NewSwarm function instead, with a WithInitOption operation.

vdemeester · 2018-08-20T12:58:22Z

internal/test/daemon/swarm.go

@@ -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) {


Same as above, we could update StartAndSwarmInit to take *operationswith one forInitRequest`

func (d *Daemon) StartAndSwarmInit(t testingT, ops ...func(*swarm.InitRequest{})) { initRequest := &swarm.InitRequest{} for _, op := range ops { op(initRequest) } // … d.SwarmInit(t, *initRequest) }

selansen · 2018-08-20T17:05:29Z

@thaJeztah @vdemeester PTAL. Modified API param from IPNet to String slice.

anshulpundir · 2018-08-20T18:02:57Z

vendor.conf

@@ -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


I would suggest updating to the latest which is cfa742c @selansen

will update now.

thaJeztah

Some minor issues in swagger.yml (and some suggestions for follow-ups), but otherwise looks good to me.

thaJeztah · 2018-08-20T18:04:10Z

api/swagger.yaml

+              DefaultAddrPool:
+                description: |
+                  Default Address Pool specifies default subnet pools for global scope networks.
+                type: "string"


This should be "array" with "string" as type for the items, e.g. similar to

moby/api/swagger.yaml

Lines 414 to 417 in 896d1b1

type: "array"

items:

type: "string"

example: "c 13:* rwm"

thaJeztah · 2018-08-20T18:05:33Z

api/swagger.yaml

              Spec:
                $ref: "#/definitions/SwarmSpec"
            example:
              ListenAddr: "0.0.0.0:2377"
              AdvertiseAddr: "192.168.1.1:2377"
+              DefaultAddrPool: "20.20.0.0/16"


DefaultAddrPool is an array, so this should be something like;

DefaultAddrPool: ["10.10.0.0/16", "20.20.0.0/16"]

thaJeztah · 2018-08-20T18:08:06Z

daemon/cluster/noderunner.go

 	// 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),


Ideally, we'd fix the int / uint32 on swarmkit side as well before GA (for a follow-up before GA)

yes. I am planning to refactor swarmkit code as Anshul wanted to move around code a bit

thaJeztah · 2018-08-20T18:15:33Z

daemon/cluster/noderunner.go

@@ -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 {


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.

thaJeztah

LGTM

thaJeztah · 2018-08-20T18:58:52Z

@selansen vendor check is failing; can you check?

18:50:21 2018/08/20 18:50:21 Running time: 1m34.119400485s
18:50:21 The result of vndr differs
18:50:21 
18:50:21  M vendor/github.com/docker/swarmkit/manager/allocator/network.go
18:50:21 
18:50:21 Please vendor your package with github.com/LK4D4/vndr.

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>

thaJeztah · 2018-08-20T22:00:28Z

PowerPC failure is unrelated; #34988

selansen · 2018-08-20T23:59:15Z

windowsRS1 failure : this doesnt look related to my change
Build timed out (after 200 minutes). Marking the build as failed.

GordonTheTurtle added the status/0-triage label Jul 30, 2018

thaJeztah added status/1-design-review area/networking impact/api and removed status/0-triage labels Jul 30, 2018

selansen force-pushed the master branch from 0710466 to e655b72 Compare July 30, 2018 20:57

selansen force-pushed the master branch 2 times, most recently from 7fb5924 to c85336d Compare August 3, 2018 21:42

selansen force-pushed the master branch 6 times, most recently from ca67176 to 13da5c1 Compare August 10, 2018 18:45

selansen requested review from tianon and tonistiigi as code owners August 10, 2018 18:45

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 10, 2018

selansen force-pushed the master branch from 13da5c1 to b37232f Compare August 10, 2018 19:12

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 10, 2018

selansen force-pushed the master branch 3 times, most recently from 890f88d to d1be4ae Compare August 10, 2018 20:26

GordonTheTurtle assigned yongtang Aug 14, 2018

selansen force-pushed the master branch 3 times, most recently from 2cf041f to fef644c Compare August 17, 2018 03:15

selansen force-pushed the master branch 2 times, most recently from 483cc86 to 2d7160c Compare August 20, 2018 12:12

thaJeztah requested changes Aug 20, 2018

View reviewed changes

selansen force-pushed the master branch from 2d7160c to 4aae640 Compare August 20, 2018 12:43

vdemeester requested changes Aug 20, 2018

View reviewed changes

selansen force-pushed the master branch from 4aae640 to 87b81c9 Compare August 20, 2018 17:02

selansen force-pushed the master branch 2 times, most recently from 7887345 to 0e185a2 Compare August 20, 2018 17:38

anshulpundir reviewed Aug 20, 2018

View reviewed changes

thaJeztah requested changes Aug 20, 2018

View reviewed changes

selansen force-pushed the master branch from 0e185a2 to f10e1bc Compare August 20, 2018 18:41

thaJeztah approved these changes Aug 20, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/1-design-review labels Aug 20, 2018

selansen force-pushed the master branch from f10e1bc to f7ad95c Compare August 20, 2018 19:07

thaJeztah mentioned this pull request Aug 20, 2018

Add "Warnings" to /info endpoint, and move detection to the daemon #37502

Merged

tiborvass merged commit 1800883 into moby:master Aug 21, 2018

thaJeztah mentioned this pull request Aug 21, 2018

Use warnings provided by daemon docker/cli#1225

Merged

kolyshkin mentioned this pull request Sep 6, 2018

TestServiceWithDefaultAddressPoolInit: avoid panic #37773

Merged

This was referenced Nov 13, 2018

Ignore default address-pools on API < 1.39 #38196

Merged

Add missing default address pool fields to swagger #38218

Merged

This was referenced Nov 20, 2018

[18.09 backport] Ignore default address-pools on API < 1.39 docker-archive/engine#118

Merged

[18.09 backport] Add missing default address pool fields to swagger docker-archive/engine#119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global Default Address Pool feature support #37558

Global Default Address Pool feature support #37558

selansen commented Jul 30, 2018

codecov bot commented Jul 30, 2018 •

edited

Loading

GordonTheTurtle commented Aug 10, 2018

selansen commented Aug 11, 2018

selansen commented Aug 16, 2018

selansen commented Aug 20, 2018

selansen commented Aug 20, 2018

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018

vdemeester Aug 20, 2018

vdemeester Aug 20, 2018

vdemeester Aug 20, 2018

vdemeester Aug 20, 2018

vdemeester Aug 20, 2018

selansen commented Aug 20, 2018

anshulpundir Aug 20, 2018

selansen Aug 20, 2018

thaJeztah left a comment

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018 •

edited

Loading

selansen Aug 20, 2018 •

edited

Loading

thaJeztah Aug 20, 2018

thaJeztah left a comment

thaJeztah commented Aug 20, 2018

thaJeztah commented Aug 20, 2018

selansen commented Aug 20, 2018

Global Default Address Pool feature support #37558

Global Default Address Pool feature support #37558

Conversation

selansen commented Jul 30, 2018

codecov bot commented Jul 30, 2018 • edited Loading

Codecov Report

GordonTheTurtle commented Aug 10, 2018

selansen commented Aug 11, 2018

selansen commented Aug 16, 2018

selansen commented Aug 20, 2018

selansen commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selansen commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

selansen Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Aug 20, 2018

thaJeztah commented Aug 20, 2018

selansen commented Aug 20, 2018

codecov bot commented Jul 30, 2018 •

edited

Loading

thaJeztah Aug 20, 2018 •

edited

Loading

selansen Aug 20, 2018 •

edited

Loading