Tested-by: Nicholas Pratte <npra...@iol.unh.edu>
Reviewed-by: Nicholas Pratte <npra...@iol.unh.edu>
On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizza...@arm.com> wrote:
>
> Make it so that interactive shells accept an implementation of `Params`
> for app arguments. Convert EalParameters to use `Params` instead.
>
> String command line parameters can still be supplied by using the
> `StrParams` implementation.
>
> Signed-off-by: Luca Vizzarro <luca.vizza...@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-pres...@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> ---
> .../remote_session/interactive_shell.py | 8 +-
> dts/framework/remote_session/testpmd_shell.py | 12 +-
> dts/framework/testbed_model/__init__.py | 2 +-
> dts/framework/testbed_model/node.py | 4 +-
> dts/framework/testbed_model/os_session.py | 4 +-
> dts/framework/testbed_model/sut_node.py | 106 ++++++++----------
> dts/tests/TestSuite_pmd_buffer_scatter.py | 3 +-
> 7 files changed, 73 insertions(+), 66 deletions(-)
>
> diff --git a/dts/framework/remote_session/interactive_shell.py
> b/dts/framework/remote_session/interactive_shell.py
> index 5cfe202e15..a2c7b30d9f 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2023 University of New Hampshire
> +# Copyright(c) 2024 Arm Limited
>
> """Common functionality for interactive shell handling.
>
> @@ -21,6 +22,7 @@
> from paramiko import Channel, SSHClient, channel # type: ignore[import]
>
> from framework.logger import DTSLogger
> +from framework.params import Params
> from framework.settings import SETTINGS
>
>
> @@ -40,7 +42,7 @@ class InteractiveShell(ABC):
> _ssh_channel: Channel
> _logger: DTSLogger
> _timeout: float
> - _app_args: str
> + _app_args: Params | None
>
> #: Prompt to expect at the end of output when sending a command.
> #: This is often overridden by subclasses.
> @@ -63,7 +65,7 @@ def __init__(
> interactive_session: SSHClient,
> logger: DTSLogger,
> get_privileged_command: Callable[[str], str] | None,
> - app_args: str = "",
> + app_args: Params | None = None,
> timeout: float = SETTINGS.timeout,
> ) -> None:
> """Create an SSH channel during initialization.
> @@ -100,7 +102,7 @@ def _start_application(self, get_privileged_command:
> Callable[[str], str] | None
> 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_args}"
> + start_command = f"{self.path} {self._app_args or ''}"
> if get_privileged_command is not None:
> start_command = get_privileged_command(start_command)
> self.send_command(start_command)
> diff --git a/dts/framework/remote_session/testpmd_shell.py
> b/dts/framework/remote_session/testpmd_shell.py
> index cb2ab6bd00..db3abb7600 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -21,6 +21,7 @@
> from typing import Callable, ClassVar
>
> from framework.exception import InteractiveCommandExecutionError
> +from framework.params import StrParams
> from framework.settings import SETTINGS
> from framework.utils import StrEnum
>
> @@ -118,8 +119,15 @@ def _start_application(self, get_privileged_command:
> Callable[[str], str] | None
> Also find the number of pci addresses which were allowed on the
> command line when the app
> was started.
> """
> - self._app_args += " -i --mask-event intr_lsc"
> - self.number_of_ports = self._app_args.count("-a ")
> + from framework.testbed_model.sut_node import EalParameters
> +
> + assert isinstance(self._app_args, EalParameters)
> +
> + if isinstance(self._app_args.app_params, StrParams):
> + self._app_args.app_params.value += " -i --mask-event intr_lsc"
> +
> + self.number_of_ports = len(self._app_args.ports) if
> self._app_args.ports is not None else 0
> +
> super()._start_application(get_privileged_command)
>
> def start(self, verify: bool = True) -> None:
> diff --git a/dts/framework/testbed_model/__init__.py
> b/dts/framework/testbed_model/__init__.py
> index 6086512ca2..ef9520df4c 100644
> --- a/dts/framework/testbed_model/__init__.py
> +++ b/dts/framework/testbed_model/__init__.py
> @@ -23,6 +23,6 @@
> from .cpu import LogicalCoreCount, LogicalCoreCountFilter, LogicalCoreList
> from .node import Node
> from .port import Port, PortLink
> -from .sut_node import SutNode
> +from .sut_node import SutNode, EalParameters
> from .tg_node import TGNode
> from .virtual_device import VirtualDevice
> diff --git a/dts/framework/testbed_model/node.py
> b/dts/framework/testbed_model/node.py
> index 74061f6262..ec9512d618 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -2,6 +2,7 @@
> # Copyright(c) 2010-2014 Intel Corporation
> # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
> # Copyright(c) 2022-2023 University of New Hampshire
> +# Copyright(c) 2024 Arm Limited
>
> """Common functionality for node management.
>
> @@ -24,6 +25,7 @@
> )
> from framework.exception import ConfigurationError
> from framework.logger import DTSLogger, get_dts_logger
> +from framework.params import Params
> from framework.settings import SETTINGS
>
> from .cpu import (
> @@ -199,7 +201,7 @@ def create_interactive_shell(
> shell_cls: Type[InteractiveShellType],
> timeout: float = SETTINGS.timeout,
> privileged: bool = False,
> - app_args: str = "",
> + app_args: Params | None = None,
> ) -> InteractiveShellType:
> """Factory for interactive session handlers.
>
> diff --git a/dts/framework/testbed_model/os_session.py
> b/dts/framework/testbed_model/os_session.py
> index d5bf7e0401..7234c975c8 100644
> --- a/dts/framework/testbed_model/os_session.py
> +++ b/dts/framework/testbed_model/os_session.py
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> # Copyright(c) 2023 University of New Hampshire
> +# Copyright(c) 2024 Arm Limited
>
> """OS-aware remote session.
>
> @@ -29,6 +30,7 @@
>
> from framework.config import Architecture, NodeConfiguration, NodeInfo
> from framework.logger import DTSLogger
> +from framework.params import Params
> from framework.remote_session import (
> CommandResult,
> InteractiveRemoteSession,
> @@ -134,7 +136,7 @@ def create_interactive_shell(
> shell_cls: Type[InteractiveShellType],
> timeout: float,
> privileged: bool,
> - app_args: str,
> + app_args: Params | None,
> ) -> InteractiveShellType:
> """Factory for interactive session handlers.
>
> diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> index 97aa26d419..3f8c3807b3 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -2,6 +2,7 @@
> # Copyright(c) 2010-2014 Intel Corporation
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> # Copyright(c) 2023 University of New Hampshire
> +# Copyright(c) 2024 Arm Limited
>
> """System under test (DPDK + hardware) node.
>
> @@ -11,6 +12,7 @@
> """
>
>
> +from dataclasses import dataclass, field
> import os
> import tarfile
> import time
> @@ -23,6 +25,8 @@
> NodeInfo,
> SutNodeConfiguration,
> )
> +from framework import params
> +from framework.params import Params, StrParams
> from framework.remote_session import CommandResult
> from framework.settings import SETTINGS
> from framework.utils import MesonArgs
> @@ -34,62 +38,51 @@
> from .virtual_device import VirtualDevice
>
>
> -class EalParameters(object):
> +def _port_to_pci(port: Port) -> str:
> + return port.pci
> +
> +
> +@dataclass(kw_only=True)
> +class EalParameters(Params):
> """The environment abstraction layer parameters.
>
> The string representation can be created by converting the instance to a
> string.
> """
>
> - def __init__(
> - self,
> - lcore_list: LogicalCoreList,
> - memory_channels: int,
> - prefix: str,
> - no_pci: bool,
> - vdevs: list[VirtualDevice],
> - ports: list[Port],
> - other_eal_param: str,
> - ):
> - """Initialize the parameters according to inputs.
> -
> - Process the parameters into the format used on the command line.
> + lcore_list: LogicalCoreList = field(metadata=params.short("l"))
> + """The list of logical cores to use."""
>
> - Args:
> - lcore_list: The list of logical cores to use.
> - memory_channels: The number of memory channels to use.
> - prefix: Set the file prefix string with which to start DPDK,
> e.g.: ``prefix='vf'``.
> - no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> - vdevs: Virtual devices, e.g.::
> + memory_channels: int = field(metadata=params.short("n"))
> + """The number of memory channels to use."""
>
> - vdevs=[
> - VirtualDevice('net_ring0'),
> - VirtualDevice('net_ring1')
> - ]
> - ports: The list of ports to allow.
> - other_eal_param: user defined DPDK EAL parameters, e.g.:
> - ``other_eal_param='--single-file-segments'``
> - """
> - self._lcore_list = f"-l {lcore_list}"
> - self._memory_channels = f"-n {memory_channels}"
> - self._prefix = prefix
> - if prefix:
> - self._prefix = f"--file-prefix={prefix}"
> - self._no_pci = "--no-pci" if no_pci else ""
> - self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> - self._ports = " ".join(f"-a {port.pci}" for port in ports)
> - self._other_eal_param = other_eal_param
> -
> - def __str__(self) -> str:
> - """Create the EAL string."""
> - return (
> - f"{self._lcore_list} "
> - f"{self._memory_channels} "
> - f"{self._prefix} "
> - f"{self._no_pci} "
> - f"{self._vdevs} "
> - f"{self._ports} "
> - f"{self._other_eal_param}"
> - )
> + prefix: str = field(metadata=params.long("file-prefix"))
> + """Set the file prefix string with which to start DPDK, e.g.:
> ``prefix="vf"``."""
> +
> + no_pci: params.Option
> + """Switch to disable PCI bus e.g.: ``no_pci=True``."""
> +
> + vdevs: list[VirtualDevice] | None = field(
> + default=None, metadata=params.multiple(params.long("vdev"))
> + )
> + """Virtual devices, e.g.::
> +
> + vdevs=[
> + VirtualDevice("net_ring0"),
> + VirtualDevice("net_ring1")
> + ]
> + """
> +
> + ports: list[Port] | None = field(
> + default=None,
> + metadata=params.field_mixins(_port_to_pci,
> metadata=params.multiple(params.short("a"))),
> + )
> + """The list of ports to allow."""
> +
> + other_eal_param: StrParams | None = None
> + """Any other EAL parameter(s)."""
> +
> + app_params: Params | None = field(default=None,
> metadata=params.options_end())
> + """Parameters to pass to the underlying DPDK app."""
>
>
> class SutNode(Node):
> @@ -350,7 +343,7 @@ def create_eal_parameters(
> ascending_cores: bool = True,
> prefix: str = "dpdk",
> append_prefix_timestamp: bool = True,
> - no_pci: bool = False,
> + no_pci: params.Option = None,
> vdevs: list[VirtualDevice] | None = None,
> ports: list[Port] | None = None,
> other_eal_param: str = "",
> @@ -393,9 +386,6 @@ def create_eal_parameters(
> if prefix:
> self._dpdk_prefix_list.append(prefix)
>
> - if vdevs is None:
> - vdevs = []
> -
> if ports is None:
> ports = self.ports
>
> @@ -406,7 +396,7 @@ def create_eal_parameters(
> no_pci=no_pci,
> vdevs=vdevs,
> ports=ports,
> - other_eal_param=other_eal_param,
> + other_eal_param=StrParams(other_eal_param),
> )
>
> def run_dpdk_app(
> @@ -442,7 +432,7 @@ def create_interactive_shell(
> shell_cls: Type[InteractiveShellType],
> timeout: float = SETTINGS.timeout,
> privileged: bool = False,
> - app_parameters: str = "",
> + app_parameters: Params | None = None,
> eal_parameters: EalParameters | None = None,
> ) -> InteractiveShellType:
> """Extend the factory for interactive session handlers.
> @@ -459,6 +449,7 @@ def create_interactive_shell(
> reading from the buffer and don't receive any data within
> the timeout
> it will throw an error.
> privileged: Whether to run the shell with administrative
> privileges.
> + app_args: The arguments to be passed to the application.
> eal_parameters: List of EAL parameters to use to launch the app.
> If this
> isn't provided or an empty string is passed, it will default
> to calling
> :meth:`create_eal_parameters`.
> @@ -470,9 +461,10 @@ def create_interactive_shell(
> """
> # We need to append the build directory and add EAL parameters for
> DPDK apps
> if shell_cls.dpdk_app:
> - if not eal_parameters:
> + if eal_parameters is None:
> eal_parameters = self.create_eal_parameters()
> - app_parameters = f"{eal_parameters} -- {app_parameters}"
> + eal_parameters.app_params = app_parameters
> + app_parameters = eal_parameters
>
> shell_cls.path = self.main_session.join_remote_path(
> self.remote_dpdk_build_dir, shell_cls.path
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py
> b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 3701c47408..4cdbdc4272 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -22,6 +22,7 @@
> from scapy.packet import Raw # type: ignore[import]
> from scapy.utils import hexstr # type: ignore[import]
>
> +from framework.params import StrParams
> from framework.remote_session.testpmd_shell import TestPmdForwardingModes,
> TestPmdShell
> from framework.test_suite import TestSuite
>
> @@ -103,7 +104,7 @@ def pmd_scatter(self, mbsize: int) -> None:
> """
> testpmd = self.sut_node.create_interactive_shell(
> TestPmdShell,
> - app_parameters=(
> + app_parameters=StrParams(
> "--mbcache=200 "
> f"--mbuf-size={mbsize} "
> "--max-pkt-len=9000 "
> --
> 2.34.1
>
