Inc_backup: add new case to shutdown vm when backup job is running #6773
+104
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Automte case: VIRT-306599 - [incremental_backup]shutdown vm when backup job is running Signed-off-by: Meina Li <[email protected]>
WalkthroughThis PR introduces a new test variant Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Contributor
Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py`:
- Around line 216-276: The test_shutdown_vm function never records the backup
target produced by prepare_backup_xml (the scratch_file_full target), so cleanup
in run() never removes it and ARG001 lint fails; update test_shutdown_vm to
accept/ensure the backup_file_list is initialized (use backup_file_list or
create a local list if None), capture the target path returned or exposed by
prepare_backup_xml/its side-effect (the scratch_file_full target), append that
path to backup_file_list, and ensure the function returns or mutates
backup_file_list so run() can remove it during cleanup (refer to
test_shutdown_vm, prepare_backup_xml, backup_file_list, and run).
🧹 Nitpick comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (1)
278-299: Consider polling instead of fixed sleeps for state/progress checks.Hard sleeps (2/8/5s) plus a strict “progress must increase” check can be flaky on slow or fast hosts. A bounded poll with timeout would be more resilient.
Comment on lines
+216
to
+276
| def test_shutdown_vm(test, params, backup_file_list): | ||
| """ | ||
| Test shutdown vm with incremental backup using --preserve-domain-on-shutdown. | ||
| :param backup_file_list: Placeholder parameter for interface compatibility, not used in this function. | ||
| """ | ||
| def get_blockjob_progress(vm_name, target_disk): | ||
| """ | ||
| Get blockjob progress percentage. | ||
| :param vm_name: VM name | ||
| :param target_disk: target disk name | ||
| :return: progress percentage as float | ||
| """ | ||
| blockjob_result = virsh.blockjob(vm_name, target_disk, debug=True) | ||
| if blockjob_result.exit_status != 0: | ||
| return None | ||
| output = blockjob_result.stdout.strip() + " " + blockjob_result.stderr.strip() | ||
| match = re.search(r'\[\s*(\d+\.?\d*)\s*%\s*\]', output) | ||
| if match: | ||
| progress = float(match.group(1)) | ||
| test.log.info("Current blockjob progress: %.2f%%" % progress) | ||
| return progress | ||
| return None | ||
|
|
||
| def check_vm_state_and_progress(vm_name, target_disk, expected_state, previous_progress, stage_name): | ||
| """ | ||
| Check VM state and verify blockjob progress increases. | ||
| :param vm_name: VM name | ||
| :param target_disk: target disk name | ||
| :param expected_state: expected VM state (e.g., "paused", "running") | ||
| :param previous_progress: previous blockjob progress percentage | ||
| :param stage_name: description of the current stage | ||
| :return: current blockjob progress percentage | ||
| """ | ||
| if not libvirt.check_vm_state(vm_name, expected_state): | ||
| current_state = virsh.domstate(vm_name).stdout.strip() | ||
| test.fail("Expected VM to be %s at '%s' stage, but got state: %s" | ||
| % (expected_state, stage_name, current_state)) | ||
|
|
||
| current_progress = get_blockjob_progress(vm_name, target_disk) | ||
| if current_progress is None: | ||
| test.fail("Blockjob should still be active at '%s' stage" % stage_name) | ||
|
|
||
| if current_progress <= previous_progress: | ||
| test.fail("Blockjob progress should increase at '%s' stage. Previous: %.2f%%, Current: %.2f%%" | ||
| % (stage_name, previous_progress, current_progress)) | ||
|
|
||
| return current_progress | ||
|
|
||
| libvirt_version.is_libvirt_feature_supported(params) | ||
| vm_name = params.get("main_vm") | ||
| target_disk = params.get("target_disk") | ||
| backup_options_extra = params.get("backup_options_extra", "") | ||
| resume_after_shutdown = params.get_boolean("resume_after_shutdown", False) | ||
|
|
||
| test.log.info("Start full backup with --preserve-domain-on-shutdown.") | ||
| backup_options = prepare_backup_xml(test, params, backup_type="full") | ||
| backup_options += backup_options_extra | ||
| virsh.backup_begin(vm_name, backup_options, debug=True, ignore_status=False) |
There was a problem hiding this comment.
Track the push backup target for cleanup (and use backup_file_list).
prepare_backup_xml(..., backup_type="full") creates a target file (scratch_file_full), but test_shutdown_vm never records it, so the file won’t be removed in run() cleanup and the ARG001 lint triggers. Consider initializing the list, appending the target path, and returning it.
♻️ Proposed fix (cleanup + lint)
def test_shutdown_vm(test, params, backup_file_list):
"""
Test shutdown vm with incremental backup using --preserve-domain-on-shutdown.
:param backup_file_list: Placeholder parameter for interface compatibility, not used in this function.
"""
+ if backup_file_list is None:
+ backup_file_list = []
+ # Keep in sync with prepare_backup_xml() scratch_file naming for backup_type="full"
+ backup_target = data_dir.get_data_dir() + '/scratch_file_full'
+ backup_file_list.append(backup_target)
+
def get_blockjob_progress(vm_name, target_disk):
"""
Get blockjob progress percentage.
@@
else:
test.log.info("Abort the blockjob and check the guest status.")
virsh.domjobabort(vm_name, debug=True, ignore_status=False)
time.sleep(5)
if not libvirt.check_vm_state(vm_name, "shut off"):
test.fail("Expected VM to be shut off after aborting blockjob, but got unexpected state: %s"
% virsh.domstate(vm_name).stdout.strip())
+ return backup_file_list🧰 Tools
🪛 Ruff (0.14.13)
216-216: Unused function argument: backup_file_list
(ARG001)
🤖 Prompt for AI Agents
In `@libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py` around
lines 216 - 276, The test_shutdown_vm function never records the backup target
produced by prepare_backup_xml (the scratch_file_full target), so cleanup in
run() never removes it and ARG001 lint fails; update test_shutdown_vm to
accept/ensure the backup_file_list is initialized (use backup_file_list or
create a local list if None), capture the target path returned or exposed by
prepare_backup_xml/its side-effect (the scratch_file_full target), append that
path to backup_file_list, and ensure the function returns or mutates
backup_file_list so run() can remove it during cleanup (refer to
test_shutdown_vm, prepare_backup_xml, backup_file_list, and run).
Contributor
Author
There was a problem hiding this comment.
No need to remove it because it's a temporary file which will removed automatically.
There was a problem hiding this comment.
@meinaLi, thank you for the clarification! If the push-mode backup target is cleaned up automatically by the framework or libvirt, then explicit tracking isn't necessary. I appreciate you explaining the difference in cleanup behavior for this scenario.
/)_/)
(•ᴗ•) Noted for future!
/>📝
✏️ Learnings added
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6773
File: libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py:216-276
Timestamp: 2026-01-22T07:22:04.670Z
Learning: In `test_shutdown_vm` for incremental backup with push mode and `--preserve-domain-on-shutdown`, the scratch_file backup target created by `prepare_backup_xml` is automatically removed and doesn't require explicit tracking in `backup_file_list` for cleanup, unlike pull-mode backups in other test functions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Automte case:
VIRT-306599 - [incremental_backup]shutdown vm when backup job is running
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.