API: add "signal" parameter to container stop and restart endpoints by thaJeztah · Pull Request #43206 · moby/moby

thaJeztah · 2022-02-02T13:07:42Z

When reviewing #41548, I recalled I had this branch, but never opened it as a PR, as I was planning to do so after #42774 was merged.

depends on / includes:

daemon: cleanupContainer(): pass ContainerRmConfig as parameter #42774

API: add "signal" parameter to container stop and restart endpoints

Containers can have a default stop-signal (--stop-signal / STOPSIGNAL) and
timeout (--stop-timeout). It is currently not possible to update either of
these after the container is created (docker update does not allow updating
them), and while either of these can be overridden through some commands, we
currently do not have a command that can override both:

command	stop-signal	stop-timeout	notes
docker kill	yes	DNA	only sends a single signal
docker restart	no	yes
docker stop	no	yes

As a result, if a user wants to stop a container with a custom signal and
timeout, the only option is to do this manually:

docker kill -s <custom signal> mycontainer
# wait <desired timeout>
# press ^C to cancel the graceful stop
# forcibly kill the container
docker kill mycontainer

This patch adds a new signal query parameter to the container "stop" and
"restart" endpoints. This parameter can be added as a new flag on the CLI,
which would allow stopping and restarting with a custom timeout and signal,
for example:

docker stop --signal=SIGWINCH --time=120 mycontainer

docker restart --signal=SIGWINCH --time=120 mycontainer

Refactoring / cleanup commits

daemon/delete: rename var that collided with import, remove output var (part of #42774)

Renamed a variable that collided with an import.

Also remove the named return variable from cleanupContainer(), as it wasn't used.

daemon: cleanupContainer(): pass ContainerRmConfig as parameter (part of #42774)

We already have this config, so might as well pass it, instead of passing each option as a separate argument.

daemon: fix inconsistent receiver name and package aliases

My IDE kept complaining about this one, so let's rename it

daemon/stop: rename imports and variables to standard naming

This is in preparation of some changes, but keeping it in a separate commit to make review of other changes easier.

daemon: move default stop-timeout to containerStop()

This avoids having to determine what the default is in various parts of the code. If no custom timeout is passed (nil), the default will be used.

Also remove the named return variable from cleanupContainer(), as it wasn't used.

daemon: containerStop(): use a regular "defer" to log container event

backend: add StopConfig to ContainerRestart and ContainerStop

And while we're modifying the interface, also add a context to both

tianon

LGTM!

daemon/stop.go

docs/api/version-history.md

thaJeztah · 2022-02-16T10:13:01Z

@AkihiroSuda @cpuguy83 ptal

thaJeztah · 2022-02-16T19:51:37Z

api/types/container/config.go

+// StopConfig holds the options to stop, or restart a container.
+type StopConfig struct {
+	StopSignal  string `json:",omitempty"` // Signal to stop the container.
+	StopTimeout *int   `json:",omitempty"` // Timeout (in seconds) to stop the container, or '-1' to wait indefinitely.
+}


🙈 and I just realised why I probably didn't push it as a PR before; I see that I didn't change the client code to allow the new options to be set.

Looking at that, I'm a bit on the fence if StopConfig should be used for both stop and restart (or if separate types should be used).

Let me temporarily move this PR back to draft, and work on the client bits

thaJeztah · 2022-04-17T13:26:56Z

@tianon @AkihiroSuda - Rebased, pushed the commits with the client-side changes that I didn't include, PTAL

tianon

I love it. 👍

tianon · 2022-04-19T23:51:58Z

daemon/delete.go

 	daemon.statsCollector.StopCollection(container)

-	if err := daemon.containerStop(container, 3); err != nil {
+	var stopTimeout = 3


IMO a good (unrelated) follow-up here would be to add a comment of some kind here explaining what this is and why we chose "3" as the hard-coded override value 😬

alright; as I also had to address @samuelkarp's comments, I added a comment; the "bad" news; "who knows??" (but I left references to the commits that set this duration). The good news: you can earn yourself a picture of a cute animal!

samuelkarp

Two nits (one in-line, one here) and one minor comment.

nit: commit "daemon: move default stop-timeout to containerStop()" has a typo in the commit message ("determin")

But aside from those, LGTM!

daemon/delete.go

daemon/stop.go

- daemon/delete: rename var that collided with import, remove output var - daemon: fix inconsistent receiver name and package aliases - daemon/stop: rename imports and variables to standard naming This is in preparation of some changes, but keeping it in a separate commit to make review of other changes easier. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

We already have this config, so might as well pass it, instead of passing each option as a separate argument. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This avoids having to determine what the default is in various parts of the code. If no custom timeout is passed (nil), the default will be used. Also remove the named return variable from cleanupContainer(), as it wasn't used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

While we're modifying the interface, also add a context to both. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Containers can have a default stop-signal (`--stop-signal` / `STOPSIGNAL`) and timeout (`--stop-timeout`). It is currently not possible to update either of these after the container is created (`docker update` does not allow updating them), and while either of these can be overridden through some commands, we currently do not have a command that can override *both*: command | stop-signal | stop-timeout | notes ----------------|-------------|--------------|---------------------------- docker kill | yes | DNA | only sends a single signal docker restart | no | yes | docker stop | no | yes | As a result, if a user wants to stop a container with a custom signal and timeout, the only option is to do this manually: docker kill -s <custom signal> mycontainer # wait <desired timeout> # press ^C to cancel the graceful stop # forcibly kill the container docker kill mycontainer This patch adds a new `signal` query parameter to the container "stop" and "restart" endpoints. This parameter can be added as a new flag on the CLI, which would allow stopping and restarting with a custom timeout and signal, for example: docker stop --signal=SIGWINCH --time=120 mycontainer docker restart --signal=SIGWINCH --time=120 mycontainer Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…onger used Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-04-20T19:47:10Z

Updated; fixed the typo in the commit message; changed cleanupContainer() to not use a pointer, and added a comment describing the 3 seconds 😅

samuelkarp

LGTM

thaJeztah · 2022-04-21T14:08:38Z

thx! Looking at the review comments, I think they've been addressed; let me get this one in, and work on docker/cli changes to expose these options 👍

thaJeztah added area/api API impact/api impact/changelog status/2-code-review labels Feb 2, 2022

thaJeztah mentioned this pull request Feb 2, 2022

[Carry 38704] Implement exec kill API #41548

Open

tianon approved these changes Feb 2, 2022

View reviewed changes

daemon/stop.go Show resolved Hide resolved

thaJeztah commented Feb 16, 2022

View reviewed changes

docs/api/version-history.md Outdated Show resolved Hide resolved

thaJeztah force-pushed the having_such_a_good_time_im_having_a_ball branch from f876056 to 36caa69 Compare February 16, 2022 10:12

thaJeztah commented Feb 16, 2022

View reviewed changes

thaJeztah marked this pull request as draft February 16, 2022 19:51

thaJeztah force-pushed the having_such_a_good_time_im_having_a_ball branch from 36caa69 to 24957b4 Compare March 11, 2022 19:15

thaJeztah force-pushed the having_such_a_good_time_im_having_a_ball branch from 24957b4 to e41a0bf Compare April 17, 2022 13:24

thaJeztah marked this pull request as ready for review April 17, 2022 13:25

thaJeztah added this to the 22.04.0 milestone Apr 17, 2022

thaJeztah force-pushed the having_such_a_good_time_im_having_a_ball branch 2 times, most recently from e9d8f5f to 0aebf51 Compare April 19, 2022 14:26

tianon approved these changes Apr 20, 2022

View reviewed changes

samuelkarp approved these changes Apr 20, 2022

View reviewed changes

daemon/delete.go Show resolved Hide resolved

daemon/stop.go Show resolved Hide resolved

thaJeztah added 9 commits April 20, 2022 21:22

daemon: cleanupContainer(): pass ContainerRmConfig as parameter

f3bce92

We already have this config, so might as well pass it, instead of passing each option as a separate argument. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: containerStop(): use a regular "defer" to log container event

952902e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

backend: add StopOptions to ContainerRestart and ContainerStop

90de570

While we're modifying the interface, also add a context to both. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client, integration-cli: remove unneeded import aliases

9060126

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client: ContainerStop(), ContainerRestart(): support stop-signal

e8fa708

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

api/types/time: remove DurationToSecondsString() utility as it's no l…

90647e2

…onger used Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the having_such_a_good_time_im_having_a_ball branch from 0aebf51 to 90647e2 Compare April 20, 2022 19:43

samuelkarp approved these changes Apr 21, 2022

View reviewed changes

thaJeztah merged commit 1a0c15a into moby:master Apr 21, 2022

thaJeztah deleted the having_such_a_good_time_im_having_a_ball branch April 21, 2022 14:08

thaJeztah mentioned this pull request May 17, 2022

add --signal option to stop and restart docker/cli#3614

Merged

thaJeztah mentioned this pull request Nov 1, 2022

daemon: cleanupContainer(): pass ContainerRmConfig as parameter #42774

Closed

Conversation

thaJeztah commented Feb 2, 2022

API: add "signal" parameter to container stop and restart endpoints

Refactoring / cleanup commits

daemon/delete: rename var that collided with import, remove output var (part of #42774)

daemon: cleanupContainer(): pass ContainerRmConfig as parameter (part of #42774)

daemon: fix inconsistent receiver name and package aliases

daemon/stop: rename imports and variables to standard naming

daemon: move default stop-timeout to containerStop()

daemon: containerStop(): use a regular "defer" to log container event

backend: add StopConfig to ContainerRestart and ContainerStop

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Feb 16, 2022

Uh oh!

thaJeztah Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 17, 2022

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Apr 20, 2022

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants