From 30a73ff19c407eee75de489355154ce1f35231a8 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 22 Nov 2024 14:13:40 +0100 Subject: [PATCH] fix: ctx should cancel image pull on run This patch fixes the context cancellation behaviour for the `runContainer` function, specifically the `createContainer` function introduced in this commit https://github.com/docker/cli/commit/991b1303daec833d662ec54002d1b34c8805afab. It delays stripping the `cancel` from the context passed into the `runContainer` function so that the `createContainer` function can be cancelled gracefully by a SIGTERM/SIGINT. This is especially true when the requested image does not exist and `docker run` needs to `pull` the image before creating the container. Although this patch does gracefully cancel the `runContainer` function it does not address the root cause. Some functions in the call path are not context aware, such as `pullImage`. Future work would still be necessary to ensure a consistent behaviour in the CLI. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/command/container/client_test.go | 6 +- cli/command/container/create_test.go | 2 +- cli/command/container/run.go | 8 +-- cli/command/container/run_test.go | 86 ++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 9 deletions(-) diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go index a2918a82d7cc..4ea877c82d6c 100644 --- a/cli/command/container/client_test.go +++ b/cli/command/container/client_test.go @@ -25,7 +25,7 @@ type fakeClient struct { platform *specs.Platform, containerName string) (container.CreateResponse, error) containerStartFunc func(containerID string, options container.StartOptions) error - imageCreateFunc func(parentReference string, options image.CreateOptions) (io.ReadCloser, error) + imageCreateFunc func(ctx context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) infoFunc func() (system.Info, error) containerStatPathFunc func(containerID, path string) (container.PathStat, error) containerCopyFromFunc func(containerID, srcPath string) (io.ReadCloser, container.PathStat, error) @@ -100,9 +100,9 @@ func (f *fakeClient) ContainerRemove(ctx context.Context, containerID string, op return nil } -func (f *fakeClient) ImageCreate(_ context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) { +func (f *fakeClient) ImageCreate(ctx context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) { if f.imageCreateFunc != nil { - return f.imageCreateFunc(parentReference, options) + return f.imageCreateFunc(ctx, parentReference, options) } return nil, nil } diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 2627d65f19c5..c02ed14fb5b9 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -132,7 +132,7 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { return container.CreateResponse{ID: containerID}, nil } }, - imageCreateFunc: func(parentReference string, options image.CreateOptions) (io.ReadCloser, error) { + imageCreateFunc: func(ctx context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) { defer func() { pullCounter++ }() return io.NopCloser(strings.NewReader("")), nil }, diff --git a/cli/command/container/run.go b/cli/command/container/run.go index feaf59dd786c..bb0340ed1b49 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -118,8 +118,6 @@ 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() @@ -141,9 +139,6 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption config.StdinOnce = false } - ctx, cancelFun := context.WithCancel(ctx) - defer cancelFun() - containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions) if err != nil { return toStatusError(err) @@ -159,6 +154,9 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption defer signal.StopCatch(sigc) } + ctx, cancelFun := context.WithCancel(context.WithoutCancel(ctx)) + defer cancelFun() + var ( waitDisplayID chan struct{} errCh chan error diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 7c2e36db6409..81b176d904c9 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -2,7 +2,9 @@ package container import ( "context" + "encoding/json" "errors" + "fmt" "io" "net" "syscall" @@ -16,7 +18,9 @@ import ( "github.com/docker/cli/internal/test/notary" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/image" "github.com/docker/docker/api/types/network" + "github.com/docker/docker/pkg/jsonmessage" specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/spf13/pflag" "gotest.tools/v3/assert" @@ -217,6 +221,88 @@ func TestRunAttachTermination(t *testing.T) { } } +func TestRunPullTermination(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + attachCh := make(chan struct{}) + fakeCLI := test.NewFakeCli(&fakeClient{ + createContainerFunc: func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, + platform *specs.Platform, containerName string, + ) (container.CreateResponse, error) { + select { + case <-ctx.Done(): + return container.CreateResponse{}, ctx.Err() + default: + } + return container.CreateResponse{}, fakeNotFound{} + }, + containerAttachFunc: func(ctx context.Context, containerID string, options container.AttachOptions) (types.HijackedResponse, error) { + return types.HijackedResponse{}, errors.New("shouldn't try to attach to a container") + }, + imageCreateFunc: func(ctx context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) { + server, client := net.Pipe() + t.Cleanup(func() { + _ = server.Close() + }) + go func() { + enc := json.NewEncoder(server) + for i := 0; i < 100; i++ { + select { + case <-ctx.Done(): + assert.NilError(t, server.Close(), "failed to close imageCreateFunc server") + return + default: + } + assert.NilError(t, enc.Encode(jsonmessage.JSONMessage{ + Status: "Downloading", + ID: fmt.Sprintf("id-%d", i), + TimeNano: time.Now().UnixNano(), + Time: time.Now().Unix(), + Progress: &jsonmessage.JSONProgress{ + Current: int64(i), + Total: 100, + Start: 0, + }, + })) + time.Sleep(100 * time.Millisecond) + } + }() + attachCh <- struct{}{} + return client, nil + }, + Version: "1.30", + }) + + cmd := NewRunCommand(fakeCLI) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{"foobar:latest"}) + + cmdErrC := make(chan error, 1) + go func() { + cmdErrC <- cmd.ExecuteContext(ctx) + }() + + select { + case <-time.After(5 * time.Second): + t.Fatal("imageCreateFunc was not called before the timeout") + case <-attachCh: + } + + cancel() + + select { + case cmdErr := <-cmdErrC: + assert.Equal(t, cmdErr, cli.StatusError{ + StatusCode: 125, + Status: "docker: context canceled\n\nRun 'docker run --help' for more information", + }) + case <-time.After(10 * time.Second): + t.Fatal("cmd did not return before the timeout") + } +} + func TestRunCommandWithContentTrustErrors(t *testing.T) { testCases := []struct { name string