From: Jeremy Spewock <jspewock@iol.unh.edu>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net,
lijuan.tu@intel.com, wathsala.vithanage@arm.com,
probb@iol.unh.edu, dev@dpdk.org
Subject: Re: [PATCH v7 1/1] dts: add smoke tests
Date: Mon, 17 Jul 2023 15:30:57 -0400 [thread overview]
Message-ID: <CAAA20UTz1dw7mqe1GWAizH6dAXdMayzpzKuXn6S8WkN7VRdE-Q@mail.gmail.com> (raw)
In-Reply-To: <CAOb5WZarzZh=v7_MwugQ3+hkjnDUYLUYQ0rLAnqmu50gLrEPHQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 17058 bytes --]
On Mon, Jul 17, 2023 at 10:50 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> I found additional things while working with the interactive shell code.
>
> On Thu, Jul 13, 2023 at 6:54 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > 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 <jspewock@iol.unh.edu>
> > ---
>
> <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._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.
[-- Attachment #2: Type: text/html, Size: 21553 bytes --]
next prev parent reply other threads:[~2023-07-17 19:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-13 16:53 [PATCH v7 0/1] Add DTS " jspewock
2023-07-13 16:53 ` [PATCH v7 1/1] dts: add " jspewock
2023-07-13 18:48 ` Jeremy Spewock
2023-07-14 7:34 ` Juraj Linkeš
2023-07-14 14:47 ` Patrick Robb
2023-07-14 15:29 ` Juraj Linkeš
2023-07-14 23:25 ` Jeremy Spewock
2023-07-17 11:11 ` Juraj Linkeš
2023-07-17 14:50 ` Juraj Linkeš
2023-07-17 19:30 ` Jeremy Spewock [this message]
2023-07-18 9:55 ` Juraj Linkeš
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAA20UTz1dw7mqe1GWAizH6dAXdMayzpzKuXn6S8WkN7VRdE-Q@mail.gmail.com \
--to=jspewock@iol.unh.edu \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=dev@dpdk.org \
--cc=juraj.linkes@pantheon.tech \
--cc=lijuan.tu@intel.com \
--cc=probb@iol.unh.edu \
--cc=thomas@monjalon.net \
--cc=wathsala.vithanage@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).