From aee9eebf349dd6d8634e4ea7d5624e276c6da0a2 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Fri, 29 Nov 2024 16:59:36 +0000 Subject: [PATCH 1/6] run: return error code when only STDIN attached During a `docker run`, the CLI has some different behavior/output depending on whether the run is "detached" or not. In some cases, the CLI is checking whether either `stdin`, `stdout` or `stderr` are attached, but in other cases we're only checking `stdout` and `stderr`, which leads to some inconsistencies: ``` $ docker run -a stdout --rm --name test alpine top [docker kill test] exit status 137 $ docker run -a stderr --rm --name test alpine top [docker kill test] exit status 137 $ docker run -a stdin --rm --name test alpine top 56820d94a89b96889478241ae68920323332c6d4cf9b51ba9340cba01e9e0565 [docker kill test] [no exit code] ``` Since we're not checking for whether `stdin` is attached when deciding whether to early exit without receiving on `statusChan`, the `docker run -a stdin` is falling into the "detached mode" logic, which simply prints the container ID and doesn't print/return the exit code. This patch makes the "attached" checks consistent. Signed-off-by: Laura Brehm --- cli/command/container/run.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index bb0340ed1b49..76369a0c3147 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -161,7 +161,8 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption waitDisplayID chan struct{} errCh chan error ) - if !config.AttachStdout && !config.AttachStderr { + attach := config.AttachStdin || config.AttachStdout || config.AttachStderr + if !attach { // Make this asynchronous to allow the client to write to stdin before having to read the ID waitDisplayID = make(chan struct{}) go func() { @@ -169,7 +170,6 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption _, _ = fmt.Fprintln(stdout, containerID) }() } - attach := config.AttachStdin || config.AttachStdout || config.AttachStderr if attach { detachKeys := dockerCli.ConfigFile().DetachKeys if runOpts.detachKeys != "" { @@ -234,7 +234,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } // Detached mode: wait for the id to be displayed and return. - if !config.AttachStdout && !config.AttachStderr { + if !attach { // Detached mode <-waitDisplayID return nil From 8431298824ef57e51d1b5c0a7af85cb17fd7a3e8 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 09:46:40 +0000 Subject: [PATCH 2/6] =?UTF-8?q?run:=20cleanup=20=E2=80=93=20use=20`attache?= =?UTF-8?q?d`=20where=20applicable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Laura Brehm --- cli/command/container/run.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 76369a0c3147..e122f3c476ab 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -215,7 +215,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption return toStatusError(err) } - if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() { + if attach && config.Tty && dockerCli.Out().IsTerminal() { if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil { _, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err) } From 446f36ce58c2c67dd727e8ec7963218e85a36ed5 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 10:16:12 +0000 Subject: [PATCH 3/6] =?UTF-8?q?run:=20cleanup=20=E2=80=93=20move=20"detach?= =?UTF-8?q?ed"=20early=20exit=20earlier?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since everything else after the `apiClient.ContainerStart` block is under an `if attach` conditional, we can move the "detached" early exit up. Signed-off-by: Laura Brehm --- cli/command/container/run.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index e122f3c476ab..83f737c9e0af 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -215,7 +215,14 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption return toStatusError(err) } - if attach && config.Tty && dockerCli.Out().IsTerminal() { + // Detached mode: wait for the id to be displayed and return. + if !attach { + // Detached mode + <-waitDisplayID + return nil + } + + if config.Tty && dockerCli.Out().IsTerminal() { if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil { _, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err) } @@ -233,13 +240,6 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } } - // Detached mode: wait for the id to be displayed and return. - if !attach { - // Detached mode - <-waitDisplayID - return nil - } - status := <-statusChan if status != 0 { return cli.StatusError{StatusCode: status} From a3d9fc49414b5e6972e589d2baa8df676a984c9b Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 10:21:53 +0000 Subject: [PATCH 4/6] =?UTF-8?q?run:=20cleanup=20=E2=80=93=20remove=20`errC?= =?UTF-8?q?h`=20nil=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now, if running in "detached" mode, we early exit at L222. Similarly, if `attachContainer` errors out, it returns an error that gets handled on L190. As such, `errCh` can never be nil on L231. Remove the nil check. Signed-off-by: Laura Brehm --- cli/command/container/run.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 83f737c9e0af..ed4c96574c5f 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -228,16 +228,14 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } } - if errCh != nil { - if err := <-errCh; err != nil { - if _, ok := err.(term.EscapeError); ok { - // The user entered the detach escape sequence. - return nil - } - - logrus.Debugf("Error hijack: %s", err) - return err + if err := <-errCh; err != nil { + if _, ok := err.(term.EscapeError); ok { + // The user entered the detach escape sequence. + return nil } + + logrus.Debugf("Error hijack: %s", err) + return err } status := <-statusChan From 30c4637f03ecb8855fe6e47f5374c89d50222577 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 10:33:17 +0000 Subject: [PATCH 5/6] run: don't hang if only attaching STDIN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If STDOUT or STDERR are attached and the container exits, the streams will be closed by the daemon while the container is exiting, causing the streamer to return an error https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/hijack.go#L53 that gets sent https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L278 and received https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L225 on `errCh`. However, if only STDIN is attached, it's not closed (since this is attached to the user's TTY) when the container exits, so the streamer doesn't exit and nothing gets sent on `errCh`, meaning the CLI execution hangs receiving on `errCh` on L231. Change the logic to receive on both `errCh` and `statusChan` – this way, if the container exits, we get notified on `statusChan` (even if only STDIN is attached), and can cancel the streamer and exit. Signed-off-by: Laura Brehm --- cli/command/container/run.go | 33 ++++++++++++++++++++++----------- e2e/container/run_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index ed4c96574c5f..5ef3e8b7bf7b 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -228,20 +228,31 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } } - if err := <-errCh; err != nil { - if _, ok := err.(term.EscapeError); ok { - // The user entered the detach escape sequence. - return nil - } + select { + case err := <-errCh: + if err != nil { + if _, ok := err.(term.EscapeError); ok { + // The user entered the detach escape sequence. + return nil + } - logrus.Debugf("Error hijack: %s", err) - return err + logrus.Debugf("Error hijack: %s", err) + return err + } + status := <-statusChan + if status != 0 { + return cli.StatusError{StatusCode: status} + } + case status := <-statusChan: + // notify hijackedIOStreamer that we're exiting and wait + // so that the terminal can be restored. + cancelFun() + <-errCh + if status != 0 { + return cli.StatusError{StatusCode: status} + } } - status := <-statusChan - if status != 0 { - return cli.StatusError{StatusCode: status} - } return nil } diff --git a/e2e/container/run_test.go b/e2e/container/run_test.go index c835f1caf294..f4e591e21d4d 100644 --- a/e2e/container/run_test.go +++ b/e2e/container/run_test.go @@ -3,11 +3,13 @@ package container import ( "bytes" "fmt" + "os/exec" "strings" "syscall" "testing" "time" + "github.com/creack/pty" "github.com/docker/cli/e2e/internal/fixtures" "github.com/docker/cli/internal/test/environment" "github.com/docker/docker/api/types/versions" @@ -38,6 +40,39 @@ func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) { golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden") } +func TestRunAttach(t *testing.T) { + skip.If(t, environment.RemoteDaemon()) + t.Parallel() + + streams := []string{"stdin", "stdout", "stderr"} + for _, stream := range streams { + t.Run(stream, func(t *testing.T) { + t.Parallel() + c := exec.Command("docker", "run", "-a", stream, "--rm", "alpine", + "sh", "-c", "sleep 1 && exit 7") + d := bytes.Buffer{} + c.Stdout = &d + c.Stderr = &d + _, err := pty.Start(c) + assert.NilError(t, err) + + done := make(chan error) + go func() { + done <- c.Wait() + }() + + select { + case <-time.After(20 * time.Second): + t.Fatal("docker run took too long, likely hang", d.String()) + case <-done: + } + + assert.Equal(t, c.ProcessState.ExitCode(), 7) + assert.Check(t, is.Contains(d.String(), "exit status 7")) + }) + } +} + // Regression test for https://github.com/docker/cli/issues/5053 func TestRunInvalidEntrypointWithAutoremove(t *testing.T) { environment.SkipIfDaemonNotLinux(t) From 7dab597e6d570e04f7f135793ba4b64fdfbc47a5 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 10:50:44 +0000 Subject: [PATCH 6/6] tests: cleanup comment Signed-off-by: Laura Brehm --- e2e/container/attach_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/e2e/container/attach_test.go b/e2e/container/attach_test.go index a6c9bc04b534..545d86ed707a 100644 --- a/e2e/container/attach_test.go +++ b/e2e/container/attach_test.go @@ -37,7 +37,6 @@ func TestAttachInterrupt(t *testing.T) { // todo(laurazard): make this test work w/ dind over ssh skip.If(t, strings.Contains(os.Getenv("DOCKER_HOST"), "ssh://")) - // if result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "sh", "-c", "trap \"exit 33\" SIGINT; for i in $(seq 100); do sleep 0.1; done; exit 34") result.Assert(t, icmd.Success)