improve passing context #4774

thaJeztah · 2024-01-11T21:09:15Z

Relates to OTEL changes and graceful exit changes

See below PRs

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2024-01-11T21:11:44Z

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 61.08%. Comparing base (8651906) to head (709df33).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4774   +/-   ##
=======================================
  Coverage   61.08%   61.08%           
=======================================
  Files         295      295           
  Lines       20660    20660           
=======================================
  Hits        12621    12621           
  Misses       7142     7142           
  Partials      897      897

thaJeztah · 2024-02-21T17:06:42Z

Let me move this one out of draft, but please have a close look to see if the approach makes sense. I recall I was fiddling a bit with passing the context with the combination of closures and call-sites of different parts of the code.

cmd/docker/builder_test.go

cmd/docker/docker_test.go

Benehiko

I think there are still a few places this can be improved, especially on setting context in the dockerCLI.
dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))

I can also move this PR forward if you'd like me to since I am working on adding more context to the docker client in moby and adding support in the CLI for it.

cmd/docker/builder_test.go

cli-plugins/plugin/plugin.go

laurazard

LGTM

laurazard · 2024-03-08T11:11:36Z

@thaJeztah can you TA(nother)L?

cli-plugins/plugin/plugin.go

vvoland

Leaving request changes for now; I don't think we should merge this for v26 since it's a breaking change to the plugin.Run function.

Benehiko · 2024-04-10T08:40:40Z

cli-plugins/plugin/plugin.go

 	otel.SetErrorHandler(debug.OTELErrorHandler)

-	dockerCli, err := command.NewDockerCli()
+	dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))


Do plugins add their own ctx to the dockerCli with the makeCmd?

compose seems to use whatever cobra's context is (https://github.com/docker/compose/blob/85567ae09246ea79191e35f060d230aa5b7c0e5b/cmd/compose/compose.go#L384)

buildx uses this func defined in buildkit though, but watch out since it already handles signals (https://github.com/docker/buildx/blob/a9575a872ab2018418e668649ed488090402a04a/commands/root.go#L35

https://github.com/moby/buildkit/blob/master/util/appcontext/appcontext.go)

I see, yeah this will be breaking for plugins. I'll see what I can do to work around that.

but watch out since it already handles signals

that's fine, this PR doesn't do any signal handling here, but this is a good point and will work around this

Benehiko · 2024-04-10T11:45:58Z

cmd/docker/docker.go

@@ -352,7 +353,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
 	// We've parsed global args already, so reset args to those
 	// which remain.
 	cmd.SetArgs(args)
-	err = cmd.Execute()
+	err = cmd.ExecuteContext(ctx)


take note, instead of passing ctx to the root command through convoluted function calls I've cleaned it up a bit by instead passing context to cobra when we call ExecuteContext. This should only affect the CLI and not any plugins that are run through the CLI.

Ah, yes, this was one of the things to look into!

(Still catching up on latest changes in this PR, but yes, this was one where my approach was probably not right)

Benehiko · 2024-04-10T11:52:27Z

cli/cobra.go

@@ -23,8 +22,7 @@ import (

 // setupCommonRootCommand contains the setup common to
 // SetupRootCommand and SetupPluginRootCommand.
-func setupCommonRootCommand(ctx context.Context, rootCmd *cobra.Command) (*cliflags.ClientOptions, *cobra.Command) {
-	rootCmd.SetContext(ctx)


removed this here since we can use the cobra ExecuteContext instead. This prevents us from passing in ctx to the plugins through RunPlugin.

v26.1.0 has been released

krissetto

LGTM

cli-plugins/plugin/plugin.go

laurazard · 2024-04-24T13:54:02Z

cmd/docker/docker.go

-	err = cmd.Execute()
+	err = cmd.ExecuteContext(ctx)
+	if err != nil {
+		return err


Did this get borked during a rebase? It's returning early, should be returning after hooks get called I think

ah true! will fix

Oh! Should it still handle context errors early though? (cancelled context? or timeout?) I guess in that case the hooks should not be called?

I think it's fine to do it regardless in any case here? The plugin can decide if it wants to print something or not. Even if it's cancelled, a hook might want to add some hint/explain something.

Yeah, not sure if a cancelled context should still be used; isn't context cancelation basically; "cancel my request" (on the first possible occasion) ? In some cases that may mean some work is needed to complete the transaction (flush to disk, e.g.) but calling hooks (and execute other binaries) feels like that should not be part of that.

that's valid, sure. @Benehiko wdyt

mmh i'm not sure if hooks should be called after a cancellation, it'd feel wrong to me

I don't have too much opinion on this either way. We currently execute the hooks even when the cobra Execute function returns an error (which is a similar thing to context cancellation).

Cancelling the context early will still execute the cobra commands, but the commands checking for context cancellation will just return (with maybe an error?).

See below an example of a cobra command not acting on the context cancellation. Even if it was acting and returning an error it will still execute.

package main import ( "context" "fmt" "github.com/spf13/cobra" ) func main() { ctx, cancel := context.WithCancel(context.Background()) rootCmd := &cobra.Command{ Use: "", RunE: func(cmd *cobra.Command, args []string) error { cmd.Help() fmt.Printf("Cobra command ctx: %v\n", cmd.Context().Err()) return nil }, } cancel() fmt.Printf("Before Cobra ctx: %v\n", ctx.Err()) err := rootCmd.ExecuteContext(ctx) fmt.Printf("%v\n", err) }

⋊> ~/G/test-cobra go run . 09:14:06 Before Cobra ctx: context canceled Usage: [flags] Flags: -h, --help help for this command Cobra command ctx: context canceled <nil>

See here what the code currently looks like on the master branch
https://github.com/docker/cli/blob/master/cmd/docker/docker.go#L355-L367

laurazard

LGTM

vvoland · 2024-04-25T08:40:07Z

Ah, one more thing, can you please add the changelog description with highlighting the API change?

EDIT: Sorry, I confused PRs 🙈 There was no API change in the end here (we didn't alter the RunPlugin signature), so no need for changelog!

Explicitly create the context and set it on the CLI, instead of depending on NewDockerCli() to instance a default context. Co-authored-by: Sebastiaan van Stijn <[email protected]> Co-authored-by: Alano Terblanche <[email protected]> Signed-off-by: Alano Terblanche <[email protected]>

Benehiko · 2024-04-25T14:58:42Z

Ah, one more thing, can you please add the changelog description with highlighting the API change?

EDIT: Sorry, I confused PRs 🙈 There was no API change in the end here (we didn't alter the RunPlugin signature), so no need for changelog!

@vvoland You had the PR right, I just reverted those changes in the PR and squashed the commits

thaJeztah force-pushed the pass_context branch 4 times, most recently from eaa6886 to ce92d38 Compare January 11, 2024 21:52

thaJeztah self-assigned this Jan 12, 2024

thaJeztah force-pushed the pass_context branch 2 times, most recently from f60ef7e to 834e912 Compare January 30, 2024 16:16

thaJeztah force-pushed the pass_context branch from 834e912 to a4f985c Compare February 21, 2024 17:02

thaJeztah changed the title ~~[WIP] improve passing context~~ improve passing context Feb 21, 2024

thaJeztah marked this pull request as ready for review February 21, 2024 17:05

thaJeztah added the status/2-code-review label Feb 21, 2024

thaJeztah requested review from laurazard, neersighted and Benehiko February 21, 2024 17:06

Benehiko reviewed Feb 22, 2024

View reviewed changes

cmd/docker/builder_test.go Outdated Show resolved Hide resolved

cmd/docker/builder_test.go Outdated Show resolved Hide resolved

cmd/docker/docker_test.go Outdated Show resolved Hide resolved

Benehiko requested changes Mar 5, 2024

View reviewed changes

cmd/docker/builder_test.go Outdated Show resolved Hide resolved

This was referenced Mar 6, 2024

feat: more context on docker client moby/moby#47518

Closed

chore: add more context #4925

Closed

laurazard reviewed Mar 7, 2024

View reviewed changes

cli-plugins/plugin/plugin.go Outdated Show resolved Hide resolved

laurazard approved these changes Mar 8, 2024

View reviewed changes

Benehiko approved these changes Mar 8, 2024

View reviewed changes

krissetto approved these changes Mar 8, 2024

View reviewed changes

vvoland reviewed Mar 14, 2024

View reviewed changes

cli-plugins/plugin/plugin.go Outdated Show resolved Hide resolved

vvoland previously requested changes Mar 14, 2024

View reviewed changes

vvoland added this to the 27.0.0 milestone Mar 14, 2024

vvoland added area/plugins area/api labels Mar 14, 2024

vvoland added the kind/refactor PR's that refactor, or clean-up code label Mar 14, 2024

Benehiko mentioned this pull request Apr 10, 2024

feat: global signal handling to cancel ctx for graceful exits #4993

Merged

Benehiko force-pushed the pass_context branch 2 times, most recently from 6ae4186 to 0c8869f Compare April 10, 2024 08:30

Benehiko requested review from vvoland, laurazard, krissetto and Benehiko April 10, 2024 08:39

Benehiko reviewed Apr 10, 2024

View reviewed changes

Benehiko force-pushed the pass_context branch from 8c17995 to a10f036 Compare April 10, 2024 11:50

Benehiko reviewed Apr 10, 2024

View reviewed changes

Benehiko force-pushed the pass_context branch 2 times, most recently from 0aad14d to 8278828 Compare April 24, 2024 08:43

vvoland approved these changes Apr 24, 2024

View reviewed changes

krissetto approved these changes Apr 24, 2024

View reviewed changes

cli-plugins/plugin/plugin.go Outdated Show resolved Hide resolved

Benehiko force-pushed the pass_context branch from 8278828 to 7f5f415 Compare April 24, 2024 12:59

laurazard reviewed Apr 24, 2024

View reviewed changes

Benehiko force-pushed the pass_context branch from 7f5f415 to 709df33 Compare April 24, 2024 14:15

laurazard approved these changes Apr 24, 2024

View reviewed changes

vvoland added the impact/changelog label Apr 25, 2024

Benehiko force-pushed the pass_context branch from 709df33 to 86162f7 Compare April 25, 2024 10:01

Benehiko merged commit 7f15dfa into docker:master Apr 25, 2024
88 checks passed

thaJeztah deleted the pass_context branch April 25, 2024 14:45

vvoland removed the impact/changelog label Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve passing context #4774

improve passing context #4774

thaJeztah commented Jan 11, 2024 •

edited by vvoland

Loading

codecov-commenter commented Jan 11, 2024 •

edited

Loading

thaJeztah commented Feb 21, 2024

Benehiko left a comment

laurazard left a comment

laurazard commented Mar 8, 2024

vvoland left a comment

Benehiko Apr 10, 2024

krissetto Apr 10, 2024 •

edited

Loading

Benehiko Apr 10, 2024

Benehiko Apr 10, 2024

thaJeztah Apr 24, 2024

Benehiko Apr 10, 2024

krissetto left a comment

laurazard Apr 24, 2024

Benehiko Apr 24, 2024

thaJeztah Apr 24, 2024

laurazard Apr 24, 2024

thaJeztah Apr 24, 2024

laurazard Apr 24, 2024

krissetto Apr 24, 2024

Benehiko Apr 25, 2024

Benehiko Apr 25, 2024

laurazard left a comment

vvoland commented Apr 25, 2024 •

edited

Loading

Benehiko commented Apr 25, 2024

improve passing context #4774

improve passing context #4774

Conversation

thaJeztah commented Jan 11, 2024 • edited by vvoland Loading

codecov-commenter commented Jan 11, 2024 • edited Loading

Codecov Report

thaJeztah commented Feb 21, 2024

Benehiko left a comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

laurazard commented Mar 8, 2024

vvoland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krissetto Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krissetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

vvoland commented Apr 25, 2024 • edited Loading

Benehiko commented Apr 25, 2024

thaJeztah commented Jan 11, 2024 •

edited by vvoland

Loading

codecov-commenter commented Jan 11, 2024 •

edited

Loading

krissetto Apr 10, 2024 •

edited

Loading

vvoland commented Apr 25, 2024 •

edited

Loading