|WARNING| [v4] dts: add support for no link topology

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165616

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-17

# DPDK Patch Review

## Overview
This patch adds support for running DTS with no traffic generator node and no ethdev interfaces (no-link topology). The changes span multiple files and introduce conditional logic to handle the absence of TG nodes and port topology.

---

## Errors

### 1. Resource leak on error path in test_run.py TestRunSetup.next()

**File:** `dts/framework/test_run.py`  
**Lines:** 346-382

The `TestRunSetup.next()` method allocates resources (node setup, DPDK setup, topology setup, VF ports, crypto ports, traffic generators) but lacks proper cleanup on error paths. If any setup step fails after earlier resources have been allocated, those resources are not released before the exception propagates.

**Fix:** Wrap the setup sequence in a try-except block that calls teardown methods for any resources successfully allocated before the failure:

```python
def next(self) -> State | None:
    """Process state and return the next one."""
    test_run = self.test_run
    ctx = test_run.ctx
    init_ctx(ctx)

    self.logger.info(f"Running on SUT node '{ctx.sut_node.name}'.")
    test_run.init_random_seed()
    test_run.remaining_tests = deque(test_run.selected_tests)

    try:
        ctx.sut_node.setup()
        if ctx.topology.type is not LinkTopology.NO_LINK and ctx.tg_node is not None:
            ctx.tg_node.setup()
        ctx.dpdk.setup()
        ctx.topology.setup()

        if test_run.config.use_virtual_functions:
            ctx.topology.instantiate_vf_ports()
        if ctx.sut_node.cryptodevs and test_run.config.crypto:
            ctx.topology.instantiate_crypto_ports()
            ctx.topology.bind_cryptodevs("dpdk")

        ctx.topology.configure_ports("sut", "dpdk")
        if ctx.func_tg and ctx.topology.type is not LinkTopology.NO_LINK:
            ctx.func_tg.setup(ctx.topology)
        if ctx.perf_tg and ctx.topology.type is not LinkTopology.NO_LINK:
            ctx.perf_tg.setup(ctx.topology)

        self.result.ports = [
            port.to_dict() for port in ctx.topology.sut_ports + ctx.topology.tg_ports
        ]
        self.result.sut_session_info = ctx.sut_node.node_info
        self.result.dpdk_build_info = ctx.dpdk_build.get_dpdk_build_info()

        self.logger.debug(f"Found capabilities to check: {test_run.required_capabilities}")
        test_run.supported_capabilities = get_supported_capabilities(
            ctx.sut_node, ctx.topology, test_run.required_capabilities
        )
        return TestRunExecution(test_run, self.result)
    except Exception:
        # Clean up any resources that were successfully allocated
        # This should mirror the TestRunTeardown logic
        # (implementation depends on which resources track their setup state)
        raise
```

Note: The exact cleanup logic depends on whether each resource tracks its own setup state. Review the `TestRunTeardown.next()` method and ensure all conditionals there are replicated in the error path here.

---

### 2. Comparison against None uses `is not` instead of explicit equality

**File:** `dts/api/test.py`  
**Line:** 117

```python
if ctx.topology.type is not LinkTopology.NO_LINK and ctx.tg_node is not None:
```

The comparison `ctx.tg_node is not None` is correct for a null check. However, DPDK coding standards prefer explicit comparisons. Since `tg_node` is typed `Node | None`, this is a pointer/object check and the `is not None` pattern is appropriate here.

**Correction:** This is actually acceptable. `is not None` is the correct pattern for None checks in Python. The DPDK C coding standard requiring explicit comparisons (e.g., `if (p != NULL)`) does not apply to Python code. No change needed.

---

### 3. Multiple redundant None checks on ctx.tg_node

**Files:** `dts/framework/test_run.py`, `dts/api/test.py`  
**Lines:** Multiple locations

The code repeatedly checks both `ctx.topology.type is not LinkTopology.NO_LINK` and `ctx.tg_node is not None` in the same condition. If `topology.type` being `NO_LINK` implies `tg_node is None`, one check should suffice. If they are independent, both are needed.

**Clarification needed:** Are these conditions redundant? If `topology.type == NO_LINK` always means `tg_node is None`, simplify to just the topology check. If they can vary independently, keep both.

Example from `test_run.py` line 202:
```python
if ctx.topology.type is not LinkTopology.NO_LINK and ctx.tg_node is not None:
```

This appears defensive but may indicate the invariant is not enforced. If the patch guarantees `NO_LINK` topology always has `tg_node = None`, remove the redundant check.

---

## Warnings

### 1. Missing release notes update

This patch introduces a significant new capability (no-link topology) but does not update release notes. New features should be documented in `doc/guides/rel_notes/release_XX_XX.rst`.

**Fix:** Add a release notes entry describing the no-link topology feature.

---

### 2. Inconsistent guard logic in node_and_ports_from_id()

**File:** `dts/framework/testbed_model/topology.py`  
**Lines:** 107-113

```python
case "tg":
    if ctx.tg_node is not None:
        return ctx.tg_node, self.tg_ports
    msg = "node tg does not exist with current topology."
    raise InternalError(msg)
msg = f"Invalid node `{node_identifier}` given."
raise InternalError(msg)
```

The control flow is confusing: the last two lines (after the match statement) can never be reached because all cases either return or raise. The final `raise InternalError` is dead code.

**Fix:** Remove the unreachable lines or restructure:

```python
case "sut":
    return ctx.sut_node, self.sut_ports
case "tg":
    if ctx.tg_node is not None:
        return ctx.tg_node, self.tg_ports
    raise InternalError("node tg does not exist with current topology.")
case _:
    raise InternalError(f"Invalid node `{node_identifier}` given.")
```

---

### 3. Logic error in validate_port_links assertion order

**File:** `dts/framework/config/__init__.py`  
**Lines:** 97-109

The assertion `tg_node_port_peer is False or sut_node_port_peer == link.left` is checking that the TG port either is not yet linked OR the SUT port matches `link.left`. However, if `sut_node_port_peer` is `False` (not yet linked), comparing it to `link.left` (a tuple) will always be False, and the assertion will fail if `tg_node_port_peer` is also `False`.

This appears to be a copy-paste error from the SUT check above. The logic should check whether the TG port is already linked to a *different* port, not whether it matches the current SUT port.

**Review needed:** Verify the intended invariant here. The assertion may be incorrect.

---

### 4. Potential uninitialized variable sut_ports/tg_ports

**File:** `dts/framework/testbed_model/topology.py`  
**Lines:** 76-77

```python
sut_ports = []
tg_ports = []
```

These variables are initialized but only used if `port_link` is not None. If `port_link` is None (the iterator is empty), the function proceeds past line 90 with `sut_ports` and `tg_ports` still empty lists, which are then used to construct the Topology object.

This is likely intentional for the NO_LINK case, but ensure the Topology constructor and all methods handle empty port lists correctly.

**Verify:** Check that `Topology.__init__` and all methods that access `sut_ports`/`tg_ports` are safe when these lists are empty.

---

### 5. Test function not updated for no-link topology

**File:** `dts/api/test.py`  
**Function:** `fail()`

The `fail()` function logs command history from both SUT and TG nodes. It now correctly guards TG access with `ctx.tg_node is not None`, but the log message "showing the last 10 commands executed on TG" will not print if TG is absent, which could confuse users debugging a test failure in no-link mode.

**Suggestion:** Add an else clause or a different log message indicating TG command history is unavailable in no-link topology.

---

### 6. Incomplete documentation in test_run.example.yaml

**File:** `dts/configurations/test_run.example.yaml`  
**Lines:** 28-29

The comment for `port_topology` says "By providing an empty list, DTS will run in no link topology mode and will not allocate a TG node."

However, the comment for `traffic_generator_node` on line 64 is incomplete: it says `# see` but doesn't reference a section. It should clarify that `traffic_generator_node` can be omitted or is ignored when `port_topology` is empty.

**Fix:**
```yaml
traffic_generator_node: "TG 1" # Optional; not used when port_topology is empty (no-link mode)
```

---

## Info

### 1. Consider adding a helper method for no-link topology checks

Multiple locations check `ctx.topology.type is not LinkTopology.NO_LINK`. Consider adding a property `ctx.topology.has_links` or `ctx.topology.is_no_link` to make the intent clearer and reduce repetition.

Example:
```python
@property
def is_no_link(self) -> bool:
    return self.type is LinkTopology.NO_LINK
```

Then use: `if not ctx.topology.is_no_link and ctx.tg_node is not None:`

---

### 2. runner.py node filtering logic could be clearer

**File:** `dts/framework/runner.py`  
**Lines:** 64-67

```python
if self._configuration.test_run.port_topology == []:
    if node_config.name == self._configuration.test_run.system_under_test_node:
        nodes.append(Node(node_config))
else:
    nodes.append(Node(node_config))
```

This logic creates only the SUT node when `port_topology` is empty. A comment explaining this would improve readability:

```python
# In no-link topology, only instantiate the SUT node
if self._configuration.test_run.port_topology == []:
    if node_config.name == self._configuration.test_run.system_under_test_node:
        nodes.append(Node(node_config))
else:
    # Normal topology: instantiate all configured nodes
    nodes.append(Node(node_config))
```

---

## Summary

**Critical issues to address:**
- Error path resource leak in `TestRunSetup.next()` (Error #1)
- Logic error in `validate_port_links` assertion (Warning #3)

**Recommended changes:**
- Add release notes for the new feature
- Fix unreachable code in `node_and_ports_from_id()`
- Improve documentation clarity in example config
- Add comment in runner.py for node filtering logic

**Verification needed:**
- Confirm `sut_ports`/`tg_ports` empty list handling is safe
- Clarify redundancy of `ctx.tg_node is not None` checks