Virtual disk: add different migration test cases #6755
Conversation
WalkthroughAdds a new migration test configuration at libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg and a new test implementation libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py providing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (4)
38-42: Consider logging the result of the device mapper cleanup command.The
clear_dm_devcommand removes device mapper devices and the volume group directory. Whileignore_status=Trueis appropriate for cleanup operations that may not have existing resources, it would be helpful to log the command output for debugging purposes. This can help identify issues if the cleanup partially succeeds or fails.📝 Suggested improvement
+ status, output = remote.run_remote_cmd(clear_dm_dev, params, ignore_status=True) - remote.run_remote_cmd(clear_dm_dev, params, ignore_status=True) + test.log.debug("Device mapper cleanup result (status=%s): %s", status, output)
75-76: Add error handling for firewall port operations.The firewall port addition should verify success before proceeding, as failure to open the port will cause the NBD migration to fail. Consider checking the return value or catching exceptions.
🔒 Suggested improvement for firewall operations
- if disk_type == "nbd": - firewall_cmd.add_port(nbd_server_port, "tcp", permanent=True) - process.run('firewall-cmd --reload') + if disk_type == "nbd": + try: + firewall_cmd.add_port(nbd_server_port, "tcp", permanent=True) + result = process.run('firewall-cmd --reload', verbose=True) + test.log.debug("Firewall port %s opened successfully", nbd_server_port) + except process.CmdError as e: + test.error(f"Failed to open firewall port {nbd_server_port}: {e}")
86-86: Evaluate whether ignoring iSCSI logout errors is appropriate.The
iscsiadm -m session -ucommand logs out of all iSCSI sessions withignore_status=True. If this command fails, it might indicate active iSCSI sessions that weren't properly cleaned up, which could affect subsequent test runs. Consider logging the result to aid debugging.📝 Suggested logging improvement
- remote.run_remote_cmd("iscsiadm -m session -u", params, ignore_status=True) + status, output = remote.run_remote_cmd("iscsiadm -m session -u", params, ignore_status=True) + if status != 0: + test.log.debug("iSCSI logout warning (status=%s): %s", status, output)
91-92: Mirror the error handling from setup for firewall cleanup.The firewall port removal in cleanup should have similar error handling as suggested for the setup to ensure symmetric behavior and proper logging.
♻️ Suggested improvement for cleanup symmetry
- if disk_type == "nbd": - firewall_cmd.remove_port(nbd_server_port, "tcp", permanent=True) - process.run('firewall-cmd --reload') + if disk_type == "nbd": + try: + firewall_cmd.remove_port(nbd_server_port, "tcp", permanent=True) + process.run('firewall-cmd --reload', verbose=True) + test.log.debug("Firewall port %s closed successfully", nbd_server_port) + except Exception as e: + test.log.warning("Failed to close firewall port %s: %s", nbd_server_port, e)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfglibvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.pyprovider/virtual_disk/disk_base.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (2)
libvirt/tests/src/migration/migrate_vm.py (1)
run_remote_cmd(759-776)provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)sync_cpu_for_mig(550-570)
⏰ 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 (4)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (1)
107-112: LGTM: Proper cleanup handling with try-finally.The use of try-finally ensures that cleanup is always executed regardless of test success or failure, which is essential for preventing resource leaks and maintaining a clean test environment.
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg (2)
38-38: No action needed.The
disk_dictsyntax is valid Python dictionary literal syntax and will be properly handled by the test framework. The cartesian config parser performs variable substitution (e.g.,${check_disk_kname_after_mig}→"sda"or"vdb") before the resulting string reachesast.literal_eval()in the test code, ensuring correct parsing.
44-44: No type conversion needed forshared_num.The
shared_numparameter follows the same pattern as other numeric configuration parameters likenbd_server_port, which are also retrieved as strings viaself.params.get()and passed directly toNbdExport()without explicit conversion. The consistent handling across the codebase indicates thatNbdExporthandles string-to-appropriate-type conversion internally or accepts both string and numeric types.Likely an incorrect or invalid review comment.
provider/virtual_disk/disk_base.py (1)
203-203: Verify thatNbdExporthandlesNoneforshared_numparameter.The
shared_numparameter is retrieved usingget()without a default value (line 203), which means it will beNoneif not present in params. While other code paths in the repository omit theshared_numparameter entirely when callingNbdExport, this code explicitly passes it even whenNone. Verify that theNbdExportconstructor gracefully handlesNonefor this parameter, or consider providing a sensible default value here to align with the pattern used for other similar parameters (e.g.,enable_tls,export_name).
meinaLi
force-pushed
the
blk_nbd_disk_migration
branch
from
64ca542 to
e91b585
Compare
|
This PR depends on avocado-framework/avocado-vt#4300. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (3)
73-75: Consider adding explicit error handling for firewall reload.The
firewall-cmd --reloadcommand on line 75 doesn't specify error handling. While the default behavior will raise an exception on failure (which is appropriate for setup), being explicit improves clarity.♻️ Optional: Add explicit error handling
if disk_type == "nbd": firewall_cmd.add_port(nbd_server_port, "tcp", permanent=True) - process.run('firewall-cmd --reload') + process.run('firewall-cmd --reload', shell=True, verbose=True)
89-91: Make firewall cleanup more defensive.In the cleanup path, the
firewall-cmd --reloadon line 91 should useignore_status=Trueto prevent cleanup failures from masking the actual test results or blocking subsequent cleanup steps.♻️ Recommended: Add ignore_status for cleanup robustness
if disk_type == "nbd": firewall_cmd.remove_port(nbd_server_port, "tcp", permanent=True) - process.run('firewall-cmd --reload') + process.run('firewall-cmd --reload', shell=True, ignore_status=True)
96-96: Minor: Fix formatting with double space.Line 96 has a double space before the assignment operator.
♻️ Formatting fix
- client_ip = params.get("client_ip") + client_ip = params.get("client_ip")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfglibvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.pyprovider/virtual_disk/disk_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg
⏰ 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.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
🔇 Additional comments (3)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (2)
17-48: LGTM!The remote iSCSI setup logic is well-structured. The defensive cleanup of existing device mapper devices before creating new ones (lines 37-41) helps prevent conflicts from previous test runs.
106-111: LGTM!The try/finally pattern ensures cleanup runs regardless of test outcome, which is essential for preventing resource leaks in test infrastructure.
provider/virtual_disk/disk_base.py (1)
203-209: Verify that NbdExport accepts theshared_numparameter.The
shared_numparameter is not used in any other NbdExport instantiation throughout the codebase, and thevirttest.utils_nbdmodule source is not available in the repository or public documentation. Confirm that theNbdExportconstructor invirttest.utils_nbdsupports theshared_numparameter and properly handlesNonevalues when the parameter is not configured.
Automate: RHEL-175001 - [Migration] Migrate vm with disk type= block - logical volume based on iscsi VIRT-19098 - [Migration] Migrate with nbd disk defined in xml Signed-off-by: Meina Li <[email protected]>
meinaLi
force-pushed
the
blk_nbd_disk_migration
branch
from
e91b585 to
9423258
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (1)
65-68: Consider using try/finally for session cleanup.If an exception occurs in
get_non_root_disk_nameordd_data_to_vm_disk, the session won't be closed properly.♻️ Suggested improvement
vm_session = vm.wait_for_login() - source_disk_name, _ = libvirt_disk.get_non_root_disk_name(vm_session) - utils_disk.dd_data_to_vm_disk(vm_session, source_disk_name) - vm_session.close() + try: + source_disk_name, _ = libvirt_disk.get_non_root_disk_name(vm_session) + utils_disk.dd_data_to_vm_disk(vm_session, source_disk_name) + finally: + vm_session.close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfglibvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.pyprovider/virtual_disk/disk_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/virtual_disk/disk_base.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (2)
provider/migration/base_steps.py (4)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)verify_default(309-324)provider/virtual_disk/disk_base.py (3)
add_vm_disk(95-112)cleanup_disk_preparation(269-302)DiskBase(25-637)
⏰ 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.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
🔇 Additional comments (6)
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg (2)
1-29: LGTM - Base migration configuration is well-structured.The base configuration correctly sets up NFS storage, migration parameters, and post-migration verification flags. Variable substitutions for migration hosts/credentials follow established patterns.
30-50: LGTM - Test variants are well-defined.The block_disk and nbd_disk variants correctly specify distinct parameters, and the migration option variants (precopy/postcopy) provide appropriate virsh options. The
shared_num = "2"parameter aligns with the NBD export constructor changes indisk_base.py.libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (4)
1-14: LGTM - Imports are appropriate.All imported modules are utilized in the test implementation.
17-48: LGTM - iSCSI remote setup logic is correct.The function correctly sets up a local iSCSI target, performs remote login, clears stale device mappings, and creates the LVM structure on the remote host. The cleanup command properly handles existing VG remnants before recreation.
77-91: LGTM - Cleanup logic correctly handles both disk types.The cleanup properly sequences iSCSI operations (remote logout before local target teardown) and handles firewall port removal for NBD. The
disk_obj.cleanup_disk_preparationhandles the local disk resources created byadd_vm_disk.
93-111: LGTM - Main test flow is well-structured.The parameter extraction, object initialization, and try/finally pattern ensure proper resource management. Using
ast.literal_evalfor parsingdisk_dictis appropriate for safely evaluating the configuration string.
1 participant
Automate:
RHEL-175001 - [Migration] Migrate vm with disk type= block - logical volume based on iscsi
VIRT-19098 - [Migration] Migrate with nbd disk defined in xml
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.