Skip to content

fix(container-runner): read cgroup v1 memory usage under gvisor - #5556

Merged
abcxff merged 11 commits into
mainfrom
stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml
Aug 11, 2026
Merged

fix(container-runner): read cgroup v1 memory usage under gvisor#5556
abcxff merged 11 commits into
mainfrom
stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml

Conversation

@abcxff

@abcxff abcxff commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, focused fix. cpu_cores is now Option<f64> instead of silently reporting 0.0 when a counter is not readable, which fixes a real "unknown vs. truly idle" ambiguity in the old code, and decoupling memory/CPU source detection correctly handles gVisor exposing cgroup v1 memory without cgroup v2/v1 CPU accounting.

Worth confirming

  • Limit/percentage reporting is dropped for all sources, not just gVisor. The doc comment justifies this because under gVisor memory.limit_in_bytes / /proc/meminfo report sandbox size rather than the container's configured limit, which is fair. But the pre-existing comment on cgroup v2 said it was "Exact against the container limits", i.e. memory.max/cpu.max were accurate there. This change removes mem_limit_mib, mem_pct, cpu_limit_cores, and cpu_pct from the log line unconditionally, so real-Linux/cgroup-v2 deployments lose a previously-accurate "how close to OOM" signal along with the misleading gVisor one. If that's intentional (e.g. simplifying the log schema), a short doc note on why the accurate cgroup v2 limit was also dropped, not just the approximate gVisor ones, would help future readers.
  • Module doc lines 4-5 ("...so memory growth toward the limit... is visible") are now slightly stale since no limit is tracked or logged anymore, just usage over time.
  • Confirmed no other file in the repo references the removed mem_pct/cpu_limit_cores/cpu_pct/mem_limit_mib log fields or the removed read_cgroup_memory_max/read_cgroup_cpu_limit_cores/read_proc_cpu_count functions, so this is a clean, self-contained removal from a build standpoint.

Test coverage
No tests were added for the new pure logic (sample_interval() env parsing, detect_mem_source()/detect_cpu_source() preference order, read_proc_memory() now returning a single value instead of a tuple). The repo already has the exact pattern for this kind of unit test (container-runner/src/input.rs plus container-runner/tests/inline/input.rs), and these are pure functions with clear expected behavior (cgroup v2 preferred over v1 preferred over /proc; 0/unparseable interval falls back to the default). Since this PR specifically fixes a subtle format-detection bug, a few inline tests via the same #[cfg(test)] #[path = "../tests/inline/monitor.rs"] mod tests; shim would guard the source-preference order and interval parsing against regressing later.

Minor

  • RIVET_RESOURCE_USAGE_INTERVAL_MS has no floor. A value of 1 would tick every millisecond and call active_actor_ids().await in a tight loop. Low risk since this is an opt-in diagnostic knob, but a sane minimum (e.g. 50-100ms) would prevent accidental self-inflicted overhead.
  • sampling_source() (surfaced in actor.rs's "resource monitor status" log) now reports only the memory source, silently dropping the CPU source that the old combined Source enum implicitly conveyed. That's a reasonable and documented tradeoff, just worth double-checking no dashboard/alert was built around that field expecting a combined signal.

Everything else (removal of the now-dead limit-reading functions, the MemSource/CpuSource split, tabs and formatting, structured tracing fields) looks correct and matches the conventions in CLAUDE.md.

@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 6402b71 to e2a0a13 Compare August 10, 2026 19:03
@abcxff
abcxff force-pushed the stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml branch from 3336470 to ba9a490 Compare August 10, 2026 19:03
@abcxff abcxff changed the title fix(container-runner): read cgroup v1 memory usage on gen1 gvisor fix(container-runner): read cgroup v1 memory usage under gvisor Aug 10, 2026
@abcxff
abcxff force-pushed the stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml branch 3 times, most recently from 488b79e to a9d2805 Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from e2a0a13 to 5eecc2d Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml branch from a9d2805 to c5a71c8 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 5eecc2d to 25a2903 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 25a2903 to 72999be Compare August 11, 2026 13:39
@abcxff
abcxff force-pushed the stack/fix-container-runner-read-cgroup-v1-memory-usage-on-gen1-gvisor-qyxytrml branch 2 times, most recently from 74687ac to 294b40e Compare August 11, 2026 14:24
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 72999be to 00dd307 Compare August 11, 2026 14:24
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.

1 participant