Repository navigation
microvm: evict snapshot page cache on restore to prevent cgroup memory pressure - #2030
Conversation
|
/assign Walter Fender (@cheftako) |
14b5d34 to
c34338d
Compare
|
/cc |
|
There is an interaction with merged #1876: the direct-local restore path sets ActorDirs.RestoreDir to the checkpoint directory and sends PreserveRestoreDir=true. In this PR, that call has restoreDir == actorDirs.GetRestoreDir(), so the preserved-source guard does not match; a single-link checkpoint then reaches the truncate-and-remove branch. That destroys the local checkpoint needed for another resume. Could this cleanup receive and honor PreserveRestoreDir? #1876's body calls out this follow-up explicitly. |
…agent-substrate#1876) ### Summary For a local pause resume (`CHECKPOINT_TYPE_LOCAL`), atelet no longer stages the snapshot into `restore-state/`. It verifies the files in `local-checkpoint/<snapshotName>/` and passes that directory to ateom as `ActorDirs.restore_dir` with `preserve_restore_dir = true`. The external/GCS `restore-state/` flow is unchanged. ### Why `main` currently hard-links the snapshot into `restore-state/`, and the only thing stopping ateom from mutating the actor's local restore point is the `Nlink > 1` side effect of that hard-link. This PR replaces that implicit guard with an explicit `preserve_restore_dir` flag: when set, ateom never deletes or rewrites files in `restore_dir` (no post-eager-restore drop of `memory-ranges`, copy-merge instead of in-place merge on the next OnDemand checkpoint, no `config.json` rewrite on a same-actor resume). This also removes the staging step and its cross-filesystem copy fallback, and gives lifecycle v2, where local snapshots are kept and reused as a cache after upload, a clear read-only contract. Resume latency and disk usage are on par with the current hard-link path. Follow-ups: - With `DATA_ON_GOLDEN` gone (agent-substrate#2105), `copyLocalCheckpoint` has no remaining callers and will be removed in a separate PR. - agent-substrate#1917 / agent-substrate#2030 should key their restore-file cleanup off `preserve_restore_dir` rather than `Nlink`, since a local restore now has a single link. ### Original benchmark vs the pre-hardlink full-copy baseline Setup: glutton, 1 user, 512Mi working set, pause/resume lifecycle. gVisor actors use 1536Mi so `pages.img` stays ~100% cached after pause (same as MicroVM); MicroVM actors use 768Mi. | | ResumeActor P50 | `download` (copy) | `ateom_restore` | |---|---|---|---| | gVisor 1536Mi, full-copy baseline | 740ms | ~310ms | ~370ms | | gVisor 1536Mi, this PR | **430ms** | 0 | ~370ms | | MicroVM 768Mi, full-copy baseline | 2900ms | ~1.8s (1.4–2.1s) | ~0.97s (0.78–1.1s) | | MicroVM 768Mi, this PR | **600ms** | 0 | ~0.57s (0.54–0.82s) | ResumeActor P50 is client-side (locust). `download` and `ateom_restore` are server-side medians from atelet's "Restore timing breakdown" log (6–8 resumes per run). - [x] Tests pass - [ ] Appropriate changes to documentation are included in the PR
c34338d to
c24607c
Compare
Rebased onto main (#1876) and updated maybeDropStagedMemoryImage / dropStagedMemoryImage / dropStagedSnapshotFile to key off preserveRestoreDir: when preserveRestoreDir is true, snapshot files are preserved on disk and we only evict clean page cache. Please review again. Thanks. |
|
Thanks Walter Fender (@cheftako) for the quick review. I resolved all comments and it's ready to review again. |
|
Thanks for addressing the PreserveRestoreDir interaction — the updated guard makes sense. Looking good. |
|
Minor nits with constants would be nice. Otherwise |
c24607c to
ea88409
Compare
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
agent caught some more things ...
| } | ||
| defer f.Close() | ||
| var cstat unix.Cachestat_t | ||
| if err := unix.Cachestat(uint(f.Fd()), &unix.CachestatRange{}, &cstat, 0); err != nil { |
There was a problem hiding this comment.
🤖 question 🟢 – cachestat needs Linux 6.5+. On an older node kernel every call here fails and nothing gets evicted except the truncate path, with no log. What kernels do we expect on GKE nodes? If older ones matter we should probably log once when cachestat isn't available, so the no-op is visible.
There was a problem hiding this comment.
I don't think we require 6.5+ yet for anything else in the repo yet?
There was a problem hiding this comment.
Right, nothing else requires 6.5+, and this doesn't hard-require it either — on < 6.5 kernels cachestat(2) returns ENOSYS and gracefully degrades to a no-op (logging a one-time warning), while the Truncate(0) path still works on all kernels. (GKE COS nodes are already on 6.6+.)
There was a problem hiding this comment.
Local development with limactl is pinned to 6.17 ... 6.18 kernels have broken nested virt on arm64.
There was a problem hiding this comment.
We also kind of need to consider other clusters, not just GKE. It looks like this degrades safely now.
| if !evictSnapshotFiles(targets) { | ||
| return | ||
| } | ||
| for _, delay := range []time.Duration{500 * time.Millisecond, 1500 * time.Millisecond, 3000 * time.Millisecond} { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – I don't think these retries can catch the case they exist for. Dirty pages are skipped, and the kernel won't write back a dirty page until it is older than dirty_expire_centisecs (30s by default) unless background writeback kicks in first. A pause snapshot resumed within a few seconds is almost certainly still dirty for this whole ~5s window, so these passes are probably no-ops for it. The measured win likely comes mostly from the truncate path.
If the checkpoint side writes back and evicts right after the snapshot is written, these files are already clean by restore time and the retries can go away. Otherwise starting writeback here first, with sync_file_range or fdatasync in the goroutine, would make this reliable without blocking the restore.
There was a problem hiding this comment.
In glutton_1user_512mi_uvm_pause_768mi (pure local pause/resume, which doesn't use the truncate path), cachestat + retries cut PauseActor P90 from 4,000 ms to 970 ms (avg 1,522 ms -> 756 ms).
With dirty_background_ratio = 10%, a 768 MiB cgroup triggers background writeback at ~25–76 MiB of dirty pages, so writing a 512 MiB snapshot starts kernel writeback immediately rather than waiting 30s for dirty_expire_centisecs, and the retries reclaim those pages once clean.
I also tested evicting on the checkpoint side or forcing immediate writeback on restore, and both regressed 1-user local pause/resume from ~410 ms to ~2.4 s: evicting on checkpoint loses the warm page cache that local resume reads from, and unconditional fdatasync on restore forces 512 MiB of disk writes during rapid pause/resume cycles even when there is no memory pressure.
There was a problem hiding this comment.
I think we can just walk RestoreDir and not try to walk all local snapshots in ateom, so it doesn't need to know about the atelet paths hardcoded.
I think we can just do this on the checkpoint side as well, which would simplify some things, we don't need the delayed retry, nlink check, etc.
An older rough cut of this:
main...BenTheElder:substrate:microvm-snapshot-nocache
ea88409 to
0629008
Compare
Thanks Benjamin Elder (@BenTheElder) Haven Xia (@HavenXia) for the comments. Updated to only walk restoreDir (dropped localCheckpointsDir, the Nlink check, and extraDirs). I keep eviction on the restore side rather than fsync + FADV_DONTNEED on checkpoint for two reasons:
|
0629008 to
9d7582d
Compare
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
I think this is OK on older kernels but keep in mind that local dev is stuck at 6.17 for the immediate future. This is in the dev docs.
| return | ||
| } | ||
| snapshotcache.DropFiles(targets) | ||
| slog.InfoContext(ctx, "dropped the staged memory image (eager restore needs no merge base)") |
There was a problem hiding this comment.
🤖 nit 🟢 – This logs "dropped" whenever FindFiles matched something, even if every DropFile skipped the file or failed to remove it. The old code only logged success when the remove succeeded. Having DropFile return whether it removed the file would keep the log honest.
| _ = f.Close() | ||
| continue | ||
| } | ||
| anyRemaining = true |
There was a problem hiding this comment.
🤖 nit 🟢 – Cache is measured before the fadvise, so a file that was fully clean and just got evicted still sets anyRemaining. Every restore then pays one extra 500ms sleep and a second pass that finds nothing. Setting anyRemaining only when the file is skipped for dirty or writeback pages avoids that.
| if err == nil && os.SameFile(info, t.Info) && info.Size() == t.Info.Size() { | ||
| var cstat unix.Cachestat_t | ||
| if err := cachestatFn(uint(f.Fd()), &unix.CachestatRange{}, &cstat, 0); err != nil { | ||
| if errors.Is(err, unix.ENOSYS) { |
There was a problem hiding this comment.
🤖 nit 🟢 – Only ENOSYS is logged. A seccomp profile that rejects cachestat with EPERM would turn eviction into a silent no-op. Logging any cachestat error once would make that visible too.
| // Dropping ahead of Resume means a Resume, agent dial, or wakeup failure leaves | ||
| // restore-state empty; atelet re-downloads with O_TRUNC and resetActorDirs | ||
| // wipes the directory on retry, so this costs a re-download on retry rather | ||
| // than correctness. |
There was a problem hiding this comment.
🤖 nit 🟢 – This comment names atelet internals (restore-state, O_TRUNC, resetActorDirs). That's the same coupling as the localCheckpointsDir thread, just in prose, and it will go stale when atelet changes. Maybe just say a failure after this point costs a re-download of the snapshot on retry?
| } | ||
| defer f.Close() | ||
| var cstat unix.Cachestat_t | ||
| if err := unix.Cachestat(uint(f.Fd()), &unix.CachestatRange{}, &cstat, 0); err != nil { |
There was a problem hiding this comment.
Local development with limactl is pinned to 6.17 ... 6.18 kernels have broken nested virt on arm64.
|
I think these can be fixed in follow-ups, but please let's fix them. |
Thanks Ben for the review! Addressed all the follow-up comments in #1917. |
Summary
After
RestoreWorkloadloads a MicroVM snapshot into guest memory, the snapshot files (memory-rangesand filesystem tarballs) remain cached in the worker pod's cgroup memory—either because staged files inrestore-state/are cleaned up too late (or not at all for.tararchives), or because preserved local checkpoints in local-checkpoint/ keep their page cache resident. Under tight pod memory limits or high density, this leftover page cache overlaps with active guest RAM and subsequent checkpoints, triggering kernel dirty-page throttling (balance_dirty_pages) and memory-reclaim stalls.This PR proactively reclaims snapshot page cache during
RestoreWorkloadbefore resuming the guest, without introducing synchronous disk I/O stalls.Changes
local-checkpoint/snapshots) viacachestatand callsPOSIX_FADV_DONTNEEDstrictly when pages are clean (Dirty == 0 && Writeback == 0), reclaiming cgroup memory as soon as background flushes finish without blocking on disk writeback.Benchmarks
Evaluated head-to-head on a 15-node
c3-highcpu-4GKE cluster comparingmainvs this PR (14b5d346).1. Memory-Constrained Local Pause/Resume (
glutton_1user_512mi_uvm_pause_768mi)1 user, 512 MiB active guest RAM (64 MiB dirtied before each pause), 768 MiB worker pod memory limit, 1m duration.
main)1.00 req/s)1.19 req/s)PauseActorAvgPauseActorP50 / P75PauseActorP90 / P99ResumeActorAvg / P50ResumeActorP90 / P992. High-Density GCS Suspend/Resume (
m1_density_512mi_uvm)50 concurrent users, 500 actors, 60 workers across 15 nodes (4 vCPU /node), 512 MiB guest RAM, 1 GiB worker memory limit, 5m duration.
main)12.29 req/s)12.74 req/s)3.59%)2.84%)ResumeActorFailure Rate21.65%)11.61%)ResumeActorAvg / P75ResumeActorP90 / P99FirstResumeAvg / P90 / P99SuspendActorFailure Rate8.69%)6.43%)SuspendActorAvg / P503. High-Density Local Pause/Resume (
m1_density_512mi_uvm_pause)50 concurrent users, 500 actors, 60 workers across 15 nodes (4 vCPU /node), 512 MiB guest RAM, 1 GiB worker memory limit, 5m duration.
main)ResumeActorP50 / AvgResumeActorFirstResumeP50PauseActorP50Testing
cmd/ateom-microvm/snapshotcache_test.go.