From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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?= <juraj.linkes@pantheon.tech>,
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 <yoan.picchi@foss.arm.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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š <juraj.linkes@pantheon.tech>
> ---
> 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 <https://docs.fabfile.org/en/latest/>`_.
>
> 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: