diff --git a/cmd/nerdctl/container/container_remove_linux_test.go b/cmd/nerdctl/container/container_remove_test.go similarity index 85% rename from cmd/nerdctl/container/container_remove_linux_test.go rename to cmd/nerdctl/container/container_remove_test.go index 6a304b80ebf..3f9eecfd679 100644 --- a/cmd/nerdctl/container/container_remove_linux_test.go +++ b/cmd/nerdctl/container/container_remove_test.go @@ -24,16 +24,18 @@ import ( func TestRemoveContainer(t *testing.T) { t.Parallel() + base := testutil.NewBase(t) tID := testutil.Identifier(t) // ignore error base.Cmd("rm", tID, "-f").AssertOK() - base.Cmd("run", "-d", "--name", tID, testutil.CommonImage, "sleep", "infinity").AssertOK() + base.Cmd("run", "-d", "--name", tID, testutil.NginxAlpineImage).AssertOK() defer base.Cmd("rm", tID, "-f").AssertOK() base.Cmd("rm", tID).AssertFail() - base.Cmd("kill", tID).AssertOK() + // `kill` does return before the container actually stops + base.Cmd("stop", tID).AssertOK() base.Cmd("rm", tID).AssertOK() } diff --git a/cmd/nerdctl/container/container_remove_windows_test.go b/cmd/nerdctl/container/container_remove_windows_test.go index f3b1a73f640..c6733ca1e9b 100644 --- a/cmd/nerdctl/container/container_remove_windows_test.go +++ b/cmd/nerdctl/container/container_remove_windows_test.go @@ -24,21 +24,6 @@ import ( "github.com/containerd/nerdctl/v2/pkg/testutil" ) -func TestRemoveProcessContainer(t *testing.T) { - base := testutil.NewBase(t) - tID := testutil.Identifier(t) - - // ignore error - base.Cmd("rm", tID, "-f").AssertOK() - - base.Cmd("run", "-d", "--name", tID, testutil.NginxAlpineImage).AssertOK() - defer base.Cmd("rm", tID, "-f").AssertOK() - base.Cmd("rm", tID).AssertFail() - - base.Cmd("kill", tID).AssertOK() - base.Cmd("rm", tID).AssertOK() -} - func TestRemoveHyperVContainer(t *testing.T) { base := testutil.NewBase(t) tID := testutil.Identifier(t) diff --git a/pkg/cmd/container/remove.go b/pkg/cmd/container/remove.go index 0445953be73..5acc7465c5a 100644 --- a/pkg/cmd/container/remove.go +++ b/pkg/cmd/container/remove.go @@ -101,11 +101,30 @@ func Remove(ctx context.Context, client *containerd.Client, containers []string, // - then and ONLY then, on a successful container remove, clean things-up on our side (volume store, etcetera) // If you do need to add more cleanup, please do so at the bottom of the defer function func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions types.GlobalCommandOptions, force bool, removeAnonVolumes bool, client *containerd.Client) (retErr error) { - // defer the storage of remove error in the dedicated label + // Get labels + containerLabels, err := c.Labels(ctx) + if err != nil { + return err + } + + // Lock the container state + lf, err := containerutil.Lock(ctx, c) + if err != nil { + return err + } + defer func() { + // If there was an error, update the label + // Note that we will (obviously) not store any unlocking or statedir removal error from below if retErr != nil { containerutil.UpdateErrorLabel(ctx, c, retErr) } + // Release the lock + retErr = errors.Join(lf.Release(), retErr) + // Note: technically, this is racy... + if retErr == nil { + retErr = os.RemoveAll(containerLabels[labels.StateDir]) + } }() // Get namespace @@ -113,11 +132,6 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions if err != nil { return err } - // Get labels - containerLabels, err := c.Labels(ctx) - if err != nil { - return err - } // Get datastore dataStore, err := clientutil.DataStore(globalOptions.DataRoot, globalOptions.Address) if err != nil { @@ -139,9 +153,8 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions return err } - // Get the container id, stateDir and name + // Get the container id and name id := c.ID() - stateDir := containerLabels[labels.StateDir] name := containerLabels[labels.Name] // This will evaluate retErr to decide if we proceed with removal or not @@ -198,11 +211,6 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions log.G(ctx).WithError(err).Warnf("failed to cleanup IPC for container %q", id) } - // Remove state dir - soft failure - if err = os.RemoveAll(stateDir); err != nil { - log.G(ctx).WithError(err).Warnf("failed to remove container state dir %s", stateDir) - } - // Enforce release name here in case the poststop hook name release fails - soft failure if name != "" { // Double-releasing may happen with containers started with --rm, so, ignore NotFound errors diff --git a/pkg/containerutil/lock.go b/pkg/containerutil/lock.go new file mode 100644 index 00000000000..896f6f1f618 --- /dev/null +++ b/pkg/containerutil/lock.go @@ -0,0 +1,52 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package containerutil + +import ( + "context" + "errors" + "path/filepath" + + "github.com/containerd/containerd/v2/client" + + "github.com/containerd/nerdctl/v2/pkg/labels" + "github.com/containerd/nerdctl/v2/pkg/store" +) + +func Lock(ctx context.Context, c client.Container) (store.Store, error) { + containerLabels, err := c.Labels(ctx) + if err != nil { + return nil, err + } + + stateDir := containerLabels[labels.StateDir] + if stateDir == "" { + return nil, errors.New("container is missing statedir label") + } + + stor, err := store.New(filepath.Join(stateDir, "oplock"), 0, 0) + if err != nil { + return nil, err + } + + err = stor.Lock() + if err != nil { + return nil, err + } + + return stor, nil +} diff --git a/pkg/imgutil/commit/commit.go b/pkg/imgutil/commit/commit.go index 44e582052cb..b504035dd22 100644 --- a/pkg/imgutil/commit/commit.go +++ b/pkg/imgutil/commit/commit.go @@ -44,6 +44,7 @@ import ( "github.com/containerd/log" "github.com/containerd/platforms" + "github.com/containerd/nerdctl/v2/pkg/containerutil" imgutil "github.com/containerd/nerdctl/v2/pkg/imgutil" "github.com/containerd/nerdctl/v2/pkg/labels" ) @@ -66,6 +67,12 @@ var ( ) func Commit(ctx context.Context, client *containerd.Client, container containerd.Container, opts *Opts) (digest.Digest, error) { + lf, err := containerutil.Lock(ctx, container) + if err != nil { + return emptyDigest, err + } + defer lf.Release() + id := container.ID() info, err := container.Info(ctx) if err != nil {