fix docker inspect return unconsistent network settings of created container and stopped container by coolljt0725 · Pull Request #18531 · moby/moby

coolljt0725 · 2015-12-09T12:31:19Z

To make docker inspect return a consistent result of networksettings
for created container and stopped container, it's better to update
the network settings on container creating. Or else docker inspect a created
container will not return the network settings.

$ docker create --name foo -ti busybox
4b5600da39e0350a3bb872f1d6a790443b327c87a2baa21300cf8e6abc09c85f
docker inspect foo
.
.
"NetworkSettings": {
        "Bridge": "",
        "SandboxID": "",
        "HairpinMode": false,
        "LinkLocalIPv6Address": "",
        "LinkLocalIPv6PrefixLen": 0,
        "Ports": null,
        "SandboxKey": "",
        "SecondaryIPAddresses": null,
        "SecondaryIPv6Addresses": null,
        "EndpointID": "",
        "Gateway": "",
        "GlobalIPv6Address": "",
        "GlobalIPv6PrefixLen": 0,
        "IPAddress": "",
        "IPPrefixLen": 0,
        "IPv6Gateway": "",
        "MacAddress": "",
        "Networks": null
    }

as you can see Networks filed is null, it should has the network bridge, but if start it and then stop it, then docker inspect will return Networks

$ docker start foo
foo
$ docker stop foo
foo
$ docker inspect foo
.
.
"Networks": {
            "bridge": {
                "EndpointID": "",
                "Gateway": "",
                "IPAddress": "",
                "IPPrefixLen": 0,
                "IPv6Gateway": "",
                "GlobalIPv6Address": "",
                "GlobalIPv6PrefixLen": 0,
                "MacAddress": ""
            }
        }

It should have the consistent result.
Signed-off-by: Lei Jitang [email protected]

thaJeztah · 2015-12-09T13:05:05Z

Will (with this change) Networks show all networks that a container was connected to when running, or only "bridge"? i.e., if a container was not connected to "bridge" will it still appear in Networks when stopped? And the reverse; if a container was connected to multiple networks, will all networks show in Networks when stopped?

coolljt0725 · 2015-12-09T14:17:21Z

@thaJeztah This change will only affect the created container.

Will (with this change) Networks show all networks that a container was connected to when running, or only "bridge"

all networks, and nothing to do with this change

i.e., if a container was not connected to "bridge" will it still appear in Networks when stopped?

no, it will disappear, and also nothing to do with this change

And the reverse; if a container was connected to multiple networks, will all networks show in
Networks when stopped

yes, all networks will show, and also nothing to do with this change

cpuguy83 · 2015-12-09T14:39:48Z

ping @aboch @mavenugo Just want to make sure you are good on this.

cpuguy83 · 2015-12-09T14:40:16Z

Design review so the nework team can verify

mavenugo · 2015-12-09T17:11:49Z

Design LGTM especially we need this in order to support multiple --net as discussed in #17796 (comment) (option #2).

But to clarify one of the questions that @thaJeztah asked

i.e., if a container was not connected to "bridge" will it still appear in Networks when stopped?

no, it will disappear, and also nothing to do with this change

The answer depends on the case. If the container is NOT connected to "bridge" but to another network, then it will show up in the Networks when the container is stopped.

I have a few comments on the code though.

To avoid Code duplication, UpdateNetworkSettings must be reused from within allocateNetwork.
If we update the NetworkSettings in Create, can you please make sure it stays if the daemon is restarted ? (I believe we must write the config into the JSON file using container.ToDiskLocking

cpuguy83 · 2015-12-09T17:13:21Z

Ok, moving to code review.
Thanks!

coolljt0725 · 2015-12-10T05:47:35Z

@mavenugo

If we update the NetworkSettings in Create, can you please make sure it stays if the daemon is restarted ? (I believe we must write the config into the JSON file using container.ToDiskLocking

It will stays if the daemon is restarted, because after daemon.UpdateNetworkSettings(container) ,
it is if err := container.ToDiskLocking();
https://github.com/docker/docker/pull/18531/files#diff-72dc1842ed7b5a92b4e98f84456a2c0cR113

coolljt0725 · 2015-12-10T10:46:23Z

@mavenugo updated

mavenugo · 2015-12-12T05:14:45Z

daemon/container_operations_unix.go

By moving the functionality into UpdateNetworkSettings, this is a case where we must handle properly.
Else, we will end up processing the allocateNetwork for NetworkDisabled || IsContainer() case, which is incorrect.

coolljt0725 · 2015-12-12T09:52:07Z

@mavenugo Thanks ,updated

thaJeztah · 2015-12-12T10:03:30Z

@coolljt0725 looks like it fails to build 😢

coolljt0725 · 2015-12-12T10:34:03Z

@thaJeztah Forgot to build on local before pushing to github....

cpuguy83 · 2015-12-12T13:00:01Z

daemon/container_operations_unix.go

Does this need to be exported?

good catch. it should not be exported.

There is already a method updateNetworkSettings, so I use UpdateNetworkSettings at the very start. what about updateContainerNetworkSettings?

To make docker inspect return a consistent result of networksettings for created container and stopped container, it's bettew to update the network settings on container creating. Signed-off-by: Lei Jitang <[email protected]>

coolljt0725 · 2015-12-13T07:44:18Z

@mavenugo Add a test, is that what you want?

mavenugo · 2015-12-13T21:55:10Z

daemon/create.go

This method must be called only the first time and that is the reason why it was originally protected with
if len(container.NetworkSettings.Networks) == 0 {
in the allocateNetwork function.

Without that protection, a container restarted with multiple networks will start to behave incorrectly (especially ungraceful daemon restart cases). I guess we need to add the above protection (``if len(container.NetworkSettings.Networks) == 0 { ) here as well.

@mavenugo This method does be called only the first time, because create method only be called once. Once the container has been created, the method will never been called again.

@coolljt0725 you are right :). confused myself with create & start.

mavenugo · 2015-12-14T21:37:01Z

Thanks @coolljt0725
LGTM.

LK4D4 · 2015-12-15T16:47:52Z

Makes sense. Thanks @coolljt0725
LGTM

…g_on_create fix docker inspect return unconsistent network settings of created container and stopped container

thaJeztah · 2016-01-13T08:15:26Z

integration-cli/docker_cli_network_unix_test.go

oh, just noticed a typo here; TestDockerNetworkRestartWithMulipleNetworks (muliple -> multiple )

@thaJeztah Thanks.. Fix this in #19291

GordonTheTurtle added the status/0-triage label Dec 9, 2015

coolljt0725 force-pushed the update_container_networking_on_create branch from 66e914e to ab6be98 Compare December 9, 2015 14:22

cpuguy83 added the area/networking Networking label Dec 9, 2015

cpuguy83 added status/1-design-review and removed status/0-triage labels Dec 9, 2015

cpuguy83 added status/2-code-review and removed status/1-design-review labels Dec 9, 2015

coolljt0725 force-pushed the update_container_networking_on_create branch 4 times, most recently from 656fa53 to 4ffa298 Compare December 10, 2015 09:28

coolljt0725 mentioned this pull request Dec 12, 2015

Add support multiple network mode on docker create/run #17796

Closed

mavenugo reviewed Dec 12, 2015
View reviewed changes

coolljt0725 force-pushed the update_container_networking_on_create branch from 4ffa298 to 49512c7 Compare December 12, 2015 09:50

coolljt0725 force-pushed the update_container_networking_on_create branch from 49512c7 to e514cf4 Compare December 12, 2015 10:33

cpuguy83 reviewed Dec 12, 2015
View reviewed changes

update network settings on container creating

c427131

To make docker inspect return a consistent result of networksettings for created container and stopped container, it's bettew to update the network settings on container creating. Signed-off-by: Lei Jitang <[email protected]>

coolljt0725 force-pushed the update_container_networking_on_create branch from e514cf4 to c427131 Compare December 13, 2015 07:43

mavenugo reviewed Dec 13, 2015
View reviewed changes

LK4D4 added a commit that referenced this pull request Dec 15, 2015

Merge pull request #18531 from coolljt0725/update_container_networkin…

812a7c2

…g_on_create fix docker inspect return unconsistent network settings of created container and stopped container

LK4D4 merged commit 812a7c2 into moby:master Dec 15, 2015

mavenugo mentioned this pull request Jan 7, 2016

Is it possible to bind a container to multi networks? #17750

Closed

tonistiigi mentioned this pull request Jan 13, 2016

Container not attached to network when hostconfig specified on start instead of create #19100

Closed

thaJeztah reviewed Jan 13, 2016
View reviewed changes

Conversation

coolljt0725 commented Dec 9, 2015

Uh oh!

thaJeztah commented Dec 9, 2015

Uh oh!

coolljt0725 commented Dec 9, 2015

Uh oh!

cpuguy83 commented Dec 9, 2015

Uh oh!

cpuguy83 commented Dec 9, 2015

Uh oh!

mavenugo commented Dec 9, 2015

Uh oh!

cpuguy83 commented Dec 9, 2015

Uh oh!

coolljt0725 commented Dec 10, 2015

Uh oh!

coolljt0725 commented Dec 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolljt0725 commented Dec 12, 2015

Uh oh!

thaJeztah commented Dec 12, 2015

Uh oh!

coolljt0725 commented Dec 12, 2015

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!

coolljt0725 commented Dec 13, 2015

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 Dec 14, 2015

Uh oh!

LK4D4 commented Dec 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants