Merge pull request containerd#3614 from haytok/issue_3568

fix: not to be deleted a container created with --rm when detaching

Author: AkihiroSuda

cmd/nerdctl/container/container_attach_linux_test.go

@@ -24,6 +24,8 @@ import (
 	"gotest.tools/v3/assert"
 
 	"github.com/containerd/nerdctl/v2/pkg/testutil"
+	"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
+	"github.com/containerd/nerdctl/v2/pkg/testutil/test"
 )
 
 // skipAttachForDocker should be called by attach-related tests that assert 'read detach keys' in stdout.
@@ -104,3 +106,56 @@ func TestAttachDetachKeys(t *testing.T) {
 	container := base.InspectContainer(containerName)
 	assert.Equal(base.T, container.State.Running, true)
 }
+
+// TestIssue3568 tests https://github.com/containerd/nerdctl/issues/3568
+func TestDetachAttachKeysForAutoRemovedContainer(t *testing.T) {
+	testCase := nerdtest.Setup()
+
+	testCase.SubTests = []*test.Case{
+		{
+			Description: "Issue #3568 - A container should be deleted when detaching and attaching a container started with the --rm option.",
+			// In nerdctl the detach return code from the container is 0, but in docker the return code is 1.
+			// This behaviour is reported in https://github.com/containerd/nerdctl/issues/3571 so this test is skipped for Docker.
+			Require: test.Require(
+				test.Not(nerdtest.Docker),
+			),
+			Setup: func(data test.Data, helpers test.Helpers) {
+				cmd := helpers.Command("run", "--rm", "-it", "--detach-keys=ctrl-a,ctrl-b", "--name", data.Identifier(), testutil.CommonImage)
+				// unbuffer(1) can be installed with `apt-get install expect`.
+				//
+				// "-p" is needed because we need unbuffer to read from stdin, and from [1]:
+				// "Normally, unbuffer does not read from stdin. This simplifies use of unbuffer in some situations.
+				//  To use unbuffer in a pipeline, use the -p flag."
+				//
+				// [1] https://linux.die.net/man/1/unbuffer
+				cmd.WithWrapper("unbuffer", "-p")
+				cmd.WithStdin(testutil.NewDelayOnceReader(bytes.NewReader([]byte{1, 2}))) // https://www.physics.udel.edu/~watson/scen103/ascii.html
+				cmd.Run(&test.Expected{
+					ExitCode: 0,
+				})
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				cmd := helpers.Command("attach", data.Identifier())
+				cmd.WithWrapper("unbuffer", "-p")
+				cmd.WithStdin(testutil.NewDelayOnceReader(strings.NewReader("exit\n")))
+				return cmd
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Errors:   []error{},
+					Output: test.All(
+						func(stdout string, info string, t *testing.T) {
+							assert.Assert(t, !strings.Contains(helpers.Capture("ps", "-a"), data.Identifier()))
+						},
+					),
+				}
+			},
+		},
+	}
+
+	testCase.Run(t)
+}

cmd/nerdctl/container/container_run.go

@@ -334,6 +334,8 @@ func processCreateCommandFlagsInRun(cmd *cobra.Command) (types.ContainerCreateOp
 // runAction is heavily based on ctr implementation:
 // https://github.com/containerd/containerd/blob/v1.4.3/cmd/ctr/commands/run/run.go
 func runAction(cmd *cobra.Command, args []string) error {
+	var isDetached bool
+
 	createOpt, err := processCreateCommandFlagsInRun(cmd)
 	if err != nil {
 		return err
@@ -378,8 +380,11 @@ func runAction(cmd *cobra.Command, args []string) error {
 	}()
 
 	id := c.ID()
-	if createOpt.Rm && !createOpt.Detach {
+	if createOpt.Rm {
 		defer func() {
+			if isDetached {
+				return
+			}
 			if err := netManager.CleanupNetworking(ctx, c); err != nil {
 				log.L.Warnf("failed to clean up container networking: %s", err)
 			}
@@ -449,6 +454,7 @@ func runAction(cmd *cobra.Command, args []string) error {
 			return errors.New("got a nil IO from the task")
 		}
 		io.Wait()
+		isDetached = true
 	case status := <-statusC:
 		if createOpt.Rm {
 			if _, taskDeleteErr := task.Delete(ctx); taskDeleteErr != nil {

cmd/nerdctl/container/container_run_linux_test.go

@@ -40,6 +40,7 @@ import (
 	"github.com/containerd/nerdctl/v2/pkg/strutil"
 	"github.com/containerd/nerdctl/v2/pkg/testutil"
 	"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
+	"github.com/containerd/nerdctl/v2/pkg/testutil/test"
 )
 
 func TestRunCustomRootfs(t *testing.T) {
@@ -520,3 +521,49 @@ func TestRunWithTtyAndDetached(t *testing.T) {
 	withTtyContainer := base.InspectContainer(withTtyContainerName)
 	assert.Equal(base.T, 0, withTtyContainer.State.ExitCode)
 }
+
+// TestIssue3568 tests https://github.com/containerd/nerdctl/issues/3568
+func TestIssue3568(t *testing.T) {
+	testCase := nerdtest.Setup()
+
+	testCase.SubTests = []*test.Case{
+		{
+			Description: "Issue #3568 - Detaching from a container started by using --rm option causes the container to be deleted.",
+			// When detaching from a container, for a session started with 'docker attach', it prints 'read escape sequence', but for one started with 'docker (run|start)', it prints nothing.
+			// However, the flag is called '--detach-keys' in all cases, so nerdctl prints 'read detach keys' for all cases, and that's why this test is skipped for Docker.
+			Require: test.Require(
+				test.Not(nerdtest.Docker),
+			),
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				cmd := helpers.Command("run", "--rm", "-it", "--detach-keys=ctrl-a,ctrl-b", "--name", data.Identifier(), testutil.CommonImage)
+				// unbuffer(1) can be installed with `apt-get install expect`.
+				//
+				// "-p" is needed because we need unbuffer to read from stdin, and from [1]:
+				// "Normally, unbuffer does not read from stdin. This simplifies use of unbuffer in some situations.
+				//  To use unbuffer in a pipeline, use the -p flag."
+				//
+				// [1] https://linux.die.net/man/1/unbuffer
+				cmd.WithWrapper("unbuffer", "-p")
+				cmd.WithStdin(testutil.NewDelayOnceReader(bytes.NewReader([]byte{1, 2}))) // https://www.physics.udel.edu/~watson/scen103/ascii.html
+				return cmd
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Errors:   []error{},
+					Output: test.All(
+						test.Contains("read detach keys"),
+						func(stdout string, info string, t *testing.T) {
+							assert.Assert(t, strings.Contains(helpers.Capture("ps"), data.Identifier()))
+						},
+					),
+				}
+			},
+		},
+	}
+
+	testCase.Run(t)
+}

pkg/cmd/container/attach.go

@@ -28,15 +28,19 @@ import (
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/consoleutil"
+	"github.com/containerd/nerdctl/v2/pkg/containerutil"
 	"github.com/containerd/nerdctl/v2/pkg/errutil"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
+	"github.com/containerd/nerdctl/v2/pkg/labels"
 	"github.com/containerd/nerdctl/v2/pkg/signalutil"
 )
 
 // Attach attaches stdin, stdout, and stderr to a running container.
 func Attach(ctx context.Context, client *containerd.Client, req string, options types.ContainerAttachOptions) error {
 	// Find the container.
 	var container containerd.Container
+	var cStatus containerd.Status
+
 	walker := &containerwalker.ContainerWalker{
 		Client: client,
 		OnFound: func(ctx context.Context, found containerwalker.Found) error {
@@ -54,6 +58,24 @@ func Attach(ctx context.Context, client *containerd.Client, req string, options
 		return fmt.Errorf("more than one containers are found given the string: %s", req)
 	}
 
+	defer func() {
+		containerLabels, err := container.Labels(ctx)
+		if err != nil {
+			log.G(ctx).WithError(err).Errorf("failed to getting container labels: %s", err)
+			return
+		}
+		rm, err := containerutil.DecodeContainerRmOptLabel(containerLabels[labels.ContainerAutoRemove])
+		if err != nil {
+			log.G(ctx).WithError(err).Errorf("failed to decode string to bool value: %s", err)
+			return
+		}
+		if rm && cStatus.Status == containerd.Stopped {
+			if err = RemoveContainer(ctx, container, options.GOptions, true, true, client); err != nil {
+				log.L.WithError(err).Warnf("failed to remove container %s: %s", req, err)
+			}
+		}
+	}()
+
 	// Attach to the container.
 	var task containerd.Task
 	detachC := make(chan struct{})
@@ -129,6 +151,10 @@ func Attach(ctx context.Context, client *containerd.Client, req string, options
 		}
 		io.Wait()
 	case status := <-statusC:
+		cStatus, err = task.Status(ctx)
+		if err != nil {
+			return err
+		}
 		code, _, err := status.Result()
 		if err != nil {
 			return err

pkg/cmd/container/create.go

@@ -340,6 +340,8 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
 		}
 	}
 
+	internalLabels.rm = containerutil.EncodeContainerRmOptLabel(options.Rm)
+
 	// TODO: abolish internal labels and only use annotations
 	ilOpt, err := withInternalLabels(internalLabels)
 	if err != nil {
@@ -655,6 +657,8 @@ type internalLabels struct {
 	ipc string
 	// log
 	logURI string
+	// a label to check whether the --rm option is specified.
+	rm string
 }
 
 // WithInternalLabels sets the internal labels for a container.
@@ -732,6 +736,10 @@ func withInternalLabels(internalLabels internalLabels) (containerd.NewContainerO
 		m[labels.IPC] = internalLabels.ipc
 	}
 
+	if internalLabels.rm != "" {
+		m[labels.ContainerAutoRemove] = internalLabels.rm
+	}
+
 	return containerd.WithAdditionalContainerLabels(m), nil
 }
 

pkg/containerutil/containerutil.go

@@ -600,3 +600,13 @@ func GetContainerName(containerLabels map[string]string) string {
 	}
 	return ""
 }
+
+// EncodeContainerRmOptLabel encodes bool value for the --rm option into string value for a label.
+func EncodeContainerRmOptLabel(rmOpt bool) string {
+	return fmt.Sprintf("%t", rmOpt)
+}
+
+// DecodeContainerRmOptLabel decodes bool value for the --rm option from string value for a label.
+func DecodeContainerRmOptLabel(rmOptLabel string) (bool, error) {
+	return strconv.ParseBool(rmOptLabel)
+}

pkg/labels/labels.go

@@ -100,4 +100,7 @@ const (
 	// Boolean value which can be parsed with strconv.ParseBool() is required.
 	// (like "nerdctl/default-network=true" or "nerdctl/default-network=false")
 	NerdctlDefaultNetwork = Prefix + "default-network"
+
+	// ContainerAutoRemove is to check whether the --rm option is specified.
+	ContainerAutoRemove = Prefix + "auto-remove"
 )