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 823074541F; Thu, 13 Jun 2024 20:16:05 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 542BD427D5; Thu, 13 Jun 2024 20:15:54 +0200 (CEST) Received: from mail-yw1-f230.google.com (mail-yw1-f230.google.com [209.85.128.230]) by mails.dpdk.org (Postfix) with ESMTP id 6CAE7427B9 for ; Thu, 13 Jun 2024 20:15:52 +0200 (CEST) Received: by mail-yw1-f230.google.com with SMTP id 00721157ae682-62f8dcbd4b5so15925867b3.0 for ; Thu, 13 Jun 2024 11:15:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718302551; x=1718907351; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=8lpCoC6hjD9iiAger6N81O2cRJjsWsj/wb+XJqDHNbU=; b=En4rU9Wv/w2jAebbHUBjn57cCh6hIxoj+rPaEX4vRpmWVO45ZlL3iUhkQKYjFAKbsm LCv/K0U/J5VA4V+L4EjtOtIQAOxYZ4Xpb62/hkulQTPEPhQWPVWD/kL3MkEvx5T8O9Rj +YM/npJ7DFqvgV2PEt3M0ken66nSaKIzjVN0A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718302551; x=1718907351; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8lpCoC6hjD9iiAger6N81O2cRJjsWsj/wb+XJqDHNbU=; b=Ts6Qdp5YlYgWJ183bWvJQ4BB3ByDGgytvBNfOzkHEhZwdtCgagQqELPKiLKHI6g+0q YTVxfxTGjM+DOf8K+O0AB9rkaNK1Qdue+fs1zOfy3tc0wiBM5hOpUfsQPDQXUQVTLeCO PjexK0uMedD7vP07Yv9DndPHIsh4lr+VQxyvfEF/bIwoGoBhxeSGHfrML9KglLq+xzOL T379zq1RDSFpOPIuKkDy99nR+YO5ADRkxOKRkVFEdJ4JECjijazpn2FsAJd9k+MXixJK EEai23Olr7aXC8zWMl0wYbmyGMlGmVxfNcLTr+f8hnkoELyjbrPsX7P0q2EzxyQ0+Gih oZQA== X-Gm-Message-State: AOJu0Ywg7Jhgg0mPzzsTsXfOj4Nn8ml6L3eZlYJLPOo4DCvcCoKPe9aN ARxXUDooPRRokTtFg8EgPxEDwwEjmnx815pDKNIURFXLT0a86a6jQdxBL8W5u4c5qWm7bQTE3Bd hykb+PCcQ+8bXtHqZrS9O6ebRqzlhyhslCG64agOiwZYgPbdm X-Google-Smtp-Source: AGHT+IGanawBRyGWHqdJeGsIqXzxkvw3jfmIICnCEIEZ2NhRzIgcKJEhjs1Kjdc6bjumWnDBFMzeI0V0fk0j X-Received: by 2002:a0d:f703:0:b0:631:45f6:7850 with SMTP id 00721157ae682-6322480cf1dmr1682767b3.39.1718302550124; Thu, 13 Jun 2024 11:15:50 -0700 (PDT) Received: from postal.iol.unh.edu (postal.iol.unh.edu. [2606:4100:3880:1234::84]) by smtp-relay.gmail.com with ESMTPS id 00721157ae682-63118da40a0sm595227b3.33.2024.06.13.11.15.49 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 13 Jun 2024 11:15:50 -0700 (PDT) X-Relaying-Domain: iol.unh.edu Received: from iol.unh.edu (unknown [IPv6:2606:4100:3880:1257::1083]) by postal.iol.unh.edu (Postfix) with ESMTP id 38D38605C380; Thu, 13 Jun 2024 14:15:49 -0400 (EDT) From: jspewock@iol.unh.edu To: juraj.linkes@pantheon.tech, probb@iol.unh.edu, yoan.picchi@foss.arm.com, npratte@iol.unh.edu, Honnappa.Nagarahalli@arm.com, wathsala.vithanage@arm.com, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, thomas@monjalon.net Cc: dev@dpdk.org, Jeremy Spewock Subject: [PATCH v4 2/4] dts: improve starting and stopping interactive shells Date: Thu, 13 Jun 2024 14:15:08 -0400 Message-ID: <20240613181510.30135-3-jspewock@iol.unh.edu> X-Mailer: git-send-email 2.45.1 In-Reply-To: <20240613181510.30135-1-jspewock@iol.unh.edu> References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240613181510.30135-1-jspewock@iol.unh.edu> 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 From: Jeremy Spewock The InteractiveShell class currently relies on being cleaned up and shutdown at the time of garbage collection, but this cleanup of the class does no verification that the session is still running prior to cleanup. So, if a user were to call this method themselves prior to garbage collection, it would be called twice and throw an exception when the desired behavior is to do nothing since the session is already cleaned up. This is solved by using a weakref and a finalize class which achieves the same result of calling the method at garbage collection, but also ensures that it is called exactly once. Additionally, this fixes issues regarding starting a primary DPDK application while another is still cleaning up via a retry when starting interactive shells. It also adds catch for attempting to send a command to an interactive shell that is not running to create a more descriptive error message. Signed-off-by: Jeremy Spewock --- .../remote_session/interactive_shell.py | 35 ++++++++++++++----- .../single_active_interactive_shell.py | 34 ++++++++++++++++-- dts/framework/remote_session/testpmd_shell.py | 2 +- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 9d124b8245..5b6f5c2a41 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -8,6 +8,9 @@ collection. """ +import weakref +from typing import Callable, ClassVar + from .single_active_interactive_shell import SingleActiveInteractiveShell @@ -15,18 +18,34 @@ class InteractiveShell(SingleActiveInteractiveShell): """Adds manual start and stop functionality to interactive shells. 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 through - the garbage collector. + 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. """ + _finalizer: weakref.finalize + #: Shells that do not require only one instance to be running shouldn't need more than 1 + #: attempt to start. + _init_attempts: ClassVar[int] = 1 + + def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None: + """Overrides :meth:`_start_application` in the parent class. + + Add a weakref finalize class after starting the application. + + Args: + get_privileged_command: A function (but could be any callable) that produces + the version of the command with elevated privileges. + """ + super()._start_application(get_privileged_command) + self._finalizer = weakref.finalize(self, self._close) + def start_application(self) -> None: """Start the application.""" self._start_application(self._get_privileged_command) def close(self) -> None: - """Properly free all resources.""" - self._close() - - def __del__(self) -> None: - """Make sure the session is properly closed before deleting the object.""" - self.close() + """Free all resources using finalize class.""" + self._finalizer() diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py index 74060be8a7..282ceec483 100644 --- a/dts/framework/remote_session/single_active_interactive_shell.py +++ b/dts/framework/remote_session/single_active_interactive_shell.py @@ -44,6 +44,10 @@ class SingleActiveInteractiveShell(ABC): 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. """ _interactive_session: SSHClient @@ -55,6 +59,9 @@ class SingleActiveInteractiveShell(ABC): _app_args: str _get_privileged_command: Callable[[str], str] | None + #: 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] = "" @@ -71,6 +78,8 @@ class SingleActiveInteractiveShell(ABC): #: for DPDK on the node will be prepended to the path to the executable. dpdk_app: ClassVar[bool] = False + is_alive: bool = False + def __init__( self, interactive_session: SSHClient, @@ -110,17 +119,34 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None This method is often overridden by subclasses as their process for starting may look different. A new SSH channel is initialized for the application to run on, then the - application is started. + application is started. 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. Args: get_privileged_command: A function (but could be any callable) that produces the version of the command with elevated privileges. """ self._init_channel() + self._ssh_channel.settimeout(5) start_command = f"{self.path} {self._app_args}" if get_privileged_command is not None: start_command = get_privileged_command(start_command) - self.send_command(start_command) + self.is_alive = True + for attempt in range(self._init_attempts): + try: + self.send_command(start_command) + break + except TimeoutError: + 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) -> str: """Send `command` and get all output before the expected ending string. @@ -142,6 +168,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str: Returns: All output in the buffer before expected string. """ + 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 diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py index 17561d4dae..805bb3a77d 100644 --- a/dts/framework/remote_session/testpmd_shell.py +++ b/dts/framework/remote_session/testpmd_shell.py @@ -230,7 +230,7 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True): def _close(self) -> None: """Overrides :meth:`~.interactive_shell.close`.""" self.stop() - self.send_command("quit", "") + self.send_command("quit", "Bye...") return super()._close() def get_capas_rxq( -- 2.45.1