libct/cg: eliminate remaining libct deps #4633
Conversation
kolyshkin
force-pushed
the
libct-cg-sep
branch
from
f9e55e4 to
f8de595
Compare
| var devices []*devices.Rule | ||
| for _, device := range specconv.AllowedDevices { | ||
| devices = append(devices, &device.Rule) | ||
| } | ||
| testDeviceFilter(t, devices, expected) |
There was a problem hiding this comment.
I'm not familiar wit this code yet, so take this as a question rather than an ask to change something.
But testDeviceFilter() calls deviceFilter() which is not trivial. If we change the allowed devices, shall we adapt this test to be meaningful again? It's a real question that I don't know the answer. If we do, or if it's not clear, maybe the dependency on specconv is fine (or moving the allowedDevices here), as both things seem simple and specconv is quite small too.
I understand the comment on specconv.AllowedDevices changing it is risky, but we just tried to change it, maybe we won't try again soon, but we might try again at some point in the future.
There was a problem hiding this comment.
Basically, the test checks that the EBPF code generated is as expected, using the default set of devices as input. If we change the specconv.AllowedDevices, we'll have two options:
- keep testing the "old" default set as input;
- update the test case code (both the input and the expected EBPF).
Either way is fine TBH.
I don't like the duplication of the data here, but this is for #4618 (i.e. we're preparing to move this code into a separate repo, github.com/opencontainers/cgroups), and having this to be dependent on libcontainers/specconv just for the sake of a test case is worse from my POV.
I have outlined all the other options in the commit descriptions -- please review and let me know if you like any of them better than the current approach.
There was a problem hiding this comment.
Yes, I had that context in mind and was reviewing commit by commit, so I checked the commit msg :)
My reason to ask was the reliability of the test if we split this out with a c&p and we change the list over time. I'll trust your judgment here, as I said I'm not familiar with this part of the code yet, and do what you think it is best.
If we stay with this option and you think it would be better to keep the test case using the updated list, I'd add a comment in the runc code saying to update the test in that case. But if you think it's not better to keep the test case using the updated list, it's fine like this :)
There was a problem hiding this comment.
To my best understanding it really doesn't matter what the device list here is -- and as I said this case can even be removed. I guess it was added as a way to test "the most frequently used" ruleset, and the functionality is also covered by lots of other tests.
We were using utils.ProcThreadSelf since commit 8e8b136, which provides two things: 1. locking the OS tread; 2. fallback to /proc/self/task/$TID when /proc/thread-self is not available (kernel < 3.17). Now, (1) is not needed since we only call readlink and not perform any file data operation, and (2) is not needed here as this code is only running when openat2 syscall is available, meaning kernel >= v5.6. Also, check the error from readlink, so when it fails, we do not try to enhance the error message. Signed-off-by: Kir Kolyshkin <[email protected]>
The _defaultDirPath was only used for testing, and the test case is quite easy to adopt to defaultDirPath. Signed-off-by: Kir Kolyshkin <[email protected]>
The code which determines inner cgroup path from cgroup config is identical in fs and fs2 drivers, and it is using utils.CleanPath. In preparation to move libcontainer/cgroups to a separate repo, we have to get rid of libcontainer/utils dependency. So, - copy the utils.CleanPath implementation to internal/path; - consolidate the two innerPath implementations to internal/path. Signed-off-by: Kir Kolyshkin <[email protected]>
Instead, we can just do filepath.Clean("/"+path) here.
While at it, add a comment telling why this is needed and important.
Signed-off-by: Kir Kolyshkin <[email protected]>
This was needed for a test case only, but we can easily copy the data needed. The alternatives are: - keep things as is (and have cgroups depend on runc/libcontainer/specconv); - remove this test case; - move AllowedDevices to cgroups/devices/config. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
libct-cg-sep
branch
from
f8de595 to
4e0f7a2
Compare
This removes all the remaining libcontainer dependencies from libcontainer/cgroups, preparing it to be moved to a separate repository.
For #4618.