Repository navigation
atelet: resolve an actor's sandbox assets from the runtimes its workers advertise - #2731
Conversation
3fc1c4e to
878b6fd
Compare
878b6fd to
4e46c75
Compare
4e46c75 to
a821c3a
Compare
a821c3a to
7470b48
Compare
| } | ||
|
|
||
| func (s *workerSandboxSyncer) removeDir(ctx context.Context, uid string) error { | ||
| s.index.forget(uid) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – removeDir calls s.index.forget(uid), but no test in cmd/atelet/worker_sandbox_syncer_test.go verifies that a deleted pod is removed from s.index.worker(uid) when processed by the syncer. Assert in TestWorkerSandboxSyncerProcess (or TestStartWorkerSandboxSyncer) that s.index.worker(uid) returns false after the pod directory is removed.
There was a problem hiding this comment.
Done in 8316c93: the process test asserts the index entry's fate in every case: gone with the pod, kept while the pod is only being deleted or its sync failed before the write, present after each written file.
| } | ||
| for _, pod := range pods { | ||
| if maxDelay > 0 { | ||
| s.registerAhead(pod) |
There was a problem hiding this comment.
🤖 nit 🟢 – Every pod in pods belongs to pool, so calling s.registerAhead(pod) inside the loop repeats identical planning, hashing, and lock acquisition for every local pod in the pool. Call s.registerAhead(pods[0]) once before the loop when maxDelay > 0 && len(pods) > 0.
There was a problem hiding this comment.
Done in df4dc0b: the resolver plans a pool directly (planPool) and enqueuePool registers once per pool before the loop, skipping a pool with no local pods. The ahead-of-jitter test runs with two pods and checks both are queued after the delay.
| // from eviction comes from. | ||
| // | ||
| // TODO: release runtimes that no pod has advertised for a long time. | ||
| type runtimeIndex struct { |
There was a problem hiding this comment.
🤖 question 🟢 – Could this index be one file per runtime instead of an in-memory map? The syncer would write /var/lib/ate/sandbox-runtimes/<runtimeHash>.json atomically before it writes the worker's versions file, in place of registerRuntimes. Run and Restore would read the file the request's hash names.
The file holds what the hash cannot be inverted to, which is the record the hash was computed from:
{
"sandboxClass": "gvisor",
"pauseImage": "registry.k8s.io/pause@sha256:…",
"assets": {
"gvisor": {"url": "gs://…/gvisor.tar.zstd", "sha256": "…"}
}
}That is the sandboxAssetsRecord shape already written to sandbox-assets.json, without the actor and snapshot fields. Run can recompute versionRuntimeHash on the record it reads and refuse a mismatch. Assets still come from the per-asset cache on demand, so nothing is prefetched and eviction is unchanged.
Because the files survive an atelet restart, every runtime a worker registered before the restart still resolves. The startup gate could then be removed entirely: InitialSync, markPending/markAttempted, waitForWorkerRuntimes, and the worker-sandbox-runtimes readiness check. This also removes the miss for a runtime that a SandboxConfig change dropped while atelet was down. The never-release semantics and the GC TODO stay as they are.
There was a problem hiding this comment.
Done in 094ed5f, with two differences from the proposal: one file (sandbox-runtime-index.json, the map serialized) rather than one per hash, and memory stays the read path with the file as write-through persistence, so Run and Restore do no file IO. A node sees a handful of configs with a few versions in a month-long life, so per-entry isolation buys nothing and the single file is easier to read; each entry's record is still checked against its key on load, and a file that does not decode starts the index empty rather than failing startup. The file is written durably (atomicfile.WriteSync, data then directory) before the entry enters memory, which is before the worker's file, so every hash a worker registered before a restart is on disk. As you predicted, that removes the startup gate entirely: InitialSync, the wait before listening and the readiness check are gone, and with them the Checkpoint/Terminate trade-off.
| // while a list/watch fails (e.g. Forbidden while an RBAC rollout lags the | ||
| // binary), files wait and existing ones stay as they are. | ||
| func startWorkerSandboxSyncer(ctx context.Context, pods coreinformers.PodInformer, workerPools informersv1alpha1.WorkerPoolInformer, configs informersv1alpha1.SandboxConfigInformer, assets sandboxAssetFetcher, workersDir string) (*workerSandboxSyncer, error) { | ||
| // The syncer never fails atelet startup, but it does hold the serving port: |
There was a problem hiding this comment.
🤖 should-fix 🟡 – "The syncer never fails atelet startup" is no longer true: cmd/atelet/main.go:417-419 now calls serverboot.Fatal when startWorkerSandboxSyncer returns an error, and the commit message says the same. Update this sentence to match.
| t.Run("caches not synced", func(t *testing.T) { | ||
| s := newTestSyncer(t) | ||
| runSyncer(t, s, false) | ||
| time.Sleep(300 * time.Millisecond) // cache.WaitForCacheSync polls every 100ms |
There was a problem hiding this comment.
🤖 nit 🟢 – This negative assertion relies on a fixed sleep and on WaitForCacheSync's 100ms polling interval. It cannot fail spuriously, but it adds 300ms to every run and would silently stop testing anything if the poll interval changed. Driving synced from a channel the test closes, and asserting before and after, would make it deterministic.
…rs advertise Run and Restore took the sandbox assets to stage straight from the request. Nothing on the node checked that the runtime they named was one a worker there could run, and nothing tied what a worker registered with the control plane, the runtime hashes in its versions file, to what atelet then staged for an actor placed on it. The worker sandbox syncer now keeps an index of the runtimes this node's workers advertise. Each time it plans a worker pod it registers every hashed version, with the asset record the hash was computed from, before it writes the versions file, so a hash is resolvable before any worker can register it; what the worker advertises is recorded only once the file is written, and only for the runtimes the file names, so the index never credits a worker with a default its file does not carry. Runtimes are kept by hash and never released: a request may still carry a hash a worker advertised earlier, config churn is rare, and an entry is a few URLs. What each worker currently advertises is kept by pod UID and by sandbox class, one class per pool today, shaped for the several a pool will be allowed to run. A SandboxConfig or WorkerPool change rewrites the files after a jitter of up to thirty seconds, so the fleet does not re-register at once. The index does not wait for that: the event handler plans the affected pods and registers their runtimes immediately, which reads the listers only. A request for the new default that reaches the node inside the jitter resolves, and what each worker advertises still changes only with its file. Run and Restore resolve through the index: the request's assets are hashed with the same function the syncer uses, and the index's record is staged. A miss fails the request. It means the control plane placed the actor on a worker whose pool does not run that config or version, or the request named a runtime from before an atelet restart that a config change removed meanwhile, and both must surface rather than be served from whatever the request describes. The hashing of the request is a stopgap until the requests carry the hash itself. After a restart the index is empty until the syncer has indexed the workers already on the node, and those workers are registered with the control plane, which may send actors the moment atelet is reachable. So the syncer reports when every pod present at its first cache sync has been processed once, and atelet opens its port only then, with a readiness check that says why until it does. A shutdown that arrives while it waits skips serving instead of opening the port into a drain. A syncer that cannot start is now fatal: without it no actor could ever run.
…rst record /readyz answered 200 from the moment the health server started until the worker runtimes check was registered, which is after the trust bundle sync, the plugin socket wait and the cache open, and then 503 until the gate opened. A check registered with the readiness object now fails until main has opened the gRPC port, so startup reads as not ready throughout. The worker runtimes check stays for its reason in the logs. Two versions with one runtime hash have the same asset names and digests and the same pause image, so they differ at most in where identical bytes are fetched from. The index kept the record registered last; it now keeps the first, which is the version the worker's file was built from. The process test also asserts the index entry's fate in every case: gone with the pod, kept while the pod is only being deleted or its sync failed before the write, present after every written file. The syncer doc now tells a syncer that cannot start, which is fatal, from one that cannot sync, which holds the port.
…-sync test on a channel A SandboxConfig or WorkerPool change registered the pool's runtimes once per local pod before enqueueing each with its jitter, though every pod of a pool plans the same config. The resolver now plans a pool directly, and the handler registers once per pool, skipping a pool with no local workers. The initial-sync test proved that the gate stays closed before the caches sync by sleeping past the cache sync's poll interval. It now drives the sync from a channel it closes, so the assertion before the close cannot pass by timing and the test no longer depends on that interval.
The index of the runtimes this node's workers advertise lived in memory only, so after a restart atelet held its port closed until the syncer had processed every worker pod already on the node, probing included; the readiness check worker-sandbox-runtimes said why. Checkpoint and Terminate for actors already on the node waited behind that too. The index is now also one file under the node state dir, the map serialized, rewritten durably before a new runtime enters memory. A worker registers a hash with the control plane only after atelet wrote the worker's own file, which happens after the index was written, so every hash a worker registered before a restart is on disk and loads before atelet serves. The gate, its bookkeeping, the wait before listening and the readiness check go. A sync that cannot persist a runtime fails and is retried rather than advertising a hash a restart would lose. On load, an entry whose record does not hash to its key is dropped and a file that does not decode starts the index empty; the syncer writes the current runtimes again either way. atomicfile gains WriteSync, which syncs the data before the rename and the directory after it, for the one file here that must survive a node crash.
7470b48 to
094ed5f
Compare
…lans it (agent-substrate#2741) > **Rebased onto main after agent-substrate#2731 merged.** Three commits: the change, then two rounds of review fixes kept separate so each thread's change is visible. Since agent-substrate#2660 the syncer fetches a gVisor version's runsc to probe its CPU features; that fetch now goes through the same asset cache and is bounded by the prewarmer's attempt timeout. Step 4 of agent-substrate#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.go` watched 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 `runtimePrewarmer` right 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 same `ensureSandboxAssets` Run 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, and `microvmNodeCapable`, 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), and `TestDefaultRuntimePrewarmThroughSyncer`, 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](https://claude.com/claude-code)
Step 3 of #2675: atelet keeps an index of the sandbox runtimes its workers advertise, persisted on the node, and resolves an actor's assets through it.
The index. The worker sandbox syncer already resolves each local worker pod to its pool's SandboxConfig and hashes every enabled version into the worker's
sandbox-runtimes.json. It now also registers each hash with the asset record it was computed from. Two maps: runtimes by hash, never released (a request may carry a hash a worker advertised earlier; entries are a few URLs; TODO to release stale ones), and what each worker currently advertises by pod UID and sandbox class. A pool runs one class today; the shape admits several.Order of registration. The hashes are registered before the worker's file is written, so a hash is resolvable before any worker can register it with the control plane; a SandboxConfig or WorkerPool change registers the affected pools' runtimes from the event handler at once, ahead of the jittered rewrite of the files, which reads the listers only. What a worker advertises is recorded only after its file is written, and only for the runtimes the file names, so the index never credits a worker with a default its file does not carry. Two versions with one hash have the same assets and pause image, so the first registration's record is kept.
Persistence, no startup gate. The runtimes map is also one file under the node state dir,
sandbox-runtime-index.json, the map serialized, rewritten durably (atomicfile.WriteSync, data then directory) before a new entry enters memory. A worker registers a hash only after atelet wrote the worker's own file, which happens after the index was written, so every hash a worker registered before an atelet restart is on disk and loads before atelet serves. There is therefore no wait on the syncer at startup and no window in which a registered worker's request can miss. On load, an entry whose record does not hash to its key is dropped and a file that does not decode starts the index empty; the syncer writes the current runtimes again either way. A sync that cannot persist a runtime fails and is retried. The file stays small: a node sees a handful of configs with a few versions in its lifetime. Memory stays the read path.Run and Restore hash the request's assets with the same function the syncer uses and stage the index's record. A miss fails the request with FailedPrecondition naming the hash: it means the control plane placed the actor on a worker whose pool does not run that config or version. There is deliberately no fallback to the request's own assets. The hashing goes away in the next PR, which resolves from
sandbox_runtime.Readiness. /readyz holds 503 from the health server's first answer until the gRPC port is open, through a check registered with the readiness object, instead of answering 200 during the trust bundle sync and the plugin wait. A syncer that cannot start is fatal: without it no runtime is ever indexed.
Tests. Index registration with a shared hash across two configs, a re-plan replacing a worker's set while the old runtime stays resolvable, forgetting a gone pod, and early registration without a worker entry. Persistence: register, reopen, resolve with provenance; a mis-keyed entry dropped alone; a corrupt file loads empty; an unwritable path fails the registration. Resolution hit, miss, and unpinned pause image. A Run for an unadvertised runtime is refused with nothing fetched and no actor dir created, then resolves once advertised. The syncer indexes the pods already on the node and retries an unresolvable one in the background; a changed config's runtimes resolve before the jittered sync with both pods queued after it; a worker's entry follows its written file through a failed sync and a partial file; the process test asserts the index entry's fate in every case. Every lifecycle test that sends assets advertises them first.
🤖 Generated with Claude Code