Skip to content
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 docker create with duplicate volume failed to remove #22103

Merged
merged 1 commit into from
May 30, 2016

Conversation

coolljt0725
Copy link
Contributor

*- What I did
fix #22093
- How I did it
During the registerMountPoints on container creation, it could be some volumes register
successfully but some failed, this will cause the container failed but with some volumes has
reference to this container, and these volume can't be remove.
The fix is to remove the mountpoints once registerMountPoints return failed.

- How to verify it
see #22093

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

@lowenna
Copy link
Member

lowenna commented Apr 18, 2016

WindowsTP5 failure on TestRunNoDupVolume appears legitimate. Probably because volumes are case insensitive on Windows. https://github.com/docker/docker/blob/master/volume/volume_windows.go#L95

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 18, 2016
@thaJeztah
Copy link
Member

@jhowardmsft beat me to it :D

https://jenkins.dockerproject.org/job/Docker-PRs-WoW-TP5/733/console

02:15:44 ----------------------------------------------------------------------
02:15:44 FAIL: docker_cli_run_test.go:506: DockerSuite.TestRunNoDupVolumes
02:15:44 
02:15:44 docker_cli_run_test.go:540:
02:15:44     dockerCmd(c, "volume", "rm", mountstr1)
02:15:44 C:/gopath/src/github.com/docker/docker/pkg/integration/dockerCmd_utils.go:42:
02:15:44     c.Assert(err, check.IsNil, check.Commentf(quote+"%v"+quote+" failed with errors: %s, %v", strings.Join(args, " "), out, err))
02:15:44 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc082250080)} ("exit status 1")
02:15:44 ... "volume rm D:\CI\CI-875e135\test1.QohZOUPDrq:c:\someplace" failed with errors: Error response from daemon: get d:\ci\ci-875e135\test1.qohzoupdrq:c:\someplace: no such volume
02:15:44 , exit status 1
02:15:44 
02:15:45 

binds := map[string]bool{}
mountPoints := map[string]*volume.MountPoint{}
defer func() {
// clean up the container mountpoints once return with error
if retErr != nil && len(mountPoints) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

no need to check len on mountPoints since it's a range

I also think this may be too deep in the stack. I'd expect the caller to unwind this on error (and use https://github.com/docker/docker/blob/master/daemon/mounts.go#L20)

Copy link
Contributor Author

@coolljt0725 coolljt0725 Apr 19, 2016

Choose a reason for hiding this comment

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

@cpuguy83 if registerMountPoints returns error, the mountpoints which has been registered in registerMountPoints would not in container.MountPoint, the caller unwind this on error in https://github.com/docker/docker/blob/master/daemon/create.go#L107 based on container.MountPoint

yup, len is duplicate here

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 19, 2016
@thaJeztah
Copy link
Member

Oh, looks like Windows is failing;

11:24:15 ----------------------------------------------------------------------
11:24:15 FAIL: docker_cli_run_test.go:506: DockerSuite.TestRunNoDupVolumes
11:24:15 
11:24:15 docker_cli_run_test.go:540:
11:24:15     dockerCmd(c, "volume", "rm", mountstr2)
11:24:15 C:/gopath/src/github.com/docker/docker/pkg/integration/dockerCmd_utils.go:42:
11:24:15     c.Assert(err, check.IsNil, check.Commentf(quote+"%v"+quote+" failed with errors: %s, %v", strings.Join(args, " "), out, err))
11:24:15 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc08231c260)} ("exit status 1")
11:24:15 ... "volume rm D:\CI\CI-ba7c24c\test2.aIuQBeHSfr:c:\someplace" failed with errors: Error response from daemon: get d:\ci\ci-ba7c24c\test2.aiuqbehsfr:c:\someplace: no such volume
11:24:15 , exit status 1
11:24:15 

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 29, 2016
@LK4D4
Copy link
Contributor

LK4D4 commented May 6, 2016

ping @coolljt0725

@coolljt0725
Copy link
Contributor Author

@LK4D4 @thaJeztah really sorry for the late response, it's very busy these days and miss the github notes. updated.

Signed-off-by: Lei Jitang <leijitang@huawei.com>
@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 12, 2016
@thaJeztah
Copy link
Member

tested this and works for me LGTM

ping @vdemeester @runcom PTAL

@runcom
Copy link
Member

runcom commented May 30, 2016

LGTM

@vdemeester
Copy link
Member

LGTM 🐸

@vdemeester vdemeester merged commit edcc957 into moby:master May 30, 2016
@thaJeztah thaJeztah added this to the 1.12.0 milestone May 30, 2016
@coolljt0725 coolljt0725 deleted the fix_22093 branch July 5, 2017 02:52
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
cherry-pick from moby#22103

fix DTS2017070404731

Signed-off-by: Lei Jitang <leijitang@huawei.com>
(cherry picked from commit 5e5e1d7)
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
Fix docker create with duplicate volume failed to remove

cherry-pick from moby#22103

fix DTS2017070404731

Signed-off-by: Lei Jitang <leijitang@huawei.com>
(cherry picked from commit 5e5e1d7)



See merge request docker/docker!645
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to remove volume when container creation fails
8 participants