Fix volumes-from/bind-mounts passed in on start #9631

cpuguy83 · 2014-12-12T16:09:10Z

cpuguy83 · 2014-12-12T17:15:38Z

ping @jfrazelle @crosbymichael
This is the issue @proppy was having yesterday on 1.4

crosbymichael · 2014-12-12T20:11:02Z

daemon/volumes.go

+	if path, exists := m.container.Volumes[m.MountToPath]; exists {
+		// If this is a bind-mount/volumes-from, maybe it was passed in at start instead of create
+		// We need to make sure bind-mounts/volumes-from passed on start can over-shadow non-binds
+		// FIXME: This is a hack until HostConfig on container start is totally unsupported


You cannot say this and we need to revert the docs that say that this is deprecated as this is not the case and will not be deprecated. For the longest time this has been the only way of passing binds and volumes and so many third party clients use this as there was no other way.

cpuguy83 · 2014-12-12T20:19:48Z

Fixed that wording.

crosbymichael · 2014-12-12T20:24:28Z

Thanks, I'm doing some testing now

LK4D4 · 2014-12-12T22:27:44Z

docs/sources/reference/api/docker_remote_api.md

@@ -61,12 +61,6 @@ You can set the new container's MAC address explicitly.
 **New!**
 Volumes are now initialized when the container is created.


Maybe makes sense to specify somewhere that this volumes will be shadowed by start options.

Can you please at least ping docs when stuff like this comes up? I have no way of discovering issues like this in a timely manner otherwise.

crosbymichael · 2014-12-13T01:08:14Z

daemon/volumes.go

+		if !m.volume.IsBindMount && m.from == nil {
+			return nil
+		}
+		if m.volume.Path == path {


What does this line mean?

This is checking if the path of the passed in volume is the same as the path in container.Volumes.

But why are you checking for this and returning?

Because if it's the same then we don't need to do anything.
Same path means the same volume as there is only 1 volume object per path.

ibuildthecloud · 2014-12-15T18:37:18Z

FYI, I tested this and it has fixed my issues I've seen with 1.4

crosbymichael · 2014-12-15T18:39:51Z

@ibuildthecloud thanks for testing

tiborvass · 2014-12-15T20:01:46Z

daemon/volumes.go

 	}

+	if err := container.applyVolumesFrom(); err != nil {


Add a comment explaining that this will reset the volumesfrom hostconfig each time and that this is needed in order for start to override what was set in create.

tiborvass · 2014-12-15T21:39:10Z

docs/sources/reference/api/docker_remote_api.md

@@ -61,12 +61,6 @@ You can set the new container's MAC address explicitly.
 **New!**
 Volumes are now initialized when the container is created.

-`POST /containers/(id)/start`


In the PR's first comment, it'd be great to add a reference to the PR that is being partially reverted. (#8683)

@cpuguy83 I don't see it

Fixes moby#9628 Slightly reverts moby#8683, HostConfig on start is _not_ deprecated. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2014-12-15T21:57:47Z

Updated.
Added new test to make sure that VolumesFrom has priorty over anything else, as is the case with the corresponding CLI test(TestRunApplyVolumesFromBeforeVolumes)

cpuguy83 · 2014-12-15T21:58:17Z

*new test and fixed an issue where that wasn't the case.

crosbymichael · 2014-12-15T22:07:10Z

@jfrazelle @LK4D4 I removed your LGTM since the code changed in the last revision

jessfraz · 2014-12-15T22:13:14Z

@ibuildthecloud if you want to retest as well that would be awesome, if not no biggie :)

LK4D4 · 2014-12-15T22:46:48Z

I retested it with docker-py and fig. All is okay. I don't see obvious bugs also(as always 😆 )

jessfraz · 2014-12-15T22:52:46Z

lftm

unclejack · 2014-12-15T23:02:47Z

Would it be too much to ask for a test in docker_api in integration-cli?

LK4D4 · 2014-12-15T23:05:18Z

@unclejack I see that there are three tests in this PR.

crosbymichael · 2014-12-15T23:07:13Z

I'm also adding the entire docker-py integration tests for the lib in #9676 for API tests not owned by us.

crosbymichael · 2014-12-15T23:34:44Z

LGTM

tiborvass · 2014-12-15T23:50:36Z

LGTM, although the code is very complicated to read :(

…_start Fix volumes-from/bind-mounts passed in on start

cpuguy83 force-pushed the 9628_fix_volumes_hostconfig_on_start branch 2 times, most recently from 7f47e7d to 432bd17 Compare December 12, 2014 16:10

crosbymichael reviewed Dec 12, 2014
View reviewed changes

cpuguy83 force-pushed the 9628_fix_volumes_hostconfig_on_start branch 2 times, most recently from 1665254 to b9ad32f Compare December 12, 2014 20:14

crosbymichael added this to the 1.4.1 milestone Dec 12, 2014

LK4D4 reviewed Dec 12, 2014
View reviewed changes

bfirsh mentioned this pull request Dec 13, 2014

File volumes are broken for docker 1.4.0 docker/compose#723

Closed

crosbymichael reviewed Dec 13, 2014
View reviewed changes

deviantony mentioned this pull request Dec 15, 2014

Docker 1.4.x support deviantony/docker-elk#2

Closed

tiborvass reviewed Dec 15, 2014
View reviewed changes

cpuguy83 force-pushed the 9628_fix_volumes_hostconfig_on_start branch from b9ad32f to 01becdc Compare December 15, 2014 20:21

tiborvass reviewed Dec 15, 2014
View reviewed changes

Fix volumes-from/bind-mounts passed in on start

d44c9f9

Fixes moby#9628 Slightly reverts moby#8683, HostConfig on start is _not_ deprecated. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the 9628_fix_volumes_hostconfig_on_start branch from 01becdc to d44c9f9 Compare December 15, 2014 21:52

crosbymichael added a commit that referenced this pull request Dec 15, 2014

Merge pull request #9631 from cpuguy83/9628_fix_volumes_hostconfig_on…

24d03b8

…_start Fix volumes-from/bind-mounts passed in on start

crosbymichael merged commit 24d03b8 into moby:master Dec 15, 2014

fredlf added /project/doc and removed /project/doc labels Dec 17, 2014

anthonylouisburns mentioned this pull request Dec 21, 2014

issues with api bind/volumesfrom on start #9628

Closed

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

cpuguy83 deleted the 9628_fix_volumes_hostconfig_on_start branch September 20, 2017 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix volumes-from/bind-mounts passed in on start #9631

Fix volumes-from/bind-mounts passed in on start #9631

cpuguy83 commented Dec 12, 2014

cpuguy83 commented Dec 12, 2014

crosbymichael Dec 12, 2014

cpuguy83 commented Dec 12, 2014

crosbymichael commented Dec 12, 2014

LK4D4 Dec 12, 2014

tiborvass Dec 15, 2014

fredlf Dec 17, 2014

crosbymichael Dec 13, 2014

cpuguy83 Dec 13, 2014

crosbymichael Dec 13, 2014

cpuguy83 Dec 13, 2014

ibuildthecloud commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

tiborvass Dec 15, 2014

tiborvass Dec 15, 2014

cpuguy83 Dec 15, 2014

tiborvass Dec 15, 2014

cpuguy83 commented Dec 15, 2014

cpuguy83 commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

jessfraz commented Dec 15, 2014

LK4D4 commented Dec 15, 2014

jessfraz commented Dec 15, 2014

unclejack commented Dec 15, 2014

LK4D4 commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

tiborvass commented Dec 15, 2014

		@@ -61,12 +61,6 @@ You can set the new container's MAC address explicitly.
		New!
		Volumes are now initialized when the container is created.

Fix volumes-from/bind-mounts passed in on start #9631

Fix volumes-from/bind-mounts passed in on start #9631

Conversation

cpuguy83 commented Dec 12, 2014

cpuguy83 commented Dec 12, 2014

Choose a reason for hiding this comment

cpuguy83 commented Dec 12, 2014

crosbymichael commented Dec 12, 2014

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuildthecloud commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

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 15, 2014

cpuguy83 commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

jessfraz commented Dec 15, 2014

LK4D4 commented Dec 15, 2014

jessfraz commented Dec 15, 2014

unclejack commented Dec 15, 2014

LK4D4 commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

crosbymichael commented Dec 15, 2014

tiborvass commented Dec 15, 2014