feat(egress): report the sidecar's own resource usage from its cgroup - #1411
feat(egress): report the sidecar's own resource usage from its cgroup#1411ferponse wants to merge 4 commits into
Conversation
egress.system.memory.usage_bytes and egress.system.cpu.utilization come from gopsutil, so inside a container they describe the node. The sidecar runs per sandbox, which means every sandbox on a node publishes the same node figure under its own sandbox_id: N identical series that look per-sandbox and invite exactly the wrong reading, at N times the cardinality. Add egress.process.memory.usage_bytes and egress.process.cpu.time, read from the sidecar's own cgroup (v2 memory.current and cpu.stat, falling back to v1 memory.usage_in_bytes and cpuacct.usage). Additive, so no existing dashboard breaks; the system gauges are now documented as node-level rather than removed. CPU is a cumulative counter of consumed seconds instead of a sampled ratio, so it composes with rate() and does not depend on the exporter's interval. Registration is conditional on the files being readable. A pod under a runtime that does not expose cgroupfs gets no series rather than a flat zero, which would be indistinguishable from an idle sidecar. cgroupRoot is a variable so the tests can point it at a fake cgroupfs and cover v2, the v1 fallback, the unavailable case, and that registration actually follows availability. Fixes opensandbox-group#1409
github-actions
Bot
added
component/egress
size/L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03a0156668
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… docs/ Two review findings. On cgroup v1 the systemd default co-mounts cpu and cpuacct, so cpuacct.usage sits under cpu,cpuacct/ rather than cpuacct/ — the layout a container usually inherits. Probing only the latter reported CPU as unavailable on a perfectly readable cgroupfs, so egress.process.cpu.time was never registered there. Try both. The metric documentation only existed in components/egress/docs, which nothing in docs/ links to, so the published site would not have shown the new metrics or the caveat about the node-level ones. Per AGENTS.md the operations-visible content belongs in docs/, so the node-vs-sidecar distinction now lives in docs/components/egress.md, whose Observability section previously named no metrics at all, with a pointer to the component page for the full inventory.
github-actions
Bot
added
the
documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 889182e389
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Both correct — fixed in 889182e. cgroup v1 co-mount. You're right, and this would have been a silent hole rather than a visible failure: on a co-mounted v1 layout the file exists, is readable, and my code would have concluded "no cgroupfs" and never registered I went with a candidate list rather than parsing Docs. Also right, and I had the rule backwards. I applied the same fix to my other two open metric PRs (#1405, #1410) rather than waiting to be told twice. |
Review pointed out that neither runtime sets OPENSANDBOX_EGRESS_SANDBOX_ID unless the caller does, so sidecars from different sandboxes export identical attribute sets and their series collide. That predates this branch and applies to every egress metric, but it matters most here: colliding gauges look flat or flapping rather than absent, which is the failure mode this branch was trying to avoid. The env var is server-side plumbing, fixed in a separate PR. Documented here so the caveat travels with the metrics that expose it.
|
The finding is correct: neither runtime sets It is not specific to this branch — it applies to every egress metric on The server-side fix is already open as #1402, which pins I would rather not duplicate that plumbing here, since the two PRs would then conflict over the same helper. What I have done in 7ebc711 is document the dependency next to the metrics that expose it, so an operator deploying this without #1402 knows why their If you would prefer this PR to stand entirely on its own, the alternative is a resource attribute derived from something the sidecar already knows — hostname or pod IP — as a fallback identity when the env var is unset. Say the word and I will add it; I left it out because two competing identity attributes is its own kind of mess. |
…metrics # Conflicts: # components/egress/pkg/telemetry/metrics_test.go # docs/components/egress.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4eb99c107
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // cgroupRoot is where the container's own cgroup is mounted. With a cgroup namespace — | ||
| // the default for containerd and CRI-O on cgroup v2 — this path is the container's cgroup | ||
| // root, so the values below describe the sidecar and not the node. Overridden in tests. | ||
| var cgroupRoot = "/sys/fs/cgroup" |
There was a problem hiding this comment.
On Docker/cgroup-v1 hosts where the sidecar uses the host cgroup namespace, the Docker launcher I checked does not force a private cgroup namespace (server/opensandbox_server/services/docker/networking.py:431-435), so /sys/fs/cgroup is the controller mount root rather than this container's cgroup. The new v1 fallbacks then read root/controller metrics such as memory/memory.usage_in_bytes or cpu,cpuacct/cpuacct.usage, making egress.process.* report host-level usage or disappear instead of sidecar usage; derive the current cgroup path from /proc/self/cgroup/mountinfo before joining these files.
Useful? React with 👍 / 👎.
1 participant
Fixes #1409.
Problem
egress.system.memory.usage_bytesandegress.system.cpu.utilizationcome from gopsutil, i.e./proc/meminfoand/proc/stat, which inside a container describe the node.The descriptions do say "System", so the metrics are not lying — but the sidecar runs per sandbox, and the values are published with the per-sandbox attribute set:
So with 30 sandboxes on a node you get 30 identical series that look per-sandbox. A "memory by sandbox" panel is a chart of one node number repeated 30 times, at 30× the cardinality, and it invites exactly the wrong conclusion.
(
ingresshas the same file, but as a single Deployment it yields one series, so it is harmless there. This is specific to the per-sandbox sidecar.)Fix
Additive — nothing is removed or renamed, so no existing dashboard breaks. The
systemgauges are now documented as node-level, and two genuinely per-sandbox metrics are added, read from the sidecar's own cgroup:egress.process.memory.usage_bytesmemory.current→memory/memory.usage_in_bytesegress.process.cpu.timecpu.statusage_usec→cpuacct/cpuacct.usageTwo deliberate choices:
rate().secure_runtimeis exactly the case where the assumption might not hold.Testing
go test ./...green across the egress module,go vetandgofmtclean, builds forlinux/amd64anddarwin/arm64(the!linuxstub).cgroupRootis a package variable so tests can point it at a fake cgroupfs. Covered:ManualReadercollection when the files are there and are absent when they are notParsers are split into
cgroup_parse.gowith build-tag-free table tests, mirroring the existingmeminfo_parse.gosplit. They rejectmax, empty content, unparseable and negative values, andusage_usecis not confused withuser_usec— which is the kind of prefix bug that would otherwise report user time as total CPU.Note on overlap
Touches
pkg/telemetry/metrics.go, as do #1405 and #1410. Independent in substance; whichever merges first leaves the others needing a trivial rebase in the registration block.