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 80F1D4339B; Wed, 22 Nov 2023 12:13:13 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4DB904028C; Wed, 22 Nov 2023 12:13:13 +0100 (CET) Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by mails.dpdk.org (Postfix) with ESMTP id DED8B40273 for ; Wed, 22 Nov 2023 12:13:11 +0100 (CET) Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-548c548c40aso1427856a12.0 for ; Wed, 22 Nov 2023 03:13:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700651591; x=1701256391; 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=86YVoYXLbd+8fm7C5Z3R6ZraJbUdMiQwJpW9QwJrngA=; b=kaPmMzpJGw8gyGPMhW30bLOEI2I+KG6TiQ9uKkGSCj8sQGdHh+ARJb6vTcBXNuGVVG kLwPFvNOl+MqjFsjaVl/KnAyO7eAY5IUlid0U1UrXQzGqq7437gUyLb1XnMrSCiKVfOS UGPjzLoP3sWZyJP8fbK7p/oyAEX0N7qDnOMIkptFBmpfCf3ce09vluiA1TQz/EwfUz5Q GeXsqFFSVmz8NX7akV8hxNikLeC4PDagpqf+exIByQhpBG8uRXKoSoxmdLFLdEN/35yj LGCJgJCO6ERJeVw65NJm164cvMeYUitdE1ePSNvOyzTQri7nqdpLOA30LOrfQacsa3pI +F9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700651591; x=1701256391; 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=86YVoYXLbd+8fm7C5Z3R6ZraJbUdMiQwJpW9QwJrngA=; b=t67ggyqmy0dDvZ/zEkwmkC5cXBC9H1WW0CBcZcqTTqwEMGqEk20MDi+ycobnaG+RNn T19DC8ATGejqDoNjovqhtzdIQWt6R2wJ8qSIOeXFLnHnBPMFUQ4wUUNCsFdKP+xqXChE FrOe5cVw7mYnkXNivbYABc5yZZUNK9RcxRC2GZLjzxB9wIVfeY4UqI9faJCYyP8TyV5v 7YAFW4KAWaLkIP7td5M748sBFOuHbn/pYWU0mDaVeG9Z7HZbv5MnubaIJ8qVIkaMjAoe HRDowWCCnAnaTISecJpx3CJ+NA8FjpvKLQfC8Bu5/fzgbWEsez8vPZBgYKqKUFNLDA4D 6bAw== X-Gm-Message-State: AOJu0YxSw8FoWVJuPWrsTaVllX6wylWK1aZnXg3by5qLLO8W+NW+UMl6 ruwdWwX4BUXb036P33wLkMv34VnBT11qHkfNOPV4vw== X-Google-Smtp-Source: AGHT+IEGaOoz1VjhcpyGJ+d4RnTe7O1Jnx7lzshPHZIp01hvp6mySJUd7KLDu1rixwOHwvKeQhQqY0MrUbkYJMR0GaE= X-Received: by 2002:a17:906:2711:b0:9c7:59ff:b7fd with SMTP id z17-20020a170906271100b009c759ffb7fdmr1529154ejc.28.1700651591480; Wed, 22 Nov 2023 03:13:11 -0800 (PST) MIME-Version: 1.0 References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-12-juraj.linkes@pantheon.tech> <95543c20-c05a-4321-a5a6-06a3eca961e3@foss.arm.com> In-Reply-To: <95543c20-c05a-4321-a5a6-06a3eca961e3@foss.arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 12:13:00 +0100 Message-ID: Subject: Re: [PATCH v7 11/21] dts: remote 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 Tue, Nov 21, 2023 at 4:36=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/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/r= emote_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 remo= te 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 a= re 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 rem= ote host (node). > > + > > +The non-interactive sessions send commands and return their output and= exit code. > > + > > +The interactive sessions open an interactive shell which is continuous= ly open, > > +allowing it to send and receive data within that particular shell. > > """ > > > > # pylama:ignore=3DW0611 > > @@ -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 suppo= rt > > + 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 exten= ded 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/frame= work/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 d= efines > > +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=3DTrue, frozen=3DTrue) > > 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 o= rder 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 serv= ices common to > > - all derived classes, such as keeping history and logging what's be= ing 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 p= rotocol (e.g. SSH) > > + to implement the methods. On top of that, it provides some basic s= ervices common to all > > + subclasses, such as keeping history and logging what's being execu= ted on the remote node. > > + > > + Attributes: > > + name: The name of the session. > > + hostname: The node's hostname. Could be an IP (possibly with p= ort, 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 con= nect 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 =3D node_config > > > > self.name =3D 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 =3D False, > > env: dict | None =3D 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 comma= nd > > - 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 executio= n. > > + > > + Args: > > + command: The command to execute. > > + timeout: Wait at most this long in seconds to execute `com= mand`. > > + verify: If :data:`True`, will check the exit code of `comm= and`. > > + env: A dictionary with environment variables to be used wi= th `command` execution. > > + > > + Raises: > > + SSHSessionDeadError: If the session isn't alive when sendi= ng `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 optio= nal 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 SSH= SessionDeadError if > > + the session is not alive and an SSHTimeoutError if the command= execution times out. > > 3 way "and". Needs comas or splitting the sentence. > What about this? The implementation must except all exceptions and raise: * SSHSessionDeadError if the session is not alive, * SSHTimeoutError if the command execution times out. > > """ > > > > def close(self, force: bool =3D 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 c= lean up all resources. > > """ > > self._logger.logger_exit() > > self._close(force) > > > > @abstractmethod > > def _close(self, force: bool =3D False) -> None: > > - """ > > - Execute protocol specific steps needed to close the session pr= operly. > > + """Protocol specific steps needed to close the session properl= y. > > + > > + Args: > > + force: Force the closure of the connection. This may not c= lean 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 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. > > - destination_file: a file or directory path on the local fi= lesystem. > > + source_file: The file on the remote Node. > > + destination_file: A file or directory path on the local fi= lesystem. > > """ > > > > @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 N= ode. > > + source_file: The file on the local filesystem. > > + destination_file: A file or directory path on the remote N= ode. > > """ > > diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framewor= k/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? > Not really, I'll remove the first occurence. > > + > > 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? > It is to wait for it to be completed. The wording is a bit confusing, what about: Wait at most this long in seconds for the command execution to complete. I'll change this in all places where timeout is documented. > > env: Extra environment variables that will be used in com= mand 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 =3D False) -> None: >