Skip to content
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

Adding network specific options to service create/update #33130

Merged
merged 2 commits into from May 18, 2017

Conversation

@abhi
Copy link
Contributor

commented May 10, 2017

- 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:
docker/swarmkit#2176 and docker/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

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

@aaronlehmann
Copy link
Contributor

left a comment

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.

@@ -0,0 +1,100 @@
package opts

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

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

This comment has been minimized.

Copy link
@abhi

abhi May 10, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 12, 2017

Contributor

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.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah May 15, 2017

Member

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

import (
"encoding/csv"
"fmt"
"github.com/docker/docker/api/types/network"

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

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

}

//Copy makes a copy of options
func (o *Options) Copy() *Options {

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

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

@@ -13,6 +13,21 @@ type IPAM struct {
Config []IPAMConfig
}

//Options represents the network options for endpoint creation

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

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

types "github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/pkg/namesgenerator"
swarmapi "github.com/docker/swarmkit/api"
gogotypes "github.com/gogo/protobuf/types"
"strings"

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

strings should not be moved down here.

@@ -3,13 +3,12 @@ package container
import (
"errors"
"fmt"
"github.com/Sirupsen/logrus"

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

logrus should not be moved here.

@@ -478,13 +477,15 @@ func getEndpointConfig(na *api.NetworkAttachment) *network.EndpointSettings {
ipv6 = ip.String()
}
}

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

An extra tab was inserted on this line.

@@ -495,6 +495,8 @@ message NetworkAttachmentConfig {
// preferred. If these addresses are not available then the
// attachment might fail.
repeated string addresses = 3;
//Driver options is a map of driver specific options for the network target
map<string, string> driverOpt = 4;

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

As a protobuf field name, this should be driver_opts.

return err
}
return nil
}(na.DriverOpt, &attachment.DriverOpt)

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

What is this for?

This comment has been minimized.

Copy link
@abhi

abhi May 10, 2017

Author Contributor

I will be making the swarmkit changes and update it.

@@ -13,6 +13,13 @@ type IPAM struct {
Config []IPAMConfig
}

// Options represents the network options for endpoint creation
type Options struct {

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 12, 2017

Contributor

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

This comment has been minimized.

Copy link
@abhi

abhi May 12, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 13, 2017

Contributor

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.

@@ -0,0 +1,100 @@
package opts

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 12, 2017

Contributor

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.

}
var netOpt network.Options

if len(fields) == 1 && !strings.Contains(fields[0], "=") {

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 12, 2017

Contributor

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)
if len(fields) == 1 && !strings.Contains(fields[0], "=") {
//Support legacy non csv format
netOpt.Target = fields[0]
goto attach

This comment has been minimized.

Copy link
@mavenugo

mavenugo May 12, 2017

Contributor

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

@mavenugo

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

ping @thaJeztah

@abhi

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

@mavenugo addressed the comments.

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

@@ -13,6 +13,13 @@ type IPAM struct {
Config []IPAMConfig
}

// EndpointOptions represents the network options for endpoint creation
type EndpointOptions struct {

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 15, 2017

Contributor

Is this structure used at all in the API?

This comment has been minimized.

Copy link
@stevvooe

stevvooe May 15, 2017

Contributor

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

This comment has been minimized.

Copy link
@abhi

abhi May 15, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 15, 2017

Contributor

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

@abhi abhi force-pushed the abhi:network-opt branch 7 times, most recently from b57d340 to 8463f78 May 15, 2017

@abhi

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

@aaronlehmann I have addressed the comments. PTAL

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

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

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

@abhi

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

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

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

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

@mavenugo

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

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

@thaJeztah

This comment has been minimized.

Copy link
Member

commented May 17, 2017

changes itself LGTM

@mavenugo

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

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

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

abhi added 2 commits May 9, 2017
Adding network specific options to service create/update
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>
swarmkit vendoring
Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

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

@aaronlehmann
Copy link
Contributor

left a comment

LGTM

@mavenugo
Copy link
Contributor

left a comment

LGTM

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

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34291 has succeeded
Details
janky Jenkins build Docker-PRs 42892 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3277 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3440 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14133 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2995 has succeeded
Details
mavenugo added a commit to mavenugo/docker that referenced this pull request Jun 7, 2017
Service alias should not be copied to task alias
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.