virtual_network: add case for pktgen with burst mode #6531
Conversation
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
4 times, most recently
from
bba7b7e to
9772c0c
Compare
|
@yanglei-rh This case already included multiple queues as your requirement, Please check. Thanks |
|
There are conflicts |
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
from
9772c0c to
b31fe88
Compare
WalkthroughAdds a pktgen burst-mode test: a new cfg entry, a QEMU test runner that sets up bridge/TAP and orchestrates VMs, a pktgen utilities provider for host/guest pktgen orchestration and result formatting, and a VM-naming tweak appending a 3-character random suffix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Runner as pktgen_burst_mode_test.run
participant NetBase as network_base.prepare_vms_with_iface
participant Host as Host OS
participant VM as Test VM(s)
participant PktUtils as pktgen_utils.run_tests_for_category
participant Results as Result File
Tester->>Runner: start(params)
Runner->>Host: create bridge/TAP
Runner->>NetBase: prepare VMs with iface
NetBase-->>Runner: vm list (names w/ rand suffix)
Runner->>VM: boot & configure
Runner->>VM: connectivity & version checks
Runner->>PktUtils: run_tests_for_category(params)
alt host-run
PktUtils->>Host: run pktgen host-side
else guest-run
PktUtils->>VM: configure & run pktgen in guest
end
PktUtils-->>Results: write MPPS/results
Runner->>Host: teardown bridge/TAP
Runner->>VM: destroy/undef non-main
Runner-->>Tester: finish
sequenceDiagram
autonumber
participant PktConfig as PktgenConfig
participant Runner as PktgenRunner
participant Target as Host/Guest
participant File as Result File
PktConfig->>PktConfig: generate_pktgen_cmd(...)
Runner->>Target: start pktgen script (cmd)
Target-->>Runner: tx_packets before/after
Runner->>Runner: compute MPPS
Runner->>File: append formatted result line
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
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: 7
🧹 Nitpick comments (5)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg (1)
10-10: Align header label with emitted column name.The runner writes “threads” values; header uses “run_threads”, which can confuse parsing.
- record_list = "pkt_size run_threads burst mpps" + record_list = "pkt_size threads burst mpps"libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (1)
108-110: Make VM cleanup resilient during the per‑VM loop.Avoid aborting the loop if destroy/undefine fails.
- virsh.destroy(vm_name) - virsh.undefine(vm_name, options='--nvram') + virsh.destroy(vm_name, ignore_status=True) + virsh.undefine(vm_name, options='--nvram', ignore_status=True)provider/virtual_network/pktgen_utils.py (3)
127-171: Use equality for script selection; avoid misleading “in” on strings.
if script in "pktgen_perf":is brittle; use==.- if script in "pktgen_perf": + if script == "pktgen_perf":
254-265: Fix docstring semantics or function name; behavior is “greater than,” not “less than.”As written, it returns True when host RHEL version > rhel_version. Either rename to
is_version_gt_rhelor correct the docstring. Ensure call sites match the intended logic.Option A (docstring fix):
- """ - Check if the system RHEL version is less than the specified version. + """ + Check if the system RHEL version is greater than the specified version.Option B (new helper + deprecate old): I can provide a follow‑up patch if you prefer.
62-64: Prefer narrower exceptions and log with traceback.Catching bare Exception hides real issues; use logging.exception for tracebacks.
- except Exception as err: - LOG_JOB.error("Error during vDPA binding process: %s", err) + except (aexpect.ShellError, process.CmdError, ValueError) as err: + LOG_JOB.exception("Error during vDPA binding process: %s", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py(1 hunks)provider/virtual_network/network_base.py(1 hunks)provider/virtual_network/pktgen_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
provider/virtual_network/network_base.py (4)
create_tap(151-164)prepare_vms_with_iface(496-512)ping_check(97-148)delete_tap(205-211)provider/virtual_network/pktgen_utils.py (5)
run_test(183-221)PktgenRunner(182-265)is_version_lt_rhel(254-265)install_package(223-252)run_tests_for_category(287-416)
provider/virtual_network/pktgen_utils.py (1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
run(33-147)run_test(52-111)
provider/virtual_network/network_base.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
generate_random_string(104-113)
🪛 Ruff (0.13.1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
25-25: Function call with shell=True parameter identified, security issue
(S604)
59-59: Function call with shell=True parameter identified, security issue
(S604)
137-137: Use of possibly insecure function; consider using ast.literal_eval
(S307)
140-140: Use of possibly insecure function; consider using ast.literal_eval
(S307)
provider/virtual_network/pktgen_utils.py
37-37: Abstract raise to an inner function
(TRY301)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
59-61: Abstract raise to an inner function
(TRY301)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
151-151: Probable insecure usage of temporary file or directory: "/tmp/pktgen_perf/%s.sh"
(S108)
232-232: Function call with shell=True parameter identified, security issue
(S604)
248-248: Probable insecure usage of temporary file or directory: "/tmp/%s.rpm"
(S108)
249-249: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Function call with shell=True parameter identified, security issue
(S604)
476-476: 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.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (2)
provider/virtual_network/pktgen_utils.py (1)
241-252: Shell usage acknowledged; keep, but sanitize inputs if sourcing from params.Calls with
shell=Trueare necessary for redirection/pipe usage here; ensure inputs aren’t derived from untrusted params.Please confirm
kernel_veranddownload_urlare constructed solely from trusted sources (uname/builtins), not arbitrary params. If any param can influence these, we should switch to argument lists and avoid shell.Also applies to: 472-476
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (1)
137-141: Replace unsafe eval(...) with ast.literal_eval and safe placeholder substitutionDirect eval on config strings is RCE‑prone — parse literals with ast.literal_eval and inject runtime tap_name safely before parsing.
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (around lines 137-141)
+import ast @@ - iface_list = eval(params.get('iface_list', '{}')) + iface_list_str = params.get('iface_list', '[]') + # replace the unquoted placeholder with the actual value safely + iface_list_str = iface_list_str.replace('tap_name', repr(tap_name)) + iface_list = ast.literal_eval(iface_list_str) @@ - vm_image_paths = eval(params.get('vm_image_paths', '{}')) + vm_image_paths = ast.literal_eval(params.get('vm_image_paths', '[]'))If iface_list may contain placeholders other than tap_name, list them so we can add safe substitutions.
| iface_list = [{'type_name': '${iface_type}', 'model': 'virtio', 'target': {'dev': tap_name, 'managed': 'no'},'driver': {'driver_attr': {'queues': '${queues}', 'name': 'vhost'}}}] | ||
| test_vm = no | ||
| tap_flag = "multi_queue" | ||
| disable_iptables_rules_cmd = "iptables -F ; nft flush ruleset" |
There was a problem hiding this comment.
Flushing firewall rules without restoring is unsafe.
iptables -F ; nft flush ruleset is destructive and not restored in teardown, leaving the host exposed after tests.
Prefer saving and restoring rules (see code suggestions in the test file comment).
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg around line
21, the teardown currently runs a destructive "iptables -F ; nft flush ruleset"
without saving state; change this to save current iptables and nftables rules to
temporary files before flushing, use the saved snapshots in the test teardown to
restore rules, and ensure both iptables (iptables-save/iptables-restore) and
nftables (nft list ruleset / nft -f) are handled with existence checks and error
handling so restoration always runs even if the test fails.
| if disable_iptables_rules_cmd: | ||
| process.system(disable_iptables_rules_cmd, shell=True) | ||
|
|
||
| pktgen_runner = pktgen_utils.PktgenRunner() |
There was a problem hiding this comment.
Save and restore firewall rules around the test.
Avoid leaving the system with flushed rules.
if disable_iptables_rules_cmd:
- process.system(disable_iptables_rules_cmd, shell=True)
+ # Save current firewall state
+ ipt_bak = "/tmp/iptables.rules.bak"
+ nft_bak = "/tmp/nft.rules.bak"
+ process.system(f"iptables-save > {ipt_bak}", shell=True)
+ process.system(f"nft list ruleset > {nft_bak}", shell=True)
+ process.system(disable_iptables_rules_cmd, shell=True)
@@
- utils_net.delete_linux_bridge_tmux(bridge_name, host_iface)
+ utils_net.delete_linux_bridge_tmux(bridge_name, host_iface)
+ # Restore firewall if we changed it
+ if disable_iptables_rules_cmd:
+ process.system("iptables-restore < /tmp/iptables.rules.bak", shell=True, ignore_status=True)
+ process.system("nft -f /tmp/nft.rules.bak", shell=True, ignore_status=True)Also applies to: 113-126
🧰 Tools
🪛 Ruff (0.13.1)
59-59: Function call with shell=True parameter identified, security issue
(S604)
There was a problem hiding this comment.
What will haveppen if utils_net.delete_linux... raised an error? It seems the iptables rules are not recovered and the system will be in bad state. If so, please make sure this won't happen.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the full content of the test file to understand the complete structure
cat libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
Length of output: 5089
@smitterl You're absolutely right to be concerned. If utils_net.delete_linux_bridge_tmux() raises an exception, any cleanup code after it (including firewall restoration) won't execute, leaving the system in a bad state.
The solution is to use nested try-finally blocks or wrap each cleanup operation independently to ensure all cleanup steps attempt to execute even if one fails. Here's a recommended approach:
def run(test, params, env):
"""
Pktgen tests with burst mode.
"""
# ... [setup_test and run_test definitions remain the same] ...
def teardown_test():
"""
Clean data.
"""
test.log.debug("TEST_TEARDOWN: Recover the env.")
# Restore firewall first (if it was disabled)
if disable_iptables_rules_cmd and os.path.exists("/tmp/iptables.rules.bak"):
try:
process.system("iptables-restore < /tmp/iptables.rules.bak",
shell=True, ignore_status=True)
process.system("nft -f /tmp/nft.rules.bak",
shell=True, ignore_status=True)
os.remove("/tmp/iptables.rules.bak")
os.remove("/tmp/nft.rules.bak")
except Exception as e:
test.log.warning("Failed to restore firewall rules: %s", e)
# Delete tap (independent cleanup)
try:
network_base.delete_tap(tap_name)
except Exception as e:
test.log.warning("Failed to delete tap %s: %s", tap_name, e)
# Delete bridge (independent cleanup)
try:
utils_net.delete_linux_bridge_tmux(bridge_name, host_iface)
except Exception as e:
test.log.warning("Failed to delete bridge %s: %s", bridge_name, e)
# Restore VM XML (independent cleanup)
try:
backup_vmxml.sync()
except Exception as e:
test.log.warning("Failed to restore VM XML: %s", e)
# ... [variable initialization remains the same] ...
try:
setup_test()
run_test()
finally:
teardown_test()And in run_test(), save the firewall state before flushing:
def run_test():
"""
Run pktgen in burst mode for vm.
"""
result_file = write_in_result_file(test, params)
if disable_iptables_rules_cmd:
# Save current firewall state
try:
process.system("iptables-save > /tmp/iptables.rules.bak", shell=True)
process.system("nft list ruleset > /tmp/nft.rules.bak", shell=True)
except Exception as e:
test.log.warning("Failed to save firewall rules: %s", e)
process.system(disable_iptables_rules_cmd, shell=True)
# ... [rest of run_test remains the same] ...This approach ensures that:
- Each cleanup operation is independent and won't block others from executing
- Firewall rules are restored even if network cleanup fails
- All errors are logged but don't prevent other cleanup steps
- The system is left in a consistent state regardless of which cleanup step fails
There was a problem hiding this comment.
@coderabbitai - I'm a fan of using a list of cleanup actions defined as lambdas that are added when they become necessary:
do_something(x)
cleanup_actions.append(lambda: undo_something(x))
do_something_else_that_does_not_require_cleanup(x)
...
finally:
cleanup_actions.reverse()
for action in cleaup_actions:
try:
action()
except Exception as e:
logging.warning(f"Failed to cleanup: {action} raised {e}")There was a problem hiding this comment.
@smitterl That's a much cleaner pattern! The lambda list approach is more maintainable and makes it explicit when each cleanup action is registered. Here's how to apply it to this test:
def run(test, params, env):
"""
Pktgen tests with burst mode.
"""
cleanup_actions = []
def setup_test():
"""
Prepare the test environment with required guest type and interface.
:return vm_list: list of vm names.
"""
utils_net.create_linux_bridge_tmux(bridge_name, host_iface)
cleanup_actions.append(lambda: utils_net.delete_linux_bridge_tmux(bridge_name, host_iface))
network_base.create_tap(tap_name, bridge_name, 'root', flag=tap_flag)
cleanup_actions.append(lambda: network_base.delete_tap(tap_name))
test.log.info("TEST_SETUP: Prepare specific guest os version and interface.")
vm_list = network_base.prepare_vms_with_iface(
params, vm_image_path=vm_image_paths, iface_list=iface_list)
return vm_list
def run_test(vm_list):
"""
Run pktgen in burst mode for vm.
"""
result_file = write_in_result_file(test, params)
cleanup_actions.append(lambda: result_file.close())
if disable_iptables_rules_cmd:
# Save current firewall state
process.system("iptables-save > /tmp/iptables.rules.bak", shell=True, ignore_status=True)
process.system("nft list ruleset > /tmp/nft.rules.bak", shell=True, ignore_status=True)
cleanup_actions.append(lambda: (
process.system("iptables-restore < /tmp/iptables.rules.bak",
shell=True, ignore_status=True),
process.system("nft -f /tmp/nft.rules.bak",
shell=True, ignore_status=True)
))
process.system(disable_iptables_rules_cmd, shell=True)
# ... rest of run_test ...
# ... variable initialization ...
try:
vm_list = setup_test()
run_test(vm_list)
finally:
test.log.debug("TEST_TEARDOWN: Executing cleanup actions.")
# Execute in reverse order (LIFO - undo in opposite order of setup)
for action in reversed(cleanup_actions):
try:
action()
except Exception as e:
test.log.warning(f"Cleanup action failed: {e}")
# VM XML restore should happen last
try:
backup_vmxml.sync()
except Exception as e:
test.log.warning(f"Failed to restore VM XML: {e}")The key benefits of this approach:
- Clear coupling: Cleanup is registered right after the action that requires it
- Automatic ordering: Reversing the list gives us LIFO cleanup (undo in reverse order)
- Resilient: Each cleanup action is isolated - one failure doesn't prevent others
- Maintainable: Adding new setup/cleanup pairs is straightforward
✏️ Learnings added
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.427Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
| guest_ver = session_serial.cmd_output(guest_ver_cmd) | ||
| result_file.write("### guest-kernel-ver :%s" % guest_ver) | ||
| if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7): | ||
| if guest_ver.count("64k"): | ||
| pktgen_runner.install_package( | ||
| guest_ver.strip(), | ||
| pagesize="64k", | ||
| vm=vm, | ||
| session_serial=session_serial) | ||
| else: | ||
| pktgen_runner.install_package( | ||
| guest_ver.strip(), vm=vm, session_serial=session_serial) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Fix kernel version retrieval and result formatting.
- Use cmd_output to fetch the guest kernel string; .cmd is not appropriate.
- Add newline after writing guest kernel version for readability.
- guest_ver = session_serial.cmd_output(guest_ver_cmd)
- result_file.write("### guest-kernel-ver :%s" % guest_ver)
- if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7):
+ guest_ver = session_serial.cmd_output(guest_ver_cmd).strip()
+ result_file.write("### guest-kernel-ver : %s\n" % guest_ver)
+ if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7):Confirm intended semantics: the helper currently returns True when host RHEL version is GREATER than the threshold (see pktgen_utils.is_version_lt_rhel). If you intended “less than,” adjust that helper or switch to a correctly named one.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo ">>> rg is_version_lt_rhel (all matches)"
rg -n --hidden -S "is_version_lt_rhel" || true
echo
echo ">>> rg def is_version_lt_rhel (definitions)"
rg -n --hidden -S "def\s+is_version_lt_rhel" || true
echo
echo ">>> rg pktgen_utils or pktgen_runner"
rg -n --hidden -S "pktgen_utils|pktgen_runner" || true
echo
echo ">>> Show libvirt tests file (first 240 lines)"
if [ -f libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py ]; then
sed -n '1,240p' libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py || true
else
echo "File not found: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py"
fi
echo
echo ">>> Print files that define is_version_lt_rhel (and first 240 lines)"
FILES=$(rg -n --hidden -S "def\s+is_version_lt_rhel" -l || true)
if [ -n "$FILES" ]; then
for f in $FILES; do
echo "----- $f -----"
sed -n '1,240p' "$f" || true
done
else
echo "No files defining is_version_lt_rhel found."
fi
echo
echo ">>> Search occurrences of session_serial.cmd and session_serial.cmd_output"
rg -n --hidden -S "session_serial\.cmd\(" || true
rg -n --hidden -S "session_serial\.cmd_output\(" || trueLength of output: 16890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo ">>> Showing pktgen_utils.is_version_lt_rhel (lines 240-300)"
sed -n '240,300p' provider/virtual_network/pktgen_utils.py || trueLength of output: 2061
Fix guest kernel retrieval, result formatting, and inverted helper
- Replace guest-side use of session_serial.cmd(...) with session_serial.cmd_output(...).strip() and add a newline to the result file for readability.
- guest_ver = session_serial.cmd_output(guest_ver_cmd)
- result_file.write("### guest-kernel-ver :%s" % guest_ver)
- if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7):
+ guest_ver = session_serial.cmd_output(guest_ver_cmd).strip()
+ result_file.write("### guest-kernel-ver : %s\n" % guest_ver)
+ if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7):- Critical: provider/virtual_network/pktgen_utils.py:254 — is_version_lt_rhel currently returns int(ver[0]) > int(rhel_version) (i.e., True when the system version is GREATER than rhel_version), which contradicts the name/docstring. Either change the implementation to use '<' for a true "less than" check or rename/update the helper and its docstring; update callsites if you change the name.
📝 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.
| guest_ver = session_serial.cmd_output(guest_ver_cmd) | |
| result_file.write("### guest-kernel-ver :%s" % guest_ver) | |
| if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7): | |
| if guest_ver.count("64k"): | |
| pktgen_runner.install_package( | |
| guest_ver.strip(), | |
| pagesize="64k", | |
| vm=vm, | |
| session_serial=session_serial) | |
| else: | |
| pktgen_runner.install_package( | |
| guest_ver.strip(), vm=vm, session_serial=session_serial) | |
| guest_ver = session_serial.cmd_output(guest_ver_cmd).strip() | |
| result_file.write("### guest-kernel-ver : %s\n" % guest_ver) | |
| if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7): | |
| if guest_ver.count("64k"): | |
| pktgen_runner.install_package( | |
| guest_ver.strip(), | |
| pagesize="64k", | |
| vm=vm, | |
| session_serial=session_serial) | |
| else: | |
| pktgen_runner.install_package( | |
| guest_ver.strip(), vm=vm, session_serial=session_serial) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py around lines
88 to 100, the guest kernel/version retrieval and result formatting use
session_serial.cmd(...) and do not add a newline; change the guest-side call to
session_serial.cmd_output(...).strip() and write the result_file entry with a
trailing newline for readability (e.g., write("### guest-kernel-ver :%s\n" %
guest_ver)). Also fix the inverted helper in
provider/virtual_network/pktgen_utils.py at the function around line 254: the
implementation currently uses int(ver[0]) > int(rhel_version) which yields True
for versions greater than rhel_version—change the comparison to int(ver[0]) <
int(rhel_version) to match the is_version_lt_rhel name/docstring (or if you
prefer to keep the current logic, rename the helper and update all callsites
accordingly); ensure all callsites (like this test) continue to use the
corrected semantics.
| rand_id = utils_misc.generate_random_string(3) | ||
| vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id | ||
| LOG.debug("TEST_LOOP: Test '%s' with guest version '%s'.", vm_name, guest_version) |
There was a problem hiding this comment.
❓ Verification inconclusive
Harden VM name derivation; avoid IndexError and reduce collision risk.
re.findall(...)[0] will crash when guest_version doesn’t contain “Win” or “RHEL”. Also, appending a 3‑char suffix without a separator increases collision risk and hurts readability.
Apply:
- rand_id = utils_misc.generate_random_string(3)
- vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id
+ rand_id = utils_misc.generate_random_string(5)
+ m = re.search(r"(Win|RHEL)", guest_version, re.IGNORECASE)
+ base = (m.group(0) if m else os.path.splitext(guest_version)[0])
+ vm_name = f"{base}_vm_{rand_id}"Run to ensure no callers assume a fixed VM name without the random suffix:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '(RHEL|Win)_vm\b' --type=pyLength of output: 0
Harden VM name derivation to avoid IndexError and reduce collision risk
- Use
re.searchwith fallback to the guest_version’s filename base instead ofre.findall(...)[0] - Increase the random suffix to 5 characters and include a clear
_separator - Verified no occurrences of literal
Win_vmorRHEL_vmin the codebase, so this change won’t break downstream callers
- rand_id = utils_misc.generate_random_string(3)
- vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id
+ rand_id = utils_misc.generate_random_string(5)
+ m = re.search(r"(Win|RHEL)", guest_version, re.IGNORECASE)
+ base = (m.group(0) if m else os.path.splitext(guest_version)[0])
+ vm_name = f"{base}_vm_{rand_id}"📝 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.
| rand_id = utils_misc.generate_random_string(3) | |
| vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id | |
| LOG.debug("TEST_LOOP: Test '%s' with guest version '%s'.", vm_name, guest_version) | |
| rand_id = utils_misc.generate_random_string(5) | |
| m = re.search(r"(Win|RHEL)", guest_version, re.IGNORECASE) | |
| base = (m.group(0) if m else os.path.splitext(guest_version)[0]) | |
| vm_name = f"{base}_vm_{rand_id}" | |
| LOG.debug("TEST_LOOP: Test '%s' with guest version '%s'.", vm_name, guest_version) |
🤖 Prompt for AI Agents
In provider/virtual_network/network_base.py around lines 506-508, the VM name
construction can raise IndexError and has weak randomness; replace the
re.findall(...)[0] usage with re.search("Win|RHEL", re.IGNORECASE) and use
match.group(0) when present, otherwise fall back to the guest_version filename
base (e.g., os.path.basename(guest_version).split('.')[0]); increase the random
suffix from 3 to 5 characters and ensure there is a clear "_" separator before
the suffix (e.g., "<prefix>_vm_<rand5>"); if os is not already imported, add the
import. Ensure the final vm_name assembly handles None safely and retains the
LOG.debug call.
| self.runner = session_serial.cmd | ||
| elif pkt_cate == "rx": |
There was a problem hiding this comment.
Runner should return command output, not status.
run_test() expects runner(cmd, ...) and runner(packets) to return stdout. session_serial.cmd typically returns status; use cmd_output.
- self.runner = session_serial.cmd
+ self.runner = session_serial.cmd_output
@@
- self.runner = session_serial.cmd
+ self.runner = session_serial.cmd_outputAlso applies to: 118-119
🤖 Prompt for AI Agents
In provider/virtual_network/pktgen_utils.py around lines 99-100 (and similarly
lines 118-119), the code assigns session_serial.cmd (which returns exit status)
to self.runner but run_test() expects a callable that returns command stdout;
change the assignment to use the session_serial method that returns output
(e.g., session_serial.cmd_output or equivalent) so runner(cmd, ...) and
runner(packets) return stdout; update both locations to assign the
output-returning helper and ensure any calls handle returned stdout rather than
status.
| if ( | ||
| session_serial | ||
| and hasattr(self.runner, "__name__") | ||
| and self.runner.__name__ == session_serial.cmd.__name__ | ||
| ): | ||
| cmd += " &" | ||
|
|
||
| return cmd |
There was a problem hiding this comment.
Deterministically collect packet counters; don’t rely on timeout exceptions.
Backgrounding with “ &” makes runner(cmd, timeout) return immediately; then packet_a stays None. Make command blocking, drop the “ &”, and ensure packet_a is always sampled.
- if (
- session_serial
- and hasattr(self.runner, "__name__")
- and self.runner.__name__ == session_serial.cmd.__name__
- ):
- cmd += " &"
+ # Always run in foreground; timeout handling is done in run_test
+ # to ensure we consistently sample packet counters after execution.
return cmdAnd in run_test:
- packet_b = runner(packets)
- packet_a = None
- runner(cmd, timeout)
+ packet_b = runner(packets)
+ packet_a = None
+ runner(cmd, timeout)
@@
- count = int(packet_a) - int(packet_b)
+ if packet_a is None:
+ packet_a = runner(packets)
+ count = int(packet_a) - int(packet_b)Also applies to: 182-221
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
2 times, most recently
from
2e65857 to
9165d9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
94-96: Use cmd_output, strip, and write newline for guest kernel; avoid cmd()Use
cmd_output(...).strip()and ensure the line ends with “\n”. Also usecmd_output("uname -r")for the version check.- guest_ver = session_serial.cmd_output(guest_ver_cmd) - result_file.write("### guest-kernel-ver :%s" % guest_ver) - if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7): + guest_ver = session_serial.cmd_output(guest_ver_cmd).strip() + result_file.write("### guest-kernel-ver : %s\n" % guest_ver) + if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7):
63-117: Always close result file and restore firewall rulesEnsure the result file is closed on exceptions and restore iptables/nftables after disabling rules.
- result_file = write_in_result_file(test, params) - - if disable_iptables_rules_cmd: - process.system(disable_iptables_rules_cmd, shell=True) + result_file = write_in_result_file(test, params) + ipt_bak = nft_bak = None + try: + if disable_iptables_rules_cmd: + ipt_bak = "/tmp/iptables.rules.bak" + nft_bak = "/tmp/nft.rules.bak" + process.system(f"iptables-save > {ipt_bak}", shell=True) + process.system(f"nft list ruleset > {nft_bak}", shell=True) + process.system(disable_iptables_rules_cmd, shell=True) @@ - result_file.close() + finally: + # Restore firewall if modified + if disable_iptables_rules_cmd: + process.system("iptables-restore < /tmp/iptables.rules.bak", shell=True, ignore_status=True) + process.system("nft -f /tmp/nft.rules.bak", shell=True, ignore_status=True) + result_file.close()provider/virtual_network/network_base.py (1)
506-507: Harden VM name derivation; prevent IndexError and reduce collisionsRework to use
re.searchwith fallback and a longer, separated suffix.- rand_id = utils_misc.generate_random_string(3) - vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id + rand_id = utils_misc.generate_random_string(5) + m = re.search(r"(Win|RHEL)", guest_version, re.IGNORECASE) + base = (m.group(0) if m else os.path.splitext(guest_version)[0]) + vm_name = f"{base}_vm_{rand_id}"provider/virtual_network/pktgen_utils.py (2)
98-100: Runner must return stdout, not statusAssign
session_serial.cmd_outputsorun_test()gets counters and command output.- self.runner = session_serial.cmd + self.runner = session_serial.cmd_output @@ - self.runner = session_serial.cmd + self.runner = session_serial.cmd_outputAlso applies to: 118-119
151-178: Foreground execution and deterministic counter sampling; fix script check
- Use equality, not substring:
script == "pktgen_perf".- Don’t append “ &”; rely on timeout and always sample
packet_aif not set.- if script in "pktgen_perf": + if script == "pktgen_perf": @@ - if ( - session_serial - and hasattr(self.runner, "__name__") - and self.runner.__name__ == session_serial.cmd.__name__ - ): - cmd += " &" + # Run in foreground; timeout handling is in run_test return cmd- packet_b = runner(packets) - packet_a = None - runner(cmd, timeout) + packet_b = runner(packets) + packet_a = None + runner(cmd, timeout) @@ - count = int(packet_a) - int(packet_b) + if packet_a is None: + packet_a = runner(packets) + count = int(packet_a) - int(packet_b)Also applies to: 182-221
🧹 Nitpick comments (3)
provider/virtual_network/network_base.py (1)
473-479: Avoid substring matches when checking existing domains
if vm_name not in virsh.dom_list("--all").stdout_text:can misfire on substrings. Prefer name-only output or a structured check (e.g., dom_list with--name, ordominfotry/except). Add a tokenized comparison if wrappers don’t support--name.libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (1)
34-34: Shell usage and eval()
- S604:
shell=Trueat Lines 34 and 66. If commands are trusted, ok; otherwise prefer arg lists andshell=False.- S307:
eval()on params at Line 144; switch toast.literal_eval.+import ast @@ - kvm_ver = process.system_output(kvm_ver_chk_cmd, shell=True).decode() + # If kvm_ver_chk_cmd is controlled, keep shell=True; otherwise pass args and set shell=False. + kvm_ver = process.system_output(kvm_ver_chk_cmd, shell=True).decode() @@ - iface_list = eval(params.get('iface_list', '{}')) + iface_list = ast.literal_eval(params.get('iface_list', '{}'))Also applies to: 66-66, 144-144
provider/virtual_network/pktgen_utils.py (1)
62-64: Log exceptions with traceback for vDPA bindingPrefer
logging.exceptionto capture stack traces.- except Exception as err: - LOG_JOB.error("Error during vDPA binding process: %s", err) + except Exception: + LOG_JOB.exception("Error during vDPA binding process")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py(1 hunks)provider/virtual_network/network_base.py(2 hunks)provider/virtual_network/pktgen_utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg
🧰 Additional context used
🧬 Code graph analysis (3)
provider/virtual_network/network_base.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
generate_random_string(104-113)
provider/virtual_network/pktgen_utils.py (1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
run(42-152)run_test(59-117)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
provider/virtual_network/network_base.py (4)
create_tap(151-164)prepare_single_vm(465-493)ping_check(97-148)delete_tap(205-211)provider/virtual_network/pktgen_utils.py (5)
run_test(183-221)PktgenRunner(182-265)is_version_lt_rhel(254-265)install_package(223-252)run_tests_for_category(287-416)
🪛 Ruff (0.14.1)
provider/virtual_network/pktgen_utils.py
37-37: Abstract raise to an inner function
(TRY301)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
59-61: Abstract raise to an inner function
(TRY301)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
151-151: Probable insecure usage of temporary file or directory: "/tmp/pktgen_perf/%s.sh"
(S108)
232-232: Function call with shell=True parameter identified, security issue
(S604)
248-248: Probable insecure usage of temporary file or directory: "/tmp/%s.rpm"
(S108)
249-249: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Function call with shell=True parameter identified, security issue
(S604)
476-476: Function call with shell=True parameter identified, security issue
(S604)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
34-34: Function call with shell=True parameter identified, security issue
(S604)
66-66: Function call with shell=True parameter identified, security issue
(S604)
144-144: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
🔇 Additional comments (1)
provider/virtual_network/pktgen_utils.py (1)
94-96: No action required—return type is correct as written.Based on analysis of 40+ call sites in the codebase,
utils_net.get_linux_ifname()exhibits clear behavior:
- With
mac_addressparameter: Returns a string (direct assignment is correct)- Without
mac_addressparameter: Returns a list (requires[0]indexing)At line 94,
guest_macis passed as the second argument, so the function returns a string. The code is correct as written; no indexing is needed.
| network_base.prepare_single_vm(params, vm_name, iface_list) | ||
|
|
There was a problem hiding this comment.
prepare_single_vm called with wrong argument order (iface_list passed as disk_path)
This breaks VM prep. Pass by keyword to avoid misbinding.
- network_base.prepare_single_vm(params, vm_name, iface_list)
+ network_base.prepare_single_vm(params, vm_name, iface_list=iface_list)🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py around lines
56-57, prepare_single_vm is being called with iface_list in the position of
disk_path which misbinds arguments; change the call to pass iface_list as a
keyword argument (or reorder positional arguments to the correct signature) so
iface_list is bound to the iface_list parameter instead of disk_path.
There was a problem hiding this comment.
@nanli1 ^^ AFAICT coderabbitai is correct, the third argument in your network_base.py of this commit is disk_path - please fix.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7): | ||
| if host_ver.count("64k"): | ||
| pktgen_runner.install_package(host_ver, pagesize="64k") | ||
| else: | ||
| pktgen_runner.install_package(host_ver) | ||
|
|
There was a problem hiding this comment.
Version check helper semantics likely inverted; package install gating is wrong
is_version_lt_rhel(uname, 7) currently returns True for versions greater than 7 (see utils). Fix helper to implement “<” or rename and adjust call sites. Otherwise you’ll install packages on newer hosts.
| def is_version_lt_rhel(self, uname_str, rhel_version): | ||
| """ | ||
| Check if the system RHEL version is less than the specified version. | ||
| :param uname_str: Output from uname command containing kernel version info | ||
| :param rhel_version: RHEL version to compare against | ||
| :return: True if system RHEL version is greater than rhel_version | ||
| """ | ||
| ver = get_host_version(uname_str) | ||
| if ver: | ||
| return int(ver[0]) > int(rhel_version) | ||
| return False |
There was a problem hiding this comment.
Fix is_version_lt_rhel semantics to match name/docstring
Currently returns True for versions greater than the threshold. Use “<”.
- def is_version_lt_rhel(self, uname_str, rhel_version):
+ def is_version_lt_rhel(self, uname_str, rhel_version):
"""
- Check if the system RHEL version is less than the specified version.
+ Check if the system RHEL version is less than the specified version.
@@
- :return: True if system RHEL version is greater than rhel_version
+ :return: True if system RHEL version is less than rhel_version
"""
ver = get_host_version(uname_str)
if ver:
- return int(ver[0]) > int(rhel_version)
+ return int(ver[0]) < int(rhel_version)
return False📝 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 is_version_lt_rhel(self, uname_str, rhel_version): | |
| """ | |
| Check if the system RHEL version is less than the specified version. | |
| :param uname_str: Output from uname command containing kernel version info | |
| :param rhel_version: RHEL version to compare against | |
| :return: True if system RHEL version is greater than rhel_version | |
| """ | |
| ver = get_host_version(uname_str) | |
| if ver: | |
| return int(ver[0]) > int(rhel_version) | |
| return False | |
| def is_version_lt_rhel(self, uname_str, rhel_version): | |
| """ | |
| Check if the system RHEL version is less than the specified version. | |
| :param uname_str: Output from uname command containing kernel version info | |
| :param rhel_version: RHEL version to compare against | |
| :return: True if system RHEL version is less than rhel_version | |
| """ | |
| ver = get_host_version(uname_str) | |
| if ver: | |
| return int(ver[0]) < int(rhel_version) | |
| return False |
🤖 Prompt for AI Agents
In provider/virtual_network/pktgen_utils.py around lines 254 to 265, the
is_version_lt_rhel function's semantics are inverted: it should return True when
the host RHEL version is less than the provided rhel_version but currently
returns True when greater. Change the comparison from ">" to "<" (i.e., return
int(ver[0]) < int(rhel_version)) so the behavior matches the name and docstring;
keep the int casts and return False when ver is falsy.
There was a problem hiding this comment.
Agree with coderabbitai, seems the function returns True if ver from uname_str is greater then rhel_version; this translates to is_ver_gt_rhel. Please fix.
I'm also not sure if you want to return False if you didn't get the major version from the uname string. Doesn't this mean that you run the test on a non-RHEL environment so that your whole preconditions are messed up? In that case you should mabye better raise a TestError
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usages of is_version_lt_rhel to understand the context
rg -n "is_version_lt_rhel" --type=py -C3
Length of output: 2226
🏁 Script executed:
#!/bin/bash
# Find how TestError or similar exceptions are imported in the codebase
rg -n "from.*import.*TestError" --type=py | head -20
Length of output: 1432
@smitterl You're absolutely right on both points. Here's the corrected version:
def is_version_lt_rhel(self, uname_str, rhel_version):
"""
Check if the system RHEL version is less than the specified version.
:param uname_str: Output from uname command containing kernel version info
:param rhel_version: RHEL version to compare against
:return: True if system RHEL version is less than rhel_version
:raises TestError: If RHEL version cannot be extracted from uname string
"""
from avocado.core.exceptions import TestError
ver = get_host_version(uname_str)
if ver:
return int(ver[0]) < int(rhel_version)
raise TestError(
"Failed to extract RHEL version from uname string: %s" % uname_str
)Changes made:
- Fixed the comparison operator from
>to<to match the function name and docstring - Replaced
return Falsewithraise TestErrorwhen version parsing fails, making test environment issues explicit - Updated the return docstring to correctly state "less than" instead of "greater than"
Important note: This logic change will invert when install_package is called. Currently at lines 68 and 95 in pktgen_burst_mode_test.py, packages are installed for RHEL 8+ (when version > 7). After this fix, packages will be installed for RHEL 6 and below (when version < 7). Please verify this matches your intended behavior for those test scenarios.
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
from
9165d9f to
2cbd5c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
provider/virtual_network/network_base.py (1)
506-507: Harden VM name derivation to avoid IndexError and reduce collision risk.
re.findall(...)[0]will crash whenguest_versiondoesn't contain "Win" or "RHEL". The 3-character random suffix also increases collision risk.Apply the fix from the previous review:
- rand_id = utils_misc.generate_random_string(3) - vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"]) + rand_id + rand_id = utils_misc.generate_random_string(5) + m = re.search(r"(Win|RHEL)", guest_version, re.IGNORECASE) + base = (m.group(0) if m else os.path.splitext(guest_version)[0]) + vm_name = f"{base}_vm_{rand_id}"libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (4)
56-56: Critical: Argument mismatch breaks VM preparation.
iface_listis passed in thedisk_pathparameter position, which will cause VM setup to fail or behave incorrectly.- network_base.prepare_single_vm(params, vm_name, iface_list) + network_base.prepare_single_vm(params, vm_name, disk_path='', iface_list=iface_list)
62-116: Ensure result file is always closed and firewall rules are restored.The result file is not wrapped in a try/finally block, risking a file handle leak if an exception occurs. Additionally, flushed iptables rules are never restored.
Move
result_fileinto a try/finally or use a context manager:def run_test(): """ Run pktgen in burst mode for vm. """ result_file = write_in_result_file(test, params) + try: + if disable_iptables_rules_cmd: + # Save current firewall state + process.system("iptables-save > /tmp/iptables.rules.bak", shell=True) + process.system(disable_iptables_rules_cmd, shell=True) - if disable_iptables_rules_cmd: - process.system(disable_iptables_rules_cmd, shell=True) - - pktgen_runner = pktgen_utils.PktgenRunner() - if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7): + pktgen_runner = pktgen_utils.PktgenRunner() + if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7): ... - virsh.destroy(vm_name) - virsh.undefine(vm_name, options='--nvram') - - result_file.close() + virsh.destroy(vm_name) + virsh.undefine(vm_name, options='--nvram') + finally: + result_file.close() + if disable_iptables_rules_cmd: + process.system("iptables-restore < /tmp/iptables.rules.bak", shell=True, ignore_status=True)
68-72: Critical: Version check helper has inverted semantics.
is_version_lt_rhelcurrently returnsTruefor versions greater than the threshold (see pktgen_utils.py:254-265), causing package installation logic to execute on the wrong RHEL versions.Fix the helper in
provider/virtual_network/pktgen_utils.py(see that file's review) or invert the condition here.
93-104: Fix guest kernel version retrieval and add newline for readability.Line 95 uses
session_serial.cmd("uname -r")which returns exit status rather than output. Usecmd_output()instead. Also add a newline after writing the guest kernel version.guest_ver = session_serial.cmd_output(guest_ver_cmd) - result_file.write("### guest-kernel-ver :%s" % guest_ver) - if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7): + result_file.write("### guest-kernel-ver : %s\n" % guest_ver.strip()) + if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7):provider/virtual_network/pktgen_utils.py (4)
99-99: Critical: Assign output-returning method to runner.
session_serial.cmdreturns exit status, butrun_test()expectsrunner()to return command output. Usecmd_outputinstead.- self.runner = session_serial.cmd + self.runner = session_serial.cmd_output
118-118: Critical: Assign output-returning method to runner.Same issue as line 99 — use
cmd_outputinstead ofcmd.- self.runner = session_serial.cmd + self.runner = session_serial.cmd_output
172-179: Critical: Backgrounding command breaks packet counter collection.Appending
" &"causes the command to background and return immediately, leavingpacket_a = Noneinrun_test(), which then crashes atint(packet_a)on line 214.Remove the backgrounding logic and ensure
packet_ais always sampled after execution:- if ( - session_serial - and hasattr(self.runner, "__name__") - and self.runner.__name__ == session_serial.cmd.__name__ - ): - cmd += " &" - return cmdAnd in
run_test(lines 197-214), ensurepacket_ais collected:try: packet_b = runner(packets) packet_a = None runner(cmd, timeout) + packet_a = runner(packets) except aexpect.ShellTimeoutError: ... + if packet_a is None: + packet_a = runner(packets) count = int(packet_a) - int(packet_b)
254-265: Critical: Fix inverted comparison to match function name and docstring.The function name and docstring say "less than" but the implementation returns
Truefor versions greater than the threshold. This breaks all call sites.def is_version_lt_rhel(self, uname_str, rhel_version): """ Check if the system RHEL version is less than the specified version. :param uname_str: Output from uname command containing kernel version info :param rhel_version: RHEL version to compare against - :return: True if system RHEL version is greater than rhel_version + :return: True if system RHEL version is less than rhel_version """ ver = get_host_version(uname_str) if ver: - return int(ver[0]) > int(rhel_version) + return int(ver[0]) < int(rhel_version) return False
🧹 Nitpick comments (2)
provider/virtual_network/pktgen_utils.py (2)
62-63: Uselogging.exceptionto preserve traceback information.Catching broad
Exceptionand logging with.error()loses the stack trace, making debugging harder.except Exception as err: - LOG_JOB.error("Error during vDPA binding process: %s", err) + LOG_JOB.exception("Error during vDPA binding process") + raise
310-312: Remove unused parameter retrievals.Lines 310-312 call
params.get()but don't assign the results, making them no-ops. Remove these dead code statements.timeout = float(params.get("pktgen_test_timeout", "240")) category = params.get("category") - params.get("pkt_size") - params.get("pktgen_threads") burst = params.get("burst")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py(1 hunks)provider/virtual_network/network_base.py(2 hunks)provider/virtual_network/pktgen_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg
🧰 Additional context used
🧬 Code graph analysis (3)
provider/virtual_network/network_base.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
generate_random_string(104-113)
provider/virtual_network/pktgen_utils.py (1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
run(42-151)run_test(58-116)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
provider/virtual_network/network_base.py (4)
create_tap(151-164)prepare_single_vm(465-493)ping_check(97-148)delete_tap(205-211)provider/virtual_network/pktgen_utils.py (5)
run_test(183-221)PktgenRunner(182-265)is_version_lt_rhel(254-265)install_package(223-252)run_tests_for_category(287-416)
🪛 Ruff (0.14.1)
provider/virtual_network/pktgen_utils.py
37-37: Abstract raise to an inner function
(TRY301)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
59-61: Abstract raise to an inner function
(TRY301)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
151-151: Probable insecure usage of temporary file or directory: "/tmp/pktgen_perf/%s.sh"
(S108)
232-232: Function call with shell=True parameter identified, security issue
(S604)
248-248: Probable insecure usage of temporary file or directory: "/tmp/%s.rpm"
(S108)
249-249: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Function call with shell=True parameter identified, security issue
(S604)
476-476: Function call with shell=True parameter identified, security issue
(S604)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
34-34: Function call with shell=True parameter identified, security issue
(S604)
65-65: Function call with shell=True parameter identified, security issue
(S604)
143-143: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (3)
provider/virtual_network/network_base.py (1)
473-473: LGTM: Expanded domain listing to include inactive VMs.The change to
virsh.dom_list("--all")correctly includes inactive domains when checking VM existence, preventing redundant initialization attempts.libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (1)
23-39: File handle management relies on caller discipline.The function returns an open file handle. Ensure the caller always closes it, preferably in a
finallyblock (see comment on lines 62-116).provider/virtual_network/pktgen_utils.py (1)
429-476: Well-structured installation function with appropriate error handling.The architecture and version detection with explicit error messages for unsupported cases is good practice. The use of
shell=Trueon lines 475-476 is acceptable here for redirecting output to console devices.
| host_iface = utils_net.get_default_gateway(iface_name=True, force_dhcp=True, json=True) | ||
| host_ip = utils_net.get_ip_address_by_interface(host_iface, ip_ver='ipv4') | ||
|
|
||
| iface_list = eval(params.get('iface_list', '{}')) |
There was a problem hiding this comment.
Use ast.literal_eval instead of eval for safer configuration parsing.
eval() can execute arbitrary code. Use ast.literal_eval() to safely parse dictionary literals from test parameters.
+import ast
+
...
- iface_list = eval(params.get('iface_list', '{}'))
+ iface_list = ast.literal_eval(params.get('iface_list', '{}'))🧰 Tools
🪛 Ruff (0.14.1)
143-143: 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 around line
143, the code uses eval(params.get('iface_list', '{}')) which is unsafe; replace
eval with ast.literal_eval to safely parse the parameter string, add "import
ast" at the module top, and wrap the literal_eval call in a try/except catching
(ValueError, SyntaxError) to fall back to an empty dict or raise a clear error
if parsing fails.
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Please update your commit header and message according to the avocado style guide - https://avocado-framework.readthedocs.io/en/latest/guides/contributor/chapters/styleguides.html#commit-style-guide
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
from
2cbd5c7 to
47e6339
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg (1)
15-15: Flushing firewall rules without restoration is unsafe; snapshot and restore.Current value runs “iptables -F ; nft flush ruleset” and leaves the host exposed post‑test if teardown doesn’t restore.
Two options:
- Config-only: add params for saved snapshots and use them in teardown (e.g., ipt_bak=/tmp/iptables.rules.bak, nft_bak=/tmp/nft.rules.bak).
- Code-side: handle save/restore in the Python test when disable_iptables_rules_cmd is set (see proposed diff in the test file comment).
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (4)
62-66: Always close result file even on exceptions.Wrap usage in try/finally (or a with‑style helper) to avoid FD leak.
Apply:
- result_file = write_in_result_file(test, params) + result_file = write_in_result_file(test, params) + try: ... - result_file.close() + finally: + result_file.close()Also applies to: 116-116
64-66: Save and restore iptables/nftables around the test.Don’t leave the host with flushed rules.
Apply:
if disable_iptables_rules_cmd: - process.system(disable_iptables_rules_cmd, shell=True) + ipt_bak = "/tmp/iptables.rules.bak" + nft_bak = "/tmp/nft.rules.bak" + process.system(f"iptables-save > {ipt_bak}", shell=True) + process.system(f"nft list ruleset > {nft_bak}", shell=True) + process.system(disable_iptables_rules_cmd, shell=True) @@ utils_net.delete_linux_bridge_tmux(bridge_name, host_iface) + # Restore firewall if changed + if disable_iptables_rules_cmd: + process.system("iptables-restore < /tmp/iptables.rules.bak", shell=True, ignore_status=True) + process.system("nft -f /tmp/nft.rules.bak", shell=True, ignore_status=True)Also applies to: 122-126
67-73: Version check/output handling: use cmd_output and fix guest kernel line.Use cmd_output().strip() and write a newline for readability. Ensure uname uses cmd_output too.
Apply:
- if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7): + if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7): ... - guest_ver = session_serial.cmd_output(guest_ver_cmd) - result_file.write("### guest-kernel-ver :%s" % guest_ver) - if pktgen_runner.is_version_lt_rhel(session_serial.cmd("uname -r"), 7): + guest_ver = session_serial.cmd_output(guest_ver_cmd).strip() + result_file.write("### guest-kernel-ver : %s\n" % guest_ver) + if pktgen_runner.is_version_lt_rhel(session_serial.cmd_output("uname -r").strip(), 7): ...Note: see utils helper semantics fix in pktgen_utils comment; once fixed, these calls will gate installs correctly.
Also applies to: 93-96
55-57: Critical: wrong positional args to prepare_single_vm; iface_list bound to disk_path.Bind by keyword to avoid misbinding and broken VM XML prep.
Apply:
- network_base.prepare_single_vm(params, vm_name, iface_list) + network_base.prepare_single_vm(params, vm_name, iface_list=iface_list)provider/virtual_network/pktgen_utils.py (4)
97-100: Use output-returning runner in guest paths.session_serial.cmd returns status, not stdout; run_test expects stdout for counters and to run commands.
Apply:
- self.runner = session_serial.cmd + self.runner = session_serial.cmd_output @@ - self.runner = session_serial.cmd + self.runner = session_serial.cmd_outputAlso applies to: 116-119
172-179: Don’t background the pktgen command (“ &”); it breaks counter sampling.Let timeout handling control duration; keep in foreground.
Apply:
- if ( - session_serial - and hasattr(self.runner, "__name__") - and self.runner.__name__ == session_serial.cmd.__name__ - ): - cmd += " &" + # Run in foreground; timeout in run_test controls duration.
195-214: Guarantee post-run sampling even when no exception is raised.If no exception is thrown and command was foreground, ensure packet_a is sampled.
Apply:
try: packet_b = runner(packets) packet_a = None runner(cmd, timeout) @@ - count = int(packet_a) - int(packet_b) + if packet_a is None: + packet_a = runner(packets) + count = int(packet_a) - int(packet_b)
254-265: Fix is_version_lt_rhel semantics (“lt” should be <, not >).Current code returns True for versions greater than rhel_version.
Apply:
- :return: True if system RHEL version is greater than rhel_version + :return: True if system RHEL version is less than rhel_version @@ - if ver: - return int(ver[0]) > int(rhel_version) + if ver: + return int(ver[0]) < int(rhel_version)Then re-check call sites in the test file (host and guest checks) to ensure intended gating.
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (1)
143-145: Avoid raw eval for iface_list; build dict or restrict eval environment.eval(params['iface_list']) is risky. Prefer constructing iface_list in code from params, or at least restrict eval’s environment.
Option A (preferred): construct programmatically.
- iface_list = eval(params.get('iface_list', '{}')) + iface_type = params.get("iface_type") + queues = int(params.get("queues", "1")) + if iface_type == "ethernet": + iface_list = [{ + "type_name": iface_type, + "model": "virtio", + "target": {"dev": tap_name, "managed": "no"}, + "driver": {"driver_attr": {"queues": str(queues), "name": "vhost"}}, + }] + else: # bridge + iface_list = [{ + "type_name": iface_type, + "model": "virtio", + "source": {"bridge": bridge_name}, + "driver": {"driver_attr": {"queues": str(queues), "name": "vhost"}}, + }]Option B: sandbox eval if you must keep the cfg literal:
- iface_list = eval(params.get('iface_list', '{}')) + _safe_globals = {"__builtins__": {}} + _safe_locals = {"tap_name": tap_name, "bridge_name": bridge_name, "queues": params.get("queues"), "iface_type": params.get("iface_type")} + iface_list = eval(params.get('iface_list', '{}'), _safe_globals, _safe_locals)provider/virtual_network/pktgen_utils.py (2)
223-253: Clean downloaded RPM on host and guest to avoid /tmp clutter.Currently guest cleanup runs, host RPM may remain when session_serial is set.
Apply:
- output_cmd(cmd_download) - cmd_clean = "rm -rf /tmp/%s.rpm" % kernel_ver + output_cmd(cmd_download) + cmd_clean = "rm -rf /tmp/%s.rpm" % kernel_ver if session_serial: output_cmd = session_serial.cmd_output local_path = "/tmp/%s.rpm" % kernel_ver remote_path = "/tmp/" vm.copy_files_to(local_path, remote_path) output_cmd(cmd_install) - output_cmd(cmd_clean) + output_cmd(cmd_clean) # guest or host depending on output_cmd + if session_serial: + # Also remove host copy + process.getoutput(cmd_clean)
25-64: Broadened exception catch; prefer logging.exception for full traceback.Catching Exception and logging error hides traceback; use logging.exception or narrow exceptions.
Apply:
- except Exception as err: - LOG_JOB.error("Error during vDPA binding process: %s", err) + except Exception: + LOG_JOB.exception("Error during vDPA binding process")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py(1 hunks)provider/virtual_network/network_base.py(2 hunks)provider/virtual_network/pktgen_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/virtual_network/network_base.py
🧰 Additional context used
🧬 Code graph analysis (2)
provider/virtual_network/pktgen_utils.py (1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
run(42-151)run_test(58-116)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
provider/virtual_network/network_base.py (4)
create_tap(151-164)prepare_single_vm(465-493)ping_check(97-148)delete_tap(205-211)provider/virtual_network/pktgen_utils.py (4)
run_test(183-221)PktgenRunner(182-265)install_package(223-252)run_tests_for_category(287-416)
🪛 Ruff (0.14.2)
provider/virtual_network/pktgen_utils.py
37-37: Abstract raise to an inner function
(TRY301)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
59-61: Abstract raise to an inner function
(TRY301)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
151-151: Probable insecure usage of temporary file or directory: "/tmp/pktgen_perf/%s.sh"
(S108)
232-232: Function call with shell=True parameter identified, security issue
(S604)
248-248: Probable insecure usage of temporary file or directory: "/tmp/%s.rpm"
(S108)
249-249: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Function call with shell=True parameter identified, security issue
(S604)
476-476: Function call with shell=True parameter identified, security issue
(S604)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
34-34: Function call with shell=True parameter identified, security issue
(S604)
65-65: Function call with shell=True parameter identified, security issue
(S604)
143-143: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (1)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg (1)
1-1: Verify merge status manually for PR 6531.The automated verification of merge conflicts could not be completed in the sandbox environment. Please check the PR status directly on GitHub to confirm whether merge conflicts exist and whether rebasing is necessary before CI can proceed.
| if script in "pktgen_perf": | ||
| dsc_option = "-m" if pkt_cate == "tx" else "-d" | ||
| cmd = "%s -i %s %s %s -t %s -s %s" % ( |
There was a problem hiding this comment.
String membership bug: use equality, not “in”.
if script in "pktgen_perf" is true for many substrings; use ==.
Apply:
- if script in "pktgen_perf":
+ if script == "pktgen_perf":📝 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.
| if script in "pktgen_perf": | |
| dsc_option = "-m" if pkt_cate == "tx" else "-d" | |
| cmd = "%s -i %s %s %s -t %s -s %s" % ( | |
| if script == "pktgen_perf": | |
| dsc_option = "-m" if pkt_cate == "tx" else "-d" | |
| cmd = "%s -i %s %s %s -t %s -s %s" % ( |
🤖 Prompt for AI Agents
In provider/virtual_network/pktgen_utils.py around lines 152 to 154, the
condition uses substring membership ("if script in \"pktgen_perf\"") which
erroneously matches many values; change it to an equality check (if script ==
"pktgen_perf") so the branch only runs when script exactly equals "pktgen_perf".
There was a problem hiding this comment.
^^coderabbit is right @nanli1 or maybe you want to say pktgen_perf in script? In any case, this seems not to be necessary.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This introduces test cases for pktgen burst mode functionality in virtual network environments using virtio-net-pci devices. The tests validate packet generation performance and reliability under different burst configurations. This change is to ensure comprehensive coverage of network performance testing scenarios, particularly for high-throughput use cases that require burst packet transmission capabilities. Reference: XXXX-300599 Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_pktgen_burst_mode_tests
branch
from
47e6339 to
b277d8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (13)
provider/virtual_network/network_base.py (1)
506-507: Harden VM name derivation to avoid IndexError and reduce collision risk.The same issue remains:
re.findall(...)[0]will crash whenguest_versiondoesn't contain "Win" or "RHEL". Additionally, a 3-character suffix increases collision risk.libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg (1)
15-15: Flushing firewall rules without restoring is unsafe.The destructive
iptables -F ; nft flush rulesetcommand is not restored in teardown, leaving the host exposed after tests.libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (6)
23-39: Ensure result file is always closed.The function returns an open file handle. If an exception occurs before the explicit close at line 116, the file handle leaks.
56-56: Critical: prepare_single_vm called with wrong argument order.
iface_listis passed as the third positional argument, which binds todisk_pathinstead ofiface_list. This will break VM preparation.
64-65: Save and restore firewall rules around the test.Flushing firewall rules without saving/restoring leaves the system exposed.
68-72: Version check helper semantics inverted.
is_version_lt_rhel(uname, 7)currently returns True for versions GREATER than 7 (see pktgen_utils.py line 264). This will install packages on the wrong hosts.
93-95: Fix guest kernel retrieval and result formatting.Line 94 is missing a trailing newline for readability. Line 95 uses
session_serial.cmd()which returns status, not output—usecmd_output().strip()instead.
143-143: Useast.literal_evalinstead ofevalfor safer configuration parsing.
eval()can execute arbitrary code. Useast.literal_eval()to safely parse dictionary literals from test parameters.provider/virtual_network/pktgen_utils.py (5)
99-99: Runner should return command output, not status.
run_test()expectsrunner(cmd, ...)andrunner(packets)to return stdout.session_serial.cmdreturns status; usecmd_output.
118-118: Runner should return command output, not status.Same issue: use
session_serial.cmd_outputinstead ofsession_serial.cmd.
152-152: String membership bug: use equality, not "in".
if script in "pktgen_perf"is true for many substrings (e.g., "pktgen", "gen"); use==.
172-177: Deterministically collect packet counters; don't rely on timeout exceptions.Backgrounding with " &" makes
runner(cmd, timeout)return immediately, thenpacket_astays None inrun_test. Make the command blocking and ensurepacket_ais always sampled.
254-265: Fix is_version_lt_rhel semantics to match name/docstring.Currently returns True for versions GREATER than the threshold. Change line 264 to use
<instead of>.
🧹 Nitpick comments (1)
provider/virtual_network/pktgen_utils.py (1)
62-63: Preferlogging.exceptionin exception handlers for better diagnostics.Replace
LOG_JOB.errorwithLOG_JOB.exceptionto automatically include the traceback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py(1 hunks)provider/virtual_network/network_base.py(2 hunks)provider/virtual_network/pktgen_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
provider/virtual_network/pktgen_utils.py (1)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
run(42-151)run_test(58-116)
provider/virtual_network/network_base.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
generate_random_string(104-113)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py (2)
provider/virtual_network/network_base.py (4)
create_tap(151-164)prepare_single_vm(465-493)ping_check(97-148)delete_tap(205-211)provider/virtual_network/pktgen_utils.py (5)
run_test(183-221)PktgenRunner(182-265)is_version_lt_rhel(254-265)install_package(223-252)run_tests_for_category(287-416)
🪛 Ruff (0.14.2)
provider/virtual_network/pktgen_utils.py
37-37: Abstract raise to an inner function
(TRY301)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
59-61: Abstract raise to an inner function
(TRY301)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
151-151: Probable insecure usage of temporary file or directory: "/tmp/pktgen_perf/%s.sh"
(S108)
232-232: Function call with shell=True parameter identified, security issue
(S604)
248-248: Probable insecure usage of temporary file or directory: "/tmp/%s.rpm"
(S108)
249-249: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Function call with shell=True parameter identified, security issue
(S604)
476-476: Function call with shell=True parameter identified, security issue
(S604)
libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
34-34: Function call with shell=True parameter identified, security issue
(S604)
65-65: Function call with shell=True parameter identified, security issue
(S604)
143-143: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (6)
provider/virtual_network/network_base.py (1)
473-473: LGTM: Broader domain enumeration.Using
--allflag correctly includes both active and inactive domains, ensuring more robust VM initialization checks.libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg (2)
17-21: LGTM: Multi-queue ethernet configuration.The interface configuration correctly sets up 8 queues with vhost driver for multi-queue testing.
22-24: LGTM: Linux bridge configuration.The bridge interface configuration correctly sets up 8 queues with vhost driver.
provider/virtual_network/pktgen_utils.py (3)
268-284: LGTM: Result formatting.The
format_resultfunction correctly handles string, int, and float types with appropriate formatting.
287-416: LGTM: Test orchestration.The
run_tests_for_categoryfunction properly orchestrates pktgen tests across multiple dimensions (scripts, categories, sizes, threads, bursts) and writes structured results.
429-476: LGTM: Brewkoji installation with architecture awareness.The
install_brewkojifunction correctly handles multiple architectures and RHEL versions with proper error handling.
nanli1
changed the title
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Hi @nanli1 Please check my comments. There were some things that actually look like bugs, some things that can be improved but also two major areas where I'd recommend:
- Remove the package installation logic and delegate that responsibility to CI
- Redesign classes
PktgenConfigandPktgenRunnerto make code readable and reduce duplication.
Please ping me on slack to address any question you might have.
| network_base.prepare_single_vm(params, vm_name, iface_list) | ||
|
|
There was a problem hiding this comment.
@nanli1 ^^ AFAICT coderabbitai is correct, the third argument in your network_base.py of this commit is disk_path - please fix.
| if disable_iptables_rules_cmd: | ||
| process.system(disable_iptables_rules_cmd, shell=True) | ||
|
|
||
| pktgen_runner = pktgen_utils.PktgenRunner() |
There was a problem hiding this comment.
What will haveppen if utils_net.delete_linux... raised an error? It seems the iptables rules are not recovered and the system will be in bad state. If so, please make sure this won't happen.
| @@ -0,0 +1,476 @@ | |||
| import aexpect | |||
There was a problem hiding this comment.
If this file was created with a code assistant please read https://avocado-framework.readthedocs.io/en/latest/guides/contributor/chapters/ai_policy.html and make sure to tag the tool in your commit message - https://avocado-framework.readthedocs.io/en/latest/guides/contributor/chapters/ai_policy.html
If not, please add your own copyright notice on top like you did in your test script file.
| output_cmd(cmd_install) | ||
| output_cmd(cmd_clean) | ||
|
|
||
| def is_version_lt_rhel(self, uname_str, rhel_version): |
There was a problem hiding this comment.
This function is of common use, I think it shouldn't be hidden below provider/virtual_network but moved for example to avocado-vt virttest.utils_misc. Please note that you can retrieve the distro version info for host through pythons distro module and in this PR I added a new function that achieves this for guest: avocado-framework/avocado-vt#4246
If you could help review 4246 then maybe we can move this logic there to avoid duplication. This can be useful for others, too.
| result_file.write(("%s\n" % line)) | ||
|
|
||
|
|
||
| def get_host_version(uname_str): |
There was a problem hiding this comment.
This naming doesn't seem correct, shouldn't it be get_major_version? But please also check my other comment.
| and hasattr(self.runner, "__name__") | ||
| and self.runner.__name__ == session_serial.cmd.__name__ | ||
| ): | ||
| cmd += " &" |
There was a problem hiding this comment.
Please explain why the __name__ has to be set.
There was a problem hiding this comment.
Why do you only send it to the background if running the command in a VM?
Please note that if the session is closed the command would be terminated, a safer way is to use nohup {cmd} &
| return value % result | ||
|
|
||
|
|
||
| def run_tests_for_category( |
There was a problem hiding this comment.
IMO it's useful to have a class PktgenRunParams to hold these parameters.
| record_line += "%s|" % format_result(record, base, fbase) | ||
|
|
||
| pktgen_config = PktgenConfig() | ||
| pktgen_runner = PktgenRunner() |
There was a problem hiding this comment.
Why isn't it PktgenConfig(runner=PktgenRunner())?
| return cmd | ||
|
|
||
|
|
||
| class PktgenRunner: |
There was a problem hiding this comment.
AFAICT this class is not functional, self is never used, it doesn't have state and seems to be a collection of several static functions, some not related directly to 'running pktgen tests`. IMO you should remove this class and just declare the static functions.
| result_file.write("Category:%s\n" % pkt_cate) | ||
| result_file.write("%s\n" % record_line.rstrip("|")) | ||
|
|
||
| for size in params.get("pkt_size", "").split(): |
There was a problem hiding this comment.
The complexity of these nested if else and for is too high. This needs to be simplified so that a human can review the logic more easily.
There was a problem hiding this comment.
For example, instead of creating here the tuples (size, threads, burst) here directly, you can first create your list of tuples in one function, so that this line here becomes for threads, size, burst in get_threads_size_burst(params).
Also, you already pass certain values to ptkgen_config so you shouldn't need to repeat them here (like script, interface, dsc).
Your class design could be in a way that you can write:
pktgen_config = PktgenConfig(params)
for threads, size, burst in get_threads_size_burst(params):
pktgen_runner = PktgenRunner(pktgen_config, threads, size, burst)
pktgen_runner.run_test()
chloerh
dismissed
their stale review
After checking the new review comments, I think this patch could be better after update
4 participants
XXXX-300599:[virtual network][virtio-net-pci] pktgen burst mode tests
Signed-off-by: nanli [email protected]
Summary by CodeRabbit
New Features
Tests
Refactor