DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v1 0/4] node and inheritance improvements
@ 2024-04-23  9:12 Juraj Linkeš
  2024-04-23  9:12 ` [PATCH v1 1/4] dts: add tg node execution setup and teardown Juraj Linkeš
                   ` (5 more replies)
  0 siblings, 6 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-04-23  9:12 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

There are two areas that are unified:
The super() calls, where the arguments were removed as they're not
needed in single inheritance.
The classes derived from object don't need to explicitly state that.

The tg node execution setup and teardown was omitted and is fixed in
this series.

And probably the most important part if the cleanup of execution and
build target setup and teardown. Build targets are relevant only for sut
nodes, so it's been moved there, same for vdevs. The execution
setup/teardown code was modified so that subclasses use super() to
extend the base methods.

Juraj Linkeš (4):
  dts: add tg node execution setup and teardown
  dts: unify class inheritance from object
  dts: unify super calls
  dts: refine pre-test setup and teardown steps

 dts/framework/remote_session/testpmd_shell.py |  2 +-
 dts/framework/runner.py                       |  2 +
 dts/framework/test_result.py                  | 16 ++---
 dts/framework/test_suite.py                   |  2 +-
 dts/framework/testbed_model/cpu.py            |  4 +-
 dts/framework/testbed_model/node.py           | 65 ++-----------------
 dts/framework/testbed_model/sut_node.py       | 53 ++++++++++-----
 dts/framework/testbed_model/tg_node.py        |  4 +-
 dts/framework/testbed_model/virtual_device.py |  2 +-
 dts/framework/utils.py                        |  4 +-
 10 files changed, 61 insertions(+), 93 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 1/4] dts: add tg node execution setup and teardown
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
@ 2024-04-23  9:12 ` Juraj Linkeš
  2024-04-23  9:18   ` Luca Vizzarro
  2024-04-23  9:12 ` [PATCH v1 2/4] dts: unify class inheritance from object Juraj Linkeš
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Juraj Linkeš @ 2024-04-23  9:12 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

The setup and teardown for the execution stage was meant to be run on
the tg node too, but was mistakenly omitted.

Fixes: cecfe0aabf58 ("dts: add traffic generator abstractions")
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/runner.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index db8e3ba96b..2f25a3e941 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -422,6 +422,7 @@ def _run_execution(
         execution_result.add_sut_info(sut_node.node_info)
         try:
             sut_node.set_up_execution(execution)
+            tg_node.set_up_execution(execution)
             execution_result.update_setup(Result.PASS)
         except Exception as e:
             self._logger.exception("Execution setup failed.")
@@ -438,6 +439,7 @@ def _run_execution(
             try:
                 self._logger.set_stage(DtsStage.execution_teardown)
                 sut_node.tear_down_execution()
+                tg_node.tear_down_execution()
                 execution_result.update_teardown(Result.PASS)
             except Exception as e:
                 self._logger.exception("Execution teardown failed.")
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 2/4] dts: unify class inheritance from object
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
  2024-04-23  9:12 ` [PATCH v1 1/4] dts: add tg node execution setup and teardown Juraj Linkeš
@ 2024-04-23  9:12 ` Juraj Linkeš
  2024-04-23  9:19   ` Luca Vizzarro
  2024-04-23 14:53   ` Patrick Robb
  2024-04-23  9:12 ` [PATCH v1 3/4] dts: unify super calls Juraj Linkeš
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-04-23  9:12 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

There are two ways we specify that a class inherits from object -
implicitly and explicitly. There's no need to explicitly specify that a
class inherits from object and is in fact mostly a remnant from Python2.
Leaving it implicit is the standard in Python3 and offers a small bonus
in cases where something would assign something else to the builtin
object variable.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/remote_session/testpmd_shell.py | 2 +-
 dts/framework/test_result.py                  | 4 ++--
 dts/framework/test_suite.py                   | 2 +-
 dts/framework/testbed_model/cpu.py            | 4 ++--
 dts/framework/testbed_model/sut_node.py       | 2 +-
 dts/framework/testbed_model/virtual_device.py | 2 +-
 dts/framework/utils.py                        | 4 ++--
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..9456de941d 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -27,7 +27,7 @@
 from .interactive_shell import InteractiveShell
 
 
-class TestPmdDevice(object):
+class TestPmdDevice:
     """The data of a device that testpmd can recognize.
 
     Attributes:
diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 28f84fd793..83e637c280 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -96,7 +96,7 @@ def __bool__(self) -> bool:
         return self is self.PASS
 
 
-class FixtureResult(object):
+class FixtureResult:
     """A record that stores the result of a setup or a teardown.
 
     :attr:`~Result.FAIL` is a sensible default since it prevents false positives (which could happen
@@ -132,7 +132,7 @@ def __bool__(self) -> bool:
         return bool(self.result)
 
 
-class BaseResult(object):
+class BaseResult:
     """Common data and behavior of DTS results.
 
     Stores the results of the setup and teardown portions of the corresponding stage.
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 9c3b516002..7efa0eae44 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -27,7 +27,7 @@
 from .utils import get_packet_summaries
 
 
-class TestSuite(object):
+class TestSuite:
     """The base class with building blocks needed by most test cases.
 
         * Test suite setup/cleanup methods to override,
diff --git a/dts/framework/testbed_model/cpu.py b/dts/framework/testbed_model/cpu.py
index 9e33b2825d..a50cf44c19 100644
--- a/dts/framework/testbed_model/cpu.py
+++ b/dts/framework/testbed_model/cpu.py
@@ -26,7 +26,7 @@
 
 
 @dataclass(slots=True, frozen=True)
-class LogicalCore(object):
+class LogicalCore:
     """Representation of a logical CPU core.
 
     A physical core is represented in OS by multiple logical cores (lcores)
@@ -50,7 +50,7 @@ def __int__(self) -> int:
         return self.lcore
 
 
-class LogicalCoreList(object):
+class LogicalCoreList:
     r"""A unified way to store :class:`LogicalCore`\s.
 
     Create a unified format used across the framework and allow the user to use
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 97aa26d419..10d56eba8d 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -34,7 +34,7 @@
 from .virtual_device import VirtualDevice
 
 
-class EalParameters(object):
+class EalParameters:
     """The environment abstraction layer parameters.
 
     The string representation can be created by converting the instance to a string.
diff --git a/dts/framework/testbed_model/virtual_device.py b/dts/framework/testbed_model/virtual_device.py
index e9b5e9c3be..569d67b007 100644
--- a/dts/framework/testbed_model/virtual_device.py
+++ b/dts/framework/testbed_model/virtual_device.py
@@ -7,7 +7,7 @@
 """
 
 
-class VirtualDevice(object):
+class VirtualDevice:
     """Base class for virtual devices used by DPDK.
 
     Attributes:
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index cc5e458cc8..f9ed1b562f 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -82,7 +82,7 @@ def __str__(self) -> str:
         return self.name
 
 
-class MesonArgs(object):
+class MesonArgs:
     """Aggregate the arguments needed to build DPDK."""
 
     _default_library: str
@@ -131,7 +131,7 @@ class _TarCompressionFormat(StrEnum):
     zstd = "zst"
 
 
-class DPDKGitTarball(object):
+class DPDKGitTarball:
     """Compressed tarball of DPDK from the repository.
 
     The class supports the :class:`os.PathLike` protocol,
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 3/4] dts: unify super calls
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
  2024-04-23  9:12 ` [PATCH v1 1/4] dts: add tg node execution setup and teardown Juraj Linkeš
  2024-04-23  9:12 ` [PATCH v1 2/4] dts: unify class inheritance from object Juraj Linkeš
@ 2024-04-23  9:12 ` Juraj Linkeš
  2024-04-23 10:06   ` Luca Vizzarro
  2024-04-23 14:57   ` Patrick Robb
  2024-04-23  9:12 ` [PATCH v1 4/4] dts: refine pre-test setup and teardown steps Juraj Linkeš
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-04-23  9:12 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

We have two ways of calling super() in the codebase. For single
inheritance, there's no benefit in listing the arguments, as the
function will do exactly what we need it to do.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/test_result.py            | 12 ++++++------
 dts/framework/testbed_model/sut_node.py |  2 +-
 dts/framework/testbed_model/tg_node.py  |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 83e637c280..0163893adf 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -249,7 +249,7 @@ def __init__(self, logger: DTSLogger):
         Args:
             logger: The logger instance the whole result will use.
         """
-        super(DTSResult, self).__init__()
+        super().__init__()
         self.dpdk_version = None
         self._logger = logger
         self._errors = []
@@ -338,7 +338,7 @@ def __init__(self, execution: ExecutionConfiguration):
         Args:
             execution: The execution's test run configuration.
         """
-        super(ExecutionResult, self).__init__()
+        super().__init__()
         self._config = execution
         self._test_suites_with_cases = []
 
@@ -430,7 +430,7 @@ def __init__(
             test_suites_with_cases: The test suites with test cases to be run in this build target.
             build_target: The build target's test run configuration.
         """
-        super(BuildTargetResult, self).__init__()
+        super().__init__()
         self.arch = build_target.arch
         self.os = build_target.os
         self.cpu = build_target.cpu
@@ -491,7 +491,7 @@ def __init__(self, test_suite_with_cases: TestSuiteWithCases):
         Args:
             test_suite_with_cases: The test suite with test cases.
         """
-        super(TestSuiteResult, self).__init__()
+        super().__init__()
         self.test_suite_name = test_suite_with_cases.test_suite_class.__name__
         self._test_suite_with_cases = test_suite_with_cases
 
@@ -534,7 +534,7 @@ def __init__(self, test_case_name: str):
         Args:
             test_case_name: The test case's name.
         """
-        super(TestCaseResult, self).__init__()
+        super().__init__()
         self.test_case_name = test_case_name
 
     def update(self, result: Result, error: Exception | None = None) -> None:
@@ -592,7 +592,7 @@ def __init__(self, dpdk_version: str | None):
         Args:
             dpdk_version: The version of tested DPDK.
         """
-        super(Statistics, self).__init__()
+        super().__init__()
         for result in Result:
             self[result.name] = 0
         self["PASS RATE"] = 0.0
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 10d56eba8d..800fbef860 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -130,7 +130,7 @@ def __init__(self, node_config: SutNodeConfiguration):
         Args:
             node_config: The SUT node's test run configuration.
         """
-        super(SutNode, self).__init__(node_config)
+        super().__init__(node_config)
         self._dpdk_prefix_list = []
         self._build_target_config = None
         self._env_vars = {}
diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
index d3206e87e0..b0126e5e3d 100644
--- a/dts/framework/testbed_model/tg_node.py
+++ b/dts/framework/testbed_model/tg_node.py
@@ -48,7 +48,7 @@ def __init__(self, node_config: TGNodeConfiguration):
         Args:
             node_config: The TG node's test run configuration.
         """
-        super(TGNode, self).__init__(node_config)
+        super().__init__(node_config)
         self.traffic_generator = create_traffic_generator(self, node_config.traffic_generator)
         self._logger.info(f"Created node: {self.name}")
 
@@ -90,4 +90,4 @@ def close(self) -> None:
         This extends the superclass method with TG cleanup.
         """
         self.traffic_generator.close()
-        super(TGNode, self).close()
+        super().close()
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 4/4] dts: refine pre-test setup and teardown steps
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
                   ` (2 preceding siblings ...)
  2024-04-23  9:12 ` [PATCH v1 3/4] dts: unify super calls Juraj Linkeš
@ 2024-04-23  9:12 ` Juraj Linkeš
  2024-04-23  9:19   ` Luca Vizzarro
  2024-04-23 10:07 ` [PATCH v1 0/4] node and inheritance improvements Luca Vizzarro
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
  5 siblings, 1 reply; 23+ messages in thread
From: Juraj Linkeš @ 2024-04-23  9:12 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

The major part is the removal of _set_up_execution() and
_tear_down_execution() of Node in lieu of using super() in the
superclasses, which simplifies the code while achieving the same thing.

The minor changes are the move of virtual devices and build target
setup/teardown into SutNode from Node since both are DPDK-related which
are only going to run on the SutNode.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/testbed_model/node.py     | 65 ++-----------------------
 dts/framework/testbed_model/sut_node.py | 49 ++++++++++++++-----
 2 files changed, 40 insertions(+), 74 deletions(-)

diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index 74061f6262..1a98209822 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -16,12 +16,7 @@
 from ipaddress import IPv4Interface, IPv6Interface
 from typing import Any, Callable, Type, Union
 
-from framework.config import (
-    OS,
-    BuildTargetConfiguration,
-    ExecutionConfiguration,
-    NodeConfiguration,
-)
+from framework.config import OS, ExecutionConfiguration, NodeConfiguration
 from framework.exception import ConfigurationError
 from framework.logger import DTSLogger, get_dts_logger
 from framework.settings import SETTINGS
@@ -36,7 +31,6 @@
 from .linux_session import LinuxSession
 from .os_session import InteractiveShellType, OSSession
 from .port import Port
-from .virtual_device import VirtualDevice
 
 
 class Node(ABC):
@@ -55,7 +49,6 @@ class Node(ABC):
         lcores: The list of logical cores that DTS can use on the node.
             It's derived from logical cores present on the node and the test run configuration.
         ports: The ports of this node specified in the test run configuration.
-        virtual_devices: The virtual devices used on the node.
     """
 
     main_session: OSSession
@@ -65,8 +58,6 @@ class Node(ABC):
     ports: list[Port]
     _logger: DTSLogger
     _other_sessions: list[OSSession]
-    _execution_config: ExecutionConfiguration
-    virtual_devices: list[VirtualDevice]
 
     def __init__(self, node_config: NodeConfiguration):
         """Connect to the node and gather info during initialization.
@@ -94,7 +85,6 @@ def __init__(self, node_config: NodeConfiguration):
         ).filter()
 
         self._other_sessions = []
-        self.virtual_devices = []
         self._init_ports()
 
     def _init_ports(self) -> None:
@@ -106,67 +96,20 @@ def _init_ports(self) -> None:
     def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
         """Execution setup steps.
 
-        Configure hugepages and call :meth:`_set_up_execution` where
-        the rest of the configuration steps (if any) are implemented.
+        Configure hugepages on all DTS node types. Additional steps can be added by
+        extending the method in subclasses with the use of super().
 
         Args:
             execution_config: The execution test run configuration according to which
                 the setup steps will be taken.
         """
         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:
-        """Optional additional execution setup steps for subclasses.
-
-        Subclasses should override this if they need to add additional execution setup steps.
-        """
 
     def tear_down_execution(self) -> None:
         """Execution teardown steps.
 
         There are currently no common execution teardown steps common to all DTS node types.
-        """
-        self.virtual_devices = []
-        self._tear_down_execution()
-
-    def _tear_down_execution(self) -> None:
-        """Optional additional execution teardown steps for subclasses.
-
-        Subclasses should override this if they need to add additional execution teardown steps.
-        """
-
-    def set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Build target setup steps.
-
-        There are currently no common build target setup steps common to all DTS node types.
-
-        Args:
-            build_target_config: The build target test run configuration according to which
-                the setup steps will be taken.
-        """
-        self._set_up_build_target(build_target_config)
-
-    def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Optional additional build target setup steps for subclasses.
-
-        Subclasses should override this if they need to add additional build target setup steps.
-        """
-
-    def tear_down_build_target(self) -> None:
-        """Build target teardown steps.
-
-        There are currently no common build target teardown steps common to all DTS node types.
-        """
-        self._tear_down_build_target()
-
-    def _tear_down_build_target(self) -> None:
-        """Optional additional build target teardown steps for subclasses.
-
-        Subclasses should override this if they need to add additional build target teardown steps.
+        Additional steps can be added by extending the method in subclasses with the use of super().
         """
 
     def create_session(self, name: str) -> OSSession:
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 800fbef860..ead6cc5a43 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -20,6 +20,7 @@
 from framework.config import (
     BuildTargetConfiguration,
     BuildTargetInfo,
+    ExecutionConfiguration,
     NodeInfo,
     SutNodeConfiguration,
 )
@@ -107,10 +108,12 @@ class SutNode(Node):
     or the git commit ID, tag ID or tree ID to test.
 
     Attributes:
-        config: The SUT node configuration
+        config: The SUT node configuration.
+        virtual_devices: The virtual devices used on the node.
     """
 
     config: SutNodeConfiguration
+    virtual_devices: list[VirtualDevice]
     _dpdk_prefix_list: list[str]
     _dpdk_timestamp: str
     _build_target_config: BuildTargetConfiguration | None
@@ -131,6 +134,7 @@ def __init__(self, node_config: SutNodeConfiguration):
             node_config: The SUT node's test run configuration.
         """
         super().__init__(node_config)
+        self.virtual_devices = []
         self._dpdk_prefix_list = []
         self._build_target_config = None
         self._env_vars = {}
@@ -228,25 +232,44 @@ def get_build_target_info(self) -> BuildTargetInfo:
     def _guess_dpdk_remote_dir(self) -> PurePath:
         return self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
 
-    def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Setup DPDK on the SUT node.
+    def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
+        """Extend the execution setup with vdev config.
 
-        Additional build target setup steps on top of those in :class:`Node`.
+        Args:
+            execution_config: The execution test run configuration according to which
+                the setup steps will be taken.
+        """
+        super().set_up_execution(execution_config)
+        for vdev in execution_config.vdevs:
+            self.virtual_devices.append(VirtualDevice(vdev))
+
+    def tear_down_execution(self) -> None:
+        """Extend the execution teardown with virtual device teardown."""
+        super().tear_down_execution()
+        self.virtual_devices = []
+
+    def set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
+        """Set up DPDK the SUT node and bind ports.
+
+        DPDK setup includes setting all internals needed for the build, the copying of DPDK tarball
+        and then building DPDK. The drivers are bound to those that DPDK needs.
+
+        Args:
+            build_target_config: The build target test run configuration according to which
+                the setup steps will be taken.
         """
-        # 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()
         self.bind_ports_to_driver()
 
-    def _tear_down_build_target(self) -> None:
-        """Bind ports to the operating system drivers.
-
-        Additional build target teardown steps on top of those in :class:`Node`.
-        """
+    def tear_down_build_target(self) -> None:
+        """Reset DPDK variables and bind port driver to the OS driver."""
+        self._env_vars = {}
+        self._build_target_config = None
+        self.__remote_dpdk_dir = None
+        self._dpdk_version = None
+        self._compiler_version = None
         self.bind_ports_to_driver(for_dpdk=False)
 
     def _configure_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/4] dts: add tg node execution setup and teardown
  2024-04-23  9:12 ` [PATCH v1 1/4] dts: add tg node execution setup and teardown Juraj Linkeš
@ 2024-04-23  9:18   ` Luca Vizzarro
  2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 1 reply; 23+ messages in thread
From: Luca Vizzarro @ 2024-04-23  9:18 UTC (permalink / raw)
  To: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	npratte
  Cc: dev

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/4] dts: unify class inheritance from object
  2024-04-23  9:12 ` [PATCH v1 2/4] dts: unify class inheritance from object Juraj Linkeš
@ 2024-04-23  9:19   ` Luca Vizzarro
  2024-04-23 14:53   ` Patrick Robb
  1 sibling, 0 replies; 23+ messages in thread
From: Luca Vizzarro @ 2024-04-23  9:19 UTC (permalink / raw)
  To: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	npratte
  Cc: dev

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 4/4] dts: refine pre-test setup and teardown steps
  2024-04-23  9:12 ` [PATCH v1 4/4] dts: refine pre-test setup and teardown steps Juraj Linkeš
@ 2024-04-23  9:19   ` Luca Vizzarro
  2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 1 reply; 23+ messages in thread
From: Luca Vizzarro @ 2024-04-23  9:19 UTC (permalink / raw)
  To: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	npratte
  Cc: dev

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 3/4] dts: unify super calls
  2024-04-23  9:12 ` [PATCH v1 3/4] dts: unify super calls Juraj Linkeš
@ 2024-04-23 10:06   ` Luca Vizzarro
  2024-04-23 14:57   ` Patrick Robb
  1 sibling, 0 replies; 23+ messages in thread
From: Luca Vizzarro @ 2024-04-23 10:06 UTC (permalink / raw)
  To: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	npratte
  Cc: dev

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/4] node and inheritance improvements
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
                   ` (3 preceding siblings ...)
  2024-04-23  9:12 ` [PATCH v1 4/4] dts: refine pre-test setup and teardown steps Juraj Linkeš
@ 2024-04-23 10:07 ` Luca Vizzarro
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
  5 siblings, 0 replies; 23+ messages in thread
From: Luca Vizzarro @ 2024-04-23 10:07 UTC (permalink / raw)
  To: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	npratte
  Cc: dev

On 23/04/2024 10:12, Juraj Linkeš wrote:
> There are two areas that are unified:
> The super() calls, where the arguments were removed as they're not
> needed in single inheritance.
> The classes derived from object don't need to explicitly state that.
> 
> The tg node execution setup and teardown was omitted and is fixed in
> this series.
> 
> And probably the most important part if the cleanup of execution and
> build target setup and teardown. Build targets are relevant only for sut
> nodes, so it's been moved there, same for vdevs. The execution
> setup/teardown code was modified so that subclasses use super() to
> extend the base methods.

Hi Juraj,

Thank you for your submission and improvements. It all looks good to me!

Best,
Luca

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/4] dts: unify class inheritance from object
  2024-04-23  9:12 ` [PATCH v1 2/4] dts: unify class inheritance from object Juraj Linkeš
  2024-04-23  9:19   ` Luca Vizzarro
@ 2024-04-23 14:53   ` Patrick Robb
  2024-04-30 16:15     ` Jeremy Spewock
  1 sibling, 1 reply; 23+ messages in thread
From: Patrick Robb @ 2024-04-23 14:53 UTC (permalink / raw)
  To: Juraj Linkeš
  Cc: thomas, Honnappa.Nagarahalli, jspewock, paul.szczepanek,
	Luca.Vizzarro, npratte, dev

[-- Attachment #1: Type: text/plain, Size: 46 bytes --]

Reviewed-by: Patrick Robb <probb@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 133 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 3/4] dts: unify super calls
  2024-04-23  9:12 ` [PATCH v1 3/4] dts: unify super calls Juraj Linkeš
  2024-04-23 10:06   ` Luca Vizzarro
@ 2024-04-23 14:57   ` Patrick Robb
  2024-04-30 16:15     ` Jeremy Spewock
  1 sibling, 1 reply; 23+ messages in thread
From: Patrick Robb @ 2024-04-23 14:57 UTC (permalink / raw)
  To: Juraj Linkeš
  Cc: thomas, Honnappa.Nagarahalli, jspewock, paul.szczepanek,
	Luca.Vizzarro, npratte, dev

[-- Attachment #1: Type: text/plain, Size: 46 bytes --]

Reviewed-by: Patrick Robb <probb@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 133 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/4] dts: add tg node execution setup and teardown
  2024-04-23  9:18   ` Luca Vizzarro
@ 2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 0 replies; 23+ messages in thread
From: Jeremy Spewock @ 2024-04-30 16:15 UTC (permalink / raw)
  To: Luca Vizzarro
  Cc: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, probb, paul.szczepanek, npratte,
	dev

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/4] dts: unify class inheritance from object
  2024-04-23 14:53   ` Patrick Robb
@ 2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 0 replies; 23+ messages in thread
From: Jeremy Spewock @ 2024-04-30 16:15 UTC (permalink / raw)
  To: Patrick Robb
  Cc: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, paul.szczepanek, Luca.Vizzarro,
	npratte, dev

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 3/4] dts: unify super calls
  2024-04-23 14:57   ` Patrick Robb
@ 2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 0 replies; 23+ messages in thread
From: Jeremy Spewock @ 2024-04-30 16:15 UTC (permalink / raw)
  To: Patrick Robb
  Cc: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, paul.szczepanek, Luca.Vizzarro,
	npratte, dev

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 4/4] dts: refine pre-test setup and teardown steps
  2024-04-23  9:19   ` Luca Vizzarro
@ 2024-04-30 16:15     ` Jeremy Spewock
  0 siblings, 0 replies; 23+ messages in thread
From: Jeremy Spewock @ 2024-04-30 16:15 UTC (permalink / raw)
  To: Luca Vizzarro
  Cc: Juraj Linkeš,
	thomas, Honnappa.Nagarahalli, probb, paul.szczepanek, npratte,
	dev

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 0/5] node and inheritance improvements
  2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
                   ` (4 preceding siblings ...)
  2024-04-23 10:07 ` [PATCH v1 0/4] node and inheritance improvements Luca Vizzarro
@ 2024-06-19 13:35 ` Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 1/5] dts: add tg node test run setup and teardown Juraj Linkeš
                     ` (5 more replies)
  5 siblings, 6 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš

There are two areas that are unified:
The super() calls, where the arguments were removed as they're not
needed in single inheritance.
The classes derived from object don't need to explicitly state that.

The tg node execution setup and teardown was omitted and is fixed in
this series.

And probably the most important part if the cleanup of execution and
build target setup and teardown. Build targets are relevant only for sut
nodes, so it's been moved there, same for vdevs. The execution
setup/teardown code was modified so that subclasses use super() to
extend the base methods.

v5:
Rebase on top of 32115 and add clean up close to the series.

Depends-on: series-32115 ("dts: rename execution to test run")

Juraj Linkeš (5):
  dts: add tg node test run setup and teardown
  dts: unify class inheritance from object
  dts: unify super calls
  dts: refine pre-test setup and teardown steps
  dts: clean up close in remote session

 .../remote_session/remote_session.py          | 21 ++----
 dts/framework/remote_session/ssh_session.py   | 11 ++--
 dts/framework/remote_session/testpmd_shell.py |  2 +-
 dts/framework/runner.py                       |  2 +
 dts/framework/test_result.py                  | 16 ++---
 dts/framework/test_suite.py                   |  2 +-
 dts/framework/testbed_model/cpu.py            |  4 +-
 dts/framework/testbed_model/node.py           | 66 ++-----------------
 dts/framework/testbed_model/os_session.py     | 12 ++--
 dts/framework/testbed_model/sut_node.py       | 53 ++++++++++-----
 dts/framework/testbed_model/tg_node.py        |  4 +-
 dts/framework/testbed_model/virtual_device.py |  2 +-
 dts/framework/utils.py                        |  4 +-
 13 files changed, 76 insertions(+), 123 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/5] dts: add tg node test run setup and teardown
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
@ 2024-06-19 13:35   ` Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 2/5] dts: unify class inheritance from object Juraj Linkeš
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš, Luca Vizzarro

The setup and teardown for the test run stage was meant to be run on
the tg node too, but was mistakenly omitted.

Fixes: cecfe0aabf58 ("dts: add traffic generator abstractions")
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/runner.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 4e0254f453..565e581310 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -428,6 +428,7 @@ def _run_test_run(
         test_run_result.add_sut_info(sut_node.node_info)
         try:
             sut_node.set_up_test_run(test_run_config)
+            tg_node.set_up_test_run(test_run_config)
             test_run_result.update_setup(Result.PASS)
         except Exception as e:
             self._logger.exception("Test run setup failed.")
@@ -448,6 +449,7 @@ def _run_test_run(
             try:
                 self._logger.set_stage(DtsStage.test_run_teardown)
                 sut_node.tear_down_test_run()
+                tg_node.tear_down_test_run()
                 test_run_result.update_teardown(Result.PASS)
             except Exception as e:
                 self._logger.exception("Test run teardown failed.")
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 2/5] dts: unify class inheritance from object
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 1/5] dts: add tg node test run setup and teardown Juraj Linkeš
@ 2024-06-19 13:35   ` Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 3/5] dts: unify super calls Juraj Linkeš
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš, Luca Vizzarro

There are two ways we specify that a class inherits from object -
implicitly and explicitly. There's no need to explicitly specify that a
class inherits from object and is in fact mostly a remnant from Python2.
Leaving it implicit is the standard in Python3 and offers a small bonus
in cases where something would assign something else to the builtin
object variable.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 2 +-
 dts/framework/test_result.py                  | 4 ++--
 dts/framework/test_suite.py                   | 2 +-
 dts/framework/testbed_model/cpu.py            | 4 ++--
 dts/framework/testbed_model/sut_node.py       | 2 +-
 dts/framework/testbed_model/virtual_device.py | 2 +-
 dts/framework/utils.py                        | 4 ++--
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..9456de941d 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -27,7 +27,7 @@
 from .interactive_shell import InteractiveShell
 
 
-class TestPmdDevice(object):
+class TestPmdDevice:
     """The data of a device that testpmd can recognize.
 
     Attributes:
diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 4693519bbc..5deccb6fd4 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -96,7 +96,7 @@ def __bool__(self) -> bool:
         return self is self.PASS
 
 
-class FixtureResult(object):
+class FixtureResult:
     """A record that stores the result of a setup or a teardown.
 
     :attr:`~Result.FAIL` is a sensible default since it prevents false positives (which could happen
@@ -132,7 +132,7 @@ def __bool__(self) -> bool:
         return bool(self.result)
 
 
-class BaseResult(object):
+class BaseResult:
     """Common data and behavior of DTS results.
 
     Stores the results of the setup and teardown portions of the corresponding stage.
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 8768f756a6..b9f8daab1a 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -27,7 +27,7 @@
 from .utils import get_packet_summaries
 
 
-class TestSuite(object):
+class TestSuite:
     """The base class with building blocks needed by most test cases.
 
         * Test suite setup/cleanup methods to override,
diff --git a/dts/framework/testbed_model/cpu.py b/dts/framework/testbed_model/cpu.py
index 9e33b2825d..a50cf44c19 100644
--- a/dts/framework/testbed_model/cpu.py
+++ b/dts/framework/testbed_model/cpu.py
@@ -26,7 +26,7 @@
 
 
 @dataclass(slots=True, frozen=True)
-class LogicalCore(object):
+class LogicalCore:
     """Representation of a logical CPU core.
 
     A physical core is represented in OS by multiple logical cores (lcores)
@@ -50,7 +50,7 @@ def __int__(self) -> int:
         return self.lcore
 
 
-class LogicalCoreList(object):
+class LogicalCoreList:
     r"""A unified way to store :class:`LogicalCore`\s.
 
     Create a unified format used across the framework and allow the user to use
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 97aa26d419..10d56eba8d 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -34,7 +34,7 @@
 from .virtual_device import VirtualDevice
 
 
-class EalParameters(object):
+class EalParameters:
     """The environment abstraction layer parameters.
 
     The string representation can be created by converting the instance to a string.
diff --git a/dts/framework/testbed_model/virtual_device.py b/dts/framework/testbed_model/virtual_device.py
index e9b5e9c3be..569d67b007 100644
--- a/dts/framework/testbed_model/virtual_device.py
+++ b/dts/framework/testbed_model/virtual_device.py
@@ -7,7 +7,7 @@
 """
 
 
-class VirtualDevice(object):
+class VirtualDevice:
     """Base class for virtual devices used by DPDK.
 
     Attributes:
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index 862bafb46c..6b5d5a805f 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -108,7 +108,7 @@ def __str__(self) -> str:
         return self.name
 
 
-class MesonArgs(object):
+class MesonArgs:
     """Aggregate the arguments needed to build DPDK."""
 
     _default_library: str
@@ -157,7 +157,7 @@ class _TarCompressionFormat(StrEnum):
     zstd = "zst"
 
 
-class DPDKGitTarball(object):
+class DPDKGitTarball:
     """Compressed tarball of DPDK from the repository.
 
     The class supports the :class:`os.PathLike` protocol,
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 3/5] dts: unify super calls
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 1/5] dts: add tg node test run setup and teardown Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 2/5] dts: unify class inheritance from object Juraj Linkeš
@ 2024-06-19 13:35   ` Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 4/5] dts: refine pre-test setup and teardown steps Juraj Linkeš
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš, Luca Vizzarro

We have two ways of calling super() in the codebase. For single
inheritance, there's no benefit in listing the arguments, as the
function will do exactly what we need it to do.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_result.py            | 12 ++++++------
 dts/framework/testbed_model/sut_node.py |  2 +-
 dts/framework/testbed_model/tg_node.py  |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 5deccb6fd4..5694a2482b 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -249,7 +249,7 @@ def __init__(self, logger: DTSLogger):
         Args:
             logger: The logger instance the whole result will use.
         """
-        super(DTSResult, self).__init__()
+        super().__init__()
         self.dpdk_version = None
         self._logger = logger
         self._errors = []
@@ -338,7 +338,7 @@ def __init__(self, test_run_config: TestRunConfiguration):
         Args:
             test_run_config: A test run configuration.
         """
-        super(TestRunResult, self).__init__()
+        super().__init__()
         self._config = test_run_config
         self._test_suites_with_cases = []
 
@@ -432,7 +432,7 @@ def __init__(
             test_suites_with_cases: The test suites with test cases to be run in this build target.
             build_target_config: The build target's test run configuration.
         """
-        super(BuildTargetResult, self).__init__()
+        super().__init__()
         self.arch = build_target_config.arch
         self.os = build_target_config.os
         self.cpu = build_target_config.cpu
@@ -493,7 +493,7 @@ def __init__(self, test_suite_with_cases: TestSuiteWithCases):
         Args:
             test_suite_with_cases: The test suite with test cases.
         """
-        super(TestSuiteResult, self).__init__()
+        super().__init__()
         self.test_suite_name = test_suite_with_cases.test_suite_class.__name__
         self._test_suite_with_cases = test_suite_with_cases
 
@@ -536,7 +536,7 @@ def __init__(self, test_case_name: str):
         Args:
             test_case_name: The test case's name.
         """
-        super(TestCaseResult, self).__init__()
+        super().__init__()
         self.test_case_name = test_case_name
 
     def update(self, result: Result, error: Exception | None = None) -> None:
@@ -594,7 +594,7 @@ def __init__(self, dpdk_version: str | None):
         Args:
             dpdk_version: The version of tested DPDK.
         """
-        super(Statistics, self).__init__()
+        super().__init__()
         for result in Result:
             self[result.name] = 0
         self["PASS RATE"] = 0.0
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 10d56eba8d..800fbef860 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -130,7 +130,7 @@ def __init__(self, node_config: SutNodeConfiguration):
         Args:
             node_config: The SUT node's test run configuration.
         """
-        super(SutNode, self).__init__(node_config)
+        super().__init__(node_config)
         self._dpdk_prefix_list = []
         self._build_target_config = None
         self._env_vars = {}
diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
index 164f790383..9dae56ed9c 100644
--- a/dts/framework/testbed_model/tg_node.py
+++ b/dts/framework/testbed_model/tg_node.py
@@ -48,7 +48,7 @@ def __init__(self, node_config: TGNodeConfiguration):
         Args:
             node_config: The TG node's test run configuration.
         """
-        super(TGNode, self).__init__(node_config)
+        super().__init__(node_config)
         self.traffic_generator = create_traffic_generator(self, node_config.traffic_generator)
         self._logger.info(f"Created node: {self.name}")
 
@@ -90,4 +90,4 @@ def close(self) -> None:
         This extends the superclass method with TG cleanup.
         """
         self.traffic_generator.close()
-        super(TGNode, self).close()
+        super().close()
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 4/5] dts: refine pre-test setup and teardown steps
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
                     ` (2 preceding siblings ...)
  2024-06-19 13:35   ` [PATCH v2 3/5] dts: unify super calls Juraj Linkeš
@ 2024-06-19 13:35   ` Juraj Linkeš
  2024-06-19 13:35   ` [PATCH v2 5/5] dts: clean up close in remote session Juraj Linkeš
  2024-06-20  2:43   ` [PATCH v2 0/5] node and inheritance improvements Thomas Monjalon
  5 siblings, 0 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš, Luca Vizzarro

The major part is the removal of _set_up_test_run() and
_tear_down_test_run() of Node in lieu of using super() in the
superclasses, which simplifies the code while achieving the same thing.

The minor changes are the move of virtual devices and build target
setup/teardown into SutNode from Node since both are DPDK-related which
are only going to run on the SutNode.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/testbed_model/node.py     | 66 ++-----------------------
 dts/framework/testbed_model/sut_node.py | 49 +++++++++++++-----
 2 files changed, 41 insertions(+), 74 deletions(-)

diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index fd01533d0e..d7f5d45826 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -16,12 +16,7 @@
 from ipaddress import IPv4Interface, IPv6Interface
 from typing import Any, Callable, Type, Union
 
-from framework.config import (
-    OS,
-    BuildTargetConfiguration,
-    NodeConfiguration,
-    TestRunConfiguration,
-)
+from framework.config import OS, NodeConfiguration, TestRunConfiguration
 from framework.exception import ConfigurationError
 from framework.logger import DTSLogger, get_dts_logger
 from framework.settings import SETTINGS
@@ -36,7 +31,6 @@
 from .linux_session import LinuxSession
 from .os_session import InteractiveShellType, OSSession
 from .port import Port
-from .virtual_device import VirtualDevice
 
 
 class Node(ABC):
@@ -55,7 +49,6 @@ class Node(ABC):
         lcores: The list of logical cores that DTS can use on the node.
             It's derived from logical cores present on the node and the test run configuration.
         ports: The ports of this node specified in the test run configuration.
-        virtual_devices: The virtual devices used on the node.
     """
 
     main_session: OSSession
@@ -66,7 +59,6 @@ class Node(ABC):
     _logger: DTSLogger
     _other_sessions: list[OSSession]
     _test_run_config: TestRunConfiguration
-    virtual_devices: list[VirtualDevice]
 
     def __init__(self, node_config: NodeConfiguration):
         """Connect to the node and gather info during initialization.
@@ -94,7 +86,6 @@ def __init__(self, node_config: NodeConfiguration):
         ).filter()
 
         self._other_sessions = []
-        self.virtual_devices = []
         self._init_ports()
 
     def _init_ports(self) -> None:
@@ -106,67 +97,20 @@ def _init_ports(self) -> None:
     def set_up_test_run(self, test_run_config: TestRunConfiguration) -> None:
         """Test run setup steps.
 
-        Configure hugepages and call :meth:`_set_up_test_run` where
-        the rest of the configuration steps (if any) are implemented.
+        Configure hugepages on all DTS node types. Additional steps can be added by
+        extending the method in subclasses with the use of super().
 
         Args:
             test_run_config: A test run configuration according to which
                 the setup steps will be taken.
         """
         self._setup_hugepages()
-        self._set_up_test_run(test_run_config)
-        self._test_run_config = test_run_config
-        for vdev in test_run_config.vdevs:
-            self.virtual_devices.append(VirtualDevice(vdev))
-
-    def _set_up_test_run(self, test_run_config: TestRunConfiguration) -> None:
-        """Optional additional test run setup steps for subclasses.
-
-        Subclasses should override this if they need to add additional test run setup steps.
-        """
 
     def tear_down_test_run(self) -> None:
         """Test run teardown steps.
 
-        There are currently no common test run teardown steps common to all DTS node types.
-        """
-        self.virtual_devices = []
-        self._tear_down_test_run()
-
-    def _tear_down_test_run(self) -> None:
-        """Optional additional test run teardown steps for subclasses.
-
-        Subclasses should override this if they need to add additional test run teardown steps.
-        """
-
-    def set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Build target setup steps.
-
-        There are currently no common build target setup steps common to all DTS node types.
-
-        Args:
-            build_target_config: The build target test run configuration according to which
-                the setup steps will be taken.
-        """
-        self._set_up_build_target(build_target_config)
-
-    def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Optional additional build target setup steps for subclasses.
-
-        Subclasses should override this if they need to add additional build target setup steps.
-        """
-
-    def tear_down_build_target(self) -> None:
-        """Build target teardown steps.
-
-        There are currently no common build target teardown steps common to all DTS node types.
-        """
-        self._tear_down_build_target()
-
-    def _tear_down_build_target(self) -> None:
-        """Optional additional build target teardown steps for subclasses.
-
-        Subclasses should override this if they need to add additional build target teardown steps.
+        There are currently no common execution teardown steps common to all DTS node types.
+        Additional steps can be added by extending the method in subclasses with the use of super().
         """
 
     def create_session(self, name: str) -> OSSession:
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 800fbef860..e2a520fede 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -22,6 +22,7 @@
     BuildTargetInfo,
     NodeInfo,
     SutNodeConfiguration,
+    TestRunConfiguration,
 )
 from framework.remote_session import CommandResult
 from framework.settings import SETTINGS
@@ -107,10 +108,12 @@ class SutNode(Node):
     or the git commit ID, tag ID or tree ID to test.
 
     Attributes:
-        config: The SUT node configuration
+        config: The SUT node configuration.
+        virtual_devices: The virtual devices used on the node.
     """
 
     config: SutNodeConfiguration
+    virtual_devices: list[VirtualDevice]
     _dpdk_prefix_list: list[str]
     _dpdk_timestamp: str
     _build_target_config: BuildTargetConfiguration | None
@@ -131,6 +134,7 @@ def __init__(self, node_config: SutNodeConfiguration):
             node_config: The SUT node's test run configuration.
         """
         super().__init__(node_config)
+        self.virtual_devices = []
         self._dpdk_prefix_list = []
         self._build_target_config = None
         self._env_vars = {}
@@ -228,25 +232,44 @@ def get_build_target_info(self) -> BuildTargetInfo:
     def _guess_dpdk_remote_dir(self) -> PurePath:
         return self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
 
-    def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-        """Setup DPDK on the SUT node.
+    def set_up_test_run(self, test_run_config: TestRunConfiguration) -> None:
+        """Extend the test run setup with vdev config.
 
-        Additional build target setup steps on top of those in :class:`Node`.
+        Args:
+            test_run_config: A test run configuration according to which
+                the setup steps will be taken.
+        """
+        super().set_up_test_run(test_run_config)
+        for vdev in test_run_config.vdevs:
+            self.virtual_devices.append(VirtualDevice(vdev))
+
+    def tear_down_test_run(self) -> None:
+        """Extend the test run teardown with virtual device teardown."""
+        super().tear_down_test_run()
+        self.virtual_devices = []
+
+    def set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
+        """Set up DPDK the SUT node and bind ports.
+
+        DPDK setup includes setting all internals needed for the build, the copying of DPDK tarball
+        and then building DPDK. The drivers are bound to those that DPDK needs.
+
+        Args:
+            build_target_config: The build target test run configuration according to which
+                the setup steps will be taken.
         """
-        # 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()
         self.bind_ports_to_driver()
 
-    def _tear_down_build_target(self) -> None:
-        """Bind ports to the operating system drivers.
-
-        Additional build target teardown steps on top of those in :class:`Node`.
-        """
+    def tear_down_build_target(self) -> None:
+        """Reset DPDK variables and bind port driver to the OS driver."""
+        self._env_vars = {}
+        self._build_target_config = None
+        self.__remote_dpdk_dir = None
+        self._dpdk_version = None
+        self._compiler_version = None
         self.bind_ports_to_driver(for_dpdk=False)
 
     def _configure_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 5/5] dts: clean up close in remote session
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
                     ` (3 preceding siblings ...)
  2024-06-19 13:35   ` [PATCH v2 4/5] dts: refine pre-test setup and teardown steps Juraj Linkeš
@ 2024-06-19 13:35   ` Juraj Linkeš
  2024-06-20  2:43   ` [PATCH v2 0/5] node and inheritance improvements Thomas Monjalon
  5 siblings, 0 replies; 23+ messages in thread
From: Juraj Linkeš @ 2024-06-19 13:35 UTC (permalink / raw)
  To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte
  Cc: dev, Juraj Linkeš, Luca Vizzarro

The close method was split into two methods, one with common code and
one that's subclass specific. There wasn't any common code so the split
doesn't make sense. And if we ever need such a split, we can use super()
in the future.
There was also an unused argument, force and the order of methods was
cleaned up a little (close is usually the last thing we call in the
lifecycle of a session object, so it was moved to the last place among
public methods).

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 .../remote_session/remote_session.py          | 21 ++++---------------
 dts/framework/remote_session/ssh_session.py   | 11 +++++-----
 dts/framework/testbed_model/os_session.py     | 12 ++++-------
 3 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
index 8db26e9efc..8c580b070f 100644
--- a/dts/framework/remote_session/remote_session.py
+++ b/dts/framework/remote_session/remote_session.py
@@ -191,23 +191,6 @@ def _send_command(self, command: str, timeout: float, env: dict | None) -> Comma
             * SSHTimeoutError if the command execution times out.
         """
 
-    def close(self, force: bool = False) -> None:
-        """Close the remote session and free all used resources.
-
-        Args:
-            force: Force the closure of the connection. This may not clean up all resources.
-        """
-        self._close(force)
-
-    @abstractmethod
-    def _close(self, force: bool = False) -> None:
-        """Protocol specific steps needed to close the session properly.
-
-        Args:
-            force: Force the closure of the connection. This may not clean up all resources.
-                This doesn't have to be implemented in the overloaded method.
-        """
-
     @abstractmethod
     def is_alive(self) -> bool:
         """Check whether the remote session is still responding."""
@@ -243,3 +226,7 @@ def copy_to(
             source_file: The file on the local filesystem.
             destination_file: A file or directory path on the remote Node.
         """
+
+    @abstractmethod
+    def close(self) -> None:
+        """Close the remote session and free all used resources."""
diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py
index 216bd25aed..66f8176833 100644
--- a/dts/framework/remote_session/ssh_session.py
+++ b/dts/framework/remote_session/ssh_session.py
@@ -74,10 +74,6 @@ def _connect(self) -> None:
         else:
             raise SSHConnectionError(self.hostname, errors)
 
-    def is_alive(self) -> bool:
-        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
-        return self.session.is_connected
-
     def _send_command(self, command: str, timeout: float, env: dict | None) -> CommandResult:
         """Send a command and return the result of the execution.
 
@@ -103,6 +99,10 @@ def _send_command(self, command: str, timeout: float, env: dict | None) -> Comma
 
         return CommandResult(self.name, command, output.stdout, output.stderr, output.return_code)
 
+    def is_alive(self) -> bool:
+        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
+        return self.session.is_connected
+
     def copy_from(
         self,
         source_file: str | PurePath,
@@ -119,5 +119,6 @@ def copy_to(
         """Overrides :meth:`~.remote_session.RemoteSession.copy_to`."""
         self.session.put(str(source_file), str(destination_file))
 
-    def _close(self, force: bool = False) -> None:
+    def close(self) -> None:
+        """Overrides :meth:`~.remote_session.RemoteSession.close`."""
         self.session.close()
diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
index bba4aca9ce..34b0a9e749 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/dts/framework/testbed_model/os_session.py
@@ -88,14 +88,6 @@ def __init__(
         self.remote_session = create_remote_session(node_config, name, logger)
         self.interactive_session = create_interactive_session(node_config, logger)
 
-    def close(self, force: bool = False) -> None:
-        """Close the underlying remote session.
-
-        Args:
-            force: Force the closure of the connection.
-        """
-        self.remote_session.close(force)
-
     def is_alive(self) -> bool:
         """Check whether the underlying remote session is still responding."""
         return self.remote_session.is_alive()
@@ -161,6 +153,10 @@ def create_interactive_shell(
             timeout,
         )
 
+    def close(self) -> None:
+        """Close the underlying remote session."""
+        self.remote_session.close()
+
     @staticmethod
     @abstractmethod
     def _get_privileged_command(command: str) -> str:
-- 
2.34.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 0/5] node and inheritance improvements
  2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
                     ` (4 preceding siblings ...)
  2024-06-19 13:35   ` [PATCH v2 5/5] dts: clean up close in remote session Juraj Linkeš
@ 2024-06-20  2:43   ` Thomas Monjalon
  5 siblings, 0 replies; 23+ messages in thread
From: Thomas Monjalon @ 2024-06-20  2:43 UTC (permalink / raw)
  To: Juraj Linkeš
  Cc: Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
	Luca.Vizzarro, npratte, dev

> Juraj Linkeš (5):
>   dts: add tg node test run setup and teardown
>   dts: unify class inheritance from object
>   dts: unify super calls
>   dts: refine pre-test setup and teardown steps
>   dts: clean up close in remote session

Applied, thanks.




^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2024-06-20  2:43 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-23  9:12 [PATCH v1 0/4] node and inheritance improvements Juraj Linkeš
2024-04-23  9:12 ` [PATCH v1 1/4] dts: add tg node execution setup and teardown Juraj Linkeš
2024-04-23  9:18   ` Luca Vizzarro
2024-04-30 16:15     ` Jeremy Spewock
2024-04-23  9:12 ` [PATCH v1 2/4] dts: unify class inheritance from object Juraj Linkeš
2024-04-23  9:19   ` Luca Vizzarro
2024-04-23 14:53   ` Patrick Robb
2024-04-30 16:15     ` Jeremy Spewock
2024-04-23  9:12 ` [PATCH v1 3/4] dts: unify super calls Juraj Linkeš
2024-04-23 10:06   ` Luca Vizzarro
2024-04-23 14:57   ` Patrick Robb
2024-04-30 16:15     ` Jeremy Spewock
2024-04-23  9:12 ` [PATCH v1 4/4] dts: refine pre-test setup and teardown steps Juraj Linkeš
2024-04-23  9:19   ` Luca Vizzarro
2024-04-30 16:15     ` Jeremy Spewock
2024-04-23 10:07 ` [PATCH v1 0/4] node and inheritance improvements Luca Vizzarro
2024-06-19 13:35 ` [PATCH v2 0/5] " Juraj Linkeš
2024-06-19 13:35   ` [PATCH v2 1/5] dts: add tg node test run setup and teardown Juraj Linkeš
2024-06-19 13:35   ` [PATCH v2 2/5] dts: unify class inheritance from object Juraj Linkeš
2024-06-19 13:35   ` [PATCH v2 3/5] dts: unify super calls Juraj Linkeš
2024-06-19 13:35   ` [PATCH v2 4/5] dts: refine pre-test setup and teardown steps Juraj Linkeš
2024-06-19 13:35   ` [PATCH v2 5/5] dts: clean up close in remote session Juraj Linkeš
2024-06-20  2:43   ` [PATCH v2 0/5] node and inheritance improvements Thomas Monjalon

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).