feat(snapshot): prefetch envd memory pages at resume for faster envd-ready - #267
feat(snapshot): prefetch envd memory pages at resume for faster envd-ready#267huajq wants to merge 2 commits into
Conversation
|
🔍 OpenCodeReview found 19 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let ps = exec | ||
| .run_command( | ||
| "sh", | ||
| &["-lc", &format!("pgrep -x {process_name} | head -1")], | ||
| ) |
There was a problem hiding this comment.
Process discovery still depends on sh, pgrep, and head from the user rootfs. Minimal/distroless images need not provide any of these, even though aenv-pagedump itself is supplied by the tools drive, so prefetch capture will be silently disabled there. Use tools-drive BusyBox explicitly (and its pidof/shell applets), or make aenv-pagedump resolve the fixed process name directly.
| let total_bytes: u64 = ranges.iter().map(|(_, len)| len).sum(); | ||
| if ranges.len() > MAX_PREFETCH_RANGES || total_bytes > MAX_PREFETCH_BYTES { | ||
| warn!( |
There was a problem hiding this comment.
This sums untrusted/externally produced range lengths with Iterator::sum, which can overflow u64 (panic in debug builds, wrap in release). Since this function is explicitly the validation boundary and claims to disable prefetch when caps are exceeded, use checked_add/try_fold and return None on overflow before comparing with the cap.
Suggestion:
| let total_bytes: u64 = ranges.iter().map(|(_, len)| len).sum(); | |
| if ranges.len() > MAX_PREFETCH_RANGES || total_bytes > MAX_PREFETCH_BYTES { | |
| warn!( | |
| let Some(total_bytes) = ranges | |
| .iter() | |
| .try_fold(0u64, |acc, (_, len)| acc.checked_add(*len)) | |
| else { | |
| warn!("prefetch manifest byte count overflow; disabling prefetch"); | |
| return None; | |
| }; | |
| if ranges.len() > MAX_PREFETCH_RANGES || total_bytes > MAX_PREFETCH_BYTES { | |
| warn!( |
| let Ok(bytes) = tokio::fs::read(prefetch_path).await else { | ||
| return; | ||
| }; | ||
| let Some(ranges) = parse_prefetch_manifest(&bytes) else { |
There was a problem hiding this comment.
The range caps are applied only after the entire repository artifact has been read and deserialized. A corrupt or malicious memory-prefetch.json can therefore cause an unbounded allocation here (and another large allocation in serde) before being rejected, potentially exhausting the launcher. Check metadata / impose a small maximum encoded size and use a bounded read before parsing.
| UblkDeviceManager::global() | ||
| .prefetch(mem_image_config_path, mem_global_config_path, ranges) | ||
| .await; |
There was a problem hiding this comment.
The manifest contains guest-physical addresses, but these values are passed directly as memory-image file offsets. That identity mapping only holds for single-region guests; for larger Firecracker guests, RAM above the architecture gap is stored sequentially in the snapshot file (for example, x86 memory above the 64-GiB GPA boundary does not start at a 64-GiB file offset). Those reads therefore warm the wrong data or fall outside the image. Translate each GPA range through Firecracker's guest-memory region layout before issuing the prefetch, or disable generation for unsupported memory layouts.
| let runtime = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .expect("build pagedump export runtime"); |
There was a problem hiding this comment.
A failure to construct the auxiliary Tokio runtime is recoverable for this best-effort optimization, but expect panics the capture task and can turn a snapshot operation into a process-level failure under runtime/resource exhaustion. Propagate this failure into the existing default/None fallback instead of unwrapping.
Suggestion:
| let runtime = tokio::runtime::Builder::new_current_thread() | |
| .enable_all() | |
| .build() | |
| .expect("build pagedump export runtime"); | |
| let Ok(runtime) = tokio::runtime::Builder::new_current_thread() | |
| .enable_all() | |
| .build() | |
| else { | |
| return None; | |
| }; |
| --build-arg "HTTP_PROXY=$(or $(HTTP_PROXY),$(http_proxy))" \ | ||
| --build-arg "HTTPS_PROXY=$(or $(HTTPS_PROXY),$(https_proxy))" \ |
There was a problem hiding this comment.
Make echoes expanded recipes, so proxy URLs—including common user:password@host credentials—will be written verbatim to local/CI build logs. Normalize/export the selected proxy variables and pass --build-arg HTTP_PROXY / --build-arg HTTPS_PROXY without values (or otherwise suppress/redact the command) so credentials are not printed.
Suggestion:
| --build-arg "HTTP_PROXY=$(or $(HTTP_PROXY),$(http_proxy))" \ | |
| --build-arg "HTTPS_PROXY=$(or $(HTTPS_PROXY),$(https_proxy))" \ | |
| --build-arg HTTP_PROXY \ | |
| --build-arg HTTPS_PROXY \ |
| --build-arg "HTTPS_PROXY=$(or $(HTTPS_PROXY),$(https_proxy))" \ | ||
| --build-arg "GOPROXY=$(GOPROXY)" \ |
There was a problem hiding this comment.
Proxy forwarding was added only to build; publish runs the same network-dependent Dockerfile (APT, Git, rustup, and Cargo) without these arguments. In environments where the shell proxy variables are required, make build can work while make publish still fails. Apply the proxy arguments consistently to the publish recipe as well.
| --build-arg "ENVD_UPSTREAM_REPO=$(ENVD_UPSTREAM_REPO)" \ | ||
| --build-arg "HTTP_PROXY=$(or $(HTTP_PROXY),$(http_proxy))" \ | ||
| --build-arg "HTTPS_PROXY=$(or $(HTTPS_PROXY),$(https_proxy))" \ | ||
| --build-arg "GOPROXY=$(GOPROXY)" \ |
There was a problem hiding this comment.
GOPROXY is not one of Docker's predefined proxy build args, and the Dockerfile does not declare ARG GOPROXY. Consequently this value is ignored and is not present during go build, so builds that require the configured Go module proxy can still fail. Declare ARG GOPROXY in the envd-builder stage (or set it explicitly on the RUN command).
Suggestion:
| --build-arg "GOPROXY=$(GOPROXY)" \ | |
| # In the envd-builder stage: | |
| ARG GOPROXY |
| if let Some((last_start, last_len)) = out.last_mut() { | ||
| if gpa == *last_start + *last_len { | ||
| *last_len += PAGE_SIZE; | ||
| continue; | ||
| } | ||
| } | ||
| out.push((gpa, PAGE_SIZE)); |
There was a problem hiding this comment.
This does not produce the documented "sorted, coalesced" GPA ranges. /proc/<pid>/maps is ordered by virtual address, while PFNs/GPA pages can be in arbitrary physical order; aliases can also produce duplicate/overlapping GPAs. Consequently only accidentally adjacent pages are merged, and the manifest can waste its range/byte caps or cause duplicate prefetch reads. Sort the GPA pages/ranges by physical address, deduplicate overlaps, then coalesce contiguous entries before emitting JSON.
| continue; | ||
| } | ||
| } | ||
| out.push((gpa, PAGE_SIZE)); |
There was a problem hiding this comment.
This helper has no bound while collecting one tuple per present page. For a large process with fragmented physical pages, out can grow to hundreds of thousands or millions of entries and the subsequent formatting allocates another large set of strings; the host's MAX_PREFETCH_RANGES check occurs only after all of this work and output. Enforce an export bound in the guest helper (preferably while collecting/normalizing ranges) so this best-effort optimization cannot cause guest memory pressure or OOM.
|
Thanks for the review! Addressed in 8ac10b2: Fixed
Reviewed but intentionally not changed
|
| self.write_memory_prefetch_file(snapshot_dir.clone()).await; | ||
|
|
||
| let (_, manifest) = match self.pause_to_dir(&snapshot_dir).await { |
There was a problem hiding this comment.
The manifest creates snapshot_dir before pause_to_dir starts. If fc_instance.pause() fails (or the subsequent directory creation fails), pause_to_dir has no cleanup path for this pre-created directory because its cleanup only wraps snapshot_to_dir; the best-effort artifact can therefore be left in the live snapshot root. Clean up snapshot_dir on every capture failure, or write the manifest only after the pause/capture succeeds.
| let Ok(bytes) = tokio::fs::read(prefetch_path).await else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
The size limit is checked via metadata and then the file is read in a separate operation. If the artifact is replaced (or grows) between these calls, fs::read can still allocate/read an arbitrarily large file, contrary to the stated bound. Open the file once and read it through a bounded reader (or verify the size on the opened handle) before parsing.
Suggestion:
| let Ok(bytes) = tokio::fs::read(prefetch_path).await else { | |
| return; | |
| }; | |
| let Ok(file) = tokio::fs::File::open(prefetch_path).await else { | |
| return; | |
| }; | |
| let Ok(bytes) = tokio::io::read_to_end(file.take(MAX_MANIFEST_BYTES + 1), Vec::new()).await else { | |
| return; | |
| }; | |
| if bytes.len() as u64 > MAX_MANIFEST_BYTES { | |
| return; | |
| } |
| const REGION1_END: u64 = 3328 * 1024 * 1024; // 3328 MiB | ||
| const REGION2_START: u64 = 64 * 1024 * 1024 * 1024; // 64 GiB |
There was a problem hiding this comment.
This translation hard-codes the x86 Firecracker layout and does not use the VM's configured memory regions or architecture. On supported non-x86 guests (and if Firecracker changes the region layout), a valid GPA can be passed through, remapped to the wrong offset, or dropped in the gap, so prefetches warm unrelated blocks or miss entirely. Derive the mapping from the same snapshot/memory-region metadata used when producing the memory image, or disable this optimization unless the layout is explicitly known to match.
| } else if start >= REGION2_START { | ||
| // (start - 64 GiB) + 3328 MiB — cannot underflow given the check. | ||
| Some((start - REGION2_START + REGION1_END, len)) |
There was a problem hiding this comment.
The start + len check above only guarantees the source range does not overflow; it does not guarantee that start - REGION2_START + REGION1_END fits in u64. A malformed/corrupt pagedump result with a GPA near u64::MAX can wrap this offset and cause prefetch reads from an unrelated low location. Use checked_add for the translated offset (and drop the range on overflow) before writing the manifest.
Suggestion:
| } else if start >= REGION2_START { | |
| // (start - 64 GiB) + 3328 MiB — cannot underflow given the check. | |
| Some((start - REGION2_START + REGION1_END, len)) | |
| } else if start >= REGION2_START { | |
| let offset = start | |
| .checked_sub(REGION2_START)? | |
| .checked_add(REGION1_END)?; | |
| Some((offset, len)) |
| if let Some(prefetch_path) = &config.memory_prefetch_path { | ||
| Self::trigger_memory_prefetch( | ||
| prefetch_path, | ||
| &config.mem_overlaybd_config.image_config_path, | ||
| &global_config.memory_snapshot.overlaybd_global_config_path, | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Awaiting prefetch() here only waits for the RPC acknowledgement; the daemon explicitly queues prefetch_chunks in a background task and returns Ok immediately. Snapshot loading and resume() therefore proceed while the reads are still running, so this does not guarantee the captured pages are prefetched before the guest resumes and can contend with the guest's demand reads. If the lifecycle contract requires prefetch-before-resume, have the RPC complete the bounded work (or return a job that is awaited) before continuing.
| total_bytes, | ||
| "start memory prefetch" | ||
| ); | ||
| tokio::spawn(prefetch_chunks(image, chunks, image_lock)); |
There was a problem hiding this comment.
Each accepted request creates a background job with six workers, but there is no daemon-wide job/worker limit, cancellation, or shutdown tracking. Repeated requests can therefore create an unbounded number of readers and hold per-image read locks for the duration of remote I/O, consuming I/O/task capacity and potentially starving guest reads or restack (and these tasks outlive daemon shutdown). Bound admission/concurrency and retain/cancel jobs during shutdown before acknowledging the request.
| !init | ||
| !pivot-init | ||
| !Makefile | ||
| !pagedump |
There was a problem hiding this comment.
This only re-includes the pagedump directory entry; the leading * still excludes the files below it. Consequently, COPY pagedump/Cargo.toml ... and COPY pagedump/src/main.rs ... can fail because those files are absent from the build context. Re-include the subtree explicitly.
Suggestion:
| !pagedump | |
| !pagedump | |
| !pagedump/** |
| FROM --platform=$BUILDPLATFORM rust:1.93.0-${DEBIAN_VERSION} AS pagedump-builder | ||
|
|
||
| ARG TARGETARCH | ||
|
|
||
| RUN rustup target add x86_64-unknown-linux-musl aarch64-unknown-linux-musl |
There was a problem hiding this comment.
This stage may fail for the non-native publish target. rustup target add installs Rust's target libraries, but it does not install a cross-architecture musl linker; the repository's release workflows build both amd64 and arm64 while this stage is forced onto BUILDPLATFORM. On a typical amd64 GitHub builder, linking aarch64-unknown-linux-musl will therefore fail. Please either run this builder stage on $TARGETPLATFORM so it links natively (under buildx/QEMU), or install and configure a proven target-specific musl linker via CARGO_TARGET_*_LINKER.
| if gpa == *last_start + *last_len { | ||
| *last_len += PAGE_SIZE; | ||
| total_bytes += PAGE_SIZE; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The total-byte limit is never checked when a page extends the previous range because this branch immediately continues. A physically contiguous working set can therefore scan and emit far beyond 256 MiB; the host then rejects the whole export for exceeding its cap, defeating prefetch and the stated work bound. Check/set truncated after every byte increment, including this coalescing path (preferably with checked arithmetic), and add a contiguous-range boundary test.
| break; | ||
| } | ||
| } | ||
| Ok(out) |
There was a problem hiding this comment.
out is built in virtual-address traversal order, not GPA order, so it is not necessarily sorted as the utility's output contract claims. Aliased mappings can also emit the same physical pages repeatedly. Although resume-time parsing later normalizes ranges, capture validates raw range count/bytes first, so duplicates can unnecessarily hit a cap and disable prefetch. Sort and coalesce GPA ranges before applying/finalizing the output limits (or collect bounded individual runs and canonicalize them), with tests for out-of-order and overlapping PFNs.
1 participant
What
Record the guest init daemon's (envd) resident guest-physical address ranges at snapshot capture and bulk-prefetch those pages into the node's remote-block cache before the VM resumes, so envd's early startup no longer waits on one-at-a-time remote reads.
Why
Resuming a snapshot stalls envd-ready on a serial demand-page-fault chain (~256 KiB × ~21 ms OSS RTT per read, bounded by vCPU count). Measured on a cold OSS-backed snapshot: envd-ready improves from 5.47 s to 2.13 s (-61%) with prefetch enabled.
Related issue
Closes #
Scope and non-goals
aenv-pagedumptools-drive binary;memory-prefetch.jsonartifact publish/resolve on OSS and posix_fs; aPrefetchRPC on the ublk daemon; resume-time trigger; tests and docs.KVM_PRE_FAULT_MEMORY(requires kernel ≥ 6.11, not available on the fleet); page-fault trace recording/replay; changes to snapshot layering or capture content; any modification of the guest kernel.Design and behavior changes
spawn_blocking+ dedicated runtime with a 10 s timeout so a wedged envd can never stall capture.aenv-pagedump) simply produce no manifest — behavior unchanged from today.Compatibility and operations
Prefetchvariant on the internal ublk-daemon Unix-socket protocol; old client → new daemon fails safe (warn only); new client → old daemon unaffected. No external API change.artifacts/{id}/memory-prefetch.json;FirecrackerSnapshotManifestgains a#[serde(skip)]runtime-only field; both are backward compatible.aenv-pagedump(newtools-image/pagedumpcrate + Dockerfile stage); older tools drives degrade gracefully.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes) — N/ACommands and results:
Skipped checks and reasons:
make -C services test— noservices/changes. Generated-code regeneration — N/A (hand-written protocol variant, no codegen involved).Risks and reviewer notes
src/sandbox/firecracker/pagedump.rs,src/snapshot/prefetch.rs,storage/ublk-daemon/src/server.rs(handle_prefetch),tools-image/pagedump/src/main.rs.Checklist