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 v8 1/1] dts: add smoke tests
Date: Tue, 18 Jul 2023 11:17:25 -0400 [thread overview]
Message-ID: <CAAA20UTuECqKH67xPXBiXM6g3z-g6bxySnDEXZsqci8_uw3qtQ@mail.gmail.com> (raw)
In-Reply-To: <CAOb5WZbiE6PUwwPd-SLRh41wVPoz2XvHHhz-M7H+DzRWexcPEQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 18049 bytes --]
On Tue, Jul 18, 2023 at 5:41 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> You forgot to add me to the patch :-)
>
Sorry about that! I was just adding everyone who was on the last one but I
must have missed a line.
>
> A few comments below.
>
> On Mon, Jul 17, 2023 at 9:37 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/remote_session/remote/interactive_shell.py
> b/dts/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 = ""
> > + _privileged: bool
> > + _get_privileged_command: Callable[[str], str]
> > + # Allows for app specific extra characters to be appended to
> commands
> > + _command_extra_chars: str = ""
> > + path: PurePath
> > + dpdk_app: bool = 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.linkes@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.
>
>
That's a good call, it probably could do with some more documentation
explaining it and how it works, I've added this.
> > + def __init__(
> > + self,
> > + interactive_session: SSHClient,
> > + logger: DTSLOG,
> > + startup_command: str,
> > + privileged: bool,
> > + _get_privileged_command: Callable[[str], str],
> > + app_args: str = "",
> > + timeout: float = 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=False and not None the same as privileged=True).
>
> 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.
>
>
I agree and prefer just using the path in the start_application. That's
also a good idea to remove the boolean and just make the parameter
nullable, that does clean things up.
> > + self._interactive_session = interactive_session
> > + self._ssh_channel = self._interactive_session.invoke_shell()
> > + self._stdin = self._ssh_channel.makefile_stdin("w")
> > + self._stdout = self._ssh_channel.makefile("r")
> > + self._ssh_channel.settimeout(timeout)
> > + self._ssh_channel.set_combine_stderr(True) # combines stdout
> and stderr streams
> > + self._logger = logger
> > + self._timeout = timeout
> > + self._startup_command = startup_command
> > + self._app_args = app_args
> > + self._get_privileged_command = _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.
>
>
That makes sense, sorry, I thought when we were talking about making it
static it was about just calling it from the class itself. The reason I
thought static wouldn't work for that is because we of course need to know
that we are working with a LinuxSession so we wouldn't know which class to
call upon which is why I just passed the function reference. But, you're
right, this makes more sense and still keeps it static so we don't need to
pass the self into it.
> > + self._privileged = privileged
> > + self._start_application()
> > +
> > + def _start_application(self) -> None:
> > + """Starts a new interactive application based on
> _startup_command.
> > +
> > + This method is often overridden by subclasses as their process
> for
> > + starting may look different.
> > + """
> > + start_command = f"{self._startup_command} {self._app_args}"
> > + if self._privileged:
> > + start_command =
> self._get_privileged_command(start_command) # type: ignore
> > + self.send_command(start_command)
> > +
> > + def send_command(self, command: str, prompt: str | None = 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.
>
Good catch, thank you.
>
> > + used for expect. For example, if you were prompted to log into
> something
> > + with a username and password, you cannot expect "username:"
> because it won't
> > + yet be in the stdout buffer. A work around for this could be
> consuming an
>
> Workaround without the space.
>
Oops, thank you.
>
> > + extra newline character to force the current prompt into the
> stdout buffer.
> > +
> > + Returns:
> > + All output in the buffer before expected string
> > + """
> > + self._logger.info(f"Sending command {command.strip()}...")
> > + if prompt is None:
> > + prompt = self._default_prompt
> > + self._stdin.write(f"{command}{self._command_extra_chars}\n")
> > + self._stdin.flush()
> > + out: str = ""
> > + for line in self._stdout:
> > + out += 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()
>
> <snip>
>
> > 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
> <snip>
> > @@ -213,6 +218,12 @@ def __init__(self, build_target:
> BuildTargetConfiguration):
> > self.os = build_target.os
> > self.cpu = build_target.cpu
> > self.compiler = build_target.compiler
> > + self.compiler_version = None
> > + self.dpdk_version = None
> > +
> > + def add_build_target_versions(self, versions: BuildTargetInfo) ->
> None:
>
> Just noticed the name - rename to add_build_target_info to align with
> everything else.
>
I guess this one must have slipped through before after I changed the
names, I'll change it.
>
> > + self.compiler_version = versions.compiler_version
> > + self.dpdk_version = versions.dpdk_version
> >
> > def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
> > test_suite_result = 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,
> create_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 = node_config
> > @@ -54,6 +57,7 @@ def __init__(self, node_config: NodeConfiguration):
> > ).filter()
> >
> > self._other_sessions = []
> > + self._virtual_devices = []
>
> This is not a private attribute, so let's drop the starting underscore.
>
>
I wasn't sure whether we would want it to be private or not but you're
right, there isn't much reason to have it as such so I'll remove it.
> >
> > self._logger.info(f"Created node: {self.name}")
> >
> > @@ -64,6 +68,9 @@ def set_up_execution(self, execution_config:
> ExecutionConfiguration) -> None:
> > """
> > self._setup_hugepages()
> > self._set_up_execution(execution_config)
> > + self._execution_config = 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 = []
>
> 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.
>
>
That's a good point, I didn't think about how the failure of this method
would cause this to never happen. The reason I put it below originally was
in case the virtual devices were needed in the teardown stage but I agree
that what you mentioned is also important to consider and putting it above
is safer.
> >
> > def _tear_down_execution(self) -> None:
> > """
>
> <snip>
>
> > diff --git a/dts/tests/TestSuite_smoke_tests.py
> b/dts/tests/TestSuite_smoke_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 = 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 -t 60",
> > + 480,
> > + verify=True,
> > + privileged=True,
> > + )
> > +
> > + def test_driver_tests(self) -> None:
> > + """
> > + Test:
> > + Run the driver-test unit-test suite through meson.
> > + """
> > + vdev_args = ""
> > + for dev in self.sut_node._virtual_devices:
> > + vdev_args += f"--vdev {dev} "
> > + vdev_args = vdev_args[:-1]
> > + driver_tests_command = (
> > + f"meson test -C {self.dpdk_build_dir_path} --suite
> driver-tests"
> > + )
> > + if vdev_args:
> > + self._logger.info(
> > + "Running driver tests with the following virtual "
> > + f"devices: {vdev_args}"
> > + )
> > + driver_tests_command += f' --test-args "{vdev_args}"'
> > +
> > + self.sut_node.main_session.send_command(
> > + driver_tests_command,
> > + 300,
> > + verify=True,
> > + privileged=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(
> > + TestPmdShell, privileged=True
> > + )
> > + dev_list: list[str] = [str(x) for x in
> testpmd_driver.get_devices()]
>
> 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.
>
Good point, I'll remove it.
>
> > + 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
> 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
> >
>
[-- Attachment #2: Type: text/html, Size: 24895 bytes --]
next prev parent reply other threads:[~2023-07-18 15:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-17 19:33 [PATCH v8 0/1] Add DTS " jspewock
2023-07-17 19:33 ` [PATCH v8 1/1] dts: add " jspewock
2023-07-18 9:41 ` Juraj Linkeš
2023-07-18 15:17 ` Jeremy Spewock [this message]
2023-07-18 12:08 ` Juraj Linkeš
2023-07-18 15:18 ` Jeremy Spewock
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=CAAA20UTuECqKH67xPXBiXM6g3z-g6bxySnDEXZsqci8_uw3qtQ@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).