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 option in csv format for service create/update #62

Merged
merged 3 commits into from May 18, 2017

Conversation

Projects
None yet
@abhi
Member

abhi commented May 10, 2017

- What I did
Changed the network option to accept csv format values in service/create update

- How I did it
Created a new networkOpt type which accepts csv format values

- 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

The cli change 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.

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

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 12, 2017

Contributor

ping @thaJeztah

Contributor

mavenugo commented May 12, 2017

ping @thaJeztah

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 12, 2017

Contributor

@abhinandanpb can you pls add a link to the moby PR that carries the API and backend changes ?

Contributor

mavenugo commented May 12, 2017

@abhinandanpb can you pls add a link to the moby PR that carries the API and backend changes ?

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 12, 2017

Contributor

@abhinandanpb am not a compose/stack expert. But I think we should support the new network csv format and driver-opt in compose format as well. am not sure what is the best way to get that done.
cc @dnephin @vdemeester can you pls advice ?

Contributor

mavenugo commented May 12, 2017

@abhinandanpb am not a compose/stack expert. But I think we should support the new network csv format and driver-opt in compose format as well. am not sure what is the best way to get that done.
cc @dnephin @vdemeester can you pls advice ?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 12, 2017

Collaborator

I believe we already support these fields in Compose. Was there a new field added, or is this just a way to have service create auto-create a network?

CSV structures on the CLI are just a hack because we don't have any other way of representing nested/structured data. In a YAML file we have an actual structure, so we don't need this hack.

Collaborator

dnephin commented May 12, 2017

I believe we already support these fields in Compose. Was there a new field added, or is this just a way to have service create auto-create a network?

CSV structures on the CLI are just a hack because we don't have any other way of representing nested/structured data. In a YAML file we have an actual structure, so we don't need this hack.

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi May 12, 2017

Member

@dnephin new option is not added rather the existing --network,--network-add option is updated to accept csv. I believe it will need some change on compose side to tie it to the backend structs.

Member

abhi commented May 12, 2017

@dnephin new option is not added rather the existing --network,--network-add option is updated to accept csv. I believe it will need some change on compose side to tie it to the backend structs.

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi May 12, 2017

Member

The change corresponds to the PRs in moby/moby#33130 and docker/swarmkit#2176

Member

abhi commented May 12, 2017

The change corresponds to the PRs in moby/moby#33130 and docker/swarmkit#2176

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 12, 2017

Collaborator

We already supported creating networks as part of docker stack deploy, so I'm not sure we need to change anything.

Collaborator

dnephin commented May 12, 2017

We already supported creating networks as part of docker stack deploy, so I'm not sure we need to change anything.

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi May 12, 2017

Member

@dnephin I probably should have clarified one thing. Previously service create/update was supporting only network name as a part of --network option. Since with this PR we will start supporting csv - we will support --network name=docknet,alias=web-prod,driver-opt=encap=vlan .
So I guess I need to make change here to convert -https://github.com/docker/cli/blob/master/cli/compose/convert/service.go#L186 .
WDYT ?

Member

abhi commented May 12, 2017

@dnephin I probably should have clarified one thing. Previously service create/update was supporting only network name as a part of --network option. Since with this PR we will start supporting csv - we will support --network name=docknet,alias=web-prod,driver-opt=encap=vlan .
So I guess I need to make change here to convert -https://github.com/docker/cli/blob/master/cli/compose/convert/service.go#L186 .
WDYT ?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 12, 2017

Collaborator

I don't think we want to add any fields to the compose format for this. We already supported creating networks with all the options on docker stack deploy.

The only change we might want to make is to have the networks created as part of the service create API call, instead of as separate network create API calls. I'm not sure if there are any significant advantages (or disadvantages) to this.

Collaborator

dnephin commented May 12, 2017

I don't think we want to add any fields to the compose format for this. We already supported creating networks with all the options on docker stack deploy.

The only change we might want to make is to have the networks created as part of the service create API call, instead of as separate network create API calls. I'm not sure if there are any significant advantages (or disadvantages) to this.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi May 13, 2017

Contributor

@dnephin No I think what this is doing is stashing network options IN services. As in, API level.

I'm not sure I understand why though?

Contributor

aluzzardi commented May 13, 2017

@dnephin No I think what this is doing is stashing network options IN services. As in, API level.

I'm not sure I understand why though?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 13, 2017

Collaborator

Yes, that was my understanding. Isn't it doing this as a way to auto-create a network with these options when the service is created? That was my assumption.

Collaborator

dnephin commented May 13, 2017

Yes, that was my understanding. Isn't it doing this as a way to auto-create a network with these options when the service is created? That was my assumption.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi May 13, 2017

Contributor

@dnephin I don't think so (I'm trying to understand the swarmkit PR which led me back here). I think those are network options specific to how a particular service will connect to a given network.

Contributor

aluzzardi commented May 13, 2017

@dnephin I don't think so (I'm trying to understand the swarmkit PR which led me back here). I think those are network options specific to how a particular service will connect to a given network.

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 13, 2017

Contributor

@dnephin @aluzzardi these are network specific options to be sent to the drivers per container/task/service. This is used by the plugins for various reasons. This is one of the most popular request from many of the network plugin users.

Contributor

mavenugo commented May 13, 2017

@dnephin @aluzzardi these are network specific options to be sent to the drivers per container/task/service. This is used by the plugins for various reasons. This is one of the most popular request from many of the network plugin users.

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 13, 2017

Contributor

@dnephin @aluzzardi fyi : moby/moby#27638 ... this is the original PR that kick started all this.

Contributor

mavenugo commented May 13, 2017

@dnephin @aluzzardi fyi : moby/moby#27638 ... this is the original PR that kick started all this.

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi May 13, 2017

Member

@dnephin @aluzzardi and this is more of a blanket issue moby/moby#31964 which is also addressed.

Member

abhi commented May 13, 2017

@dnephin @aluzzardi and this is more of a blanket issue moby/moby#31964 which is also addressed.

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi May 17, 2017

Member

@dnephin @aluzzardi I was hoping to get this merged. Do you have any additional comments ?

Member

abhi commented May 17, 2017

@dnephin @aluzzardi I was hoping to get this merged. Do you have any additional comments ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2017

Member

Looks like;

Member

thaJeztah commented May 17, 2017

Looks like;

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2017

Member

design LGTM (code as well afaics, but needs the moby/moby changes first)

(count it as an LGTM if 💚 )

Member

thaJeztah commented May 17, 2017

design LGTM (code as well afaics, but needs the moby/moby changes first)

(count it as an LGTM if 💚 )

abhi added some commits May 10, 2017

moby vendoring
Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
moving opts to cli repo
Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

@mavenugo mavenugo merged commit 3dfb834 into docker:master May 18, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
dco-signed All commits are signed
@eyz

This comment has been minimized.

Show comment
Hide comment
@eyz

eyz May 30, 2017

I've verified that the CSV network options make it into the resultant container's NetworkSettings.Networks, as follows -

[root@centos7swarmtest01 ~]# docker network create -d overlay docknet
t1aiu9bmupo6uhx8o7lxbce5i

[root@centos7swarmtest01 ~]# docker service create --name web --network name=docknet,alias=web1,driver-opt=field1=value1 nginx
8v9ckdplxjh84s1pl2dgma0da
Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.

[root@centos7swarmtest01 ~]# docker service ls
ID                  NAME                MODE                REPLICAS            IMAGE                                                                                             PORTS
8v9ckdplxjh8        web                 replicated          1/1                 docker.io/library/nginx@sha256:41ad9967ea448d7c2b203c699b429abe1ed5af331cd92533900c6d77490e0268   

[root@centos7swarmtest01 ~]# docker service ps 8v9
ID                  NAME                IMAGE                                                                                             NODE                DESIRED STATE       CURRENT STATE            ERROR               PORTS
il232jickcx6        web.1               docker.io/library/nginx@sha256:41ad9967ea448d7c2b203c699b429abe1ed5af331cd92533900c6d77490e0268   centos7swarmtest01   Running             Running 31 seconds ago  
                     
[root@centos7swarmtest01 ~]# docker ps
CONTAINER ID        IMAGE                                                         COMMAND                  CREATED             STATUS              PORTS                    NAMES
7c88f2648dee        nginx                                                         "nginx -g 'daemon ..."   35 seconds ago      Up 34 seconds       80/tcp                   web.1.il232jickcx65g9t08qg1w6hx

[root@centos7swarmtest01 ~]# docker inspect 7c8 | jq .[0].NetworkSettings.Networks
{
  "docknet": {
    "DriverOpts": {
      "field1": "value1"
    },
    "MacAddress": "02:42:0a:00:00:03",
    "GlobalIPv6PrefixLen": 0,
    "GlobalIPv6Address": "",
    "IPv6Gateway": "",
    "IPAMConfig": {
      "IPv4Address": "10.0.0.3"
    },
    "Links": null,
    "Aliases": [
      "web1",
      "7c88f2648dee"
    ],
    "NetworkID": "t1aiu9bmupo6uhx8o7lxbce5i",
    "EndpointID": "663a8cbe2f1c0eca178217fb218a402fab98dbda6a2a17362a89868cf7769630",
    "Gateway": "",
    "IPAddress": "10.0.0.3",
    "IPPrefixLen": 24
  }
}

eyz commented May 30, 2017

I've verified that the CSV network options make it into the resultant container's NetworkSettings.Networks, as follows -

[root@centos7swarmtest01 ~]# docker network create -d overlay docknet
t1aiu9bmupo6uhx8o7lxbce5i

[root@centos7swarmtest01 ~]# docker service create --name web --network name=docknet,alias=web1,driver-opt=field1=value1 nginx
8v9ckdplxjh84s1pl2dgma0da
Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.

[root@centos7swarmtest01 ~]# docker service ls
ID                  NAME                MODE                REPLICAS            IMAGE                                                                                             PORTS
8v9ckdplxjh8        web                 replicated          1/1                 docker.io/library/nginx@sha256:41ad9967ea448d7c2b203c699b429abe1ed5af331cd92533900c6d77490e0268   

[root@centos7swarmtest01 ~]# docker service ps 8v9
ID                  NAME                IMAGE                                                                                             NODE                DESIRED STATE       CURRENT STATE            ERROR               PORTS
il232jickcx6        web.1               docker.io/library/nginx@sha256:41ad9967ea448d7c2b203c699b429abe1ed5af331cd92533900c6d77490e0268   centos7swarmtest01   Running             Running 31 seconds ago  
                     
[root@centos7swarmtest01 ~]# docker ps
CONTAINER ID        IMAGE                                                         COMMAND                  CREATED             STATUS              PORTS                    NAMES
7c88f2648dee        nginx                                                         "nginx -g 'daemon ..."   35 seconds ago      Up 34 seconds       80/tcp                   web.1.il232jickcx65g9t08qg1w6hx

[root@centos7swarmtest01 ~]# docker inspect 7c8 | jq .[0].NetworkSettings.Networks
{
  "docknet": {
    "DriverOpts": {
      "field1": "value1"
    },
    "MacAddress": "02:42:0a:00:00:03",
    "GlobalIPv6PrefixLen": 0,
    "GlobalIPv6Address": "",
    "IPv6Gateway": "",
    "IPAMConfig": {
      "IPv4Address": "10.0.0.3"
    },
    "Links": null,
    "Aliases": [
      "web1",
      "7c88f2648dee"
    ],
    "NetworkID": "t1aiu9bmupo6uhx8o7lxbce5i",
    "EndpointID": "663a8cbe2f1c0eca178217fb218a402fab98dbda6a2a17362a89868cf7769630",
    "Gateway": "",
    "IPAddress": "10.0.0.3",
    "IPPrefixLen": 24
  }
}
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 30, 2017

Member

Thanks @eyz !

Member

thaJeztah commented May 30, 2017

Thanks @eyz !

abhi added a commit to abhi/cli that referenced this pull request Jul 10, 2017

Cli change to pass driver specific options to docker run
The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's.
The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since docker#62 . This commit extends this support to docker run command as well.
For docker connect command --driver-opt is added to pass driver specific options for the network the container is connecting to.

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

abhi added a commit to abhi/cli that referenced this pull request Jul 10, 2017

Cli change to pass driver specific options to docker run
The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's.
The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since docker#62 . This commit extends this support to docker run command as well.
For docker connect command --driver-opt is added to pass driver specific options for the network the container is connecting to.

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

abhi added a commit to abhi/cli that referenced this pull request Jul 10, 2017

Cli change to pass driver specific options to docker run
The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's.
The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since docker#62 . This commit extends this support to docker run command as well.
For docker connect command --driver-opt is added to pass driver specific options for the network the container is connecting to.

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 17, 2017

Member

ping @abhinandanpb @mstanleyjones looks like this was not yet documented

Member

thaJeztah commented Aug 17, 2017

ping @abhinandanpb @mstanleyjones looks like this was not yet documented

@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks

mistyhacks Aug 17, 2017

Contributor

@abhinandanpb can you please update the service_create.md and service_update.md in this repository to add this info to the correct sections?

Contributor

mistyhacks commented Aug 17, 2017

@abhinandanpb can you please update the service_create.md and service_update.md in this repository to add this info to the correct sections?

@abhi

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi Aug 17, 2017

Member

@mstanleyjones @thaJeztah thanks. I will get this done today

Member

abhi commented Aug 17, 2017

@mstanleyjones @thaJeztah thanks. I will get this done today

@daledude

This comment has been minimized.

Show comment
Hide comment
@daledude

daledude Sep 25, 2017

@abhinandanpb were these doc changes ever pushed to this repo?

daledude commented Sep 25, 2017

@abhinandanpb were these doc changes ever pushed to this repo?

abhi added a commit to abhi/cli that referenced this pull request Feb 22, 2018

Cli change to pass driver specific options to docker run
The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's.
The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since docker#62 . This commit extends this support to docker run command as well.
For docker connect command --driver-opt is added to pass driver specific options for the network the container is connecting to.

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment