Re: [PATCH v4 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell

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

The subject line should start with a lowercase character and it's also 
over the character limit. This fits and basically says the same:
use PythonShell for Scapy instead of XML-RPC

Scapy TG didn't fit but I think the above still works.

> diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py

> @@ -93,9 +94,13 @@ def __init__(
>           timeout: float = SETTINGS.timeout,
>           app_params: Params = Params(),
>           name: str | None = None,
> +        **kwargs,
>       ) -> None:
>           """Create an SSH channel during initialization.
>   
> +        Additional key-word arguments can be passed through `kwargs` if needed for fulfilling other

key-word -> keyword

> +        constructors in the case of multiple-inheritance.

I also didn't notice this hyphen, it shouldn't be there.

These hyphens are in a lot of places, I don't if I caught them all.

> diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py

> @@ -6,311 +6,150 @@
>   
>   A traffic generator used for functional testing, implemented with
>   `the Scapy library <https://scapy.readthedocs.io/en/latest/>`_.
> -The traffic generator uses an XML-RPC server to run Scapy on the remote TG node.
> +The traffic generator uses an interactive shell to run Scapy on the remote TG node.
>   
> -The traffic generator uses the :mod:`xmlrpc.server` module to run an XML-RPC server
> -in an interactive remote Python SSH session. The communication with the server is facilitated
> -with a local server proxy from the :mod:`xmlrpc.client` module.
> +The traffic generator extends :class:`framework.remote_session.python_shell.PythonShell` to
> +implement the methods for handling packets by sending commands into the interactive shell.

Since this only mentions PythonShell, people could understand that's the 
only thing the tg extends. We should either add a qualifier such as 
additionally extends or just explicitly say it also extends the 
capturing TG (which is also useful information as it tells us which kind 
of TG it is).


> +class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator):
> +    """Provides access to scapy functions on a traffic generator node.

> +    This class extends the base with remote execution of scapy functions. All methods for
> +    processing packets are implemented using an underlying
> +    :class:`framework.remote_session.python_shell.PythonShell` which imports the Scapy library.

> +    Because of the dual-inheritance, this class has both methods that wrap scapy commands sent into

The hyphen doesn't sound right. And maybe double would be a better term. 
I understand dual to mean two of the same and that doesn't fit for me as 
well.

> +    the shell and methods that run locally to fulfill traffic generation needs. To help make a
> +    clear distinction between the two, the names of the methods that wrap the logic of the
> +    underlying shell should be prepended with "shell".

I think there would be value in explicitly saying that the shell runs on 
the TG node.


> +    Note that the order of inheritance is important for this class. In order to instantiate this
> +    class, the abstract methods of :class:`~.capturing_traffic_generator.CapturingTrafficGenerator`
> +    must be implemented. Since some of these methods are implemented in the underlying interactive
> +    shell, according to Python's Method Resolution Order (MRO), the interactive shell must come
> +    first.

I didn't notice this before. Is this because of the close() method? Do 
we need to add any special steps to close the TG? Closing the 
interactive session should be enough, but I wanted to check with you.


> +    def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig, **kwargs):
> +        """Extend the constructor with Scapy TG specifics.
>   
> -    def __init__(self, *args, **kwargs):
> -        """Extend the XML-RPC server initialization.
> +        Initializes both the traffic generator and the interactive shell used to handle Scapy
> +        functions. The interactive shell will be started on `tg_node`. The additional key-word
> +        arguments in `kwargs` are used to pass into the constructor for the interactive shell.
>   
>           Args:
> -            args: The positional arguments that will be passed to the superclass's constructor.
> -            kwargs: The keyword arguments that will be passed to the superclass's constructor.
> -                The `allow_none` argument will be set to :data:`True`.
> +            tg_node: The node where the traffic generator resides.
> +            config: The traffic generator's test run configuration.
> +            kwargs: Keyword arguments all traffic generators must support in order to allow for
> +                multiple-inheritance.

I'm not sure why all and why they must support them. We can just say the 
supported keyword arguments corresdpond to the parameters of 
:meth:`PythonShell.__init__`.

Also, multiple-inheritance -> multiple inheritance


> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> index 4ce1148706..176d5e9065 100644
> --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> @@ -16,23 +16,29 @@
>   from framework.logger import DTSLogger, get_dts_logger
>   from framework.testbed_model.node import Node
>   from framework.testbed_model.port import Port
> -from framework.utils import get_packet_summaries
> +from framework.utils import MultiInheritanceBaseClass, get_packet_summaries
>   
>   
> -class TrafficGenerator(ABC):
> +class TrafficGenerator(MultiInheritanceBaseClass, ABC):
>       """The base traffic generator.
>   
>       Exposes the common public methods of all traffic generators and defines private methods
> -    that must implement the traffic generation logic in subclasses.
> +    that must implement the traffic generation logic in subclasses. This class also extends from
> +    :class:`framework.utils.MultiInheritanceBaseClass` to allow subclasses the ability to inherit
> +    from multiple classes to fulfil the traffic generating functionality without breaking
> +    single-inheritance.

Hyphen again.

>       """
>   
>       _config: TrafficGeneratorConfig
>       _tg_node: Node
>       _logger: DTSLogger
>   
> -    def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> +    def __init__(self, tg_node: Node, config: TrafficGeneratorConfig, **kwargs):
>           """Initialize the traffic generator.
>   
> +        Additional key-word arguments can be passed through `kwargs` if needed for fulfilling other
> +        constructors in the case of multiple-inheritance.
> +

keyword, multiple inheritance