Add case to support 8021q vlan among guests and host #6623
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 6
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/bridge_vlan.py (1)
214-233: Restarting guest network after manual VLAN config can drop the VLAN.
utils_net.restart_guest_network(session)may revert NM/ifcfg-managed state and remove your ad-hocip link addVLANs. Given you manually configured interfaces and also stopped NM earlier, consider removing this restart or replacing it with a targetedip link set <vlan_if> up.Example:
- utils_net.restart_guest_network(session) + session.cmd_output_safe("ip link set dev %s up" % vm_vlan_if[vm_index])Also applies to: 239-242
libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg (1)
33-34: Firewall/NM controls are heavy-handed across distros.Mixing
service iptables stopand systemd may fail on newer OSes. Consider gating by distro/version or usingsystemctl stop firewalld || trueonly, and avoid restarting NM mid-test (see code comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_vlan.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_vlan.py (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🔇 Additional comments (3)
libvirt/tests/src/virtual_network/qemu/bridge_vlan.py (2)
7-16: Custom NetPingError looks good.Clear message with destination and details; consistent with utils_net.NetError.
110-126: Confirm argument semantics ofutils_test.pingwithinterfaceparameter.The code passes
vlan_ifto theinterfaceparameter ofutils_test.ping(). However:
- The documented
virttest.utils_test.pingsignature usesiface=None(notinterface=)- Other code in the repository passes an IP address to
interface(e.g.,libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py:217)- The code under review passes a device name (
vlan_if)Verify that
utils_test.pingin your virttest version actually accepts theinterfaceparameter (notiface) and that it interprets device names correctly (asping -I <ifname>) rather than as source IPs.libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg (1)
1-3: Verify that the "switch" bridge is available on test hosts, or explicitly setnetdst.The configuration file
libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfgdoes not define thenetdstparameter. The test code atlibvirt/tests/src/virtual_network/qemu/bridge_vlan.py:151defaults tohost_br = params.get("netdst", "switch"), meaning the code will attempt to use a bridge named "switch".Either:
- Add
netdst = "switch"(or appropriate bridge name) explicitly to the config to make the requirement clear- Or ensure test environments have a bridge named "switch" available
| main_vm = avocado-vt-vm1 | ||
| type = bridge_vlan | ||
| vms += " vm2" |
There was a problem hiding this comment.
Potential trailing space in main_vm may break env lookups.
main_vm = avocado-vt-vm1 appears to include a trailing space. This can cause env.get_vm(params["main_vm"]) to fail.
- main_vm = avocado-vt-vm1
+ main_vm = avocado-vt-vm1📝 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.
| main_vm = avocado-vt-vm1 | |
| type = bridge_vlan | |
| vms += " vm2" | |
| main_vm = avocado-vt-vm1 | |
| type = bridge_vlan | |
| vms += " vm2" |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg around lines 7 to 9,
the value for main_vm has a trailing space which can break env lookups; remove
the trailing space so the line reads exactly main_vm = avocado-vt-vm1, and while
here ensure the vms line has no unintended leading/trailing spaces (e.g., vms +=
"vm2") or normalize spacing; save and run the test to confirm
env.get_vm(params["main_vm"]) resolves correctly.
| deviation_time = 20 | ||
| netperf_output_unit = m | ||
| netperf_local_cpu = yes | ||
| netperf_remote_cpu = yes | ||
| disable_firewall = "service iptables stop; systemctl stop firewalld.service" | ||
| disable_nm = "systemctl stop NetworkManager" | ||
| deviation_time = 3 |
There was a problem hiding this comment.
Duplicate deviation_time keys; last one silently wins.
Both 20 and 3 are set; only the last will apply. Pick one to avoid confusion.
- deviation_time = 20
@@
- deviation_time = 3
+ deviation_time = 20 # or 3, but keep a single value📝 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.
| deviation_time = 20 | |
| netperf_output_unit = m | |
| netperf_local_cpu = yes | |
| netperf_remote_cpu = yes | |
| disable_firewall = "service iptables stop; systemctl stop firewalld.service" | |
| disable_nm = "systemctl stop NetworkManager" | |
| deviation_time = 3 | |
| netperf_output_unit = m | |
| netperf_local_cpu = yes | |
| netperf_remote_cpu = yes | |
| disable_firewall = "service iptables stop; systemctl stop firewalld.service" | |
| disable_nm = "systemctl stop NetworkManager" | |
| deviation_time = 20 # or 3, but keep a single value |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg around lines 29 to 35
there are duplicate deviation_time entries (20 and 3) so the last one silently
overrides the first; remove one of the two entries so only the intended
deviation_time remains (decide whether the correct value is 20 or 3), keep a
single deviation_time line with the chosen value, and run the config/linters or
tests to confirm no other duplicate keys exist.
| def add_vlan(interface, v_id, session=None): | ||
| """ | ||
| Create a vlan-device on interface. | ||
| :params interface: Interface. | ||
| :params v_id: Vlan id. | ||
| :params session: VM session or none. | ||
| """ | ||
| vlan_if = "%s.%s" % (interface, v_id) | ||
| add_cmd = params["add_vlan_cmd"] % (interface, vlan_if, v_id) | ||
| error_context.context( | ||
| "Create vlan interface '%s' on %s" % (vlan_if, interface), test.log.info | ||
| ) | ||
| if session: | ||
| session.cmd_output_safe(add_cmd) | ||
| else: | ||
| process.system(add_cmd) | ||
| return vlan_if | ||
|
|
There was a problem hiding this comment.
Host-side VLAN creation uses process.system without shell; command may not execute.
process.system(add_cmd) is invoked without shell=True. You already wrapped _system() to force shell=True. Use it to avoid failures on host-side VLAN creation.
- else:
- process.system(add_cmd)
+ else:
+ _system(add_cmd)📝 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.
| def add_vlan(interface, v_id, session=None): | |
| """ | |
| Create a vlan-device on interface. | |
| :params interface: Interface. | |
| :params v_id: Vlan id. | |
| :params session: VM session or none. | |
| """ | |
| vlan_if = "%s.%s" % (interface, v_id) | |
| add_cmd = params["add_vlan_cmd"] % (interface, vlan_if, v_id) | |
| error_context.context( | |
| "Create vlan interface '%s' on %s" % (vlan_if, interface), test.log.info | |
| ) | |
| if session: | |
| session.cmd_output_safe(add_cmd) | |
| else: | |
| process.system(add_cmd) | |
| return vlan_if | |
| def add_vlan(interface, v_id, session=None): | |
| """ | |
| Create a vlan-device on interface. | |
| :params interface: Interface. | |
| :params v_id: Vlan id. | |
| :params session: VM session or none. | |
| """ | |
| vlan_if = "%s.%s" % (interface, v_id) | |
| add_cmd = params["add_vlan_cmd"] % (interface, vlan_if, v_id) | |
| error_context.context( | |
| "Create vlan interface '%s' on %s" % (vlan_if, interface), test.log.info | |
| ) | |
| if session: | |
| session.cmd_output_safe(add_cmd) | |
| else: | |
| _system(add_cmd) | |
| return vlan_if |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_vlan.py around lines 47 to 64,
the host-side VLAN creation calls process.system(add_cmd) which does not force a
shell and can fail for complex commands; replace that call with the internal
wrapper that forces shell=True (i.e., use process._system(add_cmd) or the
provided _system() wrapper) so the command executes correctly on the host,
keeping the session branch unchanged and preserving existing error handling and
return of vlan_if.
| def set_ip_vlan(vlan_if, vlan_ip, session=None): | ||
| """ | ||
| Set ip address of vlan interface. | ||
| :params vlan_if: Vlan interface. | ||
| :params vlan_ip: Vlan internal ip. | ||
| :params session: VM session or none. | ||
| """ | ||
| error_context.context( | ||
| "Assign IP '%s' to vlan interface '%s'" % (vlan_ip, vlan_if), test.log.info | ||
| ) | ||
| if session: | ||
| disable_firewall = params.get("disable_firewall", "") | ||
| session.cmd_output_safe(disable_firewall) | ||
| disable_nm = params.get("disable_nm", "") | ||
| session.cmd_output_safe(disable_nm) | ||
| session.cmd_output_safe("ifconfig %s 0.0.0.0" % vlan_if) | ||
| session.cmd_output_safe("ifconfig %s down" % vlan_if) | ||
| session.cmd_output_safe("ifconfig %s %s up" % (vlan_if, vlan_ip)) | ||
| else: | ||
| process.system("ifconfig %s %s up" % (vlan_if, vlan_ip)) | ||
|
|
There was a problem hiding this comment.
Host-side IP assignment also misses shell=True.
Same issue as above; the ifconfig command may not run correctly without shell=True.
- else:
- process.system("ifconfig %s %s up" % (vlan_if, vlan_ip))
+ else:
+ _system("ifconfig %s %s up" % (vlan_if, vlan_ip))Also consider moving to ip addr add <ip>/24 dev <if>; ip link set <if> up for consistency with other ip usage.
Committable suggestion skipped: line range outside the PR's diff.
| def set_mac_vlan(vlan_if, mac_str, session): | ||
| """ | ||
| Give a new mac address for vlan interface in guest. | ||
| :params: vlan_if: Vlan interface. | ||
| :params: mac_str: New mac address for vlan. | ||
| :params: session: VM session. | ||
| """ | ||
| mac_cmd = "ip link set %s add %s up" % (vlan_if, mac_str) | ||
| error_context.context( | ||
| "Give a new mac address '%s' for vlan interface '%s'" % (mac_str, vlan_if), | ||
| test.log.info, | ||
| ) | ||
| session.cmd_output_safe(mac_cmd) | ||
|
|
There was a problem hiding this comment.
Broken MAC change command on VLAN iface (will fail).
ip link set %s add %s up is invalid; the keyword must be address (optionally with dev). This will make MAC assignment silently fail or error out, breaking downstream connectivity checks.
Apply:
- mac_cmd = "ip link set %s add %s up" % (vlan_if, mac_str)
+ mac_cmd = "ip link set dev %s address %s up" % (vlan_if, mac_str)📝 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.
| def set_mac_vlan(vlan_if, mac_str, session): | |
| """ | |
| Give a new mac address for vlan interface in guest. | |
| :params: vlan_if: Vlan interface. | |
| :params: mac_str: New mac address for vlan. | |
| :params: session: VM session. | |
| """ | |
| mac_cmd = "ip link set %s add %s up" % (vlan_if, mac_str) | |
| error_context.context( | |
| "Give a new mac address '%s' for vlan interface '%s'" % (mac_str, vlan_if), | |
| test.log.info, | |
| ) | |
| session.cmd_output_safe(mac_cmd) | |
| def set_mac_vlan(vlan_if, mac_str, session): | |
| """ | |
| Give a new mac address for vlan interface in guest. | |
| :params: vlan_if: Vlan interface. | |
| :params: mac_str: New mac address for vlan. | |
| :params: session: VM session. | |
| """ | |
| mac_cmd = "ip link set dev %s address %s up" % (vlan_if, mac_str) | |
| error_context.context( | |
| "Give a new mac address '%s' for vlan interface '%s'" % (mac_str, vlan_if), | |
| test.log.info, | |
| ) | |
| session.cmd_output_safe(mac_cmd) | |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_vlan.py around lines 86 to 99,
the ip command used to set the MAC on the VLAN interface is invalid ("ip link
set %s add %s up"); replace it with a valid form using the address keyword (for
example "ip link set dev <vlan_if> address <mac_str> up" or "ip link set
<vlan_if> address <mac_str> up") so the MAC assignment actually takes effect,
and keep calling session.cmd_output_safe(...) with the corrected command string.
| def netperf_vlan(client="main_vm", server="localhost", sub_type="netperf_stress"): | ||
| """ | ||
| Test netperf stress among guests and host. | ||
| :params client: Netperf client. | ||
| :params server: Netperf server. | ||
| :params sub_type: Sub_type to run. | ||
| """ | ||
| params["netperf_client"] = client | ||
| params["netperf_server"] = server | ||
| error_context.context( | ||
| "Run netperf stress test among guests and host, " | ||
| "server: %s, client: %s" % (server, client), | ||
| test.log.info, | ||
| ) | ||
| session.cmd_output_safe("systemctl restart NetworkManager") | ||
| utils_test.run_virt_sub_test(test, params, env, sub_type) | ||
|
|
There was a problem hiding this comment.
Fragile closure over session and unintended NetworkManager restart.
netperf_vlan()uses an implicit outersessionvariable; outside the per-VM loop it will point to the last guest’s session (vm2), not the intended client. This is brittle and can target the wrong VM.- You previously stop NetworkManager in guests (
disable_nm), butnetperf_vlan()unconditionally restarts it, risking loss of manual VLAN/IFCFG state and test flakiness.
Make session explicit and gate NM restart:
-def netperf_vlan(client="main_vm", server="localhost", sub_type="netperf_stress"):
+def netperf_vlan(client="main_vm", server="localhost", sub_type="netperf_stress", session=None):
@@
- session.cmd_output_safe("systemctl restart NetworkManager")
+ # Optional: only if explicitly requested; avoid undoing manual net config
+ if session and params.get("netperf_restart_nm", "no") == "yes":
+ session.cmd_output_safe("systemctl restart NetworkManager || true")
utils_test.run_virt_sub_test(test, params, env, sub_type)Update call sites to pass the intended client’s session:
- netperf_vlan(client=vm.name, server="localhost")
+ netperf_vlan(client=vm.name, server="localhost", session=session)
@@
- netperf_vlan(client=params["main_vm"], server="vm2")
+ # Use main_vm as client explicitly
+ netperf_vlan(client=params["main_vm"], server="vm2", session=sessions[0])Also applies to: 239-242, 248-250
Automate VIRT-10613 8021q vlan among guests and host Signed-off-by: Lei Yang <[email protected]>
yanglei-rh
force-pushed
the
LIBVIRTAT-22106
branch
from
3ea820f to
032dc0d
Compare
|
@nanli1 Please help review this patch, thanks a lot. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_vlan.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_vlan.py (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/qemu/bridge_vlan.py (1)
1-21: LGTM! Clean imports and helper utilities.The
NetPingErrorexception class is well-defined with proper inheritance and error formatting. The_system()wrapper correctly forcesshell=Truefor all shell commands.
| host_vlan_id = params.get("host_vlan_id", "10") | ||
| host_vlan_ip = params.get("host_vlan_ip", "192.168.10.10") | ||
| subnet = params.get("subnet", "192.168") | ||
| mac_str = params.get("mac_str").split(",") |
There was a problem hiding this comment.
Missing parameter will raise AttributeError.
If mac_str is not defined in the config, params.get("mac_str") returns None, and calling .split(",") on None raises AttributeError.
Apply this fix:
- mac_str = params.get("mac_str").split(",")
+ mac_str = params.get("mac_str", "").split(",")Or validate that the parameter exists:
- mac_str = params.get("mac_str").split(",")
+ mac_str = params.get("mac_str")
+ if not mac_str:
+ test.cancel("mac_str parameter is required")
+ mac_str = mac_str.split(",")📝 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.
| mac_str = params.get("mac_str").split(",") | |
| mac_str = params.get("mac_str", "").split(",") |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_vlan.py around line 155,
params.get("mac_str").split(",") will raise AttributeError when mac_str is
missing; update the code to handle a missing or None mac_str by either providing
a safe default (e.g., use params.get("mac_str", "") and then split only if
non-empty, producing an empty list when absent) or explicitly validate and raise
a clear error if mac_str is required before calling split.
| for vm_index, vm in enumerate(vms): | ||
| session = sessions[vm_index] | ||
| error_context.context("Add vlan interface on guest '%s'" % vm.name) | ||
| session.cmd_output("ifconfig %s 0.0.0.0" % ifname[vm_index], safe=True) |
There was a problem hiding this comment.
Incorrect API usage: cmd_output does not accept safe parameter.
The method is cmd_output_safe(), not cmd_output(..., safe=True). This will raise TypeError: cmd_output() got an unexpected keyword argument 'safe'.
Apply this fix:
- session.cmd_output("ifconfig %s 0.0.0.0" % ifname[vm_index], safe=True)
+ session.cmd_output_safe("ifconfig %s 0.0.0.0" % ifname[vm_index])📝 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.
| session.cmd_output("ifconfig %s 0.0.0.0" % ifname[vm_index], safe=True) | |
| session.cmd_output_safe("ifconfig %s 0.0.0.0" % ifname[vm_index]) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_vlan.py around line 224, the
call session.cmd_output("ifconfig %s 0.0.0.0" % ifname[vm_index], safe=True)
uses a non-existent safe kwarg; replace it with the correct API call
session.cmd_output_safe("ifconfig %s 0.0.0.0" % ifname[vm_index]) (preserve the
same command string and remove the safe keyword).
1 participant
Automate VIRT-10613 8021q vlan among guests and host
ID: LIBVIRTAT-22106
Signed-off-by: Lei Yang [email protected]
Summary by CodeRabbit