test(hypervisor): add incremental pagemap_anon snapshot restore tests by fslongjin · Pull Request #1576 · TencentCloud/CubeSandbox

fslongjin · 2026-08-31T04:07:35Z

Summary

Adds integration tests covering incremental (pagemap_anon) snapshots, where the incremental snapshot overlays CoW anonymous pages onto an existing dest memory-ranges base file.

Changes

test_incremental_snapshot_requires_base_memory_ranges: empty dest must fail with Base snapshot file not found, and must not create memory-ranges.
test_snapshot_restore_incremental_after_restore: Cubelet Tier 2 — restore from a full snapshot, dirty tmpfs (/dev/shm) pages, copy the base memory-ranges into a new dest (simulating Cubelet reflink), incremental snapshot, restore again, and verify the tmpfs md5sum.
remote_command delegates to remote_command_w_args; snapshot event helpers share one implementation.
assert_memory_ranges_full_size checks that memory-ranges keeps the full guest RAM logical size.

Testing

Covered by the two new integration tests in hypervisor/tests/integration.rs.

cubesandboxbot · 2026-08-31T04:18:33Z

+                .ssh_command("dd if=/dev/urandom of=/dev/null bs=1M count=64")
+                .unwrap();
+
+            incremental_snapshot_and_check_events(


This incremental snapshot targets the same snapshot_dir the full snapshot above just wrote to, so the snapshot API call will fail. The VMM still runs Vm::send on the incremental path, and it creates config.json and state.json with OpenOptions::new().create_new(true) (hypervisor/vmm/src/vm.rs:2825 and :2849). Those files already exist from the full snapshot, so the open returns AlreadyExists, ch-remote exits non-zero, remote_command_w_args returns false, and the assert! in incremental_snapshot_and_check_events panics.

An incremental snapshot only needs the base memory-ranges file to pre-exist at the destination (send_pagemap_anon_memory, hypervisor/vmm/src/memory_manager.rs:2418-2431); it cannot tolerate pre-existing config.json/state.json. Give this incremental snapshot a fresh destination seeded with a copy of the base memory-ranges — exactly what test_snapshot_restore_incremental_after_restore does at lines 6896-6906.

Agreed. Same-dir incremental after a full snapshot hits create_new(true) on config.json/state.json (Vm::send), so that test would fail at runtime.

Dropped test_snapshot_restore_incremental. Incremental is only covered by:

test_incremental_snapshot_requires_base_memory_ranges — empty dest must fail

test_snapshot_restore_incremental_after_restore — Cubelet Tier 2 (restore → copy memory-ranges → incremental → restore)

Also switched the guest dirty step to write /root/dirty.bin so the second test actually CoWs pages.

cubesandboxbot · 2026-08-31T04:18:42Z

Review: `test(hypervisor): add incremental pagemap_anon snapshot restore tests`

AI-generated review. Not human-approved.

The PR adds two integration tests for incremental (pagemap_anon) snapshots, plus a small test-infra refactor (remote_command delegation, stderr capture in ssh_command_ip_with_auth, a shared snapshot-event helper). The test design is sound: the tmpfs (/dev/shm) payload genuinely exercises CoW-page capture across restore — the base memory-ranges file copied into the dest does not contain the tmpfs data, so the final md5sum can only match if the incremental snapshot actually overlaid the anonymous pages. I traced the key flows against the base tree (error propagation, event emission, restore memory mapping) and confirmed the tests should exercise what they claim. A few minor issues below.

Findings

Phase-3 assertion outside catch_unwind leaks the child process on failure (hypervisor/tests/integration.rs:6823). Every other assertion in test_snapshot_restore_incremental_after_restore wraps the VM child in catch_unwind + kill_child + handle_child_output, but the final restore's check_sequential_events_exact(...) is asserted before the catch_unwind block. If it fails, the cloud-hypervisor child is dropped without being killed or reaped, and the child's stdout/stderr is never dumped, making failures harder to diagnose. Move it inside the catch_unwind (or at least add a cleanup path).
assert_memory_ranges_full_size couples the file length to MemoryConfig.size (hypervisor/tests/integration.rs:2220). The equality memory-ranges file len == mem_config.size holds for the plain size=1G config used here (the full snapshot writes every RAM region, so the file is exactly 1 GiB), but it is an implicit assumption that breaks for file=-backed memory, zones, or hotplug where region lengths differ from the logical size. Worth an explanatory comment, or deriving the expected length from the snapshot's memory-range table instead.
Style nit (hypervisor/tests/integration.rs:6619): &[&url, "--snapshot-type", "incremental"] relies on &String→&str deref coercion inside the array literal; using url.as_str() (as the same PR does in snapshot_with_extra_args_and_check_events) is more explicit and consistent.

Verified against the base tree

The stderr.contains("Base snapshot file not found") assertion works end-to-end: send_pagemap_anon_memory returns MigratableError::MigrateSend(anyhow!("Base snapshot file not found ...")) → the API layer responds 500 with that body → ch-remote prints Error running command: Server responded with an error: ... on stderr.
ch-remote snapshot <url> --snapshot-type incremental arg order is accepted (positional snapshot_config + --snapshot-type flag in clap).
Event sequences used by the helpers (pausing/paused, snapshotting/snapshotted, restoring, restored/resuming/resumed, and starting) all match the VMM's emission points (main.rs, vm.rs).
Restore maps guest RAM with MAP_PRIVATE (snap_file branch in create_ram_region), so the pagemap_anon CoW capture and the tmpfs md5 verification are meaningful rather than trivially satisfied.
The ssh_command_ip_with_auth change (clear stdout/stderr per retry, capture stderr) is a genuine improvement for diagnosing SSH failures.
MemoryConfig::parse("size=1G", None) yields size = 1 << 30, matching the file length written by the full snapshot.

Minor observations (not blockers)

Both new tests rely on fixed sleep(10s) after spawn/restore, matching the file's existing convention, but the second test boots/restores three 1 GiB VMs sequentially in mod common_parallel; if it proves flaky under parallel load, consider whether it needs serialization.
The ssh_command_ip_with_auth change reads stdout to EOF before draining stderr. For the small-output commands used here this is fine, but it inherits the pre-existing (not newly introduced) deadlock risk if a remote command ever emits very large stderr while stdout is still open.

cubesandboxbot · 2026-08-31T06:22:02Z

+            guest.check_devices_common(Some(&socket), Some(&console_text), None);
+
+            let dirty_size = guest
+                .ssh_command("sudo stat -c %s /root/dirty.bin")


This assertion doesn't actually verify that the incremental (pagemap_anon) snapshot captured the CoW pages — the data it checks is persisted on disk, not in the snapshot.

/root/dirty.bin is written with conv=fsync, so it's durably on the shared disk image (cloud-hypervisor snapshots don't include disks; VM1/VM2/VM3 all share the same qcow2). After the second restore, stat reads the file size from disk metadata, which survives independently of the memory snapshot. A pagemap_anon filter that silently dropped the dirty page-cache pages would still pass this check, because the guest can re-read the file from disk.

To genuinely exercise the CoW-capture path, dirty a memory-backed filesystem instead (e.g. sudo mount -t tmpfs tmpfs /mnt or /dev/shm) and verify content with a checksum (md5sum) before and after restore. That content only survives if the CoW pages are actually captured in the incremental memory file.

Agreed — /root/dirty.bin lives on the shared disk image, so stat after restore does not prove pagemap_anon captured the CoW pages.

Changed the payload to /dev/shm/dirty.bin (tmpfs) and compare md5sum before vs after the incremental restore. That content only survives if the dirty RAM pages are in the memory snapshot.

cubesandboxbot · 2026-08-31T06:22:04Z


+fn remote_command_w_args(api_socket: &str, command: &str, args: &[&str]) -> bool {
+    let mut cmd = Command::new(clh_command("ch-remote"));
+    cmd.args([&format!("--api-socket={}", api_socket), command]);


This new helper is a near-verbatim copy of the existing remote_command above (same ch-remote invocation, same error-printing block). Since the file already has three near-identical helpers (remote_command, remote_command_w_output, and now remote_command_w_args), consider having remote_command delegate to remote_command_w_args with a one-element slice (or vice-versa) so the command-execution/error-reporting logic lives in one place.

Done — remote_command now delegates to remote_command_w_args.

cubesandboxbot · 2026-08-31T06:36:36Z

+            assert!(remote_command(&api_socket, "pause", None));
+
+            let url = format!("file://{}", snapshot_dir);
+            assert!(


This assertion can pass for the wrong reason. remote_command_w_args returns false on any ch-remote failure, and stderr is discarded. In send_pagemap_anon_memory, the filter_memory_ranges_by_pagemap_anon call (memory_manager.rs) runs before the base-file existence check, so if pagemap filtering errors on the host (permissions, kernel restrictions), the command fails and no memory-ranges is created — and this test passes without ever exercising the "incremental requires an existing base" path it is meant to pin.

Consider asserting on the actual error message, e.g. add a remote_command_w_args_output variant that returns stderr and check it mentions the missing base file (the VMM error is "Base snapshot file not found at ...").

Good catch — filter_memory_ranges_by_pagemap_anon runs before the dest existence check, so any pagemap error would also fail the command.

The negative test now uses remote_command_w_args_output and asserts stderr contains Base snapshot file not found.

cubesandboxbot · 2026-08-31T06:36:38Z


+// Incremental (pagemap_anon) snapshot: dest must already contain a
+// memory-ranges base file. Only CoW anonymous pages are overwritten.
+fn incremental_snapshot_and_check_events(api_socket: &str, snapshot_dir: &str, event_path: &str) {


This helper is a near-verbatim copy of snapshot_and_check_events — the only difference is the extra --snapshot-type incremental argument to ch-remote snapshot. Consider parameterizing the existing helper by snapshot type (or by the extra args slice) instead of duplicating the pause + event-check + sleep logic. Since you already introduced remote_command_w_args, snapshot_and_check_events could delegate to it with &[] for the full path and ["--snapshot-type", "incremental"] for this one, keeping a single source of truth for the event assertions.

Done — snapshot_and_check_events and incremental_snapshot_and_check_events now share snapshot_with_extra_args_and_check_events.

Add integration tests covering incremental (pagemap_anon) snapshots, where the incremental snapshot overlays CoW anonymous pages onto an existing dest memory-ranges base file: - test_snapshot_restore_incremental: take a full snapshot first, then an incremental one onto the same dest directory, restore and verify events. - test_snapshot_restore_incremental_after_restore: restore a VM from a full snapshot, dirty more pages, copy the base memory-ranges into a new dest (simulating Cubelet reflink), then incremental snapshot and restore again. Also add a remote_command_w_args helper for ch-remote commands with extra arguments, and assert_memory_ranges_full_size to check the memory-ranges file keeps the full guest RAM logical size. Signed-off-by: Songqian Li <sionli@tencent.com> Signed-off-by: jinlong <jinlong@tencent.com>

cubesandboxbot · 2026-08-31T08:08:10Z

+        "incremental snapshot did not produce {}",
+        memory_ranges.display()
+    );
+    let mem_config = MemoryConfig::parse(mem_params, None).unwrap();


assert_memory_ranges_full_size compares the on-disk memory-ranges length against MemoryConfig::parse(mem_params, None).size, re-deriving the expected size from the CLI string. That equality only holds when guest RAM is a single, unzoned, non-hotplug region — which is exactly this test's config (size=1G), so the assertion is correct here. But the helper reads like a general invariant, and it would silently mis-validate for configs with hotplug_size/zones, where the sum of the snapshot's RAM regions (what actually goes into memory-ranges) is not equal to config.size. Consider deriving the expected length from the actual snapshot memory ranges (or the snapshot's config.json) and/or documenting the single-zone assumption in the helper.

cubesandboxbot · 2026-08-31T08:08:12Z

+                "incremental snapshot without dest memory-ranges must fail"
+            );
+            assert!(
+                stderr.contains("Base snapshot file not found"),


This test pins the current VMM behavior that an incremental snapshot with no base memory-ranges fails with Base snapshot file not found. Worth noting that send_pagemap_anon_memory's own doc comment (hypervisor/vmm/src/memory_manager.rs:2382-2384) still describes the opposite behavior — "When no base snapshot exists (cold start), a new file is created and only anonymous pages are written; non-anonymous regions are left as holes (zeros)". The code (memory_manager.rs:2420-2425) instead errors out, so the doc is stale. Since this test now locks in the error behavior, it's worth reconciling the doc vs. the code so the intended cold-start semantics (if any) are clear.

cubesandboxbot · 2026-08-31T08:08:13Z

+            guest
+                .ssh_command("dd if=/dev/urandom of=/dev/shm/dirty.bin bs=1M count=64")
+                .unwrap();
+            let sum = guest.ssh_command("md5sum /dev/shm/dirty.bin").unwrap();


Flake risk to be aware of: the md5 comparison at the end depends on the 64 MiB of freshly written /dev/shm/dirty.bin staying resident in the guest page cache between this md5sum and the incremental snapshot. If the guest reclaims those (clean) tmpfs pages before the pause/snapshot, the second restore's md5 would legitimately differ even though the snapshot feature works. Unlikely here (1 GiB guest, 64 MiB file, snapshot taken immediately after), but it's the main source of non-determinism in this otherwise well-constructed test.

Capture the remote channel's stderr via `channel.stderr()` and print it alongside stdout when `ssh_command_ip_with_auth` gives up after exhausting retries. Both buffers are reset before each retry so the reported output reflects the final failed attempt only. Signed-off-by: Songqian Li <sionli@tencent.com>

cubesandboxbot · 2026-08-31T08:41:42Z

+                device_id: None,
+            },
+        ];
+        assert!(check_sequential_events_exact(


This assertion is the only one in this test that runs outside catch_unwind. If check_sequential_events_exact panics (e.g. the restore hasn't emitted starting/restoring within the 10s wait), the cloud-hypervisor child is dropped without kill_child/wait_with_output, leaving an orphaned VM process and no child output dumped for diagnosis. Every other assertion in this test wraps the child in catch_unwind + kill_child + handle_child_output. Consider moving this check inside the catch_unwind block below.

cubesandboxbot · 2026-08-31T08:41:43Z

+        "incremental snapshot did not produce {}",
+        memory_ranges.display()
+    );
+    let mem_config = MemoryConfig::parse(mem_params, None).unwrap();


This asserts memory-ranges file length == MemoryConfig.size. That equality holds for the plain size=1G config used here (the full snapshot writes every RAM region), but it's an implicit assumption: it would break for file=-backed memory, NUMA zones, or hotplug, where the sum of region lengths differs from the logical size. Worth a one-line comment documenting the assumption, or deriving the expected length from the snapshot's memory ranges instead of re-parsing the CLI memory string.

cubesandboxbot · 2026-08-31T08:41:45Z

+            let (ok, stderr) = remote_command_w_args_output(
+                &api_socket,
+                "snapshot",
+                &[&url, "--snapshot-type", "incremental"],


Style nit: this array literal relies on &String → &str deref coercion for &url. url.as_str() would be more explicit and matches how the same PR builds the arg list in snapshot_with_extra_args_and_check_events (args.push(url.as_str())).

lisongqian

LGTM

lkml-likexu · 2026-09-16T03:20:32Z

@fslongjin Please click "Resolve conversation" to ensure that all comments from cubesandboxbot are either approved or rejected.

fslongjin requested review from lisongqian and up2wing as code owners August 31, 2026 04:07

fslongjin force-pushed the ci/ch-intergration branch from 8c9bf58 to d9033dc Compare August 31, 2026 04:09

fslongjin assigned fslongjin and lisongqian Aug 31, 2026

cubesandboxbot Bot reviewed Aug 31, 2026

View reviewed changes

fslongjin force-pushed the ci/ch-intergration branch from d9033dc to 50e754f Compare August 31, 2026 06:13

cubesandboxbot Bot reviewed Aug 31, 2026

View reviewed changes

fslongjin force-pushed the ci/ch-intergration branch from 50e754f to c0ac932 Compare August 31, 2026 06:27

cubesandboxbot Bot reviewed Aug 31, 2026

View reviewed changes

fslongjin force-pushed the ci/ch-intergration branch from c0ac932 to ca0893d Compare August 31, 2026 07:57

cubesandboxbot Bot reviewed Aug 31, 2026

View reviewed changes

lisongqian approved these changes Sep 9, 2026

View reviewed changes

lkml-likexu merged commit 31ac2d4 into TencentCloud:master Sep 16, 2026
39 checks passed

Conversation

fslongjin commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

fslongjin Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: test(hypervisor): add incremental pagemap_anon snapshot restore tests

Findings

Verified against the base tree

Minor observations (not blockers)

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

fslongjin Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

fslongjin Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

fslongjin Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

fslongjin Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

lisongqian left a comment

Choose a reason for hiding this comment

Uh oh!

lkml-likexu commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fslongjin commented Aug 31, 2026 •

edited

Loading

cubesandboxbot Bot commented Aug 31, 2026 •

edited

Loading

Review: `test(hypervisor): add incremental pagemap_anon snapshot restore tests`