From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6D7DC44098; Wed, 22 May 2024 15:53:55 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 06B54402E0; Wed, 22 May 2024 15:53:55 +0200 (CEST) Received: from mail-oo1-f52.google.com (mail-oo1-f52.google.com [209.85.161.52]) by mails.dpdk.org (Postfix) with ESMTP id 4678F400D6 for ; Wed, 22 May 2024 15:53:53 +0200 (CEST) Received: by mail-oo1-f52.google.com with SMTP id 006d021491bc7-5b295d6b7fbso2819284eaf.0 for ; Wed, 22 May 2024 06:53:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1716386032; x=1716990832; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=4Xzpvf0ItaldnEEWtMcMhYWLdd33QDxqXviSzHynkhk=; b=dxHc1jzq82H+NtPEjUErkHwyV/SixWbZCDNwq1AsXfYyqJKHGjMP3ItAaEdwxfufd0 OMt2sAdb6pQVT3rzEg1MDPecis2Ix1fIVKBS548MbRL67zKbF4zPQQf0iQZXgQB5xHT+ KISRUv+TDzf0/4LUuol/zISiR/YYZrTEK0sJQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716386032; x=1716990832; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4Xzpvf0ItaldnEEWtMcMhYWLdd33QDxqXviSzHynkhk=; b=mc2nsrEnMGucoa9ucvlwTywkVn9b7Y+BiiuWsMC83XLE1dSQUzwa7fM1z9HNYBgSeN lB5hu+Pq2QH8Vvn8kQChFxZCfxf4cuuPpCmkxhoNSDPnuIlANI334zJSxprjkY2vtG5P MEXG3PgaGMRM3j8fC9j0rlcESLqzhc95hZjy1J3zPvDfDf2GlK/z/Zx8jwcGE+JtrNR0 /csAyXqawrPu5O4e3RY7COEO7hC+xD/m6/mKKbJ4pZyW4yW2rL6AxNY/GHazQpxaY2r7 +HR7khXlKjXz7D7kIIuRRHsfFjZU+uutUGRkChNaMFmKt5iTJC4vl/t85lyxY/S5/OQx emlg== X-Forwarded-Encrypted: i=1; AJvYcCUomGc46S6ZkJ53V4gUzineVA/pr0utIxwRqPn87fbEoD9b+Dy8hHpObk7aLCspq2IUqWA60Q1+OPNsVlg= X-Gm-Message-State: AOJu0YxlVXXpiHutKLtAvSm1CzrFqwrxUEnHRj4Xb2n+kMiUZKx+2BKh Z0WjvLCzqKTq7QOJK5bzwG8S8gbxx967a5ZUttgMAnEgMwhY5pX3xGQ9As4KDDZZgBER7sZscoS Rns+17clIfmuryzFCAkgrIs99XFseQ9nDACMtdQ== X-Google-Smtp-Source: AGHT+IFVMWNG63yKNyfhUoxM31RBV4OFokjd2yRTDS4uOm0RLrhZdRJYGo/UOLrXn4Yo8NJwIEluoYxcbC5mdFehhUA= X-Received: by 2002:a05:6820:1ad0:b0:5b2:8256:7f3d with SMTP id 006d021491bc7-5b6a0e081d6mr2213562eaf.1.1716386032302; Wed, 22 May 2024 06:53:52 -0700 (PDT) MIME-Version: 1.0 References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240514201436.2496-3-jspewock@iol.unh.edu> In-Reply-To: <20240514201436.2496-3-jspewock@iol.unh.edu> From: Patrick Robb Date: Wed, 22 May 2024 09:53:41 -0400 Message-ID: Subject: Re: [PATCH v1 2/4] dts: add context manager for interactive shells To: jspewock@iol.unh.edu Cc: yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, Luca.Vizzarro@arm.com, wathsala.vithanage@arm.com, thomas@monjalon.net, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Reviewed-by: Patrick Robb I don't have any comments beyond Luca's suggestions, but saw the typo below= . On Tue, May 14, 2024 at 4:15=E2=80=AFPM wrote: > + def __exit__(self, type: BaseException, value: BaseException, traceb= ack: 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 i= t's close method. Note that it's -> its On Tue, May 14, 2024 at 4:15=E2=80=AFPM wrote: > > From: Jeremy Spewock > > 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 > --- > .../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_she= ll.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 ha= ndles 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 p= rimary 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 =3D TypeVar( > + "CriticalInteractiveShellType", bound=3D"CriticalInteractiveShell" > +) > + > + > +class CriticalInteractiveShell(InteractiveShell): > + """The base class for interactive critical applications. > + > + This class is a wrapper around :class:`~.interactive_shell.Interacti= veShell` and should always > + implement the exact same functionality with the primary difference b= eing how the application > + is started and stopped. In contrast to normal interactive shells, th= is class does not start the > + application upon initialization of the class. Instead, the applicati= on 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 =3D "", > + timeout: float =3D SETTINGS.timeout, > + ) -> None: > + """Store parameters for creating an interactive shell, but do no= t start the application. > + > + Note that this method also does not create the channel for the a= pplication, as this is > + something that isn't needed until the application starts. > + > + Args: > + interactive_session: The SSH session dedicated to interactiv= e 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 wi= ll not be started > + with elevated privileges. > + app_args: The command line arguments to be passed to the app= lication on startup. > + timeout: The timeout used for the SSH channel that is dedica= ted to this interactive > + shell. This timeout is for collecting output, so if read= ing from the buffer > + and no output is gathered within the timeout, an excepti= on is thrown. The default > + value for this argument may be modified using the :optio= n:`--timeout` command-line > + argument or the :envvar:`DTS_TIMEOUT` environment variab= le. > + """ > + self._interactive_session =3D interactive_session > + self._logger =3D logger > + self._timeout =3D timeout > + self._app_args =3D app_args > + self._get_priviledged_command =3D get_privileged_command > + > + def __enter__(self: CriticalInteractiveShellType) -> CriticalInterac= tiveShellType: > + """Enter the context block. > + > + Upon entering a context block with this class, the desired behav= ior is to create the > + channel for the application to use, and then start the applicati= on. > + > + Returns: > + Reference to the object for the application after it has bee= n started. > + """ > + self._init_channel() > + self._start_application(self._get_priviledged_command) > + return self > + > + def __exit__(self, type: BaseException, value: BaseException, traceb= ack: 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 i= t's close method. Note that > + because this method returns :data:`None` if an exception was rai= sed within the block, it is > + not handled and will be re-raised after the application is close= d. > + > + 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/fram= ework/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 excepti= on is thrown. > """ > self._interactive_session =3D interactive_session > - self._ssh_channel =3D self._interactive_session.invoke_shell() > - self._stdin =3D self._ssh_channel.makefile_stdin("w") > - self._stdout =3D self._ssh_channel.makefile("r") > - self._ssh_channel.settimeout(timeout) > - self._ssh_channel.set_combine_stderr(True) # combines stdout an= d stderr streams > self._logger =3D logger > self._timeout =3D timeout > self._app_args =3D app_args > + self._init_channel() > self._start_application(get_privileged_command) > > + def _init_channel(self): > + self._ssh_channel =3D self._interactive_session.invoke_shell() > + self._stdin =3D self._ssh_channel.makefile_stdin("w") > + self._stdout =3D self._ssh_channel.makefile("r") > + self._ssh_channel.settimeout(self._timeout) > + self._ssh_channel.set_combine_stderr(True) # combines stdout an= d 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/framewor= k/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 =3D 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/test= bed_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] =3D set() > self._logger.debug(f"Checking which capabilities from {capabilit= ies} NIC are supported.") > testpmd_shell =3D self.create_interactive_shell(TestPmdShell, pr= ivileged=3DTrue) > - for capability in capabilities: > - if capability not in supported_capas or capability not in un= supported_capas: > - capability.value(testpmd_shell, supported_capas, unsuppo= rted_capas) > - del testpmd_shell > + with testpmd_shell as running_testpmd: > + for capability in capabilities: > + if capability not in supported_capas or capability not i= n unsupported_capas: > + capability.value(running_testpmd, supported_capas, u= nsupported_capas) > return supported_capas > > def _set_up_build_target(self, build_target_config: BuildTargetConfi= guration) -> None: > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSu= ite_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 =3D self.sut_node.create_interactive_shell( > + testpmd_shell =3D self.sut_node.create_interactive_shell( > TestPmdShell, > app_parameters=3D( > "--mbcache=3D200 " > @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None: > ), > privileged=3DTrue, > ) > - testpmd.set_forward_mode(TestPmdForwardingModes.mac) > - testpmd.start() > - > - for offset in [-1, 0, 1, 4, 5]: > - recv_payload =3D self.scatter_pktgen_send_packet(mbsize + of= fset) > - self._logger.debug(f"Payload of scattered packet after forwa= rding: \n{recv_payload}") > - self.verify( > - ("58 " * 8).strip() in recv_payload, > - f"Payload of scattered packet did not match expected pay= load 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 =3D self.scatter_pktgen_send_packet(mbsize = + offset) > + self._logger.debug( > + f"Payload of scattered packet after forwarding: \n{r= ecv_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_smo= ke_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 =3D self.sut_node.create_interactive_shell(TestPm= dShell, privileged=3DTrue) > - dev_list =3D [str(x) for x in testpmd_driver.get_devices()] > + with testpmd_driver as testpmd: > + dev_list =3D [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 >