Re: [PATCH 1/1] dts: add binding to different drivers to TG node

[Nicholas Pratte <npratte@iol.unh.edu>]

Makes sense to me!

Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

On Mon, Aug 12, 2024 at 1:23 PM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> The DTS framework in its current state supports binding ports to
> different drivers on the SUT node but not the TG node. The TG node
> already has the information that it needs about the different drivers
> that it has available in the configuration file, but it did not
> previously have access to the devbind script, so it did not use that
> information for anything.
>
> This patch moves the steps to copy the DPDK tarball into the node class
> rather than the SUT node class, and calls this function on the TG node
> as well as the SUT. It also moves the driver binding step into the Node
> class and triggers the same pattern of binding to ports that existed on
> the SUT on the TG.
>
> Bugzilla ID: 1420
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/runner.py                 |   2 +
>  dts/framework/testbed_model/node.py     | 106 +++++++++++++++++++++++-
>  dts/framework/testbed_model/sut_node.py |  86 +------------------
>  3 files changed, 109 insertions(+), 85 deletions(-)
>
> diff --git a/dts/framework/runner.py b/dts/framework/runner.py
> index 6b6f6a05f5..ed9e58b172 100644
> --- a/dts/framework/runner.py
> +++ b/dts/framework/runner.py
> @@ -484,6 +484,7 @@ def _run_build_target(
>
>          try:
>              sut_node.set_up_build_target(build_target_config)
> +            tg_node.set_up_build_target(build_target_config)
>              self._result.dpdk_version = sut_node.dpdk_version
>              build_target_result.add_build_target_info(sut_node.get_build_target_info())
>              build_target_result.update_setup(Result.PASS)
> @@ -498,6 +499,7 @@ def _run_build_target(
>              try:
>                  self._logger.set_stage(DtsStage.build_target_teardown)
>                  sut_node.tear_down_build_target()
> +                tg_node.tear_down_build_target()
>                  build_target_result.update_teardown(Result.PASS)
>              except Exception as e:
>                  self._logger.exception("Build target teardown failed.")
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index 12a40170ac..8e6181e424 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -13,11 +13,19 @@
>  The :func:`~Node.skip_setup` decorator can be used without subclassing.
>  """
>
> +import os
> +import tarfile
>  from abc import ABC
>  from ipaddress import IPv4Interface, IPv6Interface
> +from pathlib import PurePath
>  from typing import Any, Callable, Union
>
> -from framework.config import OS, NodeConfiguration, TestRunConfiguration
> +from framework.config import (
> +    OS,
> +    BuildTargetConfiguration,
> +    NodeConfiguration,
> +    TestRunConfiguration,
> +)
>  from framework.exception import ConfigurationError
>  from framework.logger import DTSLogger, get_dts_logger
>  from framework.settings import SETTINGS
> @@ -58,8 +66,11 @@ class Node(ABC):
>      lcores: list[LogicalCore]
>      ports: list[Port]
>      _logger: DTSLogger
> +    _remote_tmp_dir: PurePath
> +    __remote_dpdk_dir: PurePath | None
>      _other_sessions: list[OSSession]
>      _test_run_config: TestRunConfiguration
> +    _path_to_devbind_script: PurePath | None
>
>      def __init__(self, node_config: NodeConfiguration):
>          """Connect to the node and gather info during initialization.
> @@ -88,6 +99,9 @@ def __init__(self, node_config: NodeConfiguration):
>
>          self._other_sessions = []
>          self._init_ports()
> +        self._remote_tmp_dir = self.main_session.get_remote_tmp_dir()
> +        self.__remote_dpdk_dir = None
> +        self._path_to_devbind_script = None
>
>      def _init_ports(self) -> None:
>          self.ports = [Port(self.name, port_config) for port_config in self.config.ports]
> @@ -95,6 +109,34 @@ def _init_ports(self) -> None:
>          for port in self.ports:
>              self.configure_port_state(port)
>
> +    def _guess_dpdk_remote_dir(self) -> PurePath:
> +        return self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
> +
> +    @property
> +    def _remote_dpdk_dir(self) -> PurePath:
> +        """The remote DPDK dir.
> +
> +        This internal property should be set after extracting the DPDK tarball. If it's not set,
> +        that implies the DPDK setup step has been skipped, in which case we can guess where
> +        a previous build was located.
> +        """
> +        if self.__remote_dpdk_dir is None:
> +            self.__remote_dpdk_dir = self._guess_dpdk_remote_dir()
> +        return self.__remote_dpdk_dir
> +
> +    @_remote_dpdk_dir.setter
> +    def _remote_dpdk_dir(self, value: PurePath) -> None:
> +        self.__remote_dpdk_dir = value
> +
> +    @property
> +    def path_to_devbind_script(self) -> PurePath:
> +        """The path to the dpdk-devbind.py script on the node."""
> +        if self._path_to_devbind_script is None:
> +            self._path_to_devbind_script = self.main_session.join_remote_path(
> +                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> +            )
> +        return self._path_to_devbind_script
> +
>      def set_up_test_run(self, test_run_config: TestRunConfiguration) -> None:
>          """Test run setup steps.
>
> @@ -114,6 +156,24 @@ def tear_down_test_run(self) -> None:
>          Additional steps can be added by extending the method in subclasses with the use of super().
>          """
>
> +    def set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
> +        """Set up DPDK the 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.
> +        """
> +        self._copy_dpdk_tarball()
> +        self.bind_ports_to_driver()
> +
> +    def tear_down_build_target(self) -> None:
> +        """Reset DPDK variables and bind port driver to the OS driver."""
> +        self.__remote_dpdk_dir = None
> +        self.bind_ports_to_driver(for_dpdk=False)
> +
>      def create_session(self, name: str) -> OSSession:
>          """Create and return a new OS-aware remote session.
>
> @@ -228,6 +288,50 @@ def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]:
>          else:
>              return func
>
> +    @skip_setup
> +    def _copy_dpdk_tarball(self) -> None:
> +        """Copy to and extract DPDK tarball on the node."""
> +        self._logger.info(f"Copying DPDK tarball to {self.name}.")
> +        self.main_session.copy_to(SETTINGS.dpdk_tarball_path, self._remote_tmp_dir)
> +
> +        # construct remote tarball path
> +        # the basename is the same on local host and on remote Node
> +        remote_tarball_path = self.main_session.join_remote_path(
> +            self._remote_tmp_dir, os.path.basename(SETTINGS.dpdk_tarball_path)
> +        )
> +
> +        # construct remote path after extracting
> +        with tarfile.open(SETTINGS.dpdk_tarball_path) as dpdk_tar:
> +            dpdk_top_dir = dpdk_tar.getnames()[0]
> +        self._remote_dpdk_dir = self.main_session.join_remote_path(
> +            self._remote_tmp_dir, dpdk_top_dir
> +        )
> +
> +        self._logger.info(
> +            f"Extracting DPDK tarball on {self.name}: "
> +            f"'{remote_tarball_path}' into '{self._remote_dpdk_dir}'."
> +        )
> +        # clean remote path where we're extracting
> +        self.main_session.remove_remote_dir(self._remote_dpdk_dir)
> +
> +        # then extract to remote path
> +        self.main_session.extract_remote_tarball(remote_tarball_path, self._remote_dpdk_dir)
> +
> +    def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
> +        """Bind all ports on the node to a driver.
> +
> +        Args:
> +            for_dpdk: If :data:`True`, binds ports to os_driver_for_dpdk.
> +                If :data:`False`, binds to os_driver.
> +        """
> +        for port in self.ports:
> +            driver = port.os_driver_for_dpdk if for_dpdk else port.os_driver
> +            self.main_session.send_command(
> +                f"{self.path_to_devbind_script} -b {driver} --force {port.pci}",
> +                privileged=True,
> +                verify=True,
> +            )
> +
>
>  def create_session(node_config: NodeConfiguration, name: str, logger: DTSLogger) -> OSSession:
>      """Factory for OS-aware sessions.
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 2855fe0276..f3fd4e2304 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -13,7 +13,6 @@
>
>
>  import os
> -import tarfile
>  import time
>  from pathlib import PurePath
>
> @@ -26,7 +25,6 @@
>  )
>  from framework.params.eal import EalParams
>  from framework.remote_session.remote_session import CommandResult
> -from framework.settings import SETTINGS
>  from framework.utils import MesonArgs
>
>  from .node import Node
> @@ -59,14 +57,11 @@ class SutNode(Node):
>      dpdk_timestamp: str
>      _build_target_config: BuildTargetConfiguration | None
>      _env_vars: dict
> -    _remote_tmp_dir: PurePath
> -    __remote_dpdk_dir: PurePath | None
>      _app_compile_timeout: float
>      _dpdk_kill_session: OSSession | None
>      _dpdk_version: str | None
>      _node_info: NodeInfo | None
>      _compiler_version: str | None
> -    _path_to_devbind_script: PurePath | None
>
>      def __init__(self, node_config: SutNodeConfiguration):
>          """Extend the constructor with SUT node specifics.
> @@ -79,8 +74,6 @@ def __init__(self, node_config: SutNodeConfiguration):
>          self.dpdk_prefix_list = []
>          self._build_target_config = None
>          self._env_vars = {}
> -        self._remote_tmp_dir = self.main_session.get_remote_tmp_dir()
> -        self.__remote_dpdk_dir = None
>          self._app_compile_timeout = 90
>          self._dpdk_kill_session = None
>          self.dpdk_timestamp = (
> @@ -89,25 +82,8 @@ def __init__(self, node_config: SutNodeConfiguration):
>          self._dpdk_version = None
>          self._node_info = None
>          self._compiler_version = None
> -        self._path_to_devbind_script = None
>          self._logger.info(f"Created node: {self.name}")
>
> -    @property
> -    def _remote_dpdk_dir(self) -> PurePath:
> -        """The remote DPDK dir.
> -
> -        This internal property should be set after extracting the DPDK tarball. If it's not set,
> -        that implies the DPDK setup step has been skipped, in which case we can guess where
> -        a previous build was located.
> -        """
> -        if self.__remote_dpdk_dir is None:
> -            self.__remote_dpdk_dir = self._guess_dpdk_remote_dir()
> -        return self.__remote_dpdk_dir
> -
> -    @_remote_dpdk_dir.setter
> -    def _remote_dpdk_dir(self, value: PurePath) -> None:
> -        self.__remote_dpdk_dir = value
> -
>      @property
>      def remote_dpdk_build_dir(self) -> PurePath:
>          """The remote DPDK build directory.
> @@ -151,15 +127,6 @@ def compiler_version(self) -> str:
>                  return ""
>          return self._compiler_version
>
> -    @property
> -    def path_to_devbind_script(self) -> PurePath:
> -        """The path to the dpdk-devbind.py script on the node."""
> -        if self._path_to_devbind_script is None:
> -            self._path_to_devbind_script = self.main_session.join_remote_path(
> -                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> -            )
> -        return self._path_to_devbind_script
> -
>      def get_build_target_info(self) -> BuildTargetInfo:
>          """Get additional build target information.
>
> @@ -170,9 +137,6 @@ def get_build_target_info(self) -> BuildTargetInfo:
>              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)
> -
>      def set_up_test_run(self, test_run_config: TestRunConfiguration) -> None:
>          """Extend the test run setup with vdev config.
>
> @@ -199,19 +163,17 @@ def set_up_build_target(self, build_target_config: BuildTargetConfiguration) ->
>              build_target_config: The build target test run configuration according to which
>                  the setup steps will be taken.
>          """
> +        super().set_up_build_target(build_target_config)
>          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:
>          """Reset DPDK variables and bind port driver to the OS driver."""
> +        super().tear_down_build_target()
>          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:
>          """Populate common environment variables and set build target config."""
> @@ -224,35 +186,6 @@ def _configure_build_target(self, build_target_config: BuildTargetConfiguration)
>                  f"'{build_target_config.compiler_wrapper} {build_target_config.compiler.name}'"
>              )  # fmt: skip
>
> -    @Node.skip_setup
> -    def _copy_dpdk_tarball(self) -> None:
> -        """Copy to and extract DPDK tarball on the SUT node."""
> -        self._logger.info("Copying DPDK tarball to SUT.")
> -        self.main_session.copy_to(SETTINGS.dpdk_tarball_path, self._remote_tmp_dir)
> -
> -        # construct remote tarball path
> -        # the basename is the same on local host and on remote Node
> -        remote_tarball_path = self.main_session.join_remote_path(
> -            self._remote_tmp_dir, os.path.basename(SETTINGS.dpdk_tarball_path)
> -        )
> -
> -        # construct remote path after extracting
> -        with tarfile.open(SETTINGS.dpdk_tarball_path) as dpdk_tar:
> -            dpdk_top_dir = dpdk_tar.getnames()[0]
> -        self._remote_dpdk_dir = self.main_session.join_remote_path(
> -            self._remote_tmp_dir, dpdk_top_dir
> -        )
> -
> -        self._logger.info(
> -            f"Extracting DPDK tarball on SUT: "
> -            f"'{remote_tarball_path}' into '{self._remote_dpdk_dir}'."
> -        )
> -        # clean remote path where we're extracting
> -        self.main_session.remove_remote_dir(self._remote_dpdk_dir)
> -
> -        # then extract to remote path
> -        self.main_session.extract_remote_tarball(remote_tarball_path, self._remote_dpdk_dir)
> -
>      @Node.skip_setup
>      def _build_dpdk(self) -> None:
>          """Build DPDK.
> @@ -335,18 +268,3 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:
>              enable: If :data:`True`, enable the forwarding, otherwise disable it.
>          """
>          self.main_session.configure_ipv4_forwarding(enable)
> -
> -    def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
> -        """Bind all ports on the SUT to a driver.
> -
> -        Args:
> -            for_dpdk: If :data:`True`, binds ports to os_driver_for_dpdk.
> -                If :data:`False`, binds to os_driver.
> -        """
> -        for port in self.ports:
> -            driver = port.os_driver_for_dpdk if for_dpdk else port.os_driver
> -            self.main_session.send_command(
> -                f"{self.path_to_devbind_script} -b {driver} --force {port.pci}",
> -                privileged=True,
> -                verify=True,
> -            )
> --
> 2.45.2
>