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 0867F42EA6; Tue, 18 Jul 2023 11:41:37 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 93FEE410D3; Tue, 18 Jul 2023 11:41:37 +0200 (CEST) Received: from mail-ed1-f42.google.com (mail-ed1-f42.google.com [209.85.208.42]) by mails.dpdk.org (Postfix) with ESMTP id 9E83B40A84 for ; Tue, 18 Jul 2023 11:41:36 +0200 (CEST) Received: by mail-ed1-f42.google.com with SMTP id 4fb4d7f45d1cf-51e344efd75so11315836a12.1 for ; Tue, 18 Jul 2023 02:41:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689673296; x=1692265296; 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=MR6gLXHBgjpesDrL4PMRDM87kmmlIB7SnDwoMOCVOOI=; b=KTpJAtuJfQrUNWb0Kjhte7tQQ+CV/jIJmbcBkDKDmFG2jZO6+yPVxfHbtH1kWS+5ua DwuzK6hR2nBIGyRv84+L5BhV/ivoivXNabP0qdnli/UhGl1m5/CElp8MBeDKLGhwQ8Pf +SSFi4UUPb0Qc54OrKM5eXzfhrU/PanPop7ucxJG7QEWoyXMuP3safYWuO8RauZ38tRq hC1iXp1VyYfkJvxMLVD9Hfabwxjg0cdWkvPAU7sjTrF2EtUTOawqMCJOtC7K3cJqpONn 4nXo35wrGuiU6bSZSAgckmuDxrXPB02IT2vOHTTLLJxwdQwhBIuCuJpp5pCrpqrt5wo1 ml6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689673296; x=1692265296; 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=MR6gLXHBgjpesDrL4PMRDM87kmmlIB7SnDwoMOCVOOI=; b=ZlW1lU07iFNPD+TStFPpJT813CbpjBNGjMPCIVifpi2hc+JrWHXpKcKrNIBEnGOkPm Imoc5ULEJD4V/wYyJPPw+PHpjnD0PzMSWZfyM+FXP/QZRdkjdVJvhNKsjmPLiQ9OHfRZ +y28aGllr+cXnkPOcQacEAoUHJyEMtIZifbuApmH42264hBleqomFDJB5hTpzh71l7K2 YGVQoNbbu5NwzzIhf97A47FJhz6TpQ8jpMR11TxcjjKb92gZQ7+zHa/Zg8ScSm/8lTwK k1Hg/zRnnE1Ep2LiXuVNY1Pki90RvE9HCP/yPsHvQFnabB8HHFuwvLx6wwGNf4OmFCxP xG4g== X-Gm-Message-State: ABy/qLb7GA+l4EFO0XP/Ce0lFTPkl25oiavmCo0kYWOBRD/lrdEF+tPh QhV7Kww1LlLqBcm3gfhN5+Bx4wDiZYjfZV/T7IafGK6Nb8CYN80/exsuPw== X-Google-Smtp-Source: APBJJlF4+rENDgCqdw9WzhehsI3KWF1aynlhlK0ZLVwjr067iHImm5Cvs6IGuHI4x6nrZs3HW7DDsfrRYIEPXXbsHMI= X-Received: by 2002:a05:6402:518f:b0:51e:4218:b91b with SMTP id q15-20020a056402518f00b0051e4218b91bmr13344335edd.1.1689673296097; Tue, 18 Jul 2023 02:41:36 -0700 (PDT) MIME-Version: 1.0 References: <20230717193705.26594-2-jspewock@iol.unh.edu> <20230717193705.26594-3-jspewock@iol.unh.edu> In-Reply-To: <20230717193705.26594-3-jspewock@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 18 Jul 2023 11:41:25 +0200 Message-ID: Subject: Re: [PATCH v8 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 You forgot to add me to the patch :-) A few comments below. On Mon, Jul 17, 2023 at 9:37=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/remote_session/remote/interactive_shell.py b/d= ts/framework/remote_session/remote/interactive_shell.py > new file mode 100644 > index 0000000000..4d9c7638a5 > --- /dev/null > +++ b/dts/framework/remote_session/remote/interactive_shell.py > @@ -0,0 +1,98 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2023 University of New Hampshire > + > +from pathlib import PurePath > +from typing import Callable > + > +from paramiko import Channel, SSHClient, channel # type: ignore > + > +from framework.logger import DTSLOG > +from framework.settings import SETTINGS > + > + > +class InteractiveShell: > + > + _interactive_session: SSHClient > + _stdin: channel.ChannelStdinFile > + _stdout: channel.ChannelFile > + _ssh_channel: Channel > + _logger: DTSLOG > + _timeout: float > + _startup_command: str > + _app_args: str > + _default_prompt: str =3D "" > + _privileged: bool > + _get_privileged_command: Callable[[str], str] > + # Allows for app specific extra characters to be appended to command= s > + _command_extra_chars: str =3D "" > + path: PurePath > + dpdk_app: bool =3D False > + We should document this class - let's use the google format: https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings You can use this as an example: http://patches.dpdk.org/project/dpdk/patch/20230511091408.236638-5-juraj.li= nkes@pantheon.tech/ A note on class attributes: we should document the public ones as well as those which should be considered to be overridden in derived classes. > + def __init__( > + self, > + interactive_session: SSHClient, > + logger: DTSLOG, > + startup_command: str, > + privileged: bool, > + _get_privileged_command: Callable[[str], str], > + app_args: str =3D "", > + timeout: float =3D SETTINGS.timeout, > + ) -> None: We can simplify the constructor signature a bit. We can combine privileged and _get_privileged_command (which shouldn't start with the underscore - only private class attributes should have that) in that get_privileged_command could be None (which would be the same as privileged=3DFalse and not None the same as privileged=3DTrue). We don't need startup_command as we're just passing self.path to it, which we already have access to. There is an alternative approach to the path manipulation logic - we don't modify shell_cls.path at all and pass the resulting path (the result of all the operations we do in Node objects) to the constructor (in this case we'd retain the startup_command parameter, although it should be then renamed to path_app or something similar). This doesn't move the logic entirely out of InteractiveShell though, as we still need to access the class attribute. For that reason, I don't see much of a point in pursuing this alternative - we'd basically be just passing one extra unneeded path argument through all the calls. > + self._interactive_session =3D interactive_session > + self._ssh_channel =3D self._interactive_session.invoke_shell() > + self._stdin =3D self._ssh_channel.makefile_stdin("w") > + self._stdout =3D self._ssh_channel.makefile("r") > + self._ssh_channel.settimeout(timeout) > + self._ssh_channel.set_combine_stderr(True) # combines stdout an= d stderr streams > + self._logger =3D logger > + self._timeout =3D timeout > + self._startup_command =3D startup_command > + self._app_args =3D app_args > + self._get_privileged_command =3D _get_privileged_command # type= : ignore We can actually make this work with a static method: 1. Don't store the method in this object. We'd need to pass it to _start_application, but this should be ok as we're only using the method when starting the application - once it's running, we'll never need it again. 2. When we stored the method, we were passing the self parameter when calling self._get_privileged_command, which is why it couldn't be static. When calling without self, static works. Note: the staticmethod decorator must be above the abstractmethod decorator, otherwise the interpreter throws an error. 3. The type can be either Callable[[str], str] or MethodType (from the types module). I'd go with callable here as it's more precise. > + self._privileged =3D privileged > + self._start_application() > + > + def _start_application(self) -> None: > + """Starts a new interactive application based on _startup_comman= d. > + > + This method is often overridden by subclasses as their process f= or > + starting may look different. > + """ > + start_command =3D f"{self._startup_command} {self._app_args}" > + if self._privileged: > + start_command =3D self._get_privileged_command(start_command= ) # type: ignore > + self.send_command(start_command) > + > + def send_command(self, command: str, prompt: str | None =3D None) ->= str: > + """Send a command and get all output before the expected ending = string. > + > + Lines that expect input are not included in the stdout buffer so= they cannot be My IDE spell checker says there should be a comma before so. > + used for expect. For example, if you were prompted to log into s= omething > + with a username and password, you cannot expect "username:" beca= use it won't > + yet be in the stdout buffer. A work around for this could be con= suming an Workaround without the space. > + extra newline character to force the current prompt into the std= out buffer. > + > + Returns: > + All output in the buffer before expected string > + """ > + self._logger.info(f"Sending command {command.strip()}...") > + if prompt is None: > + prompt =3D self._default_prompt > + self._stdin.write(f"{command}{self._command_extra_chars}\n") > + self._stdin.flush() > + out: str =3D "" > + for line in self._stdout: > + out +=3D line > + if prompt in line and not line.rstrip().endswith( > + command.rstrip() > + ): # ignore line that sent command > + break > + self._logger.debug(f"Got output: {out}") > + return out > + > + def close(self) -> None: > + self._stdin.close() > + self._ssh_channel.close() > + > + def __del__(self) -> None: > + self.close() > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py > index 743919820c..724e7328eb 100644 > --- a/dts/framework/test_result.py > +++ b/dts/framework/test_result.py > @@ -213,6 +218,12 @@ def __init__(self, build_target: BuildTargetConfigur= ation): > self.os =3D build_target.os > self.cpu =3D build_target.cpu > self.compiler =3D build_target.compiler > + self.compiler_version =3D None > + self.dpdk_version =3D None > + > + def add_build_target_versions(self, versions: BuildTargetInfo) -> No= ne: Just noticed the name - rename to add_build_target_info to align with everything else. > + self.compiler_version =3D versions.compiler_version > + self.dpdk_version =3D versions.dpdk_version > > def add_test_suite(self, test_suite_name: str) -> TestSuiteResult: > test_suite_result =3D TestSuiteResult(test_suite_name) > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_= model/node.py > index d48fafe65d..39237a95d6 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -7,7 +7,7 @@ > A node is a generic host that DTS connects to and manages. > """ > > -from typing import Any, Callable > +from typing import Any, Callable, Type > > from framework.config import ( > BuildTargetConfiguration, > @@ -15,7 +15,7 @@ > NodeConfiguration, > ) > from framework.logger import DTSLOG, getLogger > -from framework.remote_session import OSSession, create_session > +from framework.remote_session import InteractiveShellType, OSSession, cr= eate_session > from framework.settings import SETTINGS > > from .hw import ( > @@ -23,6 +23,7 @@ > LogicalCoreCount, > LogicalCoreList, > LogicalCoreListFilter, > + VirtualDevice, > lcore_filter, > ) > > @@ -40,6 +41,8 @@ class Node(object): > lcores: list[LogicalCore] > _logger: DTSLOG > _other_sessions: list[OSSession] > + _execution_config: ExecutionConfiguration > + _virtual_devices: list[VirtualDevice] > > def __init__(self, node_config: NodeConfiguration): > self.config =3D node_config > @@ -54,6 +57,7 @@ def __init__(self, node_config: NodeConfiguration): > ).filter() > > self._other_sessions =3D [] > + self._virtual_devices =3D [] This is not a private attribute, so let's drop the starting underscore. > > self._logger.info(f"Created node: {self.name}") > > @@ -64,6 +68,9 @@ def set_up_execution(self, execution_config: ExecutionC= onfiguration) -> None: > """ > self._setup_hugepages() > self._set_up_execution(execution_config) > + self._execution_config =3D execution_config > + for vdev in execution_config.vdevs: > + self._virtual_devices.append(VirtualDevice(vdev)) > > def _set_up_execution(self, execution_config: ExecutionConfiguration= ) -> None: > """ > @@ -77,6 +84,7 @@ def tear_down_execution(self) -> None: > this node is part of concludes. > """ > self._tear_down_execution() > + self._virtual_devices =3D [] I'd prefer this to be above self._tear_down_execution(). If there's a failure in self._tear_down_execution(), the devices won't be removed so removing them before the call is safer. > > def _tear_down_execution(self) -> None: > """ > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smo= ke_tests.py > new file mode 100644 > index 0000000000..83712e75c6 > --- /dev/null > +++ b/dts/tests/TestSuite_smoke_tests.py > @@ -0,0 +1,114 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2023 University of New Hampshire > + > +import re > + > +from framework.config import PortConfig > +from framework.remote_session import 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 -t 60", > + 480, > + verify=3DTrue, > + privileged=3DTrue, > + ) > + > + def test_driver_tests(self) -> None: > + """ > + Test: > + Run the driver-test unit-test suite through meson. > + """ > + vdev_args =3D "" > + for dev in self.sut_node._virtual_devices: > + vdev_args +=3D f"--vdev {dev} " > + vdev_args =3D vdev_args[:-1] > + driver_tests_command =3D ( > + f"meson test -C {self.dpdk_build_dir_path} --suite driver-te= sts" > + ) > + if vdev_args: > + self._logger.info( > + "Running driver tests with the following virtual " > + f"devices: {vdev_args}" > + ) > + driver_tests_command +=3D f' --test-args "{vdev_args}"' > + > + self.sut_node.main_session.send_command( > + driver_tests_command, > + 300, > + verify=3DTrue, > + privileged=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( > + TestPmdShell, privileged=3DTrue > + ) > + dev_list: list[str] =3D [str(x) for x in testpmd_driver.get_devi= ces()] The type hint is not necessary, at least according to my mypy. We can keep it, but we don't do this anywhere else, so I'd go for consistency. > + for nic in self.nics_in_node: > + self.verify( > + nic.pci in dev_list, > + 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 >