Taking stop-signal into account when docker kill #26464
Conversation
|
/cc @mlaventure @thaJeztah @cpuguy83 👼 |
| @@ -47,7 +47,7 @@ func (s *DockerSuite) TestKillDifferentUserContainer(c *check.C) { | |||
|
|
|||
| // regression test about correct signal parsing see #13665 | |||
| func (s *DockerSuite) TestKillWithSignal(c *check.C) { | |||
| // Cannot port to Windows - does not support signals in the same was a Linux does | |||
| // Cannot port to Windows - does not support signals in the same way a Linux does | |||
There was a problem hiding this comment.
nit: "same way a Linux does"
| @@ -105,6 +115,9 @@ func (daemon *Daemon) Kill(container *container.Container) error { | |||
| return errNotRunning{container.ID} | |||
| } | |||
|
|
|||
| // FIXME(vdemeester) should we disable restartpolicy for SIGKILL or not ? | |||
There was a problem hiding this comment.
From my POV I would expect the RestartPolicy to only be disabled if the kill was generated as part of a docker stop command.
WDYT?
There was a problem hiding this comment.
@mlaventure you mean the SIGKILL sent after the grace period right ?
I'm not really sure in fact. Naively, I would consider SIGKILL as the fatal signal, and thus, the only signal that always disable restart policy.
There was a problem hiding this comment.
But the SIGKILL could have been sent as part of the oom policy in which case, I'd probably want my container restarted since the targeted process are more or less random
There was a problem hiding this comment.
@mlaventure hum… true, so SIGKILL would be treated exactly as the other signals then. docker stop sends the stop-signal if present, and thus, the restart manager would disable the restart policy, even if the SIGKILL is sent afterwards.
@crosbymichael @LK4D4 @cpuguy83 @thaJeztah WDYT ? I can update to reflect that.
There was a problem hiding this comment.
I need to read up on this issue again 😄, and see what's the most logical behavior here
There was a problem hiding this comment.
So, this is only in the case of a shutdown, correct? I think a shutdown should not influence the restart policy, independent of the signal that was used (i.e. if the container was running, and it's killed as part of the shutdown, the intent was to have it running, so --restart=unless-stopped still applies).
There was a problem hiding this comment.
Ok after some discussion; it's better to keep the current behavior here; can your remove this?
| @@ -105,6 +115,9 @@ func (daemon *Daemon) Kill(container *container.Container) error { | |||
| return errNotRunning{container.ID} | |||
| } | |||
|
|
|||
| // FIXME(vdemeester) should we disable restartpolicy for SIGKILL or not ? | |||
There was a problem hiding this comment.
Ok after some discussion; it's better to keep the current behavior here; can your remove this?
thaJeztah
added
status/1-design-review
status/2-code-review
and removed
status/1-design-review
labels
If a container sets a stop-signal, taking it into account to disable or not the restart policy. Signed-off-by: Vincent Demeester <vincent@sbr.pm>
vdemeester
force-pushed
the
11065-stop-signal-and-restart-policies
branch
from
e7184cc to
d2e6424
Compare
|
PR updated, withouth changes on |
| container.ExitOnNext() | ||
| } | ||
| } else { | ||
| container.ExitOnNext() |
There was a problem hiding this comment.
Just realized, shouldn't we check that the signal being sent is the stop signal here too before disabling the restartmanager?
There was a problem hiding this comment.
If the container has no StopSignal then doing the same as before (for retro-compatibility sake 😅) 👼
There was a problem hiding this comment.
Yes, I noticed it was the same before. It just feels weird. This can be done in a subsequent PR anyhow I guess.
cc @tonistiigi
|
LGTM |
If a container sets a stop-signal, taking it into account to disable or not the restart policy. fix DTS2017062802264 backport from moby#26464 without conflicts. Signed-off-by: Vincent Demeester <vincent@sbr.pm> Signed-off-by: Yuanhong Peng <pengyuanhong@huawei.com>
Taking stop-signal into account when docker kill If a container sets a stop-signal, taking it into account to disable or not the restart policy. fix DTS2017062802264 backport from moby#26464 without conflicts. Signed-off-by: Vincent Demeester <vincent@sbr.pm> Signed-off-by: Yuanhong Peng <pengyuanhong@huawei.com> See merge request docker/docker!604
- What I did
This fix tries to address the issue #11065, taking into account
stop-signalif present so that non-fatal signal (i.e.SIGKILL) is sent usingdocker killcommand, it will not disable the restart policy. 🐯- How to verify it
Added 2 integration tests to handle those cases.
- Still todo
SIGKILLbehaviorThis fix fixes #11065.
/cc @tonistiigi @crosbymichael
🐸
Signed-off-by: Vincent Demeester vincent@sbr.pm