Improve migrate_with_bridge_type_interface to adapt to new testing tools#6806
Improve migrate_with_bridge_type_interface to adapt to new testing tools#6806yanglei-rh wants to merge 1 commit intoautotest:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR updates the migrate_with_bridge_type_interface config and test: adds VM lifecycle controls and a Linux-only gate; replaces per-bridge variant blocks with top-level bridge_name and iface_type="bridge"; renames flow steps start_with_interface → start_with_iface and hotplug_interface → hotplug_iface; adjusts migration variant structures. In the Python test, 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (2)
101-108: 🛠️ Refactor suggestion | 🟠 MajorRemove unused
cleanup_nvramfunction.This function is defined but never called anywhere in the test. The NVRAM cleanup is now handled by the framework via
kill_vm_libvirt_options = --nvramin the config file for OVMF guests.🗑️ Suggested removal
- def cleanup_nvram(): - """ - Clean up NVRAM file to avoid UEFI boot issues - """ - 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) -
167-169: 🛠️ Refactor suggestion | 🟠 MajorRemove unused variables:
bridge_type,ovs_bridge_name,network_dict.These variables are retrieved from params but never used in the test logic. They appear to be remnants of the removed legacy bridge setup code.
🗑️ Suggested removal
outside_ip = params.get("outside_ip") - bridge_type = params.get("bridge_type") - ovs_bridge_name = params.get("ovs_bridge_name") - network_dict = eval(params.get("network_dict", "{}")) interface_timing = params.get("interface_timing")
🤖 Fix all issues with AI agents
In
`@libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg`:
- Around line 36-38: The config defines iface_base_attrs but the test builds
iface_attrs directly (see iface_attrs usage in the Python test), so either
remove the unused iface_base_attrs entry or document its purpose; to fix, delete
the iface_base_attrs = {...} line from migrate_with_bridge_type_interface.cfg if
it’s not used, or prepend a one-line comment explaining it’s a
placeholder/example for future tests, or alternatively update the test to read
and use iface_base_attrs when constructing iface_attrs (referencing the
iface_base_attrs symbol in the config) so the config and test stay consistent.
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (3)
76-80: Consider defensive handling for emptynetdstparameter.If
netdstis empty or not set,bridge_namebecomes an empty string, resulting in{'bridge': ''}being set iniface_attrs['source']. This could cause issues during interface attachment.Consider adding validation:
🛡️ Suggested defensive check
bridge_name = params.get('netdst', '').split(',')[0] + if not bridge_name: + test.error("netdst parameter is required but not set") iface_attrs['source'] = {'bridge': bridge_name}
196-196: Remove unused variablenew_xml.This variable is assigned but never used in the test.
🗑️ Suggested removal
vm = env.get_vm(vm_name) - new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
11-11:osimport will be unused after removingcleanup_nvram.If the unused
cleanup_nvramfunction is removed as suggested, this import should also be removed.🗑️ Conditional removal
import aexpect.remote import re -import os
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfg
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22320
branch
from
e2d11b7 to
8cce947
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py`:
- Around line 70-97: The code currently derives bridge_name only from
params.get('netdst') which can be empty; update the logic that sets bridge_name
so it first uses params.get('bridge_name') (or params['bridge_name'] if you want
fail-fast) and falls back to params.get('netdst').split(',')[0] only if
bridge_name is still empty, and if no bridge name is found raise/abort early
with a clear error before assigning iface_attrs['source']; adjust the place that
sets iface_attrs (the iface_attrs dict creation and the line setting
iface_attrs['source']) and ensure network_base.setup_ovs_bridge_attrs(params,
iface_attrs) still runs after a valid source is set.
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
Show resolved
Hide resolved
|
Test pass. |
yanglei-rh
force-pushed
the
LIBVIRTAT-22320
branch
from
8cce947 to
e7ea69c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py (3)
196-196:new_xmlis assigned but never used.
new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)is a leftover from the old implementation. It makes an unnecessary libvirt call and also makesfrom virttest.libvirt_xml import vm_xml(line 15) redundant given the directVMXMLimport on line 17.♻️ Proposed cleanup
- new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) - remote_session = aexpect.remote.remote_login(And remove the now-redundant module-level import:
-from virttest.libvirt_xml import vm_xml from virttest.libvirt_xml.vm_xml import VMXML🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py` at line 196, Remove the unused assignment new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) and any dead call to vm_xml.VMXML.new_from_inactive_dumpxml in this file, and delete the now-redundant module import "from virttest.libvirt_xml import vm_xml" since VMXML is already imported directly; update imports and usages so only the direct VMXML symbol is used.
101-108:cleanup_nvram()is dead code — remove it (andimport os).The function is defined but never invoked anywhere in
run(). NVRAM cleanup is now delegated to the framework viakill_vm_libvirt_options = --nvramin the cfg. Removing it also makesimport os(line 11) unused.♻️ Proposed removal
-import os - ... - def cleanup_nvram(): - """ - Clean up NVRAM file to avoid UEFI boot issues - """ - 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)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py` around lines 101 - 108, Remove the dead cleanup_nvram() helper and the now-unused import os: delete the cleanup_nvram() function definition (it’s never called from run()) and remove the corresponding import os at the top of the file; NVRAM cleanup is handled via kill_vm_libvirt_options = --nvram so no additional code is required.
163-169: Remove unused local variables left over from old bridge-setup code.
bridge_type,ovs_bridge_name, andnetwork_dictare assigned but never referenced after these lines.network_base.setup_ovs_bridge_attrs(params, iface_attrs)reads them directly fromparams, so local extractions are unnecessary.♻️ Proposed cleanup
- bridge_type = params.get("bridge_type") - ovs_bridge_name = params.get("ovs_bridge_name") - network_dict = eval(params.get("network_dict", "{}")) interface_timing = params.get("interface_timing")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py` around lines 163 - 169, Remove the unused local extractions left from the old bridge setup: delete the assignments for bridge_type, ovs_bridge_name, and network_dict (the lines that set bridge_type = params.get("bridge_type"), ovs_bridge_name = params.get("ovs_bridge_name"), and network_dict = eval(params.get("network_dict", "{}"))); network_base.setup_ovs_bridge_attrs(params, iface_attrs) reads these values directly from params so keeping local copies is redundant; keep the needed server_ip/server_user/server_pwd/outside_ip assignments but remove the three unused variables to clean up the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py`:
- Line 196: Remove the unused assignment new_xml =
vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) and any dead call to
vm_xml.VMXML.new_from_inactive_dumpxml in this file, and delete the
now-redundant module import "from virttest.libvirt_xml import vm_xml" since
VMXML is already imported directly; update imports and usages so only the direct
VMXML symbol is used.
- Around line 101-108: Remove the dead cleanup_nvram() helper and the now-unused
import os: delete the cleanup_nvram() function definition (it’s never called
from run()) and remove the corresponding import os at the top of the file; NVRAM
cleanup is handled via kill_vm_libvirt_options = --nvram so no additional code
is required.
- Around line 163-169: Remove the unused local extractions left from the old
bridge setup: delete the assignments for bridge_type, ovs_bridge_name, and
network_dict (the lines that set bridge_type = params.get("bridge_type"),
ovs_bridge_name = params.get("ovs_bridge_name"), and network_dict =
eval(params.get("network_dict", "{}")));
network_base.setup_ovs_bridge_attrs(params, iface_attrs) reads these values
directly from params so keeping local copies is redundant; keep the needed
server_ip/server_user/server_pwd/outside_ip assignments but remove the three
unused variables to clean up the code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfglibvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py
1.Add some parameters to allow the framework provide guest xml 2.Change the method for specifying different bridge types via the test command intead of case cfg varaint 3.Add "only Linux" to limit guest type Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22320
branch
from
e7ea69c to
d46715c
Compare
| 'driver': {'driver_attr': {'queues': str(iface_queues)}} | ||
| } | ||
|
|
||
| bridge_name = params.get('netdst', '').split(',')[0] |
There was a problem hiding this comment.
Hi @yanglei-rh
Please refer to #6796 (comment), which we discussed.
Thanks.
2 participants
1.Add some parameters to allow the framework provide guest xml
2.Change the method for specifying different bridge types via the test command intead of case cfg varaint
3.Add "only Linux" to limit guest type
ID: LIBVIRTAT-22320
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit