Re: [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 5. 6. 2024 23:31, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> There are methods within DTS currently that support updating the MTU of
> ports on a node, but the methods for doing this in a linux session rely
> on the ip command and the port being bound to the kernel driver. Since
> test suites are run while bound to the driver for DPDK, there needs to
> be a way to modify the value while bound to said driver as well. This is
> done by using testpmd to modify the MTU.
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>   .../remote_session/interactive_shell.py       | 14 +++-
>   dts/framework/remote_session/testpmd_shell.py | 76 ++++++++++++++++++-
>   2 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 6dee7ebce0..34d1acf439 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -139,7 +139,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>               raise InteractiveCommandExecutionError("Failed to start application.")
>           self._ssh_channel.settimeout(self._timeout)
>   
> -    def send_command(self, command: str, prompt: str | None = None) -> str:
> +    def send_command(
> +        self, command: str, prompt: str | None = None, print_to_debug: bool = False
> +    ) -> str:

As I mentioned in v2, this really should be in a separate patch, as it 
affects other parts of the code and the solution should be designed with 
that in mind.

>           """Send `command` and get all output before the expected ending string.
>   
>           Lines that expect input are not included in the stdout buffer, so they cannot
> @@ -155,6 +157,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>               command: The command to send.
>               prompt: After sending the command, `send_command` will be expecting this string.
>                   If :data:`None`, will use the class's default prompt.
> +            print_to_debug: If :data:`True` the logging message that displays what command is
> +                being sent prior to sending it will be logged at the debug level instead of the
> +                info level. Useful when a single action requires multiple commands to complete to
> +                avoid clutter in the logs.
>   
>           Returns:
>               All output in the buffer before expected string.
> @@ -163,7 +169,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>               raise InteractiveCommandExecutionError(
>                   f"Cannot send command {command} to application because the shell is not running."
>               )
> -        self._logger.info(f"Sending: '{command}'")
> +        log_message = f"Sending: '{command}'"
> +        if print_to_debug:
> +            self._logger.debug(log_message)
> +        else:
> +            self._logger.info(log_message)
>           if prompt is None:
>               prompt = self._default_prompt
>           self._stdin.write(f"{command}{self._command_extra_chars}\n")
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ca30aac264..f2fa842b7f 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -135,10 +135,11 @@ def start(self, verify: bool = True) -> None:
>               InteractiveCommandExecutionError: If `verify` is :data:`True` and forwarding fails to
>                   start or ports fail to come up.
>           """
> -        self.send_command("start")
> +        self._logger.info('Starting packet forwarding and waiting for port links to be "up".')
> +        self.send_command("start", print_to_debug=True)
>           if verify:
>               # If forwarding was already started, sending "start" again should tell us
> -            start_cmd_output = self.send_command("start")
> +            start_cmd_output = self.send_command("start", print_to_debug=True)
>               if "Packet forwarding already started" not in start_cmd_output:
>                   self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}")
>                   raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.")
> @@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>                   f"Test pmd failed to set fwd mode to {mode.value}"
>               )
>   
> +    def _stop_port(self, port_id: int, verify: bool = True) -> None:
> +        """Stop port `port_id` in testpmd.

Either "Stop `port_id` in testpmd." or "Stop port with `port_id` in 
testpmd.". I like the latter more.

> +
> +        Depending on the PMD, the port may need to be stopped before configuration can take place.
> +        This method wraps the command needed to properly stop ports and take their link down.
> +
> +        Args:
> +            port_id: ID of the port to take down.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                stopping of ports was successful. Defaults to True.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
> +                successfully stop.
> +        """
> +        stop_port_output = self.send_command(f"port stop {port_id}", print_to_debug=True)
> +        if verify and ("Done" not in stop_port_output):
> +            self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
> +            raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
> +
> +    def _start_port(self, port_id: int, verify: bool = True) -> None:
> +        """Start port `port_id` in testpmd.
> +
> +        Because the port may need to be stopped to make some configuration changes, it naturally
> +        follows that it will need to be started again once those changes have been made.
> +
> +        Args:
> +            port_id: ID of the port to start.
> +            verify: If :data:`True` the output will be scanned in an attempt to verify that the
> +                port came back up without error. Defaults to True.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
> +                back up.
> +        """
> +        start_port_output = self.send_command(f"port start {port_id}", print_to_debug=True)
> +        if verify and ("Done" not in start_port_output):
> +            self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
> +            raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
> +
> +    def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
> +        """Change the MTU of a port using testpmd.
> +
> +        Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
> +        stop the port before configuring in cases where it isn't required, so we first stop ports,
> +        then update the MTU, then start the ports again afterwards.
> +
> +        Args:
> +            port_id: ID of the port to adjust the MTU on.
> +            mtu: Desired value for the MTU to be set to.
> +            verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
> +                verify that the mtu was properly set on the port. Defaults to True.

The second instance of True should also be :data:`True`.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
> +                properly updated on the port matching `port_id`.
> +        """
> +        self._logger.info(f"Changing MTU of port {port_id} to be {mtu}")
> +        self._stop_port(port_id, verify)
> +        set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}", print_to_debug=True)
> +        self._start_port(port_id, verify)

Would making _stop_port and _start_port a decorator work? Can we do the 
verification even if the port is stopped?

> +        if verify and (
> +            f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}", print_to_debug=True)
> +        ):
> +            self._logger.debug(
> +                f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
> +            )
> +            raise InteractiveCommandExecutionError(
> +                f"Test pmd failed to update mtu of port {port_id} to {mtu}"
> +            )
> +
>       def _close(self) -> None:
>           """Overrides :meth:`~.interactive_shell.close`."""
>           self.send_command("quit", "Bye...")