<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 17, 2023 at 9:31 PM Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu">jspewock@iol.unh.edu</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div style="font-family:arial,sans-serif"> </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 17, 2023 at 10:50 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I found additional things while working with the interactive shell code. On Thu, Jul 13, 2023 at 6:54 PM <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> wrote: > > From: Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> > > 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 <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> > --- <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._<a href="http://logger.info" rel="noreferrer" target="_blank">logger.info</a>( > + "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 > </blockquote><div> </div><div style="font-family:arial,sans-serif">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: <a href="https://github.com/python/mypy/issues/2427" target="_blank">https://github.com/python/mypy/issues/2427</a> . Please see my new version. </div></div></div></blockquote><div> </div><div>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.</div><div> </div></div></div>