On Mon, Jul 17, 2023 at 9:31 PM Jeremy Spewock wrote: > > > 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. > 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.