Conversation
|
@ebiggers if you think it's way too much stuff, we can make tests more minimal, but I used them to make sure the implementation is correct, so I decided to include them in the PR. |
|
It's a lot to review. Are you sure it cannot be simplified at all? |
|
@ebiggers are you worried about A lot of the module is actually dedicated to cgroup v1, which could also be dropped. Unfortunately there's no standard way in Go to get cgroup information, nor a userspace API in Linux, so every library has to resort to parse the cgroup file system. You can see it e.g. in the automemlimit and automaxprocs libraries, which are a lot bigger. An alternative is to depend on those, but some important functions are not public, and I don't think it makes a lot of sense to add them as dependencies in the first place. Last idea would be to actually copy-paste those files here and do minor modifications. Both are MIT licensed. What do you think? |
|
Well, both It sounds like modern systems use cgroup v2, so perhaps only support that? Would that make sense? Given that this is brand new code, and |
|
OK, I can drop v1 as I don't really need it. It was there for completeness, as most other libraries have it. About the unit tests, I could keep the Let me know if that works for you. |
|
I haven't looked at this in detail yet, but yes that sounds good. |
|
I removed |
|
Looks good, thanks. All the tests passed too. I did a squash "merge", since a lot of the commits were just undoing changes made by earlier ones. |
|
Thanks for the prompt review! |
It turned out to be a bit harder than I thought, but here we are.
This PR makes fscrypt aware of cgroup limits its running under.
Note that the implementation is touching just 2 files. The rest is really test data or helper scripts for the test data.
The main implementation decisions were:
cgroup/testdata/cgroupv1.yaml, but it's not required to run the tests, only to generate test data. We can also decide to remove that if it's confusing.Close #442.
Why no extra libs
The cgroup reading logic is implemented directly (~350 lines) rather than using a third-party library. The alternatives were evaluated and found unsuitable:
github.com/containerd/cgroups/v3: Designed for managing cgroups (create/update/delete), not for reading limits. ItsStat()method returnsmemory.maxviaUsageLimitbut does not expose the CPU quota fromcpu.max(theCPUMax.extractQuotaAndPeriod()helper is unexported). It also brings in heavy transitive dependencies (cilium/ebpf, godbus/dbus, coreos/go-systemd, sirupsen/logrus, opencontainers/runtime-spec, protobuf).github.com/mr-karan/cgroup-stats: Lightweight but only supports cgroup v2, which is insufficient for hosts still running v1.github.com/KimMachineGun/automemlimit: focuses on settingGOMEMLIMIT(i.e. GC tuning), rather than returning the limit. It only handle memory, not CPU.GOMAXPROCS, but the project targets Go 1.24, the runtime's cgroup package is internal (unexportable), and it only handles CPU, not memory.