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

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

Merged
merged 2 commits into from Oct 18, 2016

Conversation

Projects
None yet
@yongtang
Copy link
Member

yongtang commented May 7, 2016

- 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

This comment has been minimized.

Copy link

pataquets commented May 9, 2016

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

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented May 9, 2016

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented May 9, 2016

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

This comment has been minimized.

Copy link
Contributor

LK4D4 commented May 9, 2016

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented May 9, 2016

@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

This comment has been minimized.

Copy link
Member Author

yongtang commented May 10, 2016

@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:

  1. shutdownContainer() (with a timeout of 10s):
    https://github.com/docker/docker/blob/e16753ce192cc80d3e207d7b3063a9dab36983cb/daemon/daemon.go#L860-L864
  2. 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:

  1. Send SIGTERM to each container, wait for 10s, and send SIGKILL if containers hasn't been stopped yet.
  2. 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

This comment has been minimized.

Copy link
Contributor

icecrime commented May 11, 2016

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 yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch from d5e1c04 to d8d49a3 May 24, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented May 24, 2016

@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

This comment has been minimized.

Copy link
Member

thaJeztah commented May 24, 2016

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 yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch 4 times, most recently from 21b2fcd to 0862031 May 24, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented May 25, 2016

@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

This comment has been minimized.

Copy link

mattyb commented May 25, 2016

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


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

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 26, 2016

Contributor

Why Linux only?

This comment has been minimized.

Copy link
@yongtang

yongtang May 27, 2016

Author Member

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented May 26, 2016

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 yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch from 0862031 to 3276329 May 27, 2016

@yongtang 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

This comment has been minimized.

Copy link
Member Author

yongtang commented May 27, 2016

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

This comment has been minimized.

Copy link
Member

thaJeztah commented May 27, 2016

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

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

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Aug 25, 2016

Contributor

why would this be < 0?

This comment has been minimized.

Copy link
@yongtang

yongtang Aug 25, 2016

Author Member

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

@vdemeester

This comment has been minimized.

Copy link
Member

vdemeester commented Sep 4, 2016

@yongtang needs another rebase 😓

@yongtang yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch from 09abc2f to 90aedbd Sep 4, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented Sep 4, 2016

Thanks @vdemeester, rebased.

@yongtang yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch from 90aedbd to 45c5cf5 Sep 17, 2016

@mlaventure

This comment has been minimized.

Copy link
Contributor

mlaventure commented Oct 17, 2016

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

yongtang added some commits May 26, 2016

Add config parameter to change per-container stop timeout during daem…
…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 #22471. Another pull request will add `--shutdown-timeout`
to daemon for #22471.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Update `docker stop` and `docker restart` to allow not specifying tim…
…eout and use the one specified at container creation time.

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

@yongtang yongtang force-pushed the yongtang:22471-daemon-shutdown-timeout branch from 45c5cf5 to cc70378 Oct 17, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented Oct 17, 2016

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

@thaJeztah
Copy link
Member

thaJeztah left a comment

docs LGTM, thanks!

ping @albers @sdurrheimer for completion scripts ❤️

@vdemeester
Copy link
Member

vdemeester left a comment

LGTM 🐸

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

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25006 has succeeded
Details
janky Jenkins build Docker-PRs 33603 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 4457 has succeeded
Details

@yongtang yongtang deleted the yongtang:22471-daemon-shutdown-timeout branch Oct 18, 2016

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 18, 2016

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

@yongtang

This comment has been minimized.

Copy link
Member Author

yongtang commented Oct 19, 2016

Thanks all for the review and help!

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13
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>

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…
…eout

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

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017

Add version annotation to various flags added in 1.13
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

srust added a commit to srust/moby that referenced this pull request Nov 30, 2017

Add version annotation to various flags added in 1.13
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.