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

Add --rm=dead or --rm=always to ease systemd integration #7245

Closed
virtuald opened this Issue Jul 25, 2014 · 10 comments

Comments

Projects
None yet
8 participants
@virtuald
Copy link

virtuald commented Jul 25, 2014

AFAIK, running a docker container 'properly' from systemd is very tricky, because of things like --rm and start/run/environment and other things. Various people have complained about this a bit, and various solutions exist -- all of which involve running a large shell script in your systemd unit file. I think I would really want the preferred way to run a container from systemd to be simple and not require shell scripting.

What about adding additional options to run --rm?

  • --rm=true/false: same as before
  • --rm=dead: if the name doesn't exist, do same as --rm=true. If a container with the name exists, removes the prior container if it was dead/not running. Raises an error if the current container is already running. Would make #6786 largely unnecessary
  • --rm=always: runs the equivalent of docker kill C; docker rm C; docker run --rm C. See #4212. Honestly, I don't know if this should be added, you could get yourself into trouble with this.

The reason something like --rm=dead is required is because I cannot always guarantee that my container will be killed on reboot, such as during a power failure. Lacking an --rm=dead functionality, I have to write a shell script to do things like stop/rm/etc before I start my container.

The 'proper' way of integrating throwaway containers with systemd would be greatly simplified, and look something like this:

[Service]
ExecStart=docker run --rm=dead --name=C container/name
ExecStop=docker stop C
@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 25, 2014

@virtuald The issue here is --rm is a feature of the CLI.

@virtuald

This comment has been minimized.

Copy link

virtuald commented Jul 25, 2014

Hm, the underlying daemon doesn't implement --rm? That explains why -d and --rm aren't compatible (I haven't dived into docker's source code yet).

Still, having that type of functionality available from the CLI would be desirable, as that's how one writes unit files at the moment. Each one of the actions above have equivalent shell script implementations using various cli commands. For things already interacting with docker directly via socket, I imagine it's less of a hassle to add the extra code to do these types of things.

@fangpenlin

This comment has been minimized.

Copy link

fangpenlin commented Jul 27, 2014

+1 on this. I was trying to manage docker containers with supervisord, and it's like a pain in ass since it's so easy to leave dead container behind. Even you always run docker with --rm, it doesn't help at all. For example:

docker run --rm --name=foobar -p 172.17.42.1:5050:5050 foobar

however, since 5050 port is already in use, so you got an error messages like this:

2014/07/27 03:24:45 Error response from daemon: Cannot start container 08b25a41e30be78410a2556ffe01e720ff0d7bd512a53e8a44d7bceb8d3cf83e: Bind for 172.17.42.1:5050 failed: port is already allocated
2014/07/27 03:24:46 Error response from daemon: Conflict, The name foobar is already assigned to 08b25a41e30b. You have to delete (or rename) that container to be able to assign foobar to a container again.
2014/07/27 03:24:48 Error response from daemon: Conflict, The name foobar is already assigned to 08b25a41e30b. You have to delete (or rename) that container to be able to assign foobar to a container again.
....

It looks like docker run client code didn't clean the dead container for you, simply because it encountered an error. And then supervisord tries to start the process again, it would never succeed since the dead container is not removed. You need to remove the container manually to keep the system running.

This breaks my ansible automatic deployment, I always need to clean dead container manually, this is super annoying.

In the mean time, a possible workaround is to have a script runs docker run command for you instead of running directly from process manager. The script should add --cid option and when it starts, it should look into the cid at certain location in local file system, remove the dead container if it exists. It should also do some signal proxying. Shouldn't be too hard, I am already working on this here: https://github.com/victorlin/rundocker

I think @virtuald is right, the --rm promise made in document is so broken:

--rm=false Automatically remove the container when it exits (incompatible with -d)

It does not only breaks the promise when starting a docker with error, it also does this when you reboot the machine, and I believe there are just way too many cases --rm command doesn't work as it advertised. So, this would be definitely a pretty wanted feature to have for docker.

@fangpenlin

This comment has been minimized.

Copy link

fangpenlin commented Jul 27, 2014

hmmmm, I just realized that it looks like cid files will disappear after reboot. I think I have to make my script more complex, looking into running containers for solving this issue :/

@rgarcia

This comment has been minimized.

Copy link

rgarcia commented Jul 29, 2014

@victorlin we are running into the same frustrations managing docker containers with supervisord: https://gist.github.com/rgarcia/dfdf41844668d4cc13bc

Going to try using your script.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 7, 2015

The way I resolve this is by having an ExecStartPre that does the docker rm -f and ignores errors.

@cjbottaro

This comment has been minimized.

Copy link

cjbottaro commented Mar 13, 2016

Why did all work on this stall out? I followed all the links and there is a lot of discussion, a lot of PRs, etc... and it all ends with comments like "So why was this closed?"

I also share the massive frustration of docker run --rm combined with Supervisor and Ansible.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 13, 2016

@cjbottaro have a look at #20848, which moves the --rm functionality from the client to the daemon

@cjbottaro

This comment has been minimized.

Copy link

cjbottaro commented Mar 13, 2016

Ahh, thank you. Sometimes it's hard to navigate all the issues/pr links.

Here's my workaround in case anyone is interested in the meantime. It's a docker-run shim that cleans up stopped containers first:

#!/usr/bin/env python

import os
import sys
import subprocess

try:
    i = sys.argv.index("--name")
    name = sys.argv[i+1]
except ValueError:
    name == None

if "--rm" in sys.argv and name:
    with open(os.devnull, "w") as devnull:
        subprocess.call(["docker", "rm", name], stdout=devnull, stderr=devnull)

# sys.argv includes the program name (this script), so we copy and replace it.
# Note that the args must include "docker" as the first arg, despite it
# being redundant in the execvp call.
args = ["docker", "run"] + sys.argv[1:]

# docker run ...
os.execvp("docker", args)

Note that it only works when you use --name some_name. It doesn't work if you do your name argument like --name=some_name

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Sep 10, 2016

I'm tentatively closing as it should be solved by #20848. Please confirm and ping us to reopen if that's not the case!

@icecrime icecrime closed this Sep 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment