Skip to content

DPDK: hotplug and MANA pmd fixes - #4710

Draft
mcgov (mcgov) wants to merge 28 commits into
mainfrom
mcgov/pr3
Draft

mcgov (mcgov) wants to merge 28 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

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.
Hot plug tests need to know exactly when the kernel adds or removes a
VF, its uverbs node, its ib device and its netdev, but DPDK's own
rte_dev_event_monitor only parses pci, uio and vfio events that carry
PCI_SLOT_NAME, so it drops every vmbus, net and infiniband uevent that
matters on Azure.

Add a small helper that listens on NETLINK_KOBJECT_UEVENT directly and
prints a tagged, single line per event that tests can match on.
MANA exposes several vports behind a single PCI bus address, which the
existing include generation could not express: it emitted one --vdev per
nic and selected devices by PCI slot, so two vports on the same bus were
indistinguishable. It also picked the pmd by inspecting which kernel
module the nic happened to be bound to, which made it impossible to ask
for a specific pmd.

- Add Pmd.MANA for driving the MANA pmd directly.
- generate_testpmd_include() now takes the whole nic list and the pmd to
  use, and returns a list of include arguments. For Pmd.MANA the nics are
  grouped by bus address and emitted as a single
  '--vdev=<bus>,<mac>,<mac>' argument per bus.
- The per-nic include generation selects on the requested pmd instead of
  nic.module_name, and on MANA includes devices by vmbus device uuid
  rather than PCI slot, since the slot is shared between vports.
- generate_testpmd_command() takes the pmd and forwards it, and every
  caller passes the pmd the test asked for.
- do_pmd_driver_setup() handles Pmd.MANA by taking the synthetic
  interfaces down and leaving the VF alone; there is no uio_hv_generic
  rebinding involved.
generate_testpmd_command() took a 'multiple_queues' boolean, picked 4 (or
8 for MANA txonly) queues from it, then halved that in a loop until the
implied core count fit in the node's total thread count. The result was
that a test never knew how many queues it actually ran with, two SKUs
could run the same 'multi queue' case at completely different queue
counts, and the core budget was computed against every thread on the box
rather than the ones testpmd could actually use.

Replace the boolean with an explicit 'queues: int' the caller picks, and
plumb it through generate_send_receive_run_info(),
generate_testpmd_multiple_port_command(), verify_dpdk_build(),
verify_dpdk_send_receive(), verify_dpdk_send_receive_multi_txrx_queue(),
verify_dpdk_mutliple_ports() and the perf suite.

The core budget is now derived from the requested queue count (one
forwarding core per queue per port, plus the service cores) and checked
against the CPU range of NUMA node 0, since that is where the cores
testpmd is pinned to have to come from. A request which does not fit
fails immediately with a message naming the queue count, port count and
what was available, instead of quietly running with fewer queues.

Split the failsafe/netvsc 'multi txrx queue' cases into explicit 1, 2, 4,
8 and 16 queue cases with min_core_count set to what each actually needs,
and pin the mtu and multi-port cases at 4 queues. Also log a greppable
DPDK_CORE_QUEUE_MAP line describing the core to queue assignment of every
generated command, and make --stats-period configurable.

Drive-by: the 'skipping txonly-multi-flow' debug message was logged for
every non-txonly run; only log it when the mode is actually txonly.
Copilot AI lite review requested due to automatic review settings September 9, 2026 00:42
Removing the unstable version of this test.

There is a commit with the new version coming soon;
removing to avoid confusion and noise when we test the new commit.

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 on lines 636 to +644
# core range argument
core_list = f"-l 1-{max_core_index}"
self._log_core_queue_mapping(
nics=nic_to_include,
mode=mode,
queues=queues,
first_forwarding_core=1,
service_cores=service_cores,
)
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 17:14

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

A NUMA CPU-range bug in testpmd core selection and a few newly introduced guideline violations (e.g., magic numbers/typos) 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|verify_dpdk_ovs_netvsc|verify_dpdk_ovs_mana

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • debian debian-12 12 latest
Review details

Suppressed comments (6)

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

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

  • run_ovs_test is missing a return type annotation, and pmd is untyped. Adding pmd: Pmd and -> None keeps signatures consistent with the rest of this module and improves static checking.

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

  • options:n_rxq=2 options:n_txq=2 introduces unexplained magic numbers that control test behavior (queue counts). Per test guidelines, these should be documented (or named as a constant) so future tuning doesn’t require reverse-engineering this command string.
        node.execute(
            (
                f"ovs-vsctl add-port {self.OVS_BRIDGE_NAME} p1 -- "
                f'set Interface p1 type=dpdk options:dpdk-devargs="{device_args}" '
                "options:n_rxq=2 options:n_txq=2"

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

  • Typo in error message: “repoted” → “reported”.
                "OVS repoted that DPDK EAL failed to initialize."

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

  • This fixed sleep uses lisa.util.sleep (which is time.sleep) and adds unconditional delay before triggering hotplug. Prefer a bounded wait on a concrete readiness signal (e.g., a stats line from testpmd, or a condition you can poll) to reduce runtime and flakiness across SKUs.
    # let testpmd run for a bit before triggering hotplug
    sleep(10)

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

  • This fixed sleep uses lisa.util.sleep (which is time.sleep) and adds unconditional delay between VF removal and rescan. Prefer a bounded wait on a concrete condition (e.g., testpmd hotplug log marker or a specific uevent) so the test runs as fast as possible while remaining robust.
    # let it run on synthetic path before restoring the VF
    sleep(10)

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

  • The NUMA-0 CPU range is computed (first, last = ...), but the generated core_list is hardcoded to start at CPU 1. If NUMA-0 CPU IDs don’t start at 0 (or aren’t contiguous from 0), -l 1-... can select CPUs outside NUMA node 0 or miss available CPUs.
        # core range argument
        core_list = f"-l 1-{max_core_index}"
        self._log_core_queue_mapping(
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 19:18

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 and robustness issues identified in the new core/NUMA validation and some documentation/timing problems that should be addressed 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
  • debian debian-12 12 latest

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

Review details

Suppressed comments (3)

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

  • The header comment’s build/run instructions reference azure-hotplug-mon and azure_hotplug_mon.c, but the actual program/file added here is azure_uevent_listener and it’s invoked as azure-uevent-listener in Python. This mismatch will confuse anyone trying to build/run it manually.
 * 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:293

  • These fixed delays use lisa.util.sleep, which is time.sleep. This makes the hotplug test behavior timing-dependent and can add avoidable runtime/flakiness; the code already has concrete signals available (testpmd start banner and uevent observations) that can be waited on with bounded polling.
    # let testpmd run for a bit before triggering hotplug
    sleep(10)

    # remove the VF via sysfs instead of asking azure to disable
    # accelerated networking, it's faster and doesn't touch the platform.
    remove_pci_devices(node, pci_slots)
    # wait for uevent listener to see each VF pci device go away
    listener.wait_for_events(
        [UeventListener.device_criteria(slot, _PCI_REMOVE_TAG) for slot in pci_slots],
        timeout=60,
    )

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

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

  • The NUMA-0 CPU availability check uses min/max to infer a contiguous range, but NUMA CPU IDs aren’t guaranteed contiguous. This can overestimate availability and then generate an invalid -l 1-N core list that includes CPUs outside NUMA-0, undermining the intent to pin within NUMA-0.
        # verify the requested queue/core count actually fits on this node's
        # NUMA-0 core range instead of silently shrinking it: a test either
        # runs with the exact queue count it asked for, or fails immediately
        # with a clear explanation of what didn't fit.
        first, last = self.node.tools[Lscpu].get_cpu_range_in_numa_node(0)
        if last <= first:
            raise AssertionError(
                self._error_message(
                    f"tool.Lscpu bug: cpu range for numa 0 found as {first}-{last}."
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread lisa/microsoft/testsuites/dpdk/dpdksuite.py
Copilot AI review requested due to automatic review settings September 9, 2026 19:28

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

The updated Testpmd core-list generation can pin processes to the wrong CPU IDs on systems where NUMA-0 CPU numbering doesn’t start at 0.

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|verify_dpdk_ovs_netvsc

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
  • debian debian-12 12 latest
Review details

Suppressed comments (2)

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

  • generate_testpmd_command() validates NUMA-0 CPU availability using get_cpu_range_in_numa_node() but then hard-codes the EAL core list to "-l 1-..." (and logs first_forwarding_core=1). If NUMA node 0 CPUs don’t start at 0, this will pin testpmd to the wrong CPU IDs even though the availability check passed.
        core_list = f"-l 1-{max_core_index}"

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

  • The header comment’s build/run instructions reference a different filename and binary name (azure_hotplug_mon.c / azure-hotplug-mon), but this file is azure_uevent_listener.c and the Python wrapper builds/runs it as "azure-uevent-listener". This mismatch makes the instructions misleading and harder to debug when building manually.
 * 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
  • Files reviewed: 9/9 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 34 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_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

Marketplace image: debian debian-12 12-arm64 latest

Result: Succeeded

View full logs in Azure DevOps

Copilot AI review requested due to automatic review settings September 10, 2026 00:36

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

OVS dpdk-extra is currently populated with shell-quoted devargs which may be passed verbatim to DPDK EAL and break device parsing, and the new hotplug flows introduce fixed sleeps that should be replaced with bounded readiness waits.

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|verify_dpdk_ovs_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)

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

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

  • This sleep(30) extends test time and may still be timing-dependent for output collection. Prefer waiting for a deterministic condition (e.g. N stats periods observed) and/or using bounded retries before terminating the processes, so the test duration scales with actual output readiness rather than a fixed delay.

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

  • The header comment’s Build/Run examples use azure-hotplug-mon / azure_hotplug_mon.c, but this file is azure_uevent_listener.c and the Python wrapper compiles/runs it as azure-uevent-listener. Updating the examples will prevent confusion when building/running this helper manually.
 * 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:282

  • This introduces an unconditional sleep(10) before triggering hotplug. Fixed sleeps are discouraged in LISA tests because they add latency and can still be racy on slow/loaded systems; prefer a bounded wait on a concrete signal (e.g. first stats line from testpmd, or another deterministic readiness condition) before proceeding.
    # let testpmd run for a bit before triggering hotplug
    sleep(10)

  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +263 to +267
eal_args_list = dpdk_tool.generate_testpmd_include(nics, 0, pmd)
eal_args = " ".join(eal_args_list)
# the devname args are built for a shell invocation, but OVS hands the
# value straight to the EAL, so the embedded quoting has to go.

@LiliDeng

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 34 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_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

Marketplace image: debian debian-12 12-arm64 latest

Result: Succeeded

View full logs in Azure DevOps

Copilot AI review requested due to automatic review settings September 10, 2026 03:05

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 functional issues in the new OVS dpdk-extra argument construction and in testpmd’s NUMA CPU range handling that can break execution on some systems.

Key Test Cases:
verify_dpdk_testpmd_hotplug_receiver_netvsc_pmd|verify_dpdk_testpmd_hotplug_sender_netvsc_pmd|verify_dpdk_ovs_netvsc|verify_dpdk_send_receive_4_queue_netvsc

Impacted LISA Features:
NetworkInterface

Tested Azure Marketplace Images:

  • canonical ubuntu-24_04-lts server latest
Review details

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/dpdkovs.py:266

  • Major: eal_args is built from DpdkTestpmd.generate_testpmd_include(), which includes embedded double-quotes (e.g. -w "0000:..."). Those quotes are then interpolated into the ovs-vsctl ... other_config:dpdk-extra="..." command, which will break the command-line quoting and can prevent OVS from receiving the intended EAL args. Strip/escape embedded quotes before formatting the ovs-vsctl command.
        eal_args_list = dpdk_tool.generate_testpmd_include(nics, 0, pmd)
        eal_args = " ".join(eal_args_list)
        # the devname args are built for a shell invocation, but OVS hands the
        # value straight to the EAL, so the embedded quoting has to go.

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

  • Major: get_cpu_range_in_numa_node() returns the actual CPU IDs for NUMA node 0, but the generated -l core list is hardcoded to 1-{max_core_index} and the log mapping assumes forwarding cores start at CPU 1. This is inconsistent with the queried first CPU ID and will select the wrong cores on systems where NUMA-0 CPUs don't start at 0.
        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:294

  • Minor: The hotplug path uses fixed sleep() delays (which is time.sleep via lisa.util.sleep) to pace the test (sleep(10) before remove, sleep(10) before rescan). Fixed sleeps tend to make the test slower and can be flaky across SKUs; prefer waiting on concrete signals (e.g., testpmd stats output cadence, or additional uevent criteria) and proceed immediately once the condition is met.
    # let testpmd run for a bit before triggering hotplug
    sleep(10)

    # remove the VF via sysfs instead of asking azure to disable
    # accelerated networking, it's faster and doesn't touch the platform.
    remove_pci_devices(node, pci_slots)
    # wait for uevent listener to see each VF pci device go away
    listener.wait_for_events(
        [UeventListener.device_criteria(slot, _PCI_REMOVE_TAG) for slot in pci_slots],
        timeout=60,
    )

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

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

Copilot AI review requested due to automatic review settings September 14, 2026 14:29

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

Unresolved review findings require remediation before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (11)

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

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

  • On every node other than Ubuntu <20.4 this now forces a v24.11 source build, including the normal SR-IOV nodes used by the failsafe tests. initialize_node_resources then explicitly skips Pmd.FAILSAFE for versions newer than 22.11, so the failsafe hotplug, functional, and performance cases are skipped by default instead of exercising their coverage. Keep the v24.11 override scoped to MANA (and retain the old-Ubuntu override), or provide an intentional path for failsafe.
    lisa/microsoft/testsuites/dpdk/dpdkutil.py:278
  • After listener.start(), remove_pci_devices, either wait_for_events, or rescan_pci_bus can raise; because listener.stop() is reached only on the success path, a timeout or missing uevent leaves the root background listener running. Put the hotplug sequence in a try/finally and stop/collect the listener on every failure.
    lisa/microsoft/testsuites/dpdk/dpdksuite.py:30
  • This hunk removes verify_dpdk_testpmd_multiple_port_receive_netvsc_pmd and its helper without a replacement, eliminating the multi-sender/multi-port integration coverage. Please restore equivalent coverage or explain why this scope reduction is intentional.

lisa/microsoft/testsuites/dpdk/dpdkovs.py:279

  • The new default 2 controls the OVS RX/TX queue counts emitted below, but its rationale is not documented. Add an inline explanation or a named constant so this test behavior is not an unexplained magic number.
        queues: int = 2,

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

  • The PR is marked as a bug fix, but the Related Issue section is empty. Consider linking the related issue for traceability.
        run_testpmd_hotplug(

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

  • [Major] This check counts only the CPUs in NUMA node 0, but the generated EAL core list below is the global range 1-{max_core_index}, and the new test requirements constrain total cores rather than NUMA-0 cores. On a multi-NUMA VM, a valid queue test can therefore be rejected merely because NUMA 0 has fewer than the total requested cores. Validate the CPUs actually selected by the core list, or consistently constrain the list and requirements to NUMA 0.
        first, last = self.node.tools[Lscpu].get_cpu_range_in_numa_node(0)
        if last <= first:

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

  • Consider linking the related issue for traceability.
def remove_pci_devices(node: Node, pci_slots: List[str]) -> None:

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

  • [Major] test_duration is passed to the timeout wrapper above, but this new unconditional kill after sleep(10) terminates both testpmd processes regardless of that setting. The runbook option documented as enabling long-running QoS/SLB runs is therefore silently ignored; use the configured duration or make this early-stop behavior an explicit separate mode.
    sleep(10)
    for kit in [sender, receiver]:
        kit.testpmd.kill_previous_testpmd_command()
    sleep(5)

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

  • [Major] If either process fails to start, the hotplug wait raises, or the sysfs operation fails, execution never reaches the kill/collection loop below. The receiver/sender testpmd processes can therefore be left running on the node, causing subsequent tests to contend for DPDK devices and ports. Wrap the process lifecycle in a finally so every started process is terminated and collected.
    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")

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

  • If clock_gettime or localtime_r fails, the fallback only updates when; ts remains uninitialized and ts.tv_nsec is still read on line 185. Initialize ts (or use a separate initialized millisecond value) before the conditional to avoid undefined output.
	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,

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

  • The copy-paste instructions name azure_hotplug_mon.c and azure-hotplug-mon, but the added file and Python wrapper use azure_uevent_listener.c and azure-uevent-listener. Following the documented build command therefore fails because the source file does not exist; update the commands to the actual names.
 *   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
  • Files reviewed: 9/9 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +283 to +285
# remove the VF via sysfs instead of asking azure to disable
# accelerated networking, it's faster and doesn't touch the platform.
remove_pci_devices(node, pci_slots)
Comment on lines +138 to +140
verify_dpdk_build(
node, log, variables, Pmd.MANA, HugePageSize.HUGE_2MB, result=result
)
@LiliDeng

Copy link
Copy Markdown
Collaborator

AI Test Case Selection

Selected 1 test case(s): smoke_test

Marketplace image: debian debian-12 12-arm64 latest

Result: Succeeded

View full logs in Azure DevOps

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