<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Format according to the Google format and PEP257, with slight<br>
deviations.<br>
<br>
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech><br>
---<br>
 dts/framework/testbed_model/os_session.py | 272 ++++++++++++++++------<br>
 1 file changed, 205 insertions(+), 67 deletions(-)<br>
<br>
diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py<br>
index 76e595a518..cfdbd1c4bd 100644<br>
--- a/dts/framework/testbed_model/os_session.py<br>
+++ b/dts/framework/testbed_model/os_session.py<br>
@@ -2,6 +2,26 @@<br>
 # Copyright(c) 2023 PANTHEON.tech s.r.o.<br>
 # Copyright(c) 2023 University of New Hampshire<br>
<br>
+"""OS-aware remote session.<br>
+<br>
+DPDK supports multiple different operating systems, meaning it can run on these different operating<br>
+systems. This module defines the common API that OS-unaware layers use and translates the API into<br>
+OS-aware calls/utility usage.<br>
+<br>
+Note:<br>
+    Running commands with administrative privileges requires OS awareness. This is the only layer<br>
+    that's aware of OS differences, so this is where non-privileged command get converted<br>
+    to privileged commands.<br>
+<br>
+Example:<br>
+    A user wishes to remove a directory on a remote :class:`~.sut_node.SutNode`.<br>
+    The :class:`~.sut_node.SutNode` object isn't aware what OS the node is running - it delegates<br>
+    the OS translation logic to :attr:`~.node.Node.main_session`. The SUT node calls<br>
+    :meth:`~OSSession.remove_remote_dir` with a generic, OS-unaware path and<br>
+    the :attr:`~.node.Node.main_session` translates that to ``rm -rf`` if the node's OS is Linux<br>
+    and other commands for other OSs. It also translates the path to match the underlying OS.<br>
+"""<br>
+<br>
 from abc import ABC, abstractmethod<br>
 from collections.abc import Iterable<br>
 from ipaddress import IPv4Interface, IPv6Interface<br>
@@ -28,10 +48,16 @@<br>
<br>
<br>
 class OSSession(ABC):<br>
-    """<br>
-    The OS classes create a DTS node remote session and implement OS specific<br>
+    """OS-unaware to OS-aware translation API definition.<br>
+<br>
+    The OSSession classes create a remote session to a DTS node and implement OS specific<br>
     behavior. There a few control methods implemented by the base class, the rest need<br>
-    to be implemented by derived classes.<br>
+    to be implemented by subclasses.<br>
+<br>
+    Attributes:<br>
+        name: The name of the session.<br>
+        remote_session: The remote session maintaining the connection to the node.<br>
+        interactive_session: The interactive remote session maintaining the connection to the node.<br>
     """<br>
<br>
     _config: NodeConfiguration<br>
@@ -46,6 +72,15 @@ def __init__(<br>
         name: str,<br>
         logger: DTSLOG,<br>
     ):<br>
+        """Initialize the OS-aware session.<br>
+<br>
+        Connect to the node right away and also create an interactive remote session.<br>
+<br>
+        Args:<br>
+            node_config: The test run configuration of the node to connect to.<br>
+            name: The name of the session.<br>
+            logger: The logger instance this session will use.<br>
+        """<br>
         self._config = node_config<br>
         <a href="http://self.name" rel="noreferrer" target="_blank">self.name</a> = name<br>
         self._logger = logger<br>
@@ -53,15 +88,15 @@ def __init__(<br>
         self.interactive_session = create_interactive_session(node_config, logger)<br>
<br>
     def close(self, force: bool = False) -> None:<br>
-        """<br>
-        Close the remote session.<br>
+        """Close the underlying remote session.<br>
+<br>
+        Args:<br>
+            force: Force the closure of the connection.<br>
         """<br>
         self.remote_session.close(force)<br>
<br>
     def is_alive(self) -> bool:<br>
-        """<br>
-        Check whether the remote session is still responding.<br>
-        """<br>
+        """Check whether the underlying remote session is still responding."""<br>
         return self.remote_session.is_alive()<br>
<br>
     def send_command(<br>
@@ -72,10 +107,23 @@ def send_command(<br>
         verify: bool = False,<br>
         env: dict | None = None,<br>
     ) -> CommandResult:<br>
-        """<br>
-        An all-purpose API in case the command to be executed is already<br>
-        OS-agnostic, such as when the path to the executed command has been<br>
-        constructed beforehand.<br>
+        """An all-purpose API for OS-agnostic commands.<br>
+<br>
+        This can be used for an execution of a portable command that's executed the same way<br>
+        on all operating systems, such as Python.<br>
+<br>
+        The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`<br>
+        environment variable configure the timeout of command execution.<br>
+<br>
+        Args:<br>
+            command: The command to execute.<br>
+            timeout: Wait at most this long in seconds for `command` execution to complete.<br>
+            privileged: Whether to run the command with administrative privileges.<br>
+            verify: If :data:`True`, will check the exit code of the command.<br>
+            env: A dictionary with environment variables to be used with the command execution.<br>
+<br>
+        Raises:<br>
+            RemoteCommandExecutionError: If verify is :data:`True` and the command failed.<br>
         """<br>
         if privileged:<br>
             command = self._get_privileged_command(command)<br>
@@ -89,8 +137,20 @@ def create_interactive_shell(<br>
         privileged: bool,<br>
         app_args: str,<br>
     ) -> InteractiveShellType:<br>
-        """<br>
-        See "create_interactive_shell" in SutNode<br>
+        """Factory for interactive session handlers.<br>
+<br>
+        Instantiate `shell_cls` according to the remote OS specifics.<br>
+<br>
+        Args:<br>
+            shell_cls: The class of the shell.<br>
+            timeout: Timeout for reading output from the SSH channel. If you are<br>
+                reading from the buffer and don't receive any data within the timeout<br>
+                it will throw an error.<br>
+            privileged: Whether to run the shell with administrative privileges.<br>
+            app_args: The arguments to be passed to the application.<br>
+<br>
+        Returns:<br>
+            An instance of the desired interactive application shell.<br>
         """<br>
         return shell_cls(<br>
             self.interactive_session.session,<br>
@@ -114,27 +174,42 @@ def _get_privileged_command(command: str) -> str:<br>
<br>
     @abstractmethod<br>
     def guess_dpdk_remote_dir(self, remote_dir: str | PurePath) -> PurePath:<br>
-        """<br>
-        Try to find DPDK remote dir in remote_dir.<br>
+        """Try to find DPDK directory in `remote_dir`.<br>
+<br>
+        The directory is the one which is created after the extraction of the tarball. The files<br>
+        are usually extracted into a directory starting with ``dpdk-``.<br>
+<br>
+        Returns:<br>
+            The absolute path of the DPDK remote directory, empty path if not found.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_remote_tmp_dir(self) -> PurePath:<br>
-        """<br>
-        Get the path of the temporary directory of the remote OS.<br>
+        """Get the path of the temporary directory of the remote OS.<br>
+<br>
+        Returns:<br>
+            The absolute path of the temporary directory.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_dpdk_build_env_vars(self, arch: Architecture) -> dict:<br>
-        """<br>
-        Create extra environment variables needed for the target architecture. Get<br>
-        information from the node if needed.<br>
+        """Create extra environment variables needed for the target architecture.<br>
+<br>
+        Different architectures may require different configuration, such as setting 32-bit CFLAGS.<br>
+<br>
+        Returns:<br>
+            A dictionary with keys as environment variables.<br>
         """<br>
<br>
     @abstractmethod<br>
     def join_remote_path(self, *args: str | PurePath) -> PurePath:<br>
-        """<br>
-        Join path parts using the path separator that fits the remote OS.<br>
+        """Join path parts using the path separator that fits the remote OS.<br>
+<br>
+        Args:<br>
+            args: Any number of paths to join.<br>
+<br>
+        Returns:<br>
+            The resulting joined path.<br>
         """<br>
<br>
     @abstractmethod<br>
@@ -143,13 +218,13 @@ def copy_from(<br>
         source_file: str | PurePath,<br>
         destination_file: str | PurePath,<br>
     ) -> None:<br>
-        """Copy a file from the remote Node to the local filesystem.<br>
+        """Copy a file from the remote node to the local filesystem.<br>
<br>
-        Copy source_file from the remote Node associated with this remote<br>
-        session to destination_file on the local filesystem.<br>
+        Copy `source_file` from the remote node associated with this remote<br>
+        session to `destination_file` on the local filesystem.<br>
<br>
         Args:<br>
-            source_file: the file on the remote Node.<br>
+            source_file: the file on the remote node.<br>
             destination_file: a file or directory path on the local filesystem.<br>
         """<br>
<br>
@@ -159,14 +234,14 @@ def copy_to(<br>
         source_file: str | PurePath,<br>
         destination_file: str | PurePath,<br>
     ) -> None:<br>
-        """Copy a file from local filesystem to the remote Node.<br>
+        """Copy a file from local filesystem to the remote node.<br>
<br>
-        Copy source_file from local filesystem to destination_file<br>
-        on the remote Node associated with this remote session.<br>
+        Copy `source_file` from local filesystem to `destination_file`<br>
+        on the remote node associated with this remote session.<br>
<br>
         Args:<br>
             source_file: the file on the local filesystem.<br>
-            destination_file: a file or directory path on the remote Node.<br>
+            destination_file: a file or directory path on the remote node.<br>
         """<br>
<br>
     @abstractmethod<br>
@@ -176,8 +251,12 @@ def remove_remote_dir(<br>
         recursive: bool = True,<br>
         force: bool = True,<br>
     ) -> None:<br>
-        """<br>
-        Remove remote directory, by default remove recursively and forcefully.<br>
+        """Remove remote directory, by default remove recursively and forcefully.<br>
+<br>
+        Args:<br>
+            remote_dir_path: The path of the directory to remove.<br>
+            recursive: If :data:`True`, also remove all contents inside the directory.<br>
+            force: If :data:`True`, ignore all warnings and try to remove at all costs.<br>
         """<br>
<br>
     @abstractmethod<br>
@@ -186,9 +265,12 @@ def extract_remote_tarball(<br>
         remote_tarball_path: str | PurePath,<br>
         expected_dir: str | PurePath | None = None,<br>
     ) -> None:<br>
-        """<br>
-        Extract remote tarball in place. If expected_dir is a non-empty string, check<br>
-        whether the dir exists after extracting the archive.<br>
+        """Extract remote tarball in its remote directory.<br>
+<br>
+        Args:<br>
+            remote_tarball_path: The path of the tarball on the remote node.<br>
+            expected_dir: If non-empty, check whether `expected_dir` exists after extracting<br>
+                the archive.<br>
         """<br>
<br>
     @abstractmethod<br>
@@ -201,69 +283,119 @@ def build_dpdk(<br>
         rebuild: bool = False,<br>
         timeout: float = SETTINGS.compile_timeout,<br>
     ) -> None:<br>
-        """<br>
-        Build DPDK in the input dir with specified environment variables and meson<br>
-        arguments.<br>
+        """Build DPDK on the remote node.<br>
+<br>
+        An extracted DPDK tarball must be present on the node. The build consists of two steps::<br>
+<br>
+            meson setup <meson args> remote_dpdk_dir remote_dpdk_build_dir<br>
+            ninja -C remote_dpdk_build_dir<br>
+<br>
+        The :option:`--compile-timeout` command line argument and the :envvar:`DTS_COMPILE_TIMEOUT`<br>
+        environment variable configure the timeout of DPDK build.<br>
+<br>
+        Args:<br>
+            env_vars: Use these environment variables then building DPDK.<br></blockquote><div><br></div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think this is meant to be "when building DPDK" instead.<br></div><div style="font-family:arial,sans-serif" class="gmail_default"> </div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+            meson_args: Use these meson arguments when building DPDK.<br>
+            remote_dpdk_dir: The directory on the remote node where DPDK will be built.<br>
+            remote_dpdk_build_dir: The target build directory on the remote node.<br>
+            rebuild: If :data:`True`, do a subsequent build with ``meson configure`` instead<br>
+                of ``meson setup``.<br>
+            timeout: Wait at most this long in seconds for the build execution to complete.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_dpdk_version(self, version_path: str | PurePath) -> str:<br>
-        """<br>
-        Inspect DPDK version on the remote node from version_path.<br>
+        """Inspect the DPDK version on the remote node.<br>
+<br>
+        Args:<br>
+            version_path: The path to the VERSION file containing the DPDK version.<br>
+<br>
+        Returns:<br>
+            The DPDK version.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:<br>
-        """<br>
-        Compose a list of LogicalCores present on the remote node.<br>
-        If use_first_core is False, the first physical core won't be used.<br>
+        r"""Get the list of :class:`~.cpu.LogicalCore`\s on the remote node.<br>
+<br>
+        Args:<br>
+            use_first_core: If :data:`False`, the first physical core won't be used.<br>
+<br>
+        Returns:<br>
+            The logical cores present on the node.<br>
         """<br>
<br>
     @abstractmethod<br>
     def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None:<br>
-        """<br>
-        Kill and cleanup all DPDK apps identified by dpdk_prefix_list. If<br>
-        dpdk_prefix_list is empty, attempt to find running DPDK apps to kill and clean.<br>
+        """Kill and cleanup all DPDK apps.<br>
+<br>
+        Args:<br>
+            dpdk_prefix_list: Kill all apps identified by `dpdk_prefix_list`.<br>
+                If `dpdk_prefix_list` is empty, attempt to find running DPDK apps to kill and clean.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str:<br>
-        """<br>
-        Get the DPDK file prefix that will be used when running DPDK apps.<br>
+        """Make OS-specific modification to the DPDK file prefix.<br>
+<br>
+        Args:<br>
+           dpdk_prefix: The OS-unaware file prefix.<br>
+<br>
+        Returns:<br>
+            The OS-specific file prefix.<br>
         """<br>
<br>
     @abstractmethod<br>
-    def setup_hugepages(self, hugepage_amount: int, force_first_numa: bool) -> None:<br>
-        """<br>
-        Get the node's Hugepage Size, configure the specified amount of hugepages<br>
+    def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None:<br>
+        """Configure hugepages on the node.<br>
+<br>
+        Get the node's Hugepage Size, configure the specified count of hugepages<br>
         if needed and mount the hugepages if needed.<br>
-        If force_first_numa is True, configure hugepages just on the first socket.<br>
+<br>
+        Args:<br>
+            hugepage_count: Configure this many hugepages.<br>
+            force_first_numa:  If :data:`True`, configure hugepages just on the first numa node.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_compiler_version(self, compiler_name: str) -> str:<br>
-        """<br>
-        Get installed version of compiler used for DPDK<br>
+        """Get installed version of compiler used for DPDK.<br>
+<br>
+        Args:<br>
+            compiler_name: The name of the compiler executable.<br>
+<br>
+        Returns:<br>
+            The compiler's version.<br>
         """<br>
<br>
     @abstractmethod<br>
     def get_node_info(self) -> NodeInfo:<br>
-        """<br>
-        Collect information about the node<br>
+        """Collect additional information about the node.<br>
+<br>
+        Returns:<br>
+            Node information.<br>
         """<br>
<br>
     @abstractmethod<br>
     def update_ports(self, ports: list[Port]) -> None:<br>
-        """<br>
-        Get additional information about ports:<br>
-            Logical name (e.g. enp7s0) if applicable<br>
-            Mac address<br>
+        """Get additional information about ports from the operating system and update them.<br>
+<br>
+        The additional information is:<br>
+<br>
+            * Logical name (e.g. ``enp7s0``) if applicable,<br>
+            * Mac address.<br>
+<br>
+        Args:<br>
+            ports: The ports to update.<br>
         """<br>
<br>
     @abstractmethod<br>
     def configure_port_state(self, port: Port, enable: bool) -> None:<br>
-        """<br>
-        Enable/disable port.<br>
+        """Enable/disable `port` in the operating system.<br>
+<br>
+        Args:<br>
+            port: The port to configure.<br>
+            enable: If :data:`True`, enable the port, otherwise shut it down.<br>
         """<br>
<br>
     @abstractmethod<br>
@@ -273,12 +405,18 @@ def configure_port_ip_address(<br>
         port: Port,<br>
         delete: bool,<br>
     ) -> None:<br>
-        """<br>
-        Configure (add or delete) an IP address of the input port.<br>
+        """Configure an IP address on `port` in the operating system.<br>
+<br>
+        Args:<br>
+            address: The address to configure.<br>
+            port: The port to configure.<br>
+            delete: If :data:`True`, remove the IP address, otherwise configure it.<br>
         """<br>
<br>
     @abstractmethod<br>
     def configure_ipv4_forwarding(self, enable: bool) -> None:<br>
-        """<br>
-        Enable IPv4 forwarding in the underlying OS.<br>
+        """Enable IPv4 forwarding in the operating system.<br>
+<br>
+        Args:<br>
+            enable: If :data:`True`, enable the forwarding, otherwise disable it.<br>
         """<br>
-- <br>
2.34.1<br>
<br>
</blockquote></div></div>