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 88D674366A; Mon, 4 Dec 2023 10:50:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0D82A40294; Mon, 4 Dec 2023 10:50:55 +0100 (CET) Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mails.dpdk.org (Postfix) with ESMTP id C0AFE4028C for ; Mon, 4 Dec 2023 10:50:52 +0100 (CET) Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-a1b7b6bf098so92504366b.1 for ; Mon, 04 Dec 2023 01:50:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1701683452; x=1702288252; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=0iHawIlp35eQpPQj1U0C2S/57xOhsyaYaL8j9/8FUcA=; b=JZV3Ww4GUpuoJyulTGcWFmHdRtT7qaX5TiTG1Matu4BLepZITqb/6LMBaPTGFHxHec p0WEAuxwrgaxZT4c6W6uTZ/Gn6VrLatoCkQCXXJMkPLJGrMHnN3DI68KA7xdx5yzfXpp 84WLeI//v0ypRlx5+5gkuBkqbDuKQiykhMc/2GLQFtlvntFJyDhSn/FdzPwdWyUIXzlE VPF5imQ+HWMGXqRzduU7tgzHmZv3zT27i+vYuvsEefjaVZMfV+EmAUft63Z5OYmnN3Mx qRiboa3px/HYC8CwRVeG6m3x0lPZLCYw8YfQX3s5jMb/f5XvGB4r/8mEfGoavUo6pKhO Ozgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701683452; x=1702288252; h=content-transfer-encoding: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=0iHawIlp35eQpPQj1U0C2S/57xOhsyaYaL8j9/8FUcA=; b=sr6DpC9wu7ntLIJ8jn5JVZaFN+dFc6PrnHrc7WeNDLe8UfETGSMSaI26eU9SZWyQTD r9gLOY0g7/Al41txYy3ut+SBgfzXLInPCroTBHu/tFiEGWdErlM0/kKZ0kgFuuuZ4G+d u8ydCJoK5aX76pKx8CDrL7pL57OebjMA0FtJuUmuxZ8TZDywvSBnnWF2givPrAPRv+cI dEVE1+CxXo+9KfgL5+KCEbS/3Y8b5zCMj3Qdivt3YrDxcbYHQhglMhFjIpsgq2djJL3J mNhZfXJsJqq5Nd7f811XusCd7rLJCuEdvBVFK19wzKcxuTpRwYvjeXGQcY/V1VGMZ1Pr Pw9Q== X-Gm-Message-State: AOJu0YyubJr3b748V3EgVBuNazf2cske7nutOjspLjwdJepEctcs+4OR uy+ifRh9a4fpZr5TOxQw4XPLG/uqZUv7EZNIbP3SoA== X-Google-Smtp-Source: AGHT+IHth5tGAIr9E8CAS+WMWXYBI6Y4moVg+3UXmJzyku5Pg4/yZwj7agYT4JSxDbsy3Kg0gKpwojFfkk70silm5PQ= X-Received: by 2002:a17:906:a289:b0:a00:ab1a:c81 with SMTP id i9-20020a170906a28900b00a00ab1a0c81mr5610860ejz.22.1701683452431; Mon, 04 Dec 2023 01:50:52 -0800 (PST) MIME-Version: 1.0 References: <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231123151344.162812-1-juraj.linkes@pantheon.tech> <20231123151344.162812-13-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 4 Dec 2023 10:50:41 +0100 Message-ID: Subject: Re: [PATCH v8 12/21] dts: interactive remote session docstring update To: Jeremy Spewock Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, Luca.Vizzarro@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 On Thu, Nov 30, 2023 at 10:50=E2=80=AFPM Jeremy Spewock wrote: > > > > On Thu, Nov 23, 2023 at 10:14=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> Format according to the Google format and PEP257, with slight >> deviations. >> >> Signed-off-by: Juraj Linke=C5=A1 >> --- >> .../interactive_remote_session.py | 36 +++---- >> .../remote_session/interactive_shell.py | 99 +++++++++++-------- >> dts/framework/remote_session/python_shell.py | 26 ++++- >> dts/framework/remote_session/testpmd_shell.py | 58 +++++++++-- >> 4 files changed, 149 insertions(+), 70 deletions(-) >> >> diff --git a/dts/framework/remote_session/interactive_remote_session.py = b/dts/framework/remote_session/interactive_remote_session.py >> index 098ded1bb0..1cc82e3377 100644 >> --- a/dts/framework/remote_session/interactive_remote_session.py >> +++ b/dts/framework/remote_session/interactive_remote_session.py >> @@ -22,27 +22,23 @@ >> class InteractiveRemoteSession: >> """SSH connection dedicated to interactive applications. >> >> - This connection is created using paramiko and is a persistent conne= ction to the >> - host. This class defines methods for connecting to the node and con= figures this >> - connection to send "keep alive" packets every 30 seconds. Because p= aramiko attempts >> - to use SSH keys to establish a connection first, providing a passwo= rd is optional. >> - This session is utilized by InteractiveShells and cannot be interac= ted with >> - directly. >> - >> - Arguments: >> - node_config: Configuration class for the node you are connectin= g to. >> - _logger: Desired logger for this session to use. >> + The connection is created using `paramiko `_ >> + and is a persistent connection to the host. This class defines the = methods for connecting >> + to the node and configures the connection to send "keep alive" pack= ets every 30 seconds. >> + Because paramiko attempts to use SSH keys to establish a connection= first, providing >> + a password is optional. This session is utilized by InteractiveShel= ls >> + and cannot be interacted with directly. >> >> Attributes: >> - hostname: Hostname that will be used to initialize a connection= to the node. >> - ip: A subsection of hostname that removes the port for the conn= ection if there >> + hostname: The hostname that will be used to initialize a connec= tion to the node. >> + ip: A subsection of `hostname` that removes the port for the co= nnection if there >> is one. If there is no port, this will be the same as hostn= ame. >> - port: Port to use for the ssh connection. This will be extracte= d from the >> - hostname if there is a port included, otherwise it will def= ault to 22. >> + port: Port to use for the ssh connection. This will be extracte= d from `hostname` >> + if there is a port included, otherwise it will default to `= `22``. >> username: User to connect to the node with. >> password: Password of the user connecting to the host. This wil= l default to an >> empty string if a password is not provided. >> - session: Underlying paramiko connection. >> + session: The underlying paramiko connection. >> >> Raises: >> SSHConnectionError: There is an error creating the SSH connecti= on. >> @@ -58,9 +54,15 @@ class InteractiveRemoteSession: >> _node_config: NodeConfiguration >> _transport: Transport | None >> >> - def __init__(self, node_config: NodeConfiguration, _logger: DTSLOG)= -> None: >> + def __init__(self, node_config: NodeConfiguration, logger: DTSLOG) = -> None: >> + """Connect to the node during initialization. >> + >> + Args: >> + node_config: The test run configuration of the node to conn= ect to. >> + logger: The logger instance this session will use. >> + """ >> self._node_config =3D node_config >> - self._logger =3D _logger >> + self._logger =3D logger >> self.hostname =3D node_config.hostname >> self.username =3D node_config.user >> self.password =3D node_config.password if node_config.password = else "" >> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/fra= mework/remote_session/interactive_shell.py >> index 4db19fb9b3..b158f963b6 100644 >> --- a/dts/framework/remote_session/interactive_shell.py >> +++ b/dts/framework/remote_session/interactive_shell.py >> @@ -3,18 +3,20 @@ >> >> """Common functionality for interactive shell handling. >> >> -This base class, InteractiveShell, is meant to be extended by other cla= sses that >> -contain functionality specific to that shell type. These derived classe= s will often >> -modify things like the prompt to expect or the arguments to pass into t= he application, >> -but still utilize the same method for sending a command and collecting = output. How >> -this output is handled however is often application specific. If an app= lication needs >> -elevated privileges to start it is expected that the method for gaining= those >> -privileges is provided when initializing the class. >> +The base class, :class:`InteractiveShell`, is meant to be extended by s= ubclasses that contain >> +functionality specific to that shell type. These subclasses will often = modify things like >> +the prompt to expect or the arguments to pass into the application, but= still utilize >> +the same method for sending a command and collecting output. How this o= utput is handled however >> +is often application specific. If an application needs elevated privile= ges to start it is expected >> +that the method for gaining those privileges is provided when initializ= ing the class. >> + >> +The :option:`--timeout` command line argument and the :envvar:`DTS_TIME= OUT` >> +environment variable configure the timeout of getting the output from c= ommand execution. >> """ >> >> from abc import ABC >> from pathlib import PurePath >> -from typing import Callable >> +from typing import Callable, ClassVar >> >> from paramiko import Channel, SSHClient, channel # type: ignore[import= ] >> >> @@ -30,28 +32,6 @@ class InteractiveShell(ABC): >> and collecting input until reaching a certain prompt. All interacti= ve applications >> will use the same SSH connection, but each will create their own ch= annel on that >> session. >> - >> - Arguments: >> - interactive_session: The SSH session dedicated to interactive s= hells. >> - logger: Logger used for displaying information in the console. >> - get_privileged_command: Method for modifying a command to allow= it to use >> - elevated privileges. If this is None, the application will = not be started >> - with elevated privileges. >> - app_args: Command line arguments to be passed to the applicatio= n on startup. >> - timeout: Timeout used for the SSH channel that is dedicated to = this interactive >> - shell. This timeout is for collecting output, so if reading= from the buffer >> - and no output is gathered within the timeout, an exception = is thrown. >> - >> - Attributes >> - _default_prompt: Prompt to expect at the end of output when sen= ding a command. >> - This is often overridden by derived classes. >> - _command_extra_chars: Extra characters to add to the end of eve= ry command >> - before sending them. This is often overridden by derived cl= asses and is >> - most commonly an additional newline character. >> - path: Path to the executable to start the interactive applicati= on. >> - dpdk_app: Whether this application is a DPDK app. If it is, the= build >> - directory for DPDK on the node will be prepended to the pat= h to the >> - executable. >> """ >> >> _interactive_session: SSHClient >> @@ -61,10 +41,22 @@ class InteractiveShell(ABC): >> _logger: DTSLOG >> _timeout: float >> _app_args: str >> - _default_prompt: str =3D "" >> - _command_extra_chars: str =3D "" >> - path: PurePath >> - dpdk_app: bool =3D False >> + >> + #: Prompt to expect at the end of output when sending a command. >> + #: This is often overridden by subclasses. >> + _default_prompt: ClassVar[str] =3D "" >> + >> + #: Extra characters to add to the end of every command >> + #: before sending them. This is often overridden by subclasses and = is >> + #: most commonly an additional newline character. >> + _command_extra_chars: ClassVar[str] =3D "" >> + >> + #: Path to the executable to start the interactive application. >> + path: ClassVar[PurePath] >> + >> + #: Whether this application is a DPDK app. If it is, the build dire= ctory >> + #: for DPDK on the node will be prepended to the path to the execut= able. >> + dpdk_app: ClassVar[bool] =3D False >> >> def __init__( >> self, >> @@ -74,6 +66,19 @@ def __init__( >> app_args: str =3D "", >> timeout: float =3D SETTINGS.timeout, >> ) -> None: >> + """Create an SSH channel during initialization. >> + >> + Args: >> + interactive_session: The SSH session dedicated to interacti= ve shells. >> + logger: The logger instance this session will use. >> + get_privileged_command: A method for modifying a command to= allow it to use >> + elevated privileges. If :data:`None`, the application w= ill not be started >> + with elevated privileges. >> + app_args: The command line arguments to be passed to the ap= plication on startup. >> + timeout: The timeout used for the SSH channel that is dedic= ated to this interactive >> + shell. This timeout is for collecting output, so if rea= ding from the buffer >> + and no output is gathered within the timeout, an except= ion is thrown. >> + """ >> self._interactive_session =3D interactive_session >> self._ssh_channel =3D self._interactive_session.invoke_shell() >> self._stdin =3D self._ssh_channel.makefile_stdin("w") >> @@ -90,6 +95,10 @@ def _start_application(self, get_privileged_command: = Callable[[str], str] | None >> >> This method is often overridden by subclasses as their process = for >> starting may look different. >> + >> + Args: >> + get_privileged_command: A function (but could be any callab= le) that produces >> + the version of the command with elevated privileges. >> """ >> start_command =3D f"{self.path} {self._app_args}" >> if get_privileged_command is not None: >> @@ -97,16 +106,24 @@ def _start_application(self, get_privileged_command= : Callable[[str], str] | None >> self.send_command(start_command) >> >> def send_command(self, command: str, prompt: str | None =3D None) -= > str: >> - """Send a command and get all output before the expected ending= string. >> + """Send `command` and get all output before the expected ending= string. >> >> Lines that expect input are not included in the stdout buffer, = so they cannot >> - be used for expect. For example, if you were prompted to log in= to something >> - with a username and password, you cannot expect "username:" bec= ause it won't >> - yet be in the stdout buffer. A workaround for this could be con= suming an >> - extra newline character to force the current prompt into the st= dout buffer. >> + be used for expect. >> + >> + Example: >> + If you were prompted to log into something with a username = and password, >> + you cannot expect ``username:`` because it won't yet be in = the stdout buffer. >> + A workaround for this could be consuming an extra newline c= haracter to force >> + the current `prompt` into the stdout buffer. >> + >> + Args: >> + command: The command to send. >> + prompt: After sending the command, `send_command` will be e= xpecting this string. >> + If :data:`None`, will use the class's default prompt. >> >> Returns: >> - All output in the buffer before expected string >> + All output in the buffer before expected string. >> """ >> self._logger.info(f"Sending: '{command}'") >> if prompt is None: >> @@ -124,8 +141,10 @@ def send_command(self, command: str, prompt: str | = None =3D None) -> str: >> return out >> >> def close(self) -> None: >> + """Properly free all resources.""" >> self._stdin.close() >> self._ssh_channel.close() >> >> def __del__(self) -> None: >> + """Make sure the session is properly closed before deleting the= object.""" >> self.close() >> diff --git a/dts/framework/remote_session/python_shell.py b/dts/framewor= k/remote_session/python_shell.py >> index cc3ad48a68..ccfd3783e8 100644 >> --- a/dts/framework/remote_session/python_shell.py >> +++ b/dts/framework/remote_session/python_shell.py >> @@ -1,12 +1,32 @@ >> # SPDX-License-Identifier: BSD-3-Clause >> # Copyright(c) 2023 PANTHEON.tech s.r.o. >> >> +"""Python interactive shell. >> + >> +Typical usage example in a TestSuite:: >> + >> + from framework.remote_session import PythonShell >> + python_shell =3D self.tg_node.create_interactive_shell( >> + PythonShell, timeout=3D5, privileged=3DTrue >> + ) >> + python_shell.send_command("print('Hello World')") >> + python_shell.close() >> +""" >> + >> from pathlib import PurePath >> +from typing import ClassVar >> >> from .interactive_shell import InteractiveShell >> >> >> class PythonShell(InteractiveShell): >> - _default_prompt: str =3D ">>>" >> - _command_extra_chars: str =3D "\n" >> - path: PurePath =3D PurePath("python3") >> + """Python interactive shell.""" >> + >> + #: Python's prompt. >> + _default_prompt: ClassVar[str] =3D ">>>" >> + >> + #: This forces the prompt to appear after sending a command. >> + _command_extra_chars: ClassVar[str] =3D "\n" >> + >> + #: The Python executable. >> + path: ClassVar[PurePath] =3D PurePath("python3") >> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewo= rk/remote_session/testpmd_shell.py >> index 08ac311016..79481e845c 100644 >> --- a/dts/framework/remote_session/testpmd_shell.py >> +++ b/dts/framework/remote_session/testpmd_shell.py >> @@ -1,41 +1,79 @@ >> # SPDX-License-Identifier: BSD-3-Clause >> # Copyright(c) 2023 University of New Hampshire >> > > Should you add to the copyright here for adding comments? > I'll add it, as it sounds fine to me (it is a real contribution), but I actually don't know. >> >> +"""Testpmd interactive shell. >> + >> +Typical usage example in a TestSuite:: >> + >> + testpmd_shell =3D self.sut_node.create_interactive_shell( >> + TestPmdShell, privileged=3DTrue >> + ) >> + devices =3D testpmd_shell.get_devices() >> + for device in devices: >> + print(device) >> + testpmd_shell.close() >> +""" >> + >> from pathlib import PurePath >> -from typing import Callable >> +from typing import Callable, ClassVar >> >> from .interactive_shell import InteractiveShell >> >> >> class TestPmdDevice(object): >> + """The data of a device that testpmd can recognize. >> + >> + Attributes: >> + pci_address: The PCI address of the device. >> + """ >> + >> pci_address: str >> >> def __init__(self, pci_address_line: str): >> + """Initialize the device from the testpmd output line string. >> + >> + Args: >> + pci_address_line: A line of testpmd output that contains a = device. >> + """ >> self.pci_address =3D pci_address_line.strip().split(": ")[1].st= rip() >> >> def __str__(self) -> str: >> + """The PCI address captures what the device is.""" >> return self.pci_address >> >> >> class TestPmdShell(InteractiveShell): >> - path: PurePath =3D PurePath("app", "dpdk-testpmd") >> - dpdk_app: bool =3D True >> - _default_prompt: str =3D "testpmd>" >> - _command_extra_chars: str =3D "\n" # We want to append an extra ne= wline to every command >> + """Testpmd interactive shell. >> + >> + The testpmd shell users should never use >> + the :meth:`~.interactive_shell.InteractiveShell.send_command` metho= d directly, but rather >> + call specialized methods. If there isn't one that satisfies a need,= it should be added. >> + """ >> + >> + #: The path to the testpmd executable. >> + path: ClassVar[PurePath] =3D PurePath("app", "dpdk-testpmd") >> + >> + #: Flag this as a DPDK app so that it's clear this is not a system = app and >> + #: needs to be looked in a specific path. >> + dpdk_app: ClassVar[bool] =3D True >> + >> + #: The testpmd's prompt. >> + _default_prompt: ClassVar[str] =3D "testpmd>" >> + >> + #: This forces the prompt to appear after sending a command. >> + _command_extra_chars: ClassVar[str] =3D "\n" >> >> def _start_application(self, get_privileged_command: Callable[[str]= , str] | None) -> None: >> - """See "_start_application" in InteractiveShell.""" >> self._app_args +=3D " -- -i" >> super()._start_application(get_privileged_command) >> >> def get_devices(self) -> list[TestPmdDevice]: >> - """Get a list of device names that are known to testpmd >> + """Get a list of device names that are known to testpmd. >> >> - Uses the device info listed in testpmd and then parses the outp= ut to >> - return only the names of the devices. >> + Uses the device info listed in testpmd and then parses the outp= ut. >> >> Returns: >> - A list of strings representing device names (e.g. 0000:14:0= 0.1) >> + A list of devices. >> """ >> dev_info: str =3D self.send_command("show device info all") >> dev_list: list[TestPmdDevice] =3D [] >> -- >> 2.34.1 >>