Support network connect/disconnect to stopped container by coolljt0725 · Pull Request #18906 · moby/moby

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

Signed-off-by: Lei Jitang [email protected]

see #17796
ping @mavenugo

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

Awesome, thanks @coolljt0725!

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

integration-cli/docker_cli_network_unix_test.go

Slight typo: netwokr -> network

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

@sirlatrom thank you :)

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

daemon/container_operations_unix.go

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

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

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

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 <[email protected]>

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

integration-cli/docker_cli_network_unix_test.go Outdated

Copy link

Contributor

sirlatrom Dec 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight typo: netwokr -> network

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

daemon/container_operations_unix.go Outdated

Copy link

Member

cpuguy83 Jan 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

errors/daemon.go Outdated

Copy link

Contributor

jamtur01 Jan 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably do:

... connected or disconnected to the network

And same with error msg below too

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 <[email protected]>

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

Comments

Conversation

coolljt0725 commented Dec 25, 2015

Uh oh!

thaJeztah commented Dec 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolljt0725 commented Dec 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Dec 28, 2015

Uh oh!

coolljt0725 commented Dec 29, 2015

Uh oh!

thaJeztah commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavenugo commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 10, 2016

Uh oh!

cpuguy83 commented Jan 11, 2016

Uh oh!

icecrime commented Jan 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamtur01 commented Jan 11, 2016

Uh oh!

vdemeester commented Jan 11, 2016

Uh oh!

coolljt0725 commented Jan 12, 2016

Uh oh!

vdemeester commented Jan 12, 2016

Uh oh!

coolljt0725 commented Jan 12, 2016

Uh oh!

thaJeztah commented Jan 12, 2016

Uh oh!

thaJeztah commented Jan 12, 2016

Uh oh!

thaJeztah commented Jan 12, 2016

Uh oh!

thaJeztah commented Jan 12, 2016

Uh oh!

sdurrheimer commented Jan 13, 2016

Uh oh!

albers commented Jan 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants