Re: [RFC PATCH v1 2/2] dts: Remove XML-RPC server for Scapy TG and instead us ScapyShell

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

On Tue, Jun 11, 2024 at 6:46 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> > diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
> > index 5676235119..2b299ad02f 100644
> > --- a/dts/framework/testbed_model/traffic_generator/scapy.py
> > +++ b/dts/framework/testbed_model/traffic_generator/scapy.py
>
> >
> >   class ScapyTrafficGenerator(CapturingTrafficGenerator):
> > -    """Provides access to scapy functions via an RPC interface.
> > +    """Provides access to scapy functions on a traffic generator.
> >
>
> traffic generator node

Ack.

>
> >       This class extends the base with remote execution of scapy functions.
> >
> > -    Any packets sent to the remote server are first converted to bytes. They are received as
> > -    :class:`~xmlrpc.client.Binary` objects on the server side. When the server sends the packets
> > -    back, they are also received as :class:`~xmlrpc.client.Binary` objects on the client side, are
> > -    converted back to :class:`~scapy.packet.Packet` objects and only then returned from the methods.
> > +    All processing of packets is handled via an instance of a
> > +    :class:`framework.remote_session.scapy_shell.ScapyShell` that runs on the underlying
> > +    :class:`framework.testbed_model.tg_node.TGNode`.
> >
>
> The module docstring should also be updated.

Oops, good catch.

>
> >       Attributes:
> >           session: The exclusive interactive remote session created by the Scapy
> > -            traffic generator where the XML-RPC server runs.
> > -        rpc_server_proxy: The object used by clients to execute functions
> > -            on the XML-RPC server.
> > +            traffic generator.
> >       """
> >
> > -    session: PythonShell
> > -    rpc_server_proxy: xmlrpc.client.ServerProxy
> > +    session: ScapyShell
> >       _config: ScapyTrafficGeneratorConfig
> >
> >       def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
> >           """Extend the constructor with Scapy TG specifics.
> >
> > -        The traffic generator first starts an XML-RPC on the remote `tg_node`.
> > -        Then it populates the server with functions which use the Scapy library
> > -        to send/receive traffic:
> > -
> > -            * :func:`scapy_send_packets_and_capture`
> > -            * :func:`scapy_send_packets`
> > -
> > -        To enable verbose logging from the xmlrpc client, use the :option:`--verbose`
> > -        command line argument or the :envvar:`DTS_VERBOSE` environment variable.
> > +        The traffic generator starts an underlying session that handles scapy interactions
> > +        that it will use in its provided methods.
> >
>
> I'm not sure what you're trying to say here - that the methods the tg
> exposes are using the scapy session?

Yeah, that is exactly what I was trying to say, just that the TG
creates a PythonShell and that's what it uses to interact with scapy
when you call it basically.

>
> >           Args:
> >               tg_node: The node where the traffic generator resides.
> > @@ -262,50 +62,11 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
> >           ), "Linux is the only supported OS for scapy traffic generation"
> >
> >           self.session = self._tg_node.create_interactive_shell(
>
> Looks like in this specific case, we could do this with multiple
> inheritance instead of composition.
>
> Composition is needed in the other use cases, since we use different
> objects based on the config (e.g. Linux or Windows session). Here, we're
> always going to use the same object (ScapyShell).
>
> The code would need to be refactored to achieve multiple inheritance
> (the __init__ methods would probably have to accept extra kwargs) and
> Luca's testpmd params patch would help a lot, as that looks at least
> somewhat suitable.
>
> I don't know how well would multiple inheritance work, if at all, but
> it's worth trying so that we don't have to basically copy-paste the same
> method signature over and over (e.g. _send_packets and send_packets in
> ScapyTrafficGenerator and ScapyShell).

I like this idea. Multiple inheritance is something that I haven't
used much myself but I agree that creating wrapper methods over and
over for the methods that I want to use is very unintuitive. I'll try
it and see how far I can get with it.

>