Skip to content

Add support for cgroup limits#443

Merged
ebiggers merged 9 commits intogoogle:masterfrom
mbrt:master
Mar 26, 2026
Merged

Add support for cgroup limits#443
ebiggers merged 9 commits intogoogle:masterfrom
mbrt:master

Conversation

@mbrt
Copy link
Copy Markdown
Contributor

@mbrt mbrt commented Mar 25, 2026

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:

  • No extra dependencies. See below for why.
  • Support both cgroup v1 and v2. Although v1 is less common today, it's still alive, so I thought we should not limit ourselves to v2.
  • Test data is generated from a live machine, so we're relatively sure the implementation is correct.
  • Getting cgroup v1 test data is harder (as most machines run on v2), so I used a Lima VM to generate it. This is fully reproducible as I committed the Lima config in 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.
  • An extra integration test depending on Docker running in CI.

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. Its Stat() method returns memory.max via UsageLimit but does not expose the CPU quota from cpu.max (the CPUMax.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 setting GOMEMLIMIT (i.e. GC tuning), rather than returning the limit. It only handle memory, not CPU.
  • Go 1.25 runtime: Go 1.25 added built-in cgroup-aware GOMAXPROCS, but the project targets Go 1.24, the runtime's cgroup package is internal (unexportable), and it only handles CPU, not memory.

@mbrt mbrt marked this pull request as ready for review March 25, 2026 15:13
@mbrt
Copy link
Copy Markdown
Contributor Author

mbrt commented Mar 25, 2026

@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.

@ebiggers
Copy link
Copy Markdown
Collaborator

It's a lot to review. Are you sure it cannot be simplified at all?

@mbrt
Copy link
Copy Markdown
Contributor Author

mbrt commented Mar 25, 2026

@ebiggers are you worried about cgroup.go or the PR in general? Because it's basically all test code that can be dropped if you prefer it to be minimal.

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?

@ebiggers
Copy link
Copy Markdown
Collaborator

Well, both cgroup.go and the tests. Simplifications in both would be appreciated.

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 fscrypt was never cgroup-aware before, the v1 support doesn't feel very worthwhile.

@mbrt
Copy link
Copy Markdown
Contributor Author

mbrt commented Mar 25, 2026

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 TestWithRootFromTestdata and TestIntegrationCgroupLimits, as they are the closest I could get to the real thing. The first requires those many testdata files, but they are trivial, and the test code would be much smaller.

Let me know if that works for you.

@ebiggers
Copy link
Copy Markdown
Collaborator

I haven't looked at this in detail yet, but yes that sounds good.

@mbrt
Copy link
Copy Markdown
Contributor Author

mbrt commented Mar 26, 2026

I removed cgroup v1 support and tests related to that. This should be a lot more minimal.

@ebiggers ebiggers merged commit 298ed2a into google:master Mar 26, 2026
10 checks passed
@ebiggers
Copy link
Copy Markdown
Collaborator

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.

@mbrt
Copy link
Copy Markdown
Contributor Author

mbrt commented Mar 27, 2026

Thanks for the prompt review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance issues under tight cgroup limits

2 participants