From df90625558d73e9cb0d3ec5aebef70886b14d0fe Mon Sep 17 00:00:00 2001 From: Tim Bai Date: Mon, 10 Aug 2026 11:45:31 -0400 Subject: [PATCH 1/3] ateom-microvm: measure the guest agent for GetWorkloadStats Fills in the measurement half of GetWorkloadStats for the micro-VM runtime, so it returns real numbers instead of Unimplemented. The gVisor runtime's half is an independent change; this one does not touch it. The sample comes from inside the guest, not from a host cgroup. The host cgroup here holds cloud-hypervisor, whose memory is the guest RAM allocation it took at boot: near-constant, and near-identical for an idle actor and a saturated one. The numbers that move with the workload are the ones the guest kernel keeps, and the kata-agent's StatsContainer is what can read them out. AgentClient grows that call; it is safe alongside the stdout/stderr forwarding, since ttrpc multiplexes the one connection, which is already what those goroutines rely on. What gets summed is the actor's overlay WORKLOADS, one per container. Their carriers are deliberately absent: a carrier is created and never started (see CreateCarrier), so it runs no process and its cgroup has nothing in it to add. Summing the containers is what turns per-container guest accounting into the one per-actor figure the proto reports. Summing the peaks is an upper bound on the peak of the sum rather than the figure itself -- two containers need not have peaked at the same moment -- and for the single-container actors this runtime mostly serves it is exact. The conversion lives in a new cmd/ateom-microvm/internal/agentstats. It is pure, taking an already-fetched CgroupStats and never talking to a guest, which keeps it testable without a live micro-VM and, unlike the rest of the micro-VM ateom, without the linux build tag. It never fails: every field the agent left out reads as zero, and nil stats -- what the agent answers for a container it has no accounting for -- is a zero sample rather than a panic on a path polled for the life of every workload. Working set subtracts the guest's reclaimable page cache, saturating at zero, and accepts both the v2 name (inactive_file) and the v1 hierarchical one (total_inactive_file). CPU time is divided by 1000: the agent reports nanoseconds, matching the runc stats struct its own is modeled on, and the proto wants microseconds. A container the agent cannot report contributes nothing instead of failing the sample, and for the common way that happens zero is the correct contribution rather than a fallback: a container that has exited took its guest cgroup with it and consumes nothing from here on. Failing an actor's telemetry because one sidecar is gone would be the wrong answer. The sample fails only when no container could be read at all, which is the guest as a whole not answering rather than one container being gone. Status codes follow the NOT_FOUND / FAILED_PRECONDITION split the proto documents, and the handler is a deliberate mirror of the gVisor one so the two runtimes answer a poller the same way. Available and a UID mismatch are both NOT_FOUND: each says the actor is not here, and the caller's worker-to-actor mapping wants re-resolving. No guest to ask yet is the transient FAILED_PRECONDITION -- a poll landing in the boot or the restore, since attribution is retained from the moment the ateom accepts the actor, or one landing mid-teardown. A guest that answers nothing at all is the same code and not Internal: the sandbox going away is a routine state here, and the next CheckpointWorkload turns it into the NOT_FOUND above. The handler takes no lock. That is a stronger constraint than on the gVisor side, where a cgroup path is a constant: the agent client lives in AteomService.running, which lock guards, so reading it from the handler would be a data race whatever the read is for. Hence AteomService.guestStats, an atomic holding the agent client and the container ids, published once the containers are up and cleared by teardownActor before it closes anything. Clearing there rather than alongside the attribution is what keeps a poll landing mid-teardown on the "no numbers right now" path instead of surfacing a closed connection as a failed read. TestGetWorkloadStatsDoesNotTakeLock pins the whole property: it holds s.lock across the call, so a handler that reached for it -- or that looked the agent up in running -- deadlocks. After the read the handler reloads activeActor and compares pointer identity, so a checkpoint plus a fresh run completing underneath it is NOT_FOUND rather than misattributed. One gap worth knowing about: a restore whose post-restore agent dial fails answers FAILED_PRECONDITION for the rest of that activation. Telemetry rides on the connection log forwarding already keeps open, and that dial is best-effort by design (a failed dial must not fail a restore whose actor is already running). A second dial of its own would not help -- whatever kept the agent from answering a 15s retry loop would keep it from answering that one too. The two accumulating fields, memory_peak_bytes and cpu_usage_usec, are scoped to an epoch that does not begin where the gVisor source's does. That is why the proto's epoch note, which lands with the gVisor change, is written per source rather than as one rule, and this is the source it is being careful about. There a restore ends the epoch: runsc delete destroys the sandbox cgroup and its counters with it. Here the counters live in the guest kernel's own memory, so restoreFullScope -- relaunch cloud-hypervisor with --restore, then resume -- brings them back with the guest RAM rather than restarting them, and a FULL restore continues the epoch across what the caller sees as a gap. DATA has no guest to resume and cold-boots, so that scope does restart at zero. DATA_ON_GOLDEN is the case that defeats the obvious detection: it resumes the template's golden guest, so an actor's first sample can begin at whatever the golden had accumulated when it was snapshotted -- an epoch beginning above the value last reported, with no decrease anywhere for a caller to notice. Nothing here can hide that. A caller wanting a lifetime figure has to accumulate one itself and treat a scope transition as a boundary rather than trusting the numbers to announce one. The field mapping is not covered by a test here, since it is a property of the guest kernel and image rather than of this code. It was instead measured against the pinned assets themselves -- the kata-static 4.0.0 kernel and rootfs that hack/microvm-assets/assemble.sh stages and ateom fetches. Which cgroup version the guest gives a container is not a question the guest can answer two ways. Its kernel is 6.18.35, built CONFIG_MEMCG=y with CONFIG_MEMCG_V1 unset, so a v1 memory controller cannot be mounted there at all; its init is systemd 255.4, which reports default-hierarchy=unified when run from that rootfs, and buildVMConfig passes no hierarchy override on the cmdline. Containers therefore get v2, and inactive_file is the spelling that appears. Whether a high-water mark is reported resolves the same way. The agent links cgroups-rs 0.5.1, whose v2 path reads memory.current into usage and memory.peak into max_usage, and passes memory.stat through verbatim. memory.peak has existed since 5.19, so on a 6.18 guest the peak is a real figure rather than the zero this defends against. The same source settles the unit conversion: v2 has no cpuacct controller, so the agent falls through to cpu.stat and multiplies usage_usec by 1000, which is the nanosecond reading the division here turns back into microseconds. The v1 spellings stay anyway. The guest image is a SandboxConfig asset, so a cluster can be pointed at a different one, and a wrong guess there should cost a low working-set figure rather than a failed sample. Still not covered: a live guest answering StatsContainer. That needs a micro-VM worker, which needs a nested-virtualization node. Part of #594 --- cmd/ateom-microvm/checkpoint.go | 13 +- .../internal/agentstats/agentstats.go | 155 ++++++++ .../internal/agentstats/agentstats_test.go | 190 ++++++++++ .../internal/kata/agentclient.go | 21 ++ cmd/ateom-microvm/main.go | 19 +- cmd/ateom-microvm/restore.go | 23 +- cmd/ateom-microvm/run.go | 29 +- cmd/ateom-microvm/stats.go | 185 +++++++++- cmd/ateom-microvm/stats_test.go | 330 ++++++++++++++++-- internal/proto/ateompb/ateom.pb.go | 14 +- internal/proto/ateompb/ateom.proto | 14 +- 11 files changed, 941 insertions(+), 52 deletions(-) create mode 100644 cmd/ateom-microvm/internal/agentstats/agentstats.go create mode 100644 cmd/ateom-microvm/internal/agentstats/agentstats_test.go diff --git a/cmd/ateom-microvm/checkpoint.go b/cmd/ateom-microvm/checkpoint.go index f55dfe2f9..8141ecc1f 100644 --- a/cmd/ateom-microvm/checkpoint.go +++ b/cmd/ateom-microvm/checkpoint.go @@ -257,6 +257,13 @@ func listFiles(dir string) ([]string, error) { // snapshot is already on disk, so this only needs to release resources. ra may be // nil (e.g. ateom restarted and lost in-memory state). func (s *AteomService) teardownActor(ctx context.Context, id string, ra *runningActor, client *ch.Client) { + // Stop offering the guest to GetWorkloadStats first, before anything below + // makes it stop answering. Clearing it here rather than alongside the + // attribution is what keeps a poll that lands mid-teardown on the + // FAILED_PRECONDITION path ("no numbers right now") instead of surfacing a + // closed connection as a failed read. + s.guestStats.Store(nil) + if client != nil { tShutdown := time.Now() shutCtx, cancel := context.WithTimeout(ctx, 10*time.Second) @@ -272,9 +279,9 @@ func (s *AteomService) teardownActor(ctx context.Context, id string, ra *running // fails the forwarding goroutines' in-flight ReadStdout/ReadStderr calls, so // they return io.EOF and exit (no goroutine leak). Guarded so a second // teardown / a never-forwarded actor is a no-op. - if ra.logAgent != nil { - _ = ra.logAgent.Close() - ra.logAgent = nil + if ra.guestAgent != nil { + _ = ra.guestAgent.Close() + ra.guestAgent = nil } // Kill the CH process ateom launched. diff --git a/cmd/ateom-microvm/internal/agentstats/agentstats.go b/cmd/ateom-microvm/internal/agentstats/agentstats.go new file mode 100644 index 000000000..abe853d7c --- /dev/null +++ b/cmd/ateom-microvm/internal/agentstats/agentstats.go @@ -0,0 +1,155 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package agentstats turns the kata guest agent's per-container cgroup +// accounting into the resource-usage sample ateom reports. +// +// The micro-VM ateom uses it to answer ateompb.Ateom/GetWorkloadStats. The host +// cgroup is the wrong place to look on this runtime: the guest's RAM is a fixed +// allocation cloud-hypervisor takes at boot, so the host cgroup reads roughly +// the same whether the actor is idle or saturated. The numbers that move with +// the workload are the ones the guest kernel keeps, and the agent is what can +// read them. +// +// This package is deliberately pure — it converts an already-fetched +// agentpb.CgroupStats and never talks to a guest — which keeps it testable +// without a live micro-VM and, unlike the rest of the micro-VM ateom, without +// the linux build tag. +package agentstats + +import ( + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" +) + +// Sample is a point-in-time reading for one container, or the sum of several. +// It carries the same four numbers as the gVisor ateom's cgroupstats.Sample, +// because both feed the same four fields of GetWorkloadStatsResponse. +// +// Anything the guest did not report reads as zero rather than failing the whole +// sample, for the same reason as there: a partial reading is more useful than +// none. FromCgroupStats says which fields can do that and why. +type Sample struct { + // MemoryCurrentBytes is what is currently charged to the container's guest + // cgroup, page cache included. + MemoryCurrentBytes uint64 + + // MemoryPeakBytes is the high-water mark of MemoryCurrentBytes. Zero when + // the guest kernel does not expose one: the agent fills it from the cgroup's + // max-usage file, which cgroup v2 only grew in Linux 5.19. + MemoryPeakBytes uint64 + + // MemoryWorkingSetBytes is MemoryCurrentBytes less the reclaimable page + // cache, floored at zero — the figure to compare against a memory limit, + // since MemoryCurrentBytes drifts upward with cache the kernel would drop + // for free under pressure. + MemoryWorkingSetBytes uint64 + + // CPUUsageUsec is cumulative CPU time consumed by the container, as seen by + // the guest kernel. + CPUUsageUsec uint64 +} + +// Keys of the memory.stat entry holding reclaimable file-backed pages. The +// agent passes the guest's memory.stat through verbatim, so which one is +// present depends on the cgroup version the guest kernel gave the container: +// v2 names it inactive_file, v1 has a per-cgroup inactive_file and the +// hierarchical total_inactive_file, and the total is the one that matches what +// v2's figure means. +const ( + inactiveFileV2 = "inactive_file" + inactiveFileV1 = "total_inactive_file" +) + +// FromCgroupStats converts one container's guest cgroup accounting. +// +// It never fails. Every field the agent left out reads as zero, and cs itself +// may be nil — the agent answers without cgroup stats for a container it has no +// accounting for, which is a normal state for one that has exited rather than +// an error. The caller decides what an all-zero container means; see the +// summing in the micro-VM ateom's GetWorkloadStats. +func FromCgroupStats(cs *agentpb.CgroupStats) Sample { + mem := cs.GetMemoryStats().GetUsage() + current := mem.GetUsage() + + // Saturating rather than wrapping. The guest reads usage and memory.stat a + // moment apart, so the reclaimable figure can legitimately exceed the usage + // read beside it; on uint64 the naive subtraction gives an absurd number + // instead of the near-zero the reading means. + workingSet := current + if inactiveFile, ok := inactiveFileBytes(cs); ok { + workingSet = 0 + if inactiveFile < current { + workingSet = current - inactiveFile + } + } + + return Sample{ + MemoryCurrentBytes: current, + MemoryPeakBytes: mem.GetMaxUsage(), + + MemoryWorkingSetBytes: workingSet, + + // The agent reports CPU time in nanoseconds, matching the runc stats + // struct its own is modeled on; the proto wants microseconds. Truncating + // division loses at most a microsecond per sample, and the field is + // cumulative, so the error does not accumulate across samples. + CPUUsageUsec: cs.GetCpuStats().GetCpuUsage().GetTotalUsage() / 1000, + } +} + +// inactiveFileBytes returns the reclaimable page cache the guest reported, and +// whether it reported one at all. Absent, the working set collapses to usage, +// which over-reports by however much reclaimable cache the container holds — +// the safe direction, since it never claims the workload is using less than it +// is. +func inactiveFileBytes(cs *agentpb.CgroupStats) (uint64, bool) { + stats := cs.GetMemoryStats().GetStats() + for _, key := range []string{inactiveFileV2, inactiveFileV1} { + if v, ok := stats[key]; ok { + return v, true + } + } + return 0, false +} + +// Plus returns the sum of two samples, for accumulating an actor's containers +// into the one figure the proto reports. +// +// Summing the peaks is an upper bound on the peak of the sum, not the peak of +// the sum itself: two containers that peaked at different moments add up to a +// total the actor never actually reached. Reporting the true figure would need +// the guest to track the actor as a unit, which it does not — each container +// gets its own cgroup (see StartOverlayWorkload). The bound is the honest +// approximation, and for the single-container actors this runtime mostly serves +// it is exact. +// +// Saturating on overflow, so a nonsensical reading from one container cannot +// wrap the total to a small number and read as healthy. +func (s Sample) Plus(o Sample) Sample { + return Sample{ + MemoryCurrentBytes: addSaturating(s.MemoryCurrentBytes, o.MemoryCurrentBytes), + MemoryPeakBytes: addSaturating(s.MemoryPeakBytes, o.MemoryPeakBytes), + MemoryWorkingSetBytes: addSaturating(s.MemoryWorkingSetBytes, o.MemoryWorkingSetBytes), + CPUUsageUsec: addSaturating(s.CPUUsageUsec, o.CPUUsageUsec), + } +} + +// addSaturating returns a+b, or the maximum uint64 if that would wrap. +func addSaturating(a, b uint64) uint64 { + if sum := a + b; sum >= a { + return sum + } + const maxUint64 = ^uint64(0) + return maxUint64 +} diff --git a/cmd/ateom-microvm/internal/agentstats/agentstats_test.go b/cmd/ateom-microvm/internal/agentstats/agentstats_test.go new file mode 100644 index 000000000..6384763d8 --- /dev/null +++ b/cmd/ateom-microvm/internal/agentstats/agentstats_test.go @@ -0,0 +1,190 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package agentstats + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" +) + +// cgroupStats builds an agent reading: usage/max bytes, the memory.stat entries +// the guest reported, and cumulative CPU nanoseconds. +func cgroupStats(usage, maxUsage uint64, stats map[string]uint64, cpuNanos uint64) *agentpb.CgroupStats { + return &agentpb.CgroupStats{ + MemoryStats: &agentpb.MemoryStats{ + Usage: &agentpb.MemoryData{Usage: usage, MaxUsage: maxUsage}, + Stats: stats, + }, + CpuStats: &agentpb.CpuStats{ + CpuUsage: &agentpb.CpuUsage{TotalUsage: cpuNanos}, + }, + } +} + +func TestFromCgroupStats(t *testing.T) { + for _, tc := range []struct { + name string + cs *agentpb.CgroupStats + want Sample + }{ + { + name: "cgroup v2 guest", + cs: cgroupStats(157286400, 209715200, map[string]uint64{"inactive_file": 20971520}, 1234567000), + want: Sample{ + MemoryCurrentBytes: 157286400, + MemoryPeakBytes: 209715200, + MemoryWorkingSetBytes: 136314880, + CPUUsageUsec: 1234567, + }, + }, + { + // A v1 guest names the hierarchical figure differently. Dropping to + // the working-set-equals-usage fallback here would over-report every + // sample from such a guest rather than fail visibly, so it is worth + // pinning that the alternate key is understood. + name: "cgroup v1 guest reports total_inactive_file", + cs: cgroupStats(1000, 2000, map[string]uint64{"total_inactive_file": 400}, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryPeakBytes: 2000, MemoryWorkingSetBytes: 600}, + }, + { + // v1 reports both: a per-cgroup figure and the hierarchical total. + // The total is the one that means what v2's inactive_file means. + name: "both keys present prefers the v2 name", + cs: cgroupStats(1000, 0, map[string]uint64{"inactive_file": 100, "total_inactive_file": 400}, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryWorkingSetBytes: 900}, + }, + { + // The two figures are not a consistent snapshot, so this is + // reachable; on uint64 the naive subtraction would report ~1.8e19. + name: "reclaimable cache above usage floors at zero", + cs: cgroupStats(1000, 0, map[string]uint64{"inactive_file": 4000}, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryWorkingSetBytes: 0}, + }, + { + name: "equal usage and reclaimable cache floors at zero", + cs: cgroupStats(1000, 0, map[string]uint64{"inactive_file": 1000}, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryWorkingSetBytes: 0}, + }, + { + // Nothing to subtract, so the working set collapses to usage: an + // over-report, which is the safe direction. + name: "no memory.stat entries", + cs: cgroupStats(1000, 2000, nil, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryPeakBytes: 2000, MemoryWorkingSetBytes: 1000}, + }, + { + name: "memory.stat without a reclaimable-cache entry", + cs: cgroupStats(1000, 0, map[string]uint64{"anon": 900}, 0), + want: Sample{MemoryCurrentBytes: 1000, MemoryWorkingSetBytes: 1000}, + }, + { + // Guests below Linux 5.19 have no cgroup v2 high-water mark. The rest + // of the sample must survive that. + name: "no peak reported", + cs: cgroupStats(1000, 0, map[string]uint64{"inactive_file": 100}, 5000), + want: Sample{MemoryCurrentBytes: 1000, MemoryWorkingSetBytes: 900, CPUUsageUsec: 5}, + }, + { + // The agent reports nanoseconds and the proto wants microseconds. + // A sub-microsecond total truncates to zero rather than rounding up. + name: "sub-microsecond cpu time truncates", + cs: cgroupStats(0, 0, nil, 999), + want: Sample{}, + }, + { + name: "cpu time rounds down to whole microseconds", + cs: cgroupStats(0, 0, nil, 1999), + want: Sample{CPUUsageUsec: 1}, + }, + { + name: "memory reported without cpu", + cs: &agentpb.CgroupStats{MemoryStats: &agentpb.MemoryStats{Usage: &agentpb.MemoryData{Usage: 4096}}}, + want: Sample{MemoryCurrentBytes: 4096, MemoryWorkingSetBytes: 4096}, + }, + { + name: "cpu reported without memory", + cs: &agentpb.CgroupStats{CpuStats: &agentpb.CpuStats{CpuUsage: &agentpb.CpuUsage{TotalUsage: 2000}}}, + want: Sample{CPUUsageUsec: 2}, + }, + { + // What the agent answers for a container it has no accounting for — + // one that has exited, most often. Must be a zero sample, not a + // panic: this is parsed on a timer for the life of every workload. + name: "nil cgroup stats", + cs: nil, + want: Sample{}, + }, + { + name: "empty cgroup stats", + cs: &agentpb.CgroupStats{}, + want: Sample{}, + }, + { + // The reclaimable figure is there but the usage it subtracts from is + // not, so the working set floors rather than wrapping. + name: "reclaimable cache without a usage message", + cs: &agentpb.CgroupStats{MemoryStats: &agentpb.MemoryStats{Stats: map[string]uint64{"inactive_file": 100}}}, + want: Sample{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if diff := cmp.Diff(tc.want, FromCgroupStats(tc.cs)); diff != "" { + t.Errorf("FromCgroupStats() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestSamplePlus(t *testing.T) { + maxUint64 := ^uint64(0) + + for _, tc := range []struct { + name string + a, b Sample + want Sample + }{ + { + name: "adds every field", + a: Sample{MemoryCurrentBytes: 100, MemoryPeakBytes: 200, MemoryWorkingSetBytes: 90, CPUUsageUsec: 10}, + b: Sample{MemoryCurrentBytes: 1, MemoryPeakBytes: 2, MemoryWorkingSetBytes: 3, CPUUsageUsec: 4}, + want: Sample{MemoryCurrentBytes: 101, MemoryPeakBytes: 202, MemoryWorkingSetBytes: 93, CPUUsageUsec: 14}, + }, + { + // The accumulator starts here, so a zero left operand must be the + // identity or every actor's first container would be dropped. + name: "zero is the identity", + a: Sample{}, + b: Sample{MemoryCurrentBytes: 7, MemoryPeakBytes: 8, MemoryWorkingSetBytes: 9, CPUUsageUsec: 10}, + want: Sample{MemoryCurrentBytes: 7, MemoryPeakBytes: 8, MemoryWorkingSetBytes: 9, CPUUsageUsec: 10}, + }, + { + // A wrapped total would read as a nearly idle actor, which is the one + // wrong answer that looks plausible. + name: "saturates instead of wrapping", + a: Sample{MemoryCurrentBytes: maxUint64, MemoryPeakBytes: maxUint64, MemoryWorkingSetBytes: maxUint64, CPUUsageUsec: maxUint64}, + b: Sample{MemoryCurrentBytes: 1, MemoryPeakBytes: 2, MemoryWorkingSetBytes: 3, CPUUsageUsec: 4}, + want: Sample{MemoryCurrentBytes: maxUint64, MemoryPeakBytes: maxUint64, MemoryWorkingSetBytes: maxUint64, CPUUsageUsec: maxUint64}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if diff := cmp.Diff(tc.want, tc.a.Plus(tc.b)); diff != "" { + t.Errorf("Sample.Plus() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/cmd/ateom-microvm/internal/kata/agentclient.go b/cmd/ateom-microvm/internal/kata/agentclient.go index f7a242694..a365c8b6f 100644 --- a/cmd/ateom-microvm/internal/kata/agentclient.go +++ b/cmd/ateom-microvm/internal/kata/agentclient.go @@ -230,6 +230,27 @@ func (a *AgentClient) ReadStderr(ctx context.Context, containerID, execID string return resp.GetData(), nil } +// StatsContainer returns the guest cgroup accounting for one container, as the +// kata-agent reads it from inside the guest. Mirrors +// grpc.AgentService/StatsContainer. +// +// It returns only the cgroup half of the response; the network counters +// alongside it are per-guest-interface rather than per-container and are not +// what ateom reports. A nil return with a nil error means the agent answered +// without cgroup stats, which callers should read as "no numbers", not zero. +// +// Safe to call while the stdout/stderr forwarding goroutines are reading over +// the same client: ttrpc multiplexes concurrent calls over the one connection, +// which is already what those goroutines rely on. +func (a *AgentClient) StatsContainer(ctx context.Context, containerID string) (*agentpb.CgroupStats, error) { + resp := &agentpb.StatsContainerResponse{} + req := &agentpb.StatsContainerRequest{ContainerId: containerID} + if err := a.client.Call(ctx, "grpc.AgentService", "StatsContainer", req, resp); err != nil { + return nil, fmt.Errorf("agent StatsContainer %q: %w", containerID, err) + } + return resp.GetCgroupStats(), nil +} + // StreamReader adapts the agent's repeated ReadStdout/ReadStderr unary calls into // an io.Reader, so the consumer can pump the container's output through the shared // actorlog forwarder like any other stream. Each Read issues one RPC with Len set diff --git a/cmd/ateom-microvm/main.go b/cmd/ateom-microvm/main.go index b46d750be..ccedb0c63 100644 --- a/cmd/ateom-microvm/main.go +++ b/cmd/ateom-microvm/main.go @@ -303,7 +303,7 @@ type AteomService struct { // // Kept here rather than on runningActor, even though that struct already // exists per actor: runningActor holds processes that do not exist until the - // guest is up (chCmd, vfsdCmd, logAgent), so it cannot be built before the + // guest is up (chCmd, vfsdCmd, guestAgent), so it cannot be built before the // boot, and an entry in running is what tells CheckpointWorkload a live VM is // there. Attribution has to outlive both of those constraints — it is needed // from the moment the ateom accepts the actor, including for a boot that @@ -317,6 +317,23 @@ type AteomService struct { // reader. As there, the type makes a lock-free read possible without making // one happen — GetWorkloadStats must not take lock at all. activeActor atomic.Pointer[ateomstats.ActorAttribution] + + // guestStats is what GetWorkloadStats measures with: the kata-agent client + // and the guest containers to sum. Nil whenever there is no guest to ask — + // before the containers are up, after teardownActor, and for the rest of an + // activation whose post-restore agent dial failed. + // + // Separate from activeActor because the two become true at different points: + // the attribution is retained from the moment the ateom accepts the actor, + // deliberately including a boot that never finishes, while this can only + // exist once the guest is answering. Non-nil here implies activeActor is + // set, never the reverse. + // + // Atomic for the same reason as activeActor, and it is the other half of the + // same rule: GetWorkloadStats must not take lock, so it cannot reach into + // running for the agent client the way a lifecycle RPC does. Written under + // lock like every other transition; the atomic is for the reader. + guestStats atomic.Pointer[guestStatsTarget] } var _ ateompb.AteomServer = (*AteomService)(nil) diff --git a/cmd/ateom-microvm/restore.go b/cmd/ateom-microvm/restore.go index 93d6518a9..1136412d3 100644 --- a/cmd/ateom-microvm/restore.go +++ b/cmd/ateom-microvm/restore.go @@ -307,14 +307,14 @@ func (s *AteomService) restoreFullScope(ctx context.Context, p actorBootParams, // _ovl (same as the cold run). Best-effort — a failed dial must not fail the // restore (the actor is already running); forwarding is just skipped. vsockPath := kata.VsockSocketPath(actorUID) - logAC, dialErr := dialAgentRetry(ctx, vsockPath, 15*time.Second) + guestAC, dialErr := dialAgentRetry(ctx, vsockPath, 15*time.Second) if dialErr != nil { - slog.WarnContext(ctx, "post-restore agent dial failed; actor log forwarding disabled for this restore", + slog.WarnContext(ctx, "post-restore agent dial failed; actor log forwarding and guest stats disabled for this restore", slog.String("id", actorUID), slog.Any("err", dialErr)) } else { - ra.logAgent = logAC + ra.guestAgent = guestAC for _, c := range containers { - s.startActorLogForwarding(logAC, p.actorRef, actorUID, templateNS, templateName, overlayWorkloadID(c.GetName()), c.GetName()) + s.startActorLogForwarding(guestAC, p.actorRef, actorUID, templateNS, templateName, overlayWorkloadID(c.GetName()), c.GetName()) } } @@ -322,6 +322,21 @@ func (s *AteomService) restoreFullScope(ctx context.Context, p actorBootParams, return err } s.running[actorUID] = ra + + // Publish the guest to GetWorkloadStats, past the last error return above + // for the same reason as in coldBootActor. Skipped when the dial failed: + // telemetry rides on the forwarding connection, so that activation answers + // FAILED_PRECONDITION until its next checkpoint. Not worth a second dial of + // its own — whatever kept the agent from answering a 15s retry loop would + // keep it from answering that one too. + if ra.guestAgent != nil { + workloadIDs := make([]string, 0, len(containers)) + for _, c := range containers { + workloadIDs = append(workloadIDs, overlayWorkloadID(c.GetName())) + } + s.guestStats.Store(&guestStatsTarget{actorUID: actorUID, agent: ra.guestAgent, workloadIDs: workloadIDs}) + } + slog.InfoContext(ctx, "Actor restored (overlay rootfs)", slog.String("id", actorUID), slog.Duration("total", time.Since(tStart))) return nil diff --git a/cmd/ateom-microvm/run.go b/cmd/ateom-microvm/run.go index eb4b34740..1ea149013 100644 --- a/cmd/ateom-microvm/run.go +++ b/cmd/ateom-microvm/run.go @@ -76,13 +76,17 @@ type runningActor struct { // un-faulted pages). Empty for cold-run actors (their snapshot is already complete). restoreSourceDir string - // logAgent is the kata-agent ttrpc client kept open for the lifetime of the - // stdout/stderr forwarding goroutines (they pump the container's output via - // ReadStdout/ReadStderr on this connection). It is NOT closed when RunWorkload / - // RestoreWorkload return — teardownActor closes it, which makes the in-flight - // ReadStdout/ReadStderr calls fail and the forwarding goroutines exit (io.EOF). - // nil if forwarding was not started (e.g. a best-effort post-restore dial failed). - logAgent *kata.AgentClient + // guestAgent is the kata-agent ttrpc client retained past boot. Two things + // share it: the stdout/stderr forwarding goroutines (they pump the + // container's output via ReadStdout/ReadStderr on this connection for the + // actor's lifetime) and GetWorkloadStats (via s.guestStats, which points at + // this same client). It is NOT closed when RunWorkload / RestoreWorkload + // return — teardownActor closes it, which makes the in-flight + // ReadStdout/ReadStderr calls fail and the forwarding goroutines exit + // (io.EOF). nil if the post-boot dial failed (e.g. a best-effort + // post-restore dial), which loses both log forwarding and guest stats for + // this activation. + guestAgent *kata.AgentClient } // baseIDFile is a tiny snapshot file (under the checkpoint/restore dir) holding @@ -489,7 +493,7 @@ func (s *AteomService) coldBootActor(ctx context.Context, p actorBootParams) (re return fmt.Errorf("while waiting for container readyz: %w", err) } - 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, guestAgent: ac} if err := s.activateActorNetworking(p.actorRef.Atespace, p.actorRef.Name, egress); err != nil { return err } @@ -499,10 +503,19 @@ func (s *AteomService) coldBootActor(ctx context.Context, p actorBootParams) (re // container/exec id is _ovl (see startOverlayContainer), so key the streams by // that and tag with the display container name. The goroutines read over ac for the // actor's lifetime and exit (io.EOF) when teardownActor closes ac. + workloadIDs := make([]string, 0, len(ctrs)) for _, c := range ctrs { s.startActorLogForwarding(ac, p.actorRef, actorUID, templateNS, templateName, overlayWorkloadID(c.name), c.name) + workloadIDs = append(workloadIDs, overlayWorkloadID(c.name)) } + // Publish the guest to GetWorkloadStats, past every error return above: a + // failing attempt closes ac on its way out (and coldBootActorRetrying may + // then try the whole boot again), so a target published earlier would leave + // the handler polling a connection nobody owns. Same client the forwarding + // above reads over — ttrpc multiplexes, and teardownActor ends both. + s.guestStats.Store(&guestStatsTarget{actorUID: actorUID, agent: ac, workloadIDs: workloadIDs}) + return nil } diff --git a/cmd/ateom-microvm/stats.go b/cmd/ateom-microvm/stats.go index cb17e6270..f8548ff8b 100644 --- a/cmd/ateom-microvm/stats.go +++ b/cmd/ateom-microvm/stats.go @@ -18,21 +18,192 @@ package main import ( "context" + "errors" + "time" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/agentstats" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" "github.com/agent-substrate/substrate/internal/proto/ateompb" ) +// statsCallTimeout bounds one container's guest-agent call. The RPC is polled +// on a timer, so it must fail fast rather than pile pollers up behind a guest +// that has stopped answering: a missed sample is cheap, a stuck handler is not. +// Generous next to a healthy read, which is a vsock round trip and four small +// file reads inside the guest, and far short of the lifecycle calls' 20-30s. +const statsCallTimeout = 2 * time.Second + +// containerStatsReader is the one guest-agent call GetWorkloadStats makes. +// *kata.AgentClient satisfies it; the narrow interface is what lets the handler +// be tested without a live micro-VM, which is otherwise the only way to get an +// agent to talk to. +type containerStatsReader interface { + StatsContainer(ctx context.Context, containerID string) (*agentpb.CgroupStats, error) +} + +// guestStatsTarget is everything GetWorkloadStats needs to sample a live guest. +// +// It exists so the handler never reads AteomService.running, which lock guards: +// the handler must not take lock (see below), and a map read racing a lifecycle +// RPC's write is a data race whatever the read is for. The fields it holds are +// the ones RunWorkload and RestoreWorkload already produce. +type guestStatsTarget struct { + // actorUID is the actor these containers belong to. Checked against the + // attribution before anything is reported, so a target left behind by a + // transition cannot file one actor's numbers under another's name. + actorUID string + + // agent is the kata-agent client the actor's log forwarding already keeps + // open for its lifetime, borrowed rather than dialed again. ttrpc + // multiplexes, so a poll and the forwarding reads share it safely. + agent containerStatsReader + + // workloadIDs are the guest containers to sum: the overlay WORKLOADS, one + // per actor container. Their carriers are deliberately absent — a carrier is + // created and never started (see CreateCarrier), so it runs no process and + // its cgroup has nothing in it to add. + workloadIDs []string +} + // GetWorkloadStats implements ateompb.Ateom/GetWorkloadStats. // -// The attribution half is wired up (see AteomService.activeActor); the measurement -// half is not. The micro-VM read goes to the guest agent's StatsContainer -// rather than the host cgroup — guest RAM is a fixed allocation, so the host -// cgroup barely moves with the workload — and lands in the follow-up to -// https://github.com/agent-substrate/substrate/issues/594, at which point this -// stops returning Unimplemented. +// The sample comes from inside the guest, not from the host cgroup. On this +// runtime the host cgroup holds cloud-hypervisor, whose memory is the guest RAM +// allocation it took at boot: near-constant, and near-identical for an idle +// actor and a saturated one. The guest kernel is what accounts for the +// workload, and the kata-agent is what can read it out. +// +// Unlike the three lifecycle RPCs this does not take s.lock, and must not +// start: it is polled on a timer for the whole life of a workload, while lock +// is held across an entire cold boot with its retry, across a snapshot write, +// and across a restore. Blocking on it would silence the poller through exactly +// the phases whose usage is most interesting. Both pieces of state it reads — +// the attribution and the guest target — are atomics for that reason. func (s *AteomService) GetWorkloadStats(ctx context.Context, req *ateompb.GetWorkloadStatsRequest) (*ateompb.GetWorkloadStatsResponse, error) { - return nil, status.Error(codes.Unimplemented, "GetWorkloadStats is not implemented yet") + if req.GetActorUid() == "" { + return nil, status.Error(codes.InvalidArgument, "actor_uid is required") + } + + // Both of these are NOT_FOUND rather than FAILED_PRECONDITION: they tell the + // caller the requested actor is not here, which no amount of retrying on the + // same timer will change. Its worker-to-actor mapping wants re-resolving. + active := s.activeActor.Load() + if active == nil { + return nil, status.Errorf(codes.NotFound, "ateom is available; it is not executing actor %q", req.GetActorUid()) + } + if active.UID != req.GetActorUid() { + return nil, status.Errorf(codes.NotFound, "ateom is executing actor %q, not the requested %q", active.UID, req.GetActorUid()) + } + + // The requested actor is the one here, but there is no guest to ask yet. + // Usually that is a poll landing in the boot or the restore: the ateom + // retains the attribution from the moment it accepts the actor, and the + // target is only published once the containers are up. It is also what a + // teardown looks like from here, since teardownActor clears the target + // before it closes the connection, and what a restore whose post-restore + // agent dial failed looks like for the rest of that activation. + // + // FAILED_PRECONDITION in all three: the answer is "no numbers right now", + // the caller should take the next sample, and for the teardown the next + // sample is the NOT_FOUND above. + target := s.guestStats.Load() + if target == nil { + return nil, status.Error(codes.FailedPrecondition, "no guest agent connection to measure yet") + } + // Belt and braces against the one thing that must never happen. The target + // is published and cleared under lock alongside the attribution, so this + // should be unreachable; if the two ever disagree, decline rather than + // report a stale guest's numbers under the requested actor's name. + if target.actorUID != active.UID { + return nil, status.Errorf(codes.FailedPrecondition, "guest agent connection belongs to actor %q, not %q", target.actorUID, active.UID) + } + + observedAt := time.Now() + sample, err := sumContainerStats(ctx, target) + if err != nil { + // Not Internal: a guest that has stopped answering is a routine state + // here, not a bug. Either the sandbox is going away — which the next + // CheckpointWorkload turns into the NOT_FOUND above — or the agent is + // briefly unreachable, and the next poll gets a number. + return nil, status.Errorf(codes.FailedPrecondition, "no container stats from the guest agent: %v", err) + } + + // Re-check that the same workload is still the active one. The calls above + // hold no lock, so a checkpoint plus a fresh run can complete underneath + // them, and the numbers would then belong to an actor other than the one + // being reported. Pointer identity is enough: activeActor is stored as a new + // pointer on every Run and Restore and never mutated in place, so an + // unchanged pointer means no transition happened across the read. + // + // NOT_FOUND, like the two checks above and for the same reason: the + // requested actor is no longer the one here, so a retry lands on one of them + // and gets that answer anyway. The same state should not report two + // different codes depending on where in the handler it was noticed. + if s.activeActor.Load() != active { + return nil, status.Errorf(codes.NotFound, "ateom stopped executing actor %q while the sample was being taken", req.GetActorUid()) + } + + return &ateompb.GetWorkloadStatsResponse{ + Atespace: active.Ref.Atespace, + ActorName: active.Ref.Name, + ActorUid: active.UID, + ActorTemplateNamespace: active.TemplateNamespace, + ActorTemplateName: active.TemplateName, + + SandboxClass: ateompb.SandboxClass_SANDBOX_CLASS_MICROVM, + Source: ateompb.StatsSource_STATS_SOURCE_GUEST_AGENT, + + MemoryCurrentBytes: sample.MemoryCurrentBytes, + MemoryPeakBytes: sample.MemoryPeakBytes, + MemoryWorkingSetBytes: sample.MemoryWorkingSetBytes, + CpuUsageUsec: sample.CPUUsageUsec, + + ObservedAtUnixNano: observedAt.UnixNano(), + }, nil +} + +// sumContainerStats reads every container of the actor and adds them up. +// +// A container the agent cannot report contributes nothing instead of failing +// the sample. That is not only the "a partial reading beats none" trade the +// gVisor side makes for a missing cgroup file — for the common way this +// happens, zero is the correct contribution rather than a fallback: a container +// that has exited took its guest cgroup with it and consumes nothing from here +// on. Failing the actor's telemetry because one sidecar is gone would be the +// wrong answer. +// +// It fails only when no container could be read at all, which is the guest as a +// whole not answering rather than one container being gone, and returns the +// last error so the caller can say why. +func sumContainerStats(ctx context.Context, target *guestStatsTarget) (agentstats.Sample, error) { + var ( + total agentstats.Sample + read int + lastErr error + ) + for _, id := range target.workloadIDs { + callCtx, cancel := context.WithTimeout(ctx, statsCallTimeout) + cs, err := target.agent.StatsContainer(callCtx, id) + cancel() + if err != nil { + lastErr = err + continue + } + read++ + total = total.Plus(agentstats.FromCgroupStats(cs)) + } + + if read == 0 { + if lastErr == nil { + // No containers to read. The actor is up with an empty container + // list, which the boot path does not produce, so treat it as "no + // numbers" rather than reporting a confident zero. + lastErr = errors.New("no containers to measure") + } + return agentstats.Sample{}, lastErr + } + return total, nil } diff --git a/cmd/ateom-microvm/stats_test.go b/cmd/ateom-microvm/stats_test.go index 449453ecf..7345f1380 100644 --- a/cmd/ateom-microvm/stats_test.go +++ b/cmd/ateom-microvm/stats_test.go @@ -18,12 +18,16 @@ package main import ( "context" + "errors" "testing" + "time" "github.com/google/go-cmp/cmp" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/testing/protocmp" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" "github.com/agent-substrate/substrate/internal/ateomstats" "github.com/agent-substrate/substrate/internal/proto/ateompb" "github.com/agent-substrate/substrate/internal/resources" @@ -97,36 +101,316 @@ func TestActorBootParamsAttributionMatchesRequest(t *testing.T) { } } -// TestGetWorkloadStatsUnimplemented pins the stub's advertised contract. The -// guest agent read replaces this body; until then a caller that gets any other code -// back would be reading numbers that are not there. -// -// The retention this stub will eventually read — s.activeActor, set by -// RunWorkload / RestoreWorkload and cleared by CheckpointWorkload — has no unit -// test, for the same reason as on the gVisor side: those three RPCs reach for -// netlink, cloud-hypervisor, and the worker pod's netns within a few lines of -// entry and cannot be driven from `go test`. Its mapping is covered above and -// in internal/ateomstats; the transitions are verified end to end once -// GetWorkloadStats returns real data. -func TestGetWorkloadStatsUnimplemented(t *testing.T) { +// The lifecycle transitions that maintain s.activeActor and s.guestStats — set +// by RunWorkload and RestoreWorkload, cleared by CheckpointWorkload's teardown — +// have no unit test, because those three RPCs each reach for netlink, +// cloud-hypervisor, and the worker pod's netns within a few lines of entry and +// cannot be driven from `go test`. The mapping they use is covered above and in +// internal/ateomstats; the transitions are verified end to end. What is testable +// here is everything GetWorkloadStats does with the result, which is where the +// polling loop will actually live. + +var testActor = ateomstats.ActorAttribution{ + Ref: resources.ActorRef{Atespace: "space-a", Name: "actor-a"}, + UID: "uid-a", + TemplateNamespace: "ns-a", + TemplateName: "template-a", +} + +// fakeAgent stands in for the kata-agent client: a canned reply per container +// id, or a canned error for the ones that are meant to fail. +type fakeAgent struct { + stats map[string]*agentpb.CgroupStats + errs map[string]error + + // calls records the container ids asked for, in order, so a test can tell + // "summed two containers" from "read one twice". + calls []string + // deadlines records whether each call's context carried one, which is how + // the per-call timeout is pinned. + deadlines []bool +} + +func (f *fakeAgent) StatsContainer(ctx context.Context, containerID string) (*agentpb.CgroupStats, error) { + f.calls = append(f.calls, containerID) + _, ok := ctx.Deadline() + f.deadlines = append(f.deadlines, ok) + if err, ok := f.errs[containerID]; ok { + return nil, err + } + return f.stats[containerID], nil +} + +// containerStats is one container's guest reading: usage bytes, peak bytes, +// reclaimable page cache, and cumulative CPU nanoseconds. +func containerStats(usage, peak, inactiveFile, cpuNanos uint64) *agentpb.CgroupStats { + return &agentpb.CgroupStats{ + MemoryStats: &agentpb.MemoryStats{ + Usage: &agentpb.MemoryData{Usage: usage, MaxUsage: peak}, + Stats: map[string]uint64{"inactive_file": inactiveFile}, + }, + CpuStats: &agentpb.CpuStats{CpuUsage: &agentpb.CpuUsage{TotalUsage: cpuNanos}}, + } +} + +// newStatsService builds a service executing testActor with the given guest +// containers published to GetWorkloadStats. +func newStatsService(agent containerStatsReader, workloadIDs ...string) *AteomService { s := &AteomService{} + s.activeActor.Store(&testActor) + s.guestStats.Store(&guestStatsTarget{actorUID: testActor.UID, agent: agent, workloadIDs: workloadIDs}) + return s +} + +func TestGetWorkloadStats(t *testing.T) { + agent := &fakeAgent{stats: map[string]*agentpb.CgroupStats{ + "app_ovl": containerStats(157286400, 209715200, 20971520, 1234567000), + }} + s := newStatsService(agent, "app_ovl") + + before := time.Now().UnixNano() + got, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-a"}) + after := time.Now().UnixNano() + if err != nil { + t.Fatalf("GetWorkloadStats() error = %v, want nil", err) + } + + if got.GetObservedAtUnixNano() < before || got.GetObservedAtUnixNano() > after { + t.Errorf("GetWorkloadStats() observed_at_unix_nano = %d, want within [%d, %d]", got.GetObservedAtUnixNano(), before, after) + } + // Checked above; zeroed so the rest can be compared as a whole. + got.ObservedAtUnixNano = 0 - resp, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-c"}) - if resp != nil { - t.Errorf("GetWorkloadStats() returned response %v, want nil", resp) + want := &ateompb.GetWorkloadStatsResponse{ + Atespace: "space-a", + ActorName: "actor-a", + ActorUid: "uid-a", + ActorTemplateNamespace: "ns-a", + ActorTemplateName: "template-a", + SandboxClass: ateompb.SandboxClass_SANDBOX_CLASS_MICROVM, + Source: ateompb.StatsSource_STATS_SOURCE_GUEST_AGENT, + MemoryCurrentBytes: 157286400, + MemoryPeakBytes: 209715200, + MemoryWorkingSetBytes: 136314880, + CpuUsageUsec: 1234567, + } + if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" { + t.Errorf("GetWorkloadStats() mismatch (-want +got):\n%s", diff) + } + + // A poll must not be able to hang on a guest that has stopped answering, + // which is only true if the handler bounds each call rather than passing the + // caller's context straight through. + if want := []bool{true}; !cmp.Equal(want, agent.deadlines) { + t.Errorf("StatsContainer call contexts had deadlines %v, want %v", agent.deadlines, want) + } +} + +// TestGetWorkloadStatsSumsContainers covers the multi-container actor: the +// guest gives one cgroup per container (see StartOverlayWorkload), and the +// proto reports one figure for the actor. +func TestGetWorkloadStatsSumsContainers(t *testing.T) { + agent := &fakeAgent{stats: map[string]*agentpb.CgroupStats{ + "app_ovl": containerStats(1000, 4000, 100, 7000), + "sidecar_ovl": containerStats(500, 800, 200, 3000), + }} + s := newStatsService(agent, "app_ovl", "sidecar_ovl") + + got, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-a"}) + if err != nil { + t.Fatalf("GetWorkloadStats() error = %v, want nil", err) + } + + if want := uint64(1500); got.GetMemoryCurrentBytes() != want { + t.Errorf("memory_current_bytes = %d, want %d", got.GetMemoryCurrentBytes(), want) + } + // The sum of the peaks, which is an upper bound on the peak of the sum: the + // two containers need not have peaked at the same moment. + if want := uint64(4800); got.GetMemoryPeakBytes() != want { + t.Errorf("memory_peak_bytes = %d, want %d", got.GetMemoryPeakBytes(), want) + } + if want := uint64(1200); got.GetMemoryWorkingSetBytes() != want { + t.Errorf("memory_working_set_bytes = %d, want %d", got.GetMemoryWorkingSetBytes(), want) + } + if want := uint64(10); got.GetCpuUsageUsec() != want { + t.Errorf("cpu_usage_usec = %d, want %d", got.GetCpuUsageUsec(), want) + } + + if want := []string{"app_ovl", "sidecar_ovl"}; !cmp.Equal(want, agent.calls) { + t.Errorf("StatsContainer called for %v, want %v", agent.calls, want) + } +} + +// TestGetWorkloadStatsSkipsUnreadableContainer pins the partial-reading trade: +// one container the agent cannot report must not silence the actor's telemetry. +// The usual way in is a container that has exited, which took its guest cgroup +// with it and consumes nothing from here on — so contributing zero is the +// correct answer for it, not merely a tolerable one. +func TestGetWorkloadStatsSkipsUnreadableContainer(t *testing.T) { + agent := &fakeAgent{ + stats: map[string]*agentpb.CgroupStats{"app_ovl": containerStats(1000, 2000, 100, 5000)}, + errs: map[string]error{"exited_ovl": errors.New("no such container")}, + } + s := newStatsService(agent, "app_ovl", "exited_ovl") + + got, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-a"}) + if err != nil { + t.Fatalf("GetWorkloadStats() error = %v, want nil", err) + } + if want := uint64(1000); got.GetMemoryCurrentBytes() != want { + t.Errorf("memory_current_bytes = %d, want %d", got.GetMemoryCurrentBytes(), want) } - if got := status.Code(err); got != codes.Unimplemented { - t.Errorf("GetWorkloadStats() error code = %v, want %v (err: %v)", got, codes.Unimplemented, err) + if want := uint64(5); got.GetCpuUsageUsec() != want { + t.Errorf("cpu_usage_usec = %d, want %d", got.GetCpuUsageUsec(), want) + } +} + +// TestGetWorkloadStatsCountsAnsweredContainer covers the boundary of the rule +// above: a container the agent answers for without cgroup stats is a reading of +// zero, not a failure, so the sample stands even when it is the only container. +func TestGetWorkloadStatsCountsAnsweredContainer(t *testing.T) { + agent := &fakeAgent{stats: map[string]*agentpb.CgroupStats{"app_ovl": nil}} + s := newStatsService(agent, "app_ovl") + + got, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-a"}) + if err != nil { + t.Fatalf("GetWorkloadStats() error = %v, want nil", err) + } + if got.GetMemoryCurrentBytes() != 0 || got.GetCpuUsageUsec() != 0 { + t.Errorf("GetWorkloadStats() = %v, want an all-zero measurement", got) + } +} + +func TestGetWorkloadStatsErrors(t *testing.T) { + healthy := &fakeAgent{stats: map[string]*agentpb.CgroupStats{"app_ovl": containerStats(1000, 2000, 100, 5000)}} + + for _, tc := range []struct { + name string + // service builds the service under test, so each case can put the two + // atomics in exactly the state it means to exercise. + service func() *AteomService + actorUID string + want codes.Code + }{ + { + // A required field the caller left off: a client bug, distinct from + // the races below, so it gets a distinct code. + name: "empty actor_uid", + service: func() *AteomService { return newStatsService(healthy, "app_ovl") }, + actorUID: "", + want: codes.InvalidArgument, + }, + { + // Not here at all. NOT_FOUND rather than FAILED_PRECONDITION, because + // what the caller should do about it is re-resolve, not retry. + name: "ateom is available", + service: func() *AteomService { return &AteomService{} }, + actorUID: "uid-a", + want: codes.NotFound, + }, + { + // The worker was recycled between the caller's view of the world and + // this call. Reporting anyway would file one actor's numbers under + // another's name, and it is the same "not here" as the case above. + name: "actor_uid does not match the executing workload", + service: func() *AteomService { return newStatsService(healthy, "app_ovl") }, + actorUID: "uid-b", + want: codes.NotFound, + }, + { + // The requested actor is the one here, but the guest is not up yet — + // a poll landing in the boot or the restore, or one landing after + // teardownActor cleared the target. The transient case. + name: "no guest agent connection yet", + service: func() *AteomService { + s := &AteomService{} + s.activeActor.Store(&testActor) + return s + }, + actorUID: "uid-a", + want: codes.FailedPrecondition, + }, + { + // Should be unreachable — the two atomics are written together under + // lock — so what is pinned here is that disagreeing state declines + // instead of misattributing. + name: "guest agent connection belongs to another actor", + service: func() *AteomService { + s := newStatsService(healthy, "app_ovl") + s.guestStats.Store(&guestStatsTarget{actorUID: "uid-b", agent: healthy, workloadIDs: []string{"app_ovl"}}) + return s + }, + actorUID: "uid-a", + want: codes.FailedPrecondition, + }, + { + // Not one container gone but the guest as a whole not answering: the + // sandbox is going away, which the next CheckpointWorkload turns into + // the NOT_FOUND above, or the agent is briefly unreachable. Either way + // it is "no numbers right now" rather than Internal — the guest not + // answering is a routine state here, not a bug in this ateom. + name: "no container answers", + service: func() *AteomService { + agent := &fakeAgent{errs: map[string]error{ + "app_ovl": errors.New("ttrpc: closed"), + "sidecar_ovl": errors.New("ttrpc: closed"), + }} + return newStatsService(agent, "app_ovl", "sidecar_ovl") + }, + actorUID: "uid-a", + want: codes.FailedPrecondition, + }, + { + // The boot path does not produce an actor with no containers, so a + // confident zero would be reporting a state we do not understand. + name: "no containers to measure", + service: func() *AteomService { return newStatsService(healthy) }, + actorUID: "uid-a", + want: codes.FailedPrecondition, + }, + } { + t.Run(tc.name, func(t *testing.T) { + resp, err := tc.service().GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: tc.actorUID}) + if resp != nil { + t.Errorf("GetWorkloadStats() returned response %v, want nil", resp) + } + if got := status.Code(err); got != tc.want { + t.Errorf("GetWorkloadStats() error code = %v, want %v (err: %v)", got, tc.want, err) + } + }) + } +} + +// TestGetWorkloadStatsDoesNotTakeLock is the regression test for the property +// the design turns on: a stats poll must not queue behind a lifecycle RPC. +// s.lock is held for the duration of the call here, so a handler that reached +// for it — or that looked up the agent client in s.running, which lock guards — +// would deadlock and fail this test by timing out rather than by assertion. +func TestGetWorkloadStatsDoesNotTakeLock(t *testing.T) { + agent := &fakeAgent{stats: map[string]*agentpb.CgroupStats{"app_ovl": containerStats(1000, 2000, 100, 5000)}} + s := newStatsService(agent, "app_ovl") + + // Stands in for a RunWorkload or CheckpointWorkload in flight, which hold + // the lock across their entire bodies. + s.lock.Lock() + defer s.lock.Unlock() + + if _, err := s.GetWorkloadStats(context.Background(), &ateompb.GetWorkloadStatsRequest{ActorUid: "uid-a"}); err != nil { + t.Errorf("GetWorkloadStats() error = %v, want nil", err) } } // TestAteomServiceStartsAvailable checks that a freshly constructed service -// retains no attribution, mirroring the gVisor ateom's test of the same name. -// GetWorkloadStats's NOT_FOUND-when-available behavior is built on this: a -// non-nil zero value here would make an idle ateom report an empty actor's -// usage instead of refusing. +// retains no attribution and offers no guest, mirroring the gVisor ateom's test +// of the same name. GetWorkloadStats's NOT_FOUND-when-available behavior is +// built on the first: a non-nil zero value would make an idle ateom report an +// empty actor's usage instead of refusing. func TestAteomServiceStartsAvailable(t *testing.T) { - if s := (&AteomService{}); s.activeActor.Load() != nil { - t.Errorf("new AteomService.activeActor = %v, want nil", s.activeActor.Load()) + s := &AteomService{} + if got := s.activeActor.Load(); got != nil { + t.Errorf("new AteomService.activeActor = %v, want nil", got) + } + if got := s.guestStats.Load(); got != nil { + t.Errorf("new AteomService.guestStats = %v, want nil", got) } } diff --git a/internal/proto/ateompb/ateom.pb.go b/internal/proto/ateompb/ateom.pb.go index cfe6b3ed5..3ff384612 100644 --- a/internal/proto/ateompb/ateom.pb.go +++ b/internal/proto/ateompb/ateom.pb.go @@ -162,7 +162,12 @@ const ( StatsSource_STATS_SOURCE_UNSPECIFIED StatsSource = 0 // Read from the sandbox's cgroup on the host. StatsSource_STATS_SOURCE_CGROUP StatsSource = 1 - // Read from inside the guest, over the guest agent's vsock connection. + // Read from inside the guest, over the guest agent's vsock connection. What + // it counts is the workload's own containers, as the guest kernel accounts + // for them: the guest kernel itself, the agent, and the VMM process on the + // host are overhead this source cannot see. The cgroup source above is the + // other way round -- the sandbox's host process is one process, so its + // runtime's overhead is charged along with the workload's. StatsSource_STATS_SOURCE_GUEST_AGENT StatsSource = 2 ) @@ -1080,8 +1085,11 @@ func (x *GetWorkloadStatsRequest) GetActorUid() string { // GetWorkloadStatsResponse is one resource-usage sample for the executing // workload. The unit of measurement is the SANDBOX, which today equals the -// actor; per-container attribution needs the gVisor sentry's own accounting and -// is not reported here. +// actor. Per-container attribution is not reported: the micro-VM source could +// give it, since the guest keeps a cgroup per container and ateom sums them, +// but the gVisor source cannot split one at all without the sentry's own +// accounting, and a field only one runtime could ever fill would be worse than +// none. type GetWorkloadStatsResponse struct { state protoimpl.MessageState `protogen:"open.v1"` // Identity of the measured actor, retained by ateom from the diff --git a/internal/proto/ateompb/ateom.proto b/internal/proto/ateompb/ateom.proto index 2d6a8d8be..f190fcd15 100644 --- a/internal/proto/ateompb/ateom.proto +++ b/internal/proto/ateompb/ateom.proto @@ -273,14 +273,22 @@ enum StatsSource { STATS_SOURCE_UNSPECIFIED = 0; // Read from the sandbox's cgroup on the host. STATS_SOURCE_CGROUP = 1; - // Read from inside the guest, over the guest agent's vsock connection. + // Read from inside the guest, over the guest agent's vsock connection. What + // it counts is the workload's own containers, as the guest kernel accounts + // for them: the guest kernel itself, the agent, and the VMM process on the + // host are overhead this source cannot see. The cgroup source above is the + // other way round -- the sandbox's host process is one process, so its + // runtime's overhead is charged along with the workload's. STATS_SOURCE_GUEST_AGENT = 2; } // GetWorkloadStatsResponse is one resource-usage sample for the executing // workload. The unit of measurement is the SANDBOX, which today equals the -// actor; per-container attribution needs the gVisor sentry's own accounting and -// is not reported here. +// actor. Per-container attribution is not reported: the micro-VM source could +// give it, since the guest keeps a cgroup per container and ateom sums them, +// but the gVisor source cannot split one at all without the sentry's own +// accounting, and a field only one runtime could ever fill would be worse than +// none. message GetWorkloadStatsResponse { // Identity of the measured actor, retained by ateom from the // RunWorkloadRequest / RestoreWorkloadRequest that started it. Echoed back so From e41b274d167a6d3e20426869f6e6170dec695b12 Mon Sep 17 00:00:00 2001 From: Tim Bai Date: Tue, 11 Aug 2026 16:19:07 -0400 Subject: [PATCH 2/3] ateom-microvm: address review on peak docs and loop bounds Cross-reference the summed-peak caveat from the MemoryPeakBytes field, correct the Plus doc (the guest does track the actor as a unit at the shared /ateomchv parent; only the agent read path is missing), and bail out of the container loop when the caller's ctx is already dead instead of returning a partial sum as success. --- .../internal/agentstats/agentstats.go | 15 ++++++++++++--- cmd/ateom-microvm/stats.go | 10 ++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/cmd/ateom-microvm/internal/agentstats/agentstats.go b/cmd/ateom-microvm/internal/agentstats/agentstats.go index abe853d7c..5fccef12b 100644 --- a/cmd/ateom-microvm/internal/agentstats/agentstats.go +++ b/cmd/ateom-microvm/internal/agentstats/agentstats.go @@ -47,6 +47,9 @@ type Sample struct { // MemoryPeakBytes is the high-water mark of MemoryCurrentBytes. Zero when // the guest kernel does not expose one: the agent fills it from the cgroup's // max-usage file, which cgroup v2 only grew in Linux 5.19. + // + // For a single container this is exact; summed across containers it is an + // upper bound, not an observed maximum — see Plus for why. MemoryPeakBytes uint64 // MemoryWorkingSetBytes is MemoryCurrentBytes less the reclaimable page @@ -128,12 +131,18 @@ func inactiveFileBytes(cs *agentpb.CgroupStats) (uint64, bool) { // // Summing the peaks is an upper bound on the peak of the sum, not the peak of // the sum itself: two containers that peaked at different moments add up to a -// total the actor never actually reached. Reporting the true figure would need -// the guest to track the actor as a unit, which it does not — each container -// gets its own cgroup (see StartOverlayWorkload). The bound is the honest +// total the actor never actually reached. The bound is the honest // approximation, and for the single-container actors this runtime mostly serves // it is exact. // +// The guest kernel does track the true figure: every container cgroup sits +// under the shared /ateomchv parent (see StartOverlayWorkload and the +// CgroupsPath defaults in internal/kata), so with hierarchical accounting the +// parent's memory.peak is the actor-level maximum this sum approximates. The +// kata-agent just has no RPC that reads a cgroup by path — StatsContainer is +// per-container only. If the agent ever grows a sandbox-level stats read, that +// replaces this summing outright. +// // Saturating on overflow, so a nonsensical reading from one container cannot // wrap the total to a small number and read as healthy. func (s Sample) Plus(o Sample) Sample { diff --git a/cmd/ateom-microvm/stats.go b/cmd/ateom-microvm/stats.go index f8548ff8b..588201737 100644 --- a/cmd/ateom-microvm/stats.go +++ b/cmd/ateom-microvm/stats.go @@ -178,6 +178,11 @@ func (s *AteomService) GetWorkloadStats(ctx context.Context, req *ateompb.GetWor // It fails only when no container could be read at all, which is the guest as a // whole not answering rather than one container being gone, and returns the // last error so the caller can say why. +// +// The loop as a whole is bounded by ctx — the caller's RPC deadline — with +// maxActorContainers * statsCallTimeout as the ceiling when the caller set +// none. statsCallTimeout is per call so that one hung container read cannot +// eat the budget the remaining containers still need. func sumContainerStats(ctx context.Context, target *guestStatsTarget) (agentstats.Sample, error) { var ( total agentstats.Sample @@ -185,6 +190,11 @@ func sumContainerStats(ctx context.Context, target *guestStatsTarget) (agentstat lastErr error ) for _, id := range target.workloadIDs { + if err := ctx.Err(); err != nil { + // The caller is gone; the per-call ctx below would fail instantly + // anyway, so stop burning through the remaining containers. + return agentstats.Sample{}, err + } callCtx, cancel := context.WithTimeout(ctx, statsCallTimeout) cs, err := target.agent.StatsContainer(callCtx, id) cancel() From 2c89c979b40c1f18eba6ce514864cf39620aa8e0 Mon Sep 17 00:00:00 2001 From: Tim Bai Date: Tue, 11 Aug 2026 17:09:37 -0400 Subject: [PATCH 3/3] ateom-microvm: clarify stats docs from review StatsContainer's doc prescribed reading a nil result as "no numbers, not zero" while its only caller deliberately folds nil into a zero contribution; describe the semantics and point at the summing decision instead. And make the workloadIDs comment self-contained: it leaned on the carrier/workload split without explaining it. --- cmd/ateom-microvm/internal/kata/agentclient.go | 5 ++++- cmd/ateom-microvm/stats.go | 12 ++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/cmd/ateom-microvm/internal/kata/agentclient.go b/cmd/ateom-microvm/internal/kata/agentclient.go index a365c8b6f..dd37e417f 100644 --- a/cmd/ateom-microvm/internal/kata/agentclient.go +++ b/cmd/ateom-microvm/internal/kata/agentclient.go @@ -237,7 +237,10 @@ func (a *AgentClient) ReadStderr(ctx context.Context, containerID, execID string // It returns only the cgroup half of the response; the network counters // alongside it are per-guest-interface rather than per-container and are not // what ateom reports. A nil return with a nil error means the agent answered -// without cgroup stats, which callers should read as "no numbers", not zero. +// without cgroup stats for this container — it has no accounting for it, which +// is a normal state for one that has exited. What that means for the actor is +// the caller's call: the summing in GetWorkloadStats folds it in as a zero +// contribution, because a gone container consumes nothing from here on. // // Safe to call while the stdout/stderr forwarding goroutines are reading over // the same client: ttrpc multiplexes concurrent calls over the one connection, diff --git a/cmd/ateom-microvm/stats.go b/cmd/ateom-microvm/stats.go index 588201737..426c2f4e5 100644 --- a/cmd/ateom-microvm/stats.go +++ b/cmd/ateom-microvm/stats.go @@ -61,10 +61,14 @@ type guestStatsTarget struct { // multiplexes, so a poll and the forwarding reads share it safely. agent containerStatsReader - // workloadIDs are the guest containers to sum: the overlay WORKLOADS, one - // per actor container. Their carriers are deliberately absent — a carrier is - // created and never started (see CreateCarrier), so it runs no process and - // its cgroup has nothing in it to add. + // workloadIDs are the guest containers to sum, one per actor container. + // Each actor container exists in the guest as TWO kata containers (see + // overlayWorkloadID): a "carrier", whose only job is to make the agent + // bind the read-only image rootfs at a fixed guest path, and the overlay + // WORKLOAD, which lays a writable upper over it and runs the container's + // actual process. Only workload ids belong here: a carrier is created but + // never started, so no process ever runs in it and its cgroup has nothing + // to add to the sum. workloadIDs []string }