From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
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
Subject: Re: [PATCH v7 1/1] dts: add smoke tests
Date: Mon, 17 Jul 2023 16:50:37 +0200 [thread overview]
Message-ID: <CAOb5WZarzZh=v7_MwugQ3+hkjnDUYLUYQ0rLAnqmu50gLrEPHQ@mail.gmail.com> (raw)
In-Reply-To: <20230713165347.21997-3-jspewock@iol.unh.edu>
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
>
next prev parent reply other threads:[~2023-07-17 14:50 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š [this message]
2023-07-17 19:30 ` Jeremy Spewock
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='CAOb5WZarzZh=v7_MwugQ3+hkjnDUYLUYQ0rLAnqmu50gLrEPHQ@mail.gmail.com' \
--to=juraj.linkes@pantheon.tech \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=dev@dpdk.org \
--cc=jspewock@iol.unh.edu \
--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).