<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 18, 2023 at 5:41 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">You forgot to add me to the patch :-)<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">Sorry about that! I was just adding everyone who was on the last one but I must have missed a line.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
A few comments below.<br>
<br>
On Mon, Jul 17, 2023 at 9:37 PM <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> wrote:<br>
><br>
> From: Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>><br>
><br>
> Adds a new test suite for running smoke tests that verify general<br>
> configuration aspects of the system under test. If any of these tests<br>
> fail, the DTS execution terminates as part of a "fail-fast" model.<br>
><br>
> Signed-off-by: Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>><br>
> ---<br>
<br>
<snip><br>
<br>
> diff --git a/dts/framework/remote_session/remote/interactive_shell.py b/dts/framework/remote_session/remote/interactive_shell.py<br>
> new file mode 100644<br>
> index 0000000000..4d9c7638a5<br>
> --- /dev/null<br>
> +++ b/dts/framework/remote_session/remote/interactive_shell.py<br>
> @@ -0,0 +1,98 @@<br>
> +# SPDX-License-Identifier: BSD-3-Clause<br>
> +# Copyright(c) 2023 University of New Hampshire<br>
> +<br>
> +from pathlib import PurePath<br>
> +from typing import Callable<br>
> +<br>
> +from paramiko import Channel, SSHClient, channel  # type: ignore<br>
> +<br>
> +from framework.logger import DTSLOG<br>
> +from framework.settings import SETTINGS<br>
> +<br>
> +<br>
> +class InteractiveShell:<br>
> +<br>
> +    _interactive_session: SSHClient<br>
> +    _stdin: channel.ChannelStdinFile<br>
> +    _stdout: channel.ChannelFile<br>
> +    _ssh_channel: Channel<br>
> +    _logger: DTSLOG<br>
> +    _timeout: float<br>
> +    _startup_command: str<br>
> +    _app_args: str<br>
> +    _default_prompt: str = ""<br>
> +    _privileged: bool<br>
> +    _get_privileged_command: Callable[[str], str]<br>
> +    # Allows for app specific extra characters to be appended to commands<br>
> +    _command_extra_chars: str = ""<br>
> +    path: PurePath<br>
> +    dpdk_app: bool = False<br>
> +<br>
<br>
We should document this class - let's use the google format:<br>
<a href="https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings" rel="noreferrer" target="_blank">https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings</a><br>
You can use this as an example:<br>
<a href="http://patches.dpdk.org/project/dpdk/patch/20230511091408.236638-5-juraj.linkes@pantheon.tech/" rel="noreferrer" target="_blank">http://patches.dpdk.org/project/dpdk/patch/20230511091408.236638-5-juraj.linkes@pantheon.tech/</a><br>
A note on class attributes: we should document the public ones as well<br>
as those which should be considered to be overridden in derived<br>
classes.<br>
<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">That's a good call, it probably could do with some more documentation explaining it and how it works, I've added this.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +    def __init__(<br>
> +        self,<br>
> +        interactive_session: SSHClient,<br>
> +        logger: DTSLOG,<br>
> +        startup_command: str,<br>
> +        privileged: bool,<br>
> +        _get_privileged_command: Callable[[str], str],<br>
> +        app_args: str = "",<br>
> +        timeout: float = SETTINGS.timeout,<br>
> +    ) -> None:<br>
<br>
We can simplify the constructor signature a bit. We can combine<br>
privileged and _get_privileged_command (which shouldn't start with the<br>
underscore - only private class attributes should have that) in that<br>
get_privileged_command could be None (which would be the same as<br>
privileged=False and not None the same as privileged=True).<br>
<br>
We don't need startup_command as we're just passing self.path to it,<br>
which we already have access to.<br>
There is an alternative approach to the path manipulation logic - we<br>
don't modify shell_cls.path at all and pass the resulting path (the<br>
result of all the operations we do in Node objects) to the constructor<br>
(in this case we'd retain the startup_command parameter, although it<br>
should be then renamed to path_app or something similar). This doesn't<br>
move the logic entirely out of InteractiveShell though, as we still<br>
need to access the class attribute. For that reason, I don't see much<br>
of a point in pursuing this alternative - we'd basically be just<br>
passing one extra unneeded path argument through all the calls.<br>
<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">I agree and prefer just using the path in the start_application. That's also a good idea to remove the boolean and just make the parameter nullable, that does clean things up.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +        self._interactive_session = interactive_session<br>
> +        self._ssh_channel = self._interactive_session.invoke_shell()<br>
> +        self._stdin = self._ssh_channel.makefile_stdin("w")<br>
> +        self._stdout = self._ssh_channel.makefile("r")<br>
> +        self._ssh_channel.settimeout(timeout)<br>
> +        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams<br>
> +        self._logger = logger<br>
> +        self._timeout = timeout<br>
> +        self._startup_command = startup_command<br>
> +        self._app_args = app_args<br>
> +        self._get_privileged_command = _get_privileged_command  # type: ignore<br>
<br>
We can actually make this work with a static method:<br>
1. Don't store the method in this object. We'd need to pass it to<br>
_start_application, but this should be ok as we're only using the<br>
method when starting the application - once it's running, we'll never<br>
need it again.<br>
2. When we stored the method, we were passing the self parameter when<br>
calling self._get_privileged_command, which is why it couldn't be<br>
static. When calling without self, static works.<br>
        Note: the staticmethod decorator must be above the<br>
abstractmethod decorator, otherwise the interpreter throws an error.<br>
3. The type can be either Callable[[str], str] or MethodType (from the<br>
types module). I'd go with callable here as it's more precise.<br>
<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">That makes sense, sorry, I thought when we were talking about making it static it was about just calling it from the class itself. The reason I thought static wouldn't work for that is because we of course need to know that we are working with a LinuxSession so we wouldn't know which class to call upon which is why I just passed the function reference. But, you're right, this makes more sense and still keeps it static so we don't need to pass the self into it.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +        self._privileged = privileged<br>
> +        self._start_application()<br>
> +<br>
> +    def _start_application(self) -> None:<br>
> +        """Starts a new interactive application based on _startup_command.<br>
> +<br>
> +        This method is often overridden by subclasses as their process for<br>
> +        starting may look different.<br>
> +        """<br>
> +        start_command = f"{self._startup_command} {self._app_args}"<br>
> +        if self._privileged:<br>
> +            start_command = self._get_privileged_command(start_command)  # type: ignore<br>
> +        self.send_command(start_command)<br>
> +<br>
> +    def send_command(self, command: str, prompt: str | None = None) -> str:<br>
> +        """Send a command and get all output before the expected ending string.<br>
> +<br>
> +        Lines that expect input are not included in the stdout buffer so they cannot be<br>
<br>
My IDE spell checker says there should be a comma before so.<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">Good catch, thank you.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +        used for expect. For example, if you were prompted to log into something<br>
> +        with a username and password, you cannot expect "username:" because it won't<br>
> +        yet be in the stdout buffer. A work around for this could be consuming an<br>
<br>
Workaround without the space.<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">Oops, thank you.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +        extra newline character to force the current prompt into the stdout buffer.<br>
> +<br>
> +        Returns:<br>
> +            All output in the buffer before expected string<br>
> +        """<br>
> +        self._<a href="http://logger.info" rel="noreferrer" target="_blank">logger.info</a>(f"Sending command {command.strip()}...")<br>
> +        if prompt is None:<br>
> +            prompt = self._default_prompt<br>
> +        self._stdin.write(f"{command}{self._command_extra_chars}\n")<br>
> +        self._stdin.flush()<br>
> +        out: str = ""<br>
> +        for line in self._stdout:<br>
> +            out += line<br>
> +            if prompt in line and not line.rstrip().endswith(<br>
> +                command.rstrip()<br>
> +            ):  # ignore line that sent command<br>
> +                break<br>
> +        self._logger.debug(f"Got output: {out}")<br>
> +        return out<br>
> +<br>
> +    def close(self) -> None:<br>
> +        self._stdin.close()<br>
> +        self._ssh_channel.close()<br>
> +<br>
> +    def __del__(self) -> None:<br>
> +        self.close()<br>
<br>
<snip><br>
<br>
> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py<br>
> index 743919820c..724e7328eb 100644<br>
> --- a/dts/framework/test_result.py<br>
> +++ b/dts/framework/test_result.py<br>
<snip><br>
> @@ -213,6 +218,12 @@ def __init__(self, build_target: BuildTargetConfiguration):<br>
>          self.os = build_target.os<br>
>          self.cpu = build_target.cpu<br>
>          self.compiler = build_target.compiler<br>
> +        self.compiler_version = None<br>
> +        self.dpdk_version = None<br>
> +<br>
> +    def add_build_target_versions(self, versions: BuildTargetInfo) -> None:<br>
<br>
Just noticed the name - rename to add_build_target_info to align with<br>
everything else.<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">I guess this one must have slipped through before after I changed the names, I'll change it.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +        self.compiler_version = versions.compiler_version<br>
> +        self.dpdk_version = versions.dpdk_version<br>
><br>
>      def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:<br>
>          test_suite_result = TestSuiteResult(test_suite_name)<br>
<br>
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py<br>
> index d48fafe65d..39237a95d6 100644<br>
> --- a/dts/framework/testbed_model/node.py<br>
> +++ b/dts/framework/testbed_model/node.py<br>
> @@ -7,7 +7,7 @@<br>
>  A node is a generic host that DTS connects to and manages.<br>
>  """<br>
><br>
> -from typing import Any, Callable<br>
> +from typing import Any, Callable, Type<br>
><br>
>  from framework.config import (<br>
>      BuildTargetConfiguration,<br>
> @@ -15,7 +15,7 @@<br>
>      NodeConfiguration,<br>
>  )<br>
>  from framework.logger import DTSLOG, getLogger<br>
> -from framework.remote_session import OSSession, create_session<br>
> +from framework.remote_session import InteractiveShellType, OSSession, create_session<br>
>  from framework.settings import SETTINGS<br>
><br>
>  from .hw import (<br>
> @@ -23,6 +23,7 @@<br>
>      LogicalCoreCount,<br>
>      LogicalCoreList,<br>
>      LogicalCoreListFilter,<br>
> +    VirtualDevice,<br>
>      lcore_filter,<br>
>  )<br>
><br>
> @@ -40,6 +41,8 @@ class Node(object):<br>
>      lcores: list[LogicalCore]<br>
>      _logger: DTSLOG<br>
>      _other_sessions: list[OSSession]<br>
> +    _execution_config: ExecutionConfiguration<br>
> +    _virtual_devices: list[VirtualDevice]<br>
><br>
>      def __init__(self, node_config: NodeConfiguration):<br>
>          self.config = node_config<br>
> @@ -54,6 +57,7 @@ def __init__(self, node_config: NodeConfiguration):<br>
>          ).filter()<br>
><br>
>          self._other_sessions = []<br>
> +        self._virtual_devices = []<br>
<br>
This is not a private attribute, so let's drop the starting underscore.<br>
<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">I wasn't sure whether we would want it to be private or not but you're right, there isn't much reason to have it as such so I'll remove it.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>          self._<a href="http://logger.info" rel="noreferrer" target="_blank">logger.info</a>(f"Created node: {<a href="http://self.name" rel="noreferrer" target="_blank">self.name</a>}")<br>
><br>
> @@ -64,6 +68,9 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:<br>
>          """<br>
>          self._setup_hugepages()<br>
>          self._set_up_execution(execution_config)<br>
> +        self._execution_config = execution_config<br>
> +        for vdev in execution_config.vdevs:<br>
> +            self._virtual_devices.append(VirtualDevice(vdev))<br>
><br>
>      def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None:<br>
>          """<br>
> @@ -77,6 +84,7 @@ def tear_down_execution(self) -> None:<br>
>          this node is part of concludes.<br>
>          """<br>
>          self._tear_down_execution()<br>
> +        self._virtual_devices = []<br>
<br>
I'd prefer this to be above self._tear_down_execution(). If there's a<br>
failure in self._tear_down_execution(), the devices won't be removed<br>
so removing them before the call is safer.<br>
<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">That's a good point, I didn't think about how the failure of this method would cause this to never happen. The reason I put it below originally was in case the virtual devices were needed in the teardown stage but I agree that what you mentioned is also important to consider and putting it above is safer.<br></div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
>      def _tear_down_execution(self) -> None:<br>
>          """<br>
<br>
<snip><br>
<br>
> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py<br>
> new file mode 100644<br>
> index 0000000000..83712e75c6<br>
> --- /dev/null<br>
> +++ b/dts/tests/TestSuite_smoke_tests.py<br>
> @@ -0,0 +1,114 @@<br>
> +# SPDX-License-Identifier: BSD-3-Clause<br>
> +# Copyright(c) 2023 University of New Hampshire<br>
> +<br>
> +import re<br>
> +<br>
> +from framework.config import PortConfig<br>
> +from framework.remote_session import TestPmdShell<br>
> +from framework.settings import SETTINGS<br>
> +from framework.test_suite import TestSuite<br>
> +from framework.utils import REGEX_FOR_PCI_ADDRESS<br>
> +<br>
> +<br>
> +class SmokeTests(TestSuite):<br>
> +    is_blocking = True<br>
> +    # dicts in this list are expected to have two keys:<br>
> +    # "pci_address" and "current_driver"<br>
> +    nics_in_node: list[PortConfig] = []<br>
> +<br>
> +    def set_up_suite(self) -> None:<br>
> +        """<br>
> +        Setup:<br>
> +            Set the build directory path and generate a list of NICs in the SUT node.<br>
> +        """<br>
> +        self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir<br>
> +        self.nics_in_node = self.sut_node.config.ports<br>
> +<br>
> +    def test_unit_tests(self) -> None:<br>
> +        """<br>
> +        Test:<br>
> +            Run the fast-test unit-test suite through meson.<br>
> +        """<br>
> +        self.sut_node.main_session.send_command(<br>
> +            f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests -t 60",<br>
> +            480,<br>
> +            verify=True,<br>
> +            privileged=True,<br>
> +        )<br>
> +<br>
> +    def test_driver_tests(self) -> None:<br>
> +        """<br>
> +        Test:<br>
> +            Run the driver-test unit-test suite through meson.<br>
> +        """<br>
> +        vdev_args = ""<br>
> +        for dev in self.sut_node._virtual_devices:<br>
> +            vdev_args += f"--vdev {dev} "<br>
> +        vdev_args = vdev_args[:-1]<br>
> +        driver_tests_command = (<br>
> +            f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests"<br>
> +        )<br>
> +        if vdev_args:<br>
> +            self._<a href="http://logger.info" rel="noreferrer" target="_blank">logger.info</a>(<br>
> +                "Running driver tests with the following virtual "<br>
> +                f"devices: {vdev_args}"<br>
> +            )<br>
> +            driver_tests_command += f' --test-args "{vdev_args}"'<br>
> +<br>
> +        self.sut_node.main_session.send_command(<br>
> +            driver_tests_command,<br>
> +            300,<br>
> +            verify=True,<br>
> +            privileged=True,<br>
> +        )<br>
> +<br>
> +    def test_devices_listed_in_testpmd(self) -> None:<br>
> +        """<br>
> +        Test:<br>
> +            Uses testpmd driver to verify that devices have been found by testpmd.<br>
> +        """<br>
> +        testpmd_driver = self.sut_node.create_interactive_shell(<br>
> +            TestPmdShell, privileged=True<br>
> +        )<br>
> +        dev_list: list[str] = [str(x) for x in testpmd_driver.get_devices()]<br>
<br>
The type hint is not necessary, at least according to my mypy. We can<br>
keep it, but we don't do this anywhere else, so I'd go for<br>
consistency.<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">Good point, I'll remove it.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +        for nic in self.nics_in_node:<br>
> +            self.verify(<br>
> +                nic.pci in dev_list,<br>
> +                f"Device {nic.pci} was not listed in testpmd's available devices, "<br>
> +                "please check your configuration",<br>
> +            )<br>
> +<br>
> +    def test_device_bound_to_driver(self) -> None:<br>
> +        """<br>
> +        Test:<br>
> +            Ensure that all drivers listed in the config are bound to the correct<br>
> +            driver.<br>
> +        """<br>
> +        path_to_devbind = self.sut_node.main_session.join_remote_path(<br>
> +            self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py"<br>
> +        )<br>
> +<br>
> +        all_nics_in_dpdk_devbind = self.sut_node.main_session.send_command(<br>
> +            f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'",<br>
> +            SETTINGS.timeout,<br>
> +        ).stdout<br>
> +<br>
> +        for nic in self.nics_in_node:<br>
> +            # This regular expression finds the line in the above string that starts<br>
> +            # with the address for the nic we are on in the loop and then captures the<br>
> +            # name of the driver in a group<br>
> +            devbind_info_for_nic = re.search(<br>
> +                f"{nic.pci}[^\\n]*drv=([\\d\\w]*) [^\\n]*",<br>
> +                all_nics_in_dpdk_devbind,<br>
> +            )<br>
> +            self.verify(<br>
> +                devbind_info_for_nic is not None,<br>
> +                f"Failed to find configured device ({nic.pci}) using dpdk-devbind.py",<br>
> +            )<br>
> +            # We know this isn't None, but mypy doesn't<br>
> +            assert devbind_info_for_nic is not None<br>
> +            self.verify(<br>
> +                devbind_info_for_nic.group(1) == nic.os_driver,<br>
> +                f"Driver for device {nic.pci} does not match driver listed in "<br>
> +                f"configuration (bound to {devbind_info_for_nic.group(1)})",<br>
> +            )<br>
> --<br>
> 2.41.0<br>
><br>
</blockquote></div></div>