On Mon, Jun 10, 2024 at 11:03 AM Juraj Linkeš
<juraj.lin...@pantheon.tech> wrote:
>
>
>
> On 5. 6. 2024 23:31, jspew...@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspew...@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 <jspew...@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...")
