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 B537F41F66; Mon, 10 Jun 2024 21:27:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 39236402CA; Mon, 10 Jun 2024 21:27:26 +0200 (CEST) Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mails.dpdk.org (Postfix) with ESMTP id 92A8C402A9 for ; Mon, 10 Jun 2024 21:27:24 +0200 (CEST) Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-2c2eb98a64fso1704814a91.2 for ; Mon, 10 Jun 2024 12:27:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718047643; x=1718652443; 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=OLR5EVhPxvwHR0UocREt+4cjFYnmBUSDHEr+QczgQMM=; b=P7YKANzXNxB6U7TwXXeEyWuTRNH/6UM6YC33/oJkzeB4fIdOH0GjKKF8Ea7v5vHSww A/V0TlF3ELl7UzJaz8BcA+J11LL/AoA6VIpRAPz1Q3mtXI2VicD4Wyxu592se77v2DcT eHNYPONXEUxVKLtA6O6JMmESoElS0i4qjDKjw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718047643; x=1718652443; 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=OLR5EVhPxvwHR0UocREt+4cjFYnmBUSDHEr+QczgQMM=; b=F0Omx5NKWONSnO+MGJgJVSVCNkk0Qhz+OiedRFdRxqgXhLCygE4F0W/4eDjwsLqwp6 IU9/tp7IwxIbGqXpAn2i7udolhNOEFW/qvIiAiBtV3V7LbwOG7OpwcIB3zLkI7CO2hPI 2ZROWQTtGfdDNoWFkg6RcWS90JUsNgDMAibKcjW4vkMZ+i1c+UM1lO9P/bwyxAXYfDtd f0Ynrtkua3GUDc5PVXpO9Ky7Fnl8i4qE1U08RnDV7SaWjihspuPCenjFyk5D95Tvf7qf Ga/8ITZDGHQeVXb1nCvMS6YdS4n7TUEeeQsIugAXXa5LdOFfRvE3LAviR3lHy5hurS3g ebDA== X-Forwarded-Encrypted: i=1; AJvYcCVLa4ewa+EWMBQQ5pGgr44Q3O+EILTyUS7qVbjMjafmyWlqbx+ULz25oEGgZM+IhJ30LeioknKhb/AOND0= X-Gm-Message-State: AOJu0YwkGqBquy583aycG3FxZLKB/uA3YT8OMBOKtMI8Me+bJlobB1j2 QG/Eqea3i5r8mdNSqL9e5kBw+wGHAxYKgopXAurgbPMM0FjQkdFCwt8fW1oLlxwuO+zq1QcCpNF m3yLyJbW9PcivCVwSscwC5HLnnKz54QHLJzYwMA== X-Google-Smtp-Source: AGHT+IFc9BH2gr8U1C3bpuECzbpOgqvfXInwe0BsGaOCBdPBiv+rm8r+lK4qhrswwOwrYC2bb0fAvJVIKvzrNBQQCtc= X-Received: by 2002:a17:90b:1049:b0:2c2:cce9:2578 with SMTP id 98e67ed59e1d1-2c2cce92bd7mr7697527a91.17.1718047643578; Mon, 10 Jun 2024 12:27:23 -0700 (PDT) MIME-Version: 1.0 References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240605213148.21371-1-jspewock@iol.unh.edu> <20240605213148.21371-2-jspewock@iol.unh.edu> <3555c2df-e2fc-417f-b999-845e9415ef47@pantheon.tech> In-Reply-To: <3555c2df-e2fc-417f-b999-845e9415ef47@pantheon.tech> From: Jeremy Spewock Date: Mon, 10 Jun 2024 15:27:11 -0400 Message-ID: Subject: Re: [PATCH v3 1/4] dts: improve starting and stopping interactive shells To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, npratte@iol.unh.edu, 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 Mon, Jun 10, 2024 at 9:36=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/fr= amework/remote_session/interactive_shell.py > > index 5cfe202e15..921c73d9df 100644 > > --- a/dts/framework/remote_session/interactive_shell.py > > +++ b/dts/framework/remote_session/interactive_shell.py > > @@ -32,6 +34,10 @@ class InteractiveShell(ABC): > > and collecting input until reaching a certain prompt. All interac= tive applications > > will use the same SSH connection, but each will create their own = channel on that > > session. > > + > > + Attributes: > > + is_started: :data:`True` if the application has started succes= sfully, :data:`False` > > + otherwise. > > """ > > > > _interactive_session: SSHClient > > @@ -41,6 +47,7 @@ class InteractiveShell(ABC): > > _logger: DTSLogger > > _timeout: float > > _app_args: str > > + _finalizer: weakref.finalize > > > > #: Prompt to expect at the end of output when sending a command. > > #: This is often overridden by subclasses. > > @@ -58,6 +65,8 @@ class InteractiveShell(ABC): > > #: for DPDK on the node will be prepended to the path to the exec= utable. > > dpdk_app: ClassVar[bool] =3D False > > > > + is_started: bool =3D False > > A better name would be is_alive to unify it with SSHSession. Ack. > > > + > > def __init__( > > self, > > interactive_session: SSHClient, > > @@ -93,17 +102,39 @@ def __init__( > > def _start_application(self, get_privileged_command: Callable[[st= r], str] | None) -> None: > > """Starts a new interactive application based on the path to = the app. > > > > - This method is often overridden by subclasses as their process= for > > - starting may look different. > > + This method is often overridden by subclasses as their process= for starting may look > > + different. Initialization of the shell on the host can be retr= ied up to 5 times. This is > > + done because some DPDK applications need slightly more time af= ter exiting their script to > > + clean up EAL before others can start. > > + > > + When the application is started we also bind a class for final= ization to this instance of > > + the shell to ensure proper cleanup of the application. > > Let's also include the explanation from the commit message. Ack. > > > > > Args: > > get_privileged_command: A function (but could be any call= able) that produces > > the version of the command with elevated privileges. > > """ > > + self._finalizer =3D weakref.finalize(self, self._close) > > This looks like exactly what we should do, but out of curiosity, do > Paramiko docs mention how we should handle channel closing? They don't say much about how to properly handle closing them. They do mention though that the channels are automatically closed when their transport is closed, or when they are garbage collected. I guess the likely reason then for why they don't say how to handle closing them is because regardless of what you do they will still class `close()` at garbage collection. > > > + max_retries =3D 5 > > + self._ssh_channel.settimeout(5) > > start_command =3D f"{self.path} {self._app_args}" > > if get_privileged_command is not None: > > start_command =3D get_privileged_command(start_command) > > - self.send_command(start_command) > > + self.is_started =3D True > > + for retry in range(max_retries): > > + try: > > + self.send_command(start_command) > > + break > > + except TimeoutError: > > + self._logger.info( > > + "Interactive shell failed to start, retrying... " > > + f"({retry+1} out of {max_retries})" > > + ) > > + else: > > + self._ssh_channel.settimeout(self._timeout) > > + self.is_started =3D False # update state on failure to st= art > > + raise InteractiveCommandExecutionError("Failed to start ap= plication.") > > + self._ssh_channel.settimeout(self._timeout) > > > > def send_command(self, command: str, prompt: str | None =3D None)= -> str: > > """Send `command` and get all output before the expected endi= ng string.