On Mon, Jul 17, 2023 at 10:50 AM Juraj Linkeš wrote: > I found additional things while working with the interactive shell code. > > On Thu, Jul 13, 2023 at 6:54 PM wrote: > > > > From: Jeremy Spewock > > > > 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 > > --- > > > > > 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 > > > @@ -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 > > > @@ -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: > > > > > 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) > > > > > 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 > > > @@ -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)) > > > > > 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.