Test managedsave-edit #6420
Test managedsave-edit #6420
Conversation
yalzhang
force-pushed
the
managed_edit
branch
2 times, most recently
from
28d73d9 to
4255093
Compare
|
run offline pass: |
yalzhang
force-pushed
the
managed_edit
branch
2 times, most recently
from
5680da0 to
72e0d67
Compare
yalzhang
force-pushed
the
managed_edit
branch
3 times, most recently
from
0392793 to
4ba0ced
Compare
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Hi Yalan. Please change your header and message according to avocado-framework/avocado#6215
Also, please have a look at my other comments. Thank you!
yalzhang
force-pushed
the
managed_edit
branch
from
4ba0ced to
55c00a9
Compare
WalkthroughAdds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
120-121: Consider more specific exception handling.While catching broad
Exceptionis common in test frameworks to ensure proper error reporting, it can mask programming errors. Consider being more specific or at least adding a comment explaining why broad catching is needed here.- except Exception as e: - test.error(f"Unexpected error happened during the test execution: {e}") + except (virsh.VirshError, IOError, OSError) as e: + test.error(f"Unexpected error happened during the test execution: {e}")Or keep it as is with a comment if broad catching is intentional for the test framework.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg(1 hunks)libvirt/tests/src/save_and_restore/managedsave_edit.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/save_and_restore/managedsave_edit.py
🪛 Ruff (0.14.3)
libvirt/tests/src/save_and_restore/managedsave_edit.py
56-56: Local variable err_msg is assigned to but never used
Remove assignment to unused variable err_msg
(F841)
63-63: Local variable vmxml is assigned to but never used
Remove assignment to unused variable vmxml
(F841)
120-120: 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.8
- GitHub Check: Python 3.11
🔇 Additional comments (9)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg (1)
1-27: Configuration looks well-structured!The test variants are properly defined with appropriate defaults, error expectations, and parameter combinations. The readonly variant correctly restricts to opt_none only, and the mutually exclusive options test case is properly configured with expected error messaging.
libvirt/tests/src/save_and_restore/managedsave_edit.py (8)
1-12: Imports and logger setup look good!All necessary modules are imported and the logger is properly configured following the framework conventions.
15-43: Function logic is correct!The
vm_state_checkfunction properly validates both the XML content and VM state. The state validation correctly handles the--runningand--pausedoptions, while implicitly accepting any state when no option is specified, which aligns with libvirt's behavior.
64-71: VM preparation logic is sound!The code correctly ensures the VM is in the required state before performing managedsave, handling both running and paused pre-states appropriately.
73-75: Managedsave operation is correctly implemented!
77-92: Disk preparation and XML construction look good!The logic correctly creates a test disk copy and constructs the XML fragments for editing. The pre-cleanup of any existing test file is good defensive programming.
94-102: XML editing logic is correctly implemented!The sed-style replacement string is properly constructed with correct escaping, and the call to
exec_virsh_editwithmanagedsave_edit=Trueappropriately handles the managedsave-edit scenario.
103-111: Result validation logic is correct!The code properly handles both expected success and expected failure scenarios, with appropriate test failures when results don't match expectations.
112-119: Positive scenario validation is well-implemented!The code correctly starts the VM and validates both XML content and VM state for successful test cases.
yalzhang
force-pushed
the
managed_edit
branch
from
55c00a9 to
a0e3406
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libvirt/tests/src/save_and_restore/managedsave_edit.py (3)
57-57: Error message validation is still missing.The
err_msgparameter is parsed but never used to validate that the actual error output contains the expected error message whenstatus_erroris True. This issue was flagged in a previous review but hasn't been addressed yet.You need to capture the error output from
exec_virsh_editand validate it containserr_msg. This requires either:
- Modifying
exec_virsh_editto return the command result with stderr, or- Capturing the virsh error output within the edit operation
Then add validation in the error handling block around lines 100-103:
if not stat: if status_error: # TODO: Validate err_msg is in the actual error output # This requires capturing stderr from exec_virsh_edit LOG.info("Failed to edit the xml, this was expected!") else: test.fail("managedsave-edit failed!")Note: This depends on avocado-framework/avocado-vt#4235 according to past comments.
64-64: Remove unused variable.The
vmxmlvariable is assigned but never used in the test. This was previously needed for backup functionality that has been removed.- vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
65-65: Potential NameError in cleanup block.If an exception occurs before line 80 where
new_pathis defined (e.g., during VM state preparation or managedsave operation), the finally block at line 121 will attempt to access an undefined variable, causing aNameError.Initialize
new_pathbefore the try block:vm_name = params.get("main_vm") vm = env.get_vm(vm_name) - vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) + new_path = None try:Then update the cleanup check:
finally: # Cleanup - if os.path.exists(new_path): + if new_path and os.path.exists(new_path): LOG.info(f"Remove the file {new_path}") os.remove(new_path)Also applies to: 121-121
🧹 Nitpick comments (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
117-118: Consider more specific exception handling.Catching bare
Exceptionis flagged by static analysis. While this provides a safety net for unexpected errors in test execution, consider whether more specific exceptions should be caught separately (e.g.,LibvirtError,CmdError) for better error categorization.However, if this pattern is standard in the libvirt test framework for top-level test error handling, it may be acceptable as-is since
test.error()properly reports the failure. Based on learnings, VM cleanup is handled by the teardown method, so this exception handler is primarily for error reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg(1 hunks)libvirt/tests/src/save_and_restore/managedsave_edit.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/save_and_restore/managedsave_edit.py
🪛 Ruff (0.14.3)
libvirt/tests/src/save_and_restore/managedsave_edit.py
57-57: Local variable err_msg is assigned to but never used
Remove assignment to unused variable err_msg
(F841)
64-64: Local variable vmxml is assigned to but never used
Remove assignment to unused variable vmxml
(F841)
117-117: 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.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg (1)
1-27: LGTM! Well-structured test configuration.The configuration comprehensively covers the test scenarios: normal operations with different VM states and virsh options, mutually exclusive flags, and readonly mode. The variant structure correctly maps pre-states to options and defines appropriate error expectations.
yalzhang
force-pushed
the
managed_edit
branch
from
a0e3406 to
9506a4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/save_and_restore/managedsave_edit.py (2)
19-33: Consider verifying the old disk path is removed from XML.The current check only verifies that the new disk path exists in the XML. For more complete validation, consider also checking that the old disk path is no longer present, ensuring a clean substitution occurred.
Example check to add:
if not re.search(xml_pattern, xml): test.fail( f"After domain restore the xml is not expected, could not find source file='{new_disk_path}'" ) else: LOG.info(f"Found source file='{new_disk_path}' in the domain xml successfully!") + + # Verify old path is not present + old_pattern = f"<source file='{re.escape(disk_path)}'" + if re.search(old_pattern, xml): + test.fail(f"Old disk path '{disk_path}' still present in XML after edit")Note: You would need to pass
disk_pathas a parameter to this function.
86-86: Add error handling for disk copy operation.The
shutil.copyoperation could fail due to insufficient disk space, permission issues, or I/O errors. While the exception will be caught by the outer try-except block, adding specific error handling here would provide clearer diagnostics.Consider adding explicit error handling:
- shutil.copy(disk_path, new_path) + try: + shutil.copy(disk_path, new_path) + except (OSError, IOError) as e: + test.error(f"Failed to copy disk from '{disk_path}' to '{new_path}': {e}") LOG.info(f"The original disk path is '{disk_path}'")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg(1 hunks)libvirt/tests/src/save_and_restore/managedsave_edit.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 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/save_and_restore/managedsave_edit.py
🪛 Ruff (0.14.3)
libvirt/tests/src/save_and_restore/managedsave_edit.py
116-116: 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.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
🔇 Additional comments (5)
libvirt/tests/src/save_and_restore/managedsave_edit.py (5)
1-12: Imports and setup look good.The imports are appropriate for the test functionality. The module is well-structured.
56-75: VM preparation logic is well-structured.The parameter parsing, VM initialization, and state preparation are properly implemented. The
new_path = Noneinitialization on line 63 correctly addresses the previous feedback about potential NameError in the cleanup block.
90-98: Edit operation is properly structured.The vim-style substitution string construction and the call to
exec_virsh_editwith appropriate parameters (managedsave_edit, readonly, virsh_opt) are correctly implemented.
99-115: Validation logic is well-implemented.The error handling properly distinguishes between expected and unexpected failures. The positive scenario validation (starting the VM and checking state) only runs when
status_erroris False, which is the correct approach.
116-123: Exception handling and cleanup are appropriate for test framework.The broad exception catch on line 116 is flagged by static analysis, but it's acceptable in this test framework context because:
- The exception is re-raised via
test.error()with context- The finally block ensures cleanup (managedsave removal, temp file deletion)
- Per the framework patterns, VM cleanup is handled by teardown methods
The cleanup logic correctly checks
new_path and os.path.exists(new_path)to avoid NameError, addressing previous feedback.Based on learnings
yalzhang
force-pushed
the
managed_edit
branch
from
9506a4e to
6e1f27d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
37-44: Fix hardcoded state values in error messages.The error messages still hardcode "Got 'paused'" and "Got 'running'" instead of using the actual
dom_statevariable. This was flagged in a previous review but not fully addressed.Apply this diff to use the actual state:
if virsh_opt == "--running" and dom_state != "running": test.fail( - "The domain state is not as expected with option '--running'. Got 'paused', expected 'running'" + f"The domain state is not as expected with option '--running'. Got '{dom_state}', expected 'running'" ) elif virsh_opt == "--paused" and dom_state != "paused": test.fail( - "The domain state is not as expected with option '--paused'. Got 'running', expected 'paused'" + f"The domain state is not as expected with option '--paused'. Got '{dom_state}', expected 'paused'" )
🧹 Nitpick comments (2)
libvirt/tests/src/save_and_restore/managedsave_edit.py (2)
6-6: Remove unused import.The
processmodule fromavocado.utilsis imported but never used in this file.Apply this diff:
-from avocado.utils import process - from virttest import virsh
116-117: Consider removing or narrowing the exception handler.The broad
Exceptioncatch may be unnecessary. Based on retrieved learnings from this test framework, the teardown method handles VM cleanup automatically. If exceptions occur during test execution, the framework typically handles them appropriately.Consider one of these approaches:
Option 1: Remove the try-except entirely and rely on framework error handling:
vm_name = params.get("main_vm") vm = env.get_vm(vm_name) new_path = None - try: - LOG.info("Preparing the VM state...") + LOG.info("Preparing the VM state...") # ... rest of the test logic ... - except Exception as e: - test.error(f"Unexpected error happened during the test execution: {e}") - finally: + try: # CleanupOption 2: Catch specific expected exceptions if there are particular error conditions to handle gracefully.
The finally block for cleanup is correct and should be retained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg(1 hunks)libvirt/tests/src/save_and_restore/managedsave_edit.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/save_and_restore/managedsave_edit.py
🧬 Code graph analysis (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(81-83)
🪛 Ruff (0.14.3)
libvirt/tests/src/save_and_restore/managedsave_edit.py
116-116: 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.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (9)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg (3)
1-4: LGTM!The basic test configuration is correct with sensible defaults.
5-18: LGTM!The virsh option variants are well-designed to test different state transitions and correctly flag mutually exclusive options as expected failures.
19-25: LGTM!The readonly variant correctly expects failure and appropriately restricts the test matrix to opt_none.
libvirt/tests/src/save_and_restore/managedsave_edit.py (6)
19-21: LGTM!XML dump and error handling are correct.
23-33: LGTM!Disk path validation logic correctly uses regex search and provides clear error messages.
47-75: LGTM!The test setup, VM state preparation, and managedsave execution are correctly implemented. The
new_pathinitialization at line 63 properly addresses the past review concern about potential NameError in cleanup.
77-107: LGTM!The disk copy and managedsave-edit execution logic is correct. The sed-style replace string construction (lines 90-91) follows virttest conventions, and the status_error validation properly handles both expected success and failure scenarios.
108-115: LGTM!The positive scenario validation correctly skips when status_error is expected and properly verifies both VM state and XML content.
118-123: LGTM!The cleanup logic in the finally block is correctly implemented with proper guards to avoid NameError, addressing the previous review feedback.
This commit introduces a new test for `virsh managedsave-edit`, verifying its ability to modify a VM's saved XML configuration, specifically the disk path, and successfully restore the VM. It includes scenarios for different VM states and validates error handling for invalid options and readonly mode. Signed-off-by: Yalan Zhang <yalzhang@redhat.com>
yalzhang
force-pushed
the
managed_edit
branch
from
6e1f27d to
55ecde3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
34-41: Update error messages to reflect actual state.The error messages still contain hardcoded state values ('paused', 'running') that don't reflect the actual
dom_statevalue. When the state is neither 'running' nor 'paused' (e.g., 'shut off', 'idle'), the error message will be misleading.Apply this diff to use the actual state in error messages:
if virsh_opt == "--running" and dom_state != "running": test.fail( - "The domain state is not as expected with option '--running'. Got 'paused', expected 'running'" + f"The domain state is not as expected with option '--running'. Got '{dom_state}', expected 'running'" ) elif virsh_opt == "--paused" and dom_state != "paused": test.fail( - "The domain state is not as expected with option '--paused'. Got 'running', expected 'paused'" + f"The domain state is not as expected with option '--paused'. Got '{dom_state}', expected 'paused'" )
🧹 Nitpick comments (1)
libvirt/tests/src/save_and_restore/managedsave_edit.py (1)
113-114: Consider more specific exception handling.While catching bare
Exceptionis flagged by static analysis and is acceptable in test framework code for ensuring cleanup, consider being more specific about which exceptions to catch or simply let exceptions propagate (the test framework will handle them).If unexpected exceptions should always be wrapped with additional context via
test.error(), the current approach is reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg(1 hunks)libvirt/tests/src/save_and_restore/managedsave_edit.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/save_and_restore/managedsave_edit.py
🪛 Ruff (0.14.3)
libvirt/tests/src/save_and_restore/managedsave_edit.py
113-113: 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.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (9)
libvirt/tests/cfg/save_and_restore/managedsave_edit.cfg (1)
1-25: LGTM!The configuration comprehensively covers the test scenarios for
virsh managedsave-edit:
- Different virsh options (none,
--running,--paused, mutually exclusive)- Various pre-states (running, paused)
- Failure scenarios (mutually exclusive options, readonly mode)
The variant structure and
status_errorflags are correctly set.libvirt/tests/src/save_and_restore/managedsave_edit.py (8)
1-9: LGTM!The imports are appropriate for the test functionality. The logger is correctly configured using the avocado test pattern.
44-60: LGTM!The parameter parsing and initialization are correct. Initializing
new_path = Noneproperly addresses the past review concern about potentialNameErrorin the cleanup block.
62-68: LGTM!The VM state preparation logic correctly handles both running and paused pre-states. The implementation ensures the VM is in the expected state before managedsave.
70-72: LGTM!The managedsave operation is correctly executed and validated using the libvirt test framework helper.
74-95: LGTM!The disk copy and managedsave-edit logic is well-implemented:
- Properly creates a test disk copy
- Constructs the sed-style replacement string with correct escaping
- Invokes the virsh edit helper with appropriate parameters
The preemptive cleanup of existing test files (lines 79-81) is a sensible approach for test reliability.
96-103: LGTM!The status validation logic correctly handles both positive and negative test scenarios, ensuring the command result matches expectations.
105-112: LGTM!The positive scenario verification correctly starts the VM (triggering restore from managedsave) and validates both the XML content and VM state.
115-120: LGTM!The cleanup logic is robust:
- Properly guards against
NameErrorby checkingnew_pathis notNone- Removes the test disk file
- Cleans up the managedsave
Based on learnings, VM cleanup is handled by the test framework's teardown, so explicit VM destroy is not needed here.
Based on learnings
|
It seems this change needed avocado-framework/avocado-vt#4235 I ran these tests and get |
|
@yalzhang Please can you confirm if 4235 avocado-vt is needed and help get those changes in? |
With that PR the test passes |
This reverts commit 875c970.
3 participants
Test the virsh cmd managedsave-edit with different vm status. Automate VIRT-297849
Summary by CodeRabbit