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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Wed, Nov 22, 2023 at 2:13 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 11/15/23 13:09, Juraj Linkeš 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 | 224 ++++++++++++++++--------
> >   dts/framework/testbed_model/tg_node.py  |  42 +++--
> >   2 files changed, 173 insertions(+), 93 deletions(-)
> >
> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> > index 17deea06e2..123b16fee0 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.
> > +"""
> > +
> > +
> >   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.
>
> I just want to make sure. We use the --tarball option also to set a git
> commit id instead of a tarball as the source?
>

Yes. The purpose of the option is to specify the DPDK to test and
there are different accepted formats for the option (a tarball or a
git commit id). There are actually three aliases for the option:
--tarball, --snapshot, --git-ref, but none of the aliases capture the
dichotomous nature of the option.

> > +
> > +    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,6 +174,7 @@ 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
> > @@ -140,12 +183,14 @@ def dpdk_version(self) -> str:
> >
> >       @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(
> > @@ -161,6 +206,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"
> > @@ -168,6 +214,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
> >           )
> > @@ -178,8 +229,9 @@ def _guess_dpdk_remote_dir(self) -> PurePath:
> >       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
> > @@ -200,9 +252,7 @@ def _tear_down_build_target(self) -> None:
> >       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(
> > @@ -217,9 +267,7 @@ def _configure_build_target(
> >
> >       @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)
> >
> > @@ -250,8 +298,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(
> > @@ -262,15 +311,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,
> > @@ -291,9 +344,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)
> > @@ -312,33 +363,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)
> >           )
> > @@ -364,14 +416,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 to execute the command.
>
> confusing timeout
>

Ack.

> > +
> > +        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(
> > @@ -381,9 +448,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.
> > @@ -393,9 +464,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()
> > @@ -414,8 +486,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 166eb8430e..69eb33ccb1 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
> > @@ -52,17 +57,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.
> > @@ -72,6 +77,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()
>