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

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

On Thu, Sep 26, 2024 at 5:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> 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

Ack.

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

I'm not sure why I put so many, haha. I'll look through and try to
find the rest.

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

Good idea, I'll add this.

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

Ack.

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

Ack.

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

Yes it is because of the close method in the traffic generator. I
think closing the shell should be all we need to do really, there
isn't anything else this traffic generator is really using on the
host.

>
>
> > +    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

I guess other leaf subclasses of traffic generators which don't
inherit from two things wouldn't actually need them, you're right. I
was just thinking that they had to be supported since the base class
supports them to allow for the multiple inheritance, but thinking more
on it now not all of them have to.

> supported keyword arguments corresdpond to the parameters of
> :meth:`PythonShell.__init__`.

Sure, I like this wording.

>
> Also, multiple-inheritance -> multiple inheritance

Clearly I need to do some reading about when I should actually be
using hyphens :).

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

Ack.

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

Ack.

>
>