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

[haytok]

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:

• Detaching from a container started with run --rm unexpectedly stops and removes it #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

[haytok]

Hi, @apostasie @djdongjin @AkihiroSuda

Could you review this PR when you have time ?

[apostasie]

Thanks for the ping @haytok

I don't think that works yet.
If you detach, then attach, then exit, the container is not removed (it should).

Run and detach

nerdctl run --rm -ti --name foo debian
root@c9951514746b:/# INFO[0001] read detach keys

then attach and CTRL+D.

nerdctl attach foo
root@c9951514746b:/#
exit

nerdctl ps -a | grep foo <- container should have been removed.

[haytok]

@apostasie

Thanks for comments !!!

I have checked the behaviour of apostasie's comments and I'm able to reproduce the behaviour with this modification applied.

Details

(24-10-31 14:57:30) <0> [~/workspace/nerdctl]
dev-dsk-haytok % n run --rm -ti --name foo debian
root@aedd1bb7ab0a:/# INFO[0001] read detach keys

(24-10-31 14:57:36) <0> [~/workspace/nerdctl]
dev-dsk-haytok % n ps
CONTAINER ID    IMAGE                              COMMAND    CREATED          STATUS    PORTS    NAMES
aedd1bb7ab0a    docker.io/library/debian:latest    "bash"     3 seconds ago    Up                 foo

(24-10-31 14:57:38) <0> [~/workspace/nerdctl]
dev-dsk-haytok % n attach foo

root@aedd1bb7ab0a:/# exit
exit

(24-10-31 14:57:51) <0> [~/workspace/nerdctl]
dev-dsk-haytok % n ps
CONTAINER ID    IMAGE    COMMAND    CREATED    STATUS    PORTS    NAMES

(24-10-31 14:57:52) <0> [~/workspace/nerdctl]
dev-dsk-haytok % n ps -a
CONTAINER ID    IMAGE                              COMMAND    CREATED           STATUS                      PORTS    NAMES
aedd1bb7ab0a    docker.io/library/debian:latest    "bash"     18 seconds ago    Exited (0) 2 seconds ago             foo

There is indeed a problem with the current implementation, so I have modified it so that the container is also deleted when detaching with ctrl+D to an attached container.

Thanks for quick review :)

[apostasie]

Thanks a lot @haytok

I will have another look soon (slow to review rn, sorry).

[haytok]

As a result of this modification, it appears that the following process is not being executed when detach is performed after attach in the above-mentioned operation check, resulting in a stopped state where the container is deleted.

- https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container/container_run.go#L452C1-L464C4

I will do additional investigations tomorrow ...

[haytok]

Hi, reviewers and maintainers

After applying this fix of this PR, when detach and attach a container that has activated the --rm option, the container is not deleted and stopped.

Applying the following fix solves this problem, but I'm not sure if this is a proper implementation...

I would appreciate advice if there is any other good implementation 🙏

Details

dev-dsk-haytokb % gd pkg/cmd/container/attach.go
diff --git a/pkg/cmd/container/attach.go b/pkg/cmd/container/attach.go
index fbbddafc..c35daa1d 100644
--- a/pkg/cmd/container/attach.go
+++ b/pkg/cmd/container/attach.go
@@ -37,6 +37,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 +56,15 @@ 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() {
+               if cStatus.Status == containerd.Stopped {
+                       if err = RemoveContainer(ctx, container, options.GOptions, true, true, client); err != nil {
+                               return
+                       }
+               }
+       }()
        // Attach to the container.
        var task containerd.Task
        detachC := make(chan struct{})
@@ -114,6 +125,7 @@ func Attach(ctx context.Context, client *containerd.Client, req string, options
        if err != nil {
                return fmt.Errorf("failed to init an async wait for the container to exit: %w", err)
        }
        select {
        // io.Wait() would return when either 1) the user detaches from the container OR 2) the container is about to exit.
        //
@@ -129,6 +141,11 @@ 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

I thought it would be nice to have a flag from the attach command process to determine if the container has run the --rm option, but now I have not been able to confirm this in the current implementation.

[apostasie]

@haytok it would be easier to comment on the code, so, suggesting you modify your PR to incorporate all your changes and we can talk from there.

From a glimpse at it, I wonder if your change would also impact containers not started with --rm?

[haytok]

Hi @apostasie

Thanks for comments !!!

I have fixed to remove a container when detaching and attaching a container started with the --rm option.

[AkihiroSuda]

Thanks