Virtual disk: add different migration test cases#6755
Virtual disk: add different migration test cases#6755meinaLi wants to merge 1 commit intoautotest:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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. The Python test exposes 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
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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.
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.
|
|
||
| if disk_type == "nbd": | ||
| firewall_cmd.add_port(nbd_server_port, "tcp", permanent=True) | ||
| process.run('firewall-cmd --reload') |
There was a problem hiding this comment.
'firewall-cmd --reload' is already included in add_port(), so this line can be removed.
There was a problem hiding this comment.
Removed it now, thanks.
| portal_ip=client_ip) | ||
| if disk_type == "nbd": | ||
| firewall_cmd.remove_port(nbd_server_port, "tcp", permanent=True) | ||
| process.run('firewall-cmd --reload') |
meinaLi
force-pushed
the
blk_nbd_disk_migration
branch
from
9423258 to
3f09072
Compare
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 <meili@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py`:
- Around line 73-74: After adding the NBD port with
firewall_cmd.add_port(nbd_server_port, "tcp", permanent=True) when disk_type ==
"nbd", ensure the runtime firewall is updated so the port is actually open: call
the firewall reload (or follow the project pattern by sleeping briefly)
immediately after add_port to apply the change at runtime (mirror the behavior
in provider/migration/base_steps.py where they wait/reload after add_port).
Modify the block handling disk_type == "nbd" to invoke the firewall reload or a
short time.sleep(2) right after firewall_cmd.add_port so the NBD server port
becomes accessible at runtime.
🧹 Nitpick comments (3)
libvirt/tests/cfg/migration/migration_with_disk/migration_with_different_disks.cfg (1)
46-50: Consider increasing postcopy timeout for stability.The
--timeout 10in postcopy migration may be tight for slower environments or larger memory guests, potentially causing premature postcopy triggers or test flakiness.libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py (2)
3-3: Remove unused import.The
processmodule fromavocado.utilsis imported but never used in this file.🧹 Proposed fix
-from avocado.utils import process - from virttest import remote
88-89: Match firewall reload in cleanup for consistency.Similar to the setup, if reload is added after
add_port, it should also be added afterremove_portto ensure the port is closed at runtime.🔧 Proposed fix
if disk_type == "nbd": firewall_cmd.remove_port(nbd_server_port, "tcp", permanent=True) + firewall_cmd.reload()
libvirt/tests/src/migration/migration_with_disk/migration_with_different_disks.py
Show resolved
Hide resolved
meinaLi
force-pushed
the
blk_nbd_disk_migration
branch
from
3f09072 to
d5cdc6a
Compare
|
@iccaszhulili Can you help review this PR? Thanks. |
2 participants
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