Re: [PATCH v8 1/1] dts: add smoke tests

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

You forgot to add me to the patch :-)

A few comments below.

On Mon, Jul 17, 2023 at 9:37 PM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Adds a new test suite for running smoke tests that verify general
> configuration aspects of the system under test. If any of these tests
> fail, the DTS execution terminates as part of a "fail-fast" model.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---

<snip>

> diff --git a/dts/framework/remote_session/remote/interactive_shell.py b/dts/framework/remote_session/remote/interactive_shell.py
> new file mode 100644
> index 0000000000..4d9c7638a5
> --- /dev/null
> +++ b/dts/framework/remote_session/remote/interactive_shell.py
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +from pathlib import PurePath
> +from typing import Callable
> +
> +from paramiko import Channel, SSHClient, channel  # type: ignore
> +
> +from framework.logger import DTSLOG
> +from framework.settings import SETTINGS
> +
> +
> +class InteractiveShell:
> +
> +    _interactive_session: SSHClient
> +    _stdin: channel.ChannelStdinFile
> +    _stdout: channel.ChannelFile
> +    _ssh_channel: Channel
> +    _logger: DTSLOG
> +    _timeout: float
> +    _startup_command: str
> +    _app_args: str
> +    _default_prompt: str = ""
> +    _privileged: bool
> +    _get_privileged_command: Callable[[str], str]
> +    # Allows for app specific extra characters to be appended to commands
> +    _command_extra_chars: str = ""
> +    path: PurePath
> +    dpdk_app: bool = False
> +

We should document this class - let's use the google format:
https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings
You can use this as an example:
http://patches.dpdk.org/project/dpdk/patch/20230511091408.236638-5-juraj.linkes@pantheon.tech/
A note on class attributes: we should document the public ones as well
as those which should be considered to be overridden in derived
classes.

> +    def __init__(
> +        self,
> +        interactive_session: SSHClient,
> +        logger: DTSLOG,
> +        startup_command: str,
> +        privileged: bool,
> +        _get_privileged_command: Callable[[str], str],
> +        app_args: str = "",
> +        timeout: float = SETTINGS.timeout,
> +    ) -> None:

We can simplify the constructor signature a bit. We can combine
privileged and _get_privileged_command (which shouldn't start with the
underscore - only private class attributes should have that) in that
get_privileged_command could be None (which would be the same as
privileged=False and not None the same as privileged=True).

We don't need startup_command as we're just passing self.path to it,
which we already have access to.
There is an alternative approach to the path manipulation logic - we
don't modify shell_cls.path at all and pass the resulting path (the
result of all the operations we do in Node objects) to the constructor
(in this case we'd retain the startup_command parameter, although it
should be then renamed to path_app or something similar). This doesn't
move the logic entirely out of InteractiveShell though, as we still
need to access the class attribute. For that reason, I don't see much
of a point in pursuing this alternative - we'd basically be just
passing one extra unneeded path argument through all the calls.

> +        self._interactive_session = interactive_session
> +        self._ssh_channel = self._interactive_session.invoke_shell()
> +        self._stdin = self._ssh_channel.makefile_stdin("w")
> +        self._stdout = self._ssh_channel.makefile("r")
> +        self._ssh_channel.settimeout(timeout)
> +        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
> +        self._logger = logger
> +        self._timeout = timeout
> +        self._startup_command = startup_command
> +        self._app_args = app_args
> +        self._get_privileged_command = _get_privileged_command  # type: ignore

We can actually make this work with a static method:
1. Don't store the method in this object. We'd need to pass it to
_start_application, but this should be ok as we're only using the
method when starting the application - once it's running, we'll never
need it again.
2. When we stored the method, we were passing the self parameter when
calling self._get_privileged_command, which is why it couldn't be
static. When calling without self, static works.
        Note: the staticmethod decorator must be above the
abstractmethod decorator, otherwise the interpreter throws an error.
3. The type can be either Callable[[str], str] or MethodType (from the
types module). I'd go with callable here as it's more precise.

> +        self._privileged = privileged
> +        self._start_application()
> +
> +    def _start_application(self) -> None:
> +        """Starts a new interactive application based on _startup_command.
> +
> +        This method is often overridden by subclasses as their process for
> +        starting may look different.
> +        """
> +        start_command = f"{self._startup_command} {self._app_args}"
> +        if self._privileged:
> +            start_command = self._get_privileged_command(start_command)  # type: ignore
> +        self.send_command(start_command)
> +
> +    def send_command(self, command: str, prompt: str | None = None) -> str:
> +        """Send a command and get all output before the expected ending string.
> +
> +        Lines that expect input are not included in the stdout buffer so they cannot be

My IDE spell checker says there should be a comma before so.

> +        used for expect. For example, if you were prompted to log into something
> +        with a username and password, you cannot expect "username:" because it won't
> +        yet be in the stdout buffer. A work around for this could be consuming an

Workaround without the space.

> +        extra newline character to force the current prompt into the stdout buffer.
> +
> +        Returns:
> +            All output in the buffer before expected string
> +        """
> +        self._logger.info(f"Sending command {command.strip()}...")
> +        if prompt is None:
> +            prompt = self._default_prompt
> +        self._stdin.write(f"{command}{self._command_extra_chars}\n")
> +        self._stdin.flush()
> +        out: str = ""
> +        for line in self._stdout:
> +            out += line
> +            if prompt in line and not line.rstrip().endswith(
> +                command.rstrip()
> +            ):  # ignore line that sent command
> +                break
> +        self._logger.debug(f"Got output: {out}")
> +        return out
> +
> +    def close(self) -> None:
> +        self._stdin.close()
> +        self._ssh_channel.close()
> +
> +    def __del__(self) -> None:
> +        self.close()

<snip>

> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
> index 743919820c..724e7328eb 100644
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
<snip>
> @@ -213,6 +218,12 @@ def __init__(self, build_target: BuildTargetConfiguration):
>          self.os = build_target.os
>          self.cpu = build_target.cpu
>          self.compiler = build_target.compiler
> +        self.compiler_version = None
> +        self.dpdk_version = None
> +
> +    def add_build_target_versions(self, versions: BuildTargetInfo) -> None:

Just noticed the name - rename to add_build_target_info to align with
everything else.

> +        self.compiler_version = versions.compiler_version
> +        self.dpdk_version = versions.dpdk_version
>
>      def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
>          test_suite_result = TestSuiteResult(test_suite_name)

> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index d48fafe65d..39237a95d6 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -7,7 +7,7 @@
>  A node is a generic host that DTS connects to and manages.
>  """
>
> -from typing import Any, Callable
> +from typing import Any, Callable, Type
>
>  from framework.config import (
>      BuildTargetConfiguration,
> @@ -15,7 +15,7 @@
>      NodeConfiguration,
>  )
>  from framework.logger import DTSLOG, getLogger
> -from framework.remote_session import OSSession, create_session
> +from framework.remote_session import InteractiveShellType, OSSession, create_session
>  from framework.settings import SETTINGS
>
>  from .hw import (
> @@ -23,6 +23,7 @@
>      LogicalCoreCount,
>      LogicalCoreList,
>      LogicalCoreListFilter,
> +    VirtualDevice,
>      lcore_filter,
>  )
>
> @@ -40,6 +41,8 @@ class Node(object):
>      lcores: list[LogicalCore]
>      _logger: DTSLOG
>      _other_sessions: list[OSSession]
> +    _execution_config: ExecutionConfiguration
> +    _virtual_devices: list[VirtualDevice]
>
>      def __init__(self, node_config: NodeConfiguration):
>          self.config = node_config
> @@ -54,6 +57,7 @@ def __init__(self, node_config: NodeConfiguration):
>          ).filter()
>
>          self._other_sessions = []
> +        self._virtual_devices = []

This is not a private attribute, so let's drop the starting underscore.

>
>          self._logger.info(f"Created node: {self.name}")
>
> @@ -64,6 +68,9 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
>          """
>          self._setup_hugepages()
>          self._set_up_execution(execution_config)
> +        self._execution_config = execution_config
> +        for vdev in execution_config.vdevs:
> +            self._virtual_devices.append(VirtualDevice(vdev))
>
>      def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
>          """
> @@ -77,6 +84,7 @@ def tear_down_execution(self) -> None:
>          this node is part of concludes.
>          """
>          self._tear_down_execution()
> +        self._virtual_devices = []

I'd prefer this to be above self._tear_down_execution(). If there's a
failure in self._tear_down_execution(), the devices won't be removed
so removing them before the call is safer.

>
>      def _tear_down_execution(self) -> None:
>          """

<snip>

> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> new file mode 100644
> index 0000000000..83712e75c6
> --- /dev/null
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +import re
> +
> +from framework.config import PortConfig
> +from framework.remote_session import TestPmdShell
> +from framework.settings import SETTINGS
> +from framework.test_suite import TestSuite
> +from framework.utils import REGEX_FOR_PCI_ADDRESS
> +
> +
> +class SmokeTests(TestSuite):
> +    is_blocking = True
> +    # dicts in this list are expected to have two keys:
> +    # "pci_address" and "current_driver"
> +    nics_in_node: list[PortConfig] = []
> +
> +    def set_up_suite(self) -> None:
> +        """
> +        Setup:
> +            Set the build directory path and generate a list of NICs in the SUT node.
> +        """
> +        self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir
> +        self.nics_in_node = self.sut_node.config.ports
> +
> +    def test_unit_tests(self) -> None:
> +        """
> +        Test:
> +            Run the fast-test unit-test suite through meson.
> +        """
> +        self.sut_node.main_session.send_command(
> +            f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests -t 60",
> +            480,
> +            verify=True,
> +            privileged=True,
> +        )
> +
> +    def test_driver_tests(self) -> None:
> +        """
> +        Test:
> +            Run the driver-test unit-test suite through meson.
> +        """
> +        vdev_args = ""
> +        for dev in self.sut_node._virtual_devices:
> +            vdev_args += f"--vdev {dev} "
> +        vdev_args = vdev_args[:-1]
> +        driver_tests_command = (
> +            f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests"
> +        )
> +        if vdev_args:
> +            self._logger.info(
> +                "Running driver tests with the following virtual "
> +                f"devices: {vdev_args}"
> +            )
> +            driver_tests_command += f' --test-args "{vdev_args}"'
> +
> +        self.sut_node.main_session.send_command(
> +            driver_tests_command,
> +            300,
> +            verify=True,
> +            privileged=True,
> +        )
> +
> +    def test_devices_listed_in_testpmd(self) -> None:
> +        """
> +        Test:
> +            Uses testpmd driver to verify that devices have been found by testpmd.
> +        """
> +        testpmd_driver = self.sut_node.create_interactive_shell(
> +            TestPmdShell, privileged=True
> +        )
> +        dev_list: list[str] = [str(x) for x in testpmd_driver.get_devices()]

The type hint is not necessary, at least according to my mypy. We can
keep it, but we don't do this anywhere else, so I'd go for
consistency.

> +        for nic in self.nics_in_node:
> +            self.verify(
> +                nic.pci in dev_list,
> +                f"Device {nic.pci} was not listed in testpmd's available devices, "
> +                "please check your configuration",
> +            )
> +
> +    def test_device_bound_to_driver(self) -> None:
> +        """
> +        Test:
> +            Ensure that all drivers listed in the config are bound to the correct
> +            driver.
> +        """
> +        path_to_devbind = self.sut_node.main_session.join_remote_path(
> +            self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> +        )
> +
> +        all_nics_in_dpdk_devbind = self.sut_node.main_session.send_command(
> +            f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'",
> +            SETTINGS.timeout,
> +        ).stdout
> +
> +        for nic in self.nics_in_node:
> +            # This regular expression finds the line in the above string that starts
> +            # with the address for the nic we are on in the loop and then captures the
> +            # name of the driver in a group
> +            devbind_info_for_nic = re.search(
> +                f"{nic.pci}[^\\n]*drv=([\\d\\w]*) [^\\n]*",
> +                all_nics_in_dpdk_devbind,
> +            )
> +            self.verify(
> +                devbind_info_for_nic is not None,
> +                f"Failed to find configured device ({nic.pci}) using dpdk-devbind.py",
> +            )
> +            # We know this isn't None, but mypy doesn't
> +            assert devbind_info_for_nic is not None
> +            self.verify(
> +                devbind_info_for_nic.group(1) == nic.os_driver,
> +                f"Driver for device {nic.pci} does not match driver listed in "
> +                f"configuration (bound to {devbind_info_for_nic.group(1)})",
> +            )
> --
> 2.41.0
>