-
Notifications
You must be signed in to change notification settings - Fork 454
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
[release-1.30] userns: Skip tests if the host doesn't support idmap mounts #1492
[release-1.30] userns: Skip tests if the host doesn't support idmap mounts #1492
Conversation
critest is used in projects like containerd, that test against older distros (like AlmaLinux 8). In those distros, CI will fail when we upgrade to runc 1.2.0. With runc 1.1 those test don't fail because runc doesn't support idmap mounts and the tests are skipped in that case. But with runc 1.2.0-rc.2, that supports idmap mounts, the tests are not skipped but fail on distros with older kernels that don't support idmap mounts. This commit just tries to detect if the path used for the container rootfs supports idmap mounts. To do that it uses the Status() message from CRI with verbose param set to true. It parses the output that containerd sets (it's quite unspecified that field), and otherwise fallbacks to "/var/lib" as the path to test idmap mounts support. Signed-off-by: Rodrigo Campos <[email protected]>
Sascha suggested to run this only once. Let's cache the answer from the runtime and move the tests that need idmap mounts on the host to `When("Host idmap mount support is needed"`. While we split the tests in that way, let's just query idmap mount support for the tests that need it, using the cache. Signed-off-by: Rodrigo Campos <[email protected]>
containerd creates a userns and inside there, it runs the critest tool. However, in that setup, the length of containerd's userns is not the whole UID space. Let's verify that the length of the userns inside the pod, when we created it with NamespaceMode_NODE (IOW, when not using a new userns for the pod) is the same as outside the pod. This works fine when contained itself runs inside a userns. Signed-off-by: Rodrigo Campos <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@rata: changing LGTM is restricted to collaborators In response to this:
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. |
The windows fail seem unrelated to this PR, the failing test are not changed here. |
The mac job seems stalled, it's not starting and running for 2hs30m:
|
Yeah, I can incorporate the fix to make CI happy. |
Signed-off-by: Sascha Grunert <[email protected]>
Thanks! And it worked, all tests green now :) |
@kubernetes-sigs/cri-tools-maintainers PTAL |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kwilczynski, rata, saschagrunert 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 |
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
Cherry-pick of #1489 into
release-1.30
.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
cc @rata
Does this PR introduce a user-facing change?