Skip to content

Conversation

@Benehiko
Copy link
Member

@Benehiko Benehiko commented Apr 8, 2024

Since other repositories import from the CLI, the signal termination handling done inside the PromptForConfirmation could cause problems for the third party repositories. Thus this PR hoists the signal termination out of the PromptForConfirmation function.

- What I did
Hoist the signal handling from the PromptForConfirmation function to the more appropriate top-level function runDocker.

- How I did it
Using the signal package I catch context cancellations as well as termination signals then pass the returned context to cobra's Execute method.

- How to verify it
Tests

- Description for the changelog

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

@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from 8e2f328 to 13e4776 Compare April 8, 2024 08:19
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 36 lines in your changes missing coverage. Please review.

Project coverage is 61.83%. Comparing base (8b924a5) to head (3f0d90a).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4993      +/-   ##
==========================================
+ Coverage   61.37%   61.83%   +0.46%     
==========================================
  Files         298      298              
  Lines       20717    20736      +19     
==========================================
+ Hits        12715    12823     +108     
+ Misses       7102     7000     -102     
- Partials      900      913      +13     

@Benehiko
Copy link
Member Author

Benehiko commented Apr 8, 2024

Thinking about this now, maybe I should just do this top-level inside the main function and then rework the plugin signal handling as well to accept a context.Done() instead.

@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch 2 times, most recently from 63a2225 to 47b6915 Compare April 9, 2024 14:03
@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from 47b6915 to 7ffda3e Compare April 10, 2024 07:50
@Benehiko
Copy link
Member Author

Benehiko commented Apr 10, 2024

This PR is blocked by moby/moby#47536. The cobra context isn't given to the prompt function when plugin install is called due to acceptPrivileges not accepting a context https://github.com/docker/cli/blob/master/cli/command/plugin/install.go#L145

Also need to update and merge #4774

@laurazard
Copy link
Collaborator

Looks like the linter's complaining, and some tests are broken because now the output from the graceful termination logic is different:

=== FAIL: e2e/cli-plugins TestPluginSocketCommunication/detached/the_main_CLI_exits_after_3_signals (1.10s)
    socket_test.go:232: assertion failed: 
        --- ←
        +++ →
        @@ -1,2 +1,2 @@
        -got 2 SIGTERM/SIGINTs, forcefully exiting
        +got 3 SIGTERM/SIGINTs, forcefully exiting

I think it should be fine to hardcode the 3 in that print, since that's the total number of signals we've received (counting the one that cancelled the context. It might also be possible to reword it some other way 😅


I guess we'll hold off on merging this one since it needs changes to moby that we want to get in after 26.1, so we can get it in after we cut a release.

@Benehiko
Copy link
Member Author

Looks like the linter's complaining, and some tests are broken because now the output from the graceful termination logic is different:

=== FAIL: e2e/cli-plugins TestPluginSocketCommunication/detached/the_main_CLI_exits_after_3_signals (1.10s)
    socket_test.go:232: assertion failed: 
        --- ←
        +++ →
        @@ -1,2 +1,2 @@
        -got 2 SIGTERM/SIGINTs, forcefully exiting
        +got 3 SIGTERM/SIGINTs, forcefully exiting

I think it should be fine to hardcode the 3 in that print, since that's the total number of signals we've received (counting the one that cancelled the context. It might also be possible to reword it some other way 😅

I guess we'll hold off on merging this one since it needs changes to moby that we want to get in after 26.1, so we can get it in after we cut a release.

Ah yeah, the tests won't pass right now. Not without the changes to moby and context passing in CLI.

I'll hardcode the 3 :)

@laurazard laurazard added this to the 27.0.0 milestone Apr 18, 2024
@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from 7ffda3e to 743fc70 Compare April 25, 2024 14:41
@Benehiko Benehiko changed the title fix: hoist signal handling from prompt confirmation feat: global signal handling to cancel ctx for graceful exits Apr 26, 2024
@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from 8b74c89 to 79fdc57 Compare June 4, 2024 13:26
Copy link
Collaborator

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM overall, but looks like there's some tests failing/linters complaining. I wonder if were doing things correctly with those deferred cancel()s in main.

@Benehiko
Copy link
Member Author

Benehiko commented Jun 6, 2024

LGTM overall, but looks like there's some tests failing/linters complaining. I wonder if were doing things correctly with those deferred cancel()s in main.

I have been looking into it. I've fixed one of the tests, specifically the plugins e2e signal handling and still busy on the containers e2e

@Benehiko Benehiko requested a review from a team June 6, 2024 09:12
vvoland
vvoland previously requested changes Jun 6, 2024
@Benehiko
Copy link
Member Author

Benehiko commented Jun 6, 2024

Wondering why this test is failing on the connhelper-ssh tests only. I haven't been able to reproduce it locally. Might be a race condition of sorts...
https://github.com/docker/cli/actions/runs/9398990847/job/25885517677?pr=4993#step:5:731

Copy link
Contributor

@krissetto krissetto left a comment

Choose a reason for hiding this comment

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

overall LGTM, just a couple minor comments

@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch 2 times, most recently from 39f4f3e to fbbc83e Compare June 6, 2024 13:59
@laurazard laurazard requested a review from vvoland June 7, 2024 11:25
Copy link
Collaborator

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

re-LGTM

@laurazard
Copy link
Collaborator

@vvoland can you take another look and then I'll merge?

@vvoland vvoland dismissed their stale review June 7, 2024 13:21

re-reviewing

@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from e5f3765 to a9b8be1 Compare June 7, 2024 14:52
@Benehiko Benehiko force-pushed the hoist-signal-from-prompt branch from a9b8be1 to 3f0d90a Compare June 7, 2024 14:59
Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@Benehiko Benehiko merged commit 6b58179 into docker:master Jun 10, 2024
@Benehiko Benehiko deleted the hoist-signal-from-prompt branch June 10, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants