Repository navigation
executor: warn when rootless drops resource limits - #7288
MalteMagnussen wants to merge 2 commits into
Conversation
The rootless runc and containerd executors remove the cgroup settings from the spec, so per-step resource limits (memory, cpu-quota, cpuset and so on) have no effect there. The daemon still advertises exec.meta.linux.resources, so clients send limits and get no error or warning. Log a warning and add a build warning to the step whenever it asks for limits that a rootless executor ignores. The limits are still dropped, so the build result does not change. Signed-off-by: Malte Hviid-Magnussen <malte.hviid-magnussen@group.one>
GPG signature cannot be verified |
There was a problem hiding this comment.
🟢 Approval recommended
The focused warning-only change preserves existing execution behavior, includes test coverage, and has no identified blocking issues.
No open issues 🎉
What changed in this PR
Makes ignored resource limits visible on rootless BuildKit steps, addressing the silent behavior reported in #7287 without changing limit enforcement.
Changes:
- Adds daemon-log and per-step build warnings when limits are ignored.
- Integrates warnings into both rootless executors.
- Adds unit and integration coverage.
| File | Description |
|---|---|
| executor/runcexecutor/executor.go | Emits warnings for rootless runc steps. |
| executor/resources_warning.go | Adds shared warning logic and resource detection. |
| executor/resources_warning_test.go | Tests warning emission and suppression. |
| executor/containerdexecutor/executor_unix.go | Emits warnings for rootless containerd steps. |
| client/client_test.go | Registers the integration test. |
| client/client_exec_test.go | Tests rootless warnings and rootful silence. |
Review effort: Balanced
|
Thanks, I hadn't added my SSH signing key to GitHub. Done now, the commits show as verified. |
| if res == nil { | ||
| return false | ||
| } | ||
| return res.Memory != 0 || res.MemorySwap != 0 || |
There was a problem hiding this comment.
nit: could use reflect to avoid listing all the struct field names
There was a problem hiding this comment.
Done in 9cbdc93. I used ProtoReflect().Range instead of reflect, since it only visits fields that are set and skips the generated message's internal fields.
hasLinuxResources listed every field of pb.LinuxResources, so a field added later would be missed. Use ProtoReflect().Range, which visits the fields that are set. Signed-off-by: Malte Hviid-Magnussen <malte.hviid-magnussen@group.one>
Rootless executors drop per-step resource limits (
ToRootlessclearsResourcesandCgroupsPath), but the daemon still advertisesexec.meta.linux.resources, so--resourcesilently does nothing. See #7287.This logs a warning and adds a build warning to the step when it asks for limits that a rootless executor (runc or containerd) ignores.
Safe for existing users: limits are dropped exactly as before. The only change is the warning on rootless steps that set limits.
Tested in an Ubuntu 26.04 VM with rootful Docker via
hack/test: a new unit test and the newtestLinuxResourcesIgnoredWarningpass onoci,oci-rootless,containerdandcontainerd-rootless, andtestLinuxResourcesstill passes.Note
This PR was made with Claude Code. I mainly want to put a spotlight on #7287 and would love to see a fix released, so
resourcesworks in rootless setups where cgroup v2 is available. I can't defend the design choices here, since Claude made them as a follow-up after my laptop froze during a build that had resource limits set. Feel free to close this if you'd rather fix it another way.