ateom: add GetWorkloadStats RPC and retain actor attribution - #667
Conversation
60c4024 to
59407c2
Compare
e591f8b to
3e687b3
Compare
3e687b3 to
be84808
Compare
|
LGTM for the o11y side but please also get a LGTM from Benjamin Elder (@BenTheElder) on the ateom side |
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
claude raises a good question:
| } | ||
|
|
||
| ra := &runningActor{chCmd: chCmd, vfsdCmd: vfsdCmd, durableVfsdCmd: durableVfsdCmd, apiSocket: apiSocket, baseID: actorUID, logAgent: ac} | ||
| ra := &runningActor{chCmd: chCmd, vfsdCmd: vfsdCmd, durableVfsdCmd: durableVfsdCmd, apiSocket: apiSocket, baseID: actorUID, logAgent: ac, activeActor: p.actorAttribution()} |
There was a problem hiding this comment.
🤖 question 🟢 – The two runtimes disagree about when a booting actor becomes attributable. Attribution attaches here, on the runningActor built after readyz, so nothing is registered until the boot succeeds. The gVisor side sets s.activeActor before its boot and says why: "so a sample taken against a workload that dies mid-boot is still attributable" (cmd/ateom-gvisor/main.go:287).
Against the proto's state machine that difference is visible: a poll during a micro-VM boot finds no runningActor and gets FAILED_PRECONDITION, i.e. "the ateom is available (nothing to measure)", when it is really mid-activation. The RPC is documented as "safe to call on a timer for the whole lifetime of a workload", so a caller polling across a resume would see that blip on micro-VM and not on gVisor.
Nothing observable yet, since both return Unimplemented — but this PR is the foundation the measurement half builds on, and the semantics are cheaper to settle now. Is the gVisor rationale meant to apply to both, or is post-boot-only the intended contract?
There was a problem hiding this comment.
Both should attach before the boot — the gVisor reason applies to micro-VM too. Post-boot-only wasn't a decision. Fixed in 9bc7fb6e.
It followed from where the field lived. gVisor has a slot on AteomService whose only job is attribution, so RunWorkload sets it before the boot. Micro-VM put attribution on runningActor, which is assembled from the boot's outputs (chCmd, vfsdCmd, apiSocket, logAgent) and so isn't constructed until just after readyz.WaitAll. The gVisor field's comment already claimed micro-VM "holds the same field", so the code asserted a symmetry it didn't have.
Nothing is observable while both handlers return Unimplemented. It bites once measurement lands: an actor that dies mid-boot or loops never reaches readyz, and that's the actor whose usage you'd most want.
Change. Micro-VM gains AteomService.activeActor, set before the boot in RunWorkload and RestoreWorkload and cleared by a deferred check on the error paths — the same points as gVisor. runningActor.activeActor is gone, so there's one source of truth. A single slot because an ateom serves one actor at a time; running is keyed by UID for lookup, not concurrency.
The alternative I rejected was inserting a half-built runningActor into running before the boot and filling it in after. It wouldn't break anything today — teardownActor and snapshotVMState nil-guard every field, because ra can already be nil after an ateom restart — and that's the problem. ra != nil currently means "we own this actor's processes"; a half-built entry takes that branch and silently no-ops. The guards that save it are per-field and incidental, not enforced. It's also only unreachable because s.lock spans both RPCs, and shrinking that hold (the readiness wait) is a follow-up I've already flagged.
Checkpoint clears at the teardown, not the snapshot — the one place the runtimes should differ. runsc's checkpoint takes the sandbox down, so gVisor clears as soon as it returns. The micro-VM guest is only paused until teardownActor, so a checkpoint that failed earlier has left it present, and reporting its usage is the honest answer. Clearing next to the delete from running keeps the two views of "is an actor here" in agreement.
Proto. The contract sentence equated FAILED_PRECONDITION with "available". There's now a third state: executing and attributable, but no sandbox to read yet. Same code on purpose — a timer-driven caller skips the sample either way — so what needed saying is that it means "no numbers right now", not "the actor is gone".
Neither field is atomic yet; each becomes an atomic.Pointer when its runtime's measurement half lands. Keeping the retention points identical now is what makes both mechanical.
Adds ateom.Ateom/GetWorkloadStats, the RPC atelet will poll for per-actor
resource usage, plus the attribution retention both ateom runtimes need to
label a sample. The measurement half of each runtime lands in the two
follow-ups; GetWorkloadStats returns Unimplemented until then, so this change
puts nothing half-populated on the wire.
GetWorkloadStats is a pure read: unlike Run/Checkpoint/Restore it does not
move the ateom between "available" and "executing", so it is safe to call on
a timer for a workload's whole lifetime. The request carries the actor UID
the caller believes is executing here, so a recycled worker is rejected with
FAILED_PRECONDITION rather than reporting a different actor's numbers under
the requested actor's name.
sandbox_class and source are enums (SandboxClass, StatsSource) rather than
strings: both are closed sets the ateom binary picks from, and a typo in a
free-form string would silently split a metric in two downstream.
Neither runtime kept the actor's identifying fields past the call that
started the workload -- they arrive on Run/Restore and nothing downstream
needed them. A usage sample is only useful once attributed, so both now hold
them for as long as they are executing:
* ateom-gvisor gains AteomService.activeActor, set by RunWorkload and
RestoreWorkload and cleared by CheckpointWorkload and by both boot-failure
paths, tracking exactly the available/executing state machine.
* ateom-microvm gains runningActor.activeActor, populated from the existing
actorBootParams on both the cold-boot and restore paths; the existing
delete from s.running in teardownActor clears it.
The extraction from the request is shared in internal/ateomstats since both
binaries need it. The type is ActorAttribution, not ActorIdentity: "actor
identity" already means a credential in this repo (ateapi's ActorIdentity
service, substratex509, ateompath.ActorIdentityDirPath), and nothing here is
a secret or is presented as proof of anything.
Part of agent-substrate#594
Review catch on the previous commit: the two runtimes disagreed about when a booting actor becomes attributable. ateom-gvisor sets AteomService.activeActor before the boot, so a workload that dies mid-boot is still attributable; ateom-microvm only built its runningActor after readyz, so nothing was retained until the boot succeeded. That was not a decision about the contract, it followed from where the field lived. runningActor also holds chCmd, vfsdCmd, apiSocket, and logAgent -- none of which exist until the guest is up -- so it cannot be constructed before the boot. The attribution had been put on the one struct that structurally could not hold it early. The gVisor field's own comment already claimed the two ateoms worked the same way, so the code asserted a symmetry it did not have. Nothing was observable yet, since both handlers return Unimplemented. It shows up once the measurement halves land: an actor that never reaches readyz is one whose usage is most worth having, and micro-VM would have been the runtime that reported nothing for it. ateom-microvm gains AteomService.activeActor, set at the top of RunWorkload and RestoreWorkload and cleared by a deferred check on the error paths, matching the gVisor ateom point for point. runningActor.activeActor goes away rather than leaving two sources of truth. The service holds a single slot because an ateom serves one actor at a time; running is keyed by UID for lookup, not because several can be live at once. Deliberately not done by publishing a half-built runningActor into running early. That is the smaller diff, but CheckpointWorkload reads that map and would find an entry with a nil chCmd and logAgent. The lock makes it unreachable today, and that is the problem: it turns "an entry means a live VM" into a rule with an exception that every teardown path then has to know about. CheckpointWorkload clears at the teardown rather than at the snapshot, which is where the two runtimes legitimately differ. runsc's checkpoint takes the sandbox down, so gVisor clears as soon as it returns; the micro-VM guest is only paused until teardownActor, so a checkpoint that failed before that point has left it present and reporting its usage is the honest answer. Clearing alongside the delete from running keeps the two views of "is an actor here" from disagreeing. The proto sentence equated FAILED_PRECONDITION with the ateom being available. With attribution attached at activation intent there is a third state it has to cover: executing and attributable, but with no sandbox to read yet. Both are the same code on purpose -- a caller polling on a timer skips the sample either way -- so what needed writing down is that it means "no numbers right now" and not "the actor is gone". Neither field is atomic yet. The gVisor one becomes an atomic.Pointer when its cgroup read lands and the micro-VM one when its guest-agent read does; keeping the retention points identical now is what makes each of those a mechanical change. Part of agent-substrate#594
… "not yet" Two review catches on the attribution retention, both about the contract the measurement halves will inherit rather than about anything observable today. Hold the attribution in an atomic.Pointer instead of under lock. Both ateoms take lock on the first line of RunWorkload, RestoreWorkload, and CheckpointWorkload and hold it for the whole handler -- across a boot, a cold boot's retry, a snapshot write, a restore. A GetWorkloadStats that reads activeActor under that same mutex would block for the full duration of each, so a caller polling on a timer would go quiet during exactly the phases whose usage is most interesting. The proto's "safe to call on a timer" was about not moving the state machine; not blocking on it is a separate property and this is what buys it. Writers keep holding lock, since they have other state to protect and the point is the reader. The type only makes a lock-free read possible. Both field comments now say the rest out loud -- GetWorkloadStats must not take lock at all -- because that is the part a future handler can quietly get wrong. Each runtime's measurement PR adds the regression test that pins it, which needs a handler with a body. Second, FAILED_PRECONDITION was carrying two meanings that want different things from the caller. "This ateom is executing your actor but has no numbers yet" is transient: skip the sample, take the next one. "This ateom is not executing your actor" is not: the caller's worker-to-actor mapping is stale and retrying on the same timer just keeps asking a worker that has moved on. The second is now NOT_FOUND, and it covers the available case as well as the recycled-onto-a-different-actor case, since both mean the same thing to the caller -- the split is by what the caller should do, not by what the ateom's state machine calls itself. NOT_FOUND rather than ABORTED for that case: ABORTED invites retrying the same request, and after a recycle that request never succeeds. This split is only expressible because of the previous commit. While micro-VM attached attribution after readyz, "available" meant both "no actor here" and "actor booting", and no pair of codes could separate them. Free to change: both handlers still return Unimplemented, so nothing reads either code yet. Part of agent-substrate#594
9376fc9 to
39ba84b
Compare
b296f96
into
agent-substrate:main
Second of the three PRs for Phase 0 of #550. #667 has merged, so this is now rebased onto `main` and stands alone as a single commit. 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 in PR 3, which closes #594. ## Where the numbers come from `/sys/fs/cgroup/pause`, the sandbox's cgroup v2 leaf. 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. That is `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. This is why a sample is attributed to the actor and not to a container, as the proto already says. 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 worker runs in a private cgroup namespace, so `/sys/fs/cgroup` is the pod's cgroup, not the host root). ## The read 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 unlike the rest of `cmd/ateom-gvisor` those tests run on every platform. 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 seemed like the wrong trade — pushback welcome if you'd rather it be all-or-nothing. Working set is `memory.current − memory.stat: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; on `uint64` the naive subtraction gives ~1.8e19 instead of ~0. ## Proto: the epoch caveat (added after review) The measurement block gains a note 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 after every restore — this source recreates the sandbox cgroup, and `memory.peak` restarts with it. The note is phrased per source rather than as one rule, because the two sources do not agree. `STATS_SOURCE_GUEST_AGENT` reads counters the guest kernel keeps in its own RAM, and `restoreFullScope` brings those back from the memory snapshot, so an epoch there does not end where it ends here. It also drops the old suggestion that watching for a decrease is enough to spot a boundary. An epoch can begin *above* the value last reported — `DATA_ON_GOLDEN` resumes the template's golden guest — and then no decrease ever appears. Comment-only, plus the regenerated `ateom.pb.go`. ## Status codes This is the first user of the `NOT_FOUND` / `FAILED_PRECONDITION` split #667 documented, and the split is by what the caller has to do about it. **`NOT_FOUND`** — the ateom is not executing the requested actor: it is available, or it was recycled onto a different one. Retrying on the same timer will never change that; the caller's worker-to-actor mapping wants re-resolving. **`FAILED_PRECONDITION`** — the requested actor *is* the one here, but there is nothing to read yet. Since #667 the attribution is retained from the moment the ateom accepts the actor, so a poll landing in the boot arrives before runsc has created the leaf. Transient: skip the sample and take the next one. **`Internal`** — the cgroup is there and does not parse. Not a routine race, so it must not be reported as one. The post-read pointer recheck returns `NOT_FOUND` for the same reason the first two do: it catches exactly the state a retry would land on, and one condition should not report two different codes depending on where in the handler it was noticed. ## Locking `AteomService.activeActor` becomes an `atomic.Pointer`. The three lifecycle RPCs still hold `AteomService.lock` for their whole bodies and keep doing so — the change is for the reader. `lock` is not what would make the read slow; it is the only thing that would make a plain field read *legal* while a writer is running. And it is held across entire boots, restores, and checkpoints (including the readiness wait), so "take the lock to read one pointer" becomes "wait out the boot", with pollers piling up behind it. `atomic.Pointer` makes the read safe on its own, so the reader never queues. The field is only ever assigned or cleared as a whole pointer, never mutated in place, which is what the type is for. The type makes a lock-free read *possible*; it does not make one happen. `TestGetWorkloadStatsDoesNotTakeLock` pins it: it holds `s.lock` across the call, so a handler that ever reaches for the lock deadlocks. The handler reloads the pointer after the read and compares identity, so a checkpoint plus a fresh run completing underneath the read is reported rather than misattributed to the wrong actor. ## Deliberately not here: panic recovery 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. `grep -rn "recover()"` finds nothing in our non-test code, so today a nil dereference in any handler ends every other RPC the ateom is serving — including an in-flight checkpoint. An earlier revision of this PR fixed that for the two ateoms. I have pulled it back out. The 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 deserves its own PR covering every server rather than the two ateoms a telemetry change happens to touch, and the real constraint is that it lands before the phase that adds a caller polling on a timer — not before this one. ## Follow-up worth filing separately The readiness wait is held under `s.lock` in all four boot paths (`main.go:364`, `:639`, and the micro-VM equivalents). It is not a runsc subcommand, so it sits outside what the lock's own comment claims to protect, and it is the longest hold in the file. Shrinking it changes lifecycle behavior, so it does not belong in a telemetry PR — but it is the highest-leverage locking fix here. ## Testing - `cgroupstats`: nine-case table over fixture trees — all files present, missing `memory.peak`, missing `cpu.stat`, missing `memory.stat`, `memory.stat` without `inactive_file`, `inactive_file` above `memory.current`, all-zero, malformed/blank/over-long lines, unparseable optional files — plus missing-cgroup and malformed-`memory.current` cases that pin which one matches `fs.ErrNotExist`. - `GetWorkloadStats`: happy path against a fixture cgroup root, a five-case error table (empty `actor_uid` → `InvalidArgument`; available and UID mismatch → `NotFound`; vanished cgroup → `FailedPrecondition`; malformed cgroup → `Internal`), and the no-lock regression test. - All nine local verifiers pass, `shellcheck.sh` and `proto-fmt.sh` included. No shell scripts changed. The `cmd/ateom-gvisor` handler tests are `//go:build linux`, and they were run rather than only compile-checked: `go test ./...` is green for the whole repo inside a `golang:1.26.3` container (linux/arm64), and the `GetWorkloadStats` tests pass under `-race`. `cgroupstats.Read` was also pointed at that container's live cgroup v2 tree as a one-off check that the parser agrees with a real kernel and not just with its own fixtures: all four files present, and the working set matching `memory.current` less `inactive_file`. The one thing a container cannot cover — that the sentry lands in `/sys/fs/cgroup/pause` — was measured on a live worker. The path is derived from `ensureContainerCgroupsPath`, but the placement is a runsc behavior, so it needed a real node. This branch's own build was deployed to a GKE cluster, an actor was run on it from the counter ActorTemplate, and the worker's processes were read through an ephemeral container sharing the ateom's PID namespace: ``` 1 ateom-gvisor 0::/…-<sandbox>.scope/ateom 28 runsc-sandbox … boot … pause 0::/…-<sandbox>.scope/pause 27 runsc-gofer --bundle=…/bundles/pause 0::/…-<sandbox>.scope/pause 119 runsc-gofer --bundle=…/bundles/counter 0::/…-<sandbox>.scope/pause ``` Both halves of the assumption hold. The sentry is in the `pause` leaf, and the gofer for the *counter* container is in that same leaf rather than one of its own — which is the one-cgroup-per-sandbox property the handler relies on to report a whole actor from a single read. `setupCgroupDelegation`'s private namespace is visible too: ateom's own processes sit in the sibling `ateom` leaf, so the sandbox's usage is not mixed with the ateom's. The kubelet's cadvisor endpoint reads the same leaf independently, and reports the four values this PR parses: `memory.current` 39,854,080, peak 41,914,368, working set 38,596,608, cpu 0.238517 s. The working set is consistent with the parser's definition — 39,854,080 less 38,596,608 leaves 1,257,472 of `inactive_file`. Not covered: `GetWorkloadStats` answering over the wire. The ateom serves its control API on a unix socket inside a distroless image, so a live RPC needs a client mounted alongside it — which is the phase that adds the caller, not this one.
Phase 0 of #550, tracked by #594. First of three stacked PRs.
Adds
ateom.Ateom/GetWorkloadStats, the RPC atelet will poll for per-actor resourceusage, plus the actor attribution both ateom runtimes need to label a sample. The measurement half of each runtime lands in the two follow-ups;
GetWorkloadStatsreturnsUnimplementeduntil then, so this change puts nothinghalf-populated on the wire.
The RPC
GetWorkloadStatsis a pure read: unlikeRunWorkload/CheckpointWorkload/RestoreWorkloadit does not move the ateom between "available" and "executing",so it is safe to call on a timer for a workload's whole lifetime.
The request carries the actor UID the caller believes is executing here. A worker
can be recycled between atelet's view of the world and the call landing, so a
mismatch is rejected with
FAILED_PRECONDITIONrather than reporting a differentactor's numbers under the requested actor's identity.
FAILED_PRECONDITIONisalso the answer when the ateom is available — there is nothing to measure.
The response is one sample, measured at sandbox granularity (which today
equals the actor; per-container attribution would need the gVisor sentry's own
accounting). It echoes the measured actor's identity so the caller can attribute
the sample without holding its own worker→actor mapping, and carries
sandbox_classandsourceso two differently-measured numbers are not silentlycompared as though they were the same thing. Both are enums — closed sets the
ateom binary picks from, where a typo in a free-form string would silently split
a metric in two downstream.
Attribution retention
Neither runtime kept the actor's identifying fields past the call that started the
workload — they arrive on Run/Restore and nothing downstream needed them. A usage
sample is only useful once attributed, so both now hold them for as long as they
are executing:
AteomService.activeActor, set byRunWorkloadandRestoreWorkload, cleared byCheckpointWorkloadand by both boot-failurepaths — tracking exactly the available/executing state machine on the service.
runningActor.activeActor, populated from the existingactorBootParamson both the cold-boot and restore paths; the existing deletefrom
s.runninginteardownActorclears it.The extraction from the request is shared in
internal/ateomstats, since bothbinaries need the same mapping.
ActorAttributioncomposes the existingresources.ActorRefrather than repeating its two fields, so the actor'sAtespacestays visibly attached to the actor — worth noting becauseTemplateNamespacebeside it is an unrelated namespace (a Kubernetes one;ActorTemplateis a namespaced CRD, while an atespace is Substrate's owntenancy unit).
It is deliberately not called
ActorIdentity: "actor identity" already meansa credential in this repo — the
ateapi.ActorIdentityservice,substratex509,ateompath.ActorIdentityDirPath, and #670 building on all three. Nothing in thistype is a secret or is presented as proof of anything; it is the tuple a usage
sample is labeled with.
Reviewer notes
activeActorhas no reader in this PR. It is written and cleared but neverread, because both
GetWorkloadStatsbodies are still stubs. The readers arrivewith the measurement in the two follow-ups; the stub comments point there.
identity, but that is a sample, not a datapoint. Per the decisions in Observability At-Scale #174,
actor/atespace identity belongs in logs and traces rather than as TSDB labels,
and only template-level dimensions become metric labels. That conversion is
atelet's, in Phase 1/2.
cpu_usage_usecresets on restore. A restore recreates the sandbox, so thecounter restarts at zero while the actor UID stays the same. Documented on the
field; callers computing deltas must treat a decrease as a reset. If that proves
too subtle in practice, an explicit epoch marker is a backward-compatible
addition later.
workerpool_nameis deliberately absent. atelet does not know it today;adding the field later is backward compatible.
Testing
internal/ateomstats—TestActorAttributionFromRequestpins the mapping fromboth request types, including the empty and nil cases (the callers are not
defensive about the request pointer, so the nil-safety of the generated getters
is load-bearing). Five deliberately distinct placeholder values, so a field
wired to the wrong source is visible.
cmd/ateom-microvm—TestActorBootParamsAttributionpins theactorBootParams→ActorAttributionmapping, andTestActorBootParamsAttributionMatchesRequestchecks that the two hops (request→ boot params → attribution, written in different files) compose back into what
the caller sent.
TestGetWorkloadStatsUnimplementedpins the stub's advertisedcontract, and
TestAteomServiceStartsAvailablechecks a fresh gVisor serviceretains no attribution (a non-nil zero value there would make an idle ateom
report an empty actor's usage instead of refusing).
RunWorkload,RestoreWorkloadandCheckpointWorkloadeach reach for netlink, runsc and theworker pod's netns within a few lines of entry, so there is no seam to drive
them from
go test. Noted incmd/ateom-gvisor/stats_test.gorather thancovered with a fake; the transitions get verified end to end once
GetWorkloadStatsreturns real data.Both ateom packages are
//go:build linux, so their tests do not execute ondarwin. They were run natively on a Linux host in addition to the local
compile-check, along with
hack/verify/shellcheck.sh(which needs docker).Still ahead
/sys/fs/cgroup/pause, notmain/: every application process runs inside the sentry, which is a singleprocess in
pause/.StatsContainerover the existingvsock connection, not the host cgroup (guest RAM is a fixed allocation, so the
host cgroup barely moves with the workload). Closes Phase 0: StatsWorkload RPC on ateom (proto, both runtime reads, identity retention) #594.