|WARNING| [v1] dts: update configuration for performance test

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-05

# DPDK Patch Review

## Patch Summary
This patch adds configuration options to the DTS (DPDK Test Suite) performance test framework, allowing per-test-case delta tolerance and configurable TRex parameters.

---

## Errors

### 1. Missing error handling for missing dictionary key

**File:** `dts/tests/TestSuite_single_core_forward_perf.py`  
**Lines:** 170, 177

The code accesses `params["delta_tolerance"]` without verifying the key exists. If a test parameter dictionary in `test_parameters` omits `delta_tolerance`, this will raise `KeyError`.

```python
params["pass"] = float(params["performance_delta"]) >= -params["delta_tolerance"]
# and later:
f"""Packets forwarded is less than {(1 - params["delta_tolerance"]) * 100}%
```

**Fix:** Use `.get()` with a default value or validate the schema at suite setup:

```python
# Option 1: Use get() with default
params["pass"] = float(params["performance_delta"]) >= -params.get("delta_tolerance", 0.05)

# Option 2: Validate in set_up_suite()
def set_up_suite(self):
    self.test_parameters = self.config.test_parameters
    for params in self.test_parameters:
        if "delta_tolerance" not in params:
            raise ValueError(f"Missing delta_tolerance in test parameters: {params}")
    self.traffic_duration = self.config.traffic_duration
    self.test_repetitions = self.config.test_repetitions
```

---

## Warnings

### 1. Missing validation for configuration values

**File:** `dts/framework/config/test_run.py`  
**Line:** 423

The `cores` field has no bounds checking. TRex requires a minimum number of cores and has practical upper limits.

**Suggest:** Add validation:

```python
cores: int = Field(gt=0, le=128)  # assuming pydantic Field validators
```

Or add runtime validation in the TRex constructor.

---

### 2. Breaking configuration change without backward compatibility

**File:** `dts/framework/config/test_run.py`  
**Line:** 423

Adding a required `cores` field to `TrexTrafficGeneratorConfig` breaks existing configurations that don't specify it. This will cause deserialization failures for users with existing YAML configs.

**Suggest:** Make the field optional with a default:

```python
cores: int = 10  # default value preserves backward compatibility
```

Then in `trex.py`:

```python
self._tg_cores = config.cores  # will use 10 if not specified in config
```

---

### 3. Missing release notes

This patch adds new user-facing configuration options (`cores`, `traffic_duration`, `test_repetitions`, per-test `delta_tolerance`) but does not update release notes to document these changes.

**Suggest:** Add a release notes entry under `doc/guides/rel_notes/release_26_03.rst` (or appropriate version) documenting the new DTS configuration parameters.

---

### 4. Inconsistent removal of default value

**File:** `dts/framework/testbed_model/traffic_generator/trex.py`  
**Line:** 85

The patch removes the default `_tg_cores: int = 10` but does not initialize `_tg_cores` in the class body, relying entirely on the constructor. This is acceptable but differs from the original pattern where a class-level default existed.

**Suggest:** Either keep the default (`_tg_cores: int = 10`) for self-documentation, or add a comment explaining why no default is set:

```python
_tg_cores: int  # Set from config.cores in __init__
```

---

### 5. Missing type annotation for test_parameters dictionary values

**File:** `dts/tests/TestSuite_single_core_forward_perf.py`  
**Lines:** 34-67

The `test_parameters` list contains dictionaries with mixed types (`int | float`) but the exact required/optional keys are not formally specified. This makes schema validation difficult and error-prone.

**Suggest:** Define a TypedDict or Pydantic model:

```python
from typing import TypedDict

class TestParameters(TypedDict):
    frame_size: int
    num_descriptors: int
    expected_mpps: float
    delta_tolerance: float
    # mark optional fields if any

test_parameters: list[TestParameters] = [...]
```

---

## Info

### 1. Configuration example update is incomplete

**File:** `dts/configurations/test_run.example.yaml`  
**Line:** 55

The example shows `cores: 10` but does not show the new `traffic_duration`, `test_repetitions`, or per-test `delta_tolerance` parameters. While these may be suite-specific, an example would help users understand the new options.

**Suggest:** Add commented examples:

```yaml
# Single core forward performance test configuration (example)
# test_parameters:
#   - frame_size: 64
#     num_descriptors: 1024
#     expected_mpps: 1.00
#     delta_tolerance: 0.05  # per-test tolerance
# traffic_duration: 5  # seconds
# test_repetitions: 5  # number of runs to average
```

---

### 2. Docstring does not match implementation

**File:** `dts/tests/TestSuite_single_core_forward_perf.py`  
**Lines:** 109-110

The docstring says "Create a testpmd session with every rule in the given list, verify jump behavior" which appears to be copy-pasted from another function and does not describe `_transmit()`.

**Suggest:** Update the docstring:

```python
def _transmit(self, testpmd: TestPmd, frame_size: int, repetitions: int = 1) -> float:
    """Transmit packets and measure average Rx throughput.
    
    Args:
        testpmd: Running TestPmd instance.
        frame_size: Frame size in bytes.
        repetitions: Number of test runs to average.
        
    Returns:
        Average Rx throughput in Mpps.
    """
```

---

## Summary

**Critical issues:**
- Error handling needed for missing `delta_tolerance` key (Error #1)

**Configuration issues:**
- Breaking change: `cores` should be optional with default (Warning #2)
- Missing validation for `cores` bounds (Warning #1)

**Documentation gaps:**
- Missing release notes (Warning #3)
- Incomplete example configuration (Info #1)
- Incorrect docstring (Info #2)

The patch is functionally sound but needs error handling for the dictionary key access and should preserve backward compatibility for the `cores` configuration field.