Snapshot: add mew manual snapshot cases #6727
Conversation
WalkthroughAdds a new Libvirt test configuration file Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libvirt/tests/src/snapshot/create_manual_snapshot.py (1)
104-106: Consider a more precise check for the "auto" flag.The check
"auto" in linecould produce false positives if a bitmap name or other field contains the substring "auto". Consider checking specifically for the flags line format.# Check for auto flag - elif "auto" in line: + elif line.startswith("flags:") and "auto" in line: current_bitmap_has_auto = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/create_manual_snapshot.cfg(1 hunks)libvirt/tests/src/snapshot/create_manual_snapshot.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/snapshot/create_manual_snapshot.py
🧬 Code graph analysis (1)
libvirt/tests/src/snapshot/create_manual_snapshot.py (2)
provider/virtual_disk/disk_base.py (2)
get_source_list(56-93)add_vm_disk(95-112)provider/snapshot/snapshot_base.py (1)
SnapshotTest(16-101)
🪛 Ruff (0.14.8)
libvirt/tests/src/snapshot/create_manual_snapshot.py
54-54: Unused function argument: disk_paths
(ARG001)
226-226: Do not catch blind exception: Exception
(BLE001)
235-235: Do not catch blind exception: Exception
(BLE001)
242-242: Local variable snap_name is assigned to but never used
Remove assignment to unused variable snap_name
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (5)
libvirt/tests/cfg/snapshot/create_manual_snapshot.cfg (1)
1-18: LGTM!The configuration is well-structured with appropriate version gating for libvirt 11.9.0 and clear variant definitions for the two test scenarios. The diskspec options correctly differentiate between the single manual snapshot case (vda=no, vdb=manual, vdc=external) and the multiple manual snapshots case (all disks=manual).
libvirt/tests/src/snapshot/create_manual_snapshot.py (4)
19-51: LGTM!The guest preparation logic correctly creates additional disk images and attaches them to the VM, skipping vda (the base system disk).
117-160: LGTM!The validation logic correctly differentiates between test variants and properly checks for bitmap presence and auto flags on the expected disks.
163-188: LGTM!The state transition validation correctly verifies the VM is paused after manual snapshot creation and resumes successfully.
217-237: Verify or explicitly document snapshot overlay cleanup withtest_obj.teardown_test().The test creates snapshots with
--no-metadata --disk-onlyoptions, which generates external snapshot overlay files. The teardown removes the original test disk images and callstest_obj.teardown_test(), which invokeslibvirt.clean_up_snapshots()from the virttest framework.However, the teardown does not explicitly list or delete snapshots before relying on
clean_up_snapshots(). According to libvirt behavior,virsh.snapshot_delete()without the--metadataflag automatically removes external snapshot files, soclean_up_snapshots()should handle this. Consider adding a comment or explicit snapshot deletion in the teardown to clarify this dependency, or verify in testing that no orphaned snapshot overlay files remain after teardown completes.
meinaLi
force-pushed
the
snapshot_manual
branch
from
97b02ef to
02ca254
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libvirt/tests/src/snapshot/create_manual_snapshot.py (3)
54-70: Consider checkingsnapshot_create_asresult and failing early on error.Right now the return value of
virsh.snapshot_create_as()is ignored; if the command fails, the test will continue and later assertions may be misleading.You can make failures clearer by asserting on the exit status:
- virsh.snapshot_create_as(vm_name, full_snap_options, **virsh_dargs) - test.log.debug(f"Created snapshot {snap_name} with options: {full_snap_options}") + result = virsh.snapshot_create_as(vm_name, full_snap_options, **virsh_dargs) + test.log.debug(f"Snapshot create cmd: virsh snapshot-create-as {vm_name} {full_snap_options}") + if result.exit_status != 0: + test.fail(f"Failed to create snapshot {snap_name}: {result.stderr_text}") + test.log.debug(f"Created snapshot {snap_name} with options: {full_snap_options}")This keeps the behavior deterministic when virsh fails.
116-159:check_image_infologic matches cfg variants; consider negative checks as a future enhancement.The mapping from
only_one_manual/with_multiple_manualtoexpected_auto_disksand the use ofdisk_pathslook correct and consistent with the cfg (vda,vdb,vdc).If you want stricter validation later, you could also assert that non-expected disks do not contain the checkpoint bitmap or auto flag, but that’s not required for this PR.
216-236: Teardown: broad exception catches and missingvirsh_dargson checkpoint ops.Two minor points in
teardown_test():
- Broad
except Exceptionfor both checkpoint deletion andos.remove()may hide unexpected issues during cleanup.checkpoint_list/checkpoint_deleteare called without**virsh_dargs, so their behavior (debugging, status handling) is slightly inconsistent with the rest of the file.If you want to tighten this up while keeping teardown robust:
- try: - checkpoints = virsh.checkpoint_list(vm_name) - if checkpoint_name in checkpoints.stdout: - virsh.checkpoint_delete(vm_name, checkpoint_name) - except Exception as e: - test.log.debug(f"Error cleaning checkpoint: {e}") + try: + checkpoints = virsh.checkpoint_list(vm_name, **virsh_dargs) + if checkpoint_name in checkpoints.stdout: + virsh.checkpoint_delete(vm_name, checkpoint_name, **virsh_dargs) + except Exception as e: # consider narrowing to specific virsh/disk errors + test.log.debug(f"Error cleaning checkpoint {checkpoint_name}: {e}") @@ - for disk_name in test_disks: - img_path = disk_paths.get(disk_name) - if img_path and os.path.exists(img_path): - try: - os.remove(img_path) - except Exception as e: - test.log.debug(f"Error removing image {img_path}: {e}") + for disk_name in test_disks: + img_path = disk_paths.get(disk_name) + if img_path and os.path.exists(img_path): + try: + os.remove(img_path) + except OSError as e: + test.log.debug(f"Error removing image {img_path}: {e}")Narrowing the second
excepttoOSErroraddresses the static-analysis concern there; for the checkpoint path, you can decide which specific error types you’re comfortable swallowing in teardown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/create_manual_snapshot.cfg(1 hunks)libvirt/tests/src/snapshot/create_manual_snapshot.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/snapshot/create_manual_snapshot.py
🧬 Code graph analysis (1)
libvirt/tests/src/snapshot/create_manual_snapshot.py (2)
provider/virtual_disk/disk_base.py (2)
get_source_list(56-93)add_vm_disk(95-112)provider/snapshot/snapshot_base.py (1)
SnapshotTest(16-101)
🪛 Ruff (0.14.8)
libvirt/tests/src/snapshot/create_manual_snapshot.py
225-225: Do not catch blind exception: Exception
(BLE001)
234-234: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (4)
libvirt/tests/src/snapshot/create_manual_snapshot.py (2)
19-51: Guest preparation and disk wiring look consistent with cfg.
prepare_guest()correctly aligns with the cfg: it derivesvdafromDiskBase.get_source_list, createsvdb/vdcunderdata_dir.get_tmp_dir(), and wires them viadisk_dictanddisk_obj.add_vm_disk(). Thedisk_pathsmap you return matches howcheck_image_info()andteardown_test()consume it, so this part looks good.
162-188: VM state checks are straightforward; confirm expected paused state semantics.The use of
virsh.domstateto assert"paused"after snapshot and"running"aftervirsh.resumeis clear and makes failures easy to diagnose.Just ensure that, for the libvirt/QEMU versions you’re targeting, the snapshot flow for this manual mode is actually expected to pause the guest (vs. e.g. staying
runningor using a different state string), otherwise the check may be too strict.libvirt/tests/cfg/snapshot/create_manual_snapshot.cfg (2)
1-1: Double‑check whether the top‑level cfg key should have a leading dash.The header line is:
- snapshot_create.manual_snapshot:In many tp-libvirt cfg files, the leading
-is typically used undervariants:rather than on the top‑level test key. If that’s the case here, this line might need to be:snapshot_create.manual_snapshot:to be parsed as a test entry instead of a list item.
Please compare with existing
snapshot_create.*cfgs in this repo to confirm the intended syntax.
5-18: Cfg parameters align well with the Python test implementation.The cfg fields (
disk_list,disk_type,disk_dict,checkpoint_name,snap_name,snap_options,only_one_manual,with_multiple_manual, anddiskspec_options) line up correctly withcreate_manual_snapshot.py:
disk_list/disk_dictuse the%splaceholder as expected byprepare_guest().only_one_manualandwith_multiple_manualmatch the flags consumed bycheck_image_info().snap_optionsand per‑variantdiskspec_optionsare combined increate_snapshots()as intended.This wiring looks consistent and should make the two variants behave as described.
| domstate = virsh.domstate(vm_name, **virsh_dargs) | ||
| if "running" not in domstate.stdout.lower(): | ||
| test.fail(f"Expected VM to be running after resume, but got state: {domstate.stdout}") | ||
| test.log.debug("VM is correctly running after resume") |
There was a problem hiding this comment.
hi meina, Could we reuse
if not libvirt.check_vm_state(vm_name, "running"):
test.fail("The guest should be down after executing 'virsh resume'.")
There was a problem hiding this comment.
I wasn’t aware of this function—thank you for the suggestion.
| domstate = virsh.domstate(vm_name, **virsh_dargs) | ||
| if "paused" not in domstate.stdout.lower(): | ||
| test.fail(f"Expected VM to be paused after snapshot, but got state: {domstate.stdout}") | ||
| test.log.debug("VM is correctly paused after snapshot creation") |
| only_one_manual = params.get("only_one_manual", "no") == "yes" | ||
| with_multiple_manual = params.get("with_multiple_manual", "no") == "yes" |
There was a problem hiding this comment.
We could use params.get_boolean
There was a problem hiding this comment.
Used params.get_boolean now.
Automate cases: VIRT-306532 - [Disk snapshot][manual mode] Use virsh cmd to create disk snapshot with multiple 'manual' disk snapshot modes This case is to verify the consistency and usability of 'manual' disk snapshot mode from libvirt-11.9.0-1 Signed-off-by: Meina Li <meili@redhat.com>
meinaLi
force-pushed
the
snapshot_manual
branch
from
02ca254 to
b2124e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libvirt/tests/src/snapshot/create_manual_snapshot.py (2)
131-132: Use consistent boolean parameter access.Line 131 uses
params.get(...) == "yes"while line 132 usesparams.get_boolean(). Preferparams.get_boolean()for both for consistency.🔎 Proposed fix
- only_one_manual = params.get("only_one_manual", "no") == "yes" + only_one_manual = params.get_boolean("only_one_manual") with_multiple_manual = params.get_boolean("with_multiple_manual")
100-114: Last bitmap entry is never validated after the loop.If the checkpoint being searched is the last bitmap in the output, the function returns
Falseeven when it has the auto flag. After the loop completes, a final check ofcurrent_bitmap_nameandcurrent_bitmap_has_autois needed.🔎 Proposed fix
# Check for bitmap name elif line.startswith("name:"): name_value = line.split("name:", 1)[1].strip() current_bitmap_name = name_value # If this is the target checkpoint and it has auto flag, return True if current_bitmap_name == checkpoint_name and current_bitmap_has_auto: return True + + # Final check for the last bitmap entry + if current_bitmap_name == checkpoint_name and current_bitmap_has_auto: + return True return False
🧹 Nitpick comments (2)
libvirt/tests/src/snapshot/create_manual_snapshot.py (2)
70-70: Remove unused variable assignment.
snapshot_listis assigned but never used. Either remove this line or use the result for validation.🔎 Proposed fix
virsh.snapshot_create_as(vm_name, full_snap_options, **virsh_dargs) - snapshot_list = virsh.snapshot_list(vm_name, **virsh_dargs) + virsh.snapshot_list(vm_name, **virsh_dargs) test.log.debug(f"Created snapshot {snap_name} with options: {full_snap_options}")
222-236: Consider catching more specific exceptions in teardown.Catching broad
Exceptionin cleanup is acceptable to ensure teardown completes, but using more specific exception types (e.g.,libvirt.libvirtError,OSError) would be cleaner and avoid masking unexpected errors.🔎 Proposed fix
try: checkpoints = virsh.checkpoint_list(vm_name) if checkpoint_name in checkpoints.stdout: virsh.checkpoint_delete(vm_name, checkpoint_name) - except Exception as e: + except (libvirt.libvirtError, Exception) as e: test.log.debug(f"Error cleaning checkpoint: {e}") test_disks = disk_list[1:] for disk_name in test_disks: img_path = disk_paths.get(disk_name) if img_path and os.path.exists(img_path): try: os.remove(img_path) - except Exception as e: + except OSError as e: test.log.debug(f"Error removing image {img_path}: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/create_manual_snapshot.cfglibvirt/tests/src/snapshot/create_manual_snapshot.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/snapshot/create_manual_snapshot.cfg
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/snapshot/create_manual_snapshot.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/snapshot/create_manual_snapshot.py
🧬 Code graph analysis (1)
libvirt/tests/src/snapshot/create_manual_snapshot.py (3)
provider/virtual_disk/disk_base.py (3)
DiskBase(25-636)get_source_list(56-93)add_vm_disk(95-112)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/snapshot/snapshot_base.py (1)
SnapshotTest(16-101)
🪛 Ruff (0.14.10)
libvirt/tests/src/snapshot/create_manual_snapshot.py
70-70: Local variable snapshot_list is assigned to but never used
Remove assignment to unused variable snapshot_list
(F841)
226-226: Do not catch blind exception: Exception
(BLE001)
235-235: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
🔇 Additional comments (4)
libvirt/tests/src/snapshot/create_manual_snapshot.py (4)
1-16: LGTM!Imports and module-level constants are appropriate for this test module.
19-51: LGTM!The function correctly prepares the guest with additional disks. The disk path mapping and creation logic are clear.
163-188: LGTM!The VM state transition checks (paused after snapshot, running after resume) are correctly implemented using
libvirt.check_vm_state().
191-255: Well-structured test with clear step documentation.The test flow is logical: prepare guest → create checkpoint → restart → create snapshots → verify results. The teardown properly cleans up test artifacts.
|
2 participants
Automate cases:
VIRT-306532 - [Disk snapshot][manual mode] Use virsh cmd to create disk snapshot with multiple 'manual' disk snapshot modes
This case is to verify the consistency and usability of 'manual' disk snapshot mode from libvirt-11.9.0-1
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.