Adding network specific options to service create/update #33130

abhi · 2017-05-10T05:11:55Z

- What I did
Added capability to service create/update to accept network option in csv format and add test case to verify the same.The change includes name, alias and driver options specific to the network. The options are passed to the driver on create and join of endpoint. The corresponding changes are in swarmkit and cli.

- How I did it
Changed --network , --network-add options to accept csv format key/value by creating a new network option type.

- How to verify it

docker service create --name web --network name=docknet,alias=web1,driver-opt=field1=value1 nginx
docker service create --name web --network docknet nginx
docker service update web --network-add name=docknet,alias=web1,driver-opt=field1=value1
docker service update web --network-rm docknet

Relevant PRs:
moby/swarmkit#2176 and moby/swarmkit#2183 (merged)
docker/cli#62

The cli change in docker/cli#62 will allow users to pass driver options, alias and network name as a part of service create and service update. The driver options information will eventually trickle down as a part of endpoint create and join which will allow the drivers to use this information for any additional processing for the endpoint.
Alias information is already part of docker run as --net-alias. However since service can be created and updated with multiple networks there is a need to pass network specific aliases which can also be done with this change.
The change in this PR ties this information to backend structs to pass the driver options, alias as generic options to libnetwork

aaronlehmann

Can you please rebase your swarmkit branch? It appears to be older than what's currently vendored, so it's effectively reverting a bunch of changes. This makes it hard to review the changes to swarmkit.

aaronlehmann · 2017-05-10T17:54:34Z

opts/network.go

This code doesn't seem to be used anywhere in moby. I think it belongs in the docker/cli repo, not here.

We will need to move the opts/* to docker/cli. I believe that should be a seperate PR. I am going to park this here until then.

Yes. I agree. There are multiple other CLIs with their CSV options all part of the opts package and are currently used only in docker/cli. I guess moving opts out to docker/cli is its own independent work.

@vdemeester opened a pull request here to move the options; docker/cli#82

aaronlehmann · 2017-05-10T17:55:08Z

opts/network.go

This import should be separated from the standard library imports by a blank line.

aaronlehmann · 2017-05-10T17:55:38Z

api/types/network/network.go

What is this method used for? I don't see it called anywhere.

aaronlehmann · 2017-05-10T17:55:47Z

api/types/network/network.go

There should be a space between // and the start of the comment.

aaronlehmann · 2017-05-10T17:56:35Z

daemon/cluster/convert/service.go

strings should not be moved down here.

aaronlehmann · 2017-05-10T17:56:47Z

daemon/cluster/executor/container/container.go

logrus should not be moved here.

aaronlehmann · 2017-05-10T17:57:17Z

daemon/cluster/executor/container/container.go

An extra tab was inserted on this line.

aaronlehmann · 2017-05-10T17:59:10Z

vendor/github.com/docker/swarmkit/api/types.proto

As a protobuf field name, this should be driver_opts.

aaronlehmann · 2017-05-10T18:04:35Z

vendor/github.com/docker/swarmkit/manager/allocator/network.go

What is this for?

I will be making the swarmkit changes and update it.

mavenugo · 2017-05-12T11:34:23Z

api/types/network/network.go

The use of Options is confusing in this context. Isnt a better name for this is EndpointNetworkConfig ?

not really i guess. Since this is part of the network type package it would be evident. Not particular about it. I can change if required.

Options in a network api package can easily indicate options of the network object. In the context of the UX/API, I can see an argument to be made for ServiceOption under the network package. If you are okay, pls consider renaming it as ServiceOption.

mavenugo · 2017-05-12T11:37:18Z

opts/network.go

Yes. I agree. There are multiple other CLIs with their CSV options all part of the opts package and are currently used only in docker/cli. I guess moving opts out to docker/cli is its own independent work.

mavenugo · 2017-05-12T11:42:13Z

opts/network.go

Can you instead use the common pattern used in other similar options such as opts/port.go.

longSyntax, err := regexp.MatchString(`\w+=\w+(,\w+=\w+)*`, value)

mavenugo · 2017-05-12T11:43:49Z

opts/network.go

Minor eye-sore... we can follow similar pattern as in opts/port.go.

mavenugo · 2017-05-12T11:45:06Z

ping @thaJeztah

abhi · 2017-05-12T17:39:33Z

@mavenugo addressed the comments.

aaronlehmann · 2017-05-15T20:22:19Z

api/types/network/network.go

Is this structure used at all in the API?

It seems to be used, but was not part of the swarmkit PR.

its used in cli and opts. I read the readme regarding consumers of the api and cli was one of them. So I parked it here.
However since cli and opts is moved to docker/cli repo I will be moving it there and will only be used in the cli.

I think this should be part of opts rather than api.

abhi · 2017-05-17T00:41:30Z

@aaronlehmann I have addressed the comments. PTAL

aaronlehmann · 2017-05-17T00:46:00Z

This is still vendoring your fork of swarmkit. Does it need commits that are not on master?

abhi · 2017-05-17T00:53:49Z

@aaronlehmann just updated the vendoring. I was waiting for a PR to be merged in swarmkit. Its merged now.

aaronlehmann · 2017-05-17T00:58:17Z

Let's merge #32981 first - that will handle the vendoring update

mavenugo · 2017-05-17T01:19:45Z

@aaronlehmann I think #32981 might take more time and based on the comments, I think it might require some more code changes. I think we should take the vendoring via this PR and let the other one rebase. wdyt ?

-- edit --

@aaronlehmann @abhinandanpb yes. Based on the CI failures I think it is better to wait for #32981. It has multiple dependencies on swarmkit and libnetwork. and it is better to have it via #32981.

aaronlehmann · 2017-05-17T01:24:51Z

Sure. To get CI to pass, this will have to take the changes to vendor.conf from #32981.

thaJeztah · 2017-05-17T22:25:44Z

changes itself LGTM

mavenugo · 2017-05-18T00:47:32Z

@abhinandanpb can you pls rebase this PR on top of d618b56 from https://github.com/aboch/docker ?
That will get the CI moving in parallel to #32981 instead of serializing it one after another.

The commit adds capability to accept csv parameters for network option in service create/update commands.The change includes name,alias driver options specific to the network. With this the following will be supported docker service create --name web --network name=docknet,alias=web1,driver-opt=field1=value1 nginx docker service create --name web --network docknet nginx docker service update web --network-add name=docknet,alias=web1,driver-opt=field1=value1 docker service update web --network-rm docknet Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

aaronlehmann

LGTM

mavenugo

LGTM

If a service alias is copied to task, then the DNS resolution on the service name will resolve to service VIP and all of Task-IPs and that will break the concept of vip based load-balancing resulting in all the dns-rr caching issues. This is a regression introduced in moby#33130 Signed-off-by: Madhu Venugopal <madhu@docker.com>

GordonTheTurtle added the status/0-triage label May 10, 2017

abhi force-pushed the network-opt branch 3 times, most recently from d290f63 to 4b199c5 Compare May 10, 2017 18:03

aaronlehmann suggested changes May 10, 2017

View reviewed changes

abhi force-pushed the network-opt branch 3 times, most recently from 7ce5e1c to 3840111 Compare May 11, 2017 06:02

abhi mentioned this pull request May 11, 2017

Adding csv format parameters to network option in service create/update #32975

Closed

thaJeztah added status/2-code-review and removed status/0-triage labels May 11, 2017

thaJeztah added this to the 17.06.0 milestone May 11, 2017

aaronlehmann added the status/needs-vendoring label May 11, 2017

mavenugo reviewed May 12, 2017

View reviewed changes

mavenugo mentioned this pull request May 12, 2017

Grpc change for network options in service spec moby/swarmkit#2176

Merged

abhi force-pushed the network-opt branch from 3840111 to 0528256 Compare May 12, 2017 14:44

abhi mentioned this pull request May 12, 2017

Adding network specific option in csv format for service create/update docker/cli#62

Merged

abhi force-pushed the network-opt branch 2 times, most recently from 241e4e5 to 1e55f65 Compare May 15, 2017 17:50

aaronlehmann reviewed May 15, 2017

View reviewed changes

abhi force-pushed the network-opt branch 7 times, most recently from b57d340 to 8463f78 Compare May 16, 2017 14:02

tiborvass mentioned this pull request May 16, 2017

Adding csv format parameters to network option in service create/update docker/cli#47

Closed

1 task

abhi force-pushed the network-opt branch from 8463f78 to 88f6786 Compare May 17, 2017 00:52

aaronlehmann added status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/needs-vendoring labels May 17, 2017

abhi force-pushed the network-opt branch 2 times, most recently from 5cfee8e to 92ad957 Compare May 18, 2017 01:28

abhi added 2 commits May 17, 2017 18:46

swarmkit vendoring

f3d8ff7

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi force-pushed the network-opt branch from 92ad957 to f3d8ff7 Compare May 18, 2017 01:46

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 18, 2017

aaronlehmann approved these changes May 18, 2017

View reviewed changes

mavenugo approved these changes May 18, 2017

View reviewed changes

mavenugo added status/4-merge and removed status/2-code-review labels May 18, 2017

mavenugo merged commit 45c6f42 into moby:master May 18, 2017

thaJeztah added the impact/changelog label May 18, 2017

mavenugo mentioned this pull request Jun 7, 2017

Service alias should not be copied to task alias #33578

Merged

abhi mentioned this pull request Jul 20, 2017

Fixing issue with driver opt not passed to drivers #34194

Merged

This was referenced Oct 5, 2017

--network-alias option missing on docker service create (as well as corresponding options on docker service update) #24787

Closed

RFC: add advanced "csv" syntax for "--net" / "--network" #31964

Open

sepich mentioned this pull request Jan 27, 2018

Document long form of --network and --network-add docker/cli#843

Merged

Adding network specific options to service create/update #33130

Adding network specific options to service create/update #33130

Uh oh!

Conversation

abhi commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann left a comment

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!

mavenugo commented May 12, 2017

Uh oh!

abhi commented May 12, 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!

abhi commented May 17, 2017

Uh oh!

aaronlehmann commented May 17, 2017

Uh oh!

abhi commented May 17, 2017

Uh oh!

aaronlehmann commented May 17, 2017

Uh oh!

mavenugo commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented May 17, 2017

Uh oh!

thaJeztah commented May 17, 2017

Uh oh!

mavenugo commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann left a comment

Choose a reason for hiding this comment

abhi commented May 10, 2017 •

edited

Loading

mavenugo commented May 17, 2017 •

edited

Loading

mavenugo commented May 18, 2017 •

edited

Loading