Closed rata closed 1 month ago
Welcome @rata!
It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
/hold
Feel free to lift the hold once we got more review or you think this is good to be merged.
The test fail was unrelated to this, some github issue:
fatal: unable to access 'https://github.com/containernetworking/plugins.git/': Failed to connect to github.com port 443 after 21 ms: Connection refused
I pushed again due to that.
@saschagrunert thanks a lot! I polished the code and fixed some edge cases now. I've also verified it passes all the tests in the containerd CI too(https://github.com/containerd/containerd/actions/runs/9859447900/job/27223414193?pr=9313) :)
/unhold
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: rata, saschagrunert
The full list of commands accepted by this bot can be found here.
The pull request process is described here
I created release-1.30
and a corresponding cherry-pick in #1492
I'm testing containerd with runc 1.2.0-rc.2 and I found this issue. This (and other fixes that belong to the containerd repo) make the CI happy again.
It will be great if we can include this in a 1.30.1 crit-tools release, so we can update it in the containerd CI. We need to prepare for the runc 1.2.0 final release, so we can just update whenever it is released :).
The containerd PR is this: https://github.com/containerd/containerd/pull/9313. I did a hack (use my fork) to test the fixes indeed fix everything on containerd CI. It does, see here: https://github.com/containerd/containerd/actions/runs/9808474525/job/27084476978?pr=9313.
Without this commit, it fails on AlmaLinux 8 saying the syscall is not implemented: https://github.com/containerd/containerd/actions/runs/9797348691/job/27053863516?pr=9313#step:9:203
Is it possible to release cri-tools 1.30.1 soon with this change?
cc @saschagrunert
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.
What type of PR is this?
/kind bug /kind failing-test
What this PR does / why we need it:
We need it to fix the CI in containerd, once we update runc to 1.2. It might be needed here too, once runc 1.2 is used here
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?