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 E067A42DD1; Tue, 11 Jul 2023 10:17:09 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7A55940A7D; Tue, 11 Jul 2023 10:17:09 +0200 (CEST) Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by mails.dpdk.org (Postfix) with ESMTP id D28A54003C for ; Tue, 11 Jul 2023 10:17:07 +0200 (CEST) Received: by mail-lf1-f41.google.com with SMTP id 2adb3069b0e04-4fbb281eec6so8301287e87.1 for ; Tue, 11 Jul 2023 01:17:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689063427; x=1691655427; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ZPTOPrSftyAkJmdeX9ipRmCOtfjovJ5YOwo139iFkPY=; b=evQnrP7zDhkA+z46id3wggX3OT1Svvx5Wk7hvr4Nx/G15SLod4qktfxfzXPZz/c80U qlCt4DyVDHfeF6JQlKHMUd4wmPkIZwUCzhLPxf+RCDMbsCqLFe+VRaUXCshbd8Un4Xxo Bm2xAye4ixz4JpK8GMY+m489uuKYeiC/v1SKv94ZrETgV2j64gp1VSku28P/VjI+WM2C HQPB90/LUoxSBFlBmJGPChS8OswRZiW7XmgiXvJQTyJGTU7PBLB9W51FrKCxunjqn46p o2aSzuBkzEYyDSJiEExXvEJ2KW/MMXikXGb+5ZDgvV8BVio9iz5DSgtUfPcIifyIm9+C ifcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689063427; x=1691655427; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZPTOPrSftyAkJmdeX9ipRmCOtfjovJ5YOwo139iFkPY=; b=aNsygDDReyWEOCXMcoXwx5HGZbklRZoe+za9XW992dj6JCq6gHkgOgZDt2A1z5ywS+ AL8R/GiIOlyaZC7IlSQN48znVslzIFpQij5wx5xgDmori+ov4aIAObWZfieDdPjyrk5U tG8CvzPXOfkxu6VaaUXXoTgNDhLacEGsFLrVkZkpjNozIT6q+6PLHim7ytAlyHG0X/Bl WEj8qwi6dZPqwxVUsp6vUFpPheVVy/WA0RjDqizejsSnkDH5KzEVIyY4O69StCzscaKv dxBANjG2s1ovsjwwRjFnQiI9e3bOEOXNn2UO0pDVp6R3pMpJDzXr96uH69xR3oA3nlfU xHEg== X-Gm-Message-State: ABy/qLbrib15e3qVJnYOYGaxMpq4OIfOD0DSdwnThYYU6txUt7b4czmw gYpmtxWrU7bPilYpSzHcSttOMJ8XEI4mbji3B7TXYw== X-Google-Smtp-Source: APBJJlGCNj192Qr0uJzZOeeRNWWJIsbNKYE2l3qVTXxvk+wVmfuDLgvAw/d1nbeqlWfADxA1LvxNFo5Rg+Ccmq9F+l8= X-Received: by 2002:a05:6512:70f:b0:4fb:889a:b410 with SMTP id b15-20020a056512070f00b004fb889ab410mr10979977lfs.65.1689063426265; Tue, 11 Jul 2023 01:17:06 -0700 (PDT) MIME-Version: 1.0 References: <20230615201318.13359-2-jspewock@iol.unh.edu> <20230615201318.13359-3-jspewock@iol.unh.edu> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 11 Jul 2023 10:16:55 +0200 Message-ID: Subject: Re: [PATCH v1 1/2] dts: add smoke tests To: Jeremy Spewock Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, lijuan.tu@intel.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Some more comments below. I agree where I didn't reply. On Sat, Jul 8, 2023 at 1:07=E2=80=AFAM Jeremy Spewock wrote: > > > > On Thu, Jul 6, 2023 at 10:54=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> There are mypy errors related to paramiko: >> framework/remote_session/remote/interactive_remote_session.py:8: error: = Library stubs not installed for "paramiko" (or incompatible with Python 3.1= 0) >> framework/remote_session/remote/interactive_remote_session.py:9: error: = Library stubs not installed for "paramiko.ssh_exception" (or incompatible w= ith Python 3.10) >> >> We do this for pexpect: >> import pexpect # type: ignore >> from pexpect import pxssh # type: ignore >> >> We should do the same for paramiko. > > > Good to know, I'll be sure to change that to make sure it passes. > >> >> >> There are also some changes which may require coordination. I'm not sure= how we'd do that, the best may be to align that off-list. >> >> More inline. >> >> On Thu, Jun 15, 2023 at 10:13=E2=80=AFPM wrote: >> > >> > From: Jeremy Spewock >> > >> > Adds a new test suite as well as configuration changes needed for runn= ing >> > smoke tests that verify general environment 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 | 13 ++ >> > dts/framework/config/__init__.py | 114 +++++++++++++-- >> > dts/framework/config/conf_yaml_schema.json | 135 +++++++++++++++++= - >> > dts/framework/dts.py | 101 ++++++++++--- >> > dts/framework/exception.py | 12 ++ >> > dts/framework/remote_session/__init__.py | 10 +- >> > dts/framework/remote_session/os_session.py | 34 ++++- >> > dts/framework/remote_session/posix_session.py | 30 ++++ >> > .../remote_session/remote/__init__.py | 12 ++ >> > .../remote/interactive_remote_session.py | 113 +++++++++++++++ >> > .../remote/interactive_shell.py | 98 +++++++++++++ >> > .../remote_session/remote/testpmd_shell.py | 58 ++++++++ >> > dts/framework/test_result.py | 38 ++++- >> > dts/framework/test_suite.py | 31 +++- >> > dts/framework/testbed_model/node.py | 2 + >> > dts/framework/testbed_model/sut_node.py | 110 +++++++++++++- >> > dts/tests/TestSuite_smoke_tests.py | 101 +++++++++++++ >> > 17 files changed, 962 insertions(+), 50 deletions(-) >> > create mode 100644 dts/framework/remote_session/remote/interactive_re= mote_session.py >> > create mode 100644 dts/framework/remote_session/remote/interactive_sh= ell.py >> > create mode 100644 dts/framework/remote_session/remote/testpmd_shell.= py >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py >> > >> > diff --git a/dts/conf.yaml b/dts/conf.yaml >> > index a9bd8a3e..03fd57e1 100644 >> > --- a/dts/conf.yaml >> > +++ b/dts/conf.yaml >> > @@ -10,6 +10,8 @@ executions: >> > compiler_wrapper: ccache >> > perf: false >> > func: true >> > + vdevs: #names of virtual devices to be used for testing >> >> Vdevs are optional, let's mention that in the comment as with the hugepa= ges config. > > > That's a good point, I'll be sure to include that for clarity. > >> >> >> > >> > + - "crypto_openssl" >> >> How (or where) is this configured in the original DTS? The vdevs are onl= y going to be used on SUTs, so it may make sense to put them to node config= . On the other hand, each execution using the SUT may use different vdevs, = so this is probably the best place. Maybe we could move it under system_und= er_test in execution? That would make more sense I think - it would be an a= ddition to the existing SUT configuration (as in here's where we augment SU= T config specific to the execution). >> > > > I added it under execution under that same line of thinking that you ment= ioned: different executions might call for use of different vdevs. I think = that is a good point however that these are only going to be used on the SU= T so it makes sense to add it under that section. > > I think this would potentially be good for other things we mentioned as w= ell like how to handle ports on the SUT node and if those should be listed = under SUT or execution. I think if we turn this system_under_test key into = an object that we can add things to the SUT based on the execution. Then, i= n the future, we could create a devices object that could be used for CPU d= evices, NICs, or things of that nature and add them to the SUT based on the= execution. It seems we have an agreement. Let's put vdevs under system_under_test and we'll put other execution specific SUT config there as we add new features. > >> > >> > test_suites: >> > - hello_world >> > system_under_test: "SUT 1" >> > @@ -20,6 +22,17 @@ nodes: >> > arch: x86_64 >> > os: linux >> > lcores: "" >> > + ports: >> > + - pci: "0000:00:08.0" >> > + dpdk_os_driver: vfio-pci >> >> We could find a better name for this driver (and maybe even the regular = os driver). It's the OS driver that dpdk uses and there could be better nam= es - os_dpdk_driver, os_driver_dpdk, os_driver_for_dpdk. Which one do you l= ike the most? Or maybe some other name? >> In either case we probably want to add a comment to both drivers that ex= plain what we mean here. The name doesn't need to be perfect then. :-) I'll= make sure to align the change with my patch if we change this. > > > I think out of those I like os_driver_for_dpdk which makes it a little mo= re clear that it is just an OS driver that DPDK is going to use, not someth= ing specific to DPDK. I agree that this still isn't perfect but it is at le= ast more clear and the comment will help clear it the rest of the way. Makes sense, I'll rename it to os_driver_for_dpdk in my patch. > >> >> > >> > + os_driver: i40e >> > + peer_node: "TG 1" >> > + peer_pci: "0000:00:08.0" >> > + - pci: "0000:00:08.1" >> > + dpdk_os_driver: vfio-pci >> > + os_driver: i40e >> > + peer_node: "TG 1" >> > + peer_pci: "0000:00:08.1" >> > use_first_core: false >> > memory_channels: 4 >> > hugepages: # optional; if removed, will use system hugepage conf= iguration >> > diff --git a/dts/framework/config/__init__.py b/dts/framework/config/_= _init__.py >> > index ebb0823f..74e5158d 100644 >> > --- a/dts/framework/config/__init__.py >> > +++ b/dts/framework/config/__init__.py >> > @@ -72,6 +72,21 @@ class HugepageConfiguration: >> > force_first_numa: bool >> > >> > >> > +@dataclass(slots=3DTrue, frozen=3DTrue) >> > +class PortConfig: >> > + id: int >> >> I removed id in my latest local version as I didn't see a real use for i= t. >> >> > >> > + node: str >> > + pci: str >> > + dpdk_os_driver: str >> > + os_driver: str >> > + peer_node: str >> > + peer_pci: str >> > + >> > + @staticmethod >> > + def from_dict(id: int, node: str, d: dict) -> "PortConfig": >> > + return PortConfig(id=3Did, node=3Dnode, **d) >> >> This would need to be changed if we remove the id. >> > > That makes sense, I don't really see a need for it either so I will also = remove it. > >> >> > + >> > + >> > @dataclass(slots=3DTrue, frozen=3DTrue) >> > class NodeConfiguration: >> > name: str >> > @@ -84,6 +99,7 @@ class NodeConfiguration: >> > use_first_core: bool >> > memory_channels: int >> > hugepages: HugepageConfiguration | None >> > + ports: list[PortConfig] >> > >> > @staticmethod >> > def from_dict(d: dict) -> "NodeConfiguration": >> > @@ -92,18 +108,46 @@ def from_dict(d: dict) -> "NodeConfiguration": >> > if "force_first_numa" not in hugepage_config: >> > hugepage_config["force_first_numa"] =3D False >> > hugepage_config =3D HugepageConfiguration(**hugepage_conf= ig) >> > + common_config =3D { >> > + "name": d["name"], >> > + "hostname": d["hostname"], >> > + "user": d["user"], >> > + "password": d.get("password"), >> > + "arch": Architecture(d["arch"]), >> > + "os": OS(d["os"]), >> > + "lcores": d.get("lcores", "1"), >> > + "use_first_core": d.get("use_first_core", False), >> > + "memory_channels": d.get("memory_channels", 1), >> > + "hugepages": hugepage_config, >> > + "ports": [ >> > + PortConfig.from_dict(i, d["name"], port) >> > + for i, port in enumerate(d["ports"]) >> > + ], >> >> This also needs to be modified when removing PortConfig.id: >> "ports": [PortConfig.from_dict(d["name"], port) for port in d["ports"]], >> >> > >> > + } >> > + >> > + return NodeConfiguration(**common_config) >> > + >> > + >> > +@dataclass(slots=3DTrue) >> > +class NodeVersionInfo: >> >> This looks like we could use it for all sorts of things in the future, s= o I'd just call it NodeInfo. >> > > > I agree that it could be used for a lot of things, I think I just added t= he version so it would be more specific but in hindsight I agree that this = could be shortened and used for multiple things. > >> >> > >> > + """Class to hold important versions within the node. >> > + >> > + This class, unlike the NodeConfiguration class, cannot be generat= ed at the start. >> > + This is because we need to initialize a connection with the node = before we can >> > + collet the information needed in this class. Therefore, it cannot= be a part of >> >> Typo: collect >> > > Oops, good catch. > >> >> > + the configuration class above. >> > + """ >> > >> > - return NodeConfiguration( >> > - name=3Dd["name"], >> > - hostname=3Dd["hostname"], >> > - user=3Dd["user"], >> > - password=3Dd.get("password"), >> > - arch=3DArchitecture(d["arch"]), >> > - os=3DOS(d["os"]), >> > - lcores=3Dd.get("lcores", "1"), >> > - use_first_core=3Dd.get("use_first_core", False), >> > - memory_channels=3Dd.get("memory_channels", 1), >> > - hugepages=3Dhugepage_config, >> > + os_name: str >> > + os_version: str >> > + kernel_version: str >> > + >> > + @staticmethod >> > + def from_dict(d: dict): >> > + return NodeVersionInfo( >> > + os_name=3Dd["os_name"], >> > + os_version=3Dd["os_version"], >> > + kernel_version=3Dd["kernel_version"], >> > ) >> > >> > >> > @@ -128,6 +172,24 @@ def from_dict(d: dict) -> "BuildTargetConfigurati= on": >> > ) >> > >> > >> > +@dataclass(slots=3DTrue) >> > +class BuildTargetVersionInfo: >> > + """Class to hold important versions within the build target. >> > + >> > + This is very similar to the NodeVersionInfo class, it just instea= d holds information >> > + for the build target. >> > + """ >> > + >> > + dpdk_version: str >> > + compiler_version: str >> > + >> > + @staticmethod >> > + def from_dict(d: dict): >> > + return BuildTargetVersionInfo( >> > + dpdk_version=3Dd["dpdk_version"], compiler_version=3Dd["c= ompiler_version"] >> > + ) >> > + >> >> Do you expect we'll be storing more fields than just these two (if so, t= hen we can again rename the class to BuildTargetInfo)? If it's just these t= wo, the extra class may not help that much (we could pass around a tuple). >> >> On the other hand, there's not really a reason to not have this class. I= could see us adding to it and I like BuildTargetInfo because of that. > > > I figured that it could be useful for storing any extra information in th= e future and like you said, does no harm so I made it a class. I think keep= ing it a class just makes it easier to extend it in the future but I'll ren= ame it like the above info class. > >> >> >> > >> > + >> > class TestSuiteConfigDict(TypedDict): >> > suite: str >> > cases: list[str] >> > @@ -157,6 +219,8 @@ class ExecutionConfiguration: >> > func: bool >> > test_suites: list[TestSuiteConfig] >> > system_under_test: NodeConfiguration >> > + vdevs: list[str] >> > + skip_smoke_tests: bool >> >> This should be added to conf.yaml to demonstrate the availability of the= option. > > > Very good point, it would be hard for people to know it was an option oth= erwise. > >> >> > >> > >> > @staticmethod >> > def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration= ": >> > @@ -167,14 +231,18 @@ def from_dict(d: dict, node_map: dict) -> "Execu= tionConfiguration": >> > map(TestSuiteConfig.from_dict, d["test_suites"]) >> > ) >> > sut_name =3D d["system_under_test"] >> > + list_of_vdevs =3D d["vdevs"] >> > + skip_smoke_tests =3D d["skip_smoke_tests"] if "skip_smoke_tes= ts" in d else False >> >> Just as with hugepages, we could skip the else branch if we set the defa= ult of skip_smoke_tests to False. I'm not sure which is the better approach= . > > > I am unsure of which is better and it doesn't make a massive difference, = but I will change this to a .get() call where the default is false to more = closely align with the huge pages configuration. That seems to be the best option. > >> >> >> > >> > assert sut_name in node_map, f"Unknown SUT {sut_name} in exec= ution {d}" >> > >> > return ExecutionConfiguration( >> > build_targets=3Dbuild_targets, >> > perf=3Dd["perf"], >> > func=3Dd["func"], >> > + skip_smoke_tests=3Dskip_smoke_tests, >> > test_suites=3Dtest_suites, >> > system_under_test=3Dnode_map[sut_name], >> > + vdevs=3Dlist_of_vdevs, >> > ) >> > >> > >> > @@ -221,3 +289,27 @@ def load_config() -> Configuration: >> > >> > >> > CONFIGURATION =3D load_config() >> > + >> > + >> > +@unique >> > +class InteractiveApp(Enum): >> > + """An enum that represents different supported interactive applic= ations >> > + >> > + The values in this enum must all be set to objects that have a ke= y called >> > + "default_path" where "default_path" represents an array for the p= ath to the >> > + application. This default path will be passed into the handler cl= ass for the >> > + application so that it can start the application. For every key o= ther than >> > + the default shell option, the path will be appended to the path t= o the DPDK >> > + build directory for the current SUT node. >> > + """ >> > + >> > + shell =3D {"default_path": [""]} >> > + testpmd =3D {"default_path": ["app", "dpdk-testpmd"]} >> >> The stored path could be a PurePath object. It'll be instantiated as an = OS-specific path, but main_session.join_remote_path will convert it properl= y. >> > > I actually was thinking about this because I saw it done when I was looki= ng through a different snippet of DTS code in one of your patches I believe= and I realized it would be a better approach. I much prefer using PurePath= here just because that is more clear and feels cleaner so I will make this= change. > >> >> > + >> > + def get_path(self): >> > + """A method for getting the default paths of an application >> > + >> > + Returns: >> > + String array that represents an OS agnostic path to the a= pplication. >> > + """ >> > + return self.value["default_path"] >> > diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framewor= k/config/conf_yaml_schema.json >> > index ca2d4a1e..3f7c301a 100644 >> > --- a/dts/framework/config/conf_yaml_schema.json >> > +++ b/dts/framework/config/conf_yaml_schema.json >> > @@ -6,6 +6,76 @@ >> > "type": "string", >> > "description": "A unique identifier for a node" >> > }, >> > + "NIC": { >> > + "type": "string", >> > + "enum": [ >> > + "ALL", >> > + "ConnectX3_MT4103", >> > + "ConnectX4_LX_MT4117", >> > + "ConnectX4_MT4115", >> > + "ConnectX5_MT4119", >> > + "ConnectX5_MT4121", >> > + "I40E_10G-10G_BASE_T_BC", >> > + "I40E_10G-10G_BASE_T_X722", >> > + "I40E_10G-SFP_X722", >> > + "I40E_10G-SFP_XL710", >> > + "I40E_10G-X722_A0", >> > + "I40E_1G-1G_BASE_T_X722", >> > + "I40E_25G-25G_SFP28", >> > + "I40E_40G-QSFP_A", >> > + "I40E_40G-QSFP_B", >> > + "IAVF-ADAPTIVE_VF", >> > + "IAVF-VF", >> > + "IAVF_10G-X722_VF", >> > + "ICE_100G-E810C_QSFP", >> > + "ICE_25G-E810C_SFP", >> > + "ICE_25G-E810_XXV_SFP", >> > + "IGB-I350_VF", >> > + "IGB_1G-82540EM", >> > + "IGB_1G-82545EM_COPPER", >> > + "IGB_1G-82571EB_COPPER", >> > + "IGB_1G-82574L", >> > + "IGB_1G-82576", >> > + "IGB_1G-82576_QUAD_COPPER", >> > + "IGB_1G-82576_QUAD_COPPER_ET2", >> > + "IGB_1G-82580_COPPER", >> > + "IGB_1G-I210_COPPER", >> > + "IGB_1G-I350_COPPER", >> > + "IGB_1G-I354_SGMII", >> > + "IGB_1G-PCH_LPTLP_I218_LM", >> > + "IGB_1G-PCH_LPTLP_I218_V", >> > + "IGB_1G-PCH_LPT_I217_LM", >> > + "IGB_1G-PCH_LPT_I217_V", >> > + "IGB_2.5G-I354_BACKPLANE_2_5GBPS", >> > + "IGC-I225_LM", >> > + "IGC-I226_LM", >> > + "IXGBE_10G-82599_SFP", >> > + "IXGBE_10G-82599_SFP_SF_QP", >> > + "IXGBE_10G-82599_T3_LOM", >> > + "IXGBE_10G-82599_VF", >> > + "IXGBE_10G-X540T", >> > + "IXGBE_10G-X540_VF", >> > + "IXGBE_10G-X550EM_A_SFP", >> > + "IXGBE_10G-X550EM_X_10G_T", >> > + "IXGBE_10G-X550EM_X_SFP", >> > + "IXGBE_10G-X550EM_X_VF", >> > + "IXGBE_10G-X550T", >> > + "IXGBE_10G-X550_VF", >> > + "brcm_57414", >> > + "brcm_P2100G", >> > + "cavium_0011", >> > + "cavium_a034", >> > + "cavium_a063", >> > + "cavium_a064", >> > + "fastlinq_ql41000", >> > + "fastlinq_ql41000_vf", >> > + "fastlinq_ql45000", >> > + "fastlinq_ql45000_vf", >> > + "hi1822", >> > + "virtio" >> > + ] >> > + }, >> > + >> >> All these NICs may be overkill, do we want to trim them? >> > > > I think in general that the more we have the better to make it more unive= rsally usable. If a NIC isn't supported by DTS anymore we could pull it out= but I don't see a problem with maintaining a list that has all supported N= ICs even if it does end up being long. > The broader question is what does it mean that a NIC is supported in DTS? That's a question we should address in the CI/DTS call and in the meantime, we could just leave the list as is. >> >> > >> > "ARCH": { >> > "type": "string", >> > "enum": [ >> > @@ -94,10 +164,24 @@ >> > "amount" >> > ] >> > }, >> > + "pci_address": { >> > + "type": "string", >> > + "pattern": "^[\\da-fA-F]{4}:[\\da-fA-F]{2}:[\\da-fA-F]{2}.\\d:?= \\w*$" >> > + }, >> > + "port_peer_address": { >> > + "description": "Peer is a TRex port, and IXIA port or a PCI add= ress", >> > + "oneOf": [ >> > + { >> > + "description": "PCI peer port", >> > + "$ref": "#/definitions/pci_address" >> > + } >> > + ] >> > + }, >> > "test_suite": { >> > "type": "string", >> > "enum": [ >> > - "hello_world" >> > + "hello_world", >> > + "smoke_tests" >> >> Is there a reason to provide both this and the execution.skip_smoke_test= s option? If we provide both, we may need to think about what happens when = both are present. I'd say it's best for everyone (users and devs) to have o= nly one option and I'd lean towards execution.skip_smoke_tests. > > > Good catch, I meant to remove this. In theory if you added it in both pla= ces it wouldn't cause too much of an issue, you would just be running smoke= tests twice in your execution. I will remove it anyway though. > >> >> > ] >> > }, >> > "test_target": { >> > @@ -165,6 +249,44 @@ >> > }, >> > "hugepages": { >> > "$ref": "#/definitions/hugepages" >> > + }, >> > + "ports": { >> > + "type": "array", >> > + "items": { >> > + "type": "object", >> > + "description": "Each port should be described on both s= ides of the connection. This makes configuration slightly more verbose but = greatly simplifies implementation. If there are an inconsistencies, then DT= S will not run until that issue is fixed. An example inconsistency would be= port 1, node 1 says it is connected to port 1, node 2, but port 1, node 2 = says it is connected to port 2, node 1.", >> > + "properties": { >> > + "pci": { >> > + "$ref": "#/definitions/pci_address", >> > + "description": "The local PCI address of the port" >> > + }, >> > + "dpdk_os_driver": { >> > + "type": "string", >> > + "description": "The driver that the kernel should b= ind this device to for DPDK to use it. (ex: vfio-pci)" >> > + }, >> > + "os_driver": { >> > + "type": "string", >> > + "description": "The driver normally used by this po= rt (ex: i40e)" >> > + }, >> > + "peer_node": { >> > + "type": "string", >> > + "description": "The name of the node the peer port = is on" >> > + }, >> > + "peer_pci": { >> > + "$ref": "#/definitions/pci_address", >> > + "description": "The PCI address of the peer port" >> > + } >> > + }, >> > + "additionalProperties": false, >> > + "required": [ >> > + "pci", >> > + "dpdk_os_driver", >> > + "os_driver", >> > + "peer_node", >> > + "peer_pci" >> > + ] >> > + }, >> > + "minimum": 1 >> > } >> > }, >> > "additionalProperties": false, >> > @@ -211,6 +333,17 @@ >> > ] >> > } >> > }, >> > + "vdevs": { >> > + "description": "Names of vdevs to be used in execution", >> > + "type": "array", >> > + "items": { >> > + "type":"string" >> >> Missing space before "string". >> >> > + } >> > + }, >> > + "skip_smoke_tests": { >> > + "description": "Optional field that allows you to skip sm= oke testing", >> > + "type":"boolean" >> >> Also the missing space. > > > Good call, I'll correct these. > >> >> >> > + }, >> > "system_under_test": { >> > "$ref": "#/definitions/node_name" >> > } >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py >> > index 05022845..f76c9ceb 100644 >> > --- a/dts/framework/dts.py >> > +++ b/dts/framework/dts.py >> > @@ -5,7 +5,13 @@ >> > >> > import sys >> > >> > -from .config import CONFIGURATION, BuildTargetConfiguration, Executio= nConfiguration >> > +from .config import ( >> > + CONFIGURATION, >> > + BuildTargetConfiguration, >> > + ExecutionConfiguration, >> > + TestSuiteConfig, >> > +) >> > +from .exception import BlockingTestSuiteError >> > from .logger import DTSLOG, getLogger >> > from .test_result import BuildTargetResult, DTSResult, ExecutionResul= t, Result >> > from .test_suite import get_test_suites >> > @@ -82,7 +88,9 @@ def _run_execution( >> > running all build targets in the given execution. >> > """ >> > dts_logger.info(f"Running execution with SUT '{execution.system_u= nder_test.name}'.") >> > - execution_result =3D result.add_execution(sut_node.config) >> > + execution_result =3D result.add_execution( >> > + sut_node.config, sut_node.get_node_versions() >> > + ) >> > >> > try: >> > sut_node.set_up_execution(execution) >> > @@ -118,14 +126,17 @@ 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.add_build_target_versions( >> > + sut_node.get_build_target_versions() >> > + ) >> > build_target_result.update_setup(Result.PASS) >> > except Exception as e: >> > dts_logger.exception("Build target setup failed.") >> > build_target_result.update_setup(Result.FAIL, e) >> > >> > else: >> > - _run_suites(sut_node, execution, build_target_result) >> > + _run_all_suites(sut_node, execution, build_target_result) >> > >> > finally: >> > try: >> > @@ -136,7 +147,46 @@ def _run_build_target( >> > build_target_result.update_teardown(Result.FAIL, e) >> > >> > >> > -def _run_suites( >> > +def _run_single_suite( >> > + sut_node: SutNode, >> > + execution: ExecutionConfiguration, >> > + build_target_result: BuildTargetResult, >> > + test_suite_config: TestSuiteConfig, >> > +) -> None: >> > + """Runs a single test suite. >> > + >> > + Args: >> > + sut_node: Node to run tests on. >> > + execution: Execution the test case belongs to. >> > + build_target_result: Build target configuration test case is = run on >> > + test_suite_config: Test suite configuration >> > + >> > + Raises: >> > + BlockingTestSuiteError: If a test suite that was marked as bl= ocking fails. >> > + """ >> > + try: >> > + full_suite_path =3D f"tests.TestSuite_{test_suite_config.test= _suite}" >> > + test_suite_classes =3D get_test_suites(full_suite_path) >> > + suites_str =3D ", ".join((x.__name__ for x in test_suite_clas= ses)) >> > + dts_logger.debug(f"Found test suites '{suites_str}' in '{full= _suite_path}'.") >> > + except Exception as e: >> > + dts_logger.exception("An error occurred when searching for te= st suites.") >> > + result.update_setup(Result.ERROR, e) >> > + >> > + else: >> > + for test_suite_class in test_suite_classes: >> > + 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() >> > + >> > + >> > +def _run_all_suites( >> > sut_node: SutNode, >> > execution: ExecutionConfiguration, >> > build_target_result: BuildTargetResult, >> > @@ -146,27 +196,32 @@ def _run_suites( >> > with possibly only a subset of test cases. >> > If no subset is specified, run all test cases. >> > """ >> > + end_build_target =3D False >> > + smoke_test_config =3D TestSuiteConfig.from_dict("smoke_tests") >> > + if not execution.skip_smoke_tests: >> > + try: >> > + _run_single_suite( >> > + sut_node, execution, build_target_result, smoke_test_= config >> > + ) >> > + except BlockingTestSuiteError as e: >> > + dts_logger.exception("Smoke tests failed, stopping build = target execution.") >> > + result.add_error(e) >> > + return >> > for test_suite_config in execution.test_suites: >> > try: >> > - full_suite_path =3D f"tests.TestSuite_{test_suite_config.= test_suite}" >> > - test_suite_classes =3D get_test_suites(full_suite_path) >> > - suites_str =3D ", ".join((x.__name__ for x in test_suite_= classes)) >> > - dts_logger.debug( >> > - f"Found test suites '{suites_str}' in '{full_suite_pa= th}'." >> > + _run_single_suite( >> > + sut_node, execution, build_target_result, test_suite_= config >> > ) >> > - except Exception as e: >> > - dts_logger.exception("An error occurred when searching fo= r test suites.") >> > - result.update_setup(Result.ERROR, e) >> > - >> > - else: >> > - for test_suite_class in test_suite_classes: >> > - test_suite =3D test_suite_class( >> > - sut_node, >> > - test_suite_config.test_cases, >> > - execution.func, >> > - build_target_result, >> > - ) >> > - test_suite.run() >> > + except BlockingTestSuiteError as e: >> > + dts_logger.exception( >> > + f"An error occurred within {test_suite_config.test_su= ite}. " >> > + "Skipping build target..." >> > + ) >> > + result.add_error(e) >> > + end_build_target =3D True >> > + # if a blocking test failed and we need to bail out of suite = executions >> > + if end_build_target: >> > + break >> > >> >> This doesn't look great to me. I think we could just add a list[TestSuit= eConfig] parameter to _run_suites. We could then just pass a list with/with= out smoke TestSuiteConfig, something like: >> test_suites =3D execution.test_suites >> if not execution.skip_smoke_tests: >> test_suites[:0] =3D [TestSuiteConfig.from_dict("smoke_tests")] >> _run_suites(sut_node, execution, build_target_result, test_suites) >> >> A note on method ordering: >> _run_single_suite before _run_all_suites was confusing to me, as the met= hods are used in the reverse order - _run_build_target calls _run_all_suite= s which only then calls _run_single_suite. I'd like the methods to be order= ed in the order they're used in the code. > > > I agree this was ugly, that's a good way to handle it and I'll migrate to= just adding it to the front of the list. I think I can actually just use t= he execution.test_suites array directly that is already being used and just= append it to the front of that. This way the list of test suites in the ex= ecution configuration is accurate to what actually ran. I don't recall why = this happened with the method ordering either but I'll get that corrected a= s well. I'm thinking it'll look something more like this: The example is missing, but not needed, just prepend it to the list if possible - my snippet operated under the assumption of config being a frozen dataset, but that may not matter for mutable types. > >> >> > >> > def _exit_dts() -> None: >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py >> > index ca353d98..dfb12df4 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,14 @@ 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_TEST= SUITE_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/__init__.py b/dts/framework/= remote_session/__init__.py >> > index ee221503..04342b8d 100644 >> > --- a/dts/framework/remote_session/__init__.py >> > +++ b/dts/framework/remote_session/__init__.py >> > @@ -1,5 +1,6 @@ >> > # SPDX-License-Identifier: BSD-3-Clause >> > # Copyright(c) 2023 PANTHEON.tech s.r.o. >> > +# Copyright(c) 2022-2023 University of New Hampshire >> >> Should the year be just 2023? I'm not sure about these copyright stateme= nts. > > > I think just 2023 makes sense, I wasn't completely sure either but I was = trying to align more with older copyright statements throughout the code an= d wasn't sure why some others had a range. In anycase, just 2023 makes sens= e to me and it matches the one above so I have no problem making it that. 2022-2023 is there in files which were applied in 2022 and modified in 2023 (by the same author/organization). This would be the first modification by UNH, hence 2023 would make more sense to me. > >> >> > >> > """ >> > The package provides modules for managing remote connections to a rem= ote host (node), >> > @@ -17,7 +18,14 @@ >> > >> > from .linux_session import LinuxSession >> > from .os_session import OSSession >> > -from .remote import CommandResult, RemoteSession, SSHSession >> > +from .remote import ( >> > + CommandResult, >> > + InteractiveRemoteSession, >> > + InteractiveShell, >> > + RemoteSession, >> > + SSHSession, >> > + TestPmdShell, >> > +) >> > >> > >> > def create_session( >> > diff --git a/dts/framework/remote_session/os_session.py b/dts/framewor= k/remote_session/os_session.py >> > index 4c48ae25..f5f53923 100644 >> > --- a/dts/framework/remote_session/os_session.py >> > +++ b/dts/framework/remote_session/os_session.py >> > @@ -12,7 +12,13 @@ >> > from framework.testbed_model import LogicalCore >> > from framework.utils import EnvVarsDict, MesonArgs >> > >> > -from .remote import CommandResult, RemoteSession, create_remote_sessi= on >> > +from .remote import ( >> > + CommandResult, >> > + InteractiveRemoteSession, >> > + RemoteSession, >> > + create_interactive_session, >> > + create_remote_session, >> > +) >> > >> > >> > class OSSession(ABC): >> > @@ -26,6 +32,7 @@ class OSSession(ABC): >> > name: str >> > _logger: DTSLOG >> > remote_session: RemoteSession >> > + interactive_session: InteractiveRemoteSession >> > >> > def __init__( >> > self, >> > @@ -37,6 +44,7 @@ def __init__( >> > self.name =3D name >> > self._logger =3D logger >> > self.remote_session =3D create_remote_session(node_config, na= me, logger) >> > + self.interactive_session =3D create_interactive_session(node_= config, name, logger) >> >> We may not want to create the interactive session at this point. This do= es create a connection to the node which we don't want (it is extra time co= nsumed) when creating an extra session on top of the main_session (with Nod= e.create_session). I think we could move this to OSSession.create_interacti= ve_shell. More below. > > > I think the idea of initializing it here was what we had discussed before= about having an open SSH session for interactive shells open in the backgr= ound throughout the entire run. If I understand what you're saying, you're = suggesting that we would only initialize this connection when we need it th= e first time and then leave it in the background afterwards. I can see how = this would be more efficient if you had a run where the interactive session= was never used, but it would add extra logic to make sure that the connect= ion is only initialized once and that it doesn't happen every time you crea= te an interactive shell. This is something that could be done, but consider= ing that this will always be initialized with smoke tests, the only way you= wouldn't have an interactive remote session created at the start is if you= disable smoke tests. I think it is easier to just have it running in the b= ackground rather than spawn it when it's used and have to worry about if a = connection currently exists or not. Right, with smoke tests almost always running, there may not be that much of an advantage in initializing it only when needed. On the other hand, the check could be very simple - the same thing we do with properties such as SutNode.os_name. >> >> >> > >> > def close(self, force: bool =3D False) -> None: >> > """ >> > @@ -173,3 +181,27 @@ def setup_hugepages(self, hugepage_amount: int, f= orce_first_numa: bool) -> None: >> > if needed and mount the hugepages if needed. >> > If force_first_numa is True, configure hugepages just on the = first socket. >> > """ >> > + >> > + @abstractmethod >> > + def get_compiler_version(self, compiler_name: str) -> str: >> > + """ >> > + Get installed version of compiler used for DPDK >> > + """ >> > + >> > + @abstractmethod >> > + def get_os_name(self) -> str: >> > + """ >> > + Get name of OS for the node >> > + """ >> > + >> > + @abstractmethod >> > + def get_os_version(self) -> str: >> > + """ >> > + Get version of OS for the node >> > + """ >> > + >> > + @abstractmethod >> > + def get_kernel_version(self) -> str: >> > + """ >> > + Get kernel version for the node >> > + """ >> >> These could be merged into one method (returning NodeVersionInfo), is th= ere a reason to separate them? We could get some performance improvements w= ith one method (e.g. getting /etc/os-release just once and then process loc= ally or we could use a script we execute remotely to have just one remote c= all, but that's something to think about in the future). > > > That's a good point. Originally I left them separate so that during the r= un you could just access what you need, but you can just access the data yo= u need from the object. > >> >> > diff --git a/dts/framework/remote_session/posix_session.py b/dts/frame= work/remote_session/posix_session.py >> > index d38062e8..dc6689f5 100644 >> > --- a/dts/framework/remote_session/posix_session.py >> > +++ b/dts/framework/remote_session/posix_session.py >> > @@ -219,3 +219,33 @@ def _remove_dpdk_runtime_dirs( >> > >> > def get_dpdk_file_prefix(self, dpdk_prefix) -> str: >> > return "" >> > + >> > + def get_compiler_version(self, compiler_name: str) -> str: >> > + match compiler_name: >> > + case "gcc": >> > + return self.send_command(f"{compiler_name} --version"= , 60).stdout.split( >> > + "\n" >> > + )[0] >> > + case "clang": >> > + return self.send_command(f"{compiler_name} --version"= , 60).stdout.split( >> > + "\n" >> > + )[0] >> > + case "msvc": >> > + return self.send_command("cl", 60).stdout >> > + case "icc": >> > + return self.send_command(f"{compiler_name} -V", 60).s= tdout >> > + case _: >> > + raise ValueError(f"Unknown compiler {compiler_name}") >> > + >> > + def get_os_name(self) -> str: >> > + return self.send_command( >> > + "awk -F=3D '$1=3D=3D\"NAME\" {print $2}' /etc/os-release"= , 60 >> > + ).stdout >> > + >> > + def get_os_version(self) -> str: >> > + return self.send_command( >> > + "awk -F=3D '$1=3D=3D\"VERSION\" {print $2}' /etc/os-relea= se", 60, True >> > + ).stdout >> > + >> > + def get_kernel_version(self) -> str: >> > + return self.send_command("uname -r", 60).stdout >> >> These 60 second timeouts seem high to me. All of these commands should e= xecute quickly so the default timeout seems fine to me. > > > That makes sense to me, no sense in making it too long. > >> >> >> > diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/fra= mework/remote_session/remote/__init__.py >> > index 8a151221..ef7b4851 100644 >> > --- a/dts/framework/remote_session/remote/__init__.py >> > +++ b/dts/framework/remote_session/remote/__init__.py >> > @@ -1,16 +1,28 @@ >> > # SPDX-License-Identifier: BSD-3-Clause >> > # Copyright(c) 2023 PANTHEON.tech s.r.o. >> > +# Copyright(c) 2022-2023 University of New Hampshire >> > >> > # pylama:ignore=3DW0611 >> > >> > +from paramiko import AutoAddPolicy, SSHClient >> > + >> > from framework.config import NodeConfiguration >> > from framework.logger import DTSLOG >> > >> > +from .interactive_remote_session import InteractiveRemoteSession >> > +from .interactive_shell import InteractiveShell >> > from .remote_session import CommandResult, RemoteSession >> > from .ssh_session import SSHSession >> > +from .testpmd_shell import TestPmdShell >> > >> > >> > 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 >> > +) -> InteractiveRemoteSession: >> > + return InteractiveRemoteSession(node_config, logger) >> > diff --git a/dts/framework/remote_session/remote/interactive_remote_se= ssion.py b/dts/framework/remote_session/remote/interactive_remote_session.p= y >> > new file mode 100644 >> > index 00000000..4e88308a >> > --- /dev/null >> > +++ b/dts/framework/remote_session/remote/interactive_remote_session.p= y >> > @@ -0,0 +1,113 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > +import socket >> > +import traceback >> > +from pathlib import PurePath >> > + >> > +from paramiko import AutoAddPolicy, SSHClient, Transport >> > +from paramiko.ssh_exception import ( >> > + AuthenticationException, >> > + BadHostKeyException, >> > + NoValidConnectionsError, >> > + SSHException, >> > +) >> > + >> > +from framework.config import InteractiveApp, NodeConfiguration >> > +from framework.exception import SSHConnectionError >> > +from framework.logger import DTSLOG >> > +from framework.settings import SETTINGS >> > + >> > +from .interactive_shell import InteractiveShell >> > +from .testpmd_shell import TestPmdShell >> > + >> > + >> > +class InteractiveRemoteSession: >> > + hostname: str >> > + ip: str >> > + port: int >> > + username: str >> > + password: str >> > + _logger: DTSLOG >> > + _node_config: NodeConfiguration >> > + session: SSHClient >> > + _transport: Transport | None >> > + >> > + def __init__(self, node_config: NodeConfiguration, _logger: DTSLO= G) -> None: >> > + self._node_config =3D node_config >> > + self._logger =3D _logger >> > + self.hostname =3D node_config.hostname >> > + self.username =3D node_config.user >> > + self.password =3D node_config.password if node_config.passwor= d else "" >> > + port =3D 22 >> >> framework/remote_session/remote/interactive_remote_session.py:45: error:= Incompatible types in assignment (expression has type "str", variable has = type "int") >> >> We can address this by making 22 a string (port =3D "22"). > > > I'll fix this as well. > >> >> >> > + self.ip =3D node_config.hostname >> > + if ":" in node_config.hostname: >> > + self.ip, port =3D node_config.hostname.split(":") >> > + self.port =3D int(port) >> > + self._logger.info( >> > + f"Initializing interactive connection for {self.username}= @{self.hostname}" >> > + ) >> > + self._connect() >> > + self._logger.info( >> > + f"Interactive connection successful for {self.username}@{= self.hostname}" >> > + ) >> > + >> > + def _connect(self) -> None: >> > + client =3D SSHClient() >> > + client.set_missing_host_key_policy(AutoAddPolicy) >> > + self.session =3D client >> > + retry_attempts =3D 10 >> > + for retry_attempt in range(retry_attempts): >> > + try: >> > + client.connect( >> > + self.ip, >> > + username=3Dself.username, >> > + port=3Dself.port, >> > + password=3Dself.password, >> > + timeout=3D20 if self.port else 10, >> > + ) >> > + except (TypeError, BadHostKeyException, AuthenticationExc= eption) as e: >> > + self._logger.exception(e) >> > + raise SSHConnectionError(self.hostname) from e >> > + except (NoValidConnectionsError, socket.error, SSHExcepti= on) as e: >> > + self._logger.debug(traceback.format_exc()) >> > + self._logger.warning(e) >> > + self._logger.info( >> > + "Retrying interactive session connection: " >> > + f"retry number {retry_attempt +1}" >> > + ) >> > + else: >> > + break >> > + else: >> > + raise SSHConnectionError(self.hostname) >> > + # Interactive sessions are used on an "as needed" basis so we= have >> > + # to set a keepalive >> > + self._transport =3D self.session.get_transport() >> > + if self._transport is not None: >> > + self._transport.set_keepalive(30) >> > + >> > + def create_interactive_shell( >> > + self, >> > + shell_type: InteractiveApp, >> > + path_to_app: PurePath, >> > + timeout: float =3D SETTINGS.timeout, >> > + eal_parameters: str =3D "", >> > + app_parameters=3D"", >> > + ) -> InteractiveShell: >> > + """ >> > + See "create_interactive_shell" in SutNode >> > + """ >> > + match (shell_type.name): >> > + case "shell": >> >> Comparing with the enum values seems better. > > > Good point, that does seem like it would be more consistent, I'll make th= e change. > >> >> >> > + return InteractiveShell( >> > + self.session, self._logger, path_to_app, timeout >> > + ) >> > + case "testpmd": >> > + return TestPmdShell( >> > + self.session, >> > + self._logger, >> > + path_to_app, >> > + timeout=3Dtimeout, >> > + eal_flags=3Deal_parameters, >> > + cmd_line_options=3Dapp_parameters, >> > + ) >> >> framework/remote_session/remote/interactive_remote_session.py:89: error:= Missing return statement >> >> This is probably because we don't have a case for the rest of the values= . > > > You're right, I didn't handle the default case. I'll change it to default= ing to just returning a base interactive shell with a message explaining th= at they didn't handle the type. > >> >> >> > diff --git a/dts/framework/remote_session/remote/interactive_shell.py = b/dts/framework/remote_session/remote/interactive_shell.py >> > new file mode 100644 >> > index 00000000..d98503be >> > --- /dev/null >> > +++ b/dts/framework/remote_session/remote/interactive_shell.py >> > @@ -0,0 +1,98 @@ >> > +from pathlib import PurePath >> > + >> > +from paramiko import Channel, SSHClient, channel >> > + >> > +from framework.logger import DTSLOG >> > +from framework.settings import SETTINGS >> > + >> > + >> > +class InteractiveShell: >> > + >> > + _interactive_session: SSHClient >> > + _stdin: channel.ChannelStdinFile >> > + _stdout: channel.ChannelFile >> > + _ssh_channel: Channel >> > + _logger: DTSLOG >> > + _timeout: float >> > + >> > + def __init__( >> > + self, >> > + interactive_session: SSHClient, >> > + logger: DTSLOG, >> > + path_to_app: PurePath | None =3D None, >> > + timeout: float =3D SETTINGS.timeout, >> > + ) -> None: >> > + self._interactive_session =3D interactive_session >> > + self._ssh_channel =3D self._interactive_session.invoke_shell(= ) >> > + self._stdin =3D self._ssh_channel.makefile_stdin("w") >> > + self._stdout =3D self._ssh_channel.makefile("r") >> > + self._ssh_channel.settimeout(timeout) >> > + self._ssh_channel.set_combine_stderr(True) # combines stdout= and stderr streams >> > + self._logger =3D logger >> > + self._timeout =3D timeout >> > + if path_to_app: >> > + self.send_command_no_output(str(path_to_app)) # starts t= he application >> >> This should be part of the derived class - the code to start an app is g= oing to be app-specific. Since this is an interactive session, the assumpti= on is that we're always going to start an app and then work with it, so sta= rting the app in the constructor should be safe. >> We actually have that for testpmd, so we just need to unify the above wi= th what we have. > > > That makes sense, the only reason I didn't do this before was because I w= as starting the application differently in test_pmd (with EAL flags and com= mand line options) so I would call it without this path when I called the s= uper init(). However, I can change this by just making a method called _sta= rt_application() that can be overridden in the subclasses and that will cle= an this process up quite a bit. > Good, I had something like that in mind. >> >> >> > + >> > + def empty_stdout_buffer(self) -> None: >> >> Same comment on ordering as above. >> >> > + """Removes all data from the stdout buffer. >> > + >> > + Because of the way paramiko handles read buffers, there is no= way to effectively >> > + remove data from, or "flush", read buffers. This method essen= tially moves our >> > + offset on the buffer to the end and thus "removes" the data f= rom the buffer. >> > + Timeouts are thrown on read operations of paramiko pipes base= d on whether data >> > + had been received before timeout so we assume that if we reac= h the timeout then >> > + we are at the end of the buffer. >> > + """ >> > + self._ssh_channel.settimeout(1) >> >> Waiting a whole second seems to be a lot. We actually may not need this = method from the use in the code - that is we change how the app starts. > > > We will still need this code whenever you send a command and don't get it= s output. What it is doing is essentially moving the pointer for the output= buffer to the end of the file which is needed because if you send a comman= d and you don't want any output, if we don't still move past the output in = the buffer then it will persist and bleed into when you send a command and = do want to collect output. Having this method allows you to know you are st= arting from an empty buffer when you are getting the output of the commands= . > Ah, I was commenting on send_command_no_output when I mentioned "this method", so I need to restate my point. We can do basically the same thing with "send_command" and the only difference I see is that we don't care about prompt in send_command_no_output. Is there a scenario where we need that? > In the case of timing however, I could cut this down to half a second, it= just gives the chance that a command that takes longer to produce its outp= ut will still contaminate the buffer. If this causes a problem we could alw= ays increase the time in the future. > The point is to not have any set time (since that's either too slow or unreliable), if we can avoid this. >> >> >> > + try: >> > + for line in self._stdout: >> > + pass >> > + except TimeoutError: >> > + pass >> > + self._ssh_channel.settimeout(self._timeout) # reset timeout >> > + >> > + def send_command_no_output(self, command: str) -> None: >> > + """Send command to channel without recording output. >> > + >> > + This method will not verify any input or output, it will simp= ly assume the >> > + command succeeded. This method will also consume all output i= n the buffer >> > + after executing the command. >> > + """ >> > + self._logger.info( >> > + f"Sending command {command.strip()} and not collecting ou= tput" >> > + ) >> > + self._stdin.write(f"{command}\n") >> > + self._stdin.flush() >> > + self.empty_stdout_buffer() >> > + >> > + def send_command_get_output(self, command: str, prompt: str) -> s= tr: >> > + """Send a command and get all output before the expected endi= ng string. >> > + >> > + 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 log int= o something >> > + with a username and password, you cannot expect "username:" b= ecause it won't >> > + yet be in the stdout buffer. A work around for this could be = consuming an >> > + extra newline character to force the current prompt into the = stdout buffer. >> > + >> > + Returns: >> > + All output in the buffer before expected string >> > + """ >> > + self._logger.info(f"Sending command {command.strip()}...") >> > + self._stdin.write(f"{command}\n") >> > + self._stdin.flush() >> > + out: str =3D "" >> > + for line in self._stdout: >> > + out +=3D line >> > + if prompt in line and not line.rstrip().endswith( >> > + command.rstrip() >> > + ): # ignore line that sent command >> > + break >> > + self._logger.debug(f"Got output: {out}") >> > + return out >> > + >> > + def close(self) -> None: >> > + self._stdin.close() >> > + self._ssh_channel.close() >> > + >> > + def __del__(self) -> None: >> > + self.close() >> > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dt= s/framework/remote_session/remote/testpmd_shell.py >> > new file mode 100644 >> > index 00000000..873f3c82 >> > --- /dev/null >> > +++ b/dts/framework/remote_session/remote/testpmd_shell.py >> > @@ -0,0 +1,58 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > + >> > +from pathlib import PurePath >> > + >> > +from paramiko import SSHClient >> > + >> > +from framework.logger import DTSLOG >> > +from framework.settings import SETTINGS >> > + >> > +from .interactive_shell import InteractiveShell >> > + >> > + >> > +class TestPmdShell(InteractiveShell): >> > + expected_prompt: str =3D "testpmd>" >> > + >> > + def __init__( >> > + self, >> > + interactive_session: SSHClient, >> > + logger: DTSLOG, >> > + path_to_testpmd: PurePath, >> > + eal_flags: str =3D "", >> > + cmd_line_options: str =3D "", >> > + timeout: float =3D SETTINGS.timeout, >> > + ) -> None: >> > + """Initializes an interactive testpmd session using specified= parameters.""" >> > + super(TestPmdShell, self).__init__( >> > + interactive_session, logger=3Dlogger, timeout=3Dtimeout >> > + ) >> > + self.send_command_get_output( >> > + f"{path_to_testpmd} {eal_flags} -- -i {cmd_line_options}\= n", >> > + self.expected_prompt, >> > + ) >> > + >> > + def send_command(self, command: str, prompt: str =3D expected_pro= mpt) -> 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.send_command_get_output(f"{command}\n", prompt) >> > + >> > + def get_devices(self) -> list[str]: >> > + """Get a list of device names that are known to testpmd >> > + >> > + Uses the device info listed in testpmd and then parses the ou= tput to >> > + return only the names of the devices. >> > + >> > + Returns: >> > + A list of strings representing device names (e.g. 0000:14= :00.1) >> > + """ >> > + dev_info: str =3D self.send_command("show device info all") >> > + dev_list: list[str] =3D [] >> > + for line in dev_info.split("\n"): >> > + if "device name:" in line.lower(): >> > + dev_list.append(line.strip().split(": ")[1].strip()) >> > + return dev_list >> >> This could be an object (or a list of TestPmdDevice object or something = similar). We could start with just the name and add to it in the future. > > > I think for the scope of smoke tests it would make sense to leave this as= just the names for now because all we are doing is making sure we can disc= over them and that they appear. I do agree that it could be useful to have = more info, but it would likely make more sense to add to it in the future w= hen we need that additional information. > The objects could contain just the name. I'd like to do this since we already do the same thing with VirtualDevice (in framework/testbed_model/hw/virtual_device.py). >> >> >> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.= py >> > index 74391982..029665fb 100644 >> > --- a/dts/framework/test_result.py >> > +++ b/dts/framework/test_result.py >> > @@ -1,5 +1,6 @@ >> > # SPDX-License-Identifier: BSD-3-Clause >> > # Copyright(c) 2023 PANTHEON.tech s.r.o. >> > +# Copyright(c) 2022-2023 University of New Hampshire >> > >> > """ >> > Generic result container and reporters >> > @@ -8,14 +9,17 @@ >> > import os.path >> > from collections.abc import MutableSequence >> > from enum import Enum, auto >> > +from typing import Dict >> >> No need, we can just use built-in dict (since Python3.9). >> >> > >> > from .config import ( >> > OS, >> > Architecture, >> > BuildTargetConfiguration, >> > + BuildTargetVersionInfo, >> > Compiler, >> > CPUType, >> > NodeConfiguration, >> > + NodeVersionInfo, >> > ) >> > from .exception import DTSError, ErrorSeverity >> > from .logger import DTSLOG >> > @@ -67,12 +71,14 @@ 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": >> > """ >> > @@ -206,6 +212,8 @@ class BuildTargetResult(BaseResult): >> > os: OS >> > cpu: CPUType >> > compiler: Compiler >> > + compiler_version: str | None >> > + dpdk_version: str | None >> > >> > def __init__(self, build_target: BuildTargetConfiguration): >> > super(BuildTargetResult, self).__init__() >> > @@ -213,6 +221,12 @@ def __init__(self, build_target: BuildTargetConfi= guration): >> > self.os =3D build_target.os >> > self.cpu =3D build_target.cpu >> > self.compiler =3D build_target.compiler >> > + self.compiler_version =3D None >> > + self.dpdk_version =3D None >> > + >> > + def add_build_target_versions(self, versions: BuildTargetVersionI= nfo) -> None: >> > + self.compiler_version =3D versions.compiler_version >> > + self.dpdk_version =3D versions.dpdk_version >> > >> > def add_test_suite(self, test_suite_name: str) -> TestSuiteResult= : >> > test_suite_result =3D TestSuiteResult(test_suite_name) >> > @@ -228,10 +242,17 @@ class ExecutionResult(BaseResult): >> > """ >> > >> > sut_node: NodeConfiguration >> > + sut_os_name: str >> > + sut_os_version: str >> > + sut_kernel_version: str >> > >> > - def __init__(self, sut_node: NodeConfiguration): >> > + def __init__(self, sut_node: NodeConfiguration, sut_version_info:= NodeVersionInfo): >> > super(ExecutionResult, self).__init__() >> > self.sut_node =3D sut_node >> > + self.sut_version_info =3D sut_version_info >> > + self.sut_os_name =3D sut_version_info.os_name >> > + self.sut_os_version =3D sut_version_info.os_version >> > + self.sut_kernel_version =3D sut_version_info.kernel_version >> > >> > def add_build_target( >> > self, build_target: BuildTargetConfiguration >> > @@ -258,6 +279,7 @@ class DTSResult(BaseResult): >> > """ >> > >> > dpdk_version: str | None >> > + output: dict | None >> > _logger: DTSLOG >> > _errors: list[Exception] >> > _return_code: ErrorSeverity >> > @@ -267,14 +289,17 @@ 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 >> > self._stats_result =3D None >> > self._stats_filename =3D os.path.join(SETTINGS.output_dir, "s= tatistics.txt") >> > >> > - def add_execution(self, sut_node: NodeConfiguration) -> Execution= Result: >> > - execution_result =3D ExecutionResult(sut_node) >> > + def add_execution( >> > + self, sut_node: NodeConfiguration, sut_version_info: NodeVers= ionInfo >> > + ) -> ExecutionResult: >> > + execution_result =3D ExecutionResult(sut_node, sut_version_in= fo) >> > self._inner_results.append(execution_result) >> > return execution_result >> > >> > @@ -296,7 +321,8 @@ 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 statis= tics >> > 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..ad66014c 100644 >> > --- a/dts/framework/test_suite.py >> > +++ b/dts/framework/test_suite.py >> > @@ -10,11 +10,24 @@ >> > import inspect >> > import re >> > from types import MethodType >> > +from typing import Dict >> > >> > -from .exception import ConfigurationError, SSHTimeoutError, TestCaseV= erifyError >> > +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, T= estSuiteResult >> > +from .test_result import ( >> > + BuildTargetResult, >> > + DTSResult, >> > + Result, >> > + TestCaseResult, >> > + TestSuiteResult, >> > +) >> > from .testbed_model import SutNode >> > >> > >> > @@ -37,10 +50,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 +63,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 +72,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.__cl= ass__.__name__) >> > + self.build_target_info =3D build_target_conf >> > + self._dts_result =3D dts_result >> > >> > def set_up_suite(self) -> None: >> > """ >> > @@ -118,6 +137,8 @@ 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_blockin= g: >> > + raise BlockingTestSuiteError(test_suite_name) >> > >> > def _execute_test_suite(self) -> None: >> > """ >> > @@ -232,6 +253,12 @@ def _execute_test_case( >> > test_case_result.update(Result.SKIP) >> > raise KeyboardInterrupt("Stop DTS") >> > >> > + def write_to_statistics_file(self, output: Dict[str, str]): >> >> This isn't used anywhere as far as I can tell. > > > It was previously but when we discussed removing the statistics output I = left this in but you're right, thinking about it now it doesn't make sense = to leave it in until we reformat statistics.txt. > >> >> >> > + if self._dts_result.output is not None: >> > + self._dts_result.output.update(output) >> > + else: >> > + self._dts_result.output =3D output >> > + >> >> This could be simplified if we init output to an empty dict in the const= ructor. >> >> > >> > def get_test_suites(testsuite_module_path: str) -> list[type[TestSuit= e]]: >> > def is_test_suite(object) -> bool: >> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testb= ed_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: Executi= onConfiguration) -> None: >> > """ >> > self._setup_hugepages() >> > self._set_up_execution(execution_config) >> > + self._execution_config =3D execution_config >> > >> > def _set_up_execution(self, execution_config: ExecutionConfigurat= ion) -> None: >> > """ >> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/t= estbed_model/sut_node.py >> > index 2b2b50d9..27849da7 100644 >> > --- a/dts/framework/testbed_model/sut_node.py >> > +++ b/dts/framework/testbed_model/sut_node.py >> > @@ -1,14 +1,21 @@ >> > # SPDX-License-Identifier: BSD-3-Clause >> > # Copyright(c) 2010-2014 Intel Corporation >> > # Copyright(c) 2023 PANTHEON.tech s.r.o. >> > +# Copyright(c) 2022-2023 University of New Hampshire >> > >> > import os >> > import tarfile >> > import time >> > from pathlib import PurePath >> > >> > -from framework.config import BuildTargetConfiguration, NodeConfigurat= ion >> > -from framework.remote_session import CommandResult, OSSession >> > +from framework.config import ( >> > + BuildTargetConfiguration, >> > + BuildTargetVersionInfo, >> > + InteractiveApp, >> > + NodeConfiguration, >> > + NodeVersionInfo, >> > +) >> > +from framework.remote_session import CommandResult, InteractiveShell,= OSSession >> > from framework.settings import SETTINGS >> > from framework.utils import EnvVarsDict, MesonArgs >> > >> > @@ -26,13 +33,17 @@ class SutNode(Node): >> > >> > _dpdk_prefix_list: list[str] >> > _dpdk_timestamp: str >> > - _build_target_config: BuildTargetConfiguration | None >> > + _build_target_config: BuildTargetConfiguration >> >> What's the reason for this change? My guess would be mypy, but the chang= e still results in mypy complaining: >> framework/testbed_model/sut_node.py:51: error: Incompatible types in ass= ignment (expression has type "None", variable has type "BuildTargetConfigur= ation") > > > I would guess it was for mypy as well which I got to be passing before I = submitted the patch, but if there is still an error with it then I will rev= ert. > >> >> >> >> > _env_vars: EnvVarsDict >> > _remote_tmp_dir: PurePath >> > __remote_dpdk_dir: PurePath | None >> > - _dpdk_version: str | None >> > _app_compile_timeout: float >> > _dpdk_kill_session: OSSession | None >> > + _dpdk_version: str | None >> > + _os_name: str | None >> > + _os_version: str | None >> > + _kernel_version: str | None >> > + _compiler_version: str | None >> > >> > def __init__(self, node_config: NodeConfiguration): >> > super(SutNode, self).__init__(node_config) >> > @@ -41,12 +52,16 @@ def __init__(self, node_config: NodeConfiguration)= : >> > self._env_vars =3D EnvVarsDict() >> > self._remote_tmp_dir =3D self.main_session.get_remote_tmp_dir= () >> > self.__remote_dpdk_dir =3D None >> > - self._dpdk_version =3D None >> > self._app_compile_timeout =3D 90 >> > self._dpdk_kill_session =3D None >> > self._dpdk_timestamp =3D ( >> > f"{str(os.getpid())}_{time.strftime('%Y%m%d%H%M%S', time.= localtime())}" >> > ) >> > + self._dpdk_version =3D None >> > + self._os_name =3D None >> > + self._os_version =3D None >> > + self._kernel_version =3D None >> > + self._compiler_version =3D None >> > >> > @property >> > def _remote_dpdk_dir(self) -> PurePath: >> > @@ -75,6 +90,44 @@ def dpdk_version(self) -> str: >> > ) >> > return self._dpdk_version >> > >> > + @property >> > + def os_name(self) -> str: >> > + if self._os_name is None: >> > + self._os_name =3D self.main_session.get_os_name() >> > + return self._os_name >> > + >> > + @property >> > + def os_version(self) -> str: >> > + if self._os_version is None: >> > + self._os_version =3D self.main_session.get_os_version() >> > + return self._os_version >> > + >> > + @property >> > + def kernel_version(self) -> str: >> > + if self._kernel_version is None: >> > + self._kernel_version =3D self.main_session.get_kernel_ver= sion() >> > + return self._kernel_version >> > + >> > + @property >> > + def compiler_version(self) -> str: >> > + if self._compiler_version is None: >> > + self._compiler_version =3D self.main_session.get_compiler= _version( >> > + self._build_target_config.compiler.name >> > + ) >> > + return self._compiler_version >> > + >> > + def get_node_versions(self) -> NodeVersionInfo: >> > + return NodeVersionInfo( >> > + os_name=3Dself.os_name, >> > + os_version=3Dself.os_version, >> > + kernel_version=3Dself.kernel_version, >> > + ) >> > + >> > + def get_build_target_versions(self) -> BuildTargetVersionInfo: >> > + return BuildTargetVersionInfo( >> > + dpdk_version=3Dself.dpdk_version, compiler_version=3Dself= .compiler_version >> > + ) >> > + >> > def _guess_dpdk_remote_dir(self) -> PurePath: >> > return self.main_session.guess_dpdk_remote_dir(self._remote_t= mp_dir) >> > >> > @@ -84,6 +137,10 @@ def _set_up_build_target( >> > """ >> > Setup DPDK on the SUT node. >> > """ >> > + # we want to ensure that dpdk_version and compiler_version is= reset for new >> > + # build targets >> > + self._dpdk_version =3D None >> > + self._compiler_version =3D None >> > self._configure_build_target(build_target_config) >> > self._copy_dpdk_tarball() >> > self._build_dpdk() >> > @@ -262,6 +319,49 @@ def run_dpdk_app( >> > f"{app_path} {eal_args}", timeout, verify=3DTrue >> > ) >> > >> > + def create_interactive_shell( >> > + self, >> > + shell_type: InteractiveApp, >> > + path_to_app: PurePath | None =3D None, >> >> Can we join shell_type and path_to_app together? Both contain the same t= ype of information. >> If we can't, let's drop path_to_app, as we don't provide (in this patch)= a way for users to modify this. We can add it later if needed. > > > The idea behind it was allowing users to specify where their application = is when they call this method. Users can pass a path into this application = if their application isn't at the default path which I think is a good addi= tion to have right now in the case of a user having their binary in a diffe= rent location. > As I mentioned in my comment, there isn't a way for users to pass the path (at least I don't see one). Users in this case is people who run DTS, not test case developers. Test cases shouldn't work with those paths which need to be framework-wide. I agree it would be good to have an option that users could configure and we can talk about how to do this, but I would be best to do in a different patch. >> >> >> > + timeout: float =3D SETTINGS.timeout, >> > + eal_parameters: str =3D "", >> >> Any reason we're not using the EalParameters class here? > > > I didn't use it originally because it requires an lcore list and memory c= hannels if you provide them. Although, I could use EalParameters and just m= ake them optional which would have the same effect but be more structured i= s what makes the most sense to me so I will make that change. The way to create EAL parameters is with the SutNode.create_eal_parameters method which addresses both of your points - there's no need to make it optional, as you can use self.create_eal_parameters() as the default value. > >> >> >> > + app_parameters=3D"", >> >> This is also currently not used, so maybe we should drop it and add it i= n the future, probably as a dedicated object representing the parameters. > > > That makes sense, you're right that a more structured class for providing= parameters would work better. The difficulty with a dedicated object howev= er is that you have different parameters for different applications so you = couldn't get very granular. Regardless, it can be removed from this patch f= or now and added back in the future. > I imagined an object with methods for adding different kinds of parameters - positional, without value, with value, long/short name etc. But that's not that relevant right now, we'll think about it when we actually need it (and the use case will make what everything clearer). >> >> >> > + ) -> InteractiveShell: >> >> This should return a Union of all the different sessions we support, oth= erwise mypy complains: >> tests/TestSuite_smoke_tests.py:68: error: Incompatible types in assignme= nt (expression has type "InteractiveShell", variable has type "TestPmdShell= ") >> > > I figured this would be allowed because they were subclasses and didn't r= ealize mypy would have a problem with it. I will fix this in the next versi= on. > >> >> > + """Create a handler for an interactive session. >> > + >> > + This method is a factory that calls a method in OSSession to = create shells for >> > + different DPDK applications. >> > + >> > + Args: >> > + shell_type: Enum value representing the desired applicati= on. >> > + path_to_app: Represents a path to the application you are= attempting to >> > + launch. This path will be executed at the start of th= e app >> > + initialization. If one isn't provided, the default sp= ecified in the >> > + enumeration will be used. >> > + timeout: Timeout for the ssh channel >> >> We should specify what sort of timeout this is - for creating the channe= l or for sent commands? > > > Very good point, this is pretty vague if you weren't the one who wrote it= . I'll make sure to clear this up. > >> >> >> > + eal_parameters: List of EAL parameters to use to launch t= he app. This is >> > + ignored for base "shell" types. >> > + app_parameters: Command-line flags to pass into the appli= cation on launch. >> > + Returns: >> > + Instance of the desired interactive application. >> > + """ >> > + default_path: PurePath | None >> > + # if we just want a default shell, there is no need to append= the DPDK build >> > + # directory to the path >> > + if shell_type.name =3D=3D "shell": >> > + default_path =3D None >> > + else: >> > + default_path =3D self.main_session.join_remote_path( >> > + self.remote_dpdk_build_dir, *shell_type.get_path() >> > + ) >> >> All this path handling looks clunky, it should be part of the Interactiv= eApp class. Would adding something like update_path to InteractiveApp work? > > > I agree this was a little messy, but after changing the default paths to = PurePaths, I could clean this up more by just defaulting to shell_type.get_= path() and then having a single if statement that checks if it isn't a shel= l type and, if not, set the default path with appending the build dir. it w= ould look something more like this: > > default_path =3D shell_type.get_path() > > if shell_type !=3D InteractiveApp.shell: > default_path =3D self.main_session.join_remote_path( > self.remote_dpdk_build_dir, shell_type.get_path() > ) > My point was that we're still passing two arguments (the app enum and the path) where we could be passing just one, I think. We could just update the path of InteractiveApp and pass only that: @unique class InteractiveApp(Enum): shell =3D [pathlib.PurePath("")] testpmd =3D [pathlib.PurePath("app", "dpdk-testpmd")] @property def path(self) -> pathlib.PurePath: return self.value[0] @path.setter def path(self, path: pathlib.PurePath): self.value[0] =3D path The above stores the default path with the option of updating it. That would be much more elegant than passing both the path and the Enum. A note on the implementation: the value has to be a mutable type, because we can't assign new values. With mutable types, we're modifying the existing value. Another important question, which I didn't think of before, is do we need the shell interactive app? It's not used anywhere and I don't think we'll ever use it, so let's remove it. We can add it later if we find an actual use for it. >> >> > + return self.main_session.interactive_session.create_interacti= ve_shell( >> > + shell_type, >> > + path_to_app or default_path, >> > + timeout, >> > + eal_parameters, >> > + app_parameters, >> > + ) >> > + >> > >> > class EalParameters(object): >> > def __init__( >> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_= smoke_tests.py >> > new file mode 100644 >> > index 00000000..a2fd9fae >> > --- /dev/null >> > +++ b/dts/tests/TestSuite_smoke_tests.py >> > @@ -0,0 +1,101 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > +from framework.config import InteractiveApp >> > +from framework.remote_session import TestPmdShell >> > +from framework.test_suite import TestSuite >> > + >> > + >> > +class SmokeTests(TestSuite): >> > + is_blocking =3D True >> > + # in this list, the first index is the address of the nic and the= second is >> > + # the driver for that nic. >> > + list_of_nics: list[tuple[str, str]] =3D [] >> >> Would using a dict make more sense? > > > I thought a tuple was a good option because of the two pieces of data but= I think a dict would also be useful because it would be more clear when it= is accessed later. I think turning this into a dict (and maybe migrating t= o an object later) is a good idea. > Now that you mentioned objects, I'd say let's just store the ports. I don't really see a reason for a dict or new object. >> >> >> > + >> > + def set_up_suite(self) -> None: >> > + """ >> > + Setup: >> > + build all DPDK >> >> This needs to be updated. > > > You're right, I guess I missed updating this. > >> >> > + """ >> > + self.dpdk_build_dir_path =3D self.sut_node.remote_dpdk_build_= dir >> > + for nic in self.sut_node.config.ports: >> > + new_tuple =3D (nic.pci, nic.os_driver.strip()) >> > + self.list_of_nics.append(new_tuple) >> >> It would be better to rename this based on what list of nics this is - t= he one we expect to find or the one we're testing? > > > Another spot that you're right, it is not nearly as clear if you weren't = the one who wrote it, I'll update this as well. > >> >> >> > + >> > + 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-t= ests", >> > + 300, >> > + verify=3DTrue, >> > + ) >> > + >> > + 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"--vdev {dev} " >> > + list_of_vdevs =3D list_of_vdevs[:-1] >> > + if list_of_vdevs: >> > + self._logger.info( >> > + "Running driver tests with the following virtual " >> > + f"devices: {list_of_vdevs}" >> > + ) >> > + self.sut_node.main_session.send_command( >> > + f"meson test -C {self.dpdk_build_dir_path} --suite dr= iver-tests " >> > + f'--test-args "{list_of_vdevs}"', >> > + 300, >> > + verify=3DTrue, >> > + ) >> > + else: >> > + self.sut_node.main_session.send_command( >> > + f"meson test -C {self.dpdk_build_dir_path} --suite dr= iver-tests", >> > + 300, >> > + verify=3DTrue, >> > + ) >> > + >> > + def test_devices_listed_in_testpmd(self) -> None: >> > + """ >> > + Test: >> > + Uses testpmd driver to verify that devices have been foun= d by testpmd >> > + """ >> >> Every other test description is written in imperative and with lowecase = first letter. Let's unite everything to imperative with uppercase first let= ter and properly ending the sentence with a dot. > > > Sounds good to me, I'll reformat them. > >> >> >> > + testpmd_driver: TestPmdShell =3D self.sut_node.create_interac= tive_shell( >> > + InteractiveApp.testpmd >> > + ) >> > + dev_list: list[str] =3D testpmd_driver.get_devices() >> > + for nic in self.list_of_nics: >> >> Here's where the more explicit naming would be beneficial - I had to thi= nk about what's what. >> >> > + self.verify( >> > + nic[0] in dev_list, >> > + f"Device {nic[0]} was not listed in testpmd's availab= le devices, " >> > + "please check your configuration", >> > + ) >> > + >> > + def test_device_bound_to_driver(self) -> None: >> > + """ >> > + Test: >> > + ensure that all drivers listed in the config are bound to= the correct driver >> > + """ >> > + path_to_dev =3D self.sut_node.main_session.join_remote_path( >> > + self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbin= d.py" >> > + ) >> > + for nic in self.list_of_nics: >> > + out =3D self.sut_node.main_session.send_command( >> > + f"{path_to_dev} --status | grep {nic[0]}", 60 >> > + ) >> >> This may be something we want to put into SutNode, something like get_de= vice_info. We'd only send the command once (not for every NIC as we're doin= g here) and parse the output. > > > I think similar to the above test for testpmd, it makes sense to isolate = the checking if the devices are bound to the right driver to smoke tests. J= ust because after smoke tests verifies that they are bound to the right dri= ver it would never be used in the SutNode again because you could then trus= t the config file. Also, if we were to later change this to be isolated to = only checking the NICs that are being used for this execution like we had m= entioned, you would want to isolate this to those nics that are actually be= ing used in the execution, not all of the NICs in the SutNode again so I th= ink keeping it in smoke tests makes more sense. > In the test we only care about the port being bound to the right driver. I was thinking more broadly - execute the command once and store all of the information we can. With the dpdk-devbind script, there's not much to store, but we don't have to use that. With that in mind, let's not bloat the patch any more. We can do the more generic implementation later. > I do agree though that this can be done in only one call to dev-bind whic= h I wasn't thinking about when writing it and that I can refactor for the n= ext version of the patch. > >> >> >> > + self.verify( >> > + len(out.stdout) !=3D 0, >> > + f"Failed to find configured device ({nic[0]}) using d= pdk-devbind.py", >> > + ) >> > + for string in out.stdout.split(" "): >> > + if "drv=3D" in string: >> > + self.verify( >> > + string.split("=3D")[1] =3D=3D nic[1], >> > + f"Driver for device {nic[0]} does not match d= river listed in " >> > + f'configuration (bound to {string.split("=3D"= )[1]})', >> > + ) >> > -- >> > 2.41.0 >> > > > > Thank you for the feedback, it's always very helpful. > -Jeremy