virtual_network: add case for migrating guest while scp files #6672
Conversation
WalkthroughAdds a new Libvirt QEMU migration test that performs file transfer during VM live migration. Introduces a configuration block Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
7b0e98b to
1e791de
Compare
![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
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
48-51: Flow for migrating the VM back looks good; please confirm required params exist.The
migrate_vm_backguard plusmigration_obj.run_migration_back()is consistent withMigrationBase.run_migration_back()and the new cfg setsmigrate_vm_back = "yes", so this path will execute.That helper, however, relies on parameters like
server_ip,server_user,server_pwd,client_ip, andclient_pwdbeing present inparams(often wired frommigrate_source_host/migrate_dest_hostin higher‑level cfg). They’re not visible in this snippet.Can you double‑check that those fields are provided by the surrounding virtual_network/qemu configs for this test variant, so the “migrate back” step doesn’t fail due to missing connection info?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)provider/virtual_network/network_base.py (1)
ping_check(98-149)
⏰ 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). (2)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg
Outdated
Show resolved
Hide resolved
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
1e791de to
053da21
Compare
nanli1
changed the title
|
Migrate when transfer file log |
nanli1
changed the title
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
053da21 to
6f7fe9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
34-36: Consider VM state validation before migration.The
is_alive()check is a basic safeguard, but it doesn't detect states like paused or crashed. If more robust pre-migration state validation isn't already handled byMigrationBase.run_migration(), consider usingvm.wait_for_login()or checkingvm.state()to ensure the VM is fully operational.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
provider/migration/migration_base.py
⏰ 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 (4)
provider/migration/migration_base.py (1)
15-15: LGTM: Import supports dynamic function execution.The
utils_testimport enables dynamic function calls througheval()inparse_funcs(used foraction_during_migparameters). Based on the PR context, the test configuration referencesutils_test.run_file_transferas an action during migration.libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (3)
11-14: LGTM: Imports are appropriate and used.All imports are utilized in the test implementation and follow standard conventions.
58-68: Parameter extraction follows framework patterns.The parameter extraction uses appropriate type-safe methods (
get_boolean,get_numeric) with sensible defaults. Line 66 adds a self-reference (params["params"] = params) which enables nested functions called viaaction_during_migto access the test context objects—this aligns with the pattern used inmigration_base.py'sparse_funcs.
70-73: LGTM: Proper cleanup with try/finally pattern.The try/finally block ensures
cleanup_connection()is always called, even if the test fails. This follows best practices for resource management in test frameworks.
| test.log.info("TEST_STEP 3: Testing network connectivity on destination host") | ||
| backup_uri = vm.connect_uri | ||
| vm.connect_uri = dest_uri | ||
|
|
||
| session = vm.wait_for_login(timeout=login_timeout) | ||
| ips = {'outside_ip': outside_ip} | ||
| network_base.ping_check(params, ips, session) | ||
| session.close() | ||
| vm.connect_uri = backup_uri |
There was a problem hiding this comment.
Wrap URI switch in try/finally to ensure restoration.
If wait_for_login or ping_check raises an exception, line 46 won't execute and vm.connect_uri won't be restored. This could leave the VM object in an inconsistent state for subsequent operations or cleanup.
Apply this diff to ensure the URI is always restored:
test.log.info("TEST_STEP 3: Testing network connectivity on destination host")
backup_uri = vm.connect_uri
vm.connect_uri = dest_uri
-
- session = vm.wait_for_login(timeout=login_timeout)
- ips = {'outside_ip': outside_ip}
- network_base.ping_check(params, ips, session)
- session.close()
- vm.connect_uri = backup_uri
+ try:
+ session = vm.wait_for_login(timeout=login_timeout)
+ ips = {'outside_ip': outside_ip}
+ network_base.ping_check(params, ips, session)
+ session.close()
+ finally:
+ vm.connect_uri = backup_uri🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py around
lines 38 to 46, the code switches vm.connect_uri then calls wait_for_login and
ping_check but if those raise the vm.connect_uri is never restored; wrap the URI
switch and subsequent operations in a try/finally: save backup_uri, set
vm.connect_uri = dest_uri, then in a finally block restore vm.connect_uri =
backup_uri; also ensure any opened session is closed in the finally (check
session was set) so both the VM URI and the session are reliably cleaned up.
6f7fe9d to
64c5e25
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
38-46: Wrap URI switch in try/finally to ensure restoration.If
wait_for_loginorping_checkraises an exception, line 46 won't execute andvm.connect_uriwon't be restored, leaving the VM object in an inconsistent state for subsequent operations or cleanup.Apply this diff to ensure the URI is always restored:
test.log.info("TEST_STEP 3: Testing network connectivity on destination host") backup_uri = vm.connect_uri vm.connect_uri = dest_uri - - session = vm.wait_for_login(timeout=login_timeout) - ips = {'outside_ip': outside_ip} - network_base.ping_check(params, ips, session) - session.close() - vm.connect_uri = backup_uri + try: + session = vm.wait_for_login(timeout=login_timeout) + ips = {'outside_ip': outside_ip} + network_base.ping_check(params, ips, session) + session.close() + finally: + vm.connect_uri = backup_uri
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg
- provider/migration/migration_base.py
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)provider/virtual_network/network_base.py (1)
ping_check(98-149)
⏰ 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.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (4)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (4)
11-14: LGTM!The imports are minimal, necessary, and correctly used throughout the test.
26-36: LGTM!The migration setup sequence is correct: logs guest XML for debugging, prepares connections, ensures the VM is running, and then executes migration. Good defensive check on VM state before migration.
58-68: LGTM!The parameter extraction uses appropriate methods (
get,get_boolean,get_numeric) with sensible defaults. TheMigrationBaseinitialization follows the expected pattern from the migration framework.
70-73: LGTM!The try/finally structure ensures cleanup happens even if the test raises an exception. This is the correct pattern for resource management in test code.
|
|
||
| test.log.info("TEST_STEP 2: Starting migration with file transfers") | ||
| migration_obj.setup_connection() | ||
| if not vm.is_alive(): |
There was a problem hiding this comment.
If start_vm = "yes" is set in the cfg, the VM will be started in migration_obj.setup_connection(). So maybe we can remove lines 34-35.
There was a problem hiding this comment.
Thanks dear, Updated
| migrate_main_vm = "${main_vm}" | ||
| login_timeout = "360" | ||
| migrate_speed = 15 | ||
| iterations = 1 |
There was a problem hiding this comment.
May I ask where this parameter is used?
There was a problem hiding this comment.
Thanks your patience liping, I remove them except migration_speed, I would like it could be added to control my migration speed.
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
64c5e25 to
776662d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
36-44: Make URI switch and session handling exception‑safe.If
vm.wait_for_login()ornetwork_base.ping_check()raises,vm.connect_uriis never restored and the session may remain open, leaving thevmobject in an inconsistent state for later steps/teardown.Wrap the URI switch and session use in a
try/finallyso both the URI and any opened session are always cleaned up, e.g.:- test.log.info("TEST_STEP 3: Testing network connectivity on destination host") - backup_uri = vm.connect_uri - vm.connect_uri = dest_uri - - session = vm.wait_for_login(timeout=login_timeout) - ips = {'outside_ip': outside_ip} - network_base.ping_check(params, ips, session) - session.close() - vm.connect_uri = backup_uri + test.log.info("TEST_STEP 3: Testing network connectivity on destination host") + backup_uri = vm.connect_uri + vm.connect_uri = dest_uri + session = None + try: + session = vm.wait_for_login(timeout=login_timeout) + ips = {'outside_ip': outside_ip} + network_base.ping_check(params, ips, session) + finally: + if session is not None: + session.close() + vm.connect_uri = backup_uri
46-48: Fix TEST_STEP 4 log message formatting for consistency.Other steps use
"TEST_STEP N: ...", but this one is missing the space afterSTEP.- if migrate_vm_back: - test.log.info("TEST_STEP4: Migrating VM back to source host") + if migrate_vm_back: + test.log.info("TEST_STEP 4: Migrating VM back to source host") migration_obj.run_migration_back()
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
30-30: Align TEST_STEP 1 message formatting with other steps (optional).For readability and consistency with steps 2–3, consider adding a space after the colon:
- test.log.info("TEST_STEP 1:Guest xml is: %s\n", VMXML.new_from_dumpxml(vm_name)) + test.log.info("TEST_STEP 1: Guest xml is: %s", VMXML.new_from_dumpxml(vm_name))(The explicit
\nis unnecessary because the logger already appends newlines.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.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.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
66-71: Good use of try/finally around migration lifecycle.Wrapping
run_test()in atry/finallythat always callsteardown_test()(and thusmigration_obj.cleanup_connection()) gives robust cleanup even on failures; this structure looks solid.
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
776662d to
5327266
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
provider/migration/migration_base.py (1)
15-15: Import ofutils_testis correct to support eval-basedaction_during_mighooksBringing
virttest.utils_testinto the module namespace is necessary so thatparse_funcscan successfullyevalstrings like"utils_test.run_file_transfer"coming fromaction_during_migconfig. This aligns with existing patterns in this file and the new migration-with-file-transfer test; no functional issues here.If you want to future‑proof this against “unused import” cleanup, consider adding a short comment such as “needed for eval('utils_test.*') in action_during_mig hooks” above the import.
Based on learnings,
virttest.utils_testis the right place for these helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py
- libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
provider/migration/migration_base.py
⏰ 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.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
|
(1/1) qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-libvirt.virtual_network.qemu_test.migrate_with_file_transfer.q35: PASS (605.60 s) |
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
2 times, most recently
from
ba91d77 to
061964e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
57-65: Wrap URI switch in try/finally to ensure restoration.If
wait_for_loginorping_checkraises an exception,vm.connect_uriwon't be restored, leaving the VM object in an inconsistent state for cleanup operations.test.log.info("TEST_STEP 3: Testing network connectivity on destination host") backup_uri = vm.connect_uri vm.connect_uri = dest_uri - - session = vm.wait_for_login(timeout=login_timeout) - ips = {'outside_ip': outside_ip} - network_base.ping_check(params, ips, session) - session.close() - vm.connect_uri = backup_uri + try: + session = vm.wait_for_login(timeout=login_timeout) + ips = {'outside_ip': outside_ip} + network_base.ping_check(params, ips, session) + session.close() + finally: + vm.connect_uri = backup_uri
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
80-81: Consider usingast.literal_evalinstead ofevalfor safer parsing.
eval()can execute arbitrary code if the config values are compromised. Since these parameters contain dictionary literals,ast.literal_evalprovides the same functionality with better security.+import ast + from virttest.libvirt_xml.vm_xml import VMXML- iommu_dict = eval(params.get('iommu_dict', '{}')) - iface_iommu_driver = eval(params.get('iface_iommu_driver', '{}')) + iommu_dict = ast.literal_eval(params.get('iommu_dict', '{}')) + iface_iommu_driver = ast.literal_eval(params.get('iface_iommu_driver', '{}'))
53-54: Redundant VM start check.Lines 53-54 check
vm.is_alive()and start the VM, but there's already a similar check at lines 47-48 (which has a bug). Once the bug at line 47 is fixed, consider whether both checks are necessary or if one can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/migration/migration_base.py
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (3)
libvirt/tests/src/passthrough/robustness/passthrough_robustness.py (1)
start(176-225)provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.6)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py
80-80: Use of possibly insecure function; consider using ast.literal_eval
(S307)
81-81: 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). (3)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
🔇 Additional comments (5)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg (2)
1-30: Configuration setup looks correct for migration with file transfer.The base configuration properly sets up NFS storage, SSH-based migration with P2P and live options, and includes reasonable defaults for file transfer testing (500MB file, 240s timeout, 15 MB/s migration speed to ensure overlap).
31-31: Verifyaction_during_migparameter format is valid for the migration framework.The
action_during_migusesparams.get('test'),params.get('params'), andparams.get('env')directly in the config file. This assumes the config parser evaluates these expressions at runtime. Confirm this syntax is supported by the test framework's config parsing mechanism, as config files typically use variable substitution (${var}) rather than Python method calls.libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (3)
96-96: Circular reference:params["params"] = params.This creates a self-referential dictionary which could cause issues with serialization, deep copies, or debugging. If the migration framework requires
paramsto be accessible viaparams.get('params'), this pattern works but be aware of potential side effects.Is the circular reference
params["params"] = paramsintentional for theaction_during_migcallback? If so, consider adding a comment explaining why this is needed.
34-41: IOMMU setup function looks correct.The
setup_testfunction properly gates IOMMU configuration behindtest_iommuflag and uses appropriate library functions for interface and IOMMU device modification.
71-75: Teardown correctly placed in finally block.The
teardown_testfunction properly callscleanup_connection()and is invoked from thefinallyblock, ensuring cleanup happens regardless of test outcome.
| s390x: | ||
| iface_iommu_driver = {'name':'vhost','iommu': 'on'} | ||
| - with_virtio_iommu: | ||
| required_qemu= [7.0.0,) |
There was a problem hiding this comment.
Fix syntax: missing space before = in required_qemu.
The config parameter required_qemu= [7.0.0,) is missing a space before the equals sign, which may cause parsing issues depending on the config parser's strictness.
- required_qemu= [7.0.0,)
+ required_qemu = [7.0.0,)📝 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.
| required_qemu= [7.0.0,) | |
| required_qemu = [7.0.0,) |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg around
line 62, the assignment for required_qemu lacks a space before the equals sign
which can break strict parsers; update the line to include a space before and
after the '=' so it reads with proper spacing (e.g., required_qemu = [7.0.0,))
and keep spacing consistent with other config entries.
061964e to
fec97d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg (2)
47-51: Fix indentation forHostCpuVendor.intelandRHEL.7.9blocks.
HostCpuVendor.intel:andRHEL.7.9:are over‑indented underiface_iommu_driver, so theiriommu_dictoverrides may not apply as intended forx86_64, i386. They should be siblings ofiface_iommu_driverunder thex86_64, i386:block.Suggested fix:
- iface_iommu_driver = {'name':'vhost','iommu': 'on', 'ats': 'on', 'disable-legacy':'on', 'disable-modern':'off'} - HostCpuVendor.intel: - iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on'}} - RHEL.7.9: - # For RHEL 7.9 compatibility - disable device IOTLB - iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on', 'device-iotlb': 'off'}} + iface_iommu_driver = {'name':'vhost','iommu': 'on', 'ats': 'on', 'disable-legacy':'on', 'disable-modern':'off'} + HostCpuVendor.intel: + iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on'}} + RHEL.7.9: + # For RHEL 7.9 compatibility - disable device IOTLB + iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on', 'device-iotlb': 'off'}}
62-62: Add spacing around=inrequired_qemufor proper parsing.
required_qemu= [7.0.0,)is missing a space before the equals sign. For consistency and to avoid parser quirks, use spaces around=:- required_qemu= [7.0.0,) + required_qemu = [7.0.0,)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
47-48: Callvm.is_alive()instead of referencing the method.
if not vm.is_alive:checks the method object, which is always truthy, so this branch never runs and the VM is never started here.Update to call the method:
- if not vm.is_alive: + if not vm.is_alive(): vm.start()
57-65: Guardvm.connect_uriand session cleanup withtry/finally.If
wait_for_loginorping_checkraises,vm.connect_uristays pointed atdest_uriand the session may remain open, leaving the VM object in an inconsistent state for later steps/cleanup.Wrapping in
try/finallykeeps the URI and session handling robust:- test.log.info("TEST_STEP 3: Testing network connectivity on destination host") - backup_uri = vm.connect_uri - vm.connect_uri = dest_uri - - session = vm.wait_for_login(timeout=login_timeout) - ips = {'outside_ip': outside_ip} - network_base.ping_check(params, ips, session) - session.close() - vm.connect_uri = backup_uri + test.log.info("TEST_STEP 3: Testing network connectivity on destination host") + backup_uri = vm.connect_uri + vm.connect_uri = dest_uri + + session = None + try: + session = vm.wait_for_login(timeout=login_timeout) + ips = {'outside_ip': outside_ip} + network_base.ping_check(params, ips, session) + finally: + if session is not None: + session.close() + vm.connect_uri = backup_uri
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
80-81: Avoidevalon config values; preferast.literal_evalor typed params.Using
evaloniommu_dict/iface_iommu_driveris unnecessary and flagged by static analysis (S307). These values appear to be simple literals in the cfg, soast.literal_eval(or direct dict parsing) would be safer and clearer.One possible improvement:
+import ast @@ - iommu_dict = eval(params.get('iommu_dict', '{}')) - iface_iommu_driver = eval(params.get('iface_iommu_driver', '{}')) + raw_iommu = params.get('iommu_dict', '{}') + raw_iface = params.get('iface_iommu_driver', '{}') + iommu_dict = ast.literal_eval(raw_iommu) if isinstance(raw_iommu, str) else raw_iommu + iface_iommu_driver = ast.literal_eval(raw_iface) if isinstance(raw_iface, str) else raw_ifacePlease verify that all callers provide literal-compatible values before switching, to avoid breaking any non-literal usages. Based on static analysis hints (S307).
94-95: Avoid makingparamsself-referential for action hooks.
params.update({"params": params, "test": test, "env": env})makesparams["params"]point back to the same dict, creating a recursive structure that can be surprising and harder to inspect or serialize.If the goal is just to expose
test,env, and the original params toaction_during_mig, consider a separate wrapper object instead of self-reference, e.g.:- # Add required objects to params for action_during_mig - params.update({"params": params, "test": test, "env": env}) + # Add required objects for action_during_mig without self-referential params + hook_ctx = {"test": test, "env": env, "params": params} + params["action_hook_ctx"] = hook_ctxand adjust the cfg / migration_base hook to read from
action_hook_ctxinstead ofparams.get('params'). This keeps the dictionary acyclic and easier to reason about.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/migration/migration_base.py
🧰 Additional context used
🪛 Ruff (0.14.6)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py
80-80: Use of possibly insecure function; consider using ast.literal_eval
(S307)
81-81: 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.9
- GitHub Check: Python 3.8
xxxx-95968 - [virtual network][virtual-nic-device] Migrate guest while scp files xxxx-296253 - [virtual network][virtual-nic-device] migrate guest with iommu_platform=on and different memory size Signed-off-by: nanli <nanli@redhat.com>
nanli1
force-pushed
the
add_case_for_migrating_guest_while_scp_files
branch
from
fec97d8 to
03f62d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg (2)
44-49: Fix indentation forHostCpuVendor.inteland nested blocks.The
HostCpuVendor.intel:block (line 45) and its children are over-indented. They should be siblings/children ofx86_64, i386:, not children ofiface_iommu_driver. Current indentation will cause these settings to be ignored or misapplied.iface_iommu_driver = {'name':'vhost','iommu': 'on', 'ats': 'on', 'disable-legacy':'on', 'disable-modern':'off'} - HostCpuVendor.intel: - iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on'}} - RHEL.7.9: - # For RHEL 7.9 compatibility - disable device IOTLB - iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on', 'device-iotlb': 'off'}} + HostCpuVendor.intel: + iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on'}} + RHEL.7.9: + # For RHEL 7.9 compatibility - disable device IOTLB + iommu_dict = {'model': 'intel', 'driver': {'intremap': 'on', 'caching_mode': 'on', 'device-iotlb': 'off'}}
58-58: Fix spacing: missing space before=.For consistency with other config parameters, add a space before the equals sign.
- required_qemu= [7.0.0,) + required_qemu = [7.0.0,)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (2)
57-65: Wrap URI switch in try/finally to ensure restoration.If
wait_for_loginorping_checkraises an exception,vm.connect_uriwon't be restored, leaving the VM object in an inconsistent state for subsequent operations or cleanup.test.log.info("TEST_STEP 3: Testing network connectivity on destination host") backup_uri = vm.connect_uri vm.connect_uri = dest_uri - - session = vm.wait_for_login(timeout=login_timeout) - ips = {'outside_ip': outside_ip} - network_base.ping_check(params, ips, session) - session.close() - vm.connect_uri = backup_uri + try: + session = vm.wait_for_login(timeout=login_timeout) + ips = {'outside_ip': outside_ip} + network_base.ping_check(params, ips, session) + session.close() + finally: + vm.connect_uri = backup_uri
47-48: Critical bug: Missing parentheses onvm.is_alivemethod call.Line 47 uses
vm.is_alivewithout parentheses, which references the method object itself (always truthy) instead of calling it to check VM state. This means the conditionif not vm.is_alive:will always beFalse, and the VM will never be started here.Note: Line 53 correctly uses
vm.is_alive()with parentheses.- if not vm.is_alive: + if not vm.is_alive(): vm.start()
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
80-81: Consider usingast.literal_evalinstead ofevalfor safer parsing.While
eval()is common in this codebase and the input comes from trusted config files,ast.literal_eval()would be safer as it only evaluates Python literals (strings, numbers, dicts, lists, etc.) and cannot execute arbitrary code.+import ast + from virttest.libvirt_xml.vm_xml import VMXML- iommu_dict = eval(params.get('iommu_dict', '{}')) - iface_iommu_driver = eval(params.get('iface_iommu_driver', '{}')) + iommu_dict = ast.literal_eval(params.get('iommu_dict', '{}')) + iface_iommu_driver = ast.literal_eval(params.get('iface_iommu_driver', '{}'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py(1 hunks)provider/migration/migration_base.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
provider/migration/migration_base.py
📚 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/virtual_network/qemu/migrate_with_file_transfer.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (4)
provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/virtual_network/network_base.py (1)
ping_check(98-149)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.14.6)
libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py
80-80: Use of possibly insecure function; consider using ast.literal_eval
(S307)
81-81: 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.9
- GitHub Check: Python 3.8
🔇 Additional comments (3)
provider/migration/migration_base.py (1)
15-15: LGTM!The import follows the existing pattern in this file and is required for
eval()to resolveutils_test.run_file_transferin theaction_during_migconfiguration. Thepylint: disable=W0611comment is appropriate since this module is accessed dynamically.libvirt/tests/cfg/virtual_network/qemu/migrate_with_file_transfer.cfg (1)
1-34: LGTM!The core migration configuration parameters are well-structured. The
action_during_mighook correctly referencesutils_test.run_file_transferwith the required parameter structure that will be resolved at runtime throughparamsupdates in the test file.libvirt/tests/src/virtual_network/qemu/migrate_with_file_transfer.py (1)
99-103: LGTM!The try/finally pattern correctly ensures
teardown_test()is called even if an exception occurs during setup or test execution. Based on tp-libvirt patterns, VM cleanup including destroying running VMs is handled by the migration object's cleanup methods.
2 participants
Signed-off-by: nanli nanli@redhat.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.