podman exec: correctly support detaching #25083
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
release-note
do-not-merge/work-in-progress
|
[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 |
openshift-ci
bot
added
the
approved
|
I still need to fix the podman-remote parts |
github-actions
bot
added
the
kind/api-change
Luap99
force-pushed
the
exec-detach
branch
2 times, most recently
from
ba4cf6b to
f8a73cc
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Luap99
force-pushed
the
exec-detach
branch
2 times, most recently
from
d0ab25e to
61fd7a4
Compare
Luap99
added a commit
to Luap99/common
that referenced
this pull request
Jan 23, 2025
The code only could handle the detach sequence when it read one byte at a time. This obviously is not correct and lead to some issues for my automated test in my podman PR[1] where I added some automated tests for detaching and the read part is really undefined and depends on the input side/kernel scheduling on how much we read at once. This is large rework to make the code check for the key sequence across the entire buffer. That is of course more work but it needs to happen for this to work correctly. I guess the only reason why this was never noticed is because normally user detach manually by typing and not in an automated way which is much slower and thus likely reads the bytes one by one. I added new test to actually confirm the behavior. And to ensure this works with various read sizes I made it a fuzz test. I had this running for a while and did not spot any issues there. The old code fails already on the simple test cases. [1] containers/podman#25083 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
added a commit
to Luap99/common
that referenced
this pull request
Jan 23, 2025
The code only could handle the detach sequence when it read one byte at a time. This obviously is not correct and lead to some issues for my automated test in my podman PR[1] where I added some automated tests for detaching and the read part is really undefined and depends on the input side/kernel scheduling on how much we read at once. This is large rework to make the code check for the key sequence across the entire buffer. That is of course more work but it needs to happen for this to work correctly. I guess the only reason why this was never noticed is because normally user detach manually by typing and not in an automated way which is much slower and thus likely reads the bytes one by one. I added new test to actually confirm the behavior. And to ensure this works with various read sizes I made it a fuzz test. I had this running for a while and did not spot any issues there. The old code fails already on the simple test cases. [1] containers/podman#25083 Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
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. Given that I could not find any existing test for the detach key functionality not even for exec I added some. This seems to reveal more issues with on podman-remote, podman-remote run detach was broken which I fixed here as well but for podman-remote exec something bigger is needed. While I thought I fixed most problems there there was a strange race condition which caused the process to just hang. Thus I skipped the remote exec test for now and filled containers#25089 to track that. Fixes containers#24895 Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Now the the detach byte sequence can be anywhere in the stream this test would flake as the randmon bytes might match the default sequence accidentally. Simply disable the sequence. Signed-off-by: Paul Holzinger <[email protected]>
When we read the stdin to forward it into the container it doesn't really make sense to have it buffered. In fact our detach.Copy() is already reading in large buffers so we do not gain anything by it. In fact this is causing a bug with the new detach.Copy() code from c/common. The Copy() function now calls into the std io.Copy() function if no detach keys are given. That function tries to use the WriterTo() method if available. bufio.Buffer has that and this function causes a empty write to the conmon attach socket[1]. I think this is a bug in the standard library. Now because the conmon socket where it writes to is a packet based socket and not a stream one the reading side (conmon) read an empty packet. This makes it think we hit EOF, not sure if this is intentional over there either. To avoid these bugs stop using bufio here. Now we keep using it in the bindings as the API there is stable so I cannot just change the type there and on the remote side it should not cause any issues as we do not write to conmon there. Now I did saw commit 1df4dba ("Switch to bufio Reader for exec streams") but I don't quite follow the reason there. The code has changed a fair bit since then so I don't think the races should still be there. If they are the CI tests should show them I presume. [1] https://cs.opensource.google/go/go/+/refs/tags/go1.23.5:src/bufio/bufio.go;l=518-521 Signed-off-by: Paul Holzinger <[email protected]>
mheon
reviewed
Jan 28, 2025
| @@ -231,3 +231,5 @@ require ( | |||
| gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect | |||
| tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/containers/common => github.com/Luap99/common v0.20.3-0.20250123195450-f0fd031b07f8 | |||
There was a problem hiding this comment.
Should this be marked WIP until this merges?
openshift-ci
bot
added
the
do-not-merge/work-in-progress
mheon
reviewed
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Given that I could not find any existing test for the detach key
functionality not even for exec I added some. This seems to reveal more
issues with on podman-remote, podman-remote run detach was broken which
I fixed here as well but for podman-remote exec something bigger is
needed. While I thought I fixed most problems there there was a strange
race condition which caused the process to just hang.
Thus I skipped the remote exec test for now and filled #25089 to track
that.
Fixes #24895
Does this PR introduce a user-facing change?