Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: force cli to exit after third sigint/sigterm #5171

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

Benehiko
Copy link
Member

- What I did

Added a global signal handler for cases where context cancellation is not respected. This will force the CLI to exit after 3 attempts of sigint/sigterm.

⋊> ~/G/docker-cli on master ⨯ ./build/docker login                                                                                                                                          15:41:40
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username: ^C^C^C
got 3 SIGTERM/SIGINTs, forcefully exiting

- How I did it

I register a go routine to catch further signals upon running a docker command. Docker plugin handling has its own signal handling and is excluded from this signal handler.

- How to verify it

Try with docker login since it's currently not respecting context cancellation.

- Description for the changelog

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

…lation

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
@Benehiko Benehiko requested a review from a team June 18, 2024 13:47
@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2024

Codecov Report

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

Project coverage is 61.72%. Comparing base (70b53a0) to head (faf7647).
Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5171      +/-   ##
==========================================
- Coverage   61.76%   61.72%   -0.05%     
==========================================
  Files         297      294       -3     
  Lines       20768    20772       +4     
==========================================
- Hits        12828    12822       -6     
- Misses       7024     7034      +10     
  Partials      916      916              

@Benehiko Benehiko self-assigned this Jun 18, 2024
@Benehiko Benehiko added the kind/bugfix PR's that fix bugs label Jun 18, 2024
@Benehiko Benehiko changed the title feat: add a global sigint/sigterm handler as a fallback to ctx cancel… fix: force cli to exit after third sigint/sigterm Jun 18, 2024
@Benehiko Benehiko added this to the 27.0.0 milestone Jun 18, 2024
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.

maybe we should also add a basic test case for this

cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
Copy link
Member

@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.

I had another idea, let me know what you think!

cmd/docker/docker.go Outdated Show resolved Hide resolved
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
cmd/docker/docker_test.go Outdated Show resolved Hide resolved
<-sig
}
_, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally non blocking, just food for thought..are we sure we want to exit with status 1 when the user decides to forcefully exit?

It could make sense to start differentiating expected status codes from totally unexpected failures, in order to be able to distinguish them from a tracing/metrics perspective as well.

Copy link
Member Author

@Benehiko Benehiko Jun 20, 2024

Choose a reason for hiding this comment

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

I think forcefully exiting the CLI like this should be considered a problem. It means that the process the user cancelled did not cancel the first time through context.Done().

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Copy link
Contributor

@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, thanks!

@vvoland vvoland merged commit 0d415ad into docker:master Jun 20, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants