[PATCH v1 0/2] dts: add context manager for interactive shells

[PATCH v1 0/2] dts: add context manager for interactive shells

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

This series is extracted from an existing series that expands the
scatter test suite. The goal of this patch series is to take this
feature that would be useful to other test suites and extract it from
the scatter series that does not directly depend on it.

This series adds a context manager for managing interactive shells and
also adds improved methods of starting and stopping interactive shells
through features like a retry when starting the shell and a weakref
finalize class to handle garbage collection.

Jeremy Spewock (2):
  dts: add context manager for interactive shells
  dts: improve starting and stopping interactive shells

 dts/framework/remote_session/dpdk_shell.py    |   9 +-
 .../remote_session/interactive_shell.py       | 171 ++-----------
 .../single_active_interactive_shell.py        | 233 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py |   9 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  26 +-
 dts/tests/TestSuite_smoke_tests.py            |   3 +-
 7 files changed, 285 insertions(+), 168 deletions(-)
 create mode 100644 dts/framework/remote_session/single_active_interactive_shell.py

-- 
2.45.2

[PATCH v1 1/2] dts: add context manager for interactive shells

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Interactive shells are managed in a way currently where they are closed
and cleaned up at the time of garbage collection. Due to there being no
guarantee of when this garbage collection happens in Python, there is no
way to consistently know when an application will be closed without
manually closing the application yourself when you are done with it.
This doesn't cause a problem in cases where you can start another
instance of the same application multiple times on a server, but this
isn't the case for primary applications in DPDK. The introduction of
primary applications, such as testpmd, adds a need for knowing previous
instances of the application have been stopped and cleaned up before
starting a new one, which the garbage collector does not provide.

To solve this problem, a new class is added which acts as a base class
for interactive shells that enforces that instances of the
application be managed using a context manager. Using a context manager
guarantees that once you leave the scope of the block where the
application is being used for any reason, the application will be closed
immediately. This avoids the possibility of the shell not being closed
due to an exception being raised or user error. The interactive shell
class then becomes shells that can be started/stopped manually or at the
time of garbage collection rather than through a context manager.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/remote_session/dpdk_shell.py    |   9 +-
 .../remote_session/interactive_shell.py       | 160 ++-------------
 .../single_active_interactive_shell.py        | 193 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py |   7 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  26 ++-
 dts/tests/TestSuite_smoke_tests.py            |   3 +-
 7 files changed, 233 insertions(+), 167 deletions(-)
 create 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 296639f37d..950c6ca670 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -11,7 +11,9 @@
 from pathlib import PurePath
 
 from framework.params.eal import EalParams
-from framework.remote_session.interactive_shell import InteractiveShell
+from framework.remote_session.single_active_interactive_shell import (
+    SingleActiveInteractiveShell,
+)
 from framework.settings import SETTINGS
 from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
 from framework.testbed_model.sut_node import SutNode
@@ -60,7 +62,7 @@ def compute_eal_params(
     return params
 
 
-class DPDKShell(InteractiveShell, ABC):
+class DPDKShell(SingleActiveInteractiveShell, ABC):
     """The base class for managing DPDK-based interactive shells.
 
     This class shouldn't be instantiated directly, but instead be extended.
@@ -79,7 +81,6 @@ def __init__(
         lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
         ascending_cores: bool = True,
         append_prefix_timestamp: bool = True,
-        start_on_init: bool = True,
         app_params: EalParams = EalParams(),
     ) -> None:
         """Extends :meth:`~.interactive_shell.InteractiveShell.__init__`.
@@ -95,7 +96,7 @@ def __init__(
             append_prefix_timestamp,
         )
 
-        super().__init__(node, privileged, timeout, start_on_init, app_params)
+        super().__init__(node, privileged, timeout, app_params)
 
     def _update_real_path(self, path: PurePath) -> None:
         """Extends :meth:`~.interactive_shell.InteractiveShell._update_real_path`.
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 254aa29f89..11dc8a0643 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -2,166 +2,32 @@
 # Copyright(c) 2023 University of New Hampshire
 # Copyright(c) 2024 Arm Limited
 
-"""Common functionality for interactive shell handling.
+"""Interactive shell with manual stop/start functionality.
 
-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.
+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.
 """
 
-from abc import ABC
-from pathlib import PurePath
-from typing import ClassVar
-
-from paramiko import Channel, channel  # type: ignore[import-untyped]
-
-from framework.logger import DTSLogger
-from framework.params import Params
-from framework.settings import SETTINGS
-from framework.testbed_model.node import Node
+from .single_active_interactive_shell import SingleActiveInteractiveShell
 
 
-class InteractiveShell(ABC):
-    """The base class for managing interactive shells.
+class InteractiveShell(SingleActiveInteractiveShell):
+    """Adds manual start and stop functionality to 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.
+    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.
     """
 
-    _node: Node
-    _stdin: channel.ChannelStdinFile
-    _stdout: channel.ChannelFile
-    _ssh_channel: Channel
-    _logger: DTSLogger
-    _timeout: float
-    _app_params: Params
-    _privileged: bool
-    _real_path: PurePath
-
-    #: 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.
-    _command_extra_chars: ClassVar[str] = ""
-
-    #: Path to the executable to start the interactive application.
-    path: ClassVar[PurePath]
-
-    def __init__(
-        self,
-        node: Node,
-        privileged: bool = False,
-        timeout: float = SETTINGS.timeout,
-        start_on_init: bool = True,
-        app_params: Params = Params(),
-    ) -> None:
-        """Create an SSH channel during initialization.
-
-        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.
-            start_on_init: Start interactive shell automatically after object initialisation.
-            app_params: The command line parameters to be passed to the application on startup.
-        """
-        self._node = node
-        self._logger = node._logger
-        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)
-
-        if start_on_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:
-        """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.
-        """
-        self._setup_ssh_channel()
-        self.send_command(self._make_start_command())
-
-    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.
-        """
-        self._logger.info(f"Sending: '{command}'")
-        if prompt is None:
-            prompt = self._default_prompt
-        self._stdin.write(f"{command}{self._command_extra_chars}\n")
-        self._stdin.flush()
-        out: str = ""
-        for line in self._stdout:
-            if skip_first_line:
-                skip_first_line = False
-                continue
-            if prompt in line and not line.rstrip().endswith(
-                command.rstrip()
-            ):  # ignore line that sent command
-                break
-            out += line
-        self._logger.debug(f"Got output: {out}")
-        return out
+        """Start the application."""
+        self._start_application()
 
     def close(self) -> None:
         """Properly free all resources."""
-        self._stdin.close()
-        self._ssh_channel.close()
+        self._close()
 
     def __del__(self) -> None:
         """Make sure the session is properly closed before deleting the object."""
         self.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)
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
new file mode 100644
index 0000000000..30c55d4703
--- /dev/null
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -0,0 +1,193 @@
+# 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  # type: ignore[import-untyped]
+from typing_extensions import Self
+
+from framework.logger import DTSLogger
+from framework.params import Params
+from framework.settings import SETTINGS
+from framework.testbed_model.node import Node
+
+
+class SingleActiveInteractiveShell(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.
+    """
+
+    _node: Node
+    _stdin: channel.ChannelStdinFile
+    _stdout: channel.ChannelFile
+    _ssh_channel: Channel
+    _logger: DTSLogger
+    _timeout: float
+    _app_params: Params
+    _privileged: bool
+    _real_path: PurePath
+
+    #: 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.
+    _command_extra_chars: ClassVar[str] = ""
+
+    #: Path to the executable to start the interactive application.
+    path: ClassVar[PurePath]
+
+    def __init__(
+        self,
+        node: Node,
+        privileged: bool = False,
+        timeout: float = SETTINGS.timeout,
+        app_params: Params = Params(),
+    ) -> None:
+        """Create an SSH channel during initialization.
+
+        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.
+        """
+        self._node = node
+        self._logger = node._logger
+        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)
+
+    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.
+        """
+        self._setup_ssh_channel()
+        self.send_command(self._make_start_command())
+
+    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.
+        """
+        self._logger.info(f"Sending: '{command}'")
+        if prompt is None:
+            prompt = self._default_prompt
+        self._stdin.write(f"{command}{self._command_extra_chars}\n")
+        self._stdin.flush()
+        out: str = ""
+        for line in self._stdout:
+            if skip_first_line:
+                skip_first_line = False
+                continue
+            if prompt in line and not line.rstrip().endswith(
+                command.rstrip()
+            ):  # ignore line that sent command
+                break
+            out += line
+        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 ec22f72221..f54a745185 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -604,7 +604,6 @@ def __init__(
         lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
         ascending_cores: bool = True,
         append_prefix_timestamp: bool = True,
-        start_on_init: bool = True,
         **app_params: Unpack[TestPmdParamsDict],
     ) -> None:
         """Overrides :meth:`~.dpdk_shell.DPDKShell.__init__`. Changes app_params to kwargs."""
@@ -615,7 +614,6 @@ def __init__(
             lcore_filter_specifier,
             ascending_cores,
             append_prefix_timestamp,
-            start_on_init,
             TestPmdParams(**app_params),
         )
 
@@ -806,7 +804,8 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
-    def close(self) -> None:
+    def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
+        self.stop()
         self.send_command("quit", "")
-        return super().close()
+        return super()._close()
diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
index bf58ad1c5e..7f0cc2bc18 100644
--- a/dts/framework/testbed_model/traffic_generator/scapy.py
+++ b/dts/framework/testbed_model/traffic_generator/scapy.py
@@ -219,6 +219,8 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
 
         self.session = PythonShell(self._tg_node, timeout=5, privileged=True)
 
+        self.session.start_application()
+
         # import libs in remote python console
         for import_statement in SCAPY_RPC_SERVER_IMPORTS:
             self.session.send_command(import_statement)
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index d954545330..db966391e8 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -102,7 +102,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = TestPmdShell(
+        testpmd_shell = TestPmdShell(
             self.sut_node,
             forward_mode=SimpleForwardingModes.mac,
             mbcache=200,
@@ -110,16 +110,20 @@ def pmd_scatter(self, mbsize: int) -> None:
             max_pkt_len=9000,
             tx_offloads=0x00008000,
         )
-        testpmd.start()
-
-        for offset in [-1, 0, 1, 4, 5]:
-            recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
-            self._logger.debug(f"Payload of scattered packet after forwarding: \n{recv_payload}")
-            self.verify(
-                ("58 " * 8).strip() in recv_payload,
-                f"Payload of scattered packet did not match expected payload with offset {offset}.",
-            )
-        testpmd.stop()
+
+        with testpmd_shell as testpmd:
+            testpmd.start()
+
+            for offset in [-1, 0, 1, 4, 5]:
+                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
+                self._logger.debug(
+                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
+                )
+                self.verify(
+                    ("58 " * 8).strip() in recv_payload,
+                    "Payload of scattered packet did not match expected payload with offset "
+                    f"{offset}.",
+                )
 
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
index eca27acfd8..377bff129d 100644
--- a/dts/tests/TestSuite_smoke_tests.py
+++ b/dts/tests/TestSuite_smoke_tests.py
@@ -100,7 +100,8 @@ def test_devices_listed_in_testpmd(self) -> None:
             List all devices found in testpmd and verify the configured devices are among them.
         """
         testpmd_driver = TestPmdShell(self.sut_node)
-        dev_list = [str(x) for x in testpmd_driver.get_devices()]
+        with testpmd_driver as testpmd:
+            dev_list = [str(x) for x in testpmd.get_devices()]
         for nic in self.nics_in_node:
             self.verify(
                 nic.pci in dev_list,
-- 
2.45.2

Re: [PATCH v1 1/2] dts: add context manager for interactive shells

[Juraj Linkeš]

On 9. 7. 2024 18:31, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> Interactive shells are managed in a way currently where they are closed
> and cleaned up at the time of garbage collection. Due to there being no
> guarantee of when this garbage collection happens in Python, there is no
> way to consistently know when an application will be closed without
> manually closing the application yourself when you are done with it.
> This doesn't cause a problem in cases where you can start another
> instance of the same application multiple times on a server, but this
> isn't the case for primary applications in DPDK. The introduction of
> primary applications, such as testpmd, adds a need for knowing previous
> instances of the application have been stopped and cleaned up before
> starting a new one, which the garbage collector does not provide.
> 
> To solve this problem, a new class is added which acts as a base class
> for interactive shells that enforces that instances of the
> application be managed using a context manager. Using a context manager
> guarantees that once you leave the scope of the block where the
> application is being used for any reason, the application will be closed
> immediately. This avoids the possibility of the shell not being closed
> due to an exception being raised or user error. The interactive shell
> class then becomes shells that can be started/stopped manually or at the
> time of garbage collection rather than through a context manager.
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Reviewed-by: Patrick Robb <probb@iol.unh.edu>
> Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---

Just one minor inconsequential point below. My tag is still valid.

> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> index eca27acfd8..377bff129d 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -100,7 +100,8 @@ def test_devices_listed_in_testpmd(self) -> None:
>               List all devices found in testpmd and verify the configured devices are among them.
>           """
>           testpmd_driver = TestPmdShell(self.sut_node)
> -        dev_list = [str(x) for x in testpmd_driver.get_devices()]
> +        with testpmd_driver as testpmd:

The usual way to use context managers in Python is without the intent of 
using the object after it leaves the context:

with TestPmdShell(self.sut_node) as testpmd:

That said, the way you did it in the scatter test case seems fine 
because it looks more readable. Maybe we can just change it here, but 
it's a minor point and doesn't really matter.

> +            dev_list = [str(x) for x in testpmd.get_devices()]
>           for nic in self.nics_in_node:
>               self.verify(
>                   nic.pci in dev_list,

Re: [PATCH v1 1/2] dts: add context manager for interactive shells

[Jeremy Spewock]

On Thu, Jul 11, 2024 at 10:35 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 9. 7. 2024 18:31, jspewock@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > Interactive shells are managed in a way currently where they are closed
> > and cleaned up at the time of garbage collection. Due to there being no
> > guarantee of when this garbage collection happens in Python, there is no
> > way to consistently know when an application will be closed without
> > manually closing the application yourself when you are done with it.
> > This doesn't cause a problem in cases where you can start another
> > instance of the same application multiple times on a server, but this
> > isn't the case for primary applications in DPDK. The introduction of
> > primary applications, such as testpmd, adds a need for knowing previous
> > instances of the application have been stopped and cleaned up before
> > starting a new one, which the garbage collector does not provide.
> >
> > To solve this problem, a new class is added which acts as a base class
> > for interactive shells that enforces that instances of the
> > application be managed using a context manager. Using a context manager
> > guarantees that once you leave the scope of the block where the
> > application is being used for any reason, the application will be closed
> > immediately. This avoids the possibility of the shell not being closed
> > due to an exception being raised or user error. The interactive shell
> > class then becomes shells that can be started/stopped manually or at the
> > time of garbage collection rather than through a context manager.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Reviewed-by: Patrick Robb <probb@iol.unh.edu>
> > Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
> > ---
>
> Just one minor inconsequential point below. My tag is still valid.
>
> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> > index eca27acfd8..377bff129d 100644
> > --- a/dts/tests/TestSuite_smoke_tests.py
> > +++ b/dts/tests/TestSuite_smoke_tests.py
> > @@ -100,7 +100,8 @@ def test_devices_listed_in_testpmd(self) -> None:
> >               List all devices found in testpmd and verify the configured devices are among them.
> >           """
> >           testpmd_driver = TestPmdShell(self.sut_node)
> > -        dev_list = [str(x) for x in testpmd_driver.get_devices()]
> > +        with testpmd_driver as testpmd:
>
> The usual way to use context managers in Python is without the intent of
> using the object after it leaves the context:
>
> with TestPmdShell(self.sut_node) as testpmd:
>
> That said, the way you did it in the scatter test case seems fine
> because it looks more readable. Maybe we can just change it here, but
> it's a minor point and doesn't really matter.
>

This is a good point. Originally I also did it in two separate lines
because it used to have to be a call to a sut_node method and was just
long and convoluted, but I think now that you instantiate the class
directly doing it this way makes more sense. I have no problem with
updating both of them.

Just as one thing to note however, this context manager is a little
different by design. When writing it I actually made some minor tweaks
specifically so that the same instance could be used multiple times. I
figured this was something that we didn't really need to use and
probably wouldn't use often, but could be useful in the future if you
needed a shell that was identical ("identical" as in parameters-wise,
of course the instances would be different) across test cases since
all leaving the context does is close the shell.

> > +            dev_list = [str(x) for x in testpmd.get_devices()]
> >           for nic in self.nics_in_node:
> >               self.verify(
> >                   nic.pci in dev_list,

[PATCH v1 2/2] dts: improve starting and stopping interactive shells

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

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 <jspewock@iol.unh.edu>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
---
 .../remote_session/interactive_shell.py       | 29 ++++++++----
 .../single_active_interactive_shell.py        | 46 +++++++++++++++++--
 dts/framework/remote_session/testpmd_shell.py |  2 +-
 3 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 11dc8a0643..fcaf1f6a5f 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -9,6 +9,9 @@
 collection.
 """
 
+import weakref
+from typing import ClassVar
+
 from .single_active_interactive_shell import SingleActiveInteractiveShell
 
 
@@ -16,18 +19,26 @@ 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) -> None:
-        """Start the application."""
+        """Start the application.
+
+        After the application has started, add a weakref finalize class to manage cleanup.
+        """
         self._start_application()
+        self._finalizer = weakref.finalize(self, self._close)
 
     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 30c55d4703..38094c0fe2 100644
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -27,6 +27,7 @@
 from paramiko import Channel, channel  # type: ignore[import-untyped]
 from typing_extensions import Self
 
+from framework.exception import InteractiveCommandExecutionError
 from framework.logger import DTSLogger
 from framework.params import Params
 from framework.settings import SETTINGS
@@ -45,6 +46,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.
     """
 
     _node: Node
@@ -57,6 +62,9 @@ class SingleActiveInteractiveShell(ABC):
     _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] = ""
@@ -69,6 +77,8 @@ class SingleActiveInteractiveShell(ABC):
     #: Path to the executable to start the interactive application.
     path: ClassVar[PurePath]
 
+    is_alive: bool = False
+
     def __init__(
         self,
         node: Node,
@@ -111,11 +121,33 @@ def _make_start_command(self) -> str:
     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.
+        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.send_command(self._make_start_command())
+        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 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, skip_first_line: bool = False
@@ -139,7 +171,15 @@ def send_command(
 
         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.
         """
+        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 f54a745185..eda6eb320f 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -807,5 +807,5 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-        self.send_command("quit", "")
+        self.send_command("quit", "Bye...")
         return super()._close()
-- 
2.45.2

Re: [PATCH v1 2/2] dts: improve starting and stopping interactive shells

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 63 bytes --]

For the whole series:
Tested-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 150 bytes --]

Re: [PATCH v1 2/2] dts: improve starting and stopping interactive shells

[Juraj Linkeš]

With the docs changes below,
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 11dc8a0643..fcaf1f6a5f 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -9,6 +9,9 @@
>   collection.
>   """
>   
> +import weakref
> +from typing import ClassVar
> +
>   from .single_active_interactive_shell import SingleActiveInteractiveShell
>   
>   
> @@ -16,18 +19,26 @@ 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.

This wording is a bit confusing. This could mean that the shells could 
require multiple instances. We could try an alternative approach:
One attempt should be enough for shells which don't have to worry about 
other instances closing before starting a new one.

Or something similar.

> +    _init_attempts: ClassVar[int] = 1
> +
>       def start_application(self) -> None:
> -        """Start the application."""
> +        """Start the application.
> +
> +        After the application has started, add a weakref finalize class to manage cleanup.

I think this could be improved to:
use :class:`weakref.finalize` to manage cleanup

> +        """
>           self._start_application()
> +        self._finalizer = weakref.finalize(self, self._close)
>   
>       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."""

Also here:
using :class:`weakref.finalize`

> +        self._finalizer()

Re: [PATCH v1 2/2] dts: improve starting and stopping interactive shells

[Jeremy Spewock]

On Thu, Jul 11, 2024 at 10:53 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
> With the docs changes below,
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index 11dc8a0643..fcaf1f6a5f 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -9,6 +9,9 @@
> >   collection.
> >   """
> >
> > +import weakref
> > +from typing import ClassVar
> > +
> >   from .single_active_interactive_shell import SingleActiveInteractiveShell
> >
> >
> > @@ -16,18 +19,26 @@ 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.
>
> This wording is a bit confusing. This could mean that the shells could
> require multiple instances. We could try an alternative approach:
> One attempt should be enough for shells which don't have to worry about
> other instances closing before starting a new one.
>
> Or something similar.

Good point, I'll make this change.

>
> > +    _init_attempts: ClassVar[int] = 1
> > +
> >       def start_application(self) -> None:
> > -        """Start the application."""
> > +        """Start the application.
> > +
> > +        After the application has started, add a weakref finalize class to manage cleanup.
>
> I think this could be improved to:
> use :class:`weakref.finalize` to manage cleanup

Good idea, referencing the actual class like this is more clear.

>
> > +        """
> >           self._start_application()
> > +        self._finalizer = weakref.finalize(self, self._close)
> >
> >       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."""
>
> Also here:
> using :class:`weakref.finalize`

Ack.

>
> > +        self._finalizer()
>

[PATCH v2 0/2] dts: add context manager

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

v2:
 * addresses the comments from version 1, adjusting documentation
   accordingly and condensing usage of the context manager.

Jeremy Spewock (2):
  dts: add context manager for interactive shells
  dts: improve starting and stopping interactive shells

 dts/framework/remote_session/dpdk_shell.py    |   9 +-
 .../remote_session/interactive_shell.py       | 171 ++-----------
 .../single_active_interactive_shell.py        | 233 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py |   9 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  26 +-
 dts/tests/TestSuite_smoke_tests.py            |   4 +-
 7 files changed, 284 insertions(+), 170 deletions(-)
 create mode 100644 dts/framework/remote_session/single_active_interactive_shell.py

-- 
2.45.2

[PATCH v2 1/2] dts: add context manager for interactive shells

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Interactive shells are managed in a way currently where they are closed
and cleaned up at the time of garbage collection. Due to there being no
guarantee of when this garbage collection happens in Python, there is no
way to consistently know when an application will be closed without
manually closing the application yourself when you are done with it.
This doesn't cause a problem in cases where you can start another
instance of the same application multiple times on a server, but this
isn't the case for primary applications in DPDK. The introduction of
primary applications, such as testpmd, adds a need for knowing previous
instances of the application have been stopped and cleaned up before
starting a new one, which the garbage collector does not provide.

To solve this problem, a new class is added which acts as a base class
for interactive shells that enforces that instances of the
application be managed using a context manager. Using a context manager
guarantees that once you leave the scope of the block where the
application is being used for any reason, the application will be closed
immediately. This avoids the possibility of the shell not being closed
due to an exception being raised or user error. The interactive shell
class then becomes shells that can be started/stopped manually or at the
time of garbage collection rather than through a context manager.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/remote_session/dpdk_shell.py    |   9 +-
 .../remote_session/interactive_shell.py       | 160 ++-------------
 .../single_active_interactive_shell.py        | 193 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py |   7 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  26 +--
 dts/tests/TestSuite_smoke_tests.py            |   4 +-
 7 files changed, 232 insertions(+), 169 deletions(-)
 create 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 296639f37d..950c6ca670 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -11,7 +11,9 @@
 from pathlib import PurePath
 
 from framework.params.eal import EalParams
-from framework.remote_session.interactive_shell import InteractiveShell
+from framework.remote_session.single_active_interactive_shell import (
+    SingleActiveInteractiveShell,
+)
 from framework.settings import SETTINGS
 from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
 from framework.testbed_model.sut_node import SutNode
@@ -60,7 +62,7 @@ def compute_eal_params(
     return params
 
 
-class DPDKShell(InteractiveShell, ABC):
+class DPDKShell(SingleActiveInteractiveShell, ABC):
     """The base class for managing DPDK-based interactive shells.
 
     This class shouldn't be instantiated directly, but instead be extended.
@@ -79,7 +81,6 @@ def __init__(
         lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
         ascending_cores: bool = True,
         append_prefix_timestamp: bool = True,
-        start_on_init: bool = True,
         app_params: EalParams = EalParams(),
     ) -> None:
         """Extends :meth:`~.interactive_shell.InteractiveShell.__init__`.
@@ -95,7 +96,7 @@ def __init__(
             append_prefix_timestamp,
         )
 
-        super().__init__(node, privileged, timeout, start_on_init, app_params)
+        super().__init__(node, privileged, timeout, app_params)
 
     def _update_real_path(self, path: PurePath) -> None:
         """Extends :meth:`~.interactive_shell.InteractiveShell._update_real_path`.
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 254aa29f89..11dc8a0643 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -2,166 +2,32 @@
 # Copyright(c) 2023 University of New Hampshire
 # Copyright(c) 2024 Arm Limited
 
-"""Common functionality for interactive shell handling.
+"""Interactive shell with manual stop/start functionality.
 
-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.
+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.
 """
 
-from abc import ABC
-from pathlib import PurePath
-from typing import ClassVar
-
-from paramiko import Channel, channel  # type: ignore[import-untyped]
-
-from framework.logger import DTSLogger
-from framework.params import Params
-from framework.settings import SETTINGS
-from framework.testbed_model.node import Node
+from .single_active_interactive_shell import SingleActiveInteractiveShell
 
 
-class InteractiveShell(ABC):
-    """The base class for managing interactive shells.
+class InteractiveShell(SingleActiveInteractiveShell):
+    """Adds manual start and stop functionality to 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.
+    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.
     """
 
-    _node: Node
-    _stdin: channel.ChannelStdinFile
-    _stdout: channel.ChannelFile
-    _ssh_channel: Channel
-    _logger: DTSLogger
-    _timeout: float
-    _app_params: Params
-    _privileged: bool
-    _real_path: PurePath
-
-    #: 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.
-    _command_extra_chars: ClassVar[str] = ""
-
-    #: Path to the executable to start the interactive application.
-    path: ClassVar[PurePath]
-
-    def __init__(
-        self,
-        node: Node,
-        privileged: bool = False,
-        timeout: float = SETTINGS.timeout,
-        start_on_init: bool = True,
-        app_params: Params = Params(),
-    ) -> None:
-        """Create an SSH channel during initialization.
-
-        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.
-            start_on_init: Start interactive shell automatically after object initialisation.
-            app_params: The command line parameters to be passed to the application on startup.
-        """
-        self._node = node
-        self._logger = node._logger
-        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)
-
-        if start_on_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:
-        """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.
-        """
-        self._setup_ssh_channel()
-        self.send_command(self._make_start_command())
-
-    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.
-        """
-        self._logger.info(f"Sending: '{command}'")
-        if prompt is None:
-            prompt = self._default_prompt
-        self._stdin.write(f"{command}{self._command_extra_chars}\n")
-        self._stdin.flush()
-        out: str = ""
-        for line in self._stdout:
-            if skip_first_line:
-                skip_first_line = False
-                continue
-            if prompt in line and not line.rstrip().endswith(
-                command.rstrip()
-            ):  # ignore line that sent command
-                break
-            out += line
-        self._logger.debug(f"Got output: {out}")
-        return out
+        """Start the application."""
+        self._start_application()
 
     def close(self) -> None:
         """Properly free all resources."""
-        self._stdin.close()
-        self._ssh_channel.close()
+        self._close()
 
     def __del__(self) -> None:
         """Make sure the session is properly closed before deleting the object."""
         self.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)
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
new file mode 100644
index 0000000000..30c55d4703
--- /dev/null
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -0,0 +1,193 @@
+# 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  # type: ignore[import-untyped]
+from typing_extensions import Self
+
+from framework.logger import DTSLogger
+from framework.params import Params
+from framework.settings import SETTINGS
+from framework.testbed_model.node import Node
+
+
+class SingleActiveInteractiveShell(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.
+    """
+
+    _node: Node
+    _stdin: channel.ChannelStdinFile
+    _stdout: channel.ChannelFile
+    _ssh_channel: Channel
+    _logger: DTSLogger
+    _timeout: float
+    _app_params: Params
+    _privileged: bool
+    _real_path: PurePath
+
+    #: 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.
+    _command_extra_chars: ClassVar[str] = ""
+
+    #: Path to the executable to start the interactive application.
+    path: ClassVar[PurePath]
+
+    def __init__(
+        self,
+        node: Node,
+        privileged: bool = False,
+        timeout: float = SETTINGS.timeout,
+        app_params: Params = Params(),
+    ) -> None:
+        """Create an SSH channel during initialization.
+
+        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.
+        """
+        self._node = node
+        self._logger = node._logger
+        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)
+
+    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.
+        """
+        self._setup_ssh_channel()
+        self.send_command(self._make_start_command())
+
+    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.
+        """
+        self._logger.info(f"Sending: '{command}'")
+        if prompt is None:
+            prompt = self._default_prompt
+        self._stdin.write(f"{command}{self._command_extra_chars}\n")
+        self._stdin.flush()
+        out: str = ""
+        for line in self._stdout:
+            if skip_first_line:
+                skip_first_line = False
+                continue
+            if prompt in line and not line.rstrip().endswith(
+                command.rstrip()
+            ):  # ignore line that sent command
+                break
+            out += line
+        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 ec22f72221..f54a745185 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -604,7 +604,6 @@ def __init__(
         lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
         ascending_cores: bool = True,
         append_prefix_timestamp: bool = True,
-        start_on_init: bool = True,
         **app_params: Unpack[TestPmdParamsDict],
     ) -> None:
         """Overrides :meth:`~.dpdk_shell.DPDKShell.__init__`. Changes app_params to kwargs."""
@@ -615,7 +614,6 @@ def __init__(
             lcore_filter_specifier,
             ascending_cores,
             append_prefix_timestamp,
-            start_on_init,
             TestPmdParams(**app_params),
         )
 
@@ -806,7 +804,8 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
-    def close(self) -> None:
+    def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
+        self.stop()
         self.send_command("quit", "")
-        return super().close()
+        return super()._close()
diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
index bf58ad1c5e..7f0cc2bc18 100644
--- a/dts/framework/testbed_model/traffic_generator/scapy.py
+++ b/dts/framework/testbed_model/traffic_generator/scapy.py
@@ -219,6 +219,8 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
 
         self.session = PythonShell(self._tg_node, timeout=5, privileged=True)
 
+        self.session.start_application()
+
         # import libs in remote python console
         for import_statement in SCAPY_RPC_SERVER_IMPORTS:
             self.session.send_command(import_statement)
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index d954545330..0d8e101e5c 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -102,24 +102,26 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = TestPmdShell(
+        with TestPmdShell(
             self.sut_node,
             forward_mode=SimpleForwardingModes.mac,
             mbcache=200,
             mbuf_size=[mbsize],
             max_pkt_len=9000,
             tx_offloads=0x00008000,
-        )
-        testpmd.start()
-
-        for offset in [-1, 0, 1, 4, 5]:
-            recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
-            self._logger.debug(f"Payload of scattered packet after forwarding: \n{recv_payload}")
-            self.verify(
-                ("58 " * 8).strip() in recv_payload,
-                f"Payload of scattered packet did not match expected payload with offset {offset}.",
-            )
-        testpmd.stop()
+        ) as testpmd:
+            testpmd.start()
+
+            for offset in [-1, 0, 1, 4, 5]:
+                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
+                self._logger.debug(
+                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
+                )
+                self.verify(
+                    ("58 " * 8).strip() in recv_payload,
+                    "Payload of scattered packet did not match expected payload with offset "
+                    f"{offset}.",
+                )
 
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
index eca27acfd8..c0b0e6bb00 100644
--- a/dts/tests/TestSuite_smoke_tests.py
+++ b/dts/tests/TestSuite_smoke_tests.py
@@ -99,8 +99,8 @@ def test_devices_listed_in_testpmd(self) -> None:
         Test:
             List all devices found in testpmd and verify the configured devices are among them.
         """
-        testpmd_driver = TestPmdShell(self.sut_node)
-        dev_list = [str(x) for x in testpmd_driver.get_devices()]
+        with TestPmdShell(self.sut_node) as testpmd:
+            dev_list = [str(x) for x in testpmd.get_devices()]
         for nic in self.nics_in_node:
             self.verify(
                 nic.pci in dev_list,
-- 
2.45.2

[PATCH v2 2/2] dts: improve starting and stopping interactive shells

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

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 <jspewock@iol.unh.edu>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 .../remote_session/interactive_shell.py       | 29 ++++++++----
 .../single_active_interactive_shell.py        | 46 +++++++++++++++++--
 dts/framework/remote_session/testpmd_shell.py |  2 +-
 3 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 11dc8a0643..9ca285b604 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -9,6 +9,9 @@
 collection.
 """
 
+import weakref
+from typing import ClassVar
+
 from .single_active_interactive_shell import SingleActiveInteractiveShell
 
 
@@ -16,18 +19,26 @@ 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
+    #: 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
+
     def start_application(self) -> None:
-        """Start the application."""
+        """Start the application.
+
+        After the application has started, use :class:`weakref.finalize` to manage cleanup.
+        """
         self._start_application()
+        self._finalizer = weakref.finalize(self, self._close)
 
     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 :class:`weakref.finalize`."""
+        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 30c55d4703..38094c0fe2 100644
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -27,6 +27,7 @@
 from paramiko import Channel, channel  # type: ignore[import-untyped]
 from typing_extensions import Self
 
+from framework.exception import InteractiveCommandExecutionError
 from framework.logger import DTSLogger
 from framework.params import Params
 from framework.settings import SETTINGS
@@ -45,6 +46,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.
     """
 
     _node: Node
@@ -57,6 +62,9 @@ class SingleActiveInteractiveShell(ABC):
     _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] = ""
@@ -69,6 +77,8 @@ class SingleActiveInteractiveShell(ABC):
     #: Path to the executable to start the interactive application.
     path: ClassVar[PurePath]
 
+    is_alive: bool = False
+
     def __init__(
         self,
         node: Node,
@@ -111,11 +121,33 @@ def _make_start_command(self) -> str:
     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.
+        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.send_command(self._make_start_command())
+        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 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, skip_first_line: bool = False
@@ -139,7 +171,15 @@ def send_command(
 
         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.
         """
+        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 f54a745185..eda6eb320f 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -807,5 +807,5 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-        self.send_command("quit", "")
+        self.send_command("quit", "Bye...")
         return super()._close()
-- 
2.45.2

Re: [PATCH v2 2/2] dts: improve starting and stopping interactive shells

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 41 bytes --]

Tested-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 107 bytes --]

Re: [PATCH v2 0/2] dts: add context manager

[Thomas Monjalon]

11/07/2024 18:34, jspewock@iol.unh.edu:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> v2:
>  * addresses the comments from version 1, adjusting documentation
>    accordingly and condensing usage of the context manager.
> 
> Jeremy Spewock (2):
>   dts: add context manager for interactive shells
>   dts: improve starting and stopping interactive shells

Applied, thanks.