Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ctx on run image pull #5645

Merged
merged 1 commit into from
Nov 29, 2024
Merged

fix: ctx on run image pull #5645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/command/container/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be combined with the previous commit, because the previous commit doesn't build;

./run_test.go:241:20: cannot use func(ctx context.Context, parentReference string, options image.CreateOptions) (io.ReadCloser, error) {…} (value of type func(ctx "context".Context, parentReference string, options "github.com/docker/docker/api/types/image".CreateOptions) (io.ReadCloser, error)) as func(parentReference string, options "github.com/docker/docker/api/types/image".CreateOptions) (io.ReadCloser, error) value in struct literal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Going back to the other conversation about commits, another rule I generally try to follow is that
every commit should be self-contained and buildable – if we need to revert a commit, it's always best if we can just revert a single commit than to have to remember to also revert other commits.

infoFunc func() (system.Info, error)
containerStatPathFunc func(containerID, path string) (container.PathStat, error)
containerCopyFromFunc func(containerID, srcPath string) (io.ReadCloser, container.PathStat, error)
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion cli/command/container/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand Down
8 changes: 3 additions & 5 deletions cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Benehiko marked this conversation as resolved.
Show resolved Hide resolved
config := containerCfg.Config
stdout, stderr := dockerCli.Out(), dockerCli.Err()
apiClient := dockerCli.Client()
Expand All @@ -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)
Expand All @@ -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()

Comment on lines +157 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this context is only used by the attachContainer (we use a separate one for the (status wait), would it make sense to move this to that location to make it be next to the comment describing why we remove the cancel;

cli/cli/command/container/run.go

Lines 181 to 184 in ba1a154

// 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(ctx, dockerCli, containerID, &errCh, config, container.AttachOptions{

❓ ⚠️ if we do, do we want statusCtx to be inherited from that context (for tracing purposes), or is that not relevant?

👇 if we do want statusCtx to be a "child" of the context used for attach, it should probably be something like this to not shadow the original context;

var cancelFun context.CancelFunc
ctx, cancelFun = context.WithCancel(context.WithoutCancel(ctx))
defer cancelFun()

closeFn, err := attachContainer(ctx, dockerCli, containerID, &errCh, config, container.AttachOptions{
// ....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👇 if we do want statusCtx to be a "child" of the context used for attach, it should probably be something like this to not shadow the original context;

Yeah, I believe this is the reason why we have context.WithCancel(context.WithoutCancel(xx))

Your suggestion looks good!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested change looks alright, although there are some reasons why I don't want to do anymore changes in this PR. The code has become quite difficult to follow (in-terms of which context is used where) since we've been patching out bugs introduced by the context changes. This makes me a bit afraid of doing too many code changes since I don't want to accidentally re-introduce an edge case bug.

By moving line 144

cli/cli/command/container/run.go

Lines 144 to 145 in ba1a154

ctx, cancelFun := context.WithCancel(ctx)
defer cancelFun()

down to the attachContainer call on line 184

cli/cli/command/container/run.go

Lines 181 to 184 in ba1a154

// 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(ctx, dockerCli, containerID, &errCh, config, container.AttachOptions{

we are making it harder to read the code since line 209 uses the cancelFun in some cases

cli/cli/command/container/run.go

Lines 204 to 211 in ba1a154

if err := apiClient.ContainerStart(ctx, containerID, container.StartOptions{}); err != nil {
// If we have hijackedIOStreamer, we should notify
// hijackedIOStreamer we are going to exit and wait
// to avoid the terminal are not restored.
if attach {
cancelFun()
<-errCh
}

Line 204 uses the context (without original cancel) from line 144

cli/cli/command/container/run.go

Line 204 in ba1a154

if err := apiClient.ContainerStart(ctx, containerID, container.StartOptions{}); err != nil {

We cancelled the top-level context originally in this commit 991b130 to revert the runContainer function to a pre-context behavior since there might exist too many unknown edge case behaviors. For that reason I tried to keep as close to the pre-context behavior as possible, by only moving the code stripping the context a few lines down.

Due to the shared ctx between line 144 and 184 and 204 and the cancelFun used on line 209, it makes me worried that by moving the ctx code from line 144 to line 184 (above attachContainer) we might have some case where the cancelFun is used but is now nil, or where a future change would be more difficult due to more spaghetti usage of ctx and cancelFun from line 144.

If it is alright with you @thaJeztah, I would rather we have future PRs that focus on making some of the code more context aware and slowly split up the runContainer function so we have less context spaghetti.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Benehiko yeah, that's fine; we can move this in a follow-up. It was mostly because the comment described something that happened elsewhere, so I thought moving the code there could help understanding, but it's not critical for sure.

var (
waitDisplayID chan struct{}
errCh chan error
Expand Down
86 changes: 86 additions & 0 deletions cli/command/container/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package container

import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net"
"syscall"
Expand All @@ -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"
Expand Down Expand Up @@ -217,6 +221,88 @@ func TestRunAttachTermination(t *testing.T) {
}
}

func TestRunPullTermination(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
laurazard marked this conversation as resolved.
Show resolved Hide resolved

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) {
Comment on lines +230 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be touching up the earlier commit (test: pull on run should be ctx cancellable); better to combine it with that one.

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: if we backport this PR, this line probably needs to be adjusted for the 27.x branch (as the "Run 'docker run --help'" part changed in master / v28)

})
case <-time.After(10 * time.Second):
t.Fatal("cmd did not return before the timeout")
}
}

func TestRunCommandWithContentTrustErrors(t *testing.T) {
testCases := []struct {
name string
Expand Down
Loading