Merge pull request containerd#3446 from apostasie/dev-3440-locking

Prevent concurrency on commit and remove

Author: AkihiroSuda

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 {