From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A991E42B9B; Thu, 25 May 2023 17:41:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8BB5C40EDF; Thu, 25 May 2023 17:41:06 +0200 (CEST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id F06B940DF8 for ; Thu, 25 May 2023 17:41:04 +0200 (CEST) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2538aa25873so1144357a91.2 for ; Thu, 25 May 2023 08:41:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1685029264; x=1687621264; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=QwvbChEXA06eAIW0MMfjN98oNqo2+jSovBgKDb0pc+c=; b=TWj2c6IwUZBTHg7eJUwkmT0kbVZ3qwQZq+RabqCu37Khu9xbXqwrvd8ymRwJE09c85 BTpHlB7IsRRkENdsUqPmvR48szQ0yHqBdja3MWYyhN/K0zUE0xLkLHc4AVXTmnKv0uRz dE8IRD32mcObl/6sQIbbE3sQnJCqHRYAhpzRc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685029264; x=1687621264; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=QwvbChEXA06eAIW0MMfjN98oNqo2+jSovBgKDb0pc+c=; b=Jd6zU7BL0UIOXvIfDNKbGP57SezGGjMwLoR84D5HzRwREGj1hBDsAnNlCbKVlATJTe Gdldh9gzgBElks8eIYKuX+MoD/Vo9EMRm17NP6AJSyW8H0LmKjdr0aA522Y9Ktd2VcxI R1gAdUPtkfgd3FQh7nS746rIHzKdZJoT3wwp58rBEZvmzwkqb8gSxINg8v9pfny3S1QC IHNWTe7ezQ7lqi2YcQj/RmDUCzspRlFusDAab3nttvehvdHFwl7IUsDFCAO500AYsFCf 6joSkoltNpGF3+UeSIr4/pn7+lOHX1ffV3S6H2+KXD2DxyG8EBJTd382OmZri59HeAG1 jaAA== X-Gm-Message-State: AC+VfDxmR9aJAxV6wNsJFkxWJHRLj1opNGgUJohkN598bT8iw1uqxsuq 4igFnEZUjOfT6gESDas+gxgkkGvhQRlyj3THdv+4Z0KH/auVgQxH X-Google-Smtp-Source: ACHHUZ7iPJy9mCj/REfEfO6Uz3mRM8UiPjDutqaDqgaLvUIufvObDfeGKa1/+YUuI7Xkgi4Vki5UZcfMLKY/hlVb2xs= X-Received: by 2002:a17:90a:f996:b0:23f:83de:7e4a with SMTP id cq22-20020a17090af99600b0023f83de7e4amr2426100pjb.7.1685029263925; Thu, 25 May 2023 08:41:03 -0700 (PDT) MIME-Version: 1.0 References: <20230512192540.401-2-jspewock@iol.unh.edu> <20230512192540.401-3-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Thu, 25 May 2023 11:40:52 -0400 Message-ID: Subject: Re: [RFC v2 1/2] dts: add smoke tests To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000b67a7905fc86721e" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000b67a7905fc86721e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, May 24, 2023 at 6:03=E2=80=AFAM Juraj Linke=C5=A1 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 =3D SSHClient() > > + client.set_missing_host_key_policy(AutoAddPolicy) > > + ip: str =3D node_config.hostname > > + logger.info(GREEN(f"Connecting to host {ip}")) > > + #Preset to 22 because paramiko doesn't accept None > > + port: int =3D 22 > > + if ":" in node_config.hostname: > > + ip, port =3D node_config.hostname.split(":") > > + port =3D int(port) > > + client.connect( > > + ip, > > + username=3Dnode_config.user, > > + port=3Dport, > > + password=3Dnode_config.password or "", > > + timeout=3D20 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.l= inkes@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 =3D > SETTINGS.timeout) -> None: > > + self._ssh_client =3D ssh_client > > + self._ssh_channel =3D self._ssh_client.invoke_shell() > > + self._stdin =3D 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-docstrin= gs 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 =3D 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 =3D "" > > + 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 +=3D 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 =3D "testpmd>" > > + interactive_handler: InteractiveScriptHandler > > + > > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str =3D "", cmd_line_options:str =3D "= ") -> > None: > > + """ > > + Sets the handler to drive the SSH session and starts testpmd > > + """ > > + self.interactive_handler =3D 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/d= pdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > > + > > + def send_command(self, command:str, expect:str =3D 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(comman= d > + "\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 > > > > > + 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 =3D > self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../u= sertools/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) !=3D 0, > > + f"Failed to find configured device ({address}) > using dpdk-devbind.py", > > + ) > > + for string in out.stdout.split(" "): > > + if 'drv=3D' in string: > > + self.verify( > > + string.split("=3D")[1] =3D=3D nic.driver.s= trip(), > > + f'Driver for device {address} does not > match driver listed in configuration (bound to {string.split("=3D")[1]})'= , > > + ) > > + > --000000000000b67a7905fc86721e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, May 24, 2023 at 6:03=E2=80=AF= AM Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
I'm sending the second p= art 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/fra= mework/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 @@
>=C2=A0 from .remote_session import CommandResult, RemoteSession
>=C2=A0 from .ssh_session import SSHSession
>
> +from paramiko import SSHClient, AutoAddPolicy
> +from framework.utils import GREEN
>
>=C2=A0 def create_remote_session(
>=C2=A0 =C2=A0 =C2=A0 node_config: NodeConfiguration, name: str, logger:= DTSLOG
>=C2=A0 ) -> RemoteSession:
>=C2=A0 =C2=A0 =C2=A0 return SSHSession(node_config, name, logger)
> +
> +def create_interactive_session(
> +=C2=A0 =C2=A0 node_config: NodeConfiguration, name: str, logger: DTSL= OG
> +) -> SSHClient:
> +=C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 Creates a paramiko SSH session that is designed to be u= sed for interactive shells
> +
> +=C2=A0 =C2=A0 This session is meant to be used on an "as needed&= quot; basis and may never be utilized
> +=C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 client: SSHClient =3D SSHClient()
> +=C2=A0 =C2=A0 client.set_missing_host_key_policy(AutoAddPolicy)
> +=C2=A0 =C2=A0 ip: str =3D node_config.hostname
> +=C2=A0 =C2=A0 logger.info(GREEN(f"Connecting to host {ip}")) > +=C2=A0 =C2=A0 #Preset to 22 because paramiko doesn't accept None<= br> > +=C2=A0 =C2=A0 port: int =3D 22
> +=C2=A0 =C2=A0 if ":" in node_config.hostname:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ip, port =3D node_config.ho= stname.split(":")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 port =3D int(port)
> +=C2=A0 =C2=A0 client.connect(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ip,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 username=3Dnode_config.user,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 port=3Dport,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 password=3Dnode_config.password or "= ",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout=3D20 if port else 10
> +=C2=A0 =C2=A0 )

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@pan= theon.tech/
as possible.

You're right, in my at= tempts to just figure it out and get everything working I neglected=C2=A0to= think about catching the error here, good catch.
=C2=A0

> +=C2=A0 =C2=A0 return client

> diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/t= estbed_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 @@
>=C2=A0 )
>=C2=A0 from .node import Node
>=C2=A0 from .sut_node import SutNode
> +
> +from .interactive_apps import (
> +=C2=A0 =C2=A0 InteractiveScriptHandler,
> +=C2=A0 =C2=A0 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 (
> +=C2=A0 =C2=A0 InteractiveScriptHandler
> +)
> +from .testpmd_driver import (
> +=C2=A0 =C2=A0 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_comma= nd.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:
> +
> +=C2=A0 =C2=A0 _ssh_client: SSHClient
> +=C2=A0 =C2=A0 _stdin: channel.ChannelStdinFile
> +=C2=A0 =C2=A0 _ssh_channel: Channel
> +
> +=C2=A0 =C2=A0 def __init__(self, ssh_client: SSHClient, timeout:float= =3D SETTINGS.timeout) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_client =3D ssh_client
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel =3D self._ssh_client.in= voke_shell()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin =3D self._ssh_channel.makefil= e_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 co= uld likely be removed.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 bel= ieve both would work in theory. I ended up choosing to go with the file-lik= e approach because it seemed simpler while still reaching the same goal. Th= e only difference I could think of between the two is that with file-like b= uffers 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 e= nd if you felt the need to. I believe with recv we would just have to repea= tedly 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 prob= lem.
=C2=A0

> +
> +=C2=A0 =C2=A0 def send_command_get_output(self, command:str, expect:s= tr) -> str:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Send a command and get all output before = the expected ending string.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 **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 somet= hing that would stand out here as that's an important=C2=A0detail.
=C2=A0


> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Lines that expect input are not included = in the stdout buffer so they cannot be
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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<= br> 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.
= =C2=A0

=C2=A0For example, if you were prompted to log into something
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 with a username and password, you cannot = expect "username:" because it wont
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 yet be in the stdout buffer. A work aroun= d for this could be consuming an
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 extra newline character to force the curr= ent prompt into the stdout buffer.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *Return*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 All output before expected = string
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 stdout =3D self._ssh_channel.makefile(&qu= ot;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 bec= ause 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 t= his would mean that stdout would include all of the output that has been co= llected since it has been opened. So, we have to only open stdout when we n= eed 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 t= he input and so it can be reused.=C2=A0

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

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.flush()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out:str =3D ""
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 documen= tation that this should be handled on newline which I would think should ha= ndle different OS's but this is a good thing to lookout for and ensure = does work.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out +=3D 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 tha= t this isn't necessary if you aren't reading in byte mode, I'll= remove it.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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 wou= ld 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 th= eir own interactive script if needed. This was something I was writing to b= e 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.
=C2=A0<= /div>

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 an= d made the stdout buffer a member variable, similar to the stdin buffer, bu= t the flush method only flushes write buffers. Because stdout is a read buf= fer, 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 empt= y the buffer but I didn't find anything other than closing it and reope= ning it as needed.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return out
> +
> +=C2=A0 =C2=A0 def close(self):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.close()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel.close()
> +
> +=C2=A0 =C2=A0 def __del__(self):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.close()
> diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driv= er.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 InteractiveScrip= tHandler
> +
> +from pathlib import PurePath
> +
> +class TestpmdDriver:
> +=C2=A0 =C2=A0 prompt:str =3D "testpmd>"
> +=C2=A0 =C2=A0 interactive_handler: InteractiveScriptHandler
> +
> +=C2=A0 =C2=A0 def __init__(self, handler: InteractiveScriptHandler, d= pdk_build_dir:PurePath, eal_flags:str =3D "", cmd_line_options:st= r =3D "") -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Sets the handler to drive the SSH session= and starts testpmd
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.interactive_handler =3D handler
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # self.interactive_handler.send_command(&= quot;sudo su")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # self.interactive_handler.send_command(&= quot;cd /root/testpmd-testing/dpdk/build")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.interactive_handler.send_command_get= _output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_lin= e_options}\n", self.prompt)
> +
> +=C2=A0 =C2=A0 def send_command(self, command:str, expect:str =3D prom= pt) -> str:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Specific way of handling the command for = testpmd
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 An extra newline character is consumed in= order to force the current line into the stdout buffer
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.interactive_handler.send_comm= and_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 th= e 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 curre= nt 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 t= he testpmd specific driver is because this work-around only works for "= ;bash-like" tools that prompt you with the same line again after accep= ting an empty string.=C2=A0=C2=A0

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

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

<snip>

> +=C2=A0 =C2=A0 def test_device_bound_to_driver(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ensure that all drivers lis= ted in the config are bound to the correct drivers
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for nic in self.sut_node._execution_confi= g.nics:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for address in nic.addresse= s:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out =3D self.= sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../use= rtools/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.
=C2=A0=

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= len(out.stdout) !=3D 0,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= f"Failed to find configured device ({address}) using dpdk-devbind.py&= quot;,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for string in= out.stdout.split(" "):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= if 'drv=3D' in string:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 string.split("=3D")[1] =3D=3D nic.dr= iver.strip(),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 f'Driver for device {address} does not mat= ch driver listed in configuration (bound to {string.split("=3D")[= 1]})',
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 )
> +
--000000000000b67a7905fc86721e--