atelet: pre-download sandbox assets from SandboxConfigs - #1358
Conversation
Sandbox assets were fetched inside the first Run/Restore on a node, putting the gVisor release download+extract on that actor's critical path. atelet now watches SandboxConfig objects and pre-downloads their assets for the node's architecture in the background: gvisor configs on every node, microvm configs only where /dev/kvm exists (the same signal the device plugin advertises, and known at startup before any WorkerPool schedules here). Downloads are jittered by up to 30s so a config rollout does not hit the bucket from every node at once. Prewarming is best-effort and shares the content-addressed cache with the on-demand fetch, which remains the correctness path; racing the two is safe because both install files via atomic rename. Retry-with-backoff on prewarm failure and GC of unreferenced cached assets are left as TODOs. The atelet ClusterRole gains get/list/watch on sandboxconfigs.
Taking a note ... this is yet another place that hardcodes knowledge of the two classes :( I think this factoring is going to prove badly later. Also ... right now we run atelet on all nodes. So even nodes that never schedule an actor will start downloading these ..? |
We need run atelet only on the nodes where we are planning to run workloads. I did not want to add dependency on the worker, since I wanted to download images ASAP. google/gvisor#14528 been merged today, so tomorrow we will update sandboxConfig parameters to use zstd images, so the first run will go down to 2sec from 23 sec. In addition to it, based on #811 (comment), the size of gVisor image will be shrink even more, so might be all the pre-warming for gVisor will not be required. On the other hand, pre-warming non gVisor images might be still usefull, since they are large. |
Yeah, I'm just a little worried about space usage. I have some ideas about how we can get that down, but we will need somewhere to host those scripts and pre-built artifacts. |
The sandbox asset prewarmer downloaded the runtime binaries but not the SandboxConfig's pause image, so every node pulled it from the registry inside its first Run/Restore. At benchmark start that is a synchronized fleet-wide pull burst — registry.k8s.io answered a 1000-node run with 429s, failing Restores — and a failed pull writes no cache record, so each subsequent Restore on the node re-pinged the registry and fed the rate limiter. The prewarm worker now pulls the pause image into the image cache concurrently with the asset downloads (they live in different backends, so neither fetch waits on or fails the other), inheriting the existing per-config jitter that spreads the fleet's pulls after a config rollout. Prewarming remains best-effort: the pull inside prepareOCIBundles is still the correctness path.
| // exist; workers are handed the real host paths, which kubelet resolves. | ||
| const hostDevRoot = "/host/dev" | ||
|
|
||
| // microvmNodeCapable reports whether this node can host micro-VM workers: |
| // Pre-download sandbox assets as SandboxConfigs appear/change so the first | ||
| // Run/Restore on this node hits the cache. Best-effort: on failure the | ||
| // on-demand fetch in ensureSandboxAssets still covers correctness. | ||
| if err := startSandboxAssetPrewarm(ctx, sandboxConfigInformer, wmService, microvmNodeCapable(hostDevRoot)); err != nil { |
There was a problem hiding this comment.
Putting the SandboxConfigs informer on ateFactory before Start makes WaitForCacheSync block forever if list/watch is Forbidden (RBAC skew during rollout).
This goes from a best effort optimization to hanging the node ...?
There was a problem hiding this comment.
Fixed in 3d7654c: the SandboxConfigs informer is now requested only after the factory's blocking WaitForCacheSync, and started with a second factory Start call (which launches only informers added since the first). A Forbidden list/watch during RBAC skew now just leaves prewarm cold while the reflector retries in the background — atelet startup no longer waits on it. A watch error handler logs the degradation explicitly.
| if rec.PauseImage == "" { | ||
| return | ||
| } | ||
| if _, err := p.herder.imageCache.EnsureImage(ctx, rec.PauseImage); err != nil { |
There was a problem hiding this comment.
this is running on main's ctxt.Background() which is never cancelled that seems bad? we should probably put some kind of timeout on this
There was a problem hiding this comment.
Fixed in 8ec2d95: prewarm now derives a 5-minute timeout context per attempt, so a download that stalls without failing can't wedge the worker. A timed-out attempt goes through the same requeue-with-backoff path as any failure. Added a test with a hanging bucket read verifying the attempt returns.
| // content-addressed files via atomic rename, and the image cache collapses | ||
| // concurrent pulls of one digest. | ||
| func (p *sandboxPrewarmer) prewarm(ctx context.Context, cfg *v1alpha1.SandboxConfig) error { | ||
| rec, err := recordFromSandboxConfig(cfg) |
There was a problem hiding this comment.
if this fails, we still fail to warm pause, which doesn't need the arch => config lookup
There was a problem hiding this comment.
Fixed in 8ec2d95: the pause image pull no longer depends on recordFromSandboxConfig — it's scheduled first and runs even when the config has no assets for the node's architecture. The projection error is still returned so the failure stays visible. Test added for the no-assets-for-arch case.
| } | ||
| } | ||
|
|
||
| func (p *sandboxPrewarmer) run(ctx context.Context) { |
There was a problem hiding this comment.
we wind up doing this in a single worker with no concurrency or timeouts, so one failed config will hang the rest
imagine one config has a bad pause image reference, we'll never be able to pull it. and it will hang forever
There was a problem hiding this comment.
Fixed in 8ec2d95 (plus the workqueue in 224f35a): each attempt now runs under a 5-minute timeout, so a config with an unpullable pause image costs the worker at most 5 minutes before it requeues with backoff and moves on. Other configs process between its retries, and after 8 failed attempts it's dropped until the next config event. Kept the single worker deliberately so prewarms don't compete for node bandwidth — with the timeout it's no longer a liveness risk.
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-time.After(rand.N(prewarmMaxJitter)): |
There was a problem hiding this comment.
we don't dedupe configs, so a relist can cause this to stack additively?
There was a problem hiding this comment.
Fixed in 224f35a: the buffered channel is replaced with a rate-limited workqueue keyed by config name, so relists and event bursts coalesce into one entry. The worker resolves the name through the informer's lister at processing time, so it always prewarms the latest revision. This also brought retry-with-backoff (1s–5min, 8 attempts) for free, closing the retry TODO.
| // fetch inside the first Run/Restore on the node is a cache hit instead of a | ||
| // download+extract on the critical path. | ||
| type sandboxPrewarmer struct { | ||
| herder *AteomHerder |
There was a problem hiding this comment.
this seems like kind of a weird touchpoint. we don't need most of it, right?
There was a problem hiding this comment.
Fixed in f7e3392: the prewarmer now takes a one-method sandboxAssetFetcher interface (still implemented by AteomHerder, so prewarm and the on-demand fetch can't diverge on cache layout) plus the image cache store directly, instead of the whole herder.
A relist or event burst used to stack duplicate SandboxConfigs in the buffered channel, each copy costing its own jitter wait and fetch pass. Queue config names in a rate-limited workqueue instead: duplicate events coalesce into one entry, the worker resolves the name to the config's latest revision through the informer's lister at processing time, and a failed prewarm retries with download-scale backoff (1s to 5min, 8 attempts) instead of staying cold until the next config event. The per-config jitter moves from a sleep inside the worker to the enqueue delay, so one config's jitter window no longer serializes unrelated configs, and duplicates arriving inside the window collapse too. The queue-full drop path is gone: the workqueue is unbounded but deduped, so a handful of cluster-scoped configs can never grow it.
A prewarm attempt ran on the daemon's never-cancelled context, so a download that hung without failing (a registry that accepts the connection and stalls, a wedged bucket read) would block the single prewarm worker — and with it every other queued config — forever. Each attempt now runs under a 5-minute timeout; a timed-out attempt requeues with backoff like any other failure, and other configs process between its retries. The pause image pull no longer depends on the per-architecture asset projection: it needs no arch lookup, so it is scheduled first and runs even when the config has no assets for the node's architecture. The projection error is still returned so the failure stays visible.
The prewarm informer was requested on the shared factory before its blocking WaitForCacheSync, so a failing list/watch — most plausibly Forbidden while the ClusterRole rollout lags the binary — would hang atelet startup forever, turning a best-effort optimization into a node outage. Request the informer only after WaitForCacheSync returns and start it with a second factory Start call, which launches only informers added since the first. A failing list/watch now degrades prewarm instead of blocking the node: the reflector retries in the background and prewarm recovers with it. A watch error handler names the degradation in the log instead of leaving only generic reflector noise. The prewarm handler is now registered before its informer starts; the initial List still replays every existing SandboxConfig into it as an Add, so a freshly booted node prewarms current configs as before.
The prewarmer took the whole AteomHerder while calling exactly two things on it: the asset fetch and the pause image pull. It now takes a one-method sandboxAssetFetcher interface — still implemented by AteomHerder, so prewarm and the on-demand Run/Restore fetch cannot diverge on cache layout — and the image cache store directly, which main already owns. The prewarmer's real dependencies are visible at the call site instead of implied by a service-wide handle.
microvmNodeCapable equates micro-VM capability with /dev/kvm presence. With /dev/mshv support proposed for AKS nodes, an mshv-only node would be wrongly reported incapable and skip prewarming micro-VM assets. Leave a TODO to treat presence of any micro-VM hypervisor device in SandboxDevices as capability once a second device exists.
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
three claude findings, given the time constraints, I think these are valid
| // schedule prewarm pause image if provided | ||
| if cfg.Spec.PauseImage != "" { | ||
| wg.Go(func() { | ||
| if _, err := p.images.EnsureImage(ctx, cfg.Spec.PauseImage); err != nil { |
There was a problem hiding this comment.
🤖 blocking 🔴 – Prewarm can fail a serving Restore.
EnsureImage collapses concurrent pulls of one digest, and the winning call's context governs the pull for every waiter (internal/imagecache/imagecache.go:368). Until now every caller was an RPC bounded by its own client. A prewarm that wins the slot imposes prewarmTimeout on a Restore waiting on the same digest, and its context is the daemon context, which cancels at SIGTERM while the gRPC server still drains for 5 minutes.
Fix in imagecache: run the singleflight body on a context detached from whichever caller won, bounded by its own pull timeout, so waiters only ever see real pull failures. The comment at imagecache.go:368 states the old assumption and needs updating either way.
There was a problem hiding this comment.
Fixed in 288edfb: the singleflight body now runs on a context detached from whichever caller starts the flight (context.WithoutCancel, so trace/log values survive), bounded by a store-level pull timeout (default 10m, WithPullTimeout to override). A waiter only ever sees a real pull failure — never another caller's cancellation, deadline, or the daemon context ending at SIGTERM. EnsureImage switched to DoChan, so each caller also stops waiting when its own context ends while the pull runs on to warm the cache for the next attempt (the prewarm worker therefore stays bounded by prewarmTimeout even when the underlying pull runs longer). The stale comment is rewritten to state the new contract, and tests cover both halves: a cancelled caller doesn't kill the pull for a later joiner, and a wedged detached pull is reclaimed by the pull timeout.
| return | ||
| } | ||
| if err == nil { | ||
| err = p.prewarm(ctx, cfg) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The class gate is enforced against a stale object.
enqueue filters on SandboxClass but queues a name. process re-resolves the current revision and prewarms whatever class it holds by then. sandboxClass is mutable, so a gvisor→microvm edit inside the jitter or backoff window makes a node without /dev/kvm download the micro-VM guest assets the gate exists to avoid.
Factor the switch in enqueue into a skipConfig(ctx, cfg) bool and call it here too, after the lister Get.
There was a problem hiding this comment.
Fixed in d785c6b: the switch is factored into skipConfig, applied both in enqueue and in process after the lister Get, so the gate holds for the revision actually prewarmed. A test covers the gvisor→microvm edit landing between enqueue and processing on a non-KVM node (skipped and forgotten, not fetched).
| arch := runtime.GOARCH | ||
| files := cfg.Spec.Assets[arch] | ||
| if len(files) == 0 { | ||
| return nil, fmt.Errorf("sandbox config %q has no assets for architecture %q", cfg.Name, arch) |
There was a problem hiding this comment.
🤖 nit 🟢 – A permanent error travelling the transient-retry path.
A config with no assets for this node's architecture will never grow them without a config update, and an update re-enqueues anyway. process cannot tell this apart from a failed download, so every node of a non-listed architecture burns prewarmMaxRetries rate-limited attempts with a warning each. Every attempt also redoes the pause-image pull that already succeeded, because prewarm joins both errors into one retry decision.
Return a sentinel error here so process can log at debug and Forget the config immediately.
There was a problem hiding this comment.
Fixed in 02616d5: recordFromSandboxConfig now wraps a sentinel errNoAssetsForArch, and prewarm treats it as nothing-to-do — logged at debug, pause image still pulled, config Forgetten immediately. One deviation from the suggestion: the sentinel is handled inside prewarm before the error join rather than in process, because errors.Is on the joined error would also match when only the pause pull failed transiently, silently dropping its retry.
… caller EnsureImage collapses concurrent pulls of one digest, and the winning call's context used to govern the pull for every waiter. Callers with very different lifetimes share these flights -- a serving Restore, a best-effort prewarm bounded by its own deadline, a caller whose daemon is draining after SIGTERM -- so a prewarm that won the slot could fail a Restore with its cancellation or timeout. The singleflight body now runs on a context that keeps the initiating caller's values but none of its cancellation, bounded by a store-level pull timeout (default 10m, WithPullTimeout to override). A waiter only ever sees a real pull failure; each caller stops waiting when its own context ends, while the pull runs on to warm the cache for the next attempt.
enqueue filters on SandboxClass but queues only the config name, and process re-resolves the current revision at processing time. sandboxClass is mutable, so a gvisor-to-microvm edit inside the jitter or backoff window made a node without /dev/kvm download the micro-VM guest assets the gate exists to avoid. The gate now lives in skipConfig, applied both at enqueue and after the lister Get in process.
…e arch A config with no assets for this node's architecture will not grow them without a config update, and an update re-enqueues anyway. process could not tell this apart from a failed download, so every node of a non-listed architecture burned all eight rate-limited attempts with a warning each -- and every attempt redid the pause-image pull that had already succeeded, because prewarm joins both errors into one retry decision. recordFromSandboxConfig now wraps a sentinel error, and prewarm treats it as nothing-to-do: logged at debug, pause image still pulled, and the worker Forgets the config immediately. The sentinel is handled before the error join rather than in process, because errors.Is on the joined error would also match when only the pause pull failed transiently, silently dropping its retry.
a9c1bd3
into
agent-substrate:main
2 participants
Summary
atelet now watches SandboxConfig objects and pre-downloads their sandbox assets into the node's content-addressed cache in the background, instead of only fetching them inside the first Run/Restore on the node.
Related to #811 — this does not fix it, it is one more optimization step toward it. It removes the cold-cache download+extract from the first Run/Restore in the common case (node boot, or a release bump while the node is idle), but the window between a SandboxConfig change and prewarm completion still exists, and the extraction itself remains on the critical path when a resume lands inside that window. The uncancellable/silent extraction called out in #811 is unchanged by this PR.
How it works
/dev/kvmexists (microvmNodeCapable, the same signal the device plugin advertises asate.dev/kvm) — known at atelet startup, before any WorkerPool schedules to the node, and avoids pulling multi-hundred-MiB guest images onto nodes that can never run them.ensureSandboxAssetsremains the correctness path. Racing the two is safe because both install content-addressed files via atomic rename.sandboxconfigs.Left as TODOs in the code: retry-with-backoff on prewarm failure, and GC of cached assets no longer referenced by any SandboxConfig or on-node actor record.
Pause image prewarm (cherry-picked)
This PR also carries
atelet: prewarm the pause image alongside sandbox assets(cherry-picked from 2c7dd31). The prewarmer downloaded the runtime binaries but not the SandboxConfig's pause image, so every node still pulled it from the registry inside its first Run/Restore. At benchmark start that is a synchronized fleet-wide pull burst — registry.k8s.io answered a 1000-node run with 429s, failing Restores — and a failed pull writes no cache record, so each subsequent Restore re-pinged the registry and fed the rate limiter. The prewarm worker now pulls the pause image into the image cache concurrently with the asset downloads (different backends, so neither fetch waits on or fails the other), inheriting the existing per-config jitter. It remains best-effort: the pull insideprepareOCIBundlesis still the correctness path.Testing
recordFromSandboxConfig), the enqueue filtering (KVM gating, unknown classes, full-queue drop), andmicrovmNodeCapablenegative cases.TestPrewarmPauseImage, pulling from a local test registry to verify the image lands in the image cache and that an asset-fetch failure does not stop the pull.go vetand the fullcmd/atelettest package pass.🤖 Generated with Claude Code