Add config parameter to change per-container stop timeout during daemon shutdown #22566

yongtang · 2016-05-07T01:29:03Z

- What I did

This fix tries to add a flag --stop-timeout to specify the timeout value (in seconds) for the container to stop before SIGKILL is issued. If stop timeout is not specified then the default timeout (10s) is used.

- How I did it

The --stop-timeout has been added to docker create and docker run.

- How to verify it

Additional test cases have been added to cover the change.

- Description for the changelog

Added a lag --stop-timeout to specify the timeout value (in seconds) for the container to stop.

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

This fix is related to #22471.

NOTE: Pull requests have been created in engine-api:
docker/engine-api#256
docker/engine-api#271

NOTE: Another pull request (#23036) has been opened to add --shutdown-timeout to daemon

Signed-off-by: Yong Tang yong.tang.github@outlook.com

pataquets · 2016-05-09T10:48:23Z

Will this option match the corresponding docker stop --time=-1 parameter value to enable waiting undefinitely for the container to stop?

yongtang · 2016-05-09T15:06:23Z

@pataquets Yes if the option is passed with -1 then the daemon will wit indefinitely for clean stop of the containers.

thaJeztah · 2016-05-09T15:19:49Z

I'm wondering if this should be a daemon level option, or something that should be specified per container (thinking of the --stop-signal option we added on docker run, and in the Dockerfile).

I like the simplicity of this PR, just thinking out loud if the timeout should be applied to all containers, or only to specific ones.

LK4D4 · 2016-05-09T21:46:57Z

I think restarts without container stops(in 1.12) will deprecate this.

thaJeztah · 2016-05-09T22:09:57Z

@LK4D4 that was mentioned in the issue, but it likely won't help if the service has to be stopped, or a server rebooted

yongtang · 2016-05-10T02:29:36Z

@thaJeztah @LK4D4 Thanks for the review.

In the existing docker code, the timeout of container stop before SIGKILL during the shutdown is controlled by two places:

shutdownContainer() (with a timeout of 10s):
https://github.com/docker/docker/blob/e16753ce192cc80d3e207d7b3063a9dab36983cb/daemon/daemon.go#L860-L864
shutdownDaemon() (with a timeout of 15s = 10s + 5s):
https://github.com/docker/docker/blob/e16753ce192cc80d3e207d7b3063a9dab36983cb/cmd/dockerd/daemon.go#L290-L294

When a shutdown of daemon is issued, the daemon will:

Send SIGTERM to each container, wait for 10s, and send SIGKILL if containers hasn't been stopped yet.
At the same time, wait for all containers to stop or 15s (max), then exit.

There are potentially two variables. One is the timeout (10s) for each container to stop (with SIGTERM) before send SIGKILL. The other is the timeout (15s) for daemon to exit.

In this PR, there is only one variable --shutdown-timeout. Basically each container will wait for shutdown-timeout before SIGKILL is issued. At the same time, the daemon will wait for shutdown-time + 5s for all containers to stop, before exit. The added value of 5s is hardcoded for the moment in this PR.

I think the per-container-stop-timeout makes sense. Maybe we could set a per-container-stop-timeout for each container, then add a shutdown-timeout for the daemon to wait overall?

Any suggestions would be appreciated.

icecrime · 2016-05-11T23:38:00Z

I think this makes sense (regardless of the potential ability for 1.12 to restart the daemon without restarting containers).

Every such PR makes me think we should really stop adding command line flags and support a configuration file only...

yongtang · 2016-05-24T14:52:19Z

@thaJeztah @LK4D4 @icecrime To implement a per-container-stop-timeout the following items are likely to be needed:

Add a field StopTimeout to engine-api/types/container/config.go (StopSignal already exists)
Add a flag of --stop-timeout=10 to docker run and docker create
Add a flag of --shutdown-timeout=15 to docker daemon for the wait time before forcefully exit daemon.
When the daemon is shutdown, each container will be stopped based on the StopTimeout in config before issuing a SIGKILL. In addition, daemon will wait for --shutdown-timeout before forcefully exit.
The only potential uncertainly is the docker stop --time=10. When docker stop --time=10 is executed, the stop timeout will be the value passed in command which already has a default value. I think it makes sense for docker stop to respect StopTimeout if the --time flag is not given.

I will move forward to implement the above items and update the PR. In the meantime, please let me know if you have any suggestions or feedbacks.

thaJeztah · 2016-05-24T15:00:45Z

Perhaps we can combine stop-signal with stop-timeout, similar to the --restart=always:10, so something like docker run --stop-signal=kill:30 (downside is that you'd always have to specify the signal as well)

yongtang · 2016-05-25T02:52:58Z

@thaJeztah @LK4D4 @icecrime I updated the PR and now --stop-signal could take both a signal and a timeout e.g., --stop-signal=SIGTERM:10.

The StopTimeout in Config is defined as *int. This is to allow the case where value StopTimeout is not assigned (then default 10s will be used).

Note: we cannot use -1 or 0 as the default value of StopTimeout because -1 means infinity and 0 means no waiting.

One thing I am not so sure is about how to deal with docker stop. Since docker stop -t always has a default value, it seems like the StopTimeout is useless for docker stop.

If we want to respect the StopTimeout even for docker stop, I think we could change the behavior so that:

If docker stop does not have the flag -t then we will use StopTimeout
Otherwise we will use the value provided by docker stop -t

Any suggestion suggestions or feedbacks on how to deal with docker stop?

mattyb · 2016-05-25T04:19:29Z

It may be nice to combine this with --start-timeout, per #22226

lowenna · 2016-05-26T18:23:57Z

integration-cli/docker_cli_daemon_test.go

+
+// Test case for #22471
+func (s *DockerDaemonSuite) TestDaemonShutdownTimeout(c *check.C) {
+	testRequires(c, SameHostDaemon, DaemonIsLinux)


Why Linux only?

@jhowardmsft Thanks for point that out. I will update the PR to cover Windows as well.

thaJeztah · 2016-05-26T18:27:53Z

We just discussed this; and there's more people that like having a separate flag for --stop-timeout after all (sorry).

Also we'd like to have the per-container feature first, and a separate PR for the default (daemon) option.
Can you do that? Thanks!

yongtang · 2016-05-27T01:28:52Z

Thanks @thaJeztah @jhowardmsft I split the pull request into two. This pull request will cover the 'per-container' stop timeout case and the pull request #23036 will cover daemon timeout case. Please let me know if there are any issues.

thaJeztah · 2016-05-27T06:35:49Z

Thanks @yongtang! I'm moving this to code review

yongtang · 2016-08-25T16:55:02Z

Thanks @vdemeester. The pull request has been rebased.

cpuguy83 · 2016-08-25T18:29:26Z

cmd/dockerd/daemon.go

 	ch := make(chan struct{})
 	go func() {
 		d.Shutdown()
 		close(ch)
 	}()
+	if shutdownTimeout < 0 {


why would this be < 0?

@cpuguy83 If it is < 0 then we will wait until daemon is shutdown (no timeout).

vdemeester · 2016-09-04T16:12:10Z

@yongtang needs another rebase 😓

yongtang · 2016-09-04T21:57:20Z

Thanks @vdemeester, rebased.

mlaventure · 2016-10-17T17:14:44Z

LGTM, but this PR needs yet another rebase for the docs 😅

…on shutdown This fix tries to add a flag `--stop-timeout` to specify the timeout value (in seconds) for the container to stop before SIGKILL is issued. If stop timeout is not specified then the default timeout (10s) is used. Additional test cases have been added to cover the change. This fix is related to moby#22471. Another pull request will add `--shutdown-timeout` to daemon for moby#22471. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

…eout and use the one specified at container creation time. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-10-17T19:58:14Z

Thanks @mlaventure for the review. The PR has been rebased and updated. Please let me know if there are any other issues 😅 .

thaJeztah

docs LGTM, thanks!

ping @albers @sdurrheimer for completion scripts ❤️

vdemeester

LGTM 🐸

thaJeztah · 2016-10-18T15:14:48Z

🎉 yeah! Sorry it took so long @yongtang - thanks so much!

yongtang · 2016-10-19T15:44:22Z

Thanks all for the review and help!

Pull request moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm` - moby#27702 added `--network` to `docker build` - moby#25962 added `--attachable` to `docker network create` - moby#27998 added `--compose-file` to `docker stack deploy` - moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby#26061 added `--init` to `docker run` and `docker create` - moby#26941 added `--init-path` to `docker run` and `docker create` - moby#27958 added `--cpus` on `docker run` / `docker create` - moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby#27596 added `--force` to `docker service update` - moby#27857 added `--hostname` to `docker service create` - moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby#28076 added `--tty` on `docker service create` / `docker service update` - moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…eout Add config parameter to change per-container stop timeout during daemon shutdown

Pull request moby/moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm` - moby/moby#27702 added `--network` to `docker build` - moby/moby#25962 added `--attachable` to `docker network create` - moby/moby#27998 added `--compose-file` to `docker stack deploy` - moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby/moby#26061 added `--init` to `docker run` and `docker create` - moby/moby#26941 added `--init-path` to `docker run` and `docker create` - moby/moby#27958 added `--cpus` on `docker run` / `docker create` - moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby/moby#27596 added `--force` to `docker service update` - moby/moby#27857 added `--hostname` to `docker service create` - moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby/moby#28076 added `--tty` on `docker service create` / `docker service update` - moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db Component: cli

GordonTheTurtle added the status/0-triage label May 7, 2016

yongtang mentioned this pull request May 7, 2016

[Feature Request] Add config parameter to change stop timeout for containers or globally #22471

Closed

thaJeztah added status/1-design-review and removed status/0-triage labels May 7, 2016

thaJeztah mentioned this pull request May 12, 2016

stopping containers before killing the daemon #22695

Closed

yongtang force-pushed the 22471-daemon-shutdown-timeout branch from d5e1c04 to d8d49a3 Compare May 24, 2016 14:38

yongtang force-pushed the 22471-daemon-shutdown-timeout branch 4 times, most recently from 21b2fcd to 0862031 Compare May 25, 2016 02:38

thaJeztah added the status/needs-attention Calls for a collective discussion during a review session label May 25, 2016

lowenna reviewed May 26, 2016
View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout branch from 0862031 to 3276329 Compare May 27, 2016 01:18

yongtang changed the title ~~Add config parameter to change stop timeout during daemon shutdown~~ Add config parameter to change per-container stop timeout during daemon shutdown May 27, 2016

yongtang mentioned this pull request May 27, 2016

Add config parameter to change stop timeout during daemon shutdown #23036

Merged

thaJeztah removed the status/needs-attention Calls for a collective discussion during a review session label May 27, 2016

cpuguy83 reviewed Aug 25, 2016
View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout branch from 09abc2f to 90aedbd Compare September 4, 2016 21:56

yongtang force-pushed the 22471-daemon-shutdown-timeout branch from 90aedbd to 45c5cf5 Compare September 17, 2016 22:21

vdemeester added status/3-docs-review and removed status/2-code-review labels Oct 17, 2016

yongtang added 2 commits October 17, 2016 12:28

Update docker stop and docker restart to allow not specifying tim…

cc70378

…eout and use the one specified at container creation time. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 22471-daemon-shutdown-timeout branch from 45c5cf5 to cc70378 Compare October 17, 2016 19:56

thaJeztah approved these changes Oct 17, 2016

View reviewed changes

thaJeztah mentioned this pull request Oct 17, 2016

[epic] add more options to service create / service update #25303

Open

vdemeester approved these changes Oct 18, 2016

View reviewed changes

vdemeester merged commit dad8cbf into moby:master Oct 18, 2016

albers mentioned this pull request Oct 18, 2016

Add bash completion for docker {run,create} --stop-timeout #27492

Merged

yongtang deleted the 22471-daemon-shutdown-timeout branch October 18, 2016 14:40

sdurrheimer mentioned this pull request Oct 25, 2016

Add zsh completion for 'docker {create,run} --stop-timeout' #27715

Merged

simplesteph mentioned this pull request Jan 4, 2017

How to trigger ECS agent actions through API? aws/amazon-ecs-agent#653

Closed

thaJeztah mentioned this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13 #30186

Merged

thaJeztah mentioned this pull request Apr 6, 2017

dockerd shutdown-timeout option does not work as expected #32357

Open

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#22566 from yongtang/22471-daemon-shutdown-tim…

3eeb334

…eout Add config parameter to change per-container stop timeout during daemon shutdown

Anvil mentioned this pull request Nov 20, 2017

docker-py does not honor Config->StopTimeout value when stopping container docker/docker-py#1815

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config parameter to change per-container stop timeout during daemon shutdown #22566

Add config parameter to change per-container stop timeout during daemon shutdown #22566

yongtang commented May 7, 2016 •

edited

Loading

pataquets commented May 9, 2016

yongtang commented May 9, 2016

thaJeztah commented May 9, 2016

LK4D4 commented May 9, 2016

thaJeztah commented May 9, 2016

yongtang commented May 10, 2016

icecrime commented May 11, 2016

yongtang commented May 24, 2016

thaJeztah commented May 24, 2016

yongtang commented May 25, 2016

mattyb commented May 25, 2016

lowenna May 26, 2016

yongtang May 27, 2016

thaJeztah commented May 26, 2016

yongtang commented May 27, 2016

thaJeztah commented May 27, 2016

yongtang commented Aug 25, 2016

cpuguy83 Aug 25, 2016

yongtang Aug 25, 2016

vdemeester commented Sep 4, 2016

yongtang commented Sep 4, 2016

mlaventure commented Oct 17, 2016

yongtang commented Oct 17, 2016

thaJeztah left a comment

vdemeester left a comment

thaJeztah commented Oct 18, 2016

yongtang commented Oct 19, 2016

Add config parameter to change per-container stop timeout during daemon shutdown #22566

Add config parameter to change per-container stop timeout during daemon shutdown #22566

Conversation

yongtang commented May 7, 2016 • edited Loading

pataquets commented May 9, 2016

yongtang commented May 9, 2016

thaJeztah commented May 9, 2016

LK4D4 commented May 9, 2016

thaJeztah commented May 9, 2016

yongtang commented May 10, 2016

icecrime commented May 11, 2016

yongtang commented May 24, 2016

thaJeztah commented May 24, 2016

yongtang commented May 25, 2016

mattyb commented May 25, 2016

lowenna May 26, 2016

Choose a reason for hiding this comment

yongtang May 27, 2016

Choose a reason for hiding this comment

thaJeztah commented May 26, 2016

yongtang commented May 27, 2016

thaJeztah commented May 27, 2016

yongtang commented Aug 25, 2016

cpuguy83 Aug 25, 2016

Choose a reason for hiding this comment

yongtang Aug 25, 2016

Choose a reason for hiding this comment

vdemeester commented Sep 4, 2016

yongtang commented Sep 4, 2016

mlaventure commented Oct 17, 2016

yongtang commented Oct 17, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 18, 2016

yongtang commented Oct 19, 2016

yongtang commented May 7, 2016 •

edited

Loading