Use Cobra's command.Context(), cancel execution when CLI is signalled

[laurazard]

docker/cli#4599 made it so that we now have a job control mechanism for the CLI to signal running plugin processes that they should exit (via cancelling the executing command.Context).

This PR incorporates those changes in the CLI by:

• bumping the CLI version to bring in the plugin-side code for this change
• replacing usages of the appcontext package, since we're now relying on cobra's command.Context() to know when to cancel
• introducing cobrautil.ConfigureContext, which sets up the signal handling logic that was previously being provided by appcontext (and cancels the commands context when we get signalled) as well as calling the OTEL tracing utilities for the command's context that we're now using.

I've made similar changes in Compose in docker/compose#11292.

I tried to test most cases and these changes seemed to work okay, but please give it a thorough look and test them.

[crazy-max]

Needs a rebase as we already bumped to docker/cli v25.0.0-rc.1 in #2175

[crazy-max]

I think we also need to handle termination signal for Windows (os.Interrupt)

[thaJeztah]

SGTM, but I think we may need eyes from @jsternberg and @tonistiigi, because I noticed the OTEL tracing packages appears to be (ab)using appcontext to set up tracing;

buildx/vendor/github.com/moby/buildkit/util/tracing/env/traceenv.go

Lines 16 to 18 in 78adfc8

	func init() {
	appcontext.Register(initContext)
	}

Which is "dot-imported" in

buildx/cmd/buildx/tracing.go

Line 8 in 78adfc8

_ "github.com/moby/buildkit/util/tracing/env"

, so that code seems to be in use.

[crazy-max]

SGTM, but I think we may need eyes from @jsternberg and @tonistiigi, because I noticed the OTEL tracing packages appears to be (ab)using appcontext to set up tracing;

Yes I was also going to report this one, we might need to handle context inits in cobrautil.HandleContextCancellation like appcontext.Register does https:/moby/buildkit/blob/585efdcdd4bce9b25fdf613b0ba3c36c9fc6259f/util/appcontext/appcontext.go#L27-L29

[laurazard]

Ohhhhh I would have totally missed that, thank you. Those "dot-imports" and inits are so sneaky 😞

[thaJeztah]

I arrived there, because I was curious why the appcontext package didn't disappear from the vendor/ directory, so went searching "so.. what code still imports it?"

[laurazard]

Submitted moby/buildkit#4548 so that we can use detect.InitContext without appcontext. I grep'd around buildx and didn't find any other calls to appcontext.Register around the vendored code.

[laurazard]

I amended the PR with a broken commit changing the vendored code (to be the same as with moby/buildkit#4548) to showcase what this would look like after it gets merged.

[tonistiigi]

If the desire is to always control the cobra context then why not simplify this to single

	rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) {
		cmd.SetContext(appcontext.Context())
	}

I think this has the same behavior as this PR without wrapping individual commands.

Regarding (future?) handling of the termination signal via unix socket close, this can also be done with appcontext.Register() function (or just sending signal to itself, maybe that was already the plan). I'm not 100% against removing appcontext but if it works and is reusable then I don't think we should duplicate its functionality.

[laurazard]

Regarding (future?) handling of the termination signal via unix socket close, this can also be done with appcontext.Register() function

Not future, the changes are implemented on the CLI/CLI plugin lib side and the only change needed on this side is to use cobra's command.Context instead of our bespoke context setup to benefit from it. I do think removing appcontext is good, since right now it's a bit of an entangled mess with Buildx depending on appcontext which exposes/is tied to internal Buildkit tracing utilities, and that is one step towards cleaning that up, but if you prefer to keep it around though then sure, appcontext.Register() might be an approach, I'd have to look into it.

(or just sending signal to itself, maybe that was already the plan)

I'm not sure I understand what this means.

I think this has the same behavior as this PR without wrapping individual commands.

If the issue is a stylistic one, I personally don't see this as much different from the ctx := appcontext.[etc] at the beginning of each command implementation, but we can implement it some other way.

---

The important change here is to use the cobra's command.Context() for the command implementations, since with the changes in the CLI, Buildx will connect to the CLI socket and listen, and cancel the command.Context() when the CLI is signalled and the plugin should exit.

[tonistiigi]

If the issue is a stylistic one, I personally don't see this as much different from the ctx := appcontext.[etc] at the beginning of each command implementation, but we can implement it some other way.

You don't need to assign it for each command. Just once in root command would do and all commands pick this up automatically.

I'll look up the CLI plugin lib update.

[tonistiigi]

I'll look up the CLI plugin lib update.

Yes, this has already been vendored in https:/docker/buildx/blame/ba43fe08f43a7239b124fc12e158843a23ca711d/vendor/github.com/docker/cli/cli-plugins/plugin/plugin.go#L84 . That kind of context injection would work well with appcontext.Register but the current code seems fine as well. It also works fine to keep the appcontext in buildx and this plugin library code can remain as is. This code is already using the PersistentPreRunE pattern for the root command that suggested in #2184 (review) , so these hooks can be just chained together in https:/docker/buildx/blob/ba43fe08f43a7239b124fc12e158843a23ca711d/commands/root.go#L33C1-L51C1 .

I'm not sure I understand what this means.

Just

p, _ := os.FindProcess(os.Getpid())
p.Signal(syscall.SIGINT)

but not needed. Current vendored code will work well and is better as the plugin signal count is unaffected.

[laurazard]

Thanks for taking over while I was out @tonistiigi. LGTM from a quick look, but I'll also take a better look later

[laurazard]

LGTM, feel free to rebase/merge @tonistiigi (and merge 👀 )

[crazy-max]

LGTM after manual testing for untested os (macos/windows). Hopefully #2206 should help catching regression in the future but would need integration tests for these platforms as well for client side behavior (will work on this in follow-up)