[PATCH v1 0/4] Add second scatter test case

[PATCH v1 0/4] Add second scatter test case

[jspewock]

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

The current test suite for testing the scatter gather capabilities of a
NIC currently does not support Mellanox NICs since these NICs require
that you first enable the scattered_rx offload when you start testpmd,
but some other PMDs do not. This patch series adds an expansion of the
scatter test suite which has a test case that tests the functionality
with the offload, and it leverages the capabilities patch to enforce
that the previous test case gets skipped when not supported.

Additionally, since this is the first time we are running testpmd
multiple times in a row, more improvements were added surrounding the
usage of interactive shells in order to make things like the starting
and cleanup more consistent.

Jeremy Spewock (4):
  dts: improve starting and stopping interactive shells
  dts: add context manager for interactive shells
  dts: add methods for modifying MTU to testpmd shell
  dts: add test case that utilizes offload to pmd_buffer_scatter

 .../critical_interactive_shell.py             | 98 +++++++++++++++++++
 .../remote_session/interactive_shell.py       | 64 +++++++++---
 dts/framework/remote_session/testpmd_shell.py | 76 +++++++++++++-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 79 ++++++++++-----
 dts/tests/TestSuite_smoke_tests.py            |  3 +-
 6 files changed, 282 insertions(+), 46 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

-- 
2.44.0

[PATCH v1 1/4] 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>
---
 .../remote_session/interactive_shell.py       | 51 ++++++++++++++++---
 dts/framework/remote_session/testpmd_shell.py |  4 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..d1a9d8a6d2 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -14,12 +14,14 @@
 environment variable configure the timeout of getting the output from command execution.
 """
 
+import weakref
 from abc import ABC
 from pathlib import PurePath
 from typing import Callable, ClassVar
 
 from paramiko import Channel, SSHClient, channel  # type: ignore[import]
 
+from framework.exception import InteractiveCommandExecutionError
 from framework.logger import DTSLogger
 from framework.settings import SETTINGS
 
@@ -32,6 +34,10 @@ class InteractiveShell(ABC):
     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.
+
+    Attributes:
+        is_started: :data:`True` if the application has started successfully, :data:`False`
+            otherwise.
     """
 
     _interactive_session: SSHClient
@@ -41,6 +47,7 @@ class InteractiveShell(ABC):
     _logger: DTSLogger
     _timeout: float
     _app_args: str
+    _finalizer: weakref.finalize
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -58,6 +65,8 @@ class InteractiveShell(ABC):
     #: for DPDK on the node will be prepended to the path to the executable.
     dpdk_app: ClassVar[bool] = False
 
+    is_started: bool = False
+
     def __init__(
         self,
         interactive_session: SSHClient,
@@ -93,17 +102,39 @@ def __init__(
     def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
+        done because some DPDK applications need slightly more time after exiting their script to
+        clean up EAL before others can start.
+
+        When the application is started we also bind a class for finalization to this instance of
+        the shell to ensure proper cleanup of the application.
 
         Args:
             get_privileged_command: A function (but could be any callable) that produces
                 the version of the command with elevated privileges.
         """
+        self._finalizer = weakref.finalize(self, self._close)
+        max_retries = 5
+        self._ssh_channel.settimeout(1)
         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_started = True
+        for retry in range(max_retries):
+            try:
+                self.send_command(start_command)
+                break
+            except TimeoutError:
+                self._logger.info(
+                    "Interactive shell failed to start, retrying... "
+                    f"({retry+1} out of {max_retries})"
+                )
+        else:
+            self._ssh_channel.settimeout(self._timeout)
+            self.is_started = 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.
@@ -125,6 +156,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_started:
+            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
@@ -140,11 +175,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._logger.debug(f"Got output: {out}")
         return out
 
-    def close(self) -> None:
-        """Properly free all resources."""
+    def _close(self) -> None:
+        self.is_started = False
         self._stdin.close()
         self._ssh_channel.close()
 
-    def __del__(self) -> None:
-        """Make sure the session is properly closed before deleting the object."""
-        self.close()
+    def close(self) -> None:
+        """Properly free all resources."""
+        self._finalizer()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index f6783af621..cb4642bf3d 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -227,10 +227,10 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
-    def close(self) -> None:
+    def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-        return super().close()
+        return super()._close()
 
     def get_capas_rxq(
         self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
-- 
2.44.0

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

[Luca Vizzarro]

Looks good to me! Thank you for your work.

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

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

[Patrick Robb]

Reviewed-by: Patrick Robb <probb@iol.unh.edu>

[PATCH v1 2/4] 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 wrapper
around the interactive shell 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.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 .../critical_interactive_shell.py             | 98 +++++++++++++++++++
 .../remote_session/interactive_shell.py       | 13 ++-
 dts/framework/remote_session/testpmd_shell.py |  4 +-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 28 +++---
 dts/tests/TestSuite_smoke_tests.py            |  3 +-
 6 files changed, 130 insertions(+), 24 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
new file mode 100644
index 0000000000..d61b203954
--- /dev/null
+++ b/dts/framework/remote_session/critical_interactive_shell.py
@@ -0,0 +1,98 @@
+r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
+
+Critical applications are defined as applications that require explicit clean-up before another
+instance of some application can be started. In DPDK these are referred to as "primary
+applications" and these applications take out a lock which stops other primary applications from
+running. Much like :class:`~.interactive_shell.InteractiveShell`\s,
+:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
+specific functionality and should never be instantiated directly.
+"""
+
+from types import TracebackType
+from typing import Callable, TypeVar
+
+from paramiko import SSHClient  # type: ignore[import]
+
+from framework.logger import DTSLogger
+from framework.settings import SETTINGS
+
+from .interactive_shell import InteractiveShell
+
+CriticalInteractiveShellType = TypeVar(
+    "CriticalInteractiveShellType", bound="CriticalInteractiveShell"
+)
+
+
+class CriticalInteractiveShell(InteractiveShell):
+    """The base class for interactive critical applications.
+
+    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
+    implement the exact same functionality with the primary difference being how the application
+    is started and stopped. In contrast to normal interactive shells, this class does not start the
+    application upon initialization of the class. Instead, the application is handled through a
+    context manager. This allows for more explicit starting and stopping of the application, and
+    more guarantees for when the application is cleaned up which are not present with normal
+    interactive shells that get cleaned up upon garbage collection.
+    """
+
+    _get_priviledged_command: Callable[[str], str] | None
+
+    def __init__(
+        self,
+        interactive_session: SSHClient,
+        logger: DTSLogger,
+        get_privileged_command: Callable[[str], str] | None,
+        app_args: str = "",
+        timeout: float = SETTINGS.timeout,
+    ) -> None:
+        """Store parameters for creating an interactive shell, but do not start the application.
+
+        Note that this method also does not create the channel for the application, as this is
+        something that isn't needed until the application starts.
+
+        Args:
+            interactive_session: The SSH session dedicated to interactive shells.
+            logger: The logger instance this session will use.
+            get_privileged_command: A method for modifying a command to allow it to use
+                elevated privileges. If :data:`None`, the application will not be started
+                with elevated privileges.
+            app_args: The command line arguments to be passed to the application on startup.
+            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. The default
+                value for this argument may be modified using the :option:`--timeout` command-line
+                argument or the :envvar:`DTS_TIMEOUT` environment variable.
+        """
+        self._interactive_session = interactive_session
+        self._logger = logger
+        self._timeout = timeout
+        self._app_args = app_args
+        self._get_priviledged_command = get_privileged_command
+
+    def __enter__(self: CriticalInteractiveShellType) -> CriticalInteractiveShellType:
+        """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._init_channel()
+        self._start_application(self._get_priviledged_command)
+        return self
+
+    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> 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 it's 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.
+
+        Args:
+            type: Type of exception that was thrown in the context block if there was one.
+            value: Value of the exception thrown in the context block if there was one.
+            traceback: Traceback of the exception thrown in the context block if there was one.
+        """
+        self.close()
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index d1a9d8a6d2..08b8ba6a3e 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -89,16 +89,19 @@ def __init__(
                 and no output is gathered within the timeout, an exception is thrown.
         """
         self._interactive_session = interactive_session
-        self._ssh_channel = self._interactive_session.invoke_shell()
-        self._stdin = self._ssh_channel.makefile_stdin("w")
-        self._stdout = self._ssh_channel.makefile("r")
-        self._ssh_channel.settimeout(timeout)
-        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
         self._logger = logger
         self._timeout = timeout
         self._app_args = app_args
+        self._init_channel()
         self._start_application(get_privileged_command)
 
+    def _init_channel(self):
+        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
         """Starts a new interactive application based on the path to the app.
 
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb4642bf3d..33b3e7c5a3 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -26,7 +26,7 @@
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
-from .interactive_shell import InteractiveShell
+from .critical_interactive_shell import CriticalInteractiveShell
 
 
 class TestPmdDevice(object):
@@ -82,7 +82,7 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
-class TestPmdShell(InteractiveShell):
+class TestPmdShell(CriticalInteractiveShell):
     """Testpmd interactive shell.
 
     The testpmd shell users should never use
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 1fb536735d..7dd39fd735 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -243,10 +243,10 @@ def get_supported_capabilities(
         unsupported_capas: set[NicCapability] = set()
         self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
         testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
-        for capability in capabilities:
-            if capability not in supported_capas or capability not in unsupported_capas:
-                capability.value(testpmd_shell, supported_capas, unsupported_capas)
-        del testpmd_shell
+        with testpmd_shell as running_testpmd:
+            for capability in capabilities:
+                if capability not in supported_capas or capability not in unsupported_capas:
+                    capability.value(running_testpmd, supported_capas, unsupported_capas)
         return supported_capas
 
     def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 3701c47408..41f6090a7e 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = self.sut_node.create_interactive_shell(
+        testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
             app_parameters=(
                 "--mbcache=200 "
@@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
             ),
             privileged=True,
         )
-        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
-        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.set_forward_mode(TestPmdForwardingModes.mac)
+            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}.",
+                )
+            testpmd.stop()
 
     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 a553e89662..360e64eb5a 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 = self.sut_node.create_interactive_shell(TestPmdShell, privileged=True)
-        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.44.0

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

[Luca Vizzarro]

On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> +class CriticalInteractiveShell(InteractiveShell):
<snip>
> +    _get_priviledged_command: Callable[[str], str] | None
typo: privileged
> +
> +    def __init__(
> +        self,
> +        interactive_session: SSHClient,
> +        logger: DTSLogger,
> +        get_privileged_command: Callable[[str], str] | None,
> +        app_args: str = "",
> +        timeout: float = SETTINGS.timeout,
> +    ) -> None:
> +        """Store parameters for creating an interactive shell, but do not start the application.
> +
> +        Note that this method also does not create the channel for the application, as this is
> +        something that isn't needed until the application starts.
> +
> +        Args:
> +            interactive_session: The SSH session dedicated to interactive shells.
> +            logger: The logger instance this session will use.
> +            get_privileged_command: A method for modifying a command to allow it to use
> +                elevated privileges. If :data:`None`, the application will not be started
> +                with elevated privileges.
> +            app_args: The command line arguments to be passed to the application on startup.
> +            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. The default
> +                value for this argument may be modified using the :option:`--timeout` command-line
> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> +        """
> +        self._interactive_session = interactive_session
> +        self._logger = logger
> +        self._timeout = timeout
> +        self._app_args = app_args
> +        self._get_priviledged_command = get_privileged_command
> +
> +    def __enter__(self: CriticalInteractiveShellType) -> CriticalInteractiveShellType:

This kind of type hinting is achievable with Python's own `Self`, which 
you can already add to this patch because mypy installs 
typing_extensions. Nevertheless, `Self` is also being introduced 
properly in my mypy update patch series.

> +
> +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> None:
Since you are not using the arguments I'd just ignore them with `_`.
> +        """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 it's 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.
> +
> +        Args:
> +            type: Type of exception that was thrown in the context block if there was one.
> +            value: Value of the exception thrown in the context block if there was one.
> +            traceback: Traceback of the exception thrown in the context block if there was one.
> +        """
> +        self.close()

> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index d1a9d8a6d2..08b8ba6a3e 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -89,16 +89,19 @@ def __init__(
>                   and no output is gathered within the timeout, an exception is thrown.
>           """
>           self._interactive_session = interactive_session
> -        self._ssh_channel = self._interactive_session.invoke_shell()
> -        self._stdin = self._ssh_channel.makefile_stdin("w")
> -        self._stdout = self._ssh_channel.makefile("r")
> -        self._ssh_channel.settimeout(timeout)
> -        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
>           self._logger = logger
>           self._timeout = timeout
>           self._app_args = app_args
> +        self._init_channel()
>           self._start_application(get_privileged_command)
>   
> +    def _init_channel(self):
> +        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
>           """Starts a new interactive application based on the path to the app.
>   
Hilariously I've made the same exact change in my testpmd params patch 
series!
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 3701c47408..41f6090a7e 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
>               ),
>               privileged=True,
>           )
> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> -        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.set_forward_mode(TestPmdForwardingModes.mac)
> +            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}.",
> +                )
> +            testpmd.stop()

Since we are exiting the context, implicitly it means we want to stop 
and close. Can't we also implicit call stop when closing?

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

[Jeremy Spewock]

On Mon, May 20, 2024 at 1:31 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> > +class CriticalInteractiveShell(InteractiveShell):
> <snip>
> > +    _get_priviledged_command: Callable[[str], str] | None
> typo: privileged

Ack.

> > +
> > +    def __init__(
> > +        self,
> > +        interactive_session: SSHClient,
> > +        logger: DTSLogger,
> > +        get_privileged_command: Callable[[str], str] | None,
> > +        app_args: str = "",
> > +        timeout: float = SETTINGS.timeout,
> > +    ) -> None:
> > +        """Store parameters for creating an interactive shell, but do not start the application.
> > +
> > +        Note that this method also does not create the channel for the application, as this is
> > +        something that isn't needed until the application starts.
> > +
> > +        Args:
> > +            interactive_session: The SSH session dedicated to interactive shells.
> > +            logger: The logger instance this session will use.
> > +            get_privileged_command: A method for modifying a command to allow it to use
> > +                elevated privileges. If :data:`None`, the application will not be started
> > +                with elevated privileges.
> > +            app_args: The command line arguments to be passed to the application on startup.
> > +            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. The default
> > +                value for this argument may be modified using the :option:`--timeout` command-line
> > +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> > +        """
> > +        self._interactive_session = interactive_session
> > +        self._logger = logger
> > +        self._timeout = timeout
> > +        self._app_args = app_args
> > +        self._get_priviledged_command = get_privileged_command
> > +
> > +    def __enter__(self: CriticalInteractiveShellType) -> CriticalInteractiveShellType:
>
> This kind of type hinting is achievable with Python's own `Self`, which
> you can already add to this patch because mypy installs
> typing_extensions. Nevertheless, `Self` is also being introduced
> properly in my mypy update patch series.

This is a great point. I actually didn't know the `Self` annotation
was a thing at the time of writing, but saw you using it in some of
your own patches. I think that would fit much better here and will
make the change.

>
> > +
> > +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> None:
> Since you are not using the arguments I'd just ignore them with `_`.

Good point. I initially left them in case there was some kind of need
for using them somehow in the future, but there really is no need for
them.

> > +        """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 it's 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.
> > +
> > +        Args:
> > +            type: Type of exception that was thrown in the context block if there was one.
> > +            value: Value of the exception thrown in the context block if there was one.
> > +            traceback: Traceback of the exception thrown in the context block if there was one.
> > +        """
> > +        self.close()
>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index d1a9d8a6d2..08b8ba6a3e 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -89,16 +89,19 @@ def __init__(
> >                   and no output is gathered within the timeout, an exception is thrown.
> >           """
> >           self._interactive_session = interactive_session
> > -        self._ssh_channel = self._interactive_session.invoke_shell()
> > -        self._stdin = self._ssh_channel.makefile_stdin("w")
> > -        self._stdout = self._ssh_channel.makefile("r")
> > -        self._ssh_channel.settimeout(timeout)
> > -        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
> >           self._logger = logger
> >           self._timeout = timeout
> >           self._app_args = app_args
> > +        self._init_channel()
> >           self._start_application(get_privileged_command)
> >
> > +    def _init_channel(self):
> > +        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
> >           """Starts a new interactive application based on the path to the app.
> >
> Hilariously I've made the same exact change in my testpmd params patch
> series!

I noticed this as well, it is very funny that we both thought to break
this into its own method. Must be a useful addition then!

> > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > index 3701c47408..41f6090a7e 100644
> > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
> >               ),
> >               privileged=True,
> >           )
> > -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > -        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.set_forward_mode(TestPmdForwardingModes.mac)
> > +            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}.",
> > +                )
> > +            testpmd.stop()
>
> Since we are exiting the context, implicitly it means we want to stop
> and close. Can't we also implicit call stop when closing?

It wouldn't hurt to also stop when we close I suppose. I really just
left close as something explicit because start is also something we do
more explicitly. When you quit testpmd in the close method it's going
to stop forwarding no matter what anyway, so I guess this isn't really
needed in the first place. Regardless, I can add something that stops
for you before quitting out of testpmd in the exit method, that will
at least save us some extra lines.

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

[Patrick Robb]

Reviewed-by: Patrick Robb <probb@iol.unh.edu>

I don't have any comments beyond Luca's suggestions, but saw the typo below.

On Tue, May 14, 2024 at 4:15 PM <jspewock@iol.unh.edu> wrote:
> +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> 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 it's close method. Note that

it's -> its

On Tue, May 14, 2024 at 4:15 PM <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 wrapper
> around the interactive shell 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.
>
> depends-on: patch-139227 ("dts: skip test cases based on capabilities")
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  .../critical_interactive_shell.py             | 98 +++++++++++++++++++
>  .../remote_session/interactive_shell.py       | 13 ++-
>  dts/framework/remote_session/testpmd_shell.py |  4 +-
>  dts/framework/testbed_model/sut_node.py       |  8 +-
>  dts/tests/TestSuite_pmd_buffer_scatter.py     | 28 +++---
>  dts/tests/TestSuite_smoke_tests.py            |  3 +-
>  6 files changed, 130 insertions(+), 24 deletions(-)
>  create mode 100644 dts/framework/remote_session/critical_interactive_shell.py
>
> diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
> new file mode 100644
> index 0000000000..d61b203954
> --- /dev/null
> +++ b/dts/framework/remote_session/critical_interactive_shell.py
> @@ -0,0 +1,98 @@
> +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
> +
> +Critical applications are defined as applications that require explicit clean-up before another
> +instance of some application can be started. In DPDK these are referred to as "primary
> +applications" and these applications take out a lock which stops other primary applications from
> +running. Much like :class:`~.interactive_shell.InteractiveShell`\s,
> +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
> +specific functionality and should never be instantiated directly.
> +"""
> +
> +from types import TracebackType
> +from typing import Callable, TypeVar
> +
> +from paramiko import SSHClient  # type: ignore[import]
> +
> +from framework.logger import DTSLogger
> +from framework.settings import SETTINGS
> +
> +from .interactive_shell import InteractiveShell
> +
> +CriticalInteractiveShellType = TypeVar(
> +    "CriticalInteractiveShellType", bound="CriticalInteractiveShell"
> +)
> +
> +
> +class CriticalInteractiveShell(InteractiveShell):
> +    """The base class for interactive critical applications.
> +
> +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
> +    implement the exact same functionality with the primary difference being how the application
> +    is started and stopped. In contrast to normal interactive shells, this class does not start the
> +    application upon initialization of the class. Instead, the application is handled through a
> +    context manager. This allows for more explicit starting and stopping of the application, and
> +    more guarantees for when the application is cleaned up which are not present with normal
> +    interactive shells that get cleaned up upon garbage collection.
> +    """
> +
> +    _get_priviledged_command: Callable[[str], str] | None
> +
> +    def __init__(
> +        self,
> +        interactive_session: SSHClient,
> +        logger: DTSLogger,
> +        get_privileged_command: Callable[[str], str] | None,
> +        app_args: str = "",
> +        timeout: float = SETTINGS.timeout,
> +    ) -> None:
> +        """Store parameters for creating an interactive shell, but do not start the application.
> +
> +        Note that this method also does not create the channel for the application, as this is
> +        something that isn't needed until the application starts.
> +
> +        Args:
> +            interactive_session: The SSH session dedicated to interactive shells.
> +            logger: The logger instance this session will use.
> +            get_privileged_command: A method for modifying a command to allow it to use
> +                elevated privileges. If :data:`None`, the application will not be started
> +                with elevated privileges.
> +            app_args: The command line arguments to be passed to the application on startup.
> +            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. The default
> +                value for this argument may be modified using the :option:`--timeout` command-line
> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> +        """
> +        self._interactive_session = interactive_session
> +        self._logger = logger
> +        self._timeout = timeout
> +        self._app_args = app_args
> +        self._get_priviledged_command = get_privileged_command
> +
> +    def __enter__(self: CriticalInteractiveShellType) -> CriticalInteractiveShellType:
> +        """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._init_channel()
> +        self._start_application(self._get_priviledged_command)
> +        return self
> +
> +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> 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 it's 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.
> +
> +        Args:
> +            type: Type of exception that was thrown in the context block if there was one.
> +            value: Value of the exception thrown in the context block if there was one.
> +            traceback: Traceback of the exception thrown in the context block if there was one.
> +        """
> +        self.close()
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index d1a9d8a6d2..08b8ba6a3e 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -89,16 +89,19 @@ def __init__(
>                  and no output is gathered within the timeout, an exception is thrown.
>          """
>          self._interactive_session = interactive_session
> -        self._ssh_channel = self._interactive_session.invoke_shell()
> -        self._stdin = self._ssh_channel.makefile_stdin("w")
> -        self._stdout = self._ssh_channel.makefile("r")
> -        self._ssh_channel.settimeout(timeout)
> -        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
>          self._logger = logger
>          self._timeout = timeout
>          self._app_args = app_args
> +        self._init_channel()
>          self._start_application(get_privileged_command)
>
> +    def _init_channel(self):
> +        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
>          """Starts a new interactive application based on the path to the app.
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index cb4642bf3d..33b3e7c5a3 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -26,7 +26,7 @@
>  from framework.settings import SETTINGS
>  from framework.utils import StrEnum
>
> -from .interactive_shell import InteractiveShell
> +from .critical_interactive_shell import CriticalInteractiveShell
>
>
>  class TestPmdDevice(object):
> @@ -82,7 +82,7 @@ class TestPmdForwardingModes(StrEnum):
>      recycle_mbufs = auto()
>
>
> -class TestPmdShell(InteractiveShell):
> +class TestPmdShell(CriticalInteractiveShell):
>      """Testpmd interactive shell.
>
>      The testpmd shell users should never use
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 1fb536735d..7dd39fd735 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -243,10 +243,10 @@ def get_supported_capabilities(
>          unsupported_capas: set[NicCapability] = set()
>          self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
>          testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
> -        for capability in capabilities:
> -            if capability not in supported_capas or capability not in unsupported_capas:
> -                capability.value(testpmd_shell, supported_capas, unsupported_capas)
> -        del testpmd_shell
> +        with testpmd_shell as running_testpmd:
> +            for capability in capabilities:
> +                if capability not in supported_capas or capability not in unsupported_capas:
> +                    capability.value(running_testpmd, supported_capas, unsupported_capas)
>          return supported_capas
>
>      def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 3701c47408..41f6090a7e 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
>          Test:
>              Start testpmd and run functional test with preset mbsize.
>          """
> -        testpmd = self.sut_node.create_interactive_shell(
> +        testpmd_shell = self.sut_node.create_interactive_shell(
>              TestPmdShell,
>              app_parameters=(
>                  "--mbcache=200 "
> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
>              ),
>              privileged=True,
>          )
> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> -        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.set_forward_mode(TestPmdForwardingModes.mac)
> +            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}.",
> +                )
> +            testpmd.stop()
>
>      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 a553e89662..360e64eb5a 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 = self.sut_node.create_interactive_shell(TestPmdShell, privileged=True)
> -        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.44.0
>

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

[Jeremy Spewock]

On Wed, May 22, 2024 at 9:53 AM Patrick Robb <probb@iol.unh.edu> wrote:
>
> Reviewed-by: Patrick Robb <probb@iol.unh.edu>
>
> I don't have any comments beyond Luca's suggestions, but saw the typo below.
>
> On Tue, May 14, 2024 at 4:15 PM <jspewock@iol.unh.edu> wrote:
> > +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> 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 it's close method. Note that
>
> it's -> its
>
Ahh, this typo plagues me all over the place, I always add the
apostrophe without thinking. Good catch.

[PATCH v1 3/4] dts: add methods for modifying MTU to testpmd shell

[jspewock]

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

There are methods within DTS currently that support updating the MTU of
ports on a node, but the methods for doing this in a linux session rely
on the ip command and the port being bound to the kernel driver. Since
test suites are run while bound to the driver for DPDK, there needs to
be a way to modify the value while bound to said driver as well. This is
done by using testpmd to modify the MTU.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 33b3e7c5a3..4e608998f9 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -227,6 +227,74 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def _stop_port(self, port_id: int, verify: bool = True) -> None:
+        """Stop port `port_id` in testpmd.
+
+        Depending on the PMD, the port may need to be stopped before configuration can take place.
+        This method wraps the command needed to properly stop ports and take their link down.
+
+        Args:
+            port_id: ID of the port to take down.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                stopping of ports was successful. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:'True` and the port did not
+                successfully stop.
+        """
+        stop_port_output = self.send_command(f"port stop {port_id}")
+        if verify and ("Done" not in stop_port_output):
+            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
+
+    def _start_port(self, port_id: int, verify: bool = True) -> None:
+        """Start port `port_id` in testpmd.
+
+        Because the port may need to be stopped to make some configuration changes, it naturally
+        follows that it will need to be started again once those changes have been made.
+
+        Args:
+            port_id: ID of the port to start.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                port came back up without error. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
+                back up.
+        """
+        start_port_output = self.send_command(f"port start {port_id}")
+        if verify and ("Done" not in start_port_output):
+            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
+
+    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
+        """Change the MTU of a port using testpmd.
+
+        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
+        stop the port before configuring in cases where it isn't required, so we first stop ports,
+        then update the MTU, then start the ports again afterwards.
+
+        Args:
+            port_id: ID of the port to adjust the MTU on.
+            mtu: Desired value for the MTU to be set to.
+            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
+                verify that the mtu was properly set on the port. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
+                properly updated on the port matching `port_id`.
+        """
+        self._stop_port(port_id, verify)
+        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}")
+        self._start_port(port_id, verify)
+        if verify and (f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}")):
+            self._logger.debug(
+                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
+            )
+            raise InteractiveCommandExecutionError(
+                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
+            )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-- 
2.44.0

Re: [PATCH v1 3/4] dts: add methods for modifying MTU to testpmd shell

[Luca Vizzarro]

On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 33b3e7c5a3..4e608998f9 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -227,6 +227,74 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>                   f"Test pmd failed to set fwd mode to {mode.value}"
>               )
>   
> +    def _stop_port(self, port_id: int, verify: bool = True) -> None:
> +        """Stop port `port_id` in testpmd.
> +
> +        Depending on the PMD, the port may need to be stopped before configuration can take place.
> +        This method wraps the command needed to properly stop ports and take their link down.
> +
> +        Args:
> +            port_id: ID of the port to take down.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                stopping of ports was successful. Defaults to True.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:'True` and the port did not
just a nit: apostrophe used instead of backtick 'True`

Re: [PATCH v1 3/4] dts: add methods for modifying MTU to testpmd shell

[Jeremy Spewock]

On Mon, May 20, 2024 at 1:35 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> > index 33b3e7c5a3..4e608998f9 100644
> > --- a/dts/framework/remote_session/testpmd_shell.py
> > +++ b/dts/framework/remote_session/testpmd_shell.py
> > @@ -227,6 +227,74 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
> >                   f"Test pmd failed to set fwd mode to {mode.value}"
> >               )
> >
> > +    def _stop_port(self, port_id: int, verify: bool = True) -> None:
> > +        """Stop port `port_id` in testpmd.
> > +
> > +        Depending on the PMD, the port may need to be stopped before configuration can take place.
> > +        This method wraps the command needed to properly stop ports and take their link down.
> > +
> > +        Args:
> > +            port_id: ID of the port to take down.
> > +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> > +                stopping of ports was successful. Defaults to True.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:'True` and the port did not
> just a nit: apostrophe used instead of backtick 'True`

Good catch, I think this does matter for doc generation, so definitely
good to fix.

>
>

Re: [PATCH v1 3/4] dts: add methods for modifying MTU to testpmd shell

[Patrick Robb]

Reviewed-by: Patrick Robb <probb@iol.unh.edu>

[PATCH v1 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[jspewock]

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

Some NICs tested in DPDK allow for the scattering of packets without an
offload and others enforce that you enable the scattered_rx offload in
testpmd. The current version of the suite for testing support of
scattering packets only tests the case where the NIC supports testing
without the offload, so an expansion of coverage is needed to cover the
second case as well.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 49 ++++++++++++++++++-----
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 41f6090a7e..6d04663c8a 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -16,14 +16,19 @@
 """
 
 import struct
+from typing import ClassVar
 
 from scapy.layers.inet import IP  # type: ignore[import]
 from scapy.layers.l2 import Ether  # type: ignore[import]
 from scapy.packet import Raw  # type: ignore[import]
 from scapy.utils import hexstr  # type: ignore[import]
 
-from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
-from framework.test_suite import TestSuite
+from framework.remote_session.testpmd_shell import (
+    NicCapability,
+    TestPmdForwardingModes,
+    TestPmdShell,
+)
+from framework.test_suite import TestSuite, requires
 
 
 class TestPmdBufferScatter(TestSuite):
@@ -48,6 +53,14 @@ class TestPmdBufferScatter(TestSuite):
        and a single byte of packet data stored in a second buffer alongside the CRC.
     """
 
+    #: Parameters for testing scatter using testpmd which are universal across all test cases.
+    base_testpmd_parameters: ClassVar[list[str]] = [
+        "--mbcache=200",
+        "--max-pkt-len=9000",
+        "--port-topology=paired",
+        "--tx-offloads=0x00008000",
+    ]
+
     def set_up_suite(self) -> None:
         """Set up the test suite.
 
@@ -91,7 +104,7 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
 
         return load
 
-    def pmd_scatter(self, mbsize: int) -> None:
+    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
         """Testpmd support of receiving and sending scattered multi-segment packets.
 
         Support for scattered packets is shown by sending 5 packets of differing length
@@ -103,17 +116,14 @@ def pmd_scatter(self, mbsize: int) -> None:
         """
         testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=(
-                "--mbcache=200 "
-                f"--mbuf-size={mbsize} "
-                "--max-pkt-len=9000 "
-                "--port-topology=paired "
-                "--tx-offloads=0x00008000"
-            ),
+            app_parameters=" ".join(testpmd_params),
             privileged=True,
         )
         with testpmd_shell as testpmd:
             testpmd.set_forward_mode(TestPmdForwardingModes.mac)
+            # adjust the MTU of the SUT ports
+            testpmd.set_port_mtu(0, 9000)
+            testpmd.set_port_mtu(1, 9000)
             testpmd.start()
 
             for offset in [-1, 0, 1, 4, 5]:
@@ -127,10 +137,27 @@ def pmd_scatter(self, mbsize: int) -> None:
                     f"{offset}.",
                 )
             testpmd.stop()
+            # reset the MTU of the SUT ports
+            testpmd.set_port_mtu(0, 1500)
+            testpmd.set_port_mtu(1, 1500)
 
+    @requires(NicCapability.scattered_rx)
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
-        self.pmd_scatter(mbsize=2048)
+        self.pmd_scatter(
+            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
+        )
+
+    def test_scatter_mbuf_2048_with_offload(self) -> None:
+        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
+        self.pmd_scatter(
+            mbsize=2048,
+            testpmd_params=[
+                *(self.base_testpmd_parameters),
+                "--mbuf-size=2048",
+                "--enable-scatter",
+            ],
+        )
 
     def tear_down_suite(self) -> None:
         """Tear down the test suite.
-- 
2.44.0

Re: [PATCH v1 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Luca Vizzarro]

On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> +            # adjust the MTU of the SUT ports
> +            testpmd.set_port_mtu(0, 9000)
> +            testpmd.set_port_mtu(1, 9000)

should you perhaps do this for every port in the testpmd shell instead?

   for port_id in range(testpmd.number_of_ports):
       testpmd.set_port_mtu(port_id, 9000)

>               testpmd.stop()
> +            # reset the MTU of the SUT ports
> +            testpmd.set_port_mtu(0, 1500)
> +            testpmd.set_port_mtu(1, 1500)

As above

Re: [PATCH v1 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Jeremy Spewock]

On Mon, May 20, 2024 at 1:56 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> > +            # adjust the MTU of the SUT ports
> > +            testpmd.set_port_mtu(0, 9000)
> > +            testpmd.set_port_mtu(1, 9000)
>
> should you perhaps do this for every port in the testpmd shell instead?
>
>    for port_id in range(testpmd.number_of_ports):
>        testpmd.set_port_mtu(port_id, 9000)

This is a good thought. I was sort of falling back on the current
assumption that we can only support 2 ports per server in test suites,
but there is nothing that ensures these ports are the first and second
in trestpmd. I think it is probably safer to just modify the MTU of
all ports that testpmd knows of since we just blindly pick the ingress
and egress ports we want to use. I didn't want to at first since the
ideal would be that we have minimal side effects in testing suites so
we wouldn't want to just change the MTU of all ports, but if there
comes a time when this matters in the future (the only one I could
really think of is implementing parallel testing) then we can just
limit what ports are in the scope of testpmd.

>
> >               testpmd.stop()
> > +            # reset the MTU of the SUT ports
> > +            testpmd.set_port_mtu(0, 1500)
> > +            testpmd.set_port_mtu(1, 1500)
>
> As above
>

Re: [PATCH v1 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Luca Vizzarro]

On 29/05/2024 21:40, Jeremy Spewock wrote:
> This is a good thought. I was sort of falling back on the current
> assumption that we can only support 2 ports per server in test suites,
> but there is nothing that ensures these ports are the first and second
> in trestpmd. I think it is probably safer to just modify the MTU of
> all ports that testpmd knows of since we just blindly pick the ingress
> and egress ports we want to use. I didn't want to at first since the
> ideal would be that we have minimal side effects in testing suites so
> we wouldn't want to just change the MTU of all ports, but if there
> comes a time when this matters in the future (the only one I could
> really think of is implementing parallel testing) then we can just
> limit what ports are in the scope of testpmd.

Well, I had originally proposed to add test-specific port filters, so 
that a test can say what it wants (and the shells/testpmd could 
integrate some test awareness). I think it was said that we want to 
introduce test configuration that could deal with this.

With my shell interaction patch this could work nicely too, so instead 
of doing:
   TestPmdShell(self.sut_node)
We could implement a Protocol in the TestSuite to ensure that 
self.sut_node is always present for type checking, and pass self:
   TestPmdShell(self)
Similary the core, port filters etc could also be part of the class per
type checking. And the underlying shell could just deal with the 
boilerplate code.

This would also spare us from having to manually write lengthy filters 
in the test suite code.

Per-test-case configurations could also be initialised easily at the 
class-level through a @test decorator as proposed in Juraj's RFC. This 
could potentially be done at class initialisation while leaving the 
current API untouched, although it won't work well with other decorators 
such as @requires.

Just food for thought.

[PATCH v2 0/4] Add second scatter test case

[jspewock]

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

This version addresses comments from the last which featured new
improvements such as the usage of the Self typehint in the critical
interactive shell, modifying the MTU of all ports within the scope of
testpmd rather than making a guess as to which should be changed, and
some fixing of typos.

This version also adds a better prompt to await when closing testpmd
rather than just breaking out of the scan at the first line. 

Jeremy Spewock (4):
  dts: improve starting and stopping interactive shells
  dts: add context manager for interactive shells
  dts: add methods for modifying MTU to testpmd shell
  dts: add test case that utilizes offload to pmd_buffer_scatter

 .../critical_interactive_shell.py             | 93 +++++++++++++++++++
 .../remote_session/interactive_shell.py       | 64 ++++++++++---
 dts/framework/remote_session/testpmd_shell.py | 87 ++++++++++++++++-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 79 +++++++++++-----
 dts/tests/TestSuite_smoke_tests.py            |  3 +-
 6 files changed, 287 insertions(+), 47 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

-- 
2.45.1

[PATCH v2 1/4] 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>
---
 .../remote_session/interactive_shell.py       | 51 ++++++++++++++++---
 dts/framework/remote_session/testpmd_shell.py |  6 +--
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..d1a9d8a6d2 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -14,12 +14,14 @@
 environment variable configure the timeout of getting the output from command execution.
 """
 
+import weakref
 from abc import ABC
 from pathlib import PurePath
 from typing import Callable, ClassVar
 
 from paramiko import Channel, SSHClient, channel  # type: ignore[import]
 
+from framework.exception import InteractiveCommandExecutionError
 from framework.logger import DTSLogger
 from framework.settings import SETTINGS
 
@@ -32,6 +34,10 @@ class InteractiveShell(ABC):
     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.
+
+    Attributes:
+        is_started: :data:`True` if the application has started successfully, :data:`False`
+            otherwise.
     """
 
     _interactive_session: SSHClient
@@ -41,6 +47,7 @@ class InteractiveShell(ABC):
     _logger: DTSLogger
     _timeout: float
     _app_args: str
+    _finalizer: weakref.finalize
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -58,6 +65,8 @@ class InteractiveShell(ABC):
     #: for DPDK on the node will be prepended to the path to the executable.
     dpdk_app: ClassVar[bool] = False
 
+    is_started: bool = False
+
     def __init__(
         self,
         interactive_session: SSHClient,
@@ -93,17 +102,39 @@ def __init__(
     def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
+        done because some DPDK applications need slightly more time after exiting their script to
+        clean up EAL before others can start.
+
+        When the application is started we also bind a class for finalization to this instance of
+        the shell to ensure proper cleanup of the application.
 
         Args:
             get_privileged_command: A function (but could be any callable) that produces
                 the version of the command with elevated privileges.
         """
+        self._finalizer = weakref.finalize(self, self._close)
+        max_retries = 5
+        self._ssh_channel.settimeout(1)
         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_started = True
+        for retry in range(max_retries):
+            try:
+                self.send_command(start_command)
+                break
+            except TimeoutError:
+                self._logger.info(
+                    "Interactive shell failed to start, retrying... "
+                    f"({retry+1} out of {max_retries})"
+                )
+        else:
+            self._ssh_channel.settimeout(self._timeout)
+            self.is_started = 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.
@@ -125,6 +156,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_started:
+            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
@@ -140,11 +175,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._logger.debug(f"Got output: {out}")
         return out
 
-    def close(self) -> None:
-        """Properly free all resources."""
+    def _close(self) -> None:
+        self.is_started = False
         self._stdin.close()
         self._ssh_channel.close()
 
-    def __del__(self) -> None:
-        """Make sure the session is properly closed before deleting the object."""
-        self.close()
+    def close(self) -> None:
+        """Properly free all resources."""
+        self._finalizer()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index f6783af621..284412e82c 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -227,10 +227,10 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
-    def close(self) -> None:
+    def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
-        self.send_command("quit", "")
-        return super().close()
+        self.send_command("quit", "Bye...")
+        return super()._close()
 
     def get_capas_rxq(
         self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
-- 
2.45.1

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

[Luca Vizzarro]

On 30/05/2024 17:33, jspewock@iol.unh.edu wrote:
> @@ -93,17 +102,39 @@ def __init__(
>       def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
> +        done because some DPDK applications need slightly more time after exiting their script to
> +        clean up EAL before others can start.
> +
> +        When the application is started we also bind a class for finalization to this instance of
> +        the shell to ensure proper cleanup of the application.
>   
>           Args:
>               get_privileged_command: A function (but could be any callable) that produces
>                   the version of the command with elevated privileges.
>           """
> +        self._finalizer = weakref.finalize(self, self._close)
> +        max_retries = 5
> +        self._ssh_channel.settimeout(1)

This timeout being too short is causing the testpmd shell (which is 
still loading in my case) to be spammed with testpmd instantiation 
commands. Unfortunately causing the next commands to fail too, as the 
testpmd shell has received a lot of garbage.

5 seconds of timeout seemed to have worked just fine in my case.

>           start_command = f"{self.path} {self._app_args}"
>           if get_privileged_command is not None:
>               start_command = get_privileged_command(start_command)

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

[Jeremy Spewock]

On Fri, May 31, 2024 at 12:37 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 30/05/2024 17:33, jspewock@iol.unh.edu wrote:
> > @@ -93,17 +102,39 @@ def __init__(
> >       def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
> > +        done because some DPDK applications need slightly more time after exiting their script to
> > +        clean up EAL before others can start.
> > +
> > +        When the application is started we also bind a class for finalization to this instance of
> > +        the shell to ensure proper cleanup of the application.
> >
> >           Args:
> >               get_privileged_command: A function (but could be any callable) that produces
> >                   the version of the command with elevated privileges.
> >           """
> > +        self._finalizer = weakref.finalize(self, self._close)
> > +        max_retries = 5
> > +        self._ssh_channel.settimeout(1)
>
> This timeout being too short is causing the testpmd shell (which is
> still loading in my case) to be spammed with testpmd instantiation
> commands. Unfortunately causing the next commands to fail too, as the
> testpmd shell has received a lot of garbage.
>
> 5 seconds of timeout seemed to have worked just fine in my case.

Ack.
It's hard to gauge exactly how long this timeout should be since it
will be different between every system (and between different NICs on
the same system) but I think 5 seconds should be a reasonable amount
of time for testpmd to start in most cases. The only blanket way I
could see to fix this would be to just clean out the buffer before
sending every command so that the previous command outputting harmless
clutter in the buffer doesn't cause every future command sent into
that shell to also break. This idea would likely slow things down
however and doesn't fit the scope of this patch regardless.


>
> >           start_command = f"{self.path} {self._app_args}"
> >           if get_privileged_command is not None:
> >               start_command = get_privileged_command(start_command)
>

[PATCH v2 2/4] 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 wrapper
around the interactive shell 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.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 .../critical_interactive_shell.py             | 93 +++++++++++++++++++
 .../remote_session/interactive_shell.py       | 13 ++-
 dts/framework/remote_session/testpmd_shell.py | 13 ++-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 28 +++---
 dts/tests/TestSuite_smoke_tests.py            |  3 +-
 6 files changed, 134 insertions(+), 24 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
new file mode 100644
index 0000000000..26bd891267
--- /dev/null
+++ b/dts/framework/remote_session/critical_interactive_shell.py
@@ -0,0 +1,93 @@
+r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
+
+Critical applications are defined as applications that require explicit clean-up before another
+instance of some application can be started. In DPDK these are referred to as "primary
+applications" and these applications take out a lock which stops other primary applications from
+running. Much like :class:`~.interactive_shell.InteractiveShell`\s,
+:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
+specific functionality and should never be instantiated directly.
+"""
+
+from typing import Callable
+
+from paramiko import SSHClient  # type: ignore[import]
+from typing_extensions import Self
+
+from framework.logger import DTSLogger
+from framework.settings import SETTINGS
+
+from .interactive_shell import InteractiveShell
+
+
+class CriticalInteractiveShell(InteractiveShell):
+    """The base class for interactive critical applications.
+
+    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
+    implement the exact same functionality with the primary difference being how the application
+    is started and stopped. In contrast to normal interactive shells, this class does not start the
+    application upon initialization of the class. Instead, the application is handled through a
+    context manager. This allows for more explicit starting and stopping of the application, and
+    more guarantees for when the application is cleaned up which are not present with normal
+    interactive shells that get cleaned up upon garbage collection.
+    """
+
+    _get_privileged_command: Callable[[str], str] | None
+
+    def __init__(
+        self,
+        interactive_session: SSHClient,
+        logger: DTSLogger,
+        get_privileged_command: Callable[[str], str] | None,
+        app_args: str = "",
+        timeout: float = SETTINGS.timeout,
+    ) -> None:
+        """Store parameters for creating an interactive shell, but do not start the application.
+
+        Note that this method also does not create the channel for the application, as this is
+        something that isn't needed until the application starts.
+
+        Args:
+            interactive_session: The SSH session dedicated to interactive shells.
+            logger: The logger instance this session will use.
+            get_privileged_command: A method for modifying a command to allow it to use
+                elevated privileges. If :data:`None`, the application will not be started
+                with elevated privileges.
+            app_args: The command line arguments to be passed to the application on startup.
+            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. The default
+                value for this argument may be modified using the :option:`--timeout` command-line
+                argument or the :envvar:`DTS_TIMEOUT` environment variable.
+        """
+        self._interactive_session = interactive_session
+        self._logger = logger
+        self._timeout = timeout
+        self._app_args = app_args
+        self._get_privileged_command = get_privileged_command
+
+    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._init_channel()
+        self._start_application(self._get_privileged_command)
+        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/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index d1a9d8a6d2..08b8ba6a3e 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -89,16 +89,19 @@ def __init__(
                 and no output is gathered within the timeout, an exception is thrown.
         """
         self._interactive_session = interactive_session
-        self._ssh_channel = self._interactive_session.invoke_shell()
-        self._stdin = self._ssh_channel.makefile_stdin("w")
-        self._stdout = self._ssh_channel.makefile("r")
-        self._ssh_channel.settimeout(timeout)
-        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
         self._logger = logger
         self._timeout = timeout
         self._app_args = app_args
+        self._init_channel()
         self._start_application(get_privileged_command)
 
+    def _init_channel(self):
+        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
         """Starts a new interactive application based on the path to the app.
 
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 284412e82c..ca30aac264 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -26,7 +26,7 @@
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
-from .interactive_shell import InteractiveShell
+from .critical_interactive_shell import CriticalInteractiveShell
 
 
 class TestPmdDevice(object):
@@ -82,7 +82,7 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
-class TestPmdShell(InteractiveShell):
+class TestPmdShell(CriticalInteractiveShell):
     """Testpmd interactive shell.
 
     The testpmd shell users should never use
@@ -253,6 +253,15 @@ def get_capas_rxq(
                 else:
                     unsupported_capabilities.add(NicCapability.scattered_rx)
 
+    def __exit__(self, *_) -> None:
+        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
+
+        Ensures that when the context is exited packet forwarding is stopped before closing the
+        application.
+        """
+        self.stop()
+        super().__exit__()
+
 
 class NicCapability(Enum):
     """A mapping between capability names and the associated :class:`TestPmdShell` methods.
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 1fb536735d..7dd39fd735 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -243,10 +243,10 @@ def get_supported_capabilities(
         unsupported_capas: set[NicCapability] = set()
         self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
         testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
-        for capability in capabilities:
-            if capability not in supported_capas or capability not in unsupported_capas:
-                capability.value(testpmd_shell, supported_capas, unsupported_capas)
-        del testpmd_shell
+        with testpmd_shell as running_testpmd:
+            for capability in capabilities:
+                if capability not in supported_capas or capability not in unsupported_capas:
+                    capability.value(running_testpmd, supported_capas, unsupported_capas)
         return supported_capas
 
     def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 3701c47408..41f6090a7e 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = self.sut_node.create_interactive_shell(
+        testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
             app_parameters=(
                 "--mbcache=200 "
@@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
             ),
             privileged=True,
         )
-        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
-        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.set_forward_mode(TestPmdForwardingModes.mac)
+            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}.",
+                )
+            testpmd.stop()
 
     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 a553e89662..360e64eb5a 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 = self.sut_node.create_interactive_shell(TestPmdShell, privileged=True)
-        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.1

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

[Luca Vizzarro]

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

[PATCH v2 3/4] dts: add methods for modifying MTU to testpmd shell

[jspewock]

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

There are methods within DTS currently that support updating the MTU of
ports on a node, but the methods for doing this in a linux session rely
on the ip command and the port being bound to the kernel driver. Since
test suites are run while bound to the driver for DPDK, there needs to
be a way to modify the value while bound to said driver as well. This is
done by using testpmd to modify the MTU.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index ca30aac264..3f425154dd 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -227,6 +227,74 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def _stop_port(self, port_id: int, verify: bool = True) -> None:
+        """Stop port `port_id` in testpmd.
+
+        Depending on the PMD, the port may need to be stopped before configuration can take place.
+        This method wraps the command needed to properly stop ports and take their link down.
+
+        Args:
+            port_id: ID of the port to take down.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                stopping of ports was successful. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
+                successfully stop.
+        """
+        stop_port_output = self.send_command(f"port stop {port_id}")
+        if verify and ("Done" not in stop_port_output):
+            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
+
+    def _start_port(self, port_id: int, verify: bool = True) -> None:
+        """Start port `port_id` in testpmd.
+
+        Because the port may need to be stopped to make some configuration changes, it naturally
+        follows that it will need to be started again once those changes have been made.
+
+        Args:
+            port_id: ID of the port to start.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                port came back up without error. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
+                back up.
+        """
+        start_port_output = self.send_command(f"port start {port_id}")
+        if verify and ("Done" not in start_port_output):
+            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
+
+    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
+        """Change the MTU of a port using testpmd.
+
+        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
+        stop the port before configuring in cases where it isn't required, so we first stop ports,
+        then update the MTU, then start the ports again afterwards.
+
+        Args:
+            port_id: ID of the port to adjust the MTU on.
+            mtu: Desired value for the MTU to be set to.
+            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
+                verify that the mtu was properly set on the port. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
+                properly updated on the port matching `port_id`.
+        """
+        self._stop_port(port_id, verify)
+        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}")
+        self._start_port(port_id, verify)
+        if verify and (f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}")):
+            self._logger.debug(
+                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
+            )
+            raise InteractiveCommandExecutionError(
+                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
+            )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "Bye...")
-- 
2.45.1

Re: [PATCH v2 3/4] dts: add methods for modifying MTU to testpmd shell

[Luca Vizzarro]

Due to the nature of this patch the console is spammed with a lot of 
commands. Would it be better to log these in debug and instead log:
   Setting port X to MTU XXXX
as INFO?

Re: [PATCH v2 3/4] dts: add methods for modifying MTU to testpmd shell

[Jeremy Spewock]

On Fri, May 31, 2024 at 12:34 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Due to the nature of this patch the console is spammed with a lot of
> commands. Would it be better to log these in debug and instead log:
>    Setting port X to MTU XXXX
> as INFO?

Potentially, but this would require a global change for how logging
works when sending commands to interactive shells in general. They are
each logged as their own message since they are each individual
commands being sent into the shell which (in general) we do want to
log. I could maybe add an optional flag to the send command function
that logs its output to debug rather than info however which might be
a nicer solution. I agree that it does get cluttered.

Re: [PATCH v2 3/4] dts: add methods for modifying MTU to testpmd shell

[Juraj Linkeš]

On 31. 5. 2024 23:08, Jeremy Spewock wrote:
> On Fri, May 31, 2024 at 12:34 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>>
>> Due to the nature of this patch the console is spammed with a lot of
>> commands. Would it be better to log these in debug and instead log:
>>     Setting port X to MTU XXXX
>> as INFO?
> 
> Potentially, but this would require a global change for how logging
> works when sending commands to interactive shells in general. They are
> each logged as their own message since they are each individual
> commands being sent into the shell which (in general) we do want to
> log. I could maybe add an optional flag to the send command function
> that logs its output to debug rather than info however which might be
> a nicer solution. I agree that it does get cluttered.

I think this would be suitable as a separate change; we should make a 
bugzilla ticket. It's likely better to move these to debug and let 
individual commands log as info the most important part if needed 
(either using a switch or a separate log call).

[PATCH v2 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[jspewock]

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

Some NICs tested in DPDK allow for the scattering of packets without an
offload and others enforce that you enable the scattered_rx offload in
testpmd. The current version of the suite for testing support of
scattering packets only tests the case where the NIC supports testing
without the offload, so an expansion of coverage is needed to cover the
second case as well.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 49 ++++++++++++++++++-----
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 41f6090a7e..1ceaab0f86 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -16,14 +16,19 @@
 """
 
 import struct
+from typing import ClassVar
 
 from scapy.layers.inet import IP  # type: ignore[import]
 from scapy.layers.l2 import Ether  # type: ignore[import]
 from scapy.packet import Raw  # type: ignore[import]
 from scapy.utils import hexstr  # type: ignore[import]
 
-from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
-from framework.test_suite import TestSuite
+from framework.remote_session.testpmd_shell import (
+    NicCapability,
+    TestPmdForwardingModes,
+    TestPmdShell,
+)
+from framework.test_suite import TestSuite, requires
 
 
 class TestPmdBufferScatter(TestSuite):
@@ -48,6 +53,14 @@ class TestPmdBufferScatter(TestSuite):
        and a single byte of packet data stored in a second buffer alongside the CRC.
     """
 
+    #: Parameters for testing scatter using testpmd which are universal across all test cases.
+    base_testpmd_parameters: ClassVar[list[str]] = [
+        "--mbcache=200",
+        "--max-pkt-len=9000",
+        "--port-topology=paired",
+        "--tx-offloads=0x00008000",
+    ]
+
     def set_up_suite(self) -> None:
         """Set up the test suite.
 
@@ -91,7 +104,7 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
 
         return load
 
-    def pmd_scatter(self, mbsize: int) -> None:
+    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
         """Testpmd support of receiving and sending scattered multi-segment packets.
 
         Support for scattered packets is shown by sending 5 packets of differing length
@@ -103,17 +116,14 @@ def pmd_scatter(self, mbsize: int) -> None:
         """
         testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=(
-                "--mbcache=200 "
-                f"--mbuf-size={mbsize} "
-                "--max-pkt-len=9000 "
-                "--port-topology=paired "
-                "--tx-offloads=0x00008000"
-            ),
+            app_parameters=" ".join(testpmd_params),
             privileged=True,
         )
         with testpmd_shell as testpmd:
             testpmd.set_forward_mode(TestPmdForwardingModes.mac)
+            # adjust the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 9000)
             testpmd.start()
 
             for offset in [-1, 0, 1, 4, 5]:
@@ -127,10 +137,27 @@ def pmd_scatter(self, mbsize: int) -> None:
                     f"{offset}.",
                 )
             testpmd.stop()
+            # reset the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 1500)
 
+    @requires(NicCapability.scattered_rx)
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
-        self.pmd_scatter(mbsize=2048)
+        self.pmd_scatter(
+            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
+        )
+
+    def test_scatter_mbuf_2048_with_offload(self) -> None:
+        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
+        self.pmd_scatter(
+            mbsize=2048,
+            testpmd_params=[
+                *(self.base_testpmd_parameters),
+                "--mbuf-size=2048",
+                "--enable-scatter",
+            ],
+        )
 
     def tear_down_suite(self) -> None:
         """Tear down the test suite.
-- 
2.45.1

Re: [PATCH v2 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Luca Vizzarro]

While testing this patch against the Intel NICs we have, I've detected 
that upon port start and stopping two ICMPv6 packets are sent out. This 
has caused these packets to appear in the first capture, causing it to 
intermittently fail if they were the first packets to arrive or not. 
Sometimes the ICMPv6 packets would be the only ones to show in the 
captured packets. This problem is not strictly related to this patch but 
could be fixed if not too annoying.

So far what appears to have fixed the issue on my side, was just add 
some wait between port setup and the sending of the packets.
time.sleep(2) seems to have done the job. But it may not be an ideal 
solution.

Re: [PATCH v2 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Jeremy Spewock]

On Fri, May 31, 2024 at 12:33 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> While testing this patch against the Intel NICs we have, I've detected
> that upon port start and stopping two ICMPv6 packets are sent out. This
> has caused these packets to appear in the first capture, causing it to
> intermittently fail if they were the first packets to arrive or not.
> Sometimes the ICMPv6 packets would be the only ones to show in the
> captured packets. This problem is not strictly related to this patch but
> could be fixed if not too annoying.
>
> So far what appears to have fixed the issue on my side, was just add
> some wait between port setup and the sending of the packets.
> time.sleep(2) seems to have done the job. But it may not be an ideal
> solution.

Ack. Maybe something I could do here is update the verification step
to instead find any packet in the list that passed the test rather
than assuming there won't be interference.

[PATCH v3 0/4] Add second scatter test case

[jspewock]

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

v3:
 * increasse timeout before retrying starting interactive shells
 * added the ability to send logging messages from sending commands to
   interactive shells at the debug level rather than always being at the
   info level
 * changed the verification step for the scatter suite so that it
   filters the list to relevant packets before checking if any were
   received and then looking through all packets in the list for one
   that meets the criteria

Jeremy Spewock (4):
  dts: improve starting and stopping interactive shells
  dts: add context manager for interactive shells
  dts: add methods for modifying MTU to testpmd shell
  dts: add test case that utilizes offload to pmd_buffer_scatter

 .../critical_interactive_shell.py             |  93 ++++++++++++++++
 .../remote_session/interactive_shell.py       |  80 +++++++++++---
 dts/framework/remote_session/testpmd_shell.py |  95 ++++++++++++++--
 dts/framework/testbed_model/sut_node.py       |   8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 101 ++++++++++++------
 dts/tests/TestSuite_smoke_tests.py            |   3 +-
 6 files changed, 321 insertions(+), 59 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

-- 
2.45.1

[PATCH v3 1/4] 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>
---
 .../remote_session/interactive_shell.py       | 51 ++++++++++++++++---
 dts/framework/remote_session/testpmd_shell.py |  6 +--
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..921c73d9df 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -14,12 +14,14 @@
 environment variable configure the timeout of getting the output from command execution.
 """
 
+import weakref
 from abc import ABC
 from pathlib import PurePath
 from typing import Callable, ClassVar
 
 from paramiko import Channel, SSHClient, channel  # type: ignore[import]
 
+from framework.exception import InteractiveCommandExecutionError
 from framework.logger import DTSLogger
 from framework.settings import SETTINGS
 
@@ -32,6 +34,10 @@ class InteractiveShell(ABC):
     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.
+
+    Attributes:
+        is_started: :data:`True` if the application has started successfully, :data:`False`
+            otherwise.
     """
 
     _interactive_session: SSHClient
@@ -41,6 +47,7 @@ class InteractiveShell(ABC):
     _logger: DTSLogger
     _timeout: float
     _app_args: str
+    _finalizer: weakref.finalize
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -58,6 +65,8 @@ class InteractiveShell(ABC):
     #: for DPDK on the node will be prepended to the path to the executable.
     dpdk_app: ClassVar[bool] = False
 
+    is_started: bool = False
+
     def __init__(
         self,
         interactive_session: SSHClient,
@@ -93,17 +102,39 @@ def __init__(
     def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
+        done because some DPDK applications need slightly more time after exiting their script to
+        clean up EAL before others can start.
+
+        When the application is started we also bind a class for finalization to this instance of
+        the shell to ensure proper cleanup of the application.
 
         Args:
             get_privileged_command: A function (but could be any callable) that produces
                 the version of the command with elevated privileges.
         """
+        self._finalizer = weakref.finalize(self, self._close)
+        max_retries = 5
+        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_started = True
+        for retry in range(max_retries):
+            try:
+                self.send_command(start_command)
+                break
+            except TimeoutError:
+                self._logger.info(
+                    "Interactive shell failed to start, retrying... "
+                    f"({retry+1} out of {max_retries})"
+                )
+        else:
+            self._ssh_channel.settimeout(self._timeout)
+            self.is_started = 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.
@@ -125,6 +156,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_started:
+            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
@@ -140,11 +175,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._logger.debug(f"Got output: {out}")
         return out
 
-    def close(self) -> None:
-        """Properly free all resources."""
+    def _close(self) -> None:
+        self.is_started = False
         self._stdin.close()
         self._ssh_channel.close()
 
-    def __del__(self) -> None:
-        """Make sure the session is properly closed before deleting the object."""
-        self.close()
+    def close(self) -> None:
+        """Properly free all resources."""
+        self._finalizer()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index f6783af621..284412e82c 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -227,10 +227,10 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
-    def close(self) -> None:
+    def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
-        self.send_command("quit", "")
-        return super().close()
+        self.send_command("quit", "Bye...")
+        return super()._close()
 
     def get_capas_rxq(
         self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
-- 
2.45.1

Re: [PATCH v3 1/4] dts: improve starting and stopping interactive shells

[Juraj Linkeš]

> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 5cfe202e15..921c73d9df 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -32,6 +34,10 @@ class InteractiveShell(ABC):
>       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.
> +
> +    Attributes:
> +        is_started: :data:`True` if the application has started successfully, :data:`False`
> +            otherwise.
>       """
>   
>       _interactive_session: SSHClient
> @@ -41,6 +47,7 @@ class InteractiveShell(ABC):
>       _logger: DTSLogger
>       _timeout: float
>       _app_args: str
> +    _finalizer: weakref.finalize
>   
>       #: Prompt to expect at the end of output when sending a command.
>       #: This is often overridden by subclasses.
> @@ -58,6 +65,8 @@ class InteractiveShell(ABC):
>       #: for DPDK on the node will be prepended to the path to the executable.
>       dpdk_app: ClassVar[bool] = False
>   
> +    is_started: bool = False

A better name would be is_alive to unify it with SSHSession.

> +
>       def __init__(
>           self,
>           interactive_session: SSHClient,
> @@ -93,17 +102,39 @@ def __init__(
>       def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
> +        done because some DPDK applications need slightly more time after exiting their script to
> +        clean up EAL before others can start.
> +
> +        When the application is started we also bind a class for finalization to this instance of
> +        the shell to ensure proper cleanup of the application.

Let's also include the explanation from the commit message.

>   
>           Args:
>               get_privileged_command: A function (but could be any callable) that produces
>                   the version of the command with elevated privileges.
>           """
> +        self._finalizer = weakref.finalize(self, self._close)

This looks like exactly what we should do, but out of curiosity, do 
Paramiko docs mention how we should handle channel closing?

> +        max_retries = 5
> +        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_started = True
> +        for retry in range(max_retries):
> +            try:
> +                self.send_command(start_command)
> +                break
> +            except TimeoutError:
> +                self._logger.info(
> +                    "Interactive shell failed to start, retrying... "
> +                    f"({retry+1} out of {max_retries})"
> +                )
> +        else:
> +            self._ssh_channel.settimeout(self._timeout)
> +            self.is_started = 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.

Re: [PATCH v3 1/4] dts: improve starting and stopping interactive shells

[Jeremy Spewock]

On Mon, Jun 10, 2024 at 9:36 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index 5cfe202e15..921c73d9df 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -32,6 +34,10 @@ class InteractiveShell(ABC):
> >       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.
> > +
> > +    Attributes:
> > +        is_started: :data:`True` if the application has started successfully, :data:`False`
> > +            otherwise.
> >       """
> >
> >       _interactive_session: SSHClient
> > @@ -41,6 +47,7 @@ class InteractiveShell(ABC):
> >       _logger: DTSLogger
> >       _timeout: float
> >       _app_args: str
> > +    _finalizer: weakref.finalize
> >
> >       #: Prompt to expect at the end of output when sending a command.
> >       #: This is often overridden by subclasses.
> > @@ -58,6 +65,8 @@ class InteractiveShell(ABC):
> >       #: for DPDK on the node will be prepended to the path to the executable.
> >       dpdk_app: ClassVar[bool] = False
> >
> > +    is_started: bool = False
>
> A better name would be is_alive to unify it with SSHSession.

Ack.

>
> > +
> >       def __init__(
> >           self,
> >           interactive_session: SSHClient,
> > @@ -93,17 +102,39 @@ def __init__(
> >       def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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 5 times. This is
> > +        done because some DPDK applications need slightly more time after exiting their script to
> > +        clean up EAL before others can start.
> > +
> > +        When the application is started we also bind a class for finalization to this instance of
> > +        the shell to ensure proper cleanup of the application.
>
> Let's also include the explanation from the commit message.

Ack.

>
> >
> >           Args:
> >               get_privileged_command: A function (but could be any callable) that produces
> >                   the version of the command with elevated privileges.
> >           """
> > +        self._finalizer = weakref.finalize(self, self._close)
>
> This looks like exactly what we should do, but out of curiosity, do
> Paramiko docs mention how we should handle channel closing?

They don't say much about how to properly handle closing them. They do
mention though that the channels are automatically closed when their
transport is closed, or when they are garbage collected. I guess the
likely reason then for why they don't say how to handle closing them
is because regardless of what you do they will still class `close()`
at garbage collection.


>
> > +        max_retries = 5
> > +        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_started = True
> > +        for retry in range(max_retries):
> > +            try:
> > +                self.send_command(start_command)
> > +                break
> > +            except TimeoutError:
> > +                self._logger.info(
> > +                    "Interactive shell failed to start, retrying... "
> > +                    f"({retry+1} out of {max_retries})"
> > +                )
> > +        else:
> > +            self._ssh_channel.settimeout(self._timeout)
> > +            self.is_started = 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.

[PATCH v3 2/4] 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 wrapper
around the interactive shell 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.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 .../critical_interactive_shell.py             | 93 +++++++++++++++++++
 .../remote_session/interactive_shell.py       | 13 ++-
 dts/framework/remote_session/testpmd_shell.py | 13 ++-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/tests/TestSuite_pmd_buffer_scatter.py     | 28 +++---
 dts/tests/TestSuite_smoke_tests.py            |  3 +-
 6 files changed, 134 insertions(+), 24 deletions(-)
 create mode 100644 dts/framework/remote_session/critical_interactive_shell.py

diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
new file mode 100644
index 0000000000..26bd891267
--- /dev/null
+++ b/dts/framework/remote_session/critical_interactive_shell.py
@@ -0,0 +1,93 @@
+r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
+
+Critical applications are defined as applications that require explicit clean-up before another
+instance of some application can be started. In DPDK these are referred to as "primary
+applications" and these applications take out a lock which stops other primary applications from
+running. Much like :class:`~.interactive_shell.InteractiveShell`\s,
+:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
+specific functionality and should never be instantiated directly.
+"""
+
+from typing import Callable
+
+from paramiko import SSHClient  # type: ignore[import]
+from typing_extensions import Self
+
+from framework.logger import DTSLogger
+from framework.settings import SETTINGS
+
+from .interactive_shell import InteractiveShell
+
+
+class CriticalInteractiveShell(InteractiveShell):
+    """The base class for interactive critical applications.
+
+    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
+    implement the exact same functionality with the primary difference being how the application
+    is started and stopped. In contrast to normal interactive shells, this class does not start the
+    application upon initialization of the class. Instead, the application is handled through a
+    context manager. This allows for more explicit starting and stopping of the application, and
+    more guarantees for when the application is cleaned up which are not present with normal
+    interactive shells that get cleaned up upon garbage collection.
+    """
+
+    _get_privileged_command: Callable[[str], str] | None
+
+    def __init__(
+        self,
+        interactive_session: SSHClient,
+        logger: DTSLogger,
+        get_privileged_command: Callable[[str], str] | None,
+        app_args: str = "",
+        timeout: float = SETTINGS.timeout,
+    ) -> None:
+        """Store parameters for creating an interactive shell, but do not start the application.
+
+        Note that this method also does not create the channel for the application, as this is
+        something that isn't needed until the application starts.
+
+        Args:
+            interactive_session: The SSH session dedicated to interactive shells.
+            logger: The logger instance this session will use.
+            get_privileged_command: A method for modifying a command to allow it to use
+                elevated privileges. If :data:`None`, the application will not be started
+                with elevated privileges.
+            app_args: The command line arguments to be passed to the application on startup.
+            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. The default
+                value for this argument may be modified using the :option:`--timeout` command-line
+                argument or the :envvar:`DTS_TIMEOUT` environment variable.
+        """
+        self._interactive_session = interactive_session
+        self._logger = logger
+        self._timeout = timeout
+        self._app_args = app_args
+        self._get_privileged_command = get_privileged_command
+
+    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._init_channel()
+        self._start_application(self._get_privileged_command)
+        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/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 921c73d9df..6dee7ebce0 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -89,16 +89,19 @@ def __init__(
                 and no output is gathered within the timeout, an exception is thrown.
         """
         self._interactive_session = interactive_session
-        self._ssh_channel = self._interactive_session.invoke_shell()
-        self._stdin = self._ssh_channel.makefile_stdin("w")
-        self._stdout = self._ssh_channel.makefile("r")
-        self._ssh_channel.settimeout(timeout)
-        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
         self._logger = logger
         self._timeout = timeout
         self._app_args = app_args
+        self._init_channel()
         self._start_application(get_privileged_command)
 
+    def _init_channel(self):
+        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
         """Starts a new interactive application based on the path to the app.
 
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 284412e82c..ca30aac264 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -26,7 +26,7 @@
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
-from .interactive_shell import InteractiveShell
+from .critical_interactive_shell import CriticalInteractiveShell
 
 
 class TestPmdDevice(object):
@@ -82,7 +82,7 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
-class TestPmdShell(InteractiveShell):
+class TestPmdShell(CriticalInteractiveShell):
     """Testpmd interactive shell.
 
     The testpmd shell users should never use
@@ -253,6 +253,15 @@ def get_capas_rxq(
                 else:
                     unsupported_capabilities.add(NicCapability.scattered_rx)
 
+    def __exit__(self, *_) -> None:
+        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
+
+        Ensures that when the context is exited packet forwarding is stopped before closing the
+        application.
+        """
+        self.stop()
+        super().__exit__()
+
 
 class NicCapability(Enum):
     """A mapping between capability names and the associated :class:`TestPmdShell` methods.
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 1fb536735d..7dd39fd735 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -243,10 +243,10 @@ def get_supported_capabilities(
         unsupported_capas: set[NicCapability] = set()
         self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
         testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
-        for capability in capabilities:
-            if capability not in supported_capas or capability not in unsupported_capas:
-                capability.value(testpmd_shell, supported_capas, unsupported_capas)
-        del testpmd_shell
+        with testpmd_shell as running_testpmd:
+            for capability in capabilities:
+                if capability not in supported_capas or capability not in unsupported_capas:
+                    capability.value(running_testpmd, supported_capas, unsupported_capas)
         return supported_capas
 
     def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 3701c47408..41f6090a7e 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = self.sut_node.create_interactive_shell(
+        testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
             app_parameters=(
                 "--mbcache=200 "
@@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
             ),
             privileged=True,
         )
-        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
-        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.set_forward_mode(TestPmdForwardingModes.mac)
+            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}.",
+                )
+            testpmd.stop()
 
     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 a553e89662..360e64eb5a 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 = self.sut_node.create_interactive_shell(TestPmdShell, privileged=True)
-        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.1

Re: [PATCH v3 2/4] dts: add context manager for interactive shells

[Juraj Linkeš]

It seems to me the patch would benefit from Luca's testpmd changes, 
mainly how the Shell is created. Not sure if we actually want to do that 
with this series, but it sound enticing.

> diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
> new file mode 100644
> index 0000000000..26bd891267
> --- /dev/null
> +++ b/dts/framework/remote_session/critical_interactive_shell.py
> @@ -0,0 +1,93 @@
> +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
> +
> +Critical applications are defined as applications that require explicit clean-up before another
> +instance of some application can be started. In DPDK these are referred to as "primary
> +applications" and these applications take out a lock which stops other primary applications from
> +running.

Sounds like this is implemented in both classes. In this class, we 
ensure that the instance is closed when we're done with it and in the 
superclass we make sure we keep trying to connect in case a previous 
instance has not yet been cleaned up. This results in a name that's not 
very accurate.

> Much like :class:`~.interactive_shell.InteractiveShell`\s,
> +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
> +specific functionality and should never be instantiated directly.
> +"""
> +
> +from typing import Callable
> +
> +from paramiko import SSHClient  # type: ignore[import]
> +from typing_extensions import Self
> +
> +from framework.logger import DTSLogger
> +from framework.settings import SETTINGS
> +
> +from .interactive_shell import InteractiveShell
> +
> +
> +class CriticalInteractiveShell(InteractiveShell):
> +    """The base class for interactive critical applications.
> +
> +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always

This actually sounds backwards to me. This should be the base class with 
InteractiveShell adding the ability to start the shell without the 
context manager (either right away or explicitly after creating the object).

If we change this, then the name (CriticalInteractiveShell) starts to 
make sense. The base class is just for critical applications and the 
subclass offers more, so a more generic name makes sense. The only thing 
is that we chose a different name for something already defined in DPDK 
(critical vs primary; I don't see why we should use a different term). 
With this in mind, I'd just call this class PrimaryAppInteractiveShell 
or maybe just ContextInteractiveShell.

> +    implement the exact same functionality with the primary difference being how the application
> +    is started and stopped. In contrast to normal interactive shells, this class does not start the
> +    application upon initialization of the class. Instead, the application is handled through a
> +    context manager. This allows for more explicit starting and stopping of the application, and
> +    more guarantees for when the application is cleaned up which are not present with normal
> +    interactive shells that get cleaned up upon garbage collection.
> +    """
> +
> +    _get_privileged_command: Callable[[str], str] | None
> +
> +    def __init__(
> +        self,
> +        interactive_session: SSHClient,
> +        logger: DTSLogger,
> +        get_privileged_command: Callable[[str], str] | None,
> +        app_args: str = "",
> +        timeout: float = SETTINGS.timeout,
> +    ) -> None > +        """Store parameters for creating an interactive shell, but 
do not start the application.
> +
> +        Note that this method also does not create the channel for the application, as this is
> +        something that isn't needed until the application starts.
> +
> +        Args:
> +            interactive_session: The SSH session dedicated to interactive shells.
> +            logger: The logger instance this session will use.
> +            get_privileged_command: A method for modifying a command to allow it to use
> +                elevated privileges. If :data:`None`, the application will not be started
> +                with elevated privileges.
> +            app_args: The command line arguments to be passed to the application on startup.
> +            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. The default
> +                value for this argument may be modified using the :option:`--timeout` command-line
> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> +        """
> +        self._interactive_session = interactive_session
> +        self._logger = logger
> +        self._timeout = timeout
> +        self._app_args = app_args
> +        self._get_privileged_command = get_privileged_command

We see here why it's backwards. We're duplicating this part of the code 
and if the class relation is the other way around we can just call 
super().__init__().

> +
> +    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._init_channel()
> +        self._start_application(self._get_privileged_command)
> +        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 284412e82c..ca30aac264 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py

> @@ -253,6 +253,15 @@ def get_capas_rxq(
>                   else:
>                       unsupported_capabilities.add(NicCapability.scattered_rx)
>   
> +    def __exit__(self, *_) -> None:
> +        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
> +
> +        Ensures that when the context is exited packet forwarding is stopped before closing the
> +        application.
> +        """
> +        self.stop()
> +        super().__exit__()
> +

I think it would more sense to add this to self.close().

>   
>   class NicCapability(Enum):
>       """A mapping between capability names and the associated :class:`TestPmdShell` methods.

> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 3701c47408..41f6090a7e 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
>           Test:
>               Start testpmd and run functional test with preset mbsize.
>           """
> -        testpmd = self.sut_node.create_interactive_shell(
> +        testpmd_shell = self.sut_node.create_interactive_shell(
>               TestPmdShell,
>               app_parameters=(
>                   "--mbcache=200 "
> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
>               ),
>               privileged=True,
>           )
> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> -        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.set_forward_mode(TestPmdForwardingModes.mac)
> +            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}.",
> +                )
> +            testpmd.stop()

This is now not needed since you added this to __exit__(), right?

But we should consider removing this (stopping forwarding) altogether 
since you mentioned we don't really need this. I'm not sure what it adds 
or what the rationale is - testpmd is going to handle this just fine, 
right? And we're not doing any other cleanup, we're leaving all of that 
to testpmd.

>   
>       def test_scatter_mbuf_2048(self) -> None:
>           """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""

Re: [PATCH v3 2/4] dts: add context manager for interactive shells

[Jeremy Spewock]

Overall, my thoughts are that it's definitely an interesting idea to
make the normal shell subclass the critical. I explain more below, but
basically I think it makes sense as long as we are fine with the
normal shells having a context manager which likely won't really be
used since it doesn't really serve a purpose for them.

On Mon, Jun 10, 2024 at 10:31 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
> It seems to me the patch would benefit from Luca's testpmd changes,
> mainly how the Shell is created. Not sure if we actually want to do that
> with this series, but it sound enticing.

It definitely would make it more sleek. I would vouch for it, but just
because this also depends on the capabilities patch it makes me
hesitant to wait on another (it already has formatting warnings
without Luca's mypy changes), but I guess ideally it would get merged
after Luca's so that I can rebase and use his changes here.

>
> > diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
> > new file mode 100644
> > index 0000000000..26bd891267
> > --- /dev/null
> > +++ b/dts/framework/remote_session/critical_interactive_shell.py
> > @@ -0,0 +1,93 @@
> > +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
> > +
> > +Critical applications are defined as applications that require explicit clean-up before another
> > +instance of some application can be started. In DPDK these are referred to as "primary
> > +applications" and these applications take out a lock which stops other primary applications from
> > +running.
>
> Sounds like this is implemented in both classes. In this class, we
> ensure that the instance is closed when we're done with it and in the
> superclass we make sure we keep trying to connect in case a previous
> instance has not yet been cleaned up. This results in a name that's not
> very accurate.

This is a good point. I ended up adding the retry functionality to try
to address this problem first, and then still found it useful after
adding the context manager so I figured I'd leave it in the top level
class. In hindsight what you are saying makes sense that this doesn't
need to be in applications that don't rely on others being stopped, so
there isn't much of a point to having it in all interactive shells.
The only difficulty with adding it here is that there would be a lot
more code duplication since I would have to do the whole
_start_application method over again in this class. Unless, of course,
we go the other route of making the normal shell a subclass of this
one, in which case the normal shell would still need a retry... I
guess the easiest way to handle this would just be making the number
of retries a parameter to the method and the normal shells don't allow
for any. That or I could just pull out the connection part like I did
with _init_channels and modify that.

>
> > Much like :class:`~.interactive_shell.InteractiveShell`\s,
> > +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
> > +specific functionality and should never be instantiated directly.
> > +"""
> > +
> > +from typing import Callable
> > +
> > +from paramiko import SSHClient  # type: ignore[import]
> > +from typing_extensions import Self
> > +
> > +from framework.logger import DTSLogger
> > +from framework.settings import SETTINGS
> > +
> > +from .interactive_shell import InteractiveShell
> > +
> > +
> > +class CriticalInteractiveShell(InteractiveShell):
> > +    """The base class for interactive critical applications.
> > +
> > +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
>
> This actually sounds backwards to me. This should be the base class with
> InteractiveShell adding the ability to start the shell without the
> context manager (either right away or explicitly after creating the object).
>

I guess I kind of see the context manager as the additional feature
rather than the ability to start and stop automatically. I actually
even deliberately did it this way because I figured that using normal
shells as a context manager wasn't really useful, so I didn't add the
ability to. It's an interesting idea and it might shorten some of the
code like you mention in other places.

> If we change this, then the name (CriticalInteractiveShell) starts to
> make sense. The base class is just for critical applications and the
> subclass offers more, so a more generic name makes sense. The only thing

I guess I was thinking of "critical" in the name being more like
"important" rather than like, "necessary" or as a "base" set of
applications if that makes sense.

> is that we chose a different name for something already defined in DPDK
> (critical vs primary; I don't see why we should use a different term).
> With this in mind, I'd just call this class PrimaryAppInteractiveShell
> or maybe just ContextInteractiveShell.

I only really deviated from the DPDK language because I didn't want it
to be like, this is a class for DPDK primary applications, as much as
I was thinking of it as generically just a class that can be used for
any application that there can only be one instance of at a time. I
guess it will mostly just be DPDK applications in this context, so
just following the DPDK way of stating it might make sense.

>
> > +    implement the exact same functionality with the primary difference being how the application
> > +    is started and stopped. In contrast to normal interactive shells, this class does not start the
> > +    application upon initialization of the class. Instead, the application is handled through a
> > +    context manager. This allows for more explicit starting and stopping of the application, and
> > +    more guarantees for when the application is cleaned up which are not present with normal
> > +    interactive shells that get cleaned up upon garbage collection.
> > +    """
> > +
> > +    _get_privileged_command: Callable[[str], str] | None
> > +
> > +    def __init__(
> > +        self,
> > +        interactive_session: SSHClient,
> > +        logger: DTSLogger,
> > +        get_privileged_command: Callable[[str], str] | None,
> > +        app_args: str = "",
> > +        timeout: float = SETTINGS.timeout,
> > +    ) -> None > +        """Store parameters for creating an interactive shell, but
> do not start the application.
> > +
> > +        Note that this method also does not create the channel for the application, as this is
> > +        something that isn't needed until the application starts.
> > +
> > +        Args:
> > +            interactive_session: The SSH session dedicated to interactive shells.
> > +            logger: The logger instance this session will use.
> > +            get_privileged_command: A method for modifying a command to allow it to use
> > +                elevated privileges. If :data:`None`, the application will not be started
> > +                with elevated privileges.
> > +            app_args: The command line arguments to be passed to the application on startup.
> > +            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. The default
> > +                value for this argument may be modified using the :option:`--timeout` command-line
> > +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> > +        """
> > +        self._interactive_session = interactive_session
> > +        self._logger = logger
> > +        self._timeout = timeout
> > +        self._app_args = app_args
> > +        self._get_privileged_command = get_privileged_command
>
> We see here why it's backwards. We're duplicating this part of the code
> and if the class relation is the other way around we can just call
> super().__init__().

I agree, this method does make it seem a little backwards.

>
> > +
> > +    def __enter__(self) -> Self:
<snip>
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> > index 284412e82c..ca30aac264 100644
> > --- a/dts/framework/remote_session/testpmd_shell.py
> > +++ b/dts/framework/remote_session/testpmd_shell.py
>
> > @@ -253,6 +253,15 @@ def get_capas_rxq(
> >                   else:
> >                       unsupported_capabilities.add(NicCapability.scattered_rx)
> >
> > +    def __exit__(self, *_) -> None:
> > +        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
> > +
> > +        Ensures that when the context is exited packet forwarding is stopped before closing the
> > +        application.
> > +        """
> > +        self.stop()
> > +        super().__exit__()
> > +
>
> I think it would more sense to add this to self.close().

Ack.

>
> >
> >   class NicCapability(Enum):
> >       """A mapping between capability names and the associated :class:`TestPmdShell` methods.
>
> > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > index 3701c47408..41f6090a7e 100644
> > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
<snip>
> >                   "--mbcache=200 "
> > @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
> >               ),
> >               privileged=True,
> >           )
> > -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > -        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.set_forward_mode(TestPmdForwardingModes.mac)
> > +            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}.",
> > +                )
> > +            testpmd.stop()
>
> This is now not needed since you added this to __exit__(), right?

Right, we don't need it here. I left it just because I like being a
little more explicit, but I can remove it since it is just an unneeded
extra line.

>
> But we should consider removing this (stopping forwarding) altogether
> since you mentioned we don't really need this. I'm not sure what it adds
> or what the rationale is - testpmd is going to handle this just fine,
> right? And we're not doing any other cleanup, we're leaving all of that
> to testpmd.

I don't think we should remove it entirely, there is something
beneficial that can come from explicitly stopping forwarding. When the
method returns None (like it does now) I agree that it is useless, but
when you stop forwarding it prints the statistics for each port. I
modified the stop method in another series that isn't out yet actually
for adding another test suite and use its output for validation.


>
> >
> >       def test_scatter_mbuf_2048(self) -> None:
> >           """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
>

Re: [PATCH v3 2/4] dts: add context manager for interactive shells

[Juraj Linkeš]

On 10. 6. 2024 22:06, Jeremy Spewock wrote:
> Overall, my thoughts are that it's definitely an interesting idea to
> make the normal shell subclass the critical. I explain more below, but
> basically I think it makes sense as long as we are fine with the
> normal shells having a context manager which likely won't really be
> used since it doesn't really serve a purpose for them.
> 
> On Mon, Jun 10, 2024 at 10:31 AM Juraj Linkeš
> <juraj.linkes@pantheon.tech> wrote:
>>
>> It seems to me the patch would benefit from Luca's testpmd changes,
>> mainly how the Shell is created. Not sure if we actually want to do that
>> with this series, but it sound enticing.
> 
> It definitely would make it more sleek. I would vouch for it, but just
> because this also depends on the capabilities patch it makes me
> hesitant to wait on another (it already has formatting warnings
> without Luca's mypy changes), but I guess ideally it would get merged
> after Luca's so that I can rebase and use his changes here.
> 

We can talk about this in the call with everyone present and agree on 
the roadmap with these three patches (capabilities, testpmd params and 
this one).

>>
>>> diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
>>> new file mode 100644
>>> index 0000000000..26bd891267
>>> --- /dev/null
>>> +++ b/dts/framework/remote_session/critical_interactive_shell.py
>>> @@ -0,0 +1,93 @@
>>> +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
>>> +
>>> +Critical applications are defined as applications that require explicit clean-up before another
>>> +instance of some application can be started. In DPDK these are referred to as "primary
>>> +applications" and these applications take out a lock which stops other primary applications from
>>> +running.
>>
>> Sounds like this is implemented in both classes. In this class, we
>> ensure that the instance is closed when we're done with it and in the
>> superclass we make sure we keep trying to connect in case a previous
>> instance has not yet been cleaned up. This results in a name that's not
>> very accurate.
> 
> This is a good point. I ended up adding the retry functionality to try
> to address this problem first, and then still found it useful after
> adding the context manager so I figured I'd leave it in the top level
> class. In hindsight what you are saying makes sense that this doesn't
> need to be in applications that don't rely on others being stopped, so
> there isn't much of a point to having it in all interactive shells.
> The only difficulty with adding it here is that there would be a lot
> more code duplication since I would have to do the whole
> _start_application method over again in this class. Unless, of course,
> we go the other route of making the normal shell a subclass of this
> one, in which case the normal shell would still need a retry... I
> guess the easiest way to handle this would just be making the number
> of retries a parameter to the method and the normal shells don't allow
> for any. That or I could just pull out the connection part like I did
> with _init_channels and modify that.
> 

My point was not to not have it regular shells, we can have it there 
too. But maybe we don't want to, I'm not sure.
If so, a parameter for the primary/critical app shell sounds good; the 
regular shell won't have it and would just pass 0 to the super() call. 
Or we could have parameter in the regular shell as well, defaulting to 0.

>>
>>> Much like :class:`~.interactive_shell.InteractiveShell`\s,
>>> +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
>>> +specific functionality and should never be instantiated directly.
>>> +"""
>>> +
>>> +from typing import Callable
>>> +
>>> +from paramiko import SSHClient  # type: ignore[import]
>>> +from typing_extensions import Self
>>> +
>>> +from framework.logger import DTSLogger
>>> +from framework.settings import SETTINGS
>>> +
>>> +from .interactive_shell import InteractiveShell
>>> +
>>> +
>>> +class CriticalInteractiveShell(InteractiveShell):
>>> +    """The base class for interactive critical applications.
>>> +
>>> +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
>>
>> This actually sounds backwards to me. This should be the base class with
>> InteractiveShell adding the ability to start the shell without the
>> context manager (either right away or explicitly after creating the object).
>>
> 
> I guess I kind of see the context manager as the additional feature
> rather than the ability to start and stop automatically. I actually
> even deliberately did it this way because I figured that using normal
> shells as a context manager wasn't really useful, so I didn't add the
> ability to. It's an interesting idea and it might shorten some of the
> code like you mention in other places.
> 

We don't really lose anything by having it in regular shells. It may be 
useful and there isn't really any extra maintenance we'd need to do.

>> If we change this, then the name (CriticalInteractiveShell) starts to
>> make sense. The base class is just for critical applications and the
>> subclass offers more, so a more generic name makes sense. The only thing
> 
> I guess I was thinking of "critical" in the name being more like
> "important" rather than like, "necessary" or as a "base" set of
> applications if that makes sense.
> 
>> is that we chose a different name for something already defined in DPDK
>> (critical vs primary; I don't see why we should use a different term).
>> With this in mind, I'd just call this class PrimaryAppInteractiveShell
>> or maybe just ContextInteractiveShell.
> 
> I only really deviated from the DPDK language because I didn't want it
> to be like, this is a class for DPDK primary applications, as much as
> I was thinking of it as generically just a class that can be used for
> any application that there can only be one instance of at a time. I
> guess it will mostly just be DPDK applications in this context, so
> just following the DPDK way of stating it might make sense.
> 

Having a more generic name is preferable, but primary doesn't have to 
mean just DPDK apps. I think we can find a better name though. Maybe 
something like SingletonInteractiveShell? It's not really a singleton, 
so we should use something else, maybe SingleActiveInteractiveShell? We 
can have as many instances we want, but just one that's 
active/alive/connected. Or SingleAppInteractiveShell?

>>
>>> +    implement the exact same functionality with the primary difference being how the application
>>> +    is started and stopped. In contrast to normal interactive shells, this class does not start the
>>> +    application upon initialization of the class. Instead, the application is handled through a
>>> +    context manager. This allows for more explicit starting and stopping of the application, and
>>> +    more guarantees for when the application is cleaned up which are not present with normal
>>> +    interactive shells that get cleaned up upon garbage collection.
>>> +    """
>>> +
>>> +    _get_privileged_command: Callable[[str], str] | None
>>> +
>>> +    def __init__(
>>> +        self,
>>> +        interactive_session: SSHClient,
>>> +        logger: DTSLogger,
>>> +        get_privileged_command: Callable[[str], str] | None,
>>> +        app_args: str = "",
>>> +        timeout: float = SETTINGS.timeout,
>>> +    ) -> None > +        """Store parameters for creating an interactive shell, but
>> do not start the application.
>>> +
>>> +        Note that this method also does not create the channel for the application, as this is
>>> +        something that isn't needed until the application starts.
>>> +
>>> +        Args:
>>> +            interactive_session: The SSH session dedicated to interactive shells.
>>> +            logger: The logger instance this session will use.
>>> +            get_privileged_command: A method for modifying a command to allow it to use
>>> +                elevated privileges. If :data:`None`, the application will not be started
>>> +                with elevated privileges.
>>> +            app_args: The command line arguments to be passed to the application on startup.
>>> +            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. The default
>>> +                value for this argument may be modified using the :option:`--timeout` command-line
>>> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
>>> +        """
>>> +        self._interactive_session = interactive_session
>>> +        self._logger = logger
>>> +        self._timeout = timeout
>>> +        self._app_args = app_args
>>> +        self._get_privileged_command = get_privileged_command
>>
>> We see here why it's backwards. We're duplicating this part of the code
>> and if the class relation is the other way around we can just call
>> super().__init__().
> 
> I agree, this method does make it seem a little backwards.
> 
>>
>>> +
>>> +    def __enter__(self) -> Self:
> <snip>
>>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>>> index 284412e82c..ca30aac264 100644
>>> --- a/dts/framework/remote_session/testpmd_shell.py
>>> +++ b/dts/framework/remote_session/testpmd_shell.py
>>
>>> @@ -253,6 +253,15 @@ def get_capas_rxq(
>>>                    else:
>>>                        unsupported_capabilities.add(NicCapability.scattered_rx)
>>>
>>> +    def __exit__(self, *_) -> None:
>>> +        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
>>> +
>>> +        Ensures that when the context is exited packet forwarding is stopped before closing the
>>> +        application.
>>> +        """
>>> +        self.stop()
>>> +        super().__exit__()
>>> +
>>
>> I think it would more sense to add this to self.close().
> 
> Ack.
> 
>>
>>>
>>>    class NicCapability(Enum):
>>>        """A mapping between capability names and the associated :class:`TestPmdShell` methods.
>>
>>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
>>> index 3701c47408..41f6090a7e 100644
>>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
>>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> <snip>
>>>                    "--mbcache=200 "
>>> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
>>>                ),
>>>                privileged=True,
>>>            )
>>> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
>>> -        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.set_forward_mode(TestPmdForwardingModes.mac)
>>> +            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}.",
>>> +                )
>>> +            testpmd.stop()
>>
>> This is now not needed since you added this to __exit__(), right?
> 
> Right, we don't need it here. I left it just because I like being a
> little more explicit, but I can remove it since it is just an unneeded
> extra line.
> 

Not just an extra line, but unnecessary (and possibly confusing) logs 
when doing it for the second time.

>>
>> But we should consider removing this (stopping forwarding) altogether
>> since you mentioned we don't really need this. I'm not sure what it adds
>> or what the rationale is - testpmd is going to handle this just fine,
>> right? And we're not doing any other cleanup, we're leaving all of that
>> to testpmd.
> 
> I don't think we should remove it entirely, there is something
> beneficial that can come from explicitly stopping forwarding. When the
> method returns None (like it does now) I agree that it is useless, but
> when you stop forwarding it prints the statistics for each port. I
> modified the stop method in another series that isn't out yet actually
> for adding another test suite and use its output for validation.
> 

Oh, that sounds great. Any extra info like this is great for debugging, 
let's definitely keep it then.

> 
>>
>>>
>>>        def test_scatter_mbuf_2048(self) -> None:
>>>            """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
>>

Re: [PATCH v3 2/4] dts: add context manager for interactive shells

[Jeremy Spewock]

On Tue, Jun 11, 2024 at 5:17 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 10. 6. 2024 22:06, Jeremy Spewock wrote:
> > Overall, my thoughts are that it's definitely an interesting idea to
> > make the normal shell subclass the critical. I explain more below, but
> > basically I think it makes sense as long as we are fine with the
> > normal shells having a context manager which likely won't really be
> > used since it doesn't really serve a purpose for them.
> >
> > On Mon, Jun 10, 2024 at 10:31 AM Juraj Linkeš
> > <juraj.linkes@pantheon.tech> wrote:
> >>
> >> It seems to me the patch would benefit from Luca's testpmd changes,
> >> mainly how the Shell is created. Not sure if we actually want to do that
> >> with this series, but it sound enticing.
> >
> > It definitely would make it more sleek. I would vouch for it, but just
> > because this also depends on the capabilities patch it makes me
> > hesitant to wait on another (it already has formatting warnings
> > without Luca's mypy changes), but I guess ideally it would get merged
> > after Luca's so that I can rebase and use his changes here.
> >
>
> We can talk about this in the call with everyone present and agree on
> the roadmap with these three patches (capabilities, testpmd params and
> this one).
>
> >>
> >>> diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
> >>> new file mode 100644
> >>> index 0000000000..26bd891267
> >>> --- /dev/null
> >>> +++ b/dts/framework/remote_session/critical_interactive_shell.py
> >>> @@ -0,0 +1,93 @@
> >>> +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
> >>> +
> >>> +Critical applications are defined as applications that require explicit clean-up before another
> >>> +instance of some application can be started. In DPDK these are referred to as "primary
> >>> +applications" and these applications take out a lock which stops other primary applications from
> >>> +running.
> >>
> >> Sounds like this is implemented in both classes. In this class, we
> >> ensure that the instance is closed when we're done with it and in the
> >> superclass we make sure we keep trying to connect in case a previous
> >> instance has not yet been cleaned up. This results in a name that's not
> >> very accurate.
> >
> > This is a good point. I ended up adding the retry functionality to try
> > to address this problem first, and then still found it useful after
> > adding the context manager so I figured I'd leave it in the top level
> > class. In hindsight what you are saying makes sense that this doesn't
> > need to be in applications that don't rely on others being stopped, so
> > there isn't much of a point to having it in all interactive shells.
> > The only difficulty with adding it here is that there would be a lot
> > more code duplication since I would have to do the whole
> > _start_application method over again in this class. Unless, of course,
> > we go the other route of making the normal shell a subclass of this
> > one, in which case the normal shell would still need a retry... I
> > guess the easiest way to handle this would just be making the number
> > of retries a parameter to the method and the normal shells don't allow
> > for any. That or I could just pull out the connection part like I did
> > with _init_channels and modify that.
> >
>
> My point was not to not have it regular shells, we can have it there
> too. But maybe we don't want to, I'm not sure.
> If so, a parameter for the primary/critical app shell sounds good; the
> regular shell won't have it and would just pass 0 to the super() call.
> Or we could have parameter in the regular shell as well, defaulting to 0.

In hindsight it probably doesn't hurt to leave it in the interactive
shell as well since we expect those to never retry anyway. I'll leave
the retry there for the InteractiveShell class as well and then if
there comes a time when we really can't allow them to retry for
whatever reason it'll be an easy fix with the inheritance swapped
anyway.

>
> >>
> >>> Much like :class:`~.interactive_shell.InteractiveShell`\s,
> >>> +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
> >>> +specific functionality and should never be instantiated directly.
> >>> +"""
> >>> +
> >>> +from typing import Callable
> >>> +
> >>> +from paramiko import SSHClient  # type: ignore[import]
> >>> +from typing_extensions import Self
> >>> +
> >>> +from framework.logger import DTSLogger
> >>> +from framework.settings import SETTINGS
> >>> +
> >>> +from .interactive_shell import InteractiveShell
> >>> +
> >>> +
> >>> +class CriticalInteractiveShell(InteractiveShell):
> >>> +    """The base class for interactive critical applications.
> >>> +
> >>> +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
> >>
> >> This actually sounds backwards to me. This should be the base class with
> >> InteractiveShell adding the ability to start the shell without the
> >> context manager (either right away or explicitly after creating the object).
> >>
> >
> > I guess I kind of see the context manager as the additional feature
> > rather than the ability to start and stop automatically. I actually
> > even deliberately did it this way because I figured that using normal
> > shells as a context manager wasn't really useful, so I didn't add the
> > ability to. It's an interesting idea and it might shorten some of the
> > code like you mention in other places.
> >
>
> We don't really lose anything by having it in regular shells. It may be
> useful and there isn't really any extra maintenance we'd need to do.

Fair enough. Thinking about it more it makes sense to at least give it
a shot and see how it looks.

>
> >> If we change this, then the name (CriticalInteractiveShell) starts to
> >> make sense. The base class is just for critical applications and the
> >> subclass offers more, so a more generic name makes sense. The only thing
> >
> > I guess I was thinking of "critical" in the name being more like
> > "important" rather than like, "necessary" or as a "base" set of
> > applications if that makes sense.
> >
> >> is that we chose a different name for something already defined in DPDK
> >> (critical vs primary; I don't see why we should use a different term).
> >> With this in mind, I'd just call this class PrimaryAppInteractiveShell
> >> or maybe just ContextInteractiveShell.
> >
> > I only really deviated from the DPDK language because I didn't want it
> > to be like, this is a class for DPDK primary applications, as much as
> > I was thinking of it as generically just a class that can be used for
> > any application that there can only be one instance of at a time. I
> > guess it will mostly just be DPDK applications in this context, so
> > just following the DPDK way of stating it might make sense.
> >
>
> Having a more generic name is preferable, but primary doesn't have to
> mean just DPDK apps. I think we can find a better name though. Maybe
> something like SingletonInteractiveShell? It's not really a singleton,
> so we should use something else, maybe SingleActiveInteractiveShell? We
> can have as many instances we want, but just one that's
> active/alive/connected. Or SingleAppInteractiveShell?

SingleActiveInteractiveShell is my preference out of those options.
Singleton is always what I want to call it because the applications
themselves are singletons, but the class has nothing about it that
really enforces that or makes it a singleton, it just manages the
sessions so that users only use it like a singleton. Maybe something
like ManagedInteractiveShell would work, but it isn't very descriptive
of how it is managed. With this role swap and thi class becoming the
base class, does it make sense then to change the very generic name of
InteractiveShell to something that gives more insight into its
difference from the SingelActiveInteractiveShells? I'm not really sure
what name would fit there, AutomatedInteractiveShells?
AutoInitInteractiveShells? Or do you think their name is fine to show
a sort of "single instance shells vs. everything else" kind of
relationship?

>
> >>
> >>> +    implement the exact same functionality with the primary difference being how the application
> >>> +    is started and stopped. In contrast to normal interactive shells, this class does not start the
> >>> +    application upon initialization of the class. Instead, the application is handled through a
> >>> +    context manager. This allows for more explicit starting and stopping of the application, and
> >>> +    more guarantees for when the application is cleaned up which are not present with normal
> >>> +    interactive shells that get cleaned up upon garbage collection.
> >>> +    """
> >>> +
> >>> +    _get_privileged_command: Callable[[str], str] | None
> >>> +
> >>> +    def __init__(
> >>> +        self,
> >>> +        interactive_session: SSHClient,
> >>> +        logger: DTSLogger,
> >>> +        get_privileged_command: Callable[[str], str] | None,
> >>> +        app_args: str = "",
> >>> +        timeout: float = SETTINGS.timeout,
> >>> +    ) -> None > +        """Store parameters for creating an interactive shell, but
> >> do not start the application.
> >>> +
> >>> +        Note that this method also does not create the channel for the application, as this is
> >>> +        something that isn't needed until the application starts.
> >>> +
> >>> +        Args:
> >>> +            interactive_session: The SSH session dedicated to interactive shells.
> >>> +            logger: The logger instance this session will use.
> >>> +            get_privileged_command: A method for modifying a command to allow it to use
> >>> +                elevated privileges. If :data:`None`, the application will not be started
> >>> +                with elevated privileges.
> >>> +            app_args: The command line arguments to be passed to the application on startup.
> >>> +            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. The default
> >>> +                value for this argument may be modified using the :option:`--timeout` command-line
> >>> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> >>> +        """
> >>> +        self._interactive_session = interactive_session
> >>> +        self._logger = logger
> >>> +        self._timeout = timeout
> >>> +        self._app_args = app_args
> >>> +        self._get_privileged_command = get_privileged_command
> >>
> >> We see here why it's backwards. We're duplicating this part of the code
> >> and if the class relation is the other way around we can just call
> >> super().__init__().
> >
> > I agree, this method does make it seem a little backwards.
> >
> >>
> >>> +
> >>> +    def __enter__(self) -> Self:
> > <snip>
> >>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> >>> index 284412e82c..ca30aac264 100644
> >>> --- a/dts/framework/remote_session/testpmd_shell.py
> >>> +++ b/dts/framework/remote_session/testpmd_shell.py
> >>
> >>> @@ -253,6 +253,15 @@ def get_capas_rxq(
> >>>                    else:
> >>>                        unsupported_capabilities.add(NicCapability.scattered_rx)
> >>>
> >>> +    def __exit__(self, *_) -> None:
> >>> +        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
> >>> +
> >>> +        Ensures that when the context is exited packet forwarding is stopped before closing the
> >>> +        application.
> >>> +        """
> >>> +        self.stop()
> >>> +        super().__exit__()
> >>> +
> >>
> >> I think it would more sense to add this to self.close().
> >
> > Ack.
> >
> >>
> >>>
> >>>    class NicCapability(Enum):
> >>>        """A mapping between capability names and the associated :class:`TestPmdShell` methods.
> >>
> >>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> >>> index 3701c47408..41f6090a7e 100644
> >>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> >>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > <snip>
> >>>                    "--mbcache=200 "
> >>> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
> >>>                ),
> >>>                privileged=True,
> >>>            )
> >>> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> >>> -        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.set_forward_mode(TestPmdForwardingModes.mac)
> >>> +            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}.",
> >>> +                )
> >>> +            testpmd.stop()
> >>
> >> This is now not needed since you added this to __exit__(), right?
> >
> > Right, we don't need it here. I left it just because I like being a
> > little more explicit, but I can remove it since it is just an unneeded
> > extra line.
> >
>
> Not just an extra line, but unnecessary (and possibly confusing) logs
> when doing it for the second time.

True, this probably is a little strange to see twice if you don't
understand why.


>
> >>
> >> But we should consider removing this (stopping forwarding) altogether
> >> since you mentioned we don't really need this. I'm not sure what it adds
> >> or what the rationale is - testpmd is going to handle this just fine,
> >> right? And we're not doing any other cleanup, we're leaving all of that
> >> to testpmd.
> >
> > I don't think we should remove it entirely, there is something
> > beneficial that can come from explicitly stopping forwarding. When the
> > method returns None (like it does now) I agree that it is useless, but
> > when you stop forwarding it prints the statistics for each port. I
> > modified the stop method in another series that isn't out yet actually
> > for adding another test suite and use its output for validation.
> >
>
> Oh, that sounds great. Any extra info like this is great for debugging,
> let's definitely keep it then.
>
> >
> >>
> >>>
> >>>        def test_scatter_mbuf_2048(self) -> None:
> >>>            """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> >>

Re: [PATCH v3 2/4] dts: add context manager for interactive shells

[Juraj Linkeš]

On 11. 6. 2024 17:33, Jeremy Spewock wrote:
> On Tue, Jun 11, 2024 at 5:17 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>>
>>
>>
>> On 10. 6. 2024 22:06, Jeremy Spewock wrote:
>>> Overall, my thoughts are that it's definitely an interesting idea to
>>> make the normal shell subclass the critical. I explain more below, but
>>> basically I think it makes sense as long as we are fine with the
>>> normal shells having a context manager which likely won't really be
>>> used since it doesn't really serve a purpose for them.
>>>
>>> On Mon, Jun 10, 2024 at 10:31 AM Juraj Linkeš
>>> <juraj.linkes@pantheon.tech> wrote:
>>>>
>>>> It seems to me the patch would benefit from Luca's testpmd changes,
>>>> mainly how the Shell is created. Not sure if we actually want to do that
>>>> with this series, but it sound enticing.
>>>
>>> It definitely would make it more sleek. I would vouch for it, but just
>>> because this also depends on the capabilities patch it makes me
>>> hesitant to wait on another (it already has formatting warnings
>>> without Luca's mypy changes), but I guess ideally it would get merged
>>> after Luca's so that I can rebase and use his changes here.
>>>
>>
>> We can talk about this in the call with everyone present and agree on
>> the roadmap with these three patches (capabilities, testpmd params and
>> this one).
>>
>>>>
>>>>> diff --git a/dts/framework/remote_session/critical_interactive_shell.py b/dts/framework/remote_session/critical_interactive_shell.py
>>>>> new file mode 100644
>>>>> index 0000000000..26bd891267
>>>>> --- /dev/null
>>>>> +++ b/dts/framework/remote_session/critical_interactive_shell.py
>>>>> @@ -0,0 +1,93 @@
>>>>> +r"""Wrapper around :class:`~.interactive_shell.InteractiveShell` that handles critical applications.
>>>>> +
>>>>> +Critical applications are defined as applications that require explicit clean-up before another
>>>>> +instance of some application can be started. In DPDK these are referred to as "primary
>>>>> +applications" and these applications take out a lock which stops other primary applications from
>>>>> +running.
>>>>
>>>> Sounds like this is implemented in both classes. In this class, we
>>>> ensure that the instance is closed when we're done with it and in the
>>>> superclass we make sure we keep trying to connect in case a previous
>>>> instance has not yet been cleaned up. This results in a name that's not
>>>> very accurate.
>>>
>>> This is a good point. I ended up adding the retry functionality to try
>>> to address this problem first, and then still found it useful after
>>> adding the context manager so I figured I'd leave it in the top level
>>> class. In hindsight what you are saying makes sense that this doesn't
>>> need to be in applications that don't rely on others being stopped, so
>>> there isn't much of a point to having it in all interactive shells.
>>> The only difficulty with adding it here is that there would be a lot
>>> more code duplication since I would have to do the whole
>>> _start_application method over again in this class. Unless, of course,
>>> we go the other route of making the normal shell a subclass of this
>>> one, in which case the normal shell would still need a retry... I
>>> guess the easiest way to handle this would just be making the number
>>> of retries a parameter to the method and the normal shells don't allow
>>> for any. That or I could just pull out the connection part like I did
>>> with _init_channels and modify that.
>>>
>>
>> My point was not to not have it regular shells, we can have it there
>> too. But maybe we don't want to, I'm not sure.
>> If so, a parameter for the primary/critical app shell sounds good; the
>> regular shell won't have it and would just pass 0 to the super() call.
>> Or we could have parameter in the regular shell as well, defaulting to 0.
> 
> In hindsight it probably doesn't hurt to leave it in the interactive
> shell as well since we expect those to never retry anyway. I'll leave
> the retry there for the InteractiveShell class as well and then if
> there comes a time when we really can't allow them to retry for
> whatever reason it'll be an easy fix with the inheritance swapped
> anyway.
> 

We don't have to swap the inheritance if we don't want the retries, we 
can do this in InteractiveShell:
def __init__(**kwargs):
     super().__init__(retries=0, **kwargs)

>>
>>>>
>>>>> Much like :class:`~.interactive_shell.InteractiveShell`\s,
>>>>> +:class:`CriticalInteractiveShell` is meant to be extended by subclasses that implement application
>>>>> +specific functionality and should never be instantiated directly.
>>>>> +"""
>>>>> +
>>>>> +from typing import Callable
>>>>> +
>>>>> +from paramiko import SSHClient  # type: ignore[import]
>>>>> +from typing_extensions import Self
>>>>> +
>>>>> +from framework.logger import DTSLogger
>>>>> +from framework.settings import SETTINGS
>>>>> +
>>>>> +from .interactive_shell import InteractiveShell
>>>>> +
>>>>> +
>>>>> +class CriticalInteractiveShell(InteractiveShell):
>>>>> +    """The base class for interactive critical applications.
>>>>> +
>>>>> +    This class is a wrapper around :class:`~.interactive_shell.InteractiveShell` and should always
>>>>
>>>> This actually sounds backwards to me. This should be the base class with
>>>> InteractiveShell adding the ability to start the shell without the
>>>> context manager (either right away or explicitly after creating the object).
>>>>
>>>
>>> I guess I kind of see the context manager as the additional feature
>>> rather than the ability to start and stop automatically. I actually
>>> even deliberately did it this way because I figured that using normal
>>> shells as a context manager wasn't really useful, so I didn't add the
>>> ability to. It's an interesting idea and it might shorten some of the
>>> code like you mention in other places.
>>>
>>
>> We don't really lose anything by having it in regular shells. It may be
>> useful and there isn't really any extra maintenance we'd need to do.
> 
> Fair enough. Thinking about it more it makes sense to at least give it
> a shot and see how it looks.
> 
>>
>>>> If we change this, then the name (CriticalInteractiveShell) starts to
>>>> make sense. The base class is just for critical applications and the
>>>> subclass offers more, so a more generic name makes sense. The only thing
>>>
>>> I guess I was thinking of "critical" in the name being more like
>>> "important" rather than like, "necessary" or as a "base" set of
>>> applications if that makes sense.
>>>
>>>> is that we chose a different name for something already defined in DPDK
>>>> (critical vs primary; I don't see why we should use a different term).
>>>> With this in mind, I'd just call this class PrimaryAppInteractiveShell
>>>> or maybe just ContextInteractiveShell.
>>>
>>> I only really deviated from the DPDK language because I didn't want it
>>> to be like, this is a class for DPDK primary applications, as much as
>>> I was thinking of it as generically just a class that can be used for
>>> any application that there can only be one instance of at a time. I
>>> guess it will mostly just be DPDK applications in this context, so
>>> just following the DPDK way of stating it might make sense.
>>>
>>
>> Having a more generic name is preferable, but primary doesn't have to
>> mean just DPDK apps. I think we can find a better name though. Maybe
>> something like SingletonInteractiveShell? It's not really a singleton,
>> so we should use something else, maybe SingleActiveInteractiveShell? We
>> can have as many instances we want, but just one that's
>> active/alive/connected. Or SingleAppInteractiveShell?
> 
> SingleActiveInteractiveShell is my preference out of those options.
> Singleton is always what I want to call it because the applications
> themselves are singletons, but the class has nothing about it that
> really enforces that or makes it a singleton, it just manages the
> sessions so that users only use it like a singleton. Maybe something
> like ManagedInteractiveShell would work, but it isn't very descriptive
> of how it is managed. With this role swap and thi class becoming the
> base class, does it make sense then to change the very generic name of
> InteractiveShell to something that gives more insight into its
> difference from the SingelActiveInteractiveShells? I'm not really sure
> what name would fit there, AutomatedInteractiveShells?
> AutoInitInteractiveShells? Or do you think their name is fine to show
> a sort of "single instance shells vs. everything else" kind of
> relationship?
> 

I'd just use InteractiveShell. I'm leaning towards not starting the 
application right away since we won't be able to use the context manager 
when needed. And the workflow would be the same for InteractiveShell and 
SingleActiveInteractiveShell - you first create the instance and then 
connect separately (and with InteractiveShell, we'd have two options).

As for the name, you're free to find a better name, 
SingleActiveInteractiveShell is fine, but maybe we can do better.

>>
>>>>
>>>>> +    implement the exact same functionality with the primary difference being how the application
>>>>> +    is started and stopped. In contrast to normal interactive shells, this class does not start the
>>>>> +    application upon initialization of the class. Instead, the application is handled through a
>>>>> +    context manager. This allows for more explicit starting and stopping of the application, and
>>>>> +    more guarantees for when the application is cleaned up which are not present with normal
>>>>> +    interactive shells that get cleaned up upon garbage collection.
>>>>> +    """
>>>>> +
>>>>> +    _get_privileged_command: Callable[[str], str] | None
>>>>> +
>>>>> +    def __init__(
>>>>> +        self,
>>>>> +        interactive_session: SSHClient,
>>>>> +        logger: DTSLogger,
>>>>> +        get_privileged_command: Callable[[str], str] | None,
>>>>> +        app_args: str = "",
>>>>> +        timeout: float = SETTINGS.timeout,
>>>>> +    ) -> None > +        """Store parameters for creating an interactive shell, but
>>>> do not start the application.
>>>>> +
>>>>> +        Note that this method also does not create the channel for the application, as this is
>>>>> +        something that isn't needed until the application starts.
>>>>> +
>>>>> +        Args:
>>>>> +            interactive_session: The SSH session dedicated to interactive shells.
>>>>> +            logger: The logger instance this session will use.
>>>>> +            get_privileged_command: A method for modifying a command to allow it to use
>>>>> +                elevated privileges. If :data:`None`, the application will not be started
>>>>> +                with elevated privileges.
>>>>> +            app_args: The command line arguments to be passed to the application on startup.
>>>>> +            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. The default
>>>>> +                value for this argument may be modified using the :option:`--timeout` command-line
>>>>> +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
>>>>> +        """
>>>>> +        self._interactive_session = interactive_session
>>>>> +        self._logger = logger
>>>>> +        self._timeout = timeout
>>>>> +        self._app_args = app_args
>>>>> +        self._get_privileged_command = get_privileged_command
>>>>
>>>> We see here why it's backwards. We're duplicating this part of the code
>>>> and if the class relation is the other way around we can just call
>>>> super().__init__().
>>>
>>> I agree, this method does make it seem a little backwards.
>>>
>>>>
>>>>> +
>>>>> +    def __enter__(self) -> Self:
>>> <snip>
>>>>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>>>>> index 284412e82c..ca30aac264 100644
>>>>> --- a/dts/framework/remote_session/testpmd_shell.py
>>>>> +++ b/dts/framework/remote_session/testpmd_shell.py
>>>>
>>>>> @@ -253,6 +253,15 @@ def get_capas_rxq(
>>>>>                     else:
>>>>>                         unsupported_capabilities.add(NicCapability.scattered_rx)
>>>>>
>>>>> +    def __exit__(self, *_) -> None:
>>>>> +        """Overrides :meth:`~.critical_interactive_shell.CriticalInteractiveShell.__exit__`.
>>>>> +
>>>>> +        Ensures that when the context is exited packet forwarding is stopped before closing the
>>>>> +        application.
>>>>> +        """
>>>>> +        self.stop()
>>>>> +        super().__exit__()
>>>>> +
>>>>
>>>> I think it would more sense to add this to self.close().
>>>
>>> Ack.
>>>
>>>>
>>>>>
>>>>>     class NicCapability(Enum):
>>>>>         """A mapping between capability names and the associated :class:`TestPmdShell` methods.
>>>>
>>>>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
>>>>> index 3701c47408..41f6090a7e 100644
>>>>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
>>>>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
>>> <snip>
>>>>>                     "--mbcache=200 "
>>>>> @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
>>>>>                 ),
>>>>>                 privileged=True,
>>>>>             )
>>>>> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
>>>>> -        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.set_forward_mode(TestPmdForwardingModes.mac)
>>>>> +            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}.",
>>>>> +                )
>>>>> +            testpmd.stop()
>>>>
>>>> This is now not needed since you added this to __exit__(), right?
>>>
>>> Right, we don't need it here. I left it just because I like being a
>>> little more explicit, but I can remove it since it is just an unneeded
>>> extra line.
>>>
>>
>> Not just an extra line, but unnecessary (and possibly confusing) logs
>> when doing it for the second time.
> 
> True, this probably is a little strange to see twice if you don't
> understand why.
> 
> 
>>
>>>>
>>>> But we should consider removing this (stopping forwarding) altogether
>>>> since you mentioned we don't really need this. I'm not sure what it adds
>>>> or what the rationale is - testpmd is going to handle this just fine,
>>>> right? And we're not doing any other cleanup, we're leaving all of that
>>>> to testpmd.
>>>
>>> I don't think we should remove it entirely, there is something
>>> beneficial that can come from explicitly stopping forwarding. When the
>>> method returns None (like it does now) I agree that it is useless, but
>>> when you stop forwarding it prints the statistics for each port. I
>>> modified the stop method in another series that isn't out yet actually
>>> for adding another test suite and use its output for validation.
>>>
>>
>> Oh, that sounds great. Any extra info like this is great for debugging,
>> let's definitely keep it then.
>>
>>>
>>>>
>>>>>
>>>>>         def test_scatter_mbuf_2048(self) -> None:
>>>>>             """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
>>>>

[PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell

[jspewock]

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

There are methods within DTS currently that support updating the MTU of
ports on a node, but the methods for doing this in a linux session rely
on the ip command and the port being bound to the kernel driver. Since
test suites are run while bound to the driver for DPDK, there needs to
be a way to modify the value while bound to said driver as well. This is
done by using testpmd to modify the MTU.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 .../remote_session/interactive_shell.py       | 14 +++-
 dts/framework/remote_session/testpmd_shell.py | 76 ++++++++++++++++++-
 2 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 6dee7ebce0..34d1acf439 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -139,7 +139,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
             raise InteractiveCommandExecutionError("Failed to start application.")
         self._ssh_channel.settimeout(self._timeout)
 
-    def send_command(self, command: str, prompt: str | None = None) -> str:
+    def send_command(
+        self, command: str, prompt: str | None = None, print_to_debug: 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
@@ -155,6 +157,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
             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.
+            print_to_debug: If :data:`True` the logging message that displays what command is
+                being sent prior to sending it will be logged at the debug level instead of the
+                info level. Useful when a single action requires multiple commands to complete to
+                avoid clutter in the logs.
 
         Returns:
             All output in the buffer before expected string.
@@ -163,7 +169,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
             raise InteractiveCommandExecutionError(
                 f"Cannot send command {command} to application because the shell is not running."
             )
-        self._logger.info(f"Sending: '{command}'")
+        log_message = f"Sending: '{command}'"
+        if print_to_debug:
+            self._logger.debug(log_message)
+        else:
+            self._logger.info(log_message)
         if prompt is None:
             prompt = self._default_prompt
         self._stdin.write(f"{command}{self._command_extra_chars}\n")
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index ca30aac264..f2fa842b7f 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -135,10 +135,11 @@ def start(self, verify: bool = True) -> None:
             InteractiveCommandExecutionError: If `verify` is :data:`True` and forwarding fails to
                 start or ports fail to come up.
         """
-        self.send_command("start")
+        self._logger.info('Starting packet forwarding and waiting for port links to be "up".')
+        self.send_command("start", print_to_debug=True)
         if verify:
             # If forwarding was already started, sending "start" again should tell us
-            start_cmd_output = self.send_command("start")
+            start_cmd_output = self.send_command("start", print_to_debug=True)
             if "Packet forwarding already started" not in start_cmd_output:
                 self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}")
                 raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.")
@@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def _stop_port(self, port_id: int, verify: bool = True) -> None:
+        """Stop port `port_id` in testpmd.
+
+        Depending on the PMD, the port may need to be stopped before configuration can take place.
+        This method wraps the command needed to properly stop ports and take their link down.
+
+        Args:
+            port_id: ID of the port to take down.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                stopping of ports was successful. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
+                successfully stop.
+        """
+        stop_port_output = self.send_command(f"port stop {port_id}", print_to_debug=True)
+        if verify and ("Done" not in stop_port_output):
+            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
+
+    def _start_port(self, port_id: int, verify: bool = True) -> None:
+        """Start port `port_id` in testpmd.
+
+        Because the port may need to be stopped to make some configuration changes, it naturally
+        follows that it will need to be started again once those changes have been made.
+
+        Args:
+            port_id: ID of the port to start.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                port came back up without error. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
+                back up.
+        """
+        start_port_output = self.send_command(f"port start {port_id}", print_to_debug=True)
+        if verify and ("Done" not in start_port_output):
+            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
+
+    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
+        """Change the MTU of a port using testpmd.
+
+        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
+        stop the port before configuring in cases where it isn't required, so we first stop ports,
+        then update the MTU, then start the ports again afterwards.
+
+        Args:
+            port_id: ID of the port to adjust the MTU on.
+            mtu: Desired value for the MTU to be set to.
+            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
+                verify that the mtu was properly set on the port. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
+                properly updated on the port matching `port_id`.
+        """
+        self._logger.info(f"Changing MTU of port {port_id} to be {mtu}")
+        self._stop_port(port_id, verify)
+        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}", print_to_debug=True)
+        self._start_port(port_id, verify)
+        if verify and (
+            f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}", print_to_debug=True)
+        ):
+            self._logger.debug(
+                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
+            )
+            raise InteractiveCommandExecutionError(
+                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
+            )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "Bye...")
-- 
2.45.1

Re: [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell

[Juraj Linkeš]

On 5. 6. 2024 23:31, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> There are methods within DTS currently that support updating the MTU of
> ports on a node, but the methods for doing this in a linux session rely
> on the ip command and the port being bound to the kernel driver. Since
> test suites are run while bound to the driver for DPDK, there needs to
> be a way to modify the value while bound to said driver as well. This is
> done by using testpmd to modify the MTU.
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>   .../remote_session/interactive_shell.py       | 14 +++-
>   dts/framework/remote_session/testpmd_shell.py | 76 ++++++++++++++++++-
>   2 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 6dee7ebce0..34d1acf439 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -139,7 +139,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>               raise InteractiveCommandExecutionError("Failed to start application.")
>           self._ssh_channel.settimeout(self._timeout)
>   
> -    def send_command(self, command: str, prompt: str | None = None) -> str:
> +    def send_command(
> +        self, command: str, prompt: str | None = None, print_to_debug: bool = False
> +    ) -> str:

As I mentioned in v2, this really should be in a separate patch, as it 
affects other parts of the code and the solution should be designed with 
that in mind.

>           """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
> @@ -155,6 +157,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>               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.
> +            print_to_debug: If :data:`True` the logging message that displays what command is
> +                being sent prior to sending it will be logged at the debug level instead of the
> +                info level. Useful when a single action requires multiple commands to complete to
> +                avoid clutter in the logs.
>   
>           Returns:
>               All output in the buffer before expected string.
> @@ -163,7 +169,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>               raise InteractiveCommandExecutionError(
>                   f"Cannot send command {command} to application because the shell is not running."
>               )
> -        self._logger.info(f"Sending: '{command}'")
> +        log_message = f"Sending: '{command}'"
> +        if print_to_debug:
> +            self._logger.debug(log_message)
> +        else:
> +            self._logger.info(log_message)
>           if prompt is None:
>               prompt = self._default_prompt
>           self._stdin.write(f"{command}{self._command_extra_chars}\n")
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ca30aac264..f2fa842b7f 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -135,10 +135,11 @@ def start(self, verify: bool = True) -> None:
>               InteractiveCommandExecutionError: If `verify` is :data:`True` and forwarding fails to
>                   start or ports fail to come up.
>           """
> -        self.send_command("start")
> +        self._logger.info('Starting packet forwarding and waiting for port links to be "up".')
> +        self.send_command("start", print_to_debug=True)
>           if verify:
>               # If forwarding was already started, sending "start" again should tell us
> -            start_cmd_output = self.send_command("start")
> +            start_cmd_output = self.send_command("start", print_to_debug=True)
>               if "Packet forwarding already started" not in start_cmd_output:
>                   self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}")
>                   raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.")
> @@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>                   f"Test pmd failed to set fwd mode to {mode.value}"
>               )
>   
> +    def _stop_port(self, port_id: int, verify: bool = True) -> None:
> +        """Stop port `port_id` in testpmd.

Either "Stop `port_id` in testpmd." or "Stop port with `port_id` in 
testpmd.". I like the latter more.

> +
> +        Depending on the PMD, the port may need to be stopped before configuration can take place.
> +        This method wraps the command needed to properly stop ports and take their link down.
> +
> +        Args:
> +            port_id: ID of the port to take down.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                stopping of ports was successful. Defaults to True.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
> +                successfully stop.
> +        """
> +        stop_port_output = self.send_command(f"port stop {port_id}", print_to_debug=True)
> +        if verify and ("Done" not in stop_port_output):
> +            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
> +            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
> +
> +    def _start_port(self, port_id: int, verify: bool = True) -> None:
> +        """Start port `port_id` in testpmd.
> +
> +        Because the port may need to be stopped to make some configuration changes, it naturally
> +        follows that it will need to be started again once those changes have been made.
> +
> +        Args:
> +            port_id: ID of the port to start.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                port came back up without error. Defaults to True.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
> +                back up.
> +        """
> +        start_port_output = self.send_command(f"port start {port_id}", print_to_debug=True)
> +        if verify and ("Done" not in start_port_output):
> +            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
> +            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
> +
> +    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
> +        """Change the MTU of a port using testpmd.
> +
> +        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
> +        stop the port before configuring in cases where it isn't required, so we first stop ports,
> +        then update the MTU, then start the ports again afterwards.
> +
> +        Args:
> +            port_id: ID of the port to adjust the MTU on.
> +            mtu: Desired value for the MTU to be set to.
> +            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
> +                verify that the mtu was properly set on the port. Defaults to True.

The second instance of True should also be :data:`True`.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
> +                properly updated on the port matching `port_id`.
> +        """
> +        self._logger.info(f"Changing MTU of port {port_id} to be {mtu}")
> +        self._stop_port(port_id, verify)
> +        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}", print_to_debug=True)
> +        self._start_port(port_id, verify)

Would making _stop_port and _start_port a decorator work? Can we do the 
verification even if the port is stopped?

> +        if verify and (
> +            f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}", print_to_debug=True)
> +        ):
> +            self._logger.debug(
> +                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
> +            )
> +            raise InteractiveCommandExecutionError(
> +                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
> +            )
> +
>       def _close(self) -> None:
>           """Overrides :meth:`~.interactive_shell.close`."""
>           self.send_command("quit", "Bye...")

Re: [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell

[Jeremy Spewock]

On Mon, Jun 10, 2024 at 11:03 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 5. 6. 2024 23:31, jspewock@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > There are methods within DTS currently that support updating the MTU of
> > ports on a node, but the methods for doing this in a linux session rely
> > on the ip command and the port being bound to the kernel driver. Since
> > test suites are run while bound to the driver for DPDK, there needs to
> > be a way to modify the value while bound to said driver as well. This is
> > done by using testpmd to modify the MTU.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
> >   .../remote_session/interactive_shell.py       | 14 +++-
> >   dts/framework/remote_session/testpmd_shell.py | 76 ++++++++++++++++++-
> >   2 files changed, 86 insertions(+), 4 deletions(-)
> >
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index 6dee7ebce0..34d1acf439 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -139,7 +139,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >               raise InteractiveCommandExecutionError("Failed to start application.")
> >           self._ssh_channel.settimeout(self._timeout)
> >
> > -    def send_command(self, command: str, prompt: str | None = None) -> str:
> > +    def send_command(
> > +        self, command: str, prompt: str | None = None, print_to_debug: bool = False
> > +    ) -> str:
>
> As I mentioned in v2, this really should be in a separate patch, as it
> affects other parts of the code and the solution should be designed with
> that in mind.

Ack.

>
> >           """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
> > @@ -155,6 +157,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >               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.
> > +            print_to_debug: If :data:`True` the logging message that displays what command is
> > +                being sent prior to sending it will be logged at the debug level instead of the
> > +                info level. Useful when a single action requires multiple commands to complete to
> > +                avoid clutter in the logs.
> >
> >           Returns:
> >               All output in the buffer before expected string.
> > @@ -163,7 +169,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >               raise InteractiveCommandExecutionError(
> >                   f"Cannot send command {command} to application because the shell is not running."
> >               )
> > -        self._logger.info(f"Sending: '{command}'")
> > +        log_message = f"Sending: '{command}'"
> > +        if print_to_debug:
> > +            self._logger.debug(log_message)
> > +        else:
> > +            self._logger.info(log_message)
> >           if prompt is None:
> >               prompt = self._default_prompt
> >           self._stdin.write(f"{command}{self._command_extra_chars}\n")
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> > index ca30aac264..f2fa842b7f 100644
> > --- a/dts/framework/remote_session/testpmd_shell.py
> > +++ b/dts/framework/remote_session/testpmd_shell.py
> > @@ -135,10 +135,11 @@ def start(self, verify: bool = True) -> None:
> >               InteractiveCommandExecutionError: If `verify` is :data:`True` and forwarding fails to
> >                   start or ports fail to come up.
> >           """
> > -        self.send_command("start")
> > +        self._logger.info('Starting packet forwarding and waiting for port links to be "up".')
> > +        self.send_command("start", print_to_debug=True)
> >           if verify:
> >               # If forwarding was already started, sending "start" again should tell us
> > -            start_cmd_output = self.send_command("start")
> > +            start_cmd_output = self.send_command("start", print_to_debug=True)
> >               if "Packet forwarding already started" not in start_cmd_output:
> >                   self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}")
> >                   raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.")
> > @@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
> >                   f"Test pmd failed to set fwd mode to {mode.value}"
> >               )
> >
> > +    def _stop_port(self, port_id: int, verify: bool = True) -> None:
> > +        """Stop port `port_id` in testpmd.
>
> Either "Stop `port_id` in testpmd." or "Stop port with `port_id` in
> testpmd.". I like the latter more.

Ack.

>
> > +
> > +        Depending on the PMD, the port may need to be stopped before configuration can take place.
> > +        This method wraps the command needed to properly stop ports and take their link down.
> > +
> > +        Args:
> > +            port_id: ID of the port to take down.
> > +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> > +                stopping of ports was successful. Defaults to True.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
> > +                successfully stop.
> > +        """
> > +        stop_port_output = self.send_command(f"port stop {port_id}", print_to_debug=True)
> > +        if verify and ("Done" not in stop_port_output):
> > +            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
> > +            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
> > +
> > +    def _start_port(self, port_id: int, verify: bool = True) -> None:
> > +        """Start port `port_id` in testpmd.
> > +
> > +        Because the port may need to be stopped to make some configuration changes, it naturally
> > +        follows that it will need to be started again once those changes have been made.
> > +
> > +        Args:
> > +            port_id: ID of the port to start.
> > +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> > +                port came back up without error. Defaults to True.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
> > +                back up.
> > +        """
> > +        start_port_output = self.send_command(f"port start {port_id}", print_to_debug=True)
> > +        if verify and ("Done" not in start_port_output):
> > +            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
> > +            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
> > +
> > +    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
> > +        """Change the MTU of a port using testpmd.
> > +
> > +        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
> > +        stop the port before configuring in cases where it isn't required, so we first stop ports,
> > +        then update the MTU, then start the ports again afterwards.
> > +
> > +        Args:
> > +            port_id: ID of the port to adjust the MTU on.
> > +            mtu: Desired value for the MTU to be set to.
> > +            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
> > +                verify that the mtu was properly set on the port. Defaults to True.
>
> The second instance of True should also be :data:`True`.

Ugh, good catch! My IDE generates boilerplate for them for me and I
caught this in another method I added here, but I guess I missed this
one.

>
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
> > +                properly updated on the port matching `port_id`.
> > +        """
> > +        self._logger.info(f"Changing MTU of port {port_id} to be {mtu}")
> > +        self._stop_port(port_id, verify)
> > +        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}", print_to_debug=True)
> > +        self._start_port(port_id, verify)
>
> Would making _stop_port and _start_port a decorator work? Can we do the
> verification even if the port is stopped?

I like this idea a lot. I just checked and it looks like you can do
the validation while the port is stopped, so I'll make this change.


>
> > +        if verify and (
> > +            f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}", print_to_debug=True)
> > +        ):
> > +            self._logger.debug(
> > +                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
> > +            )
> > +            raise InteractiveCommandExecutionError(
> > +                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
> > +            )
> > +
> >       def _close(self) -> None:
> >           """Overrides :meth:`~.interactive_shell.close`."""
> >           self.send_command("quit", "Bye...")

[PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[jspewock]

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

Some NICs tested in DPDK allow for the scattering of packets without an
offload and others enforce that you enable the scattered_rx offload in
testpmd. The current version of the suite for testing support of
scattering packets only tests the case where the NIC supports testing
without the offload, so an expansion of coverage is needed to cover the
second case as well.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 77 ++++++++++++++++-------
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 41f6090a7e..76eabb51f6 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -16,14 +16,19 @@
 """
 
 import struct
+from typing import ClassVar
 
 from scapy.layers.inet import IP  # type: ignore[import]
 from scapy.layers.l2 import Ether  # type: ignore[import]
-from scapy.packet import Raw  # type: ignore[import]
+from scapy.packet import Raw, Packet  # type: ignore[import]
 from scapy.utils import hexstr  # type: ignore[import]
 
-from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
-from framework.test_suite import TestSuite
+from framework.remote_session.testpmd_shell import (
+    NicCapability,
+    TestPmdForwardingModes,
+    TestPmdShell,
+)
+from framework.test_suite import TestSuite, requires
 
 
 class TestPmdBufferScatter(TestSuite):
@@ -48,6 +53,14 @@ class TestPmdBufferScatter(TestSuite):
        and a single byte of packet data stored in a second buffer alongside the CRC.
     """
 
+    #: Parameters for testing scatter using testpmd which are universal across all test cases.
+    base_testpmd_parameters: ClassVar[list[str]] = [
+        "--mbcache=200",
+        "--max-pkt-len=9000",
+        "--port-topology=paired",
+        "--tx-offloads=0x00008000",
+    ]
+
     def set_up_suite(self) -> None:
         """Set up the test suite.
 
@@ -64,19 +77,19 @@ def set_up_suite(self) -> None:
         self.tg_node.main_session.configure_port_mtu(9000, self._tg_port_egress)
         self.tg_node.main_session.configure_port_mtu(9000, self._tg_port_ingress)
 
-    def scatter_pktgen_send_packet(self, pktsize: int) -> str:
+    def scatter_pktgen_send_packet(self, pktsize: int) -> list[Packet]:
         """Generate and send a packet to the SUT then capture what is forwarded back.
 
         Generate an IP packet of a specific length and send it to the SUT,
-        then capture the resulting received packet and extract its payload.
-        The desired length of the packet is met by packing its payload
+        then capture the resulting received packets and filter them down to the ones that have the
+        correct layers. The desired length of the packet is met by packing its payload
         with the letter "X" in hexadecimal.
 
         Args:
             pktsize: Size of the packet to generate and send.
 
         Returns:
-            The payload of the received packet as a string.
+            The filtered down list of packets.
         """
         packet = Ether() / IP() / Raw()
         packet.getlayer(2).load = ""
@@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
         for X_in_hex in payload:
             packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
         received_packets = self.send_packet_and_capture(packet)
+        # filter down the list to packets that have the appropriate structure
+        received_packets = list(
+            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
+        )
         self.verify(len(received_packets) > 0, "Did not receive any packets.")
-        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
 
-        return load
+        return received_packets
 
-    def pmd_scatter(self, mbsize: int) -> None:
+    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
         """Testpmd support of receiving and sending scattered multi-segment packets.
 
         Support for scattered packets is shown by sending 5 packets of differing length
@@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
         """
         testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=(
-                "--mbcache=200 "
-                f"--mbuf-size={mbsize} "
-                "--max-pkt-len=9000 "
-                "--port-topology=paired "
-                "--tx-offloads=0x00008000"
-            ),
+            app_parameters=" ".join(testpmd_params),
             privileged=True,
         )
         with testpmd_shell as testpmd:
             testpmd.set_forward_mode(TestPmdForwardingModes.mac)
+            # adjust the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 9000)
             testpmd.start()
 
             for offset in [-1, 0, 1, 4, 5]:
-                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
+                # This list should only ever contain one element
+                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
                 self._logger.debug(
-                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
+                    f"Relevant captured packets: \n{recv_packets}"
                 )
+
                 self.verify(
-                    ("58 " * 8).strip() in recv_payload,
+                    any(
+                        " ".join(["58"]*8) in hexstr(pakt.getlayer(2), onlyhex=1)
+                        for pakt in recv_packets
+                    ),
                     "Payload of scattered packet did not match expected payload with offset "
                     f"{offset}.",
                 )
             testpmd.stop()
+            # reset the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 1500)
 
+    @requires(NicCapability.scattered_rx)
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
-        self.pmd_scatter(mbsize=2048)
+        self.pmd_scatter(
+            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
+        )
+
+    def test_scatter_mbuf_2048_with_offload(self) -> None:
+        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
+        self.pmd_scatter(
+            mbsize=2048,
+            testpmd_params=[
+                *(self.base_testpmd_parameters),
+                "--mbuf-size=2048",
+                "--enable-scatter",
+            ],
+        )
 
     def tear_down_suite(self) -> None:
         """Tear down the test suite.
-- 
2.45.1

Re: [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Juraj Linkeš]

> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 41f6090a7e..76eabb51f6 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py

> @@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
>           for X_in_hex in payload:
>               packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
>           received_packets = self.send_packet_and_capture(packet)
> +        # filter down the list to packets that have the appropriate structure
> +        received_packets = list(
> +            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
> +        )
>           self.verify(len(received_packets) > 0, "Did not receive any packets.")
> -        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
>   
> -        return load
> +        return received_packets
>   
> -    def pmd_scatter(self, mbsize: int) -> None:
> +    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:

Since base_testpmd_parameters is a class var, the method is always going 
to have access to it and we only need to pass the extra parameters. 
There's not much of a point in passing what's common to all tests to 
this method, as it should contain the common parts.

>           """Testpmd support of receiving and sending scattered multi-segment packets.
>   
>           Support for scattered packets is shown by sending 5 packets of differing length
> @@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
>           """
>           testpmd_shell = self.sut_node.create_interactive_shell(
>               TestPmdShell,
> -            app_parameters=(
> -                "--mbcache=200 "
> -                f"--mbuf-size={mbsize} "
> -                "--max-pkt-len=9000 "
> -                "--port-topology=paired "
> -                "--tx-offloads=0x00008000"
> -            ),
> +            app_parameters=" ".join(testpmd_params),
>               privileged=True,
>           )
>           with testpmd_shell as testpmd:
>               testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> +            # adjust the MTU of the SUT ports
> +            for port_id in range(testpmd.number_of_ports):
> +                testpmd.set_port_mtu(port_id, 9000)
>               testpmd.start()
>   
>               for offset in [-1, 0, 1, 4, 5]:
> -                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> +                # This list should only ever contain one element

Which list is the comment referring to? recv_packets? There could be 
more than just one packet, right?

> +                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
>                   self._logger.debug(
> -                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
> +                    f"Relevant captured packets: \n{recv_packets}"
>                   )
> +
>                   self.verify(
> -                    ("58 " * 8).strip() in recv_payload,
> +                    any(
> +                        " ".join(["58"]*8) in hexstr(pakt.getlayer(2), onlyhex=1)
> +                        for pakt in recv_packets
> +                    ),
>                       "Payload of scattered packet did not match expected payload with offset "
>                       f"{offset}.",
>                   )
>               testpmd.stop()
> +            # reset the MTU of the SUT ports
> +            for port_id in range(testpmd.number_of_ports):
> +                testpmd.set_port_mtu(port_id, 1500)
>   
> +    @requires(NicCapability.scattered_rx)
>       def test_scatter_mbuf_2048(self) -> None:
>           """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> -        self.pmd_scatter(mbsize=2048)
> +        self.pmd_scatter(
> +            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
> +        )
> +

I'm curious why you moved the --mbuf-size parameter here. It's always 
going to be (or should be) equal to mbsize, which we already pass (and 
now we're essentially passing the same thing twice), so I feel this just 
creates opportunities for mistakes.

> +    def test_scatter_mbuf_2048_with_offload(self) -> None:
> +        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
> +        self.pmd_scatter(
> +            mbsize=2048,
> +            testpmd_params=[
> +                *(self.base_testpmd_parameters),
> +                "--mbuf-size=2048",
> +                "--enable-scatter",
> +            ],
> +        )
>   
>       def tear_down_suite(self) -> None:
>           """Tear down the test suite.

Re: [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Jeremy Spewock]

On Mon, Jun 10, 2024 at 11:22 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
> > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > index 41f6090a7e..76eabb51f6 100644
> > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
>
> > @@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
> >           for X_in_hex in payload:
> >               packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
> >           received_packets = self.send_packet_and_capture(packet)
> > +        # filter down the list to packets that have the appropriate structure
> > +        received_packets = list(
> > +            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
> > +        )
> >           self.verify(len(received_packets) > 0, "Did not receive any packets.")
> > -        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
> >
> > -        return load
> > +        return received_packets
> >
> > -    def pmd_scatter(self, mbsize: int) -> None:
> > +    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
>
> Since base_testpmd_parameters is a class var, the method is always going
> to have access to it and we only need to pass the extra parameters.
> There's not much of a point in passing what's common to all tests to
> this method, as it should contain the common parts.

Ack.

>
> >           """Testpmd support of receiving and sending scattered multi-segment packets.
> >
> >           Support for scattered packets is shown by sending 5 packets of differing length
> > @@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
> >           """
> >           testpmd_shell = self.sut_node.create_interactive_shell(
> >               TestPmdShell,
> > -            app_parameters=(
> > -                "--mbcache=200 "
> > -                f"--mbuf-size={mbsize} "
> > -                "--max-pkt-len=9000 "
> > -                "--port-topology=paired "
> > -                "--tx-offloads=0x00008000"
> > -            ),
> > +            app_parameters=" ".join(testpmd_params),
> >               privileged=True,
> >           )
> >           with testpmd_shell as testpmd:
> >               testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > +            # adjust the MTU of the SUT ports
> > +            for port_id in range(testpmd.number_of_ports):
> > +                testpmd.set_port_mtu(port_id, 9000)
> >               testpmd.start()
> >
> >               for offset in [-1, 0, 1, 4, 5]:
> > -                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> > +                # This list should only ever contain one element
>
> Which list is the comment referring to? recv_packets? There could be
> more than just one packet, right?

There technically could be in very strange cases, but this change also
adds a filter to `scatter_pktgen_send_packet()` that filters the list
before it is returned here. I imagine there wouldn't be (and in my
testing there aren't) any other packets that have the structure
Ether() / IP() / Raw() getting sent by anything on the wire, so I just
noted it to make it more clear that the call to `any()` probably isn't
going to have to consume much. I did the filtering in the other method
because I wanted to be able to distinguish between getting nothing,
and getting something that has the right structure but not the right
payload (as, presumably, if this test were to fail it would be shown
in the payload).

>
<snip>
> > +    @requires(NicCapability.scattered_rx)
> >       def test_scatter_mbuf_2048(self) -> None:
> >           """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> > -        self.pmd_scatter(mbsize=2048)
> > +        self.pmd_scatter(
> > +            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
> > +        )
> > +
>
> I'm curious why you moved the --mbuf-size parameter here. It's always
> going to be (or should be) equal to mbsize, which we already pass (and
> now we're essentially passing the same thing twice), so I feel this just
> creates opportunities for mistakes.

Honestly, when it's phrased like that, I have no good reason at all,
haha. I just put it there because I got stuck in some mentality of
"testpmd parameters go in this list, so it has to go here", but it did
feel weird to hardcode the same value twice like that. I'll adjust
this.


>
> > +    def test_scatter_mbuf_2048_with_offload(self) -> None:
> > +        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
> > +        self.pmd_scatter(
> > +            mbsize=2048,
> > +            testpmd_params=[
> > +                *(self.base_testpmd_parameters),
> > +                "--mbuf-size=2048",
> > +                "--enable-scatter",
> > +            ],
> > +        )
> >
> >       def tear_down_suite(self) -> None:
> >           """Tear down the test suite.

Re: [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Juraj Linkeš]

On 10. 6. 2024 22:08, Jeremy Spewock wrote:
> On Mon, Jun 10, 2024 at 11:22 AM Juraj Linkeš
> <juraj.linkes@pantheon.tech> wrote:
>>
>>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
>>> index 41f6090a7e..76eabb51f6 100644
>>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
>>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
>>
>>> @@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
>>>            for X_in_hex in payload:
>>>                packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
>>>            received_packets = self.send_packet_and_capture(packet)
>>> +        # filter down the list to packets that have the appropriate structure
>>> +        received_packets = list(
>>> +            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
>>> +        )
>>>            self.verify(len(received_packets) > 0, "Did not receive any packets.")
>>> -        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
>>>
>>> -        return load
>>> +        return received_packets
>>>
>>> -    def pmd_scatter(self, mbsize: int) -> None:
>>> +    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
>>
>> Since base_testpmd_parameters is a class var, the method is always going
>> to have access to it and we only need to pass the extra parameters.
>> There's not much of a point in passing what's common to all tests to
>> this method, as it should contain the common parts.
> 
> Ack.
> 
>>
>>>            """Testpmd support of receiving and sending scattered multi-segment packets.
>>>
>>>            Support for scattered packets is shown by sending 5 packets of differing length
>>> @@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
>>>            """
>>>            testpmd_shell = self.sut_node.create_interactive_shell(
>>>                TestPmdShell,
>>> -            app_parameters=(
>>> -                "--mbcache=200 "
>>> -                f"--mbuf-size={mbsize} "
>>> -                "--max-pkt-len=9000 "
>>> -                "--port-topology=paired "
>>> -                "--tx-offloads=0x00008000"
>>> -            ),
>>> +            app_parameters=" ".join(testpmd_params),
>>>                privileged=True,
>>>            )
>>>            with testpmd_shell as testpmd:
>>>                testpmd.set_forward_mode(TestPmdForwardingModes.mac)
>>> +            # adjust the MTU of the SUT ports
>>> +            for port_id in range(testpmd.number_of_ports):
>>> +                testpmd.set_port_mtu(port_id, 9000)
>>>                testpmd.start()
>>>
>>>                for offset in [-1, 0, 1, 4, 5]:
>>> -                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
>>> +                # This list should only ever contain one element
>>
>> Which list is the comment referring to? recv_packets? There could be
>> more than just one packet, right?
> 
> There technically could be in very strange cases, but this change also
> adds a filter to `scatter_pktgen_send_packet()` that filters the list
> before it is returned here. I imagine there wouldn't be (and in my
> testing there aren't) any other packets that have the structure
> Ether() / IP() / Raw() getting sent by anything on the wire, so I just
> noted it to make it more clear that the call to `any()` probably isn't
> going to have to consume much. I did the filtering in the other method
> because I wanted to be able to distinguish between getting nothing,
> and getting something that has the right structure but not the right
> payload (as, presumably, if this test were to fail it would be shown
> in the payload).
> 

Right, but maybe in other setups this won't be true. We can just make 
the comment say the list contains filtered packets with the expected 
structure, as that would be more in line with the verification code 
(where we don't assume it's just one packet).

>>
> <snip>
>>> +    @requires(NicCapability.scattered_rx)
>>>        def test_scatter_mbuf_2048(self) -> None:
>>>            """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
>>> -        self.pmd_scatter(mbsize=2048)
>>> +        self.pmd_scatter(
>>> +            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
>>> +        )
>>> +
>>
>> I'm curious why you moved the --mbuf-size parameter here. It's always
>> going to be (or should be) equal to mbsize, which we already pass (and
>> now we're essentially passing the same thing twice), so I feel this just
>> creates opportunities for mistakes.
> 
> Honestly, when it's phrased like that, I have no good reason at all,
> haha. I just put it there because I got stuck in some mentality of
> "testpmd parameters go in this list, so it has to go here", but it did
> feel weird to hardcode the same value twice like that. I'll adjust
> this.
> 
> 
>>
>>> +    def test_scatter_mbuf_2048_with_offload(self) -> None:
>>> +        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
>>> +        self.pmd_scatter(
>>> +            mbsize=2048,
>>> +            testpmd_params=[
>>> +                *(self.base_testpmd_parameters),
>>> +                "--mbuf-size=2048",
>>> +                "--enable-scatter",
>>> +            ],
>>> +        )
>>>
>>>        def tear_down_suite(self) -> None:
>>>            """Tear down the test suite.

Re: [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Jeremy Spewock]

On Tue, Jun 11, 2024 at 5:22 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 10. 6. 2024 22:08, Jeremy Spewock wrote:
> > On Mon, Jun 10, 2024 at 11:22 AM Juraj Linkeš
> > <juraj.linkes@pantheon.tech> wrote:
> >>
> >>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> >>> index 41f6090a7e..76eabb51f6 100644
> >>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> >>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> >>
> >>> @@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
> >>>            for X_in_hex in payload:
> >>>                packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
> >>>            received_packets = self.send_packet_and_capture(packet)
> >>> +        # filter down the list to packets that have the appropriate structure
> >>> +        received_packets = list(
> >>> +            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
> >>> +        )
> >>>            self.verify(len(received_packets) > 0, "Did not receive any packets.")
> >>> -        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
> >>>
> >>> -        return load
> >>> +        return received_packets
> >>>
> >>> -    def pmd_scatter(self, mbsize: int) -> None:
> >>> +    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:
> >>
> >> Since base_testpmd_parameters is a class var, the method is always going
> >> to have access to it and we only need to pass the extra parameters.
> >> There's not much of a point in passing what's common to all tests to
> >> this method, as it should contain the common parts.
> >
> > Ack.
> >
> >>
> >>>            """Testpmd support of receiving and sending scattered multi-segment packets.
> >>>
> >>>            Support for scattered packets is shown by sending 5 packets of differing length
> >>> @@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
> >>>            """
> >>>            testpmd_shell = self.sut_node.create_interactive_shell(
> >>>                TestPmdShell,
> >>> -            app_parameters=(
> >>> -                "--mbcache=200 "
> >>> -                f"--mbuf-size={mbsize} "
> >>> -                "--max-pkt-len=9000 "
> >>> -                "--port-topology=paired "
> >>> -                "--tx-offloads=0x00008000"
> >>> -            ),
> >>> +            app_parameters=" ".join(testpmd_params),
> >>>                privileged=True,
> >>>            )
> >>>            with testpmd_shell as testpmd:
> >>>                testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> >>> +            # adjust the MTU of the SUT ports
> >>> +            for port_id in range(testpmd.number_of_ports):
> >>> +                testpmd.set_port_mtu(port_id, 9000)
> >>>                testpmd.start()
> >>>
> >>>                for offset in [-1, 0, 1, 4, 5]:
> >>> -                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> >>> +                # This list should only ever contain one element
> >>
> >> Which list is the comment referring to? recv_packets? There could be
> >> more than just one packet, right?
> >
> > There technically could be in very strange cases, but this change also
> > adds a filter to `scatter_pktgen_send_packet()` that filters the list
> > before it is returned here. I imagine there wouldn't be (and in my
> > testing there aren't) any other packets that have the structure
> > Ether() / IP() / Raw() getting sent by anything on the wire, so I just
> > noted it to make it more clear that the call to `any()` probably isn't
> > going to have to consume much. I did the filtering in the other method
> > because I wanted to be able to distinguish between getting nothing,
> > and getting something that has the right structure but not the right
> > payload (as, presumably, if this test were to fail it would be shown
> > in the payload).
> >
>
> Right, but maybe in other setups this won't be true. We can just make
> the comment say the list contains filtered packets with the expected
> structure, as that would be more in line with the verification code
> (where we don't assume it's just one packet).

That's fair, it's probably better to be more clear here, I'll update this.


>
> >>
> > <snip>
> >>> +    @requires(NicCapability.scattered_rx)
> >>>        def test_scatter_mbuf_2048(self) -> None:
> >>>            """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> >>> -        self.pmd_scatter(mbsize=2048)
> >>> +        self.pmd_scatter(
> >>> +            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
> >>> +        )
> >>> +
> >>
> >> I'm curious why you moved the --mbuf-size parameter here. It's always
> >> going to be (or should be) equal to mbsize, which we already pass (and
> >> now we're essentially passing the same thing twice), so I feel this just
> >> creates opportunities for mistakes.
> >
> > Honestly, when it's phrased like that, I have no good reason at all,
> > haha. I just put it there because I got stuck in some mentality of
> > "testpmd parameters go in this list, so it has to go here", but it did
> > feel weird to hardcode the same value twice like that. I'll adjust
> > this.
> >
> >
> >>
> >>> +    def test_scatter_mbuf_2048_with_offload(self) -> None:
> >>> +        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
> >>> +        self.pmd_scatter(
> >>> +            mbsize=2048,
> >>> +            testpmd_params=[
> >>> +                *(self.base_testpmd_parameters),
> >>> +                "--mbuf-size=2048",
> >>> +                "--enable-scatter",
> >>> +            ],
> >>> +        )
> >>>
> >>>        def tear_down_suite(self) -> None:
> >>>            """Tear down the test suite.

[PATCH v4 0/4] Add second scatter test case

[jspewock]

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

v4:
 * comment and formatting adjustments based on feedback on previous
   version
 * switch class inheritance order for interactive shells and singe
   active interactive shells
 * method decorator for starting and stopping ports in testpmd shell
   that gets used for the update MTU method.

Jeremy Spewock (4):
  dts: add context manager for interactive shells
  dts: improve starting and stopping interactive shells
  dts: add methods for modifying MTU to testpmd shell
  dts: add test case that utilizes offload to pmd_buffer_scatter

 dts/framework/remote_session/__init__.py      |   1 +
 .../remote_session/interactive_shell.py       | 153 +++---------
 .../single_active_interactive_shell.py        | 218 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py | 113 ++++++++-
 dts/framework/testbed_model/os_session.py     |   4 +-
 dts/framework/testbed_model/sut_node.py       |   8 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  94 +++++---
 dts/tests/TestSuite_smoke_tests.py            |   3 +-
 9 files changed, 427 insertions(+), 169 deletions(-)
 create mode 100644 dts/framework/remote_session/single_active_interactive_shell.py

-- 
2.45.1

[PATCH v4 1/4] 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.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/__init__.py      |   1 +
 .../remote_session/interactive_shell.py       | 146 ++------------
 .../single_active_interactive_shell.py        | 188 ++++++++++++++++++
 dts/framework/remote_session/testpmd_shell.py |   9 +-
 dts/framework/testbed_model/os_session.py     |   4 +-
 dts/framework/testbed_model/sut_node.py       |   8 +-
 .../testbed_model/traffic_generator/scapy.py  |   2 +
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  27 +--
 dts/tests/TestSuite_smoke_tests.py            |   3 +-
 9 files changed, 233 insertions(+), 155 deletions(-)
 create mode 100644 dts/framework/remote_session/single_active_interactive_shell.py

diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
index f18a9f2259..81839410b9 100644
--- a/dts/framework/remote_session/__init__.py
+++ b/dts/framework/remote_session/__init__.py
@@ -21,6 +21,7 @@
 from .interactive_shell import InteractiveShell
 from .python_shell import PythonShell
 from .remote_session import CommandResult, RemoteSession
+from .single_active_interactive_shell import SingleActiveInteractiveShell
 from .ssh_session import SSHSession
 from .testpmd_shell import NicCapability, TestPmdShell
 
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..9d124b8245 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -1,149 +1,31 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 University of New Hampshire
 
-"""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 Callable, ClassVar
-
-from paramiko import Channel, SSHClient, channel  # type: ignore[import]
-
-from framework.logger import DTSLogger
-from framework.settings import SETTINGS
+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.
     """
 
-    _interactive_session: SSHClient
-    _stdin: channel.ChannelStdinFile
-    _stdout: channel.ChannelFile
-    _ssh_channel: Channel
-    _logger: DTSLogger
-    _timeout: float
-    _app_args: str
-
-    #: 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]
-
-    #: Whether this application is a DPDK app. If it is, the build directory
-    #: for DPDK on the node will be prepended to the path to the executable.
-    dpdk_app: ClassVar[bool] = False
-
-    def __init__(
-        self,
-        interactive_session: SSHClient,
-        logger: DTSLogger,
-        get_privileged_command: Callable[[str], str] | None,
-        app_args: str = "",
-        timeout: float = SETTINGS.timeout,
-    ) -> None:
-        """Create an SSH channel during initialization.
-
-        Args:
-            interactive_session: The SSH session dedicated to interactive shells.
-            logger: The logger instance this session will use.
-            get_privileged_command: A method for modifying a command to allow it to use
-                elevated privileges. If :data:`None`, the application will not be started
-                with elevated privileges.
-            app_args: The command line arguments to be passed to the application on startup.
-            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.
-        """
-        self._interactive_session = interactive_session
-        self._ssh_channel = self._interactive_session.invoke_shell()
-        self._stdin = self._ssh_channel.makefile_stdin("w")
-        self._stdout = self._ssh_channel.makefile("r")
-        self._ssh_channel.settimeout(timeout)
-        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
-        self._logger = logger
-        self._timeout = timeout
-        self._app_args = app_args
-        self._start_application(get_privileged_command)
-
-    def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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.
-
-        Args:
-            get_privileged_command: A function (but could be any callable) that produces
-                the version of the command with elevated privileges.
-        """
-        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)
-
-    def send_command(self, command: str, prompt: str | None = None) -> 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.
-
-        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:
-            out += line
-            if prompt in line and not line.rstrip().endswith(
-                command.rstrip()
-            ):  # ignore line that sent command
-                break
-        self._logger.debug(f"Got output: {out}")
-        return out
+    def start_application(self) -> None:
+        """Start the application."""
+        self._start_application(self._get_privileged_command)
 
     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."""
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..74060be8a7
--- /dev/null
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -0,0 +1,188 @@
+# 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 Callable, ClassVar
+
+from paramiko import Channel, SSHClient, channel  # type: ignore[import]
+from typing_extensions import Self
+
+from framework.exception import InteractiveCommandExecutionError
+from framework.logger import DTSLogger
+from framework.settings import SETTINGS
+
+
+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.
+    """
+
+    _interactive_session: SSHClient
+    _stdin: channel.ChannelStdinFile
+    _stdout: channel.ChannelFile
+    _ssh_channel: Channel
+    _logger: DTSLogger
+    _timeout: float
+    _app_args: str
+    _get_privileged_command: Callable[[str], str] | None
+
+    #: 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]
+
+    #: Whether this application is a DPDK app. If it is, the build directory
+    #: for DPDK on the node will be prepended to the path to the executable.
+    dpdk_app: ClassVar[bool] = False
+
+    def __init__(
+        self,
+        interactive_session: SSHClient,
+        logger: DTSLogger,
+        get_privileged_command: Callable[[str], str] | None,
+        app_args: str = "",
+        timeout: float = SETTINGS.timeout,
+    ) -> None:
+        """Create an SSH channel during initialization.
+
+        Args:
+            interactive_session: The SSH session dedicated to interactive shells.
+            logger: The logger instance this session will use.
+            get_privileged_command: A method for modifying a command to allow it to use
+                elevated privileges. If :data:`None`, the application will not be started
+                with elevated privileges.
+            app_args: The command line arguments to be passed to the application on startup.
+            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.
+        """
+        self._interactive_session = interactive_session
+        self._logger = logger
+        self._timeout = timeout
+        self._app_args = app_args
+        self._get_privileged_command = get_privileged_command
+
+    def _init_channel(self):
+        self._ssh_channel = self._interactive_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 _start_application(self, get_privileged_command: Callable[[str], str] | None) -> 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. A new SSH channel is initialized for the application to run on, then the
+        application is started.
+
+        Args:
+            get_privileged_command: A function (but could be any callable) that produces
+                the version of the command with elevated privileges.
+        """
+        self._init_channel()
+        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)
+
+    def send_command(self, command: str, prompt: str | None = None) -> 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.
+
+        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:
+            out += line
+            if prompt in line and not line.rstrip().endswith(
+                command.rstrip()
+            ):  # ignore line that sent command
+                break
+        self._logger.debug(f"Got output: {out}")
+        return out
+
+    def _close(self) -> None:
+        self._stdin.close()
+        self._ssh_channel.close()
+
+    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(self._get_privileged_command)
+        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 f6783af621..17561d4dae 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -26,7 +26,7 @@
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
-from .interactive_shell import InteractiveShell
+from .single_active_interactive_shell import SingleActiveInteractiveShell
 
 
 class TestPmdDevice(object):
@@ -82,7 +82,7 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
-class TestPmdShell(InteractiveShell):
+class TestPmdShell(SingleActiveInteractiveShell):
     """Testpmd interactive shell.
 
     The testpmd shell users should never use
@@ -227,10 +227,11 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
-    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()
 
     def get_capas_rxq(
         self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
index d5bf7e0401..745f0317f8 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/dts/framework/testbed_model/os_session.py
@@ -32,8 +32,8 @@
 from framework.remote_session import (
     CommandResult,
     InteractiveRemoteSession,
-    InteractiveShell,
     RemoteSession,
+    SingleActiveInteractiveShell,
     create_interactive_session,
     create_remote_session,
 )
@@ -43,7 +43,7 @@
 from .cpu import LogicalCore
 from .port import Port
 
-InteractiveShellType = TypeVar("InteractiveShellType", bound=InteractiveShell)
+InteractiveShellType = TypeVar("InteractiveShellType", bound=SingleActiveInteractiveShell)
 
 
 class OSSession(ABC):
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 1fb536735d..7dd39fd735 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -243,10 +243,10 @@ def get_supported_capabilities(
         unsupported_capas: set[NicCapability] = set()
         self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
         testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
-        for capability in capabilities:
-            if capability not in supported_capas or capability not in unsupported_capas:
-                capability.value(testpmd_shell, supported_capas, unsupported_capas)
-        del testpmd_shell
+        with testpmd_shell as running_testpmd:
+            for capability in capabilities:
+                if capability not in supported_capas or capability not in unsupported_capas:
+                    capability.value(running_testpmd, supported_capas, unsupported_capas)
         return supported_capas
 
     def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
index df3069d516..ba3a56df79 100644
--- a/dts/framework/testbed_model/traffic_generator/scapy.py
+++ b/dts/framework/testbed_model/traffic_generator/scapy.py
@@ -221,6 +221,8 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
             PythonShell, 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 3701c47408..645a66b607 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -101,7 +101,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         Test:
             Start testpmd and run functional test with preset mbsize.
         """
-        testpmd = self.sut_node.create_interactive_shell(
+        testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
             app_parameters=(
                 "--mbcache=200 "
@@ -112,17 +112,20 @@ def pmd_scatter(self, mbsize: int) -> None:
             ),
             privileged=True,
         )
-        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
-        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.set_forward_mode(TestPmdForwardingModes.mac)
+            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 a553e89662..360e64eb5a 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 = self.sut_node.create_interactive_shell(TestPmdShell, privileged=True)
-        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.1

Re: [PATCH v4 1/4] dts: add context manager for interactive shells

[Juraj Linkeš]

On 13. 6. 2024 20:15, 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.
> 
> depends-on: patch-139227 ("dts: skip test cases based on capabilities")
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>

Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

[PATCH v4 2/4] 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>
---
 .../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

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

[Juraj Linkeš]

> @@ -15,18 +18,34 @@ class InteractiveShell(SingleActiveInteractiveShell):

> +    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)

I think we can just add the above line to start_application() to achieve 
the same thing. And we should move the docstring to the public method.

> +
>       def start_application(self) -> None:
>           """Start the application."""
>           self._start_application(self._get_privileged_command)
>   

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

[Jeremy Spewock]

On Tue, Jun 18, 2024 at 11:54 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
> > @@ -15,18 +18,34 @@ class InteractiveShell(SingleActiveInteractiveShell):
>
> > +    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)
>
> I think we can just add the above line to start_application() to achieve
> the same thing. And we should move the docstring to the public method.

Sure, makes sense to me, we only need the finalizer when we start
manually anyway, there's no need to set it up when you use it as a
context manager. Actually, I wonder if this would throw an exception
at the time of garbage collection if you used an InteractiveShell as a
context manager. I think it might because the context manager doesn't
trigger the finalizer, so it probably would try to clean up twice.

Good catch!

>
> > +
> >       def start_application(self) -> None:
> >           """Start the application."""
> >           self._start_application(self._get_privileged_command)
> >

[PATCH v4 3/4] dts: add methods for modifying MTU to testpmd shell

[jspewock]

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

There are methods within DTS currently that support updating the MTU of
ports on a node, but the methods for doing this in a linux session rely
on the ip command and the port being bound to the kernel driver. Since
test suites are run while bound to the driver for DPDK, there needs to
be a way to modify the value while bound to said driver as well. This is
done by using testpmd to modify the MTU.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 102 +++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 805bb3a77d..09f80cb250 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -20,7 +20,7 @@
 from enum import Enum, auto
 from functools import partial
 from pathlib import PurePath
-from typing import Callable, ClassVar
+from typing import Any, Callable, ClassVar
 
 from framework.exception import InteractiveCommandExecutionError
 from framework.settings import SETTINGS
@@ -82,6 +82,39 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
+def stop_then_start_port_decorator(
+    func: Callable[["TestPmdShell", int, Any, bool], None]
+) -> Callable[["TestPmdShell", int, Any, bool], None]:
+    """Decorator that stops a port, runs decorated function, then starts the port.
+
+    The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
+    port ID (as an int) as its first parameter and has a "verify" parameter (as a bool) as its last
+    parameter. The port ID and verify parameters will be passed into
+    :meth:`TestPmdShell._stop_port` so that the correct port is stopped/started and verification
+    takes place if desired.
+
+    Args:
+        func: The function to run while the port is stopped.
+
+    Returns:
+        Wrapper function that stops a port, runs the decorated function, then starts the port.
+    """
+
+    def wrapper(shell: "TestPmdShell", port_id: int, *args, **kwargs) -> None:
+        """Function that wraps the instance method of :class:`TestPmdShell`.
+
+        Args:
+            shell: Instance of the shell containing the method to decorate.
+            port_id: ID of the port to stop/start.
+        """
+        verify_value = kwargs["verify"] if "verify" in kwargs else args[-1]
+        shell._stop_port(port_id, verify_value)
+        func(shell, port_id, *args, **kwargs)
+        shell._start_port(port_id, verify_value)
+
+    return wrapper
+
+
 class TestPmdShell(SingleActiveInteractiveShell):
     """Testpmd interactive shell.
 
@@ -227,6 +260,73 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def _stop_port(self, port_id: int, verify: bool = True) -> None:
+        """Stop port with `port_id` in testpmd.
+
+        Depending on the PMD, the port may need to be stopped before configuration can take place.
+        This method wraps the command needed to properly stop ports and take their link down.
+
+        Args:
+            port_id: ID of the port to take down.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                stopping of ports was successful. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
+                successfully stop.
+        """
+        stop_port_output = self.send_command(f"port stop {port_id}")
+        if verify and ("Done" not in stop_port_output):
+            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
+
+    def _start_port(self, port_id: int, verify: bool = True) -> None:
+        """Start port `port_id` in testpmd.
+
+        Because the port may need to be stopped to make some configuration changes, it naturally
+        follows that it will need to be started again once those changes have been made.
+
+        Args:
+            port_id: ID of the port to start.
+            verify: If :data:`True` the output will be scanned in an attempt to verify that the
+                port came back up without error. Defaults to True.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
+                back up.
+        """
+        start_port_output = self.send_command(f"port start {port_id}")
+        if verify and ("Done" not in start_port_output):
+            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
+            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
+
+    @stop_then_start_port_decorator
+    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
+        """Change the MTU of a port using testpmd.
+
+        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
+        stop the port before configuring in cases where it isn't required, so we first stop ports,
+        then update the MTU, then start the ports again afterwards.
+
+        Args:
+            port_id: ID of the port to adjust the MTU on.
+            mtu: Desired value for the MTU to be set to.
+            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
+                verify that the mtu was properly set on the port. Defaults to :data:`True`.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
+                properly updated on the port matching `port_id`.
+        """
+        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}")
+        if verify and (f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}")):
+            self._logger.debug(
+                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
+            )
+            raise InteractiveCommandExecutionError(
+                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
+            )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.45.1

Re: [PATCH v4 3/4] dts: add methods for modifying MTU to testpmd shell

[Juraj Linkeš]

> +def stop_then_start_port_decorator(

The name shouldn't contain "decorator". Just the docstring should 
mention it's a decorator.

> +    func: Callable[["TestPmdShell", int, Any, bool], None]

I'm thinking about this type. Sounds like there are too many conditions 
that need to be satisfied. The problem is with the verify parameter. Do 
we actually need it? If we're decorating a function, that may imply we 
always want to verify the port stop/start. In which circumstance we 
wouldn't want to verify that (the decorated function would need to not 
verify what it's doing, but what function would that be and would we 
actually want to not verify the port stop/start even then)?

The requirement of port_id is fine, as this only work with ports (so 
port_id must be somewhere among the parameters) and it being the first 
is also fine, but we should document it. A good place seems to be the 
class docstring, somewhere around "If there isn't one that satisfies a 
need, it should be added.".

> +) -> Callable[["TestPmdShell", int, Any, bool], None]:
> +    """Decorator that stops a port, runs decorated function, then starts the port.
> +
> +    The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
> +    port ID (as an int) as its first parameter and has a "verify" parameter (as a bool) as its last
> +    parameter. The port ID and verify parameters will be passed into
> +    :meth:`TestPmdShell._stop_port` so that the correct port is stopped/started and verification
> +    takes place if desired.
> +
> +    Args:
> +        func: The function to run while the port is stopped.

The description of required argument should probably be here (maybe just 
here, but could also be above).

> +
> +    Returns:
> +        Wrapper function that stops a port, runs the decorated function, then starts the port.

This would be the function that's already been wrapped, right?


> +    def _start_port(self, port_id: int, verify: bool = True) -> None:
> +        """Start port `port_id` in testpmd.

with `port_id`

> +
> +        Because the port may need to be stopped to make some configuration changes, it naturally
> +        follows that it will need to be started again once those changes have been made.
> +
> +        Args:
> +            port_id: ID of the port to start.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                port came back up without error. Defaults to True.

The second True is not marked as :data: (also in the other method).
A note on docstrings of private members: we don't need to be as detailed 
as these are not rendered. We should still provide some docs if those 
would be helpful for developers (but don't need to document everything 
for people using the API).

Re: [PATCH v4 3/4] dts: add methods for modifying MTU to testpmd shell

[Jeremy Spewock]

On Wed, Jun 19, 2024 at 4:16 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
> > +def stop_then_start_port_decorator(
>
> The name shouldn't contain "decorator". Just the docstring should
> mention it's a decorator.

Ack.

>
> > +    func: Callable[["TestPmdShell", int, Any, bool], None]
>
> I'm thinking about this type. Sounds like there are too many conditions
> that need to be satisfied. The problem is with the verify parameter. Do
> we actually need it? If we're decorating a function, that may imply we
> always want to verify the port stop/start. In which circumstance we
> wouldn't want to verify that (the decorated function would need to not
> verify what it's doing, but what function would that be and would we
> actually want to not verify the port stop/start even then)?

I agree the parameter requirements are a little clunky and I played
with them for a while, but this one seemed the most "correct" to me.
We could create a policy that any method that is decorated with this
function must verify the port stopping and starting worked, but if we
did that I think you would have to also add to the policy that the
method being decorated must also always verify it was successful. I
don't think it makes sense to call a function and specify that you
don't want to verify success, and then still verify some component of
what the function is doing (starting and stopping ports in this case).
I think it would be fine to have this as a policy, but it's slightly
more limiting for users than other methods that testpmd shell offers.

I don't really know of an example when you wouldn't want to verify any
of the methods in TestpmdShell other than in case the developer is
expecting it to fail or it might not matter to the test if it was
successful or not for some reason. Considering we are already
following the path of optionally verifying all methods that can be
verified in the testpmd shell anyway, I didn't see the verify boolean
as anything extra for the methods to really have. We could instead
change to always verifying everything, but a change like that probably
doesn't fit the scope of this patch.

>
> The requirement of port_id is fine, as this only work with ports (so
> port_id must be somewhere among the parameters) and it being the first
> is also fine, but we should document it. A good place seems to be the
> class docstring, somewhere around "If there isn't one that satisfies a
> need, it should be added.".

That's a good point, I'll add some more notes about it.

>
> > +) -> Callable[["TestPmdShell", int, Any, bool], None]:
> > +    """Decorator that stops a port, runs decorated function, then starts the port.
> > +
> > +    The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
> > +    port ID (as an int) as its first parameter and has a "verify" parameter (as a bool) as its last
> > +    parameter. The port ID and verify parameters will be passed into
> > +    :meth:`TestPmdShell._stop_port` so that the correct port is stopped/started and verification
> > +    takes place if desired.
> > +
> > +    Args:
> > +        func: The function to run while the port is stopped.
>
> The description of required argument should probably be here (maybe just
> here, but could also be above).

Ack.

>
> > +
> > +    Returns:
> > +        Wrapper function that stops a port, runs the decorated function, then starts the port.
>
> This would be the function that's already been wrapped, right?

I was trying to convey that this returns the function that wraps the
decorated function so I called it the "wrapper function", but maybe my
wording was a little confusing.

>
>
> > +    def _start_port(self, port_id: int, verify: bool = True) -> None:
> > +        """Start port `port_id` in testpmd.
>
> with `port_id`

Ack.

>
> > +
> > +        Because the port may need to be stopped to make some configuration changes, it naturally
> > +        follows that it will need to be started again once those changes have been made.
> > +
> > +        Args:
> > +            port_id: ID of the port to start.
> > +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> > +                port came back up without error. Defaults to True.
>
> The second True is not marked as :data: (also in the other method).
> A note on docstrings of private members: we don't need to be as detailed
> as these are not rendered. We should still provide some docs if those
> would be helpful for developers (but don't need to document everything
> for people using the API).

Right, I noticed that in some places they were less formal than others
and figured it was because they were private. I was sticking with the
general API layout regardless just as I felt it would be better to
have more information than necessary rather than less, but I'll keep
this in mind and not include some of the more redundant things like
the args in this case.

>

[PATCH v4 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[jspewock]

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

Some NICs tested in DPDK allow for the scattering of packets without an
offload and others enforce that you enable the scattered_rx offload in
testpmd. The current version of the suite for testing support of
scattering packets only tests the case where the NIC supports testing
without the offload, so an expansion of coverage is needed to cover the
second case as well.

depends-on: patch-139227 ("dts: skip test cases based on capabilities")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 75 ++++++++++++++++-------
 1 file changed, 53 insertions(+), 22 deletions(-)

diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 645a66b607..f7bdd4fbcf 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -16,14 +16,19 @@
 """
 
 import struct
+from typing import ClassVar
 
 from scapy.layers.inet import IP  # type: ignore[import]
 from scapy.layers.l2 import Ether  # type: ignore[import]
-from scapy.packet import Raw  # type: ignore[import]
+from scapy.packet import Packet, Raw  # type: ignore[import]
 from scapy.utils import hexstr  # type: ignore[import]
 
-from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
-from framework.test_suite import TestSuite
+from framework.remote_session.testpmd_shell import (
+    NicCapability,
+    TestPmdForwardingModes,
+    TestPmdShell,
+)
+from framework.test_suite import TestSuite, requires
 
 
 class TestPmdBufferScatter(TestSuite):
@@ -48,6 +53,14 @@ class TestPmdBufferScatter(TestSuite):
        and a single byte of packet data stored in a second buffer alongside the CRC.
     """
 
+    #: Parameters for testing scatter using testpmd which are universal across all test cases.
+    base_testpmd_parameters: ClassVar[list[str]] = [
+        "--mbcache=200",
+        "--max-pkt-len=9000",
+        "--port-topology=paired",
+        "--tx-offloads=0x00008000",
+    ]
+
     def set_up_suite(self) -> None:
         """Set up the test suite.
 
@@ -64,19 +77,19 @@ def set_up_suite(self) -> None:
         self.tg_node.main_session.configure_port_mtu(9000, self._tg_port_egress)
         self.tg_node.main_session.configure_port_mtu(9000, self._tg_port_ingress)
 
-    def scatter_pktgen_send_packet(self, pktsize: int) -> str:
+    def scatter_pktgen_send_packet(self, pktsize: int) -> list[Packet]:
         """Generate and send a packet to the SUT then capture what is forwarded back.
 
         Generate an IP packet of a specific length and send it to the SUT,
-        then capture the resulting received packet and extract its payload.
-        The desired length of the packet is met by packing its payload
+        then capture the resulting received packets and filter them down to the ones that have the
+        correct layers. The desired length of the packet is met by packing its payload
         with the letter "X" in hexadecimal.
 
         Args:
             pktsize: Size of the packet to generate and send.
 
         Returns:
-            The payload of the received packet as a string.
+            The filtered down list of received packets.
         """
         packet = Ether() / IP() / Raw()
         packet.getlayer(2).load = ""
@@ -86,51 +99,69 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
         for X_in_hex in payload:
             packet.load += struct.pack("=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16))
         received_packets = self.send_packet_and_capture(packet)
+        # filter down the list to packets that have the appropriate structure
+        received_packets = list(
+            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
+        )
         self.verify(len(received_packets) > 0, "Did not receive any packets.")
-        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
 
-        return load
+        return received_packets
 
-    def pmd_scatter(self, mbsize: int) -> None:
+    def pmd_scatter(self, mbsize: int, extra_testpmd_params: list[str] = []) -> None:
         """Testpmd support of receiving and sending scattered multi-segment packets.
 
         Support for scattered packets is shown by sending 5 packets of differing length
         where the length of the packet is calculated by taking mbuf-size + an offset.
         The offsets used in the test are -1, 0, 1, 4, 5 respectively.
 
+        Args:
+            mbsize: Size to set memory buffers to when starting testpmd.
+            extra_testpmd_params: Additional parameters to add to the base list when starting
+                testpmd.
+
         Test:
-            Start testpmd and run functional test with preset mbsize.
+            Start testpmd and run functional test with preset `mbsize`.
         """
         testpmd_shell = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=(
-                "--mbcache=200 "
-                f"--mbuf-size={mbsize} "
-                "--max-pkt-len=9000 "
-                "--port-topology=paired "
-                "--tx-offloads=0x00008000"
+            app_parameters=" ".join(
+                [*self.base_testpmd_parameters, f"--mbuf-size={mbsize}", *extra_testpmd_params]
             ),
             privileged=True,
         )
         with testpmd_shell as testpmd:
             testpmd.set_forward_mode(TestPmdForwardingModes.mac)
+            # adjust the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 9000)
             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}"
-                )
+                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
+                self._logger.debug(f"Relevant captured packets: \n{recv_packets}")
+
                 self.verify(
-                    ("58 " * 8).strip() in recv_payload,
+                    any(
+                        " ".join(["58"] * 8) in hexstr(pakt.getlayer(2), onlyhex=1)
+                        for pakt in recv_packets
+                    ),
                     "Payload of scattered packet did not match expected payload with offset "
                     f"{offset}.",
                 )
+            testpmd.stop()
+            # reset the MTU of the SUT ports
+            for port_id in range(testpmd.number_of_ports):
+                testpmd.set_port_mtu(port_id, 1500)
 
+    @requires(NicCapability.scattered_rx)
     def test_scatter_mbuf_2048(self) -> None:
         """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
         self.pmd_scatter(mbsize=2048)
 
+    def test_scatter_mbuf_2048_with_offload(self) -> None:
+        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
+        self.pmd_scatter(mbsize=2048, extra_testpmd_params=["--enable-scatter"])
+
     def tear_down_suite(self) -> None:
         """Tear down the test suite.
 
-- 
2.45.1

Re: [PATCH v4 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Juraj Linkeš]

> -    def scatter_pktgen_send_packet(self, pktsize: int) -> str:
> +    def scatter_pktgen_send_packet(self, pktsize: int) -> list[Packet]:

A note: We should make this method a part of TestSuite (so that we have 
a common way to filter packets across all test suites) in a separate 
patchset as part of https://bugs.dpdk.org/show_bug.cgi?id=1438.

>           """Generate and send a packet to the SUT then capture what is forwarded back.
>   
>           Generate an IP packet of a specific length and send it to the SUT,
> -        then capture the resulting received packet and extract its payload.
> -        The desired length of the packet is met by packing its payload
> +        then capture the resulting received packets and filter them down to the ones that have the
> +        correct layers. The desired length of the packet is met by packing its payload
>           with the letter "X" in hexadecimal.
>   
>           Args:
>               pktsize: Size of the packet to generate and send.
>   
>           Returns:
> -            The payload of the received packet as a string.
> +            The filtered down list of received packets.
>           """

<snip>

>           with testpmd_shell as testpmd:
>               testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> +            # adjust the MTU of the SUT ports
> +            for port_id in range(testpmd.number_of_ports):
> +                testpmd.set_port_mtu(port_id, 9000)

For a second I thought about maybe somehow using the decorator from the 
previous patch, but that only works with testpmd methods.

But then I thought about us setting this multiple times (twice (9000, 
then back to 1500) in each test case) and that a "better" place to put 
this would be set_up_suite() (and tear_down_suite()), but that has a 
major downside of starting testpmd two more times. Having it all in one 
place in set_up_suite() would surely make the whole test suite more 
understandable, but starting testpmd multiple times is not ideal. Maybe 
we have to do it like in this patch.

I also noticed that we don't really document why we're setting MTU to 
9000. The relation between MTU and mbuf size (I think that relation is 
the reason, correct me if I'm wrong) should be better documented, 
probably in set_up_suite().

>               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}"
> -                )
> +                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
> +                self._logger.debug(f"Relevant captured packets: \n{recv_packets}")
> +
>                   self.verify(
> -                    ("58 " * 8).strip() in recv_payload,
> +                    any(
> +                        " ".join(["58"] * 8) in hexstr(pakt.getlayer(2), onlyhex=1)
> +                        for pakt in recv_packets
> +                    ),
>                       "Payload of scattered packet did not match expected payload with offset "
>                       f"{offset}.",
>                   )
> +            testpmd.stop()

This sneaked right back in.

Re: [PATCH v4 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

[Jeremy Spewock]

On Wed, Jun 19, 2024 at 4:51 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
> > -    def scatter_pktgen_send_packet(self, pktsize: int) -> str:
> > +    def scatter_pktgen_send_packet(self, pktsize: int) -> list[Packet]:
>
> A note: We should make this method a part of TestSuite (so that we have
> a common way to filter packets across all test suites) in a separate
> patchset as part of https://bugs.dpdk.org/show_bug.cgi?id=1438.

That's a good idea.

>
> >           """Generate and send a packet to the SUT then capture what is forwarded back.
> >
> >           Generate an IP packet of a specific length and send it to the SUT,
> > -        then capture the resulting received packet and extract its payload.
> > -        The desired length of the packet is met by packing its payload
> > +        then capture the resulting received packets and filter them down to the ones that have the
> > +        correct layers. The desired length of the packet is met by packing its payload
> >           with the letter "X" in hexadecimal.
> >
> >           Args:
> >               pktsize: Size of the packet to generate and send.
> >
> >           Returns:
> > -            The payload of the received packet as a string.
> > +            The filtered down list of received packets.
> >           """
>
> <snip>
>
> >           with testpmd_shell as testpmd:
> >               testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > +            # adjust the MTU of the SUT ports
> > +            for port_id in range(testpmd.number_of_ports):
> > +                testpmd.set_port_mtu(port_id, 9000)
>
> For a second I thought about maybe somehow using the decorator from the
> previous patch, but that only works with testpmd methods.
>
> But then I thought about us setting this multiple times (twice (9000,
> then back to 1500) in each test case) and that a "better" place to put
> this would be set_up_suite() (and tear_down_suite()), but that has a
> major downside of starting testpmd two more times. Having it all in one
> place in set_up_suite() would surely make the whole test suite more
> understandable, but starting testpmd multiple times is not ideal. Maybe
> we have to do it like in this patch.

Right, I ended up putting it here just because the shell was already
started here so it was convenient, but setting the MTU and resetting
it multiple times is also definitely not ideal. I'm not really sure of
exactly the best way to handle it either unfortunately. Something else
I could do is have my own boolean that just tracks if the MTU has been
updated yet and only do it the first time, but then there would have
to be some kind of way to track which case is the last one to run
which is also a whole can of worms. I think overall the cost of
switching MTUs more than we need to is less than that of starting
testpmd 2 extra times with only these two test cases, but if more are
added it could end up being the opposite.

As a note though, from what I have recently seen while testing this,
this change of MTU seems like it is generally needed when you are
bound to the kernel driver while running DPDK instead of vfio-pci. One
of the parameters that is passed into testpmd in this suite is
--max-pkt-len and this adjusts the MTU of the ports before starting
testpmd. However, since some NICs use the kernel driver as their
driver for DPDK as well, this is not sufficient in all cases since the
MTU of the kernel interface is not updated by this parameter and the
packets still get dropped.  So, for example, if you start testpmd with
a Mellanox NIC bound to mlx5_core and the parameter
--max-pkt-len=9000, the MTU of the port when you do a `show port info
0` will be 8982, but if you do an `ip a` command you will see that the
network interface still shows an MTU value of 1500 and the packets
will be dropped if they exceed the MTU set on the network interface.
In all cases the MTU must be higher than 2048, so I set it using
testpmd to be agnostic of which driver you are bound to, as long as it
is a DPDK driver.

I'm not sure if this is a bug or intentional because of something that
blocks the updating of the network interface for some reason, but it
might be worth mentioning to testpmd/ethdev maintainers regardless and
I can raise it to them. If the `--max-pkt-len` parameter did update
this MTU or always allowed receiving traffic at that size then we
would not need to set the MTU in any test cases and it would be
handled by testpmd on startup. In the meantime, there has to be this
manual adjustment of MTU for the test cases to pass on any NIC that
runs DPDK on its kernel driver.

>
> I also noticed that we don't really document why we're setting MTU to
> 9000. The relation between MTU and mbuf size (I think that relation is
> the reason, correct me if I'm wrong) should be better documented,
> probably in set_up_suite().

It isn't as much to do with the relation to the mbuf size as much as
it is to test the scattering of packets you have to send and receive
packets that are greater than that mbuf size so we have to increase
the MTU to transmit those packets. Testpmd can run with the given
parameters (--mbuf-size=2048, --max-pkt-len=9000, or both together)
without the MTU change, but as I alluded to above, the MTU in testpmd
isn't always true to what the network interface says it is.

>
> >               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}"
> > -                )
> > +                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
> > +                self._logger.debug(f"Relevant captured packets: \n{recv_packets}")
> > +
> >                   self.verify(
> > -                    ("58 " * 8).strip() in recv_payload,
> > +                    any(
> > +                        " ".join(["58"] * 8) in hexstr(pakt.getlayer(2), onlyhex=1)
> > +                        for pakt in recv_packets
> > +                    ),
> >                       "Payload of scattered packet did not match expected payload with offset "
> >                       f"{offset}.",
> >                   )
> > +            testpmd.stop()
>
> This sneaked right back in.

It did, but this time it actually is needed. With the MTU of ports
being reset back to 1500 at the end of the test, we have to stop
packet forwarding first so that the individual ports can be stopped
for modification of their MTUs.