feat: add mincore snapshot pre-fault - #219
Conversation
|
🔍 OpenCodeReview found 25 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| anyhow::ensure!( | ||
| path.is_file(), | ||
| "--firecracker-binary does not name a file: {}", | ||
| path.display() | ||
| ); |
There was a problem hiding this comment.
This validates only that the path is a regular file, although the option contract says it requires an executable. A non-executable file passes parsing and fails much later during sandbox startup. On Unix, also validate that at least one execute mode bit is set (or adjust the diagnostic to avoid claiming executable validation).
| let mut sandbox = setup_sandbox_with_vcpu(vcpu_count).await?; | ||
| let reported_vcpu_count = guest_vcpu_count(&sandbox).await?; | ||
| anyhow::ensure!( | ||
| reported_vcpu_count == vcpu_count, | ||
| "guest reports {reported_vcpu_count} vCPUs; benchmark requested {vcpu_count}" | ||
| ); |
There was a problem hiding this comment.
Either the nproc query or the count assertion can return after startup without calling sandbox.stop(). This leaks the same daemon-managed resources when a requested multi-vCPU configuration is unsupported or misreported. Ensure the sandbox is explicitly stopped on every validation/pause error path, not only on success.
| run_guest_shell( | ||
| &sandbox, | ||
| PREFAULT_WORKLOAD_SETUP, | ||
| "prefault workload fixture setup", | ||
| ) | ||
| .await?; | ||
| let snapshot = sandbox.pause().await?; | ||
| sandbox.stop().await?; |
There was a problem hiding this comment.
A fixture setup or pause error bypasses sandbox.stop(), leaving the started sandbox's daemon-managed resources allocated. Run setup/pause into a captured result and stop unconditionally before propagating that result (while preserving both errors if cleanup also fails).
| Some(expected) => { | ||
| let reported = guest_vcpu_count(&sandbox).await?; | ||
| anyhow::ensure!( | ||
| reported == expected, | ||
| "restored guest reports {reported} vCPUs; expected {expected}" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This ? can return before sandbox.stop() runs. FirecrackerSandbox::drop only performs best-effort process/network cleanup and explicitly does not release daemon-managed ublk devices, so a failed nproc call can leave benchmark resources allocated and contaminate later samples. Capture this result like command_result, then always call stop() before propagating it; the hot-holder readiness error path in prefault_measurement_samples needs the same cleanup guarantee.
| let sandbox = FirecrackerSandbox::resume_from_snapshot_config_with_prefault( | ||
| snapshot, | ||
| prefault_enabled, | ||
| ) |
There was a problem hiding this comment.
The hot-path holder uses the arm's pre-fault setting. In the enabled arm it actively faults the profiled ranges and keeps their backing pages resident, while the disabled holder does not, so the two hot arms begin from different cache/residency states beyond the operation being timed. This can attribute holder-induced cache warmth to pre-fault performance. Warm both arms with an identical holder policy (or explicitly normalize residency) before measuring.
| let envd_ready_started = std::time::Instant::now(); | ||
| self.wait_for_ready().await?; | ||
| timings.envd_ready = envd_ready_started.elapsed(); |
There was a problem hiding this comment.
envd_ready is documented and reported by the benchmark as the envd-readiness stage, but wait_for_ready() also releases background downloads and executes envd_instance.init(). Likewise, snapshot_load currently includes MMDS setup and disk-rate-limiter reconciliation after load_snapshot_file. These measurements therefore do not isolate the advertised boundaries and can misattribute meaningful latency. Time wait_for_envd_ready() directly (then perform initialization separately), and stop the snapshot-load timer immediately after the load request or rename/document the broader stages.
| warn_if_mincore_host_has_swap(); | ||
| let mut profiler = Self::from_profiling_snapshot_config(snapshot)?; | ||
| let result = async { |
There was a problem hiding this comment.
Cleanup here is not cancellation-safe. stop() runs only after the inner future returns, so dropping/aborting this public profiling future while it waits for envd (or caller workload) drops profiler without asynchronously releasing profiling_mem_ublk_device; UblkDevice is a pure data handle and FirecrackerSandbox::drop does not release it. This affects all profiling methods using this pattern. Use a cancellation-safe async cleanup guard/owned task, or add a drop fallback that schedules release of the exclusive device.
| let working_set = resident_ranges_to_working_set(&newly_resident, ®ions, limits)?; | ||
| debug!( |
There was a problem hiding this comment.
The returned working set is mislabeled for this API. resident_ranges_to_working_set constructs it through GuestMemoryWorkingSet::new, which hard-codes observation_window = "snapshot-resume-to-envd-ready", but this method samples after envd initialization and the caller-supplied workload. If this result is published or compared by its metadata, consumers cannot distinguish it from the ready-only profile. Construct the result with a workload-specific observation window (and update validation to recognize that typed value), or keep workload profiles in a separate diagnostic type that cannot be published as ready-window metadata.
| if let Some(mem_device) = self.profiling_mem_ublk_device.take() { | ||
| if let Err(e) = UblkDeviceManager::global() | ||
| .release_device(&mem_device) | ||
| .await | ||
| { | ||
| warn!(error = %e, "failed to release profiler memory ublk device during stop"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This cleanup is skipped whenever fc_instance.stop(...).await above returns an error because stop() returns immediately at that ?. Unlike SharedMemDevice, this exclusive profiler handle has no drop cleanup, so a failed Firecracker stop leaks the daemon device even when the profiling future runs its explicit cleanup path. Preserve the Firecracker stop error, still release the profiler device (and other resources), then return the accumulated error.
| let stats = | ||
| PrefaultCompletionStats::from_api(api_stats, ranges.len(), bytes) | ||
| .context("validate Firecracker pre-fault completion stats")?; |
There was a problem hiding this comment.
A validation discrepancy in this optional performance hint aborts snapshot resume even though Firecracker has already accepted/completed the pre-fault request. The same method also propagates transport/read failures while only HTTP statuses are fail-open. When pre-faulting is enabled, transient API errors or a compatible server with differing statistics can therefore make an otherwise restorable VM unavailable. Log these failures and continue without verified stats; reserve a hard error for failures that leave VM state unsafe to resume.
1 participant
Scope
This draft implements the public AgentENV mincore snapshot pre-fault path, including correctness repairs required for the production restore path and reproducible benchmark coverage.
main@58732505ba415ac691f2eee2fe7b1af39ba7058ev1.15.1-patch-v2artifact does not return the completion stats this revision requires).What changes
envd-readyboundary rather thanenvd-initialized.--max-prefault-bytesoverride and a deterministic fixed-512-MiB mechanism benchmark. Product defaults remain unchanged at 256 MiB.Validation
Passed on Madsys:
cargo test -p agentenv --lib prefault_stats— 3 passedcargo test -p agentenv --lib manifest— 33 passedcargo test -p firecracker_clientcargo check -p agentenv-benchmarks --bench snapshotcargo bench -p agentenv-benchmarks --bench snapshot --no-runcargo fmt --checkandgit diff --checkmake test-unit PROFILE=debug— AgentENV 806 passed / 4 ignored; linux-cap 3 passedFirecracker-specific unit, integration, and API-route validation is recorded in firecracker#21.
Performance evidence
The included raw logs and report distinguish an isolated fixed working-set mechanism test from the product path. All fixed-512-MiB samples verified
requested == completed == 536,870,912andremaining == 0.Product resume → envd-ready
Fixed 512 MiB multi-vCPU mechanism test
All configurations pre-fault the same deterministic 512 MiB GPA range; only snapshot vCPU/worker count changes. Each row is 10 samples of complete pre-fault wall time, not guest scan or end-to-end resume time.
Per-worker monotonic timelines show the 2-worker ioctls overlap by about 99.8–100%, so this is not userspace dispatch/join serialization. In a focused 1→2 diagnostic, wall time rose 231.2 → 276.7 ms (+19%) while completed uBlk read bytes changed only 578.4 → 589.0 MiB (+1.8%); successful reads rose 4,580 → 6,012 (+31%) and mean request size fell about 126 → 98 KiB.
The supported conclusion is therefore limited: workers execute concurrently and completion accounting is correct, but this host does not show near-linear multi-vCPU pre-fault scaling. The 2-worker regression is accompanied by backing-read request fragmentation, placing the remaining question below userspace worker dispatch in the KVM / host page-fault / backing-uBlk path. This PR deliberately makes no multi-vCPU performance claim.
The product command measured here is
snapshot load → envd-ready; it is not a webpage/application first-request benchmark.Review / merge boundary
Do not merge or release this AgentENV PR until the Firecracker dependency PR is accepted and a matching immutable binary asset is published. The project CI jobs for this head have passed; a separate code-review workflow is still in progress.