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 8A179424C1; Mon, 10 Jun 2024 16:31:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 164EC402A9; Mon, 10 Jun 2024 16:31:51 +0200 (CEST) Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by mails.dpdk.org (Postfix) with ESMTP id 1A73D40294 for ; Mon, 10 Jun 2024 16:31:50 +0200 (CEST) Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-421cd1e5f93so467055e9.0 for ; Mon, 10 Jun 2024 07:31:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1718029910; x=1718634710; 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=vaHxdnPJhoui6Hr3rzG7c4dqExc6PXrzMuEEj3k9Ttk=; b=ZkjDeetQim0zylQUh1nweTaYEoTcWtJ+SRz05BeXQ4SLUqBJDMpEr1r/BLBOCALQSV /hexi7adaGf/wOeYEk6M+cbYTWWdREY2stXmPAb+KkFG9XWiXEqaNf3OHRzkS5+7ocqS yhk6UyVifhl4Ao0/x9fJVNBZFaltxGGKTWMOcs4FeWmTkIg8Z/sBqxRJwJ+QoQrvPrPq MVHSDTXPklFVC+HXJlhnUbx9LH7Fp9U94vmxbadhpzc2MkTPbJXj7cODJhle2Gl28DVs TVJMt6LTAy7CmcL8U0lxv9johjDcWJlG1jZHhQVZFeRUIy/eNMIJ89bzHHNVno2qSA9p RcEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718029910; x=1718634710; 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=vaHxdnPJhoui6Hr3rzG7c4dqExc6PXrzMuEEj3k9Ttk=; b=ExRXwaA+HGpJQmL29+tcNEigvW9/PVOgnt9H7V38ygpnYfFN6RFelvv3eIVITRw6Jt oybnK1g1Qej8X4xKpkaXDujdw6j0yKmoBTOBnwbFQWFAqPEcvyv3e7TjIzUR2+QmHTFY LwgR6hslTtkMrhH3QcgtRl6WtYVneJCboLtrrdQivUQ5txBAVnYH50ur3HStyaJaFOlS R2CUj9DRrfI5n32+EWrFGqgxfYBDBhOQu3U7CK3jAIaZkI2m4CpeYZq+tIjAVSethIVP uwZqiBm/CO6zzzIZoenxId6+oDHkWF1v2nGUElRU2pTH7HfxAGTSHMOPCFBlqmJ3MAa/ eMFA== X-Gm-Message-State: AOJu0YzN0WTYOJVopS4ZM0ixfM4yXofAXe6rQyZIg3RRXRGvHIw6QFgb AqSATv49gm/KAIX4bhJnUajgCl2NHAnZBAiktpfQmgrurkMGLMu8O664GCWA9f4= X-Google-Smtp-Source: AGHT+IFt9dHJVBWlrpUWVNe5UrW8gsB8MHOwyGnNVvltu354wki3lZ1fn+F+akzCB7eqYIVp6+W7lA== X-Received: by 2002:a05:600c:4f8a:b0:421:7e88:805 with SMTP id 5b1f17b1804b1-4217e8837bbmr43837995e9.35.1718029909477; Mon, 10 Jun 2024 07:31:49 -0700 (PDT) Received: from [192.168.1.113] ([84.245.121.236]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-421aace8288sm44142755e9.47.2024.06.10.07.31.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 10 Jun 2024 07:31:49 -0700 (PDT) Message-ID: <5639da83-922d-4872-8b90-089048e46908@pantheon.tech> Date: Mon, 10 Jun 2024 16:31:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/4] dts: add context manager for interactive shells To: jspewock@iol.unh.edu, 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 Cc: 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> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240605213148.21371-3-jspewock@iol.unh.edu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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."""