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

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

- 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 --data-path-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--data-path-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 --data-path-addr

- Description for the changelog

Data path traffic separation option in swarm mode

Fixes #32446

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

fcrisciani · 2017-04-19T18:11:59Z

@aaronlehmann @aboch had to reopen it because the previous pull #32615 was not getting updated.
Please review this one

aaronlehmann · 2017-04-25T01:39:19Z

LGTM

aboch · 2017-04-25T18:01:35Z

looks good to me

cpuguy83 · 2017-04-26T01:54:39Z

Can you squash so there are two commits, one for libnetwork and one for the new flag/test/etc

auntunif · 2017-04-26T02:38:21Z

/ test / etc

fcrisciani · 2017-04-26T03:21:20Z

@cpuguy83 good like that?

fcrisciani · 2017-04-26T18:58:09Z

@cpuguy83 the powerpc pipeline is stuck

cpuguy83 · 2017-04-26T21:08:23Z

Can we move these cli changes over to the docker/cli repo?

aaronlehmann · 2017-04-26T21:11:07Z

I thought the CLI code hadn't been moved over yet. Shouldn't we make changes here until it is?

cpuguy83 · 2017-04-26T21:19:20Z

Every change is 1 more rebase.
As I understand it's fairly imminent.

aaronlehmann · 2017-04-26T21:25:01Z

I'm concerned about having two diverging implementations. I think we should continue making changes here until the switch happens.

Among other things, the integration test being added here wouldn't pass because there would be a circular dependency between the test and the code being added in docker/cli. I think this is one of the things we need to solve before splitting out the CLI. I haven't heard a proposal yet.

dongluochen · 2017-04-26T21:44:35Z

daemon/cluster/swarm.go

If --data-path-addr not specified, should it use AdvertiseAddr, or empty string?

According to document, it should use AdvertiseAddr.

+If unspecified, Docker will use the same IP address or interface that is used for the +advertise address.

The config is saved consistently with the user input, this way we are not losing track of the original value inserted from the CLI.
The final decision on what to use in case the parameter is not specified is left to the final user of the flag itself.
Today libnetwork if the DataPathAddr is not specified will fallback using the AdvertiseAddr.

Make sense. I think the GetDataPathAddress should clarify its description.

dongluochen · 2017-04-26T22:02:02Z

daemon/cluster/cluster.go

GetDataPathAddress returns the address configured for data path traffic.

Would you prefer something like: GetDataPathAddress returns the address to be used for the data path traffic, if specified.

That sounds good.

k will change it

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) Negative test added for invalid flag arguments Signed-off-by: Flavio Crisciani <[email protected]>

dongluochen · 2017-04-26T22:41:24Z

looks good to me

cpuguy83

LGTM

thaJeztah · 2017-04-29T02:21:35Z

/cc @johndmulhausen @mstanleyjones for docs

fntlnz · 2017-05-22T15:58:55Z

daemon/cluster/listen_addr.go

-	if err != errNoSuchInterface {
+	specifiedIP, err := resolveInputIPAddr(specifiedHost, true)
+	if err != nil {
+		if err == errBadNetworkIdentifier {


@fcrisciani why this error is overwritten?

This is the resolveListenAddr that validates the listening address so the error is converted in a way that report an issue on the listening address

fntlnz · 2017-05-22T16:04:39Z

integration-cli/docker_cli_swarm_test.go

+func (s *DockerSwarmSuite) TestSwarmInitUnspecifiedDataPathAddr(c *check.C) {
+	d := s.AddDaemon(c, false, false)
+
+	out, err := d.Cmd("swarm", "init", "--data-path-addr", "0.0.0.0")


Here I would add a test with a completely invalid data path addr to check if the error is consistent

non valid IP addresses are captured by the net.ParseIP() and all collapsed to the same error message errBadNetworkIdentifier.

fntlnz · 2017-05-22T16:08:51Z

vendor.conf


 #get libnetwork packages
-github.com/docker/libnetwork b13e0604016a4944025aaff521d9c125850b0d04
+github.com/docker/libnetwork 5dc95a3f9ce4b70bf08492ca37ec55c5b6d84975


I see that this commit implements your required changes from libnetwork but why not using the latest commit at this stage?

for those who need a compare:

Proposed commit
moby/libnetwork@b13e060...5dc95a3

Master:
moby/libnetwork@b13e060...master

Is there something I'm missing?

Not sure I follow, this patch got merged already several days ago

@fntlnz this PR was merged 25 days ago, so wouldn't be able to get all changes in master that are available now :)

daf*** haven't noticed the merged label :D had it in my review list and finally found time to look at it! Anyway @fcrisciani super nice feature!

praving55 · 2017-07-04T06:48:37Z

This looks to be broken - #33936

thaJeztah · 2017-07-04T07:03:44Z

@praving55 ah! I see you commented here as well; I see the flag was renamed during the design/PR review process (originally --datapath-addr, later --data-path-addr. I updated the top description to prevent this confusion (already commented in #33936 (comment))

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

fcrisciani force-pushed the data_path branch 3 times, most recently from 30952b1 to 3aee03b Compare April 19, 2017 18:20

aaronlehmann added status/2-code-review status/needs-vendoring and removed status/0-triage labels Apr 19, 2017

fcrisciani changed the title ~~Data path~~ Data path traffic separation option in swarm mode Apr 20, 2017

fcrisciani force-pushed the data_path branch from 3aee03b to 68f1028 Compare April 24, 2017 20:59

aboch mentioned this pull request Apr 25, 2017

[17.05.x] Vendoring libnetwork @5d4e5de #32819

Merged

fcrisciani force-pushed the data_path branch from 68f1028 to 6223014 Compare April 25, 2017 18:50

fcrisciani force-pushed the data_path branch from 6223014 to ac92fbc Compare April 26, 2017 03:21

cpuguy83 added the rebuild/powerpc label Apr 26, 2017

GordonTheTurtle removed the rebuild/powerpc label Apr 26, 2017

dongluochen reviewed Apr 26, 2017

View reviewed changes

Flavio Crisciani added 2 commits April 26, 2017 15:33

Libnetwork vendoring

b96988f

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

fcrisciani force-pushed the data_path branch from ac92fbc to 8dc8cd4 Compare April 26, 2017 22:34

cpuguy83 approved these changes Apr 27, 2017

View reviewed changes

cpuguy83 added rebuild/experimental status/4-merge labels Apr 27, 2017

GordonTheTurtle removed rebuild/experimental labels Apr 27, 2017

cpuguy83 merged commit 0307fe1 into moby:master Apr 27, 2017

aaronlehmann mentioned this pull request Apr 28, 2017

Flaky test: TestAPISwarmLeaderElection #32673

Open

fcrisciani deleted the data_path branch April 29, 2017 00:52

thaJeztah added impact/changelog impact/cli impact/documentation and removed status/needs-vendoring labels Apr 29, 2017

thaJeztah added this to the 17.06.0 milestone Apr 29, 2017

fntlnz reviewed May 22, 2017

View reviewed changes

mdlinville mentioned this pull request Jun 2, 2017

Document Data path traffic separation option in swarm mode docker/docs#3478

Closed

This was referenced Jul 4, 2017

Swarm: unknown flag: --datapath-addr #33936

Closed

Add missing API documentation for DataPathAddr #33941

Merged

thaJeztah mentioned this pull request Nov 9, 2017

SWARM Mode - Docker binds ingress ports always on 0.0.0.0 #35318

Closed

thaJeztah mentioned this pull request Jul 30, 2018

Add API-version anotation for --data-path-addr docker/cli#1240

Merged

thaJeztah mentioned this pull request Nov 9, 2018

VXLAN UDP Port configuration support #38102

Merged

Conversation

fcrisciani commented Apr 19, 2017 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

aaronlehmann commented Apr 25, 2017

Uh oh!

aboch commented Apr 25, 2017

Uh oh!

cpuguy83 commented Apr 26, 2017

Uh oh!

auntunif commented Apr 26, 2017

Uh oh!

fcrisciani commented Apr 26, 2017

Uh oh!

fcrisciani commented Apr 26, 2017

Uh oh!

cpuguy83 commented Apr 26, 2017

Uh oh!

aaronlehmann commented Apr 26, 2017

Uh oh!

cpuguy83 commented Apr 26, 2017

Uh oh!

aaronlehmann commented Apr 26, 2017

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!

dongluochen commented Apr 26, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 29, 2017

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!

fntlnz May 22, 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!

fntlnz May 22, 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!

praving55 commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 4, 2017

Uh oh!

Reviewers

Assignees

Labels

fcrisciani commented Apr 19, 2017 •

edited by thaJeztah

Loading

fcrisciani commented Apr 19, 2017 •

edited

Loading

fntlnz May 22, 2017 •

edited

Loading

fntlnz May 22, 2017 •

edited

Loading

praving55 commented Jul 4, 2017 •

edited

Loading