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 7E1F54339B; Wed, 22 Nov 2023 12:56:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4702640DDC; Wed, 22 Nov 2023 12:56:50 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id D66294028C for ; Wed, 22 Nov 2023 12:56:48 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 187BC1595; Wed, 22 Nov 2023 03:57:35 -0800 (PST) Received: from [10.57.72.130] (unknown [10.57.72.130]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 496D93F7A6; Wed, 22 Nov 2023 03:56:46 -0800 (PST) Message-ID: Date: Wed, 22 Nov 2023 11:56:39 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 19/21] dts: base traffic generators docstring update Content-Language: en-US To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, dev@dpdk.org References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-20-juraj.linkes@pantheon.tech> <77f91fce-7208-4918-badc-0213f254a75e@foss.arm.com> From: Yoan Picchi In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 11/22/23 11:38, Juraj Linkeš wrote: > On Tue, Nov 21, 2023 at 5:20 PM Yoan Picchi wrote: >> >> On 11/15/23 13:09, Juraj Linkeš wrote: >>> Format according to the Google format and PEP257, with slight >>> deviations. >>> >>> Signed-off-by: Juraj Linkeš >>> --- >>> .../traffic_generator/__init__.py | 22 ++++++++- >>> .../capturing_traffic_generator.py | 46 +++++++++++-------- >>> .../traffic_generator/traffic_generator.py | 33 +++++++------ >>> 3 files changed, 68 insertions(+), 33 deletions(-) >>> >>> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py >>> index 11bfa1ee0f..51cca77da4 100644 >>> --- a/dts/framework/testbed_model/traffic_generator/__init__.py >>> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py >>> @@ -1,6 +1,19 @@ >>> # SPDX-License-Identifier: BSD-3-Clause >>> # Copyright(c) 2023 PANTHEON.tech s.r.o. >>> >>> +"""DTS traffic generators. >>> + >>> +A traffic generator is capable of generating traffic and then monitor returning traffic. >>> +A traffic generator may just count the number of received packets >>> +and it may additionally capture individual packets. >> >> The sentence feels odd. Isn't it supposed to be "or" here? and no need >> for that early of a line break >> > > There are two mays, so there probably should be an or. But I'd like to > reword it to this: > > All traffic generators count the number of received packets, and they > may additionally > capture individual packets. > > What do you think? I think it's better with the new sentence. But I think it'd be even better to split into two sentences to highlight the must/may: All traffic generators must count the number of received packets. Some may additionally capture individual packets. > >>> + >>> +A traffic generator may be software running on generic hardware or it could be specialized hardware. >>> + >>> +The traffic generators that only count the number of received packets are suitable only for >>> +performance testing. In functional testing, we need to be able to dissect each arrived packet >>> +and a capturing traffic generator is required. >>> +""" >>> + >>> from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType >>> from framework.exception import ConfigurationError >>> from framework.testbed_model.node import Node >>> @@ -12,8 +25,15 @@ >>> def create_traffic_generator( >>> tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig >>> ) -> CapturingTrafficGenerator: >>> - """A factory function for creating traffic generator object from user config.""" >>> + """The factory function for creating traffic generator objects from the test run configuration. >>> + >>> + Args: >>> + tg_node: The traffic generator node where the created traffic generator will be running. >>> + traffic_generator_config: The traffic generator config. >>> >>> + Returns: >>> + A traffic generator capable of capturing received packets. >>> + """ >>> match traffic_generator_config.traffic_generator_type: >>> case TrafficGeneratorType.SCAPY: >>> return ScapyTrafficGenerator(tg_node, traffic_generator_config) >>> diff --git a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py >>> index e521211ef0..b0a43ad003 100644 >>> --- a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py >>> +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py >>> @@ -23,19 +23,22 @@ >>> >>> >>> 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): >>> """Capture packets after sending traffic. >>> >>> - A mixin interface which enables a packet generator to declare that it can capture >>> + The intermediary interface which enables a packet generator to declare that it can capture >>> packets and return them to the user. >>> >>> + Similarly to >>> + :class:`~framework.testbed_model.traffic_generator.traffic_generator.TrafficGenerator`, >>> + this class exposes the public methods specific to capturing traffic generators and defines >>> + a private method that must implement the traffic generation and capturing logic in subclasses. >>> + >>> The methods of capturing traffic generators obey the following workflow: >>> + >>> 1. send packets >>> 2. capture packets >>> 3. write the capture to a .pcap file >>> @@ -44,6 +47,7 @@ class CapturingTrafficGenerator(TrafficGenerator): >>> >>> @property >>> def is_capturing(self) -> bool: >>> + """This traffic generator can capture traffic.""" >>> return True >>> >>> def send_packet_and_capture( >>> @@ -54,11 +58,12 @@ def send_packet_and_capture( >>> duration: float, >>> capture_name: str = _get_default_capture_name(), >>> ) -> list[Packet]: >>> - """Send a packet, return received traffic. >>> + """Send `packet` and capture received traffic. >>> + >>> + Send `packet` on `send_port` and then return all traffic captured >>> + on `receive_port` for the given `duration`. >>> >>> - 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. >>> + The captured traffic is recorded in the `capture_name`.pcap file. >>> >>> Args: >>> packet: The packet to send. >>> @@ -68,7 +73,7 @@ def send_packet_and_capture( >>> 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. >>> + The received packets. May be empty if no packets are captured. >>> """ >>> return self.send_packets_and_capture( >>> [packet], send_port, receive_port, duration, capture_name >>> @@ -82,11 +87,14 @@ def send_packets_and_capture( >>> duration: float, >>> capture_name: str = _get_default_capture_name(), >>> ) -> list[Packet]: >>> - """Send packets, return received traffic. >>> + """Send `packets` and capture 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. >>> + Send `packets` on `send_port` and then return all traffic captured >>> + on `receive_port` for the given `duration`. >>> + >>> + The captured traffic is recorded in the `capture_name`.pcap file. The target directory >>> + can be configured with the :option:`--output-dir` command line argument or >>> + the :envvar:`DTS_OUTPUT_DIR` environment variable. >>> >>> Args: >>> packets: The packets to send. >>> @@ -96,7 +104,7 @@ def send_packets_and_capture( >>> 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. >>> + The received packets. May be empty if no packets are captured. >>> """ >>> self._logger.debug(get_packet_summaries(packets)) >>> self._logger.debug( >>> @@ -124,10 +132,12 @@ def _send_packets_and_capture( >>> 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. >>> + """The implementation of :method:`send_packets_and_capture`. >>> + >>> + The subclasses must implement this method which sends `packets` on `send_port` >>> + and receives packets on `receive_port` for the specified `duration`. >>> + >>> + It must be able to handle no received packets. >> >> This sentence feels odd too. Maybe "It must be able to handle receiving >> no packets." >> > > Right, your suggestion is better. > >>> """ >>> >>> def _write_capture_from_packets( >>> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py >>> index ea7c3963da..ed396c6a2f 100644 >>> --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py >>> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py >>> @@ -22,7 +22,8 @@ >>> class TrafficGenerator(ABC): >>> """The base traffic generator. >>> >>> - Defines the few basic methods that each traffic generator must implement. >>> + Exposes the common public methods of all traffic generators and defines private methods >>> + that must implement the traffic generation logic in subclasses. >>> """ >>> >>> _config: TrafficGeneratorConfig >>> @@ -30,6 +31,12 @@ class TrafficGenerator(ABC): >>> _logger: DTSLOG >>> >>> def __init__(self, tg_node: Node, config: TrafficGeneratorConfig): >>> + """Initialize the traffic generator. >>> + >>> + Args: >>> + tg_node: The traffic generator node where the created traffic generator will be running. >>> + config: The traffic generator's test run configuration. >>> + """ >>> self._config = config >>> self._tg_node = tg_node >>> self._logger = getLogger( >>> @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig): >>> ) >>> >>> def send_packet(self, packet: Packet, port: Port) -> None: >>> - """Send a packet and block until it is fully sent. >>> + """Send `packet` and block until it is fully sent. >>> >>> - What fully sent means is defined by the traffic generator. >>> + Send `packet` on `port`, then wait until `packet` is fully sent. >>> >>> Args: >>> packet: The packet to send. >>> @@ -48,9 +55,9 @@ def send_packet(self, packet: Packet, port: Port) -> None: >>> self.send_packets([packet], port) >>> >>> def send_packets(self, packets: list[Packet], port: Port) -> None: >>> - """Send packets and block until they are fully sent. >>> + """Send `packets` and block until they are fully sent. >>> >>> - What fully sent means is defined by the traffic generator. >>> + Send `packets` on `port`, then wait until `packets` are fully sent. >>> >>> Args: >>> packets: The packets to send. >>> @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], port: Port) -> None: >>> >>> @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. >>> + """The implementation of :method:`send_packets`. >>> + >>> + The subclasses must implement this method which sends `packets` on `port`. >>> + The method should block until all `packets` are fully sent. >>> + >>> + What full sent means is defined by the traffic generator. >> >> full -> fully >> >>> """ >>> >>> @property >>> def is_capturing(self) -> bool: >>> - """Whether this traffic generator can capture traffic. >>> - >>> - Returns: >>> - True if the traffic generator can capture traffic, False otherwise. >>> - """ >>> + """This traffic generator can't capture traffic.""" >>> return False >>> >>> @abstractmethod >>