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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions libvirt/tests/cfg/virtual_network/qemu/pktgen_burst_mode_test.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
- virtual_network.qemu_test.pktgen_burst_mode_test:
type = pktgen_burst_mode_test
start_vm = no
category = "tx"
pktgen_script = "pktgen_sample03_burst_single_flow"
pktgen_threads = 1
pkt_size = 64
burst = 3
pktgen_test_timeout = 30
record_list = "pkt_size run_threads burst mpps"
guest_ver_cmd = "uname -r"
kvm_ver_chk_cmd = "rpm -qa qemu-kvm-rhev && rpm -qa qemu-kvm"
queues = 8
test_vm = no
disable_iptables_rules_cmd = "iptables -F ; nft flush ruleset"
variants:
- ethernet_multi_queues:
iface_type = "ethernet"
pre_tap = "yes"
iface_list = [{'type_name': '${iface_type}', 'model': 'virtio', 'target': {'dev': tap_name, 'managed': 'no'},'driver': {'driver_attr': {'queues': '${queues}', 'name': 'vhost'}}}]
tap_flag = "multi_queue"
- linux_bridge:
iface_type = "bridge"
iface_list = [{'type_name': '${iface_type}', 'model': 'virtio', 'source': {'bridge': bridge_name}, 'driver': {'driver_attr': {'queues': '${queues}', 'name': 'vhost'}}}]
151 changes: 151 additions & 0 deletions libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#
# Copyright Redhat
#
# SPDX-License-Identifier: GPL-2.0
# Author: Nannan Li<[email protected]>
#
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

import os

from avocado.utils import process

from virttest import utils_misc
from virttest import utils_net
from virttest import virsh
from virttest.libvirt_xml import vm_xml

from provider.virtual_network import pktgen_utils
from provider.virtual_network import network_base


def write_in_result_file(test, params):
"""
Write some qemu, kvm version info and write them into pktgen test result.
:params, params
:return result_file, pktgen test result file.
"""
kvm_ver_chk_cmd = params.get("kvm_ver_chk_cmd")

result_path = utils_misc.get_path(test.resultsdir, "pktgen_perf.RHS")
result_file = open(result_path, "w")
kvm_ver = process.system_output(kvm_ver_chk_cmd, shell=True).decode()
host_ver = os.uname()[2]
result_file.write("### kvm-userspace-ver : %s\n" % kvm_ver)
result_file.write("### kvm_version : %s\n" % host_ver)

return result_file


def run(test, params, env):
"""
Pktgen tests with burst mode.
"""
def setup_test():
"""
Prepare the test environment with required guest type and interface.
"""
utils_net.create_linux_bridge_tmux(bridge_name, host_iface)
if params.get_boolean("pre_tap"):
network_base.create_tap(tap_name, bridge_name, 'root', flag=tap_flag)

test.log.info("TEST_SETUP: guest and interface.")
network_base.prepare_single_vm(params, vm_name, iface_list)

Comment on lines +56 to +57
Copy link

@coderabbitai coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nanli1 ^^ AFAICT coderabbitai is correct, the third argument in your network_base.py of this commit is disk_path - please fix.

Copy link

Choose a reason for hiding this comment

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

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!

def run_test():
"""
Run pktgen in burst mode for vm.
"""
result_file = write_in_result_file(test, params)

if disable_iptables_rules_cmd:
process.system(disable_iptables_rules_cmd, shell=True)

pktgen_runner = pktgen_utils.PktgenRunner()
Comment on lines +64 to +67
Copy link

@coderabbitai coderabbitai bot Sep 21, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

🧩 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:

  1. Each cleanup operation is independent and won't block others from executing
  2. Firewall rules are restored even if network cleanup fails
  3. All errors are logged but don't prevent other cleanup steps
  4. The system is left in a consistent state regardless of which cleanup step fails

Copy link
Contributor

@smitterl smitterl Nov 3, 2025

Choose a reason for hiding this comment

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

@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}")

Copy link

Choose a reason for hiding this comment

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

@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:

  1. Clear coupling: Cleanup is registered right after the action that requires it
  2. Automatic ordering: Reversing the list gives us LIFO cleanup (undo in reverse order)
  3. Resilient: Each cleanup action is isolated - one failure doesn't prevent others
  4. 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.

if pktgen_runner.is_version_lt_rhel(process.getoutput("uname -r"), 7):
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of process.getoutput it's better to use python's distro module.

if host_ver.count("64k"):
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is an odd un-pythonic way to check, why not use simply "64" in host_ver?

pktgen_runner.install_package(host_ver, pagesize="64k")
else:
pktgen_runner.install_package(host_ver)

Comment on lines +68 to +73
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

test.log.debug("TEST_STEP: Test with guest: %s.", vm_name)
env.create_vm(vm_type='libvirt', target="", name=vm_name, params=params, bindir=test.bindir)
vm = env.get_vm(vm_name)
vm.start()
test.log.debug("Guest xml:%s", vm_xml.VMXML.new_from_dumpxml(vm_name))

test.log.debug("TEST_STEP: Test with guest and host connectivity")
session_serial = vm.wait_for_serial_login(restart_network=True)
mac = vm.get_mac_address()
params.update({
'vm_ping_host': 'pass'}
)
ips = {
'vm_ip': utils_net.get_guest_ip_addr(session_serial, mac),
'host_ip': host_ip
}
network_base.ping_check(params, ips, session_serial, force_ipv4=True)

test.log.debug("TEST_STEP: Install package and Run pktgen in burst mode.")
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)

pktgen_utils.run_tests_for_category(
params, result_file, test_vm, vm, session_serial)

test.log.debug("TEST_STEP: Check the guest dmesg without error messages.")
vm.verify_dmesg()
vm.verify_kernel_crash()

virsh.destroy(vm_name)
virsh.undefine(vm_name, options='--nvram')

result_file.close()

def teardown_test():
"""
Clean data.
"""
test.log.debug("TEST_TEARDOWN: Recover the env.")
network_base.delete_tap(tap_name)
utils_net.delete_linux_bridge_tmux(bridge_name, host_iface)

backup_vmxml.sync()

vm_name = params.get("main_vm")
vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
backup_vmxml = vmxml.copy()

disable_iptables_rules_cmd = params.get("disable_iptables_rules_cmd")
guest_ver_cmd = params["guest_ver_cmd"]
test_vm = params.get_boolean("test_vm")
host_ver = os.uname()[2]

rand_id = utils_misc.generate_random_string(3)
bridge_name = 'br_' + rand_id
tap_name = 'tap_' + rand_id
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', '{}'))
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

tap_flag = params.get("tap_flag")

try:
setup_test()
run_test()

finally:
teardown_test()
5 changes: 3 additions & 2 deletions provider/virtual_network/network_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ def prepare_single_vm(params, vm_name, disk_path='', iface_list=[]):
:params disk_path: path of the disk image.
:params iface_list: list of interface for vm.
"""
if vm_name not in virsh.dom_list().stdout_text:
if vm_name not in virsh.dom_list("--all").stdout_text:
params.update(
{"vm_name_bootc": vm_name,
"vm_disk_image_path": disk_path,
Expand Down Expand Up @@ -503,7 +503,8 @@ def prepare_vms_with_iface(params, vm_image_path=[], iface_list=[]):
vm_list = []
for image_path in vm_image_path:
guest_version = os.path.basename(image_path)
vm_name = "".join([re.findall("Win|RHEL", guest_version, re.IGNORECASE)[0], "_vm"])
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)
Comment on lines +506 to 508
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

❓ 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=py

Length of output: 0


Harden VM name derivation to avoid IndexError and reduce collision risk

  • Use re.search with fallback to the guest_version’s filename base instead of re.findall(...)[0]
  • Increase the random suffix to 5 characters and include a clear _ separator
  • Verified no occurrences of literal Win_vm or RHEL_vm in 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.

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


prepare_single_vm(params, vm_name, image_path, iface_list)
Expand Down
Loading