On Tue, Jul 18, 2023 at 5:41 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
You forgot to add me to the patch :-)
Sorry about that! I was just adding everyone who was on the last one but I must have missed a line.
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.
That's a good call, it probably could do with some more documentation explaining it and how it works, I've added this.
> + 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.
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.
> + 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.
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.
> + 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.
I guess this one must have slipped through before after I changed the names, I'll change it.
> + 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.
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.
>
> 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.
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.
>
> 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.
Good point, I'll remove it.
> + 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
>