New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 7, 2017

Conversation

Projects
None yet
7 participants
@yongtang
Member

yongtang commented Dec 20, 2016

- 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)

aww-cat-cute-kitten-favim com-288946_large

This fix fixes #21845.

/cc @cpuguy83 @vdemeester @thaJeztah

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

Show outdated Hide outdated daemon/volumes.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 20, 2016

Member

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 
Member

thaJeztah commented Dec 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 20, 2016

Member

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

Member

thaJeztah commented Dec 20, 2016

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

Show outdated Hide outdated daemon/volumes.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 22, 2016

Member

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.

Member

yongtang commented Dec 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 23, 2016

Member

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().

Member

yongtang commented Dec 23, 2016

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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 23, 2016

Member

@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.

Member

yongtang commented Dec 23, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@vdemeester
Member

vdemeester commented Jan 12, 2017

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 27, 2017

Contributor

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

Contributor

cpuguy83 commented Jan 27, 2017

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

Show outdated Hide outdated daemon/volumes.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

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.

Member

yongtang commented Jan 27, 2017

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

LK4D4 approved these changes Jan 30, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 31, 2017

Member

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)

Member

thaJeztah commented Jan 31, 2017

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.

Show outdated Hide outdated daemon/volumes.go Outdated
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.

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

LGTM

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

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30476 has succeeded
Details
janky Jenkins build Docker-PRs 39092 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10146 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 7, 2017

@yongtang yongtang deleted the yongtang:21845-duplicate-mount-point-volumes-from branch Feb 7, 2017

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 7, 2017

Member

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.

Member

yongtang commented Feb 7, 2017

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.

@vieux vieux referenced this pull request Feb 23, 2017

Merged

17.03 cherry-picks #31266

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

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

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

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'rm_volume_fail' into 'huawei-1.11.2'
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment