On Mon, Jul 17, 2023 at 9:31 PM Jeremy Spewock <jspew...@iol.unh.edu> wrote:
>
>
> On Mon, Jul 17, 2023 at 10:50 AM Juraj Linkeš <juraj.lin...@pantheon.tech>
> wrote:
>
>> I found additional things while working with the interactive shell code.
>>
>> On Thu, Jul 13, 2023 at 6:54 PM <jspew...@iol.unh.edu> wrote:
>> >
>> > From: Jeremy Spewock <jspew...@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 <jspew...@iol.unh.edu>
>> > ---
>>
>> <snip>
>>
>> > diff --git a/dts/framework/config/conf_yaml_schema.json
>> b/dts/framework/config/conf_yaml_schema.json
>> > index ca2d4a1ef2..61f52b4365 100644
>> > --- a/dts/framework/config/conf_yaml_schema.json
>> > +++ b/dts/framework/config/conf_yaml_schema.json
>> <snip>
>> > @@ -211,8 +332,27 @@
>> > ]
>> > }
>> > },
>> > + "skip_smoke_tests": {
>> > + "description": "Optional field that allows you to skip
>> smoke testing",
>> > + "type": "boolean"
>> > + },
>> > "system_under_test": {
>> > - "$ref": "#/definitions/node_name"
>> > + "type":"object",
>> > + "properties": {
>> > + "node_name": {
>> > + "$ref": "#/definitions/node_name"
>> > + },
>> > + "vdevs": {
>> > + "description": "Opentional list of names of vdevs to
>> be used in execution",
>>
>> Typo in Opentional
>>
>> > + "type": "array",
>> > + "items": {
>> > + "type": "string"
>> > + }
>> > + }
>> > + },
>> > + "required": [
>> > + "node_name"
>> > + ]
>> > }
>> > },
>> > "additionalProperties": false,
>> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py
>> > index 0502284580..7b09d8fba8 100644
>> > --- a/dts/framework/dts.py
>> > +++ b/dts/framework/dts.py
>> <snip>
>> > @@ -118,14 +124,15 @@ def _run_build_target(
>> >
>> > try:
>> > sut_node.set_up_build_target(build_target)
>> > - result.dpdk_version = sut_node.dpdk_version
>> > + # result.dpdk_version = sut_node.dpdk_version
>> > +
>> build_target_result.add_build_target_versions(sut_node.get_build_target_info())
>>
>> The additions to execution_result and build_target_result are
>> inconsistent - one modifies __init__, the other doesn't. The time at
>> which we have the info available is different, which is the reason for
>> the inconsistency, but I'd rather make all results classes as
>> consistent as possible - create from config, add other info after
>> that.
>>
>> > build_target_result.update_setup(Result.PASS)
>> > except Exception as e:
>> > dts_logger.exception("Build target setup failed.")
>> > build_target_result.update_setup(Result.FAIL, e)
>> >
>> > else:
>> > - _run_suites(sut_node, execution, build_target_result)
>> > + _run_all_suites(sut_node, execution, build_target_result)
>> >
>> > finally:
>> > try:
>>
>> <snip>
>>
>> > diff --git a/dts/framework/remote_session/remote/__init__.py
>> b/dts/framework/remote_session/remote/__init__.py
>> > index 8a1512210a..03fd309f2b 100644
>> > --- a/dts/framework/remote_session/remote/__init__.py
>> > +++ b/dts/framework/remote_session/remote/__init__.py
>> > @@ -1,16 +1,26 @@
>> > # SPDX-License-Identifier: BSD-3-Clause
>> > # Copyright(c) 2023 PANTHEON.tech s.r.o.
>> > +# Copyright(c) 2023 University of New Hampshire
>> >
>> > # pylama:ignore=W0611
>> >
>> > from framework.config import NodeConfiguration
>> > from framework.logger import DTSLOG
>> >
>> > +from .interactive_remote_session import InteractiveRemoteSession
>> > +from .interactive_shell import InteractiveShell
>> > from .remote_session import CommandResult, RemoteSession
>> > from .ssh_session import SSHSession
>> > +from .testpmd_shell import TestPmdDevice, TestPmdShell
>> >
>> >
>> > def create_remote_session(
>> > node_config: NodeConfiguration, name: str, logger: DTSLOG
>> > ) -> RemoteSession:
>> > return SSHSession(node_config, name, logger)
>> > +
>> > +
>> > +def create_interactive_session(
>> > + node_config: NodeConfiguration, name: str, logger: DTSLOG
>> > +) -> InteractiveRemoteSession:
>>
>> The name parameter is not used, remove it please.
>>
>> > + return InteractiveRemoteSession(node_config, logger)
>>
>> <snip>
>>
>> > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py
>> b/dts/framework/remote_session/remote/testpmd_shell.py
>> > new file mode 100644
>> > index 0000000000..c0261c00f6
>> > --- /dev/null
>> > +++ b/dts/framework/remote_session/remote/testpmd_shell.py
>> > @@ -0,0 +1,74 @@
>> > +# SPDX-License-Identifier: BSD-3-Clause
>> > +# Copyright(c) 2023 University of New Hampshire
>> > +
>> > +from pathlib import PurePath
>> > +
>> > +from paramiko import SSHClient # type: ignore
>> > +
>> > +from framework.logger import DTSLOG
>> > +from framework.settings import SETTINGS
>> > +
>> > +from .interactive_shell import InteractiveShell
>> > +
>> > +
>> > +class TestPmdDevice(object):
>> > + pci_address: str
>> > +
>> > + def __init__(self, pci_address: str):
>> > + self.pci_address = pci_address
>> > +
>> > + def __str__(self) -> str:
>> > + return self.pci_address
>> > +
>> > +
>> > +class TestPmdShell(InteractiveShell):
>> > + expected_prompt: str = "testpmd>"
>> > + _eal_flags: str
>> > +
>> > + def __init__(
>> > + self,
>> > + interactive_session: SSHClient,
>> > + logger: DTSLOG,
>> > + path_to_testpmd: PurePath,
>> > + eal_flags: str,
>> > + timeout: float = SETTINGS.timeout,
>> > + ) -> None:
>> > + """Initializes an interactive testpmd session using specified
>> parameters."""
>> > + self._eal_flags = eal_flags
>> > +
>> > + super(TestPmdShell, self).__init__(
>> > + interactive_session,
>> > + logger=logger,
>> > + path_to_app=path_to_testpmd,
>> > + timeout=timeout,
>> > + )
>> > +
>> > + def _start_application(self) -> None:
>> > + """Starts a new interactive testpmd shell using
>> _path_to_app."""
>> > + self.send_command(
>> > + f"{self._path_to_app} {self._eal_flags} -- -i",
>> > + )
>> > +
>> > + def send_command(self, command: str, prompt: str =
>> expected_prompt) -> str:
>> > + """Specific way of handling the command for testpmd
>> > +
>> > + An extra newline character is consumed in order to force the
>> current line into
>> > + the stdout buffer.
>> > + """
>> > + return self.send_command_get_output(f"{command}\n", prompt)
>> > +
>>
>> We don't really need two methods with different names which basically
>> do the same - both send a command and get output.
>> We could use super() to modify the method in subclasses. Or we could
>> introduce a new class attribute storing the command suffix, similarly
>> to expected_prompt.
>>
>> A note on the class attributes: we should properly document them in
>> InteractiveShell so that it's clear what should be considered to be
>> overridden in derived classes.
>>
>> > + def get_devices(self) -> list[TestPmdDevice]:
>> > + """Get a list of device names that are known to testpmd
>> > +
>> > + Uses the device info listed in testpmd and then parses the
>> output to
>> > + return only the names of the devices.
>> > +
>> > + Returns:
>> > + A list of strings representing device names (e.g.
>> 0000:14:00.1)
>> > + """
>> > + dev_info: str = self.send_command("show device info all")
>> > + dev_list: list[TestPmdDevice] = []
>> > + for line in dev_info.split("\n"):
>> > + if "device name:" in line.lower():
>> > + dev_list.append(TestPmdDevice(line.strip().split(":
>> ")[1].strip()))
>>
>> We should pass the full line to TestPmdDevice and process the line in
>> the class. This splits the logic properly - the full line is needed to
>> get other information about the device.
>>
>> > + return dev_list
>> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
>> > index 743919820c..fe1994673e 100644
>> > --- a/dts/framework/test_result.py
>> > +++ b/dts/framework/test_result.py
>> <snip>
>> > @@ -267,14 +288,17 @@ class DTSResult(BaseResult):
>> > def __init__(self, logger: DTSLOG):
>> > super(DTSResult, self).__init__()
>> > self.dpdk_version = None
>> > + self.output = None
>> > self._logger = logger
>> > self._errors = []
>> > self._return_code = ErrorSeverity.NO_ERR
>> > self._stats_result = None
>> > self._stats_filename = os.path.join(SETTINGS.output_dir,
>> "statistics.txt")
>> >
>> > - def add_execution(self, sut_node: NodeConfiguration) ->
>> ExecutionResult:
>> > - execution_result = ExecutionResult(sut_node)
>> > + def add_execution(
>> > + self, sut_node: NodeConfiguration, sut_version_info: NodeInfo
>> > + ) -> ExecutionResult:
>> > + execution_result = ExecutionResult(sut_node, sut_version_info)
>> > self._inner_results.append(execution_result)
>> > return execution_result
>> >
>> > @@ -296,7 +320,8 @@ def process(self) -> None:
>> > for error in self._errors:
>> > self._logger.debug(repr(error))
>> >
>> > - self._stats_result = Statistics(self.dpdk_version)
>> > + self._stats_result = Statistics(self.output)
>>
>> By changing this (and the semi-related line 121 in dts.py) we've
>> changed the behavior a bit. We've talked about removing the part which
>> modifies self.output (or something similar) and these (and other
>> related parts of the code) look like the remnants of that change.
>> Let's remove these as well.
>>
>> > + # add information gathered from the smoke tests to the
>> statistics
>> > self.add_stats(self._stats_result)
>> > with open(self._stats_filename, "w+") as stats_file:
>> > stats_file.write(str(self._stats_result))
>>
>> <snip>
>>
>> > diff --git a/dts/tests/TestSuite_smoke_tests.py
>> b/dts/tests/TestSuite_smoke_tests.py
>> > new file mode 100644
>> > index 0000000000..9cf547205f
>> > --- /dev/null
>> > +++ b/dts/tests/TestSuite_smoke_tests.py
>> > @@ -0,0 +1,113 @@
>> > +# SPDX-License-Identifier: BSD-3-Clause
>> > +# Copyright(c) 2023 University of New Hampshire
>> > +
>> > +import re
>> > +
>> > +from framework.config import InteractiveApp, PortConfig
>> > +from framework.remote_session import TestPmdDevice, 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",
>> > + 300,
>> > + verify=True,
>> > + )
>>
>> Just a note: we can just add privileged=True to these calls to make
>> then run as root.
>>
>> > +
>> > + def test_driver_tests(self) -> None:
>> > + """
>> > + Test:
>> > + Run the driver-test unit-test suite through meson.
>> > + """
>> > + list_of_vdevs = ""
>>
>> This looks more like a string of vdev args :-). I'd rename it to
>> vdev_args.
>>
>> > + for dev in self.sut_node._execution_config.vdevs:
>>
>> The test suite should be working with sut_node objects, not
>> configuration. We should create the VirtualDevice objects from
>> _execution_config.vdevs in _set_up_execution and then only work with
>> those. And maybe reset the list in _tear_down_execution.
>>
>> > + list_of_vdevs += f"--vdev {dev} "
>> > + list_of_vdevs = list_of_vdevs[:-1]
>> > + if list_of_vdevs:
>> > + self._logger.info(
>> > + "Running driver tests with the following virtual "
>> > + f"devices: {list_of_vdevs}"
>> > + )
>> > + self.sut_node.main_session.send_command(
>> > + f"meson test -C {self.dpdk_build_dir_path} --suite
>> driver-tests "
>> > + f'--test-args "{list_of_vdevs}"',
>> > + 300,
>> > + verify=True,
>> > + )
>> > + else:
>> > + self.sut_node.main_session.send_command(
>> > + f"meson test -C {self.dpdk_build_dir_path} --suite
>> driver-tests",
>> > + 300,
>> > + verify=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(InteractiveApp.testpmd)
>> > + # We know it should always be a TestPmdShell but mypy doesn't
>> > + assert isinstance(testpmd_driver, TestPmdShell)
>> > + dev_list: list[TestPmdDevice] = testpmd_driver.get_devices()
>> > + for nic in self.nics_in_node:
>> > + self.verify(
>> > + nic.pci in map(str, dev_list),
>>
>> map(str, dev_list) gets called for every nic. I'm more inclined toward
>> composing the list right when assigning to dev_list. Or you could look
>> into returning (from get_devices()) a derived List overloading the
>> __contains__ method :-)
>>
>> > + 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
>> >
>>
>
> Thanks for the comments, I went through and addressed them. As we talked
> about today, I did also add parts from your patch in regards to the
> interactive shells and I added the ability for them to use sudo. We had
> talked about using a static method for this, but this would not work
> because what type of session you are using is an important distinction. I
> could not pass the entire OSSession class into the interactive shell, so I
> instead added just a reference to the method for accessing elevated
> privileges. I had to add a few # type: ignore lines for this to work due to
> the issue marked here: https://github.com/python/mypy/issues/2427 .
> Please see my new version.
>
I have some more comments on the new version, the main being that we can
actually pass a static method (from the proper object thus from the proper
class). I think that makes more sense since the method should've been
static from the get go. I've explained a bit in the comments and I'm
attaching a diff here.
diff --git a/dts/framework/remote_session/linux_session.py b/dts/framework/remote_session/linux_session.py
index f13f399121..f64aa8efb0 100644
--- a/dts/framework/remote_session/linux_session.py
+++ b/dts/framework/remote_session/linux_session.py
@@ -14,7 +14,8 @@ class LinuxSession(PosixSession):
The implementation of non-Posix compliant parts of Linux remote sessions.
"""
- def _get_privileged_command(self, command: str) -> str:
+ @staticmethod
+ def _get_privileged_command(command: str) -> str:
return f"sudo -- sh -c '{command}'"
def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:
diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py
index f03275b21d..dad8d4da1b 100644
--- a/dts/framework/remote_session/os_session.py
+++ b/dts/framework/remote_session/os_session.py
@@ -100,8 +100,9 @@ def create_interactive_shell(
timeout,
)
+ @staticmethod
@abstractmethod
- def _get_privileged_command(self, command: str) -> str:
+ def _get_privileged_command(command: str) -> str:
"""Modify the command so that it executes with administrative privileges.
Args:
diff --git a/dts/framework/remote_session/remote/interactive_shell.py b/dts/framework/remote_session/remote/interactive_shell.py
index 4d9c7638a5..3512f1f8a9 100644
--- a/dts/framework/remote_session/remote/interactive_shell.py
+++ b/dts/framework/remote_session/remote/interactive_shell.py
@@ -22,7 +22,6 @@ class InteractiveShell:
_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
@@ -34,7 +33,7 @@ def __init__(
logger: DTSLOG,
startup_command: str,
privileged: bool,
- _get_privileged_command: Callable[[str], str],
+ get_privileged_command: Callable[[str], str],
app_args: str = "",
timeout: float = SETTINGS.timeout,
) -> None:
@@ -48,11 +47,10 @@ def __init__(
self._timeout = timeout
self._startup_command = startup_command
self._app_args = app_args
- self._get_privileged_command = _get_privileged_command # type: ignore
self._privileged = privileged
- self._start_application()
+ self._start_application(get_privileged_command)
- def _start_application(self) -> None:
+ def _start_application(self, get_privileged_command: Callable[[str], str]) -> None:
"""Starts a new interactive application based on _startup_command.
This method is often overridden by subclasses as their process for
@@ -60,7 +58,7 @@ def _start_application(self) -> None:
"""
start_command = f"{self._startup_command} {self._app_args}"
if self._privileged:
- start_command = self._get_privileged_command(start_command) # type: ignore
+ start_command = get_privileged_command(start_command)
self.send_command(start_command)
def send_command(self, command: str, prompt: str | None = None) -> str:
diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dts/framework/remote_session/remote/testpmd_shell.py
index 4abe70d421..af8ac2a057 100644
--- a/dts/framework/remote_session/remote/testpmd_shell.py
+++ b/dts/framework/remote_session/remote/testpmd_shell.py
@@ -2,6 +2,7 @@
# Copyright(c) 2023 University of New Hampshire
from pathlib import PurePath
+from typing import Callable
from .interactive_shell import InteractiveShell
@@ -24,10 +25,10 @@ class TestPmdShell(InteractiveShell):
"\n" # We want to append an extra newline to every command
)
- def _start_application(self) -> None:
+ def _start_application(self, get_privileged_command: Callable[[str], str]) -> None:
"""See "_start_application" in InteractiveShell."""
self._app_args += " -- -i"
- super()._start_application()
+ super()._start_application(get_privileged_command)
def get_devices(self) -> list[TestPmdDevice]:
"""Get a list of device names that are known to testpmd
