add signal option to container stop command #3892
Conversation
Signed-off-by: Arjun Raja Yogidas <[email protected]>
djdongjin
left a comment
djdongjin
left a comment
There was a problem hiding this comment.
LGTM overall. Can we add a test for this?
One possible test is let a container sleep and stop it with both timeout and SIGKILL signal, then the container should be stopped before the timeout. But feel free to use other test that can test new signal is take effect.
djdongjin
left a comment
djdongjin
left a comment
There was a problem hiding this comment.
LGTM overall, just one minor comment, thanks
coderbirju
force-pushed
the
add-signal-container-stop
branch
from
5a0f902 to
b542f5e
Compare
Signed-off-by: Arjun Raja Yogidas <[email protected]>
coderbirju
force-pushed
the
add-signal-container-stop
branch
from
b542f5e to
d3f5d10
Compare
| trap 'echo "SIGQUIT received"; quit=1' QUIT | ||
| echo "Trap set" | ||
| while true; do | ||
| if [ $quit -eq 1 ]; then | ||
| echo "Quitting loop" | ||
| break | ||
| fi | ||
| echo "In loop" |
There was a problem hiding this comment.
probably ok since it's just test
There was a problem hiding this comment.
Does not change test logic, added a bit more logs and better loop break mechanism
There was a problem hiding this comment.
This is now racy: https://github.com/containerd/nerdctl/actions/runs/13462973052/job/37622397545#step:8:2221
The additional echo is probably exacerbating the condition.
Suggesting this test (and other similar) to first consume containers logs and wait to see "trap set" before sending the signal.
There was a problem hiding this comment.
Moving conversation to #3908
This is impacting a lot of recent builds on the Docker test part.
| SilenceErrors: true, | ||
| } | ||
| stopCommand.Flags().IntP("time", "t", 10, "Seconds to wait before sending a SIGKILL") | ||
| stopCommand.Flags().StringP("signal", "s", "SIGTERM", "Signal to send to the container") |
There was a problem hiding this comment.
The docs needs to be updated too https://github.com/containerd/nerdctl/blob/main/docs/command-reference.md#whale-nerdctl-stop
5 participants
This PR adds
signalflag to thenerdctl container stopcommand