Repository navigation
atelet: serve shared snapshot restores from the node-local file cache - #2796
Conversation
180069b to
24f1e86
Compare
…etric family (agent-substrate#2889) Follow-up to agent-substrate#2756, so that agent-substrate#2796 can give the snapshot cache a budget without a third copy of the eviction loop. **What moves.** The sandbox asset cache's eviction loop, flags, instruments, target formula and shortfall backoff move from `cmd/atelet/sandbox_asset_gc.go` into a package, `cmd/atelet/internal/filecachegc`, written against the filecache store. The loop's behavior is unchanged: immediate first pass, serialized passes, panic recovery, dry run, budget with a 90% low water, shortfall warnings that back off to a periodic reminder. **How a cache uses it.** - `filecachegc.NewFlags("sandbox-asset-cache", "sandbox asset cache")` registers `--sandbox-asset-cache-gc-period`, `--sandbox-asset-cache-max-bytes` and `--sandbox-asset-cache-gc-dry-run`, so the asset cache's flags are the same as before. The snapshot cache will call it once with its own prefix. - `filecachegc.NewInstruments(meter)` creates the instruments once per process. - `filecachegc.Start(ctx, name, store, flags, instruments)` reports the cache's size, warns once when there is no budget, and runs the loop when there is a budget and a period. **One metric family.** `ate.filecache.size`, `ate.filecache.evictions` and `ate.filecache.gc.passes`, labeled by `ate.filecache.name` (today `sandbox_asset`) and, for passes, `ate.filecache.gc.outcome`, replace `ate.sandboxassetcache.size`, `.evictions` and `.gc.passes`. The caches run on one store and one loop, so one set of instruments describes them, and "which cache is over budget on this node" is one query grouped by name. The registry, `ateattr` and the observability guide are updated. The name is `filecache` rather than something broader because the family is about the filecache store; the image cache, which measures layers against volume watermarks, is not a member and keeps its loop. **Tests.** The asset GC's tests move into the package unchanged in what they prove, plus one that puts two stores on the same instruments and asserts two size series under their names. **Docs and e2e.** The sandbox asset cache page from agent-substrate#2792 and the e2e metrics suite follow the rename in the last commit: the suite asserts `ate_filecache_size`, and the page's metrics table and runbook use the new names and say where the label comes from. **Follow-up.** agent-substrate#2796 adopts the driver for the snapshot cache and adds the `snapshot` label value. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
e4a8a56 to
d9b82f5
Compare
|
Chenyi Wang (@chw120), #2796 changes one effect of #1917 for shared restores and I want your read. A gVisor restore from a shared snapshot now gets |
d9b82f5 to
7a25af3
Compare
| // How the snapshot at snapshot_uri may be consumed, as the control plane | ||
| // knows it. Read on Restore, where it decides whether atelet may serve the | ||
| // snapshot's files from its node-local cache; meaningless where atelet | ||
| // writes to the snapshot. | ||
| // | ||
| // +k8s:optional | ||
| // +k8s:maximum=2 # keep this in sync with the SnapshotSharing enum | ||
| SnapshotSharing sharing = 2; |
There was a problem hiding this comment.
🤖 Should sharing live one level up on Snapshot (or RestoreRequest) rather than inside ObjectStorage?
- In
ateapipb.Snapshot, ownership (SnapshotOwner owner:ACTORvs.TAG) is a property of theSnapshot, not ofObjectSnapshot. Whether a snapshot generation is private to one actor (replaced on next suspend) or immutable and shared across actors is a lifecycle property of the snapshot itself rather than an object-storage transport detail. - Thinking ahead to a future
BlockStoragesibling onSnapshot, the node restore path will need the samePRIVATEvs.SHAREDdistinction.
There was a problem hiding this comment.
for blockStorage the golden snapshot is expected to be cloned to the disk, therefore the flag will be irrelevant for this type storage.
| sharing := ateletpb.SnapshotSharing_SNAPSHOT_SHARING_PRIVATE | ||
| if !src.SnapshotURI.OwnedBy(actorSnapshotOwner(actor)) { | ||
| sharing = ateletpb.SnapshotSharing_SNAPSHOT_SHARING_SHARED | ||
| } |
There was a problem hiding this comment.
🤖 Instead of inferring ownership by parsing the object-storage URI (src.SnapshotURI.OwnedBy(actorSnapshotOwner(actor))), can we derive sharing from snap.GetOwner() (SNAPSHOT_OWNER_TAG vs. SNAPSHOT_OWNER_ACTOR) carried on resumeSnapshotSource?
ateapipb.Snapshot.owneris already persisted in the DB and used byactorscope.goto distinguish actor-owned vs. tag-owned snapshots.- Using
snap.GetOwner()avoids relying on object-storage URI path layout in the control plane (which won't exist for future non-object snapshot backends likeBlockStorage) and removes the need for theownSnapshotURI-rewriting helper inworkflow_resume_detach_test.go. - Also worth setting
Sharingconsistently in thelocalSnap != nilbranch above (line 855), sinceSnapshot.Objectis populated there as a fallback when a background detach removes the local copy beforeRestoreruns.
There was a problem hiding this comment.
Done in 3e33210: the resume source carries the snapshot's owner, snapshotSharing(owner) maps a tag's to SHARED and the actor's own, or an unrecorded owner, to PRIVATE, and both branches use it, the local-copy fallback included. The URI check is gone from the resume path, and the detach test no longer rewrites its fixture.
| // to the end by the descriptors runsc holds open. The images' pages stay | ||
| // in the page cache for the next restore of the snapshot, since the | ||
| // worker's unlink no longer takes the inode to zero links. |
There was a problem hiding this comment.
🤖 Regarding the cgroup page-cache question in the PR description (#1917): keeping a hard link in the snapshot cache (nlink >= 1) likely reintroduces cgroup memory pressure for the first restoring actor on a node.
- During initial download,
ateletwrites the cache entry andfilecache.GetFileTocallsFADV_DONTNEEDbefore publishing, so the cached file starts with no resident pages. - When the first gVisor actor restores from that cached entry,
runsc restoreruns inside the actor's<uid>-_pausecgroup (which has the actor'smemory.maxlimit, unlike checkpointing which moves the sentry to<uid>-_ckpt). Faulting inpages.imgduring restore charges those page-cache pages to that first actor's memory cgroup. - Previously (
nlink == 1), whendropActorCheckpointCacheAsyncunlinkedrestore-state/pages.img(PreserveRestoreDir: false) andrunscclosed the FD,nlinkdropped to0and the kernel immediately evicted the inode's page cache and uncharged the actor's cgroup (evictCheckpointCache/FADV_DONTNEEDonly runs whenpreserveRestoreDir == true). - With
cacheModeLink, unlinkingrestore-state/pages.imgleavesnlink == 1in the snapshot cache without callingFADV_DONTNEED, sopages.imgstays resident and charged to the first actor's cgroup after restore finishes.
There was a problem hiding this comment.
You are right, and the chain holds all the way: with the cache's link the unlink went two to one, the pages stayed charged to the first actor's cgroup, and neither reclaim path ran. Fixed in 3438887 on the worker side: dropActorCheckpointCacheAsync now treats a file with another link like a preserved restore dir, waits for the restore's page loading, evicts, then unlinks. The actor ends charged for its guest memory alone, as #1917 intended, and the next restore reads the images from local disk. Tested on Linux with the images hard-linked into a second directory. The rationale in snapshotcache.go and the description say the same now; the page-cache sentence you quoted is gone.
There was a problem hiding this comment.
I also opened #2974 to consider keeping golden snapshots in memory in the atelet level, to expedite first resume
| mode := snapshotCacheMode(durable, cacheModeFor(runtimeRec.SandboxClass)) | ||
| return s.downloadExternalCheckpoint(gctx, durable.GetSnapshotUri(), checkpointDir, sandboxRec.SnapshotFiles, mode) |
There was a problem hiding this comment.
🤖 Above on line 1544, s.fetchManifest(ctx, durable.GetSnapshotUri()) still fetches manifest.json directly from the object-store plugin on every restore even when sharing == SNAPSHOT_SHARING_SHARED. Is manifest.json intentionally left uncached for now, or should it also be served through snapshotCache?
There was a problem hiding this comment.
It was deliberate, and you changed my mind: on a golden resume aiming at under 100 ms the round trip is measurable, and a tag's snapshot cannot be republished by design, so the manifest is as immutable as the files. a2ad472 reads it through the cache for a shared snapshot, as a link into a scratch dir dropped after the read, so a restore that finds the snapshot on the node makes no request to the bucket at all. A private snapshot's manifest is still read fresh: it is the commit marker.
|
Follow-up to the question above: msau42 traced the same thing in review, and the answer is that the charge did come back, so I fixed it on the worker side in 3438887 rather than documenting it. |
Whether a snapshot's files may be cached node-locally is the control plane's knowledge, not the node's: atelet should not infer it from URI layout or atespace conventions, which change as tags supersede golden snapshots. ObjectStorage gains a SnapshotSharing, the property rather than the snapshot's kind, so new shared snapshot kinds need no atelet change. ate-api-server sets it on Restore: SHARED when the snapshot is not the actor's own, which today means a tag's snapshot borrowed at creation, PRIVATE for the actor's own, which its next suspend replaces. Nothing consumes the field yet.
Shared snapshot files, the snapshot a tag holds and every actor created from it restores, are immutable once published, yet every restore that needs them downloads them again into its own per-actor dir. This opens the node-level cache that will hold them: a filecache.Store rooted under the node state dir, since cache hits will be served as hard links into per-actor restore dirs and link(2) needs one mounted filesystem, behind --snapshot-cache-dir with crash debris swept before atelet serves. An empty dir disables it; a dir outside the node state dir fails startup, since the object-store plugin writes only there and could never fill it. --snapshot-cache-fetch-timeout bounds one download, the only bound on a fetch that runs detached from the restores waiting on it. The cache evicts to a byte budget through the eviction loop every file cache on the node shares, filecachegc: --snapshot-cache-gc-period, --snapshot-cache-max-bytes and --snapshot-cache-gc-dry-run, with its size, evictions and passes under the ate.filecache.* instruments as the snapshot cache. The flags are validated whether or not the cache is enabled; a disabled cache runs no loop and reports no size. No restore behavior changes yet: the store is wired into the service but nothing reads through it. The cached download path follows separately. The image cache's check that its dir sits under the base path moves to nodepath.UnderBasePath, which both caches now use.
Snapshot files were downloaded into every actor's restore dir, even when a dozen actors created from one tag resume on the same node. A restore whose durable snapshot the control plane declared SHARED now stages each file through the snapshot cache: concurrent restores of one snapshot share a single download, and later restores read the node's disk instead of object storage. A private snapshot, or one from a caller that did not say, downloads fresh as before; atelet never infers cacheability from the URI. How a hit is served depends on the sandbox class that consumes it, the one the request's runtime resolved to. ateworker-gvisor reads restore state and gets hard links to the cached copy. ateworker-microvm gets private hole-preserving copies, not because a link would corrupt the cache, it replaces config.json by rename and its delta merge copies when the inode is shared, but because a link would cost more than it saves: an on-demand restore keeps memory-ranges open for the VM's life, which would pin the entry that long and turn every suspend's in-place merge into a full copy, and the worker's page-cache eviction would drop the cached pages for every other restore. Still one download per snapshot per node. A link that fails with EXDEV, from a cache bind-mounted apart from the restore dirs, degrades to a copy, warned once per process. The key of each cached file is the object's address without its query, so a signed URL's credential reaches neither the key nor the logs, and a restore that fails after staging removes its restore-state dir, so the links do not hold the cache's entries until the next lifecycle call. The cache fetches through the object-store plugin like the direct path: one file into a scratch dir on the cache's filesystem, then into the entry. Since the cache then writes the restore dir by path rather than through the plugin's os.Root, a symlink waiting there, as the file or as the checkpoint subdirectory, fails the restore instead of being followed.
The cached restore path created a file's subdirectory from the goroutine fetching that file, and the files of one checkpoint are fetched in parallel, so two files under fs/ raced to create it and one failed with EEXIST. Every restore with a filesystem layer, which is every resume of a suspended actor, failed that way in e2e. The subdirectories are now made once, before the fetches start, and the test stages two files in the subdirectory.
The resume workflow decided whether a snapshot was the actor's own by parsing its object-storage URI against the actor's prefix. The record already says who owns the snapshot, the actor or a tag, and that is the property sharing describes; the URI layout is an object-storage detail a block-storage snapshot would not have. Derive it from the owner, in the local-copy branch too, whose fallback object declared nothing. The detach test no longer needs to move its fixture under the actor's UID.
…en another link holds it A restore from object storage unlinks its staged images as soon as the restore returns: the only link gone, the kernel frees pages and blocks when the sentry closes its descriptors, and the actor's cgroup stops paying for them. A shared snapshot restored through atelet's snapshot cache stages those images as hard links into the cache's entry, so the unlink took the inode from two links to one, the pages stayed resident, and the first actor to restore a snapshot on a node kept the whole image's page cache charged to its memory limit for as long as it lived. Treat a file with another link like a preserved restore dir: wait for the restore's page loading, evict the pages, then unlink. The actor ends its restore charged for its guest memory alone, as before, and the next restore of the snapshot reads the images from local disk. atelet's rationale for linking gVisor restores says so.
The manifest was fetched from object storage on every restore, cached snapshot or not. For a shared snapshot it is as immutable as the files it lists, so read it through the snapshot cache too: a restore that finds the snapshot on the node then makes no request to object storage at all, which on a golden resume is a measurable share of the time. A private snapshot's manifest is read fresh as before: it is the commit marker that tells a complete upload from a partial one.
a2ad472 to
cfbb741
Compare
Looked at 3438887, LGTM. That is the right fix for the #1917 charge. |
…lib/ate/snapshots (agent-substrate#2746) Moves each actor's snapshot staging directories (`checkpoint-state`, `restore-state`, `local-checkpoint`) from `/var/lib/ate/actors/<uid>/` to `/var/lib/ate/snapshots/<uid>/`, so a dedicated disk mounted at `/var/lib/ate/snapshots` keeps heavy guest disk writes from slowing checkpoint and restore. - Adds `nodepath.SnapshotsDir` and `ateletpath.ActorSnapshotPath`, and moves the three staging directories under it. - `removeActorDirs` removes both trees on Terminate. - `atelet` and `objectstore-plugin` mount `/var/lib/ate` with `mountPropagation: HostToContainer`, as worker pods already do, so they see a disk mounted there after they start. - With that propagation atelet also sees external volumes a failed Run or Restore left published, so Run and Restore leave the volume mount points in place when they reset the actor's dirs and publish again over them (NodePublishVolume is idempotent). Checkpoint and Terminate still unpublish first and then remove them. - The `ActorDirs` comment notes that `checkpoint_dir` and `restore_dir` may be on a different filesystem from `root_dir`. - `docs/node-storage.md` documents the mount point. - The shared snapshot cache from agent-substrate#2796 stays under `/var/lib/ate` on the boot disk. With a dedicated snapshot disk its hard links into `restore-state` cross mounts, so gVisor restores from a shared snapshot fall back to copying the cached files (no second download); micro-VM restores already copy. The doc notes this, and the cache's comment and cross-mount warning now point at `SnapshotsDir`. - Removes `copyLocalCheckpoint` and its helpers, which had no caller outside tests. #### Breaking change atelet looks for local pause snapshots only under the new path, so an actor paused by the previous release whose snapshot was not uploaded yet cannot resume after an in-place upgrade. Before upgrading atelet, resume or suspend paused actors; `kubectl ate revert actor` recovers one that was missed. The old staging directories of such an actor stay under `/var/lib/ate/actors/<uid>` until it is terminated. Custom manifests that mount a disk under `/var/lib/ate` after atelet starts also need `mountPropagation: HostToContainer` on the `/var/lib/ate` mounts of `atelet` and `objectstore-plugin`. #### How I tested it - `go test -race ./internal/nodepath/... ./cmd/atelet/... ./cmd/ateworker-microvm/... ./internal/objectstoreplugin/...`, including a local snapshot lifecycle with `SnapshotsDir` on another mount. - On a GKE `c3-highmem-192-metal` node with a dedicated disk at `/var/lib/ate/snapshots`, on an earlier revision of this PR (later changes are covered by unit tests): a mount made under it after the pods started was visible in atelet, objectstore-plugin, and a worker; an actor went through suspend, resume, pause, and resume with its snapshot directories on the dedicated disk; deleting a running actor removed both trees. - Same node and build, 32 and 64 concurrent SWE-bench actors, with snapshots on the default boot disk (100 GB, 3,600 IOPS, 290 MiB/s) vs a dedicated Hyperdisk Balanced disk (12,000 IOPS, 1,200 MiB/s) at `/var/lib/ate/snapshots`. The objectstore-plugin ran with `--upload-memory-budget=16Gi`; at the 512Mi default, queued uploads dominated suspend latency (70 s / 134 s on the dedicated disk). | p95, 32 / 64 actors | Boot disk | Dedicated disk | | --- | --- | --- | | Suspend | 46.6 s / 127.6 s | 9.6 s / 18.2 s | | Local snapshot write (`worker_checkpoint`) | 44 s / over 60 s | 0.97 s / 0.97 s | - With 8 actors writing fsync'd 64 MiB chunks to their durable dirs as fast as they could (boot disk 97% busy), probe suspend and resume p95 stayed in line with the quiet baseline (ratios 0.69 and 0.98; one 12-minute run). - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
Replaces #1551, which was four commits from September that no longer applied: the restore source message, the download path and the module path all changed underneath it. Same design, ported onto today's main, after #2756 and #2889, in four commits, plus three from review: the sharing derived from the snapshot's owner, the gVisor worker evicting a shared file's pages before unlinking it, and the manifest read through the cache.
Milestone M2 of the node-local artifact cache from #690: an actor restoring from a shared snapshot, the one a tag holds and every actor created from it starts on, gets its files from a node-level cache instead of its own download.
Sharing is the control plane's knowledge.
ObjectStoragegainsSnapshotSharing, the property rather than the snapshot's kind, so new shared snapshot kinds need no atelet change. ate-api-server sets it on Restore from the snapshot's recorded owner: SHARED for a tag's snapshot, borrowed at creation and immutable, PRIVATE for the actor's own, which its next suspend replaces, and for an unrecorded owner; the local-copy fallback object carries it too. Neither side infers it from the URI.The cache. A
filecache.Storeunder/var/lib/ate/snapshot-cache, behind--snapshot-cache-dir(empty disables),--snapshot-cache-min-ageand--snapshot-cache-fetch-timeout, opened at startup with crash debris swept before atelet serves. The directory must be under the node state dir: the object-store plugin writes only there, so a cache elsewhere could never be filled, and startup refuses it. Every flag is validated whether or not the cache is enabled, and the help text says which are read only while it is. The image cache's base-path check moved tonodepath.UnderBasePath, which both caches use.Eviction. The cache runs on the eviction loop every file cache on the node shares,
cmd/atelet/internal/filecachegcfrom #2889:--snapshot-cache-gc-period,--snapshot-cache-max-bytesand--snapshot-cache-gc-dry-run, a budget with a 90% low water, least recently used first, never an entry a restore still links. Its size, evictions and passes appear under the existingate.filecache.*instruments as a second value ofate.filecache.name,snapshot; no new instrument and no new loop. Every flag is validated whether or not the cache is enabled, as the other flags in atelet are, and the help text says which ones are read only while it is; a disabled cache starts no loop and reports no size.The restore path. A restore whose durable snapshot is declared SHARED stages each file through the cache; concurrent restores of one snapshot share a single download, later ones read disk. How a hit is served depends on the sandbox class the request's runtime resolved to. ateworker-gvisor gets hard links. Once the restore's page loading is done the worker evicts the memory and filesystem images from the page cache and unlinks them from its restore dir, keeping only the small descriptors linked: a file with another link is treated like a preserved restore dir, since unlinking it would free nothing and would leave its pages charged to the actor's cgroup, the case #1917 fixed. So a link protects an image's entry through the restore and the min age, not for the actor's life; an entry evicted later is still read to the end through the descriptors runsc holds open, and the next restore reads the images from local disk. ateworker-microvm gets private hole-preserving copies, not because a link would corrupt the cache, it replaces
config.jsonby rename and its delta merge copies when the inode is shared, but because a link would cost more than it saves: an on-demand restore keepsmemory-rangesopen for the VM's life, which would pin the entry that long and turn every suspend's in-place merge into a full copy, and the worker's page-cache eviction would drop the cached pages for every other restore; #2948 tracks what links would take. EXDEV on a link, from a cache bind-mounted apart from the restore dirs, degrades to a copy, warned once per process. The manifest of a shared snapshot is read through the cache too, so a restore that finds the snapshot on the node makes no request to object storage; a private snapshot's manifest is read fresh, as the commit marker it is. The cache key is the object's address without its query, so a signed URL's credential reaches neither the key, the cache's errors,meta.jsonnor the logs; and a restore that fails after staging removes its restore-state dir, so the links do not hold the cache's entries until the next lifecycle call. The fetch goes through the object-store plugin as the direct path does, one file into a scratch dir on the cache's filesystem; because the cache then writes the restore dir by path rather than through the plugin'sos.Root, a symlink waiting there fails the restore instead of being followed.Not here. Private snapshots stay uncached by design. Links for ateworker-microvm are #2948, a worker change. The open-and-sweep code the two file caches share in shape is still duplicated, since the snapshot cache can be disabled and degrades to copies across mounts where the asset cache refuses to start.
Tests. ate-api functional: a golden restore declares SHARED, the actor's own snapshot PRIVATE. ate-api unit: the detach-and-resume test from #2852 compares the whole Restore snapshot, so its fixture now puts the snapshot under the actor's own UID, and it expects PRIVATE for the downloaded copy and nothing for a local one. atelet: flag validation, enabled or not, including the eviction flags, the fetch timeout, and a directory outside the node state dir; two shared manifest reads download once and a private one reads fresh; open disabled, creating its root, sweeping debris; the sharing gate and the class modes; through the plugin-backed test atelet, two link restores share one download and one inode and the file is read-only, copy mode gives private inodes and survives a consumer's mutation, a nil cache downloads, a missing file's error reaches the caller with nothing left behind, a plugin error through the cached modes keeps its code and leaks no credential from a signed URI, a restore that fails at the worker after staging leaves no file in restore-state and no link on the cache's entry, four concurrent restores download once, a file in the checkpoint subdirectory lands there, and symlinks in place of the file or the subdirectory fail; on Linux, a restore dir on another mount gets a copy with no second download and one warning for two restores. ateworker-gvisor, on Linux: staged images hard-linked elsewhere are evicted after the restore wait and only then unlinked, with the other links intact.
🤖 Generated with Claude Code