[PATCH v7 11/21] dts: remote session docstring update
Yoan Picchi
yoan.picchi at foss.arm.com
Wed Nov 22 12:25:49 CET 2023
On 11/22/23 11:13, Juraj Linkeš wrote:
> On Tue, Nov 21, 2023 at 4:36 PM Yoan Picchi <yoan.picchi at foss.arm.com> wrote:
>>
>> 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 at 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.
>>
>
> 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.
>
Sounds good.
>
>>> """
>>>
>>> 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?
>>
>
> 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 <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?
>>
>
> 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.
Sounds good. I think I saw this confusing wording 3 times so far, but I
can't quite remember which files it was.
>
>>> 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:
>>
More information about the dev
mailing list