Right-size actor sandboxes to declared ActorTemplate resource limits - #679
Conversation
0e66671 to
c1f9f03
Compare
|
This PR looks like it implements exactly what we proposed, but I think the premise was flawed (my bad!) I think maybe instead what we should actually do is:
In the current state we will wind up heavily pinning to the worker it first ran on but maybe not considering it when scheduling again? cc Julian Gutierrez Oschmann (@juli4n) 3) should be reusable from the current state, but I think 1/2 might need to replace the downward API approach. |
Implements BenTheElder's 3-step redesign (PR agent-substrate#679), replacing the downward-API pod-sizing approach: 1. ActorTemplate.Spec.Resources declares per-actor compute limits, baked into the immutable spec (so it can seed microVM snapshots). 2. Scheduling gates worker selection on advertised capacity: the syncer publishes each worker's ateom-container limits as Worker capacity, and the scheduler skips workers whose capacity is below the actor's limits (zero constraint or zero capacity = unconstrained, graceful fallback). 3. Limits flow over the actor RPCs (ateapi -> atelet -> ateom) via new cpu_milli/memory_bytes fields on Run/Restore/RunWorkload/RestoreWorkload requests, applied to the OCI spec by internal/sizing. internal/sizing loses env Discover(); sizes now come from RPC via FromLimits(), and its PodSize type is renamed SandboxSize (the size is no longer derived from the pod). atecontroller no longer injects resource-fieldRef env. docs/api-guide.md is updated: the WorkerPool resources section documents worker capacity (the scheduling envelope), and a new ActorTemplate spec.resources section documents actor right-sizing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6f01be6 to
58d0da9
Compare
Chenyi Wang (chw120)
changed the title
| vcpus = v | ||
| } | ||
| if sz.MemoryBytes > 0 { | ||
| if m := int(sz.MemoryBytes/(1024*1024)) - s.memReserveMiB; m > 0 { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – When the reserve swallows the whole limit this silently falls back to the kata default, which is larger than what the actor declared. guestConfig defaults to 2048 MiB, so an actor declaring 256Mi — exactly the default reserve — takes the m > 0 branch as false and boots a 2 GiB VM, 8x its declared size. The scheduler already placed it against 256Mi, so a worker sized for that actor is overcommitted by the same factor and the pod OOMs rather than the actor being capped.
The band just above the reserve is the other half: 320Mi leaves 64 MiB of guest RAM, which will not boot a kata guest, and the failure surfaces as a boot hang rather than a sizing error.
Both cases are quiet. Failing the run with a message naming the declared limit and the reserve would make them obvious, and a minimum on spec.resources at admission would catch them before an actor is ever scheduled — there is no validation on that field today.
There was a problem hiding this comment.
Thanks Ben for reviewing the PR. I updated the code with the detailed error message and the validation check. Please review again.
dc1c37c to
69f77e9
Compare
copyFile used a plain `io.Copy`, which reads a hole as zeroes and writes it back as data. The biggest thing it copies is a guest memory image, which is mostly unallocated: staging a local checkpoint for a restore inflated a 164MiB snapshot into its full 2GiB logical size. (Note: That itself is a bug, see #679) That costs disk and I/O on every local checkpoint restore, and it compounds, because cloud-hypervisor then loads the whole image and the next checkpoint it writes is dense in turn. Five pause cycles on one actor left 8.2GB of local checkpoints where a few hundred MiB would do. Copy only the populated extents, located with `SEEK_DATA` and `SEEK_HOLE`, and fall back to the dense copy when the filesystem cannot report holes. > It's a good idea to open an issue first for discussion. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
|
[needs a rebase, a lot changed] |
69f77e9 to
178d926
Compare
| # sized by the ActorTemplate's spec.resources below, not by these; keep the | ||
| # actor memory below this limit so the VMM reserve fits (see internal/sizing). | ||
| resources: | ||
| requests: |
There was a problem hiding this comment.
given that we snapshot/restore, I'm not sure we should support requests, vs only limits for now?
unless we play with the balloon driver, uVM has to have max memory allocated up front, and then on restore it must have the same size.
from my past experiments, the balloon can be pretty slow.
There was a problem hiding this comment.
SG! limits is only being used in scheduling. Removed requests in demos to avoid the confusion.
| // Provision the sentry's vCPU count from the cgroup CPU quota written by | ||
| // sizing.ApplyToOCISpec, so the sandbox is sized to the pod's limit (runsc | ||
| // otherwise sizes to all host CPUs). Global flag: before the subcommand. | ||
| "--cpu-num-from-quota", |
There was a problem hiding this comment.
🤖 blocking 🔴 – -root was dropped from cmdCreate. This --cpu-num-from-quota line replaced "-root", ateompath.RunSCStateDir(r.actorUID) instead of being added alongside it.
Every other subcommand still passes it — cmdStart (line 161), cmdCheckpoint, cmdFsCheckpoint, cmdRestore, cmdDelete, cmdState — so runsc create now writes container state to runsc's default root while runsc start looks in <actor>/runsc-state. They no longer agree on where state lives, which should break gVisor cold boot at the start step.
cmdRestore at line 267 shows the intended shape: -root kept, the new flag added after it.
There was a problem hiding this comment.
Good catch. Restored it.
| if v := sz.VCPUs(); v > 0 { | ||
| vcpus = v | ||
| } | ||
| memMiB, err = resolveGuestMemMiB(sz.MemoryBytes, s.memReserveMiB, memMiB) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The guest container's memory cgroup and the VM's RAM get sized from different numbers. buildActorContainers ran above with p.size, so ApplyToOCISpec already set the container's Memory.Limit to the full declared limit, while this line gives the VM that limit minus the VMM reserve. A 1Gi actor ends up with a 1024Mi cgroup cap inside a VM holding 768Mi of RAM.
The cap can then never bind. The workload exhausts guest RAM before it reaches the cgroup limit, so instead of a clean OOM kill you get the guest kernel's OOM killer or a hang — the failure mode minGuestMemMiB exists to prevent. Sizing the container cgroup to the post-reserve guest RAM would make the limit meaningful.
restoreFullScope calls buildActorContainers with p.size too, so the same gap applies to a FULL-scope restore.
There was a problem hiding this comment.
Updated code to size guest container memory cgroup to post-reserve guest RAM.
| // for gVisor, the kata config for the micro-VM). | ||
| // | ||
| // +optional | ||
| Resources *corev1.ResourceRequirements `json:"resources,omitempty"` |
There was a problem hiding this comment.
Human edit: upgrading to should-fix 🟡
🤖 question 🟢 – The field takes a full corev1.ResourceRequirements, but only Limits is ever read (actorResourceLimits in workflow_resume.go). Nothing rejects requests, so a user can set them, watch them persist into the spec, and get no effect. Claims is accepted on the same terms. The doc comment says requests aren't consulted, which only helps the people who read it.
If your requests-vs-limits question on the demo template lands on "limits only for now", a CEL rule rejecting resources.requests would turn a silent no-op into an error at create time. Worth settling either way, since this is in the immutable spec and can't be corrected on an existing template.
There was a problem hiding this comment.
Added more validations to reject it.
|
|
||
| // The object storage URI of the ActorTemplate's golden snapshot. | ||
| // Set only when scope is SNAPSHOT_SCOPE_DATA_ON_GOLDEN. Mirrors the | ||
| // snapshot_uri contract (field 8). |
There was a problem hiding this comment.
🤖 nit 🟢 – A line went missing here. This previously read "Set only when scope is SNAPSHOT_SCOPE_DATA_ON_GOLDEN. Mirrors the snapshot_uri contract (field 8)." and now reads "...golden snapshot." followed by a dangling "snapshot_uri contract (field 8)." The deletion looks incidental to this PR, and it drops the only statement of when the field is set.
There was a problem hiding this comment.
Restored.
- gvisor: restore missing -root flag in runsc cmdCreate - microvm: size guest container memory cgroup to post-reserve guest RAM - actortemplate: reject requests and claims via CEL validation rules - demos: simplify WorkerPool template resources to limits only - proto: restore missing comment line on golden_snapshot_uri in ateom.proto
379bf24 to
bddc290
Compare
- gvisor: restore missing -root flag in runsc cmdCreate - microvm: size guest container memory cgroup to post-reserve guest RAM - actortemplate: reject requests and claims via CEL validation rules - demos: simplify WorkerPool template resources to limits only - proto: restore missing comment line on golden_snapshot_uri in ateom.proto
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
mostly resolved now, two more agent findings. thanks for continuing to work on this
| } | ||
| memMiB, err := resolveGuestMemMiB(sz.MemoryBytes, s.memReserveMiB, 0) | ||
| if err != nil { | ||
| return sz |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This swallows the error and returns the size unreduced, which is the mismatch the fix was for. When resolveGuestMemMiB fails, sz.MemoryBytes is still the outer declared limit, so the guest container cgroup goes back to being larger than the VM's RAM.
Cold boot is protected because it checks resolveGuestMemMiB's error itself and aborts. restoreFullScope doesn't — the VM size comes from the snapshot — so the reachable case is raising --vmm-mem-reserve-mib after an actor was snapshotted: the next restore quietly gets the bad pairing instead of a clear failure. TestGuestSize covers the reduce and unset paths but not this one.
Returning the error, or at least logging, would keep it from failing silently.
Related: coldBootActor at line 403 does the same translation inline (guestSize := sz; guestSize.MemoryBytes = memMiB * MiB) rather than calling this helper. Two implementations that agree on the happy path and disagree on this one.
There was a problem hiding this comment.
Updated line 403 to call guestSize().
| // limits: CPU passes through unchanged (kata-agent sets the CFS quota), while | ||
| // memory is reduced by the VMM reserve so the container cgroup limit inside the | ||
| // guest matches the guest VM's actual RAM rather than the (larger) outer limit. | ||
| func (s *AteomService) guestSize(sz sizing.SandboxSize) sizing.SandboxSize { |
There was a problem hiding this comment.
🤖 question 🟢 – With this change the container cgroup limit lands at exactly the guest's total RAM, so it still can't bind before the guest kernel OOMs — the kernel, agent and init need some of that RAM, so the workload hits the guest OOM killer just short of its cgroup limit. Closer than before, but the clean per-container OOM kill is still out of reach.
It also applies uniformly: every container in a multi-container actor gets a limit equal to the whole guest RAM, so they aren't bounded relative to each other. That may well be the intent — the VM's RAM is the real envelope and the per-container cgroup is belt-and-braces — but if so it's worth saying in the guestSize comment, since "right-sized to the actor's limits" reads as if the cgroup is doing the enforcing.
There was a problem hiding this comment.
Added more comments to clarify it. If it's better to subtract the guest-side overhead, I can make changes in this or following PR.
| // only places an actor on a worker whose capacity is >= the actor's declared | ||
| // resource limits. Zero means "unknown/unset": treated as unconstrained so | ||
| // placement is not blocked (matching the pre-capacity behavior). | ||
| int64 cpu_milli_capacity = 12; // CPU capacity in millicores. |
There was a problem hiding this comment.
maybe wrap these two fields in a message for better logical grouping?
There was a problem hiding this comment.
Created message WorkerCapacity.
microvm: guestSize returns an error instead of the unreduced size when resolveGuestMemMiB fails. restoreFullScope now checks it, so raising --vmm-mem-reserve-mib after an actor was snapshotted fails loudly rather than pairing the guest with a container cgroup limit larger than the VM's RAM. coldBootActor calls the helper instead of reimplementing the same translation inline, and TestGuestSize covers the error path. Document in guestSize why the per-container cgroup limit is the whole guest RAM: the VM's RAM is the real envelope and the cgroup is belt-and-braces, so the guest OOM killer fires just before the cgroup limit binds and multi-container actors are not bounded relative to each other. ateapi: group the worker capacity fields into a WorkerCapacity message (cpu_milli, memory_bytes), matching the field naming used for limits in ateom.proto and atelet.proto.
e828ab1 to
b1f2805
Compare
- gvisor: restore missing -root flag in runsc cmdCreate - microvm: size guest container memory cgroup to post-reserve guest RAM - actortemplate: reject requests and claims via CEL validation rules - demos: simplify WorkerPool template resources to limits only - proto: restore missing comment line on golden_snapshot_uri in ateom.proto
microvm: guestSize returns an error instead of the unreduced size when resolveGuestMemMiB fails. restoreFullScope now checks it, so raising --vmm-mem-reserve-mib after an actor was snapshotted fails loudly rather than pairing the guest with a container cgroup limit larger than the VM's RAM. coldBootActor calls the helper instead of reimplementing the same translation inline, and TestGuestSize covers the error path. Document in guestSize why the per-container cgroup limit is the whole guest RAM: the VM's RAM is the real envelope and the cgroup is belt-and-braces, so the guest OOM killer fires just before the cgroup limit binds and multi-container actors are not bounded relative to each other. ateapi: group the worker capacity fields into a WorkerCapacity message (cpu_milli, memory_bytes), matching the field naming used for limits in ateom.proto and atelet.proto.
| // An error means the declared limit cannot be honored (see resolveGuestMemMiB); | ||
| // callers must not fall back to the unreduced size, which is the mismatch this | ||
| // translation exists to avoid. | ||
| func (s *AteomService) guestSize(sz sizing.SandboxSize) (sizing.SandboxSize, error) { |
There was a problem hiding this comment.
🤖 question 🟢 – Memory gets a VMM reserve, a guest floor and an admission check; CPU gets none of that, so the same declared number means different things on the two runtimes.
On gVisor the sentry is the workload and sits in the same cgroup leaf, so a 2000m limit covers the sandbox too. On the micro-VM the guest cgroup is capped at 2000m, but cloud-hypervisor's vCPU threads, virtiofsd and ateom are host processes sharing the worker pod's CPU quota alongside it — the same reason memory needs vmmMemReserveMiB. The scheduler compares the declared limit against worker capacity with >=, so an actor may be placed with the whole worker's CPU and leave nothing for the VMM.
CPU is compressible, so this throttles rather than fails, which is presumably why there is no reserve. Worth saying so in this comment though — "passes through unchanged" reads as "the workload gets what it asked for", and on this runtime it gets somewhat less.
There was a problem hiding this comment.
[we should defer this to follow-up but I think it probably needs considering]
There was a problem hiding this comment.
Updated the comment.
|
this PR might be affecting e2e? |
63b6f6f to
d55a461
Compare
- gvisor: restore missing -root flag in runsc cmdCreate - microvm: size guest container memory cgroup to post-reserve guest RAM - actortemplate: reject requests and claims via CEL validation rules - demos: simplify WorkerPool template resources to limits only - proto: restore missing comment line on golden_snapshot_uri in ateom.proto
microvm: guestSize returns an error instead of the unreduced size when resolveGuestMemMiB fails. restoreFullScope now checks it, so raising --vmm-mem-reserve-mib after an actor was snapshotted fails loudly rather than pairing the guest with a container cgroup limit larger than the VM's RAM. coldBootActor calls the helper instead of reimplementing the same translation inline, and TestGuestSize covers the error path. Document in guestSize why the per-container cgroup limit is the whole guest RAM: the VM's RAM is the real envelope and the cgroup is belt-and-braces, so the guest OOM killer fires just before the cgroup limit binds and multi-container actors are not bounded relative to each other. ateapi: group the worker capacity fields into a WorkerCapacity message (cpu_milli, memory_bytes), matching the field naming used for limits in ateom.proto and atelet.proto.
Actors previously ran in sandboxes sized to the whole node; there was no way to declare how much CPU/memory a given actor should get. This adds an explicit, immutable sizing knob on the ActorTemplate and plumbs it all the way into the sandbox's OCI spec, for both the gVisor and micro-VM runtimes. - API: ActorTemplate.spec.resources (*corev1.ResourceRequirements). The Limits size the sandbox and are baked into the immutable spec; the CRD and generated code are regenerated accordingly. - internal/sizing: new SandboxSize value (FromLimits / VCPUs / ApplyToOCISpec). ApplyToOCISpec writes CPU quota+period and the memory limit onto the OCI spec and is a no-op when neither dimension is set, so 0 means "unconstrained". - Plumbing: ateapi reads the template limits (actorResourceLimits → tmpl.Spec.Resources), supplies CpuMilli/MemoryBytes over the actor RPCs (ateapi → atelet → ateom), and ateom applies them — gVisor via the cgroup leaf (runsc --cpu-num-from-quota provisions the sentry vCPU count), micro-VM via the guest spec. Proto messages carry the two fields. - Scheduling: worker capacity is taken from the WorkerPool's per-worker limits and the scheduler only places an actor on a worker whose capacity >= the actor's declared limits; a missing worker or actor dimension is treated as unconstrained so placement is never blocked by absent data. - Tests: unit tests for sizing and scheduling, plus an e2e suite (internal/e2e/suites/sizing) that resumes an actor and asserts, via the probe fixture's new /resources endpoint, that the running sandbox observes the declared CPU/memory from the inside. - Docs & demos: document the model in api-guide.md; the counter, sandbox, and micro-VM demos declare actor limits and their WorkerPool comments now describe the real model (worker limits size the worker pod + advertise capacity; the sandbox is sized by ActorTemplate.spec.resources).
Fail cold boot with a clear error (naming the declared limit, the reserve, and the guest minimum) instead of silently falling back to the larger kata default when the VMM reserve leaves too little guest RAM to boot. Add a matching admission floor on ActorTemplate.spec.resources.limits.memory (>= VMM reserve + guest minimum) for sandboxClass microvm so the case is caught at create time, before scheduling.
- gvisor: restore missing -root flag in runsc cmdCreate - microvm: size guest container memory cgroup to post-reserve guest RAM - actortemplate: reject requests and claims via CEL validation rules - demos: simplify WorkerPool template resources to limits only - proto: restore missing comment line on golden_snapshot_uri in ateom.proto
microvm: guestSize returns an error instead of the unreduced size when resolveGuestMemMiB fails. restoreFullScope now checks it, so raising --vmm-mem-reserve-mib after an actor was snapshotted fails loudly rather than pairing the guest with a container cgroup limit larger than the VM's RAM. coldBootActor calls the helper instead of reimplementing the same translation inline, and TestGuestSize covers the error path. Document in guestSize why the per-container cgroup limit is the whole guest RAM: the VM's RAM is the real envelope and the cgroup is belt-and-braces, so the guest OOM killer fires just before the cgroup limit binds and multi-container actors are not bounded relative to each other. ateapi: group the worker capacity fields into a WorkerCapacity message (cpu_milli, memory_bytes), matching the field naming used for limits in ateom.proto and atelet.proto.
…lable guestSize's "CPU passes through unchanged" was about the number, not about what the workload gets. The declared limit reaches the guest cgroup intact, but cloud-hypervisor's vCPU threads, virtiofsd and ateom are host processes drawing on the same worker-pod CPU quota, and the scheduler's capacity check (>=) sets none of it aside, so the host throttles the guest before its own quota ever binds. Say so, along with why the asymmetry with memory is deliberate: an unreduced memory limit takes the pod past its own limit and gets it OOM-killed, whereas CPU is compressible and the shortfall only costs throughput. A CPU reserve is left for a follow-up. The demo worker pools declared limits and no requests, so Kubernetes copied the limits into the requests: the counter pool asked for 5 CPUs and the micro-VM pool for 4 more, against the single 4-vCPU kind node the e2e job runs on, which leaves worker pods Pending. Capacity is advertised from the limits, so a request below the limit changes nothing an actor can see: give each pool a CPU request well under its limit, and leave the memory request at the limit because that memory is really occupied.
The sized probe fixture set spec.pauseImage on its ActorTemplate, but agent-substrate#848 moved that field to SandboxConfig. The CRD decodes strictly, so `ko apply` failed with strict decoding error: unknown field "spec.pauseImage" and the sizing suite died in deploySizedProbe before it ever asserted anything. The pause image comes from the SandboxConfig the WorkerPool resolves, exactly as it does for the plain probe fixture next door, so the field is simply dropped rather than moved. While here, give the sized probe the same readyz gate the plain probe has: the suite makes a single un-retried GET /resources after resuming, so the golden snapshot must be taken with the listener already up.
1655546 to
88df94e
Compare
71b0063
into
agent-substrate:main
3 participants
Actors previously ran in sandboxes sized to the whole node; there was no way to declare how much CPU/memory a given actor should get. This adds an explicit, immutable sizing knob on the ActorTemplate and plumbs it into the sandbox's OCI spec for both the gVisor and micro-VM runtimes.
API —
ActorTemplate.spec.resources(*corev1.ResourceRequirements). Thelimitssize the sandbox and are baked into the immutable spec; the CRD and generated code are regenerated accordingly.internal/sizing (shared by both runtimes) — new
SandboxSizevalue (FromLimits/VCPUs/ApplyToOCISpec).ApplyToOCISpecwrites CPU quota+period and the memory limit onto the OCI spec, and is a no-op when neither dimension is set, so 0 means "unconstrained".VCPUsrounds milliCPU up to wholevCPUs.
Plumbing — ateapi reads the template limits (
actorResourceLimits→tmpl.Spec.Resources) and suppliesCpuMilli/MemoryBytesover the actor RPCs (ateapi → atelet → ateom); ateom applies them — gVisor via the cgroup leaf (runsc --cpu-num-from-quotaprovisions the sentry vCPU count), micro-VM via the guest VmConfig. The two fields are carried on the proto messages.Scheduling — worker capacity is taken from the WorkerPool's per-worker limits and advertised on the Worker; the scheduler only places an actor on a worker whose capacity >= the actor's declared limits. A missing worker or actor
dimension is treated as unconstrained, so placement is never blocked by absent data.
Docs & demos — document the model in
api-guide.md; the counter, sandbox, and micro-VM demos declare actor limits, and their WorkerPool comments now describe the real model (worker limits size the worker pod + advertise scheduling capacity; the sandbox itself is sized byActorTemplate.spec.resources).internal/e2e/suites/sizingresumes an actor and asserts, via the probe fixture's/resourcesendpoint, that the running sandbox observes the declared CPU/memory from the inside)