Skip to content

DPDK: hotplug and MANA pmd fixes - #4710

Open
mcgov (mcgov) wants to merge 31 commits into
mainfrom
mcgov/pr3
Open

mcgov (mcgov) wants to merge 31 commits into
mainfrom
mcgov/pr3

Conversation

@mcgov

Copy link
Copy Markdown
Collaborator

Description

dpdk: hotplug VFs through sysfs and verify with a uevent listener
The VF removal tests previously toggled accelerated networking through
the Azure platform API, which is slow, racy against the SSH connection,
and only verifies that pps changed.

Replace that with a direct sysfs remove/rescan of the VF pci device and
watch the kernel uevent stream while it happens:

  • testpmd_start_process waits for "start packet forwarding" so the
    hotplug is not triggered before the forwarder is actually running.
  • get_vf_pci_slots/remove_pci_devices/rescan_pci_bus perform the
    surprise-removal and re-discovery through /sys/bus/pci.
  • switch_sriov_for_nic(s) drives one uevent listener for the whole set
    of nics, waits for the PCI_REMOVE and PCI_ADD events of every slot,
    and asserts they were observed before reloading the nic info.
  • UeventEntry/UeventListener wrap the azure_uevent_listener helper
    program, compiling it on the node and parsing its output into
    matchable events.
  • run_testpmd_hotplug replaces run_testpmd_concurrent/
    start_testpmd_concurrent for the send and receive hotplug tests.

Related Issue

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Documentation update

Checklist

  • Description is filled in above
  • No credentials, secrets, or internal details are included
  • Peer review requested (if not, add required peer reviewers after raising PR)
  • Tests executed and results posted below

Test Validation

verify_dpdk_testpmd_hotplug_.*

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd|verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_failsafe_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd

Impacted LISA Features:

Tested Azure Marketplace Images:
canonical ubuntu-24_04-lts server latest

Test Results

Image VM Size Result
PASSED / FAILED / SKIPPED

Copilot AI lite review requested due to automatic review settings September 9, 2026 00:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces at least one functional bug in MANA --vdev argument construction and also adds multiple fixed sleeps (time.sleep) in hotplug test control flow, which can make tests flaky and slow.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd|verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_failsafe_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the DPDK test suite’s SR-IOV hotplug coverage by switching VF removal/re-add to a guest-side sysfs remove/rescan flow and validating kernel uevents via a new listener helper, while also refining PMD handling (including MANA) and making queue counts explicit across several test paths.

Changes:

  • Add a C-based kernel uevent listener helper and Python wrappers to capture/parse hotplug-related events.
  • Replace Azure accelerated-networking toggling with sysfs-based VF remove/rescan and uevent-based verification.
  • Refactor testpmd command generation to take an explicit queues count and adjust test cases accordingly.
File summaries
File Description
lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c New helper program to observe and report relevant kernel uevents during hotplug.
lisa/microsoft/testsuites/dpdk/dpdkutil.py Adds sysfs VF remove/rescan helpers, uevent listener wrapper, and new hotplug run flow.
lisa/microsoft/testsuites/dpdk/dpdktestpmd.py Refactors device include generation and makes queue/core mapping explicit in generated commands.
lisa/microsoft/testsuites/dpdk/dpdksuite.py Updates/expands send/receive queue-based test cases and switches hotplug tests to the new flow.
lisa/microsoft/testsuites/dpdk/dpdkperf.py Plumbs explicit queues values into perf test execution.
lisa/microsoft/testsuites/dpdk/common.py Extends Pmd enum with a MANA option.
Review details

Suppressed comments (1)

lisa/microsoft/testsuites/dpdk/dpdkutil.py:359

  • run_testpmd_hotplug relies on fixed sleeps (sleep() is time.sleep() via lisa.util) to control how long testpmd runs and when to process results. This tends to be flaky and slow compared to enforcing runtime with the Timeout tool / bounded waits, and it also risks under/over-running depending on scheduling and hotplug timing.
    # let it run for a bit
    sleep(30)
    # kill testpmd and process the output
    for kit in all_kits:
        kit.testpmd.kill_previous_testpmd_command()
        sleep(1)
        # allow time for SIGINT/SIGKILL shutdown and stats flush
        kit.testpmd.process_testpmd_output(processes[kit].wait_result(timeout=120))
  • Files reviewed: 6/6 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lisa/microsoft/testsuites/dpdk/dpdktestpmd.py
Comment thread lisa/microsoft/testsuites/dpdk/dpdkutil.py
Comment thread lisa/microsoft/testsuites/dpdk/common.py Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are correctness issues in the new MANA vdev argument formatting and in CPU core list generation that can break testpmd startup or device selection.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd|verify_dpdk_send_receive_4_queue_netvsc

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/dpdktestpmd.py:492

  • Major: For MANA vports, the generated --vdev arguments don’t follow the documented format (--vdev="<bus>,mac=<mac>,mac=<mac>"). The current code emits --vdev=<bus>,<mac>,<mac> (missing mac= and quoting), which is likely to be rejected by testpmd/EAL or misparsed.
            bus_to_mac: Dict[str, List[str]] = {}
            for nic in nics:
                bus_to_mac.setdefault(nic.pci_slot, []).append(nic.mac_addr)
            for bus, macs in bus_to_mac.items():
                nic_include_infos += [f"--vdev={bus}," + ",".join(macs)]

lisa/microsoft/testsuites/dpdk/common.py:525

  • Minor: Pmd enum values are otherwise lowercase (e.g. failsafe, netvsc), but MANA is uppercase. Since Pmd subclasses str, this can break any future/string-based parsing (e.g. Pmd("mana")) and is inconsistent with other code paths that use the lowercase string "mana".
    # librte_net_netvsc
    # https://doc.dpdk.org/guides/nics/netvsc.html
    NETVSC = "netvsc"
    # direct use of MANA pmd.
    MANA = "MANA"

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:26

  • Nit: The Build/Run instructions in the file header use the old azure-hotplug-mon binary name and azure_hotplug_mon.c source file, which don’t match the actual filename (azure_uevent_listener.c) and wrapper binary name (azure-uevent-listener).
 * Build:
 *   gcc -O2 -Wall -Wextra -o azure-hotplug-mon azure_hotplug_mon.c
 *
 * Run (needs CAP_NET_ADMIN, i.e. root in practice):
 *   sudo ./azure-hotplug-mon          # relevant events only
 *   sudo ./azure-hotplug-mon -a       # every subsystem
 *   sudo ./azure-hotplug-mon -v       # dump all properties per event
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread lisa/microsoft/testsuites/dpdk/dpdktestpmd.py
Comment thread lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 01:03
@LiliDeng

LiliDeng commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 29 test case(s): verify_dpdk_build_netvsc,verify_dpdk_symmetric_mp,verify_dpdk_build_gb_hugepages_netvsc,verify_dpdk_build_failsafe,verify_dpdk_build_gb_hugepages_failsafe,verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd,verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd,verify_dpdk_testpmd_hotplug_sender_failsafe_pmd,verify_dpdk_testpmd_hotplug_sender_netvsc_pmd,verify_dpdk_send_receive_1_queue_failsafe,verify_dpdk_send_receive_2_queue_failsafe,verify_dpdk_send_receive_4_queue_failsafe,verify_dpdk_send_receive_8_queue_failsafe,verify_dpdk_send_receive_16_queue_failsafe,verify_dpdk_send_receive_4_queue_max_mtu_netvsc,verify_dpdk_send_receive_4_queue_1500_mtu_netvsc,verify_dpdk_send_receive_4_queue_4k_mtu_netvsc,verify_dpdk_send_receive_4_queue_8k_mtu_netvsc,verify_dpdk_send_receive_1_queue_netvsc,verify_dpdk_send_receive_2_queue_netvsc,verify_dpdk_send_receive_4_queue_netvsc,verify_dpdk_send_receive_8_queue_netvsc,verify_dpdk_send_receive_16_queue_netvsc,verify_dpdk_send_receive_failsafe,verify_dpdk_send_receive_gb_hugepages_failsafe,verify_dpdk_send_receive_netvsc,verify_dpdk_send_receive_gb_hugepages_netvsc,verify_dpdk_l3fwd_ntttcp_tcp_hotplug,verify_dpdk_l3fwd_ntttcp_tcp_gb_hugepages

Marketplace image: debian debian-12 12 latest

Result: Canceled

View full logs in Azure DevOps

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

There are a few correctness/clarity issues in newly added code/comments (notably misleading build/run docs and an unnecessary fixed delay) that should be addressed before approval.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd|verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_failsafe_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:367

  • usage() accepts -h (getopt string includes 'h'), but the usage text doesn’t document it. This can confuse users when they discover -h works but isn’t listed.

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:26

  • The header comment’s Build/Run examples use old filenames/binary names (azure-hotplug-mon / azure_hotplug_mon.c). This is misleading given the actual source file is azure_uevent_listener.c and the Python wrapper compiles/runs "azure-uevent-listener".
 * Build:
 *   gcc -O2 -Wall -Wextra -o azure-hotplug-mon azure_hotplug_mon.c
 *
 * Run (needs CAP_NET_ADMIN, i.e. root in practice):
 *   sudo ./azure-hotplug-mon          # relevant events only
 *   sudo ./azure-hotplug-mon -a       # every subsystem
 *   sudo ./azure-hotplug-mon -v       # dump all properties per event

lisa/microsoft/testsuites/dpdk/dpdkutil.py:282

  • switch_sriov_for_nics() uses a fixed sleep(10) delay before hotplug. This makes the test slower and adds unnecessary timing sensitivity; testpmd_start_process() already waits for "start packet forwarding", and listener.start() waits for its banner before hotplug is triggered.
    # let testpmd run for a bit before triggering hotplug
    sleep(10)

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@LiliDeng

LiliDeng commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 1 test case(s): smoke_test

Marketplace image: debian debian-12 12 latest

Result: Succeeded

View full logs in Azure DevOps

Copilot AI review requested due to automatic review settings September 9, 2026 03:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

There are correctness and test-hygiene issues (CPU core-list computation, fixed sleeps) plus an unaddressed integration coverage reduction and PR-description mismatch.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd|verify_dpdk_send_receive_4_queue_failsafe|verify_dpdk_send_receive_4_queue_netvsc

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • redhat rhel 9_5 latest
Review details

Suppressed comments (9)

Previously missed (3) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/dpdk/dpdkutil.py:281

  • These fixed sleeps are not needed for correctness: testpmd_start_process() already blocks until forwarding has started, and uevent synchronization is done via wait_for_events(). Keeping unconditional sleeps here increases test runtime and can add flakiness on slow/fast SKUs.

This issue also appears on line 328 of the same file.
lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:26

  • The build/run instructions at the top of this file refer to a different source/binary name (azure_hotplug_mon.c / azure-hotplug-mon). That will mislead future maintainers and doesn’t match how the Python wrapper compiles/runs this helper (azure_uevent_listener.c → azure-uevent-listener).
    lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:368
  • The help text doesn’t mention the supported -h flag (even though getopt() accepts it), which makes the CLI slightly confusing.

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:186

  • print_event() prints ts.tv_nsec even when clock_gettime() fails, but ts is uninitialized in that case, so the timestamp can contain garbage. Initializing ts avoids undefined behavior and keeps output stable.
{
	struct timespec ts;
	struct tm tm;
	char when[16] = "??:??:??";

	if (clock_gettime(CLOCK_REALTIME, &ts) == 0 &&
	    localtime_r(&ts.tv_sec, &tm) != NULL)
		strftime(when, sizeof(when), "%H:%M:%S", &tm);

	printf("[%s.%03ld] %-18s subsystem=%s", when, ts.tv_nsec / 1000000,
	       tag, ev->subsystem != NULL ? ev->subsystem : "?");

lisa/microsoft/testsuites/dpdk/dpdktestpmd.py:642

  • generate_testpmd_command() verifies the NUMA-0 CPU range (first/last) but then hard-codes the core list as "-l 1-N". If NUMA-0 CPU IDs don’t start at 0 (or if cpu0 isn’t in NUMA-0), the generated core list can target the wrong CPUs. Use the discovered first index to build the core list and to log the mapping.
        core_list = f"-l 1-{max_core_index}"
        self._log_core_queue_mapping(
            nics=nic_to_include,
            mode=mode,
            queues=queues,

lisa/microsoft/testsuites/dpdk/dpdkutil.py:295

  • This fixed sleep is not needed for correctness: after remove_pci_devices() you already wait for the PCI_REMOVE uevents. The unconditional delay increases runtime without adding determinism.
    )

    # let it run on synthetic path before restoring the VF
    sleep(10)

    rescan_pci_bus(node)

lisa/microsoft/testsuites/dpdk/dpdkutil.py:359

  • run_testpmd_hotplug() uses several fixed sleeps to control runtime and shutdown. Per repository guidelines, avoid sleeps in tests; prefer running testpmd under the Timeout tool (start_with_timeout) or other bounded waits so the test duration and shutdown behavior are deterministic without adding arbitrary delays.
# run the send/receive hotplug test.
def run_testpmd_hotplug(
    kit_cmd_pairs: Dict[DpdkTestResources, str],
    sender: DpdkTestResources,
    receiver: Optional[DpdkTestResources] = None,
    hotplug: bool = True,
) -> None:
    processes: Dict[DpdkTestResources, Process] = {}

    collect_from = receiver if receiver else sender
    all_kits = [sender]
    if receiver:
        all_kits += [receiver]
        processes[receiver] = testpmd_start_process(receiver, kit_cmd_pairs[receiver])

    processes[sender] = testpmd_start_process(sender, kit_cmd_pairs[sender])
    if hotplug:
        node = collect_from.node
        # gather the VF pci slot up front, the uevent match criteria are
        # built from it. The slot is stable across a remove/rescan cycle.
        test_nic = node.nics.get_nic_by_subnet("10.0.1.0/24")
        switch_sriov_for_nic(node, test_nic)

    # let it run for a bit
    sleep(30)
    # kill testpmd and process the output
    for kit in all_kits:
        kit.testpmd.kill_previous_testpmd_command()
        sleep(1)
        # allow time for SIGINT/SIGKILL shutdown and stats flush
        kit.testpmd.process_testpmd_output(processes[kit].wait_result(timeout=120))

lisa/microsoft/testsuites/dpdk/dpdksuite.py:459

  • The PR description’s Test Validation block lists hotplug test cases for the failsafe PMD (verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd / verify_dpdk_testpmd_hotplug_sender_failsafe_pmd), but those test methods are no longer present in this suite. Please either restore the failsafe hotplug test coverage or update the PR description/test plan to match the new set of tests.
    @TestCaseMetadata(
        description="""
            test sriov failsafe with netvsc during vf revoke (receive side)
        """,
        priority=2,
        maturity="preview",
        requirement=simple_requirement(
            min_core_count=8,
            min_nic_count=2,
            network_interface=Sriov(),
            unsupported_features=[Gpu, Infiniband],
            min_count=2,
            supported_features=[IsolatedResource],
        ),
    )
    def verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd(
        self,
        environment: Environment,
        log: Logger,
        variables: Dict[str, Any],
    ) -> None:
        self.run_testpmd_hotplug_recv_test(environment, log, variables, pmd=Pmd.NETVSC)

    @TestCaseMetadata(
        description="""
            testpmd with hotplug vf for netvsc pmd (send only version)
        """,
        priority=2,
        maturity="preview",
        requirement=simple_requirement(
            min_core_count=8,
            min_nic_count=2,
            network_interface=Sriov(),
            unsupported_features=[Gpu, Infiniband],
            supported_features=[IsolatedResource],
        ),
    )
    def verify_dpdk_testpmd_hotplug_sender_netvsc_pmd(
        self, node: Node, log: Logger, variables: Dict[str, Any]
    ) -> None:
        self.run_testpmd_hotplug_send_test(node, log, variables, pmd=Pmd.NETVSC)

lisa/microsoft/testsuites/dpdk/dpdksuite.py:1333

  • This PR removes the existing multi-port netvsc receive test case (verify_dpdk_testpmd_multiple_port_receive_netvsc_pmd). If this is intentional, please add justification in the PR description and/or replace it with an equivalent test that still covers the multi-port scenario, since this reduces integration coverage.
    @TestCaseMetadata(
        description=("""
                Run the L3 forwarding test for DPDK.
                This test creates a DPDK port forwarding setup between
                two NICs on the same VM. It forwards packets from a sender on
                subnet_a to a receiver on subnet_b. Without l3fwd,
                packets will not be able to jump the subnets.  This imitates
                a network virtual appliance setup, firewall, or other data plane
                tool for managing network traffic with DPDK.
        """),
        priority=3,
        maturity="preview",
        requirement=simple_requirement(
            supported_os=[Ubuntu],
            min_core_count=8,
            min_count=3,
            min_nic_count=3,
            network_interface=Sriov(),
            unsupported_features=[Gpu, Infiniband],
        ),
    )
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 03:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

There is a confirmed CPU pinning bug in the new testpmd core list generation, and the hotplug runner introduces fixed sleeps that violate repository test guidelines and increase flakiness.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd|verify_dpdk_send_receive_4_queue_netvsc|verify_dpdk_send_receive_4_queue_failsafe

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Review details

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/dpdk/dpdksuite.py:421

  • The test description says “failsafe with netvsc”, but this case is explicitly NETVSC. Also, this PR removes the FAILSAFE hotplug test cases (sender/receiver) even though the PR description’s Test Validation block still lists them; please either restore those tests or update the PR description/validation expectations accordingly.
    @TestCaseMetadata(
        description="""
            test sriov failsafe during vf revoke (receive side)
        """,

lisa/microsoft/testsuites/dpdk/dpdkutil.py:282

  • This introduces an unconditional sleep(10) to “let testpmd run” before triggering hotplug. Per LISA test guidelines, avoid sleep-based delays in tests (they add flakiness and unnecessary runtime); prefer a bounded wait on a concrete condition/output.

This issue also appears on line 358 of the same file.

lisa/microsoft/testsuites/dpdk/dpdktestpmd.py:641

  • core_list is hard-coded to "-l 1-..." even though CPU selection is validated using the actual NUMA-0 CPU ID range (first/last). On systems where NUMA-0 CPUs don’t start at 0/1, this can pin testpmd to CPUs outside the intended NUMA node and/or outside the allowed range.
        # core range argument
        core_list = f"-l 1-{max_core_index}"
        self._log_core_queue_mapping(
            nics=nic_to_include,
            mode=mode,

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:24

  • The header comment’s Build/Run examples refer to "azure-hotplug-mon" and "azure_hotplug_mon.c", but this file is azure_uevent_listener.c and the Python wrapper uses the binary name "azure-uevent-listener". This mismatch makes it harder to build/run/debug the helper independently.
 * Build:
 *   gcc -O2 -Wall -Wextra -o azure-hotplug-mon azure_hotplug_mon.c
 *
 * Run (needs CAP_NET_ADMIN, i.e. root in practice):
 *   sudo ./azure-hotplug-mon          # relevant events only

lisa/microsoft/testsuites/dpdk/dpdkutil.py:294

  • This introduces a second unconditional sleep(10) after VF removal. Per LISA test guidelines, fixed sleeps tend to be flaky and slow; prefer a bounded wait for a specific expected state/event (e.g., uevents for net/infiniband subsystems if required) before proceeding to rescan.
    # let it run on synthetic path before restoring the VF
    sleep(10)

lisa/microsoft/testsuites/dpdk/dpdkutil.py:357

  • This uses sleep(30) to control how long testpmd runs after hotplug. Per LISA test guidelines, avoid fixed sleeps in tests; prefer running the process with an explicit bounded timeout or waiting for a concrete condition to accumulate enough samples.
    # let it run for a bit
    sleep(30)
    # kill testpmd and process the output

lisa/microsoft/testsuites/dpdk/dpdkutil.py:362

  • sleep(1) after kill_previous_testpmd_command is redundant: kill_previous_testpmd_command already waits/retries for graceful termination and stats flush. Keeping an extra sleep adds avoidable runtime and still doesn’t guarantee stats were flushed any better than the existing wait logic.
    for kit in all_kits:
        kit.testpmd.kill_previous_testpmd_command()
        sleep(1)
        # allow time for SIGINT/SIGKILL shutdown and stats flush
        kit.testpmd.process_testpmd_output(processes[kit].wait_result(timeout=120))
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 04:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

There are confirmed reliability/robustness bugs (listener/process cleanup on failure paths) and correctness/documentation issues in the new uevent listener that should be fixed before merging.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd|verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_failsafe_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Review details

Suppressed comments (6)

Previously missed (5) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/dpdk/dpdkutil.py:305

  • UeventListener is started but not guaranteed to be stopped if any of the hotplug steps or wait_for_events() calls raise (timeout/command failure). That can leak a background root process into subsequent test cases and make later hotplug waits race/flap. Wrap the hotplug sequence so listener.stop() runs on error paths too.
    lisa/microsoft/testsuites/dpdk/dpdkutil.py:362
  • run_testpmd_hotplug() starts long-running testpmd processes but doesn’t have a finally/cleanup path. If hotplug/assertions raise, testpmd can be left running on the node and interfere with subsequent tests. Add a try/finally with best-effort kill to ensure processes are always terminated.
    lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:229
  • If SO_PASSCRED can’t be enabled, uev_recv() will later drop every message (it requires SCM_CREDENTIALS). The current behavior prints a warning but then effectively turns the listener into a no-op; it should fail fast (or implement a deliberate fallback).
    lisa/microsoft/testsuites/dpdk/dpdkutil.py:489
  • receiver_includes is accumulated here but never used when building rcv_cmd below (the receiver command is generated from receiver_nics directly). This looks like leftover code; it also risks confusing readers about whether these includes are required. Consider removing both this accumulation and the now-unused receiver_includes variable.
    lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:24
  • The build/run examples still refer to "azure-hotplug-mon" and "azure_hotplug_mon.c", which don’t match the actual file/binary names used elsewhere in this PR (azure_uevent_listener.c / azure-uevent-listener). This is likely to confuse anyone building/running it manually.

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:179

  • print_event() prints ts.tv_nsec even when clock_gettime() fails, but ts is uninitialized in that case. Initialize ts so the timestamp formatting can’t read garbage values.
print_event(const struct uevent *ev, const char *tag, bool vf)
{
	struct timespec ts;
	struct tm tm;
	char when[16] = "??:??:??";
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 04:18
@LiliDeng

LiliDeng commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 29 test case(s): verify_dpdk_build_netvsc,verify_dpdk_symmetric_mp,verify_dpdk_build_gb_hugepages_netvsc,verify_dpdk_build_failsafe,verify_dpdk_build_gb_hugepages_failsafe,verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd,verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd,verify_dpdk_testpmd_hotplug_sender_failsafe_pmd,verify_dpdk_testpmd_hotplug_sender_netvsc_pmd,verify_dpdk_send_receive_1_queue_failsafe,verify_dpdk_send_receive_2_queue_failsafe,verify_dpdk_send_receive_4_queue_failsafe,verify_dpdk_send_receive_8_queue_failsafe,verify_dpdk_send_receive_16_queue_failsafe,verify_dpdk_send_receive_4_queue_max_mtu_netvsc,verify_dpdk_send_receive_4_queue_1500_mtu_netvsc,verify_dpdk_send_receive_4_queue_4k_mtu_netvsc,verify_dpdk_send_receive_4_queue_8k_mtu_netvsc,verify_dpdk_send_receive_1_queue_netvsc,verify_dpdk_send_receive_2_queue_netvsc,verify_dpdk_send_receive_4_queue_netvsc,verify_dpdk_send_receive_8_queue_netvsc,verify_dpdk_send_receive_16_queue_netvsc,verify_dpdk_send_receive_failsafe,verify_dpdk_send_receive_gb_hugepages_failsafe,verify_dpdk_send_receive_netvsc,verify_dpdk_send_receive_gb_hugepages_netvsc,verify_dpdk_l3fwd_ntttcp_tcp_hotplug,verify_dpdk_l3fwd_ntttcp_tcp_gb_hugepages

Marketplace image: debian debian-12 12 latest

Result: Canceled

View full logs in Azure DevOps

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There is a correctness issue in NETVSC include-flag selection (vmbus uuid vs VF PCI selection) that can break NETVSC-mode testpmd runs.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd|verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_failsafe_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd

Impacted LISA Features:
NetworkInterface, Sriov

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/dpdk/dpdkutil.py:282

  • This fixed sleep is redundant with testpmd_start_process() already waiting for "start packet forwarding" before hotplug is triggered, and it also makes the test slower/flakier across SKUs. Prefer event-driven waits (or no wait here).
    lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:22
  • The header comment's build instructions reference a different binary name (azure-hotplug-mon) and source filename (azure_hotplug_mon.c) than the actual file and the Python wrapper (which compiles/runs azure-uevent-listener). This can mislead anyone trying to build it manually.

lisa/microsoft/testsuites/dpdk/uevent_listener/azure_uevent_listener.c:26

  • The header comment's run examples use ./azure-hotplug-mon, but the wrapper runs azure-uevent-listener. Keeping these consistent will avoid confusion when troubleshooting on a node.
 * Run (needs CAP_NET_ADMIN, i.e. root in practice):
 *   sudo ./azure-hotplug-mon          # relevant events only
 *   sudo ./azure-hotplug-mon -a       # every subsystem
 *   sudo ./azure-hotplug-mon -v       # dump all properties per event
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread lisa/microsoft/testsuites/dpdk/dpdktestpmd.py
@LiliDeng

LiliDeng commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 1 test case(s): smoke_test

Marketplace image: debian debian-12 12 latest

Result: Succeeded

View full logs in Azure DevOps

testpmd = test_kit.testpmd
test_nic = node.nics.get_secondary_nic()
testpmd_cmd = testpmd.generate_testpmd_command([test_nic], 0, "txonly")
test_nic = node.nics.get_nic_by_subnet("10.0.1.0/24")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be hardcoded.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I really can't avoid it; we need some way to select the secondary interface without relying on ordering of node.nics and without relying on interface names (which can change). The ARM template uses this convention for creating subnets of secondary nics. If there isn't a secondary subnet I can't really test anything, so this will raise an appropriate AssertionError. I'll comment the need for the convention in the ARM template?

Comment thread lisa/microsoft/testsuites/dpdk/dpdksuite.py Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 13:09
OVS test hadn't been updated since 2022 so would always fail on MANA.

Add MANA support to the OVS test and add a scheme for parsing the
documentation to resolve which ovs verison to use for a given dpdk
version.
l3fwd has had a silent bug where apt undid the routing changes
because systemd-networkd was marked as 'needs restart' after
installing something needed for ntttcp.

This wiped the routing table changes. Now ntttcp is
forced to install before anything else it touched on the node.

This also makes sure to set entries for the arp table
so that l3fwd and ntttcp aren't relying on implicit behavior.
several ip tool operaitons were not marked force run
when they should be. mark force run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved critical Python 3.8 compatibility and multiple moderate correctness, cleanup, and test-coverage issues remain.

Review effort: Lite
Findings: 5 High severity · 3 Medium severity · 1 Low severity

Open (9)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Clean up persistent Azure networking changes before environment reuse

lisa/​microsoft/​testsuites/​dpdk/​dpdkutil.py:1399

The old mark_dirty() calls were removed, but this test still changes persistent Azure state: __enable_ip_forwarding enables forwarding on the NICs and the later create_route_table calls create/attach VNet route tables. DpdkCleanupManager only reboots the guests and does not undo either control-plane change, so a reused environment can affect subsequent tests. Restore the dirty-environment behavior or register cleanup that explicitly reverses all Azure changes before allowing reuse.

    # enable ip forwarding on secondary and tertiary nics
    run_in_parallel(
        [partial(__enable_ip_forwarding, node) for node in environment.nodes.list()]
    )
Medium severity Check lcore capacity against the command's actual CPU set

lisa/​microsoft/​testsuites/​dpdk/​dpdktestpmd.py:631

This changes the capacity check from the total logical-thread count used previously to only the min/max range of NUMA node 0. The test requirements specify total min_core_count and the generated -l 1-N list is not restricted to NUMA 0, so multi-NUMA VMs with enough total cores can now fail this assertion even though the requested lcores fit. Base the check on the same CPU set used by the command, or make the requirement explicitly NUMA-0-specific.

)

test_nics: Dict[str, Dict[Node, NicInfo]] = {}
expected_subnets: Optional[set[str]] = None
Copilot AI lite review requested due to automatic review settings September 30, 2026 00:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment on lines +212 to +218
if isinstance(node.os, Ubuntu) and node.os.information.version < "20.4.0":
variables["dpdk_source"] = variables.get("dpdk_source", DPDK_STABLE_GIT_REPO)
variables["dpdk_branch"] = variables.get("dpdk_branch", "v20.11")
else:
# otherwise just use a new build of DPDK unless another is specified
variables["dpdk_source"] = variables.get("dpdk_source", DPDK_STABLE_GIT_REPO)
variables["dpdk_branch"] = variables.get("dpdk_branch", "v24.11")
Copilot AI lite review requested due to automatic review settings September 30, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LiliDeng

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 47 test case(s): verify_dpdk_build_netvsc,verify_dpdk_build_mana,verify_dpdk_symmetric_mp,verify_dpdk_build_gb_hugepages_netvsc,verify_dpdk_build_failsafe,verify_dpdk_build_gb_hugepages_failsafe,verify_dpdk_ovs_netvsc,verify_dpdk_nff_go,verify_dpdk_multiprocess,verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd,verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd,verify_dpdk_testpmd_hotplug_sender_failsafe_pmd,verify_dpdk_testpmd_hotplug_sender_netvsc_pmd,verify_dpdk_vpp,verify_dpdk_ring_ping,verify_dpdk_send_receive_1_queue_failsafe,verify_dpdk_send_receive_2_queue_failsafe,verify_dpdk_send_receive_4_queue_failsafe,verify_dpdk_send_receive_8_queue_failsafe,verify_dpdk_send_receive_16_queue_failsafe,verify_dpdk_send_receive_4_queue_max_mtu_netvsc,verify_dpdk_send_receive_4_queue_1500_mtu_netvsc,verify_dpdk_send_receive_4_queue_4k_mtu_netvsc,verify_dpdk_send_receive_4_queue_8k_mtu_netvsc,verify_dpdk_send_receive_1_queue_netvsc,verify_dpdk_send_receive_2_queue_netvsc,verify_dpdk_send_receive_4_queue_netvsc,verify_dpdk_send_receive_8_queue_netvsc,verify_dpdk_send_receive_16_queue_netvsc,verify_dpdk_send_receive_failsafe,verify_dpdk_send_receive_gb_hugepages_failsafe,verify_dpdk_send_receive_netvsc,verify_dpdk_send_receive_gb_hugepages_netvsc,verify_dpdk_l3fwd_ntttcp_tcp_hotplug,verify_dpdk_l3fwd_ntttcp_tcp_gb_hugepages,verify_uio_binding,verify_ib_naming,verify_hpc_over_sriov,verify_hpc_over_nd,verify_ping_pong,verify_intel_mpi,verify_open_mpi,verify_ibm_mpi,verify_mvapich_mpi,verify_network_interface_reload_via_ip_link,verify_xfrm_interface,verify_xdp_with_different_mtu

Marketplace image: debian debian-12 12-arm64 latest

Result: Succeeded

View full logs in Azure DevOps

@mcgov
mcgov (mcgov) marked this pull request as ready for review September 30, 2026 19:25
@LiliDeng

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 47 test case(s): verify_dpdk_build_netvsc,verify_dpdk_build_mana,verify_dpdk_symmetric_mp,verify_dpdk_build_gb_hugepages_netvsc,verify_dpdk_build_failsafe,verify_dpdk_build_gb_hugepages_failsafe,verify_dpdk_ovs_netvsc,verify_dpdk_nff_go,verify_dpdk_multiprocess,verify_dpdk_testpmd_hotplug_receiver_failsafe_pmd,verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd,verify_dpdk_testpmd_hotplug_sender_failsafe_pmd,verify_dpdk_testpmd_hotplug_sender_netvsc_pmd,verify_dpdk_vpp,verify_dpdk_ring_ping,verify_dpdk_send_receive_1_queue_failsafe,verify_dpdk_send_receive_2_queue_failsafe,verify_dpdk_send_receive_4_queue_failsafe,verify_dpdk_send_receive_8_queue_failsafe,verify_dpdk_send_receive_16_queue_failsafe,verify_dpdk_send_receive_4_queue_max_mtu_netvsc,verify_dpdk_send_receive_4_queue_1500_mtu_netvsc,verify_dpdk_send_receive_4_queue_4k_mtu_netvsc,verify_dpdk_send_receive_4_queue_8k_mtu_netvsc,verify_dpdk_send_receive_1_queue_netvsc,verify_dpdk_send_receive_2_queue_netvsc,verify_dpdk_send_receive_4_queue_netvsc,verify_dpdk_send_receive_8_queue_netvsc,verify_dpdk_send_receive_16_queue_netvsc,verify_dpdk_send_receive_failsafe,verify_dpdk_send_receive_gb_hugepages_failsafe,verify_dpdk_send_receive_netvsc,verify_dpdk_send_receive_gb_hugepages_netvsc,verify_dpdk_l3fwd_ntttcp_tcp_hotplug,verify_dpdk_l3fwd_ntttcp_tcp_gb_hugepages,verify_uio_binding,verify_ib_naming,verify_hpc_over_sriov,verify_hpc_over_nd,verify_ping_pong,verify_intel_mpi,verify_open_mpi,verify_ibm_mpi,verify_mvapich_mpi,verify_network_interface_reload_via_ip_link,verify_xfrm_interface,verify_xdp_with_different_mtu

Marketplace image: debian debian-12 12-arm64 latest

Result: Succeeded

View full logs in Azure DevOps

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants