[PATCH v1] dts: add flow rule dataclass to testpmd shell

[PATCH v1] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index eda6eb320f..d6c111da0a 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -19,7 +19,7 @@
 from dataclasses import dataclass, field
 from enum import Flag, auto
 from pathlib import PurePath
-from typing import ClassVar
+from typing import ClassVar, Optional
 
 from typing_extensions import Self, Unpack
 
@@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class flow_func:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: Optional[int] = None
+    #:
+    priority_level: Optional[int] = None
+    #:
+    user_id: Optional[int] = None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = []
+        ret.append(f"flow create {self.port_id} ")
+        ret.append(f"group {self.group_id} " if self.group_id is not None else "")
+        ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "")
+        ret.append("ingress " if self.ingress else "egress ")
+        ret.append(f"user_id {self.user_id} " if self.user_id is not None else "")
+        ret.append(f"pattern {self.pattern} ")
+        ret.append(" / end actions ")
+        ret.append(f"{self.actions} / end")
+        return "".join(ret)
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, cmd: str, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            cmd: String from flow_func instance to send as a flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(cmd)
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.44.0

Re: [PATCH v1] dts: add flow rule dataclass to testpmd shell

[Jeremy Spewock]

I think Luca made some great points and I agree with what he said, I
just had one other though as well. Great work!

On Fri, Jul 26, 2024 at 10:22 AM Dean Marx <dmarx@iol.unh.edu> wrote:
<snip>
> +
>  class TestPmdShell(DPDKShell):
>      """Testpmd interactive shell.
>
> @@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> +    def flow_create(self, cmd: str, verify: bool = True) -> None:

It might make more sense for this method to take in the actual
dataclass rather than a string, and then it can convert it to a string
when it sends the command. That way users don't have to make the class
and then always do str() on it before passing it into this method.
Additionally, it discourages people from just putting anything in the
command section and shows the expectation that you should be using the
dataclass to make the flow rules.

> +        """Creates a flow rule in the testpmd session.
> +
> +        Args:
> +            cmd: String from flow_func instance to send as a flow rule.
> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was created successfully.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If flow rule is invalid.
> +        """
> +        flow_output = self.send_command(cmd)
> +        if verify:
> +            if "created" not in flow_output:
> +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to create flow rule:\n{flow_output}"
> +                )
> +
>      def _close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
> --
> 2.44.0
>

[PATCH v2] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 43e9f56517..59b2bb914b 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -19,7 +19,7 @@
 from dataclasses import dataclass, field
 from enum import Flag, auto
 from pathlib import PurePath
-from typing import ClassVar
+from typing import ClassVar, Optional
 
 from typing_extensions import Self, Unpack
 
@@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class flow_func:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: Optional[int] = None
+    #:
+    priority_level: Optional[int] = None
+    #:
+    user_id: Optional[int] = None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = []
+        ret.append(f"flow create {self.port_id} ")
+        ret.append(f"group {self.group_id} " if self.group_id is not None else "")
+        ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "")
+        ret.append("ingress " if self.ingress else "egress ")
+        ret.append(f"user_id {self.user_id} " if self.user_id is not None else "")
+        ret.append(f"pattern {self.pattern} ")
+        ret.append(" / end actions ")
+        ret.append(f"{self.actions} / end")
+        return "".join(ret)
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -806,6 +843,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, cmd: flow_func, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            cmd: String from flow_func instance to send as a flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(str(cmd))
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.44.0

[PATCH v3] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 43e9f56517..6876f93e9e 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -577,6 +577,44 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class FlowRule:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: int | None
+    #:
+    priority_level: int | None
+    #:
+    user_id: int | None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = f"flow create {self.port_id} "
+        if self.group_id is not None:
+            ret += f"group {self.group_id} "
+        if self.priority_level is not None:
+            ret += f"priority {self.priority_level} "
+        ret += "ingress " if self.ingress else "egress "
+        if self.user_id is not None:
+            ret += f"user_id {self.user_id} "
+        ret += f"pattern {self.pattern} / end "
+        ret += f"actions {self.actions} / end"
+        return ret
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -806,6 +844,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, cmd: FlowRule, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            cmd: String from flow_func instance to send as a flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(str(cmd))
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.44.0

[PATCH v3] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 43e9f56517..17b9c7811d 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -577,6 +577,44 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class FlowRule:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: int | None = None
+    #:
+    priority_level: int | None = None
+    #:
+    user_id: int | None = None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = f"flow create {self.port_id} "
+        if self.group_id is not None:
+            ret += f"group {self.group_id} "
+        if self.priority_level is not None:
+            ret += f"priority {self.priority_level} "
+        ret += "ingress " if self.ingress else "egress "
+        if self.user_id is not None:
+            ret += f"user_id {self.user_id} "
+        ret += f"pattern {self.pattern} / end "
+        ret += f"actions {self.actions} / end"
+        return ret
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -806,6 +844,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, cmd: FlowRule, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            cmd: String from flow_func instance to send as a flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(str(cmd))
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.44.0

Re: [PATCH v3] dts: add flow rule dataclass to testpmd shell

[Juraj Linkeš]

On 13. 8. 2024 16:41, Dean Marx wrote:
> add dataclass for passing in flow rule creation arguments, as well as a

Capitalize please.

> __str__ method for converting to a sendable testpmd command. Add
> flow_create method to TestPmdShell class for initializing flow rules.
> 
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>   dts/framework/remote_session/testpmd_shell.py | 57 +++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 43e9f56517..17b9c7811d 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -577,6 +577,44 @@ class TestPmdPortStats(TextParser):
>       tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
>   
>   
> +@dataclass
> +class FlowRule:
> +    """Dataclass for setting flow rule parameters."""
> +
> +    #:
> +    port_id: int
> +    #:
> +    ingress: bool
> +    #:
> +    pattern: str
> +    #:
> +    actions: str
> +
> +    #:
> +    group_id: int | None = None
> +    #:
> +    priority_level: int | None = None
> +    #:
> +    user_id: int | None = None
> +
> +    def __str__(self) -> str:
> +        """Returns the string representation of a flow_func instance.
> +
> +        In this case, a properly formatted flow create command that can be sent to testpmd.

I think it would be beneficial for a complicated command like this to 
add the actual format of the command as returned by testpmd, which 
should be (according to app/test-pmd/cmdline.c):

flow create {port_id}
  [group {group_id}] [priority {level}]
  [ingress] [egress]
  pattern {item} [/ {item} [...]] / end
  actions {action} [/ {action} [...]] / end

Looking at this, there could be multiple patterns and actions. Maybe we 
should add classes for these two as well - what do the patterns and 
actions look like?

> +        """
> +        ret = f"flow create {self.port_id} "
> +        if self.group_id is not None:
> +            ret += f"group {self.group_id} "
> +        if self.priority_level is not None:
> +            ret += f"priority {self.priority_level} "
> +        ret += "ingress " if self.ingress else "egress "
> +        if self.user_id is not None:
> +            ret += f"user_id {self.user_id} "
> +        ret += f"pattern {self.pattern} / end "
> +        ret += f"actions {self.actions} / end"
> +        return ret
> +
> +
>   class TestPmdShell(DPDKShell):
>       """Testpmd interactive shell.
>   
> @@ -806,6 +844,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>   
>           return TestPmdPortStats.parse(output)
>   
> +    def flow_create(self, cmd: FlowRule, verify: bool = True) -> None:

Do we also need to add a method for deleting (I guess it would be called 
destroying per the command) the rule? Or any other flow rule commands? 
We could expand the test suite (I assume we're adding this because of a 
test suite) if such tests make sense.

> +        """Creates a flow rule in the testpmd session.
> +
> +        Args:
> +            cmd: String from flow_func instance to send as a flow rule.

The cmd docstring has not been updated. I'd also rename cmd to flow_rule.

> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was created successfully.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If flow rule is invalid.
> +        """
> +        flow_output = self.send_command(str(cmd))
> +        if verify:
> +            if "created" not in flow_output:
> +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to create flow rule:\n{flow_output}"
> +                )
> +
>       def _close(self) -> None:
>           """Overrides :meth:`~.interactive_shell.close`."""
>           self.stop()

[PATCH v4] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

Add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index b9e7875615..aaae7e5515 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -672,6 +672,44 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class FlowRule:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: int | None = None
+    #:
+    priority_level: int | None = None
+    #:
+    user_id: int | None = None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = f"flow create {self.port_id} "
+        if self.group_id is not None:
+            ret += f"group {self.group_id} "
+        if self.priority_level is not None:
+            ret += f"priority {self.priority_level} "
+        ret += "ingress " if self.ingress else "egress "
+        if self.user_id is not None:
+            ret += f"user_id {self.user_id} "
+        ret += f"pattern {self.pattern} / end "
+        ret += f"actions {self.actions} / end"
+        return ret
+
+
 class PacketOffloadFlag(Flag):
     """Flag representing the Packet Offload Features Flags in DPDK.
 
@@ -1717,6 +1755,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, flow_rule: FlowRule, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            flow_rule: FlowRule object used for creating testpmd flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(str(flow_rule))
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     @requires_stopped_ports
     def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
         """Change the MTU of a port using testpmd.
-- 
2.44.0

Re: [PATCH v4] dts: add flow rule dataclass to testpmd shell

[Luca Vizzarro]

Hi Dean,

This looks fine for most of it, but some of Juraj's prior comments still 
apply. See comments in line.

On 10/10/2024 22:06, Dean Marx wrote:
> +@dataclass

I'd make this kw_only=True:

   @dataclass(kw_only=True)

this enforces the construction to include field names for the sake of 
readability. Having:

   FlowRule(0, True, "ip", "rss")

is not really as self-descriptive as:

   FlowRule(port_id=0, ingress=True, pattern="ip", actions="rss")

> +class FlowRule:
> +    """Dataclass for setting flow rule parameters."""

We should add the pattern that this is meant to represent in the class 
docstring here above, as suggested by Juraj. Also I'd change the current 
docstring to what the class is actually doing, e.g.:

   Class representation of flow rule parameters.

   This class represents the parameters of any flow rule as per the
   following pattern:

     [group {group_id}] [priority {level}] [ingress] [egress]
     [user_id {user_id}] pattern {item} [/ {item} [...]] / end
     actions {action} [/ {action} [...]] / end

Mind that the pattern above is taking from the testpmd guide[1], I 
excluded the bits that you haven't implemented.

> +
> +    #:
> +    port_id: int

I would leave out the `port_id` here, if we wanted to apply the same 
flow rule to multiple ports, this would complicate things. This should 
belong under the `flow_create` function.

> +    #:
> +    ingress: bool
> +    #:
> +    pattern: str

The rule pattern above also suggests that this...

> +    #:
> +    actions: str

...and this can be `list[str]`, and then joined in __str__:

   "/ ".join(self.actions)

> +
> +    #:
> +    group_id: int | None = None
> +    #:
> +    priority_level: int | None = None
> +    #:
> +    user_id: int | None = None

Perhaps, order the fields in the way they are represented in the flow 
rule. So this should go before actions and patterns.

> +
> +    def __str__(self) -> str:
> +        """Returns the string representation of a flow_func instance.

s/flow_func/FlowRule, or just don't mention it at all as it's implicit:

   Returns the string representation of this instance.

> +
> +        In this case, a properly formatted flow create command that can be sent to testpmd.

I reckon that `flow create` should be created by the flow create command 
function. Let's keep this to just produce the parameters settings...

> +        """
> +        ret = f"flow create {self.port_id} "

...and make this an empty string:

   ret = ""

> +        if self.group_id is not None:
> +            ret += f"group {self.group_id} "
> +        if self.priority_level is not None:
> +            ret += f"priority {self.priority_level} "
> +        ret += "ingress " if self.ingress else "egress "

The pattern above suggests we can have flow rules for both ingress and 
egress. Therefore I'd have a field for egress as well, or we want to get 
fancy, create a FlowRuleDirection enum.Flag, so that we can do:

   FlowRule(direction=FlowRuleDirection.INGRESS|EGRESS, ...)

Or to not overcomplicate we can just rely on mypy's checking:

   class FlowRule:
     ...
     direction: Literal["ingress", "egress", "both"]

   FlowRule(direction="ingress")

> +        if self.user_id is not None:
> +            ret += f"user_id {self.user_id} "
> +        ret += f"pattern {self.pattern} / end "
> +        ret += f"actions {self.actions} / end"
> +        return ret
> +
> +
>   class PacketOffloadFlag(Flag):
>       """Flag representing the Packet Offload Features Flags in DPDK.
>   
> @@ -1717,6 +1755,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>   
>           return TestPmdPortStats.parse(output)
>   
> +    def flow_create(self, flow_rule: FlowRule, verify: bool = True) -> None:

As mentioned, let's put the port_id here.

> +        """Creates a flow rule in the testpmd session.
> +
> +        Args:
> +            flow_rule: FlowRule object used for creating testpmd flow rule.

Reference to the class missing:

   :class:`FlowRule` object

> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was created successfully.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If flow rule is invalid.
> +        """
> +        flow_output = self.send_command(str(flow_rule))

And this can be come:

   flow_output = self.send_command(f"flow create {port_id} {flow_rule}")

> +        if verify:
> +            if "created" not in flow_output:
> +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to create flow rule:\n{flow_output}"
> +                )
> +
>       @requires_stopped_ports
>       def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
>           """Change the MTU of a port using testpmd.

Finally, I also agree with Juraj's comment to make a structure for 
actions and patterns. But that's overkill for now. We can just deal with 
it if we have to deal with their complexity in the future.

But like also Juraj's said, I'd introduce a flow_delete function with 
flow_create. You should make it so flow_create returns the flow rule ID, 
so that we can manipulate it, and use it with flow_delete.

Last thing, I'd split the FlowRule class and the flow_(create|delete) 
commands in two commits.

I hope my comments are helpful and make sense. Also mind that I don't 
have a thorough knowledge of flows, I've just had a crash course for the 
purpose of this review. So I may not understand everything and could be 
wrong, in which case please correct me.

Best,
Luca

[1] 
https://doc.dpdk.org/guides/testpmd_app_ug/testpmd_funcs.html#flow-syntax

[PATCH v5 1/2] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

Add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 44 ++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index d187eaea94..177fcf2e81 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -22,7 +22,7 @@
 from enum import Flag, auto
 from os import environ
 from pathlib import PurePath
-from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, ParamSpec, TypeAlias
+from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, Literal, ParamSpec, TypeAlias
 
 if TYPE_CHECKING or environ.get("DTS_DOC_BUILD"):
     from enum import Enum as NoAliasEnum
@@ -705,6 +705,48 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass(kw_only=True)
+class FlowRule:
+    """Class representation of flow rule parameters.
+
+    This class represents the parameters of any flow rule as per the
+    following pattern:
+
+    [group {group_id}] [priority {level}] [ingress] [egress]
+    [user_id {user_id}] pattern {item} [/ {item} [...]] / end
+    actions {action} [/ {action} [...]] / end
+    """
+
+    #:
+    group_id: int | None = None
+    #:
+    priority_level: int | None = None
+    #:
+    direction: Literal["ingress", "egress", "both"]
+    #:
+    user_id: int | None = None
+    #:
+    pattern: list[str]
+    #:
+    actions: list[str]
+
+    def __str__(self) -> str:
+        """Returns the string representation of this instance."""
+        ret = ""
+        pattern = " / ".join(self.pattern)
+        action = " / ".join(self.actions)
+        if self.group_id is not None:
+            ret += f"group {self.group_id} "
+        if self.priority_level is not None:
+            ret += f"priority {self.priority_level} "
+        ret += f"{self.direction} "
+        if self.user_id is not None:
+            ret += f"user_id {self.user_id} "
+        ret += f"pattern {pattern} / end "
+        ret += f"actions {action} / end"
+        return ret
+
+
 class PacketOffloadFlag(Flag):
     """Flag representing the Packet Offload Features Flags in DPDK.
 
-- 
2.44.0

[PATCH v5 2/2] dts: add flow create/delete to testpmd shell

[Dean Marx]

Add flow create/delete methods to TestPmdShell class
for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 59 ++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 177fcf2e81..bdb0e760b9 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -22,7 +22,15 @@
 from enum import Flag, auto
 from os import environ
 from pathlib import PurePath
-from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, Literal, ParamSpec, TypeAlias
+from typing import (
+    TYPE_CHECKING,
+    Any,
+    ClassVar,
+    Concatenate,
+    Literal,
+    ParamSpec,
+    TypeAlias,
+)
 
 if TYPE_CHECKING or environ.get("DTS_DOC_BUILD"):
     from enum import Enum as NoAliasEnum
@@ -1878,6 +1886,55 @@ def csum_set_hw(
                                                            {port_id}:\n{csum_output}"""
                     )
 
+    def flow_create(self, flow_rule: FlowRule, port_id: int, verify: bool = True) -> int:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            flow_rule: :class:`FlowRule` object used for creating testpmd flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+
+        Returns:
+            Id of created flow rule as an integer.
+        """
+        flow_output = self.send_command(f"flow create {port_id} {flow_rule}")
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+        match = re.search(r"#(\d+)", flow_output)
+        if match is not None:
+            match_str = match.group(1)
+            flow_id = int(match_str)
+            return flow_id
+        else:
+            self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+            raise InteractiveCommandExecutionError(f"Failed to create flow rule:\n{flow_output}")
+
+    def flow_delete(self, flow_id: int, port_id: int, verify: bool = True) -> None:
+        """Deletes the specified flow rule from the testpmd session.
+
+        Args:
+            flow_id: :class:`FlowRule` id used for deleting testpmd flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was deleted successfully.
+
+        Raises:
+            InteractiveCommandExectuionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(f"flow destroy {port_id} rule {flow_id}")
+        if verify:
+            if "destroyed" not in flow_output:
+                self._logger.debug(f"Failed to delete flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to delete flow rule:\n{flow_output}"
+                )
+
     @requires_stopped_ports
     def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
         """Change the MTU of a port using testpmd.
-- 
2.44.0

Re: [PATCH v2] dts: add flow rule dataclass to testpmd shell

[Jeremy Spewock]

On Thu, Aug 1, 2024 at 5:24 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Hi Dean, thank you for your work! Some minor comments.
>
> On 26/07/2024 15:15, Dean Marx wrote:
> > add dataclass for passing in flow rule creation arguments, as well as a
> > __str__ method for converting to a sendable testpmd command. Add
> > flow_create method to TestPmdShell class for initializing flow rules.
> >
> > Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> > ---
> >   dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++-
> >   1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> > index eda6eb320f..d6c111da0a 100644
> > --- a/dts/framework/remote_session/testpmd_shell.py
> > +++ b/dts/framework/remote_session/testpmd_shell.py
> > @@ -19,7 +19,7 @@
> >   from dataclasses import dataclass, field
> >   from enum import Flag, auto
> >   from pathlib import PurePath
> > -from typing import ClassVar
> > +from typing import ClassVar, Optional
> >
> >   from typing_extensions import Self, Unpack
> >
> > @@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser):
> >       tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
> >
> >
> > +@dataclass
> > +class flow_func:
> Class names should be UpperCamelCase, also should be a suitable name for
> what it's describing. I believe FlowRule should work.
> > +    """Dataclass for setting flow rule parameters."""
> > +
> > +    #:
> > +    port_id: int
> > +    #:
> > +    ingress: bool
> > +    #:
> > +    pattern: str
> > +    #:
> > +    actions: str
> > +
> > +    #:
> > +    group_id: Optional[int] = None
> > +    #:
> > +    priority_level: Optional[int] = None
> > +    #:
> > +    user_id: Optional[int] = None
> Optional[..] is an outdated notation. `int | None` is preferred instead.
> See PEP 604[1].
> > +
> > +    def __str__(self) -> str:
> > +        """Returns the string representation of a flow_func instance.
> > +
> > +        In this case, a properly formatted flow create command that can be sent to testpmd.
> > +        """
> > +        ret = []
> > +        ret.append(f"flow create {self.port_id} ")
> > +        ret.append(f"group {self.group_id} " if self.group_id is not None else "")
> > +        ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "")
> > +        ret.append("ingress " if self.ingress else "egress ")
> > +        ret.append(f"user_id {self.user_id} " if self.user_id is not None else "")
> > +        ret.append(f"pattern {self.pattern} ")
> > +        ret.append(" / end actions ")
> > +        ret.append(f"{self.actions} / end")
> > +        return "".join(ret)
>
> Using a list with inline conditional appending is not particularly
> readable. A regular string with conditional appending should do:
>
>     ret = f"flow create {self.port_id} "
>     if self.group_id is not None:
>          ret += f"group {self.group_id} "
>     ...
>
> Also the latest three append lines can all be in one, if you like the
> separation you can just do a multi-line string:
>
>     ret += (
>          f"pattern {self.pattern} / end "
>          f"actions {self.actions} / end"
>     )
>     # or actually this may be just fine:
>     ret += f"pattern {self.pattern} / end "
>     ret += f"actions {self.actions} / end"
>
> I guess the way it's split is more of a game changer.
>
> If you really want to use a list (in a way that is similar to what I've
> described here) then I'd take advantage of it... by omitting leading and
> trailing whitespaces and then use the join to add them in between: "
> ".join(ret)
>
> > +
> > +
> >   class TestPmdShell(DPDKShell):
> >       """Testpmd interactive shell.
> >
> > @@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
> >
> >           return TestPmdPortStats.parse(output)
> >
> > +    def flow_create(self, cmd: str, verify: bool = True) -> None:
>
> Not a comment, but a discussion point. Normally we'd want a function to
> be read as an action such as:
>
>     create_flow
>
> But I understand this is basically mirroring the command format... I
> wonder which one would be the best. I am personally inclined in verb
> first. Maybe others can give their opinion.

That's funny because Patrick also raised this point about whether to
use a more testpmd-oriented naming scheme or a more human-readable
one. I forget which patch it was on exactly, but Patrick did raise a
good point that if our goal is to have DPDK developers be able to
easily pick up and work with this API, they'll probably be more
familiar with the testpmd methods as they are now. I also lean more
towards the verb first method just because it makes it more readable I
think, but I can't speak for DPDK developers. Even if it were
`create_flow` though I'm sure people can just type `testpmd.flow` and
their IDE will be able to help with the auto-complete for the flow
rule options.

Maybe it is something we should discuss more however since it is come
up twice now.

> > +        """Creates a flow rule in the testpmd session.
> > +
> > +        Args:
> > +            cmd: String from flow_func instance to send as a flow rule.
> > +            verify: If :data:`True`, the output of the command is scanned
> > +            to ensure the flow rule was created successfully.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If flow rule is invalid.
> > +        """
> > +        flow_output = self.send_command(cmd)
> > +        if verify:
> > +            if "created" not in flow_output:
> > +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> > +                raise InteractiveCommandExecutionError(
> > +                    f"Failed to create flow rule:\n{flow_output}"
> > +                )
> > +
> >       def _close(self) -> None:
> >           """Overrides :meth:`~.interactive_shell.close`."""
> >           self.stop()
>
> [1] https://peps.python.org/pep-0604/
>

Re: [PATCH v2] dts: add flow rule dataclass to testpmd shell

[Luca Vizzarro]

Hi Dean, thank you for your work! Some minor comments.

On 26/07/2024 15:15, Dean Marx wrote:
> add dataclass for passing in flow rule creation arguments, as well as a
> __str__ method for converting to a sendable testpmd command. Add
> flow_create method to TestPmdShell class for initializing flow rules.
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>   dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index eda6eb320f..d6c111da0a 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -19,7 +19,7 @@
>   from dataclasses import dataclass, field
>   from enum import Flag, auto
>   from pathlib import PurePath
> -from typing import ClassVar
> +from typing import ClassVar, Optional
>   
>   from typing_extensions import Self, Unpack
>   
> @@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser):
>       tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
>   
>   
> +@dataclass
> +class flow_func:
Class names should be UpperCamelCase, also should be a suitable name for 
what it's describing. I believe FlowRule should work.
> +    """Dataclass for setting flow rule parameters."""
> +
> +    #:
> +    port_id: int
> +    #:
> +    ingress: bool
> +    #:
> +    pattern: str
> +    #:
> +    actions: str
> +
> +    #:
> +    group_id: Optional[int] = None
> +    #:
> +    priority_level: Optional[int] = None
> +    #:
> +    user_id: Optional[int] = None
Optional[..] is an outdated notation. `int | None` is preferred instead. 
See PEP 604[1].
> +
> +    def __str__(self) -> str:
> +        """Returns the string representation of a flow_func instance.
> +
> +        In this case, a properly formatted flow create command that can be sent to testpmd.
> +        """
> +        ret = []
> +        ret.append(f"flow create {self.port_id} ")
> +        ret.append(f"group {self.group_id} " if self.group_id is not None else "")
> +        ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "")
> +        ret.append("ingress " if self.ingress else "egress ")
> +        ret.append(f"user_id {self.user_id} " if self.user_id is not None else "")
> +        ret.append(f"pattern {self.pattern} ")
> +        ret.append(" / end actions ")
> +        ret.append(f"{self.actions} / end")
> +        return "".join(ret)

Using a list with inline conditional appending is not particularly 
readable. A regular string with conditional appending should do:

    ret = f"flow create {self.port_id} "
    if self.group_id is not None:
         ret += f"group {self.group_id} "
    ...

Also the latest three append lines can all be in one, if you like the 
separation you can just do a multi-line string:

    ret += (
         f"pattern {self.pattern} / end "
         f"actions {self.actions} / end"
    )
    # or actually this may be just fine:
    ret += f"pattern {self.pattern} / end "
    ret += f"actions {self.actions} / end"

I guess the way it's split is more of a game changer.

If you really want to use a list (in a way that is similar to what I've 
described here) then I'd take advantage of it... by omitting leading and 
trailing whitespaces and then use the join to add them in between: " 
".join(ret)

> +
> +
>   class TestPmdShell(DPDKShell):
>       """Testpmd interactive shell.
>   
> @@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>   
>           return TestPmdPortStats.parse(output)
>   
> +    def flow_create(self, cmd: str, verify: bool = True) -> None:

Not a comment, but a discussion point. Normally we'd want a function to 
be read as an action such as:

    create_flow

But I understand this is basically mirroring the command format... I 
wonder which one would be the best. I am personally inclined in verb 
first. Maybe others can give their opinion.
> +        """Creates a flow rule in the testpmd session.
> +
> +        Args:
> +            cmd: String from flow_func instance to send as a flow rule.
> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was created successfully.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If flow rule is invalid.
> +        """
> +        flow_output = self.send_command(cmd)
> +        if verify:
> +            if "created" not in flow_output:
> +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to create flow rule:\n{flow_output}"
> +                )
> +
>       def _close(self) -> None:
>           """Overrides :meth:`~.interactive_shell.close`."""
>           self.stop()

[1] https://peps.python.org/pep-0604/

[PATCH v2] dts: add flow rule dataclass to testpmd shell

[Dean Marx]

add dataclass for passing in flow rule creation arguments, as well as a
__str__ method for converting to a sendable testpmd command. Add
flow_create method to TestPmdShell class for initializing flow rules.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index eda6eb320f..d6c111da0a 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -19,7 +19,7 @@
 from dataclasses import dataclass, field
 from enum import Flag, auto
 from pathlib import PurePath
-from typing import ClassVar
+from typing import ClassVar, Optional
 
 from typing_extensions import Self, Unpack
 
@@ -577,6 +577,43 @@ class TestPmdPortStats(TextParser):
     tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
 
 
+@dataclass
+class flow_func:
+    """Dataclass for setting flow rule parameters."""
+
+    #:
+    port_id: int
+    #:
+    ingress: bool
+    #:
+    pattern: str
+    #:
+    actions: str
+
+    #:
+    group_id: Optional[int] = None
+    #:
+    priority_level: Optional[int] = None
+    #:
+    user_id: Optional[int] = None
+
+    def __str__(self) -> str:
+        """Returns the string representation of a flow_func instance.
+
+        In this case, a properly formatted flow create command that can be sent to testpmd.
+        """
+        ret = []
+        ret.append(f"flow create {self.port_id} ")
+        ret.append(f"group {self.group_id} " if self.group_id is not None else "")
+        ret.append(f"priority {self.priority_level} " if self.priority_level is not None else "")
+        ret.append("ingress " if self.ingress else "egress ")
+        ret.append(f"user_id {self.user_id} " if self.user_id is not None else "")
+        ret.append(f"pattern {self.pattern} ")
+        ret.append(" / end actions ")
+        ret.append(f"{self.actions} / end")
+        return "".join(ret)
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -804,6 +841,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    def flow_create(self, cmd: str, verify: bool = True) -> None:
+        """Creates a flow rule in the testpmd session.
+
+        Args:
+            cmd: String from flow_func instance to send as a flow rule.
+            verify: If :data:`True`, the output of the command is scanned
+            to ensure the flow rule was created successfully.
+
+        Raises:
+            InteractiveCommandExecutionError: If flow rule is invalid.
+        """
+        flow_output = self.send_command(cmd)
+        if verify:
+            if "created" not in flow_output:
+                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
+                raise InteractiveCommandExecutionError(
+                    f"Failed to create flow rule:\n{flow_output}"
+                )
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
-- 
2.44.0