Support network connect/disconnect to stopped container #18906

coolljt0725 · 2015-12-25T07:27:48Z

Signed-off-by: Lei Jitang leijitang@huawei.com

thaJeztah · 2015-12-25T12:19:26Z

sirlatrom · 2015-12-27T10:23:47Z

integration-cli/docker_cli_network_unix_test.go

+	dockerCmd(c, "network", "connect", "test", "foo")
+	networks, err := inspectField("foo", "NetworkSettings.Networks")
+	c.Assert(err, checker.IsNil)
+	c.Assert(networks, checker.Contains, "test", check.Commentf("Should contain 'test' netwokr"))


Slight typo: netwokr -> network

coolljt0725 · 2015-12-28T00:58:23Z

@sirlatrom thank you :)

cpuguy83 · 2015-12-28T13:25:27Z

daemon/container_operations_unix.go

@@ -610,7 +610,25 @@ func (daemon *Daemon) getNetworkSandbox(container *container.Container) libnetwo
 // ConnectToNetwork connects a container to a network
 func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName string) error {
 	if !container.Running {
-		return derr.ErrorCodeNotRunning.WithArgs(container.ID)
+		if container.HostConfig.NetworkMode.IsContainer() {


Instead of branching with a whole bunch of logic, can we move this to a new function?
I also haven't looked yet, but I'm guessing there is probably some duplicated logic here?

Yes. we can avoid the code duplication by simply moving the if !container.Running { check to https://github.com/docker/docker/blob/master/daemon/container_operations_unix.go#L645.
This will avoid the code duplication. But the tricky part will be differentiate the common code-path for connectToNetwork both from docker run command and docker network connect. We may have to check for a better container state than container.Running because it might be false when called via docker run & hence might cause issues.

or (though I don't like this approach) add another bool flag to connectToNetwork.

The real issue here is that when the container is not running we are doing something completely different and then returning 100% of the time, there is no shared logic with the code beneath, so for readability it'd be best to move this out to a separate function.

yes. that is also a way to avoid the code-duplication issue & I like it as well.

cpuguy83 · 2015-12-28T13:25:49Z

ping @mrjana

coolljt0725 · 2015-12-29T08:58:50Z

@cpuguy83 @mavenugo updated

thaJeztah · 2016-01-07T16:25:58Z

@coolljt0725 not in docs review yet, but there are some minor changes needed in the documentation;

https://github.com/docker/docker/blob/master/docs/userguide/networking/work-with-networks.md

To connect a container to a network, the container must be running.

https://github.com/docker/docker/blob/master/docs/reference/commandline/network_connect.md

Connects a running container to a network.
(just remove "running")

same here; https://github.com/docker/docker/blob/master/man/docker-network-connect.1.md

mavenugo · 2016-01-07T16:27:15Z

daemon/container_operations_unix.go

@@ -607,10 +607,40 @@ func (daemon *Daemon) getNetworkSandbox(container *container.Container) libnetwo
 	return sb
 }

+func (daemon *Daemon) updateNetworks(container *container.Container, idOrName string, updateSettings bool) (libnetwork.Network, error) {


minor nit. The appropriate function name would be updateNetworkConfig since it does update both the container.Config and also the network specific settings. Can you please change that ?

mavenugo · 2016-01-07T16:33:19Z

@coolljt0725 minor comments. Rest of the code and logic LGTM.

cpuguy83 · 2016-01-07T16:34:42Z

daemon/container_operations_unix.go

-		return derr.ErrorCodeNotRunning.WithArgs(container.ID)
-	}
+		if container.RemovalInProgress || container.Dead {
+			return fmt.Errorf("Container is marked for removal and cannot be disconnect to network.")


I would use the same "new errcode" referenced in the comment above.

thaJeztah · 2016-01-10T18:43:13Z

early docs review; docs LGTM

cpuguy83 · 2016-01-11T17:46:43Z

LGTM, nice work!

Moving to docs review.

icecrime · 2016-01-11T18:31:07Z

Ping @jamtur01 @MHBauer @vdemeester 🌴

jamtur01 · 2016-01-11T18:54:45Z

errors/daemon.go

+	// container but it's marked for removal.
+	ErrorCodeRemovalContainer = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "REMOVALCONTAINER",
+		Message:        "Container %s is marked for removal and cannot be connected/disconnected to network",


I'd probably do:

... connected or disconnected to the network

And same with error msg below too

jamtur01 · 2016-01-11T18:55:39Z

Docs LGTM - one minor comment.

vdemeester · 2016-01-11T21:09:30Z

@coolljt0725 agree with @jamtur01 comment, once taking care of, LGTM 🐰

Signed-off-by: Lei Jitang <leijitang@huawei.com>

coolljt0725 · 2016-01-12T01:31:34Z

@jamtur01 @vdemeester thanks, updated

vdemeester · 2016-01-12T10:20:02Z

docs LGTM

Hum, I don't get why the documentation build has failures… I do not reproduce the failures locally 😓.

coolljt0725 · 2016-01-12T11:09:33Z

@vdemeester It's weird, I just taken care of the comment of @jamtur01 , no other changes

thaJeztah · 2016-01-12T14:36:59Z

FATAL: 2016/01/12 mkdir /docs/public/engine/userguide/networking/default_network/images/: no such file or directory

hmf, yes, that error is not related, it's an issue with Hugo (probably); see gohugoio/hugo#1754 @SvenDowideit was looking into those, but not sure if he has progress.

thaJeztah · 2016-01-12T14:49:26Z

Hmm, looks like GitHub is having issues with their SVN integration on docker/compose?

Step 3 : RUN svn checkout https://github.com/docker/compose/trunk/docs /docs/content/compose
 ---> Running in b2981df85fd6
svn: E175002: Unexpected HTTP status 500 'Internal Server Error' on '/docker/compose/trunk/docs'

svn: E175002: Additional errors:
svn: E175002: PROPFIND request on '/docker/compose/trunk/docs' failed: 500 Internal Server Error
The command '/bin/sh -c svn checkout https://github.com/docker/compose/trunk/docs /docs/content/compose' returned a non-zero code: 1

Manually doing a svn checkout also produces an error (just on the compose repository);

svn checkout https://github.com/docker/compose/trunk/docs
svn: E175002: Unexpected HTTP status 500 'Internal Server Error' on '/docker/compose/!svn/vcc/default'

svn: E175002: Additional errors:
svn: E175002: REPORT request on '/docker/compose/!svn/vcc/default' failed: 500 Internal Server Error

thaJeztah · 2016-01-12T15:03:28Z

Sent an e-mail to GitHub support for the SVN issue

thaJeztah · 2016-01-12T15:06:18Z

LGTM - docs failure is not related

Support network connect/disconnect to stopped container

sdurrheimer · 2016-01-13T07:25:13Z

ping @albers We may need to update bash and zsh completion

albers · 2016-01-13T09:41:16Z

@sdurrheimer Thanks for the ping, created #19293 for bash completion.

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

coolljt0725 force-pushed the connect_to_created branch 2 times, most recently from 71e7a76 to c4c4997 Compare December 25, 2015 08:25

thaJeztah added status/2-code-review and removed status/0-triage labels Dec 25, 2015

sirlatrom reviewed Dec 27, 2015
View reviewed changes

coolljt0725 force-pushed the connect_to_created branch from c4c4997 to 366fa37 Compare December 28, 2015 00:57

cpuguy83 reviewed Dec 28, 2015
View reviewed changes

coolljt0725 force-pushed the connect_to_created branch 3 times, most recently from d74dd44 to 430b24c Compare December 29, 2015 08:57

thaJeztah added this to the 1.10.0 milestone Jan 7, 2016

mavenugo mentioned this pull request Jan 7, 2016

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

Closed

mavenugo reviewed Jan 7, 2016
View reviewed changes

cpuguy83 reviewed Jan 7, 2016
View reviewed changes

thaJeztah mentioned this pull request Jan 8, 2016

Having multiple NICs (network interfaces) on a Docker image before it boots #1824

Closed

coolljt0725 force-pushed the connect_to_created branch from 430b24c to 80dcd11 Compare January 8, 2016 03:22

thaJeztah added the impact/changelog label Jan 10, 2016

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Jan 11, 2016

jamtur01 reviewed Jan 11, 2016
View reviewed changes

coolljt0725 force-pushed the connect_to_created branch from 06b78d0 to 79d4f0f Compare January 12, 2016 01:06

Add docker network connect/disconnect to non-running container

79d4f0f

Signed-off-by: Lei Jitang <leijitang@huawei.com>

thaJeztah added a commit that referenced this pull request Jan 12, 2016

Merge pull request #18906 from coolljt0725/connect_to_created

301627c

Support network connect/disconnect to stopped container

thaJeztah merged commit 301627c into moby:master Jan 12, 2016

albers mentioned this pull request Jan 13, 2016

bash completion for connecting non-running containers to networks #19293

Merged

mavenugo mentioned this pull request Jan 15, 2016

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

Closed

sdurrheimer mentioned this pull request Jan 16, 2016

Modify zsh completion for connecting/disconnecting non-running contai… #19387

Merged

aboch mentioned this pull request Jan 21, 2016

Move Unsupp Network & IP check to the new updateNetworkConfig() func #19527

Merged

mavenugo mentioned this pull request Jan 31, 2016

docker run with multiple networks #19863

Closed

rayward mentioned this pull request Feb 8, 2016

Docker provider tries to connect networks before it is running. hashicorp/terraform#5033

Closed

CpuID mentioned this pull request Oct 10, 2016

Ability to select private/public IP for specific task/port hashicorp/nomad#646

Closed

hashibot mentioned this pull request Jun 13, 2017

Docker provider tries to connect networks before it is running. hashicorp/terraform-provider-docker#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support network connect/disconnect to stopped container #18906

Support network connect/disconnect to stopped container #18906

coolljt0725 commented Dec 25, 2015

thaJeztah commented Dec 25, 2015

sirlatrom Dec 27, 2015

coolljt0725 commented Dec 28, 2015

cpuguy83 Dec 28, 2015

mavenugo Dec 28, 2015

mavenugo Dec 28, 2015

cpuguy83 Dec 28, 2015

mavenugo Dec 28, 2015

cpuguy83 commented Dec 28, 2015

coolljt0725 commented Dec 29, 2015

thaJeztah commented Jan 7, 2016

mavenugo Jan 7, 2016

mavenugo commented Jan 7, 2016

cpuguy83 Jan 7, 2016

thaJeztah commented Jan 10, 2016

cpuguy83 commented Jan 11, 2016

icecrime commented Jan 11, 2016

jamtur01 Jan 11, 2016

jamtur01 commented Jan 11, 2016

vdemeester commented Jan 11, 2016

coolljt0725 commented Jan 12, 2016

vdemeester commented Jan 12, 2016

coolljt0725 commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

sdurrheimer commented Jan 13, 2016

albers commented Jan 13, 2016

Support network connect/disconnect to stopped container #18906

Support network connect/disconnect to stopped container #18906

Conversation

coolljt0725 commented Dec 25, 2015

thaJeztah commented Dec 25, 2015

Choose a reason for hiding this comment

coolljt0725 commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Dec 28, 2015

coolljt0725 commented Dec 29, 2015

thaJeztah commented Jan 7, 2016

Choose a reason for hiding this comment

mavenugo commented Jan 7, 2016

Choose a reason for hiding this comment

thaJeztah commented Jan 10, 2016

cpuguy83 commented Jan 11, 2016

icecrime commented Jan 11, 2016

Choose a reason for hiding this comment

jamtur01 commented Jan 11, 2016

vdemeester commented Jan 11, 2016

coolljt0725 commented Jan 12, 2016

vdemeester commented Jan 12, 2016

coolljt0725 commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

thaJeztah commented Jan 12, 2016

sdurrheimer commented Jan 13, 2016

albers commented Jan 13, 2016