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

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

[-- Attachment #1: Type: text/plain, Size: 17093 bytes --]

On Wed, May 24, 2023 at 6:03 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

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

You're right, in my attempts to just figure it out and get everything
working I neglected to think about catching the error here, good catch.


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

It does work without the binary mode as well. I don't think there are any
that necessarily need the binary data so you're right, this could likely be
removed.


>
> > +        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?
>
>
I did try using recv instead and I believe both would work in theory. I
ended up choosing to go with the file-like approach because it seemed
simpler while still reaching the same goal. The only difference I could
think of between the two is that with file-like buffers we can stop
receiving output as soon as you hit the expected prompt so you could expect
something in the center of the output rather than the end if you felt the
need to. I believe with recv we would just have to repeatedly accept all
the data in the buffer and then stop if that data had the expected output.
In our case however, this likely wouldn't cause a problem.


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

The newline in this buffer is treated as just sending the command. The
username/password prompts were just an example I used of something that
would expect input from the user, you're right that this is handled
internally (and maybe I should change my example!). Adding a newline here
is just simulating you pressing enter after writing the command and is used
by the shell to know that you're finished adding input.


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


I'll adapt to this format, I was just trying to write something that would
stand out here as that's an important detail.


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

That is correct, without the workaround that adds an extra newline, the
prompt "testpmd>" wouldn't make it into the buffer at the end of the output.


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

The different workflows for stdin and stdout exist because you can't
actually flush stdout using the flush method. Originally, I wanted to have
them both as buffers that are members of the class but this would mean that
stdout would include all of the output that has been collected since it has
been opened. So, we have to only open stdout when we need to capture output
and close it when we no longer want any output (this also disposes of old
output) whereas stdin is flushed every time you send the input and so it
can be reused.

As for the stderr, I believe most DPDK apps only emit data to stdout, but
this is a good idea to capture. I had thought that it was still picked up
by the output file here but after some more testing this isn't the case so
I'll look into capturing stderr too for detecting errors.


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

I don't believe we do, I think this is something that is handled by the
next() method of the buffer. It's listed in the paramiko documentation that
this should be handled on newline which I would think should handle
different OS's but this is a good thing to lookout for and ensure does work.


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

Looking back at the documentation, you're right that this isn't necessary
if you aren't reading in byte mode, I'll remove it.


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

It would be in terms of DPDK apps, but on this level I was thinking it
might make sense to allow users to expect any part of the output, not just
the end of the line. This way it could potentially also be used by a
developer for their own interactive script if needed. This was something I
was writing to be generic enough to be used with anything, not just DPDK
apps, so I thought it would make sense to not enforce the restriction and
allow the expected string to be anywhere in the output. However, I see how
enforcing it at the end of the line could make it more clear and consistent.


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

Originally, I tried this and made the stdout buffer a member variable,
similar to the stdin buffer, but the flush method only flushes write
buffers. Because stdout is a read buffer, when you call flush() on it the
method doesn't do anything and the old data in the stdout buffer remains. I
tried to find another way to empty the buffer but I didn't find anything
other than closing it and reopening it as needed.


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

The newline we are adding here is the workaround which does end up adding
two new lines. The first newline (added in send_command_get_output) is to
actually send the command, the second one (added here) is a specific
workaround that forces an additional newline to be entered. What this does
is it causes the current line after the command finishes (in this case, a
"testpmd>" line that is expecting input) to accept an empty string which
does nothing but add the current line into the output buffer. The reason I
left it in the testpmd specific driver is because this work-around only
works for "bash-like" tools that prompt you with the same line again after
accepting an empty string.

I was trying to keep send_command_get_output generic enough to be used on
any interactive environment, not just DPDK apps or "bash-like" environments
and left this work around in the TestpmdDriver because it seemed more
specific to that tool.


>
> > \ 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).
>

This definitely does need to be os-agnostic, but that's a good point about
removing the reliance on the devbind script and might be a good thing to
implement.


>
> > +                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]})',
> > +                        )
> > +
>

[-- Attachment #2: Type: text/html, Size: 21965 bytes --]