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 v1 1/2] dts: add smoke tests
Date: Thu, 6 Jul 2023 16:53:48 +0200 [thread overview]
Message-ID: <CAOb5WZbGDJJn+0AR0840O8vTfxe6WXZFFFGySU_RL+=ZckTq3g@mail.gmail.com> (raw)
In-Reply-To: <20230615201318.13359-3-jspewock@iol.unh.edu>
[-- Attachment #1: Type: text/plain, Size: 67316 bytes --]
There are mypy errors related to paramiko:
framework/remote_session/remote/interactive_remote_session.py:8: error:
Library stubs not installed for "paramiko" (or incompatible with Python
3.10)
framework/remote_session/remote/interactive_remote_session.py:9: error:
Library stubs not installed for "paramiko.ssh_exception" (or incompatible
with Python 3.10)
We do this for pexpect:
import pexpect # type: ignore
from pexpect import pxssh # type: ignore
We should do the same for paramiko.
There are also some changes which may require coordination. I'm not sure
how we'd do that, the best may be to align that off-list.
More inline.
On Thu, Jun 15, 2023 at 10:13 PM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Adds a new test suite as well as configuration changes needed for running
> smoke tests that verify general environment 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>
> ---
> dts/conf.yaml | 13 ++
> dts/framework/config/__init__.py | 114 +++++++++++++--
> dts/framework/config/conf_yaml_schema.json | 135 +++++++++++++++++-
> dts/framework/dts.py | 101 ++++++++++---
> dts/framework/exception.py | 12 ++
> dts/framework/remote_session/__init__.py | 10 +-
> dts/framework/remote_session/os_session.py | 34 ++++-
> dts/framework/remote_session/posix_session.py | 30 ++++
> .../remote_session/remote/__init__.py | 12 ++
> .../remote/interactive_remote_session.py | 113 +++++++++++++++
> .../remote/interactive_shell.py | 98 +++++++++++++
> .../remote_session/remote/testpmd_shell.py | 58 ++++++++
> dts/framework/test_result.py | 38 ++++-
> dts/framework/test_suite.py | 31 +++-
> dts/framework/testbed_model/node.py | 2 +
> dts/framework/testbed_model/sut_node.py | 110 +++++++++++++-
> dts/tests/TestSuite_smoke_tests.py | 101 +++++++++++++
> 17 files changed, 962 insertions(+), 50 deletions(-)
> create mode 100644
dts/framework/remote_session/remote/interactive_remote_session.py
> create mode 100644
dts/framework/remote_session/remote/interactive_shell.py
> create mode 100644 dts/framework/remote_session/remote/testpmd_shell.py
> create mode 100644 dts/tests/TestSuite_smoke_tests.py
>
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index a9bd8a3e..03fd57e1 100644
> --- a/dts/conf.yaml
> +++ b/dts/conf.yaml
> @@ -10,6 +10,8 @@ executions:
> compiler_wrapper: ccache
> perf: false
> func: true
> + vdevs: #names of virtual devices to be used for testing
Vdevs are optional, let's mention that in the comment as with the hugepages
config.
>
> + - "crypto_openssl"
How (or where) is this configured in the original DTS? The vdevs are only
going to be used on SUTs, so it may make sense to put them to node config.
On the other hand, each execution using the SUT may use different vdevs, so
this is probably the best place. Maybe we could move it under
system_under_test in execution? That would make more sense I think - it
would be an addition to the existing SUT configuration (as in here's where
we augment SUT config specific to the execution).
>
> test_suites:
> - hello_world
> system_under_test: "SUT 1"
> @@ -20,6 +22,17 @@ nodes:
> arch: x86_64
> os: linux
> lcores: ""
> + ports:
> + - pci: "0000:00:08.0"
> + dpdk_os_driver: vfio-pci
We could find a better name for this driver (and maybe even the regular os
driver). It's the OS driver that dpdk uses and there could be better names
- os_dpdk_driver, os_driver_dpdk, os_driver_for_dpdk. Which one do you like
the most? Or maybe some other name?
In either case we probably want to add a comment to both drivers that
explain what we mean here. The name doesn't need to be perfect then. :-)
I'll make sure to align the change with my patch if we change this.
>
> + os_driver: i40e
> + peer_node: "TG 1"
> + peer_pci: "0000:00:08.0"
> + - pci: "0000:00:08.1"
> + dpdk_os_driver: vfio-pci
> + os_driver: i40e
> + peer_node: "TG 1"
> + peer_pci: "0000:00:08.1"
> use_first_core: false
> memory_channels: 4
> hugepages: # optional; if removed, will use system hugepage
configuration
> diff --git a/dts/framework/config/__init__.py
b/dts/framework/config/__init__.py
> index ebb0823f..74e5158d 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -72,6 +72,21 @@ class HugepageConfiguration:
> force_first_numa: bool
>
>
> +@dataclass(slots=True, frozen=True)
> +class PortConfig:
> + id: int
I removed id in my latest local version as I didn't see a real use for it.
>
> + node: str
> + pci: str
> + dpdk_os_driver: str
> + os_driver: str
> + peer_node: str
> + peer_pci: str
> +
> + @staticmethod
> + def from_dict(id: int, node: str, d: dict) -> "PortConfig":
> + return PortConfig(id=id, node=node, **d)
This would need to be changed if we remove the id.
> +
> +
> @dataclass(slots=True, frozen=True)
> class NodeConfiguration:
> name: str
> @@ -84,6 +99,7 @@ class NodeConfiguration:
> use_first_core: bool
> memory_channels: int
> hugepages: HugepageConfiguration | None
> + ports: list[PortConfig]
>
> @staticmethod
> def from_dict(d: dict) -> "NodeConfiguration":
> @@ -92,18 +108,46 @@ def from_dict(d: dict) -> "NodeConfiguration":
> if "force_first_numa" not in hugepage_config:
> hugepage_config["force_first_numa"] = False
> hugepage_config = HugepageConfiguration(**hugepage_config)
> + common_config = {
> + "name": d["name"],
> + "hostname": d["hostname"],
> + "user": d["user"],
> + "password": d.get("password"),
> + "arch": Architecture(d["arch"]),
> + "os": OS(d["os"]),
> + "lcores": d.get("lcores", "1"),
> + "use_first_core": d.get("use_first_core", False),
> + "memory_channels": d.get("memory_channels", 1),
> + "hugepages": hugepage_config,
> + "ports": [
> + PortConfig.from_dict(i, d["name"], port)
> + for i, port in enumerate(d["ports"])
> + ],
This also needs to be modified when removing PortConfig.id:
"ports": [PortConfig.from_dict(d["name"], port) for port in d["ports"]],
>
> + }
> +
> + return NodeConfiguration(**common_config)
> +
> +
> +@dataclass(slots=True)
> +class NodeVersionInfo:
This looks like we could use it for all sorts of things in the future, so
I'd just call it NodeInfo.
>
> + """Class to hold important versions within the node.
> +
> + This class, unlike the NodeConfiguration class, cannot be generated
at the start.
> + This is because we need to initialize a connection with the node
before we can
> + collet the information needed in this class. Therefore, it cannot be
a part of
Typo: collect
> + the configuration class above.
> + """
>
> - return NodeConfiguration(
> - name=d["name"],
> - hostname=d["hostname"],
> - user=d["user"],
> - password=d.get("password"),
> - arch=Architecture(d["arch"]),
> - os=OS(d["os"]),
> - lcores=d.get("lcores", "1"),
> - use_first_core=d.get("use_first_core", False),
> - memory_channels=d.get("memory_channels", 1),
> - hugepages=hugepage_config,
> + os_name: str
> + os_version: str
> + kernel_version: str
> +
> + @staticmethod
> + def from_dict(d: dict):
> + return NodeVersionInfo(
> + os_name=d["os_name"],
> + os_version=d["os_version"],
> + kernel_version=d["kernel_version"],
> )
>
>
> @@ -128,6 +172,24 @@ def from_dict(d: dict) -> "BuildTargetConfiguration":
> )
>
>
> +@dataclass(slots=True)
> +class BuildTargetVersionInfo:
> + """Class to hold important versions within the build target.
> +
> + This is very similar to the NodeVersionInfo class, it just instead
holds information
> + for the build target.
> + """
> +
> + dpdk_version: str
> + compiler_version: str
> +
> + @staticmethod
> + def from_dict(d: dict):
> + return BuildTargetVersionInfo(
> + dpdk_version=d["dpdk_version"],
compiler_version=d["compiler_version"]
> + )
> +
Do you expect we'll be storing more fields than just these two (if so, then
we can again rename the class to BuildTargetInfo)? If it's just these two,
the extra class may not help that much (we could pass around a tuple).
On the other hand, there's not really a reason to not have this class. I
could see us adding to it and I like BuildTargetInfo because of that.
>
> +
> class TestSuiteConfigDict(TypedDict):
> suite: str
> cases: list[str]
> @@ -157,6 +219,8 @@ class ExecutionConfiguration:
> func: bool
> test_suites: list[TestSuiteConfig]
> system_under_test: NodeConfiguration
> + vdevs: list[str]
> + skip_smoke_tests: bool
This should be added to conf.yaml to demonstrate the availability of the
option.
>
>
> @staticmethod
> def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration":
> @@ -167,14 +231,18 @@ def from_dict(d: dict, node_map: dict) ->
"ExecutionConfiguration":
> map(TestSuiteConfig.from_dict, d["test_suites"])
> )
> sut_name = d["system_under_test"]
> + list_of_vdevs = d["vdevs"]
> + skip_smoke_tests = d["skip_smoke_tests"] if "skip_smoke_tests"
in d else False
Just as with hugepages, we could skip the else branch if we set the default
of skip_smoke_tests to False. I'm not sure which is the better approach.
>
> assert sut_name in node_map, f"Unknown SUT {sut_name} in
execution {d}"
>
> return ExecutionConfiguration(
> build_targets=build_targets,
> perf=d["perf"],
> func=d["func"],
> + skip_smoke_tests=skip_smoke_tests,
> test_suites=test_suites,
> system_under_test=node_map[sut_name],
> + vdevs=list_of_vdevs,
> )
>
>
> @@ -221,3 +289,27 @@ def load_config() -> Configuration:
>
>
> CONFIGURATION = load_config()
> +
> +
> +@unique
> +class InteractiveApp(Enum):
> + """An enum that represents different supported interactive
applications
> +
> + The values in this enum must all be set to objects that have a key
called
> + "default_path" where "default_path" represents an array for the path
to the
> + application. This default path will be passed into the handler class
for the
> + application so that it can start the application. For every key
other than
> + the default shell option, the path will be appended to the path to
the DPDK
> + build directory for the current SUT node.
> + """
> +
> + shell = {"default_path": [""]}
> + testpmd = {"default_path": ["app", "dpdk-testpmd"]}
The stored path could be a PurePath object. It'll be instantiated as an
OS-specific path, but main_session.join_remote_path will convert it
properly.
> +
> + def get_path(self):
> + """A method for getting the default paths of an application
> +
> + Returns:
> + String array that represents an OS agnostic path to the
application.
> + """
> + return self.value["default_path"]
> diff --git a/dts/framework/config/conf_yaml_schema.json
b/dts/framework/config/conf_yaml_schema.json
> index ca2d4a1e..3f7c301a 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
> @@ -6,6 +6,76 @@
> "type": "string",
> "description": "A unique identifier for a node"
> },
> + "NIC": {
> + "type": "string",
> + "enum": [
> + "ALL",
> + "ConnectX3_MT4103",
> + "ConnectX4_LX_MT4117",
> + "ConnectX4_MT4115",
> + "ConnectX5_MT4119",
> + "ConnectX5_MT4121",
> + "I40E_10G-10G_BASE_T_BC",
> + "I40E_10G-10G_BASE_T_X722",
> + "I40E_10G-SFP_X722",
> + "I40E_10G-SFP_XL710",
> + "I40E_10G-X722_A0",
> + "I40E_1G-1G_BASE_T_X722",
> + "I40E_25G-25G_SFP28",
> + "I40E_40G-QSFP_A",
> + "I40E_40G-QSFP_B",
> + "IAVF-ADAPTIVE_VF",
> + "IAVF-VF",
> + "IAVF_10G-X722_VF",
> + "ICE_100G-E810C_QSFP",
> + "ICE_25G-E810C_SFP",
> + "ICE_25G-E810_XXV_SFP",
> + "IGB-I350_VF",
> + "IGB_1G-82540EM",
> + "IGB_1G-82545EM_COPPER",
> + "IGB_1G-82571EB_COPPER",
> + "IGB_1G-82574L",
> + "IGB_1G-82576",
> + "IGB_1G-82576_QUAD_COPPER",
> + "IGB_1G-82576_QUAD_COPPER_ET2",
> + "IGB_1G-82580_COPPER",
> + "IGB_1G-I210_COPPER",
> + "IGB_1G-I350_COPPER",
> + "IGB_1G-I354_SGMII",
> + "IGB_1G-PCH_LPTLP_I218_LM",
> + "IGB_1G-PCH_LPTLP_I218_V",
> + "IGB_1G-PCH_LPT_I217_LM",
> + "IGB_1G-PCH_LPT_I217_V",
> + "IGB_2.5G-I354_BACKPLANE_2_5GBPS",
> + "IGC-I225_LM",
> + "IGC-I226_LM",
> + "IXGBE_10G-82599_SFP",
> + "IXGBE_10G-82599_SFP_SF_QP",
> + "IXGBE_10G-82599_T3_LOM",
> + "IXGBE_10G-82599_VF",
> + "IXGBE_10G-X540T",
> + "IXGBE_10G-X540_VF",
> + "IXGBE_10G-X550EM_A_SFP",
> + "IXGBE_10G-X550EM_X_10G_T",
> + "IXGBE_10G-X550EM_X_SFP",
> + "IXGBE_10G-X550EM_X_VF",
> + "IXGBE_10G-X550T",
> + "IXGBE_10G-X550_VF",
> + "brcm_57414",
> + "brcm_P2100G",
> + "cavium_0011",
> + "cavium_a034",
> + "cavium_a063",
> + "cavium_a064",
> + "fastlinq_ql41000",
> + "fastlinq_ql41000_vf",
> + "fastlinq_ql45000",
> + "fastlinq_ql45000_vf",
> + "hi1822",
> + "virtio"
> + ]
> + },
> +
All these NICs may be overkill, do we want to trim them?
>
> "ARCH": {
> "type": "string",
> "enum": [
> @@ -94,10 +164,24 @@
> "amount"
> ]
> },
> + "pci_address": {
> + "type": "string",
> + "pattern":
"^[\\da-fA-F]{4}:[\\da-fA-F]{2}:[\\da-fA-F]{2}.\\d:?\\w*$"
> + },
> + "port_peer_address": {
> + "description": "Peer is a TRex port, and IXIA port or a PCI
address",
> + "oneOf": [
> + {
> + "description": "PCI peer port",
> + "$ref": "#/definitions/pci_address"
> + }
> + ]
> + },
> "test_suite": {
> "type": "string",
> "enum": [
> - "hello_world"
> + "hello_world",
> + "smoke_tests"
Is there a reason to provide both this and the execution.skip_smoke_tests
option? If we provide both, we may need to think about what happens when
both are present. I'd say it's best for everyone (users and devs) to have
only one option and I'd lean towards execution.skip_smoke_tests.
> ]
> },
> "test_target": {
> @@ -165,6 +249,44 @@
> },
> "hugepages": {
> "$ref": "#/definitions/hugepages"
> + },
> + "ports": {
> + "type": "array",
> + "items": {
> + "type": "object",
> + "description": "Each port should be described on both
sides of the connection. This makes configuration slightly more verbose but
greatly simplifies implementation. If there are an inconsistencies, then
DTS will not run until that issue is fixed. An example inconsistency would
be port 1, node 1 says it is connected to port 1, node 2, but port 1, node
2 says it is connected to port 2, node 1.",
> + "properties": {
> + "pci": {
> + "$ref": "#/definitions/pci_address",
> + "description": "The local PCI address of the port"
> + },
> + "dpdk_os_driver": {
> + "type": "string",
> + "description": "The driver that the kernel should bind
this device to for DPDK to use it. (ex: vfio-pci)"
> + },
> + "os_driver": {
> + "type": "string",
> + "description": "The driver normally used by this port
(ex: i40e)"
> + },
> + "peer_node": {
> + "type": "string",
> + "description": "The name of the node the peer port is
on"
> + },
> + "peer_pci": {
> + "$ref": "#/definitions/pci_address",
> + "description": "The PCI address of the peer port"
> + }
> + },
> + "additionalProperties": false,
> + "required": [
> + "pci",
> + "dpdk_os_driver",
> + "os_driver",
> + "peer_node",
> + "peer_pci"
> + ]
> + },
> + "minimum": 1
> }
> },
> "additionalProperties": false,
> @@ -211,6 +333,17 @@
> ]
> }
> },
> + "vdevs": {
> + "description": "Names of vdevs to be used in execution",
> + "type": "array",
> + "items": {
> + "type":"string"
Missing space before "string".
> + }
> + },
> + "skip_smoke_tests": {
> + "description": "Optional field that allows you to skip smoke
testing",
> + "type":"boolean"
Also the missing space.
> + },
> "system_under_test": {
> "$ref": "#/definitions/node_name"
> }
> diff --git a/dts/framework/dts.py b/dts/framework/dts.py
> index 05022845..f76c9ceb 100644
> --- a/dts/framework/dts.py
> +++ b/dts/framework/dts.py
> @@ -5,7 +5,13 @@
>
> import sys
>
> -from .config import CONFIGURATION, BuildTargetConfiguration,
ExecutionConfiguration
> +from .config import (
> + CONFIGURATION,
> + BuildTargetConfiguration,
> + ExecutionConfiguration,
> + TestSuiteConfig,
> +)
> +from .exception import BlockingTestSuiteError
> from .logger import DTSLOG, getLogger
> from .test_result import BuildTargetResult, DTSResult, ExecutionResult,
Result
> from .test_suite import get_test_suites
> @@ -82,7 +88,9 @@ def _run_execution(
> running all build targets in the given execution.
> """
> dts_logger.info(f"Running execution with SUT '{
execution.system_under_test.name}'.")
> - execution_result = result.add_execution(sut_node.config)
> + execution_result = result.add_execution(
> + sut_node.config, sut_node.get_node_versions()
> + )
>
> try:
> sut_node.set_up_execution(execution)
> @@ -118,14 +126,17 @@ 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_versions()
> + )
> 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:
> @@ -136,7 +147,46 @@ def _run_build_target(
> build_target_result.update_teardown(Result.FAIL, e)
>
>
> -def _run_suites(
> +def _run_single_suite(
> + sut_node: SutNode,
> + execution: ExecutionConfiguration,
> + build_target_result: BuildTargetResult,
> + test_suite_config: TestSuiteConfig,
> +) -> None:
> + """Runs a single test suite.
> +
> + Args:
> + sut_node: Node to run tests on.
> + execution: Execution the test case belongs to.
> + build_target_result: Build target configuration test case is run
on
> + test_suite_config: Test suite configuration
> +
> + Raises:
> + BlockingTestSuiteError: If a test suite that was marked as
blocking fails.
> + """
> + try:
> + full_suite_path =
f"tests.TestSuite_{test_suite_config.test_suite}"
> + test_suite_classes = get_test_suites(full_suite_path)
> + suites_str = ", ".join((x.__name__ for x in test_suite_classes))
> + dts_logger.debug(f"Found test suites '{suites_str}' in
'{full_suite_path}'.")
> + except Exception as e:
> + dts_logger.exception("An error occurred when searching for test
suites.")
> + result.update_setup(Result.ERROR, e)
> +
> + else:
> + for test_suite_class in test_suite_classes:
> + test_suite = test_suite_class(
> + sut_node,
> + test_suite_config.test_cases,
> + execution.func,
> + build_target_result,
> + sut_node._build_target_config,
> + result,
> + )
> + test_suite.run()
> +
> +
> +def _run_all_suites(
> sut_node: SutNode,
> execution: ExecutionConfiguration,
> build_target_result: BuildTargetResult,
> @@ -146,27 +196,32 @@ def _run_suites(
> with possibly only a subset of test cases.
> If no subset is specified, run all test cases.
> """
> + end_build_target = False
> + smoke_test_config = TestSuiteConfig.from_dict("smoke_tests")
> + if not execution.skip_smoke_tests:
> + try:
> + _run_single_suite(
> + sut_node, execution, build_target_result,
smoke_test_config
> + )
> + except BlockingTestSuiteError as e:
> + dts_logger.exception("Smoke tests failed, stopping build
target execution.")
> + result.add_error(e)
> + return
> for test_suite_config in execution.test_suites:
> try:
> - full_suite_path =
f"tests.TestSuite_{test_suite_config.test_suite}"
> - test_suite_classes = get_test_suites(full_suite_path)
> - suites_str = ", ".join((x.__name__ for x in
test_suite_classes))
> - dts_logger.debug(
> - f"Found test suites '{suites_str}' in
'{full_suite_path}'."
> + _run_single_suite(
> + sut_node, execution, build_target_result,
test_suite_config
> )
> - except Exception as e:
> - dts_logger.exception("An error occurred when searching for
test suites.")
> - result.update_setup(Result.ERROR, e)
> -
> - else:
> - for test_suite_class in test_suite_classes:
> - test_suite = test_suite_class(
> - sut_node,
> - test_suite_config.test_cases,
> - execution.func,
> - build_target_result,
> - )
> - test_suite.run()
> + except BlockingTestSuiteError as e:
> + dts_logger.exception(
> + f"An error occurred within
{test_suite_config.test_suite}. "
> + "Skipping build target..."
> + )
> + result.add_error(e)
> + end_build_target = True
> + # if a blocking test failed and we need to bail out of suite
executions
> + if end_build_target:
> + break
>
This doesn't look great to me. I think we could just add a
list[TestSuiteConfig] parameter to _run_suites. We could then just pass a
list with/without smoke TestSuiteConfig, something like:
test_suites = execution.test_suites
if not execution.skip_smoke_tests:
test_suites[:0] = [TestSuiteConfig.from_dict("smoke_tests")]
_run_suites(sut_node, execution, build_target_result, test_suites)
A note on method ordering:
_run_single_suite before _run_all_suites was confusing to me, as the
methods are used in the reverse order - _run_build_target
calls _run_all_suites which only then calls _run_single_suite. I'd like the
methods to be ordered in the order they're used in the code.
>
> def _exit_dts() -> None:
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> index ca353d98..dfb12df4 100644
> --- a/dts/framework/exception.py
> +++ b/dts/framework/exception.py
> @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum):
> SSH_ERR = 4
> DPDK_BUILD_ERR = 10
> TESTCASE_VERIFY_ERR = 20
> + BLOCKING_TESTSUITE_ERR = 25
>
>
> class DTSError(Exception):
> @@ -144,3 +145,14 @@ def __init__(self, value: str):
>
> def __str__(self) -> str:
> return repr(self.value)
> +
> +
> +class BlockingTestSuiteError(DTSError):
> + suite_name: str
> + severity: ClassVar[ErrorSeverity] =
ErrorSeverity.BLOCKING_TESTSUITE_ERR
> +
> + def __init__(self, suite_name: str) -> None:
> + self.suite_name = suite_name
> +
> + def __str__(self) -> str:
> + return f"Blocking suite {self.suite_name} failed."
> diff --git a/dts/framework/remote_session/__init__.py
b/dts/framework/remote_session/__init__.py
> index ee221503..04342b8d 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022-2023 University of New Hampshire
Should the year be just 2023? I'm not sure about these copyright statements.
>
> """
> The package provides modules for managing remote connections to a remote
host (node),
> @@ -17,7 +18,14 @@
>
> from .linux_session import LinuxSession
> from .os_session import OSSession
> -from .remote import CommandResult, RemoteSession, SSHSession
> +from .remote import (
> + CommandResult,
> + InteractiveRemoteSession,
> + InteractiveShell,
> + RemoteSession,
> + SSHSession,
> + TestPmdShell,
> +)
>
>
> def create_session(
> diff --git a/dts/framework/remote_session/os_session.py
b/dts/framework/remote_session/os_session.py
> index 4c48ae25..f5f53923 100644
> --- a/dts/framework/remote_session/os_session.py
> +++ b/dts/framework/remote_session/os_session.py
> @@ -12,7 +12,13 @@
> from framework.testbed_model import LogicalCore
> from framework.utils import EnvVarsDict, MesonArgs
>
> -from .remote import CommandResult, RemoteSession, create_remote_session
> +from .remote import (
> + CommandResult,
> + InteractiveRemoteSession,
> + RemoteSession,
> + create_interactive_session,
> + create_remote_session,
> +)
>
>
> class OSSession(ABC):
> @@ -26,6 +32,7 @@ class OSSession(ABC):
> name: str
> _logger: DTSLOG
> remote_session: RemoteSession
> + interactive_session: InteractiveRemoteSession
>
> def __init__(
> self,
> @@ -37,6 +44,7 @@ def __init__(
> self.name = name
> self._logger = logger
> self.remote_session = create_remote_session(node_config, name,
logger)
> + self.interactive_session =
create_interactive_session(node_config, name, logger)
We may not want to create the interactive session at this point. This does
create a connection to the node which we don't want (it is extra time
consumed) when creating an extra session on top of the main_session (with
Node.create_session). I think we could move this to
OSSession.create_interactive_shell. More below.
>
> def close(self, force: bool = False) -> None:
> """
> @@ -173,3 +181,27 @@ def setup_hugepages(self, hugepage_amount: int,
force_first_numa: bool) -> None:
> if needed and mount the hugepages if needed.
> If force_first_numa is True, configure hugepages just on the
first socket.
> """
> +
> + @abstractmethod
> + def get_compiler_version(self, compiler_name: str) -> str:
> + """
> + Get installed version of compiler used for DPDK
> + """
> +
> + @abstractmethod
> + def get_os_name(self) -> str:
> + """
> + Get name of OS for the node
> + """
> +
> + @abstractmethod
> + def get_os_version(self) -> str:
> + """
> + Get version of OS for the node
> + """
> +
> + @abstractmethod
> + def get_kernel_version(self) -> str:
> + """
> + Get kernel version for the node
> + """
These could be merged into one method (returning NodeVersionInfo), is there
a reason to separate them? We could get some performance improvements with
one method (e.g. getting /etc/os-release just once and then process locally
or we could use a script we execute remotely to have just one remote call,
but that's something to think about in the future).
> diff --git a/dts/framework/remote_session/posix_session.py
b/dts/framework/remote_session/posix_session.py
> index d38062e8..dc6689f5 100644
> --- a/dts/framework/remote_session/posix_session.py
> +++ b/dts/framework/remote_session/posix_session.py
> @@ -219,3 +219,33 @@ def _remove_dpdk_runtime_dirs(
>
> def get_dpdk_file_prefix(self, dpdk_prefix) -> str:
> return ""
> +
> + def get_compiler_version(self, compiler_name: str) -> str:
> + match compiler_name:
> + case "gcc":
> + return self.send_command(f"{compiler_name} --version",
60).stdout.split(
> + "\n"
> + )[0]
> + case "clang":
> + return self.send_command(f"{compiler_name} --version",
60).stdout.split(
> + "\n"
> + )[0]
> + case "msvc":
> + return self.send_command("cl", 60).stdout
> + case "icc":
> + return self.send_command(f"{compiler_name} -V",
60).stdout
> + case _:
> + raise ValueError(f"Unknown compiler {compiler_name}")
> +
> + def get_os_name(self) -> str:
> + return self.send_command(
> + "awk -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60
> + ).stdout
> +
> + def get_os_version(self) -> str:
> + return self.send_command(
> + "awk -F= '$1==\"VERSION\" {print $2}' /etc/os-release", 60,
True
> + ).stdout
> +
> + def get_kernel_version(self) -> str:
> + return self.send_command("uname -r", 60).stdout
These 60 second timeouts seem high to me. All of these commands should
execute quickly so the default timeout seems fine to me.
> diff --git a/dts/framework/remote_session/remote/__init__.py
b/dts/framework/remote_session/remote/__init__.py
> index 8a151221..ef7b4851 100644
> --- a/dts/framework/remote_session/remote/__init__.py
> +++ b/dts/framework/remote_session/remote/__init__.py
> @@ -1,16 +1,28 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022-2023 University of New Hampshire
>
> # pylama:ignore=W0611
>
> +from paramiko import AutoAddPolicy, SSHClient
> +
> 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 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:
> + return InteractiveRemoteSession(node_config, logger)
> diff --git
a/dts/framework/remote_session/remote/interactive_remote_session.py
b/dts/framework/remote_session/remote/interactive_remote_session.py
> new file mode 100644
> index 00000000..4e88308a
> --- /dev/null
> +++ b/dts/framework/remote_session/remote/interactive_remote_session.py
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +import socket
> +import traceback
> +from pathlib import PurePath
> +
> +from paramiko import AutoAddPolicy, SSHClient, Transport
> +from paramiko.ssh_exception import (
> + AuthenticationException,
> + BadHostKeyException,
> + NoValidConnectionsError,
> + SSHException,
> +)
> +
> +from framework.config import InteractiveApp, NodeConfiguration
> +from framework.exception import SSHConnectionError
> +from framework.logger import DTSLOG
> +from framework.settings import SETTINGS
> +
> +from .interactive_shell import InteractiveShell
> +from .testpmd_shell import TestPmdShell
> +
> +
> +class InteractiveRemoteSession:
> + hostname: str
> + ip: str
> + port: int
> + username: str
> + password: str
> + _logger: DTSLOG
> + _node_config: NodeConfiguration
> + session: SSHClient
> + _transport: Transport | None
> +
> + def __init__(self, node_config: NodeConfiguration, _logger: DTSLOG)
-> None:
> + self._node_config = node_config
> + self._logger = _logger
> + self.hostname = node_config.hostname
> + self.username = node_config.user
> + self.password = node_config.password if node_config.password
else ""
> + port = 22
framework/remote_session/remote/interactive_remote_session.py:45: error:
Incompatible types in assignment (expression has type "str", variable has
type "int")
We can address this by making 22 a string (port = "22").
> + self.ip = node_config.hostname
> + if ":" in node_config.hostname:
> + self.ip, port = node_config.hostname.split(":")
> + self.port = int(port)
> + self._logger.info(
> + f"Initializing interactive connection for {self.username}@
{self.hostname}"
> + )
> + self._connect()
> + self._logger.info(
> + f"Interactive connection successful for {self.username}@
{self.hostname}"
> + )
> +
> + def _connect(self) -> None:
> + client = SSHClient()
> + client.set_missing_host_key_policy(AutoAddPolicy)
> + self.session = client
> + retry_attempts = 10
> + for retry_attempt in range(retry_attempts):
> + try:
> + client.connect(
> + self.ip,
> + username=self.username,
> + port=self.port,
> + password=self.password,
> + timeout=20 if self.port else 10,
> + )
> + except (TypeError, BadHostKeyException,
AuthenticationException) as e:
> + self._logger.exception(e)
> + raise SSHConnectionError(self.hostname) from e
> + except (NoValidConnectionsError, socket.error, SSHException)
as e:
> + self._logger.debug(traceback.format_exc())
> + self._logger.warning(e)
> + self._logger.info(
> + "Retrying interactive session connection: "
> + f"retry number {retry_attempt +1}"
> + )
> + else:
> + break
> + else:
> + raise SSHConnectionError(self.hostname)
> + # Interactive sessions are used on an "as needed" basis so we
have
> + # to set a keepalive
> + self._transport = self.session.get_transport()
> + if self._transport is not None:
> + self._transport.set_keepalive(30)
> +
> + def create_interactive_shell(
> + self,
> + shell_type: InteractiveApp,
> + path_to_app: PurePath,
> + timeout: float = SETTINGS.timeout,
> + eal_parameters: str = "",
> + app_parameters="",
> + ) -> InteractiveShell:
> + """
> + See "create_interactive_shell" in SutNode
> + """
> + match (shell_type.name):
> + case "shell":
Comparing with the enum values seems better.
> + return InteractiveShell(
> + self.session, self._logger, path_to_app, timeout
> + )
> + case "testpmd":
> + return TestPmdShell(
> + self.session,
> + self._logger,
> + path_to_app,
> + timeout=timeout,
> + eal_flags=eal_parameters,
> + cmd_line_options=app_parameters,
> + )
framework/remote_session/remote/interactive_remote_session.py:89: error:
Missing return statement
This is probably because we don't have a case for the rest of the values.
> 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 00000000..d98503be
> --- /dev/null
> +++ b/dts/framework/remote_session/remote/interactive_shell.py
> @@ -0,0 +1,98 @@
> +from pathlib import PurePath
> +
> +from paramiko import Channel, SSHClient, channel
> +
> +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
> +
> + def __init__(
> + self,
> + interactive_session: SSHClient,
> + logger: DTSLOG,
> + path_to_app: PurePath | None = None,
> + timeout: float = SETTINGS.timeout,
> + ) -> None:
> + 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
> + if path_to_app:
> + self.send_command_no_output(str(path_to_app)) # starts the
application
This should be part of the derived class - the code to start an app is
going to be app-specific. Since this is an interactive session, the
assumption is that we're always going to start an app and then work with
it, so starting the app in the constructor should be safe.
We actually have that for testpmd, so we just need to unify the above with
what we have.
> +
> + def empty_stdout_buffer(self) -> None:
Same comment on ordering as above.
> + """Removes all data from the stdout buffer.
> +
> + Because of the way paramiko handles read buffers, there is no
way to effectively
> + remove data from, or "flush", read buffers. This method
essentially moves our
> + offset on the buffer to the end and thus "removes" the data from
the buffer.
> + Timeouts are thrown on read operations of paramiko pipes based
on whether data
> + had been received before timeout so we assume that if we reach
the timeout then
> + we are at the end of the buffer.
> + """
> + self._ssh_channel.settimeout(1)
Waiting a whole second seems to be a lot. We actually may not need this
method from the use in the code - that is we change how the app starts.
> + try:
> + for line in self._stdout:
> + pass
> + except TimeoutError:
> + pass
> + self._ssh_channel.settimeout(self._timeout) # reset timeout
> +
> + def send_command_no_output(self, command: str) -> None:
> + """Send command to channel without recording output.
> +
> + This method will not verify any input or output, it will simply
assume the
> + command succeeded. This method will also consume all output in
the buffer
> + after executing the command.
> + """
> + self._logger.info(
> + f"Sending command {command.strip()} and not collecting
output"
> + )
> + self._stdin.write(f"{command}\n")
> + self._stdin.flush()
> + self.empty_stdout_buffer()
> +
> + def send_command_get_output(self, command: str, prompt: str) -> 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
> + 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
> + 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()}...")
> + self._stdin.write(f"{command}\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()
> 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 00000000..873f3c82
> --- /dev/null
> +++ b/dts/framework/remote_session/remote/testpmd_shell.py
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +
> +from pathlib import PurePath
> +
> +from paramiko import SSHClient
> +
> +from framework.logger import DTSLOG
> +from framework.settings import SETTINGS
> +
> +from .interactive_shell import InteractiveShell
> +
> +
> +class TestPmdShell(InteractiveShell):
> + expected_prompt: str = "testpmd>"
> +
> + def __init__(
> + self,
> + interactive_session: SSHClient,
> + logger: DTSLOG,
> + path_to_testpmd: PurePath,
> + eal_flags: str = "",
> + cmd_line_options: str = "",
> + timeout: float = SETTINGS.timeout,
> + ) -> None:
> + """Initializes an interactive testpmd session using specified
parameters."""
> + super(TestPmdShell, self).__init__(
> + interactive_session, logger=logger, timeout=timeout
> + )
> + self.send_command_get_output(
> + f"{path_to_testpmd} {eal_flags} -- -i {cmd_line_options}\n",
> + self.expected_prompt,
> + )
> +
> + 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)
> +
> + def get_devices(self) -> list[str]:
> + """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[str] = []
> + for line in dev_info.split("\n"):
> + if "device name:" in line.lower():
> + dev_list.append(line.strip().split(": ")[1].strip())
> + return dev_list
This could be an object (or a list of TestPmdDevice object or something
similar). We could start with just the name and add to it in the future.
> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
> index 74391982..029665fb 100644
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022-2023 University of New Hampshire
>
> """
> Generic result container and reporters
> @@ -8,14 +9,17 @@
> import os.path
> from collections.abc import MutableSequence
> from enum import Enum, auto
> +from typing import Dict
No need, we can just use built-in dict (since Python3.9).
>
> from .config import (
> OS,
> Architecture,
> BuildTargetConfiguration,
> + BuildTargetVersionInfo,
> Compiler,
> CPUType,
> NodeConfiguration,
> + NodeVersionInfo,
> )
> from .exception import DTSError, ErrorSeverity
> from .logger import DTSLOG
> @@ -67,12 +71,14 @@ class Statistics(dict):
> Using a dict provides a convenient way to format the data.
> """
>
> - def __init__(self, dpdk_version):
> + def __init__(self, output_info: Dict[str, str] | None):
> super(Statistics, self).__init__()
> for result in Result:
> self[result.name] = 0
> self["PASS RATE"] = 0.0
> - self["DPDK VERSION"] = dpdk_version
> + if output_info:
> + for info_key, info_val in output_info.items():
> + self[info_key] = info_val
>
> def __iadd__(self, other: Result) -> "Statistics":
> """
> @@ -206,6 +212,8 @@ class BuildTargetResult(BaseResult):
> os: OS
> cpu: CPUType
> compiler: Compiler
> + compiler_version: str | None
> + dpdk_version: str | None
>
> def __init__(self, build_target: BuildTargetConfiguration):
> super(BuildTargetResult, self).__init__()
> @@ -213,6 +221,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:
BuildTargetVersionInfo) -> None:
> + 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)
> @@ -228,10 +242,17 @@ class ExecutionResult(BaseResult):
> """
>
> sut_node: NodeConfiguration
> + sut_os_name: str
> + sut_os_version: str
> + sut_kernel_version: str
>
> - def __init__(self, sut_node: NodeConfiguration):
> + def __init__(self, sut_node: NodeConfiguration, sut_version_info:
NodeVersionInfo):
> super(ExecutionResult, self).__init__()
> self.sut_node = sut_node
> + self.sut_version_info = sut_version_info
> + self.sut_os_name = sut_version_info.os_name
> + self.sut_os_version = sut_version_info.os_version
> + self.sut_kernel_version = sut_version_info.kernel_version
>
> def add_build_target(
> self, build_target: BuildTargetConfiguration
> @@ -258,6 +279,7 @@ class DTSResult(BaseResult):
> """
>
> dpdk_version: str | None
> + output: dict | None
> _logger: DTSLOG
> _errors: list[Exception]
> _return_code: ErrorSeverity
> @@ -267,14 +289,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:
NodeVersionInfo
> + ) -> ExecutionResult:
> + execution_result = ExecutionResult(sut_node, sut_version_info)
> self._inner_results.append(execution_result)
> return execution_result
>
> @@ -296,7 +321,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)
> + # 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))
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 0705f38f..ad66014c 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -10,11 +10,24 @@
> import inspect
> import re
> from types import MethodType
> +from typing import Dict
>
> -from .exception import ConfigurationError, SSHTimeoutError,
TestCaseVerifyError
> +from .config import BuildTargetConfiguration
> +from .exception import (
> + BlockingTestSuiteError,
> + ConfigurationError,
> + SSHTimeoutError,
> + TestCaseVerifyError,
> +)
> from .logger import DTSLOG, getLogger
> from .settings import SETTINGS
> -from .test_result import BuildTargetResult, Result, TestCaseResult,
TestSuiteResult
> +from .test_result import (
> + BuildTargetResult,
> + DTSResult,
> + Result,
> + TestCaseResult,
> + TestSuiteResult,
> +)
> from .testbed_model import SutNode
>
>
> @@ -37,10 +50,12 @@ class TestSuite(object):
> """
>
> sut_node: SutNode
> + is_blocking = False
> _logger: DTSLOG
> _test_cases_to_run: list[str]
> _func: bool
> _result: TestSuiteResult
> + _dts_result: DTSResult
>
> def __init__(
> self,
> @@ -48,6 +63,8 @@ def __init__(
> test_cases: list[str],
> func: bool,
> build_target_result: BuildTargetResult,
> + build_target_conf: BuildTargetConfiguration,
> + dts_result: DTSResult,
> ):
> self.sut_node = sut_node
> self._logger = getLogger(self.__class__.__name__)
> @@ -55,6 +72,8 @@ def __init__(
> self._test_cases_to_run.extend(SETTINGS.test_cases)
> self._func = func
> self._result =
build_target_result.add_test_suite(self.__class__.__name__)
> + self.build_target_info = build_target_conf
> + self._dts_result = dts_result
>
> def set_up_suite(self) -> None:
> """
> @@ -118,6 +137,8 @@ def run(self) -> None:
> f"the next test suite may be affected."
> )
> self._result.update_setup(Result.ERROR, e)
> + if len(self._result.get_errors()) > 0 and self.is_blocking:
> + raise BlockingTestSuiteError(test_suite_name)
>
> def _execute_test_suite(self) -> None:
> """
> @@ -232,6 +253,12 @@ def _execute_test_case(
> test_case_result.update(Result.SKIP)
> raise KeyboardInterrupt("Stop DTS")
>
> + def write_to_statistics_file(self, output: Dict[str, str]):
This isn't used anywhere as far as I can tell.
> + if self._dts_result.output is not None:
> + self._dts_result.output.update(output)
> + else:
> + self._dts_result.output = output
> +
This could be simplified if we init output to an empty dict in the
constructor.
>
> def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]]:
> def is_test_suite(object) -> bool:
> diff --git a/dts/framework/testbed_model/node.py
b/dts/framework/testbed_model/node.py
> index d48fafe6..c5147e0e 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -40,6 +40,7 @@ class Node(object):
> lcores: list[LogicalCore]
> _logger: DTSLOG
> _other_sessions: list[OSSession]
> + _execution_config: ExecutionConfiguration
>
> def __init__(self, node_config: NodeConfiguration):
> self.config = node_config
> @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config:
ExecutionConfiguration) -> None:
> """
> self._setup_hugepages()
> self._set_up_execution(execution_config)
> + self._execution_config = execution_config
>
> def _set_up_execution(self, execution_config:
ExecutionConfiguration) -> None:
> """
> diff --git a/dts/framework/testbed_model/sut_node.py
b/dts/framework/testbed_model/sut_node.py
> index 2b2b50d9..27849da7 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -1,14 +1,21 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2010-2014 Intel Corporation
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022-2023 University of New Hampshire
>
> import os
> import tarfile
> import time
> from pathlib import PurePath
>
> -from framework.config import BuildTargetConfiguration, NodeConfiguration
> -from framework.remote_session import CommandResult, OSSession
> +from framework.config import (
> + BuildTargetConfiguration,
> + BuildTargetVersionInfo,
> + InteractiveApp,
> + NodeConfiguration,
> + NodeVersionInfo,
> +)
> +from framework.remote_session import CommandResult, InteractiveShell,
OSSession
> from framework.settings import SETTINGS
> from framework.utils import EnvVarsDict, MesonArgs
>
> @@ -26,13 +33,17 @@ class SutNode(Node):
>
> _dpdk_prefix_list: list[str]
> _dpdk_timestamp: str
> - _build_target_config: BuildTargetConfiguration | None
> + _build_target_config: BuildTargetConfiguration
What's the reason for this change? My guess would be mypy, but the change
still results in mypy complaining:
framework/testbed_model/sut_node.py:51: error: Incompatible types in
assignment (expression has type "None", variable has type
"BuildTargetConfiguration")
> _env_vars: EnvVarsDict
> _remote_tmp_dir: PurePath
> __remote_dpdk_dir: PurePath | None
> - _dpdk_version: str | None
> _app_compile_timeout: float
> _dpdk_kill_session: OSSession | None
> + _dpdk_version: str | None
> + _os_name: str | None
> + _os_version: str | None
> + _kernel_version: str | None
> + _compiler_version: str | None
>
> def __init__(self, node_config: NodeConfiguration):
> super(SutNode, self).__init__(node_config)
> @@ -41,12 +52,16 @@ def __init__(self, node_config: NodeConfiguration):
> self._env_vars = EnvVarsDict()
> self._remote_tmp_dir = self.main_session.get_remote_tmp_dir()
> self.__remote_dpdk_dir = None
> - self._dpdk_version = None
> self._app_compile_timeout = 90
> self._dpdk_kill_session = None
> self._dpdk_timestamp = (
> f"{str(os.getpid())}_{time.strftime('%Y%m%d%H%M%S',
time.localtime())}"
> )
> + self._dpdk_version = None
> + self._os_name = None
> + self._os_version = None
> + self._kernel_version = None
> + self._compiler_version = None
>
> @property
> def _remote_dpdk_dir(self) -> PurePath:
> @@ -75,6 +90,44 @@ def dpdk_version(self) -> str:
> )
> return self._dpdk_version
>
> + @property
> + def os_name(self) -> str:
> + if self._os_name is None:
> + self._os_name = self.main_session.get_os_name()
> + return self._os_name
> +
> + @property
> + def os_version(self) -> str:
> + if self._os_version is None:
> + self._os_version = self.main_session.get_os_version()
> + return self._os_version
> +
> + @property
> + def kernel_version(self) -> str:
> + if self._kernel_version is None:
> + self._kernel_version = self.main_session.get_kernel_version()
> + return self._kernel_version
> +
> + @property
> + def compiler_version(self) -> str:
> + if self._compiler_version is None:
> + self._compiler_version =
self.main_session.get_compiler_version(
> + self._build_target_config.compiler.name
> + )
> + return self._compiler_version
> +
> + def get_node_versions(self) -> NodeVersionInfo:
> + return NodeVersionInfo(
> + os_name=self.os_name,
> + os_version=self.os_version,
> + kernel_version=self.kernel_version,
> + )
> +
> + def get_build_target_versions(self) -> BuildTargetVersionInfo:
> + return BuildTargetVersionInfo(
> + dpdk_version=self.dpdk_version,
compiler_version=self.compiler_version
> + )
> +
> def _guess_dpdk_remote_dir(self) -> PurePath:
> return
self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
>
> @@ -84,6 +137,10 @@ def _set_up_build_target(
> """
> Setup DPDK on the SUT node.
> """
> + # we want to ensure that dpdk_version and compiler_version is
reset for new
> + # build targets
> + self._dpdk_version = None
> + self._compiler_version = None
> self._configure_build_target(build_target_config)
> self._copy_dpdk_tarball()
> self._build_dpdk()
> @@ -262,6 +319,49 @@ def run_dpdk_app(
> f"{app_path} {eal_args}", timeout, verify=True
> )
>
> + def create_interactive_shell(
> + self,
> + shell_type: InteractiveApp,
> + path_to_app: PurePath | None = None,
Can we join shell_type and path_to_app together? Both contain the same type
of information.
If we can't, let's drop path_to_app, as we don't provide (in this patch) a
way for users to modify this. We can add it later if needed.
> + timeout: float = SETTINGS.timeout,
> + eal_parameters: str = "",
Any reason we're not using the EalParameters class here?
> + app_parameters="",
This is also currently not used, so maybe we should drop it and add it in
the future, probably as a dedicated object representing the parameters.
> + ) -> InteractiveShell:
This should return a Union of all the different sessions we support,
otherwise mypy complains:
tests/TestSuite_smoke_tests.py:68: error: Incompatible types in assignment
(expression has type "InteractiveShell", variable has type "TestPmdShell")
> + """Create a handler for an interactive session.
> +
> + This method is a factory that calls a method in OSSession to
create shells for
> + different DPDK applications.
> +
> + Args:
> + shell_type: Enum value representing the desired application.
> + path_to_app: Represents a path to the application you are
attempting to
> + launch. This path will be executed at the start of the
app
> + initialization. If one isn't provided, the default
specified in the
> + enumeration will be used.
> + timeout: Timeout for the ssh channel
We should specify what sort of timeout this is - for creating the channel
or for sent commands?
> + eal_parameters: List of EAL parameters to use to launch the
app. This is
> + ignored for base "shell" types.
> + app_parameters: Command-line flags to pass into the
application on launch.
> + Returns:
> + Instance of the desired interactive application.
> + """
> + default_path: PurePath | None
> + # if we just want a default shell, there is no need to append
the DPDK build
> + # directory to the path
> + if shell_type.name == "shell":
> + default_path = None
> + else:
> + default_path = self.main_session.join_remote_path(
> + self.remote_dpdk_build_dir, *shell_type.get_path()
> + )
All this path handling looks clunky, it should be part of
the InteractiveApp class. Would adding something like update_path to
InteractiveApp work?
> + return
self.main_session.interactive_session.create_interactive_shell(
> + shell_type,
> + path_to_app or default_path,
> + timeout,
> + eal_parameters,
> + app_parameters,
> + )
> +
>
> class EalParameters(object):
> def __init__(
> diff --git a/dts/tests/TestSuite_smoke_tests.py
b/dts/tests/TestSuite_smoke_tests.py
> new file mode 100644
> index 00000000..a2fd9fae
> --- /dev/null
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +from framework.config import InteractiveApp
> +from framework.remote_session import TestPmdShell
> +from framework.test_suite import TestSuite
> +
> +
> +class SmokeTests(TestSuite):
> + is_blocking = True
> + # in this list, the first index is the address of the nic and the
second is
> + # the driver for that nic.
> + list_of_nics: list[tuple[str, str]] = []
Would using a dict make more sense?
> +
> + def set_up_suite(self) -> None:
> + """
> + Setup:
> + build all DPDK
This needs to be updated.
> + """
> + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir
> + for nic in self.sut_node.config.ports:
> + new_tuple = (nic.pci, nic.os_driver.strip())
> + self.list_of_nics.append(new_tuple)
It would be better to rename this based on what list of nics this is - the
one we expect to find or the one we're testing?
> +
> + 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,
> + )
> +
> + def test_driver_tests(self) -> None:
> + """
> + Test:
> + run the driver-test unit-test suite through meson
> + """
> + list_of_vdevs = ""
> + for dev in self.sut_node._execution_config.vdevs:
> + 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
> + """
Every other test description is written in imperative and with lowecase
first letter. Let's unite everything to imperative with uppercase first
letter and properly ending the sentence with a dot.
> + testpmd_driver: TestPmdShell =
self.sut_node.create_interactive_shell(
> + InteractiveApp.testpmd
> + )
> + dev_list: list[str] = testpmd_driver.get_devices()
> + for nic in self.list_of_nics:
Here's where the more explicit naming would be beneficial - I had to think
about what's what.
> + self.verify(
> + nic[0] in dev_list,
> + f"Device {nic[0]} 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_dev = self.sut_node.main_session.join_remote_path(
> + self.sut_node._remote_dpdk_dir, "usertools",
"dpdk-devbind.py"
> + )
> + for nic in self.list_of_nics:
> + out = self.sut_node.main_session.send_command(
> + f"{path_to_dev} --status | grep {nic[0]}", 60
> + )
This may be something we want to put into SutNode, something like
get_device_info. We'd only send the command once (not for every NIC as
we're doing here) and parse the output.
> + self.verify(
> + len(out.stdout) != 0,
> + f"Failed to find configured device ({nic[0]}) using
dpdk-devbind.py",
> + )
> + for string in out.stdout.split(" "):
> + if "drv=" in string:
> + self.verify(
> + string.split("=")[1] == nic[1],
> + f"Driver for device {nic[0]} does not match
driver listed in "
> + f'configuration (bound to
{string.split("=")[1]})',
> + )
> --
> 2.41.0
>
[-- Attachment #2: Type: text/html, Size: 86726 bytes --]
next prev parent reply other threads:[~2023-07-06 14:54 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-15 20:10 [PATCH v1 0/2] Add DTS " jspewock
2023-06-15 20:10 ` [PATCH v1 1/2] dts: add " jspewock
2023-07-06 14:53 ` Juraj Linkeš [this message]
2023-07-07 23:06 ` Jeremy Spewock
2023-07-11 8:16 ` Juraj Linkeš
2023-07-11 18:09 ` Jeremy Spewock
2023-07-12 6:30 ` Juraj Linkeš
2023-07-12 13:46 ` Jeremy Spewock
2023-06-15 20:11 ` [PATCH v1 2/2] dts: added paramiko to dependencies jspewock
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='CAOb5WZbGDJJn+0AR0840O8vTfxe6WXZFFFGySU_RL+=ZckTq3g@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).