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

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

On Mon, Jun 10, 2024 at 11:03 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> 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.

Ack.

>
> >           """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.

Ack.

>
> > +
> > +        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`.

Ugh, good catch! My IDE generates boilerplate for them for me and I
caught this in another method I added here, but I guess I missed this
one.

>
> > +
> > +        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?

I like this idea a lot. I just checked and it looks like you can do
the validation while the port is stopped, so I'll make this change.


>
> > +        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...")