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

podman exec: correctly support detaching #25180

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 31, 2025

podman exec support detaching early via the detach key sequence. In that
case the podman process should exit successfully but the container exec
process keeps running.

Now I wrote automated test for both podman run and exec detach but this
uncovered several larger issues:

  • detach sequence parsing is broken[1]
  • podman-remote exec detach is broken[2]
  • detach in general seems to be buggy/racy, seeing lot of flakes that
    fail to restore the terminal and get an EIO instead, i.e.
    "Unable to restore terminal: input/output error"

Thus I cannot add tests for now but this commit should at least fix the
obvoius case as reported by the user so I like to get this in regardless
and I will work through the other issues once I have more time.

Fixes #24895

[1] containers/common#2302
[2] #25089

Does this PR introduce a user-facing change?

podman exec can now detach with the detach keys without  causing delays and throwing an error.

podman exec support detaching early via the detach key sequence. In that
case the podman process should exit successfully but the container exec
process keeps running.

Now I wrote automated test for both podman run and exec detach but this
uncovered several larger issues:
 - detach sequence parsing is broken[1]
 - podman-remote exec detach is broken[2]
 - detach in general seems to be buggy/racy, seeing lot of flakes that
   fail to restore the terminal and get an EIO instead, i.e.
   "Unable to restore terminal: input/output error"

Thus I cannot add tests for now but this commit should at least fix the
obvoius case as reported by the user so I like to get this in regardless
and I will work through the other issues once I have more time.

Fixes containers#24895

[1] containers/common#2302
[2] containers#25089

Signed-off-by: Paul Holzinger <[email protected]>
Copy link
Contributor

openshift-ci bot commented Jan 31, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2025
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 31, 2025
@Luap99 Luap99 added No New Tests Allow PR to proceed without adding regression tests approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 31, 2025
@Luap99
Copy link
Member Author

Luap99 commented Jan 31, 2025

@mheon PTAL I think it would be good to get this bug into 5.4. It doesn't most issues I found but it fixes the original user issue so it would be good to have at least that fix out in the meantime and not block it on the other bugs.

@Luap99 Luap99 added the v5.4 label Jan 31, 2025
@mheon
Copy link
Member

mheon commented Jan 31, 2025

LGTM

@baude
Copy link
Member

baude commented Feb 3, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
@baude
Copy link
Member

baude commented Feb 3, 2025

/cherry-pick v5.4

@openshift-cherrypick-robot
Copy link
Collaborator

@baude: once the present PR merges, I will cherry-pick it on top of v5.4 in a new PR and assign it to you.

In response to this:

/cherry-pick v5.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 7afb601 into containers:main Feb 3, 2025
80 checks passed
@openshift-cherrypick-robot
Copy link
Collaborator

@baude: new pull request created: #25197

In response to this:

/cherry-pick v5.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. No New Tests Allow PR to proceed without adding regression tests release-note v5.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detaching from an attached container results in an error
4 participants