libct/cg/sd/v1: fix freezeBeforeSet (alt)#3151
libct/cg/sd/v1: fix freezeBeforeSet (alt)#3151kolyshkin wants to merge 2 commits intoopencontainers:masterfrom
Conversation
90393eb to
f378ef1
Compare
Commit f2db879 was not working as expected: 1. It did not do the right job getting unit properties; 2. The check that deviceAllow is empty was not working. While there is a way to fix both issues, let's employ a simpler check -- if r.SkipDevices is set, and devices.list only contains "allow all" rule, we can safely skip freezing. The only issue with the patch, in case we call freezeBeforeSet before Apply(), the m.paths are not yet initialized, so we have to manually figure the path to devices subsystem in haveDeviceRules. This is being fixed elsewhere. Fixes: f2db879 Reported-by: Odin Ugedal <odin@uged.al> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test for freezeBeforeSet, checking various scenarios including those that were failing before the fix in the previous commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
f378ef1 to
d13950c
Compare
|
cirrus-ci failure on centos-stream-8 is a flake:
CI restarted. If this will happen frequently, we need a workaround (something similar to e.g. commit c27b8e7) |
|
@odinuge @AkihiroSuda ptal |
|
Some performance analysis: #3143 (comment) |
|
Still wrapping my head around this, and if this is the correct way to do it; both for k8s and for the runc binary. It is possible that a container has a parent without full device access. In that case, the scope/slice for the container will have As is now, I am not 110% comfortable with backporting this to old k8s releases, as this will potentially have a bigger impact. (Again, a simple flag would be best for k8s; both for performance and for safety). I am more comfortable with back porting #3143, as that does not have the potential issue from above.. |
|
Right, this one is not good enough. Querying unit properties seems too expensive to me. I can't think of any other option right now... Heck, let's do a flag (as long as other maintainers agree) as a stop-gap measure. Long term solution is for k8s to stop using libcontainer/cgroups (kubernetes/kubernetes#104325). |
|
Closing in favor of #3161. |
This is an alternative to #3148/#3143, switching to a faster/easier check in freezeBeforeSet.
Commit f2db879 (PR #3082) was not working:
While there is a way to fix both issues, let's employ a simpler check --
if r.SkipDevices is set, and devices.list only contains "allow all" rule,
we can safely skip freezing.
The only issue with the patch, in case we call freezeBeforeSet before
Apply(), the m.paths are not yet initialized, so we have to manually
figure the path to devices subsystem in haveDeviceRules. This is being
fixed elsewhere (currently in #3131 but I will split it out).
The test case added by #3082 was not adequate either -- it only checked
that the freeze is performed if necessary, but did not check that the freeze
is avoided if possible.
Add a test case specific to freezeBeforeSet.
Fixes: f2db879
Co-authored-by: Odin Ugedal odin@uged.al
Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com