Re: [PATCH v2 7/8] dts: rework interactive shells

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Thu, May 9, 2024 at 7:21 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The way nodes and interactive shells interact makes it difficult to
> develop for static type checking and hinting. The current system relies
> on a top-down approach, attempting to give a generic interface to the
> test developer, hiding the interaction of concrete shell classes as much
> as possible. When working with strong typing this approach is not ideal,
> as Python's implementation of generics is still rudimentary.
>
> This rework reverses the tests interaction to a bottom-up approach,
> allowing the test developer to call concrete shell classes directly,
> and let them ingest nodes independently. While also re-enforcing type
> checking and making the code easier to read.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>  dts/framework/params/eal.py                   |   6 +-
>  dts/framework/remote_session/dpdk_shell.py    | 104 ++++++++++++++++
>  .../remote_session/interactive_shell.py       |  75 +++++++-----
>  dts/framework/remote_session/python_shell.py  |   4 +-
>  dts/framework/remote_session/testpmd_shell.py |  64 +++++-----
>  dts/framework/testbed_model/node.py           |  36 +-----
>  dts/framework/testbed_model/os_session.py     |  36 +-----
>  dts/framework/testbed_model/sut_node.py       | 112 +-----------------
>  .../testbed_model/traffic_generator/scapy.py  |   4 +-
>  dts/tests/TestSuite_hello_world.py            |   7 +-
>  dts/tests/TestSuite_pmd_buffer_scatter.py     |  21 ++--
>  dts/tests/TestSuite_smoke_tests.py            |   2 +-
>  12 files changed, 201 insertions(+), 270 deletions(-)
>  create mode 100644 dts/framework/remote_session/dpdk_shell.py
>
<snip>
>      def __init__(
>          self,
> -        interactive_session: SSHClient,
> -        logger: DTSLogger,
> -        get_privileged_command: Callable[[str], str] | None,
> +        node: Node,
>          app_params: Params = Params(),
> +        privileged: bool = False,
>          timeout: float = SETTINGS.timeout,
> +        start_on_init: bool = True,
>      ) -> None:
>          """Create an SSH channel during initialization.
>
>          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.
> +            node: The node on which to run start the interactive shell.
>              app_params: The command line parameters to be passed to the application on startup.
> +            privileged: Enables the shell to run as superuser.
>              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.
> +            start_on_init: Start interactive shell automatically after object initialisation.
>          """
> -        self._interactive_session = interactive_session
> -        self._ssh_channel = self._interactive_session.invoke_shell()
> +        self._node = node
> +        self._logger = node._logger
> +        self._app_params = app_params
> +        self._privileged = privileged
> +        self._timeout = timeout
> +        # Ensure path is properly formatted for the host
> +        self._update_path(self._node.main_session.join_remote_path(self.path))
> +
> +        self.__post_init__()
> +
> +        if start_on_init:
> +            self.start_application()

What's the reason for including start_on_init? Is there a time when
someone would create an application but not want to start it when they
create it? It seems like it is always true currently and I'm not sure
we would want it to be delayed otherwise (except in cases like the
context manager patch where we want to enforce that it is only started
for specific periods of time).

> +
> +    def __post_init__(self):

Is the name of this method meant to mimic that of the dataclasses? It
might also make sense to call it something like `_post_init()` as just
a regular private method, I'm not sure it matters either way.
Additionally, I think in other super classes which contained functions
that were optionally implemented by subclasses we omitted the `pass`
and just left the function stub empty other than the doc-string.
Either way this does the same thing, but it might be better to make
them consistent one way or the other.

> +        """Overridable. Method called after the object init and before application start."""
> +        pass
> +
<snip>
>
> -    def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
> +    def start_application(self) -> None:
>          """Starts a new interactive application based on the path to the app.
>
>          This method is often overridden by subclasses as their process for
>          starting may look different.
> -
> -        Args:
> -            get_privileged_command: A function (but could be any callable) that produces
> -                the version of the command with elevated privileges.
>          """
> -        start_command = f"{self.path} {self._app_params}"
> -        if get_privileged_command is not None:
> -            start_command = get_privileged_command(start_command)
> +        self._setup_ssh_channel()
> +
> +        start_command = self._make_start_command()
> +        if self._privileged:
> +            start_command = self._node.main_session._get_privileged_command(start_command)
>          self.send_command(start_command)
>
> +    def _make_start_command(self) -> str:

It might make sense to put this above the start_application method
since that is where it gets called.

> +        """Makes the command that starts the interactive shell."""
> +        return f"{self.path} {self._app_params or ''}"
> +
<snip>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ef3f23c582..92930d7fbb 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -7,9 +7,7 @@
>
>  Typical usage example in a TestSuite::
>
> -    testpmd_shell = self.sut_node.create_interactive_shell(
> -            TestPmdShell, privileged=True
> -        )
> +    testpmd_shell = TestPmdShell()

Maybe adding a parameter to this instantiation in the example would
still be useful. So something like `TestPmdShell(self.sut_node)`
instead just because this cannot be instantiated without any
arguments.

>      devices = testpmd_shell.get_devices()
>      for device in devices:
>          print(device)
<snip>
> 2.34.1
>