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 DD9C345EFC; Fri, 20 Dec 2024 18:24:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C748040156; Fri, 20 Dec 2024 18:24:55 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id D68144029F for ; Fri, 20 Dec 2024 18:24:53 +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 5A6F61480; Fri, 20 Dec 2024 09:25:21 -0800 (PST) Received: from localhost.localdomain (unknown [10.57.61.169]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 3BD273F720; Fri, 20 Dec 2024 09:24:51 -0800 (PST) From: Luca Vizzarro To: dev@dpdk.org Cc: Paul Szczepanek , Patrick Robb , Luca Vizzarro Subject: [RFC PATCH 2/2] dts: revert back shell split Date: Fri, 20 Dec 2024 17:24:44 +0000 Message-ID: <20241220172444.2194904-1-luca.vizzarro@arm.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20241220172337.2194523-1-luca.vizzarro@arm.com> References: <20241220172337.2194523-1-luca.vizzarro@arm.com> MIME-Version: 1.0 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 The InteractiveShell was previously renamed to SingleActiveInteractiveShell to represent a shell that can only be run once. The mechanism used to enforce this was a context manager, which turned out to be more constrictive on test suite development. Shell closure is now handled by the scoping mechanism, and an attribute is used to enforce the single active shell. Also the split has been reverted. Signed-off-by: Luca Vizzarro --- dts/framework/remote_session/dpdk_shell.py | 8 +- .../remote_session/interactive_shell.py | 262 +++++++++++++++-- .../single_active_interactive_shell.py | 268 ------------------ dts/framework/remote_session/testpmd_shell.py | 4 +- dts/framework/testbed_model/capability.py | 2 +- dts/framework/testbed_model/node.py | 10 +- 6 files changed, 250 insertions(+), 304 deletions(-) delete mode 100644 dts/framework/remote_session/single_active_interactive_shell.py diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py index c11d9ab81c..c37dcb2b62 100644 --- a/dts/framework/remote_session/dpdk_shell.py +++ b/dts/framework/remote_session/dpdk_shell.py @@ -8,10 +8,11 @@ from abc import ABC from pathlib import PurePath +from typing import ClassVar from framework.params.eal import EalParams -from framework.remote_session.single_active_interactive_shell import ( - SingleActiveInteractiveShell, +from framework.remote_session.interactive_shell import ( + InteractiveShell, ) from framework.settings import SETTINGS from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList @@ -61,7 +62,7 @@ def compute_eal_params( return params -class DPDKShell(SingleActiveInteractiveShell, ABC): +class DPDKShell(InteractiveShell, ABC): """The base class for managing DPDK-based interactive shells. This class shouldn't be instantiated directly, but instead be extended. @@ -71,6 +72,7 @@ class DPDKShell(SingleActiveInteractiveShell, ABC): _node: SutNode _app_params: EalParams + _single_active_per_node: ClassVar[bool] = True def __init__( self, diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 9ca285b604..a136419181 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -1,44 +1,256 @@ # SPDX-License-Identifier: BSD-3-Clause -# Copyright(c) 2023 University of New Hampshire +# Copyright(c) 2024 University of New Hampshire # Copyright(c) 2024 Arm Limited -"""Interactive shell with manual stop/start functionality. +"""Common functionality for interactive shell handling. -Provides a class that doesn't require being started/stopped using a context manager and can instead -be started and stopped manually, or have the stopping process be handled at the time of garbage -collection. +The base class, :class:`InteractiveShell`, is meant to be extended by subclasses that +contain functionality specific to that shell type. These subclasses will often modify things like +the prompt to expect or the arguments to pass into the application, but still utilize +the same method for sending a command and collecting output. How this output is handled however +is often application specific. If an application needs elevated privileges to start it is expected +that the method for gaining those privileges is provided when initializing the class. + +The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT` +environment variable configure the timeout of getting the output from command execution. """ -import weakref +from abc import ABC +from pathlib import PurePath from typing import ClassVar -from .single_active_interactive_shell import SingleActiveInteractiveShell +from paramiko import Channel, channel + +from framework.exception import ( + InteractiveCommandExecutionError, + InteractiveSSHSessionDeadError, + InteractiveSSHTimeoutError, + InternalError, +) +from framework.logger import DTSLogger, get_dts_logger +from framework.params import Params +from framework.settings import SETTINGS +from framework.testbed_model.node import Node +from framework.utils import MultiInheritanceBaseClass + + +class InteractiveShell(MultiInheritanceBaseClass, ABC): + """The base class for managing interactive shells. + This class shouldn't be instantiated directly, but instead be extended. It contains + methods for starting interactive shells as well as sending commands to these shells + and collecting input until reaching a certain prompt. All interactive applications + will use the same SSH connection, but each will create their own channel on that + session. -class InteractiveShell(SingleActiveInteractiveShell): - """Adds manual start and stop functionality to interactive shells. + Interactive shells are started and stopped using a context manager. This allows for the start + and cleanup of the application to happen at predictable times regardless of exceptions or + interrupts. - Like its super-class, this class should not be instantiated directly and should instead be - extended. This class also provides an option for automated cleanup of the application using a - weakref and a finalize class. This finalize class allows for cleanup of the class at the time - of garbage collection and also ensures that cleanup only happens once. This way if a user - initiates the closing of the shell manually it is not repeated at the time of garbage - collection. + Attributes: + is_alive: :data:`True` if the application has started successfully, :data:`False` + otherwise. """ - _finalizer: weakref.finalize - #: One attempt should be enough for shells which don't have to worry about other instances - #: closing before starting a new one. - _init_attempts: ClassVar[int] = 1 + _node: Node + _stdin: channel.ChannelStdinFile + _stdout: channel.ChannelFile + _ssh_channel: Channel + _logger: DTSLogger + _timeout: float + _app_params: Params + _privileged: bool + _real_path: PurePath + + #: The number of times to try starting the application before considering it a failure. + _init_attempts: ClassVar[int] = 5 + + #: Prompt to expect at the end of output when sending a command. + #: This is often overridden by subclasses. + _default_prompt: ClassVar[str] = "" + + #: Extra characters to add to the end of every command + #: before sending them. This is often overridden by subclasses and is + #: most commonly an additional newline character. This additional newline + #: character is used to force the line that is currently awaiting input + #: into the stdout buffer so that it can be consumed and checked against + #: the expected prompt. + _command_extra_chars: ClassVar[str] = "" + + #: Condition which constraints the user of the class from attempting to run more than one + #: shell on the same node at the same time. + _single_active_per_node: ClassVar[bool] = False + + #: Path to the executable to start the interactive application. + path: ClassVar[PurePath] + + is_alive: bool = False + + def __init__( + self, + node: Node, + privileged: bool = False, + timeout: float = SETTINGS.timeout, + app_params: Params = Params(), + name: str | None = None, + **kwargs, + ) -> None: + """Create an SSH channel during initialization. + + Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other + constructors in the case of multiple inheritance. + + Args: + node: The node on which to run start the interactive shell. + privileged: Enables the shell to run as superuser. + timeout: The timeout used for the SSH channel that is dedicated to this interactive + shell. This timeout is for collecting output, so if reading from the buffer + and no output is gathered within the timeout, an exception is thrown. + app_params: The command line parameters to be passed to the application on startup. + name: Name for the interactive shell to use for logging. This name will be appended to + the name of the underlying node which it is running on. + **kwargs: Any additional arguments if any. + + Raises: + InternalError: If :attr:`_single_active_per_node` is :data:`True` and another shell of + the same class is already running. + """ + if self._single_active_per_node and node.find_active_shell(type(self)) is not None: + raise InternalError( + "Attempted to run a single-active shell while another one was already open." + ) + + node.register_shell(self) + self._node = node + + if name is None: + name = type(self).__name__ + self._logger = get_dts_logger(f"{node.name}.{name}") + self._app_params = app_params + self._privileged = privileged + self._timeout = timeout + # Ensure path is properly formatted for the host + self._update_real_path(self.path) + super().__init__() + + self.start_application() + + def _setup_ssh_channel(self): + self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell() + self._stdin = self._ssh_channel.makefile_stdin("w") + self._stdout = self._ssh_channel.makefile("r") + self._ssh_channel.settimeout(self._timeout) + self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams + + def _make_start_command(self) -> str: + """Makes the command that starts the interactive shell.""" + start_command = f"{self._real_path} {self._app_params or ''}" + if self._privileged: + start_command = self._node.main_session._get_privileged_command(start_command) + return start_command def start_application(self) -> None: - """Start the application. + """Starts a new interactive application based on the path to the app. + + This method is often overridden by subclasses as their process for starting may look + different. Initialization of the shell on the host can be retried up to + `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly + more time after exiting their script to clean up EAL before others can start. + + Raises: + InteractiveCommandExecutionError: If the application fails to start within the allotted + number of retries. + """ + self._setup_ssh_channel() + self._ssh_channel.settimeout(5) + start_command = self._make_start_command() + self.is_alive = True + for attempt in range(self._init_attempts): + try: + self.send_command(start_command) + break + except InteractiveSSHTimeoutError: + self._logger.info( + f"Interactive shell failed to start (attempt {attempt+1} out of " + f"{self._init_attempts})" + ) + else: + self._ssh_channel.settimeout(self._timeout) + self.is_alive = False # update state on failure to start + raise InteractiveCommandExecutionError("Failed to start application.") + self._ssh_channel.settimeout(self._timeout) + + def send_command( + self, command: str, prompt: str | None = None, skip_first_line: bool = False + ) -> str: + """Send `command` and get all output before the expected ending string. + + Lines that expect input are not included in the stdout buffer, so they cannot + be used for expect. + + Example: + If you were prompted to log into something with a username and password, + you cannot expect ``username:`` because it won't yet be in the stdout buffer. + A workaround for this could be consuming an extra newline character to force + the current `prompt` into the stdout buffer. - After the application has started, use :class:`weakref.finalize` to manage cleanup. + Args: + command: The command to send. + prompt: After sending the command, `send_command` will be expecting this string. + If :data:`None`, will use the class's default prompt. + skip_first_line: Skip the first line when capturing the output. + + Returns: + All output in the buffer before expected string. + + Raises: + InteractiveCommandExecutionError: If attempting to send a command to a shell that is + not currently running. + InteractiveSSHSessionDeadError: The session died while executing the command. + InteractiveSSHTimeoutError: If command was sent but prompt could not be found in + the output before the timeout. """ - self._start_application() - self._finalizer = weakref.finalize(self, self._close) + if not self.is_alive: + raise InteractiveCommandExecutionError( + f"Cannot send command {command} to application because the shell is not running." + ) + self._logger.info(f"Sending: '{command}'") + if prompt is None: + prompt = self._default_prompt + out: str = "" + try: + self._stdin.write(f"{command}{self._command_extra_chars}\n") + self._stdin.flush() + for line in self._stdout: + if skip_first_line: + skip_first_line = False + continue + if line.rstrip().endswith(prompt): + break + out += line + except TimeoutError as e: + self._logger.exception(e) + self._logger.debug( + f"Prompt ({prompt}) was not found in output from command before timeout." + ) + raise InteractiveSSHTimeoutError(command) from e + except OSError as e: + self._logger.exception(e) + raise InteractiveSSHSessionDeadError( + self._node.main_session.interactive_session.hostname + ) from e + finally: + self._logger.debug(f"Got output: {out}") + return out def close(self) -> None: - """Free all resources using :class:`weakref.finalize`.""" - self._finalizer() + """Close the shell.""" + if not self._stdin.closed: + self._stdin.close() + if not self._ssh_channel.closed: + self._ssh_channel.close() + self.is_alive = False + + def _update_real_path(self, path: PurePath) -> None: + """Updates the interactive shell's real path used at command line.""" + self._real_path = self._node.main_session.join_remote_path(path) diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py deleted file mode 100644 index 910af8f655..0000000000 --- a/dts/framework/remote_session/single_active_interactive_shell.py +++ /dev/null @@ -1,268 +0,0 @@ -# SPDX-License-Identifier: BSD-3-Clause -# Copyright(c) 2024 University of New Hampshire - -"""Common functionality for interactive shell handling. - -The base class, :class:`SingleActiveInteractiveShell`, is meant to be extended by subclasses that -contain functionality specific to that shell type. These subclasses will often modify things like -the prompt to expect or the arguments to pass into the application, but still utilize -the same method for sending a command and collecting output. How this output is handled however -is often application specific. If an application needs elevated privileges to start it is expected -that the method for gaining those privileges is provided when initializing the class. - -This class is designed for applications like primary applications in DPDK where only one instance -of the application can be running at a given time and, for this reason, is managed using a context -manager. This context manager starts the application when you enter the context and cleans up the -application when you exit. Using a context manager for this is useful since it allows us to ensure -the application is cleaned up as soon as you leave the block regardless of the reason. - -The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT` -environment variable configure the timeout of getting the output from command execution. -""" - -from abc import ABC -from pathlib import PurePath -from typing import ClassVar - -from paramiko import Channel, channel -from typing_extensions import Self - -from framework.exception import ( - InteractiveCommandExecutionError, - InteractiveSSHSessionDeadError, - InteractiveSSHTimeoutError, -) -from framework.logger import DTSLogger, get_dts_logger -from framework.params import Params -from framework.settings import SETTINGS -from framework.testbed_model.node import Node -from framework.utils import MultiInheritanceBaseClass - - -class SingleActiveInteractiveShell(MultiInheritanceBaseClass, ABC): - """The base class for managing interactive shells. - - This class shouldn't be instantiated directly, but instead be extended. It contains - methods for starting interactive shells as well as sending commands to these shells - and collecting input until reaching a certain prompt. All interactive applications - will use the same SSH connection, but each will create their own channel on that - session. - - Interactive shells are started and stopped using a context manager. This allows for the start - and cleanup of the application to happen at predictable times regardless of exceptions or - interrupts. - - Attributes: - is_alive: :data:`True` if the application has started successfully, :data:`False` - otherwise. - """ - - _node: Node - _stdin: channel.ChannelStdinFile - _stdout: channel.ChannelFile - _ssh_channel: Channel - _logger: DTSLogger - _timeout: float - _app_params: Params - _privileged: bool - _real_path: PurePath - - #: The number of times to try starting the application before considering it a failure. - _init_attempts: ClassVar[int] = 5 - - #: Prompt to expect at the end of output when sending a command. - #: This is often overridden by subclasses. - _default_prompt: ClassVar[str] = "" - - #: Extra characters to add to the end of every command - #: before sending them. This is often overridden by subclasses and is - #: most commonly an additional newline character. This additional newline - #: character is used to force the line that is currently awaiting input - #: into the stdout buffer so that it can be consumed and checked against - #: the expected prompt. - _command_extra_chars: ClassVar[str] = "" - - #: Path to the executable to start the interactive application. - path: ClassVar[PurePath] - - is_alive: bool = False - - def __init__( - self, - node: Node, - privileged: bool = False, - timeout: float = SETTINGS.timeout, - app_params: Params = Params(), - name: str | None = None, - **kwargs, - ) -> None: - """Create an SSH channel during initialization. - - Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other - constructors in the case of multiple inheritance. - - Args: - node: The node on which to run start the interactive shell. - privileged: Enables the shell to run as superuser. - timeout: The timeout used for the SSH channel that is dedicated to this interactive - shell. This timeout is for collecting output, so if reading from the buffer - and no output is gathered within the timeout, an exception is thrown. - app_params: The command line parameters to be passed to the application on startup. - name: Name for the interactive shell to use for logging. This name will be appended to - the name of the underlying node which it is running on. - **kwargs: Any additional arguments if any. - """ - node.register_shell(self) - self._node = node - - if name is None: - name = type(self).__name__ - self._logger = get_dts_logger(f"{node.name}.{name}") - self._app_params = app_params - self._privileged = privileged - self._timeout = timeout - # Ensure path is properly formatted for the host - self._update_real_path(self.path) - super().__init__() - - def _setup_ssh_channel(self): - self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell() - self._stdin = self._ssh_channel.makefile_stdin("w") - self._stdout = self._ssh_channel.makefile("r") - self._ssh_channel.settimeout(self._timeout) - self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams - - def _make_start_command(self) -> str: - """Makes the command that starts the interactive shell.""" - start_command = f"{self._real_path} {self._app_params or ''}" - if self._privileged: - start_command = self._node.main_session._get_privileged_command(start_command) - return start_command - - def _start_application(self) -> None: - """Starts a new interactive application based on the path to the app. - - This method is often overridden by subclasses as their process for starting may look - different. Initialization of the shell on the host can be retried up to - `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly - more time after exiting their script to clean up EAL before others can start. - - Raises: - InteractiveCommandExecutionError: If the application fails to start within the allotted - number of retries. - """ - self._setup_ssh_channel() - self._ssh_channel.settimeout(5) - start_command = self._make_start_command() - self.is_alive = True - for attempt in range(self._init_attempts): - try: - self.send_command(start_command) - break - except InteractiveSSHTimeoutError: - self._logger.info( - f"Interactive shell failed to start (attempt {attempt+1} out of " - f"{self._init_attempts})" - ) - else: - self._ssh_channel.settimeout(self._timeout) - self.is_alive = False # update state on failure to start - raise InteractiveCommandExecutionError("Failed to start application.") - self._ssh_channel.settimeout(self._timeout) - - def send_command( - self, command: str, prompt: str | None = None, skip_first_line: bool = False - ) -> str: - """Send `command` and get all output before the expected ending string. - - Lines that expect input are not included in the stdout buffer, so they cannot - be used for expect. - - Example: - If you were prompted to log into something with a username and password, - you cannot expect ``username:`` because it won't yet be in the stdout buffer. - A workaround for this could be consuming an extra newline character to force - the current `prompt` into the stdout buffer. - - Args: - command: The command to send. - prompt: After sending the command, `send_command` will be expecting this string. - If :data:`None`, will use the class's default prompt. - skip_first_line: Skip the first line when capturing the output. - - Returns: - All output in the buffer before expected string. - - Raises: - InteractiveCommandExecutionError: If attempting to send a command to a shell that is - not currently running. - InteractiveSSHSessionDeadError: The session died while executing the command. - InteractiveSSHTimeoutError: If command was sent but prompt could not be found in - the output before the timeout. - """ - if not self.is_alive: - raise InteractiveCommandExecutionError( - f"Cannot send command {command} to application because the shell is not running." - ) - self._logger.info(f"Sending: '{command}'") - if prompt is None: - prompt = self._default_prompt - out: str = "" - try: - self._stdin.write(f"{command}{self._command_extra_chars}\n") - self._stdin.flush() - for line in self._stdout: - if skip_first_line: - skip_first_line = False - continue - if line.rstrip().endswith(prompt): - break - out += line - except TimeoutError as e: - self._logger.exception(e) - self._logger.debug( - f"Prompt ({prompt}) was not found in output from command before timeout." - ) - raise InteractiveSSHTimeoutError(command) from e - except OSError as e: - self._logger.exception(e) - raise InteractiveSSHSessionDeadError( - self._node.main_session.interactive_session.hostname - ) from e - finally: - self._logger.debug(f"Got output: {out}") - return out - - def _close(self) -> None: - self._stdin.close() - self._ssh_channel.close() - - def _update_real_path(self, path: PurePath) -> None: - """Updates the interactive shell's real path used at command line.""" - self._real_path = self._node.main_session.join_remote_path(path) - - def __enter__(self) -> Self: - """Enter the context block. - - Upon entering a context block with this class, the desired behavior is to create the - channel for the application to use, and then start the application. - - Returns: - Reference to the object for the application after it has been started. - """ - self._start_application() - return self - - def __exit__(self, *_) -> None: - """Exit the context block. - - Upon exiting a context block with this class, we want to ensure that the instance of the - application is explicitly closed and properly cleaned up using its close method. Note that - because this method returns :data:`None` if an exception was raised within the block, it is - not handled and will be re-raised after the application is closed. - - The desired behavior is to close the application regardless of the reason for exiting the - context and then recreate that reason afterwards. All method arguments are ignored for - this reason. - """ - self._close() diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py index d187eaea94..cb8cd6f0ca 100644 --- a/dts/framework/remote_session/testpmd_shell.py +++ b/dts/framework/remote_session/testpmd_shell.py @@ -2077,11 +2077,11 @@ def set_verbose(self, level: int, verify: bool = True) -> None: f"Testpmd failed to set verbose level to {level}." ) - def _close(self) -> None: + def close(self) -> None: """Overrides :meth:`~.interactive_shell.close`.""" self.stop() self.send_command("quit", "Bye...") - return super()._close() + return super().close() """ ====== Capability retrieval methods ====== diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py index e883f59d11..ad497e8ad9 100644 --- a/dts/framework/testbed_model/capability.py +++ b/dts/framework/testbed_model/capability.py @@ -237,7 +237,7 @@ def get_supported_capabilities( for capability in capabilities: if capability.nic_capability in supported_capabilities: supported_conditional_capabilities.add(capability) - testpmd_shell._close() + testpmd_shell.close() logger.debug(f"Found supported capabilities {supported_conditional_capabilities}.") return supported_conditional_capabilities diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py index 4f06968adc..62d0c09e78 100644 --- a/dts/framework/testbed_model/node.py +++ b/dts/framework/testbed_model/node.py @@ -37,13 +37,13 @@ from .port import Port if TYPE_CHECKING: - from framework.remote_session.single_active_interactive_shell import ( - SingleActiveInteractiveShell, + from framework.remote_session.interactive_shell import ( + InteractiveShell, ) T = TypeVar("T") Scope = Literal["unknown", "suite", "case"] -ScopedShell = tuple[Scope, SingleActiveInteractiveShell] +ScopedShell = tuple[Scope, InteractiveShell] class Node(ABC): @@ -164,7 +164,7 @@ def exit_scope(self) -> Scope: self.clean_up_shells(current_scope) return current_scope - def register_shell(self, shell: SingleActiveInteractiveShell) -> None: + def register_shell(self, shell: InteractiveShell) -> None: """Register a new shell to the pool of active shells.""" self._active_shells.append((self.current_scope, shell)) @@ -179,7 +179,7 @@ def clean_up_shells(self, scope: Scope) -> None: ] for i in reversed(zombie_shells_indices): - self._active_shells[i][1]._close() + self._active_shells[i][1].close() del self._active_shells[i] def create_session(self, name: str) -> OSSession: -- 2.43.0