From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1CB3A4339B; Wed, 22 Nov 2023 12:50:24 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 011FF40DDC; Wed, 22 Nov 2023 12:50:24 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 1384D4028C for ; Wed, 22 Nov 2023 12:50:22 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 54F811595; Wed, 22 Nov 2023 03:51:08 -0800 (PST) Received: from [10.57.72.130] (unknown [10.57.72.130]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E56D93F7A6; Wed, 22 Nov 2023 03:50:19 -0800 (PST) Message-ID: Date: Wed, 22 Nov 2023 11:50:18 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 15/21] dts: os session docstring update Content-Language: en-US To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-16-juraj.linkes@pantheon.tech> From: Yoan Picchi In-Reply-To: <20231115130959.39420-16-juraj.linkes@pantheon.tech> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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š > --- > dts/framework/testbed_model/os_session.py | 275 ++++++++++++++++------ > 1 file changed, 208 insertions(+), 67 deletions(-) > > diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py > index 76e595a518..72b9193a61 100644 > --- a/dts/framework/testbed_model/os_session.py > +++ b/dts/framework/testbed_model/os_session.py > @@ -2,6 +2,29 @@ > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > > +"""OS-aware remote session. > + > +DPDK supports multiple different operating systems, meaning it can run on these different operating > +systems. This module defines the common API that OS-unaware layers use and translates the API into > +OS-aware calls/utility usage. > + > +Note: > + Running commands with administrative privileges requires OS awareness. This is the only layer > + that's aware of OS differences, so this is where non-privileged command get converted > + to privileged commands. > + > +Example: > + A user wishes to remove a directory on > + a remote :class:`~framework.testbed_model.sut_node.SutNode`. > + The :class:`~framework.testbed_model.sut_node.SutNode` object isn't aware what OS the node > + is running - it delegates the OS translation logic > + to :attr:`~framework.testbed_model.node.Node.main_session`. The SUT node calls > + :meth:`~OSSession.remove_remote_dir` with a generic, OS-unaware path and > + the :attr:`~framework.testbed_model.node.Node.main_session` translates that > + to ``rm -rf`` if the node's OS is Linux and other commands for other OSs. > + It also translates the path to match the underlying OS. > +""" > + > from abc import ABC, abstractmethod > from collections.abc import Iterable > from ipaddress import IPv4Interface, IPv6Interface > @@ -28,10 +51,16 @@ > > > class OSSession(ABC): > - """ > - The OS classes create a DTS node remote session and implement OS specific > + """OS-unaware to OS-aware translation API definition. > + > + The OSSession classes create a remote session to a DTS node and implement OS specific > behavior. There a few control methods implemented by the base class, the rest need > - to be implemented by derived classes. > + to be implemented by subclasses. > + > + Attributes: > + name: The name of the session. > + remote_session: The remote session maintaining the connection to the node. > + interactive_session: The interactive remote session maintaining the connection to the node. > """ > > _config: NodeConfiguration > @@ -46,6 +75,15 @@ def __init__( > name: str, > logger: DTSLOG, > ): > + """Initialize the OS-aware session. > + > + Connect to the node right away and also create an interactive remote session. > + > + Args: > + node_config: The test run configuration of the node to connect to. > + name: The name of the session. > + logger: The logger instance this session will use. > + """ > self._config = node_config > self.name = name > self._logger = logger > @@ -53,15 +91,15 @@ def __init__( > self.interactive_session = create_interactive_session(node_config, logger) > > def close(self, force: bool = False) -> None: > - """ > - Close the remote session. > + """Close the underlying remote session. > + > + Args: > + force: Force the closure of the connection. > """ > self.remote_session.close(force) > > def is_alive(self) -> bool: > - """ > - Check whether the remote session is still responding. > - """ > + """Check whether the underlying remote session is still responding.""" > return self.remote_session.is_alive() > > def send_command( > @@ -72,10 +110,23 @@ def send_command( > verify: bool = False, > env: dict | None = None, > ) -> CommandResult: > - """ > - An all-purpose API in case the command to be executed is already > - OS-agnostic, such as when the path to the executed command has been > - constructed beforehand. > + """An all-purpose API for OS-agnostic commands. > + > + This can be used for an execution of a portable command that's executed the same way > + on all operating systems, such as Python. > + > + The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT` > + environment variable configure the timeout of command execution. > + > + Args: > + command: The command to execute. > + timeout: Wait at most this long in seconds to execute the command. confusing start/end of execution > + privileged: Whether to run the command with administrative privileges. > + verify: If :data:`True`, will check the exit code of the command. > + env: A dictionary with environment variables to be used with the command execution. > + > + Raises: > + RemoteCommandExecutionError: If verify is :data:`True` and the command failed. > """ > if privileged: > command = self._get_privileged_command(command) > @@ -89,8 +140,20 @@ def create_interactive_shell( > privileged: bool, > app_args: str, > ) -> InteractiveShellType: > - """ > - See "create_interactive_shell" in SutNode > + """Factory for interactive session handlers. > + > + Instantiate `shell_cls` according to the remote OS specifics. > + > + Args: > + shell_cls: The class of the shell. > + timeout: Timeout for reading output from the SSH channel. If you are > + reading from the buffer and don't receive any data within the timeout > + it will throw an error. > + privileged: Whether to run the shell with administrative privileges. > + app_args: The arguments to be passed to the application. > + > + Returns: > + An instance of the desired interactive application shell. > """ > return shell_cls( > self.interactive_session.session, > @@ -114,27 +177,42 @@ def _get_privileged_command(command: str) -> str: > > @abstractmethod > def guess_dpdk_remote_dir(self, remote_dir: str | PurePath) -> PurePath: > - """ > - Try to find DPDK remote dir in remote_dir. > + """Try to find DPDK directory in `remote_dir`. > + > + The directory is the one which is created after the extraction of the tarball. The files > + are usually extracted into a directory starting with ``dpdk-``. > + > + Returns: > + The absolute path of the DPDK remote directory, empty path if not found. > """ > > @abstractmethod > def get_remote_tmp_dir(self) -> PurePath: > - """ > - Get the path of the temporary directory of the remote OS. > + """Get the path of the temporary directory of the remote OS. > + > + Returns: > + The absolute path of the temporary directory. > """ > > @abstractmethod > def get_dpdk_build_env_vars(self, arch: Architecture) -> dict: > - """ > - Create extra environment variables needed for the target architecture. Get > - information from the node if needed. > + """Create extra environment variables needed for the target architecture. > + > + Different architectures may require different configuration, such as setting 32-bit CFLAGS. > + > + Returns: > + A dictionary with keys as environment variables. > """ > > @abstractmethod > def join_remote_path(self, *args: str | PurePath) -> PurePath: > - """ > - Join path parts using the path separator that fits the remote OS. > + """Join path parts using the path separator that fits the remote OS. > + > + Args: > + args: Any number of paths to join. > + > + Returns: > + The resulting joined path. > """ > > @abstractmethod > @@ -143,13 +221,13 @@ def copy_from( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > - """Copy a file from the remote Node to the local filesystem. > + """Copy a file from the remote node to the local filesystem. > > - Copy source_file from the remote Node associated with this remote > - session to destination_file on the local filesystem. > + Copy `source_file` from the remote node associated with this remote > + session to `destination_file` on the local filesystem. > > Args: > - source_file: the file on the remote Node. > + source_file: the file on the remote node. > destination_file: a file or directory path on the local filesystem. > """ > > @@ -159,14 +237,14 @@ def copy_to( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > - """Copy a file from local filesystem to the remote Node. > + """Copy a file from local filesystem to the remote node. > > - Copy source_file from local filesystem to destination_file > - on the remote Node associated with this remote session. > + Copy `source_file` from local filesystem to `destination_file` > + on the remote node associated with this remote session. > > Args: > source_file: the file on the local filesystem. > - destination_file: a file or directory path on the remote Node. > + destination_file: a file or directory path on the remote node. > """ > > @abstractmethod > @@ -176,8 +254,12 @@ def remove_remote_dir( > recursive: bool = True, > force: bool = True, > ) -> None: > - """ > - Remove remote directory, by default remove recursively and forcefully. > + """Remove remote directory, by default remove recursively and forcefully. > + > + Args: > + remote_dir_path: The path of the directory to remove. > + recursive: If :data:`True`, also remove all contents inside the directory. > + force: If :data:`True`, ignore all warnings and try to remove at all costs. > """ > > @abstractmethod > @@ -186,9 +268,12 @@ def extract_remote_tarball( > remote_tarball_path: str | PurePath, > expected_dir: str | PurePath | None = None, > ) -> None: > - """ > - Extract remote tarball in place. If expected_dir is a non-empty string, check > - whether the dir exists after extracting the archive. > + """Extract remote tarball in its remote directory. > + > + Args: > + remote_tarball_path: The path of the tarball on the remote node. > + expected_dir: If non-empty, check whether `expected_dir` exists after extracting > + the archive. > """ > > @abstractmethod > @@ -201,69 +286,119 @@ def build_dpdk( > rebuild: bool = False, > timeout: float = SETTINGS.compile_timeout, > ) -> None: > - """ > - Build DPDK in the input dir with specified environment variables and meson > - arguments. > + """Build DPDK on the remote node. > + > + An extracted DPDK tarball must be present on the node. The build consists of two steps:: > + > + meson setup remote_dpdk_dir remote_dpdk_build_dir > + ninja -C remote_dpdk_build_dir > + > + The :option:`--compile-timeout` command line argument and the :envvar:`DTS_COMPILE_TIMEOUT` > + environment variable configure the timeout of DPDK build. > + > + Args: > + env_vars: Use these environment variables then building DPDK. > + meson_args: Use these meson arguments when building DPDK. > + remote_dpdk_dir: The directory on the remote node where DPDK will be built. > + remote_dpdk_build_dir: The target build directory on the remote node. > + rebuild: If :data:`True`, do a subsequent build with ``meson configure`` instead > + of ``meson setup``. > + timeout: Wait at most this long in seconds for the build to execute. confusing start/end of execution > """ > > @abstractmethod > def get_dpdk_version(self, version_path: str | PurePath) -> str: > - """ > - Inspect DPDK version on the remote node from version_path. > + """Inspect the DPDK version on the remote node. > + > + Args: > + version_path: The path to the VERSION file containing the DPDK version. > + > + Returns: > + The DPDK version. > """ > > @abstractmethod > def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]: > - """ > - Compose a list of LogicalCores present on the remote node. > - If use_first_core is False, the first physical core won't be used. > + r"""Get the list of :class:`~framework.testbed_model.cpu.LogicalCore`\s on the remote node. > + > + Args: > + use_first_core: If :data:`False`, the first physical core won't be used. > + > + Returns: > + The logical cores present on the node. > """ > > @abstractmethod > def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None: > - """ > - Kill and cleanup all DPDK apps identified by dpdk_prefix_list. If > - dpdk_prefix_list is empty, attempt to find running DPDK apps to kill and clean. > + """Kill and cleanup all DPDK apps. > + > + Args: > + dpdk_prefix_list: Kill all apps identified by `dpdk_prefix_list`. > + If `dpdk_prefix_list` is empty, attempt to find running DPDK apps to kill and clean. > """ > > @abstractmethod > def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str: > - """ > - Get the DPDK file prefix that will be used when running DPDK apps. > + """Make OS-specific modification to the DPDK file prefix. > + > + Args: > + dpdk_prefix: The OS-unaware file prefix. > + > + Returns: > + The OS-specific file prefix. > """ > > @abstractmethod > - def setup_hugepages(self, hugepage_amount: int, force_first_numa: bool) -> None: > - """ > - Get the node's Hugepage Size, configure the specified amount of hugepages > + def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None: > + """Configure hugepages on the node. > + > + Get the node's Hugepage Size, configure the specified count of hugepages > if needed and mount the hugepages if needed. > - If force_first_numa is True, configure hugepages just on the first socket. > + > + Args: > + hugepage_count: Configure this many hugepages. > + force_first_numa: If :data:`True`, configure hugepages just on the first socket. force *numa* configures the first *socket* ? > """ > > @abstractmethod > def get_compiler_version(self, compiler_name: str) -> str: > - """ > - Get installed version of compiler used for DPDK > + """Get installed version of compiler used for DPDK. > + > + Args: > + compiler_name: The name of the compiler executable. > + > + Returns: > + The compiler's version. > """ > > @abstractmethod > def get_node_info(self) -> NodeInfo: > - """ > - Collect information about the node > + """Collect additional information about the node. > + > + Returns: > + Node information. > """ > > @abstractmethod > def update_ports(self, ports: list[Port]) -> None: > - """ > - Get additional information about ports: > - Logical name (e.g. enp7s0) if applicable > - Mac address > + """Get additional information about ports from the operating system and update them. > + > + The additional information is: > + > + * Logical name (e.g. ``enp7s0``) if applicable, > + * Mac address. > + > + Args: > + ports: The ports to update. > """ > > @abstractmethod > def configure_port_state(self, port: Port, enable: bool) -> None: > - """ > - Enable/disable port. > + """Enable/disable `port` in the operating system. > + > + Args: > + port: The port to configure. > + enable: If :data:`True`, enable the port, otherwise shut it down. > """ > > @abstractmethod > @@ -273,12 +408,18 @@ def configure_port_ip_address( > port: Port, > delete: bool, > ) -> None: > - """ > - Configure (add or delete) an IP address of the input port. > + """Configure an IP address on `port` in the operating system. > + > + Args: > + address: The address to configure. > + port: The port to configure. > + delete: If :data:`True`, remove the IP address, otherwise configure it. > """ > > @abstractmethod > def configure_ipv4_forwarding(self, enable: bool) -> None: > - """ > - Enable IPv4 forwarding in the underlying OS. > + """Enable IPv4 forwarding in the operating system. > + > + Args: > + enable: If :data:`True`, enable the forwarding, otherwise disable it. > """