Prevent concurrency on commit and remove

Some container operations require exclusive access to the container, and/or have no elegant way to deal with
concurrent container removal.
See for example: containerd#1391

This PR adds a locking method that containers can use, backed by a store instance inside the container statedir,
and enforces exclusive locking for `Commit` and `Remove`.
There may be other container operations where we would like to apply the lock as well (to be evaluated).
Finally note that this locking mechanism is to prevent another instance of nerdctl from executing concurrently,
and does not offer guarantees that the container will not be manipulated by a different cli or otherwise change
state on its own.

Signed-off-by: apostasie <[email protected]>

Author: apostasie

cmd/nerdctl/container/container_remove_linux_test.go renamed to 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()
 }

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)

pkg/cmd/container/remove.go

@@ -101,23 +101,37 @@ 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
 	containerNamespace, err := namespaces.NamespaceRequired(ctx)
 	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

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
+}

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 {