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 A316B42E50; Wed, 12 Jul 2023 08:30:14 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7CE6340A7D; Wed, 12 Jul 2023 08:30:14 +0200 (CEST) Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by mails.dpdk.org (Postfix) with ESMTP id EB49F406BA for ; Wed, 12 Jul 2023 08:30:12 +0200 (CEST) Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-51d95aed33aso7954082a12.3 for ; Tue, 11 Jul 2023 23:30:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689143412; x=1691735412; 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=88oyFc/HXOb26q4q/wr9fW8zX6S1BrFG0RLWdDZHZQo=; b=hhN1XrR7sGnhNlh67rYgWPT3wbIsoaKswrvUYcfjmreDd6okdybOkwVaZ1RbuJh7sb 3fldOPbop3MYVMcAOa50c1pWGD+c4ZwmTd7t7T1YaDe63nB6qPaGzZOWkiQg1T/rmhXd p8yatBeHhVWvArJo3HpYDx7eqKTa6fwiV4MTa0Y2UcXWdDZzrKkK0wsFT3W6Xnl9gsrU t1NDGxzg7EfitCzq58vv8o5IVgSe25qTqgJO64ExJ9GiBIVdScT5VQQsogda+aKQDIcL zMNZ/C/LNn+eTizS+uNueLpLOrDhpi0ai7ku2dDAOKkUffxYf/lzZ9SCNTu30lly5v69 yl5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689143412; x=1691735412; 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=88oyFc/HXOb26q4q/wr9fW8zX6S1BrFG0RLWdDZHZQo=; b=TVkkTo4Q0p+em6HcW4ApFWvhUTkf61qAbWFptt5eSLaazT77ArETWqJQipZJenD8gP Pb1Cc410rN0/7fqUcTizduFT59tToF2oHvi2SQTE1NaJ/FNVZFYqyFSOPL4GenotLCQm QFzGq9AK935GR1qyqDDmvBDwq1mPxnHO3v6xya3jAq+rEslWsuBSkXdL1rxIqVh4mCEZ Z0vITArBJP50djcc/0kB5uF4tvXIR+LLsKFoTwyxuoSjaUuo2rcetAkwgwE6t7CjEOn5 35aPMQeFP/rK2gZtGIRp1vpXopRjQJ9+W958T31+c8oawfXZ3wkfeFprRzafLNj8R6NI ioCg== X-Gm-Message-State: ABy/qLbEnwnqFEd5S0k4+aqQKYZYR6Q6PkoLYQY7zD94WS2EyYiKpU8R dpkq2+S7e6aEt3tHyhLzMABUBVOWTrGuto4ivda3kQ== X-Google-Smtp-Source: APBJJlGSdYxylzAJYGva8TqRFpdBuHyIBl3arbe0cnk2mA1ZyNXfR0VQOV0D/5VyhMuNmWPbpKnwXQJQgGcvpjt522w= X-Received: by 2002:a05:6402:b2e:b0:516:9fef:f8e7 with SMTP id bo14-20020a0564020b2e00b005169feff8e7mr15432711edb.3.1689143412464; Tue, 11 Jul 2023 23:30:12 -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: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 12 Jul 2023 08:30:01 +0200 Message-ID: Subject: Re: [PATCH v1 1/2] dts: add smoke tests To: Jeremy Spewock 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: 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 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/frame= work/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 un= iversally 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 supporte= d 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 there = should be more discussion on it. It probably is better to leave the list lo= nger 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/frame= work/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_se= ssion >> >> > +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_config,= name, logger) >> >> > + self.interactive_session =3D create_interactive_session(no= de_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 main_session (with = Node.create_session). I think we could move this to OSSession.create_intera= ctive_shell. More below. >> > >> > >> > I think the idea of initializing it here was what we had discussed bef= ore about having an open SSH session for interactive shells open in the bac= kground 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 h= ow this would be more efficient if you had a run where the interactive sess= ion was never used, but it would add extra logic to make sure that the conn= ection is only initialized once and that it doesn't happen every time you c= reate an interactive shell. This is something that could be done, but consi= dering 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 th= e background rather than spawn it when it's used 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 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 cause a slow du= ring the test suite and instead during initialization. If you disagree howe= ver, we could easily change this in the future and do it as needed as I thi= nk, in the rare case, you are right that it would be more efficient, but ot= herwise 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 es= sentially moves our >> >> > + offset on the buffer to the end and thus "removes" the dat= a from the buffer. >> >> > + Timeouts are thrown on read operations of paramiko pipes b= ased on whether data >> >> > + had been received before timeout so we assume that if we r= each 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 th= is 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 pointer for the out= put buffer to the end of the file which is needed because if you send a com= mand 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 a= nd do want to collect output. Having this method allows you to know you are= starting from an empty buffer when you are getting the output of the comma= nds. >> > >> >> 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 ba= ck when discussing how to handle interactive applications. The scenario whe= re you want to run an application but you cannot consume a newline characte= r because the line you are on requires input. In the case of testpmd and "b= ash-like" applications, we can consume a newline character safely but you c= an't with every interactive environment. The example I used then was if you= ran a script and it asked you to enter a password or a name for something.= Consuming a newline in this case might not give you the prompt again but r= ather 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. >> >> >> > 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 o= utput 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 case aros= e 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 include all of t= he output provided, this offers some way to at least clear the buffer someh= ow. > 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).