Re: [RFC v1 1/3] dts: add UDP tunnel command to testpmd shell

[Jeremy Spewock <jspewock@iol.unh.edu>]

Hey Dean, these changes look good to me, I just had a few minor
comments/suggestions.

One thing I did notice was that the methods added here don't have
type-hints for their return-types, obviously functionally it makes no
difference since they don't return anything, but just adding the note
that says the return None is helpful for type checkers and
understanding the method at a glance.

On Thu, Jul 25, 2024 at 12:23 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> add udp_tunnel_port command to testpmd shell class,
> also ports over set verbose method from vlan suite
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 51 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index eda6eb320f..26114091d6 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -804,7 +804,56 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> -    def _close(self) -> None:

It looks like this method might have been renamed by mistake in a
rebase, the name on main right now is _close. This could cause some
weird behavior in your testing since this is what the context manager
uses to close the session, but I don't think it would have any drastic
effect since the channel is still closed.

> +    def set_verbose(self, level: int, verify: bool = True):
> +        """Set debug verbosity level.
> +
> +        Args:
> +            level: 0 - silent except for error
> +                1 - fully verbose except for Tx packets
> +                2 - fully verbose except for Rx packets
> +                >2 - fully verbose
> +            verify: If :data:`True` the command output will be scanned to verify that verbose level
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and verbose level
> +            is not correctly set.
> +        """
> +        verbose_output = self.send_command(f"set verbose {level}")
> +        if verify:
> +            if "Change verbose level" not in verbose_output:
> +                self._logger.debug(f"Failed to set verbose level to {level}: \n{verbose_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set verbose level to {level}."
> +                )
> +
> +    def udp_tunnel_port(
> +        self, port_id: int, add: bool, udp_port: int, protocol: str, verify: bool = True
> +    ):
> +        """Configures a UDP tunnel on the specified port, for the specified protocol.
> +
> +        Args:
> +            port_id: ID of the port to configure tunnel on.
> +            add: If :data:`True`, adds tunnel, otherwise removes tunnel.
> +            udp_port: ID of the UDP port to configure tunnel on.
> +            protocol: Name of tunnelling protocol to use; options are vxlan, geneve, ecpri

If there are explicit choices that this has to be like this it might
be better to put these options into an enum and then pass that in as
the parameter here. That way it is very clear from just calling the
methods what your options are.



> +            verify: If :data:`True`, checks the output of the command to verify that
> +                no errors were thrown.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If verify is :data:`True` and command
> +                output shows an error.
> +        """
> +        action = "add" if add else "rm"
> +        cmd_output = self.send_command(
> +            f"port config {port_id} udp_tunnel_port {action} {protocol} {udp_port}"
> +        )
> +        if verify:
> +            if "Operation not supported" in cmd_output or "Bad arguments" in cmd_output:
> +                self._logger.debug(f"Failed to set UDP tunnel: \n{cmd_output}")
> +                raise InteractiveCommandExecutionError(f"Failed to set UDP tunnel: \n{cmd_output}")
> +
> +    def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
>          self.send_command("quit", "Bye...")
> --
> 2.44.0
>