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 0D48843390; Tue, 21 Nov 2023 16:36:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 83CF142EA6; Tue, 21 Nov 2023 16:36:17 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 569AC42E9D for ; Tue, 21 Nov 2023 16:36:15 +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 7C4FAFEC; Tue, 21 Nov 2023 07:37:01 -0800 (PST) Received: from [10.1.34.179] (e125442.arm.com [10.1.34.179]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2BD213F7A6; Tue, 21 Nov 2023 07:36:12 -0800 (PST) Message-ID: <95543c20-c05a-4321-a5a6-06a3eca961e3@foss.arm.com> Date: Tue, 21 Nov 2023 15:36:11 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 11/21] dts: remote 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-12-juraj.linkes@pantheon.tech> From: Yoan Picchi In-Reply-To: <20231115130959.39420-12-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/remote_session/__init__.py | 39 +++++- > .../remote_session/remote_session.py | 128 +++++++++++++----- > dts/framework/remote_session/ssh_session.py | 16 +-- > 3 files changed, 135 insertions(+), 48 deletions(-) > > diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py > index 5e7ddb2b05..51a01d6b5e 100644 > --- a/dts/framework/remote_session/__init__.py > +++ b/dts/framework/remote_session/__init__.py > @@ -2,12 +2,14 @@ > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > > -""" > -The package provides modules for managing remote connections to a remote host (node), > -differentiated by OS. > -The package provides a factory function, create_session, that returns the appropriate > -remote connection based on the passed configuration. The differences are in the > -underlying transport protocol (e.g. SSH) and remote OS (e.g. Linux). > +"""Remote interactive and non-interactive sessions. > + > +This package provides modules for managing remote connections to a remote host (node). > + > +The non-interactive sessions send commands and return their output and exit code. > + > +The interactive sessions open an interactive shell which is continuously open, > +allowing it to send and receive data within that particular shell. > """ > > # pylama:ignore=W0611 > @@ -26,10 +28,35 @@ > def create_remote_session( > node_config: NodeConfiguration, name: str, logger: DTSLOG > ) -> RemoteSession: > + """Factory for non-interactive remote sessions. > + > + The function returns an SSH session, but will be extended if support > + for other protocols is added. > + > + 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. > + > + Returns: > + The SSH remote session. > + """ > return SSHSession(node_config, name, logger) > > > def create_interactive_session( > node_config: NodeConfiguration, logger: DTSLOG > ) -> InteractiveRemoteSession: > + """Factory for interactive remote sessions. > + > + The function returns an interactive SSH session, but will be extended if support > + for other protocols is added. > + > + Args: > + node_config: The test run configuration of the node to connect to. > + logger: The logger instance this session will use. > + > + Returns: > + The interactive SSH remote session. > + """ > return InteractiveRemoteSession(node_config, logger) > diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py > index 0647d93de4..629c2d7b9c 100644 > --- a/dts/framework/remote_session/remote_session.py > +++ b/dts/framework/remote_session/remote_session.py > @@ -3,6 +3,13 @@ > # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. > # Copyright(c) 2022-2023 University of New Hampshire > > +"""Base remote session. > + > +This module contains the abstract base class for remote sessions and defines > +the structure of the result of a command execution. > +""" > + > + > import dataclasses > from abc import ABC, abstractmethod > from pathlib import PurePath > @@ -15,8 +22,14 @@ > > @dataclasses.dataclass(slots=True, frozen=True) > class CommandResult: > - """ > - The result of remote execution of a command. > + """The result of remote execution of a command. > + > + Attributes: > + name: The name of the session that executed the command. > + command: The executed command. > + stdout: The standard output the command produced. > + stderr: The standard error output the command produced. > + return_code: The return code the command exited with. > """ > > name: str > @@ -26,6 +39,7 @@ class CommandResult: > return_code: int > > def __str__(self) -> str: > + """Format the command outputs.""" > return ( > f"stdout: '{self.stdout}'\n" > f"stderr: '{self.stderr}'\n" > @@ -34,13 +48,24 @@ def __str__(self) -> str: > > > class RemoteSession(ABC): > - """ > - The base class for defining which methods must be implemented in order to connect > - to a remote host (node) and maintain a remote session. The derived classes are > - supposed to implement/use some underlying transport protocol (e.g. SSH) to > - implement the methods. On top of that, it provides some basic services common to > - all derived classes, such as keeping history and logging what's being executed > - on the remote node. > + """Non-interactive remote session. > + > + The abstract methods must be implemented in order to connect to a remote host (node) > + and maintain a remote session. > + The subclasses must use (or implement) some underlying transport protocol (e.g. SSH) > + to implement the methods. On top of that, it provides some basic services common to all > + subclasses, such as keeping history and logging what's being executed on the remote node. > + > + Attributes: > + name: The name of the session. > + hostname: The node's hostname. Could be an IP (possibly with port, separated by a colon) > + or a domain name. > + ip: The IP address of the node or a domain name, whichever was used in `hostname`. > + port: The port of the node, if given in `hostname`. > + username: The username used in the connection. > + password: The password used in the connection. Most frequently empty, > + as the use of passwords is discouraged. > + history: The executed commands during this session. > """ > > name: str > @@ -59,6 +84,16 @@ def __init__( > session_name: str, > logger: DTSLOG, > ): > + """Connect to the node during initialization. > + > + Args: > + node_config: The test run configuration of the node to connect to. > + session_name: The name of the session. > + logger: The logger instance this session will use. > + > + Raises: > + SSHConnectionError: If the connection to the node was not successful. > + """ > self._node_config = node_config > > self.name = session_name > @@ -79,8 +114,13 @@ def __init__( > > @abstractmethod > def _connect(self) -> None: > - """ > - Create connection to assigned node. > + """Create a connection to the node. > + > + The implementation must assign the established session to self.session. > + > + The implementation must except all exceptions and convert them to an SSHConnectionError. > + > + The implementation may optionally implement retry attempts. > """ > > def send_command( > @@ -90,11 +130,24 @@ def send_command( > verify: bool = False, > env: dict | None = None, > ) -> CommandResult: > - """ > - Send a command to the connected node using optional env vars > - and return CommandResult. > - If verify is True, check the return code of the executed command > - and raise a RemoteCommandExecutionError if the command failed. > + """Send `command` to the connected node. > + > + 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 `command`. > + verify: If :data:`True`, will check the exit code of `command`. > + env: A dictionary with environment variables to be used with `command` execution. > + > + Raises: > + SSHSessionDeadError: If the session isn't alive when sending `command`. > + SSHTimeoutError: If `command` execution timed out. > + RemoteCommandExecutionError: If verify is :data:`True` and `command` execution failed. > + > + Returns: > + The output of the command along with the return code. > """ > self._logger.info( > f"Sending: '{command}'" + (f" with env vars: '{env}'" if env else "") > @@ -115,29 +168,36 @@ def send_command( > def _send_command( > self, command: str, timeout: float, env: dict | None > ) -> CommandResult: > - """ > - Use the underlying protocol to execute the command using optional env vars > - and return CommandResult. > + """Send a command to the connected node. > + > + The implementation must execute the command remotely with `env` environment variables > + and return the result. > + > + The implementation must except all exceptions and raise an SSHSessionDeadError if > + the session is not alive and an SSHTimeoutError if the command execution times out. 3 way "and". Needs comas or splitting the sentence. > """ > > def close(self, force: bool = False) -> None: > - """ > - Close the remote session and free all used resources. > + """Close the remote session and free all used resources. > + > + Args: > + force: Force the closure of the connection. This may not clean up all resources. > """ > self._logger.logger_exit() > self._close(force) > > @abstractmethod > def _close(self, force: bool = False) -> None: > - """ > - Execute protocol specific steps needed to close the session properly. > + """Protocol specific steps needed to close the session properly. > + > + Args: > + force: Force the closure of the connection. This may not clean up all resources. > + This doesn't have to be implemented in the overloaded method. > """ > > @abstractmethod > def is_alive(self) -> bool: > - """ > - Check whether the remote session is still responding. > - """ > + """Check whether the remote session is still responding.""" > > @abstractmethod > def copy_from( > @@ -147,12 +207,12 @@ def copy_from( > ) -> None: > """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. > - destination_file: a file or directory path on the local filesystem. > + source_file: The file on the remote Node. > + destination_file: A file or directory path on the local filesystem. > """ > > @abstractmethod > @@ -163,10 +223,10 @@ def copy_to( > ) -> None: > """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. > + source_file: The file on the local filesystem. > + destination_file: A file or directory path on the remote Node. > """ > diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py > index cee11d14d6..7186490a9a 100644 > --- a/dts/framework/remote_session/ssh_session.py > +++ b/dts/framework/remote_session/ssh_session.py > @@ -1,6 +1,8 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2023 PANTHEON.tech s.r.o. > > +"""SSH session remote session.""" Is the double "session" intended? > + > import socket > import traceback > from pathlib import PurePath > @@ -26,13 +28,8 @@ > class SSHSession(RemoteSession): > """A persistent SSH connection to a remote Node. > > - The connection is implemented with the Fabric Python library. > - > - Args: > - node_config: The configuration of the Node to connect to. > - session_name: The name of the session. > - logger: The logger used for logging. > - This should be passed from the parent OSSession. > + The connection is implemented with > + `the Fabric Python library `_. > > Attributes: > session: The underlying Fabric SSH connection. > @@ -80,6 +77,7 @@ def _connect(self) -> None: > raise SSHConnectionError(self.hostname, errors) > > def is_alive(self) -> bool: > + """Overrides :meth:`~.remote_session.RemoteSession.is_alive`.""" > return self.session.is_connected > > def _send_command( > @@ -89,7 +87,7 @@ def _send_command( > > Args: > command: The command to execute. > - timeout: Wait at most this many seconds for the execution to complete. > + timeout: Wait at most this long in seconds to execute the command. Is the timeout actually to start running the command and not to wait for it to be completed? > env: Extra environment variables that will be used in command execution. > > Raises: > @@ -118,6 +116,7 @@ def copy_from( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > + """Overrides :meth:`~.remote_session.RemoteSession.copy_from`.""" > self.session.get(str(destination_file), str(source_file)) > > def copy_to( > @@ -125,6 +124,7 @@ def copy_to( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > + """Overrides :meth:`~.remote_session.RemoteSession.copy_to`.""" > self.session.put(str(source_file), str(destination_file)) > > def _close(self, force: bool = False) -> None: