Fix duplicate mount points for multiple `--volumes-from` in `docker run` #29563

yongtang · 2016-12-20T02:18:53Z

- What I did
This fix tries to fix the issue raised in #21845.

The issue with #21845 is that if multiple --volumes-from with the same destination has been specified, then one volume will be overridden by the other. This will mess up reference and prevent the overridden volume from being removed at the end.

Issue #21845 was observed with docker-compose though it is possible to emulate the same behavior with docker alone:

$ cat Dockerfile
FROM busybox
VOLUME ["/tmp/data"]
$ docker build -t vimage .
$ docker run --name=data1 vimage true
$ docker run --name=data2 vimage true
$ docker run --name=app --volumes-from=data1 --volumes-from=data2 -d busybox top
$ docker rm -f -v $(docker ps -aq)
$ docker volume ls
$ docker volume rm ...

- How I did it

This fix tries to address the issue by return an error if duplicate mount points was used with --volumes-from.

- How to verify it

An integration test has been added.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #21845.

/cc @cpuguy83 @vdemeester @thaJeztah

Signed-off-by: Yong Tang yong.tang.github@outlook.com

AkihiroSuda · 2016-12-20T02:30:07Z

daemon/volumes.go

@@ -103,6 +103,9 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}

 		for _, m := range c.MountPoints {
+			if _, ok := mountPoints[m.Destination]; ok {


I wonder similar checking about mountPoints is needed for this loop (for _, cfg := range hostConfig.Mounts {}) as well: https://github.com/yongtang/docker/blob/d231e33926a58a11974bbdd9a038e79fac2217df/daemon/volumes.go#L164-L206

Thanks @AkihiroSuda for the reminder. I will double check for this case.

thaJeztah · 2016-12-20T13:49:24Z

CI looks to be failing

02:45:57 
02:45:57 ----------------------------------------------------------------------
02:45:57 FAIL: docker_deprecated_api_v124_test.go:87: DockerSuite.TestDeprecatedContainerAPIStartVolumesFrom
02:45:57 
02:45:57 docker_deprecated_api_v124_test.go:110:
02:45:57     c.Assert(status, checker.Equals, http.StatusNoContent)
02:45:57 ... obtained int = 500
02:45:57 ... expected int = 204
02:45:57

thaJeztah · 2016-12-20T14:03:23Z

Interesting, these work:

$ docker run -v /bla -v /bla --rm alpine sh
$ docker run -v /bla -v bla:/bla --rm alpine sh
$ docker run -v /bla --tmpfs /bla --rm alpine sh

These don't

$ docker run -v bla:/bla -v /bla --tmpfs /bla --rm alpine sh
docker: Error response from daemon: Duplicate mount point '/bla'.
See 'docker run --help'.

$ docker run -v bla:/bla --tmpfs /bla --rm alpine sh
docker: Error response from daemon: Duplicate mount point '/bla'.
See 'docker run --help'.

Not sure if it's all expected (i.e., can it be a problem as well for anonymous volumes?),
but thought I'd post it here for reference

cpuguy83 · 2016-12-21T01:55:11Z

daemon/volumes.go

@@ -103,6 +103,9 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}

 		for _, m := range c.MountPoints {
+			if _, ok := mountPoints[m.Destination]; ok {
+				return fmt.Errorf("Duplicate mount point '%s'", m.Destination)


This is really a behavior change.
I think this will end up breaking people who have already brought in a volume and then also do --volumes-from on a container with the same volume.

I think we'll need to pick which one wins and log an error for the other one.
And the one we pick is whatever happens to win right now.

@cpuguy83 @thaJeztah Got that, thanks. Was wondering how to best handle this situation. Will take a look and make sure the previous behavior is preserved.

yongtang · 2016-12-22T20:52:28Z

Update on this PR:

In addition to the issue described above, the following will also prevent volumes from removed:

$ docker build -t vimage .
$ docker run --name=data1 vimage true
$ docker run --name=data2 vimage true
$ docker run --name=app --volumes-from=data1 --volumes-from=data2 -v /tmp/data:/tmp/data -d busybox top
$ docker rm -f -v $(docker ps -aq)
$ docker volume ls
$ docker volume rm ...

The bind -v /tmp/data:/tmp/data will override the mount point as well.

Both the original issue and the bind override issue have been fixed.

Will continue to investigate other scenarios.

yongtang · 2016-12-23T05:01:27Z

Update of the PR (case 3):
In addition to the above two mentioned cases, it is also possible to specify bind with HostConfig.Mounts in API:

$ docker build -t vimage .
$ docker run --name=data1 vimage true
$ docker run --name=data2 vimage true
$ curl --unix-socket /var/run/docker.sock ...
$ docker rm -f -v $(docker ps -aq)
$ docker volume ls
$ docker volume rm ...

This case will also trigger volume is in use - ... error. This case is different from case (2) in that HostConfig.Mounts is only available in API. Case (2) and Case (3) happens in different places in registerMountpoint().

yongtang · 2016-12-23T05:02:49Z

@cpuguy83 @AkihiroSuda @thaJeztah. The PR has been updated with different possible combinations of scenarios covered. Please take a look and let me know if there are any issues.

vdemeester · 2017-01-12T17:34:08Z

ping @thaJeztah @cpuguy83

cpuguy83 · 2017-01-27T17:07:05Z

Is this actually fixed now that references are stored as a map instead of a slice?

LK4D4 · 2017-01-27T17:11:36Z

daemon/volumes.go

@@ -121,7 +121,12 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 				}
 				cp.Volume = v
 			}
-
+			if v, ok := mountPoints[m.Destination]; ok {


These seven lines are the same three times; maybe we should have a helper for it.

Thanks @LK4D4. The PR has been updated with helper func. Please take a look.

yongtang · 2017-01-27T23:35:25Z

Thanks @cpuguy83 for the review. To preserve the behavior we need to deference the old volume that points to the same mount point (when old and new volume point to the same mount point). So the change is needed for the fix.

LK4D4

LGTM

thaJeztah · 2017-01-31T00:58:50Z

Not sure if it's the same part that's causing this error, but;

docker run -ti --rm --tmpfs /dev/shm:rw,nosuid,nodev,size=8g busybox
docker: Error response from daemon: linux mounts: Duplicate mount point '/dev/shm'.

As mentioned here; #6758 (comment)

cpuguy83

LGTM

This looks like it's fixing a real problem with refcounting.
However our code looks really awful here because we're essentially leaking volumes.
Not sure what to do here at the moment, but we should figure out how to do this better.

cpuguy83 · 2017-02-07T15:29:04Z

daemon/volumes.go

@@ -85,6 +85,15 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}
 	}()

+	dereferenceIfExists := func(destination string) {
+		if v, ok := mountPoints[destination]; ok {
+			logrus.Errorf("Duplicate mount point '%s'", destination)


Actually, sorry this looks like it should be Debug, not Error

This fix tries to fix the issue raised in 21845. The issue with 21845 is that if multiple `--volumes-from` with the same destination has been specified, then one volume will be overridden by the other. This will mess up with volumes reference and prevent the overridden volume from being removed at the end. Issue 21845 was observed with `docker-compose` though it is possible to emulate the same behavior with `docker` alone: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Second case: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -v /tmp/data:/tmp/data -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Third case: Combination of --volumes-from and `HostConfig.Mounts` (API only) This fix tries to address the issue by return an error if duplicate mount points was used with `--volumes-from`. An integration test has been added. This fix fixes 21845. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

cpuguy83

LGTM

yongtang · 2017-02-07T21:50:35Z

Thanks @cpuguy83. I agree the current way of handling it is less than ideal. I will spend some time to investigate it and see if we could come up with something better.

This fix tries to fix the issue raised in 21845. The issue with 21845 is that if multiple `--volumes-from` with the same destination has been specified, then one volume will be overridden by the other. This will mess up with volumes reference and prevent the overridden volume from being removed at the end. Issue 21845 was observed with `docker-compose` though it is possible to emulate the same behavior with `docker` alone: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Second case: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -v /tmp/data:/tmp/data -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Third case: Combination of --volumes-from and `HostConfig.Mounts` (API only) This fix tries to address the issue by return an error if duplicate mount points was used with `--volumes-from`. An integration test has been added. This fix fixes 21845. cherry-picked from upstream moby#29563 Fix issue moby#338 Fix DTS2017070606746 note: Our code does not support API HostConfig.Mounts, so i delete the test case of third case. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Signed-off-by: Fengtu Wang <wangfengtu@huawei.com> Conflicts: daemon/volumes.go integration-cli/docker_cli_volume_test.go

Fix duplicate mount point for `--volumes-from` in `docker run` This fix tries to fix the issue raised in 21845. The issue with 21845 is that if multiple `--volumes-from` with the same destination has been specified, then one volume will be overridden by the other. This will mess up with volumes reference and prevent the overridden volume from being removed at the end. Issue 21845 was observed with `docker-compose` though it is possible to emulate the same behavior with `docker` alone: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Second case: ``` $ cat Dockerfile FROM busybox VOLUME ["/tmp/data"] $ docker build -t vimage . $ docker run --name=data1 vimage true $ docker run --name=data2 vimage true $ docker run --name=app --volumes-from=data1 --volumes-from=data2 -v /tmp/data:/tmp/data -d busybox top $ docker rm -f -v $(docker ps -aq) $ docker volume ls $ docker volume rm ... ``` NOTE: Third case: Combination of --volumes-from and `HostConfig.Mounts` (API only) This fix tries to address the issue by return an error if duplicate mount points was used with `--volumes-from`. An integration test has been added. This fix fixes 21845. cherry-picked from upstream moby#29563 Fix issue moby#338 Fix DTS2017070606746 note: Our code does not support API HostConfig.Mounts, so i delete the test case of third case. Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Signed-off-by: Fengtu Wang <wangfengtu@huawei.com> Conflicts: daemon/volumes.go integration-cli/docker_cli_volume_test.go See merge request docker/docker!661

GordonTheTurtle added the status/0-triage label Dec 20, 2016

yongtang mentioned this pull request Dec 20, 2016

Cannot remove docker volume #21845

Closed

AkihiroSuda reviewed Dec 20, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Dec 20, 2016

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 20, 2016

cpuguy83 requested changes Dec 21, 2016

View reviewed changes

yongtang force-pushed the 21845-duplicate-mount-point-volumes-from branch 3 times, most recently from 0b89a8c to 679ff0f Compare December 22, 2016 20:35

yongtang force-pushed the 21845-duplicate-mount-point-volumes-from branch from 679ff0f to 496ab3d Compare December 23, 2016 04:56

yongtang force-pushed the 21845-duplicate-mount-point-volumes-from branch 2 times, most recently from 282e61b to 6d8ee89 Compare December 26, 2016 19:18

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 27, 2016

LK4D4 reviewed Jan 27, 2017

View reviewed changes

yongtang force-pushed the 21845-duplicate-mount-point-volumes-from branch from 6d8ee89 to 4b699af Compare January 27, 2017 23:27

LK4D4 approved these changes Jan 30, 2017

View reviewed changes

thaJeztah mentioned this pull request Jan 30, 2017

Enable 'exec' on /dev/shm, without --privileged #6758

Closed

cpuguy83 approved these changes Feb 7, 2017

View reviewed changes

cpuguy83 requested changes Feb 7, 2017

View reviewed changes

yongtang force-pushed the 21845-duplicate-mount-point-volumes-from branch from 4b699af to 9526e5c Compare February 7, 2017 16:47

cpuguy83 approved these changes Feb 7, 2017

View reviewed changes

cpuguy83 merged commit 5381f9f into moby:master Feb 7, 2017

GordonTheTurtle added this to the 1.14.0 milestone Feb 7, 2017

yongtang deleted the 21845-duplicate-mount-point-volumes-from branch February 7, 2017 21:09

vieux mentioned this pull request Feb 23, 2017

17.03 cherry-picks #31266

Merged

thaJeztah modified the milestones: 17.03.0, 17.04.0 Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix duplicate mount points for multiple `--volumes-from` in `docker run` #29563

Fix duplicate mount points for multiple `--volumes-from` in `docker run` #29563

yongtang commented Dec 20, 2016 •

edited

Loading

AkihiroSuda Dec 20, 2016

yongtang Dec 21, 2016

thaJeztah commented Dec 20, 2016

thaJeztah commented Dec 20, 2016

cpuguy83 Dec 21, 2016

cpuguy83 Dec 21, 2016

yongtang Dec 21, 2016

yongtang commented Dec 22, 2016

yongtang commented Dec 23, 2016

yongtang commented Dec 23, 2016

vdemeester commented Jan 12, 2017

cpuguy83 commented Jan 27, 2017

LK4D4 Jan 27, 2017

yongtang Jan 27, 2017

yongtang commented Jan 27, 2017

LK4D4 left a comment

thaJeztah commented Jan 31, 2017

cpuguy83 left a comment

cpuguy83 Feb 7, 2017

cpuguy83 left a comment

yongtang commented Feb 7, 2017

Fix duplicate mount points for multiple --volumes-from in docker run #29563

Fix duplicate mount points for multiple --volumes-from in docker run #29563

Conversation

yongtang commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Dec 20, 2016

thaJeztah commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Dec 22, 2016

yongtang commented Dec 23, 2016

yongtang commented Dec 23, 2016

vdemeester commented Jan 12, 2017

cpuguy83 commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Jan 27, 2017

LK4D4 left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 31, 2017

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

yongtang commented Feb 7, 2017

Fix duplicate mount points for multiple `--volumes-from` in `docker run` #29563

Fix duplicate mount points for multiple `--volumes-from` in `docker run` #29563

yongtang commented Dec 20, 2016 •

edited

Loading