Repository navigation
multi actor worker support - #1836
Scott Funkenhauser (sfunkenhauser) merged 12 commits into
Conversation
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 Initial AI-generated review, reviewed by me.
Reviewed commit 3e5536e7692f99da7bfc27236d128bb8ba5bbf10.
-
Recheck activation identity before publishing discovery stats. Both microVM and gVisor retain attribution from the initial hosted-actor snapshot, but subsequently resolve the guest/cgroup by UID. If an actor suspends, switches to a compatible template, and resumes on the same worker during that window, the UID stays unchanged and the new activation's measurements receive the old template's labels. Changing the template reference while suspended is explicitly supported by UpdateActor. This can misattribute template aggregates and periodic usage events. Targeted probes reproduced this in both runtimes and passed with a hosted-record identity recheck before accepting each sample. Please restore that check and report a pending sample or skip the sample when the activation changed.
-
Stop virtiofsd before unhosting a failed full restore. The failure cleanup is registered after the virtiofsd kill/wait, so Go executes unhosting first. With delegated cgroups, if TAP setup or VMM launch fails while virtiofsd is waiting for its client, removing the populated cgroup fails with
EBUSY.unhostActorhas already deleted the actor record, so the outer cleanup skips retrying removal after virtiofsd exits. Failed restores can therefore leave cgroups behind. Please run unhosting after child-process teardown; the outerRestoreWorkloadfailure cleanup already runs afterrestoreFullScope's defers. This finding is based on code inspection and cgroup removal semantics, rather than a live cgroup reproduction.
Existing focused tests for the changed runtime, networking, capacity, and locking packages passed, as did race checks for actorlock, atunnel, and both runtimes. Live E2E, privileged cgroup checks, and full make verify were not run.
3e5536e to
7045e6c
Compare
ACK fixing.
There's a larger class of "clean up after previous failed $actoruid" that exists in main which I'd like to fix in a follow-up, I think that's going to be deserving of it's own careful review. Both gvisor and uVM have poor handling of this currently. |
4f097ff to
306402c
Compare
|
eh, after some cleanup passes it's not that bad, I'm going to fold in most of these fixes |
306402c to
c5f251b
Compare
c2d87dc to
8fc5c25
Compare
…t-substrate#1843) The sweep for a sandbox's leftover cloud-hypervisor and virtiofsd matched on binary names, but atelet stages both under content-addressed names, so it never killed anything and a failed boot left its VMM holding guest RAM. Match on the staged location instead. Split out from agent-substrate#1836 as a small pre-existing fix.
8fc5c25 to
59fecb8
Compare
haiyanmeng
left a comment
haiyanmeng
left a comment
There was a problem hiding this comment.
Benjamin Elder (@BenTheElder) , should we add a field into WorkerPool to configure max-actors?
Bowei Du (bowei)
added
kind/feature
…substrate#1847) Teardown killed the pause container, and with it the sandbox, before deleting the actor's other containers. The dead sentry stays a zombie until the ateom reaps it, which it defers while any runsc command runs, and runsc delete mistakes the zombie for a live sandbox and fails. Leave the pause container to cleanupContainers, which deletes it last. Fixes what is a rare flake on main, that can become more common in agent-substrate#1836
59fecb8 to
4fefa7e
Compare
Share cancelable, reference-counted locks between the two runtimes. Track in-flight RPCs for startup cancellation and shutdown draining.
Key ingress by actor reference and egress by actor UID. Keep proxy connection pools separate for each activation.
Track sessions, networks, and stats by actor UID. Serialize lifecycle RPCs per actor, enforce capacity, and drain all sessions on shutdown.
Track VMs, networks, and stats by actor UID. Serialize lifecycle RPCs per actor, enforce capacity, and drain all guests on shutdown.
Report each runtime's admission limit instead of a fixed capacity of one.
Verify placement, routing identity, and independent suspension on a single worker with either runtime.
Capture the activation before starting each sandbox listener so accepted connections cannot acquire credentials from a later activation of the same actor. Cancel listeners on deactivation and discard unused startup bindings.
Move the gVisor ateom's cgroup delegation into its own package so the micro-VM ateom can use it to give each actor a leaf.
Every actor's VMM and virtiofsd ran in the worker's pod cgroup, so one busy guest could starve the others. Delegate the pod cgroup as the gVisor ateom does and start each actor's processes directly in a leaf of its own, capped at the actor's declared CPU.
Before a boot or restore, and at teardown, kill everything in the actor's cgroup leaf and wait for it to empty. That reaches exactly the actor's processes, including any they forked, where the command-line sweep has to guess.
A Run or Restore for an actor whose VM is still running replaced its record and left the VM running untracked. Tear it down first.
A worker is no longer limited to one actor, so its capacity is a budget its actors share rather than a per-actor ceiling. Say so, and document the actor limit alongside it.
4fefa7e to
2ee007b
Compare
I don't think we should do that yet. The shape is there that we can add it if needed, but that limit isn't really meant to be load bearing yet. We need to rework workerpool more generally. |
…pending sweeps (agent-substrate#1885) Closes the remaining items in agent-substrate#1640: the atelet side of multi-actor telemetry. Since agent-substrate#1836 an ateom returns one `GetActiveWorkloadStats` entry per hosted actor. atelet's sweep already folded every entry, but nothing pinned the behavior at occupancy above one, and reviewing that fold surfaced three CPU-accounting defects: two in how baselines survive a pending sweep, one in the decrease rule. ## What changed **Tests pin the multi-actor fold.** One worker, several actors: same-template entries sum under one key, a different template gets its own, a pending sibling contributes nothing to aggregates or events, CPU baselines are per actor (independent advance, independent epoch reset), one probe serves the whole response. Each test was verified to fail against the corresponding mutation. **Three CPU-accounting defects found and fixed along the way:** 1. *Undercount.* A pending entry (boot, restore, or a micro-VM guest the ateom did not reach in its 45s budget) never wrote the actor's baseline, and `lastCPU` is rebuilt each sweep — so measured → pending → measured lost the whole interval. On the guest-agent source the counter survives a restore, so that was real CPU time dropped, and on a full micro-VM worker it would happen routinely. Pending now carries the baseline forward; only actors absent from every response are dropped. 2. *Overcount opened by fix 1.* Carrying the baseline unconditionally meant that during a restore spanning one sweep (source still reports the actor measured at C1, destination reports it pending), goroutine order could let the stale C0 overwrite C1, and the next sweep charged the interval twice. The pending branch now writes only if no worker has measured the actor this sweep. 3. *Overcount on a guest-agent decrease.* A CPU decrease was charged as the whole new value. That is right for a cgroup counter, which restarts at zero on restore. It is not always right for a guest-agent counter: a FULL or DATA_ON_GOLDEN restore resumes a guest at that guest's counter (after a golden restore, main billed the golden guest's CPU to the actor), a cold boot restarts it at zero, and a container exit lowers the sum. A sample cannot tell these apart, so a guest-agent decrease now charges nothing and re-baselines. A cgroup decrease still charges the new value, with or without a pending sweep between. **Trade-off, new versus main:** a guest-agent decrease loses the usage from the event to the first sample. Main counted it correctly only after a cold boot (a DATA restore, or a Run after a crash); in every other case (golden restore, FULL restore from an older snapshot, container exit) main overcounted it. Remaining limit, documented in the proto: an epoch that starts *above* the last reported value cannot be detected from consecutive samples. Main has this for guest-agent restores between sweeps; carrying the baseline extends it to restores that span a pending sweep. The overcount is bounded by the golden guest's counter minus the actor's last value, so it needs an actor that used less CPU than the golden guest had at snapshot. The test for (2) forces both fold orders deterministically: the sweep closes a probe's connection right after folding its response, so the fixture's closer releases the gated probe from `Close` — no sleeps, no polling. **Timeout rationale reconciled.** The poll floor (50s) is one actor's worst-case micro-VM read: 25 containers at 2s each. It never prevented overlap, because sweeps are sequential; it bounds guest-agent load, and the comment now says so. The RPC timeout (55s) was documented as covering that single-actor worst case. With several actors per worker, the micro-VM ateom caps its own read at `statsSweepBudget` (45s, chosen to stay under atelet's 55s) and reports unreached guests as pending, so the timeout stays a constant; its comment now points at that budget. ## Not in scope, recorded - Two workers both *measuring* one actor in one sweep double-charges. Pre-existing and unchanged here. It is rare: it needs the same probe skew as defect 2, with the destination read after its restore finished. - `ateom.proto` still says the discovery list holds "at most one entry until multi-actor workers land". Stale; fixed in a separate change. ## Verification `gofmt`, `go vet`, and race-enabled tests pass. Mutation checks: dropping the carry, the measured-wins guard, or either side of the per-source decrease rule each fails its test.
…bstrate#1915) ### Summary With multi-actor worker support (agent-substrate#1836) enabled by default (`--max-actors=1000`), selecting a single worker uniformly at random across all candidates with remaining capacity can stack actors onto busy workers while lighter workers exist, whereas strict global least-loaded selection over the asynchronous watch cache can cause concurrent bursts to herd onto the same worker. This change updates `Scheduler.Schedule` to use **power of two choices**: sample two distinct eligible candidates that satisfy `HasRoom` and pick the less-loaded one, comparing actor-slot utilization (`Allocated.Actors / Capacity.Actors`) first and breaking ties on dominant compute-resource utilization (CPU/memory). ### Testing - Added unit tests covering power-of-two-choices ordering, heterogeneous worker actor-capacity utilization, compute-resource utilization tie-breaking, fallback when an idle worker lacks resource room, and tie-breaking reachability in `cmd/ateapi/internal/scheduling/scheduling_test.go`. - [x] Tests pass - [ ] Appropriate changes to documentation are included in the PR
Fixes #1266
Builds on #1689 #1283 in particular.
Outstanding:
- There are some resource management / cleanup bugs that are pre-existing. I'm trying to keep this PR size down but do plan to submit fixes. Both gVisor and uVM workers need improvements to dealing with hanging sandbox processes from previous actors. This is more concerning with multi-actor but not new.EDIT: