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

yongtang · 2016-05-27T01:26:11Z

- What I did

This fix tries to add a daemon config parameter --shutdown-timeout that specifies the timeout value to stop containers gracefully (before SIGKILL). The default value is 15s.

- How I did it

The --shutdown-timeout parameter is added to daemon options and config file. It will also be updated during daemon reload.

- How to verify it

Additional test cases have been added to cover the change.

- Description for the changelog

Add --shutdown-timeout to dockerd to specify the timeout (in seconds) to stop containers gracefully before daemon exit.

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

This fix is related to #22471 and #22566.

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

thaJeztah · 2016-07-14T18:52:41Z

Ok, we're okay with adding this, so moving to code review.

can you rebase?

yongtang · 2016-07-15T23:46:15Z

Thanks @thaJeztah the pull request has been rebased.

thaJeztah · 2016-07-29T11:12:28Z

sorry @yongtang, needs another rebase

yongtang · 2016-07-29T14:21:00Z

Thanks @thaJeztah, the PR has been rebased.

vdemeester · 2016-08-25T15:25:53Z

LGTM 🐸

cpuguy83 · 2016-08-25T15:39:58Z

daemon/config.go

@@ -121,6 +125,10 @@ type CommonConfig struct {
 	// may take place at a time for each push.
 	MaxConcurrentUploads *int `json:"max-concurrent-uploads,omitempty"`

+	// ShutdownTimeout is the timeout value (in seconds) the daemon will wait for the container
+	// to stop when daemon is being shutdown
+	ShutdownTimeout int `json:"shutdown-timeout,omitempty"`


time.Duration ?

@cpuguy83 One potential issue with time.Duration is that the default marshal/unmarshal of time.Duration is nano-seconds (cannot parse a string like "5s"). In other words, if user wants to specify 5 seconds, it will be "shutdown-timeout": 5000000000, which might not be ideal.

Maybe we could use a customized type, e.g.,

type Duration struct { time.Duration } func (d Duration) MarshalJSON() ([]byte, error) {

so that user could specify 5s instead of 5000000000?

I would expect this to be handled on the client, not the API (even though I'm sure we probably do this elsewhere...)

@cpuguy83 The --shutdown-timeout is the global timeout and is handled in dockerd. It could be unmarshaled from --config-file=/etc/docker/daemon.json.

Another PR (#22566) is the per-container timeout which could be handled in client.

Maybe for this PR we use seconds , and for the other PR we could change the per-container timeout (in #22566) to time.Duration?

vdemeester · 2016-09-04T16:13:39Z

@yongtang needs a rebase 😓

yongtang · 2016-09-04T22:37:51Z

Thanks @vdemeester. The pull request has been rebased.

vdemeester · 2016-09-23T12:52:07Z

LGTM 🐸

mlaventure

Couple of nits.

LGTM otherwise.

Also, requires a rebase.

mlaventure · 2016-10-17T17:19:31Z

daemon/daemon.go

@@ -730,7 +736,7 @@ func (daemon *Daemon) Shutdown() error {
 	}

 	if daemon.containers != nil {
-		logrus.Debug("starting clean shutdown of all containers...")
+		logrus.Debugf("starting clean shutdown of all containers in %d seconds...", daemon.configStore.ShutdownTimeout)


The wording feels to me like we are only going to start shutting down the containers after ShutdownTimeout seconds have expired.

Maybe use something like start clean shutdown of all containers with a %d seconds timeout...?

cc @thaJeztah for better wording 😅

Thanks @mlaventure @thaJeztah . I will temporarily settle with start clean shutdown of all containers with a %d seconds timeout... but let me know if there are other suggestions.

mlaventure · 2016-10-17T17:20:12Z

docs/reference/commandline/dockerd.md

@@ -1125,6 +1126,7 @@ This is a full example of the allowed configuration options on Linux:
 	"cluster-advertise": "",
 	"max-concurrent-downloads": 3,
 	"max-concurrent-uploads": 5,
+        "shutdown-timeout": 15,


alignment issue

Thanks @mlaventure. Done.

yongtang · 2016-10-17T20:32:42Z

The PR has been rebased and updated. Please let me know if there are other issues.

mlaventure · 2016-10-17T20:35:08Z

LGTM if still 💚

ping @vdemeester @cpuguy83 @thaJeztah

thaJeztah · 2016-10-17T20:49:16Z

docs/reference/commandline/dockerd.md

@@ -64,6 +64,7 @@ Options:
      --raw-logs                              Full timestamps without ANSI coloring
      --registry-mirror value                 Preferred Docker registry mirror (default [])
      --selinux-enabled                       Enable selinux support
+      --shutdown-timeout=15                   Set the shutdown timeout value in seconds


don't forget to update the man page as well https://github.com/docker/docker/blob/master/man/dockerd.8.md

Thanks @thaJeztah. The man page has been updated.

thaJeztah · 2016-10-17T20:50:22Z

docs/reference/commandline/dockerd.md

@@ -1118,6 +1119,7 @@ This is a full example of the allowed configuration options on Linux:
 	"cluster-advertise": "",
 	"max-concurrent-downloads": 3,
 	"max-concurrent-uploads": 5,
+	"shutdown-timeout": 15,


It's only supported on Linux, or Windows as well? If so, the "Windows" example needs to be updated as well 😄

Thanks @thaJeztah. The docs has been updated.

thaJeztah · 2016-10-17T20:53:50Z

left some docs notes already; @mlaventure IIRC, systemd will kill all processes if the systemd (unit-file) timeout is exceeded; do you think we should mention that in the documentation?

Also, this'll need changes in the bash and zsh completion scripts @albers @sdurrheimer

mlaventure · 2016-10-17T21:00:00Z

@thaJeztah I wouldn't make it systemd specific. Most service manager would have a similar (configurable) behavior.

Mentioning that their service manager may kill docker regardless if the daemon is stopped via it wouldn't hurt though.

thaJeztah

LGTM

ping @vdemeester PTAL

vdemeester

docs LGTM 🐸

vdemeester · 2016-10-18T12:34:08Z

@yongtang needs a rebase though 👼

This fix tries to add a daemon config parameter `--shutdown-timeout` that specifies the timeout value to stop containers gracefully (before SIGKILL). The default value is 15s. The `--shutdown-timeout` parameter is added to daemon options and config file. It will also be updated during daemon reload. Additional test cases have been added to cover the change. This fix fixes moby#22471. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-10-18T18:01:28Z

Thanks @vdemeester. The PR has been rebased and all tests passed.

mlaventure · 2016-10-19T16:45:57Z

ping @thaJeztah @vdemeester final look before merging? :)

vdemeester · 2016-10-19T16:47:44Z

Still LGTM 👼
merging 😝

yongtang · 2016-10-19T16:54:18Z

Thanks all for the review and help! 👍

albers · 2016-10-19T17:01:46Z

Added bash completion: #27550

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

yongtang mentioned this pull request May 27, 2016

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

Merged

vdemeester added status/2-code-review and removed status/0-triage labels May 27, 2016

thaJeztah mentioned this pull request May 27, 2016

More graceful daemon restarts #9433

Closed

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from bc87e08 to bd9183b Compare June 1, 2016 04:01

LK4D4 added status/1-design-review and removed status/2-code-review labels Jun 20, 2016

thaJeztah added status/2-code-review and removed status/1-design-review labels Jul 14, 2016

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from bd9183b to 023477b Compare July 15, 2016 23:45

thaJeztah added this to the 1.13.0 milestone Jul 22, 2016

thaJeztah mentioned this pull request Jul 22, 2016

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

Closed

thaJeztah added the impact/changelog label Jul 29, 2016

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from 023477b to b39cb13 Compare July 29, 2016 14:13

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from b39cb13 to 80b0923 Compare August 13, 2016 21:50

cpuguy83 reviewed Aug 25, 2016
View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from 80b0923 to d4111ab Compare August 25, 2016 19:08

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from d4111ab to bbc01a1 Compare September 4, 2016 22:37

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch 2 times, most recently from 30da883 to 4ddbe01 Compare September 19, 2016 23:35

mlaventure suggested changes Oct 17, 2016

View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from 4ddbe01 to d991cf6 Compare October 17, 2016 20:30

mlaventure approved these changes Oct 17, 2016

View reviewed changes

thaJeztah reviewed Oct 17, 2016

View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from d991cf6 to 3945662 Compare October 18, 2016 00:44

thaJeztah approved these changes Oct 18, 2016

View reviewed changes

vdemeester approved these changes Oct 18, 2016

View reviewed changes

yongtang force-pushed the 22471-daemon-shutdown-timeout-daemon branch from 3945662 to d7be6b2 Compare October 18, 2016 15:09

mlaventure added status/3-docs-review and removed status/2-code-review labels Oct 18, 2016

vdemeester merged commit 4a4f028 into moby:master Oct 19, 2016

yongtang deleted the 22471-daemon-shutdown-timeout-daemon branch October 19, 2016 16:54

albers mentioned this pull request Oct 19, 2016

Add bash completion for dockerd --shutdown-timeout #27550

Merged

sdurrheimer mentioned this pull request Oct 25, 2016

Add zsh completion for 'dockerd --shutdown-timeout' #27716

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 Apr 6, 2017

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

Open

KevinTHU mentioned this pull request Jun 22, 2017

v1.9.1 unable to remove network due to active endpoints #19261

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yongtang commented May 27, 2016

thaJeztah commented Jul 14, 2016

yongtang commented Jul 15, 2016

thaJeztah commented Jul 29, 2016

yongtang commented Jul 29, 2016

vdemeester commented Aug 25, 2016

cpuguy83 Aug 25, 2016

yongtang Aug 25, 2016

cpuguy83 Aug 25, 2016

yongtang Aug 25, 2016

vdemeester commented Sep 4, 2016

yongtang commented Sep 4, 2016

vdemeester commented Sep 23, 2016

mlaventure left a comment

mlaventure Oct 17, 2016

yongtang Oct 17, 2016

mlaventure Oct 17, 2016

yongtang Oct 17, 2016

yongtang commented Oct 17, 2016

mlaventure commented Oct 17, 2016

thaJeztah Oct 17, 2016

yongtang Oct 18, 2016

thaJeztah Oct 17, 2016

yongtang Oct 18, 2016

thaJeztah commented Oct 17, 2016

mlaventure commented Oct 17, 2016

thaJeztah left a comment

vdemeester left a comment

vdemeester commented Oct 18, 2016

yongtang commented Oct 18, 2016

mlaventure commented Oct 19, 2016

vdemeester commented Oct 19, 2016

yongtang commented Oct 19, 2016

albers commented Oct 19, 2016

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

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

Conversation

yongtang commented May 27, 2016

thaJeztah commented Jul 14, 2016

yongtang commented Jul 15, 2016

thaJeztah commented Jul 29, 2016

yongtang commented Jul 29, 2016

vdemeester commented Aug 25, 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

vdemeester commented Sep 4, 2016

yongtang commented Sep 4, 2016

vdemeester commented Sep 23, 2016

mlaventure left a 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

yongtang commented Oct 17, 2016

mlaventure commented Oct 17, 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

thaJeztah commented Oct 17, 2016

mlaventure 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

vdemeester commented Oct 18, 2016

yongtang commented Oct 18, 2016

mlaventure commented Oct 19, 2016

vdemeester commented Oct 19, 2016

yongtang commented Oct 19, 2016

albers commented Oct 19, 2016