add case for migrating guest with bridge interface #6545
Conversation
nanli1
force-pushed
the
add_case_for_migrating_guest_with_bridge_type_interface
branch
2 times, most recently
from
489a1e6 to
27a65b3
Compare
WalkthroughAdds a new test configuration and a test module to exercise VM migration with bridge-type network interfaces (linux_bridge and ovs_bridge), covering precopy/postcopy/cancel migration modes, interface timing (boot vs hotplug), remote SSH destination handling, guest verification, and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Src as Source Host
participant Dst as Destination Host
participant VM as Guest VM
rect rgb(235,245,255)
Runner->>Src: Create bridge/network (linux_bridge or ovs)
Runner->>Src: Clean NVRAM & adjust VM XML
alt start_with_interface
Runner->>Src: Add interface in XML and start VM
else hotplug_interface
Runner->>Src: Start VM then hotplug interface
end
end
rect rgb(245,255,235)
Runner->>Dst: Open SSH session (migration target)
Runner->>Src: Initiate migration (precopy/postcopy/cancel)
Src->>Dst: Transfer state over SSH
Dst->>VM: Activate migrated VM
end
Runner->>VM: Verify driver, multiqueue, and reachability
alt cancel variant
Runner->>Src: Observe cancellation/error handling
end
Runner->>Dst: Optionally migrate back to source
Runner->>Src: Cleanup networks & close sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
nanli1
force-pushed
the
add_case_for_migrating_guest_with_bridge_type_interface
branch
9 times, most recently
from
52747dc to
25d83d3
Compare
nanli1
force-pushed
the
add_case_for_migrating_guest_with_bridge_type_interface
branch
from
25d83d3 to
4608380
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: 2
🧹 Nitpick comments (7)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (7)
40-44: Fix the typo in the docstring parameter.There's a typo in the docstring -
:paramsshould be:param.Apply this fix:
- :params, vm_session: vm session object. + :param vm_session: vm session object.
56-57: Use formatting placeholders correctly in the output assertion.The test uses
%dfor integer formatting but passesiface_queuesdirectly in the regex pattern, which could lead to issues.Apply this fix to properly format the assertion message:
- test.fail("Expected Current hardware settings Combined: %d" % iface_queues) + test.fail("Expected Current hardware settings Combined: %s" % iface_queues)
86-95: Improve NVRAM cleanup implementation.The NVRAM cleanup function has several issues:
- The
import osshould be at module level, not inside the function- Using f-strings is good, but mixing styles with
%selsewhere is inconsistent- The function should use the VM name parameter rather than relying on closure
Consider moving the import to the top of the file and passing
vm_nameas a parameter:- def cleanup_nvram(): + def cleanup_nvram(vm_name): """ Clean up NVRAM file to avoid UEFI boot issues + + :param vm_name: Name of the VM """ - import os nvram_file = f"/var/lib/libvirt/qemu/nvram/{vm_name}_VARS.fd" if os.path.exists(nvram_file): test.log.info(f"Removing NVRAM file: {nvram_file}") os.remove(nvram_file)And update the call site:
- cleanup_nvram() + cleanup_nvram(vm_name)
138-138: Remove unused variable assignment.The variable
backup_uriis assigned but never used in the code. This appears to be dead code.Apply this fix:
- backup_uri, vm.connect_uri = vm.connect_uri, dest_uri + vm.connect_uri = dest_uri
203-203: Remove unused variable assignment.The variable
src_uriis assigned but never used in the code.Apply this fix:
- src_uri = params.get("virsh_migrate_connect_uri") dest_uri = params.get("virsh_migrate_desturi")
143-143: Add error handling for DHCP operations.The DHCP command could fail, which would leave the test in an unpredictable state. Consider adding error handling.
Consider checking the command status:
- vm_session_after_mig.cmd("dhclient -r; dhclient", timeout=120) + status, output = vm_session_after_mig.cmd_status_output("dhclient -r; dhclient", timeout=120) + if status != 0: + test.log.warning("DHCP renewal encountered issues: %s", output)
224-227: Ensure proper resource cleanup in the finally block.The
remote_sessioncheck should verify both existence and that the session is alive before attempting to close it.Consider a more defensive cleanup approach:
finally: teardown_test() - if remote_session: + if remote_session and hasattr(remote_session, 'is_alive') and remote_session.is_alive(): remote_session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (3)
provider/migration/base_steps.py (4)
setup_connection(338-359)run_migration_back(236-280)cleanup_connection(361-371)MigrationBase(27-463)provider/virtual_network/network_base.py (1)
ping_check(94-145)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.13.1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
138-138: Local variable backup_uri is assigned to but never used
Remove assignment to unused variable backup_uri
(F841)
192-192: Use of possibly insecure function; consider using ast.literal_eval
(S307)
203-203: Local variable src_uri is assigned to but never used
Remove assignment to unused variable src_uri
(F841)
205-205: Use of possibly insecure function; consider using ast.literal_eval
(S307)
206-206: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🔇 Additional comments (1)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg (1)
57-57: Review the action_during_mig parameter syntax.The
action_during_migparameter contains complex nested quotes and string formatting which could be error-prone. Consider if this could be simplified or validated.The current value uses nested quotes and string interpolation:
action_during_mig = '[{"func": "virsh.domjobabort", "after_event": "iteration: '1'", "func_param": "'%s' % params.get('migrate_main_vm')"}]'This appears to be mixing JSON-like syntax with Python string formatting. Please verify this is the correct format expected by the test framework.
| migrate_dest_host = "EXAMPLE.DEST.HOST" | ||
| migrate_source_host = "EXAMPLE.SOURCE.HOST" |
There was a problem hiding this comment.
Replace placeholder hostnames with actual test environment values.
The configuration contains placeholder values EXAMPLE.DEST.HOST and EXAMPLE.SOURCE.HOST which need to be replaced with actual hostnames for the test to run.
These placeholder values must be replaced with valid hostnames before the test can execute properly:
- migrate_dest_host = "EXAMPLE.DEST.HOST"
- migrate_source_host = "EXAMPLE.SOURCE.HOST"
+ migrate_dest_host = "<actual_destination_hostname>"
+ migrate_source_host = "<actual_source_hostname>"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg
around lines 21 to 22, the migrate_dest_host and migrate_source_host entries use
placeholder hostnames EXAMPLE.DEST.HOST and EXAMPLE.SOURCE.HOST; replace these
placeholders with the real, reachable hostnames (or FQDNs/IPs) from your test
environment (e.g., dest.example.com and source.example.com or their IPs),
ensuring they match your testbed and any SSH/transport configuration, and verify
connectivity before running the test.
| bridge_name = params.get("bridge_name") | ||
| bridge_type = params.get("bridge_type") | ||
| ovs_bridge_name = params.get("ovs_bridge_name") | ||
| network_dict = eval(params.get("network_dict", "{}")) |
There was a problem hiding this comment.
Replace eval() with ast.literal_eval() for safer parsing.
Using eval() is a security risk as it can execute arbitrary Python code. Use ast.literal_eval() instead, which safely evaluates literal expressions.
First, add the import at the top of the file:
import astThen apply these fixes:
- network_dict = eval(params.get("network_dict", "{}"))
+ network_dict = ast.literal_eval(params.get("network_dict", "{}"))- expected_xpath = eval(params.get("expected_xpath"))
+ expected_xpath = ast.literal_eval(params.get("expected_xpath"))- iface_dict = eval(params.get("iface_dict", "{}"))
+ iface_dict = ast.literal_eval(params.get("iface_dict", "{}"))Also applies to: 205-206
🧰 Tools
🪛 Ruff (0.13.1)
192-192: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
around line 192 (and also lines 205-206), replace the insecure use of
eval(params.get("network_dict", "{}")) with ast.literal_eval for safe parsing;
add "import ast" at the top of the file, and change the two other eval usages on
lines 205-206 to ast.literal_eval as well, ensuring you handle potential
ValueError/SyntaxError from literal_eval if needed (e.g., wrap in try/except or
provide a safe default).
| migrate_dest_host = "EXAMPLE.DEST.HOST" | ||
| migrate_source_host = "EXAMPLE.SOURCE.HOST" | ||
| nfs_server_ip = "${migrate_source_host}" | ||
| migrate_desturi_port = "49152-49216" |
There was a problem hiding this comment.
Generally, for 'qemu+tcp://...', migrate_desturi_port = 16509.
For 'qemu+ssh://...', migrate_desturi_port = 22.
There was a problem hiding this comment.
Thanks liping ,Updated :D
| new_queues = "3" | ||
| ssh_remote_auth = True | ||
| unprivileged_user = "" | ||
| migrate_dest_host = "EXAMPLE.DEST.HOST" |
There was a problem hiding this comment.
The migrate_dest_host/migrate_source_host is already defined in the base.cfg.
There was a problem hiding this comment.
Thanks liping ,Updated :D
| from virttest.utils_libvirt import libvirt_network | ||
| from virttest.utils_libvirt import libvirt_vmxml | ||
| from virttest.libvirt_xml.vm_xml import VMXML | ||
| import aexpect.remote |
There was a problem hiding this comment.
Maybe this line should be moved above.
There was a problem hiding this comment.
Thanks liping ,Updated :D lala
| """ | ||
| Clean up NVRAM file to avoid UEFI boot issues | ||
| """ | ||
| import os |
There was a problem hiding this comment.
Maybe this line should be moved above.
There was a problem hiding this comment.
Thanks liping ,Updated :D
| migration_obj.cleanup_connection() | ||
|
|
||
| # Params to update disk using shared storage | ||
| params["disk_type"] = params.get("disk_type", "file") |
There was a problem hiding this comment.
Since disk_type and disk_source_protocol are already defined in the cfg file, maybe L181 and L182 can be remove.
There was a problem hiding this comment.
Thanks liping ,Updated :D
| # Params to update disk using shared storage | ||
| params["disk_type"] = params.get("disk_type", "file") | ||
| params["disk_source_protocol"] = params.get("disk_source_protocol", "netfs") | ||
| params["mnt_path_name"] = params.get("nfs_mount_dir") |
There was a problem hiding this comment.
Maybe this line can be removed. Add 'mnt_path_name = ${nfs_mount_dir}' in cfg file.
There was a problem hiding this comment.
Thanks liping ,Updated :D
| vm_name = guest_os.get_vm(params) | ||
| vm = env.get_vm(vm_name) | ||
| new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
| backup_xml = new_xml.copy() |
There was a problem hiding this comment.
L61 in tp-libvirt/provider/migration/base_steps.py already backs up the XML, so this line might be unnecessary.
There was a problem hiding this comment.
Thanks liping ,Updated :D
nanli1
force-pushed
the
add_case_for_migrating_guest_with_bridge_type_interface
branch
from
4608380 to
90174f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (3)
provider/migration/base_steps.py (4)
setup_connection(338-359)run_migration_back(236-280)cleanup_connection(361-371)MigrationBase(27-463)provider/virtual_network/network_base.py (1)
ping_check(94-145)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.13.1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
138-138: Local variable backup_uri is assigned to but never used
Remove assignment to unused variable backup_uri
(F841)
186-186: Use of possibly insecure function; consider using ast.literal_eval
(S307)
197-197: Local variable src_uri is assigned to but never used
Remove assignment to unused variable src_uri
(F841)
199-199: Use of possibly insecure function; consider using ast.literal_eval
(S307)
200-200: Use of possibly insecure function; consider using ast.literal_eval
(S307)
207-207: Local variable new_xml is assigned to but never used
Remove assignment to unused variable new_xml
(F841)
⏰ 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.12
- GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (1)
186-200: Replace eval() with ast.literal_eval() for params parsingUsing
eval()onparams[...]exposes the test harness to arbitrary code execution whenever the parameter content is attacker-controlled (cfg files, environment). This was flagged previously and still needs to be fixed. Please switch toast.literal_eval()(or explicit JSON parsing) and surface a clear failure if parsing fails. Remember to addimport astat the top.Apply this diff to harden the parsing:
- network_dict = eval(params.get("network_dict", "{}")) + try: + network_dict = ast.literal_eval(params.get("network_dict", "{}")) + except (ValueError, SyntaxError) as err: + test.fail(f"Invalid network_dict: {err}") @@ - expected_xpath = eval(params.get("expected_xpath")) + try: + expected_xpath = ast.literal_eval(params.get("expected_xpath")) + except (ValueError, SyntaxError) as err: + test.fail(f"Invalid expected_xpath: {err}") @@ - iface_dict = eval(params.get("iface_dict", "{}")) + try: + iface_dict = ast.literal_eval(params.get("iface_dict", "{}")) + except (ValueError, SyntaxError) as err: + test.fail(f"Invalid iface_dict: {err}")Add this import near the top of the file:
import ast
| if not vm.is_alive(): | ||
| vm.start() | ||
| iface = libvirt_vmxml.create_vm_device_by_type('interface', iface_dict) | ||
| virsh.attach_device(vm_name, iface.xml, flagstr="--config", | ||
| debug=True, ignore_status=False) |
There was a problem hiding this comment.
Hotplug path never attaches the interface to the running guest
Line 77 currently calls virsh attach-device with only --config, while the VM is already running. That updates the persistent XML but does not hotplug into the live guest, so the hotplug cases run migration without the intended bridge interface. Please pass --live together with --config (or stop the VM before the attach) to ensure the interface is present during migration.
Apply this diff to fix the issue:
- virsh.attach_device(vm_name, iface.xml, flagstr="--config",
+ virsh.attach_device(vm_name, iface.xml, flagstr="--config --live",
debug=True, ignore_status=False)📝 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() | |
| iface = libvirt_vmxml.create_vm_device_by_type('interface', iface_dict) | |
| virsh.attach_device(vm_name, iface.xml, flagstr="--config", | |
| debug=True, ignore_status=False) | |
| if not vm.is_alive(): | |
| vm.start() | |
| iface = libvirt_vmxml.create_vm_device_by_type('interface', iface_dict) | |
| virsh.attach_device(vm_name, iface.xml, flagstr="--config --live", | |
| debug=True, ignore_status=False) |
🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
around lines 74 to 78, the hotplug path only updates persistent XML by calling
virsh.attach_device with "--config" while the VM is running; change the attach
call to include the live flag (pass "--config --live" as the flagstr) so the
interface is hotplugged into the running guest (alternatively, stop the VM
before attaching), ensuring the interface is present during migration.
90174f3 to
becd5a4
Compare
xxxx-299038: [bridge] migrate guest with bridge type interface Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_migrating_guest_with_bridge_type_interface
branch
from
becd5a4 to
e2d7c5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (5)
70-72: Avoid shadowing module namevm_xmlwith a local variableThis shadows the imported module and hurts readability.
- vm_xml = VMXML.new_from_inactive_dumpxml(vm_name) - vm_xml.remove_all_device_by_type('interface') - vm_xml.sync() + vmxml_obj = VMXML.new_from_inactive_dumpxml(vm_name) + vmxml_obj.remove_all_device_by_type('interface') + vmxml_obj.sync()
41-65: Close session after checks to avoid leaking a guest session
vm_session_after_migis never closed.libvirt_vmxml.check_guest_xml_by_xpaths( VMXML.new_from_dumpxml(vm_name, virsh_instance=virsh_obj), expected_xpath) + vm_session_after_mig.close()
138-138: Remove unusedbackup_uriassignmentIt’s unused and flagged by lint.
As per static analysis hints.
- backup_uri, vm.connect_uri = vm.connect_uri, dest_uri + vm.connect_uri = dest_uri
197-197: Remove unused variablesrc_uriNot referenced later.
As per static analysis hints.
- src_uri = params.get("virsh_migrate_connect_uri")
207-207: Remove unused variablenew_xmlNot referenced later.
As per static analysis hints.
- new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg (3)
14-14: Prefer an IP to avoid DNS dependency for outside pingUsing a hostname can fail in DNS-restricted labs. Consider a well-known IP.
- outside_ip = "www.google.com" + outside_ip = "1.1.1.1"Verify reachability in your environment before changing.
33-34: Use a less generic bridge name to avoid collisions
br0is commonly present; use a test-specific name.- bridge_name = "br0" + bridge_name = "testbr0"
28-28: Remove unused knob or gate behavior with it
check_network_accessibility_after_migisn’t consumed in the test; either remove or gate the ping step with it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (3)
provider/migration/base_steps.py (4)
setup_connection(338-359)run_migration_back(236-280)cleanup_connection(361-371)MigrationBase(27-463)provider/virtual_network/network_base.py (1)
ping_check(94-145)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.13.1)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
138-138: Local variable backup_uri is assigned to but never used
Remove assignment to unused variable backup_uri
(F841)
186-186: Use of possibly insecure function; consider using ast.literal_eval
(S307)
197-197: Local variable src_uri is assigned to but never used
Remove assignment to unused variable src_uri
(F841)
199-199: Use of possibly insecure function; consider using ast.literal_eval
(S307)
200-200: Use of possibly insecure function; consider using ast.literal_eval
(S307)
207-207: Local variable new_xml is assigned to but never used
Remove assignment to unused variable new_xml
(F841)
⏰ 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 (2)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (2)
73-79: Hotplug attach is not live; add --live to actually hotplugUsing only --config updates persistent XML but does not hotplug into the running guest.
Apply:
- virsh.attach_device(vm_name, iface.xml, flagstr="--config", + virsh.attach_device(vm_name, iface.xml, flagstr="--config --live", debug=True, ignore_status=False)
186-186: Replace eval() with ast.literal_eval() and handle parse errors
evalon params is unsafe and flagged by lint.+import ast- network_dict = eval(params.get("network_dict", "{}")) + try: + network_dict = ast.literal_eval(params.get("network_dict", "{}")) + except (ValueError, SyntaxError): + test.error("Invalid network_dict literal in params")- expected_xpath = eval(params.get("expected_xpath")) + try: + expected_xpath = ast.literal_eval(params.get("expected_xpath")) + except (ValueError, SyntaxError): + test.error("Invalid expected_xpath literal in params")- iface_dict = eval(params.get("iface_dict", "{}")) + try: + iface_dict = ast.literal_eval(params.get("iface_dict", "{}")) + except (ValueError, SyntaxError): + test.error("Invalid iface_dict literal in params")Run to find any other unsafe evals:
#!/bin/bash rg -nP --type=py -C2 '\beval\s*\('Also applies to: 199-200
|
@yalzhang hi yalan , Could you help review this one? |
|
@chloerh hi haijiao, Could you help review |
3 participants
xxxx-299038: [bridge] migrate guest with bridge type interface
Signed-off-by: nanli [email protected]
depend on avocado-framework/avocado-vt#4241
Summary by CodeRabbit