Add config parameter to change stop timeout during daemon shutdown #23036
Conversation
bc87e08 to
bd9183b
Compare
|
Ok, we're okay with adding this, so moving to code review. can you rebase? |
bd9183b to
023477b
Compare
|
Thanks @thaJeztah the pull request has been rebased. |
|
sorry @yongtang, needs another rebase |
023477b to
b39cb13
Compare
|
Thanks @thaJeztah, the PR has been rebased. |
b39cb13 to
80b0923
Compare
|
LGTM 🐸 |
daemon/config.go
Outdated
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
I would expect this to be handled on the client, not the API (even though I'm sure we probably do this elsewhere...)
There was a problem hiding this comment.
@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?
80b0923 to
d4111ab
Compare
|
@yongtang needs a rebase 😓 |
d4111ab to
bbc01a1
Compare
|
Thanks @vdemeester. The pull request has been rebased. |
30da883 to
4ddbe01
Compare
|
LGTM 🐸 |
mlaventure
left a comment
There was a problem hiding this comment.
Couple of nits.
LGTM otherwise.
Also, requires a rebase.
daemon/daemon.go
Outdated
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
4ddbe01 to
d991cf6
Compare
|
The PR has been rebased and updated. Please let me know if there are other issues. |
|
LGTM if still 💚 |
There was a problem hiding this comment.
don't forget to update the man page as well https://github.com/docker/docker/blob/master/man/dockerd.8.md
There was a problem hiding this comment.
Thanks @thaJeztah. The man page has been updated.
There was a problem hiding this comment.
It's only supported on Linux, or Windows as well? If so, the "Windows" example needs to be updated as well 😄
There was a problem hiding this comment.
Thanks @thaJeztah. The docs has been updated.
|
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 |
|
@thaJeztah I wouldn't make it Mentioning that their service manager may kill docker regardless if the daemon is stopped via it wouldn't hurt though. |
d991cf6 to
3945662
Compare
|
@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>
3945662 to
d7be6b2
Compare
|
Thanks @vdemeester. The PR has been rebased and all tests passed. |
|
ping @thaJeztah @vdemeester final look before merging? :) |
|
Still LGTM 👼 |
|
Thanks all for the review and help! 👍 |
|
Added bash completion: #27550 |
- What I did
This fix tries to add a daemon config parameter
--shutdown-timeoutthat specifies the timeout value to stop containers gracefully (before SIGKILL). The default value is 15s.- How I did it
The
--shutdown-timeoutparameter 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-timeouttodockerdto 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