From 5777558c770992fd7d860be339be81a027b847dd Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Mon, 8 Jul 2024 17:44:33 +0200 Subject: [PATCH 1/3] fix: container stream should not be terminated by ctx Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/command/container/run.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 749071b17332..bec70dd8cbbf 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -178,7 +178,10 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption detachKeys = runOpts.detachKeys } - closeFn, err := attachContainer(ctx, dockerCli, containerID, &errCh, config, container.AttachOptions{ + // ctx should not be cancellable here, as this would kill the stream to the container + // and we want to keep the stream open until the process in the container exits or until + // the user forcefully terminates the CLI. + closeFn, err := attachContainer(context.WithoutCancel(ctx), dockerCli, containerID, &errCh, config, container.AttachOptions{ Stream: true, Stdin: config.AttachStdin, Stdout: config.AttachStdout, From d36bdb0d845d30ad05f2aae82833e701a8ce4239 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 9 Jul 2024 13:23:38 +0200 Subject: [PATCH 2/3] test: e2e SIGTERM attached container on `docker run` Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- e2e/container/run_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/e2e/container/run_test.go b/e2e/container/run_test.go index fa8ea72b0334..d4fea8176901 100644 --- a/e2e/container/run_test.go +++ b/e2e/container/run_test.go @@ -1,8 +1,10 @@ package container import ( + "bytes" "fmt" "strings" + "syscall" "testing" "time" @@ -13,6 +15,7 @@ import ( is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/golden" "gotest.tools/v3/icmd" + "gotest.tools/v3/poll" "gotest.tools/v3/skip" ) @@ -221,3 +224,26 @@ func TestMountSubvolume(t *testing.T) { }) } } + +func TestProcessTermination(t *testing.T) { + var out bytes.Buffer + cmd := icmd.Command("docker", "run", "--rm", "-i", fixtures.AlpineImage, + "sh", "-c", "echo 'starting trap'; trap 'echo got signal; exit 0;' TERM; while true; do sleep 10; done") + cmd.Stdout = &out + cmd.Stderr = &out + + result := icmd.StartCmd(cmd).Assert(t, icmd.Success) + + poll.WaitOn(t, func(t poll.LogT) poll.Result { + if strings.Contains(result.Stdout(), "starting trap") { + return poll.Success() + } + return poll.Continue("waiting for process to trap signal") + }, poll.WithDelay(1*time.Second), poll.WithTimeout(5*time.Second)) + + assert.NilError(t, result.Cmd.Process.Signal(syscall.SIGTERM)) + + icmd.WaitOnCmd(time.Second*10, result).Assert(t, icmd.Expected{ + ExitCode: 0, + }) +} From 333103d93ff13dc5ce125ef65294f6bf9f56fccc Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 11 Jul 2024 09:49:14 +0200 Subject: [PATCH 3/3] chore: restore ctx without cancel on container run Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/command/container/run.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index bec70dd8cbbf..562e8029208b 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -119,6 +119,8 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro //nolint:gocyclo func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOptions, copts *containerOptions, containerCfg *containerConfig) error { + ctx = context.WithoutCancel(ctx) + config := containerCfg.Config stdout, stderr := dockerCli.Out(), dockerCli.Err() apiClient := dockerCli.Client() @@ -181,7 +183,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption // ctx should not be cancellable here, as this would kill the stream to the container // and we want to keep the stream open until the process in the container exits or until // the user forcefully terminates the CLI. - closeFn, err := attachContainer(context.WithoutCancel(ctx), dockerCli, containerID, &errCh, config, container.AttachOptions{ + closeFn, err := attachContainer(ctx, dockerCli, containerID, &errCh, config, container.AttachOptions{ Stream: true, Stdin: config.AttachStdin, Stdout: config.AttachStdout,