[PATCH v1 0/3] dts: rework topology definition in dts config

[PATCH v1 0/3] dts: rework topology definition in dts config

[Nicholas Pratte]

Series to rework how testbed topologies are defined within the dts
configuration. Bugzilla ID 1360 originally asked for a rework to
define physical NIC devices on nodes. However, defining testbeds
purely by device negates the ability to create specific, multi-device
testers running performance tests, for example. We could keep the device
design specified, but adding in this flexibility might make
configuration more puzzling and complicated than necessary. Instead, it
may make sense to use unique identifiers on ports/interfaces, and have
DTS users select ports based on these identifiers within test run
configuration; that is what this series achieves.

Nicholas Pratte (3):
  dts: rework port attributes in config module
  dts: rework testbed_model Port objects to contain unique identifiers
  dts: rework test suite and dts runner to include test_run configs

 dts/conf.yaml                              | 32 ++++++-------
 dts/framework/config/__init__.py           | 12 +++--
 dts/framework/config/conf_yaml_schema.json | 52 +++++++++++++---------
 dts/framework/config/types.py              | 19 +++++---
 dts/framework/runner.py                    | 16 +++++--
 dts/framework/test_suite.py                | 33 +++++++++-----
 dts/framework/testbed_model/port.py        | 45 +++++--------------
 7 files changed, 113 insertions(+), 96 deletions(-)

-- 
2.44.0

[PATCH v1 1/3] dts: rework port attributes in config module

[Nicholas Pratte]

The current design requires that a peer pci port is identified so that
test suites can create the correct port links. While this can work, it
also creates a lot of room for user error. Instead, devices should be
given a unique identifier which is referenced in defined test runs.

Both defined testbeds for the SUT and TG must have an equal number of
specified ports. In each given array or ports, SUT port 0 is connected
to TG port 0, SUT port 1 is connected to TG port 1, etc.

Bugzilla ID: 1478

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/conf.yaml                              | 32 ++++++-------
 dts/framework/config/__init__.py           | 12 +++--
 dts/framework/config/conf_yaml_schema.json | 52 +++++++++++++---------
 dts/framework/config/types.py              | 19 +++++---
 4 files changed, 69 insertions(+), 46 deletions(-)

diff --git a/dts/conf.yaml b/dts/conf.yaml
index 7d95016e68..16214ee267 100644
--- a/dts/conf.yaml
+++ b/dts/conf.yaml
@@ -20,10 +20,17 @@ test_runs:
     # The machine running the DPDK test executable
     system_under_test_node:
       node_name: "SUT 1"
+      test_bed:
+        - "Intel SUT Port 1"
+        - "Intel SUT Port 2"
       vdevs: # optional; if removed, vdevs won't be used in the test run
         - "crypto_openssl"
     # Traffic generator node to use for this test run
-    traffic_generator_node: "TG 1"
+    traffic_generator_node:
+      node_name: "TG 1"
+      test_bed:
+        - "Mellanox TG Port 1"
+        - "Broadcom TG Port 1"
 nodes:
   # Define a system under test node, having two network ports physically
   # connected to the corresponding ports in TG 1 (the peer node)
@@ -40,17 +47,14 @@ nodes:
         force_first_numa: false
     ports:
       # sets up the physical link between "SUT 1"@000:00:08.0 and "TG 1"@0000:00:08.0
-      - pci: "0000:00:08.0"
+      - name: "Intel SUT Port 1"
+        pci: "0000:00:08.0"
         os_driver_for_dpdk: vfio-pci # OS driver that DPDK will use
         os_driver: i40e              # OS driver to bind when the tests are not running
-        peer_node: "TG 1"
-        peer_pci: "0000:00:08.0"
-      # sets up the physical link between "SUT 1"@000:00:08.1 and "TG 1"@0000:00:08.1
-      - pci: "0000:00:08.1"
+      - name: "Intel SUT Port 2"
+        pci: "0000:00:08.1"
         os_driver_for_dpdk: vfio-pci
         os_driver: i40e
-        peer_node: "TG 1"
-        peer_pci: "0000:00:08.1"
   # Define a Scapy traffic generator node, having two network ports
   # physically connected to the corresponding ports in SUT 1 (the peer node).
   - name: "TG 1"
@@ -59,18 +63,14 @@ nodes:
     arch: x86_64
     os: linux
     ports:
-      # sets up the physical link between "TG 1"@000:00:08.0 and "SUT 1"@0000:00:08.0
-      - pci: "0000:00:08.0"
+      - name: "Mellanox TG Port 1"
+        pci: "0000:00:08.0"
         os_driver_for_dpdk: rdma
         os_driver: rdma
-        peer_node: "SUT 1"
-        peer_pci: "0000:00:08.0"
-      # sets up the physical link between "SUT 1"@000:00:08.0 and "TG 1"@0000:00:08.0
-      - pci: "0000:00:08.1"
+      - name: "Broadcom TG Port 1"
+        pci: "0000:00:08.1"
         os_driver_for_dpdk: rdma
         os_driver: rdma
-        peer_node: "SUT 1"
-        peer_pci: "0000:00:08.1"
     hugepages_2mb: # optional; if removed, will use system hugepage configuration
         number_of: 256
         force_first_numa: false
diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
index df60a5030e..534821ed22 100644
--- a/dts/framework/config/__init__.py
+++ b/dts/framework/config/__init__.py
@@ -151,11 +151,10 @@ class PortConfig:
     """
 
     node: str
+    name: str
     pci: str
     os_driver_for_dpdk: str
     os_driver: str
-    peer_node: str
-    peer_pci: str
 
     @classmethod
     def from_dict(cls, node: str, d: PortConfigDict) -> Self:
@@ -487,12 +486,19 @@ def from_dict(
             system_under_test_node, SutNodeConfiguration
         ), f"Invalid SUT configuration {system_under_test_node}"
 
-        tg_name = d["traffic_generator_node"]
+        tg_name = d["traffic_generator_node"]["node_name"]
         assert tg_name in node_map, f"Unknown TG {tg_name} in test run {d}"
         traffic_generator_node = node_map[tg_name]
         assert isinstance(
             traffic_generator_node, TGNodeConfiguration
         ), f"Invalid TG configuration {traffic_generator_node}"
+        assert len(traffic_generator_node.ports) == len(
+            system_under_test_node.ports
+        ), "Insufficient ports defined on nodes."
+        for port_name in d["system_under_test_node"]["test_bed"]:
+            assert port_name in {port.name: port for port in system_under_test_node.ports}
+        for port_name in d["traffic_generator_node"]["test_bed"]:
+            assert port_name in {port.name: port for port in traffic_generator_node.ports}
 
         vdevs = (
             d["system_under_test_node"]["vdevs"] if "vdevs" in d["system_under_test_node"] else []
diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
index f02a310bb5..91667b01cc 100644
--- a/dts/framework/config/conf_yaml_schema.json
+++ b/dts/framework/config/conf_yaml_schema.json
@@ -6,6 +6,10 @@
       "type": "string",
       "description": "A unique identifier for a node"
     },
+    "port_name": {
+      "type": "string",
+      "description": "A unique identifier for a node's NIC port."
+    },
     "NIC": {
       "type": "string",
       "enum": [
@@ -190,6 +194,24 @@
         "pmd_buffer_scatter"
       ]
     },
+    "test_run_node": {
+      "type": "object",
+      "properties": {
+        "node_name": {
+          "$ref": "#/definitions/node_name"
+        },
+        "test_bed": {
+          "type": "array",
+          "items": {
+            "$ref": "#/definitions/port_name"
+          }
+        }
+      },
+      "required": [
+        "node_name",
+        "test_bed"
+      ]
+    },
     "test_target": {
       "type": "object",
       "properties": {
@@ -260,8 +282,11 @@
             "type": "array",
             "items": {
               "type": "object",
-              "description": "Each port should be described on both sides of the connection. This makes configuration slightly more verbose but greatly simplifies implementation. If there are inconsistencies, then DTS 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.",
+              "description": "A list of relevant ports on a node and its corresponding driver, dpdk driver, and pci address.",
               "properties": {
+                "name": {
+                  "$ref": "#/definitions/port_name"
+                },
                 "pci": {
                   "$ref": "#/definitions/pci_address",
                   "description": "The local PCI address of the port"
@@ -273,23 +298,14 @@
                 "os_driver": {
                   "type": "string",
                   "description": "The driver normally used by this port (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": [
+                "name",
                 "pci",
                 "os_driver_for_dpdk",
-                "os_driver",
-                "peer_node",
-                "peer_pci"
+                "os_driver"
               ]
             },
             "minimum": 1
@@ -360,11 +376,7 @@
             "type": "boolean"
           },
           "system_under_test_node": {
-            "type":"object",
-            "properties": {
-              "node_name": {
-                "$ref": "#/definitions/node_name"
-              },
+              "$ref": "#/definitions/test_run_node",
               "vdevs": {
                 "description": "Optional list of names of vdevs to be used in the test run",
                 "type": "array",
@@ -372,13 +384,9 @@
                   "type": "string"
                 }
               }
-            },
-            "required": [
-              "node_name"
-            ]
           },
           "traffic_generator_node": {
-            "$ref": "#/definitions/node_name"
+            "$ref": "#/definitions/test_run_node"
           }
         },
         "additionalProperties": false,
diff --git a/dts/framework/config/types.py b/dts/framework/config/types.py
index cf16556403..c91f3b9009 100644
--- a/dts/framework/config/types.py
+++ b/dts/framework/config/types.py
@@ -14,16 +14,14 @@
 class PortConfigDict(TypedDict):
     """Allowed keys and values."""
 
+    #:
+    name: str
     #:
     pci: str
     #:
     os_driver_for_dpdk: str
     #:
     os_driver: str
-    #:
-    peer_node: str
-    #:
-    peer_pci: str
 
 
 class TrafficGeneratorConfigDict(TypedDict):
@@ -101,9 +99,20 @@ class TestRunSUTConfigDict(TypedDict):
     #:
     node_name: str
     #:
+    test_bed: list[str]
+    #:
     vdevs: list[str]
 
 
+class TestRunTGConfigDict(TypedDict):
+    """Allowed keys and values."""
+
+    #:
+    node_name: str
+    #:
+    test_bed: list[str]
+
+
 class TestRunConfigDict(TypedDict):
     """Allowed keys and values."""
 
@@ -120,7 +129,7 @@ class TestRunConfigDict(TypedDict):
     #:
     system_under_test_node: TestRunSUTConfigDict
     #:
-    traffic_generator_node: str
+    traffic_generator_node: TestRunTGConfigDict
 
 
 class ConfigurationDict(TypedDict):
-- 
2.44.0

Re: [PATCH v1 1/3] dts: rework port attributes in config module

[Jeremy Spewock]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
<snip>
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index df60a5030e..534821ed22 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -151,11 +151,10 @@ class PortConfig:
>      """
>
>      node: str
> +    name: str
>      pci: str
>      os_driver_for_dpdk: str
>      os_driver: str
> -    peer_node: str
> -    peer_pci: str
>
>      @classmethod
>      def from_dict(cls, node: str, d: PortConfigDict) -> Self:
> @@ -487,12 +486,19 @@ def from_dict(
>              system_under_test_node, SutNodeConfiguration
>          ), f"Invalid SUT configuration {system_under_test_node}"
>
> -        tg_name = d["traffic_generator_node"]
> +        tg_name = d["traffic_generator_node"]["node_name"]
>          assert tg_name in node_map, f"Unknown TG {tg_name} in test run {d}"
>          traffic_generator_node = node_map[tg_name]
>          assert isinstance(
>              traffic_generator_node, TGNodeConfiguration
>          ), f"Invalid TG configuration {traffic_generator_node}"
> +        assert len(traffic_generator_node.ports) == len(
> +            system_under_test_node.ports
> +        ), "Insufficient ports defined on nodes."

Do we want to assert that the list of ports on the tester is the same
as the number on the SUT, or would it be better here to assert that
the same number of SUT and TG ports  are present in the test_run?
There could be a case where a TG is the generator for multiple SUT
hosts and therefore maybe the TG has 4 ports, but each of the SUT
nodes only has 2 which would get flagged as invalid in this assertion.

Maybe something better to compare here is the length of
d["traffic_generator_node"]["test_bed"] and
d["system_under_test_node"]["test_bed"].

> +        for port_name in d["system_under_test_node"]["test_bed"]:
> +            assert port_name in {port.name: port for port in system_under_test_node.ports}

A little nit-picky, but you could achieve the same thing here using a
list rather than a dictionary here that might be a little easier to
read since we don't need the port info anyway:

assert port_name in [port.name for port in system_under_test_node.ports]


> +        for port_name in d["traffic_generator_node"]["test_bed"]:
> +            assert port_name in {port.name: port for port in traffic_generator_node.ports}
>
>          vdevs = (
>              d["system_under_test_node"]["vdevs"] if "vdevs" in d["system_under_test_node"] else []
<snip>
> 2.44.0
>

Re: [PATCH v1 1/3] dts: rework port attributes in config module

[Juraj Linkeš]

As a general note, it's possible we should wait with these changes for 
the Pydantic changes which could simplify a lot of what we want to do 
with the config (not just this, but also the split and removing excess 
attributes).

On 21. 8. 2024 20:43, Nicholas Pratte wrote:
> The current design requires that a peer pci port is identified so that
> test suites can create the correct port links. While this can work, it
> also creates a lot of room for user error. Instead, devices should be
> given a unique identifier which is referenced in defined test runs.
> 
> Both defined testbeds for the SUT and TG must have an equal number of
> specified ports. In each given array or ports, SUT port 0 is connected
> to TG port 0, SUT port 1 is connected to TG port 1, etc.
> 
> Bugzilla ID: 1478
> 
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
>   dts/conf.yaml                              | 32 ++++++-------
>   dts/framework/config/__init__.py           | 12 +++--
>   dts/framework/config/conf_yaml_schema.json | 52 +++++++++++++---------
>   dts/framework/config/types.py              | 19 +++++---
>   4 files changed, 69 insertions(+), 46 deletions(-)
> 
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index 7d95016e68..16214ee267 100644
> --- a/dts/conf.yaml
> +++ b/dts/conf.yaml
> @@ -20,10 +20,17 @@ test_runs:
>       # The machine running the DPDK test executable
>       system_under_test_node:
>         node_name: "SUT 1"
> +      test_bed:

test_bed is the whole thing we're testing. Let's find a better name. 
We're already using unique identifiers for SUTs and TGs and referencing 
those with node_name, so we could reference port identifiers with 
port_names.

> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index df60a5030e..534821ed22 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -151,11 +151,10 @@ class PortConfig:
>       """
>   
>       node: str
> +    name: str
>       pci: str
>       os_driver_for_dpdk: str
>       os_driver: str
> -    peer_node: str
> -    peer_pci: str
>   
>       @classmethod
>       def from_dict(cls, node: str, d: PortConfigDict) -> Self:
> @@ -487,12 +486,19 @@ def from_dict(
>               system_under_test_node, SutNodeConfiguration
>           ), f"Invalid SUT configuration {system_under_test_node}"
>   
> -        tg_name = d["traffic_generator_node"]
> +        tg_name = d["traffic_generator_node"]["node_name"]
>           assert tg_name in node_map, f"Unknown TG {tg_name} in test run {d}"
>           traffic_generator_node = node_map[tg_name]
>           assert isinstance(
>               traffic_generator_node, TGNodeConfiguration
>           ), f"Invalid TG configuration {traffic_generator_node}"
> +        assert len(traffic_generator_node.ports) == len(
> +            system_under_test_node.ports
> +        ), "Insufficient ports defined on nodes."

This checks the number of ports specified in the nodes section, but 
these don't necessarily have to correspond. Consider this scenario:

TG has four ports, two connected to SUT1, two connected to SUT2

With the above, we could have two different test run configurations, one 
with TG and SUT1 and the other with TG and SUT2.

We should check that the number of SUT/TG ports specified in test_run is 
the same. The message should also be more precise - something like "The 
number of ports on SUT/TG nodes specified in test_run is different.".

> +        for port_name in d["system_under_test_node"]["test_bed"]:
> +            assert port_name in {port.name: port for port in system_under_test_node.ports}

This is missing an error message. And we're also creating the dictionary 
for each port, the dictionary could be created before the cycle. Or we 
could just look at sets of ports of nodes and those specified in test_run:
sut_test_run_ports = {port_name for port_name in 
d["system_under_test_node"]["test_bed"]}
tg_test_run_ports = {port_name for port_name in 
d["traffic_generator_node"]["test_bed"]}

assert not sut_test_run_ports - {port.name for port in 
system_under_test_node.ports}, "err msg"
assert not tg_test_run_ports - {port.name for port in 
traffic_generator_node.ports}, "err msg"

We should also enforce that the port name are unique across the node config.

> +        for port_name in d["traffic_generator_node"]["test_bed"]:
> +            assert port_name in {port.name: port for port in traffic_generator_node.ports}
>   
>           vdevs = (
>               d["system_under_test_node"]["vdevs"] if "vdevs" in d["system_under_test_node"] else []
> diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
> index f02a310bb5..91667b01cc 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
> @@ -6,6 +6,10 @@
>         "type": "string",
>         "description": "A unique identifier for a node"
>       },
> +    "port_name": {
> +      "type": "string",
> +      "description": "A unique identifier for a node's NIC port."
> +    },
>       "NIC": {
>         "type": "string",
>         "enum": [
> @@ -190,6 +194,24 @@
>           "pmd_buffer_scatter"
>         ]
>       },
> +    "test_run_node": {
> +      "type": "object",
> +      "properties": {
> +        "node_name": {
> +          "$ref": "#/definitions/node_name"
> +        },
> +        "test_bed": {
> +          "type": "array",
> +          "items": {
> +            "$ref": "#/definitions/port_name"
> +          }
> +        }
> +      },
> +      "required": [
> +        "node_name",
> +        "test_bed"
> +      ]

Ports are not actually strictly required, we do have some simple tests 
that don't require any ports (hello world and some of the smoke tests).

Re: [PATCH v1 1/3] dts: rework port attributes in config module

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:

> The current design requires that a peer pci port is identified so that
> test suites can create the correct port links. While this can work, it
> also creates a lot of room for user error. Instead, devices should be
> given a unique identifier which is referenced in defined test runs.
>
> Both defined testbeds for the SUT and TG must have an equal number of
> specified ports. In each given array or ports, SUT port 0 is connected
> to TG port 0, SUT port 1 is connected to TG port 1, etc.
>
> Bugzilla ID: 1478
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
>

Aside from Jeremy/Juraj's comments and assuming this will get extended off
the pydantic series:

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 1251 bytes --]

[PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers

[Nicholas Pratte]

In order to leverage the usability of unique identifiers on ports, the
testbed_model Port object needs some refactoring/trimming of obsolete or
needless attributes.

Bugzilla ID: 1478

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/framework/testbed_model/port.py | 45 +++++++----------------------
 1 file changed, 10 insertions(+), 35 deletions(-)

diff --git a/dts/framework/testbed_model/port.py b/dts/framework/testbed_model/port.py
index 817405bea4..75bc38f16e 100644
--- a/dts/framework/testbed_model/port.py
+++ b/dts/framework/testbed_model/port.py
@@ -14,43 +14,30 @@
 from framework.config import PortConfig
 
 
-@dataclass(slots=True, frozen=True)
-class PortIdentifier:
-    """The port identifier.
-
-    Attributes:
-        node: The node where the port resides.
-        pci: The PCI address of the port on `node`.
-    """
-
-    node: str
-    pci: str
-
-
 @dataclass(slots=True)
 class Port:
     """Physical port on a node.
 
-    The ports are identified by the node they're on and their PCI addresses. The port on the other
-    side of the connection is also captured here.
+    The ports are identified using a unique, user-defined name/identifier.
     Each port is serviced by a driver, which may be different for the operating system (`os_driver`)
     and for DPDK (`os_driver_for_dpdk`). For some devices, they are the same, e.g.: ``mlx5_core``.
 
     Attributes:
-        identifier: The PCI address of the port on a node.
+        node_name: Node the port exists on.
+        name: User-defined unique identifier of the port.
+        pci: The pci address assigned to the port.
         os_driver: The operating system driver name when the operating system controls the port,
             e.g.: ``i40e``.
         os_driver_for_dpdk: The operating system driver name for use with DPDK, e.g.: ``vfio-pci``.
-        peer: The identifier of a port this port is connected with.
-            The `peer` is on a different node.
         mac_address: The MAC address of the port.
         logical_name: The logical name of the port. Must be discovered.
     """
 
-    identifier: PortIdentifier
+    node: str
+    name: str
+    pci: str
     os_driver: str
     os_driver_for_dpdk: str
-    peer: PortIdentifier
     mac_address: str = ""
     logical_name: str = ""
 
@@ -61,23 +48,11 @@ def __init__(self, node_name: str, config: PortConfig):
             node_name: The name of the port's node.
             config: The test run configuration of the port.
         """
-        self.identifier = PortIdentifier(
-            node=node_name,
-            pci=config.pci,
-        )
+        self.node = node_name
+        self.name = config.name
+        self.pci = config.pci
         self.os_driver = config.os_driver
         self.os_driver_for_dpdk = config.os_driver_for_dpdk
-        self.peer = PortIdentifier(node=config.peer_node, pci=config.peer_pci)
-
-    @property
-    def node(self) -> str:
-        """The node where the port resides."""
-        return self.identifier.node
-
-    @property
-    def pci(self) -> str:
-        """The PCI address of the port."""
-        return self.identifier.pci
 
 
 @dataclass(slots=True, frozen=True)
-- 
2.44.0

Re: [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers

[Jeremy Spewock]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> In order to leverage the usability of unique identifiers on ports, the
> testbed_model Port object needs some refactoring/trimming of obsolete or
> needless attributes.
>
> Bugzilla ID: 1478
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

Re: [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers

[Juraj Linkeš]

On 21. 8. 2024 20:43, Nicholas Pratte wrote:
> In order to leverage the usability of unique identifiers on ports, the
> testbed_model Port object needs some refactoring/trimming of obsolete or
> needless attributes.
> 
> Bugzilla ID: 1478
> 
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
>   dts/framework/testbed_model/port.py | 45 +++++++----------------------
>   1 file changed, 10 insertions(+), 35 deletions(-)
> 
> diff --git a/dts/framework/testbed_model/port.py b/dts/framework/testbed_model/port.py
> index 817405bea4..75bc38f16e 100644
> --- a/dts/framework/testbed_model/port.py
> +++ b/dts/framework/testbed_model/port.py
> @@ -14,43 +14,30 @@
>   from framework.config import PortConfig
>   
>   
> -@dataclass(slots=True, frozen=True)
> -class PortIdentifier:
> -    """The port identifier.
> -
> -    Attributes:
> -        node: The node where the port resides.
> -        pci: The PCI address of the port on `node`.
> -    """
> -
> -    node: str
> -    pci: str
> -
> -
>   @dataclass(slots=True)
>   class Port:
>       """Physical port on a node.
>   
> -    The ports are identified by the node they're on and their PCI addresses. The port on the other
> -    side of the connection is also captured here.
> +    The ports are identified using a unique, user-defined name/identifier.
>       Each port is serviced by a driver, which may be different for the operating system (`os_driver`)
>       and for DPDK (`os_driver_for_dpdk`). For some devices, they are the same, e.g.: ``mlx5_core``.
>   
>       Attributes:
> -        identifier: The PCI address of the port on a node.
> +        node_name: Node the port exists on.

This would be more accurate as "The name of the node the port exists 
on.". It's not a big deal since the type is str, but being explicit is 
better.

> +        name: User-defined unique identifier of the port.
> +        pci: The pci address assigned to the port.
>           os_driver: The operating system driver name when the operating system controls the port,
>               e.g.: ``i40e``.
>           os_driver_for_dpdk: The operating system driver name for use with DPDK, e.g.: ``vfio-pci``.
> -        peer: The identifier of a port this port is connected with.
> -            The `peer` is on a different node.
>           mac_address: The MAC address of the port.
>           logical_name: The logical name of the port. Must be discovered.
>       """
>   
> -    identifier: PortIdentifier
> +    node: str

Let's rename this to node_name. This is only used in one place in the 
framework so it's not a big change and it better reflects what the 
variable stores.

Re: [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:

> In order to leverage the usability of unique identifiers on ports, the
> testbed_model Port object needs some refactoring/trimming of obsolete or
> needless attributes.
>
> Bugzilla ID: 1478
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
>

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 787 bytes --]

[PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Nicholas Pratte]

The TestSuite object needs to access the running test run config in
order to identify the ports that need to be linked. To do this,
DTSRunner needs to be tweaked to accept test run configs to test suites,
and the TestSuite constructor needs to be reworked to accept test run
configs.

Bugzilla ID: 1478

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/framework/runner.py     | 16 +++++++++++++---
 dts/framework/test_suite.py | 33 +++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 6b6f6a05f5..a5629c2072 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -444,6 +444,7 @@ def _run_test_run(
                     tg_node,
                     build_target_config,
                     build_target_result,
+                    test_run_config,
                     test_suites_with_cases,
                 )
 
@@ -463,6 +464,7 @@ def _run_build_target(
         tg_node: TGNode,
         build_target_config: BuildTargetConfiguration,
         build_target_result: BuildTargetResult,
+        test_run_config: TestRunConfiguration,
         test_suites_with_cases: Iterable[TestSuiteWithCases],
     ) -> None:
         """Run the given build target.
@@ -477,6 +479,7 @@ def _run_build_target(
             build_target_config: A build target's test run configuration.
             build_target_result: The build target level result object associated
                 with the current build target.
+            test_run_config: The current test run configuration to be used by test suites.
             test_suites_with_cases: The test suites with test cases to run.
         """
         self._logger.set_stage(DtsStage.build_target_setup)
@@ -492,7 +495,9 @@ def _run_build_target(
             build_target_result.update_setup(Result.FAIL, e)
 
         else:
-            self._run_test_suites(sut_node, tg_node, build_target_result, test_suites_with_cases)
+            self._run_test_suites(
+                sut_node, tg_node, build_target_result, test_suites_with_cases, test_run_config
+            )
 
         finally:
             try:
@@ -509,6 +514,7 @@ def _run_test_suites(
         tg_node: TGNode,
         build_target_result: BuildTargetResult,
         test_suites_with_cases: Iterable[TestSuiteWithCases],
+        test_run_config: TestRunConfiguration,
     ) -> None:
         """Run `test_suites_with_cases` with the current build target.
 
@@ -524,12 +530,15 @@ def _run_test_suites(
             build_target_result: The build target level result object associated
                 with the current build target.
             test_suites_with_cases: The test suites with test cases to run.
+            test_run_config: The current test run config running the test suites.
         """
         end_build_target = False
         for test_suite_with_cases in test_suites_with_cases:
             test_suite_result = build_target_result.add_test_suite(test_suite_with_cases)
             try:
-                self._run_test_suite(sut_node, tg_node, test_suite_result, test_suite_with_cases)
+                self._run_test_suite(
+                    sut_node, tg_node, test_suite_result, test_run_config, test_suite_with_cases
+                )
             except BlockingTestSuiteError as e:
                 self._logger.exception(
                     f"An error occurred within {test_suite_with_cases.test_suite_class.__name__}. "
@@ -546,6 +555,7 @@ def _run_test_suite(
         sut_node: SutNode,
         tg_node: TGNode,
         test_suite_result: TestSuiteResult,
+        test_run_config: TestRunConfiguration,
         test_suite_with_cases: TestSuiteWithCases,
     ) -> None:
         """Set up, execute and tear down `test_suite_with_cases`.
@@ -572,7 +582,7 @@ def _run_test_suite(
         self._logger.set_stage(
             DtsStage.test_suite_setup, Path(SETTINGS.output_dir, test_suite_name)
         )
-        test_suite = test_suite_with_cases.test_suite_class(sut_node, tg_node)
+        test_suite = test_suite_with_cases.test_suite_class(sut_node, tg_node, test_run_config)
         try:
             self._logger.info(f"Starting test suite setup: {test_suite_name}")
             test_suite.set_up_suite()
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 694b2eba65..fd51796a06 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -20,6 +20,7 @@
 from scapy.layers.l2 import Ether  # type: ignore[import-untyped]
 from scapy.packet import Packet, Padding  # type: ignore[import-untyped]
 
+from framework.config import TestRunConfiguration
 from framework.testbed_model.port import Port, PortLink
 from framework.testbed_model.sut_node import SutNode
 from framework.testbed_model.tg_node import TGNode
@@ -64,6 +65,7 @@ class TestSuite:
 
     sut_node: SutNode
     tg_node: TGNode
+    test_run_config: TestRunConfiguration
     #: Whether the test suite is blocking. A failure of a blocking test suite
     #: will block the execution of all subsequent test suites in the current build target.
     is_blocking: ClassVar[bool] = False
@@ -78,11 +80,7 @@ class TestSuite:
     _tg_ip_address_ingress: Union[IPv4Interface, IPv6Interface]
     _tg_ip_address_egress: Union[IPv4Interface, IPv6Interface]
 
-    def __init__(
-        self,
-        sut_node: SutNode,
-        tg_node: TGNode,
-    ):
+    def __init__(self, sut_node: SutNode, tg_node: TGNode, test_run_config: TestRunConfiguration):
         """Initialize the test suite testbed information and basic configuration.
 
         Find links between ports and set up default IP addresses to be used when
@@ -91,9 +89,11 @@ def __init__(
         Args:
             sut_node: The SUT node where the test suite will run.
             tg_node: The TG node where the test suite will run.
+            test_run_config: The test run configuration that the test suite is running in.
         """
         self.sut_node = sut_node
         self.tg_node = tg_node
+        self.test_run_config = test_run_config
         self._logger = get_dts_logger(self.__class__.__name__)
         self._port_links = []
         self._process_links()
@@ -112,13 +112,22 @@ def __init__(
 
     def _process_links(self) -> None:
         """Construct links between SUT and TG ports."""
-        for sut_port in self.sut_node.ports:
-            for tg_port in self.tg_node.ports:
-                if (sut_port.identifier, sut_port.peer) == (
-                    tg_port.peer,
-                    tg_port.identifier,
-                ):
-                    self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port))
+        sut_ports = []
+        for port in self.sut_node.ports:
+            if port.name in [
+                sut_port.name for sut_port in self.test_run_config.system_under_test_node.ports
+            ]:
+                sut_ports.append(port)
+        tg_ports = []
+        for port in self.tg_node.ports:
+            if port.name in [
+                tg_port.name for tg_port in self.test_run_config.traffic_generator_node.ports
+            ]:
+                tg_ports.append(port)
+
+        # Both the TG and SUT nodes will have an equal number of ports.
+        for i in range(len(sut_ports)):
+            self._port_links.append(PortLink(sut_ports[i], tg_ports[i]))
 
     def set_up_suite(self) -> None:
         """Set up test fixtures common to all test cases.
-- 
2.44.0

Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Jeremy Spewock]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
<snip>
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 694b2eba65..fd51796a06 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -20,6 +20,7 @@
>  from scapy.layers.l2 import Ether  # type: ignore[import-untyped]
>  from scapy.packet import Packet, Padding  # type: ignore[import-untyped]
>
> +from framework.config import TestRunConfiguration
>  from framework.testbed_model.port import Port, PortLink
>  from framework.testbed_model.sut_node import SutNode
>  from framework.testbed_model.tg_node import TGNode
> @@ -64,6 +65,7 @@ class TestSuite:
<snip>
>      def _process_links(self) -> None:
>          """Construct links between SUT and TG ports."""
> -        for sut_port in self.sut_node.ports:
> -            for tg_port in self.tg_node.ports:
> -                if (sut_port.identifier, sut_port.peer) == (

It might be better to squash this commit into the previous just
because this line will cause an error in the previous commit due to
the removal of the identifier and peer attributes. While it is easier
to read broken apart, squashing saves the history from having a
"broken" commit.

> -                    tg_port.peer,
> -                    tg_port.identifier,
> -                ):
> -                    self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port))
> +        sut_ports = []
> +        for port in self.sut_node.ports:
> +            if port.name in [
> +                sut_port.name for sut_port in self.test_run_config.system_under_test_node.ports
> +            ]:

I'm not sure I understand what this check is doing fully. You're
looping through all ports in the SUT's list of ports, and then you are
checking that the name of that port exists in the configuration for
the SUT node in the test run, but aren't the list of ports from the
testrun config going to be the same as the ones from self.sut_node?
The list of ports in self.sut_node is created from the list of ports
that is in the NodeConfiguration, so as long as self.sut_node is the
node that is currently being used in the test run, which should be
handled elsewhere, this will always be True I think. Correct me if I
am misunderstanding though.

I think what you might be trying to do is access the
`system_under_test_node` field in `test_run` inside of conf.yaml, but
`self.test_run_config.system_under_test_node` does not point to that,
it points to the configuration of the SUT node from `nodes` in
conf.yaml. That would make sense since we really want to limit the
test suites to only having access to the ports that are listed in the
test_run configuration, but if you have only 2 ports in the test_run
configuration with this series applied and 3 in the node
configuration, this list will contain all 3 ports on the node. Maybe
something you could do to solve this is adding `sut_ports` and
`tg_ports` attributes to the TestRunConfiguration and only adding
ports to the test suite if they are in those lists. Admittedly, the
fact that `self.test_run_config.system_under_test_node` is named the
same as something in conf.yaml but points to a different thing than
that key in conf.yaml is pretty confusing. I had to do a couple
double-takes and look through the code path for making these config
classes myself to make sure this was doing what I thought it was.
Maybe we should rename this attribute in the TestRunConfiguration to
be something more like `sut_config` so it is more clear it is pointing
to the configuration of the whole SUT node.

> +                sut_ports.append(port)
> +        tg_ports = []
> +        for port in self.tg_node.ports:
> +            if port.name in [
> +                tg_port.name for tg_port in self.test_run_config.traffic_generator_node.ports
> +            ]:
> +                tg_ports.append(port)
> +
> +        # Both the TG and SUT nodes will have an equal number of ports.
> +        for i in range(len(sut_ports)):
> +            self._port_links.append(PortLink(sut_ports[i], tg_ports[i]))
>
>      def set_up_suite(self) -> None:
>          """Set up test fixtures common to all test cases.
> --
> 2.44.0
>

Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Juraj Linkeš]

On 4. 9. 2024 20:18, Jeremy Spewock wrote:
> On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
> <snip>
>> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>> index 694b2eba65..fd51796a06 100644
>> --- a/dts/framework/test_suite.py
>> +++ b/dts/framework/test_suite.py
>> @@ -20,6 +20,7 @@
>>   from scapy.layers.l2 import Ether  # type: ignore[import-untyped]
>>   from scapy.packet import Packet, Padding  # type: ignore[import-untyped]
>>
>> +from framework.config import TestRunConfiguration
>>   from framework.testbed_model.port import Port, PortLink
>>   from framework.testbed_model.sut_node import SutNode
>>   from framework.testbed_model.tg_node import TGNode
>> @@ -64,6 +65,7 @@ class TestSuite:
> <snip>
>>       def _process_links(self) -> None:
>>           """Construct links between SUT and TG ports."""
>> -        for sut_port in self.sut_node.ports:
>> -            for tg_port in self.tg_node.ports:
>> -                if (sut_port.identifier, sut_port.peer) == (
> 
> It might be better to squash this commit into the previous just
> because this line will cause an error in the previous commit due to
> the removal of the identifier and peer attributes. While it is easier
> to read broken apart, squashing saves the history from having a
> "broken" commit.
> 

Yes, this should be squashed. I don't think there's a clean way to split 
the commit without breaking the functionality.

>> -                    tg_port.peer,
>> -                    tg_port.identifier,
>> -                ):
>> -                    self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port))
>> +        sut_ports = []
>> +        for port in self.sut_node.ports:
>> +            if port.name in [
>> +                sut_port.name for sut_port in self.test_run_config.system_under_test_node.ports
>> +            ]:
> 
> I'm not sure I understand what this check is doing fully. You're
> looping through all ports in the SUT's list of ports, and then you are
> checking that the name of that port exists in the configuration for
> the SUT node in the test run, but aren't the list of ports from the
> testrun config going to be the same as the ones from self.sut_node?
> The list of ports in self.sut_node is created from the list of ports
> that is in the NodeConfiguration, so as long as self.sut_node is the
> node that is currently being used in the test run, which should be
> handled elsewhere, this will always be True I think. Correct me if I
> am misunderstanding though.
> 
> I think what you might be trying to do is access the
> `system_under_test_node` field in `test_run` inside of conf.yaml, but
> `self.test_run_config.system_under_test_node` does not point to that,
> it points to the configuration of the SUT node from `nodes` in
> conf.yaml. That would make sense since we really want to limit the
> test suites to only having access to the ports that are listed in the
> test_run configuration, but if you have only 2 ports in the test_run
> configuration with this series applied and 3 in the node
> configuration, this list will contain all 3 ports on the node. Maybe
> something you could do to solve this is adding `sut_ports` and
> `tg_ports` attributes to the TestRunConfiguration and only adding
> ports to the test suite if they are in those lists. Admittedly, the
> fact that `self.test_run_config.system_under_test_node` is named the
> same as something in conf.yaml but points to a different thing than
> that key in conf.yaml is pretty confusing. I had to do a couple
> double-takes and look through the code path for making these config
> classes myself to make sure this was doing what I thought it was.
> Maybe we should rename this attribute in the TestRunConfiguration to
> be something more like `sut_config` so it is more clear it is pointing
> to the configuration of the whole SUT node.
> 

You raise all of the important points. They way config is done now, we 
lose access to the subset of ports defined in 
test_run.system_under_test_node.test_bed (which is what the code was 
likely trying to access).

We should have the subset somewhere in the config. The two new 
attributes make sense since they already mirror what we already have 
with vdevs, but maybe we could do it some other way.

As to the confusion, the two attributes are defined as this:
system_under_test_node: SutNodeConfiguration
traffic_generator_node: TGNodeConfiguration

The name doesn't reflect what it's storing very well and [sut|tg]_config 
would be a better name, altough it could still be confusing, since it 
doesn't match the config. Maybe we could add a dict that would mirror 
the structure of the user config:

sut_config: TestRunSutConfigDict

Where TestRunSutConfigDict would be
class TestRunSutConfigDict(TypedDict):
     #: Node configuration
     node_conf: SutNodeConfiguration
     #: The ports specified in test run config
     ports: list[str]
     #: The vdevs specified in test run config
     vdevs: list[str]

This would mirror the config (except node_name became node_conf). Not 
sure what's best.

>> +                sut_ports.append(port)
>> +        tg_ports = []
>> +        for port in self.tg_node.ports:
>> +            if port.name in [
>> +                tg_port.name for tg_port in self.test_run_config.traffic_generator_node.ports
>> +            ]:
>> +                tg_ports.append(port)
>> +
>> +        # Both the TG and SUT nodes will have an equal number of ports.
>> +        for i in range(len(sut_ports)):
>> +            self._port_links.append(PortLink(sut_ports[i], tg_ports[i]))
>>
>>       def set_up_suite(self) -> None:
>>           """Set up test fixtures common to all test cases.
>> --
>> 2.44.0
>>

Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Jeremy Spewock]

On Tue, Sep 10, 2024 at 7:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
<snip>
> >> -                    tg_port.peer,
> >> -                    tg_port.identifier,
> >> -                ):
> >> -                    self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port))
> >> +        sut_ports = []
> >> +        for port in self.sut_node.ports:
> >> +            if port.name in [
> >> +                sut_port.name for sut_port in self.test_run_config.system_under_test_node.ports
> >> +            ]:
> >
> > I'm not sure I understand what this check is doing fully. You're
> > looping through all ports in the SUT's list of ports, and then you are
> > checking that the name of that port exists in the configuration for
> > the SUT node in the test run, but aren't the list of ports from the
> > testrun config going to be the same as the ones from self.sut_node?
> > The list of ports in self.sut_node is created from the list of ports
> > that is in the NodeConfiguration, so as long as self.sut_node is the
> > node that is currently being used in the test run, which should be
> > handled elsewhere, this will always be True I think. Correct me if I
> > am misunderstanding though.
> >
> > I think what you might be trying to do is access the
> > `system_under_test_node` field in `test_run` inside of conf.yaml, but
> > `self.test_run_config.system_under_test_node` does not point to that,
> > it points to the configuration of the SUT node from `nodes` in
> > conf.yaml. That would make sense since we really want to limit the
> > test suites to only having access to the ports that are listed in the
> > test_run configuration, but if you have only 2 ports in the test_run
> > configuration with this series applied and 3 in the node
> > configuration, this list will contain all 3 ports on the node. Maybe
> > something you could do to solve this is adding `sut_ports` and
> > `tg_ports` attributes to the TestRunConfiguration and only adding
> > ports to the test suite if they are in those lists. Admittedly, the
> > fact that `self.test_run_config.system_under_test_node` is named the
> > same as something in conf.yaml but points to a different thing than
> > that key in conf.yaml is pretty confusing. I had to do a couple
> > double-takes and look through the code path for making these config
> > classes myself to make sure this was doing what I thought it was.
> > Maybe we should rename this attribute in the TestRunConfiguration to
> > be something more like `sut_config` so it is more clear it is pointing
> > to the configuration of the whole SUT node.
> >
>
> You raise all of the important points. They way config is done now, we
> lose access to the subset of ports defined in
> test_run.system_under_test_node.test_bed (which is what the code was
> likely trying to access).
>
> We should have the subset somewhere in the config. The two new
> attributes make sense since they already mirror what we already have
> with vdevs, but maybe we could do it some other way.
>
> As to the confusion, the two attributes are defined as this:
> system_under_test_node: SutNodeConfiguration
> traffic_generator_node: TGNodeConfiguration
>
> The name doesn't reflect what it's storing very well and [sut|tg]_config
> would be a better name, altough it could still be confusing, since it
> doesn't match the config. Maybe we could add a dict that would mirror
> the structure of the user config:
>
> sut_config: TestRunSutConfigDict
>
> Where TestRunSutConfigDict would be
> class TestRunSutConfigDict(TypedDict):
>      #: Node configuration
>      node_conf: SutNodeConfiguration
>      #: The ports specified in test run config
>      ports: list[str]
>      #: The vdevs specified in test run config
>      vdevs: list[str]
>
> This would mirror the config (except node_name became node_conf). Not
> sure what's best.

I like the idea of making this dict and having something that better
reflects what is present in the config file. This matching would make
things much more clear to me in general.

>
> >> +                sut_ports.append(port)
> >> +        tg_ports = []
> >> +        for port in self.tg_node.ports:
> >> +            if port.name in [
> >> +                tg_port.name for tg_port in self.test_run_config.traffic_generator_node.ports
> >> +            ]:
> >> +                tg_ports.append(port)
> >> +
> >> +        # Both the TG and SUT nodes will have an equal number of ports.
> >> +        for i in range(len(sut_ports)):
> >> +            self._port_links.append(PortLink(sut_ports[i], tg_ports[i]))
> >>
> >>       def set_up_suite(self) -> None:
> >>           """Set up test fixtures common to all test cases.
> >> --
> >> 2.44.0
> >>
>

Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Dean Marx]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:

> The TestSuite object needs to access the running test run config in
> order to identify the ports that need to be linked. To do this,
> DTSRunner needs to be tweaked to accept test run configs to test suites,
> and the TestSuite constructor needs to be reworked to accept test run
> configs.
>
> Bugzilla ID: 1478
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
>

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 916 bytes --]