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