[v2,1/1] dts: add text parser for testpmd verbose output

Message ID 20240730133459.21907-2-jspewock@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series dts: testpmd verbose parser |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Jeremy Spewock July 30, 2024, 1:34 p.m. UTC
From: Jeremy Spewock <jspewock@iol.unh.edu>

Multiple test suites from the old DTS framework rely on being able to
consume and interpret the verbose output of testpmd. The new framework
doesn't have an elegant way for handling the verbose output, but test
suites are starting to be written that rely on it. This patch creates a
TextParser class that can be used to extract the verbose information
from any testpmd output and also adjusts the `stop` method of the shell
to return all output that it collected.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---

One thing to note here is I don't love the regex in
extract_verbose_output(). It works great when there is a bunch of
verbose output in a row, but any chunk that isn't followed by another
piece of verbose output will contain everything that comes after it in
the match group. This could be solved by changing the regex to look
ahead only for the next port X/queue Y line instead of also including
the end of the string, and then having another alternate route which is
solely dedicated to the last block of verbose output which greedily
consumes everything until the end of ol_flags, but I didn't want to over
complicate the regex since the text parser will extract the specific
information it needs anyways. For reference, I was thinking it could be
something like this:

r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+)|port \d+/queue \d+:.*ol_flags: [\w ]+)"

but this has a lot of repition (some of which that could be ripped out
with a simple variable) and it is a little more confusing to read I
think.

 dts/framework/parser.py                       |  30 ++++
 dts/framework/remote_session/testpmd_shell.py | 146 +++++++++++++++++-
 dts/framework/utils.py                        |   1 +
 3 files changed, 175 insertions(+), 2 deletions(-)
  

Comments

Nicholas Pratte July 30, 2024, 3:41 p.m. UTC | #1
Good work! I left some probing/clarifying comments below for you.

<snip>
> +@dataclass
> +class TestPmdVerboseOutput(TextParser):
> +    """Verbose output generated by Testpmd.
> +
> +    This class is the top level of the output, containing verbose output delimited by
> +    "port X/queue Y: sent/received Z packets".
> +    """
> +
> +    #: ID of the port that handled the packet.
> +    port_id: int = field(metadata=TextParser.find_int(r"port (\d+)/queue \d+"))
> +    #: ID of the queue that handled the packet.
> +    queue_id: int = field(metadata=TextParser.find_int(r"port \d+/queue (\d+)"))
> +    #: Whether the packet was received or sent by the queue/port.
> +    was_received: bool = field(metadata=TextParser.find(r"received \d+ packets"))
> +    #: List of packets handed by the port/queue in this section.
> +    packets: list[TestPmdVerbosePacket] = field(
> +        metadata=TextParser.wrap(
> +            TextParser.find_all(r"(src=[\w\s=:-]+ol_flags: [\w ]+)"),
> +            lambda matches_arr: list(map(TestPmdVerbosePacket.parse, matches_arr)),
> +        )
> +    )
> +
> +
>  class TestPmdShell(DPDKShell):
>      """Testpmd interactive shell.
>
> @@ -645,7 +767,7 @@ def start(self, verify: bool = True) -> None:
>                          "Not all ports came up after starting packet forwarding in testpmd."
>                      )
>
> -    def stop(self, verify: bool = True) -> None:
> +    def stop(self, verify: bool = True) -> str:
>          """Stop packet forwarding.
>
>          Args:
> @@ -656,6 +778,9 @@ def stop(self, verify: bool = True) -> None:
>          Raises:
>              InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop
>                  forwarding results in an error.
> +
> +        Returns:
> +            Output gathered from sending the stop command.
>          """
>          stop_cmd_output = self.send_command("stop")
>          if verify:
> @@ -665,6 +790,7 @@ def stop(self, verify: bool = True) -> None:
>              ):
>                  self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
>                  raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
> +        return stop_cmd_output
<snip>
>
> +    @staticmethod
> +    def extract_verbose_output(output: str) -> list[TestPmdVerboseOutput]:
> +        """Extract the verbose information present in given testpmd output.
> +
> +        This method extracts sections of verbose output that begin with the line
> +        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
> +
> +        Args:
> +            output: Testpmd output that contains verbose information
> +
> +        Returns:
> +            List of parsed packet information gathered from verbose information in `output`.
> +        """
> +        iter = re.finditer(r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+|$))", output, re.S)
> +        return [TestPmdVerboseOutput.parse(s.group(0)) for s in iter]
> +

I'm trying to think of circumstances where we as developers would be
looking for anything else besides verbose output from the stop method.
Running the command outputs some statistics, but information like port
stats is displayed after the stop command is initiated. I'm
implementing this system right now for one of my test suites, and I'm
wondering if there might be any feasible way to extract output without
needing to input any explicit outputs into this method. I'm putting
output = testpmd.stop() and then calling this method. It looks
something like this:

verbose_output = testpmd.extract_verbose_output(testpmd.stop())

This is easy enough, but it might be a bit confusing for someone new
to the framework. The way that output is gathered is still elusive to
me, and I'm guessing that any commands run in-between setting verbose
mode and when you stop testpmd might influence how output is
generated. But in my experience so far, any statistics or information
I need is gathered before packets are sent, and the need for verbose
output in test cases is one after the other (i send a packet, look at
verbose output, and then move onto the next packet).
<snip>
  
Jeremy Spewock July 30, 2024, 9:30 p.m. UTC | #2
On Tue, Jul 30, 2024 at 11:41 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> Good work! I left some probing/clarifying comments below for you.
>
> <snip>
> > +@dataclass
> > +class TestPmdVerboseOutput(TextParser):
> > +    """Verbose output generated by Testpmd.
> > +
> > +    This class is the top level of the output, containing verbose output delimited by
> > +    "port X/queue Y: sent/received Z packets".
> > +    """
> > +
> > +    #: ID of the port that handled the packet.
> > +    port_id: int = field(metadata=TextParser.find_int(r"port (\d+)/queue \d+"))
> > +    #: ID of the queue that handled the packet.
> > +    queue_id: int = field(metadata=TextParser.find_int(r"port \d+/queue (\d+)"))
> > +    #: Whether the packet was received or sent by the queue/port.
> > +    was_received: bool = field(metadata=TextParser.find(r"received \d+ packets"))
> > +    #: List of packets handed by the port/queue in this section.
> > +    packets: list[TestPmdVerbosePacket] = field(
> > +        metadata=TextParser.wrap(
> > +            TextParser.find_all(r"(src=[\w\s=:-]+ol_flags: [\w ]+)"),
> > +            lambda matches_arr: list(map(TestPmdVerbosePacket.parse, matches_arr)),
> > +        )
> > +    )
> > +
> > +
> >  class TestPmdShell(DPDKShell):
> >      """Testpmd interactive shell.
> >
> > @@ -645,7 +767,7 @@ def start(self, verify: bool = True) -> None:
> >                          "Not all ports came up after starting packet forwarding in testpmd."
> >                      )
> >
> > -    def stop(self, verify: bool = True) -> None:
> > +    def stop(self, verify: bool = True) -> str:
> >          """Stop packet forwarding.
> >
> >          Args:
> > @@ -656,6 +778,9 @@ def stop(self, verify: bool = True) -> None:
> >          Raises:
> >              InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop
> >                  forwarding results in an error.
> > +
> > +        Returns:
> > +            Output gathered from sending the stop command.
> >          """
> >          stop_cmd_output = self.send_command("stop")
> >          if verify:
> > @@ -665,6 +790,7 @@ def stop(self, verify: bool = True) -> None:
> >              ):
> >                  self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
> >                  raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
> > +        return stop_cmd_output
> <snip>
> >
> > +    @staticmethod
> > +    def extract_verbose_output(output: str) -> list[TestPmdVerboseOutput]:
> > +        """Extract the verbose information present in given testpmd output.
> > +
> > +        This method extracts sections of verbose output that begin with the line
> > +        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
> > +
> > +        Args:
> > +            output: Testpmd output that contains verbose information
> > +
> > +        Returns:
> > +            List of parsed packet information gathered from verbose information in `output`.
> > +        """
> > +        iter = re.finditer(r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+|$))", output, re.S)
> > +        return [TestPmdVerboseOutput.parse(s.group(0)) for s in iter]
> > +
>
> I'm trying to think of circumstances where we as developers would be
> looking for anything else besides verbose output from the stop method.
> Running the command outputs some statistics, but information like port
> stats is displayed after the stop command is initiated. I'm
> implementing this system right now for one of my test suites, and I'm
> wondering if there might be any feasible way to extract output without
> needing to input any explicit outputs into this method. I'm putting
> output = testpmd.stop() and then calling this method. It looks
> something like this:
>
> verbose_output = testpmd.extract_verbose_output(testpmd.stop())
>
> This is easy enough, but it might be a bit confusing for someone new
> to the framework. The way that output is gathered is still elusive to
> me, and I'm guessing that any commands run in-between setting verbose
> mode and when you stop testpmd might influence how output is
> generated. But in my experience so far, any statistics or information
> I need is gathered before packets are sent, and the need for verbose
> output in test cases is one after the other (i send a packet, look at
> verbose output, and then move onto the next packet).

You're right that in most cases it would come from the stop output,
but the output from that stop command has one other thing as well that
I would consider valuable which is statistics of packets handled by
ports specifically for the duration of the packet forwarding you are
stopping. It is also true that sending other testpmd commands while
verbose output is being sent will change what is collected, so I
didn't want to tie the method specifically to the stop command since
if you did a start command then checked port statistics for example,
it would consume all of the verbose output up until the command to
check port statistics.

For the reason stated above I think it actually might make sense to
make it so that every testpmd method (including ones that currently
return dataclasses) return their original, unmodified collected output
from the testpmd shell as well. Things like port stats can return both
in a tuple potentially. This way if there is asynchronous output like
there is with verbose output other commands don't completely remove
it.

> <snip>
  
Jeremy Spewock July 30, 2024, 9:33 p.m. UTC | #3
On Tue, Jul 30, 2024 at 9:37 AM <jspewock@iol.unh.edu> wrote:
<snip>
>
>  class TestPmdDevice:
> @@ -577,6 +577,128 @@ class TestPmdPortStats(TextParser):
>      tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
>
>
> +class VerboseOLFlag(Flag):
> +    """Flag representing the OL flags of a packet from Testpmd verbose output."""
> +
> +    #:
> +    RTE_MBUF_F_RX_RSS_HASH = auto()
> +
> +    #:
> +    RTE_MBUF_F_RX_L4_CKSUM_GOOD = auto()
> +    #:
> +    RTE_MBUF_F_RX_L4_CKSUM_BAD = auto()
> +    #:
> +    RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN = auto()
> +
> +    #:
> +    RTE_MBUF_F_RX_IP_CKSUM_GOOD = auto()
> +    #:
> +    RTE_MBUF_F_RX_IP_CKSUM_BAD = auto()
> +    #:
> +    RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = auto()
> +
> +    #:
> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD = auto()
> +    #:
> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD = auto()
> +    #:
> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN = auto()
> +

After reading more of the API and using this patch to write a test
suite, I believe there is more expansion of these OL flags that should
take place. For starters, there are the Tx OL flags that, while not
seeming to be very useful for the current test suites we are writing,
wouldn't hurt to also include as they seem to be fairly different.
Additionally, there are some other less common RX OL flags that should
be included here just to cover all options. I will work on adding
these into the next version.

<snip>
>
  
Luca Vizzarro Aug. 1, 2024, 8:41 a.m. UTC | #4
Great work Jeremy! Just a couple of minor passable improvement points.

On 30/07/2024 14:34, jspewock@iol.unh.edu wrote:

> +@dataclass
> +class TestPmdVerbosePacket(TextParser):
> +    """Packet information provided by verbose output in Testpmd.
> +
> +    The "receive/sent queue" information is not included in this dataclass because this value is
> +    captured on the outer layer of input found in :class:`TestPmdVerboseOutput`.
> +    """
> +
> +    #:
> +    src_mac: str = field(metadata=TextParser.find(r"src=({})".format(REGEX_FOR_MAC_ADDRESS)))
Just a(n optional) nit: TextParser.find(f"src=({REGEX_FOR_MAC_ADDRESS})")
The raw string is only needed to prevent escaping, which we don't do here.
> +    #:
> +    dst_mac: str = field(metadata=TextParser.find(r"dst=({})".format(REGEX_FOR_MAC_ADDRESS)))
As above.
> +    #: Memory pool the packet was handled on.
> +    pool: str = field(metadata=TextParser.find(r"pool=(\S+)"))
> +    #: Packet type in hex.
> +    p_type: int = field(metadata=TextParser.find_int(r"type=(0x[a-fA-F\d]+)"))
> +    #:

<snip>

> +    @staticmethod
> +    def extract_verbose_output(output: str) -> list[TestPmdVerboseOutput]:
> +        """Extract the verbose information present in given testpmd output.
> +
> +        This method extracts sections of verbose output that begin with the line
> +        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
> +
> +        Args:
> +            output: Testpmd output that contains verbose information
> +
> +        Returns:
> +            List of parsed packet information gathered from verbose information in `output`.
> +        """
> +        iter = re.finditer(r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+|$))", output, re.S)

How about using a regex that matches what you described? ;) Keeping re.S:

    (port \d+/queue \d+.+?ol_flags: [\w ]+)

Would spare you from using complex lookahead constructs and 4.6x less 
steps. Maybe it doesn't work with every scenario? Looks like it works 
well with the sample output I have. Let me know if it works for you.

Best,
Luca
  
Luca Vizzarro Aug. 1, 2024, 8:43 a.m. UTC | #5
On 30/07/2024 22:33, Jeremy Spewock wrote:
> On Tue, Jul 30, 2024 at 9:37 AM <jspewock@iol.unh.edu> wrote:
> <snip>
>> +class VerboseOLFlag(Flag):
>> +    """Flag representing the OL flags of a packet from Testpmd verbose output."""
>> +
>> +    #:
>> +    RTE_MBUF_F_RX_RSS_HASH = auto()
>> +
>> +    #:
>> +    RTE_MBUF_F_RX_L4_CKSUM_GOOD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_L4_CKSUM_BAD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN = auto()
>> +
>> +    #:
>> +    RTE_MBUF_F_RX_IP_CKSUM_GOOD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_IP_CKSUM_BAD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = auto()
>> +
>> +    #:
>> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD = auto()
>> +    #:
>> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN = auto()
>> +
> After reading more of the API and using this patch to write a test
> suite, I believe there is more expansion of these OL flags that should
> take place. For starters, there are the Tx OL flags that, while not
> seeming to be very useful for the current test suites we are writing,
> wouldn't hurt to also include as they seem to be fairly different.
> Additionally, there are some other less common RX OL flags that should
> be included here just to cover all options. I will work on adding
> these into the next version.

If you wanted to cover even more, hw_ptype and sw_ptype look like they 
could use a data structure. I reckon a flag like the above would also work.
  
Jeremy Spewock Aug. 2, 2024, 1:35 p.m. UTC | #6
On Thu, Aug 1, 2024 at 4:41 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Great work Jeremy! Just a couple of minor passable improvement points.
>
> On 30/07/2024 14:34, jspewock@iol.unh.edu wrote:
>
> > +@dataclass
> > +class TestPmdVerbosePacket(TextParser):
> > +    """Packet information provided by verbose output in Testpmd.
> > +
> > +    The "receive/sent queue" information is not included in this dataclass because this value is
> > +    captured on the outer layer of input found in :class:`TestPmdVerboseOutput`.
> > +    """
> > +
> > +    #:
> > +    src_mac: str = field(metadata=TextParser.find(r"src=({})".format(REGEX_FOR_MAC_ADDRESS)))
> Just a(n optional) nit: TextParser.find(f"src=({REGEX_FOR_MAC_ADDRESS})")
> The raw string is only needed to prevent escaping, which we don't do here.

Ack. I really just left it this way because it also adjusts
highlighting in some IDEs, but there isn't much to see here anyway.

> > +    #:
> > +    dst_mac: str = field(metadata=TextParser.find(r"dst=({})".format(REGEX_FOR_MAC_ADDRESS)))
> As above.

Ack.

> > +    #: Memory pool the packet was handled on.
> > +    pool: str = field(metadata=TextParser.find(r"pool=(\S+)"))
> > +    #: Packet type in hex.
> > +    p_type: int = field(metadata=TextParser.find_int(r"type=(0x[a-fA-F\d]+)"))
> > +    #:
>
> <snip>
>
> > +    @staticmethod
> > +    def extract_verbose_output(output: str) -> list[TestPmdVerboseOutput]:
> > +        """Extract the verbose information present in given testpmd output.
> > +
> > +        This method extracts sections of verbose output that begin with the line
> > +        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
> > +
> > +        Args:
> > +            output: Testpmd output that contains verbose information
> > +
> > +        Returns:
> > +            List of parsed packet information gathered from verbose information in `output`.
> > +        """
> > +        iter = re.finditer(r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+|$))", output, re.S)
>
> How about using a regex that matches what you described? ;) Keeping re.S:
>
>     (port \d+/queue \d+.+?ol_flags: [\w ]+)
>
> Would spare you from using complex lookahead constructs and 4.6x less
> steps. Maybe it doesn't work with every scenario? Looks like it works
> well with the sample output I have. Let me know if it works for you.
>

I tried using something like this actually which is probably why the
docstring reflects that type of language, but I didn't use it because
it doesn't match one specific case. I'm not sure how common it is, and
I haven't seen it happen in my recent testing, but since the verbose
output specifically states that it sent/received X number of packets,
I presume there is a case where that number will be more than 1, and
there will be more than one set of packet information after that first
line. I think I've seen it happen in the past, but I couldn't recreate
it in testing.

For context to this next section, if it wasn't clear, I consider the
`port \d+/queue \d+` line to be the header line and the start of a
"block" of verbose output.

Basically though the problem with this is that if there are more than
one packet under that header line, the lazy approach will only consume
up until the ol_flags of the first packet of a block, and the greedy
approach consumes everything until the last packet of the entire
output. You could use the lazy approach with the next port/queue line
as your delimiter, but then the opening line of the next block of
output is included in the previous block's group. The only way I could
find to get around this and go with the idea of "take everything from
the start of this group until another group starts" but without
capturing the opening of the next block was a look ahead. Again
though, I definitely don't love the regex that I wrote and would love
to find a better alternative.

> Best,
> Luca
>
  
Jeremy Spewock Aug. 2, 2024, 1:40 p.m. UTC | #7
On Thu, Aug 1, 2024 at 4:43 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 30/07/2024 22:33, Jeremy Spewock wrote:
> > On Tue, Jul 30, 2024 at 9:37 AM <jspewock@iol.unh.edu> wrote:
> > <snip>
> >> +class VerboseOLFlag(Flag):
> >> +    """Flag representing the OL flags of a packet from Testpmd verbose output."""
> >> +
> >> +    #:
> >> +    RTE_MBUF_F_RX_RSS_HASH = auto()
> >> +
> >> +    #:
> >> +    RTE_MBUF_F_RX_L4_CKSUM_GOOD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_L4_CKSUM_BAD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN = auto()
> >> +
> >> +    #:
> >> +    RTE_MBUF_F_RX_IP_CKSUM_GOOD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_IP_CKSUM_BAD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = auto()
> >> +
> >> +    #:
> >> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD = auto()
> >> +    #:
> >> +    RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN = auto()
> >> +
> > After reading more of the API and using this patch to write a test
> > suite, I believe there is more expansion of these OL flags that should
> > take place. For starters, there are the Tx OL flags that, while not
> > seeming to be very useful for the current test suites we are writing,
> > wouldn't hurt to also include as they seem to be fairly different.
> > Additionally, there are some other less common RX OL flags that should
> > be included here just to cover all options. I will work on adding
> > these into the next version.
>
> If you wanted to cover even more, hw_ptype and sw_ptype look like they
> could use a data structure. I reckon a flag like the above would also work.

Ack, it's probably worth making those a little more structured as well.

>
  
Nicholas Pratte Aug. 2, 2024, 2:54 p.m. UTC | #8
<snip>
> You're right that in most cases it would come from the stop output,
> but the output from that stop command has one other thing as well that
> I would consider valuable which is statistics of packets handled by
> ports specifically for the duration of the packet forwarding you are
> stopping. It is also true that sending other testpmd commands while
> verbose output is being sent will change what is collected, so I
> didn't want to tie the method specifically to the stop command since
> if you did a start command then checked port statistics for example,
> it would consume all of the verbose output up until the command to
> check port statistics.
>
> For the reason stated above I think it actually might make sense to
> make it so that every testpmd method (including ones that currently
> return dataclasses) return their original, unmodified collected output
> from the testpmd shell as well. Things like port stats can return both
> in a tuple potentially. This way if there is asynchronous output like
> there is with verbose output other commands don't completely remove
> it.

I agree! I think giving each testpmd method its own output would add
more consistency. An idea I had floating around that kind of relates
to your suggestion above was introducing some instance variables that
could enable the testpmd shell object to be smart enough to
automatically scan, and keep a record of, any verbose output that
comes out across any command run. The TestPMDShell class could track
whether verbose mode is on or not, and if True, run additional logic
to scan for verbose output and add it to a data structure for access
every time a command is run. Then users, from the perspective of
writing a test suite, could do something like 'output in
testpmd.verbose_output', where verbose_output is an instance data
structure of the TestPMDShell. This might be overcomplicated to
implement, but it was an idea I had that might make using verbose mode
more streamlined. What are your thoughts?
  
Jeremy Spewock Aug. 2, 2024, 5:38 p.m. UTC | #9
On Fri, Aug 2, 2024 at 10:54 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> <snip>
> > You're right that in most cases it would come from the stop output,
> > but the output from that stop command has one other thing as well that
> > I would consider valuable which is statistics of packets handled by
> > ports specifically for the duration of the packet forwarding you are
> > stopping. It is also true that sending other testpmd commands while
> > verbose output is being sent will change what is collected, so I
> > didn't want to tie the method specifically to the stop command since
> > if you did a start command then checked port statistics for example,
> > it would consume all of the verbose output up until the command to
> > check port statistics.
> >
> > For the reason stated above I think it actually might make sense to
> > make it so that every testpmd method (including ones that currently
> > return dataclasses) return their original, unmodified collected output
> > from the testpmd shell as well. Things like port stats can return both
> > in a tuple potentially. This way if there is asynchronous output like
> > there is with verbose output other commands don't completely remove
> > it.
>
> I agree! I think giving each testpmd method its own output would add
> more consistency. An idea I had floating around that kind of relates
> to your suggestion above was introducing some instance variables that
> could enable the testpmd shell object to be smart enough to
> automatically scan, and keep a record of, any verbose output that
> comes out across any command run. The TestPMDShell class could track
> whether verbose mode is on or not, and if True, run additional logic
> to scan for verbose output and add it to a data structure for access
> every time a command is run. Then users, from the perspective of
> writing a test suite, could do something like 'output in
> testpmd.verbose_output', where verbose_output is an instance data
> structure of the TestPMDShell. This might be overcomplicated to
> implement, but it was an idea I had that might make using verbose mode
> more streamlined. What are your thoughts?

I like the sound of this idea a lot actually since it would remove the
chance of the output just completely being thrown away. In my own test
suite I managed to dance around this by strategically placing my
testpmd commands, but this could save people some headache in the
future. I feel like this wouldn't be something overly complicated to
implement either, all we would have to do is extend the send_command
method in the TestpmdShell class and check a boolean for if verbose is
on, extract this output. If/how to clear this list would be something
to think about, but I would say that, in general, the idea of making
sure we don't lose information is something that I'm all for.
  
Nicholas Pratte Aug. 5, 2024, 1:20 p.m. UTC | #10
> I like the sound of this idea a lot actually since it would remove the
> chance of the output just completely being thrown away. In my own test
> suite I managed to dance around this by strategically placing my
> testpmd commands, but this could save people some headache in the
> future. I feel like this wouldn't be something overly complicated to
> implement either, all we would have to do is extend the send_command
> method in the TestpmdShell class and check a boolean for if verbose is
> on, extract this output. If/how to clear this list would be something
> to think about, but I would say that, in general, the idea of making
> sure we don't lose information is something that I'm all for.

That's a good point that you could just modify the send_command
method. In my head I was thinking that we'd have to modify each
individual method! Totally forget that all those testpmd methods I was
thinking about stem from send_command().
  

Patch

diff --git a/dts/framework/parser.py b/dts/framework/parser.py
index 741dfff821..0b39025a48 100644
--- a/dts/framework/parser.py
+++ b/dts/framework/parser.py
@@ -160,6 +160,36 @@  def _find(text: str) -> Any:
 
         return ParserFn(TextParser_fn=_find)
 
+    @staticmethod
+    def find_all(
+        pattern: str | re.Pattern[str],
+        flags: re.RegexFlag = re.RegexFlag(0),
+    ) -> ParserFn:
+        """Makes a parser function that finds all of the regular expression matches in the text.
+
+        If there are no matches found in the text than None will be returned, otherwise a list
+        containing all matches will be returned. Patterns that contain multiple groups will pack
+        the matches for each group into a tuple.
+
+        Args:
+            pattern: The regular expression pattern.
+            flags: The regular expression flags. Ignored if the given pattern is already compiled.
+
+        Returns:
+            A :class:`ParserFn` that can be used as metadata for a dataclass field.
+        """
+        if isinstance(pattern, str):
+            pattern = re.compile(pattern, flags)
+
+        def _find_all(text: str) -> list[str] | None:
+            m = pattern.findall(text)
+            if len(m) == 0:
+                return None
+
+            return m
+
+        return ParserFn(TextParser_fn=_find_all)
+
     @staticmethod
     def find_int(
         pattern: str | re.Pattern[str],
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 43e9f56517..dedf1553cf 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -31,7 +31,7 @@ 
 from framework.settings import SETTINGS
 from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
 from framework.testbed_model.sut_node import SutNode
-from framework.utils import StrEnum
+from framework.utils import REGEX_FOR_MAC_ADDRESS, StrEnum
 
 
 class TestPmdDevice:
@@ -577,6 +577,128 @@  class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+class VerboseOLFlag(Flag):
+    """Flag representing the OL flags of a packet from Testpmd verbose output."""
+
+    #:
+    RTE_MBUF_F_RX_RSS_HASH = auto()
+
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN = auto()
+
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = auto()
+
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN = auto()
+
+    @classmethod
+    def from_str_list(cls, arr: list[str]) -> Self:
+        """Makes an instance from a list containing the flag members.
+
+        Args:
+            arr: A list of strings containing ol_flag values.
+
+        Returns:
+            A new instance of the flag.
+        """
+        flag = cls(0)
+        for name in cls.__members__:
+            if name in arr:
+                flag |= cls[name]
+        return flag
+
+    @classmethod
+    def make_parser(cls) -> ParserFn:
+        """Makes a parser function.
+
+        Returns:
+            ParserFn: A dictionary for the `dataclasses.field` metadata argument containing a
+                parser function that makes an instance of this flag from text.
+        """
+        return TextParser.wrap(
+            TextParser.wrap(TextParser.find(r"ol_flags: ([^\n]+)"), str.split),
+            cls.from_str_list,
+        )
+
+
+@dataclass
+class TestPmdVerbosePacket(TextParser):
+    """Packet information provided by verbose output in Testpmd.
+
+    The "receive/sent queue" information is not included in this dataclass because this value is
+    captured on the outer layer of input found in :class:`TestPmdVerboseOutput`.
+    """
+
+    #:
+    src_mac: str = field(metadata=TextParser.find(r"src=({})".format(REGEX_FOR_MAC_ADDRESS)))
+    #:
+    dst_mac: str = field(metadata=TextParser.find(r"dst=({})".format(REGEX_FOR_MAC_ADDRESS)))
+    #: Memory pool the packet was handled on.
+    pool: str = field(metadata=TextParser.find(r"pool=(\S+)"))
+    #: Packet type in hex.
+    p_type: int = field(metadata=TextParser.find_int(r"type=(0x[a-fA-F\d]+)"))
+    #:
+    length: int = field(metadata=TextParser.find_int(r"length=(\d+)"))
+    #: Number of segments in the packet.
+    nb_segs: int = field(metadata=TextParser.find_int(r"nb_segs=(\d+)"))
+    #: Hardware packet type. Collected as a string delimited by whitespace.
+    hw_ptype: str = field(metadata=TextParser.find(r"hw ptype: ([^-]+)"))
+    #: Software packet type. Collected as a string delimited by whitespace.
+    sw_ptype: str = field(metadata=TextParser.find(r"sw ptype: ([^-]+)"))
+    #:
+    l2_len: int = field(metadata=TextParser.find_int(r"l2_len=(\d+)"))
+    #:
+    ol_flags: VerboseOLFlag = field(metadata=VerboseOLFlag.make_parser())
+    #: RSS has of the packet in hex.
+    rss_hash: int | None = field(
+        default=None, metadata=TextParser.find_int(r"RSS hash=(0x[a-fA-F\d]+)")
+    )
+    #: RSS queue that handled the packet in hex.
+    rss_queue: int | None = field(
+        default=None, metadata=TextParser.find_int(r"RSS queue=(0x[a-fA-F\d]+)")
+    )
+    #:
+    l3_len: int | None = field(default=None, metadata=TextParser.find_int(r"l3_len=(\d+)"))
+    #:
+    l4_len: int | None = field(default=None, metadata=TextParser.find_int(r"l4_len=(\d+)"))
+
+
+@dataclass
+class TestPmdVerboseOutput(TextParser):
+    """Verbose output generated by Testpmd.
+
+    This class is the top level of the output, containing verbose output delimited by
+    "port X/queue Y: sent/received Z packets".
+    """
+
+    #: ID of the port that handled the packet.
+    port_id: int = field(metadata=TextParser.find_int(r"port (\d+)/queue \d+"))
+    #: ID of the queue that handled the packet.
+    queue_id: int = field(metadata=TextParser.find_int(r"port \d+/queue (\d+)"))
+    #: Whether the packet was received or sent by the queue/port.
+    was_received: bool = field(metadata=TextParser.find(r"received \d+ packets"))
+    #: List of packets handed by the port/queue in this section.
+    packets: list[TestPmdVerbosePacket] = field(
+        metadata=TextParser.wrap(
+            TextParser.find_all(r"(src=[\w\s=:-]+ol_flags: [\w ]+)"),
+            lambda matches_arr: list(map(TestPmdVerbosePacket.parse, matches_arr)),
+        )
+    )
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -645,7 +767,7 @@  def start(self, verify: bool = True) -> None:
                         "Not all ports came up after starting packet forwarding in testpmd."
                     )
 
-    def stop(self, verify: bool = True) -> None:
+    def stop(self, verify: bool = True) -> str:
         """Stop packet forwarding.
 
         Args:
@@ -656,6 +778,9 @@  def stop(self, verify: bool = True) -> None:
         Raises:
             InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop
                 forwarding results in an error.
+
+        Returns:
+            Output gathered from sending the stop command.
         """
         stop_cmd_output = self.send_command("stop")
         if verify:
@@ -665,6 +790,7 @@  def stop(self, verify: bool = True) -> None:
             ):
                 self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
                 raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
+        return stop_cmd_output
 
     def get_devices(self) -> list[TestPmdDevice]:
         """Get a list of device names that are known to testpmd.
@@ -806,6 +932,22 @@  def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    @staticmethod
+    def extract_verbose_output(output: str) -> list[TestPmdVerboseOutput]:
+        """Extract the verbose information present in given testpmd output.
+
+        This method extracts sections of verbose output that begin with the line
+        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
+
+        Args:
+            output: Testpmd output that contains verbose information
+
+        Returns:
+            List of parsed packet information gathered from verbose information in `output`.
+        """
+        iter = re.finditer(r"(port \d+/queue \d+:.*?(?=port \d+/queue \d+|$))", output, re.S)
+        return [TestPmdVerboseOutput.parse(s.group(0)) for s in iter]
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index 6b5d5a805f..9c64cf497f 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -27,6 +27,7 @@ 
 from .exception import ConfigurationError
 
 REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
+REGEX_FOR_MAC_ADDRESS: str = r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}"
 
 
 def expand_range(range_str: str) -> list[int]: