From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DACEF42E9D; Mon, 17 Jul 2023 16:50:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7B75A40DFD; Mon, 17 Jul 2023 16:50:50 +0200 (CEST) Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com [209.85.208.52]) by mails.dpdk.org (Postfix) with ESMTP id 8C10C40A80 for ; Mon, 17 Jul 2023 16:50:49 +0200 (CEST) Received: by mail-ed1-f52.google.com with SMTP id 4fb4d7f45d1cf-51e566b1774so5999059a12.1 for ; Mon, 17 Jul 2023 07:50:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689605449; x=1692197449; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=jIQHkQqbOK0Uwp+wu7g/TsElHQaKlvAbp6/wZxQTEAs=; b=kTm3EAhhJ+JZ/6TExpcRd4sNHuZyMopqysPFNetVNB1luN1TZ2WYId12c+EHytxwX+ njQlSQZ/sPy+4UAYjA2c72Urqx7e4ZxibCzMeV7mXURbSsltGeHRYFtyZZ7HCc2BwevW p7BABvAujmYRCIQf8bC8BUcoxqkWDMCJ2uQKypxb4YKjRK6tLEalSRdi77KvNjiyg9qU ea6aszmnb+NA17enkMHHvpOVYy7eNAtSW+pE92rWEGEOtcgtJmXhttY1LjCaO6weQaa/ ywwBySAGwdTRD157awN3XFJ+mBBtHr1AVz+iVAHd7PLemOXvl2hSNVR7iGqgWuHZmL48 oNxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689605449; x=1692197449; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jIQHkQqbOK0Uwp+wu7g/TsElHQaKlvAbp6/wZxQTEAs=; b=GS2HJ5uAwGT3o11XYV3b+jQLPJ2N9taKw6OyZwoQ/kcdTOmrOZFXlUANG68G5oxnHM tygh+OMFa2roZMOHRihMXJK/pd3ETO1+VF1D3n7AtOeSdUVN6fgRyC/9IDP99eXv090r PfPDjIYLaKgHyb6WSD5FXpjGki6fHLvU0j5LCqfqqknvP11Aw/2W9jen5eYAdrH3cgrO nYT69BGFTCQa55fFAoNCghv4peZf0QTtw+bzYniVD3SrRqCmm52PyqPAoqHeTYPSgy87 4arg239pkrMPESSUWUEG06WxBljMryse+izkE1/Xm2/nk0qHrjvKQwkN44IRzEvvTelJ okIA== X-Gm-Message-State: ABy/qLYB9A4xrG6stbAorbOIpi6Ppsfa/pZVNMGMsN7ruCcq3Zo+FUNe iQcf7t+Vkz5xG2zlJgBwZMU7C6C4p6ZRNk8LqKbBBA== X-Google-Smtp-Source: APBJJlFNn8ayUg7XXFQR1WFdNd+WdA0/fQst2FuKe5RWwzqrpM2rjlK9pZLruCDsluGzAibOu5U0Xo7UadprRbF+6Gc= X-Received: by 2002:aa7:dbd9:0:b0:521:7417:1131 with SMTP id v25-20020aa7dbd9000000b0052174171131mr5509746edt.15.1689605449000; Mon, 17 Jul 2023 07:50:49 -0700 (PDT) MIME-Version: 1.0 References: <20230713165347.21997-2-jspewock@iol.unh.edu> <20230713165347.21997-3-jspewock@iol.unh.edu> In-Reply-To: <20230713165347.21997-3-jspewock@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 17 Jul 2023 16:50:37 +0200 Message-ID: Subject: Re: [PATCH v7 1/1] dts: add smoke tests To: jspewock@iol.unh.edu Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, lijuan.tu@intel.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org I found additional things while working with the interactive shell code. On Thu, Jul 13, 2023 at 6:54=E2=80=AFPM 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/c= onfig/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 =3D sut_node.dpdk_version > + # result.dpdk_version =3D 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/framew= ork/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=3DW0611 > > 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/f= ramework/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 =3D pci_address > + > + def __str__(self) -> str: > + return self.pci_address > + > + > +class TestPmdShell(InteractiveShell): > + expected_prompt: str =3D "testpmd>" > + _eal_flags: str > + > + def __init__( > + self, > + interactive_session: SSHClient, > + logger: DTSLOG, > + path_to_testpmd: PurePath, > + eal_flags: str, > + timeout: float =3D SETTINGS.timeout, > + ) -> None: > + """Initializes an interactive testpmd session using specified pa= rameters.""" > + self._eal_flags =3D eal_flags > + > + super(TestPmdShell, self).__init__( > + interactive_session, > + logger=3Dlogger, > + path_to_app=3Dpath_to_testpmd, > + timeout=3Dtimeout, > + ) > + > + 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 =3D expected_prompt= ) -> str: > + """Specific way of handling the command for testpmd > + > + An extra newline character is consumed in order to force the cur= rent 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 outpu= t 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 =3D self.send_command("show device info all") > + dev_list: list[TestPmdDevice] =3D [] > + 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 =3D None > + self.output =3D None > self._logger =3D logger > self._errors =3D [] > self._return_code =3D ErrorSeverity.NO_ERR > self._stats_result =3D None > self._stats_filename =3D os.path.join(SETTINGS.output_dir, "stat= istics.txt") > > - def add_execution(self, sut_node: NodeConfiguration) -> ExecutionRes= ult: > - execution_result =3D ExecutionResult(sut_node) > + def add_execution( > + self, sut_node: NodeConfiguration, sut_version_info: NodeInfo > + ) -> ExecutionResult: > + execution_result =3D 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 =3D Statistics(self.dpdk_version) > + self._stats_result =3D 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 statistic= s > 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_smo= ke_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 =3D True > + # dicts in this list are expected to have two keys: > + # "pci_address" and "current_driver" > + nics_in_node: list[PortConfig] =3D [] > + > + 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 =3D self.sut_node.remote_dpdk_build_dir > + self.nics_in_node =3D 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-test= s", > + 300, > + verify=3DTrue, > + ) Just a note: we can just add privileged=3DTrue 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 =3D "" 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 +=3D f"--vdev {dev} " > + list_of_vdevs =3D 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 drive= r-tests " > + f'--test-args "{list_of_vdevs}"', > + 300, > + verify=3DTrue, > + ) > + else: > + self.sut_node.main_session.send_command( > + f"meson test -C {self.dpdk_build_dir_path} --suite drive= r-tests", > + 300, > + verify=3DTrue, > + ) > + > + def test_devices_listed_in_testpmd(self) -> None: > + """ > + Test: > + Uses testpmd driver to verify that devices have been found b= y testpmd. > + """ > + testpmd_driver =3D self.sut_node.create_interactive_shell(Intera= ctiveApp.testpmd) > + # We know it should always be a TestPmdShell but mypy doesn't > + assert isinstance(testpmd_driver, TestPmdShell) > + dev_list: list[TestPmdDevice] =3D 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 th= e correct driver. > + """ > + path_to_devbind =3D self.sut_node.main_session.join_remote_path( > + self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.p= y" > + ) > + > + all_nics_in_dpdk_devbind =3D self.sut_node.main_session.send_com= mand( > + 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 the= n captures the > + # name of the driver in a group > + devbind_info_for_nic =3D re.search( > + f"{nic.pci}[^\\n]*drv=3D([\\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 dpd= k-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) =3D=3D nic.os_driver, > + f"Driver for device {nic.pci} does not match driver list= ed in " > + f"configuration (bound to {devbind_info_for_nic.group(1)= })", > + ) > -- > 2.41.0 >