* [PATCH v1 1/3] dts: rework port attributes in config module
2024-08-21 18:43 [PATCH v1 0/3] dts: rework topology definition in dts config Nicholas Pratte
@ 2024-08-21 18:43 ` Nicholas Pratte
2024-09-04 18:18 ` Jeremy Spewock
2024-09-10 10:11 ` Juraj Linkeš
2024-08-21 18:43 ` [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers Nicholas Pratte
2024-08-21 18:43 ` [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs Nicholas Pratte
2 siblings, 2 replies; 10+ messages in thread
From: Nicholas Pratte @ 2024-08-21 18:43 UTC (permalink / raw)
To: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, jspewock,
alex.chapman
Cc: dev, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 1/3] dts: rework port attributes in config module
2024-08-21 18:43 ` [PATCH v1 1/3] dts: rework port attributes in config module Nicholas Pratte
@ 2024-09-04 18:18 ` Jeremy Spewock
2024-09-10 10:11 ` Juraj Linkeš
1 sibling, 0 replies; 10+ messages in thread
From: Jeremy Spewock @ 2024-09-04 18:18 UTC (permalink / raw)
To: Nicholas Pratte
Cc: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, alex.chapman, dev
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 1/3] dts: rework port attributes in config module
2024-08-21 18:43 ` [PATCH v1 1/3] dts: rework port attributes in config module Nicholas Pratte
2024-09-04 18:18 ` Jeremy Spewock
@ 2024-09-10 10:11 ` Juraj Linkeš
1 sibling, 0 replies; 10+ messages in thread
From: Juraj Linkeš @ 2024-09-10 10:11 UTC (permalink / raw)
To: Nicholas Pratte, yoan.picchi, luca.vizzarro, probb,
paul.szczepanek, Honnappa.Nagarahalli, dmarx, jspewock,
alex.chapman
Cc: dev
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).
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers
2024-08-21 18:43 [PATCH v1 0/3] dts: rework topology definition in dts config Nicholas Pratte
2024-08-21 18:43 ` [PATCH v1 1/3] dts: rework port attributes in config module Nicholas Pratte
@ 2024-08-21 18:43 ` Nicholas Pratte
2024-09-04 18:18 ` Jeremy Spewock
2024-09-10 10:17 ` Juraj Linkeš
2024-08-21 18:43 ` [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs Nicholas Pratte
2 siblings, 2 replies; 10+ messages in thread
From: Nicholas Pratte @ 2024-08-21 18:43 UTC (permalink / raw)
To: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, jspewock,
alex.chapman
Cc: dev, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers
2024-08-21 18:43 ` [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers Nicholas Pratte
@ 2024-09-04 18:18 ` Jeremy Spewock
2024-09-10 10:17 ` Juraj Linkeš
1 sibling, 0 replies; 10+ messages in thread
From: Jeremy Spewock @ 2024-09-04 18:18 UTC (permalink / raw)
To: Nicholas Pratte
Cc: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, alex.chapman, dev
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers
2024-08-21 18:43 ` [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers Nicholas Pratte
2024-09-04 18:18 ` Jeremy Spewock
@ 2024-09-10 10:17 ` Juraj Linkeš
1 sibling, 0 replies; 10+ messages in thread
From: Juraj Linkeš @ 2024-09-10 10:17 UTC (permalink / raw)
To: Nicholas Pratte, yoan.picchi, luca.vizzarro, probb,
paul.szczepanek, Honnappa.Nagarahalli, dmarx, jspewock,
alex.chapman
Cc: dev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs
2024-08-21 18:43 [PATCH v1 0/3] dts: rework topology definition in dts config Nicholas Pratte
2024-08-21 18:43 ` [PATCH v1 1/3] dts: rework port attributes in config module Nicholas Pratte
2024-08-21 18:43 ` [PATCH v1 2/3] dts: rework testbed_model Port objects to contain unique identifiers Nicholas Pratte
@ 2024-08-21 18:43 ` Nicholas Pratte
2024-09-04 18:18 ` Jeremy Spewock
2 siblings, 1 reply; 10+ messages in thread
From: Nicholas Pratte @ 2024-08-21 18:43 UTC (permalink / raw)
To: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, jspewock,
alex.chapman
Cc: dev, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs
2024-08-21 18:43 ` [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs Nicholas Pratte
@ 2024-09-04 18:18 ` Jeremy Spewock
2024-09-10 11:05 ` Juraj Linkeš
0 siblings, 1 reply; 10+ messages in thread
From: Jeremy Spewock @ 2024-09-04 18:18 UTC (permalink / raw)
To: Nicholas Pratte
Cc: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, juraj.linkes, dmarx, alex.chapman, dev
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs
2024-09-04 18:18 ` Jeremy Spewock
@ 2024-09-10 11:05 ` Juraj Linkeš
0 siblings, 0 replies; 10+ messages in thread
From: Juraj Linkeš @ 2024-09-10 11:05 UTC (permalink / raw)
To: Jeremy Spewock, Nicholas Pratte
Cc: yoan.picchi, luca.vizzarro, probb, paul.szczepanek,
Honnappa.Nagarahalli, dmarx, alex.chapman, dev
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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread