From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E2A8242EB8; Wed, 19 Jul 2023 15:24:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A656D40E03; Wed, 19 Jul 2023 15:24:01 +0200 (CEST) Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by mails.dpdk.org (Postfix) with ESMTP id 1238040DFD for ; Wed, 19 Jul 2023 15:24:00 +0200 (CEST) Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-5216f44d881so7371500a12.1 for ; Wed, 19 Jul 2023 06:24:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689773039; x=1690377839; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=tG2GzO4ygVd/e7DV96T870jU8+QJQuQN/wx2qNy3Tlc=; b=E07rUR67BTlyqgSOUzFfjEoBI/bL0mVIi15Bns+AwtfpYlvzLPNOMHuDe0ZKMCSzeG KNFO+8b1aV+JnnS2/dVKnrUovhIbFoUMz9XFY7TTgbl3SQDi5n1wZNZLMNwCSZqKBmeo 9UAWp0eMHi6rjdiznRGsaYTGCWuTY2b3pX43hM8RyNz4ezAERwj6oChnmsh8Yy3Lv7Uh MGIt66/RR+p9Hd1alr2anAbCt2Dtc1N3677v6VHcJpxvE4gbVuF0S5mrzibvjlQOQxQk 4wy0sWwMloS/gmY+XQNo9q77UFt3rJTbCWOTeHg8d1cp3F1mKjxNCbqqiWmCqBLz6H1L ZW+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689773039; x=1690377839; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tG2GzO4ygVd/e7DV96T870jU8+QJQuQN/wx2qNy3Tlc=; b=YtYR2gqD8GOZ0CQEqhwDrEyh+hFDdQuM0NdpaBYOz4T4SSN9iD0OzLPRouoXAxQjb0 RG8UOxsPLuA+1lB+leQchqFNkLR3LV/lk5pBW+JHZJOYh6GDLjDZDb18D2wF9y1IJQNB Otp7C8hZ3nGPE3qUi9Udt4vcmnmCLmXUY1uneeCFpxBKQ4pWcOnGHblcNViPIO1OauNK FgPbHNWYWrO/4DlCFuBkq4ZdjSWPcrrqzsD7Dw6r5zpvNeR3UnvU4dtWPq79MIijizVZ V2HC4phWcI9A6ASoF7K3nUEksf5ju9ULegjyBimsW+fx6jtc3YmRyEb1vNxvR3/cmfjg XZww== X-Gm-Message-State: ABy/qLYE7q3UZzw9N+dV1i1uPyoUy2hjEIV1FtD2u+4ljOjvHV8aQCGq pN5OHl6cdhyfv9MpEtQJLT9vCvicYsNzzaK1GKFktQ== X-Google-Smtp-Source: APBJJlGPyLnh1Bm9a4qmoqlSqCHMXnjSYrVHCWlmUNsJ/9eThSBnuIgzeHi//2jfVeU05n9gJmEYh1OsMFERxxaDG04= X-Received: by 2002:a05:6402:350:b0:51a:5a25:6631 with SMTP id r16-20020a056402035000b0051a5a256631mr2392063edw.3.1689773039554; Wed, 19 Jul 2023 06:23:59 -0700 (PDT) MIME-Version: 1.0 References: <20230420093109.594704-1-juraj.linkes@pantheon.tech> <20230717110709.39220-1-juraj.linkes@pantheon.tech> <20230717110709.39220-4-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 19 Jul 2023 15:23:48 +0200 Message-ID: Subject: Re: [PATCH v2 3/6] dts: traffic generator abstractions To: Jeremy Spewock Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, lijuan.tu@intel.com, probb@iol.unh.edu, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jul 18, 2023 at 9:56=E2=80=AFPM Jeremy Spewock wrote: > > > Hey Juraj, > > Just a couple of comments below, but very minor stuff. Just a few docstri= ng that I commented on and one question about the factory for traffic gener= ators that I was wondering what you thought about. More below: > >> 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 t= he 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 work= flow: >> + 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 =3D _get_default_capture_name(), >> + ) -> list[Packet]: >> + """Send a packet, return received traffic. >> + >> + Send a packet on the send_port and then return all traffic capt= ured >> + on the receive_port for the given duration. Also record the cap= tured 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 sen= ding 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 =3D _get_default_capture_name(), >> + ) -> list[Packet]: >> + """Send packets, return received traffic. >> + >> + Send packets on the send_port and then return all traffic captu= red >> + on the receive_port for the given duration. Also record the cap= tured 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 sen= ding 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 =3D 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 receiv= ed packets. >> + """ >> + >> + def _write_capture_from_packets(self, capture_name: str, packets: l= ist[Packet]): >> + file_name =3D f"{SETTINGS.output_dir}/{capture_name}.pcap" >> + self._logger.debug(f"Writing packets to {file_name}.") >> + scapy.utils.wrpcap(file_name, packets) >> diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/test= bed_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 nod= e. >> +""" >> + >> +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 Generato= r node has >> + specialized methods for handling the traffic generator running on i= t. >> + >> + Arguments: >> + node_config: The user configuration of the traffic generator no= de. >> + >> + 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 =3D 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 =3D 1, >> + ) -> list[Packet]: >> + """Send a packet, return received traffic. >> + >> + Send a packet on the send_port and then return all traffic capt= ured >> + on the receive_port for the given duration. Also record the cap= tured 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 sen= ding 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: ScapyTrafficGeneratorCon= fig >> +) -> CapturingTrafficGenerator: >> + """A factory function for creating traffic generator object from us= er config.""" >> + >> + from .scapy import ScapyTrafficGenerator >> + >> + match traffic_generator_config.traffic_generator_type: >> + case TrafficGeneratorType.SCAPY: >> + return ScapyTrafficGenerator(tg_node, traffic_generator_con= fig) >> + 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_inte= ractive_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, ScapyTraffi= cGenerator), but I think it would be possible to change in the from_dict me= thod 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 w= e would map from conf.yaml to one of the classes and all of those needing t= o be imported in config/__init__.py) but it would save developers in the fu= ture from having to add to two different places (the enum in config/__ini= t__.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 traf= fic 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 b= e 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/fram= ework/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 impl= ement. >> + """ >> + >> + _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 p= ackets >> + 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 ot= herwise. >> + """ >> + 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) =3D=3D 1: >> + packet_summaries =3D packets[0].summary() >> + else: >> + packet_summaries =3D json.dumps( >> + list(map(lambda pkt: pkt.summary(), packets)), indent=3D4 >> + ) >> + return f"Packet contents: \n{packet_summaries}" >> + >> + >> def RED(text: str) -> str: >> return f"\u001B[31;1m{str(text)}\u001B[0m" >> >> -- >> 2.34.1 >>