[v3,1/3] Added VLAN commands to testpmd_shell class

Message ID 20240614150238.26374-2-dmarx@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series VLAN Test Suite |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dean Marx June 14, 2024, 3:02 p.m. UTC
  The testpmd_shell class is intended for sending commands to
 a remote testpmd session within a test suite, and as of right now 
it is missing most commands. Added commands for vlan filtering, 
stripping, and insertion, as well as stopping and starting ports 
during testpmd runtime.

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

Comments

Patrick Robb June 14, 2024, 3:59 p.m. UTC | #1
Unaddressed
On Fri, Jun 14, 2024 at 11:03 AM Dean Marx <dmarx@iol.unh.edu> wrote:
> +    def vlan_filter_set_on(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter on.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was enabled successfully. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter on {port}")

I wonder whether, when convenient, we want to name the methods more or
less 1:1 according to the actual testpmd text command they send? I.e.
in this case should the method be named vlan_set_filter_on instead of
vlan_filter_set_on (aligns better with "vlan set filter on {port}")?
The intellisense provided by the testpmd methods is indeed a QoL
improvement for folks writing testsuites, but at the same time people
who use testpmd will always have the real commands ingrained in their
thoughts, so if we try to stay as true to those as possible, we get
the stability and intellisense and also the method names are still
intuitive.

Maybe even think tiny things like renaming the method set_forward_mode
to set_fwd_mode to align 1:1 with testpmd is good.

That's just my perspective though - I would be interested to see
whether others feel the same or not.

> +        if verify:
> +            if "Invalid port" in filter_cmd_output:
> +                self._logger.debug(f"Failed to enable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to enable vlan filter.")
> +
> +    def vlan_filter_set_off(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter off.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was disabled successfully. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter off {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output:
> +                self._logger.debug(f"Failed to disable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to disable vlan filter.")
> +
> +    def rx_vlan_add(self, vlan: int = 0, port: int = 0, verify: bool = True):
> +        """Add specified vlan tag to the filter list on a port.
> +
> +        Args:
> +            vlan: The vlan tag to add, should be within 1-4094.

Worth clarifying that 4094 is the extended vlan range? Normal range is 1-1005?

> +
> +    def port_stop_all(self):
> +        """Stop all ports."""
> +        self.send_command("port stop all")

So the idea is to have distinct port_stop_all() and port_stop(self,
port: int) methods? I think this is reasonable.

> +
> +    def port_start_all(self):
> +        """Start all ports."""
> +        self.send_command("port start all")
> +
> +    def tx_vlan_set(self, port: int = 0, vlan: int = 0, verify: bool = True):
> +        """Set hardware insertion of vlan tags in packets sent on a port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            vlan: The vlan tag to insert, should be within 1-4094.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan insertion was enabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_insert_output = self.send_command(f"tx_vlan set {port} {vlan}")
> +        if verify:
> +            if ("Please stop port" in vlan_insert_output or "Invalid vlan_id" in vlan_insert_output
> +            or "Invalid port" in vlan_insert_output):
> +                self._logger.debug(f"Failed to set vlan insertion tag: \n{vlan_insert_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to set vlan insertion tag.")
> +
> +    def tx_vlan_reset(self, port: int = 0, verify: bool = True):
> +        """Disable hardware insertion of vlan tags in packets sent on a port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan insertion was disabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_insert_output = self.send_command(f"tx_vlan set {port}")
> +        if verify:
> +            if "Please stop port" in vlan_insert_output or "Invalid port" in vlan_insert_output:
> +                self._logger.debug(f"Failed to reset vlan insertion: \n{vlan_insert_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to reset vlan insertion.")

Could possibly be combined with tx_vlan_set() as one method which
handles both set and reset, but I think splitting them out is fine.

> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.44.0
>
  
Jeremy Spewock June 14, 2024, 8:29 p.m. UTC | #2
On Fri, Jun 14, 2024 at 12:00 PM Patrick Robb <probb@iol.unh.edu> wrote:
>
> On Fri, Jun 14, 2024 at 11:03 AM Dean Marx <dmarx@iol.unh.edu> wrote:
> > +    def vlan_filter_set_on(self, port: int = 0, verify: bool = True):
> > +        """Set vlan filter on.
> > +
> > +        Args:
> > +            port: The port number to use, should be within 0-32.
> > +            verify: If :data:`True`, the output of the command is scanned to verify that
> > +                vlan filtering was enabled successfully. If not, it is
> > +                considered an error.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> > +                fails to update.
> > +        """
> > +        filter_cmd_output = self.send_command(f"vlan set filter on {port}")
>
> I wonder whether, when convenient, we want to name the methods more or
> less 1:1 according to the actual testpmd text command they send? I.e.
> in this case should the method be named vlan_set_filter_on instead of
> vlan_filter_set_on (aligns better with "vlan set filter on {port}")?
> The intellisense provided by the testpmd methods is indeed a QoL
> improvement for folks writing testsuites, but at the same time people
> who use testpmd will always have the real commands ingrained in their
> thoughts, so if we try to stay as true to those as possible, we get
> the stability and intellisense and also the method names are still
> intuitive.

I generally try to name these methods in a more human-readable way, so
my intuition would lean more towards naming it something like
`set_vlan_filter_on` or `set_vlan_strip_on`. I could see how it might
make it easier for some testpmd developers, so I'm not sure which
would be better. Personally, as someone who is less familiar with all
the ins and outs of testpmd, I prefer the human-readable approach.

>
> Maybe even think tiny things like renaming the method set_forward_mode
> to set_fwd_mode to align 1:1 with testpmd is good.
>
> That's just my perspective though - I would be interested to see
> whether others feel the same or not.
>
<snip>
> > 2.44.0
> >
  
Patrick Robb June 14, 2024, 9:24 p.m. UTC | #3
On Fri, Jun 14, 2024 at 4:29 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:

> > I wonder whether, when convenient, we want to name the methods more or
> > less 1:1 according to the actual testpmd text command they send? I.e.
> > in this case should the method be named vlan_set_filter_on instead of
> > vlan_filter_set_on (aligns better with "vlan set filter on {port}")?
> > The intellisense provided by the testpmd methods is indeed a QoL
> > improvement for folks writing testsuites, but at the same time people
> > who use testpmd will always have the real commands ingrained in their
> > thoughts, so if we try to stay as true to those as possible, we get
> > the stability and intellisense and also the method names are still
> > intuitive.
>
> I generally try to name these methods in a more human-readable way, so
> my intuition would lean more towards naming it something like
> `set_vlan_filter_on` or `set_vlan_strip_on`. I could see how it might
> make it easier for some testpmd developers, so I'm not sure which
> would be better. Personally, as someone who is less familiar with all
> the ins and outs of testpmd, I prefer the human-readable approach.

I think this is compelling, but to play devil's advocate, I also think
it's not just testpmd developers who might care. The broad community
of DPDK developers, who are working on DPDK features and might want to
write DTS testsuites in the future, are probably also already pretty
familiar with testpmd params. If they start using the framework, it
may be a benefit for the method names to be intuitive, so they don't
have to in essence relearn how to use testpmd just to write testsuites
for DTS. Probably not a big deal given as you say the human readable
approach is pretty intuitive too... anyways maybe this can be a
discussion point at the DTS meeting next week.

I guess people can always fall back on testpmd.send_command() if they
don't like the human readable methods...? Or would we want to ban
that?

https://doc.dpdk.org/guides/testpmd_app_ug/testpmd_funcs.html

Should testpmd class have a method for each runtime command? It's a
long list. Or just methods for the most common commands? Well, we'll
chat at the DTS meeting.

>
> >
> > Maybe even think tiny things like renaming the method set_forward_mode
> > to set_fwd_mode to align 1:1 with testpmd is good.
> >
> > That's just my perspective though - I would be interested to see
> > whether others feel the same or not.
> >
> <snip>
> > > 2.44.0
> > >
  
Jeremy Spewock June 17, 2024, 2:37 p.m. UTC | #4
In the methods you added here, you made all of the parameters to them
optional with a default. This might make sense to allow developers to
be less verbose when calling the method with the defaults, but in the
case of things like the port ID, vlan IDs, and things of that nature I
think making these positional arguments that are required makes more
sense for readability. For example, if I were to call
testpmd.vlan_filter_set_on(), it is not clear where this vlan filter
is being modified. The default is 0 so it will set it on port 0, but
if you're just reading the test suite you wouldn't know that without
looking at the testpmd class. Verify is left as an optional parameter
because we really want to verify that the commands were successful in
all cases, but there could be an obscure reason in the future that a
developer might not really care if a command to testpmd was successful
or not.

I also noticed in most cases where you were verifying commands that
you had to check if multiple failures messages were present. Would it
be easier to check if the command was successful by querying things
like VLANs on a port or VLAN filtering status of a port instead of
trying to cover all error cases?

On Fri, Jun 14, 2024 at 11:03 AM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> The testpmd_shell class is intended for sending commands to
>  a remote testpmd session within a test suite, and as of right now
> it is missing most commands. Added commands for vlan filtering,
> stripping, and insertion, as well as stopping and starting ports
> during testpmd runtime.

While it's true that the testpmd class is missing a lot of commands
that exist in testpmd, that isn't really why you are adding them.
Maybe switching this to say something like "It is missing
functionality required for verifying VLAN features on a PMD." might be
more descriptive.

>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
<snip>
> +    def vlan_filter_set_on(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter on.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was enabled successfully. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter on {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output:

This doesn't really verify that the filter was set as much as it
verifies that the port exists. Is there anything that testpmd prints
when it is successful, or possibly a way to query existing VLAN
filters on a port to see if it was set?

> +                self._logger.debug(f"Failed to enable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to enable vlan filter.")
> +
> +    def vlan_filter_set_off(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter off.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was disabled successfully. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter off {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output:

Same thing as above comment about verification.

> +                self._logger.debug(f"Failed to disable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to disable vlan filter.")
> +
<snip>
> +    def vlan_strip_set_on(self, port: int = 0, verify: bool = True):
> +        """Enable vlan stripping on the specified port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan stripping was enabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip on {port}")
> +        if verify:
> +            if "Invalid port" in vlan_strip_output:

This is similar to above where it isn't really verifying that the vlan
stripping was enabled.

> +                self._logger.debug(f"Failed to add vlan tag: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to add vlan tag.")
> +
> +    def vlan_strip_set_off(self, port: int = 0, verify: bool = True):
> +        """Disable vlan stripping on the specified port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan stripping was disabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip off {port}")
> +        if verify:
> +            if "Invalid port" in vlan_strip_output:

Same here.

> +                self._logger.debug(f"Failed to enable stripping: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to enable stripping on port {port}.")

On all of the other verification methods the message for the error
doesn't include specifics like what port it was on or what vlan tag
failed. I am generally more in favor of including specific information
like this and many of the other testpmd methods do, so if possible I
think we should add some of this information to the other methods here
as well to make it consistent.

> +
<snip>
> 2.44.0
>
  

Patch

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..5bbc7d3b1c 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -225,6 +225,170 @@  def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def vlan_filter_set_on(self, port: int = 0, verify: bool = True):
+        """Set vlan filter on.
+
+        Args:
+            port: The port number to use, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan filtering was enabled successfully. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        filter_cmd_output = self.send_command(f"vlan set filter on {port}")
+        if verify:
+            if "Invalid port" in filter_cmd_output:
+                self._logger.debug(f"Failed to enable vlan filter: \n{filter_cmd_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to enable vlan filter.")
+
+    def vlan_filter_set_off(self, port: int = 0, verify: bool = True):
+        """Set vlan filter off.
+
+        Args:
+            port: The port number to use, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan filtering was disabled successfully. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        filter_cmd_output = self.send_command(f"vlan set filter off {port}")
+        if verify:
+            if "Invalid port" in filter_cmd_output:
+                self._logger.debug(f"Failed to disable vlan filter: \n{filter_cmd_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to disable vlan filter.")
+
+    def rx_vlan_add(self, vlan: int = 0, port: int = 0, verify: bool = True):
+        """Add specified vlan tag to the filter list on a port.
+
+        Args:
+            vlan: The vlan tag to add, should be within 1-4094.
+            port: The port number to add the tag on, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                the vlan tag was added to the filter list on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_add_output = self.send_command(f"rx_vlan add {vlan} {port}")
+        if verify:
+            if "VLAN-filtering disabled" in vlan_add_output or "Invalid vlan_id" in vlan_add_output:
+                self._logger.debug(f"Failed to add vlan tag: \n{vlan_add_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to add vlan tag.")
+
+    def rx_vlan_rm(self, vlan: int = 0, port: int = 0, verify: bool = True):
+        """Remove specified vlan tag from filter list on a port.
+
+        Args:
+            vlan: The vlan tag to remove, should be within 1-4094.
+            port: The port number to remove the tag from, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                the vlan tag was removed from the filter list on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_rm_output = self.send_command(f"rx_vlan rm {vlan} {port}")
+        if verify:
+            if "VLAN-filtering disabled" in vlan_rm_output or "Invalid vlan_id" in vlan_rm_output:
+                self._logger.debug(f"Failed to add vlan tag: \n{vlan_rm_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to add vlan tag.")
+
+    def vlan_strip_set_on(self, port: int = 0, verify: bool = True):
+        """Enable vlan stripping on the specified port.
+
+        Args:
+            port: The port number to use, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan stripping was enabled on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_strip_output = self.send_command(f"vlan set strip on {port}")
+        if verify:
+            if "Invalid port" in vlan_strip_output:
+                self._logger.debug(f"Failed to add vlan tag: \n{vlan_strip_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to add vlan tag.")
+
+    def vlan_strip_set_off(self, port: int = 0, verify: bool = True):
+        """Disable vlan stripping on the specified port.
+
+        Args:
+            port: The port number to use, should be within 0-32
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan stripping was disabled on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_strip_output = self.send_command(f"vlan set strip off {port}")
+        if verify:
+            if "Invalid port" in vlan_strip_output:
+                self._logger.debug(f"Failed to enable stripping: \n{vlan_strip_output}")
+                raise InteractiveCommandExecutionError(f"Testpmd failed to enable stripping on port {port}.")
+
+    def port_stop_all(self):
+        """Stop all ports."""
+        self.send_command("port stop all")
+
+    def port_start_all(self):
+        """Start all ports."""
+        self.send_command("port start all")
+
+    def tx_vlan_set(self, port: int = 0, vlan: int = 0, verify: bool = True):
+        """Set hardware insertion of vlan tags in packets sent on a port.
+
+        Args:
+            port: The port number to use, should be within 0-32.
+            vlan: The vlan tag to insert, should be within 1-4094.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan insertion was enabled on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_insert_output = self.send_command(f"tx_vlan set {port} {vlan}")
+        if verify:
+            if ("Please stop port" in vlan_insert_output or "Invalid vlan_id" in vlan_insert_output
+            or "Invalid port" in vlan_insert_output):
+                self._logger.debug(f"Failed to set vlan insertion tag: \n{vlan_insert_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to set vlan insertion tag.")
+
+    def tx_vlan_reset(self, port: int = 0, verify: bool = True):
+        """Disable hardware insertion of vlan tags in packets sent on a port.
+
+        Args:
+            port: The port number to use, should be within 0-32.
+            verify: If :data:`True`, the output of the command is scanned to verify that
+                vlan insertion was disabled on the specified port. If not, it is
+                considered an error.
+
+        Raises:
+            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
+                fails to update.
+        """
+        vlan_insert_output = self.send_command(f"tx_vlan set {port}")
+        if verify:
+            if "Please stop port" in vlan_insert_output or "Invalid port" in vlan_insert_output:
+                self._logger.debug(f"Failed to reset vlan insertion: \n{vlan_insert_output}")
+                raise InteractiveCommandExecutionError("Testpmd failed to reset vlan insertion.")
+
     def close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")