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

Support service network attachment changes #32062

Merged
merged 3 commits into from Apr 10, 2017

Conversation

@aaronlehmann
Contributor

aaronlehmann commented Mar 24, 2017

  • Vendor a new version of swarmkit that allows service network attachments to change
  • Add --network-add and --network-rm options to docker service update
  • Resolve network IDs on the client side (see #32060). Resolution in the daemon is left in place for backward compatibility.
  • Avoid filling in deprecated Spec.Networks field
  • Sort networks in the TaskSpec for update stability
  • Add an integration test for changing service networks

Fixes #28247

cc @dhiltgen @mavenugo @aluzzardi @yongtang

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Mar 24, 2017

Member

LGTM

Might want to try stack deploy

Member

aluzzardi commented Mar 24, 2017

LGTM

Might want to try stack deploy

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 25, 2017

Contributor

Tested this with stack deploy, and it didn't work as expected because stack deploy is using the deprecated ServiceSpec.Networks field, instead of TaskTemplate.Networks.

I've added a commit that uses TaskTemplate.Networks instead when the API version is recent enough. This change can't be unconditional, because it causes the client to move the networks of stack-related services from ServiceSpec to TaskTemplate, and this is only allowed by engines that have the swarmkit change included in this PR.

cc @dnephin

Contributor

aaronlehmann commented Mar 25, 2017

Tested this with stack deploy, and it didn't work as expected because stack deploy is using the deprecated ServiceSpec.Networks field, instead of TaskTemplate.Networks.

I've added a commit that uses TaskTemplate.Networks instead when the API version is recent enough. This change can't be unconditional, because it causes the client to move the networks of stack-related services from ServiceSpec to TaskTemplate, and this is only allowed by engines that have the swarmkit change included in this PR.

cc @dnephin

Show outdated Hide outdated cli/command/service/opts.go Outdated
@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Mar 27, 2017

Contributor

@aaronlehmann #31714 is merged now. pls rebase

Contributor

mavenugo commented Mar 27, 2017

@aaronlehmann #31714 is merged now. pls rebase

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 27, 2017

Member

Code LGTM.

Avoid filling deprecated Spec.Networks field helps a lot in the future. There were many confusions about this part from user as far as I could remember.

Member

yongtang commented Mar 27, 2017

Code LGTM.

Avoid filling deprecated Spec.Networks field helps a lot in the future. There were many confusions about this part from user as far as I could remember.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 27, 2017

Contributor

Rebased and addressed comments.

Contributor

aaronlehmann commented Mar 27, 2017

Rebased and addressed comments.

@vdemeester

code LGTM 🐸

if err != nil {
return nil, err
}
nets = append(nets, swarm.NetworkAttachmentConfig{Target: network.ID})

This comment has been minimized.

@dnephin

dnephin Mar 27, 2017

Member

Sorry, I'm -1 on this change and on #32060 in general. The engine API handles this already so that every client doesn't need to handle it.

I also don't think that the convert code should be making API calls.

@dnephin

dnephin Mar 27, 2017

Member

Sorry, I'm -1 on this change and on #32060 in general. The engine API handles this already so that every client doesn't need to handle it.

I also don't think that the convert code should be making API calls.

This comment has been minimized.

@aaronlehmann

aaronlehmann Mar 27, 2017

Contributor

The client is preparing a spec here, which serves as configuration for the service. It's quite unexpected for the engine to be rewriting the spec before storing it. I see it as a analagous to docker compose up rewriting the compose file on disk.

In theory this change could be split out to a different PR, but I feel strongly that we shouldn't be relying on the engine to magically rewrite the service's configuration, which is supposed to be completely determined by the client. We're effectively overloading a field that's defined to contain a network ID, and saying that it's okay to put a network name there in certain cases, because that field will be rewritten before it's acted upon.

More details in #32060 (comment)

@aaronlehmann

aaronlehmann Mar 27, 2017

Contributor

The client is preparing a spec here, which serves as configuration for the service. It's quite unexpected for the engine to be rewriting the spec before storing it. I see it as a analagous to docker compose up rewriting the compose file on disk.

In theory this change could be split out to a different PR, but I feel strongly that we shouldn't be relying on the engine to magically rewrite the service's configuration, which is supposed to be completely determined by the client. We're effectively overloading a field that's defined to contain a network ID, and saying that it's okay to put a network name there in certain cases, because that field will be rewritten before it's acted upon.

More details in #32060 (comment)

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

If it's an issue, can we go ahead and split it out?
Also we can probably optimize this by hitting NetworkList with filters for the network names.

@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

If it's an issue, can we go ahead and split it out?
Also we can probably optimize this by hitting NetworkList with filters for the network names.

This comment has been minimized.

@dnephin

dnephin Apr 6, 2017

Member

I see it as a analagous to docker compose up rewriting the compose file on disk.

I don't really see it that way. The user controlled data is the command line they run. Either way "something" takes the names they provide and turns them into IDs. What does it matter to the user if that's done in the client or the daemon?

If it's done in the client then every single API user has to duplicate the lookup logic.

Is there any practical advantage to doing it in the engine client vs the daemon (which is acting the swarmkit client)?

@dnephin

dnephin Apr 6, 2017

Member

I see it as a analagous to docker compose up rewriting the compose file on disk.

I don't really see it that way. The user controlled data is the command line they run. Either way "something" takes the names they provide and turns them into IDs. What does it matter to the user if that's done in the client or the daemon?

If it's done in the client then every single API user has to duplicate the lookup logic.

Is there any practical advantage to doing it in the engine client vs the daemon (which is acting the swarmkit client)?

}
return nets
sort.Sort(byNetworkTarget(nets))

This comment has been minimized.

@dnephin

dnephin Mar 27, 2017

Member

The input is already a slice, why do we need to sort them?

If the order isn't relevant then the server which processes this data should perform a sort before operating on it.

@dnephin

dnephin Mar 27, 2017

Member

The input is already a slice, why do we need to sort them?

If the order isn't relevant then the server which processes this data should perform a sort before operating on it.

This comment has been minimized.

@aaronlehmann

aaronlehmann Mar 27, 2017

Contributor

Arguably you have a point here. We sort these so that the manager won't see two tasks with the same set of networks as different. Right now the manager is doing a simple reflect.DeepEqual on the TaskSpec. Ideally, the manager would do this comparison in a smarter way. That said, an order-insensitive comparison might end up being inefficient. We might have to pay the overhead of sorting on every comparison. But reflect is very inefficient to begin with, so extra overhead from sorting may be comparatively small.

Since this would be a somewhat tangential change in swarmkit, I suggest that we leave in the sort for the moment, and consider removing the need for it through a separate change. Feel free to open an issue and assign it to me.

@aaronlehmann

aaronlehmann Mar 27, 2017

Contributor

Arguably you have a point here. We sort these so that the manager won't see two tasks with the same set of networks as different. Right now the manager is doing a simple reflect.DeepEqual on the TaskSpec. Ideally, the manager would do this comparison in a smarter way. That said, an order-insensitive comparison might end up being inefficient. We might have to pay the overhead of sorting on every comparison. But reflect is very inefficient to begin with, so extra overhead from sorting may be comparatively small.

Since this would be a somewhat tangential change in swarmkit, I suggest that we leave in the sort for the moment, and consider removing the need for it through a separate change. Feel free to open an issue and assign it to me.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

Wouldn't also be less overhead for the server if the slice is pre-sorted, since it won't have to actually move anything around even if it does call sort?

@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

Wouldn't also be less overhead for the server if the slice is pre-sorted, since it won't have to actually move anything around even if it does call sort?

Show outdated Hide outdated integration-cli/docker_cli_swarm_test.go Outdated
Show outdated Hide outdated cli/compose/convert/service.go Outdated
Show outdated Hide outdated cli/command/service/update.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 27, 2017

Member

I tested manually on --network-add and --network-rm parts, and they work as expected.

Member

yongtang commented Mar 27, 2017

I tested manually on --network-add and --network-rm parts, and they work as expected.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 31, 2017

Member

@aaronlehmann needs a rebase 👼 🙇

Member

vdemeester commented Mar 31, 2017

@aaronlehmann needs a rebase 👼 🙇

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 31, 2017

Contributor

Rebased

Contributor

aaronlehmann commented Mar 31, 2017

Rebased

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 3, 2017

Contributor

ping @dnephin: Is the switch from submitting network names on the client side to using network IDs still a concern?

The issue about client side vs. server side sorting is being handled in docker/swarmkit#2065, but I don't expect that will make 17.05 at this point.

Contributor

aaronlehmann commented Apr 3, 2017

ping @dnephin: Is the switch from submitting network names on the client side to using network IDs still a concern?

The issue about client side vs. server side sorting is being handled in docker/swarmkit#2065, but I don't expect that will make 17.05 at this point.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 6, 2017

Member

Started doing some testing

Init swarm, create two networks, the second having the first one's ID as name (heh,
always worth testing);

docker swarm init
docker pull nginx:alpine

docker network create -d overlay foo
jnb6r5655lgauwaxc2xk3bjkg

docker network create -d overlay jnb6r5655lgauwaxc2xk3bjkg
o53gxh9we29bzcwszsi53t4h0

docker network ls
NETWORK ID          NAME                        DRIVER              SCOPE
f23f49c32e1a        bridge                      bridge              local
b05cf8bbb91a        docker_gwbridge             bridge              local
jnb6r5655lga        foo                         overlay             swarm
1a4bcf679928        host                        host                local
7frgm3zxl00t        ingress                     overlay             swarm
o53gxh9we29b        jnb6r5655lgauwaxc2xk3bjkg   overlay             swarm
193c0931de9a        none                        null                local

Create a service, attaching to network by ID:

docker service create --name web --network=jnb6r5655lgauwaxc2xk3bjkg nginx:alpine
uyjck7mt1ee6r4au1ak5l6508

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

That looks right 👍

I did notice this (network name nor ID is no longer in .Spec.Networks);

docker service inspect -f '{{json .Spec.Networks }}'  web | jq .
null

Remove the network from the service (looks OK);

docker service update --network-rm=jnb6r5655lgauwaxc2xk3bjkg web
web

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
null

Doing again, but by "name";

docker service create --name web2 --network=foo nginx:alpine
uazr3rgs0anvfe8gilfjhlmb3

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web2 | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

Here also, the network name as specified by the user is not to be found:

docker service inspect -f '{{json .Spec.Networks }}'  web2 | jq .
null

docker service inspect --pretty web2

ID:		uazr3rgs0anvfe8gilfjhlmb3
Name:		web2
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Max failure ratio: 0
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Max failure ratio: 0
ContainerSpec:
 Image:		nginx:alpine@sha256:b65b240c4d8418167731f6c82544c6e7d29655530e4ed26c9f5eb27efba269af
Resources:
Endpoint Mode:	vip
Member

thaJeztah commented Apr 6, 2017

Started doing some testing

Init swarm, create two networks, the second having the first one's ID as name (heh,
always worth testing);

docker swarm init
docker pull nginx:alpine

docker network create -d overlay foo
jnb6r5655lgauwaxc2xk3bjkg

docker network create -d overlay jnb6r5655lgauwaxc2xk3bjkg
o53gxh9we29bzcwszsi53t4h0

docker network ls
NETWORK ID          NAME                        DRIVER              SCOPE
f23f49c32e1a        bridge                      bridge              local
b05cf8bbb91a        docker_gwbridge             bridge              local
jnb6r5655lga        foo                         overlay             swarm
1a4bcf679928        host                        host                local
7frgm3zxl00t        ingress                     overlay             swarm
o53gxh9we29b        jnb6r5655lgauwaxc2xk3bjkg   overlay             swarm
193c0931de9a        none                        null                local

Create a service, attaching to network by ID:

docker service create --name web --network=jnb6r5655lgauwaxc2xk3bjkg nginx:alpine
uyjck7mt1ee6r4au1ak5l6508

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

That looks right 👍

I did notice this (network name nor ID is no longer in .Spec.Networks);

docker service inspect -f '{{json .Spec.Networks }}'  web | jq .
null

Remove the network from the service (looks OK);

docker service update --network-rm=jnb6r5655lgauwaxc2xk3bjkg web
web

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
null

Doing again, but by "name";

docker service create --name web2 --network=foo nginx:alpine
uazr3rgs0anvfe8gilfjhlmb3

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web2 | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

Here also, the network name as specified by the user is not to be found:

docker service inspect -f '{{json .Spec.Networks }}'  web2 | jq .
null

docker service inspect --pretty web2

ID:		uazr3rgs0anvfe8gilfjhlmb3
Name:		web2
Service Mode:	Replicated
 Replicas:	1
Placement:
UpdateConfig:
 Parallelism:	1
 On failure:	pause
 Max failure ratio: 0
RollbackConfig:
 Parallelism:	1
 On failure:	pause
 Max failure ratio: 0
ContainerSpec:
 Image:		nginx:alpine@sha256:b65b240c4d8418167731f6c82544c6e7d29655530e4ed26c9f5eb27efba269af
Resources:
Endpoint Mode:	vip
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 6, 2017

Member

The original (as specified by the user) name of the network to not be included in the spec is a regression from 17.03/17.04. I think we should preserve both a name and an ID somewhere in the spec (ideally, in such a way that the relation is clear).

edit I see the comment here https://github.com/docker/docker/pull/32062/files#diff-dc2678e1e65c05f198d9b84656ed2f00R139 that .Spec.Networks was deprecated. Which lead to the question;

  • what happens when downgrading a daemon to the previous version? (perhaps resolve the name, and set both .Spec.Networks and .Spec.TaskTemplate.Networks, would that work?)
  • we still need a way for the user to show which network(s) a service is connected to, other than only seeing an "id" (see my comment above)
Member

thaJeztah commented Apr 6, 2017

The original (as specified by the user) name of the network to not be included in the spec is a regression from 17.03/17.04. I think we should preserve both a name and an ID somewhere in the spec (ideally, in such a way that the relation is clear).

edit I see the comment here https://github.com/docker/docker/pull/32062/files#diff-dc2678e1e65c05f198d9b84656ed2f00R139 that .Spec.Networks was deprecated. Which lead to the question;

  • what happens when downgrading a daemon to the previous version? (perhaps resolve the name, and set both .Spec.Networks and .Spec.TaskTemplate.Networks, would that work?)
  • we still need a way for the user to show which network(s) a service is connected to, other than only seeing an "id" (see my comment above)
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 6, 2017

Member

Combine add and remove in a single update;

docker service create --name web --network=foo nginx:alpine
9vlyasko7cspst2py2kjgzoy4

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

docker service update --network-add=foo --network-rm=foo web
web

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
null

That's not correct; we should do remove before add, so that removing/adding the network in the same update becomes a no-op. In this case, it's not a very useful thing to do, but if we add "advanced" options to networks, and want to update options, the network should not be removed but updated (--network-rm=foo --network-add=name=foo,option=bar)

Member

thaJeztah commented Apr 6, 2017

Combine add and remove in a single update;

docker service create --name web --network=foo nginx:alpine
9vlyasko7cspst2py2kjgzoy4

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
[
  {
    "Target": "jnb6r5655lgauwaxc2xk3bjkg"
  }
]

docker service update --network-add=foo --network-rm=foo web
web

docker service inspect -f '{{json .Spec.TaskTemplate.Networks }}'  web | jq .
null

That's not correct; we should do remove before add, so that removing/adding the network in the same update becomes a no-op. In this case, it's not a very useful thing to do, but if we add "advanced" options to networks, and want to update options, the network should not be removed but updated (--network-rm=foo --network-add=name=foo,option=bar)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 6, 2017

Member

FWIW, I did a similar change for "secrets"; #29802

Member

thaJeztah commented Apr 6, 2017

FWIW, I did a similar change for "secrets"; #29802

Show outdated Hide outdated integration-cli/docker_cli_swarm_test.go Outdated
if err != nil {
return nil, err
}
nets = append(nets, swarm.NetworkAttachmentConfig{Target: network.ID})

This comment has been minimized.

@dnephin

dnephin Apr 6, 2017

Member

I see it as a analagous to docker compose up rewriting the compose file on disk.

I don't really see it that way. The user controlled data is the command line they run. Either way "something" takes the names they provide and turns them into IDs. What does it matter to the user if that's done in the client or the daemon?

If it's done in the client then every single API user has to duplicate the lookup logic.

Is there any practical advantage to doing it in the engine client vs the daemon (which is acting the swarmkit client)?

@dnephin

dnephin Apr 6, 2017

Member

I see it as a analagous to docker compose up rewriting the compose file on disk.

I don't really see it that way. The user controlled data is the command line they run. Either way "something" takes the names they provide and turns them into IDs. What does it matter to the user if that's done in the client or the daemon?

If it's done in the client then every single API user has to duplicate the lookup logic.

Is there any practical advantage to doing it in the engine client vs the daemon (which is acting the swarmkit client)?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

edit I see the comment here https://github.com/docker/docker/pull/32062/files#diff-dc2678e1e65c05f198d9b84656ed2f00R139 that .Spec.Networks was deprecated. Which lead to the question;

  • what happens when downgrading a daemon to the previous version? (perhaps resolve the name, and set both .Spec.Networks and .Spec.TaskTemplate.Networks, would that work?)

.Spec.Networks was deprecated before 1.12.0 final. We left support for this in swarmkit to support upgrades from -rc versions, but the swarmkit team didn't expect .Spec.Networks to continue to be set by the CLI. We were surprised to find that it was still being used at all.

  • we still need a way for the user to show which network(s) a service is connected to, other than only seeing an "id" (see my comment above)

I'm not sure I understand this one correctly. Is this a regression with this PR? In the old code, the network name was being sent to the REST API, but my understanding is that the daemon would rewrite it to an ID before storing the service in swarmkit's data store. Was it possible to see the network name before this PR?

Contributor

aaronlehmann commented Apr 6, 2017

edit I see the comment here https://github.com/docker/docker/pull/32062/files#diff-dc2678e1e65c05f198d9b84656ed2f00R139 that .Spec.Networks was deprecated. Which lead to the question;

  • what happens when downgrading a daemon to the previous version? (perhaps resolve the name, and set both .Spec.Networks and .Spec.TaskTemplate.Networks, would that work?)

.Spec.Networks was deprecated before 1.12.0 final. We left support for this in swarmkit to support upgrades from -rc versions, but the swarmkit team didn't expect .Spec.Networks to continue to be set by the CLI. We were surprised to find that it was still being used at all.

  • we still need a way for the user to show which network(s) a service is connected to, other than only seeing an "id" (see my comment above)

I'm not sure I understand this one correctly. Is this a regression with this PR? In the old code, the network name was being sent to the REST API, but my understanding is that the daemon would rewrite it to an ID before storing the service in swarmkit's data store. Was it possible to see the network name before this PR?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Apr 6, 2017

Contributor

LGTM

Contributor

stevvooe commented Apr 6, 2017

LGTM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 6, 2017

Member

It's also unexpected that if you submit a service spec and later inspect it, you see your network name has magically been replaced with a network ID.

I don't think this PR changes that behaviour. You submit something from the CLI with a name, and you the inspect shows an ID. Is that not the case?

Member

dnephin commented Apr 6, 2017

It's also unexpected that if you submit a service spec and later inspect it, you see your network name has magically been replaced with a network ID.

I don't think this PR changes that behaviour. You submit something from the CLI with a name, and you the inspect shows an ID. Is that not the case?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 6, 2017

Member

I'm not sure I understand this one correctly. Is this a regression with this PR? In the old code, the network name was being sent to the REST API, but my understanding is that the daemon would rewrite it to an ID before storing the service in swarmkit's data store. Was it possible to see the network name before this PR?

Yes, if I run in docker 17.04, I see the network name in service inspect. I think that's useful information to have because as a used, I'd be interested in what networks my services are connected to

Member

thaJeztah commented Apr 6, 2017

I'm not sure I understand this one correctly. Is this a regression with this PR? In the old code, the network name was being sent to the REST API, but my understanding is that the daemon would rewrite it to an ID before storing the service in swarmkit's data store. Was it possible to see the network name before this PR?

Yes, if I run in docker 17.04, I see the network name in service inspect. I think that's useful information to have because as a used, I'd be interested in what networks my services are connected to

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

Yes, if I run in docker 17.04, I see the network name in service inspect. I think that's useful information to have because as a used, I'd be interested in what networks my services are connected to

Oh, I think this is because the old CLI code wrote to both the deprecated field and the non-deprecated field. The code in the engine that translates an ID to a name would only translate the non-deprecated field if both were present. So you would still see the name in the deprecated field.

Contributor

aaronlehmann commented Apr 6, 2017

Yes, if I run in docker 17.04, I see the network name in service inspect. I think that's useful information to have because as a used, I'd be interested in what networks my services are connected to

Oh, I think this is because the old CLI code wrote to both the deprecated field and the non-deprecated field. The code in the engine that translates an ID to a name would only translate the non-deprecated field if both were present. So you would still see the name in the deprecated field.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

I don't think this PR changes that behaviour. You submit something from the CLI with a name, and you the inspect shows an ID. Is that not the case?

I'm talking about the API behavior, not the CLI behavior.

And the PR doesn't change the API's behavior (to preserve backward compatibility), but I think it's better not to rely on this behavior and be explicit about submitting IDs in places that really should have IDs.

Contributor

aaronlehmann commented Apr 6, 2017

I don't think this PR changes that behaviour. You submit something from the CLI with a name, and you the inspect shows an ID. Is that not the case?

I'm talking about the API behavior, not the CLI behavior.

And the PR doesn't change the API's behavior (to preserve backward compatibility), but I think it's better not to rely on this behavior and be explicit about submitting IDs in places that really should have IDs.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

@thaJeztah: I've updated the CLI to apply network removals before network additions. However, I noticed that for everything except secrets, we seem to use the opposite approach. So if you feel this is wrong, we should have a separate PR that fixes these cases. Feel free to open an issue.

As for visibility of network names, the object model is defined so that tasks reference networks by IDs. The fact that a network name was appearing the spec before was an error caused by the daemon only rewriting one of the two fields the CLI inserted (another reason I'm not a fan of rewriting fields in the daemon). I propose that service inspect --pretty translate the IDs to names for display. I'll work on a separate PR to do this, since I feel it's out of scope for this one. Does that sound good?

Contributor

aaronlehmann commented Apr 7, 2017

@thaJeztah: I've updated the CLI to apply network removals before network additions. However, I noticed that for everything except secrets, we seem to use the opposite approach. So if you feel this is wrong, we should have a separate PR that fixes these cases. Feel free to open an issue.

As for visibility of network names, the object model is defined so that tasks reference networks by IDs. The fact that a network name was appearing the spec before was an error caused by the daemon only rewriting one of the two fields the CLI inserted (another reason I'm not a fan of rewriting fields in the daemon). I propose that service inspect --pretty translate the IDs to names for display. I'll work on a separate PR to do this, since I feel it's out of scope for this one. Does that sound good?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

As for visibility of network names, the object model is defined so that tasks reference networks by IDs. The fact that a network name was appearing the spec before was an error caused by the daemon only rewriting one of the two fields the CLI inserted (another reason I'm not a fan of rewriting fields in the daemon). I propose that service inspect --pretty translate the IDs to names for display. I'll work on a separate PR to do this, since I feel it's out of scope for this one. Does that sound good?

Actually, I decided to add it to this PR, since I realized docker service inspect --pretty isn't showing useful network info without this change. PTAL.

Contributor

aaronlehmann commented Apr 7, 2017

As for visibility of network names, the object model is defined so that tasks reference networks by IDs. The fact that a network name was appearing the spec before was an error caused by the daemon only rewriting one of the two fields the CLI inserted (another reason I'm not a fan of rewriting fields in the daemon). I propose that service inspect --pretty translate the IDs to names for display. I'll work on a separate PR to do this, since I feel it's out of scope for this one. Does that sound good?

Actually, I decided to add it to this PR, since I realized docker service inspect --pretty isn't showing useful network info without this change. PTAL.

aaronlehmann added some commits Mar 24, 2017

cli: Allow service's networks to be updated
Resolve networks IDs on the client side.

Avoid filling in deprecated Spec.Networks field.

Sort networks in the TaskSpec for update stability.

Add an integration test for changing service networks.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
cli: Deploying a compose file must use TaskTemplate.Networks
This is the non-deprecated field, and the one that can be changed in a
service update.

Since old daemon versions don't allow migrating from one field to the
other, make this conditional on the API version.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Show network names in "docker service inspect --pretty"
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 8, 2017

Member

thanks! that satisfies my concerns; not able to review code right now, but if it has enough LGTM's, don't let me stop this

Member

thaJeztah commented Apr 8, 2017

thanks! that satisfies my concerns; not able to review code right now, but if it has enough LGTM's, don't let me stop this

@thaJeztah

LGTM, thanks!

As a follow up, we could consider adding the names to the "non-pretty" case as well, for people that want to use --format to print names 😃

@thaJeztah thaJeztah merged commit b0831ac into moby:master Apr 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32713 has succeeded
Details
janky Jenkins build Docker-PRs 41324 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1508 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12444 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1348 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

@aaronlehmann aaronlehmann deleted the aaronlehmann:change-network-attachments branch Apr 11, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32062 from aaronlehmann/change-network-attach…
…ments

Support service network attachment changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment