[4.4] fix regression with runc --privileged rootless containers #17301
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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/test-infra repository. |
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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
|
LGTM |
|
The PR being reverted is #17127 but there's no explanation of why it's being reverted |
|
I am not sure yet whether the issue is in Podman or in runc, I've found it out with |
pkg/util/utils_linux.go
Outdated
| * for backwards compatibility. | ||
| */ | ||
| if d.Path == "/dev/ptmx" || isVirtualConsoleDevice(d.Path) { | ||
| if d.Path == "/dev/ptmx" || strings.HasPrefix(d.Path, "/dev/tty") { |
There was a problem hiding this comment.
The cause could be that isVirtualConsoleDevice evaluates to false for /dev/tty before strings.HasPrefix(d.Path, "/dev/tty") evaluated to true for it.
There was a problem hiding this comment.
This might also cause an issue in non-privileged mode that has the same change (it's not reverted in this PR).
There was a problem hiding this comment.
@fho would you mind opening a PR for upstream?
Perhaps I can just cherry-pick the fix if that proves to be obvious
There was a problem hiding this comment.
I'm hesitant to create a PR, to skip mounting /dev/tty because I don't know how to reason it.
Why must /dev/tty not be mounted and why mounting it would cause an error like: merged/dev/tty: no such device or address?
/dev/tty is not a pseudo terminal. We actually even had a short discussion about it when that change was introduced: #17055 (comment).
https://systemd.io/CONTAINER_INTERFACE/, #15878 and dcermak@5a2405a does not mention that /dev/tty must not be mounted. Am I missing something?
I also tried to reproduce the bug locally with podman e787872 but could not:
bin/podman run --rm -d --privileged quay.io/libpod/testimage:20221018 top works fine. The container starts and keeps running.
I'm using crun 1.7.2 locally, not the runc version from the bug report though.
Is that maybe an incompatibility with runc or a bug in that runc version?
|
Reverting 579c5dc should not be needed, if using isVirtualConsoleDevice in the kept for the non-privileged codepath |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@giuseppe: Sorry for introducing the regression... I sent an alternate PR (#17304) to address this regression, without reverting the fixes @fho and I worked on to address the regression that came in podman 4.3... Would that be acceptable? I can write more tests tomorrow to make sure regressions like this won't happen again.
Agreed. |
[NO NEW TESTS NEEDED] Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2165875 Signed-off-by: Martin Roukala (né Peres) <[email protected]> (cherry picked from commit d10860a)
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
cherry picked the fix instead of reverting the feature |
Glad it fixed the issue, and sorry again for the regression! |
no reason to be sorry! Issues happen all the time, that is why we have release branches :-) |
|
Looking forward to integrating podman 4.4 in boot2container :) |
|
@vrothberg PTAL |
|
/lgtm |
ashley-cui
removed
the
do-not-merge/release-note-label-needed
|
Sorry, this slipped through my mails. |
github-actions
bot
added
the
locked - please file new issue/PR
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2165875
Does this PR introduce a user-facing change?