Migrate pktgen perf tests from tp-qemu to tp-libvirt #6781
Conversation
WalkthroughAdds a new libvirt test configuration file pktgen_perf_test.cfg with test parameters and variants (including a vhost_vdpa variant). Updates pktgen_burst_mode_test to perform pre-start vhost-vdpa interface setup when vdpa is used (stop VM, adjust XML, attach vhost-vdpa, adjust MAC/nettype), ensure VM start timing, and use an active session for dmesg verification for vdpa. Enhances pktgen utilities to support configurable scripts and params, remote client execution via virttest.remote, extended run_test to accept session_serial and pkt category, and adjust packet counter reads and runner selection for tx/rx and remote cases. 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🤖 Fix all issues with AI agents
In `@libvirt/tests/cfg/virtual_network/qemu/pktgen_perf_test.cfg`:
- Line 44: The config parameter name client_pktgen_iface does not match the key
read in pktgen_utils.py (pktgen_rx_iface), causing the iface to be None; fix by
making the names consistent: either rename the config entry to pktgen_rx_iface
or change the lookup in pktgen_utils.py (where pktgen_rx_iface is fetched around
line 66) to read client_pktgen_iface (and update any other references to
pktgen_rx_iface in that module to use the chosen name) so the RX test receives
the correct interface value.
In `@libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py`:
- Line 76: Replace the unsafe use of eval when parsing vm_attrs: instead of
vm_attrs = eval(params.get("vm_attrs", "{}")), import the ast module and call
ast.literal_eval on the params.get(...) value to safely parse the dictionary
literal; update the code path around vm_attrs and ensure any default "{}" is
passed through ast.literal_eval as needed.
In `@provider/virtual_network/pktgen_utils.py`:
- Around line 43-46: Add null/validation checks before using vm and
session_serial in pktgen_utils.py: ensure vm is not None and has get_mac_address
(used to set guest_mac) and ensure session_serial is not None before calling
utils_net.get_linux_ifname (used to set guest_eth); if either is missing, raise
a clear ValueError or return early with a descriptive error message. Update the
block that currently calls vm.get_mac_address(0) and
utils_net.get_linux_ifname(session_serial, guest_mac) to guard those calls and
include the function/variable names (get_mac_address, get_linux_ifname,
guest_mac, guest_eth) in the error messages so the failure point is obvious.
- Around line 67-69: The remote SSH session created via remote.remote_login is
assigned only as a command method to self.runner, causing the remote_session
object to be dropped and never closed; modify the PktgenConfig instantiation to
save the session to an instance attribute (e.g., self.remote_session =
remote_session) instead of only storing its cmd, and add a cleanup method on
PktgenConfig (e.g., def cleanup(self): if getattr(self, "remote_session", None):
self.remote_session.close()) and ensure callers invoke cleanup when tests finish
to avoid leaking SSH connections.
- Around line 119-153: The run_test function uses session_serial unconditionally
(calls session_serial.cmd(packets)) while it defaults to None; update run_test
to handle a missing session_serial: either make session_serial a required
argument in the function signature or add guards where
session_serial.cmd(packets) is called—use runner(packets) when session_serial is
None (or raise a clear ValueError at the start if you prefer requiring it).
Specifically, modify the calls that reference session_serial.cmd (the initial
packet_b read, the packet_a reads in the try and both except blocks) to check if
session_serial is not None before calling session_serial.cmd, and fall back to
runner(packets) or raise a descriptive error; keep the existing kill logic that
checks session_serial and runner equality intact.
🧹 Nitpick comments (2)
provider/virtual_network/pktgen_utils.py (2)
29-30: Unusedscriptparameter.The
scriptparameter is added but never used withinconfigure_pktgen. Either remove it or document its intended future use.Suggested fix
If not needed, remove the parameter:
def configure_pktgen( self, pkt_cate, vm=None, session_serial=None, - script=None, params=None ):And update the call site at line 246:
- pkt_cate, vm, session_serial, script=script, params=params + pkt_cate, vm, session_serial, params=params
227-228: Same null-safety concern asconfigure_pktgen.
vmandsession_serialdefault toNone, butguest_macandguest_ethcomputation will fail if they are. Consider either removing the defaults or adding validation, consistent with any fix applied toconfigure_pktgen.
| # shell_port_client = 22 | ||
| # shell_client_client = ssh | ||
| # shell_prompt_client = [$#%] | ||
| # client_pktgen_iface = ethX # external host iface used by pktgen |
There was a problem hiding this comment.
Parameter name mismatch with pktgen_utils.py.
The config defines client_pktgen_iface but pktgen_utils.py line 66 retrieves it as pktgen_rx_iface. This mismatch will cause the interface to be None when running rx tests with a client.
Suggested fix
Either rename here to match the code:
- # client_pktgen_iface = ethX # external host iface used by pktgen
+ # pktgen_rx_iface = ethX # external host iface used by pktgenOr update pktgen_utils.py line 66:
- self.interface = params.get("pktgen_rx_iface")
+ self.interface = params.get("client_pktgen_iface")📝 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.
| # client_pktgen_iface = ethX # external host iface used by pktgen | |
| # pktgen_rx_iface = ethX # external host iface used by pktgen |
🤖 Prompt for AI Agents
In `@libvirt/tests/cfg/virtual_network/qemu/pktgen_perf_test.cfg` at line 44, The
config parameter name client_pktgen_iface does not match the key read in
pktgen_utils.py (pktgen_rx_iface), causing the iface to be None; fix by making
the names consistent: either rename the config entry to pktgen_rx_iface or
change the lookup in pktgen_utils.py (where pktgen_rx_iface is fetched around
line 66) to read client_pktgen_iface (and update any other references to
pktgen_rx_iface in that module to use the chosen name) so the RX test receives
the correct interface value.
| recreate_serial_console = True | ||
|
|
||
| libvirt_vmxml.remove_vm_devices_by_type(vm, "interface") | ||
| vm_attrs = eval(params.get("vm_attrs", "{}")) |
There was a problem hiding this comment.
Use ast.literal_eval instead of eval for security.
eval() can execute arbitrary code if vm_attrs contains malicious input. Since this parses a dictionary literal from configuration, use the safer ast.literal_eval.
Suggested fix
Add the import at the top:
import os
+import astThen replace eval:
- vm_attrs = eval(params.get("vm_attrs", "{}"))
+ vm_attrs = ast.literal_eval(params.get("vm_attrs", "{}"))📝 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.
| vm_attrs = eval(params.get("vm_attrs", "{}")) | |
| vm_attrs = ast.literal_eval(params.get("vm_attrs", "{}")) |
🧰 Tools
🪛 Ruff (0.14.13)
76-76: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In `@libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py` at line 76,
Replace the unsafe use of eval when parsing vm_attrs: instead of vm_attrs =
eval(params.get("vm_attrs", "{}")), import the ast module and call
ast.literal_eval on the params.get(...) value to safely parse the dictionary
literal; update the code path around vm_attrs and ensure any default "{}" is
passed through ast.literal_eval as needed.
| params = params or {} | ||
| guest_mac = vm.get_mac_address(0) | ||
| guest_eth = utils_net.get_linux_ifname(session_serial, guest_mac) | ||
|
|
There was a problem hiding this comment.
Missing null checks for vm and session_serial.
guest_mac and guest_eth are computed unconditionally, but vm and session_serial have default values of None. If either is None, this will raise an AttributeError.
Suggested fix
Add validation at the start:
params = params or {}
+ if vm is None or session_serial is None:
+ raise ValueError("vm and session_serial are required for configure_pktgen")
guest_mac = vm.get_mac_address(0)
guest_eth = utils_net.get_linux_ifname(session_serial, guest_mac)📝 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.
| params = params or {} | |
| guest_mac = vm.get_mac_address(0) | |
| guest_eth = utils_net.get_linux_ifname(session_serial, guest_mac) | |
| params = params or {} | |
| if vm is None or session_serial is None: | |
| raise ValueError("vm and session_serial are required for configure_pktgen") | |
| guest_mac = vm.get_mac_address(0) | |
| guest_eth = utils_net.get_linux_ifname(session_serial, guest_mac) | |
🤖 Prompt for AI Agents
In `@provider/virtual_network/pktgen_utils.py` around lines 43 - 46, Add
null/validation checks before using vm and session_serial in pktgen_utils.py:
ensure vm is not None and has get_mac_address (used to set guest_mac) and ensure
session_serial is not None before calling utils_net.get_linux_ifname (used to
set guest_eth); if either is missing, raise a clear ValueError or return early
with a descriptive error message. Update the block that currently calls
vm.get_mac_address(0) and utils_net.get_linux_ifname(session_serial, guest_mac)
to guard those calls and include the function/variable names (get_mac_address,
get_linux_ifname, guest_mac, guest_eth) in the error messages so the failure
point is obvious.
| remote_session = remote.remote_login("ssh", client_ip, "22", | ||
| username, password, r'[$#%]') | ||
| self.runner = remote_session.cmd |
There was a problem hiding this comment.
Remote session resource leak.
remote_session is created but never closed. The session handle is assigned to self.runner as a method reference, losing the session object needed for cleanup. This can exhaust SSH connections over multiple test iterations.
Suggested fix
Store the session for later cleanup:
remote_session = remote.remote_login("ssh", client_ip, "22",
username, password, r'[$#%]')
+ self.remote_session = remote_session # Store for cleanup
self.runner = remote_session.cmdAdd a cleanup method to PktgenConfig:
def cleanup(self):
"""Clean up resources."""
if hasattr(self, 'remote_session') and self.remote_session:
self.remote_session.close()📝 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.
| remote_session = remote.remote_login("ssh", client_ip, "22", | |
| username, password, r'[$#%]') | |
| self.runner = remote_session.cmd | |
| remote_session = remote.remote_login("ssh", client_ip, "22", | |
| username, password, r'[$#%]') | |
| self.remote_session = remote_session # Store for cleanup | |
| self.runner = remote_session.cmd |
🤖 Prompt for AI Agents
In `@provider/virtual_network/pktgen_utils.py` around lines 67 - 69, The remote
SSH session created via remote.remote_login is assigned only as a command method
to self.runner, causing the remote_session object to be dropped and never
closed; modify the PktgenConfig instantiation to save the session to an instance
attribute (e.g., self.remote_session = remote_session) instead of only storing
its cmd, and add a cleanup method on PktgenConfig (e.g., def cleanup(self): if
getattr(self, "remote_session", None): self.remote_session.close()) and ensure
callers invoke cleanup when tests finish to avoid leaking SSH connections.
| def run_test(script, cmd, runner, interface, timeout, session_serial=None, pkt_cate="tx"): | ||
| """ | ||
| Run pktgen script on remote and gather packet numbers/time and | ||
| calculate mpps. | ||
| :param script: pktgen script name. | ||
| :param cmd: The command to execute the pktgen script | ||
| :param runner: The command runner function | ||
| :param interface: The network interface used by pktgen. | ||
| :param interface: The VM Ethernet interface used to collect packet counters. | ||
| :param timeout: The maximum time allowed for the test to run | ||
| :param session_serial: Session serial for VM | ||
| :param pkt_cate: Packet category (tx/rx), used to select counter type | ||
| :return: The calculated MPPS (Million Packets Per Second) | ||
| """ | ||
|
|
||
| packets = "cat /sys/class/net/%s/statistics/tx_packets" % interface | ||
| counter = "rx_packets" if pkt_cate == "rx" else "tx_packets" | ||
| packets = "cat /sys/class/net/%s/statistics/%s" % (interface, counter) | ||
| LOG_JOB.info("Start pktgen test by cmd '%s'", cmd) | ||
| try: | ||
| packet_b = runner(packets) | ||
| packet_b = session_serial.cmd(packets) | ||
| packet_a = None | ||
| runner(cmd, timeout) | ||
| packet_a = session_serial.cmd(packets) | ||
| except aexpect.ShellTimeoutError: | ||
| # when pktgen script is running on guest, the pktgen process | ||
| # need to be killed. | ||
| kill_cmd = ( | ||
| "kill -9 `ps -ef | grep %s --color | grep -v grep | " | ||
| "awk '{print $2}'`" % script | ||
| ) | ||
| runner(kill_cmd) | ||
| packet_a = runner(packets) | ||
| if session_serial and runner == session_serial.cmd: | ||
| kill_cmd = ( | ||
| "kill -9 `ps -ef | grep %s --color | grep -v grep | " | ||
| "awk '{print $2}'`" % script | ||
| ) | ||
| runner(kill_cmd) | ||
| packet_a = session_serial.cmd(packets) | ||
| except process.CmdError: | ||
| # when pktgen script is running on host, the pktgen process | ||
| # will be quit when timeout triggers, so no need to kill it. | ||
| packet_a = runner(packets) | ||
| packet_a = session_serial.cmd(packets) |
There was a problem hiding this comment.
session_serial used unconditionally but has None default.
Lines 136, 139, 149, and 153 all call session_serial.cmd(packets), but session_serial defaults to None. This will raise AttributeError if the function is called without a session.
Either make session_serial required or add None checks.
Suggested fix - make parameter required
-def run_test(script, cmd, runner, interface, timeout, session_serial=None, pkt_cate="tx"):
+def run_test(script, cmd, runner, interface, timeout, session_serial, pkt_cate="tx"):🤖 Prompt for AI Agents
In `@provider/virtual_network/pktgen_utils.py` around lines 119 - 153, The
run_test function uses session_serial unconditionally (calls
session_serial.cmd(packets)) while it defaults to None; update run_test to
handle a missing session_serial: either make session_serial a required argument
in the function signature or add guards where session_serial.cmd(packets) is
called—use runner(packets) when session_serial is None (or raise a clear
ValueError at the start if you prefer requiring it). Specifically, modify the
calls that reference session_serial.cmd (the initial packet_b read, the packet_a
reads in the try and both except blocks) to check if session_serial is not None
before calling session_serial.cmd, and fall back to runner(packets) or raise a
descriptive error; keep the existing kill logic that checks session_serial and
runner equality intact.
- Add pktgen_perf_test.cfg: new perf test config with tx/rx, multiple scripts, and vhost_vdpa variant (dev_type=vdpa, iface_dict, client, pktgen_tx_dst_mac). - Extend pktgen_burst_mode_test.py: vhost-vdpa setup and session-based verify_dmesg. - Extend pktgen_utils.py: vdpa/rx/client support Signed-off-by: Wenli Quan <[email protected]>
quanwenli
force-pushed
the
pktgen-perf
branch
from
05f9504 to
9cd25f3
Compare
|
Case 1 result with vdpa Case 2 result with bridge |
2 participants
id: LIBVIRTAT-22276
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.