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 48E4142BAB; Fri, 26 May 2023 15:24:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1C3BE40EE7; Fri, 26 May 2023 15:24:59 +0200 (CEST) Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) by mails.dpdk.org (Postfix) with ESMTP id E8F3040DDA for ; Fri, 26 May 2023 15:24:56 +0200 (CEST) Received: by mail-pg1-f180.google.com with SMTP id 41be03b00d2f7-5304913530fso665771a12.0 for ; Fri, 26 May 2023 06:24:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1685107496; x=1687699496; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=2gBwPvahQ2EKme3nDrFZmxvf7x2cqj2LSNe2OgkGQvA=; b=LUNBcMiFcBqZkBMh9xvXVdhUFS628pdls9ORe+H8z1ElEU/Qh2PwZYOMVfcn3t5Yd4 Kx1juMUdyNC+L9/H/TNTpIeMnWtEYDVZAzMEU4NAkr64vRTFIbdcliB1UO0OAo2lStKv SVPZeEvZeJ9wAdyvRFoWsyoPS4kltIoh3JtUM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685107496; x=1687699496; 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=2gBwPvahQ2EKme3nDrFZmxvf7x2cqj2LSNe2OgkGQvA=; b=Lc9+rAKjmBeRO5xG205VdW38UD0rMFCg20bxxenKRFQS9M1kUk6L+drNmwGOUJh3vR CZZGskUcHQQNMN+JxcUrR43m5HD0zFOmP0Xst+Pht4oYn1gj0Ba9SrjIGh1qdytW8SRc oOsqhpmOcYAEyv6HThGagCu1WBgVglPD3lu96S4WuLjKRpwNJWr6RHdGsyMcB6pnlzI5 JHLdxv5X3maPz6R5Pev0rTKvft+266/iis6EZ3UJi0Xkw2sqcZ7ytCvPnESneuyGKzXC o0yl0BPLjPikfplvXpU3wdjAe5b1A1P1OFh4dGpPXISQh7MnwzB+uT7XU6iIgm/tIq2c z/Fg== X-Gm-Message-State: AC+VfDx3+ESO0+7aWyKlL2zOIV8WcqX9ZTeHIWVK04cdlvFuhBMYrCRi jRE5Zg4fFwQKgembvawGXDMn3A2qXYE1KpUuOxOUZDo1PTDTd3qJ X-Google-Smtp-Source: ACHHUZ7/kRcIp/QhgoUKGhgB1e6NIpAzOOrFFIAaj7V5LzCmpBuyN0nWRX2WJqDcXO9VtkexA2ylgOasit6wEb9z2hY= X-Received: by 2002:a17:903:230f:b0:1ad:bb42:7672 with SMTP id d15-20020a170903230f00b001adbb427672mr3034384plh.29.1685107495705; Fri, 26 May 2023 06:24:55 -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: Jeremy Spewock Date: Fri, 26 May 2023 09:24:44 -0400 Message-ID: Subject: Re: [RFC v2 1/2] dts: add smoke tests To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000b0bf8c05fc98a903" 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 --000000000000b0bf8c05fc98a903 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, May 26, 2023 at 3:36=E2=80=AFAM Juraj Linke=C5=A1 wrote: > 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 t= he > >> >> devlist to cc. > >> > > >> > > >> > Thank you for the tip! I'm still new to sending patches and didn't > think 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 shou= ld > >> >> 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 ha= ve > >> >> more ideas. > >> >> > >> >> On Fri, May 12, 2023 at 9:28=E2=80=AFPM wrot= e: > >> >> > > >> >> > 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 > tests > >> >> > 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/interactive_command.py > >> >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/testpmd_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 > create a subclass for it off of remote_session because the idea behind it > was only allowing users to interact with the session through the > InteractiveScriptHandler/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 written now. I could do this but it seems like a > bigger discussion about whether interactive sessions should create a new > SSH session every time or instead just use one existing session and creat= e > 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, jus= t > >> >> as Owen did in his internal GitLab patch. If you can access it, hav= e > a > >> >> look at how he did it. > >> >> This brings an interesting question of where we want to specify whi= ch > >> >> 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 b= y > >> >> testing needs. What makes the most sense? We could specify NIC > details > >> >> 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 > multiple NICs in your SUT but want to separate them into different > executions. > >> > >> 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 > target because it was talked about previously that then is when we should > run the smoke tests. I think however it would make sense to specify the N= IC > you are using for that execution rather than having to assume or specify > elsewhere. > >> > > >> > >> 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 > change 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 > easier 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, > ExecutionConfiguration > >> >> > from .logger import DTSLOG, getLogger > >> >> > from .test_result import BuildTargetResult, DTSResult, > ExecutionResult, 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 i= t > 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_config.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 > suite executions > >> >> > + if end_execution: > >> >> > + break > >> >> > > >> >> > > >> >> > def _exit_dts() -> None: > >> >> > diff --git a/dts/framework/exception.py > b/dts/framework/exception.py > >> >> > 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/framework/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_session > >> >> > +from .remote import CommandResult, RemoteSession, > create_remote_session, 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_confi= g, > name, logger) > >> >> > + self._interactive_session =3D > create_interactive_session(node_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 sessi= on > that gets created once per node. However, I could just as easily have > SutNode store it and call upon OSSession to create one inside its > constructor. This would make creating classes for DPDK apps easier as wel= l > once those are subclasses 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 > inside of SutNode as well to create these objects then we don't even real= ly > need a method to return this object because it would never be interacted > with 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 > may 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 > here 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 insi= de > InteractiveScriptHandler is because that would mean there would have to b= e > 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 > exists in the background for the duration of the execution and then each > application 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 the 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 > method in the subclass that generates DpdkApps. This way you have a prope= r > class that maintains the interactive session and InteractiveScriptHandler= s > 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 jus= t > uses it to make a channel. > > > > Originally, I avoided this because I didn't want to implement the > send_command methods from inside RemoteSession, but a solution to this > could just be creating an InteractiveScriptHandler, sending the command > blindly, and not returning the output. Or maybe there would be some kind = of > default prompt I could expect to still collect the output, but I'm not su= re > there would be something that is os-agnostic for this. I'm not sure the > best way to show that I don't want people to use the method on the new > subclass that I'm going to create though. I guess one way to handle this > would be just not creating a method that returns this subclass in SutNode= . > SutNode could just have a method that returns DPDK apps which should > discourage touching this session 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). > I think I also prefer 2, I can work on putting that together. These comments definitely gave me a lot of good insight into where to go from here though, thank you! > > >> > >> >> > >> >> > >> >> > + 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_result.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[info_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 > statistics > >> >> > + # 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, > TestCaseVerifyError > >> >> > +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, > TestCaseResult, 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_blocking: > >> >> > + 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_case_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[TestSuite]]: > >> >> > 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_module, is_test_suite) > >> >> > ] > >> >> > + > >> >> > + > >> >> > diff --git a/dts/framework/testbed_model/__init__.py > b/dts/framework/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/interactive_command.py > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > >> >> > new file mode 100644 > >> >> > index 00000000..7467911b > >> >> > --- /dev/null > >> >> > +++ > b/dts/framework/testbed_model/interactive_apps/interactive_command.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 > SETTINGS.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 confusin= g > >> >> in the original DTS. > >> > > >> > > >> > It definitely can be confusing, I'll change it accordingly as you're > right, prompt is more clear. > >> > > >> >> > >> >> > >> >> > + """ > >> >> > + Send a command and get all output before the expected > ending string. > >> >> > + > >> >> > + **NOTE** > >> >> > + Lines that expect input are not included in the stdout > buffer so they cannot be > >> >> > + used for expect. For example, if you were prompted to lo= g > 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 coul= d > be consuming an > >> >> > + extra newline character to force the current prompt into > the 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_driver.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.py > >> >> > @@ -0,0 +1,24 @@ > >> >> > +from framework.testbed_model.interactive_apps import > InteractiveScriptHandler > >> >> > + > >> >> > +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 > as 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 abo= ut > 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 need to have a reference to the handler within the object. > >> > > >> >> > >> >> > >> >> > + prompt:str =3D "testpmd>" > >> >> > + interactive_handler: InteractiveScriptHandler > >> >> > + > >> >> > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str =3D "", cmd_line_options:str =3D "= ") -> > None: > >> >> > + """ > >> >> > + Sets the handler to drive the SSH session and starts > testpmd > >> >> > + """ > >> >> > + 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_build_dir}/app/d= pdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > >> >> > >> >> The paths need to be handled in os-agnostic manner in os_session an= d > >> >> 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 > the current line into the stdout buffer > >> >> > + """ > >> >> > + return > self.interactive_handler.send_command_get_output(command + "\n", expect) > >> >> > \ No newline at end of file > >> >> > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_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: > ExecutionConfiguration) -> None: > >> >> > """ > >> >> > self._setup_hugepages() > >> >> > self._set_up_execution(execution_config) > >> >> > + self._execution_config =3D execution_config > >> >> > > >> >> > def _set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > >> >> > """ > >> >> > diff --git a/dts/framework/testbed_model/sut_node.py > b/dts/framework/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) -> > InteractiveScriptHandler: > >> >> > + """ > >> >> > + Create a handler for interactive sessions > >> >> > + """ > >> >> > + return > InteractiveScriptHandler(self.main_session._interactive_session) > >> >> > > >> >> > > >> >> > class EalParameters(object): > >> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py > b/dts/tests/TestSuite_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 TestpmdDriv= er > >> >> > + > >> >> > +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"{compiler_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 i= s > >> >> 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 > wouldn't be run with the rest of the suite but it does make sense to make > this and the other comments os-agnostic. > >> > > >> >> > >> >> > >> >> > + case "clang": > >> >> > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > >> >> > + case "msvc": > >> >> > + return sut_node.main_session.send_command(f"cl", > 60).stdout > >> >> > + case "icc": > >> >> > + return > sut_node.main_session.send_command(f"{compiler_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_build_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 > {self.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 > and 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 > {list_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("a= wk > -F=3D '$1=3D=3D\"NAME\" {print $2}' /etc/os-release", 60).stdout > >> >> > + out["OS VERSION"] =3D > self.sut_node.main_session.send_command("awk -F=3D '$1=3D=3D\"VERSION\" {= print > $2}' /etc/os-release", 60, True).stdout > >> >> > + out["COMPILER VERSION"] =3D get_compiler_version( > self.build_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.send_command("uname -r", 60).stdout > >> >> > + elif self.build_target_info.os.name =3D=3D "windows": > >> >> > + out['KERNEL VERSION'] =3D > self.sut_node.main_session.send_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 > stored > >> >> 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 the= ir > respective classes. 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 could 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 > testpmd app > >> >> > + """ > >> >> > + driver: TestpmdDriver =3D > TestpmdDriver(self.sut_node.create_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 > various DPDK apps in SutNode? I had assumed it would be cleaner to just > make the 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 name 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 > SSHClient 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}/../u= sertools/dpdk-devbind.py > --status | grep {address}", 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 > to > >> >> 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 > avoid 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.driver.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 abou= t > what I had mentioned about moving the connect logic to the > InteractiveScriptHandler in the comments above. I had originally written = it > to use one session throughout rather than opening and closing SSH > connections for every application but I'd like to hear which you think > would be easier/better if there is a difference. > >> > >> Yes, let's do one session with each app having its own channel. > --000000000000b0bf8c05fc98a903 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, May 26, 2023 at 3:36=E2=80=AF= AM Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Thu, May 25, 2023 at 8:03= =E2=80=AFPM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> Hey Juraj,
>
> On Thu, May 25, 2023 at 4:33=E2=80=AFAM Juraj Linke=C5=A1 <juraj.li= nkes@pantheon.tech> wrote:
>>
>> One more point that doesn't fit elsewhere:
>>
>> On Wed, May 24, 2023 at 10:45=E2=80=AFPM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>> >
>> >
>> >
>> > On Tue, May 23, 2023 at 4:05=E2=80=AFAM Juraj Linke=C5=A1 <= ;juraj.linkes@pantheon.tech> wrote:
>> >>
>> >> Hi Jeremy, first, a few general points:
>> >>
>> >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lij= uan 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 a= nd didn't think 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 curre= nt
>> >> abstractions. The basic idea is that the test suite devel= opers should
>> >> ideally only use the sut/tg node objects and those object= s should
>> >> delegate logic further to their nested objects. More belo= w.
>> >>
>> >> I have many comments about the implementation, but I have= n't run it
>> >> yet. I'm going to do that after this round of comment= s and I may have
>> >> more ideas.
>> >>
>> >> On Fri, May 12, 2023 at 9:28=E2=80=AFPM <jspewock@iol.unh.edu> w= rote:
>> >> >
>> >> > From: Jeremy Spewock <jspewock@iol.unh.edu>
>> >> >
>> >> > Adds a new test suite for running smoke tests that v= erify general
>> >> > configuration aspects of the system under test. If a= ny of these tests
>> >> > fail, the DTS execution terminates as part of a &quo= t;fail-fast" model.
>> >> >
>> >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
>> >> > ---
>> >> >=C2=A0 dts/conf.yaml=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 9 ++
>> >> >=C2=A0 dts/framework/config/__init__.py=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 21 +++++
>> >> >=C2=A0 dts/framework/config/conf_yaml_schema.json=C2= =A0 =C2=A0 | 32 ++++++-
>> >> >=C2=A0 dts/framework/dts.py=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 19 +++= -
>> >> >=C2=A0 dts/framework/exception.py=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 11 +++
>> >> >=C2=A0 dts/framework/remote_session/os_session.py=C2= =A0 =C2=A0 |=C2=A0 6 +-
>> >> >=C2=A0 .../remote_session/remote/__init__.py=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0| 28 ++++++
>> >> >=C2=A0 dts/framework/test_result.py=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 13 ++-
>> >> >=C2=A0 dts/framework/test_suite.py=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 24 ++++-
>> >> >=C2=A0 dts/framework/testbed_model/__init__.py=C2=A0 = =C2=A0 =C2=A0 =C2=A0|=C2=A0 5 +
>> >> >=C2=A0 .../interactive_apps/__init__.py=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 6 ++
>> >> >=C2=A0 .../interactive_apps/interactive_command.py=C2= =A0 =C2=A0| 57 +++++++++++
>> >> >=C2=A0 .../interactive_apps/testpmd_driver.py=C2=A0 = =C2=A0 =C2=A0 =C2=A0 | 24 +++++
>> >> >=C2=A0 dts/framework/testbed_model/node.py=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 2 +
>> >> >=C2=A0 dts/framework/testbed_model/sut_node.py=C2=A0 = =C2=A0 =C2=A0 =C2=A0|=C2=A0 6 ++
>> >> >=C2=A0 dts/tests/TestSuite_smoke_tests.py=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 94 +++++++++++++++++++
>> >> >=C2=A0 16 files changed, 348 insertions(+), 9 deletio= ns(-)
>> >> >=C2=A0 create mode 100644 dts/framework/testbed_model= /interactive_apps/__init__.py
>> >> >=C2=A0 create mode 100644 dts/framework/testbed_model= /interactive_apps/interactive_command.py
>> >> >=C2=A0 create mode 100644 dts/framework/testbed_model= /interactive_apps/testpmd_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) a= nd we don't
>> >> need it. I'd also like to move this functionality int= o 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 create a subclass for it off of remote_session because the idea= behind it was only allowing users to interact with the session through the= InteractiveScriptHandler/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 written now. I could do this but it seems like a bigger discussio= n about whether interactive sessions should create a new SSH session every = time or instead 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.=
>
>>
>>
>> >>
>> >>
>> >> >=C2=A0 create mode 100644 dts/tests/TestSuite_smoke_t= ests.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:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 compiler_wrapper: = ccache
>> >> >=C2=A0 =C2=A0 =C2=A0 perf: false
>> >> >=C2=A0 =C2=A0 =C2=A0 func: true
>> >> > +=C2=A0 =C2=A0 nics: #physical devices to be used fo= r testing
>> >> > +=C2=A0 =C2=A0 =C2=A0 - addresses:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "0000:11:= 00.0"
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "0000:11:= 00.1"
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 driver: "i40e"= ;
>> >> > +=C2=A0 =C2=A0 vdevs: #names of virtual devices to b= e used for testing
>> >> > +=C2=A0 =C2=A0 =C2=A0 - "crypto_openssl" >> >>
>> >> I believe we specified the NICs under SUTs in the origina= l DTS, just
>> >> as Owen did in his internal GitLab patch. If you can acce= ss it, have a
>> >> look at how he did it.
>> >> This brings an interesting question of where we want to s= pecify which
>> >> NICs/vdevs to test. It could be on the SUT level, but als= o on the
>> >> execution or even the build target level. This should be = informed by
>> >> testing needs. What makes the most sense? We could specif= y NIC details
>> >> per SUT/TG and then just reference the NICs on the execut= ion/build
>> >> target level.
>> >
>> >
>> > I put it on the execution level with the thought that you mig= ht have multiple NICs in your SUT but want to separate them into different = executions.
>>
>> This is a good point. Is this something you're interested in i= n the
>> lab? We should ask Lijuan what she thinks of this. In general, I l= ike
>> 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 ad= ding it
>> now.
>>
>> > I guess in the case of smoke tests, you'd only need to kn= ow them per target because it was talked about previously that then is when= we should run the smoke tests. I think however it would make sense to spec= ify the NIC you are using for that execution rather than having to assume o= r specify elsewhere.
>> >
>>
>> Nothing is set in stone, we don't have to run them per build t= arget.
>> 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 tha= t.
>> If it's not much extra work, we could just do the split like t= hat.
>>
>
> 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 nee= ds to change is when they are run. This would probably take some more plann= ing as to what we want to run at which points during the run and it might b= e easier to get these tests in a better spot first. I do like the idea thou= gh, and agree that it's worth exploring.
>
>> >>
>> >>
>> >> >=C2=A0 =C2=A0 =C2=A0 test_suites:
>> >> > +=C2=A0 =C2=A0 =C2=A0 - smoke_tests
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 - hello_world
>> >> >=C2=A0 =C2=A0 =C2=A0 system_under_test: "SUT 1&q= uot;
>> >> >=C2=A0 nodes:
>> >> >=C2=A0 =C2=A0 - name: "SUT 1"
>> >> >=C2=A0 =C2=A0 =C2=A0 hostname: sut1.change.me.localho= st
>> >> >=C2=A0 =C2=A0 =C2=A0 user: root
>> >> > +=C2=A0 =C2=A0 password: ""
>> >>
>> >> This was deliberately left out to discourage the use of p= asswords.
>> >>
>> >> >=C2=A0 =C2=A0 =C2=A0 arch: x86_64
>> >> >=C2=A0 =C2=A0 =C2=A0 os: linux
>> >> >=C2=A0 =C2=A0 =C2=A0 lcores: ""
>> >>
>> >> <snip>
>> >>
>> >> > diff --git a/dts/framework/dts.py b/dts/framework/dt= s.py
>> >> > index 05022845..0d03e158 100644
>> >> > --- a/dts/framework/dts.py
>> >> > +++ b/dts/framework/dts.py
>> >> > @@ -5,6 +5,8 @@
>> >> >
>> >> >=C2=A0 import sys
>> >> >
>> >> > +from .exception import BlockingTestSuiteError
>> >> > +
>> >> >=C2=A0 from .config import CONFIGURATION, BuildTarget= Configuration, ExecutionConfiguration
>> >> >=C2=A0 from .logger import DTSLOG, getLogger
>> >> >=C2=A0 from .test_result import BuildTargetResult, DT= SResult, ExecutionResult, Result
>> >> > @@ -49,6 +51,7 @@ def run_all() -> None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 nodes[sut_node.name] =3D sut_node
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if s= ut_node:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 #SMOKE TEST EXECUTION GOES HERE!
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 _run_execution(sut_node, execution, result)
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 except Exception as e:
>> >> > @@ -118,7 +121,7 @@ def _run_build_target(
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 try:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_node.set_up_bu= ild_target(build_target)
>> >> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 result.dpdk_version =3D= sut_node.dpdk_version
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # result.dpdk_version = =3D sut_node.dpdk_version
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_resul= t.update_setup(Result.PASS)
>> >> >=C2=A0 =C2=A0 =C2=A0 except Exception as e:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dts_logger.excepti= on("Build target setup failed.")
>> >> > @@ -146,6 +149,7 @@ def _run_suites(
>> >> >=C2=A0 =C2=A0 =C2=A0 with possibly only a subset of t= est cases.
>> >> >=C2=A0 =C2=A0 =C2=A0 If no subset is specified, run a= ll test cases.
>> >> >=C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 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 r= esult is a
>> >> BlockingTestSuiteError (or better, just call a new method= of result
>> >> that will check that), which we could do at multiple stag= es.
>> >
>> >
>> > 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 becau= se 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 cou= ld
>> easily compare the two to sometimes quickly identify the failure),= so
>> if there's a possibility that the test would pass on another b= uild
>> target, we should try to run the build target.
>>
>> In this case, let's just rename the variable to end_build_targ= et or
>> something similar.
>>
>
> Alright, will do.
>
>>
>> >>
>> >>
>> >> >=C2=A0 =C2=A0 =C2=A0 for test_suite_config in executi= on.test_suites:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 full= _suite_path =3D f"tests.TestSuite_{test_suite_config.test_suite}"=
>> >> > @@ -160,13 +164,24 @@ def _run_suites(
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for = test_suite_class in test_suite_classes:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 #HERE NEEDS CHANGING
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 test_suite =3D test_suite_class(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 sut_node,
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 test_suite_config.test_cases,
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 execution.func,
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 build_target_result,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 sut_node._build_target_config,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 result
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 )
>> >> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 test_suite.run()
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 try:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 test_suite.run()
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 except BlockingTestSuiteError as e:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 dts_logger.exception("An error occurred within a = blocking TestSuite, execution will now end.")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 result.add_error(e)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 end_execution =3D True
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 #if a blocking test fai= led and we need to bail out of suite executions
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if end_execution:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break
>> >> >
>> >> >
>> >> >=C2=A0 def _exit_dts() -> None:
>> >> > diff --git a/dts/framework/exception.py b/dts/framew= ork/exception.py
>> >> > index ca353d98..4e3f63d1 100644
>> >> > --- a/dts/framework/exception.py
>> >> > +++ b/dts/framework/exception.py
>> >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum):
>> >> >=C2=A0 =C2=A0 =C2=A0 SSH_ERR =3D 4
>> >> >=C2=A0 =C2=A0 =C2=A0 DPDK_BUILD_ERR =3D 10
>> >> >=C2=A0 =C2=A0 =C2=A0 TESTCASE_VERIFY_ERR =3D 20
>> >> > +=C2=A0 =C2=A0 BLOCKING_TESTSUITE_ERR =3D 25
>> >> >
>> >> >
>> >> >=C2=A0 class DTSError(Exception):
>> >> > @@ -144,3 +145,13 @@ def __init__(self, value: str):=
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __str__(self) -> str:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return repr(self.v= alue)
>> >> > +
>> >> > +class BlockingTestSuiteError(DTSError):
>> >> > +=C2=A0 =C2=A0 suite_name: str
>> >> > +=C2=A0 =C2=A0 severity: ClassVar[ErrorSeverity]=C2= =A0 =3D ErrorSeverity.BLOCKING_TESTSUITE_ERR
>> >> > +
>> >> > +=C2=A0 =C2=A0 def __init__(self, suite_name:str) -&= gt; None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.suite_name =3D sui= te_name
>> >> > +
>> >> > +=C2=A0 =C2=A0 def __str__(self) -> str:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return f"Blocking = suite {self.suite_name} failed."
>> >> > diff --git a/dts/framework/remote_session/os_session= .py b/dts/framework/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 @@
>> >> >=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 CommandResult, RemoteSession, c= reate_remote_session, create_interactive_session
>> >> > +
>> >> > +from paramiko import SSHClient
>> >> >
>> >> >
>> >> >=C2=A0 class OSSession(ABC):
>> >> > @@ -26,6 +28,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: SSHClient
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self,
>> >> > @@ -37,6 +40,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_sessi= on =3D create_interactive_session(node_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 creat= ed 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 i= s because I was essentially thinking of it the same way as the remote sessi= on that gets created once per node. However, I could just as easily have Su= tNode store it and call upon OSSession to create one inside its constructor= . This would make creating classes for DPDK apps easier as well once those = are subclasses of the InteractiveScriptHandler.
>> >
>>
>> I was thinking of this interactive session more like
>> Node._other_sessions, created only when needed. Now that I think a= bout
>> 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 tes= t
>> suites. So the SutNode object should have a method that returns th= e
>> 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 enu= m inside 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 intera= cted with directly.
>
>>
>>
>> >>
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def close(self, force: bool =3D = False) -> None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > diff --git a/dts/framework/remote_session/remote/__i= nit__.py b/dts/framework/remote_session/remote/__init__.py
>> >> > index 8a151221..abca8edc 100644
>> >> > --- a/dts/framework/remote_session/remote/__init__.p= y
>> >> > +++ b/dts/framework/remote_session/remote/__init__.p= y
>> >> > @@ -9,8 +9,36 @@
>> >> >=C2=A0 from .remote_session import CommandResult, Rem= oteSession
>> >> >=C2=A0 from .ssh_session import SSHSession
>> >> >
>> >> > +from paramiko import SSHClient, AutoAddPolicy
>> >> > +from framework.utils import GREEN
>> >> >
>> >> >=C2=A0 def create_remote_session(
>> >> >=C2=A0 =C2=A0 =C2=A0 node_config: NodeConfiguration, = name: str, logger: DTSLOG
>> >> >=C2=A0 ) -> RemoteSession:
>> >> >=C2=A0 =C2=A0 =C2=A0 return SSHSession(node_config, n= ame, logger)
>> >> > +
>> >> > +def create_interactive_session(
>> >> > +=C2=A0 =C2=A0 node_config: NodeConfiguration, name:= str, logger: DTSLOG
>> >> > +) -> SSHClient:
>> >> > +=C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 Creates a paramiko SSH session that i= s designed to be used for interactive shells
>> >> > +
>> >> > +=C2=A0 =C2=A0 This session is meant to be used on a= n "as needed" basis and may never be utilized
>> >> > +=C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 client: SSHClient =3D SSHClient()
>> >> > +=C2=A0 =C2=A0 client.set_missing_host_key_policy(Au= toAddPolicy)
>> >> > +=C2=A0 =C2=A0 ip: str =3D node_config.hostname
>> >> > +=C2=A0 =C2=A0 logger.info(GREEN(f"Connecting to h= ost {ip}"))
>> >>
>> >> Using colors is a remnant from the original DTS. If we wa= nt 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 th= e color here really isn't necessary either so I'll remove it.
>> >
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 #Preset to 22 because paramiko doesn&= #39;t accept None
>> >> > +=C2=A0 =C2=A0 port: int =3D 22
>> >>
>> >> This configuration handling should be moved to NodeConfig= uration.
>> >> The logic should also be moved to InteractiveScriptHandle= r. We also
>> >> probably don't need a factory for this.
>> >
>> >
>> > I agree that the configuration could be moved into NodeConfig= uration, however the reason I use a factory and don't have this connect= logic inside InteractiveScriptHandler is because that would mean there wou= ld have to be an SSH session per application. This is one potential solutio= n, but what I am doing here instead is creating one SSH session for interac= tive scripts at the start alongside the usual remote session. This session = essentially exists in the background for the duration of the execution and = then each application 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 the session itself stays open.
>> >
>>
>> Ok, I see. What do you think about this:
>> Let's then move the session creation into a subclass of Remote= Session.
>> The session would be stored as a class variable and with that we c= ould
>> subclass further:
>> RemoteSession -> new subclass -> InteractiveScriptHandler -&= gt; 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 chec= k 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.<= br> >>
>
> I like the idea of making a subclass of the remote session and then a = method in the subclass that generates DpdkApps. This way you have a proper = class that maintains the interactive session and InteractiveScriptHandlers = as well as any other DpdkApp can be created and destroyed as needed. The on= ly reason I wouldn't make InteractiveScriptHandler a subclass of Remote= Session is because then it would make the InteractiveScriptHandler somethin= g 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_command methods from inside RemoteSession, but a solution to this coul= d just be creating an InteractiveScriptHandler, sending the command blindly= , and not returning the output. Or maybe there would be some kind of defaul= t prompt I could expect to still collect the output, but I'm not sure t= here would be something that is os-agnostic for this. I'm not sure the = best way to show that I don't want people to use the method on the new = subclass that I'm going to create though. I guess one way to handle thi= s would be just not creating a method that returns this subclass in SutNode= . SutNode could just have a method that returns DPDK apps which should disc= ourage touching this session 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 wh= at
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).

I think I also prefer = 2, I can work on=C2=A0putting that together. These comments definitely gave= me a lot of good insight into where to go from here though, thank you!
=C2=A0

>>
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 if ":" in node_config.hostn= ame:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ip, port = =3D node_config.hostname.split(":")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 port =3D = int(port)
>> >> > +=C2=A0 =C2=A0 client.connect(
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ip,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 username=3Dnode_config.= user,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 port=3Dport,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 password=3Dnode_config.= password or "",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout=3D20 if port el= se 10
>> >> > +=C2=A0 =C2=A0 )
>> >> > +=C2=A0 =C2=A0 return client
>> >> > diff --git a/dts/framework/test_result.py b/dts/fram= ework/test_result.py
>> >> > index 74391982..77202ae2 100644
>> >> > --- a/dts/framework/test_result.py
>> >> > +++ b/dts/framework/test_result.py
>> >> > @@ -8,6 +8,7 @@
>> >> >=C2=A0 import os.path
>> >> >=C2=A0 from collections.abc import MutableSequence >> >> >=C2=A0 from enum import Enum, auto
>> >> > +from typing import Dict
>> >> >
>> >> >=C2=A0 from .config import (
>> >> >=C2=A0 =C2=A0 =C2=A0 OS,
>> >> > @@ -67,12 +68,13 @@ class Statistics(dict):
>> >> >=C2=A0 =C2=A0 =C2=A0 Using a dict provides a convenie= nt way to format the data.
>> >> >=C2=A0 =C2=A0 =C2=A0 """
>> >> >
>> >> > -=C2=A0 =C2=A0 def __init__(self, dpdk_version):
>> >> > +=C2=A0 =C2=A0 def __init__(self, output_info: Dict[= str, str] | None):
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(Statistics, = self).__init__()
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for result in Resu= lt:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self= [result= .name] =3D 0
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self["PASS RA= TE"] =3D 0.0
>> >> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self["DPDK VERSION= "] =3D dpdk_version
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if output_info:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for info_= key, info_val in output_info.items(): self[info_key] =3D info_val
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __iadd__(self, other: Result= ) -> "Statistics":
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult):
>> >> >=C2=A0 =C2=A0 =C2=A0 """
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 dpdk_version: str | None
>> >> > +=C2=A0 =C2=A0 output: dict | None
>> >> >=C2=A0 =C2=A0 =C2=A0 _logger: DTSLOG
>> >> >=C2=A0 =C2=A0 =C2=A0 _errors: list[Exception]
>> >> >=C2=A0 =C2=A0 =C2=A0 _return_code: ErrorSeverity
>> >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult):
>> >> >=C2=A0 =C2=A0 =C2=A0 def __init__(self, logger: DTSLO= G):
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(DTSResult, s= elf).__init__()
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_version = =3D None
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.output =3D None >> >> >=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._errors =3D [= ]
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._return_code = =3D ErrorSeverity.NO_ERR
>> >> > @@ -296,7 +300,10 @@ def process(self) -> None: >> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for = error in self._errors:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 self._logger.debug(repr(error))
>> >> >
>> >> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_result =3D = Statistics(self.dpdk_version)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_result =3D = Statistics(self.output)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 #add information gather= ed from the smoke tests to the statistics
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # for info_key, info_va= l in smoke_test_info.items(): self._stats_result[info_key] =3D info_val
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # print(self._stats_res= ult)
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_stats(sel= f._stats_result)
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 with open(self._st= ats_filename, "w+") as stats_file:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stat= s_file.write(str(self._stats_result))
>> >> > diff --git a/dts/framework/test_suite.py b/dts/frame= work/test_suite.py
>> >> > index 0705f38f..1518fb8a 100644
>> >> > --- a/dts/framework/test_suite.py
>> >> > +++ b/dts/framework/test_suite.py
>> >> > @@ -10,11 +10,14 @@
>> >> >=C2=A0 import inspect
>> >> >=C2=A0 import re
>> >> >=C2=A0 from types import MethodType
>> >> > +from typing import Dict
>> >> >
>> >> > -from .exception import ConfigurationError, SSHTimeo= utError, TestCaseVerifyError
>> >> > +from .config import BuildTargetConfiguration
>> >> > +
>> >> > +from .exception import BlockingTestSuiteError, Conf= igurationError, SSHTimeoutError, TestCaseVerifyError
>> >> >=C2=A0 from .logger import DTSLOG, getLogger
>> >> >=C2=A0 from .settings import SETTINGS
>> >> > -from .test_result import BuildTargetResult, Result,= TestCaseResult, TestSuiteResult
>> >> > +from .test_result import BuildTargetResult, DTSResu= lt, Result, TestCaseResult, TestSuiteResult
>> >> >=C2=A0 from .testbed_model import SutNode
>> >> >
>> >> >
>> >> > @@ -37,10 +40,12 @@ class TestSuite(object):
>> >> >=C2=A0 =C2=A0 =C2=A0 """
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 sut_node: SutNode
>> >> > +=C2=A0 =C2=A0 is_blocking =3D False
>> >> >=C2=A0 =C2=A0 =C2=A0 _logger: DTSLOG
>> >> >=C2=A0 =C2=A0 =C2=A0 _test_cases_to_run: list[str] >> >> >=C2=A0 =C2=A0 =C2=A0 _func: bool
>> >> >=C2=A0 =C2=A0 =C2=A0 _result: TestSuiteResult
>> >> > +=C2=A0 =C2=A0 _dts_result: DTSResult
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self,
>> >> > @@ -48,6 +53,8 @@ def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_cases: list[s= tr],
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 func: bool,
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_resul= t: BuildTargetResult,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_conf: Buil= dTargetConfiguration,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dts_result: DTSResult >> >> >=C2=A0 =C2=A0 =C2=A0 ):
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node =3D = sut_node
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger =3D g= etLogger(self.__class__.__name__)
>> >> > @@ -55,6 +62,8 @@ def __init__(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._test_cases_t= o_run.extend(SETTINGS.test_cases)
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._func =3D fun= c
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._result =3D b= uild_target_result.add_test_suite(self.__class__.__name__)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.build_target_info = =3D build_target_conf
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._dts_result =3D dt= s_result
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def set_up_suite(self) -> Non= e:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > @@ -118,6 +127,9 @@ def run(self) -> None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 f"the next test suite may be affected."<= br> >> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 )
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 self._result.update_setup(Result.ERROR, e)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if len(se= lf._result.get_errors()) > 0 and self.is_blocking:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 raise BlockingTestSuiteError(test_suite_name)
>> >> > +
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def _execute_test_suite(self) -&= gt; None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -&= gt; None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"Attempt number {attempt_nr} o= ut of {all_attempts}."
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 )
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 self._run_test_case(test_case_method, test_case_re= sult)
>> >> > +
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def _get_functional_test_cases(s= elf) -> list[MethodType]:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > @@ -232,6 +245,11 @@ def _execute_test_case(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test= _case_result.update(Result.SKIP)
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rais= e KeyboardInterrupt("Stop DTS")
>> >> >
>> >> > +=C2=A0 =C2=A0 def write_to_statistics_file(self, ou= tput: Dict[str, str]):
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self._dts_result.out= put !=3D None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._dts= _result.output.update(output)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._dts= _result.output =3D output
>> >> >
>> >> >=C2=A0 def get_test_suites(testsuite_module_path: str= ) -> list[type[TestSuite]]:
>> >> >=C2=A0 =C2=A0 =C2=A0 def is_test_suite(object) -> = bool:
>> >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> = bool:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_suite_class >> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for _, test_suite_= class in inspect.getmembers(testcase_module, is_test_suite)
>> >> >=C2=A0 =C2=A0 =C2=A0 ]
>> >> > +
>> >> > +
>> >> > diff --git a/dts/framework/testbed_model/__init__.py= b/dts/framework/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 @@
>> >> >=C2=A0 )
>> >> >=C2=A0 from .node import Node
>> >> >=C2=A0 from .sut_node import SutNode
>> >> > +
>> >> > +from .interactive_apps import (
>> >> > +=C2=A0 =C2=A0 InteractiveScriptHandler,
>> >> > +=C2=A0 =C2=A0 TestpmdDriver
>> >> > +)
>> >> > diff --git a/dts/framework/testbed_model/interactive= _apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.p= y
>> >> > 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 (
>> >> > +=C2=A0 =C2=A0 InteractiveScriptHandler
>> >> > +)
>> >> > +from .testpmd_driver import (
>> >> > +=C2=A0 =C2=A0 TestpmdDriver
>> >> > +)
>> >> > \ No newline at end of file
>> >> > diff --git a/dts/framework/testbed_model/interactive= _apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps= /interactive_command.py
>> >> > new file mode 100644
>> >> > index 00000000..7467911b
>> >> > --- /dev/null
>> >> > +++ b/dts/framework/testbed_model/interactive_apps/i= nteractive_command.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 remo= te_session,
>> >> I'd rename it to InteractiveAppSession or something s= imilar.
>> >
>> >
>> > Good point, I'll rename the class and the file.
>> >
>> >>
>> >>
>> >> > +
>> >> > +=C2=A0 =C2=A0 _ssh_client: SSHClient
>> >> > +=C2=A0 =C2=A0 _stdin: channel.ChannelStdinFile
>> >> > +=C2=A0 =C2=A0 _ssh_channel: Channel
>> >> > +
>> >> > +=C2=A0 =C2=A0 def __init__(self, ssh_client: SSHCli= ent, timeout:float =3D SETTINGS.timeout) -> None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_client =3D ss= h_client
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel =3D s= elf._ssh_client.invoke_shell()
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin =3D self._s= sh_channel.makefile_stdin("wb")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel.setti= meout(timeout)
>> >> > +
>> >> > +=C2=A0 =C2=A0 def send_command(self, command:str) -= > None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Send command to channel= without recording output.
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 This method will not ve= rify any input or output, it will
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 simply assume the comma= nd succeeded
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.write(comma= nd + '\n')
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.flush()
>> >> > +
>> >> > +=C2=A0 =C2=A0 def send_command_get_output(self, com= mand: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 accordingl= y as you're right, prompt is more clear.
>> >
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Send a command and get = all output before the expected ending string.
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 **NOTE**
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Lines that expect input= are not included in the stdout buffer so they cannot be
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 used for expect. For ex= ample, if you were prompted to log into something
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 with a username and pas= sword, you cannot expect "username:" because it wont
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 yet be in the stdout bu= ffer. A work around for this could be consuming an
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 extra newline character= to force the current prompt into the stdout buffer.
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *Return*
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 All outpu= t before expected string
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 stdout =3D self._ssh_ch= annel.makefile("r")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.write(comma= nd + '\n')
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.flush()
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out:str =3D ""= ;
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for line in stdout:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out +=3D = str(line)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if expect= in str(line):
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 break
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 stdout.close() #close t= he buffer to flush the output
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return out
>> >> > +
>> >> > +=C2=A0 =C2=A0 def close(self):
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stdin.close()
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._ssh_channel.close= ()
>> >> > +
>> >> > +=C2=A0 =C2=A0 def __del__(self):
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.close()
>> >> > diff --git a/dts/framework/testbed_model/interactive= _apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/test= pmd_driver.py
>> >> > new file mode 100644
>> >> > index 00000000..1993eae6
>> >> > --- /dev/null
>> >> > +++ b/dts/framework/testbed_model/interactive_apps/t= estpmd_driver.py
>> >> > @@ -0,0 +1,24 @@
>> >> > +from framework.testbed_model.interactive_apps impor= t InteractiveScriptHandler
>> >> > +
>> >> > +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 drive= rs would
>> >> contain app-specific functionality.
>> >
>> >
>> > Originally the reason I avoided doing this was because I view= ed them as 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 a= lso avoids the need to have a reference to the handler within the object. >> >
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 prompt:str =3D "testpmd>"= ;
>> >> > +=C2=A0 =C2=A0 interactive_handler: InteractiveScrip= tHandler
>> >> > +
>> >> > +=C2=A0 =C2=A0 def __init__(self, handler: Interacti= veScriptHandler, dpdk_build_dir:PurePath, eal_flags:str =3D "", c= md_line_options:str =3D "") -> None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Sets the handler to dri= ve the SSH session and starts testpmd
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.interactive_handle= r =3D handler
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # self.interactive_hand= ler.send_command("sudo su")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # self.interactive_hand= ler.send_command("cd /root/testpmd-testing/dpdk/build")
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.interactive_handle= r.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_fla= gs} -- -i {cmd_line_options}\n", self.prompt)
>> >>
>> >> The paths need to be handled in os-agnostic manner in os_= session and
>> >> then passed here.
>> >>
>> >> > +
>> >> > +=C2=A0 =C2=A0 def send_command(self, command:str, e= xpect:str =3D prompt) -> str:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Specific way of handlin= g the command for testpmd
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 An extra newline charac= ter is consumed in order to force the current line into the stdout buffer >> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.interactive= _handler.send_command_get_output(command + "\n", expect)
>> >> > \ No newline at end of file
>> >> > diff --git a/dts/framework/testbed_model/node.py b/d= ts/framework/testbed_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):
>> >> >=C2=A0 =C2=A0 =C2=A0 lcores: list[LogicalCore]
>> >> >=C2=A0 =C2=A0 =C2=A0 _logger: DTSLOG
>> >> >=C2=A0 =C2=A0 =C2=A0 _other_sessions: list[OSSession]=
>> >> > +=C2=A0 =C2=A0 _execution_config: ExecutionConfigura= tion
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def __init__(self, node_config: = NodeConfiguration):
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.config =3D no= de_config
>> >> > @@ -64,6 +65,7 @@ def set_up_execution(self, executi= on_config: ExecutionConfiguration) -> None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._setup_hugepa= ges()
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._set_up_execu= tion(execution_config)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._execution_config = =3D execution_config
>> >> >
>> >> >=C2=A0 =C2=A0 =C2=A0 def _set_up_execution(self, exec= ution_config: ExecutionConfiguration) -> None:
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """=
>> >> > diff --git a/dts/framework/testbed_model/sut_node.py= b/dts/framework/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 @@
>> >> >
>> >> >=C2=A0 from .hw import LogicalCoreCount, LogicalCoreL= ist, VirtualDevice
>> >> >=C2=A0 from .node import Node
>> >> > +from .interactive_apps import InteractiveScriptHand= ler
>> >> >
>> >> >
>> >> >=C2=A0 class SutNode(Node):
>> >> > @@ -261,6 +262,11 @@ def run_dpdk_app(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.main_s= ession.send_command(
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f&qu= ot;{app_path} {eal_args}", timeout, verify=3DTrue
>> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>> >> > +=C2=A0 =C2=A0 def create_interactive_session_handle= r(self) -> InteractiveScriptHandler:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Create a handler for in= teractive sessions
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return InteractiveScrip= tHandler(self.main_session._interactive_session)
>> >> >
>> >> >
>> >> >=C2=A0 class EalParameters(object):
>> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dt= s/tests/TestSuite_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 SutNod= e
>> >> > +
>> >> > +from framework.testbed_model.interactive_apps impor= t TestpmdDriver
>> >> > +
>> >> > +def get_compiler_version(compiler_name: str, sut_no= de: SutNode) -> str:
>> >>
>> >> I don't see a reason why this is outside SmokeTest. >> >>
>> >>
>> >> > +=C2=A0 =C2=A0 match compiler_name:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case &quo= t;gcc":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 return sut_node.main_session.send_command(f"{compiler_name} --v= ersion", 60).stdout.split("\n")[0]
>> >>
>> >> As I alluded to earlier, the call here should be
>> >> sut_node.get_compiler_version(). This is to hide implemen= tation
>> >> 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_sessi= on and
>> >> implemented in os specific classes.
>> >
>> >
>> > I originally placed the method outside of smoke tests just so= it wouldn't be run with the rest of the suite but it does make sense t= o make this and the other comments os-agnostic.
>> >
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case &quo= t;clang":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 return sut_node.main_session.send_command(f"{compiler_name} --v= ersion", 60).stdout.split("\n")[0]
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case &quo= t;msvc":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 return sut_node.main_session.send_command(f"cl", 60).stdou= t
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case &quo= t;icc":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 return sut_node.main_session.send_command(f"{compiler_name} -V&= quot;, 60).stdout
>> >> > +
>> >> > +class SmokeTests(TestSuite):
>> >> > +=C2=A0 =C2=A0 is_blocking =3D True
>> >> > +
>> >> > +=C2=A0 =C2=A0 def set_up_suite(self) -> None: >> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Setup:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build all= DPDK
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_build_dir_pat= h =3D self.sut_node.remote_dpdk_build_dir
>> >> > +
>> >> > +
>> >> > +=C2=A0 =C2=A0 def test_unit_tests(self) -> None:=
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 run the f= ast-test unit-test suite through meson
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node.main_sess= ion.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite fa= st-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 and 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 t= he
>> suite? I'm thinking it would make the code a bit more readable= .
>>
>
> I could look into this.
>
>>
>> >>
>> >>
>> >> > +
>> >> > +=C2=A0 =C2=A0 def test_driver_tests(self) -> Non= e:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 run the d= river-test unit-test suite through meson
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 list_of_vdevs =3D "= ;"
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for dev in self.sut_nod= e._execution_config.vdevs:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 list_of_v= devs +=3D f"{dev},"
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 print(list_of_vdevs) >> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if len(list_of_vdevs) &= gt; 0:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_= node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_pa= th} --suite driver-tests --test-args \"--vdev {list_of_vdevs}\"&q= uot;, 300)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_= node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_pa= th} --suite driver-tests", 300)
>> >> > +
>> >> > +=C2=A0 =C2=A0 def test_gather_info(self) -> None= :
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gather in= formation about the system and send output to statistics.txt
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out =3D {}
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out['OS'] =3D s= elf.sut_node.main_session.send_command("awk -F=3D '$1=3D=3D\"= NAME\" {print $2}' /etc/os-release", 60).stdout
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out["OS VERSION&qu= ot;] =3D self.sut_node.main_session.send_command("awk -F=3D '$1=3D= =3D\"VERSION\" {print $2}' /etc/os-release", 60, True).s= tdout
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out["COMPILER VERS= ION"] =3D get_compiler_version(self.build_target_inf= o.compiler.name, self.sut_node)
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out["DPDK VERSION&= quot;] =3D self.sut_node.dpdk_version
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self.bui= ld_target_info.os.name =3D=3D "linux":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out['= KERNEL VERSION'] =3D self.sut_node.main_session.send_command("unam= e -r", 60).stdout
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 elif self.b= uild_target_info.os.name =3D=3D "windows":
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out['= KERNEL VERSION'] =3D self.sut_node.main_session.send_command("unam= e -a", 60).stdout
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.write_to_statistic= s_file(out)
>> >> > +
>> >>
>> >> This is not really a test. If we want to add this, it sho= uld be stored
>> >> elsewhere (in their respective objects, probably in resul= t 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 s= tatistics, as the
>> >> statistics are aggregated over all executions and build t= argets. In
>> >> case of multiple SUTs (in different executions) and multi= ple 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 any= thing, 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 te= st outline. It's definitely out of place though and I can move it out t= o their respective classes. I think it might make sense to organize/label d= ata based on the SUT it comes from, I just thought statistics made the most= sense because you could see the test statistics as well as what it was tes= ting 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, b= ut I
>> think changing the format is out of the scope of this patch) and t= his
>> 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.
>
>>
>> >>
>> >>
>> >> > +
>> >> > +
>> >> > +=C2=A0 =C2=A0 def test_start_testpmd(self) -> No= ne:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Creates and instance of= the testpmd driver to run the testpmd app
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 driver: TestpmdDriver = =3D TestpmdDriver(self.sut_node.create_interactive_session_handler(), self.= dpdk_build_dir_path)
>> >>
>> >> The driver should be returned by a method of self.sut_nod= e.
>> >
>> >
>> > Does it make more sense to have methods for creating handlers= for various DPDK apps in SutNode? I had assumed it would be cleaner to jus= t make the classes as needed so that you don't have multiple methods do= ing something very similar in SutNode (basically just making a class with a= different name that takes in similar information). I suppose that if you w= ere trying to have developers only interact with the SutNode class this mak= es sense.
>> >
>>
>> That's what I meant. The method would return the appropriate a= pp 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-interactiv= e
>> apps here as well (such as the helloworld app that's already i= n 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= SSHClient in the SutNode and I could see how it would be useful.
>> >
>> >>
>> >>
>> >> > +
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 print(driver.send_comma= nd("show port summary all"))
>> >> > +
>> >> > +=C2=A0 =C2=A0 def test_device_bound_to_driver(self)= -> None:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ensure th= at all drivers listed in the config are bound to the correct drivers
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for nic in self.sut_nod= e._execution_config.nics:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for addre= ss in nic.addresses:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 out =3D self.sut_node.main_session.send_command(f"{self.dpdk_bu= ild_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}",= 60)
>> >>
>> >> This should follow the same abstractions as I outlined ab= ove.
>> >> However, we don't need to use dpdk-devbind here. It&#= 39;s probably safe to
>> >> 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 he= re to just avoid the reliance on another script.
>> >
>> >>
>> >>
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 self.verify(
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 len(out.stdout) !=3D 0,
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 f"Failed to find configured device ({address}) us= ing dpdk-devbind.py",
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 )
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 for string in out.stdout.split(" "):
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 if 'drv=3D' in string:
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 string.split("=3D&quo= t;)[1] =3D=3D nic.driver.strip(),
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f'Driver for device {a= ddress} does not match driver listed in configuration (bound to {string.spl= it("=3D")[1]})',
>> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>> >> > +
>> >> > \ 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= InteractiveScriptHandler in the comments above. I had originally written i= t to use one session throughout rather than opening and closing SSH connect= ions for every application but I'd like to hear which you think would b= e easier/better if there is a difference.
>>
>> Yes, let's do one session with each app having its own channel= .
--000000000000b0bf8c05fc98a903--