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 8F34942BA7; Fri, 26 May 2023 09:36:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6827940DDA; Fri, 26 May 2023 09:36:59 +0200 (CEST) Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) by mails.dpdk.org (Postfix) with ESMTP id 3DB1A40A89 for ; Fri, 26 May 2023 09:36:57 +0200 (CEST) Received: by mail-ed1-f46.google.com with SMTP id 4fb4d7f45d1cf-51478f6106cso228523a12.1 for ; Fri, 26 May 2023 00:36:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon-tech.20221208.gappssmtp.com; s=20221208; t=1685086617; x=1687678617; 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=BBZ5DVChLXlxFYQwirrfyt1fPggfSPkDB9trTYQaArI=; b=FoA3ydxYJLevjhgPRSMEAa0Mtia7uDUnaVvZ41inxyJ01Y0XUX8F6I5HYG5ZuMd5iW Pgykk9vGvlguk6Cu2+JaWBFhleWbo4l/5yafR4ezjhCweQxGy2VkPod0B7xfZqY/esR3 pvU8EhZr8XcnCR6cCxtdZKKj8SFKg65C7sB/6ThIas61mnMQko29k9P/TU4PJr38u9ep 7fsI/IiJYTAuncz5ZzFrAW8Hp/sOvwJ/4crW+8je8K7TRss50iDAVCtv2yb2r5D7sdFN K3e+RE3yZPLYFJOChbCGHfcXvTapYafNInmQIZ5kx9O2aHqD0p41BBGk5OC4E09fLfIM wVqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685086617; x=1687678617; 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=BBZ5DVChLXlxFYQwirrfyt1fPggfSPkDB9trTYQaArI=; b=UrkDvWvAkeqFisMO8dh8KanMX62TrSqEweHJIwPFNxOiPsnmbJT1S4c4J4e/fnPviK yiLUvhg3dw0aag0ea5rohnPozF6z1RNGrKaJSIDc84bGjQ1HLn1bfID8vw6etCUIcQof 81BxRrkUBk6VBTmY4KyVO5NcNpi0Kp0+EL2VhtgiSR9+zkPXaDMrJCunMeX5DEdpLWV4 HLzzESEUxW0V5OU10b59H4aSz1Ke+X6dsY9Iv5RSVK6BYV9qrdWapZjK+B6yGopNaZmU ptnXZUur/dIU5v9We+3lEMAXKDVxzBlx1lEZF84AkbzRDwSXXOMO108vj+onz1zE1WaS TBbg== X-Gm-Message-State: AC+VfDxJeJIX5Zo8Z6rKw1E6ALXoo3hHnEqYVCSZKhgPjHrlE5++KYEk lR6VgDq/GiQoyEiy9JwjAeR6r3Zv1ope5b2uF6sxFA== X-Google-Smtp-Source: ACHHUZ6IDdHKdMfUuuDHSrw0cZx31NxBS13S0UJoXPZlSqL51j4ujTJEz/O86IDtib1BcM6CKwpG5FMdnER5/ytjP2Q= X-Received: by 2002:a17:907:960f:b0:96f:d8a9:d045 with SMTP id gb15-20020a170907960f00b0096fd8a9d045mr945472ejc.59.1685086616210; Fri, 26 May 2023 00:36:56 -0700 (PDT) MIME-Version: 1.0 References: <20230512192540.401-2-jspewock@iol.unh.edu> <20230512192540.401-3-jspewock@iol.unh.edu> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Fri, 26 May 2023 09:36:44 +0200 Message-ID: Subject: Re: [RFC v2 1/2] dts: add smoke tests To: Jeremy Spewock Cc: 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, May 25, 2023 at 8:03=E2=80=AFPM Jeremy Spewock wrote: > > Hey Juraj, > > On Thu, May 25, 2023 at 4:33=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> One more point that doesn't fit elsewhere: >> >> On Wed, May 24, 2023 at 10:45=E2=80=AFPM Jeremy Spewock wrote: >> > >> > >> > >> > On Tue, May 23, 2023 at 4:05=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> >> >> Hi Jeremy, first, a few general points: >> >> >> >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and >> >> anyone else involved with DTS or who might be interested) and add the >> >> devlist to cc. >> > >> > >> > Thank you for the tip! I'm still new to sending patches and didn't thi= nk to do something like this but I will in the future. >> > >> >> >> >> 2. Run the linter script before submitting. >> > >> > >> > I did forget to run this, I will in the future. >> > >> >> >> >> 3. The use of the various nested objects breaks the current >> >> abstractions. The basic idea is that the test suite developers should >> >> ideally only use the sut/tg node objects and those objects should >> >> delegate logic further to their nested objects. More below. >> >> >> >> I have many comments about the implementation, but I haven't run it >> >> yet. I'm going to do that after this round of comments and I may have >> >> more ideas. >> >> >> >> On Fri, May 12, 2023 at 9:28=E2=80=AFPM wrote: >> >> > >> >> > From: Jeremy Spewock >> >> > >> >> > Adds a new test suite for running smoke tests that verify general >> >> > configuration aspects of the system under test. If any of these tes= ts >> >> > fail, the DTS execution terminates as part of a "fail-fast" model. >> >> > >> >> > Signed-off-by: Jeremy Spewock >> >> > --- >> >> > dts/conf.yaml | 9 ++ >> >> > dts/framework/config/__init__.py | 21 +++++ >> >> > dts/framework/config/conf_yaml_schema.json | 32 ++++++- >> >> > dts/framework/dts.py | 19 +++- >> >> > dts/framework/exception.py | 11 +++ >> >> > dts/framework/remote_session/os_session.py | 6 +- >> >> > .../remote_session/remote/__init__.py | 28 ++++++ >> >> > dts/framework/test_result.py | 13 ++- >> >> > dts/framework/test_suite.py | 24 ++++- >> >> > dts/framework/testbed_model/__init__.py | 5 + >> >> > .../interactive_apps/__init__.py | 6 ++ >> >> > .../interactive_apps/interactive_command.py | 57 +++++++++++ >> >> > .../interactive_apps/testpmd_driver.py | 24 +++++ >> >> > dts/framework/testbed_model/node.py | 2 + >> >> > dts/framework/testbed_model/sut_node.py | 6 ++ >> >> > dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++= ++++ >> >> > 16 files changed, 348 insertions(+), 9 deletions(-) >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/__= init__.py >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/in= teractive_command.py >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/te= stpmd_driver.py >> >> >> >> Let's not add any more levels. I don't like even the current hw >> >> subdirectory (which I want to remove in the docs patch) and we don't >> >> need it. I'd also like to move this functionality into remote_session= , >> >> as it's handling a type of remote session. >> > >> > >> > I think it makes sense to add a proper wrapper for it, I just didn't c= reate a subclass for it off of remote_session because the idea behind it wa= s only allowing users to interact with the session through the InteractiveS= criptHandler/DPDK app handlers. If it were part of the remote_session class= it would have to include operations for sending commands the way it is wri= tten now. I could do this but it seems like a bigger discussion about wheth= er interactive sessions should create a new SSH session every time or inste= ad just use one existing session and create channels off of it. >> > >> >> I wasn't clear, I meant to move the python modules into the >> remote_session folder. The subclassing would be there only to have a >> common API across these remote sessions (as the >> InteractiveScriptHandler is a kind of a remote session). If at all >> possible, this should be our aim. > > > I see what you mean now, moving them there shouldn't be a problem. > >> >> >> >> >> >> >> >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py >> >> > >> >> > diff --git a/dts/conf.yaml b/dts/conf.yaml >> >> > index a9bd8a3e..042ef954 100644 >> >> > --- a/dts/conf.yaml >> >> > +++ b/dts/conf.yaml >> >> > @@ -10,13 +10,22 @@ executions: >> >> > compiler_wrapper: ccache >> >> > perf: false >> >> > func: true >> >> > + nics: #physical devices to be used for testing >> >> > + - addresses: >> >> > + - "0000:11:00.0" >> >> > + - "0000:11:00.1" >> >> > + driver: "i40e" >> >> > + vdevs: #names of virtual devices to be used for testing >> >> > + - "crypto_openssl" >> >> >> >> I believe we specified the NICs under SUTs in the original DTS, just >> >> as Owen did in his internal GitLab patch. If you can access it, have = a >> >> look at how he did it. >> >> This brings an interesting question of where we want to specify which >> >> NICs/vdevs to test. It could be on the SUT level, but also on the >> >> execution or even the build target level. This should be informed by >> >> testing needs. What makes the most sense? We could specify NIC detail= s >> >> per SUT/TG and then just reference the NICs on the execution/build >> >> target level. >> > >> > >> > I put it on the execution level with the thought that you might have m= ultiple NICs in your SUT but want to separate them into different execution= s. >> >> This is a good point. Is this something you're interested in in the >> lab? We should ask Lijuan what she thinks of this. In general, I like >> this, so we should at least think of how to add NIC config >> implementation so that we could add this later if we're not adding it >> now. >> >> > I guess in the case of smoke tests, you'd only need to know them per t= arget because it was talked about previously that then is when we should ru= n the smoke tests. I think however it would make sense to specify the NIC y= ou are using for that execution rather than having to assume or specify els= ewhere. >> > >> >> Nothing is set in stone, we don't have to run them per build target. >> We could have two smoke test suites, one per execution and one per >> build target. I actually like that a lot and we should explore that. >> If it's not much extra work, we could just do the split like that. >> > > I also like the sound of that. In theory it doesn't seem that hard, if we= made two different suites that tested different things all that needs to c= hange is when they are run. This would probably take some more planning as = to what we want to run at which points during the run and it might be easie= r to get these tests in a better spot first. I do like the idea though, and= agree that it's worth exploring. > >> >> >> >> >> >> > test_suites: >> >> > + - smoke_tests >> >> > - hello_world >> >> > system_under_test: "SUT 1" >> >> > nodes: >> >> > - name: "SUT 1" >> >> > hostname: sut1.change.me.localhost >> >> > user: root >> >> > + password: "" >> >> >> >> This was deliberately left out to discourage the use of passwords. >> >> >> >> > arch: x86_64 >> >> > os: linux >> >> > lcores: "" >> >> >> >> >> >> >> >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py >> >> > index 05022845..0d03e158 100644 >> >> > --- a/dts/framework/dts.py >> >> > +++ b/dts/framework/dts.py >> >> > @@ -5,6 +5,8 @@ >> >> > >> >> > import sys >> >> > >> >> > +from .exception import BlockingTestSuiteError >> >> > + >> >> > from .config import CONFIGURATION, BuildTargetConfiguration, Execu= tionConfiguration >> >> > from .logger import DTSLOG, getLogger >> >> > from .test_result import BuildTargetResult, DTSResult, ExecutionRe= sult, Result >> >> > @@ -49,6 +51,7 @@ def run_all() -> None: >> >> > nodes[sut_node.name] =3D sut_node >> >> > >> >> > if sut_node: >> >> > + #SMOKE TEST EXECUTION GOES HERE! >> >> > _run_execution(sut_node, execution, result) >> >> > >> >> > except Exception as e: >> >> > @@ -118,7 +121,7 @@ def _run_build_target( >> >> > >> >> > try: >> >> > sut_node.set_up_build_target(build_target) >> >> > - result.dpdk_version =3D sut_node.dpdk_version >> >> > + # result.dpdk_version =3D sut_node.dpdk_version >> >> > build_target_result.update_setup(Result.PASS) >> >> > except Exception as e: >> >> > dts_logger.exception("Build target setup failed.") >> >> > @@ -146,6 +149,7 @@ def _run_suites( >> >> > with possibly only a subset of test cases. >> >> > If no subset is specified, run all test cases. >> >> > """ >> >> > + end_execution =3D False >> >> >> >> This only ends the build target stage, not the execution stage. We >> >> should either find a better name for the variable or make sure that >> >> the whole execution is blocked. I think we're aiming for the latter - >> >> maybe we could just check whether the last exception in result is a >> >> BlockingTestSuiteError (or better, just call a new method of result >> >> that will check that), which we could do at multiple stages. >> > >> > >> > Good catch. When writing this and figuring out how to get it to work, = I was thinking about smoke tests on a per build target basis because if it = failed on one build target it could theoretically pass on another, but you'= d likely want your entire execution to fail than have partial results. >> > >> >> Well, these will be well defined partial results (and definitely >> valuable, imagine one build target passing and one failing, we could >> easily compare the two to sometimes quickly identify the failure), so >> if there's a possibility that the test would pass on another build >> target, we should try to run the build target. >> >> In this case, let's just rename the variable to end_build_target or >> something similar. >> > > Alright, will do. > >> >> >> >> >> >> >> > for test_suite_config in execution.test_suites: >> >> > try: >> >> > full_suite_path =3D f"tests.TestSuite_{test_suite_conf= ig.test_suite}" >> >> > @@ -160,13 +164,24 @@ def _run_suites( >> >> > >> >> > else: >> >> > for test_suite_class in test_suite_classes: >> >> > + #HERE NEEDS CHANGING >> >> > test_suite =3D test_suite_class( >> >> > sut_node, >> >> > test_suite_config.test_cases, >> >> > execution.func, >> >> > build_target_result, >> >> > + sut_node._build_target_config, >> >> > + result >> >> > ) >> >> > - test_suite.run() >> >> > + try: >> >> > + test_suite.run() >> >> > + except BlockingTestSuiteError as e: >> >> > + dts_logger.exception("An error occurred within= a blocking TestSuite, execution will now end.") >> >> > + result.add_error(e) >> >> > + end_execution =3D True >> >> > + #if a blocking test failed and we need to bail out of suit= e executions >> >> > + if end_execution: >> >> > + break >> >> > >> >> > >> >> > def _exit_dts() -> None: >> >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.p= y >> >> > index ca353d98..4e3f63d1 100644 >> >> > --- a/dts/framework/exception.py >> >> > +++ b/dts/framework/exception.py >> >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): >> >> > SSH_ERR =3D 4 >> >> > DPDK_BUILD_ERR =3D 10 >> >> > TESTCASE_VERIFY_ERR =3D 20 >> >> > + BLOCKING_TESTSUITE_ERR =3D 25 >> >> > >> >> > >> >> > class DTSError(Exception): >> >> > @@ -144,3 +145,13 @@ def __init__(self, value: str): >> >> > >> >> > def __str__(self) -> str: >> >> > return repr(self.value) >> >> > + >> >> > +class BlockingTestSuiteError(DTSError): >> >> > + suite_name: str >> >> > + severity: ClassVar[ErrorSeverity] =3D ErrorSeverity.BLOCKING_= TESTSUITE_ERR >> >> > + >> >> > + def __init__(self, suite_name:str) -> None: >> >> > + self.suite_name =3D suite_name >> >> > + >> >> > + def __str__(self) -> str: >> >> > + return f"Blocking suite {self.suite_name} failed." >> >> > diff --git a/dts/framework/remote_session/os_session.py b/dts/frame= work/remote_session/os_session.py >> >> > index 4c48ae25..22776bc1 100644 >> >> > --- a/dts/framework/remote_session/os_session.py >> >> > +++ b/dts/framework/remote_session/os_session.py >> >> > @@ -12,7 +12,9 @@ >> >> > 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, RemoteSession, create_remote_se= ssion, create_interactive_session >> >> > + >> >> > +from paramiko import SSHClient >> >> > >> >> > >> >> > class OSSession(ABC): >> >> > @@ -26,6 +28,7 @@ class OSSession(ABC): >> >> > name: str >> >> > _logger: DTSLOG >> >> > remote_session: RemoteSession >> >> > + _interactive_session: SSHClient >> >> > >> >> > def __init__( >> >> > self, >> >> > @@ -37,6 +40,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(n= ode_config, name, logger) >> >> >> >> This session should be stored in SutNode (it should be os-agnostic >> >> (the apps should behave the same on all os's, right?) and SutNode >> >> could use it without accessing another object), but created in >> >> OSSession (this way we make sure any os-specific inputs (such as >> >> paths) are passed properly). >> > >> > >> > I can move it into SutNode, the main reason I left it there is because= I was essentially thinking of it the same way as the remote session that g= ets created once per node. However, I could just as easily have SutNode sto= re it and call upon OSSession to create one inside its constructor. This wo= uld make creating classes for DPDK apps easier as well once those are subcl= asses of the InteractiveScriptHandler. >> > >> >> I was thinking of this interactive session more like >> Node._other_sessions, created only when needed. Now that I think about >> it, it's always going to be needed if we use it in smoke tests, so we >> might as well create it in OSSession (or its subclasses if needed). >> >> Thinking further, we want to use only the app driver object in test >> suites. So the SutNode object should have a method that returns the >> object and the rest of the implementation needs to be delegated to the >> rest of the objects in the hierarchy to ensure proper remote OS >> handling. > > > I like the sound of this. If we use a method and something like an enum i= nside of SutNode as well to create these objects then we don't even really = need a method to return this object because it would never be interacted wi= th directly. > >> >> >> >> >> >> > >> >> > def close(self, force: bool =3D False) -> None: >> >> > """ >> >> > 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 m= ay never be utilized >> >> > + """ >> >> > + client: SSHClient =3D SSHClient() >> >> > + client.set_missing_host_key_policy(AutoAddPolicy) >> >> > + ip: str =3D node_config.hostname >> >> > + logger.info(GREEN(f"Connecting to host {ip}")) >> >> >> >> Using colors is a remnant from the original DTS. If we want to >> >> (re-)introduce colors I'd do that in a separate patch in a uniform >> >> manner. >> > >> > >> > I agree this is likely outside the scope of this patch and the color h= ere really isn't necessary either so I'll remove it. >> > >> >> >> >> >> >> > + #Preset to 22 because paramiko doesn't accept None >> >> > + port: int =3D 22 >> >> >> >> This configuration handling should be moved to NodeConfiguration. >> >> The logic should also be moved to InteractiveScriptHandler. We also >> >> probably don't need a factory for this. >> > >> > >> > I agree that the configuration could be moved into NodeConfiguration, = however the reason I use a factory and don't have this connect logic inside= InteractiveScriptHandler is because that would mean there would have to be= an SSH session per application. This is one potential solution, but what I= am doing here instead is creating one SSH session for interactive scripts = at the start alongside the usual remote session. This session essentially e= xists in the background for the duration of the execution and then each app= lication for DPDK creates a channel off that session when they are created.= Then, when you are done using the application, it closes the channel but t= he session itself stays open. >> > >> >> Ok, I see. What do you think about this: >> Let's then move the session creation into a subclass of RemoteSession. >> The session would be stored as a class variable and with that we could >> subclass further: >> RemoteSession -> new subclass -> InteractiveScriptHandler -> DpdkApp >> >> Everything could be handled from within the DdpdkApp objects, but we'd >> need to be careful about cleanup (maybe we'd only need to check that >> there are no open channels when closing the session). >> >> Or alternatively, the new subclass could just have a method that >> returns DpdkApps. We'd have two different class relations: >> RemoteSession -> new subclass and InteractiveScriptHandler -> DpdkApp. >> >> I'd put all of the classes into the remote_session directory. >> > > I like the idea of making a subclass of the remote session and then a met= hod in the subclass that generates DpdkApps. This way you have a proper cla= ss that maintains the interactive session and InteractiveScriptHandlers as = well as any other DpdkApp can be created and destroyed as needed. The only = reason I wouldn't make InteractiveScriptHandler a subclass of RemoteSession= is because then it would make the InteractiveScriptHandler something that = stored the main SSH session rather than something that just uses it to make= a channel. > > Originally, I avoided this because I didn't want to implement the send_co= mmand methods from inside RemoteSession, but a solution to this could just = be creating an InteractiveScriptHandler, sending the command blindly, and n= ot returning the output. Or maybe there would be some kind of default promp= t I could expect to still collect the output, but I'm not sure there would = be something that is os-agnostic for this. I'm not sure the best way to sho= w that I don't want people to use the method on the new subclass that I'm g= oing to create though. I guess one way to handle this would be just not cre= ating a method that returns this subclass in SutNode. SutNode could just ha= ve a method that returns DPDK apps which should discourage touching this se= ssion directly. > Ah right, the fact that the workflow with interactive sessions is a bit different doesn't make subclassing RemoteSession ideal - the abstract methods _send_command and copy_file don't make much sense in the new class. With that in mind, I think we have two options: 1. Don't use one session with multiple channels. This would make it more in line with RemoteSession, but the copy_file method still doesn't make sense, so I prefer 2. 2. Don't subclass RemoteSession as it's a bit too different from what we need. Just create a new class (named InteractiveRemoteSession perpahs) that would basically be a factory for drivers (with two parts - ssh session init and then the creation of driver/app objects). The InteractiveScriptHandler (I'm thinking of renaming it to InteractiveShell now) -> DpdkApp would be the other part. I'd put InteractiveRemoteSession and InteractiveScriptHandler/InteractiveShell into the same file (those two basically define the API on the user and developer side). >> >> >> >> >> >> >> > + if ":" in node_config.hostname: >> >> > + ip, port =3D node_config.hostname.split(":") >> >> > + port =3D int(port) >> >> > + client.connect( >> >> > + ip, >> >> > + username=3Dnode_config.user, >> >> > + port=3Dport, >> >> > + password=3Dnode_config.password or "", >> >> > + timeout=3D20 if port else 10 >> >> > + ) >> >> > + return client >> >> > diff --git a/dts/framework/test_result.py b/dts/framework/test_resu= lt.py >> >> > index 74391982..77202ae2 100644 >> >> > --- a/dts/framework/test_result.py >> >> > +++ b/dts/framework/test_result.py >> >> > @@ -8,6 +8,7 @@ >> >> > import os.path >> >> > from collections.abc import MutableSequence >> >> > from enum import Enum, auto >> >> > +from typing import Dict >> >> > >> >> > from .config import ( >> >> > OS, >> >> > @@ -67,12 +68,13 @@ class Statistics(dict): >> >> > Using a dict provides a convenient way to format the data. >> >> > """ >> >> > >> >> > - def __init__(self, dpdk_version): >> >> > + def __init__(self, output_info: Dict[str, str] | None): >> >> > super(Statistics, self).__init__() >> >> > for result in Result: >> >> > self[result.name] =3D 0 >> >> > self["PASS RATE"] =3D 0.0 >> >> > - self["DPDK VERSION"] =3D dpdk_version >> >> > + if output_info: >> >> > + for info_key, info_val in output_info.items(): self[in= fo_key] =3D info_val >> >> > >> >> > def __iadd__(self, other: Result) -> "Statistics": >> >> > """ >> >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): >> >> > """ >> >> > >> >> > dpdk_version: str | None >> >> > + output: dict | None >> >> > _logger: DTSLOG >> >> > _errors: list[Exception] >> >> > _return_code: ErrorSeverity >> >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): >> >> > def __init__(self, logger: DTSLOG): >> >> > super(DTSResult, self).__init__() >> >> > self.dpdk_version =3D None >> >> > + self.output =3D None >> >> > self._logger =3D logger >> >> > self._errors =3D [] >> >> > self._return_code =3D ErrorSeverity.NO_ERR >> >> > @@ -296,7 +300,10 @@ def process(self) -> None: >> >> > for error in self._errors: >> >> > self._logger.debug(repr(error)) >> >> > >> >> > - self._stats_result =3D Statistics(self.dpdk_version) >> >> > + self._stats_result =3D Statistics(self.output) >> >> > + #add information gathered from the smoke tests to the stat= istics >> >> > + # for info_key, info_val in smoke_test_info.items(): self.= _stats_result[info_key] =3D info_val >> >> > + # print(self._stats_result) >> >> > self.add_stats(self._stats_result) >> >> > with open(self._stats_filename, "w+") as stats_file: >> >> > stats_file.write(str(self._stats_result)) >> >> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite= .py >> >> > index 0705f38f..1518fb8a 100644 >> >> > --- a/dts/framework/test_suite.py >> >> > +++ b/dts/framework/test_suite.py >> >> > @@ -10,11 +10,14 @@ >> >> > import inspect >> >> > import re >> >> > from types import MethodType >> >> > +from typing import Dict >> >> > >> >> > -from .exception import ConfigurationError, SSHTimeoutError, TestCa= seVerifyError >> >> > +from .config import BuildTargetConfiguration >> >> > + >> >> > +from .exception import BlockingTestSuiteError, ConfigurationError,= SSHTimeoutError, TestCaseVerifyError >> >> > from .logger import DTSLOG, getLogger >> >> > from .settings import SETTINGS >> >> > -from .test_result import BuildTargetResult, Result, TestCaseResult= , TestSuiteResult >> >> > +from .test_result import BuildTargetResult, DTSResult, Result, Tes= tCaseResult, TestSuiteResult >> >> > from .testbed_model import SutNode >> >> > >> >> > >> >> > @@ -37,10 +40,12 @@ class TestSuite(object): >> >> > """ >> >> > >> >> > sut_node: SutNode >> >> > + is_blocking =3D False >> >> > _logger: DTSLOG >> >> > _test_cases_to_run: list[str] >> >> > _func: bool >> >> > _result: TestSuiteResult >> >> > + _dts_result: DTSResult >> >> > >> >> > def __init__( >> >> > self, >> >> > @@ -48,6 +53,8 @@ def __init__( >> >> > test_cases: list[str], >> >> > func: bool, >> >> > build_target_result: BuildTargetResult, >> >> > + build_target_conf: BuildTargetConfiguration, >> >> > + dts_result: DTSResult >> >> > ): >> >> > self.sut_node =3D sut_node >> >> > self._logger =3D getLogger(self.__class__.__name__) >> >> > @@ -55,6 +62,8 @@ def __init__( >> >> > self._test_cases_to_run.extend(SETTINGS.test_cases) >> >> > self._func =3D func >> >> > self._result =3D build_target_result.add_test_suite(self._= _class__.__name__) >> >> > + self.build_target_info =3D build_target_conf >> >> > + self._dts_result =3D dts_result >> >> > >> >> > def set_up_suite(self) -> None: >> >> > """ >> >> > @@ -118,6 +127,9 @@ def run(self) -> None: >> >> > f"the next test suite may be affected." >> >> > ) >> >> > self._result.update_setup(Result.ERROR, e) >> >> > + if len(self._result.get_errors()) > 0 and self.is_bloc= king: >> >> > + raise BlockingTestSuiteError(test_suite_name) >> >> > + >> >> > >> >> > def _execute_test_suite(self) -> None: >> >> > """ >> >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: >> >> > f"Attempt number {attempt_nr} out of {all_= attempts}." >> >> > ) >> >> > self._run_test_case(test_case_method, test_cas= e_result) >> >> > + >> >> > >> >> > def _get_functional_test_cases(self) -> list[MethodType]: >> >> > """ >> >> > @@ -232,6 +245,11 @@ def _execute_test_case( >> >> > test_case_result.update(Result.SKIP) >> >> > raise KeyboardInterrupt("Stop DTS") >> >> > >> >> > + def write_to_statistics_file(self, output: Dict[str, str]): >> >> > + if self._dts_result.output !=3D None: >> >> > + self._dts_result.output.update(output) >> >> > + else: >> >> > + self._dts_result.output =3D output >> >> > >> >> > def get_test_suites(testsuite_module_path: str) -> list[type[TestS= uite]]: >> >> > def is_test_suite(object) -> bool: >> >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: >> >> > test_suite_class >> >> > for _, test_suite_class in inspect.getmembers(testcase_mod= ule, is_test_suite) >> >> > ] >> >> > + >> >> > + >> >> > diff --git a/dts/framework/testbed_model/__init__.py b/dts/framewor= k/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/interacti= ve_command.py b/dts/framework/testbed_model/interactive_apps/interactive_co= mmand.py >> >> > new file mode 100644 >> >> > index 00000000..7467911b >> >> > --- /dev/null >> >> > +++ b/dts/framework/testbed_model/interactive_apps/interactive_comm= and.py >> >> >> >> In general, the file name should match the class name. >> >> >> >> > @@ -0,0 +1,57 @@ >> >> > +# import paramiko >> >> > +from paramiko import SSHClient, Channel, channel >> >> > +from framework.settings import SETTINGS >> >> > + >> >> > +class InteractiveScriptHandler: >> >> >> >> Once merged with the init functionality and moved to remote_session, >> >> I'd rename it to InteractiveAppSession or something similar. >> > >> > >> > Good point, I'll rename the class and the file. >> > >> >> >> >> >> >> > + >> >> > + _ssh_client: SSHClient >> >> > + _stdin: channel.ChannelStdinFile >> >> > + _ssh_channel: Channel >> >> > + >> >> > + def __init__(self, ssh_client: SSHClient, timeout:float =3D SE= TTINGS.timeout) -> None: >> >> > + self._ssh_client =3D ssh_client >> >> > + self._ssh_channel =3D self._ssh_client.invoke_shell() >> >> > + self._stdin =3D self._ssh_channel.makefile_stdin("wb") >> >> > + self._ssh_channel.settimeout(timeout) >> >> > + >> >> > + 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() >> >> > + >> >> > + def send_command_get_output(self, command:str, expect:str) -> = str: >> >> >> >> Let's rename expect to prompt. At least for me it was just confusing >> >> in the original DTS. >> > >> > >> > It definitely can be confusing, I'll change it accordingly as you're r= ight, prompt is more clear. >> > >> >> >> >> >> >> > + """ >> >> > + Send a command and get all output before the expected endi= ng string. >> >> > + >> >> > + **NOTE** >> >> > + Lines that expect input are not included in the stdout buf= fer so they cannot be >> >> > + used for expect. 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 t= he stdout buffer. >> >> > + >> >> > + *Return* >> >> > + All output before expected string >> >> > + """ >> >> > + stdout =3D self._ssh_channel.makefile("r") >> >> > + self._stdin.write(command + '\n') >> >> > + self._stdin.flush() >> >> > + out:str =3D "" >> >> > + for line in stdout: >> >> > + out +=3D str(line) >> >> > + if expect in str(line): >> >> > + break >> >> > + stdout.close() #close the buffer to flush the output >> >> > + 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_d= river.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.p= y >> >> > @@ -0,0 +1,24 @@ >> >> > +from framework.testbed_model.interactive_apps import InteractiveSc= riptHandler >> >> > + >> >> > +from pathlib import PurePath >> >> > + >> >> > +class TestpmdDriver: >> >> >> >> This could also be called TestPmdSession. Not sure which is better. >> >> This should extend InteractiveScriptHandler, which should contain >> >> basic functionality common to all apps and then the drivers would >> >> contain app-specific functionality. >> > >> > >> > Originally the reason I avoided doing this was because I viewed them a= s separate layers of the process essentially. I wrote it in a way where the= InteractiveScriptHandler was almost like an interface for any CLI and then= DPDK apps could interact with this interface, but after thinking about it = more, this isn't necessary. There is no reason really that the applications= themselves can't just use these methods directly and it also avoids the ne= ed to have a reference to the handler within the object. >> > >> >> >> >> >> >> > + prompt:str =3D "testpmd>" >> >> > + interactive_handler: InteractiveScriptHandler >> >> > + >> >> > + def __init__(self, handler: InteractiveScriptHandler, dpdk_bui= ld_dir:PurePath, eal_flags:str =3D "", cmd_line_options:str =3D "") -> None= : >> >> > + """ >> >> > + Sets the handler to drive the SSH session and starts testp= md >> >> > + """ >> >> > + self.interactive_handler =3D 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_b= uild_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.pr= ompt) >> >> >> >> The paths need to be handled in os-agnostic manner in os_session and >> >> then passed here. >> >> >> >> > + >> >> > + def send_command(self, command:str, expect:str =3D prompt) -> = str: >> >> > + """ >> >> > + Specific way of handling the command for testpmd >> >> > + >> >> > + An extra newline character is consumed in order to force t= he current line into the stdout buffer >> >> > + """ >> >> > + return self.interactive_handler.send_command_get_output(co= mmand + "\n", expect) >> >> > \ No newline at end of file >> >> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/te= stbed_model/node.py >> >> > index d48fafe6..c5147e0e 100644 >> >> > --- a/dts/framework/testbed_model/node.py >> >> > +++ b/dts/framework/testbed_model/node.py >> >> > @@ -40,6 +40,7 @@ class Node(object): >> >> > lcores: list[LogicalCore] >> >> > _logger: DTSLOG >> >> > _other_sessions: list[OSSession] >> >> > + _execution_config: ExecutionConfiguration >> >> > >> >> > def __init__(self, node_config: NodeConfiguration): >> >> > self.config =3D node_config >> >> > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: Exec= utionConfiguration) -> None: >> >> > """ >> >> > self._setup_hugepages() >> >> > self._set_up_execution(execution_config) >> >> > + self._execution_config =3D execution_config >> >> > >> >> > def _set_up_execution(self, execution_config: ExecutionConfigu= ration) -> None: >> >> > """ >> >> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framewor= k/testbed_model/sut_node.py >> >> > index 2b2b50d9..8c39a66d 100644 >> >> > --- a/dts/framework/testbed_model/sut_node.py >> >> > +++ b/dts/framework/testbed_model/sut_node.py >> >> > @@ -14,6 +14,7 @@ >> >> > >> >> > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice >> >> > from .node import Node >> >> > +from .interactive_apps import InteractiveScriptHandler >> >> > >> >> > >> >> > class SutNode(Node): >> >> > @@ -261,6 +262,11 @@ def run_dpdk_app( >> >> > return self.main_session.send_command( >> >> > f"{app_path} {eal_args}", timeout, verify=3DTrue >> >> > ) >> >> > + def create_interactive_session_handler(self) -> InteractiveScr= iptHandler: >> >> > + """ >> >> > + Create a handler for interactive sessions >> >> > + """ >> >> > + return InteractiveScriptHandler(self.main_session._interac= tive_session) >> >> > >> >> > >> >> > class EalParameters(object): >> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSui= te_smoke_tests.py >> >> > new file mode 100644 >> >> > index 00000000..bacf289d >> >> > --- /dev/null >> >> > +++ b/dts/tests/TestSuite_smoke_tests.py >> >> > @@ -0,0 +1,94 @@ >> >> > +from framework.test_suite import TestSuite >> >> > +from framework.testbed_model.sut_node import SutNode >> >> > + >> >> > +from framework.testbed_model.interactive_apps import TestpmdDriver >> >> > + >> >> > +def get_compiler_version(compiler_name: str, sut_node: SutNode) ->= str: >> >> >> >> I don't see a reason why this is outside SmokeTest. >> >> >> >> >> >> > + match compiler_name: >> >> > + case "gcc": >> >> > + return sut_node.main_session.send_command(f"{compi= ler_name} --version", 60).stdout.split("\n")[0] >> >> >> >> As I alluded to earlier, the call here should be >> >> sut_node.get_compiler_version(). This is to hide implementation >> >> details from test suite developers. >> >> Then, SutNode.get_compiler_version should then call >> >> self.main_session.get_compiler_version(). The reason here is this is >> >> an os-agnostic call. >> >> get_compiler_version() should then be defined in os_session and >> >> implemented in os specific classes. >> > >> > >> > I originally placed the method outside of smoke tests just so it would= n't be run with the rest of the suite but it does make sense to make this a= nd the other comments os-agnostic. >> > >> >> >> >> >> >> > + case "clang": >> >> > + return sut_node.main_session.send_command(f"{compi= ler_name} --version", 60).stdout.split("\n")[0] >> >> > + case "msvc": >> >> > + return sut_node.main_session.send_command(f"cl", 6= 0).stdout >> >> > + case "icc": >> >> > + return sut_node.main_session.send_command(f"{compi= ler_name} -V", 60).stdout >> >> > + >> >> > +class SmokeTests(TestSuite): >> >> > + is_blocking =3D True >> >> > + >> >> > + def set_up_suite(self) -> None: >> >> > + """ >> >> > + Setup: >> >> > + build all DPDK >> >> > + """ >> >> > + self.dpdk_build_dir_path =3D self.sut_node.remote_dpdk_bui= ld_dir >> >> > + >> >> > + >> >> > + def test_unit_tests(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + run the fast-test unit-test suite through meson >> >> > + """ >> >> > + self.sut_node.main_session.send_command(f"meson test -C {s= elf.dpdk_build_dir_path} --suite fast-tests", 300) >> >> >> >> Same here, there already are methods that build dpdk. If anything >> >> needs to be added, we should expand those methods. >> > >> > >> > I don't think this is building DPDK, this is just running fast tests a= nd ensuring that you are in the correct directory. >> > >> >> Ah, right. These would be probably used only in smoke tests, so it's >> probably fine to leave them here. Maybe we could add a method to the >> suite? I'm thinking it would make the code a bit more readable. >> > > I could look into this. > >> >> >> >> >> >> >> > + >> >> > + def test_driver_tests(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + run the driver-test unit-test suite through meson >> >> > + """ >> >> > + list_of_vdevs =3D "" >> >> > + for dev in self.sut_node._execution_config.vdevs: >> >> > + list_of_vdevs +=3D f"{dev}," >> >> > + print(list_of_vdevs) >> >> > + if len(list_of_vdevs) > 0: >> >> > + self.sut_node.main_session.send_command(f"meson test -= C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev {lis= t_of_vdevs}\"", 300) >> >> > + else: >> >> > + self.sut_node.main_session.send_command(f"meson test -= C {self.dpdk_build_dir_path} --suite driver-tests", 300) >> >> > + >> >> > + def test_gather_info(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + gather information about the system and send output to= statistics.txt >> >> > + """ >> >> > + out =3D {} >> >> > + >> >> > + out['OS'] =3D self.sut_node.main_session.send_command("awk= -F=3D '$1=3D=3D\"NAME\" {print $2}' /etc/os-release", 60).stdout >> >> > + out["OS VERSION"] =3D self.sut_node.main_session.send_comm= and("awk -F=3D '$1=3D=3D\"VERSION\" {print $2}' /etc/os-release", 60, True)= .stdout >> >> > + out["COMPILER VERSION"] =3D get_compiler_version(self.buil= d_target_info.compiler.name, self.sut_node) >> >> > + out["DPDK VERSION"] =3D self.sut_node.dpdk_version >> >> > + if self.build_target_info.os.name =3D=3D "linux": >> >> > + out['KERNEL VERSION'] =3D self.sut_node.main_session.s= end_command("uname -r", 60).stdout >> >> > + elif self.build_target_info.os.name =3D=3D "windows": >> >> > + out['KERNEL VERSION'] =3D self.sut_node.main_session.s= end_command("uname -a", 60).stdout >> >> > + self.write_to_statistics_file(out) >> >> > + >> >> >> >> This is not really a test. If we want to add this, it should be store= d >> >> elsewhere (in their respective objects, probably in result objects). >> >> Three of these (os, os and kernel versions) are node data, the >> >> remaining two are build target data. >> >> I'm not sure it's a good idea to put these into statistics, as the >> >> statistics are aggregated over all executions and build targets. In >> >> case of multiple SUTs (in different executions) and multiple build >> >> targets we'd record misleading data. We could include data from all >> >> build targets and SUTs though. >> >> The reason we (and original DTS) are storing DPDK version in the >> >> current manner is that that doesn't change across anything, we're >> >> always testing the same tarball. >> > >> > >> > You're right that this isn't really testing anything, I had originally= included it here because it was just part of the smoke test outline. It's = definitely out of place though and I can move it out to their respective cl= asses. I think it might make sense to organize/label data based on the SUT = it comes from, I just thought statistics made the most sense because you co= uld see the test statistics as well as what it was testing in one place. >> > >> >> We could put anything we want into statistics (it's not a good idea >> now, but if we change the format of statistics it'd be fine, but I >> think changing the format is out of the scope of this patch) and this >> would make sense there, but the first step is properly storing the >> data. Moving them to statistics would be trivial then. >> > > Right, that makes sense. > >> >> >> >> >> >> >> > + >> >> > + >> >> > + def test_start_testpmd(self) -> None: >> >> > + """ >> >> > + Creates and instance of the testpmd driver to run the test= pmd app >> >> > + """ >> >> > + driver: TestpmdDriver =3D TestpmdDriver(self.sut_node.crea= te_interactive_session_handler(), self.dpdk_build_dir_path) >> >> >> >> The driver should be returned by a method of self.sut_node. >> > >> > >> > Does it make more sense to have methods for creating handlers for vari= ous DPDK apps in SutNode? I had assumed it would be cleaner to just make th= e classes as needed so that you don't have multiple methods doing something= very similar in SutNode (basically just making a class with a different na= me that takes in similar information). I suppose that if you were trying to= have developers only interact with the SutNode class this makes sense. >> > >> >> That's what I meant. The method would return the appropriate app based >> on input (which could be an enum) - I think that's preferable to >> having a method for each app. We should think about non-interactive >> apps here as well (such as the helloworld app that's already in use). >> > > I like this idea for how to handle the creation of the DpdkApps. > >> >> > This is also something that could make good use of having the SSHClien= t in the SutNode and I could see how it would be useful. >> > >> >> >> >> >> >> > + >> >> > + print(driver.send_command("show port summary all")) >> >> > + >> >> > + 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 =3D self.sut_node.main_session.send_command(f"= {self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {ad= dress}", 60) >> >> >> >> This should follow the same abstractions as I outlined above. >> >> However, we don't need to use dpdk-devbind here. It's probably safe t= o >> >> do so (the script is likely not going to be changed), but we could >> > >> > >> > That's a good point about dev-bind, it might be better here to just av= oid the reliance on another script. >> > >> >> >> >> >> >> > + self.verify( >> >> > + len(out.stdout) !=3D 0, >> >> > + f"Failed to find configured device ({address})= using dpdk-devbind.py", >> >> > + ) >> >> > + for string in out.stdout.split(" "): >> >> > + if 'drv=3D' in string: >> >> > + self.verify( >> >> > + string.split("=3D")[1] =3D=3D nic.driv= er.strip(), >> >> > + f'Driver for device {address} does not= match driver listed in configuration (bound to {string.split("=3D")[1]})', >> >> > + ) >> >> > + >> >> > \ No newline at end of file >> >> > -- >> >> > 2.40.1 >> >> > >> > >> > >> > I'll work on these changes, but I'd like to hear what you think about = what I had mentioned about moving the connect logic to the InteractiveScrip= tHandler in the comments above. I had originally written it to use one sess= ion throughout rather than opening and closing SSH connections for every ap= plication but I'd like to hear which you think would be easier/better if th= ere is a difference. >> >> Yes, let's do one session with each app having its own channel.