Configurable stop/kill behavior on crane run -r, crane lift -r, crane rm -f & crane stop

[bjaglin]

Just a suggestion regarding the now-default kill behavior of run & lift on --recreate: could this be configurable per container, utilizing the crane config? After all, it's up to the config maintainer to decide whether a graceful stop is required (mounted volumes or other side effects outside the container, ongoing requests, etc) or a kill is enough.

To implement that, rm would have a --force instead of --kill (mapping docker semantics), and a new per-container option stop-grace-period (potentially set to 0) would control the default timeout passed to docker stop (used by all code paths potentially stopping a container).

lift [-r] would then become [docker stop -t stop-grace-period && docker rm &&] provision && run. If someone really wants to kill a container despite what the config says, crane kill is still there.

[michaelsauter]

Yea I thought about something similar, but decided to implement just kill for now. It's not perfect yet though.

I thought about adding an optional stop configuration similar to the run configuration for each container. The Docker stop command supports a time which is set to 10 by default (see http://docs.docker.com/reference/commandline/cli/#stop). If we use stop everywhere instead of kill in Crane, and take the time value for every container as an argument, the user can decide how long each container should have time to stop before being killed (so in your case, you would set the time of all containers to 0).

I didn't implement this because
a) docker rm does not support time, which I find weird, so Crane wouldn't map exactly
b) it's a bit cumbersome to add it to all containers
c) a time would set the time for all operations, and maybe you do want to stop it gracefully sometimes, but not at other times ... maybe we could overcome this issue by allowing to override time via a flag when the user calls stop directly.

You're idea of a default time for recreate (that's how I would see it as) sounds good though.

[bjaglin]

Maybe I wasn't clear, but mapping the time argument of docker stop was precisely what I was suggesting. As for your counter-arguments:

1. the current crane rm --kill doesn't map exactly to docker rm -f either
2. as mentioned, crane kill is still there if you need a quick shutdown, ignoring the timeout you have configured in your config

Agreed it's not a priority right now, and in no way a showstopper for, but I wanted to get the conversation started.

[michaelsauter]

Ah, somehow I missed that in the first post. So yea, seems like we had the same idea :)

I'm still unsure about --force to map to docker rm --force as the timeout can't be controlled. I don't quite understand why Docker does it that way. Say you do want a timeout for docker stop in your config, but at some point just decide to kill the containers. The --kill makes that possible, --force does not. I know --kill is not mapping to a Docker command, but that's why I chose to name it --kill instead of --force to make it clear that is doing something different.

[michaelsauter]

Maybe crane stop should get a time flag as well to override the config?

[bjaglin]

About your last comment, why would I want to precisely override the grace period? Either I want to respect what the config says (since the config maintainer knows better what is a sensible timeout), or just kill it right away. That was my point 2). I don't really feel the need for introducing a runtime flag controlling the time, to me it makes more sense to have it only in the config. If you think about it, the config is currently really just the set of flags passed to docker run (that cannot be overriden when running crane run), so that new grace period flag could have the same semantics, except that it's mapped to docker stop.

[michaelsauter]

I think my main concern is that by just offering a stop configuration, people will misuse it to achieve what they would do otherwise by passing --kill.

So, I'm all in for adding a stop configuration to set the time for each container. And yes, probably doesn't make sense to override it. However, it might make sense to kill by default for recreate (or stop by default and offer a --kill option) in order to not have to set the stop time to 0 just to get the containers recreated quickly.

Maybe adding the stop configuration and an additional option --graceful for lift --recreate/run --recreate if you do want to use stop instead of kill? Or I'll just keep the default kill and if you do want to stop gracefully, you'll need to do a stop and then lift / run.

[bjaglin]

Closing this, as the current solution is probably enough, and because https://github.com/dotcloud/docker/pull/6987/files shows that by the next release of docker crane rm & docker rm will actually be aligned with the introduction of -k/--kill in the latter. If needed, we can always introduce the renamed -s/--stop in a separate PR.

[michaelsauter]

Oh nice!

[bjaglin]

@michaelsauter actually that initial PR was reverted in moby/moby#7374, so it's still docker rm -f/--force but it now has the kill semantics as of Docker 1.2.0. Worth aligning crane maybe?

[michaelsauter]

Yup, that sounds reasonably. I'll do that.

[michaelsauter]

Changed in d52a3f6.