Skip to content

Conversation

@laurazard
Copy link
Collaborator

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