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

479 - add -rm support to docker run #1589

Merged

Conversation

unclejack
Copy link
Contributor

This PR adds support for automatically removing the container when the container exits (or a signal to stop is received). This PR fixes #479.

It's currently supported only when -d isn't provided. It doesn't make sense to automatically remove a container created via docker run -d. There are two reasons why this is implemented this way: 1) we might want to retrieve some kind of exit status or logs before removing the container 2) making this run on the server side is difficult in the current architecture.

Please feel free to provide feedback for this PR.

@vieux
Copy link
Contributor

vieux commented Aug 19, 2013

Not sure why you call POST /wait, CmdRun already wait if you don't add -d,
so couldn't you simply add your if at the end of the method ?

@unclejack
Copy link
Contributor Author

@vieux I recall I've tried that and it was attempting to remove the container right away. I will check it out again and provide feedback or update the code.

@crosbymichael
Copy link
Contributor

@unclejack Why did you decide to place this flag in HostConfig?

@griff
Copy link
Contributor

griff commented Aug 19, 2013

@vieux the reason the wait is needed is because run is kept alive by reading from the stdout pipe until it closes (which happens when the program ends).

@vieux
Copy link
Contributor

vieux commented Aug 29, 2013

ping @unclejack

@unclejack
Copy link
Contributor Author

@vieux It should be ready now. Please take a look.

@unclejack
Copy link
Contributor Author

@vieux Please take a look at this PR. It should be OK now.

@vieux
Copy link
Contributor

vieux commented Sep 17, 2013

There is a race between the container destroy and the wait from CmdRun, try to run docker run -rm busybox ls multiple times, sometime you will get:

#client
bin
dev
etc
lib
lib64
proc
sbin
sys
tmp
usr
2013/09/17 12:47:42 Error: No such container: 96ce4d69dba6

#server
[debug] container.go:565 All jobs completed successfully
#the container destroy happen here I believe
[debug] api.go:947 Calling POST /containers/{name:.*}/wait
2013/09/17 12:47:42 POST /v1.5/containers/96ce4d69dba6/wait
[debug] api.go:973 Error: No such container: 96ce4d69dba6
[debug] api.go:71 [error 404] No such container: 96ce4d69dba6

@unclejack
Copy link
Contributor Author

@vieux I'm not sure what should be done to fix this in a way that still lets docker run retrieve the exit code of the container with the current code base. As this has been discussed on IRC as well, I don't see a simple solution for this issue.

-d shouldn't run into issues when used with -rm if the current code base gets modified to not wait if -rm -d are passed to docker. run -i -rm would still run into issues because the exit code will have to be retrieved.

I've asked for feedback on this issue in #docker-dev, but no conclusion was reached. I don't know how to proceed to fix these issues because I don't know what would be the preferred fix for this issue.

@crosbymichael
Copy link
Contributor

@unclejack

@creack Just fixed the issue with the race in master by not using Wait but inspecting the container's state. Could you look at that and see if it solves your problem?

@unclejack
Copy link
Contributor Author

@crosbymichael After having rebased on master, everything seems to be working fine. However, the test for removing containers isn't passing. I'm looking into that now.

@crosbymichael
Copy link
Contributor

awesome!

@vieux
Copy link
Contributor

vieux commented Sep 24, 2013

@unclejack see #1948 and getExitCode in commands.go

@unclejack
Copy link
Contributor Author

@vieux @crosbymichael This should be ready. The AutoRemove test should be OK now. There was another test which was failing, but it's also failing on master, so I don't think the failure is being caused by these changes.

--- FAIL: TestCreateStartRestartStopStartKillRm (0.96 seconds)
        server_test.go:149: Expected 1 container, 4 found
        server_test.go:183: Expected 0 container, 3 found

@vieux
Copy link
Contributor

vieux commented Sep 25, 2013

LGTM ping @creack @crosbymichael

@keeb-zz
Copy link
Contributor

keeb-zz commented Sep 27, 2013

Please make the commit more concise by squashing and writing a detailed summary of the changes.

add AutoRemove to HostConfig
add -rm flag to docker run
add TestRunAutoRemove to test -rm
docs: add -rm to commandline/command/run
add hostConfig to container monitor
make monitor destroy the container via -rm

This adds support for automatically removing a container after it
exits. The removal of the container is handled on the server side.
@unclejack
Copy link
Contributor Author

@keeb It's done.

@keeb-zz
Copy link
Contributor

keeb-zz commented Sep 27, 2013

QA Review: LGTM

@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Sep 27, 2013
Add -rm to docker run for removing a container on exit
@crosbymichael crosbymichael merged commit db869ec into moby:master Sep 27, 2013
@unclejack unclejack deleted the 479-add_rm_support_to_docker_run branch September 27, 2013 18:46
@andreisoare
Copy link

Not sure if this is the right place to report this, but:

ubuntu@staging-andrei ~ $ sudo docker run -rm ubuntu /bin/date
Fri Oct 25 21:09:47 UTC 2013
2013/10/25 14:09:47 Error: Error destroying container ea4027b6ef13: Unable to unmount container ea4027b6ef1374085c4957c9a3099dd9aa9e7e028999e80d1916dbc5e96de8c4: no such file or directory

So -rm flag doesn't work with any command.

ubuntu@staging-andrei ~ $ sudo docker version
Client version: 0.6.4
Go version (client): go1.1.2
Git commit (client): 2f74b1c
Server version: 0.6.4
Git commit (server): 2f74b1c
Go version (server): go1.1.2
Last stable version: 0.6.4

@vieux
Copy link
Contributor

vieux commented Oct 25, 2013

I'll be fixed by #2385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-remove a container after running it
6 participants