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 4098A42E53; Wed, 12 Jul 2023 15:46:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C201F40A7D; Wed, 12 Jul 2023 15:46:48 +0200 (CEST) Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) by mails.dpdk.org (Postfix) with ESMTP id 90083406BA for ; Wed, 12 Jul 2023 15:46:47 +0200 (CEST) Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-4fbaef9871cso10938019e87.0 for ; Wed, 12 Jul 2023 06:46:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1689169607; x=1691761607; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=UAXJm3CjY+KOVqw3Q4NF1ca9HwKxYP639YwDxtVVrpo=; b=inWXFoWPCA9DAw0rdrc+1rhK5DSo02+Ss6UXzdOYLzNHbKt4JzK1rDvAxX6hgEH906 Ek3+k+6+GLWnF9Q4P63Vfr1bs4S/6YSP/4BFstcCviuOCN3LO2vwZreHryIAa3gv/sa2 9HhvhlJct2uGEPjZahXKEa/j1p47zPJnUXNIU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689169607; x=1691761607; 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=UAXJm3CjY+KOVqw3Q4NF1ca9HwKxYP639YwDxtVVrpo=; b=AdhgxYJ+yjWzOS5L03V2Wz7uzeF+E9bMGeJXi50VQJRhnTEc5QgEH45OCsN7etvUIB +9cC2wUazctex1YWiBhOfQAE6sMyjsKrHBg5Gj8wiSHMtlanNhbQQER882GRJ9F1StqK uYtOCa6MM3m34/Nx0OYDucp82Bs9skH8AxlO03xwahy9hv12VPPZbaZPcund86ybBmyY bbHDMlV9sTQ3o+eTaahbzYIZr8FbraEuxyP+LSl5xNbC+lo5mBol+lX4bSzGSQ0MrCaV lXnFfb5W/0V/gZ6SUr2isjXu4Vmd+r9QQvHWSxuAYYHS2Xo6JGiliJJLL0qO/0g7kiLD I0qA== X-Gm-Message-State: ABy/qLYR5kwPhh+obhKti9fBnm5tn/VBH7hHhLSPBV3NN48ePZO1YlRw Q1/659ORB5r3FSC2JoQwjlBxEBPyXDsLAwfBNiFkaw== X-Google-Smtp-Source: APBJJlFjVk5ekoPxeZ+T5acyk6sLzkL6gfOExQpQ/CaIj1p5pqBP7Q8aVb3diJQQdVzwvUHWRRCFdHcScqzDeELzTbI= X-Received: by 2002:ac2:4c49:0:b0:4f8:6b7f:c6d6 with SMTP id o9-20020ac24c49000000b004f86b7fc6d6mr19340479lfk.48.1689169606671; Wed, 12 Jul 2023 06:46:46 -0700 (PDT) MIME-Version: 1.0 References: <20230615201318.13359-2-jspewock@iol.unh.edu> <20230615201318.13359-3-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Wed, 12 Jul 2023 09:46:34 -0400 Message-ID: Subject: Re: [PATCH v1 1/2] dts: add smoke tests To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, lijuan.tu@intel.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, dev@dpdk.org Content-Type: multipart/alternative; boundary="0000000000005f14b506004a72c7" 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 --0000000000005f14b506004a72c7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jul 12, 2023 at 2:30=E2=80=AFAM Juraj Linke=C5=A1 wrote: > I think we're basically there, just one more point that needs to be > addressed - the send_command_no_output method. > > >> >> > diff --git a/dts/framework/config/conf_yaml_schema.json > b/dts/framework/config/conf_yaml_schema.json > >> >> > index ca2d4a1e..3f7c301a 100644 > >> >> > --- a/dts/framework/config/conf_yaml_schema.json > >> >> > +++ b/dts/framework/config/conf_yaml_schema.json > >> >> > @@ -6,6 +6,76 @@ > >> >> > "type": "string", > >> >> > "description": "A unique identifier for a node" > >> >> > }, > >> >> > + "NIC": { > >> >> > + "type": "string", > >> >> > + "enum": [ > >> >> > + "ALL", > >> >> > + "ConnectX3_MT4103", > >> >> > + "ConnectX4_LX_MT4117", > >> >> > + "ConnectX4_MT4115", > >> >> > + "ConnectX5_MT4119", > >> >> > + "ConnectX5_MT4121", > >> >> > + "I40E_10G-10G_BASE_T_BC", > >> >> > + "I40E_10G-10G_BASE_T_X722", > >> >> > + "I40E_10G-SFP_X722", > >> >> > + "I40E_10G-SFP_XL710", > >> >> > + "I40E_10G-X722_A0", > >> >> > + "I40E_1G-1G_BASE_T_X722", > >> >> > + "I40E_25G-25G_SFP28", > >> >> > + "I40E_40G-QSFP_A", > >> >> > + "I40E_40G-QSFP_B", > >> >> > + "IAVF-ADAPTIVE_VF", > >> >> > + "IAVF-VF", > >> >> > + "IAVF_10G-X722_VF", > >> >> > + "ICE_100G-E810C_QSFP", > >> >> > + "ICE_25G-E810C_SFP", > >> >> > + "ICE_25G-E810_XXV_SFP", > >> >> > + "IGB-I350_VF", > >> >> > + "IGB_1G-82540EM", > >> >> > + "IGB_1G-82545EM_COPPER", > >> >> > + "IGB_1G-82571EB_COPPER", > >> >> > + "IGB_1G-82574L", > >> >> > + "IGB_1G-82576", > >> >> > + "IGB_1G-82576_QUAD_COPPER", > >> >> > + "IGB_1G-82576_QUAD_COPPER_ET2", > >> >> > + "IGB_1G-82580_COPPER", > >> >> > + "IGB_1G-I210_COPPER", > >> >> > + "IGB_1G-I350_COPPER", > >> >> > + "IGB_1G-I354_SGMII", > >> >> > + "IGB_1G-PCH_LPTLP_I218_LM", > >> >> > + "IGB_1G-PCH_LPTLP_I218_V", > >> >> > + "IGB_1G-PCH_LPT_I217_LM", > >> >> > + "IGB_1G-PCH_LPT_I217_V", > >> >> > + "IGB_2.5G-I354_BACKPLANE_2_5GBPS", > >> >> > + "IGC-I225_LM", > >> >> > + "IGC-I226_LM", > >> >> > + "IXGBE_10G-82599_SFP", > >> >> > + "IXGBE_10G-82599_SFP_SF_QP", > >> >> > + "IXGBE_10G-82599_T3_LOM", > >> >> > + "IXGBE_10G-82599_VF", > >> >> > + "IXGBE_10G-X540T", > >> >> > + "IXGBE_10G-X540_VF", > >> >> > + "IXGBE_10G-X550EM_A_SFP", > >> >> > + "IXGBE_10G-X550EM_X_10G_T", > >> >> > + "IXGBE_10G-X550EM_X_SFP", > >> >> > + "IXGBE_10G-X550EM_X_VF", > >> >> > + "IXGBE_10G-X550T", > >> >> > + "IXGBE_10G-X550_VF", > >> >> > + "brcm_57414", > >> >> > + "brcm_P2100G", > >> >> > + "cavium_0011", > >> >> > + "cavium_a034", > >> >> > + "cavium_a063", > >> >> > + "cavium_a064", > >> >> > + "fastlinq_ql41000", > >> >> > + "fastlinq_ql41000_vf", > >> >> > + "fastlinq_ql45000", > >> >> > + "fastlinq_ql45000_vf", > >> >> > + "hi1822", > >> >> > + "virtio" > >> >> > + ] > >> >> > + }, > >> >> > + > >> >> > >> >> All these NICs may be overkill, do we want to trim them? > >> >> > >> > > >> > > >> > I think in general that the more we have the better to make it more > universally usable. If a NIC isn't supported by DTS anymore we could pull > it out but I don't see a problem with maintaining a list that has all > supported NICs even if it does end up being long. > >> > > >> > >> The broader question is what does it mean that a NIC is supported in > >> DTS? That's a question we should address in the CI/DTS call and in the > >> meantime, we could just leave the list as is. > >> > > > > I think this would be a very good thing to bring up and agree that ther= e > should be more discussion on it. It probably is better to leave the list > longer in the meantime like you were saying as well. > > > > I'm keeping notes on everything we need to talk about - we'll do that > after release. > > > >> > > >> >> > >> >> > > >> >> > """ > >> >> > The package provides modules for managing remote connections to = a > remote host (node), > >> >> > @@ -17,7 +18,14 @@ > >> >> > > >> >> > from .linux_session import LinuxSession > >> >> > from .os_session import OSSession > >> >> > -from .remote import CommandResult, RemoteSession, SSHSession > >> >> > +from .remote import ( > >> >> > + CommandResult, > >> >> > + InteractiveRemoteSession, > >> >> > + InteractiveShell, > >> >> > + RemoteSession, > >> >> > + SSHSession, > >> >> > + TestPmdShell, > >> >> > +) > >> >> > > >> >> > > >> >> > def create_session( > >> >> > diff --git a/dts/framework/remote_session/os_session.py > b/dts/framework/remote_session/os_session.py > >> >> > index 4c48ae25..f5f53923 100644 > >> >> > --- a/dts/framework/remote_session/os_session.py > >> >> > +++ b/dts/framework/remote_session/os_session.py > >> >> > @@ -12,7 +12,13 @@ > >> >> > from framework.testbed_model import LogicalCore > >> >> > from framework.utils import EnvVarsDict, MesonArgs > >> >> > > >> >> > -from .remote import CommandResult, RemoteSession, > create_remote_session > >> >> > +from .remote import ( > >> >> > + CommandResult, > >> >> > + InteractiveRemoteSession, > >> >> > + RemoteSession, > >> >> > + create_interactive_session, > >> >> > + create_remote_session, > >> >> > +) > >> >> > > >> >> > > >> >> > class OSSession(ABC): > >> >> > @@ -26,6 +32,7 @@ class OSSession(ABC): > >> >> > name: str > >> >> > _logger: DTSLOG > >> >> > remote_session: RemoteSession > >> >> > + interactive_session: InteractiveRemoteSession > >> >> > > >> >> > def __init__( > >> >> > self, > >> >> > @@ -37,6 +44,7 @@ def __init__( > >> >> > self.name =3D name > >> >> > self._logger =3D logger > >> >> > self.remote_session =3D create_remote_session(node_confi= g, > name, logger) > >> >> > + self.interactive_session =3D > create_interactive_session(node_config, name, logger) > >> >> > >> >> We may not want to create the interactive session at this point. > This does create a connection to the node which we don't want (it is extr= a > time consumed) when creating an extra session on top of the main_session > (with Node.create_session). I think we could move this to > OSSession.create_interactive_shell. More below. > >> > > >> > > >> > I think the idea of initializing it here was what we had discussed > before about having an open SSH session for interactive shells open in th= e > background throughout the entire run. If I understand what you're saying, > you're suggesting that we would only initialize this connection when we > need it the first time and then leave it in the background afterwards. I > can see how this would be more efficient if you had a run where the > interactive session was never used, but it would add extra logic to make > sure that the connection is only initialized once and that it doesn't > happen every time you create an interactive shell. This is something that > could be done, but considering that this will always be initialized with > smoke tests, the only way you wouldn't have an interactive remote session > created at the start is if you disable smoke tests. I think it is easier = to > just have it running in the background rather than spawn it when it's use= d > and have to worry about if a connection currently exists or not. > >> > >> Right, with smoke tests almost always running, there may not be that > >> much of an advantage in initializing it only when needed. On the other > >> hand, the check could be very simple - the same thing we do with > >> properties such as SutNode.os_name. > >> > > > > I agree that it wouldn't be hard to check if it were defined, I was jus= t > thinking that if we were going to spend the time more often than not > anyway, it would make sense to do it initially so that it doesn't cause a > slow during the test suite and instead during initialization. If you > disagree however, we could easily change this in the future and do it as > needed as I think, in the rare case, you are right that it would be more > efficient, but otherwise it made more sense to me to run it during the > initialization stages of the run. > > > > Yes, it fits in init (we're initializing something after all :-)), but > performance-wise, the property approach is better. Since the > performance consideration is basically negligible, let's leave it as > is. > > > >> >> > + > >> >> > + def empty_stdout_buffer(self) -> None: > >> >> > >> >> Same comment on ordering as above. > >> >> > >> >> > + """Removes all data from the stdout buffer. > >> >> > + > >> >> > + Because of the way paramiko handles read buffers, there > is no way to effectively > >> >> > + remove data from, or "flush", read buffers. This method > essentially moves our > >> >> > + offset on the buffer to the end and thus "removes" the > data from the buffer. > >> >> > + Timeouts are thrown on read operations of paramiko pipes > based on whether data > >> >> > + had been received before timeout so we assume that if we > reach the timeout then > >> >> > + we are at the end of the buffer. > >> >> > + """ > >> >> > + self._ssh_channel.settimeout(1) > >> >> > >> >> Waiting a whole second seems to be a lot. We actually may not need > this method from the use in the code - that is we change how the app star= ts. > >> > > >> > > >> > We will still need this code whenever you send a command and don't > get its output. What it is doing is essentially moving the pointer for th= e > output buffer to the end of the file which is needed because if you send = a > command and you don't want any output, if we don't still move past the > output in the buffer then it will persist and bleed into when you send a > command and do want to collect output. Having this method allows you to > know you are starting from an empty buffer when you are getting the outpu= t > of the commands. > >> > > >> > >> Ah, I was commenting on send_command_no_output when I mentioned "this > >> method", so I need to restate my point. We can do basically the same > >> thing with "send_command" and the only difference I see is that we > >> don't care about prompt in send_command_no_output. Is there a scenario > >> where we need that? > > > > > > This method was to address the situation that I had brought up a while > back when discussing how to handle interactive applications. The scenario > where you want to run an application but you cannot consume a newline > character because the line you are on requires input. In the case of > testpmd and "bash-like" applications, we can consume a newline character > safely but you can't with every interactive environment. The example I us= ed > then was if you ran a script and it asked you to enter a password or a na= me > for something. Consuming a newline in this case might not give you the > prompt again but rather would end up taking in an unintended newline. > > > > Ah, so there are cases where we won't get the prompt back. For now, > these are hypothetical scenarios which si why I'm not keen on having > this method - we may not ever need it. > That's fair, I can remove it for now. > > >> > >> > >> > In the case of timing however, I could cut this down to half a > second, it just gives the chance that a command that takes longer to > produce its output will still contaminate the buffer. If this causes a > problem we could always increase the time in the future. > >> > > >> > >> The point is to not have any set time (since that's either too slow or > >> unreliable), if we can avoid this. > > > > > > I agree that it isn't super reliable, but I think it is good to have > even if it isn't used as often. The reason for this is because if the cas= e > arose where you didn't want to collect output up until a point in the > middle of the stdout string or maybe passed in a prompt that didn't inclu= de > all of the output provided, this offers some way to at least clear the > buffer somehow. > > > > Yea, we need to clear the buffer, I just don't like the solution. :-) > I'd rather remove the method (send_command_no_output) and only include > it when we actually need it. We can then think about the best solution > (possibly tailored to the use case). > I definitely agree with you, it's not a super clean way to clear the buffer and I don't really like it either but after a lot of testing reading from the buffer was the only way I could find to clear the buffer. Doing it with the timeout, while not the most reliable, was the best way I found in my testing but we can explore this more after the release. --0000000000005f14b506004a72c7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jul 12, 2023 at 2:30=E2=80=AFA= M Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
I think we're basically t= here, just one more point that needs to be
addressed - the send_command_no_output method.

>> >> > diff --git a/dts/framework/config/conf_yaml_schema.j= son b/dts/framework/config/conf_yaml_schema.json
>> >> > index ca2d4a1e..3f7c301a 100644
>> >> > --- a/dts/framework/config/conf_yaml_schema.json
>> >> > +++ b/dts/framework/config/conf_yaml_schema.json
>> >> > @@ -6,6 +6,76 @@
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 "type": "s= tring",
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 "description": = "A unique identifier for a node"
>> >> >=C2=A0 =C2=A0 =C2=A0 },
>> >> > +=C2=A0 =C2=A0 "NIC": {
>> >> > +=C2=A0 =C2=A0 =C2=A0 "type": "string= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 "enum": [
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ALL",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ConnectX3_MT4103&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ConnectX4_LX_MT41= 17",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ConnectX4_MT4115&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ConnectX5_MT4119&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ConnectX5_MT4121&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_10G-10G_BASE= _T_BC",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_10G-10G_BASE= _T_X722",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_10G-SFP_X722= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_10G-SFP_XL71= 0",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_10G-X722_A0&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_1G-1G_BASE_T= _X722",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_25G-25G_SFP2= 8",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_40G-QSFP_A&q= uot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "I40E_40G-QSFP_B&q= uot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IAVF-ADAPTIVE_VF&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IAVF-VF", >> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IAVF_10G-X722_VF&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ICE_100G-E810C_QS= FP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ICE_25G-E810C_SFP= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ICE_25G-E810_XXV_= SFP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB-I350_VF"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82540EM&qu= ot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82545EM_CO= PPER",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82571EB_CO= PPER",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82574L&quo= t;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82576"= ;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82576_QUAD= _COPPER",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82576_QUAD= _COPPER_ET2",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-82580_COPP= ER",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-I210_COPPE= R",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-I350_COPPE= R",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-I354_SGMII= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-PCH_LPTLP_= I218_LM",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-PCH_LPTLP_= I218_V",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-PCH_LPT_I2= 17_LM",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_1G-PCH_LPT_I2= 17_V",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGB_2.5G-I354_BAC= KPLANE_2_5GBPS",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGC-I225_LM"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IGC-I226_LM"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-82599_S= FP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-82599_S= FP_SF_QP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-82599_T= 3_LOM",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-82599_V= F",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X540T&q= uot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X540_VF= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550EM_= A_SFP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550EM_= X_10G_T",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550EM_= X_SFP",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550EM_= X_VF",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550T&q= uot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "IXGBE_10G-X550_VF= ",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "brcm_57414",=
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "brcm_P2100G"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "cavium_0011"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "cavium_a034"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "cavium_a063"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "cavium_a064"= ,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "fastlinq_ql41000&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "fastlinq_ql41000_= vf",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "fastlinq_ql45000&= quot;,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "fastlinq_ql45000_= vf",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "hi1822",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "virtio"
>> >> > +=C2=A0 =C2=A0 =C2=A0 ]
>> >> > +=C2=A0 =C2=A0 },
>> >> > +
>> >>
>> >> All these NICs may be overkill, do we want to trim them?<= br> >> >>
>> >
>> >
>> > I think in general that the more we have the better to make i= t more universally usable. If a NIC isn't supported by DTS anymore we c= ould pull it out but I don't see a problem with maintaining a list that= has all supported NICs even if it does end up being long.
>> >
>>
>> The broader question is what does it mean that a NIC is supported = in
>> DTS? That's a question we should address in the CI/DTS call an= d in the
>> meantime, we could just leave the list as is.
>>
>
> I think this would be a very good thing to bring up and agree that the= re should be more discussion on it. It probably is better to leave the list= longer in the meantime like you were saying as well.
>

I'm keeping notes on everything we need to talk about - we'll do th= at
after release.


>> >
>> >>
>> >> >
>> >> >=C2=A0 """
>> >> >=C2=A0 The package provides modules for managing remo= te connections to a remote host (node),
>> >> > @@ -17,7 +18,14 @@
>> >> >
>> >> >=C2=A0 from .linux_session import LinuxSession
>> >> >=C2=A0 from .os_session import OSSession
>> >> > -from .remote import CommandResult, RemoteSession, S= SHSession
>> >> > +from .remote import (
>> >> > +=C2=A0 =C2=A0 CommandResult,
>> >> > +=C2=A0 =C2=A0 InteractiveRemoteSession,
>> >> > +=C2=A0 =C2=A0 InteractiveShell,
>> >> > +=C2=A0 =C2=A0 RemoteSession,
>> >> > +=C2=A0 =C2=A0 SSHSession,
>> >> > +=C2=A0 =C2=A0 TestPmdShell,
>> >> > +)
>> >> >
>> >> >
>> >> >=C2=A0 def create_session(
>> >> > diff --git a/dts/framework/remote_session/os_session= .py b/dts/framework/remote_session/os_session.py
>> >> > index 4c48ae25..f5f53923 100644
>> >> > --- a/dts/framework/remote_session/os_session.py
>> >> > +++ b/dts/framework/remote_session/os_session.py
>> >> > @@ -12,7 +12,13 @@
>> >> >=C2=A0 from framework.testbed_model import LogicalCor= e
>> >> >=C2=A0 from framework.utils import EnvVarsDict, Meson= Args
>> >> >
>> >> > -from .remote import CommandResult, RemoteSession, c= reate_remote_session
>> >> > +from .remote import (
>> >> > +=C2=A0 =C2=A0 CommandResult,
>> >> > +=C2=A0 =C2=A0 InteractiveRemoteSession,
>> >> > +=C2=A0 =C2=A0 RemoteSession,
>> >> > +=C2=A0 =C2=A0 create_interactive_session,
>> >> > +=C2=A0 =C2=A0 create_remote_session,
>> >> > +)
>> >> >
>> >> >
>> >> >=C2=A0 class OSSession(ABC):
>> >> > @@ -26,6 +32,7 @@ class OSSession(ABC):
>> >> >=C2=A0 =C2=A0 =C2=A0 name: str
>> >> >=C2=A0 =C2=A0 =C2=A0 _logger: DTSLOG
>> >> >=C2=A0 =C2=A0 =C2=A0 remote_session: RemoteSession >> >> > +=C2=A0 =C2=A0 interactive_session: InteractiveRemot= eSession
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self,
>> >> > @@ -37,6 +44,7 @@ def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.name =3D name
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger =3D l= ogger
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.remote_sessio= n =3D create_remote_session(node_config, name, logger)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.interactive_sessio= n =3D create_interactive_session(node_config, name, logger)
>> >>
>> >> We may not want to create the interactive session at this= point. This does create a connection to the node which we don't want (= it is extra time consumed) when creating an extra session on top of the mai= n_session (with Node.create_session). I think we could move this to OSSessi= on.create_interactive_shell. More below.
>> >
>> >
>> > I think the idea of initializing it here was what we had disc= ussed before about having an open SSH session for interactive shells open i= n the background throughout the entire run. If I understand what you're= saying, you're suggesting that we would only initialize this connectio= n when we need it the first time and then leave it in the background afterw= ards. I can see how this would be more efficient if you had a run where the= interactive session was never used, but it would add extra logic to make s= ure that the connection is only initialized once and that it doesn't ha= ppen every time you create an interactive shell. This is something that cou= ld be done, but considering that this will always be initialized with smoke= tests, the only way you wouldn't have an interactive remote session cr= eated at the start is if you disable smoke tests. I think it is easier to j= ust have it running in the background rather than spawn it when it's us= ed and have to worry about if a connection currently exists or not.
>>
>> Right, with smoke tests almost always running, there may not be th= at
>> much of an advantage in initializing it only when needed. On the o= ther
>> hand, the check could be very simple - the same thing we do with >> properties such as SutNode.os_name.
>>
>
> I agree that it wouldn't be hard to check if it were defined, I wa= s just thinking that if we were going to spend the time more often than not= anyway, it would make sense to do it initially so that it doesn't caus= e a slow during the test suite and instead during initialization. If you di= sagree however, we could easily change this in the future and do it as need= ed as I think, in the rare case, you are right that it would be more effici= ent, but otherwise it made more sense to me to run it during the initializa= tion stages of the run.
>

Yes, it fits in init (we're initializing something after all :-)), but<= br> performance-wise, the property approach is better. Since the
performance consideration is basically negligible, let's leave it as is.


>> >> > +
>> >> > +=C2=A0 =C2=A0 def empty_stdout_buffer(self) -> N= one:
>> >>
>> >> Same comment on ordering as above.
>> >>
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Remov= es all data from the stdout buffer.
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Because of the way para= miko handles read buffers, there is no way to effectively
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 remove data from, or &q= uot;flush", read buffers. This method essentially moves our
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 offset on the buffer to= the end and thus "removes" the data from the buffer.
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Timeouts are thrown on = read operations of paramiko pipes based on whether data
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 had been received befor= e timeout so we assume that if we reach the timeout then
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 we are at the end of th= e buffer.
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel.setti= meout(1)
>> >>
>> >> Waiting a whole second seems to be a lot. We actually may= not need this method from the use in the code - that is we change how the = app starts.
>> >
>> >
>> > We will still need this code whenever you send a command and = don't get its output. What it is doing is essentially moving the pointe= r for the output buffer to the end of the file which is needed because if y= ou send a command and you don't want any output, if we don't still = move past the output in the buffer then it will persist and bleed into when= you send a command and do want to collect output. Having this method allow= s you to know you are starting from an empty buffer when you are getting th= e output of the commands.
>> >
>>
>> Ah, I was commenting on send_command_no_output when I mentioned &q= uot;this
>> method", so I need to restate my point. We can do basically t= he same
>> thing with "send_command" and the only difference I see = is that we
>> don't care about prompt in send_command_no_output. Is there a = scenario
>> where we need that?
>
>
> This method was to address the situation that I had brought up a while= back when discussing how to handle interactive applications. The scenario = where you want to run an application but you cannot consume a newline chara= cter because the line you are on requires input. In the case of testpmd and= "bash-like" applications, we can consume a newline character saf= ely but you can't with every interactive environment. The example I use= d then was if you ran a script and it asked you to enter a password or a na= me for something. Consuming a newline in this case might not give you the p= rompt again but rather would end up taking in an unintended newline.
>

Ah, so there are cases where we won't get the prompt back. For now,
these are hypothetical scenarios which si why I'm not keen on having this method - we may not ever need it.

=
That= 9;s fair, I can remove it for now.
=C2=A0

>>
>>
>> > In the case of timing however, I could cut this down to half = a second, it just gives the chance that a command that takes longer to prod= uce its output will still contaminate the buffer. If this causes a problem = we could always increase the time in the future.
>> >
>>
>> The point is to not have any set time (since that's either too= slow or
>> unreliable), if we can avoid this.
>
>
> I agree that it isn't super reliable, but I think it is good to ha= ve even if it isn't used as often. The reason for this is because if th= e case arose where you didn't want to collect output up until a point i= n the middle of the stdout string or maybe passed in a prompt that didn'= ;t include all of the output provided, this offers some way to at least cle= ar the buffer somehow.
>

Yea, we need to clear the buffer, I just don't like the solution. :-) I'd rather remove the method (send_command_no_output) and only include<= br> it when we actually need it. We can then think about the best solution
(possibly tailored to the use case).

I definitely a= gree with you, it's not a super clean way to clear the buffer and I don= 't really like it either but after a lot of testing reading from the bu= ffer was the only way I could find to clear the buffer. Doing it with the t= imeout, while not the most reliable, was the best way I found in my testing= but we can explore this more after the release.
--0000000000005f14b506004a72c7--