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

[27.1 backport] attach: wait for exit code from ContainerWait #5302

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Jul 26, 2024

backport #5297

- Description for the changelog

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

@laurazard laurazard requested review from thaJeztah and vvoland July 26, 2024 13:13
@laurazard laurazard force-pushed the 27-attach-exit-code branch from a3f618a to 0cec554 Compare July 26, 2024 13:16
@laurazard laurazard self-assigned this Jul 26, 2024
@vvoland vvoland added the kind/bugfix PR's that fix bugs label Jul 26, 2024
@vvoland vvoland added this to the 27.1.2 milestone Jul 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.50%. Comparing base (fd3157b) to head (1cf3637).

Additional details and impacted files
@@           Coverage Diff           @@
##             27.x    #5302   +/-   ##
=======================================
  Coverage   61.50%   61.50%           
=======================================
  Files         299      299           
  Lines       20866    20867    +1     
=======================================
+ Hits        12834    12835    +1     
  Misses       7116     7116           
  Partials      916      916           

@vvoland
Copy link
Collaborator

vvoland commented Jul 26, 2024

Failures seem related to: #5229 (comment)

It added the "exit status " message, but wasn't backported to the 27.0 branch

@laurazard
Copy link
Member Author

Ah, I guess it doesn't make since to backport #5229, so I'll adjust the expected output. WDYT @vvoland?

@vvoland
Copy link
Collaborator

vvoland commented Jul 26, 2024

I'm still unsure if we should have that message at all 😅 I think we still haven't decided if this is the expected behavior or not.
@thaJeztah WDYT?

@vvoland vvoland changed the title [27.0 backport] attach: wait for exit code from ContainerWait [27.x backport] attach: wait for exit code from ContainerWait Jul 26, 2024
@vvoland vvoland changed the title [27.x backport] attach: wait for exit code from ContainerWait [27.1 backport] attach: wait for exit code from ContainerWait Jul 26, 2024
@thaJeztah
Copy link
Member

Oh! Good one; yes, I was considering if we need an explicit option on cli.StatusError to mark it as "silent" or something; effectively cli.StatusError should (probably?) be used as a wrapper for existing errors to add "status code" to them. Or at least, considering something like

err := foo()
if errdefs.IsNotFound(err) {
    // wrap the error with a atatus-code (and message?)
} 

For this branch I would update the tests to not check for the error-message if that works while we work out some of that.

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 <[email protected]>
(cherry picked from commit 7b46bfc)
Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard force-pushed the 27-attach-exit-code branch from 0cec554 to 1cf3637 Compare July 26, 2024 14:48
@laurazard
Copy link
Member Author

For this branch I would update the tests to not check for the error-message if that works while we work out some of that.

Updated.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland vvoland merged commit a35c363 into docker:27.x Jul 26, 2024
87 checks passed
renovate bot added a commit to earthly/dind that referenced this pull request Aug 19, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [docker/docker](https://togithub.com/docker/docker) | patch | `27.1.1`
-> `27.1.2` |

---

### Release Notes

<details>
<summary>docker/docker (docker/docker)</summary>

### [`v27.1.2`](https://togithub.com/moby/moby/releases/tag/v27.1.2)

[Compare
Source](https://togithub.com/docker/docker/compare/v27.1.1...v27.1.2)

#### 27.1.2

For a full list of pull requests and changes in this release, refer to
the relevant GitHub milestones:

- [docker/cli, 27.1.2
milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.1.2)
- [moby/moby, 27.1.2
milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.1.2)
- Deprecated and removed features, see [Deprecated
Features](https://togithub.com/docker/cli/blob/v27.1.2/docs/deprecated.md).
- Changes to the Engine API, see [API version
history](https://togithub.com/moby/moby/blob/v27.1.2/docs/api/version-history.md).

##### Bug fixes and enhancements

- Fix a regression that could result in a `ResourceExhausted desc =
grpc: received message larger than max` error when building from a large
Dockerfile. [moby/moby#48245](https://togithub.com/moby/moby/pull/48245)
- CLI: Fix `docker attach` printing a spurious `context cancelled` error
message. [docker/cli#5296](https://togithub.com/docker/cli/pull/5296)
- CLI: Fix `docker attach` exiting on `SIGINT` instead of forwarding the
signal to the container and waiting for it to exit.
[docker/cli#5302](https://togithub.com/docker/cli/pull/5302)
- CLI: Fix `--device-read-bps` and `--device-write-bps` options not
taking effect.
[docker/cli#5339](https://togithub.com/docker/cli/pull/5339)
- CLI: Fix a panic happening in some cases while running a plugin.
[docker/cli#5337](https://togithub.com/docker/cli/pull/5337)

##### Packaging updates

- Update BuildKit to
[v0.15.1](https://togithub.com/moby/buildkit/releases/tag/v0.15.1).
[moby/moby#48246](https://togithub.com/moby/moby/pull/48246)
- Update Buildx to
[v0.16.2](https://togithub.com/docker/buildx/releases/tag/v0.16.2).
[docker/docker-ce-packaging#1043](https://togithub.com/docker/docker-ce-packaging/pull/1043)
- Update Go runtime to 1.21.13.
[moby/moby#48301](https://togithub.com/moby/moby/pull/48301),
[docker/cli#5325](https://togithub.com/docker/cli/pull/5325),
[docker/docker-ce-packaging#1046](https://togithub.com/docker/docker-ce-packaging/pull/1046)
- Remove unused `docker-proxy.exe` binary from Windows packages.
[docker/docker-ce-packaging#1045](https://togithub.com/docker/docker-ce-packaging/pull/1045)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge
- At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/earthly/dind).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to earthly/dind that referenced this pull request Aug 19, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [docker/docker](https://togithub.com/docker/docker) | patch | `27.1.1`
-> `27.1.2` |

---

### Release Notes

<details>
<summary>docker/docker (docker/docker)</summary>

### [`v27.1.2`](https://togithub.com/moby/moby/releases/tag/v27.1.2)

[Compare
Source](https://togithub.com/docker/docker/compare/v27.1.1...v27.1.2)

#### 27.1.2

For a full list of pull requests and changes in this release, refer to
the relevant GitHub milestones:

- [docker/cli, 27.1.2
milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.1.2)
- [moby/moby, 27.1.2
milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.1.2)
- Deprecated and removed features, see [Deprecated
Features](https://togithub.com/docker/cli/blob/v27.1.2/docs/deprecated.md).
- Changes to the Engine API, see [API version
history](https://togithub.com/moby/moby/blob/v27.1.2/docs/api/version-history.md).

##### Bug fixes and enhancements

- Fix a regression that could result in a `ResourceExhausted desc =
grpc: received message larger than max` error when building from a large
Dockerfile. [moby/moby#48245](https://togithub.com/moby/moby/pull/48245)
- CLI: Fix `docker attach` printing a spurious `context cancelled` error
message. [docker/cli#5296](https://togithub.com/docker/cli/pull/5296)
- CLI: Fix `docker attach` exiting on `SIGINT` instead of forwarding the
signal to the container and waiting for it to exit.
[docker/cli#5302](https://togithub.com/docker/cli/pull/5302)
- CLI: Fix `--device-read-bps` and `--device-write-bps` options not
taking effect.
[docker/cli#5339](https://togithub.com/docker/cli/pull/5339)
- CLI: Fix a panic happening in some cases while running a plugin.
[docker/cli#5337](https://togithub.com/docker/cli/pull/5337)

##### Packaging updates

- Update BuildKit to
[v0.15.1](https://togithub.com/moby/buildkit/releases/tag/v0.15.1).
[moby/moby#48246](https://togithub.com/moby/moby/pull/48246)
- Update Buildx to
[v0.16.2](https://togithub.com/docker/buildx/releases/tag/v0.16.2).
[docker/docker-ce-packaging#1043](https://togithub.com/docker/docker-ce-packaging/pull/1043)
- Update Go runtime to 1.21.13.
[moby/moby#48301](https://togithub.com/moby/moby/pull/48301),
[docker/cli#5325](https://togithub.com/docker/cli/pull/5325),
[docker/docker-ce-packaging#1046](https://togithub.com/docker/docker-ce-packaging/pull/1046)
- Remove unused `docker-proxy.exe` binary from Windows packages.
[docker/docker-ce-packaging#1045](https://togithub.com/docker/docker-ce-packaging/pull/1045)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge
- At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/earthly/dind).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bugfix PR's that fix bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants