ateom gvisor: drop privileged: true - #496
Merged
Zoe Zhao (zoez7) merged 4 commits intoJul 24, 2026
Merged
Conversation
Actor packets enter the worker pod via the host-side veth and leave through the pod's eth0, which requires IPv4 forwarding. Without privileged the runtime bind-mounts /proc/sys read-only, so the worker (which holds CAP_SYS_ADMIN with no user namespace, leaving the ro flag unlocked) clears the flag with a bind-remount, writes the sysctl, and restores read-only.
Replace the --ignore-cgroups workaround with real cgroup delegation so runsc creates a per-actor-container cgroup leaf (with cpu/memory/pids accounting) nested under the worker pod's own cgroup, so those stats roll up to the pod and kubelet pod metrics capture the actor. At startup the ateom prepares the pod cgroup for delegation (setupCgroupDelegation): the unprivileged worker runs in a private cgroup namespace, so /sys/fs/cgroup is the pod's own scope. It remounts that scope read-write (the runtime bind-mounts it read-only), moves the worker's own processes into a dedicated "ateom" leaf to satisfy the cgroup v2 no-internal-processes rule, and enables the delegated controllers in cgroup.subtree_control. This only runs inside a private cgroup namespace; a privileged worker inherits the host cgroup namespace (the true root, full of unmovable kernel threads), so inPrivateCgroupNamespace() detects that via /proc/self/cgroup and skips delegation. Each container's cgroupsPath is set to "/<containerName>" in the bundle's OCI spec before runsc create/restore (ensureContainerCgroupsPath). atelet emits a runtime-agnostic spec, so the gVisor ateom fills in its own convention here, mirroring how the micro-VM ateom assigns /ateomchv/<id>.
Give the ateom container a per-sandbox-class security context instead of always running privileged. gVisor workers now run unprivileged: drop ALL capabilities and add only the set runsc needs (the gofer's user-namespace identity map needs SETUID/SETGID/SETPCAP/SETFCAP; the sandbox needs SYS_ADMIN/SYS_CHROOT/SYS_PTRACE; actor networking needs NET_ADMIN/NET_RAW; OCI rootfs setup needs DAC_OVERRIDE/FOWNER/CHOWN/MKNOD). The default seccomp profile is retained; AppArmor is set to Unconfined because runsc's mounts and the worker's cgroup remount are denied by the default profile (enforced on GKE COS/Ubuntu), which a privileged worker got implicitly. Micro-VM workers stay privileged (kata + cloud-hypervisor needs broad host access). A tailored AppArmor profile for the gVisor worker is left as a follow-up.
Davanum Srinivas (dims)
approved these changes
Jul 22, 2026
Davanum Srinivas (dims)
left a comment
Davanum Srinivas (dims)
left a comment
Collaborator
There was a problem hiding this comment.
LGTM in principle :)
Collaborator
|
I tested the cgroups fix locally and got the output below I'm wondering why is the "counter" cgroup empty with no process IDs in it? is this intended? |
Collaborator
Author
|
Ugh, I guess I didn't actually submit my reply?
Yes. So the pause / sandbox container has the whole gvisor ~pod under it. The other leaf cgroups are ~harmless. I left it versus adding more branching/complexity to the code for now but I could see either way. gvisor should internally enforce the resources etc. |
Zoe Zhao (zoez7)
previously approved these changes
Jul 23, 2026
Zoe Zhao (zoez7)
left a comment
Zoe Zhao (zoez7)
left a comment
Collaborator
There was a problem hiding this comment.
Left some questions. To be honest I'm totally pretending to know what I'm reviewing in this PR :). Manual verification looked good. LGTM to ship this since this is a improvement from what we have now, and let Tim and Taahir complain when they are back from vacation.
| // only ever lists processes that are not already in a child cgroup, and the list | ||
| // shrinks as we drain it, so loop until the source is empty. | ||
| func moveProcs(ctx context.Context, srcProcs, dstProcs string) error { | ||
| for range 100 { |
Collaborator
There was a problem hiding this comment.
Nit: could you leave a comment to explain where the number "100" comes from?
Collaborator
Author
There was a problem hiding this comment.
Good point.
Collaborator
Author
There was a problem hiding this comment.
This is a somewhat arbitrary upper bound on attempts. Added a code comment.
| if private, err := inPrivateCgroupNamespace(); err != nil { | ||
| return fmt.Errorf("while detecting cgroup namespace: %w", err) | ||
| } else if !private { | ||
| slog.InfoContext(ctx, "not in a private cgroup namespace; skipping cgroup delegation (worker is likely privileged)") |
Collaborator
There was a problem hiding this comment.
When do we expect worker to be privileged when using gvisor sandbox class?
Collaborator
Author
There was a problem hiding this comment.
This is a skew guard.
Though technically we could also hit this on say, a cgroup v1 host.
Also very technically: cgroupv2 unprivileged container == private cgroupns is more of a convention across docker/podman/containerd/cri-o.
You could also explicitly opt into into it in cgroupv1, but there was some sort of agreement to make it default with the migration to v2. But they're not actually linked from the kernel perspective, only major container runtimes.
| sc := corev1ac.SecurityContext(). | ||
| WithRunAsUser(0). | ||
| WithRunAsGroup(0) | ||
| if class == atev1alpha1.SandboxClassMicroVM { |
Collaborator
There was a problem hiding this comment.
Does this mean the "shape" of cgroup hierarchy will look different depending on microvm vs. givsor?
Collaborator
Author
There was a problem hiding this comment.
Yeah, though that's only mildly relevant, since the actual actor cgroups are inside the microVM guest environment anyhow. It's a different kernel etc.
I'm also looking at patching uVM for this, for overlapping reasons (the device passthrough angle), but it's tricky.
Collaborator
|
Also fyi Haven Xia (@HavenXia) changes like this will rely on the upgrade strategy that you are working on :) |
Review feedback on agent-substrate#496: document why the loop re-reads cgroup.procs and where the 100-iteration cap comes from.
Collaborator
|
Can we hold this? for egress/ingress work - we probably going to need privileges for installing nftable rules for redirection from the actor to the tunneling component. |
Collaborator
Author
We already install nftables rules, the actor has |
Collaborator
Yes - for system upgrade with such change we will need some batch suspend and resume mechanism to handle instead of dropping everything. |
2 tasks
Tim Bai (baizhenyu)
added a commit
to baizhenyu/substrate
that referenced
this pull request
Aug 4, 2026
Fills in the measurement half of GetWorkloadStats for the gVisor runtime, so it returns real numbers instead of Unimplemented. The micro-VM runtime keeps its stub until the guest-agent read lands. The sample comes from the sandbox's cgroup v2 leaf at /sys/fs/cgroup/pause. gVisor runs every container of a sandbox inside one host process, the sentry, and runsc places that process in the cgroup of the container that created the sandbox -- "pause", the first container RunWorkload and RestoreWorkload create. The actor's own containers get leaves too, but their memory and CPU are the sentry's and are charged there instead, which is why a sample is attributed to the actor rather than to a container. The path follows the "/" + containerName convention runsc.ensureContainerCgroupsPath writes into the OCI spec, resolved against the pod's own cgroup scope that setupCgroupDelegation prepares. The read lives in a new cmd/ateom-gvisor/internal/cgroupstats. It takes the cgroup directory as an argument rather than reaching for an absolute path, so the parsing is testable from a fixture tree without root or a live sandbox, and it carries no build tag so those tests run everywhere. It fails only when memory.current is missing or unparseable -- wrapping fs.ErrNotExist in the first case, so the handler can tell "the sandbox is gone" from "the format is not what we parse". Every other file degrades to zero on its own field, because each has a legitimate reason to be absent on a healthy node: memory.peak predates kernel 5.19, and setupCgroupDelegation enables controllers best-effort, so a cgroup with memory but no cpu is reachable. Reporting no memory numbers because the node could not report CPU would be the wrong trade. Working set is memory.current less memory.stat's inactive_file, saturating at zero rather than wrapping: the two files are read a moment apart and are not a consistent snapshot, so inactive_file can legitimately exceed the memory.current read just before it. AteomService.activeActor becomes an atomic.Pointer. The three lifecycle RPCs still hold AteomService.lock for their whole bodies and keep doing so; the point is the reader. GetWorkloadStats is polled on a timer for a workload's whole lifetime while lock is held across entire boots and checkpoints, so a lock-guarded read would park each poll behind a multi-second runsc call and let pollers pile up -- and holding the lock across the cgroup read would put a CheckpointWorkload behind telemetry, which is the worse direction. The field is only ever assigned or cleared as a whole pointer, never mutated in place, which is what atomic.Pointer is for. The handler reloads it after the read and compares pointer identity, so a checkpoint plus a fresh run completing underneath the read is reported as a failed precondition rather than misattributed. Both ateoms also gain a panic-recovery interceptor, chained ahead of InternalServerUnaryInterceptor. grpc-go serves each RPC on its own goroutine and does not recover handler panics, and the Go runtime kills the process on an unrecovered panic in any goroutine, so today a nil dereference in any handler ends every other RPC the ateom is serving -- including an in-flight checkpoint. That gap predates this change, but adding a caller that polls on a timer for the life of every workload is what makes it reachable, and the stats paths are exactly the shape that hits it: the "no workload here" state is a nil pointer, and cgroup files are parsed line by line. Not covered here: that the sentry lands in /sys/fs/cgroup/pause on a live node is a runsc placement behavior, derived from the spec convention above but not verified from a unit test. Process listings in agent-substrate#161 confirm runsc-sandbox and both gofers sit in the "pause" cgroup, but those runs predate agent-substrate#496, so they establish the leaf name rather than the absolute path. The leaf holds the sentry's own overhead and the gofers alongside the actor's work, which is what the proto means by measuring the SANDBOX; splitting the actor's share out would need the sentry's own accounting. Part of agent-substrate#594
Tim Bai (baizhenyu)
added a commit
to baizhenyu/substrate
that referenced
this pull request
Aug 7, 2026
Fills in the measurement half of GetWorkloadStats for the gVisor runtime, so it returns real numbers instead of Unimplemented. The micro-VM runtime keeps its stub until the guest-agent read lands. The sample comes from the sandbox's cgroup v2 leaf at /sys/fs/cgroup/pause. gVisor runs every container of a sandbox inside one host process, the sentry, and runsc places that process in the cgroup of the container that created the sandbox -- "pause", the first container RunWorkload and RestoreWorkload create. The actor's own containers get leaves too, but their memory and CPU are the sentry's and are charged there instead, which is why a sample is attributed to the actor rather than to a container. The path follows the "/" + containerName convention runsc.ensureContainerCgroupsPath writes into the OCI spec, resolved against the pod's own cgroup scope that setupCgroupDelegation prepares. The read lives in a new cmd/ateom-gvisor/internal/cgroupstats. It takes the cgroup directory as an argument rather than reaching for an absolute path, so the parsing is testable from a fixture tree without root or a live sandbox, and it carries no build tag so those tests run everywhere. It fails only when memory.current is missing or unparseable -- wrapping fs.ErrNotExist in the first case, so the handler can tell "no cgroup to measure" from "the format is not what we parse". Every other file degrades to zero on its own field, because each has a legitimate reason to be absent on a healthy node: memory.peak predates kernel 5.19, and setupCgroupDelegation enables controllers best-effort, so a cgroup with memory but no cpu is reachable. Reporting no memory numbers because the node could not report CPU would be the wrong trade. Working set is memory.current less memory.stat's inactive_file, saturating at zero rather than wrapping: the two files are read a moment apart and are not a consistent snapshot, so inactive_file can legitimately exceed the memory.current read just before it. The handler is the first user of the NOT_FOUND / FAILED_PRECONDITION split the previous commit documented, and follows it: available and a UID mismatch are both NOT_FOUND, since each tells the caller the actor is not here and its worker-to-actor mapping wants re-resolving. A missing cgroup under a matching UID is the transient FAILED_PRECONDITION -- usually a poll landing in the boot, since attribution is now retained from the moment the ateom accepts the actor, before runsc has created the leaf. A malformed cgroup is Internal: not a routine race, so it must not be reported as one. The handler takes no lock, which is what the atomic on activeActor bought and what TestGetWorkloadStatsDoesNotTakeLock pins: it holds s.lock across the call, so a handler that reached for it deadlocks. After the read it reloads activeActor and compares pointer identity, so a checkpoint plus a fresh run completing underneath the read is NOT_FOUND rather than misattributed -- the same answer a retry would get, rather than a second code for one state. Both ateoms also gain a panic-recovery interceptor, chained ahead of InternalServerUnaryInterceptor. grpc-go serves each RPC on its own goroutine and does not recover handler panics, and the Go runtime kills the process on an unrecovered panic in any goroutine, so today a nil dereference in any handler ends every other RPC the ateom is serving -- including an in-flight checkpoint. That gap predates this change, but adding a caller that polls on a timer for the life of every workload is what makes it reachable, and the stats paths are exactly the shape that hits it: the "no workload here" state is a nil pointer, and cgroup files are parsed line by line. Not covered here: that the sentry lands in /sys/fs/cgroup/pause on a live node is a runsc placement behavior, derived from the spec convention above but not verified from a unit test. Process listings in agent-substrate#161 confirm runsc-sandbox and both gofers sit in the "pause" cgroup, but those runs predate agent-substrate#496, so they establish the leaf name rather than the absolute path. The leaf holds the sentry's own overhead and the gofers alongside the actor's work, which is what the proto means by measuring the SANDBOX; splitting the actor's share out would need the sentry's own accounting. Part of agent-substrate#594
Benjamin Elder (BenTheElder)
pushed a commit
that referenced
this pull request
Aug 7, 2026
…502) ## Summary - `atecontroller` propagates a pool's `nvidia.com/gpu` request onto the `ateom` container and mounts the host NVIDIA toolkit read-only (path overridable via `ATE_NVIDIA_TOOLKIT_HOST_PATH`) - `ateom-gvisor` generates a CDI spec with `nvidia-ctk` and injects the device nodes, driver-library mounts, and env into each actor container's OCI spec - runs the CDI `createContainer` hooks except `update-ldcache` which needs a privileged ateom, staging the SONAME symlinks it would create from each library's ELF `DT_SONAME` - enables `runsc --nvproxy` at sandbox creation Requesting `nvidia.com/gpu` on the pool is the only configuration needed; a pool that requests N GPUs makes all N usable. Two details of the CDI spec are worth calling out, because getting either wrong fails at runtime rather than at parse time. `nvidia-ctk` leaves `major`/`minor` unset — CDI delegates that to the OCI runtime — so each device node is resolved by stat-ing the host; without it the actor gets `0,0` char devices and NVML reports it cannot communicate with the driver. And it emits per-index, per-UUID, and `all` devices that repeat the same nodes, so only `all` is applied. The spec is plain JSON, so `encoding/json` suffices and no CDI library is vendored. `update-ldcache` is the one hook that cannot run here: its `ldconfig` unshares a mount namespace and mounts a private `/proc`, which `mount_too_revealing()` rejects under the pod's masked `/proc`. Permitting it would need `procMount: Unmasked`, which Kubernetes only allows with `hostUsers: false`, and that user namespace breaks the per-actor cgroup delegation from #496. Skipping it avoids the whole chain, so a GPU worker keeps the same posture as any other unprivileged gVisor worker. `create-symlinks` and `enable-cuda-compat` still run unmodified. `--nvproxy` must be set when the sandbox is created — the `pause` container, which holds no GPU devices — so runsc's auto-detection never fires on its own; without the flag the GPU subcontainer crashes the sentry on start. GPU detection matches any device index rather than assuming `/dev/nvidia0`, since a worker sharing a multi-GPU node can be assigned `/dev/nvidia2` and `/dev/nvidia3`. GPU pools must set `spec.ateomImage` to a glibc build (`KO_DEFAULTBASEIMAGE=debian:stable-slim ko build ./cmd/ateom-gvisor`) because the distroless default cannot exec `nvidia-ctk`; the default base is unchanged for every other pool. `atelet` also has to run on the GPU nodes to restore actors there, so its DaemonSet needs a toleration for whatever taint they carry. Both are documented in the API guide rather than defaulted. ## Testing - `make test` - `env -u NO_COLOR make verify` - Real GPU, Tesla T4 / driver 580.65.06, through the full actor flow: `nvidia-smi`, `vectorAdd`, `nbody` at 3.77 TFLOP/s, PyTorch matmul via cuBLAS at 3.5 TFLOP/s (T4 peak FP32 is ~8.1, so no measurable sandbox penalty) - Actor whose entrypoint runs the CUDA sample directly exits 0, confirming the injected env reaches the workload without help from the test harness - Two workers holding two GPUs each on one 4-GPU node see disjoint device sets Snapshot and restore work when the workload holds no CUDA context. A live CUDA context cannot be checkpointed - gVisor fails with `can't save with live nvproxy clients` and the failed checkpoint terminates the sandbox so a GPU actor can only be suspended between CUDA workloads. Documented as a known limitation in the API guide; a follow-up issue will track lifting it via `cuda-checkpoint`. Fixes #627 - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR --------- Signed-off-by: Eliran Wolff <eliranw@nvidia.com>
Tim Bai (baizhenyu)
added a commit
to baizhenyu/substrate
that referenced
this pull request
Aug 10, 2026
Fills in the measurement half of GetWorkloadStats for the gVisor runtime, so it returns real numbers instead of Unimplemented. The micro-VM runtime keeps its stub until the guest-agent read lands. The sample comes from the sandbox's cgroup v2 leaf at /sys/fs/cgroup/pause. gVisor runs every container of a sandbox inside one host process, the sentry, and runsc places that process in the cgroup of the container that created the sandbox -- "pause", the first container RunWorkload and RestoreWorkload create. The actor's own containers get leaves too, but their memory and CPU are the sentry's and are charged there instead, which is why a sample is attributed to the actor rather than to a container. The path follows the "/" + containerName convention runsc.ensureContainerCgroupsPath writes into the OCI spec, resolved against the pod's own cgroup scope that setupCgroupDelegation prepares. The read lives in a new cmd/ateom-gvisor/internal/cgroupstats. It takes the cgroup directory as an argument rather than reaching for an absolute path, so the parsing is testable from a fixture tree without root or a live sandbox, and it carries no build tag so those tests run everywhere. It fails only when memory.current is missing or unparseable -- wrapping fs.ErrNotExist in the first case, so the handler can tell "no cgroup to measure" from "the format is not what we parse". Every other file degrades to zero on its own field, because each has a legitimate reason to be absent on a healthy node: memory.peak predates kernel 5.19, and setupCgroupDelegation enables controllers best-effort, so a cgroup with memory but no cpu is reachable. Reporting no memory numbers because the node could not report CPU would be the wrong trade. Working set is memory.current less memory.stat's inactive_file, saturating at zero rather than wrapping: the two files are read a moment apart and are not a consistent snapshot, so inactive_file can legitimately exceed the memory.current read just before it. The handler is the first user of the NOT_FOUND / FAILED_PRECONDITION split the previous commit documented, and follows it: available and a UID mismatch are both NOT_FOUND, since each tells the caller the actor is not here and its worker-to-actor mapping wants re-resolving. A missing cgroup under a matching UID is the transient FAILED_PRECONDITION -- usually a poll landing in the boot, since attribution is now retained from the moment the ateom accepts the actor, before runsc has created the leaf. A malformed cgroup is Internal: not a routine race, so it must not be reported as one. The handler takes no lock, which is what the atomic on activeActor bought and what TestGetWorkloadStatsDoesNotTakeLock pins: it holds s.lock across the call, so a handler that reached for it deadlocks. After the read it reloads activeActor and compares pointer identity, so a checkpoint plus a fresh run completing underneath the read is NOT_FOUND rather than misattributed -- the same answer a retry would get, rather than a second code for one state. Deliberately not here: a panic-recovery interceptor. grpc-go serves each RPC on its own goroutine and does not recover handler panics, and the Go runtime kills the process on an unrecovered panic in any goroutine, so a nil dereference in any handler today ends every other RPC the ateom is serving, including an in-flight checkpoint. That gap predates this change and is not made reachable by it: nothing calls GetWorkloadStats yet, and neither the handler (which nil-checks before every dereference) nor the parser (which length-checks before indexing) has a panic path. It wants its own PR, covering every server rather than the two ateoms a telemetry change happens to touch, and it needs to land before the phase that adds a caller polling on a timer. Not covered here: that the sentry lands in /sys/fs/cgroup/pause on a live node is a runsc placement behavior, derived from the spec convention above but not verified from a unit test. Process listings in agent-substrate#161 confirm runsc-sandbox and both gofers sit in the "pause" cgroup, but those runs predate agent-substrate#496, so they establish the leaf name rather than the absolute path. The leaf holds the sentry's own overhead and the gofers alongside the actor's work, which is what the proto means by measuring the SANDBOX; splitting the actor's share out would need the sentry's own accounting. Part of agent-substrate#594
Tim Bai (baizhenyu)
added a commit
to baizhenyu/substrate
that referenced
this pull request
Aug 10, 2026
Fills in the measurement half of GetWorkloadStats for the gVisor runtime, so it returns real numbers instead of Unimplemented. The micro-VM runtime keeps its stub until the guest-agent read lands. The sample comes from the sandbox's cgroup v2 leaf at /sys/fs/cgroup/pause. gVisor runs every container of a sandbox inside one host process, the sentry, and runsc places that process in the cgroup of the container that created the sandbox -- "pause", the first container RunWorkload and RestoreWorkload create. The actor's own containers get leaves too, but their memory and CPU are the sentry's and are charged there instead, which is why a sample is attributed to the actor rather than to a container. The path follows the "/" + containerName convention runsc.ensureContainerCgroupsPath writes into the OCI spec, resolved against the pod's own cgroup scope that setupCgroupDelegation prepares. The read lives in a new cmd/ateom-gvisor/internal/cgroupstats. It takes the cgroup directory as an argument rather than reaching for an absolute path, so the parsing is testable from a fixture tree without root or a live sandbox, and it carries no build tag so those tests run everywhere. It fails only when memory.current is missing or unparseable -- wrapping fs.ErrNotExist in the first case, so the handler can tell "no cgroup to measure" from "the format is not what we parse". Every other file degrades to zero on its own field, because each has a legitimate reason to be absent on a healthy node: memory.peak predates kernel 5.19, and setupCgroupDelegation enables controllers best-effort, so a cgroup with memory but no cpu is reachable. Reporting no memory numbers because the node could not report CPU would be the wrong trade. Working set is memory.current less memory.stat's inactive_file, saturating at zero rather than wrapping: the two files are read a moment apart and are not a consistent snapshot, so inactive_file can legitimately exceed the memory.current read just before it. The handler is the first user of the NOT_FOUND / FAILED_PRECONDITION split the previous commit documented, and follows it: available and a UID mismatch are both NOT_FOUND, since each tells the caller the actor is not here and its worker-to-actor mapping wants re-resolving. A missing cgroup under a matching UID is the transient FAILED_PRECONDITION -- usually a poll landing in the boot, since attribution is now retained from the moment the ateom accepts the actor, before runsc has created the leaf. A malformed cgroup is Internal: not a routine race, so it must not be reported as one. The handler takes no lock, which is what the atomic on activeActor bought and what TestGetWorkloadStatsDoesNotTakeLock pins: it holds s.lock across the call, so a handler that reached for it deadlocks. After the read it reloads activeActor and compares pointer identity, so a checkpoint plus a fresh run completing underneath the read is NOT_FOUND rather than misattributed -- the same answer a retry would get, rather than a second code for one state. Deliberately not here: a panic-recovery interceptor. grpc-go serves each RPC on its own goroutine and does not recover handler panics, and the Go runtime kills the process on an unrecovered panic in any goroutine, so a nil dereference in any handler today ends every other RPC the ateom is serving, including an in-flight checkpoint. That gap predates this change and is not made reachable by it: nothing calls GetWorkloadStats yet, and neither the handler (which nil-checks before every dereference) nor the parser (which length-checks before indexing) has a panic path. It wants its own PR, covering every server rather than the two ateoms a telemetry change happens to touch, and it needs to land before the phase that adds a caller polling on a timer. Not covered here: that the sentry lands in /sys/fs/cgroup/pause on a live node is a runsc placement behavior, derived from the spec convention above but not verified from a unit test. Process listings in agent-substrate#161 confirm runsc-sandbox and both gofers sit in the "pause" cgroup, but those runs predate agent-substrate#496, so they establish the leaf name rather than the absolute path. The leaf holds the sentry's own overhead and the gofers alongside the actor's work, which is what the proto means by measuring the SANDBOX; splitting the actor's share out would need the sentry's own accounting. Part of agent-substrate#594
Tim Bai (baizhenyu)
added a commit
to baizhenyu/substrate
that referenced
this pull request
Aug 10, 2026
Fills in the measurement half of GetWorkloadStats for the gVisor runtime, so it returns real numbers instead of Unimplemented. The micro-VM runtime keeps its stub until the guest-agent read lands. The sample comes from the sandbox's cgroup v2 leaf at /sys/fs/cgroup/pause. gVisor runs every container of a sandbox inside one host process, the sentry, and runsc places that process in the cgroup of the container that created the sandbox -- "pause", the first container RunWorkload and RestoreWorkload create. The actor's own containers get leaves too, but their memory and CPU are the sentry's and are charged there instead, which is why a sample is attributed to the actor rather than to a container. The path follows the "/" + containerName convention runsc.ensureContainerCgroupsPath writes into the OCI spec, resolved against the pod's own cgroup scope that setupCgroupDelegation prepares. The read lives in a new cmd/ateom-gvisor/internal/cgroupstats. It takes the cgroup directory as an argument rather than reaching for an absolute path, so the parsing is testable from a fixture tree without root or a live sandbox, and it carries no build tag so those tests run everywhere. It fails only when memory.current is missing or unparseable -- wrapping fs.ErrNotExist in the first case, so the handler can tell "no cgroup to measure" from "the format is not what we parse". Every other file degrades to zero on its own field, because each has a legitimate reason to be absent on a healthy node: memory.peak predates kernel 5.19, and setupCgroupDelegation enables controllers best-effort, so a cgroup with memory but no cpu is reachable. Reporting no memory numbers because the node could not report CPU would be the wrong trade. Working set is memory.current less memory.stat's inactive_file, saturating at zero rather than wrapping: the two files are read a moment apart and are not a consistent snapshot, so inactive_file can legitimately exceed the memory.current read just before it. The proto's measurement block gains the epoch caveat it was missing. It already said cpu_usage_usec is per-epoch; memory_peak_bytes accumulates the same way and said nothing, so a caller reading it as a lifetime peak would silently under-report -- this source recreates the sandbox cgroup on every restore, and memory.peak restarts with it. The note is phrased per source rather than as one rule, because the two sources do not agree: the guest agent reads counters the guest kernel keeps in its own RAM, and a restored guest brings those back, so an epoch there does not end where it ends here. It also drops the suggestion that watching for a decrease is enough to spot a boundary. An epoch can begin above the value last reported, and then no decrease ever appears. The handler is the first user of the NOT_FOUND / FAILED_PRECONDITION split the previous commit documented, and follows it: available and a UID mismatch are both NOT_FOUND, since each tells the caller the actor is not here and its worker-to-actor mapping wants re-resolving. A missing cgroup under a matching UID is the transient FAILED_PRECONDITION -- usually a poll landing in the boot, since attribution is now retained from the moment the ateom accepts the actor, before runsc has created the leaf. A malformed cgroup is Internal: not a routine race, so it must not be reported as one. The handler takes no lock, which is what the atomic on activeActor bought and what TestGetWorkloadStatsDoesNotTakeLock pins: it holds s.lock across the call, so a handler that reached for it deadlocks. After the read it reloads activeActor and compares pointer identity, so a checkpoint plus a fresh run completing underneath the read is NOT_FOUND rather than misattributed -- the same answer a retry would get, rather than a second code for one state. Deliberately not here: a panic-recovery interceptor. grpc-go serves each RPC on its own goroutine and does not recover handler panics, and the Go runtime kills the process on an unrecovered panic in any goroutine, so a nil dereference in any handler today ends every other RPC the ateom is serving, including an in-flight checkpoint. That gap predates this change and is not made reachable by it: nothing calls GetWorkloadStats yet, and neither the handler (which nil-checks before every dereference) nor the parser (which length-checks before indexing) has a panic path. It wants its own PR, covering every server rather than the two ateoms a telemetry change happens to touch, and it needs to land before the phase that adds a caller polling on a timer. Not covered here: that the sentry lands in /sys/fs/cgroup/pause on a live node is a runsc placement behavior, derived from the spec convention above but not verified from a unit test. Process listings in agent-substrate#161 confirm runsc-sandbox and both gofers sit in the "pause" cgroup, but those runs predate agent-substrate#496, so they establish the leaf name rather than the absolute path. The leaf holds the sentry's own overhead and the gofers alongside the actor's work, which is what the proto means by measuring the SANDBOX; splitting the actor's share out would need the sentry's own accounting. Part of agent-substrate#594
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #288
x-ref #174
Also, by not running with
privileged: truewe should stop mounting all host devices, so we can start thinking about passing through specific devices. cc Davanum Srinivas (@dims) Omer Yahud (@omeryahud)We do this by:
privileged: true, which also ensures we're in a private cgroupns (we assume cgroups v2 container ecosystem which does this, cgroup v1 is broadly deprecated).To be clear: ateom-gvisor is still highly permissioned, it's just not literally
privileged: trueanymore, so we drop unwanted side-effects from that.This PR was AI assisted. I have tested everything on kind + GKE locally and reviewed the changes.