rootless: support --pid=host #41893
Merged
rootless: support --pid=host #41893
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
thaJeztah
requested changes
Jan 19, 2021
rootless/specconv/specconv_linux.go
Outdated
| // Replace procfs mount with rbind | ||
| // https://github.com/containers/podman/blob/v3.0.0-rc1/pkg/specgen/generate/oci.go#L248-L257 | ||
| for i, m := range spec.Mounts { | ||
| if filepath.Clean(m.Destination) == "/proc" { |
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" { |
There was a problem hiding this comment.
That makes the code longer and I feel less readable, but just my feeling
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():
https://github.com/moby/buildkit/blob/master/util/rootless/specconv/specconv_linux.go#L28-L29
So I'm not sure whether it is needed here.
AkihiroSuda
force-pushed
the
fix-41457
branch
2 times, most recently
from
2ce0056
to
7b15fc8
Compare
AkihiroSuda
force-pushed
the
fix-41457
branch
from
7b15fc8
to
a48f086
Compare
cpuguy83
reviewed
Jan 29, 2021
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Fix moby#41457 related: https://github.com/containers/podman/blob/v3.0.0-rc1/pkg/specgen/generate/oci.go#L248-L257 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
fix-41457
branch
from
a48f086
to
227687f
Compare
|
@cpuguy83 ptal |
kolyshkin
reviewed
Mar 26, 2021
| @@ -38,7 +27,7 @@ func testRunWithCgroupNs(t *testing.T, daemonNsMode string, containerOpts ...fun | |||
| poll.WaitOn(t, container.IsInState(ctx, client, cID, "running"), poll.WithDelay(100*time.Millisecond)) | |||
|
|
|||
| daemonCgroup := d.CgroupNamespace(t) | |||
| containerCgroup := containerCgroupNamespace(ctx, t, client, cID) | |||
| containerCgroup := container.GetContainerNS(ctx, t, client, cID, "cgroup") | |||
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 |
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.
|
Mergeable? @kolyshkin @thaJeztah |
|
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)).
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.
- 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)
馃惂