feat(sandbox): add optional ZFile compression for memory snapshots - #92
feat(sandbox): add optional ZFile compression for memory snapshots#92huajq wants to merge 3 commits into
Conversation
Add [memory_snapshot] compression_enabled, compression_algorithm (lz4/zstd), and compression_workers config (default disabled, lz4, 1 worker, max 64). Implement direct memory snapshot path: Firecracker dirty ranges are converted to SegmentMappings and read via ProcessVmReader (process_vm_readv) directly into compact_to, skipping the intermediate sparse mem.bin file. The output can be raw OverlayBD or ZFile-compressed via ZFileCompactWriter. OverlaybdCompactOutput routes compression config through create_commit_args and compact_layers for memory delta layers and runtime suffix compaction. Rootfs layers remain raw. Snapshot repository preserves zfile memory lower bytes during import. Extend CompactWriter with requires_ordered_writes and finalize hooks so compact_to can drive ordered writers (ZFile) correctly.
… compression Replace per-batch spawn_blocking with a persistent WorkerPool using std::thread and crossbeam-channel bounded queues. Each worker owns a compressor and processes WorkItems by sequence number; an ordered committer collects results via BTreeMap and writes them in seq order. CompressedSegment eliminates per-block Vec allocations by compressing directly into a contiguous output buffer. commit_compressed_batch uses LocalFile::write_at_sync_pwrite for >4KiB batches (bypassing io_uring submit/completion overhead on local page-cache writes) and falls back to write_all_at for non-local destinations. Increase the compression batch from 32KiB to 512KiB to amortize worker dispatch. Cap physical writes at 4KiB for non-LocalFile fallback paths. Add cancellation-safe poisoning: any builder write/finish await pre-marks failed, cleared only on success; catch_unwind prevents worker panics from deadlocking the drain loop. Switch LZ4 from lz4_flex to lz4-sys (C liblz4) for ~25% higher throughput. Add crossbeam-channel dependency. compact_to improvements: ordered writer support (buffered reads + serial writes), fix zeroed-index sort (Critical: zeroed mappings were pushed before data entries, corrupting partition_point lookups), clamp chunk entries to Segment::MAX_LENGTH, remove redundant semaphore.
|
🔍 OpenCodeReview found 11 issue(s) in this PR.
[test · medium] 📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| CompressOptions::DEFAULT_BLOCK_SIZE, | ||
| 0, | ||
| )); | ||
| compress_args.workers = workers.max(1); |
There was a problem hiding this comment.
[performance · medium]
The documented 64-worker safety limit is only applied in from_memory_snapshot_config, but this crate-visible helper accepts a directly constructed OverlaybdCompactOutput::ZFile and only enforces the lower bound. Since ZFileCompactWriter creates one persistent OS thread per worker, bypassing the constructor can spawn an unbounded number of threads. Clamp here as well (or make the variant fields private and validate construction) so the resource invariant is enforced at the thread-creation boundary.
Suggestion:
| compress_args.workers = workers.max(1); | |
| compress_args.workers = workers.clamp(1, OverlaybdCompactOutput::MAX_COMPRESSION_WORKERS); |
| let lower_tmp = output_path.with_extension("commit.tmp"); | ||
| let build_result: Result<()> = async { |
There was a problem hiding this comment.
[bug · high]
This deterministic temporary name is not unique or exclusively created. Two output paths with the same stem (for example, layer.commit and layer.raw) both map to layer.commit.tmp, and concurrent compactions for the same output also share it. LocalFile::new truncates an existing file rather than using create-new semantics, so one operation can truncate, rename, or remove the other operation's in-progress output, potentially publishing corrupt or wrong data. Use a uniquely named sibling temporary file (for example, NamedTempFile::new_in(output_path.parent())) and retain ownership until an atomic persist/rename.
| tokio::fs::rename(&lower_tmp, output_path) | ||
| .await |
There was a problem hiding this comment.
[bug · medium]
The completed file is renamed without syncing it first, and the containing directory is not synced after the rename. CompactWriter::finalize explicitly does not provide stable-storage durability, so a crash after this function returns success can leave the snapshot missing or incomplete. Sync the temporary file before rename and sync the parent directory afterward if successful snapshot publication is expected to survive a host crash.
| if build_result.is_err() { | ||
| let _ = tokio::fs::remove_file(&lower_tmp).await; | ||
| } | ||
| build_result?; |
There was a problem hiding this comment.
[bug · medium]
Cleanup only runs when the inner future returns Err; if compact_layers is cancelled or its task is aborted during the merge/rename, this code never executes and the partial temporary file remains. Use an RAII-owned temporary path whose Drop removes the file (and only disarm it after successful publication), so cleanup also occurs when the future is dropped.
| fn lz4_compress_bound(src_size: usize) -> usize { | ||
| src_size + (src_size / 255) + 16 | ||
| let size = i32::try_from(src_size).expect("lz4 block size exceeds c_int"); | ||
| let bound = unsafe { lz4_sys::LZ4_compressBound(size) }; |
There was a problem hiding this comment.
[bug · high]
CompressOptions::new accepts any nonzero u32 block size, so values above i32::MAX reach this production path and panic during compressor construction. The FFI methods below also cast slice lengths with as i32, which can silently truncate. Validate the configured LZ4 block size once with checked i32::try_from and return an error, and use checked conversions for every length passed across the FFI boundary.
| return self.compress_and_commit(source, block_size).await; | ||
| } | ||
|
|
||
| self.write_full_blocks_owned(source.to_vec()).await |
There was a problem hiding this comment.
[performance · medium]
The pooled borrowed path copies the entire batch here, then write_full_blocks_owned copies each segment again into its WorkItem. For the 512 KiB batch size this adds about 1 MiB of transient copying per call before compressed-output allocation. Consider partitioning one shared Arc<[u8]>/owned buffer with ranges for workers, or provide an ownership-taking public path so each source byte is transferred rather than copied twice.
| let (first_error, segments) = { | ||
| let pool = self.pool.as_ref().expect("checked is_some above"); |
There was a problem hiding this comment.
[other · medium]
This async write path subsequently calls blocking crossbeam_channel::Receiver::recv() directly on the Tokio executor thread. Compressing a 512 KiB batch can therefore park a current-thread runtime and reduce capacity on a multi-thread runtime. Move submission/result collection to spawn_blocking (or use an async result channel) rather than leaving the known blocking behavior as a TODO in production code.
| let buf_size = writer.buffer_size(); | ||
| ensure!( | ||
| buf_size.is_multiple_of(ALIGNMENT_USIZE), | ||
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), | ||
| "CompactWriter buffer size {buf_size} not aligned" | ||
| ); |
There was a problem hiding this comment.
[bug · medium]
This validation occurs after the header has already been sent through writer.write_all_at. For implementations using the trait's default write_all_at, a zero buffer size reaches data.chunks(0) and panics before this ensure! can return an error; the validation is also skipped entirely when zero detection is enabled. Validate writer.buffer_size() immediately after obtaining the writer and before the first write, so an invalid writer cannot panic on ordinary input.
Suggestion:
| let buf_size = writer.buffer_size(); | |
| ensure!( | |
| buf_size.is_multiple_of(ALIGNMENT_USIZE), | |
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), | |
| "CompactWriter buffer size {buf_size} not aligned" | |
| ); | |
| let writer = commit_args.writer; | |
| let concurrency = commit_args.concurrency.max(1); | |
| let buf_size = writer.buffer_size(); | |
| ensure!( | |
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), | |
| "CompactWriter buffer size {buf_size} not aligned" | |
| ); | |
| writer.write_all_at(&header_buf, 0).await?; |
| if direct { | ||
| assert!(snapshot_dir.path().join("mem_image.json").exists()); | ||
| } |
There was a problem hiding this comment.
[test · low]
snapshot_memory_to_overlaybd writes mem_image.json after both the direct and legacy branches, so its presence is part of both layouts. Restricting this assertion to direct mode allows a legacy regression that omits the resume image config to escape the artifact-layout check (and fail only indirectly later). Assert it unconditionally.
Suggestion:
| if direct { | |
| assert!(snapshot_dir.path().join("mem_image.json").exists()); | |
| } | |
| assert!(snapshot_dir.path().join("mem_image.json").exists()); |
| if !config.memory_snapshot.direct_overlaybd { | ||
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
[test · medium]
This makes the test report success whenever the process configuration does not select the direct path. The checked-in default is direct_overlaybd = true with compression disabled, and there are no test-runner/config references that invoke these new tests under the promised LZ4/Zstd and legacy configurations. Consequently, normal CI exercises only the raw direct case while the other modes silently remain untested. Add an explicit matrix runner/configuration that launches all modes, or parameterize/setup each case so a missing mode fails rather than skips.
5b3bcd7 to
794099c
Compare
| #[config(default = 1)] | ||
| pub compression_workers: usize, |
There was a problem hiding this comment.
[bug · medium]
compression_workers accepts 0 and arbitrarily large values even though the documented domain starts at 1. The consumer currently clamps the value to 1..=64, so invalid operator input is silently changed rather than rejected, making the loaded configuration differ from runtime behavior. Add this field to Config::validate() (and cover 0 and the upper bound in tests), or model it as a validated/nonzero bounded type so startup reports an actionable configuration error.
Suggestion:
| #[config(default = 1)] | |
| pub compression_workers: usize, | |
| #[config(default = 1)] | |
| pub compression_workers: usize, |
| let lower_tmp = output_path.with_extension("commit.tmp"); | ||
| let build_result = async { |
There was a problem hiding this comment.
[bug · medium]
This fixed temporary path is not cancellation-safe. Cleanup only runs after the inner future returns, so dropping/aborting the snapshot future leaves a partial file; pause_to_dir also cannot run its error cleanup when its own future is cancelled. A retry against the caller-managed directory then reopens the same file with LocalFile::new, which neither truncates nor claims it exclusively, and concurrent/retried publication can overwrite or rename stale/mixed bytes. The sparse conversion below has the same pattern. Use a unique sibling tempfile held by an RAII guard (or create-new semantics), then persist/rename it only after finalization so cancellation automatically removes it.
| CompressOptions::DEFAULT_BLOCK_SIZE, | ||
| 0, | ||
| )); | ||
| compress_args.workers = workers.max(1); |
There was a problem hiding this comment.
[performance · medium]
The advertised 64-worker safety limit is enforced only in from_memory_snapshot_config, not at the resource-allocation boundary. Because this crate-visible helper accepts directly constructed ZFile modes, a caller can bypass that limit; ZFileCompactWriter then permits up to 256 persistent OS threads. Clamp here as well (or make construction validated/private) so every call path preserves the production worker cap.
Suggestion:
| compress_args.workers = workers.max(1); | |
| compress_args.workers = workers.clamp(1, OverlaybdCompactOutput::MAX_COMPRESSION_WORKERS); |
| let lower_tmp = output_path.with_extension(format!("commit.{}.tmp", Uuid::now_v7())); | ||
| let build_result: Result<()> = async { |
There was a problem hiding this comment.
[bug · medium]
The UUID temp file is only removed after this inner future returns Err. If compact_layers is cancelled or its task is aborted while creating/compressing/merging the layer, execution never reaches the cleanup block and the potentially large snapshot file remains on disk. Use a drop-based temporary-path/scope guard (for example, a tempfile abstraction retained until the rename succeeds) so unwinding or future cancellation also removes the file; explicitly disarm/persist the guard after a successful rename.
| .await | ||
| .expect("compact mixed raw/lz4/zstd layers"); | ||
| assert_eq!(published.as_deref(), Some(output_path.as_path())); | ||
| assert!(!output_path.with_extension("commit.tmp").exists()); |
There was a problem hiding this comment.
[test · low]
This does not inspect the temporary file created by compact_layers: production adds a UUID (commit.<uuid>.tmp), while this assertion checks only commit.tmp. Consequently the cleanup test passes even when the randomized temp file leaks (the same mismatch occurs in the failure assertion below). Enumerate the output directory and assert that no filename matching the commit.*.tmp convention remains.
| ensure!( | ||
| src_blk_size <= i32::MAX as usize, | ||
| "block_size {src_blk_size} exceeds LZ4 i32 limit" | ||
| ); | ||
|
|
||
| Ok(Self { | ||
| max_dst_size: lz4_compress_bound(src_blk_size), |
There was a problem hiding this comment.
[bug · high]
i32::MAX is not liblz4's maximum accepted source size. LZ4_compressBound() returns 0 above LZ4_MAX_INPUT_SIZE (currently 0x7E000000), so an externally supplied block size in that range reaches the production assert! in lz4_compress_bound instead of returning a configuration error. Validate against LZ4_MAX_INPUT_SIZE and make the bound helper return Result; additionally use checked i32::try_from conversions for every slice length passed to the FFI rather than relying on the configurable block size being “4KiB-scale”.
| if let Some(local) = local_dest { | ||
| let written = local.write_at_sync_pwrite(self.moffset, &batch.bytes)?; | ||
| ensure!(written == batch.bytes.len(), "short sync pwrite"); |
There was a problem hiding this comment.
[bug · high]
Downcasting to LocalFile does not establish the buffered-I/O invariant required by write_at_sync_pwrite. LocalFileBuilder can create a LocalFile with direct_io(true), and these Vec buffers, compressed lengths, and offsets are generally not O_DIRECT-aligned; the pwrite path will then fail with EINVAL whereas the normal VirtualFile::write_at path supplies the required alignment handling. Gate this optimization on an explicit buffered-I/O capability (and otherwise use the async path).
| match pool.recv() { | ||
| Ok(batch) => { |
There was a problem hiding this comment.
[other · medium]
This is a blocking crossbeam receive inside an async method. While compression runs, it parks a Tokio executor thread; on a current-thread runtime it prevents all timers and unrelated tasks from progressing, and concurrent builders can exhaust a multi-thread runtime. The TODO acknowledges this, but this should not ship in the request path: collect results in spawn_blocking/block_in_place, or bridge worker completions through an async channel so waiting yields to the runtime.
| self.write_full_blocks_borrowed(head).await?; | ||
| buf = tail; |
There was a problem hiding this comment.
[bug · high]
A single public write() can commit several batches before a later batch returns an error or the future is cancelled. commit_compressed_batch() has already advanced block_len/moffset, but raw_data_size is only assigned at the end of write(). The builder then remains reusable and finish() can emit metadata whose logical size excludes committed blocks (or a retry can duplicate them), corrupting the zfile. The compact-writer wrapper poisons itself, but ZFileBuilder is public and used directly. Mark the builder failed before the first write await and reject subsequent writes/finalization after any error or cancellation, or make each public write transactionally update all logical state.
| #[tokio::test] | ||
| async fn memory_snapshot_format_matches_config_and_resumes() -> Result<()> { | ||
| common::setup().await; | ||
| let config = agentenv::cfg::ConfigManager::global().config(); | ||
| if !config.memory_snapshot.direct_overlaybd { | ||
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| run_memory_snapshot_format_and_resume_case(true).await | ||
| } |
There was a problem hiding this comment.
[test · medium]
The repository's integration runner invokes this test binary only once with config/default.toml, whose new settings are direct_overlaybd = true and compression_enabled = false. I could not find any runner or temporary configs that launch this test under LZ4, Zstd, or legacy settings. Consequently, this test only covers the direct/raw case, while the new legacy test always skips and the compression branches in assert_memory_layer_matches_config are never exercised. Please add explicit test orchestration that runs separate processes with AENV_CONFIG_PATH pointing to direct raw/LZ4/Zstd and legacy configs (a separate process is required because ConfigManager is global).
Suggestion:
| #[tokio::test] | |
| async fn memory_snapshot_format_matches_config_and_resumes() -> Result<()> { | |
| common::setup().await; | |
| let config = agentenv::cfg::ConfigManager::global().config(); | |
| if !config.memory_snapshot.direct_overlaybd { | |
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | |
| return Ok(()); | |
| } | |
| run_memory_snapshot_format_and_resume_case(true).await | |
| } | |
| #[tokio::test] | |
| async fn memory_snapshot_format_matches_config_and_resumes() -> Result<()> { | |
| common::setup().await; | |
| let config = agentenv::cfg::ConfigManager::global().config(); | |
| if !config.memory_snapshot.direct_overlaybd { | |
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | |
| return Ok(()); | |
| } | |
| run_memory_snapshot_format_and_resume_case(true).await | |
| } |
| #[config(default = 1)] | ||
| pub compression_workers: usize, |
There was a problem hiding this comment.
[bug · medium]
Validate this configured thread count instead of accepting every usize. The consumer currently silently clamps it to 1..=64, so values such as 0 or 1000 load successfully but do not represent the effective configuration. This can hide deployment mistakes and makes configuration behavior inconsistent with the existing background-download bounds checks. Add config validation (and rejection tests) for the supported range, ideally sharing the maximum with the consumer.
Suggestion:
| #[config(default = 1)] | |
| pub compression_workers: usize, | |
| #[config(default = 1)] | |
| pub compression_workers: usize, |
| let lower_tmp = output_path.with_extension(format!("commit.{}.tmp", Uuid::now_v7())); | ||
| let build_result: Result<()> = async { |
There was a problem hiding this comment.
[bug · medium]
Cleanup only runs after this inner future returns. If compact_layers is cancelled or its task is aborted during merge/compression/rename, control never reaches remove_file, leaving a potentially large UUID-named snapshot file behind. This lifecycle operation can be cancelled during shutdown/error propagation, and there is no startup cleanup for this naming pattern. Use a cancellation-safe temporary-file guard whose Drop removes the unpublished path (disarming it after a successful rename), or otherwise arrange cleanup independently of polling this future to completion.
| .await | ||
| .expect("compact mixed raw/lz4/zstd layers"); | ||
| assert_eq!(published.as_deref(), Some(output_path.as_path())); | ||
| assert!(!output_path.with_extension("commit.tmp").exists()); |
There was a problem hiding this comment.
[test · low]
This assertion cannot detect leaked temporary files. The implementation creates <stem>.commit.<uuid>.tmp, while with_extension("commit.tmp") checks <stem>.commit.tmp, a path that is never created. Scan the output directory for names matching the stable compacted-{name}.commit.*.tmp prefix/suffix (and make the same correction to the failure-path assertion below).
| pub fn write_at_sync_pwrite(&self, offset: u64, buf: &[u8]) -> Result<usize> { | ||
| #[cfg(test)] | ||
| self.write_calls | ||
| .fetch_add(1, std::sync::atomic::Ordering::SeqCst); |
There was a problem hiding this comment.
[bug · medium]
This entry point relies on the file being buffered, but LocalFile can be opened with direct_io(true) and the method does not enforce that precondition. The new concrete-type fast path can therefore select this method for an O_DIRECT LocalFile; an ordinary Vec<u8> batch is not guaranteed to have an aligned address or length, so pwrite will fail with EINVAL. Reject O_DIRECT here with an explicit check and ensure the caller falls back to the aligned async write path (or expose a capability predicate used before selecting this method).
Suggestion:
| pub fn write_at_sync_pwrite(&self, offset: u64, buf: &[u8]) -> Result<usize> { | |
| #[cfg(test)] | |
| self.write_calls | |
| .fetch_add(1, std::sync::atomic::Ordering::SeqCst); | |
| pub fn write_at_sync_pwrite(&self, offset: u64, buf: &[u8]) -> Result<usize> { | |
| ensure!( | |
| !self.direct_io, | |
| "write_at_sync_pwrite does not support O_DIRECT files" | |
| ); | |
| #[cfg(test)] | |
| self.write_calls | |
| .fetch_add(1, std::sync::atomic::Ordering::SeqCst); |
| if !config.memory_snapshot.direct_overlaybd { | ||
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
[test · medium]
The claimed raw/LZ4/Zstd and direct/legacy matrix is not actually launched by the changed test harness. make test-agent-integration runs this integration binary once with the default config (direct_overlaybd = true, compression disabled), and the only new e2e manifest change adds a dependency; there are no temporary-config invocations or compression environment overrides. Consequently this test only covers direct/raw, while the legacy test returns success and both compressed modes are never executed. Add explicit test-runner invocations/config fixtures for all intended modes (or make the test itself create isolated processes/configurations), so missing matrix entries cannot silently pass.
Add direct and legacy Firecracker integration tests that verify LZ4 compressed memory snapshots pause and resume correctly with workers=4. Extract shared helper for the two test bodies with mode-specific skip guards. Add OSS e2e test for zfile memory lower byte preservation during snapshot repository import.
| #[config(default = 1)] | ||
| pub compression_workers: usize, |
There was a problem hiding this comment.
[bug · medium]
Validate this setting at the configuration boundary (for example, require 1..=64) rather than accepting every usize. The only consumer currently clamps it to that range, so 0 and values above 64 load successfully but silently run with a different worker count than configured. This can hide deployment mistakes and makes the parsed AppConfig inconsistent with runtime behavior. Add the check to AppConfig::validate and cover both bounds in the config tests.
Suggestion:
| #[config(default = 1)] | |
| pub compression_workers: usize, | |
| #[config(default = 1)] | |
| pub compression_workers: usize, |
| let lower_tmp = output_path.with_extension("commit.tmp"); | ||
| let build_result = async { |
There was a problem hiding this comment.
[other · medium]
This fixed temporary filename makes publication unsafe when two snapshot attempts target the same output path. Both operations can truncate/write the same file, and one operation's error cleanup can delete the other's in-progress output; a rename may then publish mixed or wrong data. Cancellation also bypasses the post-await cleanup and leaves this shared partial file behind. Use a unique sibling temporary file (as compact_layers does with a UUID), ideally with a cleanup guard that runs when the future is dropped, before atomically renaming it into place.
| let lower_tmp = data_path.with_extension("commit.tmp"); | ||
| let build_result: Result<()> = async { |
There was a problem hiding this comment.
[other · medium]
The sparse-memory path has the same deterministic-temp race: overlapping publications can truncate, rename, or remove each other's overlaybd.commit.tmp, and cancellation leaves a partial shared temp file. Create a unique temp file per attempt and arrange drop/cancellation cleanup before renaming it to data_path.
| let lower_tmp = output_path.with_extension(format!("commit.{}.tmp", Uuid::now_v7())); | ||
| let build_result: Result<()> = async { |
There was a problem hiding this comment.
[bug · medium]
Temporary-file cleanup is not cancellation-safe. If this future is dropped while LocalFile::new, merge_files_ro, or rename is pending (for example during snapshot cancellation/shutdown), control never reaches the later remove_file, leaving a potentially large UUID-named snapshot file behind. Please hold an RAII cleanup guard/temporary-path owner that removes the file on Drop, disarming it only after the rename succeeds; cleanup failures on ordinary error paths should also be logged rather than silently discarded.
| .await | ||
| .expect("compact mixed raw/lz4/zstd layers"); | ||
| assert_eq!(published.as_deref(), Some(output_path.as_path())); | ||
| assert!(!output_path.with_extension("commit.tmp").exists()); |
There was a problem hiding this comment.
[test · medium]
This assertion does not check the filename created by production. For compacted-raw.commit, production creates compacted-raw.commit.<UUID>.tmp, while this checks only compacted-raw.commit.tmp; therefore the test still passes if the actual UUID-named temporary file leaks. Enumerate the output directory and assert that no files matching the production <stem>.commit.*.tmp pattern remain (both here and in the failure-path assertion below).
| fs::write( | ||
| &manifest.memory.image_config_path, | ||
| format!(r#"{{"lowers":[{{"file":"{}"}}]}}"#, zfile_lower.display()), | ||
| ) |
There was a problem hiding this comment.
[test · medium]
Avoid interpolating Path::display() into JSON. A temp root containing ", \, or control characters will produce invalid JSON or a different path, making this regression test environment-dependent; display() is also lossy for non-UTF-8 paths. Build the value with serde_json so JSON escaping is handled correctly (and any unsupported path encoding fails explicitly).
Suggestion:
| fs::write( | |
| &manifest.memory.image_config_path, | |
| format!(r#"{{"lowers":[{{"file":"{}"}}]}}"#, zfile_lower.display()), | |
| ) | |
| fs::write( | |
| &manifest.memory.image_config_path, | |
| serde_json::to_vec(&json!({ | |
| "lowers": [{ "file": zfile_lower }], | |
| })) | |
| .expect("serialize zfile memory image config"), | |
| ) |
| let buf_size = writer.buffer_size(); | ||
| ensure!( | ||
| buf_size.is_multiple_of(ALIGNMENT_USIZE), | ||
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), |
There was a problem hiding this comment.
[bug · medium]
This validation occurs only after writer.write_all_at(&header_buf, 0). For a custom writer using the trait's default write_all_at, buffer_size() == 0 reaches data.chunks(0) and panics before this ensure! can return an error. Read and validate buffer_size() before the first writer operation (and reuse the validated value here) so invalid public inputs fail through Result rather than panicking.
Suggestion:
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), | |
| buf_size >= ALIGNMENT_USIZE && buf_size.is_multiple_of(ALIGNMENT_USIZE), |
| let mut resumed = FirecrackerSandbox::resume_from_snapshot_config(&snapshot).await?; | ||
| verify_disk_marker(&mut resumed).await?; |
There was a problem hiding this comment.
[test · medium]
The round-trip assertion only verifies a marker on the persistent disk. It proves that the VM can resume and access its disk, but not that guest-memory bytes survived compression/decompression correctly; corruption in anonymous memory or dirty-range handling may remain unnoticed. Preserve a memory-only value or running process across pause() and verify that state after resume, in addition to checking ZFile metadata and disk contents.
| if !config.memory_snapshot.direct_overlaybd { | ||
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
[test · medium]
This makes missing configuration coverage indistinguishable from success. In the repository, the default config only runs the raw/direct case, and no test runner or CI entry was found that launches this test under LZ4, Zstd, or the legacy configuration. As a result, the newly claimed compression/legacy coverage can silently never execute. Add explicit test invocations/config fixtures for every required mode (or fail when the expected mode is not selected) rather than relying on skip-based external processes.
| if !config.memory_snapshot.direct_overlaybd { | ||
| eprintln!("skipping direct overlaybd integration test; direct_overlaybd is disabled"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| run_memory_snapshot_format_and_resume_case(true).await |
There was a problem hiding this comment.
[test · medium]
The new raw/LZ4/Zstd and direct/legacy coverage is not actually wired into the repository's integration-test runner. make test-agent-integration invokes this test binary once with config/default.toml (direct_overlaybd = true, compression_enabled = false), so this only exercises the direct/raw case; the legacy test returns Ok(()), and no process selects LZ4 or Zstd. Add explicit test-runner/CI invocations using temporary configs for every promised mode (and avoid treating a missing required configuration as a successful skip), otherwise compression and legacy regressions will pass CI.
1 participant
What
Add optional ZFile compression support for Firecracker memory snapshots. When enabled, dirty memory pages are compressed during snapshot creation using LZ4 (default) or Zstd, reducing snapshot artifact size by ~70-88% at the cost of ~40-90% longer pause latency.
Why
Memory snapshots for large VMs produce multi-GB artifacts. Compressing them during creation reduces storage and transfer costs, which is especially valuable for snapshot templates shared across nodes.
Related issue
Closes #
Scope and non-goals
Included:
[memory_snapshot]config:compression_enabled(default false),compression_algorithm(lz4/zstd),compression_workers(default 1, max 64)ProcessVmReader(process_vm_readv) →compact_to→ raw orZFileCompactWriter→ temp → renameWorkerPool(std::thread + crossbeam-channel),CompressedSegment(contiguous output, no per-block Vec), 512 KiB compression batch, ordered committer, cancellation-safe poisoningLocalFile::write_at_sync_pwritefor >4KiB batches (bypasses io_uring overhead on local page-cache writes)compact_toordered writer support, zeroed-index sort fix (Critical), MAX_LENGTH clampExcluded:
Design and behavior changes
Data flow
WorkerPool architecture
Key design decisions
failed=true, cleared only on success; future cancellation or errors leave the writer permanently poisonedPerformance (1 GiB structured text payload, 1536 MiB VM)
LZ4 compression adds ~40% pause latency for ~70% size reduction. Zstd adds ~90% for ~88% reduction.
Zeroed-index sort fix (Critical bug fix included)
compact_topreviously pushed zeroedSegmentMappingentries intocompact_indexduring phase 1, then appended data entries in phase 2. This produced an unsorted index ([all zeros..., all data...]), causingpartition_pointlookups inReadOnlyIndexto silently miss mappings and read committed data as zeros. Fixed by sortingcompact_indexbeforecompress_raw_index.Compatibility and operations
[memory_snapshot]keys with safe defaults (disabled, lz4, 1 worker)crossbeam-channeldependency. No new system requirements.Validation
make fmt(cargo fmt --all -- --check)make clippy(cargo clippy -p overlaybd -p agentenv --all-targets -- -D warnings, zero warnings)make test-unit(overlaybd lib 304/0/1 ignored, agentenv 6 core tests 6/6)make -C services test(N/A, no services/ changes)maketarget (N/A, no generated code changes)Commands and results:
Skipped checks and reasons:
make -C services test: noservices/changesRisks and reviewer notes
Correctness:
.await, so cancellation cannot strand stale results. Worker panics are caught viacatch_unwindand converted to per-seq errors.test_verify_builderasserts workers=4 vs workers=1 produce identical files.Performance:
pool.recv()blocks the calling Tokio task for up to one 512 KiB batch compression time (~ms). This is bounded and documented with a TODO to wrap inspawn_blockingif latency-sensitive.process_vm_readvremains synchronous on the Tokio worker thread (existing TODO). Raw pause is unaffected; compression pause is dominated by codec CPU.Compatibility:
lz4_flextolz4-sys(C liblz4). Both produce standard LZ4 block format; old layers remain readable. Content-addressed caches may see different digests for the same input (LZ4 output is not canonical).compression_workersis clamped to [1, 64] to prevent absurd configs from spawning excessive OS threads.Key files for review:
storage/overlaybd/src/compression/zfile.rs(WorkerPool, CompressedSegment, commit_compressed_batch, tests)src/sandbox/firecracker/overlaybd_snapshot.rs(direct memory snapshot)src/sandbox/ublk/overlaybd.rs(compression config routing)storage/overlaybd/src/lsmt/file/helper.rs(compact_to ordered writer, zeroed-index fix)storage/overlaybd/src/backend/local.rs(write_at_sync_pwrite)Checklist