Add case to support jumbo frame test #6658
Conversation
WalkthroughAdds a jumbo-frame test for libvirt virtual networks: a new config Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yanglei-rh
force-pushed
the
LIBVIRTAT-22154
branch
from
bec809d to
3c8fede
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 (4)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg (1)
1-13: Confirm Windows MTU registry command is defined somewhere in the config stackThis cfg provides
mtu = 9000andmtu_key = MTUforvirtio_neton Windows, which matches howjumbo.pyconsumesmtu/mtu_key, but the test code also requires areg_mtu_cmdparameter for Windows (reg_set_mtu_pattern = params.get("reg_mtu_cmd")and thenreg_set_mtu_pattern % ...). Please confirm thatreg_mtu_cmdis defined in a shared/base cfg or add it here to avoid runtimeTypeErrorwhen running the Windows variant.libvirt/tests/src/virtual_network/qemu/jumbo.py (3)
160-183: Avoid callingrandom.randrangein default argument and fix loss‑ratio message
size_increase_pinguses a default argument computed viarandom.randrange:def size_increase_ping(step=random.randrange(90, 110)):Default arguments are evaluated at import time, not at call time. This has two downsides:
- The step is effectively fixed for the lifetime of the module, which is surprising.
- It triggers the B008 warning (and static analysis here).
Refactor to compute
stepinside the function:-def size_increase_ping(step=random.randrange(90, 110)): +def size_increase_ping(step=None): test.log.info("Size increase ping") + if step is None: + step = random.randrange(90, 110)Additionally, the failure message hard‑codes
50%even thoughfail_ratiois read from params:fail_ratio = int(params.get("fail_ratio", 50)) if utils_test.get_loss_ratio(output) > fail_ratio: test.fail("Ping loss ratio is greater than 50% for size %s" % size)Consider:
- fail_ratio = int(params.get("fail_ratio", 50)) - if utils_test.get_loss_ratio(output) > fail_ratio: - test.fail( - "Ping loss ratio is greater than 50% for size %s" % size - ) + fail_ratio = int(params.get("fail_ratio", 50)) + loss = utils_test.get_loss_ratio(output) + if loss > fail_ratio: + test.fail( + "Ping loss ratio %s is greater than %s%% for size %s" + % (loss, fail_ratio, size) + )This keeps behavior predictable and messages accurate.
90-123: Check Windows‑specific parameters and closesession_serialin cleanupWithin the Windows branch you rely on:
reg_set_mtu_pattern = params.get("reg_mtu_cmd") mtu_key_word = params.get("mtu_key", "MTU") reg_set_mtu = reg_set_mtu_pattern % (int(index), mtu_key_word, int(mtu)) guest_mtu_cmd = "%s " % reg_set_mtu ... utils_net.restart_windows_guest_network(session_serial, connection_id, mode=mode)Two minor points:
If
reg_mtu_cmdis missing from the combined cfg,reg_set_mtu_patternwill beNoneand this will raise aTypeError. Align this with the cfg (either ensure it’s always defined or guard with an explicit error).
session_serialis opened but never closed in thefinallyblock; onlysessionis closed. To avoid leaking long‑lived serial connections during large test runs, consider also closingsession_serialwhen present.Example:
- finally: - # Environment clean - if session: - session.close() + finally: + # Environment clean + if session: + session.close() + if 'session_serial' in locals() and session_serial: + session_serial.close()
36-44: Shell commands viashell=True– low risk here but worth constraining inputsSeveral
process.run/process.getoutput/process.systemcalls useshell=Truewith interpolated values (e.g.netdst,iface,ifname,guest_ip,iface_mac). In this test harness those inputs ultimately come from test config and libvirt/OS introspection, so the risk is low, but static analysis correctly flags them.Where convenient, consider:
- Switching to argument lists (
shell=False) for simple commands likeip link,arp,grep,ovs-vsctl.- Or, at minimum, ensuring all interpolated values are validated (interface names/IPs match expected patterns), especially if any part can be influenced externally.
This will make the test more robust and quieter for security linters.
Also applies to: 78-89, 118-123, 187-190, 209-210, 215-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/jumbo.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/jumbo.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/jumbo.py
43-43: Function call with shell=True parameter identified, security issue
(S604)
88-88: Function call with shell=True parameter identified, security issue
(S604)
118-118: Function call with shell=True parameter identified, security issue
(S604)
122-122: Function call with shell=True parameter identified, security issue
(S604)
160-160: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
160-160: Do not perform function call random.randrange in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
187-187: Function call with shell=True parameter identified, security issue
(S604)
209-209: Function call with shell=True parameter identified, security issue
(S604)
210-210: Function call with shell=True parameter identified, security issue
(S604)
215-215: Function call with shell=True parameter identified, security issue
(S604)
|
Tested pass from my side. |
yanglei-rh
force-pushed
the
LIBVIRTAT-22154
branch
from
3c8fede to
3a59ffd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/jumbo.py (1)
57-60: Capture and restore original MTU values instead of hard-coding 1500The test hard-codes
mtu_default = 1500(line 58) and restores all bridge NICs to this value during cleanup (lines 219-221). On systems where interfaces are intentionally configured with non-1500 MTUs, this corrupts the host configuration after the test completes.Store each interface's original MTU before modification (lines 86-88):
+ orig_mtu = {} error_context.context("Change all Bridge NICs MTU to %s" % mtu, test.log.info) for iface in target_ifaces: + # Capture original MTU before changing + orig_mtu[iface] = int(process.getoutput(f"cat /sys/class/net/{iface}/mtu", shell=True)) process.run(host_mtu_cmd % (iface, mtu), shell=True)Then restore using captured values in cleanup (lines 219-221):
- test.log.info("Change back Bridge NICs MTU to %s", mtu_default) + test.log.info("Restoring Bridge NIC MTUs to original values") for iface in target_ifaces: - process.run(host_mtu_cmd % (iface, mtu_default), shell=True) + original = orig_mtu.get(iface, mtu_default) + process.run(host_mtu_cmd % (iface, original), shell=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/jumbo.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/jumbo.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/jumbo.py
43-43: Function call with shell=True parameter identified, security issue
(S604)
88-88: Function call with shell=True parameter identified, security issue
(S604)
118-118: Function call with shell=True parameter identified, security issue
(S604)
122-122: Function call with shell=True parameter identified, security issue
(S604)
163-163: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
191-191: Function call with shell=True parameter identified, security issue
(S604)
215-215: Function call with shell=True parameter identified, security issue
(S604)
216-216: Function call with shell=True parameter identified, security issue
(S604)
221-221: Function call with shell=True parameter identified, security issue
(S604)
⏰ 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.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (6)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg (1)
1-13: LGTM! Configuration structure is sound.The jumbo frame test configuration is well-structured with appropriate parameters:
- MTU 9000 is standard for jumbo frames
- Device-specific variants correctly restrict e1000e to Linux and provide Windows-specific MTU registry key for virtio_net
- VM lifecycle parameters align with the test implementation
libvirt/tests/src/virtual_network/qemu/jumbo.py (5)
1-34: LGTM! Well-structured test entry point.Imports are appropriate, the
@error_context.context_awaredecorator enables helpful test logging, and the docstring clearly outlines the 10-step jumbo frame test sequence.
36-43: Helper function is straightforward and appropriate.The
get_ovs_portsfunction correctly retrieves OVS bridge interfaces. The use ofshell=Trueis acceptable in test code where command inputs are controlled.
120-186: Test helpers implement comprehensive MTU validation.The ARP entry addition and test helper functions provide thorough jumbo frame verification:
is_mtu_okfor readiness checkverify_mtufor path MTU validation with packet loss detectionflood_pingfor stress testinglarge_frame_pingfor sustained large-frame transmissionsize_increase_pingwith progressive sizing and retry logicThe use of
random.randrangeat line 163 is appropriate for varying test step sizes (not cryptographic use).
187-206: LGTM! Test execution flow is well-structured.The test properly waits for MTU readiness (with helpful debug output on timeout), executes comprehensive functional validation, and concludes with stress testing. The sequence matches the documented test steps.
208-218: Cleanup logic is sound.The
finallyblock ensures proper cleanup:
- Sessions are closed with defensive checks
- Temporary ARP entry is correctly removed (using proper integer comparison at line 215)
- Helpful logging confirms ARP cleanup success
(Note: MTU restoration issue at lines 219-221 is addressed in a separate comment.)
Automate VIRT-95907 Jumbo Frame Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22154
branch
from
3a59ffd to
f0c45e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/jumbo.py (1)
69-69: Consider conditional serial session creation for Windows only.The serial session is created for all OS types at line 69, but it's only used for Windows (lines 113-115 for network restart). For Linux guests, this creates an unused resource.
Consider moving serial session creation into the Windows-specific block:
session = vm.wait_for_login(timeout=timeout) -session_serial = vm.wait_for_serial_login(timeout=timeout) iface_mac = vm.get_mac_address(0) ... else: + session_serial = vm.wait_for_serial_login(timeout=timeout) connection_id = utils_net.get_windows_nic_attribute(Adjust the cleanup check at line 212 accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/jumbo.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/jumbo.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/jumbo.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/jumbo.py
43-43: Function call with shell=True parameter identified, security issue
(S604)
88-88: Function call with shell=True parameter identified, security issue
(S604)
118-118: Function call with shell=True parameter identified, security issue
(S604)
122-122: Function call with shell=True parameter identified, security issue
(S604)
163-163: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
191-191: Function call with shell=True parameter identified, security issue
(S604)
215-215: Function call with shell=True parameter identified, security issue
(S604)
216-216: Function call with shell=True parameter identified, security issue
(S604)
221-221: Function call with shell=True parameter identified, security issue
(S604)
⏰ 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.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
🔇 Additional comments (5)
libvirt/tests/cfg/virtual_network/qemu/jumbo.cfg (1)
1-13: LGTM! Configuration is well-structured.The jumbo frame test configuration is appropriate:
- Root privileges and VM lifecycle settings are correctly specified
- MTU 9000 is the standard jumbo frame size
- Device restrictions (e1000e for Linux only) align with driver capabilities
- Windows MTU configuration via registry key is properly defined
libvirt/tests/src/virtual_network/qemu/jumbo.py (4)
1-34: LGTM! Well-documented test function.The imports are appropriate and the docstring clearly outlines the 10-step test flow for RX jumbo frame validation.
36-43: LGTM! Helper function is clear and correct.The OVS port enumeration logic is straightforward. The
shell=Trueparameter is acceptable in the libvirt test framework context.
117-118: LGTM! Typo fixed.The log message now correctly reads "Changing the MTU of host tap" (previous review noted the typo "Chaning").
214-217: LGTM! ARP cleanup logic fixed.The integer comparison
== 0at line 215 is now correct (previous review identified this was comparing to string"0"). The ARP entry cleanup will now work properly.
|
Hi @qiankehan @nanli1 Please help review this patch which about test jumbo frame inside guest, thanks a lot. It's important to note that this differs from directly adding jumbo frames in the current XML; the two are designed for different customer needs. |
2 participants
Automate VIRT-95907 Jumbo Frame
ID: LIBVIRTAT-22154
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit