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 4873B424C9; Tue, 11 Jun 2024 11:17:52 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E49D74021F; Tue, 11 Jun 2024 11:17:51 +0200 (CEST) Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) by mails.dpdk.org (Postfix) with ESMTP id 56F8B400D6 for ; Tue, 11 Jun 2024 11:17:50 +0200 (CEST) Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-52c82101407so4502729e87.3 for ; Tue, 11 Jun 2024 02:17:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1718097470; x=1718702270; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=F0Z1bg+6KBCLrNV8WEkt30MyUfmfXCK9rMSDUF2a7tE=; b=WUMTbzbFlC+OOWvJSC/Ze91UpaK2V7IRJ5mbuo0HuDpkDlxz4dLDLFureZDG9QjZ9q XXZyyBPG8+WgsUIS3kYuf/i8YLVPYm6WSc462NQO2JwqmQT+aZLFeTtx35EVxBYG9nms oUyOfJ/V85cMAUtC9tNpvRLC0upRhtf0fJbH73YcYI6fEtce2moO0H21QiyGBjEpNkkB EOpBwmck2rvQ1GpsPlO2WM+4iwSvBGZnXwDqYugbVkL+IRx70nx46OgyTm/L+uAJVz8f VsW/tYUZKcgSR5S5KeLhmLCwwpgQm24MFcMZA+Rv4sbupvjg9TRAgNjidyLAUcsPrVHN 08ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718097470; x=1718702270; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=F0Z1bg+6KBCLrNV8WEkt30MyUfmfXCK9rMSDUF2a7tE=; b=ARkyTNRYM8yg96BvH2+1tIGMj2XBZtzq4PS6XNkPXxB/9nRRdhQajNtY3gGreQm8FS 8Vr92bovR8ogc3Ow6s9yty8Zdbe7VrpLhFuwSV/C95Hj6g1NF+glMAUh05BvZo4tIf1y Mzg5RsQrkwGGtdTt8FKUboeUIk8QnqGuZtPt9xL8OLPiLYbrxgkxT8iW6Dxy43p0zHTy mPxfaw73hAMF7ATvty1OkgXScs1k6Xa0oBUT03ae49kEP+JLfrC0tZTt0I3l7AYGm6US lMwntQbrIAeUHDCB8bG+IjzqNEw/yq0e9YL2B5I2MRly2DRaVjhcgGHkTJDiHJU/8yIN eHeA== X-Forwarded-Encrypted: i=1; AJvYcCVEuNU7gRlUdrzCs7HzoBOhoAyWnq/u7fO30nA74l1ns4tPDSN7Y18m/RZzv63cdklcy37CWiZqkfNfxmQ= X-Gm-Message-State: AOJu0Yyh9nuxIUEAEMlIwb4/9JX7uRON+kUWJ4MtIeDVUd+kb4flrSAS kYS0B7oN1oi/Ti4Gz2c8POnUvO8gmwCpzRJP7zwC6ZU0+ql5Aecl7YDFvPs7GYc= X-Google-Smtp-Source: AGHT+IEbJUTdgxoXxCz3MQfNOApompfbg/gX3DDjx6vA1BeHZEyr72ql2rlwT2iflxMFUnVNH1CBNg== X-Received: by 2002:a19:5f53:0:b0:52c:76ac:329b with SMTP id 2adb3069b0e04-52c76ac335cmr5361362e87.35.1718097469693; Tue, 11 Jun 2024 02:17:49 -0700 (PDT) Received: from [10.12.0.137] (81.89.53.154.host.vnet.sk. [81.89.53.154]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a6f0d20a616sm406082166b.30.2024.06.11.02.17.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 11 Jun 2024 02:17:49 -0700 (PDT) Message-ID: Date: Tue, 11 Jun 2024 11:17:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/4] dts: add context manager for interactive shells To: Jeremy Spewock 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 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> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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š > 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: > >>> 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 > >>> "--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.""" >>