Move --rm to daemon side #20848

WeiZhang555 · 2016-03-02T06:11:54Z

Fixes #20334
Fixes #16575
Fixes #6003
Fixes #20744
Fixes #12157
So many related issues, I won't list them one by one.

--rm is a client side flag which caused lots of problems:

if client lost connection to daemon, including client crash or be killed, there's no way to clean garbage container.
if docker stop a --rm container, this container won't be autoremoved.
if docker daemon restart, container is also left over.
bug: docker run --rm busybox fakecmd will exit without cleanup.

In a word, client side --rm flag isn't sufficient for garbage collection. Move the --rm flag to daemon will be more reasonable.

What this commit do is:

implement a --rm on daemon side, adding one flag AutoRemove into HostConfig.
allow run --rm -d, no conflicting --rm and -d any more, auto-remove can work on detach mode.
docker restart a --rm container will succeed, the container won't be autoremoved.

This commit will help a lot for daemon to do garbage collection for temporary containers for short term job.

Signed-off-by: Zhang Wei zhangwei555@huawei.com

WeiZhang555 · 2016-03-02T06:14:52Z

And if we gracefully shut down daemon, container will be auto removed, but if we forcefully shut down daemon(for example, press CTRL+C three time on daemon), container will be left over. I'm still working on this.

Also test cases are on working.

ping @vdemeester @duglin @tianon @calavera for design review.

tianon · 2016-03-02T06:16:41Z

Your description of the proposed behavior seems sane IMO 👍

(So happy to see someone tackling this again 😄)

phemmer · 2016-03-02T13:33:45Z

Happy to see this feature. Do we want to also allow something like --rm=always and --rm=on-success, similar to the restart policy (with the default --rm behaving like --rm=always)? That way if a container fails for some reason, we give the user the option of inspecting it.
This wouldn't have to be implemented in this PR since it would a new feature.

WeiZhang555 · 2016-03-02T14:00:55Z

@phemmer Is there any scenario or benefit for --rm=always style user interface? It seems complicated and unnecessary from my point of view.

So my current thought is keeping daemon side --rm flag similar to previous behavior, which means --rm is a bool option instead of string option.

I'm glad to hear more feedbacks from you 😄

duglin · 2016-03-02T15:19:49Z

daemon/restart.go

@@ -39,8 +39,10 @@ func (daemon *Daemon) containerRestart(container *container.Container, seconds i
 		return err
 	}

-	if err := daemon.containerStart(container); err != nil {
-		return err
+	if !container.HostConfig.AutoRemove {


I'm not sure about this one. I think we should allow the client to restart a container w/o it being removed. Granted this means that "docker stop" and "docker start" will not be the exact same as "docker restart" in this case but I think this is what the user would expect - otherwise they wouldn't be trying to do "docker restart" at all.

This is limited by current design of restart policy, if we run docker stop on one container, it will stop immediately no matter what its restart policy is. According to our previous discussion, --rm should mean "delete the container when the container stops and won't restart any more", so if it stops, the container is removed, there's no need to start it again, and the start is bound to fail.

That's why I'm saying:
"4. docker restart a --rm container will behave like a docker stop, container will be stopped and removed."

It's also restricted by technique, I can't find a method to distinguish stop command from docker restart or pure docker stop, I can only detect the container is stopped and should be removed.

But if you don't add this if-statement then I think it'll act the way the end user expects. There's a difference between "docker stop" and "docker restart". Stop is asking for the container to fully stop running. Restart is just asking for it to be rebooted NOT stopped - not unlike a daemon reboot would. If we don't do this then there's no difference between "docker stop" and "docker restart" - and I don't think that's correct. A restart is the user's way of explicitly telling us that they don't want the container to be fully stopped, otherwise they would just use "stop".

I totally agree with you about the behavior, but I think remove this if clause won't make it work. Maybe I need to do some trick, for example, set --rm=false before restart, and restore it after restart. I'll make some try.

I'll try to play with it too- one thing to try is to set the restart policy to "always" temporarily so that we don't need to explicitly call start ourselves :-)

I'm afraid setting restart policy to always won't work, if you docker stop a container with restart policy "always", it will stop and don't restart. This is strange, I would expect it will restart no matter how it is stopped.

I guess this is also limited by technique.

duglin · 2016-03-02T15:20:53Z

Overall design looks good!! Just one question about "docker restart".

estesp · 2016-03-02T16:39:57Z

Design seems good and looks like behavior is conforming to test expectations, except for one test: TestExternalVolumeDriverUnnamed is not getting removal of the volume properly, it seems.

@duglin's question is interesting, but was an impossible case in current --rm; once stopped your container was always gone, so there was no way to even consider restart of a auto-removed container.

WeiZhang555 · 2016-03-02T16:49:16Z

@estesp I'll try to fix the test case.
I'm planning to play a trick for docker restart, to set the "AutoRemove" flag to false before container stop and restore it after container stop. Then the docker restart can ignore AutoRemove flag.
I make a little test and it works, what do you think of this?

WeiZhang555 · 2016-03-02T16:58:40Z

Code for docker restart is updated, it's a little tricky but works. So I'll define current behavior of docker restart as:

4. `docker restart` a `--rm` container will succeed, the container won't be autoremoved.

What do you think of this? About bahavior and code implementation. @duglin @estesp

duglin · 2016-03-02T17:01:29Z

@WeiZhang555 yes something like that looks good. I need to think more about the use of the defer in that case but in general yes!

duglin · 2016-03-02T19:20:06Z

daemon/restart.go

+	// set AutoRemove flag to false before stop so the container won't be
+	// removed during restart process
+	autoRemove := container.HostConfig.AutoRemove
+	defer func() {


Let me know if you think I'm being paranoid, but I'd prefer this:

// set AutoRemove flag to false before stop so the container won't be // removed during restart process autoRemove := container.HostConfig.AutoRemove container.HostConfig.AutoRemove = false err := daemon.containerStop(container, seconds) container.HostConfig.AutoRemove = autoRemove if err != nil { return err } if err := daemon.containerStart(container); err != nil { return err }

because I'm worried about the defer resetting the container.HostConfig.AutoRemove when perhaps it shouldn't. For example, normally I wouldn't expect containerStart() to change that value but you never know what might happen in the future. So to prevent unexpected side-effects of the defer, I think it would be safer to just clear and then reset AutoRemove right around the containerStop() since that's really the only code we need to be worried about.

This is good, I will modify as per your suggestion 😄

duglin · 2016-03-02T19:30:45Z

keep forgetting that this is still in design review, so making it official: SGTM

thaJeztah · 2016-03-03T11:25:35Z

Design sgtm as well, some questions / thoughts:

do we want AutoRemove to be a boolean in HostConfig, or make it a string with a default (e.g. "on-stop"). This would allow future expansion without having to change its type
how will this interact with older clients? i.e. a docker 1.10 client will do the remove client-side. oh, think this would work correctly, because older clients won't pass the "remove" policy to the daemon, so should be ok
new clients talking to an older daemon; do we want to support that? i.e. should the client be aware it's talking to an older daemon and (in that case), do the remove client-side as we do right now?

vdemeester · 2016-03-03T12:18:09Z

new clients talking to an older daemon; do we want to support that? i.e. should the client be aware it's talking to an older daemon and (in that case), do the remove client-side as we do right now?

Good point, what should we do in case of DOCKER_API_VERSION=… ? 👼

duglin · 2016-03-03T12:27:06Z

Don't we have a policy of not supporting newer clients with older daemons? If nothing else I think the daemon will complain, no?

thaJeztah · 2016-03-03T12:40:56Z

We don't really support new client + older daemon, so should probably be fine. Guess the daemon will silently ignore the option, just the user may be surprised that the container is not removed in that case

WeiZhang555 · 2016-03-03T16:17:00Z

do we want AutoRemove to be a boolean in HostConfig, or make it a string with a default (e.g. "on-stop"). This would allow future expansion without having to change its type

Yes, a string type will be better for future extension, but I can't identify its scenarios clearly. For example:

"--rm" can represent "when" to auto remove, so "--rm=on-stop" and "--rm=?", what can be another value?
"--rm" can be "How", so "--rm=rm-with-volume" or "--rm=rm-without-volumes", but I think current default behavior is already good enough, which means always remove with anonymous volumes, if I understand it right.

So maybe let "--rm bool" only do simple auto clean work is already good enough?

how will this interact with older clients? i.e. a docker 1.10 client will do the remove client-side. oh, think this would work correctly, because older clients won't pass the "remove" policy to the daemon, so should be ok

Yes, it will work.

new clients talking to an older daemon; do we want to support that? i.e. should the client be aware it's talking to an older daemon and (in that case), do the remove client-side as we do right now?

Ah, this is awful, I prefer not support this. Just did a quick test, new client+older daemon, it reports:

$ docker run -tid --rm busybox sh
docker: Error response from daemon: client is newer than server (client API version: 1.23, server API version: 1.21).
See 'docker run --help'.

Older daemon did a good job, I think this is already good enough.

What is everyone thinking of these?

Docs for moby#20848: move "--rm" to daemon side. Add description for introduced API changes. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

codingground · 2016-11-30T05:20:31Z

Hi,

May I know when docker 1.13.0 is coming out?

thaJeztah · 2016-11-30T09:14:41Z

@codingground release candidates are available for testing now https://github.com/docker/docker/releases

codingground · 2016-12-07T08:42:21Z

Thanks ThaJeztah. Its running great and --rm option with server is lovely. Now I'm able to create upto 1000 instances with ease and my Coding Ground is up and running without any further issue.

http://www.tutorialspoint.com/codingground.htm

tiborvass · 2016-12-07T22:40:42Z

@codingground thanks for confirming!

907th · 2017-08-01T11:40:48Z

This seems not to work with docker-compose.

cpuguy83 · 2017-08-01T12:27:34Z

@907th Not sure what that means, but if you have a specific issue with docker compose, please open an issue on github.com/docker/compose.

Thanks.

thaJeztah · 2017-08-01T12:27:50Z

@907th please don't comment on old PR's and issues, I answered on your other comment; #28747 (comment)

thaJeztah · 2017-08-01T12:29:43Z

I'm locking the conversation on this PR. If you arrive here to report a bug, please open a new issue with details. Commenting on closed PR's is not the right place to report bugs, as they easily go unnoticed, and may miss the required information.

GordonTheTurtle added the status/0-triage label Mar 2, 2016

WeiZhang555 force-pushed the move-rm-to-daemon branch 2 times, most recently from 346a55c to 16bfba2 Compare March 2, 2016 06:32

coolljt0725 added status/1-design-review and removed status/0-triage labels Mar 2, 2016

WeiZhang555 force-pushed the move-rm-to-daemon branch 4 times, most recently from 92300d6 to 3702383 Compare March 2, 2016 13:25

duglin reviewed Mar 2, 2016
View reviewed changes

WeiZhang555 force-pushed the move-rm-to-daemon branch from 3702383 to c2fcacb Compare March 2, 2016 16:52

duglin reviewed Mar 2, 2016
View reviewed changes

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

WeiZhang555 force-pushed the move-rm-to-daemon branch from c2fcacb to 9568a41 Compare March 3, 2016 16:35

icecrime removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 3, 2016

yongtang mentioned this pull request Sep 5, 2016

Update man page for docker create to add --rm flag #26320

Merged

johnflavin mentioned this pull request Sep 13, 2016

Question -> docker run --rm spotify/docker-client#488

Closed

thaJeztah mentioned this pull request Sep 20, 2016

short running containers with -rm failed to destroy #3968

Closed

hannes-ucsc mentioned this pull request Sep 21, 2016

Handle post-calling-job fate of containers (resolves #14) BD2KGenomics/toil-lib#31

Merged

This was referenced Sep 27, 2016

docker build fails with "Error removing intermediate container: The given container is <nil>" #8663

Closed

Copying files to container on run before executing ENTRYPOINT/CMD #26944

Closed

resouer pushed a commit to resouer/docker that referenced this pull request Oct 24, 2016

Add docs for auto-removal on daemon

71fecfa

Docs for moby#20848: move "--rm" to daemon side. Add description for introduced API changes. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

shaynewang mentioned this pull request Oct 27, 2016

docker run Conflicting options: --rm and -d #27812

Closed

thaJeztah mentioned this pull request Nov 15, 2016

Handling of --rm broken when talking to older daemons #28439

Closed

thaJeztah mentioned this pull request Nov 23, 2016

AutoRemove always false #28747

Closed

coolljt0725 mentioned this pull request Nov 30, 2016

Docker -rm implementation for run -d option #28970

Closed

vshlapakov mentioned this pull request Jan 31, 2017

image test: drop check for sh-ep-scrapy scrapinghub/shub#229

Merged

sudo-bmitch mentioned this pull request Feb 7, 2017

docker run --rm reports error attempting to remove container in version 1.13 with 1.24 API #30775

Closed

fryfrog mentioned this pull request Mar 7, 2017

Switch create to run, use --rm and add padding. linuxserver/docker-radarr#6

Closed

jlhawn mentioned this pull request Mar 13, 2017

Use ContainerWait() before starting container #31794

Closed

mapk0y mentioned this pull request Mar 27, 2017

Update "man" of docker-run #32146

Merged

dongluochen mentioned this pull request Apr 8, 2017

Auto removal with '--rm', does not remove container or produce error docker-archive/classicswarm#2647

Closed

illera88 mentioned this pull request May 8, 2017

Simultaneous detach and remove not supported docker/docker-py#1594

Closed

ipazc mentioned this pull request May 25, 2017

Incompatible flags -d and --rm in running examples from docker. ipazc/oculus-crawl#1

Open

strund3r mentioned this pull request Jun 20, 2017

Running container with Docker Run using the flags -d and --rm docker/docs#3666

Closed

ixdy mentioned this pull request Jul 5, 2017

build/ docker scripts hang on Docker 17.06.0-ce kubernetes/kubernetes#48478

Closed

moby locked and limited conversation to collaborators Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move --rm to daemon side #20848

Move --rm to daemon side #20848

WeiZhang555 commented Mar 2, 2016 •

edited by thaJeztah

WeiZhang555 commented Mar 2, 2016

tianon commented Mar 2, 2016

phemmer commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

duglin Mar 2, 2016

WeiZhang555 Mar 2, 2016

WeiZhang555 Mar 2, 2016

duglin Mar 2, 2016

WeiZhang555 Mar 2, 2016

duglin Mar 2, 2016

WeiZhang555 Mar 2, 2016

duglin commented Mar 2, 2016

estesp commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

duglin commented Mar 2, 2016

duglin Mar 2, 2016

WeiZhang555 Mar 3, 2016

duglin commented Mar 2, 2016

thaJeztah commented Mar 3, 2016

vdemeester commented Mar 3, 2016

duglin commented Mar 3, 2016

thaJeztah commented Mar 3, 2016

WeiZhang555 commented Mar 3, 2016

codingground commented Nov 30, 2016

thaJeztah commented Nov 30, 2016

codingground commented Dec 7, 2016

tiborvass commented Dec 7, 2016

907th commented Aug 1, 2017

cpuguy83 commented Aug 1, 2017

thaJeztah commented Aug 1, 2017

thaJeztah commented Aug 1, 2017

Move --rm to daemon side #20848

Move --rm to daemon side #20848

Conversation

WeiZhang555 commented Mar 2, 2016 • edited by thaJeztah

WeiZhang555 commented Mar 2, 2016

tianon commented Mar 2, 2016

phemmer commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

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

duglin commented Mar 2, 2016

estesp commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

WeiZhang555 commented Mar 2, 2016

duglin commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Mar 2, 2016

thaJeztah commented Mar 3, 2016

vdemeester commented Mar 3, 2016

duglin commented Mar 3, 2016

thaJeztah commented Mar 3, 2016

WeiZhang555 commented Mar 3, 2016

codingground commented Nov 30, 2016

thaJeztah commented Nov 30, 2016

codingground commented Dec 7, 2016

tiborvass commented Dec 7, 2016

907th commented Aug 1, 2017

cpuguy83 commented Aug 1, 2017

thaJeztah commented Aug 1, 2017

thaJeztah commented Aug 1, 2017

WeiZhang555 commented Mar 2, 2016 •

edited by thaJeztah