Re: [RFC v2 1/2] dts: add smoke tests

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

I'm sending the second part of the review separately. This is only
about implementation-specific details, the other comments still apply
(such as how I'd like to move/structure the code).

In general, it really looks like we have to go this route. It's kind
of a pexpect-lite approach, which seems fine for this simple use case
(by which I mean the prompt is very well defined and we don't need to
cover exotic corner cases).

> diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py
> index 8a151221..abca8edc 100644
> --- a/dts/framework/remote_session/remote/__init__.py
> +++ b/dts/framework/remote_session/remote/__init__.py
> @@ -9,8 +9,36 @@
>  from .remote_session import CommandResult, RemoteSession
>  from .ssh_session import SSHSession
>
> +from paramiko import SSHClient, AutoAddPolicy
> +from framework.utils import GREEN
>
>  def create_remote_session(
>      node_config: NodeConfiguration, name: str, logger: DTSLOG
>  ) -> RemoteSession:
>      return SSHSession(node_config, name, logger)
> +
> +def create_interactive_session(
> +    node_config: NodeConfiguration, name: str, logger: DTSLOG
> +) -> SSHClient:
> +    """
> +    Creates a paramiko SSH session that is designed to be used for interactive shells
> +
> +    This session is meant to be used on an "as needed" basis and may never be utilized
> +    """
> +    client: SSHClient = SSHClient()
> +    client.set_missing_host_key_policy(AutoAddPolicy)
> +    ip: str = node_config.hostname
> +    logger.info(GREEN(f"Connecting to host {ip}"))
> +    #Preset to 22 because paramiko doesn't accept None
> +    port: int = 22
> +    if ":" in node_config.hostname:
> +            ip, port = node_config.hostname.split(":")
> +            port = int(port)
> +    client.connect(
> +        ip,
> +        username=node_config.user,
> +        port=port,
> +        password=node_config.password or "",
> +        timeout=20 if port else 10
> +    )

The connect call needs to be in a try/except block where we should
catch the exceptions listed in paramiko docs and convert them into
either SSHConnectionError or SSHTimeoutError. I'd like to make it as
close to what we're doing in
http://patches.dpdk.org/project/dpdk/patch/20230424133537.58698-1-juraj.linkes@pantheon.tech/
as possible.

> +    return client

> diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py
> index f54a9470..63f17cc3 100644
> --- a/dts/framework/testbed_model/__init__.py
> +++ b/dts/framework/testbed_model/__init__.py
> @@ -20,3 +20,8 @@
>  )
>  from .node import Node
>  from .sut_node import SutNode
> +
> +from .interactive_apps import (
> +    InteractiveScriptHandler,
> +    TestpmdDriver
> +)
> diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py
> new file mode 100644
> index 00000000..0382d7e0
> --- /dev/null
> +++ b/dts/framework/testbed_model/interactive_apps/__init__.py
> @@ -0,0 +1,6 @@
> +from .interactive_command import (
> +    InteractiveScriptHandler
> +)
> +from .testpmd_driver import (
> +    TestpmdDriver
> +)
> \ No newline at end of file
> diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py
> new file mode 100644
> index 00000000..7467911b
> --- /dev/null
> +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py
> @@ -0,0 +1,57 @@
> +# import paramiko
> +from paramiko import SSHClient, Channel, channel
> +from framework.settings import SETTINGS
> +
> +class InteractiveScriptHandler:
> +
> +    _ssh_client: SSHClient
> +    _stdin: channel.ChannelStdinFile
> +    _ssh_channel: Channel
> +
> +    def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None:
> +        self._ssh_client = ssh_client
> +        self._ssh_channel = self._ssh_client.invoke_shell()
> +        self._stdin = self._ssh_channel.makefile_stdin("wb")

Do we need to open this channel in binary mode? Are there any dpdk
apps which send binary data instead of text?

> +        self._ssh_channel.settimeout(timeout)

Have you tried using the recv and send methods? Reading the docs, it
looks like using the file-like object approach may be more
straightforward for our case (as the prompt we're expecting is at the
end of a line). What are the differences in practice?

> +
> +    def send_command(self, command:str) -> None:
> +        """
> +        Send command to channel without recording output.
> +
> +        This method will not verify any input or output, it will
> +        simply assume the command succeeded
> +        """
> +        self._stdin.write(command + '\n')
> +        self._stdin.flush()

I don't see this used anywhere. You mentioned it would be needed for
username/password prompts, but paramiko handles that. Do we need this
for any DPDK app? And even if we do, we should only add it when the
need arises.

> +
> +    def send_command_get_output(self, command:str, expect:str) -> str:
> +        """
> +        Send a command and get all output before the expected ending string.
> +
> +        **NOTE**

The note preface is not needed. We haven't yet 100% agreed on the
docstring format, but in the meantime, let's use the best candidate,
google docs format:
https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings

> +        Lines that expect input are not included in the stdout buffer so they cannot be
> +        used for expect.

I'm curious, in the case of testpmd, aren't the lines that contain the
prompt also the ones that expect input? In which case we'd never match
the prompt without the workaround below. Or am I misunderstanding
something?

 For example, if you were prompted to log into something
> +        with a username and password, you cannot expect "username:" because it wont
> +        yet be in the stdout buffer. A work around for this could be consuming an
> +        extra newline character to force the current prompt into the stdout buffer.
> +
> +        *Return*
> +            All output before expected string
> +        """
> +        stdout = self._ssh_channel.makefile("r")

Why the different workflow for stdin and stdout?
And where's stderr? I imagine we'd want to capture that as well (to
check errors), or do dpdk apps not emit anything to stderr?

> +        self._stdin.write(command + '\n')

Let's use an f-string here like everywhere else. There are other
places where this applies as well.
We should also add a logger so that we capture what we send and receive.

> +        self._stdin.flush()
> +        out:str = ""
> +        for line in stdout:

Do we need to worry about different line breaks on different OS's? Or
do dpdk apps always terminate lines with \n?

> +            out += str(line)

Do we need the string conversion even if the channel has not been
opened in binary mode? Or is it opened as such by default?

> +            if expect in str(line):

The prompt should always be at the end of the line, right? Would
checking the line.endswith(expect) make more sense (or
line.rstrip().endswith(expect))?

> +                break
> +        stdout.close() #close the buffer to flush the output

The docs say closing only does flushing, so we might as well call
flush so the code is more consistent (we're calling flush on stdin,
not close).

> +        return out
> +
> +    def close(self):
> +        self._stdin.close()
> +        self._ssh_channel.close()
> +
> +    def __del__(self):
> +        self.close()
> diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py
> new file mode 100644
> index 00000000..1993eae6
> --- /dev/null
> +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py
> @@ -0,0 +1,24 @@
> +from framework.testbed_model.interactive_apps import InteractiveScriptHandler
> +
> +from pathlib import PurePath
> +
> +class TestpmdDriver:
> +    prompt:str = "testpmd>"
> +    interactive_handler: InteractiveScriptHandler
> +
> +    def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None:
> +        """
> +        Sets the handler to drive the SSH session and starts testpmd
> +        """
> +        self.interactive_handler = handler
> +        # self.interactive_handler.send_command("sudo su")
> +        # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build")
> +        self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt)
> +
> +    def send_command(self, command:str, expect:str = prompt) -> str:
> +        """
> +        Specific way of handling the command for testpmd
> +
> +        An extra newline character is consumed in order to force the current line into the stdout buffer
> +        """
> +        return self.interactive_handler.send_command_get_output(command + "\n", expect)

The newline is already being added. Or is this the workaround (i.e. we
need two newlines?). If so, move the second newline to
send_command_get_output.

> \ No newline at end of file
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py

<snip>

> +    def test_device_bound_to_driver(self) -> None:
> +        """
> +        Test:
> +            ensure that all drivers listed in the config are bound to the correct drivers
> +        """
> +        for nic in self.sut_node._execution_config.nics:
> +            for address in nic.addresses:
> +                out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60)

I didn't finish my previous comment, so here's the finish: :-)
This should follow the same abstractions as I outlined above.
However, we don't need to use dpdk-devbind here. It's probably safe to
do so (the script is likely not going to be changed), but we could
interrogate the device the same way the script does:
ls /sys/bus/pci/devices/

And then check that the PCI is listed. Inside the PCI dir, we can
check what driver it's bound to (if any) and basically anything else
we need about the device (but not everything - firmware version comes
to mind).

> +                self.verify(
> +                    len(out.stdout) != 0,
> +                    f"Failed to find configured device ({address}) using dpdk-devbind.py",
> +                )
> +                for string in out.stdout.split(" "):
> +                    if 'drv=' in string:
> +                        self.verify(
> +                            string.split("=")[1] == nic.driver.strip(),
> +                            f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})',
> +                        )
> +