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 0E27A43382; Wed, 22 Nov 2023 14:27:27 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F3CFF40EDF; Wed, 22 Nov 2023 14:27:26 +0100 (CET) Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by mails.dpdk.org (Postfix) with ESMTP id 8C1A740ED9 for ; Wed, 22 Nov 2023 14:27:25 +0100 (CET) Received: by mail-ed1-f49.google.com with SMTP id 4fb4d7f45d1cf-54a94e68fb1so42839a12.0 for ; Wed, 22 Nov 2023 05:27:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700659645; x=1701264445; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=5r0OTyu9VZ4PDen82/RIYSKhKYvUUgCyerf7v9a+pZc=; b=E8tGLrtNUHpb7nE7bvm2twQyIAW5g3KSRrgCySksGlk+yj0/fJSjcSE7d2bt6Kt/Bn mtggpYg4WlKieGlTC1+6PbYn8jqNm6/3YdA4YVh7wiPpd7MszS6XOZmIeANzThRnEtas XnAjHidj27GffCa5WRrV9MxtEJsET012zuUzvpp4D3rgnWrjY1sLAt5BNVyzcjEBitnE ExFCNG3t81b/xhLNoGPWJQnocaErUtOcHkd7vdGHALg8t/a+G1QkhOdIRBSD71fBCHGY 3kC+sURhY/NI/2CZYZViB78Rfe84dd5/8+3T9oWbVoD/H7gfLkHanZRSdyD4e7Sw7Hra BlQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700659645; x=1701264445; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5r0OTyu9VZ4PDen82/RIYSKhKYvUUgCyerf7v9a+pZc=; b=SOTWk+E8fhBEQlbuwuDm8RIebMtis3s9teh3GE63iycAhnwZmxzWpcB6reDxGLvzO7 cJFr6Vn8q2KtsK7xC+xrE0XoiBrBXM/g531Sj1P5awHy/CTo47JM2wvgOJHejWnIwLf7 BFFe4ToxvL5jRgNTcY1imgN1S0g9LfuZdD5wX2mYZRUFVoFuyE3WJKkhjbz0FybSZISM cKSwxUIVI29jJBqKp+ai9MwstwWfuwNslwysWcIBnvN6K/Kp2CodT6woPQbchCTG/a0P HOuvQb37Epm6w6t/wVF8qhPruHy1m/B89w/zAjWp9yGhIMSzXTFDkFsS6OVbY0VfNUSD CdgA== X-Gm-Message-State: AOJu0YzsT8au4w0Z4L7VRFBQnUxr8a+AFc6NmYDCRpepwj1OTvatUqsz FGBl5Qns2Cl7tY7Yc1WZ5zU1Lix+JlsI6TRHvAFlWQ== X-Google-Smtp-Source: AGHT+IF5MebyteyA/OIWbZm8GwmueQcx7KhD4RYccVA1BVh8+0Cr2hwm9rFAqgBoA10v9i9T2/mNvhgpB0INU6WT9KM= X-Received: by 2002:a17:906:7d0:b0:a00:1263:8c4c with SMTP id m16-20020a17090607d000b00a0012638c4cmr2091491ejc.18.1700659645101; Wed, 22 Nov 2023 05:27:25 -0800 (PST) MIME-Version: 1.0 References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-16-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 14:27:14 +0100 Message-ID: Subject: Re: [PATCH v7 15/21] dts: os session docstring update To: Yoan Picchi Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Wed, Nov 22, 2023 at 12:50=E2=80=AFPM Yoan Picchi wrote: > > On 11/15/23 13:09, Juraj Linke=C5=A1 wrote: > > Format according to the Google format and PEP257, with slight > > deviations. > > > > Signed-off-by: Juraj Linke=C5=A1 > > --- > > 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 awaren= ess. This is the only layer > > + that's aware of OS differences, so this is where non-privileged co= mmand 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 SU= T node calls > > + :meth:`~OSSession.remove_remote_dir` with a generic, OS-unaware pa= th and > > + the :attr:`~framework.testbed_model.node.Node.main_session` transl= ates that > > + to ``rm -rf`` if the node's OS is Linux and other commands for oth= er 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 s= pecific > > + """OS-unaware to OS-aware translation API definition. > > + > > + The OSSession classes create a remote session to a DTS node and im= plement OS specific > > behavior. There a few control methods implemented by the base cla= ss, 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 maintainin= g 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 con= nect to. > > + name: The name of the session. > > + logger: The logger instance this session will use. > > + """ > > self._config =3D node_config > > self.name =3D name > > self._logger =3D logger > > @@ -53,15 +91,15 @@ def __init__( > > self.interactive_session =3D create_interactive_session(node_= config, logger) > > > > def close(self, force: bool =3D 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 respon= ding.""" > > return self.remote_session.is_alive() > > > > def send_command( > > @@ -72,10 +110,23 @@ def send_command( > > verify: bool =3D False, > > env: dict | None =3D None, > > ) -> CommandResult: > > - """ > > - An all-purpose API in case the command to be executed is alrea= dy > > - 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 executio= n. > > + > > + Args: > > + command: The command to execute. > > + timeout: Wait at most this long in seconds to execute the = command. > > confusing start/end of execution > Ack. > > + privileged: Whether to run the command with administrative= privileges. > > + verify: If :data:`True`, will check the exit code of the c= ommand. > > + env: A dictionary with environment variables to be used wi= th the command execution. > > + > > + Raises: > > + RemoteCommandExecutionError: If verify is :data:`True` and= the command failed. > > """ > > if privileged: > > command =3D 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 wit= hin the timeout > > + it will throw an error. > > + privileged: Whether to run the shell with administrative p= rivileges. > > + 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) -> Pu= rePath: > > - """ > > - 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 archi= tecture. Get > > - information from the node if needed. > > + """Create extra environment variables needed for the target ar= chitecture. > > + > > + Different architectures may require different configuration, s= uch 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 remo= te 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 rem= ote > > - session to destination_file on the local filesystem. > > + Copy `source_file` from the remote node associated with this r= emote > > + 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 f= ilesystem. > > """ > > > > @@ -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 N= ode. > > + destination_file: a file or directory path on the remote n= ode. > > """ > > > > @abstractmethod > > @@ -176,8 +254,12 @@ def remove_remote_dir( > > recursive: bool =3D True, > > force: bool =3D True, > > ) -> None: > > - """ > > - Remove remote directory, by default remove recursively and for= cefully. > > + """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 insid= e the directory. > > + force: If :data:`True`, ignore all warnings and try to rem= ove at all costs. > > """ > > > > @abstractmethod > > @@ -186,9 +268,12 @@ def extract_remote_tarball( > > remote_tarball_path: str | PurePath, > > expected_dir: str | PurePath | None =3D None, > > ) -> None: > > - """ > > - Extract remote tarball in place. If expected_dir is a non-empt= y 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` e= xists after extracting > > + the archive. > > """ > > > > @abstractmethod > > @@ -201,69 +286,119 @@ def build_dpdk( > > rebuild: bool =3D False, > > timeout: float =3D SETTINGS.compile_timeout, > > ) -> None: > > - """ > > - Build DPDK in the input dir with specified environment variabl= es and meson > > - arguments. > > + """Build DPDK on the remote node. > > + > > + An extracted DPDK tarball must be present on the node. The bui= ld 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 DP= DK. > > + meson_args: Use these meson arguments when building DPDK. > > + remote_dpdk_dir: The directory on the remote node where DP= DK will be built. > > + remote_dpdk_build_dir: The target build directory on the r= emote node. > > + rebuild: If :data:`True`, do a subsequent build with ``mes= on configure`` instead > > + of ``meson setup``. > > + timeout: Wait at most this long in seconds for the build t= o execute. > > confusing start/end of execution > Ack. > > """ > > > > @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[LogicalCo= re]: > > - """ > > - Compose a list of LogicalCores present on the remote node. > > - If use_first_core is False, the first physical core won't be u= sed. > > + r"""Get the list of :class:`~framework.testbed_model.cpu.Logic= alCore`\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 t= o 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 runnin= g 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 a= pps. > > + """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 o= f hugepages > > + def setup_hugepages(self, hugepage_count: int, force_first_numa: b= ool) -> 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 f= irst socket. > > + > > + Args: > > + hugepage_count: Configure this many hugepages. > > + force_first_numa: If :data:`True`, configure hugepages ju= st on the first socket. > > force *numa* configures the first *socket* ? > Good catch, should be numa node, not 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 s= ystem 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 i= t 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. > > """ >