[PATCH v1 0/4] node and inheritance improvements

[PATCH v1 0/4] node and inheritance improvements

[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

[PATCH v1 1/4] dts: add tg node execution setup and teardown

[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

Re: [PATCH v1 1/4] dts: add tg node execution setup and teardown

[Luca Vizzarro]

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

Re: [PATCH v1 1/4] dts: add tg node execution setup and teardown

[Jeremy Spewock]

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

[PATCH v1 2/4] dts: unify class inheritance from object

[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

Re: [PATCH v1 2/4] dts: unify class inheritance from object

[Luca Vizzarro]

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

Re: [PATCH v1 2/4] dts: unify class inheritance from object

[Patrick Robb]

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

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

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

Re: [PATCH v1 2/4] dts: unify class inheritance from object

[Jeremy Spewock]

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

[PATCH v1 3/4] dts: unify super calls

[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

Re: [PATCH v1 3/4] dts: unify super calls

[Luca Vizzarro]

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

Re: [PATCH v1 3/4] dts: unify super calls

[Patrick Robb]

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

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

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

Re: [PATCH v1 3/4] dts: unify super calls

[Jeremy Spewock]

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

[PATCH v1 4/4] dts: refine pre-test setup and teardown steps

[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

Re: [PATCH v1 4/4] dts: refine pre-test setup and teardown steps

[Luca Vizzarro]

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

Re: [PATCH v1 4/4] dts: refine pre-test setup and teardown steps

[Jeremy Spewock]

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

Re: [PATCH v1 0/4] node and inheritance improvements

[Luca Vizzarro]

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