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 1BA5942B8C; Wed, 24 May 2023 12:03:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A56D4406BC; Wed, 24 May 2023 12:03:49 +0200 (CEST) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by mails.dpdk.org (Postfix) with ESMTP id 8C6FF4067E for ; Wed, 24 May 2023 12:03:48 +0200 (CEST) Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-96aae59bbd6so112508466b.3 for ; Wed, 24 May 2023 03:03:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon-tech.20221208.gappssmtp.com; s=20221208; t=1684922628; x=1687514628; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Gi9Qs7JLTc3DfRZoseE0I6ndKKX2R3W7SwmiTUQ+AL8=; b=b6nAD0+ff+PdcZ6srSUq+Y+HPsZ335hB/92/GqHMVbgecqjm8CfhD24b7IGbsQ2B+j poYqoB2QiFMw2yoQrtJHBSgNbvLRlluxZuBSN+EtQfk6K8S0350i7onLlX0hI75rFusd SMFRxhxkbfQDZcMtcrMCtxZ8CgXghYnSOIxOcH2qlh++8+29mlGbb7X1N9L3w8aNhOgv q5C4Ps8hDCIPzDYpNNp5kpPcAbMCGWIbXOsObbG7fAEGlBxye4Hclt+RhznSKm0965xb Ro8nZ9/jYvBuE/wq4jse9CVMHxZgbCxLFdqGDvEmzLdS+6qTin3bNrFZuzrgqvN9Q9r/ R46Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684922628; x=1687514628; 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=Gi9Qs7JLTc3DfRZoseE0I6ndKKX2R3W7SwmiTUQ+AL8=; b=G88H/mme33Ac5qbgruxWB7/43dMFMVFyjEKzVOu4RY8s0MbB9jIZWSew+bsRYTCDrI bN7hMz149991G8Deg9lu8D6QjjdWQWXrX06quDLeokhjkECbCQ2YGb0KgnmrvIwpTfx7 zflxPijvMpEaFsbLhZkv7yolFcUVjMHy7woJaPIwnESFsunqUUomkx3YO54nY45QYQPn PfSQ5YSCdePpNjC7PjvA0soBuKoaGO+ZT7q8hbKj1PU3MbzOHAGAJ+LKbaFo3khkjPd0 ugYbR2Z9gQf2ewLORnT/eSKaZLtIGylsdChjVXQJxFB/Bsae1cF2xv+Oayz8AB3kxvdm cjnQ== X-Gm-Message-State: AC+VfDwHAAu8e2tS/VTuTqd7h8LoEwh7pXNdEMxDn/kTHm18Y1lFmf4I hsqFO731nsa6HlFMUPrQq7SkvI8tMcf64SFPAIYyh+Wn4UeQHfPb2+c= X-Google-Smtp-Source: ACHHUZ7tXEzfZ22PrLLixibXyeGhlyNTqOuatW36sUkKOQVXqZmtKpg+rc8sIGM205nSD2UoYwCBd5x4KPiCdt9J5F4= X-Received: by 2002:a17:906:dacb:b0:94a:7979:41f5 with SMTP id xi11-20020a170906dacb00b0094a797941f5mr15812587ejb.71.1684922627987; Wed, 24 May 2023 03:03:47 -0700 (PDT) MIME-Version: 1.0 References: <20230512192540.401-2-jspewock@iol.unh.edu> <20230512192540.401-3-jspewock@iol.unh.edu> In-Reply-To: <20230512192540.401-3-jspewock@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 24 May 2023 12:03:37 +0200 Message-ID: Subject: Re: [RFC v2 1/2] dts: add smoke tests To: jspewock@iol.unh.edu Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" 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 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 > + 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]})', > + ) > +