Up: teardown when command context is cancelled #11292

laurazard · 2023-12-21T12:19:29Z

What I did

Previously, if a long-lived plugin process (such as an execution of compose up) was running and then detached from a terminal, signalling the parent CLI process to exit would leave the plugin process behind.

To address this, changes were introduced on the CLI side (see: docker/cli#4599) to enable the CLI to notify a running plugin process that it should exit. This makes it so that, when the parent CLI process is going to exit, the command context of the plugin command being executed is cancelled.

This PR takes advantage of that change by using that channel on an up to know when to teardown.

Also fixed the "cancellable context" detection in cmd/compose/compose.go, as the string suffix matching was missing contexts such as:

context.Background.WithCancel.WithValue(type trace.traceContextKeyType, val <not Stringer>).WithValue(type api.DryRunKey, val <not Stringer>)

Before:

Screen.Recording.2023-12-20.at.17.40.21.mov

After:

Screen.Recording.2023-12-20.at.17.43.07.mov

Related issue
docker/cli#4599

(not mandatory) A picture of a cute animal, if possible in relation to what you did

laurazard · 2023-12-21T12:37:04Z

Looks like TestComposeCancel/metrics_on_cancel_Compose_build is failing, I'll take a look.

`AdaptCmd` was previously checking for a `.WithCancel` suffix on context strings, however it's possible for a context to be cancellable without ending in that suffix, such as when `context.WithValue` was called after `WithContext`, e.g.: ```go context.Background.WithCancel.WithValue(type trace.traceContextKeyType, val <not Stringer>).WithValue(type api.DryRunKey, val <not Stringer>) ``` Signed-off-by: Laura Brehm <laurabrehm@hey.com>

ndeloof · 2023-12-22T13:43:48Z

cmd/compose/compose.go

@@ -277,7 +278,12 @@ const PluginName = "compose"

 // RunningAsStandalone detects when running as a standalone program
 func RunningAsStandalone() bool {


Should have been replaced by https://github.com/docker/cli/blob/ae41fd913eea86eb4be153782846c33935a1039e/cli-plugins/plugin/plugin.go#L223

Agree! 😅

codecov · 2023-12-22T13:49:01Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (17da54d) 56.36% compared to head (dcbf005) 56.22%.
Report is 11 commits behind head on main.

Files	Patch %	Lines
pkg/compose/up.go	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11292      +/-   ##
==========================================
- Coverage   56.36%   56.22%   -0.15%     
==========================================
  Files         134      135       +1     
  Lines       11615    11659      +44     
==========================================
+ Hits         6547     6555       +8     
- Misses       4421     4451      +30     
- Partials      647      653       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Previously, if a long-lived plugin process (such as an execution of `compose up`) was running and then detached from a terminal, signalling the parent CLI process to exit would leave the plugin process behind. To address this, changes were introduced on the CLI side (see: docker/cli#4599) to enable the CLI to notify a running plugin process that it should exit. This makes it so that, when the parent CLI process is going to exit, the command context of the plugin command being executed is cancelled. This commit takes advantage of these changes by tapping into the command context's done channel and using it to teardown on an up. Signed-off-by: Laura Brehm <laurabrehm@hey.com>

thaJeztah

LGTM

milas

LGTM, and you're my hero for fixing this!

I tested standalone, with Compose CLI trampoline, and with Docker CLI and all looks good. (Well, the standalone case does return exit code 130 but that's not a new regression; I'll open a quick PR for that later.)

Changes were made in docker#4599 to provide a mechanism for the CLI to notify running plugin processes that they should exit, in order to improve the general CLI/plugin UX. The current implementation boils down to: 1. The CLI creates a socket 2. The CLI executes the plugin 3. The plugin connects to the socket 4. (When) the CLI receives a termination signal, it uses the socket to notify the plugin that it should exit 5. The plugin's gets notified via the socket, and cancels it's `cmd.Context`, which then gets handled appropriately This change works in most cases and fixes the issue it sets out to solve (see: docker/compose#11292) however, in the case where the user has a TTY attached and the plugin is not already handling received signals, steps 4+ changes: 4. (When) the CLI receives a termination signal, before it can use the socket to notify the plugin that it should exit, the plugin process also receives a signal due to sharing the pgid with the CLI Since we now have a proper "job control" mechanism, we can simplify the scenarios by executing the plugins with their own process group id, thereby removing the "double notification" issue and making it so that plugins can handle the same whether attached to a TTY or not. In order to make this change "plugin-binary" backwards-compatible, in the case that a plugin does not connect to the socket, the CLI passes the signal to the plugin process. Signed-off-by: Laura Brehm <laurabrehm@hey.com>

Changes were made in 1554ac3 to provide a mechanism for the CLI to notify running plugin processes that they should exit, in order to improve the general CLI/plugin UX. The current implementation boils down to: 1. The CLI creates a socket 2. The CLI executes the plugin 3. The plugin connects to the socket 4. (When) the CLI receives a termination signal, it uses the socket to notify the plugin that it should exit 5. The plugin's gets notified via the socket, and cancels it's `cmd.Context`, which then gets handled appropriately This change works in most cases and fixes the issue it sets out to solve (see: docker/compose#11292) however, in the case where the user has a TTY attached and the plugin is not already handling received signals, steps 4+ changes: 4. (When) the CLI receives a termination signal, before it can use the socket to notify the plugin that it should exit, the plugin process also receives a signal due to sharing the pgid with the CLI Since we now have a proper "job control" mechanism, we can simplify the scenarios by executing the plugins with their own process group id, thereby removing the "double notification" issue and making it so that plugins can handle the same whether attached to a TTY or not. In order to make this change "plugin-binary" backwards-compatible, in the case that a plugin does not connect to the socket, the CLI passes the signal to the plugin process. Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com> Signed-off-by: Laura Brehm <laurabrehm@hey.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

laurazard requested review from a team, ndeloof, glours and milas and removed request for a team December 21, 2023 12:20

laurazard self-assigned this Dec 21, 2023

laurazard marked this pull request as draft December 21, 2023 12:35

laurazard force-pushed the update-cli-signal-handling branch from 95800a7 to 2ff71b9 Compare December 21, 2023 12:48

laurazard force-pushed the update-cli-signal-handling branch from 2ff71b9 to 0a3cc5f Compare December 22, 2023 13:39

ndeloof reviewed Dec 22, 2023

View reviewed changes

laurazard force-pushed the update-cli-signal-handling branch from 0a3cc5f to dcbf005 Compare December 23, 2023 02:49

laurazard marked this pull request as ready for review December 23, 2023 02:50

thaJeztah approved these changes Dec 23, 2023

View reviewed changes

thaJeztah requested a review from ndeloof December 23, 2023 11:32

milas approved these changes Jan 2, 2024

View reviewed changes

milas merged commit 06af729 into docker:main Jan 2, 2024
26 checks passed

laurazard mentioned this pull request Jan 10, 2024

plugins: run plugin with new process group ID docker/cli#4769

Merged

laurazard mentioned this pull request Jan 10, 2024

Use Cobra's command.Context(), cancel execution when CLI is signalled docker/buildx#2184

Merged

dnhn mentioned this pull request Jan 11, 2024

docker-compose 2.24.0 Homebrew/homebrew-core#159659

Merged

laurazard mentioned this pull request Jan 18, 2024

cli/plugins: use same pgid + skip signal forwarding when attached to a TTY docker/cli#4792

Merged

laurazard mentioned this pull request Mar 6, 2024

SIGINT/SIGTERM signals not workig with docker compose up docker/for-mac#7150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Up: teardown when command context is cancelled #11292

Up: teardown when command context is cancelled #11292

laurazard commented Dec 21, 2023

laurazard commented Dec 21, 2023

ndeloof Dec 22, 2023

laurazard Dec 23, 2023

codecov bot commented Dec 22, 2023 •

edited

Loading

thaJeztah left a comment

milas left a comment

		@@ -277,7 +278,12 @@ const PluginName = "compose"

		// RunningAsStandalone detects when running as a standalone program
		func RunningAsStandalone() bool {

Up: teardown when command context is cancelled #11292

Up: teardown when command context is cancelled #11292

Conversation

laurazard commented Dec 21, 2023

laurazard commented Dec 21, 2023

ndeloof Dec 22, 2023

Choose a reason for hiding this comment

laurazard Dec 23, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

thaJeztah left a comment

Choose a reason for hiding this comment

milas left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2023 •

edited

Loading