Virtual disk: add sata product attribute test #6603
Conversation
WalkthroughAdds a new Libvirt virtual-disks product-info test: a configuration file declaring positive/negative product-length cases and SATA version gating, and a Python test that provisions a disk with product metadata, updates VM XML, boots the guest, runs sg_inq to verify the product string, and performs cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant L as Libvirt/virsh
participant VM as VM Domain
participant D as Disk Helper
participant G as Guest OS
rect rgb(235,245,255)
note over T: Parse cfg (product variants, SATA gating)
T->>L: Check libvirt version / feature support
end
T->>D: Create/prepare disk per `disk_dict` (type/driver/product)
D-->>T: Disk prepared or error
alt Expected error (negative case)
T->>T: Compare error to `expected_error` → return expected-failure
else Success path
T->>L: Update/define VM XML to attach disk
L-->>T: XML applied
T->>L: Start VM (if configured)
L-->>VM: Boot
T->>G: Login to guest
G-->>T: Session established
T->>G: Identify non-root disk and run `sg_inq -p di -v`
G-->>T: Command output
T->>T: Assert `disk_product` present in output
end
rect rgb(245,235,245)
note over T,VM: Cleanup
T->>L: Destroy/stop VM
T->>L: Restore original VM XML
T->>D: Detach/remove prepared disk
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 4
🧹 Nitpick comments (2)
libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg (1)
11-11: Remove redundant disk_bus assignment.The
disk_bus = "sata"assignment is redundant since it's already set at line 5. Removing this line will improve clarity.Apply this diff:
- sata_bus: func_supported_since_libvirt_ver = (11, 1, 0) - disk_bus = "sata" variants:libvirt/tests/src/virtual_disks/virtual_disks_product.py (1)
41-44: Consider more robust validation of the product string.The current implementation has two potential issues:
- The
sg_inqcommand assumessg3_utilsis installed in the guest, but doesn't handle the case where the command fails.- The substring search for
disk_productin the output could produce false positives if the string appears in other fields.Consider adding error handling for the command execution and parsing the output more precisely to ensure the product string is in the correct field.
For example:
def check_guest(): """ Check the disk info in guest. """ vm_session = vm.wait_for_login() new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session) sg_command = "sg_inq -p di -v /dev/%s" % new_disk - sg_output = vm_session.cmd_output(sg_command) + status, sg_output = vm_session.cmd_status_output(sg_command) + if status != 0: + test.fail(f"sg_inq command failed (exit code {status}). Is sg3_utils installed? Output: {sg_output}") if disk_product not in sg_output: test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg(1 hunks)libvirt/tests/src/virtual_disks/virtual_disks_product.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_disks/virtual_disks_product.py (1)
provider/virtual_disk/disk_base.py (3)
add_vm_disk(95-112)DiskBase(25-633)cleanup_disk_preparation(268-301)
🪛 Ruff (0.13.3)
libvirt/tests/src/virtual_disks/virtual_disks_product.py
22-22: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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 (2)
libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg (1)
12-25: LGTM!The test variant structure properly covers positive cases (product strings at and below the 40-character limit) and negative cases (exceeding the limit). The disk_dict aggregation correctly combines all necessary disk configuration parameters.
libvirt/tests/src/virtual_disks/virtual_disks_product.py (1)
40-40: No issue with disk selection in virtual_disks_product.py
This test adds a single non-root disk, so get_non_root_disk_name() unambiguously returns the newly added disk.
| if not vm.is_alive(): | ||
| vm.start() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for VM start.
The VM start operation doesn't handle potential failures. If vm.start() fails, the test should provide a clear failure message rather than proceeding to the guest check.
Apply this diff:
if not vm.is_alive():
- vm.start()
+ try:
+ vm.start()
+ except Exception as e:
+ test.fail(f"Failed to start VM: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not vm.is_alive(): | |
| vm.start() | |
| if not vm.is_alive(): | |
| try: | |
| vm.start() | |
| except Exception as e: | |
| test.fail(f"Failed to start VM: {e}") |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 62-63,
the call to vm.start() lacks error handling; wrap the start operation in a
try/except (or check its return status) and, on failure, make the test fail
immediately with a clear message (e.g., use pytest.fail or self.fail depending
on the test framework) including VM identifier and the caught exception; also
consider verifying vm.is_alive() after a successful start and failing with a
clear message if it is not alive.
| finally: | ||
| if vm.is_alive(): | ||
| vm.destroy() | ||
| backup_xml.sync() | ||
| disk_obj.cleanup_disk_preparation(disk_type) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure cleanup completes even if VM destruction fails.
The current cleanup logic could leave resources if vm.destroy() raises an exception, preventing backup_xml.sync() and cleanup_disk_preparation() from executing. Use nested try-except blocks to ensure all cleanup steps attempt to complete.
Apply this diff:
finally:
- if vm.is_alive():
- vm.destroy()
- backup_xml.sync()
- disk_obj.cleanup_disk_preparation(disk_type)
+ try:
+ if vm.is_alive():
+ vm.destroy()
+ finally:
+ backup_xml.sync()
+ disk_obj.cleanup_disk_preparation(disk_type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally: | |
| if vm.is_alive(): | |
| vm.destroy() | |
| backup_xml.sync() | |
| disk_obj.cleanup_disk_preparation(disk_type) | |
| finally: | |
| try: | |
| if vm.is_alive(): | |
| vm.destroy() | |
| finally: | |
| backup_xml.sync() | |
| disk_obj.cleanup_disk_preparation(disk_type) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 68 to
72, the finally block currently calls vm.destroy(), backup_xml.sync(), and
disk_obj.cleanup_disk_preparation(disk_type) sequentially so an exception from
vm.destroy() will short-circuit the remaining cleanup; wrap vm.destroy() in its
own try-except (log the exception but do not re-raise), then unconditionally
call backup_xml.sync() and disk_obj.cleanup_disk_preparation(disk_type) in the
outer finally so all cleanup steps are attempted even if VM destruction fails.
Automate case: VIRT-304006 - Start vm with sata disk which has product configuration This case is to verify that "product" info can be set for sata disk. Signed-off-by: Meina Li <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_disks/virtual_disks_product.py (2)
67-68: Add error handling for VM start operation.The VM start operation doesn't handle potential failures. This issue was flagged in a previous review but hasn't been addressed. If
vm.start()fails, the test should provide a clear failure message.Apply this diff:
if not vm.is_alive(): - vm.start() + try: + vm.start() + except Exception as e: + test.fail(f"Failed to start VM: {e}")
73-77: Ensure all cleanup steps execute even if VM destruction fails.The cleanup logic could leave resources if
vm.destroy()raises an exception, preventingbackup_xml.sync()andcleanup_disk_preparation()from executing. This issue was flagged in a previous review but hasn't been addressed.Apply this diff to ensure all cleanup attempts:
finally: - if vm.is_alive(): - vm.destroy() - backup_xml.sync() - disk_obj.cleanup_disk_preparation(disk_type) + try: + if vm.is_alive(): + vm.destroy() + except Exception as e: + test.log.warn(f"Failed to destroy VM during cleanup: {e}") + finally: + try: + backup_xml.sync() + except Exception as e: + test.log.warn(f"Failed to sync backup XML during cleanup: {e}") + try: + disk_obj.cleanup_disk_preparation(disk_type) + except Exception as e: + test.log.warn(f"Failed to cleanup disk during cleanup: {e}")
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_disks/virtual_disks_product.py (2)
46-47: Consider verifying sg_inq availability in the guest.The test assumes
sg_inqis available in the guest OS. If it's missing, the command will fail, potentially with a confusing error message.Consider adding a check:
vm_session = vm.wait_for_login() # Verify sg_inq is available status, output = vm_session.cmd_status_output("which sg_inq") if status != 0: test.cancel("sg_inq command not available in guest. Please install sg3_utils package.") new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session)
31-31: Consider using f-string conversion flag instead of str().The static analysis tool suggests using the
!sconversion flag in the f-string instead of explicitly callingstr().Apply this diff:
- test.fail(f"Failed to define VM:\n {str(xml_error)}") + test.fail(f"Failed to define VM:\n {xml_error!s}")This is a minor style improvement that aligns with modern Python f-string conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg(1 hunks)libvirt/tests/src/virtual_disks/virtual_disks_product.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_disks/virtual_disks_product.py (1)
provider/virtual_disk/disk_base.py (3)
add_vm_disk(95-112)DiskBase(25-633)cleanup_disk_preparation(268-301)
🪛 Ruff (0.14.0)
libvirt/tests/src/virtual_disks/virtual_disks_product.py
31-31: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg (2)
16-18: Verify the product string lengths match the variant names.Ensure the test strings have the correct character counts:
length_less_max: "20BE0061MC20BE0061MC" (20 characters) ✓length_equal_max: "20BE0061MC20BE0061MC20BE0061MC20BE0061MC" (40 characters) ✓The lengths appear correct for testing the 40-character limit.
23-24: Verify the negative test string exceeds the 40-character limit.The
length_more_maxvariant uses "20BE0061MC20BE0061MC20BE0061MC20BE0061MC1" (41 characters), which correctly exceeds the 40-character limit. The expected error message accurately reflects this constraint.libvirt/tests/src/virtual_disks/virtual_disks_product.py (2)
1-9: LGTM: Imports are correct and secure.The use of
literal_evalinstead ofevaladdresses the security concern from previous reviews. All necessary modules are imported.
24-38: LGTM: Error handling properly validates expected errors.The function correctly addresses previous review feedback by:
- Using
literal_evalfor safe parsing- Validating that
expected_erroris present in the actual exception message (lines 33-35)- Providing clear failure messages
| sg_command = "sg_inq -p di -v /dev/%s" % new_disk | ||
| sg_output = vm_session.cmd_output(sg_command) | ||
| if disk_product not in sg_output: | ||
| test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}") |
There was a problem hiding this comment.
Add validation for disk_product parameter.
The code uses disk_product without checking if it's None. If the parameter is missing from the test configuration, line 48 will raise a TypeError when attempting the in operation.
Apply this diff to add validation:
def check_guest():
"""
Check the disk info in guest.
"""
+ if not disk_product:
+ test.error("disk_product parameter is required but not provided")
vm_session = vm.wait_for_login()
new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session)
sg_command = "sg_inq -p di -v /dev/%s" % new_disk
sg_output = vm_session.cmd_output(sg_command)
if disk_product not in sg_output:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sg_command = "sg_inq -p di -v /dev/%s" % new_disk | |
| sg_output = vm_session.cmd_output(sg_command) | |
| if disk_product not in sg_output: | |
| test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}") | |
| def check_guest(): | |
| """ | |
| Check the disk info in guest. | |
| """ | |
| if not disk_product: | |
| test.error("disk_product parameter is required but not provided") | |
| vm_session = vm.wait_for_login() | |
| new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session) | |
| sg_command = "sg_inq -p di -v /dev/%s" % new_disk | |
| sg_output = vm_session.cmd_output(sg_command) | |
| if disk_product not in sg_output: | |
| test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}") |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 46 to
49, the code uses disk_product without validating it, causing a TypeError if
it's None; add an explicit check right before running sg_inq: if disk_product is
None (or falsy), call test.fail with a clear message (or raise an appropriate
test skip/error), otherwise proceed to run sg_inq and check membership as before
so the in operation is safe.
|
1 participant
Automate case:
VIRT-304006 - Start vm with sata disk which has product configuration
This case is to verify that "product" info can be set for sata disk.
Summary by CodeRabbit