Data path traffic separation option in swarm mode by fcrisciani · Pull Request #32615 · moby/moby

fcrisciani · 2017-04-14T00:08:48Z

- What I did
In SWARM mode is now possible to pass a new flag to specify which is the interface to use for data path traffic. This will allow to separate data traffic (container to container pkts) from control traffic (grpc between workers and managers)

- How I did it
Introduced a new flag --datapath-addr in the docker swarm init and docker swarm join

- How to verify it
Create a swarm cluster on hosts with at least 2 interfaces, one will be dedicated for the management and will be the one that will be used in the --advertise-addr, the other instead will be the one for the --datapath-addr flag.
Second step is to create an overlay network
Third step is to launch containers on different workers using the overlay network.
At this point the traffic between the containers on different workers will happen on the interface identified by the --datapath-addr

- Description for the changelog

Data path traffic separation option in swarm mode

- A picture of a cute animal (not mandatory but encouraged)

aboch · 2017-04-14T17:49:38Z

daemon/cluster/cluster.go

I think you no longer need this function

I take it back, this is actually needed when libnetwork retrieves the addresss from the clusterProvider

aboch · 2017-04-14T17:49:43Z

daemon/cluster/listen_addr.go

I would handle the empty case right away and avoid the extra indentation here with

if dataPathAddr == "" { return "", nil }

aaronlehmann · 2017-04-14T21:32:10Z

contrib/completion/zsh/_docker

I'd make IP uppercase (in "Data path ip...") to be consistent with other descriptions.

aaronlehmann · 2017-04-14T21:32:18Z

contrib/completion/zsh/_docker

I'd make IP uppercase (in "Data path ip...") to be consistent with other descriptions.

aaronlehmann · 2017-04-14T22:34:04Z

docs/reference/commandline/swarm_init.md

Please clarify in the documentation that this is an address that's used by other nodes, rather than one used for local binding.

done, let me know if now is clearer

aaronlehmann · 2017-04-14T22:39:28Z

daemon/cluster/listen_addr.go

These conditions could be folded into the outer if.

if ip := net.ParseIP(input); ip != nil && (isUnspecifiedValid || !ip.IsUnspecified()) { return ip, nil }

Or perhaps something like this:

ip := net.ParseIP(input) if ip == nil { return nil, errBadNetworkIdentifier } if ip.IsUnspecified() && !isUnspecifiedValid{ return nil, errBadNetworkIdentifier } return return ip, nil

aaronlehmann · 2017-04-14T22:40:44Z

integration-cli/docker_cli_swarm_test.go

Perhaps we could also check that a --datapath-addr argument with a port number is rejected.

aaronlehmann · 2017-04-14T22:41:35Z

cli/command/swarm/opts.go

Wondering if this should be data-path-addr. Not sure either way but wanted to bring it up for discussion.

changed to data-path-addr

aaronlehmann · 2017-04-14T22:43:13Z

docs/reference/commandline/swarm_init.md

The usage info here should match what's printed by swarm init --help. I believe the flags are sorted alphabetically in swarm init --help.

aaronlehmann · 2017-04-14T22:43:24Z

docs/reference/commandline/swarm_join.md

The usage info here should match what's printed by swarm join --help. I believe the flags are sorted alphabetically in swarm join --help.

aaronlehmann · 2017-04-14T22:44:39Z

vendor/github.com/docker/libnetwork/agent.go

Maybe this log message should exclude Data-addr if it's an empty string, for better readability.

Unrelated to this change, but the space between Remote-addr and =%s is inconsistent with the other fields.

aaronlehmann · 2017-04-14T22:45:40Z

vendor/github.com/docker/libnetwork/endpoint.go

Why this change?

Oh, I see the defer now. That's kind of ugly. If a defer is going to inspect the value of err, the function should use a "named return":

func (ep *endpoint) sbJoin(sb *sandbox, options ...EndpointOption) (err error) {

Then returning an error will automatically assign it to err.

This is part of another change brought in by the vendoring.
It is to make sure the existing defer function which performs the cleanup in case of err != nil is being executed.

Yes, I considered the named returned but I decided to not go for that change because I did not want to add refactoring changes to the fix, to speed up review. And to avoid any collateral bugs for changes not strictly needed, because that function has some delicate logic. After the next stable release is out, I will consider refactoring that.

Vendor libnetwork changes for --data-path-addr flag Signed-off-by: Flavio Crisciani <[email protected]>

This new flag will allow the configuration of an interface that can be used for data path traffic to be isolated from control plane traffic. This flag is simply percolated down to libnetwork and will be used by all the global scope drivers (today overlay) Signed-off-by: Flavio Crisciani <[email protected]>

Signed-off-by: Flavio Crisciani <[email protected]>

aaronlehmann · 2017-04-17T22:46:23Z

contrib/completion/bash/docker

 	case "$cur" in
 		-*)
-			COMPREPLY=( $( compgen -W "--advertise-addr --autolock --availability --cert-expiry --dispatcher-heartbeat --external-ca --force-new-cluster --help --listen-addr --max-snapshots --snapshot-interval --task-history-limit" -- "$cur" ) )
+			COMPREPLY=( $( compgen -W "--advertise-addr --datapath-addr --autolock --availability --cert-expiry --dispatcher-heartbeat --external-ca --force-new-cluster --help --listen-addr --max-snapshots --snapshot-interval --task-history-limit" -- "$cur" ) )


Needs to be updated to --data-path-addr

aaronlehmann · 2017-04-17T22:46:27Z

contrib/completion/bash/docker

 	case "$cur" in
 		-*)
-			COMPREPLY=( $( compgen -W "--advertise-addr --availability --help --listen-addr --token" -- "$cur" ) )
+			COMPREPLY=( $( compgen -W "--advertise-addr --datapath-addr --availability --help --listen-addr --token" -- "$cur" ) )


Needs to be updated to --data-path-addr

aaronlehmann · 2017-04-17T22:53:11Z

docs/reference/commandline/swarm_init.md

+### `--data-path-addr`
+
+This flag specifies the address that global scope drivers are going to publish towards
+other workers in order to reach the containers running on this worker node.


I assume this would apply to managers as well (by default, containers can be scheduled to managers as well as workers), so "workers" should be "nodes" and "worker node" should be "node".

Instead of "going to publish", how about "will publish"?

Maybe "global scope drivers" should be "global scope network drivers", for clarity?

aaronlehmann · 2017-04-17T22:53:46Z

integration-cli/docker_cli_swarm_test.go

 	d1 := s.AddDaemon(c, false, false)

 	out, err = d1.Cmd("swarm", "join", "--availability=drain", "--token", token, d.ListenAddr)
+	fmt.Printf("** token:%s, listen:%s\n", token, d.ListenAddr)


Please remove the Printf before merging.

aaronlehmann · 2017-04-17T22:55:16Z

daemon/cluster/listen_addr.go

+	errBadNetworkIdentifier    = errors.New("must specify a valid IP address or interface name")
 	errBadListenAddr           = errors.New("listen address must be an IP address or network interface (with optional port number)")
 	errBadAdvertiseAddr        = errors.New("advertise address must be a non-zero IP address or network interface (with optional port number)")
+	errBadDataPathAddr         = errors.New("data path address must be a non-zero IP address or network interface")


can you add (without a port number) to this, like errBadDefaultAdvertiseAddr?

fcrisciani · 2017-04-19T18:10:06Z

will close this one because does not reflect the commits update

GordonTheTurtle added the status/0-triage label Apr 14, 2017

fcrisciani force-pushed the data_path branch from 5d4d428 to ace2ed5 Compare April 14, 2017 00:32

mavenugo requested a review from aaronlehmann April 14, 2017 01:36

AkihiroSuda added status/1-design-review status/needs-vendoring and removed status/0-triage labels Apr 14, 2017

aboch reviewed Apr 14, 2017

View reviewed changes

aaronlehmann reviewed Apr 14, 2017

View reviewed changes

Flavio Crisciani added 3 commits April 14, 2017 16:53

Libnetwork vendoring

2aed4aa

Vendor libnetwork changes for --data-path-addr flag Signed-off-by: Flavio Crisciani <[email protected]>

Add test for the new flag

610559c

Signed-off-by: Flavio Crisciani <[email protected]>

fcrisciani force-pushed the data_path branch from ace2ed5 to 610559c Compare April 15, 2017 00:40

aaronlehmann reviewed Apr 17, 2017

View reviewed changes

fcrisciani force-pushed the data_path branch 2 times, most recently from 06a7fe7 to fb25eed Compare April 19, 2017 17:34

fcrisciani closed this Apr 19, 2017

fcrisciani mentioned this pull request Apr 19, 2017

Data path traffic separation option in swarm mode #32717

Merged

Conversation

fcrisciani commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcrisciani commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aboch Apr 14, 2017 •

edited

Loading