[AI Generated] sriov: validate SR-IOV VF ethtool operations - #4720
Open
VENNELA2132 wants to merge 10 commits into
Open
VENNELA2132 wants to merge 10 commits into
VENNELA2132 wants to merge 10 commits into
Conversation
added 10 commits
September 16, 2026 14:59
Ten call sites decided whether a driver supports an operation by
looking for "Operation not supported" in result.stdout, but ethtool
writes that diagnostic to stderr, which lisa keeps as a separate
stream, so every one of those conditions was permanently false and a
refused operation surfaced as a LisaException instead of the
UnsupportedOperationException callers turn into a skip. Route the
checks through a single _is_unsupported helper that searches both
streams, with extra match patterns opt in so the statistics sites keep
"no stats available" without widening it to opcodes that never emit it,
and leave each site's exit code condition as it was. Also add the
missing f prefix on the -l message, which would otherwise have raised
with a literal {interface}.
Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that changes the RX and TX ring sizes of every SR-IOV VF on the node, checks the driver applies and reports them back, and then restores the original values. The target is one power-of-two step up rather than the reported maximum, because rings are preallocated, so the maximum costs about 1 GB on a 32 queue VF and can fail on -ENOMEM, against about 140 MB for a single step. A VF driver reports back the ring size it was given, so the check is an equality rather than a range. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that reads the RSS hash key of every SR-IOV VF, writes a different key of the same length, verifies the key is applied exactly, and restores the original key. Ensure each interface is up while the key is changed and restore its original state after validation. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that reads the RSS indirection table of every SR-IOV VF, checks that entries reference valid receive queues, redistributes the table evenly across a subset of queues, verifies an oversized request is rejected without changing the table, and restores the driver default. Add the matching parser and get and set methods to Ethtool, exposing the table size separately from the RX ring count. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add reusable support for reading and updating interrupt coalescing settings, including numeric and not-applicable values reported by ethtool -c. Detect packaged clients that do not expose rx-cqe-frames and build checksum-pinned ethtool v7.1 with distribution-specific libmnl dependencies only when needed. Add focused selftests for parsing coalescing output and rejecting the fallback on unsupported distributions. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that validates rx-cqe-frames on every SR-IOV VF. Probe an alternate value accepted by the driver, verify it is reported back, and ensure an out-of-range u32 value is rejected without changing the active configuration. Restore the original coalescing value and interface state on every path so subsequent cases are not affected. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that tests every combined channel count from one through the effective maximum on each SR-IOV VF. Verify traffic after the datapath is rebuilt and restore the original counts. Record each original count before mutation and restore every VF independently so readback or restore failures cannot leak a modified configuration. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Calculate counter deltas from a copy so cached DeviceStatistics continue to represent the absolute values returned by ethtool. Retain the raw statistics snapshot for consumers that need to validate parser completeness without running ethtool a second time. Add regression coverage for the delta result, cache integrity, and raw output. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Add a case that validates every VF exposes complete RX and TX per-queue packet statistics. Send a controlled 1,000-packet workload through each VF and verify both counter directions account for it. Use exact base packet-counter names to avoid inflating deltas with TSO counters. Validate names and values from one ethtool snapshot, and require IP-bearing Accelerated Networking pairings before traffic. Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
Signed-off-by: Erni Sri Satya Vennela <ernis@microsoft.com>
VENNELA2132
requested review from
LiliDeng and
Johnson (johnsongeorge-w)
as code owners
September 16, 2026 10:38
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved command-injection risk and multiple restoration, cleanup, and cache issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds SR-IOV VF ethtool integration coverage and extends shared ethtool parsing and operations.
Changes:
- Adds six validation cases for rings, RSS, coalescing, channels, and statistics.
- Adds ethtool parsers, operations, unsupported-operation detection, and cache handling.
- Adds unit tests for parsing and statistics behavior.
File summaries
| File | Summary and final review findings |
|---|---|
selftests/test_ethtool.py |
Adds ethtool unit tests. Nit (1 vote): add normal and malformed/missing-row coverage for DeviceRssIndirectionTable. |
lisa/tools/ethtool.py |
Adds ethtool operations and parsers. Critical (3 votes): validate and safely quote spec and interface values before using shell=True. Moderate (1 vote each): invalidate or refresh RSS indirection and coalescing caches after successful setters. |
lisa/microsoft/testsuites/network/sriov.py |
Adds SR-IOV integration scenarios and cleanup. Moderate (3 votes): guard ring-buffer changes so readback failures still trigger restoration (lines 401/540). Moderate (1 vote): restore the exact original RSS indirection table. Moderate (2 votes): catch restoration AssertionError so cleanup continues across VFs. Moderate (1 vote): handle zero or non-tunable ring maxima safely. Moderate (1 vote): mark the node dirty when mutation is attempted. Nit (2 votes): link the related issue. Nit (1 vote): include command output and diagnostic guidance on setting failures. |
Review details
Suppressed comments (8)
lisa/microsoft/testsuites/network/sriov.py:541
- Major:
change_device_rss_hash_keysets the key and then reads it back, but that call is outside thetry. If the setter succeeds and the readback/parser fails, the original key is never restored. Put the change call inside the guarded block so thefinallyrestores the key after any post-apply failure.
changed = ethtool.change_device_rss_hash_key(vf_nic, new_key)
try:
lisa/microsoft/testsuites/network/sriov.py:671
- Major: Restoring with
ethtool -X ... defaultis not equivalent to restoringoriginal.tablewhen the VF starts with a non-default indirection table. The assertion then fails and, more importantly, the test leaves the device at the driver default instead of its pre-test state. Restore the exact snapshot (or explicitly verify the initial table is the default before changing it).
log.info(f"Restoring {vf_nic} RSS indirection table to the default")
restored = ethtool.change_device_rss_indirection_table(
vf_nic, "default"
)
lisa/microsoft/testsuites/network/sriov.py:462
maximum=0is a valid report for a non-tunable RX/TX ring, but1 << (maximum.bit_length() - 1)then shifts by-1and raisesValueError. Handle non-positive maxima and the no-alternate-value case by skipping this VF instead of failing the test with an unrelated arithmetic error.
ceiling = 1 << (maximum.bit_length() - 1)
step_up = 1 << current.bit_length()
if step_up <= ceiling:
return step_up
# Already at the largest power of two the device allows.
return max(ceiling // 2, 1)
lisa/microsoft/testsuites/network/sriov.py:865
- This failure path raises with only
Couldn't set ..., without the command output or guidance for investigation. Include stdout/stderr and point to checking ethtool netlink/driver support so an unexpected rejection is diagnosable.
result.assert_exit_code(
message=f"Couldn't set {vf_nic} {parameter} to {candidate}.",
include_output=True,
)
lisa/microsoft/testsuites/network/sriov.py:403
- These cases mutate VF network settings and interface state but never mark the node dirty. If cleanup is incomplete or the driver leaves transient state, LISA can reuse a contaminated VM; mark the node dirty when mutation is attempted, or provide an explicit reuse-safe cleanup contract.
actual_settings = ethtool.change_device_ring_buffer_settings(
vf_nic, expected_rx, expected_tx
)
lisa/tools/ethtool.py:1100
- The successful
set_device_rss_indirection_tablepath leavesDeviceSettings.device_rss_indirection_tableuntouched. After a caller sets a table, a laterget_device_rss_indirection_table(interface)withoutforce_run=Truecan therefore return the old cached table instead of the active one. Invalidate or refresh this cache after a successful set.
return self.run(
f"-X {interface} {spec}",
sudo=True,
force_run=True,
shell=True,
)
lisa/tools/ethtool.py:1148
- The successful
set_device_coalesce_settingpath does not invalidatedevice_coalesce_settings. Once a value has been cached, callers that invoke this setter and then use the defaultget_device_coalesce_settingscall will observe stale settings unless they know to passforce_run=True; invalidate or refresh the cache after a successful set.
return self.run(
f"-C {shlex.quote(interface)} {shlex.quote(parameter)} "
f"{shlex.quote(str(value))}",
sudo=True,
force_run=True,
shell=True,
)
lisa/tools/ethtool.py:424
- The new
DeviceRssIndirectionTableparser has no unit coverage inselftests/test_ethtool.py, even though that file tests the other new parsers. Add cases for a normal multi-row table and malformed or missing rows so format changes cannot silently corrupt the table used by all RSS tests.
class DeviceRssIndirectionTable:
# The indirection table shares the "ethtool -x" output with the hash key:
# RX flow hash indirection table for eth0 with 4 RX ring(s):
# 0: 0 1 2 3 0 1 2 3
# 8: 0 1 2 3 0 1 2 3
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1095
to
+1100
| return self.run( | ||
| f"-X {interface} {spec}", | ||
| sudo=True, | ||
| force_run=True, | ||
| shell=True, | ||
| ) |
Comment on lines
+401
to
+421
| actual_settings = ethtool.change_device_ring_buffer_settings( | ||
| vf_nic, expected_rx, expected_tx | ||
| ) | ||
|
|
||
| try: | ||
| # A VF driver reports back the ring size it was given, so | ||
| # this is an equality check rather than a range. | ||
| assert_that( | ||
| int(actual_settings.current_ring_buffer_settings["RX"]), | ||
| "Changing RX Ringbuffer setting didn't succeed", | ||
| ).is_equal_to(expected_rx) | ||
| assert_that( | ||
| int(actual_settings.current_ring_buffer_settings["TX"]), | ||
| "Changing TX Ringbuffer setting didn't succeed", | ||
| ).is_equal_to(expected_tx) | ||
| finally: | ||
| # Restore even when the checks above fail, so a failure does | ||
| # not hand a resized ring to the cases that follow. | ||
| reverted_settings = ethtool.change_device_ring_buffer_settings( | ||
| vf_nic, original_rx, original_tx | ||
| ) |
| ) | ||
| log.error(message) | ||
| restoration_failures.append(message) | ||
| except LisaException as identifier: |
| sriov_basic_test(environment) | ||
| sriov_vf_connection_test(environment, vm_nics, turn_off_lower=True) | ||
|
|
||
| @TestCaseMetadata( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add SR-IOV VF integration coverage for ethtool ring buffers, RSS hash keys,
RSS indirection tables, receive CQE coalescing, channel configuration, and
per-queue statistics.
Extend the shared Ethtool implementation with the parsers and operations needed
by these cases. Unsupported-operation detection now checks stderr, statistics
deltas no longer mutate cached absolute counters, and raw statistics output is
retained for completeness validation.
Mutable settings and interface state are restored on every path. These cases do
not mark nodes dirty.
Related Issue
None.
Type of Change
Checklist
Test Validation
Key Test Cases:
verify_sriov_ethtool_ring_buffer_settings|verify_sriov_ethtool_rss_hash_key|verify_sriov_ethtool_rss_indirection_table|verify_sriov_ethtool_coalesce_settings|verify_sriov_ethtool_channels|verify_sriov_ethtool_statistics
Impacted LISA Features:
Sriov
Tested Azure Marketplace Images:
Test Results
Coverage overview
Each integration case was run independently across the same 19-image matrix:
Standard_D2ads_v5Standard_D2ds_v5Standard_D2pds_v5infrastructure failures from testcase failures.
Standard_D2ads_v6, including a three-VFconfiguration.
Final per-testcase matrix outcomes
These counts are final resolved outcomes after retries. Infrastructure/platform
blocks occurred before the testcase body and are not counted as testcase
failures.
1 -> 4, rejected4294967296, and restored4 -> 1on three VFsThe extra targeted CQE result is outside the 114-entry matrix total.
Matrix inventory
x64 Gen1 (4 images):
x64 Gen2 (11 images):
ARM64 Gen2 (4 images):
The Oracle Linux 9.4 image was unavailable/retired in
westus3; Oracle Linux9.8 was exercised as its current replacement.