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

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

> diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
> new file mode 100644
> index 0000000000..25e3df4eaa
> --- /dev/null
> +++ b/dts/framework/remote_session/dpdk_shell.py
> @@ -0,0 +1,104 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Arm Limited
> +
> +"""DPDK-based interactive shell.

I think this means the shell uses DPDK libraries. This would be better 
worded as "Base interactive shell for DPDK applications."

> +
> +Provides a base class to create interactive shells based on DPDK.
> +"""
> +
> +
> +from abc import ABC
> +
> +from framework.params.eal import EalParams
> +from framework.remote_session.interactive_shell import InteractiveShell
> +from framework.settings import SETTINGS
> +from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
> +from framework.testbed_model.sut_node import SutNode
> +
> +
> +def compute_eal_params(
> +    node: SutNode,

Let's rename this sut_node. I got confused a bit when reading the code.

> +    params: EalParams | None = None,
> +    lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
> +    ascending_cores: bool = True,
> +    append_prefix_timestamp: bool = True,
> +) -> EalParams:
> +    """Compute EAL parameters based on the node's specifications.
> +
> +    Args:
> +        node: The SUT node to compute the values for.
> +        params: The EalParams object to amend, if set to None a new object is created and returned.

This could use some additional explanation about how it's amended - 
what's replaced, what isn't and in general what happens.

> +        lcore_filter_specifier: A number of lcores/cores/sockets to use
> +            or a list of lcore ids to use.
> +            The default will select one lcore for each of two cores
> +            on one socket, in ascending order of core ids.
> +        ascending_cores: Sort cores in ascending order (lowest to highest IDs).
> +            If :data:`False`, sort in descending order.
> +        append_prefix_timestamp: If :data:`True`, will append a timestamp to DPDK file prefix.
> +    """
> +    if params is None:
> +        params = EalParams()
> +
> +    if params.lcore_list is None:
> +        params.lcore_list = LogicalCoreList(
> +            node.filter_lcores(lcore_filter_specifier, ascending_cores)
> +        )
> +
> +    prefix = params.prefix
> +    if append_prefix_timestamp:
> +        prefix = f"{prefix}_{node._dpdk_timestamp}"
> +    prefix = node.main_session.get_dpdk_file_prefix(prefix)
> +    if prefix:
> +        node._dpdk_prefix_list.append(prefix)

We should make _dpdk_prefix_list public. Also _dpdk_timestamp.

> +    params.prefix = prefix
> +
> +    if params.ports is None:
> +        params.ports = node.ports
> +
> +    return params
> +
> +
> +class DPDKShell(InteractiveShell, ABC):
> +    """The base class for managing DPDK-based interactive shells.
> +
> +    This class shouldn't be instantiated directly, but instead be extended.
> +    It automatically injects computed EAL parameters based on the node in the
> +    supplied app parameters.
> +    """
> +
> +    _node: SutNode

Same here, better to be explicit with _sut_node.

> +    _app_params: EalParams
> +
> +    _lcore_filter_specifier: LogicalCoreCount | LogicalCoreList
> +    _ascending_cores: bool
> +    _append_prefix_timestamp: bool
> +
> +    def __init__(
> +        self,
> +        node: SutNode,
> +        app_params: EalParams,
> +        privileged: bool = True,
> +        timeout: float = SETTINGS.timeout,
> +        lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
> +        ascending_cores: bool = True,
> +        append_prefix_timestamp: bool = True,
> +        start_on_init: bool = True,
> +    ) -> None:
> +        """Overrides :meth:`~.interactive_shell.InteractiveShell.__init__`."""
> +        self._lcore_filter_specifier = lcore_filter_specifier
> +        self._ascending_cores = ascending_cores
> +        self._append_prefix_timestamp = append_prefix_timestamp
> +
> +        super().__init__(node, app_params, privileged, timeout, start_on_init)
> +
> +    def _post_init(self):
> +        """Computes EAL params based on the node capabilities before start."""

We could just put this before calling super().__init__() in this class 
if we update path some other way, right? It's probably better to 
override the class method (_update_path()) in subclasses than having 
this _post_init() method.

> +        self._app_params = compute_eal_params(
> +            self._node,
> +            self._app_params,
> +            self._lcore_filter_specifier,
> +            self._ascending_cores,
> +            self._append_prefix_timestamp,
> +        )
> +
> +        self._update_path(self._node.remote_dpdk_build_dir.joinpath(self.path))
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 9da66d1c7e..4be7966672 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py

> @@ -56,55 +58,63 @@ class InteractiveShell(ABC):
<snip>
> +    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)

This update of the command should be in _make_start_command().

>           self.send_command(start_command)
>   
>       def send_command(self, command: str, prompt: str | None = None) -> str:

> @@ -49,52 +48,48 @@ def __str__(self) -> str:
<snip>
> +    def __init__(
> +        self,
> +        node: SutNode,
> +        privileged: bool = True,
> +        timeout: float = SETTINGS.timeout,
> +        lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = LogicalCoreCount(),
> +        ascending_cores: bool = True,
> +        append_prefix_timestamp: bool = True,
> +        start_on_init: bool = True,
> +        **app_params,
> +    ) -> None:
> +        """Overrides :meth:`~.dpdk_shell.DPDKShell.__init__`. Changes app_params to kwargs."""
> +        super().__init__(
> +            node,
> +            TestPmdParams(**app_params),
> +            privileged,
> +            timeout,
> +            lcore_filter_specifier,
> +            ascending_cores,
> +            append_prefix_timestamp,
> +            start_on_init,

Just a note on the differences in signatures. TestPmdShell has the 
parameters at the end while DPDKShell and InteractiveShell have them 
second. I think we could make app_params the last parameter in all of 
these classes - that works for both kwargs and just singular Params.

>           )
>   
> -        super()._start_application(get_privileged_command)
> -
>       def start(self, verify: bool = True) -> None:
>           """Start packet forwarding with the current configuration.
>