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

Data path traffic separation option in swarm mode #32717

Merged
merged 2 commits into from Apr 27, 2017

Conversation

@fcrisciani
Contributor

fcrisciani commented Apr 19, 2017

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

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Apr 19, 2017

Contributor

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

Contributor

fcrisciani commented Apr 19, 2017

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

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

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 25, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Apr 25, 2017

LGTM

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Apr 25, 2017

Contributor

looks good to me

Contributor

aboch commented Apr 25, 2017

looks good to me

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 26, 2017

Contributor

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

Contributor

cpuguy83 commented Apr 26, 2017

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

@auntunif

This comment has been minimized.

Show comment
Hide comment
@auntunif

auntunif Apr 26, 2017

/ test / etc

auntunif commented Apr 26, 2017

/ test / etc

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Apr 26, 2017

Contributor

@cpuguy83 good like that?

Contributor

fcrisciani commented Apr 26, 2017

@cpuguy83 good like that?

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Apr 26, 2017

Contributor

@cpuguy83 the powerpc pipeline is stuck

Contributor

fcrisciani commented Apr 26, 2017

@cpuguy83 the powerpc pipeline is stuck

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 26, 2017

Contributor

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

Contributor

cpuguy83 commented Apr 26, 2017

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

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 26, 2017

Contributor

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

Contributor

aaronlehmann commented Apr 26, 2017

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 26, 2017

Contributor

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

Contributor

cpuguy83 commented Apr 26, 2017

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

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 26, 2017

Contributor

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.

Contributor

aaronlehmann commented Apr 26, 2017

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.

@@ -93,6 +98,7 @@ func (c *Cluster) Init(req types.InitRequest) (string, error) {
LocalAddr: localAddr,
ListenAddr: net.JoinHostPort(listenHost, listenPort),
AdvertiseAddr: net.JoinHostPort(advertiseHost, advertisePort),
DataPathAddr: dataPathAddr,

This comment has been minimized.

@dongluochen

dongluochen Apr 26, 2017

Contributor

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

@dongluochen

dongluochen Apr 26, 2017

Contributor

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

This comment has been minimized.

@dongluochen

dongluochen Apr 26, 2017

Contributor

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

dongluochen Apr 26, 2017

Contributor

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.

This comment has been minimized.

@fcrisciani

fcrisciani Apr 26, 2017

Contributor

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.

@fcrisciani

fcrisciani Apr 26, 2017

Contributor

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.

This comment has been minimized.

@dongluochen

dongluochen Apr 26, 2017

Contributor

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

@dongluochen

dongluochen Apr 26, 2017

Contributor

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

fcrisciani added some commits Apr 14, 2017

Libnetwork vendoring
Vendor libnetwork changes for --data-path-addr flag

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
Inroduce SWARM --data-path-addr flag
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 <flavio.crisciani@docker.com>
@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Apr 26, 2017

Contributor

looks good to me

Contributor

dongluochen commented Apr 26, 2017

looks good to me

@cpuguy83

LGTM

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

6 of 7 checks passed

experimental Jenkins build Docker-PRs-experimental 33287 has failed
Details
dco-signed All commits are signed
janky Jenkins build Docker-PRs 41893 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2124 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3222 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13055 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1942 has succeeded
Details

@fcrisciani fcrisciani deleted the fcrisciani:data_path branch Apr 29, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 29, 2017

Member

/cc @johndmulhausen @mstanleyjones for docs

Member

thaJeztah commented Apr 29, 2017

/cc @johndmulhausen @mstanleyjones for docs

@thaJeztah thaJeztah added this to the 17.06.0 milestone Apr 29, 2017

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

This comment has been minimized.

@fntlnz

fntlnz May 22, 2017

Member

@fcrisciani why this error is overwritten?

@fntlnz

fntlnz May 22, 2017

Member

@fcrisciani why this error is overwritten?

This comment has been minimized.

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

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

This comment has been minimized.

@fntlnz

fntlnz May 22, 2017

Member

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

@fntlnz

fntlnz May 22, 2017

Member

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

This comment has been minimized.

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

@@ -25,7 +25,7 @@ github.com/RackSec/srslog 456df3a81436d29ba874f3590eeeee25d666f8a5
github.com/imdario/mergo 0.2.1
#get libnetwork packages
github.com/docker/libnetwork b13e0604016a4944025aaff521d9c125850b0d04
github.com/docker/libnetwork 5dc95a3f9ce4b70bf08492ca37ec55c5b6d84975

This comment has been minimized.

@fntlnz

fntlnz May 22, 2017

Member

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

@fntlnz

fntlnz May 22, 2017

Member

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

This comment has been minimized.

@fntlnz

fntlnz May 22, 2017

Member

for those who need a compare:

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

Master:
docker/libnetwork@b13e060...master

Is there something I'm missing?

@fntlnz

fntlnz May 22, 2017

Member

for those who need a compare:

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

Master:
docker/libnetwork@b13e060...master

Is there something I'm missing?

This comment has been minimized.

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

@fcrisciani

fcrisciani May 22, 2017

Contributor

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

This comment has been minimized.

@thaJeztah

thaJeztah May 22, 2017

Member

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

@thaJeztah

thaJeztah May 22, 2017

Member

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

This comment has been minimized.

@fntlnz

fntlnz May 22, 2017

Member

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!

@fntlnz

fntlnz May 22, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@praving55

praving55 Jul 4, 2017

This looks to be broken - #33936

praving55 commented Jul 4, 2017

This looks to be broken - #33936

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 4, 2017

Member

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

Member

thaJeztah commented Jul 4, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment