Re: [PATCH v2 3/6] dts: traffic generator abstractions

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

On Tue, Jul 18, 2023 at 9:56 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
> Hey Juraj,
>
> Just a couple of comments below, but very minor stuff. Just a few docstring that I commented on and one question about the factory for traffic generators that I was wondering what you thought about. More below:
>

<snip>

>> diff --git a/dts/framework/testbed_model/capturing_traffic_generator.py b/dts/framework/testbed_model/capturing_traffic_generator.py
>> new file mode 100644
>> index 0000000000..1130d87f1e
>> --- /dev/null
>> +++ b/dts/framework/testbed_model/capturing_traffic_generator.py
>> @@ -0,0 +1,135 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2022 University of New Hampshire
>> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
>> +
>> +"""Traffic generator that can capture packets.
>> +
>> +In functional testing, we need to interrogate received packets to check their validity.
>> +Here we define the interface common to all
>> +traffic generators capable of capturing traffic.
>
>
> Is there a reason for the line break here? Just to keep things consistent I think it might make sense to extend this line to be the same length as the one above.
>

The reason is the full line is above the character limit. I was using
Thomas's heuristic of splitting the lines where it makes logical
sense, but it does look kinda wonky, so I'll try to reformat it more
sensibly.

>>
>> +"""
>> +
>> +import uuid
>> +from abc import abstractmethod
>> +
>> +import scapy.utils  # type: ignore[import]
>> +from scapy.packet import Packet  # type: ignore[import]
>> +
>> +from framework.settings import SETTINGS
>> +from framework.utils import get_packet_summaries
>> +
>> +from .hw.port import Port
>> +from .traffic_generator import TrafficGenerator
>> +
>> +
>> +def _get_default_capture_name() -> str:
>> +    """
>> +    This is the function used for the default implementation of capture names.
>> +    """
>> +    return str(uuid.uuid4())
>> +
>> +
>> +class CapturingTrafficGenerator(TrafficGenerator):
>> +    """
>> +    A mixin interface which enables a packet generator to declare that it can capture
>> +    packets and return them to the user.
>
>
> This is missing the one line summary at the top of the comment. Obviously this is not a big issue, but we likely would want this to be uniform with the rest of the module which does have the summary at the top.
>

Good catch. I plan on reviewing all of the docstring when updating the
format of the docstrings so this means less work in the future. :-)

>>
>> +
>> +    The methods of capturing traffic generators obey the following workflow:
>> +        1. send packets
>> +        2. capture packets
>> +        3. write the capture to a .pcap file
>> +        4. return the received packets
>> +    """
>> +
>> +    @property
>> +    def is_capturing(self) -> bool:
>> +        return True
>> +
>> +    def send_packet_and_capture(
>> +        self,
>> +        packet: Packet,
>> +        send_port: Port,
>> +        receive_port: Port,
>> +        duration: float,
>> +        capture_name: str = _get_default_capture_name(),
>> +    ) -> list[Packet]:
>> +        """Send a packet, return received traffic.
>> +
>> +        Send a packet on the send_port and then return all traffic captured
>> +        on the receive_port for the given duration. Also record the captured traffic
>> +        in a pcap file.
>> +
>>
>> +        Args:
>> +            packet: The packet to send.
>> +            send_port: The egress port on the TG node.
>> +            receive_port: The ingress port in the TG node.
>> +            duration: Capture traffic for this amount of time after sending the packet.
>> +            capture_name: The name of the .pcap file where to store the capture.
>> +
>> +        Returns:
>> +             A list of received packets. May be empty if no packets are captured.
>> +        """
>> +        return self.send_packets_and_capture(
>> +            [packet], send_port, receive_port, duration, capture_name
>> +        )
>> +
>> +    def send_packets_and_capture(
>> +        self,
>> +        packets: list[Packet],
>> +        send_port: Port,
>> +        receive_port: Port,
>> +        duration: float,
>> +        capture_name: str = _get_default_capture_name(),
>> +    ) -> list[Packet]:
>> +        """Send packets, return received traffic.
>> +
>> +        Send packets on the send_port and then return all traffic captured
>> +        on the receive_port for the given duration. Also record the captured traffic
>> +        in a pcap file.
>> +
>> +        Args:
>> +            packets: The packets to send.
>> +            send_port: The egress port on the TG node.
>> +            receive_port: The ingress port in the TG node.
>> +            duration: Capture traffic for this amount of time after sending the packets.
>> +            capture_name: The name of the .pcap file where to store the capture.
>> +
>> +        Returns:
>> +             A list of received packets. May be empty if no packets are captured.
>> +        """
>> +        self._logger.debug(get_packet_summaries(packets))
>> +        self._logger.debug(
>> +            f"Sending packet on {send_port.logical_name}, "
>> +            f"receiving on {receive_port.logical_name}."
>> +        )
>> +        received_packets = self._send_packets_and_capture(
>> +            packets,
>> +            send_port,
>> +            receive_port,
>> +            duration,
>> +        )
>> +
>> +        self._logger.debug(
>> +            f"Received packets: {get_packet_summaries(received_packets)}"
>> +        )
>> +        self._write_capture_from_packets(capture_name, received_packets)
>> +        return received_packets
>> +
>> +    @abstractmethod
>> +    def _send_packets_and_capture(
>> +        self,
>> +        packets: list[Packet],
>> +        send_port: Port,
>> +        receive_port: Port,
>> +        duration: float,
>> +    ) -> list[Packet]:
>> +        """
>> +        The extended classes must implement this method which
>> +        sends packets on send_port and receives packets on the receive_port
>> +        for the specified duration. It must be able to handle no received packets.
>> +        """
>> +
>> +    def _write_capture_from_packets(self, capture_name: str, packets: list[Packet]):
>> +        file_name = f"{SETTINGS.output_dir}/{capture_name}.pcap"
>> +        self._logger.debug(f"Writing packets to {file_name}.")
>> +        scapy.utils.wrpcap(file_name, packets)

<snip>

>> diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
>> new file mode 100644
>> index 0000000000..27025cfa31
>> --- /dev/null
>> +++ b/dts/framework/testbed_model/tg_node.py
>> @@ -0,0 +1,99 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2010-2014 Intel Corporation
>> +# Copyright(c) 2022 University of New Hampshire
>> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
>> +
>> +"""Traffic generator node.
>> +
>> +This is the node where the traffic generator resides.
>> +The distinction between a node and a traffic generator is as follows:
>> +A node is a host that DTS connects to. It could be a baremetal server,
>> +a VM or a container.
>> +A traffic generator is software running on the node.
>> +A traffic generator node is a node running a traffic generator.
>> +A node can be a traffic generator node as well as system under test node.
>> +"""
>> +
>> +from scapy.packet import Packet  # type: ignore[import]
>> +
>> +from framework.config import (
>> +    ScapyTrafficGeneratorConfig,
>> +    TGNodeConfiguration,
>> +    TrafficGeneratorType,
>> +)
>> +from framework.exception import ConfigurationError
>> +
>> +from .capturing_traffic_generator import CapturingTrafficGenerator
>> +from .hw.port import Port
>> +from .node import Node
>> +
>> +
>> +class TGNode(Node):
>> +    """Manage connections to a node with a traffic generator.
>> +
>> +    Apart from basic node management capabilities, the Traffic Generator node has
>> +    specialized methods for handling the traffic generator running on it.
>> +
>> +    Arguments:
>> +        node_config: The user configuration of the traffic generator node.
>> +
>> +    Attributes:
>> +        traffic_generator: The traffic generator running on the node.
>> +    """
>> +
>> +    traffic_generator: CapturingTrafficGenerator
>> +
>> +    def __init__(self, node_config: TGNodeConfiguration):
>> +        super(TGNode, self).__init__(node_config)
>> +        self.traffic_generator = create_traffic_generator(
>> +            self, node_config.traffic_generator
>> +        )
>> +        self._logger.info(f"Created node: {self.name}")
>> +
>> +    def send_packet_and_capture(
>> +        self,
>> +        packet: Packet,
>> +        send_port: Port,
>> +        receive_port: Port,
>> +        duration: float = 1,
>> +    ) -> list[Packet]:
>> +        """Send a packet, return received traffic.
>> +
>> +        Send a packet on the send_port and then return all traffic captured
>> +        on the receive_port for the given duration. Also record the captured traffic
>> +        in a pcap file.
>> +
>> +        Args:
>> +            packet: The packet to send.
>> +            send_port: The egress port on the TG node.
>> +            receive_port: The ingress port in the TG node.
>> +            duration: Capture traffic for this amount of time after sending the packet.
>> +
>> +        Returns:
>> +             A list of received packets. May be empty if no packets are captured.
>> +        """
>> +        return self.traffic_generator.send_packet_and_capture(
>> +            packet, send_port, receive_port, duration
>> +        )
>> +
>> +    def close(self) -> None:
>> +        """Free all resources used by the node"""
>> +        self.traffic_generator.close()
>> +        super(TGNode, self).close()
>> +
>> +
>> +def create_traffic_generator(
>> +    tg_node: TGNode, traffic_generator_config: ScapyTrafficGeneratorConfig
>> +) -> CapturingTrafficGenerator:
>> +    """A factory function for creating traffic generator object from user config."""
>> +
>> +    from .scapy import ScapyTrafficGenerator
>> +
>> +    match traffic_generator_config.traffic_generator_type:
>> +        case TrafficGeneratorType.SCAPY:
>> +            return ScapyTrafficGenerator(tg_node, traffic_generator_config)
>> +        case _:
>> +            raise ConfigurationError(
>> +                "Unknown traffic generator: "
>> +                f"{traffic_generator_config.traffic_generator_type}"
>> +            )
>
>
> Would it be possible here to do something like what we did in create_interactive_shell with a TypeVar where we can initialize it directly? It would change from using the enum to setting the traffic_generator_config.traffic_generator_type to a specific class in the config (in this case, ScapyTrafficGenerator), but I think it would be possible to change in the from_dict method where we could set this type to the class directly instead of the enum (or maybe had the enum relate it's values to the classes themselves).
>
> I think this would make some things slightly more complicated (like how we would map from conf.yaml to one of the classes and all of those needing to be imported in config/__init__.py) but it would save developers in the future from having to add to two different places  (the enum in  config/__init__.py and this match statement) and save this list from being arbitrarily long. I think this is fine for this patch but maybe when we expand the traffic generator or the scapy generator it could be worth thinking about.
>
> Do you think it would make sense to change it in this way or would that be somewhat unnecessary in your eyes?
>

That would be great, but the problem with using any object from the
framework in config is cyclical imports. Almost everything imports
config, so config can't import anything from the framework, at least
on the top level.
But it looks like it can be done with imports in the from_dict method.
This is an interesting approach, I'll think about it. The best would
probably be to leave this as is and use this approach for more things
than just the traffic generator.
There's one more thing to consider and that is will the imports work
with the constraints imposed by documentation generation. I'm thinking
let's leave this as is and try this when updating the code for doc's
needs.

>>
>> diff --git a/dts/framework/testbed_model/traffic_generator.py b/dts/framework/testbed_model/traffic_generator.py
>> new file mode 100644
>> index 0000000000..28c35d3ce4
>> --- /dev/null
>> +++ b/dts/framework/testbed_model/traffic_generator.py
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2022 University of New Hampshire
>> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
>> +
>> +"""The base traffic generator.
>> +
>> +These traffic generators can't capture received traffic,
>> +only count the number of received packets.
>> +"""
>> +
>> +from abc import ABC, abstractmethod
>> +
>> +from scapy.packet import Packet  # type: ignore[import]
>> +
>> +from framework.logger import DTSLOG
>> +from framework.utils import get_packet_summaries
>> +
>> +from .hw.port import Port
>> +
>> +
>> +class TrafficGenerator(ABC):
>> +    """The base traffic generator.
>> +
>> +    Defines the few basic methods that each traffic generator must implement.
>> +    """
>> +
>> +    _logger: DTSLOG
>> +
>> +    def send_packet(self, packet: Packet, port: Port) -> None:
>> +        """Send a packet and block until it is fully sent.
>> +
>> +        What fully sent means is defined by the traffic generator.
>> +
>> +        Args:
>> +            packet: The packet to send.
>> +            port: The egress port on the TG node.
>> +        """
>> +        self.send_packets([packet], port)
>> +
>> +    def send_packets(self, packets: list[Packet], port: Port) -> None:
>> +        """Send packets and block until they are fully sent.
>> +
>> +        What fully sent means is defined by the traffic generator.
>> +
>> +        Args:
>> +            packets: The packets to send.
>> +            port: The egress port on the TG node.
>> +        """
>> +        self._logger.info(f"Sending packet{'s' if len(packets) > 1 else ''}.")
>> +        self._logger.debug(get_packet_summaries(packets))
>> +        self._send_packets(packets, port)
>> +
>> +    @abstractmethod
>> +    def _send_packets(self, packets: list[Packet], port: Port) -> None:
>> +        """
>> +        The extended classes must implement this method which
>> +        sends packets on send_port. The method should block until all packets
>> +        are fully sent.
>> +        """
>> +
>> +    @property
>> +    def is_capturing(self) -> bool:
>> +        """Whether this traffic generator can capture traffic.
>> +
>> +        Returns:
>> +            True if the traffic generator can capture traffic, False otherwise.
>> +        """
>> +        return False
>> +
>> +    @abstractmethod
>> +    def close(self) -> None:
>> +        """Free all resources used by the traffic generator."""
>> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
>> index 60abe46edf..d27c2c5b5f 100644
>> --- a/dts/framework/utils.py
>> +++ b/dts/framework/utils.py
>> @@ -4,6 +4,7 @@
>>  # Copyright(c) 2022-2023 University of New Hampshire
>>
>>  import atexit
>> +import json
>>  import os
>>  import subprocess
>>  import sys
>> @@ -11,6 +12,8 @@
>>  from pathlib import Path
>>  from subprocess import SubprocessError
>>
>> +from scapy.packet import Packet  # type: ignore[import]
>> +
>>  from .exception import ConfigurationError
>>
>>
>> @@ -64,6 +67,16 @@ def expand_range(range_str: str) -> list[int]:
>>      return expanded_range
>>
>>
>> +def get_packet_summaries(packets: list[Packet]):
>> +    if len(packets) == 1:
>> +        packet_summaries = packets[0].summary()
>> +    else:
>> +        packet_summaries = json.dumps(
>> +            list(map(lambda pkt: pkt.summary(), packets)), indent=4
>> +        )
>> +    return f"Packet contents: \n{packet_summaries}"
>> +
>> +
>>  def RED(text: str) -> str:
>>      return f"\u001B[31;1m{str(text)}\u001B[0m"
>>
>> --
>> 2.34.1
>>