Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions libvirt/tests/cfg/virtual_network/qemu/bridge_vlan.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
- virtual_network.qemu_test.bridge_vlan: image_copy
only Linux
only bridge
create_vm_libvirt = yes
master_images_clone = img1
kill_vm_libvirt = yes
main_vm = avocado-vt-vm1
type = bridge_vlan
vms += " vm2"
host_vlan_id = 10
subnet = "192.168"
host_vlan_ip = "${subnet}.${host_vlan_id}.10"
mac_str = 54:52:00:01:0a:01,54:52:00:01:0a:02
add_vlan_cmd = "ip link add link %s name %s type vlan id %s"
rm_host_vlan_cmd = "ip link delete %s type vlan"
# netperf stress config
netperf_link = netperf-2.7.1.tar.bz2
server_path = /var/tmp/
client_path = /var/tmp/
netperf_test_duration = 120
netperf_para_sessions = 1
test_protocols = UDP_STREAM
netperf_client = ${main_vm}
netperf_server = vm2
sub_type = netperf_stress
sub_exit_timeout = 10
# netperf vlan config
netperf_vlan_test = yes
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
Comment on lines +29 to +35
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

266 changes: 266 additions & 0 deletions libvirt/tests/src/virtual_network/qemu/bridge_vlan.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,266 @@
import time

from avocado.utils import linux_modules, process
from virttest import error_context, funcatexit, utils_net, utils_test


class NetPingError(utils_net.NetError):
def __init__(self, src, dst, details):
utils_net.NetError.__init__(self, src, details)
self.dst = dst

def __str__(self):
e_msg = "Can't ping from %s to %s" % (self.src, self.dst)
if self.details is not None:
e_msg += " : %s" % self.details
return e_msg


def _system(*args, **kwargs):
kwargs["shell"] = True
return process.system(*args, **kwargs)


@error_context.context_aware
def run(test, params, env):
"""
Test 802.1Q vlan of NIC among guests and host with linux bridge backend.

1) Configure vlan interface over host bridge interface.
2) Create two VMs over vlan interface.
3) Load 8021q module in guest.
4) Configure ip address of guest with 192.168.*.*
5) Test by ping between guest and host, should fail.
6) Test by ping beween guests, should pass.
7) Setup vlan in guests and using hard-coded ip address 192.168.*.*
8) Test by ping between guest and host, should pass.
9) Test by ping among guests, should pass.
10) Test by netperf between guests and host.
11) Test by netperf between guests.
12) Delete vlan interface in host.

:param test: Libvirt test object.
:param params: Dictionary with the test parameters.
:param env: Dictionary with test environment.
"""

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

Comment on lines +47 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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))

Comment on lines +65 to +85
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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)

Comment on lines +86 to +99
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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 set_arp_ignore(session):
"""
Enable arp_ignore for all ipv4 device in guest
"""
error_context.context(
"Enable arp_ignore for all ipv4 device in guest", test.log.info
)
ignore_cmd = "echo 1 > /proc/sys/net/ipv4/conf/all/arp_ignore"
session.cmd_output_safe(ignore_cmd)

def ping_vlan(vm, dest, vlan_if, session):
"""
Test ping between vlans, from guest to host/guest.
:params vm: VM object
:params dest: Dest ip to ping.
:params vlan_if: Vlan interface.
:params session: VM session.
"""
error_context.context(
"Test ping from '%s' to '%s' on guest '%s'" % (vlan_if, dest, vm.name)
)
status, output = utils_test.ping(
dest=dest, count=10, interface=vlan_if, session=session, timeout=30
)
if status:
raise NetPingError(vlan_if, dest, output)

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)

Comment on lines +127 to +143
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fragile closure over session and unintended NetworkManager restart.

  • netperf_vlan() uses an implicit outer session variable; 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), but netperf_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

vms = []
sessions = []
ifname = []
vm_ip = []
vm_vlan_ip = []
vm_vlan_if = []
sub_type = params["sub_type"]
host_br = params.get("netdst", "switch")
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(",")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


br_backend = utils_net.find_bridge_manager(host_br)
if not isinstance(br_backend, utils_net.Bridge):
test.cancel("Host does not use Linux Bridge")

linux_modules.load_module("8021q")

host_vlan_if = "%s.%s" % (host_br, host_vlan_id)
if host_vlan_if not in utils_net.get_net_if():
host_vlan_if = add_vlan(interface=host_br, v_id=host_vlan_id)
if host_vlan_if in utils_net.get_net_if():
set_ip_vlan(vlan_if=host_vlan_if, vlan_ip=host_vlan_ip)
rm_host_vlan_cmd = params["rm_host_vlan_cmd"] % host_vlan_if
funcatexit.register(env, params["type"], _system, rm_host_vlan_cmd)
else:
test.cancel("Fail to set up vlan over bridge interface in host!")

vms.append(env.get_vm(params["main_vm"]))
vms.append(env.get_vm("vm2"))
for vm_ in vms:
vm_.verify_alive()

for vm_index, vm in enumerate(vms):
error_context.context("Prepare test env on %s" % vm.name)
session = vm.wait_for_serial_login()
if not session:
err_msg = "Could not log into guest %s" % vm.name
test.error(err_msg)

interface = utils_net.get_linux_ifname(session, vm.get_mac_address())

error_context.context("Load 8021q module in guest %s" % vm.name, test.log.info)
session.cmd_output_safe("modprobe 8021q")

error_context.context(
"Setup vlan environment in guest %s" % vm.name, test.log.info
)
inter_ip = "%s.%s.%d" % (subnet, host_vlan_id, vm_index + 1)
set_ip_vlan(interface, inter_ip, session=session)
set_arp_ignore(session)
params["vlan_nic"] = "%s.%s" % (interface, host_vlan_id)
error_context.context(
"Test ping from guest '%s' to host with "
"interface '%s'" % (vm.name, interface),
test.log.info,
)
try:
ping_vlan(vm, dest=host_vlan_ip, vlan_if=interface, session=session)
except NetPingError:
test.log.info(
"Guest ping fail to host as expected with interface '%s'", interface
)
else:
test.fail("Guest ping to host should fail with interface '%s'" % interface)
ifname.append(interface)
vm_ip.append(inter_ip)
sessions.append(session)

# Ping succeed between guests
error_context.context(
"Test ping between guests with interface %s" % ifname[0], test.log.info
)
ping_vlan(vms[0], dest=vm_ip[1], vlan_if=ifname[0], session=sessions[0])

# set vlan tag for guest
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).

vlan_if = add_vlan(
interface=ifname[vm_index], v_id=host_vlan_id, session=session
)
vm_vlan_if.append(vlan_if)
set_mac_vlan(vlan_if, mac_str[vm_index], session=session)
vlan_ip = "%s.%s.%d" % (subnet, host_vlan_id, vm_index + 11)
set_ip_vlan(vlan_if, vlan_ip, session=session)
vm_vlan_ip.append(vlan_ip)

error_context.context(
"Test ping from interface '%s' on guest "
"'%s' to host." % (vm_vlan_if[vm_index], vm.name),
test.log.info,
)
utils_net.restart_guest_network(session)
ping_vlan(vm, dest=host_vlan_ip, vlan_if=vm_vlan_if[vm_index], session=session)
netperf_vlan(client=vm.name, server="localhost")

error_context.context(
"Test ping and netperf between guests with "
"interface '%s'" % vm_vlan_if[vm_index],
test.log.info,
)
ping_vlan(vms[0], dest=vm_vlan_ip[1], vlan_if=vm_vlan_if[0], session=sessions[0])
netperf_vlan(client=params["main_vm"], server="vm2")

exithandlers = "exithandlers__%s" % sub_type
sub_exit_timeout = int(params.get("sub_exit_timeout", 10))
start_time = time.time()
end_time = start_time + float(sub_exit_timeout)

while time.time() < end_time:
test.log.debug(
"%s (%f secs)", sub_type + " is running", (time.time() - start_time)
)
if env.data.get(exithandlers):
break
time.sleep(1)

for sess in sessions:
if sess:
sess.close()