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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

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."""
>>