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 715F443382; Wed, 22 Nov 2023 14:11:42 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E2115402E8; Wed, 22 Nov 2023 14:11:41 +0100 (CET) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by mails.dpdk.org (Postfix) with ESMTP id E5B494028C for ; Wed, 22 Nov 2023 14:11:40 +0100 (CET) Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-a00ac0101d9so355457866b.0 for ; Wed, 22 Nov 2023 05:11:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700658699; x=1701263499; darn=dpdk.org; 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=tM1OQwKxwC/jaOKiznkGNn8WXkur3+BQhhl5MhQgmQo=; b=jkvh2UkzpK5xnaKfQ5oZjtjuOO+F5k/9Pgu/B/gifyKtiYNOofy17qGODXvWp56rT9 64byRnBSVvc68sJxhDoE0m2es+azJJVZlLcz8xu3Ozu7XWB5h2PahQJCP+L89+3PnXL1 hZ/C6TQP/9gNDna+Iv3ZuiyS5I0dybce4t4cgj7MZPUzAnKQl/3HawZqgHpWB6OKi+xe CzE2ESaG1I1DxqFQH1wYoW9xyJ6mzWn8wmYhcIszYq7SUFZ8NceVroSPT5B5Qyu2zawE Z5/729VmT9M5grOFVZAtRTmSbJE1kSymZKSi0FCzFlGufb457oNwHgss1ubg34RDW+V7 zDUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700658699; x=1701263499; 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=tM1OQwKxwC/jaOKiznkGNn8WXkur3+BQhhl5MhQgmQo=; b=gag+NvVeVqK3js8HrnAG9j5vgUlaeaLuBLzCZICGSMWPlaTtIPN+cGVdM2HYuttNc1 nVu9GESTL8E7OlnqGO9jxO2f/VWMrpUySpgPzgsABVQKE+X4PhY7WhiHYQUi4mdArDV4 /IN1bPYPKg+JFsuFwvxU7i6FOBCeFHZFp9I89rk7FP/YlR7Avda/54fRDRso0upEys73 W5e5o5a3+wHRw8/nkq4lgOeTFuJTL0nWVhNee3DFjFeCqABIVSiz5vR86+Zvpp7TsuTk ZJeW3pd5R/Kr71JEPKr7GR9iY2nQO+7xR/JEV/rhqNlYMNre3Agv33eGHi6XGbzFrbua /Agg== X-Gm-Message-State: AOJu0YwdGgMKVi2DdbjW0x8CL2Q7E8VjlMzjP4awEkXza+j+FHJFc2xr NBGjgbUM89G6IxHz8ohbjej8zliy7qqOb2mAVVqfeMLA3IYFbPOYELg= X-Google-Smtp-Source: AGHT+IFteheM6W4MiAWmAXeMFs7doWmZ4r58ARGEKY7rfn9tId/u3BkSqzOlpb3QNUx+FPtAqb18VyxoUtjf4wbV30c= X-Received: by 2002:a17:906:c3a4:b0:a03:9671:14d5 with SMTP id t36-20020a170906c3a400b00a03967114d5mr300142ejz.32.1700658699619; Wed, 22 Nov 2023 05:11:39 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 14:11:28 +0100 Message-ID: Subject: Re: [PATCH v7 19/21] dts: base traffic generators docstring update To: Yoan Picchi Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, 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 Wed, Nov 22, 2023 at 1:05=E2=80=AFPM Yoan Picchi wrote: > > On 11/22/23 11:38, Juraj Linke=C5=A1 wrote: > > On Tue, Nov 21, 2023 at 5:20=E2=80=AFPM Yoan Picchi wrote: > >> > >> On 11/15/23 13:09, Juraj Linke=C5=A1 wrote: > >>> Format according to the Google format and PEP257, with slight > >>> deviations. > >>> > >>> Signed-off-by: Juraj Linke=C5=A1 > >>> --- > >>> .../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__.p= y 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 monito= r 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. > I like this, I'll reword it. > > > >>> + > >>> +A traffic generator may be software running on generic hardware or i= t could be specialized hardware. > >>> + > >>> +The traffic generators that only count the number of received packet= s are suitable only for > >>> +performance testing. In functional testing, we need to be able to di= ssect each arrived packet > >>> +and a capturing traffic generator is required. > >>> +""" > >>> + > >>> from framework.config import ScapyTrafficGeneratorConfig, TrafficG= eneratorType > >>> 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: ScapyTrafficGenerator= Config > >>> ) -> CapturingTrafficGenerator: > >>> - """A factory function for creating traffic generator object from= user config.""" > >>> + """The factory function for creating traffic generator objects f= rom the test run configuration. > >>> + > >>> + Args: > >>> + tg_node: The traffic generator node where the created traffi= c 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_generato= r_config) > >>> diff --git a/dts/framework/testbed_model/traffic_generator/capturing_= traffic_generator.py b/dts/framework/testbed_model/traffic_generator/captur= ing_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 capt= ure names. > >>> - """ > >>> return str(uuid.uuid4()) > >>> > >>> > >>> class CapturingTrafficGenerator(TrafficGenerator): > >>> """Capture packets after sending traffic. > >>> > >>> - A mixin interface which enables a packet generator to declare th= at it can capture > >>> + The intermediary interface which enables a packet generator to d= eclare that it can capture > >>> packets and return them to the user. > >>> > >>> + Similarly to > >>> + :class:`~framework.testbed_model.traffic_generator.traffic_gener= ator.TrafficGenerator`, > >>> + this class exposes the public methods specific to capturing traf= fic 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 =3D _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 cap= tured > >>> + on `receive_port` for the given `duration`. > >>> > >>> - Send a packet on the send_port and then return all traffic c= aptured > >>> - 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 stor= e 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 ca= ptured. > >>> """ > >>> return self.send_packets_and_capture( > >>> [packet], send_port, receive_port, duration, capture_n= ame > >>> @@ -82,11 +87,14 @@ def send_packets_and_capture( > >>> duration: float, > >>> capture_name: str =3D _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 ca= ptured > >>> - 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 ca= ptured > >>> + 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 li= ne 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 stor= e 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 ca= ptured. > >>> """ > >>> 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 recei= ve_port > >>> - for the specified duration. It must be able to handle no rec= eived packets. > >>> + """The implementation of :method:`send_packets_and_capture`. > >>> + > >>> + The subclasses must implement this method which sends `packe= ts` on `send_port` > >>> + and receives packets on `receive_port` for the specified `du= ration`. > >>> + > >>> + It must be able to handle no received packets. > >> > >> This sentence feels odd too. Maybe "It must be able to handle receivin= g > >> no packets." > >> > > > > Right, your suggestion is better. > > > >>> """ > >>> > >>> def _write_capture_from_packets( > >>> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_ge= nerator.py b/dts/framework/testbed_model/traffic_generator/traffic_generato= r.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 i= mplement. > >>> + 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: TrafficGeneratorConf= ig): > >>> + """Initialize the traffic generator. > >>> + > >>> + Args: > >>> + tg_node: The traffic generator node where the created tr= affic generator will be running. > >>> + config: The traffic generator's test run configuration. > >>> + """ > >>> self._config =3D config > >>> self._tg_node =3D tg_node > >>> self._logger =3D getLogger( > >>> @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGe= neratorConfig): > >>> ) > >>> > >>> 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 s= ent. > >>> > >>> 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) -> N= one: > >>> - """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 full= y sent. > >>> > >>> Args: > >>> packets: The packets to send. > >>> @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], por= t: 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 al= l packets > >>> - are fully sent. > >>> + """The implementation of :method:`send_packets`. > >>> + > >>> + The subclasses must implement this method which sends `packe= ts` 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 > >> >