Re: [PATCH v8 18/21] dts: sut and tg nodes docstring update

[Jeremy Spewock <jspewock@iol.unh.edu>]

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

On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> Format according to the Google format and PEP257, with slight
> deviations.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>  dts/framework/testbed_model/sut_node.py | 230 ++++++++++++++++--------
>  dts/framework/testbed_model/tg_node.py  |  42 +++--
>  2 files changed, 176 insertions(+), 96 deletions(-)
>
> diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> index 5ce9446dba..c4acea38d1 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -3,6 +3,14 @@
>  # Copyright(c) 2023 PANTHEON.tech s.r.o.
>  # Copyright(c) 2023 University of New Hampshire
>
> +"""System under test (DPDK + hardware) node.
> +
> +A system under test (SUT) is the combination of DPDK
> +and the hardware we're testing with DPDK (NICs, crypto and other devices).
> +An SUT node is where this SUT runs.
> +"""
>

I think this should just be "A SUT node"


> +
> +
>  import os
>  import tarfile
>  import time
> @@ -26,6 +34,11 @@
>
>
>  class EalParameters(object):
> +    """The environment abstraction layer parameters.
> +
> +    The string representation can be created by converting the instance
> to a string.
> +    """
> +
>      def __init__(
>          self,
>          lcore_list: LogicalCoreList,
> @@ -35,21 +48,23 @@ def __init__(
>          vdevs: list[VirtualDevice],
>          other_eal_param: str,
>      ):
> -        """
> -        Generate eal parameters character string;
> -        :param lcore_list: the list of logical cores to use.
> -        :param memory_channels: the number of memory channels to use.
> -        :param prefix: set file prefix string, eg:
> -                        prefix='vf'
> -        :param no_pci: switch of disable PCI bus eg:
> -                        no_pci=True
> -        :param vdevs: virtual device list, eg:
> -                        vdevs=[
> -                            VirtualDevice('net_ring0'),
> -                            VirtualDevice('net_ring1')
> -                        ]
> -        :param other_eal_param: user defined DPDK eal parameters, eg:
> -                        other_eal_param='--single-file-segments'
> +        """Initialize the parameters according to inputs.
> +
> +        Process the parameters into the format used on the command line.
> +
> +        Args:
> +            lcore_list: The list of logical cores to use.
> +            memory_channels: The number of memory channels to use.
> +            prefix: Set the file prefix string with which to start DPDK,
> e.g.: ``prefix='vf'``.
> +            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> +            vdevs: Virtual devices, e.g.::
> +
> +                vdevs=[
> +                    VirtualDevice('net_ring0'),
> +                    VirtualDevice('net_ring1')
> +                ]
> +            other_eal_param: user defined DPDK EAL parameters, e.g.:
> +                ``other_eal_param='--single-file-segments'``
>          """
>          self._lcore_list = f"-l {lcore_list}"
>          self._memory_channels = f"-n {memory_channels}"
> @@ -61,6 +76,7 @@ def __init__(
>          self._other_eal_param = other_eal_param
>
>      def __str__(self) -> str:
> +        """Create the EAL string."""
>          return (
>              f"{self._lcore_list} "
>              f"{self._memory_channels} "
> @@ -72,11 +88,21 @@ def __str__(self) -> str:
>
>
>  class SutNode(Node):
> -    """
> -    A class for managing connections to the System under Test, providing
> -    methods that retrieve the necessary information about the node (such
> as
> -    CPU, memory and NIC details) and configuration capabilities.
> -    Another key capability is building DPDK according to given build
> target.
> +    """The system under test node.
> +
> +    The SUT node extends :class:`Node` with DPDK specific features:
> +
> +        * DPDK build,
> +        * Gathering of DPDK build info,
> +        * The running of DPDK apps, interactively or one-time execution,
> +        * DPDK apps cleanup.
> +
> +    The :option:`--tarball` command line argument and the
> :envvar:`DTS_DPDK_TARBALL`
> +    environment variable configure the path to the DPDK tarball
> +    or the git commit ID, tag ID or tree ID to test.
> +
> +    Attributes:
> +        config: The SUT node configuration
>      """
>
>      config: SutNodeConfiguration
> @@ -94,6 +120,11 @@ class SutNode(Node):
>      _path_to_devbind_script: PurePath | None
>
>      def __init__(self, node_config: SutNodeConfiguration):
> +        """Extend the constructor with SUT node specifics.
> +
> +        Args:
> +            node_config: The SUT node's test run configuration.
> +        """
>          super(SutNode, self).__init__(node_config)
>          self._dpdk_prefix_list = []
>          self._build_target_config = None
> @@ -113,6 +144,12 @@ def __init__(self, node_config: SutNodeConfiguration):
>
>      @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
> @@ -123,6 +160,11 @@ def _remote_dpdk_dir(self, value: PurePath) -> None:
>
>      @property
>      def remote_dpdk_build_dir(self) -> PurePath:
> +        """The remote DPDK build directory.
> +
> +        This is the directory where DPDK was built.
> +        We assume it was built in a subdirectory of the extracted tarball.
> +        """
>          if self._build_target_config:
>              return self.main_session.join_remote_path(
>                  self._remote_dpdk_dir, self._build_target_config.name
> @@ -132,18 +174,21 @@ def remote_dpdk_build_dir(self) -> PurePath:
>
>      @property
>      def dpdk_version(self) -> str:
> +        """Last built DPDK version."""
>          if self._dpdk_version is None:
>              self._dpdk_version =
> self.main_session.get_dpdk_version(self._remote_dpdk_dir)
>          return self._dpdk_version
>
>      @property
>      def node_info(self) -> NodeInfo:
> +        """Additional node information."""
>          if self._node_info is None:
>              self._node_info = self.main_session.get_node_info()
>          return self._node_info
>
>      @property
>      def compiler_version(self) -> str:
> +        """The node's compiler version."""
>          if self._compiler_version is None:
>              if self._build_target_config is not None:
>                  self._compiler_version =
> self.main_session.get_compiler_version(
> @@ -158,6 +203,7 @@ def compiler_version(self) -> str:
>
>      @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"
> @@ -165,6 +211,11 @@ def path_to_devbind_script(self) -> PurePath:
>          return self._path_to_devbind_script
>
>      def get_build_target_info(self) -> BuildTargetInfo:
> +        """Get additional build target information.
> +
> +        Returns:
> +            The build target information,
> +        """
>          return BuildTargetInfo(
>              dpdk_version=self.dpdk_version,
> compiler_version=self.compiler_version
>          )
> @@ -173,8 +224,9 @@ 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.
> +        """Setup DPDK on the SUT node.
> +
> +        Additional build target setup steps on top of those in
> :class:`Node`.
>          """
>          # we want to ensure that dpdk_version and compiler_version is
> reset for new
>          # build targets
> @@ -186,16 +238,14 @@ def _set_up_build_target(self, build_target_config:
> BuildTargetConfiguration) ->
>          self.bind_ports_to_driver()
>
>      def _tear_down_build_target(self) -> None:
> -        """
> -        This method exists to be optionally overwritten by derived
> classes and
> -        is not decorated so that the derived class doesn't have to use
> the decorator.
> +        """Bind ports to the operating system drivers.
> +
> +        Additional build target teardown steps on top of those in
> :class:`Node`.
>          """
>          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.
> -        """
> +        """Populate common environment variables and set build target
> config."""
>          self._env_vars = {}
>          self._build_target_config = build_target_config
>
>  self._env_vars.update(self.main_session.get_dpdk_build_env_vars(build_target_config.arch))
> @@ -207,9 +257,7 @@ def _configure_build_target(self, build_target_config:
> BuildTargetConfiguration)
>
>      @Node.skip_setup
>      def _copy_dpdk_tarball(self) -> None:
> -        """
> -        Copy to and extract DPDK tarball on the SUT node.
> -        """
> +        """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)
>
> @@ -238,8 +286,9 @@ def _copy_dpdk_tarball(self) -> None:
>
>      @Node.skip_setup
>      def _build_dpdk(self) -> None:
> -        """
> -        Build DPDK. Uses the already configured target. Assumes that the
> tarball has
> +        """Build DPDK.
> +
> +        Uses the already configured target. Assumes that the tarball has
>          already been copied to and extracted on the SUT node.
>          """
>          self.main_session.build_dpdk(
> @@ -250,15 +299,19 @@ def _build_dpdk(self) -> None:
>          )
>
>      def build_dpdk_app(self, app_name: str, **meson_dpdk_args: str |
> bool) -> PurePath:
> -        """
> -        Build one or all DPDK apps. Requires DPDK to be already built on
> the SUT node.
> -        When app_name is 'all', build all example apps.
> -        When app_name is any other string, tries to build that example
> app.
> -        Return the directory path of the built app. If building all apps,
> return
> -        the path to the examples directory (where all apps reside).
> -        The meson_dpdk_args are keyword arguments
> -        found in meson_option.txt in root DPDK directory. Do not use -D
> with them,
> -        for example: enable_kmods=True.
> +        """Build one or all DPDK apps.
> +
> +        Requires DPDK to be already built on the SUT node.
> +
> +        Args:
> +            app_name: The name of the DPDK app to build.
> +                When `app_name` is ``all``, build all example apps.
> +            meson_dpdk_args: The arguments found in ``meson_options.txt``
> in root DPDK directory.
> +                Do not use ``-D`` with them.
> +
> +        Returns:
> +            The directory path of the built app. If building all apps,
> return
> +            the path to the examples directory (where all apps reside).
>          """
>          self.main_session.build_dpdk(
>              self._env_vars,
> @@ -277,9 +330,7 @@ def build_dpdk_app(self, app_name: str,
> **meson_dpdk_args: str | bool) -> PurePa
>          )
>
>      def kill_cleanup_dpdk_apps(self) -> None:
> -        """
> -        Kill all dpdk applications on the SUT. Cleanup hugepages.
> -        """
> +        """Kill all dpdk applications on the SUT, then clean up
> hugepages."""
>          if self._dpdk_kill_session and self._dpdk_kill_session.is_alive():
>              # we can use the session if it exists and responds
>
>  self._dpdk_kill_session.kill_cleanup_dpdk_apps(self._dpdk_prefix_list)
> @@ -298,33 +349,34 @@ def create_eal_parameters(
>          vdevs: list[VirtualDevice] | None = None,
>          other_eal_param: str = "",
>      ) -> "EalParameters":
> -        """
> -        Generate eal parameters character string;
> -        :param lcore_filter_specifier: a number of lcores/cores/sockets
> to use
> -                        or a list of lcore ids to use.
> -                        The default will select one lcore for each of two
> cores
> -                        on one socket, in ascending order of core ids.
> -        :param ascending_cores: True, use cores with the lowest numerical
> id first
> -                        and continue in ascending order. If False, start
> with the
> -                        highest id and continue in descending order. This
> ordering
> -                        affects which sockets to consider first as well.
> -        :param prefix: set file prefix string, eg:
> -                        prefix='vf'
> -        :param append_prefix_timestamp: if True, will append a timestamp
> to
> -                        DPDK file prefix.
> -        :param no_pci: switch of disable PCI bus eg:
> -                        no_pci=True
> -        :param vdevs: virtual device list, eg:
> -                        vdevs=[
> -                            VirtualDevice('net_ring0'),
> -                            VirtualDevice('net_ring1')
> -                        ]
> -        :param other_eal_param: user defined DPDK eal parameters, eg:
> -                        other_eal_param='--single-file-segments'
> -        :return: eal param string, eg:
> -                '-c 0xf -a 0000:88:00.0
> --file-prefix=dpdk_1112_20190809143420';
> -        """
> +        """Compose the EAL parameters.
> +
> +        Process the list of cores and the DPDK prefix and pass that along
> with
> +        the rest of the arguments.
>
> +        Args:
> +            lcore_filter_specifier: A number of lcores/cores/sockets to
> use
> +                or a list of lcore ids to use.
> +                The default will select one lcore for each of two cores
> +                on one socket, in ascending order of core ids.
> +            ascending_cores: Sort cores in ascending order (lowest to
> highest IDs).
> +                If :data:`False`, sort in descending order.
> +            prefix: Set the file prefix string with which to start DPDK,
> e.g.: ``prefix='vf'``.
> +            append_prefix_timestamp: If :data:`True`, will append a
> timestamp to DPDK file prefix.
> +            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> +            vdevs: Virtual devices, e.g.::
> +
> +                vdevs=[
> +                    VirtualDevice('net_ring0'),
> +                    VirtualDevice('net_ring1')
> +                ]
> +            other_eal_param: user defined DPDK EAL parameters, e.g.:
> +                ``other_eal_param='--single-file-segments'``.
> +
> +        Returns:
> +            An EAL param string, such as
> +            ``-c 0xf -a 0000:88:00.0
> --file-prefix=dpdk_1112_20190809143420``.
> +        """
>          lcore_list =
> LogicalCoreList(self.filter_lcores(lcore_filter_specifier, ascending_cores))
>
>          if append_prefix_timestamp:
> @@ -348,14 +400,29 @@ def create_eal_parameters(
>      def run_dpdk_app(
>          self, app_path: PurePath, eal_args: "EalParameters", timeout:
> float = 30
>      ) -> CommandResult:
> -        """
> -        Run DPDK application on the remote node.
> +        """Run DPDK application on the remote node.
> +
> +        The application is not run interactively - the command that
> starts the application
> +        is executed and then the call waits for it to finish execution.
> +
> +        Args:
> +            app_path: The remote path to the DPDK application.
> +            eal_args: EAL parameters to run the DPDK application with.
> +            timeout: Wait at most this long in seconds for `command`
> execution to complete.
> +
> +        Returns:
> +            The result of the DPDK app execution.
>          """
>          return self.main_session.send_command(
>              f"{app_path} {eal_args}", timeout, privileged=True,
> verify=True
>          )
>
>      def configure_ipv4_forwarding(self, enable: bool) -> None:
> +        """Enable/disable IPv4 forwarding on the node.
> +
> +        Args:
> +            enable: If :data:`True`, enable the forwarding, otherwise
> disable it.
> +        """
>          self.main_session.configure_ipv4_forwarding(enable)
>
>      def create_interactive_shell(
> @@ -365,9 +432,13 @@ def create_interactive_shell(
>          privileged: bool = False,
>          eal_parameters: EalParameters | str | None = None,
>      ) -> InteractiveShellType:
> -        """Factory method for creating a handler for an interactive
> session.
> +        """Extend the factory for interactive session handlers.
> +
> +        The extensions are SUT node specific:
>
> -        Instantiate shell_cls according to the remote OS specifics.
> +            * The default for `eal_parameters`,
> +            * The interactive shell path `shell_cls.path` is prepended
> with path to the remote
> +              DPDK build directory for DPDK apps.
>
>          Args:
>              shell_cls: The class of the shell.
> @@ -377,9 +448,10 @@ def create_interactive_shell(
>              privileged: Whether to run the shell with administrative
> privileges.
>              eal_parameters: List of EAL parameters to use to launch the
> app. If this
>                  isn't provided or an empty string is passed, it will
> default to calling
> -                create_eal_parameters().
> +                :meth:`create_eal_parameters`.
> +
>          Returns:
> -            Instance of the desired interactive application.
> +            An instance of the desired interactive application shell.
>          """
>          if not eal_parameters:
>              eal_parameters = self.create_eal_parameters()
> @@ -396,8 +468,8 @@ def bind_ports_to_driver(self, for_dpdk: bool = True)
> -> None:
>          """Bind all ports on the SUT to a driver.
>
>          Args:
> -            for_dpdk: Boolean that, when True, binds ports to
> os_driver_for_dpdk
> -            or, when False, binds to os_driver. Defaults to True.
> +            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
> diff --git a/dts/framework/testbed_model/tg_node.py
> b/dts/framework/testbed_model/tg_node.py
> index 8a8f0019f3..f269d4c585 100644
> --- a/dts/framework/testbed_model/tg_node.py
> +++ b/dts/framework/testbed_model/tg_node.py
> @@ -5,13 +5,8 @@
>
>  """Traffic generator node.
>
> -This is the node where the traffic generator resides.
> -The distinction between a node and a traffic generator is as follows:
> -A node is a host that DTS connects to. It could be a baremetal server,
> -a VM or a container.
> -A traffic generator is software running on the node.
> -A traffic generator node is a node running a traffic generator.
> -A node can be a traffic generator node as well as system under test node.
> +A traffic generator (TG) generates traffic that's sent towards the SUT
> node.
> +A TG node is where the TG runs.
>  """
>
>  from scapy.packet import Packet  # type: ignore[import]
> @@ -24,13 +19,16 @@
>
>
>  class TGNode(Node):
> -    """Manage connections to a node with a traffic generator.
> +    """The traffic generator node.
>
> -    Apart from basic node management capabilities, the Traffic Generator
> node has
> -    specialized methods for handling the traffic generator running on it.
> +    The TG node extends :class:`Node` with TG specific features:
>
> -    Arguments:
> -        node_config: The user configuration of the traffic generator node.
> +        * Traffic generator initialization,
> +        * The sending of traffic and receiving packets,
> +        * The sending of traffic without receiving packets.
> +
> +    Not all traffic generators are capable of capturing traffic, which is
> why there
> +    must be a way to send traffic without that.
>
>      Attributes:
>          traffic_generator: The traffic generator running on the node.
> @@ -39,6 +37,13 @@ class TGNode(Node):
>      traffic_generator: CapturingTrafficGenerator
>
>      def __init__(self, node_config: TGNodeConfiguration):
> +        """Extend the constructor with TG node specifics.
> +
> +        Initialize the traffic generator on the TG node.
> +
> +        Args:
> +            node_config: The TG node's test run configuration.
> +        """
>          super(TGNode, self).__init__(node_config)
>          self.traffic_generator = create_traffic_generator(self,
> node_config.traffic_generator)
>          self._logger.info(f"Created node: {self.name}")
> @@ -50,17 +55,17 @@ def send_packet_and_capture(
>          receive_port: Port,
>          duration: float = 1,
>      ) -> list[Packet]:
> -        """Send a packet, return received traffic.
> +        """Send `packet`, return received traffic.
>
> -        Send a packet on the send_port and then return all traffic
> captured
> -        on the receive_port for the given duration. Also record the
> captured traffic
> +        Send `packet` on `send_port` and then return all traffic captured
> +        on `receive_port` for the given duration. Also record the
> captured traffic
>          in a pcap file.
>
>          Args:
>              packet: The packet to send.
>              send_port: The egress port on the TG node.
>              receive_port: The ingress port in the TG node.
> -            duration: Capture traffic for this amount of time after
> sending the packet.
> +            duration: Capture traffic for this amount of time after
> sending `packet`.
>
>          Returns:
>               A list of received packets. May be empty if no packets are
> captured.
> @@ -70,6 +75,9 @@ def send_packet_and_capture(
>          )
>
>      def close(self) -> None:
> -        """Free all resources used by the node"""
> +        """Free all resources used by the node.
> +
> +        This extends the superclass method with TG cleanup.
> +        """
>          self.traffic_generator.close()
>          super(TGNode, self).close()
> --
> 2.34.1
>
>

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