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

attach: wait for exit code from ContainerWait #5297

Merged

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Jul 25, 2024

- What I did

Follow up to #5295

Such as with docker run, if a user CTRL-Cs while attached to a container, we should forward the signal and wait for the exit from ContainerWait, instead of just returning.

(see docker attach docs for a clear explanation of the desired behavior)

- How I did it

Used an uncancellable context for the ContainerWait call.

- How to verify it

Run the test:

TESTDIRS="./e2e/container/..." TESTFLAGS="-test.run=TestAttachInterrupt" make -f docker.Makefile test-e2e-non-experimental

- Description for the changelog

Fix `docker attach` exiting on `SIGINT` instead of forwarding the signal to the container and waiting for it to exit.

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

@laurazard laurazard self-assigned this Jul 25, 2024
@laurazard laurazard requested review from thaJeztah and vvoland July 25, 2024 09:03
@laurazard
Copy link
Member Author

Oop I forgot to commit something 😅

@laurazard laurazard marked this pull request as draft July 25, 2024 09:14
@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch from f4bb939 to 4ecb706 Compare July 25, 2024 10:04
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.00%. Comparing base (788e996) to head (7b46bfc).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5297      +/-   ##
==========================================
- Coverage   61.45%   61.00%   -0.46%     
==========================================
  Files         299      296       -3     
  Lines       20856    20847       -9     
==========================================
- Hits        12818    12717     -101     
- Misses       7122     7212      +90     
- Partials      916      918       +2     

@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch 4 times, most recently from 5a22c77 to cf42238 Compare July 25, 2024 14:08
@laurazard
Copy link
Member Author

I've spent too long trying to figure out why I'd broken it on the connhelper-ssh tests until I remembered maybe it didn't work before 🙃 It does work, just not while running inside dind, but it already didn't before. I'll try to figure out how to make the tests pass on CI, but for now I'm skipping them.

@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch from cf42238 to 3529c25 Compare July 25, 2024 16:13
@laurazard laurazard marked this pull request as ready for review July 25, 2024 16:14
@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch from 3529c25 to 8310aec Compare July 25, 2024 18:13
@laurazard
Copy link
Member Author

@thaJeztah @vvoland PTAL

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; although I'd love to have some tests that would also verify if the returned exit code came from the right place (CLI vs container process), for example:

  1. docker run --name asdf -d alpine sh -c 'trap "exit 33" SIGINT; sleep 5; exit 34'
  2. docker attach asdf
  3. send SIGINT to the cli process
  4. check if the exit code is 33 and not 130 (SIGINT)

@laurazard
Copy link
Member Author

laurazard commented Jul 26, 2024

@vvoland Oh! That was my idea with this test, since if the CLI was exiting the exit code would not be 130 but 0, but your suggestion is better since we won't be overloading 130 which could be either the CLI or the container SIGINT'ing. I'll change it.

@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch from 8310aec to 9853b17 Compare July 26, 2024 12:09
Such as with `docker run`, if a user CTRL-Cs while attached to a
container, we should forward the signal and wait for the exit from
`ContainerWait`, instead of just returning.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard laurazard force-pushed the fix-context-cancel-attach-exit-code branch from 9853b17 to 7b46bfc Compare July 26, 2024 13:05
@laurazard laurazard enabled auto-merge July 26, 2024 13:06
@laurazard laurazard merged commit bc7e64d into docker:master Jul 26, 2024
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants