From 718e7cd2bc9a6f62197ea0b5c84e0a0edb915481 Mon Sep 17 00:00:00 2001 From: Hayato Kiwata Date: Mon, 4 Nov 2024 18:48:57 +0900 Subject: [PATCH] fix: not to be deleted a container created with --rm when detaching In the current implementation, detaching from a container started with `nerdctl run --rm ...` unexpectedly removes it. The behaviour before this modification is as follows. ``` > nerdctl run --rm -it --detach-keys=ctrl-a,ctrl-b --name test alpine / # INFO[0002] read detach keys > nerdctl ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES ``` When the same operation is performed in the Docker CLI, the container will not be deleted. This issue is reported in the following: - https://github.com/containerd/nerdctl/issues/3568 Therefore, this commit resolves this behaviour of a container not being deleted on detachment. Note that the behaviour after this modification is as follows. ``` > nerdctl run --rm -it --detach-keys=ctrl-a,ctrl-b --name test alpine / # INFO[0010] read detach keys > nerdctl ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 46f4c829e5cc docker.io/library/alpine:latest "/bin/sh" 15 seconds ago Up test ``` This PR has also been modified to remove a container when detaching and attaching a container started with the --rm option. The detailed behaviour is as follows. ``` > nerdctl attach test / # exit > nerdctl ps -a CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES ``` Signed-off-by: Hayato Kiwata --- .../container/container_attach_linux_test.go | 55 +++++++++++++++++++ cmd/nerdctl/container/container_run.go | 8 ++- .../container/container_run_linux_test.go | 47 ++++++++++++++++ pkg/cmd/container/attach.go | 26 +++++++++ pkg/cmd/container/create.go | 8 +++ pkg/containerutil/containerutil.go | 10 ++++ pkg/labels/labels.go | 3 + 7 files changed, 156 insertions(+), 1 deletion(-) diff --git a/cmd/nerdctl/container/container_attach_linux_test.go b/cmd/nerdctl/container/container_attach_linux_test.go index cdd4e689981..8d90897230e 100644 --- a/cmd/nerdctl/container/container_attach_linux_test.go +++ b/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) +} diff --git a/cmd/nerdctl/container/container_run.go b/cmd/nerdctl/container/container_run.go index f608a915f6c..855d1512cdf 100644 --- a/cmd/nerdctl/container/container_run.go +++ b/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 { diff --git a/cmd/nerdctl/container/container_run_linux_test.go b/cmd/nerdctl/container/container_run_linux_test.go index 0b16d2363df..dc33702e1bd 100644 --- a/cmd/nerdctl/container/container_run_linux_test.go +++ b/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) +} diff --git a/pkg/cmd/container/attach.go b/pkg/cmd/container/attach.go index fbbddafcb40..177a9fd03db 100644 --- a/pkg/cmd/container/attach.go +++ b/pkg/cmd/container/attach.go @@ -28,8 +28,10 @@ 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" ) @@ -37,6 +39,8 @@ import ( 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 diff --git a/pkg/cmd/container/create.go b/pkg/cmd/container/create.go index b0effcff12d..c5c6e222820 100644 --- a/pkg/cmd/container/create.go +++ b/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 } diff --git a/pkg/containerutil/containerutil.go b/pkg/containerutil/containerutil.go index 814156117fc..8f215ecf679 100644 --- a/pkg/containerutil/containerutil.go +++ b/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) +} diff --git a/pkg/labels/labels.go b/pkg/labels/labels.go index d88aee02a7d..e470c7230dc 100644 --- a/pkg/labels/labels.go +++ b/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" )