|SUCCESS| [v1] dts: report dut/NIC info during DTS run

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164819

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-02

Reviewing patch: dts: report dut/NIC info during DTS run

Git SHA: 20260602163647.101815-1-knimoji at iol.unh.edu
Author: Koushik Bhargav Nimoji <knimoji at iol.unh.edu>
Subject: [PATCH v1] dts: report dut/NIC info during DTS run

This patch gathers NIC info during a DTS run and writes it to an output
json file. This allows the json file to be used when reporting results
on the DTS results dashboard.

> diff --git a/dts/framework/test_run.py b/dts/framework/test_run.py
> index 94dc6023a7..eebea280d7 100644
> --- a/dts/framework/test_run.py
> +++ b/dts/framework/test_run.py
> @@ -98,6 +98,7 @@
>          "InternalError" -> "exit":ew
>  """
>  
> +import json
>  import random
>  from collections import deque
>  from collections.abc import Iterable

[ ... ]

> @@ -370,6 +371,15 @@ def next(self) -> State | None:
>          test_run.supported_capabilities = get_supported_capabilities(
>              test_run.ctx.sut_node, test_run.ctx.topology, test_run.required_capabilities
>          )
> +
> +        used_nic_info: list[dict[str, object]] = (
> +            self.test_run.ctx.sut_node.main_session.get_nic_info()
> +        )
> +        with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
> +            json.dump(used_nic_info, file, indent=3)
> +            file.close()

Does this code leak the file handle on json.dump() failure? The 
explicit file.close() after json.dump() seems unnecessary since the 
with statement handles closure, but if json.dump() raises an exception, 
does the with context manager still guarantee closure?

> +        self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
> +
>          return TestRunExecution(test_run, self.result)
>  
>      def on_error(self, ex: BaseException) -> State | None:

[ ... ]

> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> index ee943462c2..2269fe60ae 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py

[ ... ]

> +    def get_nic_info(self) -> list[dict[str, object]]:
> +        """Overrides :meth`~.os_session.OSSession.get_nic_info`.
> +
> +        Raises:
> +            ConfigurationError: If the NIC info could not be found.
> +        """
> +        port_data = {
> +            port.get("businfo"): port for port in self._lshw_net_info if port.get("businfo")
> +        }
> +
> +        all_nic_info = []
> +        for port in self._config.ports:
> +            pci_addr = port.pci
> +
> +            command_result = self.send_command(
> +                f"sudo lshw -c network -businfo | grep '{pci_addr}' | cut -d'@' -f1"
> +            )
> +            bus_type = (
> +                command_result.stdout
> +                if command_result.return_code == 0 and command_result.stdout
> +                else None
> +            )
> +            if bus_type is None:
> +                raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
> +            bus_info = f"{bus_type}@{pci_addr}"
> +
> +            nic_port: LshwOutput | None = port_data.get(bus_info)
> +            if nic_port is None:
> +                raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")
> +
> +            config: LshwConfigurationOutput | None = nic_port.get("configuration")
> +            if config is None:
> +                raise ConfigurationError(
> +                    f"Configuration info for port {pci_addr} could not be found on the node."
> +                )
> +
> +            command_result = self.send_command(
> +                f"sudo lspci -vv -s {pci_addr} | grep 'Engineering changes'"
> +            )
> +            hardware_version = (
> +                command_result.stdout.split(":")[-1].strip()
> +                if command_result.return_code == 0 and command_result.stdout
> +                else None
> +            )
> +            if hardware_version is None:
> +                self._logger.error(f"Unable to get hardware version for NIC: {pci_addr}")

Does this code continue execution after logging the error? Should this
raise ConfigurationError like the earlier checks, or is continuing with
"Unknown" the intended behavior? The inconsistency between this error
handling and the earlier ConfigurationError raises is unclear.

> +
> +            nic_name = nic_port.get("logicalname")
> +            nic_speed = None
> +            if nic_name is not None:
> +                command_result = self.send_command(
> +                    f"ethtool {nic_name} | grep 'Speed:' | awk '{{print $2}}'"
> +                )
> +                nic_speed = (
> +                    command_result.stdout
> +                    if command_result.return_code == 0 and command_result.stdout
> +                    else None
> +                )
> +            if nic_speed is None:
> +                self._logger.error(f"Unable to get speed for NIC: {pci_addr}")

Same question here - does this code continue after the error log with
nic_speed as None, resulting in "Unknown" in the output?

> +
> +            dut_json = {
> +                "make": nic_port.get("vendor"),
> +                "model": nic_port.get("product"),
> +                "hardware version": hardware_version or "Unknown",
> +                "firmware version": config.get("firmware"),
> +                "deviceBusType": bus_type,
> +                "deviceId": nic_port.get("serial"),
> +                "pmd": config.get("driver"),
> +                "speed": nic_speed or "Unknown",
> +            }

Can any of these nic_port.get() or config.get() calls return None,
resulting in None values in the JSON output? Should these be checked or
defaulted to "Unknown" like hardware_version and nic_speed?

> +            all_nic_info.append(dut_json)
> +
> +        return all_nic_info