Add case to test 802.1q vlan in guest #6551
Conversation
|
Hi @nanli1 Could you please help review this patch? Thanks in advance. |
| for vm_ in vms: | ||
| vm_.verify_alive() | ||
|
|
||
| for vm_index, vm in enumerate(vms): | ||
| if params["os_type"] == "windows": | ||
| session = vm.wait_for_login(timeout=login_timeout) | ||
| session = utils_test.qemu.windrv_check_running_verifier( | ||
| session, vm, test, driver_verifier | ||
| ) |
There was a problem hiding this comment.
For guest version loop(rhel9.7 rhel10.1 windows_xxx), Ci is not generating vm automatically for ourt guest version request, So we need to install vm by our self. such as #6531 usage , if you have more thought, we can discuss offline
There was a problem hiding this comment.
This is a point that confuses me. Does each case require a separate virtual machine to be prepared?
Hi @nanli1 If you read the code carefully, you will find that each key step is explained, but it is not a direct way like test.log. |
Could you please provide more details about this comment? |
yanglei-rh
force-pushed
the
LIBVIRTAT-22004
branch
from
14e2f59 to
f5846b2
Compare
WalkthroughAdds a new VLAN test configuration and a Python test module that implement 802.1Q VLAN validation across two VMs, supporting Linux and Windows guests, multiple VLANs, connectivity and data-transfer checks, and cleanup/error handling driven by many configurable parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Script
participant G1 as VM1
participant G2 as VM2
rect rgb(220,235,255)
Note over Runner,G2: Setup & login
Runner->>G1: Start VM & login
Runner->>G2: Start VM & login
end
rect rgb(220,255,230)
Note over Runner,G2: VLAN configuration (Linux/Windows)
Runner->>G1: Load 802.1Q / run Windows VLAN cmd
Runner->>G2: Load 802.1Q / run Windows VLAN cmd
Runner->>G1: Create VLAN iface(s) & set IP
Runner->>G2: Create VLAN iface(s) & set IP
end
rect rgb(255,245,200)
Note over Runner,G2: Connectivity & transfer
Runner->>G1: Ping G2 on VLAN(s)
Runner->>G2: Respond to ping
Runner->>G1: Start nc listener / send file
Runner->>G2: Send / receive file
Runner->>Runner: Verify MD5 checksum
end
rect rgb(255,220,220)
Note over Runner,G2: Cleanup
Runner->>G1: Kill nc, remove VLAN ifaces
Runner->>G2: Kill nc, remove VLAN ifaces
Runner->>G1: Close session
Runner->>G2: Close session
end
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 (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: 4
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/vlan.py (2)
240-243: Replace fixed sleep with a wait loop.Hard sleeps are flaky. Use
utils_misc.wait_forto poll until NIC is enabled.+from virttest import utils_misc @@ - time.sleep(10) - nicid = utils_net.get_windows_nic_attribute( - session=session, key="netenabled", value=True, target="netconnectionID" - ) + def _nic_up(): + return utils_net.get_windows_nic_attribute( + session=session, key="netenabled", value=True, target="netconnectionID" + ) + nicid = utils_misc.wait_for(_nic_up, timeout=90, first=1, step=3) + if not nicid: + test.error("NIC did not become enabled after VLAN change")This also addresses the reviewer’s previous concern about hard-coded timeouts.
254-285: Add TEST_SETUP/STEP/TEARDOWN breadcrumbs.Your
error_contextusage is good. If you want the exact breadcrumbs the reviewer suggested, add succincttest.log.info("TEST_SETUP|TEST_STEP|TEST_TEARDOWN: ...")around major phases (login, module load, VLAN plumb, tests, cleanup). I can draft the placements if you want.
🧹 Nitpick comments (6)
libvirt/tests/cfg/virtual_network/qemu/vlan.cfg (1)
12-13: Add nc timeouts to avoid indefinite hangs.To prevent a blocked transfer from stalling the test, add
-w(timeout) to both commands:- listen_cmd = "nc -l %s > %s" - send_cmd = "nc %s %s < %s" + listen_cmd = "nc -w 30 -l %s > %s" + send_cmd = "nc -w 30 %s %s < %s"Adjust
30via params if needed.libvirt/tests/src/virtual_network/qemu/vlan.py (5)
288-303: Simplify Windows ping validation; rely on status.
TTL=parsing can be locale/implementation dependent. Preferstatus == 0and optionally check loss ratio.- loss = utils_test.get_loss_ratio(output) - if not loss and ("TTL=" in output): - pass - # window get loss=0 when ping fail sometimes, need further check - else: - test.fail( - "Guests ping test hit unexpected loss, error info: %s" % output - ) + loss = utils_test.get_loss_ratio(output) + if status == 0 and (loss == 0): + test.log.info("Windows ping succeeded: %s -> %s", vm.name, vm_ip[(vm_index + 1) % 2]) + else: + test.fail("Windows ping failed or unexpected loss: %s" % output)
254-266: Optional: log VM XML before guest-side configuration.To address the reviewer’s request, dump the domain XML once per VM for traceability.
session = vm.wait_for_login(timeout=login_timeout) @@ test.log.info("Logged in %s successful", vm.name) + try: + if hasattr(vm, "get_xml"): + test.log.debug("Guest XML before test (%s):\n%s", vm.name, vm.get_xml()) + except Exception as e: + test.log.debug("Failed to dump XML for %s: %s", vm.name, e)
189-197: Ensure flood-ping session closes on exceptions.Wrap with try/finally to avoid session leaks if ping raises.
- session_flood = vms[src].wait_for_login(timeout=60) - utils_test.ping( - vlan_ip[dst], - flood=True, - interface=ifname[src], - session=session_flood, - timeout=10, - ) - session_flood.close() + session_flood = vms[src].wait_for_login(timeout=60) + try: + utils_test.ping( + vlan_ip[dst], + flood=True, + interface=ifname[src], + session=session_flood, + timeout=10, + ) + finally: + session_flood.close()
314-317: Nit: unused loop variablevm(B007).Rename to
_to satisfy linters and clarify intent.Example:
- for vm_index, vm in enumerate(vms): + for vm_index, _ in enumerate(vms):Repeat similarly in the teardown loop.
Also applies to: 347-354
337-341: Netcat server/client robustness.Consider adding
-q 1(BSD netcat) or-N(GNU ncat) where available to ensure EOF closes the connection promptly; keep as paramized strings in cfg to remain portable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/vlan.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/vlan.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/vlan.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.14.0)
libvirt/tests/src/virtual_network/qemu/vlan.py
288-288: Loop control variable vm not used within loop body
(B007)
314-314: Loop control variable vm not used within loop body
(B007)
347-347: Loop control variable vm not used within loop body
(B007)
🔇 Additional comments (1)
libvirt/tests/cfg/virtual_network/qemu/vlan.cfg (1)
33-36: Scope variants explicitly.If Windows is not yet covered end-to-end for scalability, constrain
vlan_scalability_testto Linux (you already haveonly Linuxhere) and ensure@vlan_connective_testinherits the Windows branch intent. Consider adding a briefcase_requirementsnote for future Windows coverage.
| msg_pattern = '8021q: Invalid VLAN id' | ||
| RHEL.7, ALT.7, RHEL.6: | ||
| msg_pattern = 'RTNETLINK answers: Numerical result out of range' | ||
| Windows: |
There was a problem hiding this comment.
Negative-test message pattern is brittle across distros/kernels.
ip link add typically returns a RTNETLINK error; the kernel log line '8021q: Invalid VLAN id' may not appear in stderr. Recommend matching RTNETLINK text (covers RHEL 8/9) or, better, let the test assert on non-zero exit only.
Option A (regex broaden):
- msg_pattern = '8021q: Invalid VLAN id'
+ # ip(8) stderr varies; match common RTNETLINK failures
+ msg_pattern = 'RTNETLINK answers: (Invalid argument|Numerical result out of range)'You can keep the RHEL.6/7 override or remove it, since the broadened default already covers those.
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/vlan.cfg around lines 18-21, the
current negative-test message pattern '8021q: Invalid VLAN id' is brittle across
distros/kernels; change the test to either (A) broaden the msg_pattern to match
the more common RTNETLINK output (e.g. a regex matching 'RTNETLINK answers:.*out
of range' which covers RHEL6/7/8/9) and remove the special-case overrides if
redundant, or (B) make the assertion check only for a non-zero exit status from
`ip link add` instead of relying on stderr/kernel log text; implement the chosen
approach so the test is robust across kernels.
| def set_ip_vlan(session, v_id, vlan_ip, iface="eth0"): | ||
| """ | ||
| Assigns an IP address to a VLAN interface. | ||
|
|
||
| :param session: The guest session to execute commands on. | ||
| :param v_id: The VLAN ID of the target interface. | ||
| :param vlan_ip: The IP address to assign. | ||
| :param iface: The parent network interface (e.g., "eth0"). | ||
| """ | ||
| iface = "%s.%s" % (iface, v_id) | ||
| txt = "Assign IP '%s' to vlan interface '%s'" % (vlan_ip, iface) | ||
| error_context.context(txt, test.log.info) | ||
| session.cmd("ifconfig %s %s" % (iface, vlan_ip)) | ||
|
|
There was a problem hiding this comment.
Avoid ifconfig dependency; use iproute2 and bring link up.
ifconfig (net-tools) isn’t present by default on RHEL 8/9+. Use ip addr and ip link instead; also set prefixlen and bring the VLAN up.
- def set_ip_vlan(session, v_id, vlan_ip, iface="eth0"):
+ def set_ip_vlan(session, v_id, vlan_ip, iface="eth0", prefixlen="24"):
@@
- iface = "%s.%s" % (iface, v_id)
- txt = "Assign IP '%s' to vlan interface '%s'" % (vlan_ip, iface)
+ iface = "%s.%s" % (iface, v_id)
+ txt = "Assign IP '%s/%s' to vlan interface '%s'" % (vlan_ip, prefixlen, iface)
error_context.context(txt, test.log.info)
- session.cmd("ifconfig %s %s" % (iface, vlan_ip))
+ session.cmd("ip addr flush dev %s || true" % iface)
+ session.cmd("ip addr add %s/%s dev %s" % (vlan_ip, prefixlen, iface))
+ session.cmd("ip link set dev %s up" % iface)This removes a common failure point on modern guests.
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/vlan.py around lines 63 to 76, replace
the ifconfig call with iproute2 usage: ensure iface remains parent.v_id, then
use "ip addr add <vlan_ip_with_prefix> dev <iface>" (if vlan_ip has no prefixlen
append a sensible default like /24) and then "ip link set dev <iface> up" to
bring the VLAN interface up; run both commands via session.cmd and log errors as
before.
| check_cmd = "netstat -nultp |awk '(NR>2){print $4}' | awk -F':' '{print $NF}'" | ||
| output = sessions[dst].cmd_output(check_cmd).strip() | ||
| if not output: | ||
| return 1025 | ||
| used_ports = {int(p) for p in output.splitlines() if p.isdigit()} | ||
| for port in range(1025, 65536): | ||
| if port not in used_ports: | ||
| return port | ||
| test.error("No free TCP/UDP ports found in guest.") |
There was a problem hiding this comment.
Replace netstat dependency with ss (fallback to netstat).
netstat may be missing; prefer ss and fall back when unavailable.
- check_cmd = "netstat -nultp |awk '(NR>2){print $4}' | awk -F':' '{print $NF}'"
+ check_cmd = ("if command -v ss >/dev/null 2>&1; then ss -H -tanlp; "
+ "else netstat -nultp; fi | awk '{print $4}' | awk -F\":\" '{print $NF}'")Optionally, add a small wait/retry if the chosen port races.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/vlan.py around lines 126 to 134, the
current logic uses netstat which may be unavailable; update the function to
prefer ss and fall back to netstat: run an ss command to list listening TCP/UDP
ports, parse the output to extract port numbers into used_ports (handle both
IPv4/IPv6 formats), if ss is not present run the existing netstat pipeline;
preserve the search from 1025..65535 returning the first free port and, to
reduce race conditions, add a small retry loop (e.g., sleep briefly and re-check
up to a few times) before raising the test.error about no free ports.
| error_context.base_context("Vlan negative test") | ||
| error_context.context("Create vlan with ID %s in guest" % bound, test.log.info) | ||
| try: | ||
| add_vlan(test, sessions[0], bound, ifname[0], cmd_type) | ||
| test.fail("Maximal ID allow to vlan is %s" % maximal) | ||
| except aexpect.ShellCmdError as detail: | ||
| pattern = params["msg_pattern"] | ||
| if not re.search(pattern, detail.output, re.M | re.I): | ||
| raise | ||
| finally: |
There was a problem hiding this comment.
Negative test should assert on non-zero exit, not fragile stderr text.
Kernel/ip(8) messages vary; asserting the presence of a specific substring can spuriously fail. Treat any ShellCmdError here as success.
- except aexpect.ShellCmdError as detail:
- pattern = params["msg_pattern"]
- if not re.search(pattern, detail.output, re.M | re.I):
- raise
+ except aexpect.ShellCmdError as detail:
+ test.log.debug("Expected failure creating VLAN %s: %s", bound, detail.output)If you still want text matching, keep it best-effort and log a warning instead of raising.
📝 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.
| error_context.base_context("Vlan negative test") | |
| error_context.context("Create vlan with ID %s in guest" % bound, test.log.info) | |
| try: | |
| add_vlan(test, sessions[0], bound, ifname[0], cmd_type) | |
| test.fail("Maximal ID allow to vlan is %s" % maximal) | |
| except aexpect.ShellCmdError as detail: | |
| pattern = params["msg_pattern"] | |
| if not re.search(pattern, detail.output, re.M | re.I): | |
| raise | |
| finally: | |
| error_context.base_context("Vlan negative test") | |
| error_context.context("Create vlan with ID %s in guest" % bound, test.log.info) | |
| try: | |
| add_vlan(test, sessions[0], bound, ifname[0], cmd_type) | |
| test.fail("Maximal ID allow to vlan is %s" % maximal) | |
| except aexpect.ShellCmdError as detail: | |
| test.log.debug("Expected failure creating VLAN %s: %s", bound, detail.output) | |
| finally: |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/vlan.py around lines 372 to 381, the
negative test currently asserts on specific stderr text from a ShellCmdError
which is fragile; change the except block to treat any aexpect.ShellCmdError as
a successful negative result (i.e., swallow the exception or return/continue)
rather than inspecting detail.output, and remove the re.search raise path; if
you still want best-effort text matching, replace the raise with a warning log
(test.log.warning) when the pattern is not found but do not fail the test.
|
Hi @nanli1 Please help review this patch again. I already add doc string for each function to match tp-libvirt requirements. I need to highlight that there are some changes about python file, because of all the code logic are directly from tp-qemu in this time, so from my perspective we can trust it. This can help us speed up automated, review, and merge. Thanks a lot. |
1. VIRT-95961 [virtual network][virtual-nic-device] Test 802.1q vlan in guest 2. All the function doc string from Gemini CLI Signed-off-by: Lei Yang <[email protected]>
yanglei-rh
force-pushed
the
LIBVIRTAT-22004
branch
from
f5846b2 to
d192be5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/vlan.py (2)
289-308: Consider using underscore for unused loop variable.The loop variable
vmis not used in the body. For clarity, consider using_:- for vm_index, vm in enumerate(vms): + for vm_index, _ in enumerate(vms):This minor cleanup makes the intent clearer.
315-315: Consider using underscore for unused loop variables.Similar to line 289, the
vmvariable is unused at lines 315 and 350:- for vm_index, vm in enumerate(vms): + for vm_index, _ in enumerate(vms):Also applies to: 350-350
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/vlan.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/vlan.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/vlan.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.14.0)
libvirt/tests/src/virtual_network/qemu/vlan.py
289-289: Loop control variable vm not used within loop body
(B007)
315-315: Loop control variable vm not used within loop body
(B007)
350-350: Loop control variable vm not used within loop body
(B007)
🔇 Additional comments (14)
libvirt/tests/cfg/virtual_network/qemu/vlan.cfg (2)
1-17: LGTM: Test configuration is well-structured.The parameter definitions are clear and appropriate for the VLAN test scenarios. The defaults (cmd_type = ip, vlan_num = 5, file_size = 10) are reasonable.
21-36: LGTM: OS-specific configuration is appropriate.The Windows-specific adjustments (extended
login_timeout, PowerShell-based VLAN setup, driver verifier) properly account for Windows guest differences. The variant structure correctly scopesvlan_scalability_testto Linux only.libvirt/tests/src/virtual_network/qemu/vlan.py (12)
1-34: LGTM: Module documentation is comprehensive.The docstring clearly outlines the test flow for both Linux and Windows guests, covering setup, VLAN creation, validation, and cleanup steps.
36-59: LGTM: VLAN creation function is well-implemented.The function properly supports both
ipandvconfigcommands with clear error handling and logging viaerror_context.
75-89: LGTM: ARP ignore configuration is correct and well-documented.The docstring clearly explains why
arp_ignoreis necessary for multi-VLAN scenarios, and the implementation correctly configures it system-wide.
91-114: LGTM: VLAN removal function mirrors add_vlan appropriately.The symmetric implementation and status return allow callers to detect removal failures.
134-169: LGTM: File transfer validation is thorough.The MD5 checksum verification ensures data integrity across VLANs, and the cleanup (killing nc, removing receive file) is handled properly even on failure.
171-195: LGTM: Flood ping implementation is clean.The docstring helpfully explains why a dedicated session is used, and the session management (create, use, close) is appropriate.
197-221: LGTM: Test initialization is clean and comprehensive.The parameter loading covers all necessary configuration, and calling
verify_alive()on each VM before testing is good practice.
224-235: LGTM: Windows guest handling is appropriate.The Windows-specific path properly handles driver verification, PowerShell-based VLAN configuration, and network restart. The NIC attribute lookups correctly obtain the connection ID and link-local IP.
Also applies to: 237-251
253-286: LGTM: Linux guest setup flow is well-structured.The setup properly sequences firewall disable, 8021q module load, VLAN creation, IP assignment, and ARP configuration with appropriate error context logging.
311-344: LGTM: VLAN connectivity validation logic is sound.The use of XOR at line 328
(vlan == vlan2) ^ (status == 0)elegantly validates that pings succeed within the same VLAN and fail across different VLANs. The flood ping and netcat transfer provide comprehensive validation.
358-376: LGTM: VLAN scalability test with proper cleanup.The maximal test correctly attempts to create VLANs up to 4094 and ensures all created VLANs are removed in the finally block, even if the test fails partway through.
388-394: LGTM: Session cleanup is properly handled.The finally block ensures all sessions are closed, with appropriate None checks to prevent errors.
2 participants
ID: LIBVIRTAT-22004
Signed-off-by: Lei Yang [email protected]
Summary by CodeRabbit