feat: support '--detach-keys' for 'nerdctl (run|start)'

Control flow: 1. Detect if the detach keys are present in stdin. 2. After detecting the detach keys, cancel the current I/O operations by calling IO.Cancel(). This means that the underlying I/O FIFOs should be left intact, and containerd should keep writing to/reading from those FIFOs, but nerdctl should stop writing to/reading from them. 3. After starting the task, we need to call IO.Wait(), and after it returns, we need to see if the container exits or it's just the user detaching from the container by checking the state of the container. Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>
containerd · Jun 8, 2023 · ec49140 · ec49140
1 parent c9174fe
commit ec49140
Show file tree

Hide file tree

Showing 15 changed files with 322 additions and 51 deletions.
diff --git a/cmd/nerdctl/compose_start.go b/cmd/nerdctl/compose_start.go
@@ -109,7 +109,7 @@ func startContainers(ctx context.Context, client *containerd.Client, containers
 			}
 
 			// in compose, always disable attach
-			if err := containerutil.Start(ctx, c, false, client); err != nil {
+			if err := containerutil.Start(ctx, c, false, client, ""); err != nil {
 				return err
 			}
 			info, err := c.Info(ctx, containerd.WithoutRefreshedMetadata)

diff --git a/cmd/nerdctl/container_run.go b/cmd/nerdctl/container_run.go
@@ -22,7 +22,6 @@ import (
 	"runtime"
 
 	"github.com/containerd/console"
-	"github.com/containerd/containerd"
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/clientutil"
 	"github.com/containerd/nerdctl/pkg/cmd/container"
@@ -91,6 +90,7 @@ func setCreateFlags(cmd *cobra.Command) {
 	})
 	cmd.Flags().String("stop-signal", "SIGTERM", "Signal to stop a container")
 	cmd.Flags().Int("stop-timeout", 0, "Timeout (in seconds) to stop a container")
+	cmd.Flags().String("detach-keys", consoleutil.DefaultDetachKeys, "Override the default detach keys")
 
 	// #region for init process
 	cmd.Flags().Bool("init", false, "Run an init process inside the container, Default to use tini")
@@ -282,6 +282,10 @@ func processCreateCommandFlagsInRun(cmd *cobra.Command) (opt types.ContainerCrea
 	if err != nil {
 		return
 	}
+	opt.DetachKeys, err = cmd.Flags().GetString("detach-keys")
+	if err != nil {
+		return
+	}
 	return opt, nil
 }
 
@@ -358,25 +362,12 @@ func runAction(cmd *cobra.Command, args []string) error {
 		return err
 	}
 	logURI := lab[labels.LogURI]
-	task, err := taskutil.NewTask(ctx, client, c, false, createOpt.Interactive, createOpt.TTY, createOpt.Detach, con, logURI)
+	detachC := make(chan struct{})
+	task, err := taskutil.NewTask(ctx, client, c, false, createOpt.Interactive, createOpt.TTY, createOpt.Detach,
+		con, logURI, createOpt.DetachKeys, detachC)
 	if err != nil {
 		return err
 	}
-	var statusC <-chan containerd.ExitStatus
-	if !createOpt.Detach {
-		defer func() {
-			if createOpt.Rm {
-				if _, taskDeleteErr := task.Delete(ctx); taskDeleteErr != nil {
-					logrus.Error(taskDeleteErr)
-				}
-			}
-		}()
-		statusC, err = task.Wait(ctx)
-		if err != nil {
-			return err
-		}
-	}
-
 	if err := task.Start(ctx); err != nil {
 		return err
 	}
@@ -390,16 +381,41 @@ func runAction(cmd *cobra.Command, args []string) error {
 			logrus.WithError(err).Error("console resize")
 		}
 	} else {
-		sigc := signalutil.ForwardAllSignals(ctx, task)
-		defer signalutil.StopCatch(sigc)
+		sigC := signalutil.ForwardAllSignals(ctx, task)
+		defer signalutil.StopCatch(sigC)
 	}
-	status := <-statusC
-	code, _, err := status.Result()
+
+	statusC, err := task.Wait(ctx)
 	if err != nil {
 		return err
 	}
-	if code != 0 {
-		return errutil.NewExitCoderErr(int(code))
+	select {
+	// io.Wait() would return when either 1) the user detaches from the container OR 2) the container is about to exit.
+	//
+	// If we replace the `select` block with io.Wait() and
+	// directly use task.Status() to check the status of the container after io.Wait() returns,
+	// it can still be running even though the container is about to exit (somehow especially for Windows).
+	//
+	// As a result, we need a separate detachC to distinguish from the 2 cases mentioned above.
+	case <-detachC:
+		io := task.IO()
+		if io == nil {
+			return errors.New("got a nil IO from the task")
+		}
+		io.Wait()
+	case status := <-statusC:
+		if createOpt.Rm {
+			if _, taskDeleteErr := task.Delete(ctx); taskDeleteErr != nil {
+				logrus.Error(taskDeleteErr)
+			}
+		}
+		code, _, err := status.Result()
+		if err != nil {
+			return err
+		}
+		if code != 0 {
+			return errutil.NewExitCoderErr(int(code))
+		}
 	}
 	return nil
 }
diff --git a/cmd/nerdctl/container_run_linux_test.go b/cmd/nerdctl/container_run_linux_test.go
@@ -327,3 +327,34 @@ func TestRunWithOOMScoreAdj(t *testing.T) {
 
 	base.Cmd("run", "--rm", "--oom-score-adj", score, testutil.AlpineImage, "cat", "/proc/self/oom_score_adj").AssertOutContains(score)
 }
+
+func TestRunWithDetachKeys(t *testing.T) {
+	t.Parallel()
+
+	if testutil.GetTarget() == testutil.Docker {
+		t.Skip("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.")
+	}
+
+	base := testutil.NewBase(t)
+	containerName := testutil.Identifier(t)
+	opts := []func(*testutil.Cmd){
+		testutil.WithStdin(testutil.NewDelayOnceReader(bytes.NewReader([]byte{1, 2}))), // https://www.physics.udel.edu/~watson/scen103/ascii.html
+	}
+	defer base.Cmd("container", "rm", "-f", containerName).AssertOK()
+	// unbuffer(1) emulates tty, which is required by `nerdctl run -t`.
+	// 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
+	base.CmdWithHelper([]string{"unbuffer", "-p"}, "run", "-it", "--detach-keys=ctrl-a,ctrl-b", "--name", containerName, testutil.CommonImage).
+		CmdOption(opts...).AssertOutContains("read detach keys")
+	container := base.InspectContainer(containerName)
+	assert.Equal(base.T, container.State.Running, true)
+}
diff --git a/cmd/nerdctl/container_start.go b/cmd/nerdctl/container_start.go
@@ -21,6 +21,7 @@ import (
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/clientutil"
 	"github.com/containerd/nerdctl/pkg/cmd/container"
+	"github.com/containerd/nerdctl/pkg/consoleutil"
 	"github.com/spf13/cobra"
 )
 
@@ -37,6 +38,7 @@ func newStartCommand() *cobra.Command {
 
 	startCommand.Flags().SetInterspersed(false)
 	startCommand.Flags().BoolP("attach", "a", false, "Attach STDOUT/STDERR and forward signals")
+	startCommand.Flags().String("detach-keys", consoleutil.DefaultDetachKeys, "Override the default detach keys")
 
 	return startCommand
 }
@@ -50,10 +52,15 @@ func processContainerStartOptions(cmd *cobra.Command) (types.ContainerStartOptio
 	if err != nil {
 		return types.ContainerStartOptions{}, err
 	}
+	detachKeys, err := cmd.Flags().GetString("detach-keys")
+	if err != nil {
+		return types.ContainerStartOptions{}, err
+	}
 	return types.ContainerStartOptions{
-		Stdout:   cmd.OutOrStdout(),
-		GOptions: globalOptions,
-		Attach:   attach,
+		Stdout:     cmd.OutOrStdout(),
+		GOptions:   globalOptions,
+		Attach:     attach,
+		DetachKeys: detachKeys,
 	}, nil
 }
 

diff --git a/cmd/nerdctl/container_start_linux_test.go b/cmd/nerdctl/container_start_linux_test.go
@@ -0,0 +1,69 @@
+/*
+   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 main
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/containerd/nerdctl/pkg/testutil"
+	"gotest.tools/v3/assert"
+)
+
+func TestStartDetachKeys(t *testing.T) {
+	t.Parallel()
+
+	if testutil.GetTarget() == testutil.Docker {
+		t.Skip("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.")
+	}
+
+	base := testutil.NewBase(t)
+	containerName := testutil.Identifier(t)
+
+	defer base.Cmd("container", "rm", "-f", containerName).AssertOK()
+	opts := []func(*testutil.Cmd){
+		// If NewDelayOnceReader is not used,
+		// the container state will be Created instead of Exited.
+		// Maybe `unbuffer` exits too early in that case?
+		testutil.WithStdin(testutil.NewDelayOnceReader(strings.NewReader("exit\n"))),
+	}
+	// unbuffer(1) emulates tty, which is required by `nerdctl run -t`.
+	// 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
+	base.CmdWithHelper([]string{"unbuffer", "-p"}, "run", "-it", "--name", containerName, testutil.CommonImage).
+		CmdOption(opts...).AssertOK()
+	container := base.InspectContainer(containerName)
+	assert.Equal(base.T, container.State.Running, false)
+
+	opts = []func(*testutil.Cmd){
+		testutil.WithStdin(testutil.NewDelayOnceReader(bytes.NewReader([]byte{1, 2}))), // https://www.physics.udel.edu/~watson/scen103/ascii.html
+	}
+	base.CmdWithHelper([]string{"unbuffer", "-p"}, "start", "-a", "--detach-keys=ctrl-a,ctrl-b", containerName).
+		CmdOption(opts...).AssertOutContains("read detach keys")
+	container = base.InspectContainer(containerName)
+	assert.Equal(base.T, container.State.Running, true)
+}
diff --git a/docs/command-reference.md b/docs/command-reference.md
@@ -150,6 +150,7 @@ Basic flags:
 - :whale: `--uts=(host)` : UTS namespace to use
 - :whale: `--stop-signal`: Signal to stop a container (default "SIGTERM")
 - :whale: `--stop-timeout`: Timeout (in seconds) to stop a container
+- :whale: `--detach-keys`: Override the default detach keys
 
 Platform flags:
 
@@ -370,7 +371,7 @@ IPFS flags:
 - :nerd_face: `--ipfs-address`: Multiaddr of IPFS API (default uses `$IPFS_PATH` env variable if defined or local directory `~/.ipfs`)
 
 Unimplemented `docker run` flags:
-    `--attach`, `--blkio-weight-device`, `--cpu-rt-*`, `--detach-keys`, `--device-*`,
+    `--attach`, `--blkio-weight-device`, `--cpu-rt-*`, `--device-*`,
     `--disable-content-trust`, `--domainname`, `--expose`, `--health-*`, `--ip6`, `--isolation`, `--no-healthcheck`,
     `--link*`, `--mac-address`, `--publish-all`, `--sig-proxy`, `--storage-opt`,
     `--userns`, `--volume-driver`, `--volumes-from`
@@ -538,8 +539,9 @@ Usage: `nerdctl start [OPTIONS] CONTAINER [CONTAINER...]`
 Flags:
 
 - :whale: `-a, --attach`: Attach STDOUT/STDERR and forward signals
+- :whale: `--detach-keys`: Override the default detach keys
 
-Unimplemented `docker start` flags: `--checkpoint`, `--checkpoint-dir`, `--detach-keys`, `--interactive`
+Unimplemented `docker start` flags: `--checkpoint`, `--checkpoint-dir`, `--interactive`
 
 ### :whale: nerdctl restart
 

diff --git a/go.mod b/go.mod
@@ -37,6 +37,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/moby/sys/mount v0.3.3
 	github.com/moby/sys/signal v0.7.0
+	github.com/moby/term v0.5.0
 	github.com/opencontainers/go-digest v1.0.0
 	github.com/opencontainers/image-spec v1.1.0-rc3
 	github.com/opencontainers/runtime-spec v1.1.0-rc.2
@@ -63,6 +64,7 @@ require (
 require (
 	github.com/AdaLogics/go-fuzz-headers v0.0.0-20230106234847-43070de90fa1 // indirect
 	github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20221215162035-5330a85ea652 // indirect
+	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
 	github.com/cilium/ebpf v0.9.1 // indirect
 	github.com/containerd/cgroups v1.1.0 // indirect
 	github.com/containerd/fifo v1.1.0 // indirect

diff --git a/go.sum b/go.sum
@@ -47,6 +47,7 @@ github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20221215162035-5330a85ea652/go.mod
 github.com/Azure/azure-sdk-for-go v16.2.1+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
 github.com/Azure/go-ansiterm v0.0.0-20170929234023-d6e3b3328b78/go.mod h1:LmzpDX56iTiv29bbRTIsUNlaFfuhWRQBWjQdVyAevI8=
 github.com/Azure/go-ansiterm v0.0.0-20210608223527-2377c96fe795/go.mod h1:LmzpDX56iTiv29bbRTIsUNlaFfuhWRQBWjQdVyAevI8=
+github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 h1:UQHMgLO+TxOElx5B5HZ4hJQsoJ/PvUvKRhJHDQXO8P8=
 github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E=
 github.com/Azure/go-autorest v10.8.1+incompatible/go.mod h1:r+4oMnoxhatjLLJ6zxSWATqVooLgysK6ZNox3g/xq24=
 github.com/Azure/go-autorest v14.2.0+incompatible/go.mod h1:r+4oMnoxhatjLLJ6zxSWATqVooLgysK6ZNox3g/xq24=
@@ -336,6 +337,7 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t
 github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/creack/pty v1.1.11/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
+github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY=
 github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4=
 github.com/cyphar/filepath-securejoin v0.2.3 h1:YX6ebbZCZP7VkM3scTTokDgBL2TY741X51MTk3ycuNI=
 github.com/cyphar/filepath-securejoin v0.2.3/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
@@ -723,6 +725,8 @@ github.com/moby/sys/symlink v0.2.0 h1:tk1rOM+Ljp0nFmfOIBtlV3rTDlWOwFRhjEeAhZB0nZ
 github.com/moby/sys/symlink v0.2.0/go.mod h1:7uZVF2dqJjG/NsClqul95CqKOBRQyYSNnJ6BMgR/gFs=
 github.com/moby/term v0.0.0-20200312100748-672ec06f55cd/go.mod h1:DdlQx2hp0Ss5/fLikoLlEeIYiATotOjgB//nb973jeo=
 github.com/moby/term v0.0.0-20210610120745-9d4ed1856297/go.mod h1:vgPCkQMyxTZ7IDy8SXRufE172gr8+K/JE/7hHFxHW3A=
+github.com/moby/term v0.5.0 h1:xt8Q1nalod/v7BqbG21f8mQPqH+xAaC9C3N3wfWbVP0=
+github.com/moby/term v0.5.0/go.mod h1:8FzsFHVUBGZdbDsJw/ot+X+d5HLUbvklYLJ9uGfcI3Y=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0=

diff --git a/pkg/api/types/container_types.go b/pkg/api/types/container_types.go
@@ -28,6 +28,8 @@ type ContainerStartOptions struct {
 	GOptions GlobalCommandOptions
 	// Attach specifies whether to attach to the container's stdio.
 	Attach bool
+	// The key sequence for detaching a container.
+	DetachKeys string
 }
 
 // ContainerKillOptions specifies options for `nerdctl (container) kill`.
@@ -59,6 +61,8 @@ type ContainerCreateOptions struct {
 	TTY bool
 	// Detach runs container in background and print container ID
 	Detach bool
+	// The key sequence for detaching a container.
+	DetachKeys string
 	// Restart specifies the policy to apply when a container exits
 	Restart string
 	// Rm specifies whether to remove the container automatically when it exits

diff --git a/pkg/cmd/container/restart.go b/pkg/cmd/container/restart.go
@@ -37,7 +37,7 @@ func Restart(ctx context.Context, client *containerd.Client, containers []string
 			if err := containerutil.Stop(ctx, found.Container, options.Timeout); err != nil {
 				return err
 			}
-			if err := containerutil.Start(ctx, found.Container, false, client); err != nil {
+			if err := containerutil.Start(ctx, found.Container, false, client, ""); err != nil {
 				return err
 			}
 			_, err := fmt.Fprintf(options.Stdout, "%s\n", found.Req)

diff --git a/pkg/cmd/container/start.go b/pkg/cmd/container/start.go
@@ -39,7 +39,7 @@ func Start(ctx context.Context, client *containerd.Client, reqs []string, option
 			if found.MatchCount > 1 {
 				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
 			}
-			if err := containerutil.Start(ctx, found.Container, options.Attach, client); err != nil {
+			if err := containerutil.Start(ctx, found.Container, options.Attach, client, options.DetachKeys); err != nil {
 				return err
 			}
 			if !options.Attach {