Conversation
|
The three failing jobs look unrelated to this change. TestSaveQuiet, TestLoadQuiet and TestSaveContent are image save/load tests, The test workflow on main has failed 14 of its last 15 runs, with different |
| // This asserts the label nerdctl itself writes, so it does not depend on the | ||
| // containerd restart monitor actually acting on it. It is containerd-only | ||
| // because docker has no equivalent label. | ||
| testCase.Require = require.Not(nerdtest.Docker) |
There was a problem hiding this comment.
Rather than checking the internal label, this test should just ensure that the restart manager is working in the same way as Docker when a non-stop signal is sent?
There was a problem hiding this comment.
Rewritten as suggested.
The test now asserts the restart behaviour rather than the label, so the docker gate is gone and it runs against both. One case sends SIGHUP, which is not the stop signal, then kills the container process directly, and expects the restart policy to bring it back with a restart count of 1. The other sends the default SIGKILL and expects the container to stay down with a restart count of 0.
Worth noting for the first case: sleep runs as PID 1 and installs no handler, so the kernel discards SIGHUP and the container keeps running. Killing the process afterwards is what makes it exit for a reason that is not a user stop, which is the condition the fix affects.
The red rootless job looks unrelated. It failed here on TestSaveMultipleImagesWithSameIDAndLoad, while the same job on main fails on different tests (TestLoadStdinFromPipe, TestRunRmTime), and 4 of the last 8 main runs have failing rootless jobs.
There was a problem hiding this comment.
I have to correct my previous reply. I reverted the behavioural rewrite and put the label assertions back, because measuring it showed the behavioural version does not test anything.
Setup was containerd 2.2.2 on Ubuntu aarch64, comparing a build of this branch against the same build with IsStopSignal forced to always return true, which is the pre-fix behaviour. The behavioural test passes in both. After kill --signal HUP and then killing the container process, the container comes back with a restart count of 1 whether or not the label was set, and I got the same result across a containerd restart. So explicitly-stopped does not change the observable restart outcome in these scenarios, and a test written against that outcome cannot tell the fix from its absence.
The label assertions do discriminate. With the fix all three subtests pass; with the pre-fix build only the non stop signal subtest fails. That is why they are back.
Separately I was wrong to say the test could run against docker. In docker 29.4 any docker kill suppresses the restart policy regardless of signal: a --restart=always container that exits on its own restarts normally, but after docker kill --signal HUP or --signal USR1 it stays exited with a restart count of 0. In daemon/kill.go, ExitOnNext is conditional on the stop signal matching, but HasBeenManuallyStopped = true a few lines below is not.
That last point cuts against the framing here, so it is your call rather than mine. If matching docker is the goal, the pre-fix behaviour was already the matching one and #5171 is a docker compatible quirk. If the goal is nerdctl's own consistency, that a running container is silently made ineligible for restart, the change stands and the label assertion is the only check I found that verifies it.
92b50ae to
cfc6906
Compare
| func TestRunRestartAfterKillSignal(t *testing.T) { | ||
| testCase := nerdtest.Setup() | ||
| if !nerdtest.IsDocker() { | ||
| testCase.Require = nerdtest.ContainerdPlugin("io.containerd.internal.v1", "restart", []string{"always"}) |
There was a problem hiding this comment.
This does not work for containerd v2
There was a problem hiding this comment.
This one is already resolved upstream. f99df631 dropped the containerd 1.x
plugin requirement from these restart tests, and #5175 is closed. The test
added here never calls nerdtest.ContainerdPlugin at all -- its only Require
is require.Not(nerdtest.Docker), which is why the thread shows as outdated.
There was a problem hiding this comment.
Squashed -- the branch is a single commit (4baa329a) on top of main.
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != tt.expected { |
There was a problem hiding this comment.
You can use gotest.tools/v3/assert
There was a problem hiding this comment.
Done, switched to gotest.tools/v3/assert.
The error case is now assert.Assert(t, err != nil), and the happy path is
assert.NilError(t, err) plus assert.Equal(t, got, tt.expected). The import
sits in the default gci group, matching pkg/imgutil/snapshotter_test.go.
I left the pre-existing tests in this file alone to keep the diff scoped.
cfc6906 to
86c0e92
Compare
`nerdctl kill` accepts any signal, but it recorded every one of them by setting containerd.io/restart.explicitly-stopped to true before sending it. That label is what tells the containerd restart monitor to leave a container alone, so sending a signal that is not meant to stop the container, such as SIGHUP, permanently suppressed its restart policy. The container kept running and looked healthy, and nothing reported that its --restart=always had stopped applying. Set the label only when the signal is expected to stop the container: SIGKILL, or a signal matching the container's own stop signal. The stop signal is resolved by the same rule `nerdctl stop` already uses, the stopSignal label if present and SIGTERM otherwise, so the two commands cannot disagree about what stops a container. An unparsable stopSignal label is returned as an error rather than guessed at, which is what `nerdctl stop` already does with it. `nerdctl stop` is unchanged. It stops the container by definition, so it still sets the label unconditionally. Assisted-by: Claude Code (Opus 5) Signed-off-by: somaz <genius5711@gmail.com>
86c0e92 to
4baa329
Compare
2 participants
nerdctl killsetcontainerd.io/restart.explicitly-stoppedbefore sendingany signal, so
nerdctl kill --signal HUPon a--restart=alwayscontainerpermanently suppressed its restart policy. Nothing reported it: the container
kept running and looked healthy.
The label is now set only when the signal is expected to stop the container:
SIGKILL, or a signal matching the container's own stop signal. That stop
signal is resolved by the same rule
nerdctl stopalready uses(
containerutil.getSignal): the stopSignal label if present, SIGTERMotherwise, so the two commands cannot disagree about what stops a container.
An unparsable stopSignal label is returned as an error rather than guessed
at, matching what
nerdctl stopalready does with it.nerdctl stopis unchanged: it stops the container by definition, so itstill sets the label unconditionally.
Validation:
make lint-go-all(linux/windows/freebsd/darwin) andmake lint-mod: 0 issues,using the golangci-lint version pinned in
install-dev-toolsmake lint-commits: PASSTestIsStopSignal(6 cases) and integration testTestRunRestartAlwaysKillSignal(3 subtests) pass against containerd 2.2.2IsStopSignalalways return true fails only the non-stop-signal subtest, and narrowing it
to SIGKILL only fails only the stop-signal-match subtest
make test: unchanged exceptpkg/resolvconf.TestGet, which fails the sameway on a clean checkout of
mainin this environmentcloses: #5171