cli icon indicating copy to clipboard operation
cli copied to clipboard

feat: global signal handling to cancel ctx for graceful exits

Open Benehiko opened this issue 1 year ago • 5 comments

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 avatar Apr 08 '24 08:04 Benehiko

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     

codecov-commenter avatar Apr 08 '24 08:04 codecov-commenter

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 avatar Apr 08 '24 08:04 Benehiko

This PR is blocked by https://github.com/moby/moby/pull/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 https://github.com/docker/cli/pull/4774

Benehiko avatar Apr 10 '24 07:04 Benehiko

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.

laurazard avatar Apr 10 '24 10:04 laurazard

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 :)

Benehiko avatar Apr 10 '24 11:04 Benehiko

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 avatar Jun 06 '24 06:06 Benehiko

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

Benehiko avatar Jun 06 '24 10:06 Benehiko

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

laurazard avatar Jun 07 '24 13:06 laurazard