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 4A74C424C3; Tue, 11 Jun 2024 17:33:18 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 18E2740299; Tue, 11 Jun 2024 17:33:18 +0200 (CEST) Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mails.dpdk.org (Postfix) with ESMTP id E3F4B4021F for ; Tue, 11 Jun 2024 17:33:16 +0200 (CEST) Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-2c3051aee3fso2073642a91.3 for ; Tue, 11 Jun 2024 08:33:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718119996; x=1718724796; 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=OBYOODKaygVOk7k5xlFgR1KYTGflUDoMsNr1WOm+gzA=; b=WSN+N7iTySRV1/uxtmqBa/Px5+1dSt8S5Ur8bPCelCfTrEfBzEGH6kICS6LfaQrq9S 4uMkJl2gl602qv6jUZxGD947+hRtI45exkYQgSQilCNnwZ/C+VlV6BqyP6EWEeT6KNDe wBpgxg7lnimaFae3T4T5DzXH3rvyxLwXqyefw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718119996; x=1718724796; 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=OBYOODKaygVOk7k5xlFgR1KYTGflUDoMsNr1WOm+gzA=; b=YsIHrNul7gFVLBuIP8I2RXdhbA4ka1nF555Gb06or53QbVXtKFHUMltO9k9wS3/QHr qExkepJi4KbeWHrp0uG28RVu92lRwkHRZv0pnlML8OCkAYTeqeybYwtPwAeR6ODP7kej TECY7DD17e7pq2eZ7Tcy49ZcVoOwU99xysZXnCcq4vOiN6BeDxepLM0NnFfHSFSn8k/k AhfTvxvqUwLFfBTfMMJna0gB5oeEc1lB0jqkgLNZQuvfaJ35YaIaxz2FciC6njxSj95C CDhSg44v6QLqPhx5a9/bkkqeBTvUmr63SMMyJNDTT/LhFIHJmgB3VmX5+i36bm+CeZca LrSA== X-Forwarded-Encrypted: i=1; AJvYcCX4nEFu5UWDUAWKg8zwXT0z92vxrKcmqHWn/IRc7WpZ4zZnwPf+jr/dF669A9PGSUhWXodeyjQLsWFAA2s= X-Gm-Message-State: AOJu0Ywqa9jB1/GmhKymgDppMm/ddX7n7q6npNKonY/VdLYxDDH2T8NC O1k3ZdCJNrH8gnu7fl6Ok8yClnI1FNNBNbqLSWP8RdcrMZsj3a2Edyer6J+m7jcWUSlCz/Ux1Xm L6ZGphhWoCvz9ppnAoJjMUlPk10r7G82M/pObhw== X-Google-Smtp-Source: AGHT+IFmgI1unYsSk2UVuv5QUCXGgJObPmKxV3d9huxPqgVhf+hWgo43CTatk7RR22sY/c2CBBIQlfMHHQCGco9xOv4= X-Received: by 2002:a17:90a:d157:b0:2c2:c988:b88f with SMTP id 98e67ed59e1d1-2c2c988bc67mr11700883a91.26.1718119995625; Tue, 11 Jun 2024 08:33:15 -0700 (PDT) MIME-Version: 1.0 References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240605213148.21371-1-jspewock@iol.unh.edu> <20240605213148.21371-3-jspewock@iol.unh.edu> <5639da83-922d-4872-8b90-089048e46908@pantheon.tech> In-Reply-To: From: Jeremy Spewock Date: Tue, 11 Jun 2024 11:33:03 -0400 Message-ID: Subject: Re: [PATCH v3 2/4] dts: add context manager for interactive shells To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, npratte@iol.unh.edu, yoan.picchi@foss.arm.com, Luca.Vizzarro@arm.com, 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 On Tue, Jun 11, 2024 at 5:17=E2=80=AFAM Juraj Linke=C5=A1 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=E2=80=AFAM Juraj Linke=C5=A1 > > 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 th= at > >> 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` tha= t handles critical applications. > >>> + > >>> +Critical applications are defined as applications that require expli= cit clean-up before another > >>> +instance of some application can be started. In DPDK these are refer= red to as "primary > >>> +applications" and these applications take out a lock which stops oth= er 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 no= t > >> 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 subclas= ses 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.Inter= activeShell` and should always > >> > >> This actually sounds backwards to me. This should be the base class wi= th > >> InteractiveShell adding the ability to start the shell without the > >> context manager (either right away or explicitly after creating the ob= ject). > >> > > > > 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 thi= ng > > > > 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 DPD= K > >> (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 differen= ce 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 appli= cation is handled through a > >>> + context manager. This allows for more explicit starting and stop= ping 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 =3D "", > >>> + timeout: float =3D SETTINGS.timeout, > >>> + ) -> None > + """Store parameters for creating an interac= tive shell, but > >> do not start the application. > >>> + > >>> + Note that this method also does not create the channel for t= he application, as this is > >>> + something that isn't needed until the application starts. > >>> + > >>> + Args: > >>> + interactive_session: The SSH session dedicated to intera= ctive 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 applicatio= n 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 de= dicated 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 exc= eption is thrown. The default > >>> + value for this argument may be modified using the :o= ption:`--timeout` command-line > >>> + argument or the :envvar:`DTS_TIMEOUT` environment va= riable. > >>> + """ > >>> + self._interactive_session =3D interactive_session > >>> + self._logger =3D logger > >>> + self._timeout =3D timeout > >>> + self._app_args =3D app_args > >>> + self._get_privileged_command =3D get_privileged_command > >> > >> We see here why it's backwards. We're duplicating this part of the cod= e > >> 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: > > > >>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/fram= ework/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.sca= ttered_rx) > >>> > >>> + def __exit__(self, *_) -> None: > >>> + """Overrides :meth:`~.critical_interactive_shell.CriticalInt= eractiveShell.__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 :clas= s:`TestPmdShell` methods. > >> > >>> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/Te= stSuite_pmd_buffer_scatter.py > >>> index 3701c47408..41f6090a7e 100644 > >>> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py > >>> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py > > > >>> "--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 = + offset) > >>> - self._logger.debug(f"Payload of scattered packet after f= orwarding: \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 =3D self.scatter_pktgen_send_packet(mbs= ize + 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 expec= ted 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 ad= ds > >> 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 tha= t > >> 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 2= 048.""" > >>