rootless: support --pid=host#41893
Merged
Merged
Conversation
thaJeztah
requested changes
Jan 19, 2021
Member
There was a problem hiding this comment.
This can use path.Clean as well (instead of filepath. Do we want a fast-path here in case no cleaning is needed?
Suggested change
| if filepath.Clean(m.Destination) == "/proc" { | |
| if m.Destination == "/proc" || path.Clean(m.Destination) == "/proc" { |
Member
Author
There was a problem hiding this comment.
That makes the code longer and I feel less readable, but just my feeling
Contributor
There was a problem hiding this comment.
Too bad we don't have a spec requirement that the paths should be absolute and cleaned.
Hmm, a somewhat similar code in buildkit do not have path.Clean():
So I'm not sure whether it is needed here.
AkihiroSuda
force-pushed
the
fix-41457
branch
2 times, most recently
from
January 26, 2021 09:18
2ce0056 to
7b15fc8
Compare
AkihiroSuda
force-pushed
the
fix-41457
branch
from
January 29, 2021 07:04
7b15fc8 to
a48f086
Compare
cpuguy83
reviewed
Jan 29, 2021
Signed-off-by: Akihiro Suda <[email protected]>
AkihiroSuda
force-pushed
the
fix-41457
branch
from
February 1, 2021 05:55
a48f086 to
227687f
Compare
Member
|
@cpuguy83 ptal |
kolyshkin
reviewed
Mar 26, 2021
|
|
||
| daemonCgroup := d.CgroupNamespace(t) | ||
| containerCgroup := containerCgroupNamespace(ctx, t, client, cID) | ||
| containerCgroup := container.GetContainerNS(ctx, t, client, cID, "cgroup") |
Contributor
There was a problem hiding this comment.
This changes the code from reading /proc/1/ns/cgroup to /proc/self/ns/cgroup but the end result should be the same, so no issue here (just noticing).
kolyshkin
reviewed
Mar 26, 2021
| if err != nil { | ||
| return false, err | ||
| } | ||
| return pidNS == selfPidNS, nil |
Contributor
There was a problem hiding this comment.
A little bit faster way would be to run stat on both paths and return (st1.Dev == st2.Dev) && (st1.Ino == st2.Ino), nil. Comparing symlinks is OK, too.
Member
Author
|
Mergeable? @kolyshkin @thaJeztah |
Member
Author
|
I see two LGTMs, so merging |
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Apr 11, 2023
The `docker/bash.sh` script should omit the `--pid=host` argument and `docker/with_the_same_user` arguments when the host's docker daemon is running in [rootless mode](https://docs.docker.com/engine/security/rootless/). The `with_the_same_user` script is unnecessary in this mode, as rootless docker daemons already use the privileges of the user. The `--pid=host` flag is required when running in CI, and while it may be useful when running tests locally, it is only supported for rootless docker in versions docker versions 22.06 or greater ([requires this commit](moby/moby#41893)).
Lunderberg
added a commit
to apache/tvm
that referenced
this pull request
Apr 18, 2023
The `docker/bash.sh` script should omit the `--pid=host` argument and `docker/with_the_same_user` arguments when the host's docker daemon is running in [rootless mode](https://docs.docker.com/engine/security/rootless/). The `with_the_same_user` script is unnecessary in this mode, as rootless docker daemons already use the privileges of the user. The `--pid=host` flag is required when running in CI, and while it may be useful when running tests locally, it is only supported for rootless docker in versions docker versions 22.06 or greater ([requires this commit](moby/moby#41893)).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
- What I did
rootless: support --pid=host
Fix #41457
Also port
TestRunModePIDHostfrom CLI test to API test- How I did it
By substituting procfs mount with rbind (https://github.com/containers/podman/blob/v3.0.0-rc1/pkg/specgen/generate/oci.go#L248-L257)
- How to verify it
DOCKER_ROOTLESS=1 TEST_SKIP_INTEGRATION_CLI=1 TESTFLAGS='-test.run TestPidHost' make test-integration- Description for the changelog
rootless: support --pid=host
- A picture of a cute animal (not mandatory but encouraged)
🐧