Repository navigation
atelet: prewarm each worker's default sandbox runtime as the syncer plans it - #2741
Conversation
630bdbe to
7e4e66f
Compare
| // hash, once the worker's call has returned. A no-op in production; a var so | ||
| // a test can wait for the worker's own prewarm call to return before it tears | ||
| // down what that call writes into. | ||
| var prewarmFinished = func(string, error) {} |
There was a problem hiding this comment.
what is this empty function for?
There was a problem hiding this comment.
It was the production no-op behind a test seam: a test replaced it to learn when the worker's prewarm call had returned, so it could wait for that before the TempDir cleanup. As of 0b5ee90 it is a nil-checked onFinished field on the prewarmer, so there is no empty function any more and the field's doc says what it is for.
| if s.prewarm != nil { | ||
| for _, v := range hashed { | ||
| if v.isDefault { | ||
| s.prewarm(v.hash) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Enqueue the default hash right after registerRuntimes (line 500), not after the write. enqueue is a non-blocking queue.Add, so the file still never waits on a download, and the hash is already in the index at that point.
As written, the prewarm starts only after buildFile has finished probing every restorable version and the file is written. That file is what lets the worker register its default with the control plane. So the first Run can arrive while the prewarm is barely queued, and it pays the pause image pull (and, for microVM, every asset) itself. Enqueueing first gives the prewarm the whole probe duration as a head start. On a fresh node that duration is the longest, because each restorable version's runsc is fetched serially.
There was a problem hiding this comment.
Done in 9443532: the hand-off follows registerRuntimes, so the build's serial fetch-and-probe is the prewarm's head start and the file still never waits on a download. TestSyncPodPrewarmsDefaultRuntime now fails if the build fetches for a probe before the default was handed over, and checks a build that fails on the default's probe still handed it over. One consequence: a build that fails permanently triggers one prewarm of a default it never registers, at most one bounded fetch of assets the config names; I left that rather than gate the enqueue on the build.
| // A download that hangs would hold a sync worker; prewarmAttemptTimeout | ||
| // bounds the fetch for the same reason it bounds prewarming. | ||
| ctx, cancel := context.WithTimeout(ctx, prewarmAttemptTimeout) |
There was a problem hiding this comment.
🤖 nit 🟢 – This comment no longer matches the code. The new prewarmAttemptTimeout doc says asset downloads are bounded by the asset cache's own fetch timeout (--sandbox-asset-fetch-timeout, 5m), so this context does not bound the fetch. What it bounds now is the runsc CPU-features exec. The 5m to 10m bump for the prewarmer also lets a hung probe hold a sync worker twice as long. Consider giving the probe its own constant and keeping it at 5m, or updating the comment to say what it actually bounds.
There was a problem hiding this comment.
Done in 0b5ee90: the probe has its own probeFetchTimeout of five minutes, what it had before this PR, and the comment now says what the context covers: the asset cache bounds each download with its own fetch timeout and the exec has cpuFeaturesTimeout, so this is the backstop around them, such as waiting on a download another caller started. The prewarmer keeps its ten minutes for itself.
| defer func() { _ = os.RemoveAll(scratch) }() | ||
| _, assetErr = p.assets.ensureSandboxAssets(ctx, rt.rec, scratch) | ||
| }) | ||
| if rt.rec.PauseImage != "" { |
| prewarmFinished(hash, err) | ||
| if err == nil { | ||
| p.queue.Forget(hash) | ||
| return |
There was a problem hiding this comment.
🤖 should-fix 🟡 – queue only deduplicates hash while it waits in dirty before Get() pops it. Any pod that syncs while the first prewarm is running re-marks hash dirty, and every pod that syncs after Done(hash) (for example across enqueuePool's 30s jitter window, on warm restarts, or on errIncomplete retries) queues hash again, re-staging the cached assets into a scratch dir and logging "Sandbox runtime prewarmed" at INFO once per pod. Since hash is content-addressed, record hashes that have succeeded and skip them in enqueue or process.
There was a problem hiding this comment.
Done in 9443532: the prewarmer keeps the hashes whose prewarm succeeded and enqueue skips them; a failed one is still queued again. TestRuntimePrewarmOnce covers both. In the eviction PR the prune of a pin drops its hash from that set, so a runtime that returns as a worker's default is prewarmed and pinned again.
| // runtime's prewarm. The asset downloads are bounded by the asset cache's | ||
| // own fetch timeout; this covers the pause image pull beside them. A var so | ||
| // tests can shorten it. | ||
| var prewarmAttemptTimeout = 10 * time.Minute |
There was a problem hiding this comment.
🤖 nit 🟢 – Make prewarmAttemptTimeout and prewarmFinished fields on runtimePrewarmer initialized in newRuntimePrewarmer (matching workerSandboxSyncer's maxJitter, scratchDir, and probe hooks in cmd/atelet/worker_sandbox_syncer.go:114). That avoids mutating package globals in tests and drops the cleanup-ordering constraint in TestDefaultRuntimePrewarmThroughSyncer.
There was a problem hiding this comment.
Done in 0b5ee90: attemptTimeout and onFinished are fields set in newRuntimePrewarmer, onFinished nil in production. The timeout test shortens its own instance, and the end-to-end test assigns its hook to the instance; its cleanup still joins the worker, but only so the pull's last writes do not race the TempDir removal.
7e4e66f to
36a79c9
Compare
…lans it The prewarmer fetched the default version of every SandboxConfig in the cluster, keyed by config name and fed by a cluster-wide informer, with a KVM check standing in for whether the node could run micro-VM configs at all. Most of what it fetched no worker on the node would ever use, and nothing tied what it fetched to what the node's workers advertise. The worker sandbox syncer now hands the default runtime hash of each worker it plans to a prewarmer, right after writing the worker's versions file, so the file never waits on a download and the workers of one pool, and pools sharing one runtime, cost one fetch. The prewarmer takes the record from the runtime index, stages it into a scratch directory through the same path Run uses, removes the scratch directory, and pulls the pause image alongside. Failures retry with backoff and are otherwise left to the on-demand fetch, which stays the correctness path. Locality comes from the pods on the node, so the KVM check goes. Only default runtimes are prewarmed: the first restore of another version a worker advertises pays its download.
…d prewarm each runtime once The syncer handed a worker's default runtime to the prewarmer after writing the worker's file. The build before the write fetches and probes every restorable version serially, longest on a fresh node, and the file is what lets the worker take its first actor, so the first Run could arrive with the prewarm barely queued. The hand-off now follows the registration of the hashes, which is all the prewarmer needs, so the build is the prewarm's head start and the file still never waits on a download. Every pod of a pool, every re-plan and every retry of a sync hands the same default over, and the queue forgets an item once it is done, so a runtime was re-staged and logged once per pod. The prewarmer now keeps the hashes whose prewarm succeeded and skips them, which the content-addressed hashes make safe; a failed one is still queued again.
…own knobs The syncer's runsc probe borrowed the prewarmer's attempt timeout and said it bounded the download, which the asset cache's fetch timeout does; what it bounds is everything around the download and the exec, and a stuck probe held a sync worker for ten minutes where it used to be five. The probe has its own five-minute backstop again, with a comment that says what it covers. The prewarmer's attempt timeout and the hook a test uses to learn when a prewarm attempt returned were package variables that tests mutated and restored in a fixed order. They are fields set in the constructor now, like the syncer's probe hooks: a test configures its own instance, and nothing in production reads a global.
0b5ee90 to
befe747
Compare
Step 4 of #2675: the default runtime of each worker on the node is prewarmed by the syncer that plans the worker, through the runtime index, and the cluster-wide prewarmer goes away.
Before.
sandbox_prewarm.gowatched every SandboxConfig in the cluster, queued by config name, and fetched each config's default version, gated only by whether the node had/dev/kvm. Most of what it fetched no worker on the node would ever run, and nothing related it to what the node's workers advertise.After. The worker sandbox syncer already plans each local worker pod, registers its runtimes in the index, and writes its versions file. It now also hands the worker's default runtime hash to a
runtimePrewarmerright after registering the worker's hashes in the index, before the build probes the versions: the build fetches and probes every restorable version serially, longest on a fresh node, and the file it ends with is what lets the worker take its first actor, so the probes are the prewarm's head start and the file never waits on a download. The prewarmer's queue is keyed by hash, and the prewarmer keeps the hashes whose prewarm succeeded and skips them, so the workers of one pool, every re-plan and every retry of a sync cost one prewarm of a runtime, not one per pod; a failed prewarm is queued again. Its single worker takes the record from the index, stages it into a scratch directory under the base path through the sameensureSandboxAssetsRun uses, removes the scratch directory, and pulls the pause image alongside. Eight retries with exponential backoff, a ten-minute bound per attempt (a field on the prewarmer, like the hook a test uses to wait for an attempt), and an unknown hash is dropped with an error log since the syncer registers before it enqueues. The syncer's runsc probe keeps its own five-minute backstop rather than borrowing the prewarmer's. Prewarm stays a latency optimization: every failure is logged and left to the on-demand fetch.Removed.
sandbox_prewarm.go, its tests, andmicrovmNodeCapable, which only it used. Locality comes from the pods on the node: a micro-VM pod only lands where it can run.Scope. Only default runtimes are prewarmed. The first restore of a non-default version a worker advertises pays its download.
Tests.
TestRuntimePrewarmProcess(retry until the cap, then forget; unknown hash forgotten at once),TestRuntimePrewarmFillsBothCaches(asset in the cache, scratch dir empty, pause image in the image cache),TestRuntimePrewarmAssetFailureStillPullsImage,TestRuntimePrewarmAttemptTimeout,TestSyncPodPrewarmsDefaultRuntime(exactly the default hash of a multi-version config, handed over before the build fetches for a probe, and still handed over when the build then fails),TestRuntimePrewarmOnce(a prewarmed hash is skipped by later enqueues, a failed one is queued again), andTestDefaultRuntimePrewarmThroughSyncer, the end-to-end run through fake clientsets and informers that the old download test did. The old enqueue-filter, class-gate, and KVM tests go with the behavior they covered.🤖 Generated with Claude Code