-
Notifications
You must be signed in to change notification settings - Fork 223
Add support for AN NIC without a linked synthetic device #4074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| # Try to find the PCI slot for this NIC by checking its device path | ||
| result = self._node.execute( | ||
| f"readlink -f /sys/class/net/{nic_name}/device", | ||
| shell=True, | ||
| ) | ||
| if result.exit_code == 0 and result.stdout.strip(): | ||
| # Extract PCI slot from device path | ||
| # Path format: /sys/devices/.../XXXX:XX:XX.X/net/nicname | ||
| match = self.__pci_slot_pattern.search(result.stdout) | ||
| if match: | ||
| pci_slot = match.group(1) | ||
| # Get the module name for this PCI device | ||
| try: | ||
| module_name = lspci.get_used_module(pci_slot) | ||
| if module_name: | ||
| nic.pci_slot = pci_slot | ||
| # For standalone PCI NICs, set module_name directly | ||
| # (no lower_module_name since there's no lower device) | ||
| nic.module_name = module_name | ||
| self._node.log.debug( | ||
| f"Associated unpaired NIC {nic_name} " | ||
| f"with PCI slot {pci_slot} (module: {module_name})" | ||
| ) | ||
| else: | ||
| self._node.log.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is the right place for this code.
If it is the right place, is it sufficient to just move the below code to a tool?
result = self._node.execute(
f"readlink -f /sys/class/net/{nic_name}/device",
shell=True,
)
# Path format: /sys/devices/.../XXXX:XX:XX.X/net/nicname
match = self.__pci_slot_pattern.search(result.stdout)
if match:
pci_slot = match.group(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, use tool is always better.
32b76f6 to
c3414f5
Compare
Handle enable/disable test cases Handle PCI NIC count
c3414f5 to
daf2203
Compare
|
@adityagesh I've opened a new pull request, #4138, to work on those changes. Once the pull request is ready, I'll request review from you. |
daf2203 to
fd5635a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds comprehensive support for Accelerated Networking (AN) NICs that operate as standalone PCI devices without synthetic NIC pairing. This architectural enhancement enables LISA to handle three distinct NIC configurations: synthetic-only NICs, traditional paired NICs (synthetic+VF), and the newly supported PCI-only NICs.
Key changes:
- Introduces a
Readlinktool to replace directreadlinkcommand execution, providing proper error handling and abstraction - Refactors NIC detection logic to discover and handle standalone PCI NICs through device path analysis
- Updates network test suites to skip synthetic-specific tests when running on PCI-only NIC configurations
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lisa/tools/readlink.py | New tool for reading symbolic links with proper error handling and two methods: get_target() and get_canonical_path() |
| lisa/tools/lspci.py | Adds get_pci_slot_from_device_path() method to extract PCI slot information from sysfs device paths |
| lisa/tools/init.py | Exports new Readlink tool |
| lisa/nic.py | Core refactoring: adds is_pci_only_nic property, renames get_lower_nics() to get_pci_nics(), adds get_synthetic_devices(), implements _discover_standalone_pci_nics() for PCI-only NIC detection |
| lisa/microsoft/testsuites/network/synthetic.py | Adds synthetic NIC availability checks to skip tests on PCI-only configurations |
| lisa/microsoft/testsuites/network/stress.py | Adds skip conditions for synthetic and SRIOV disable/enable tests on PCI-only NICs |
| lisa/microsoft/testsuites/network/sriov.py | Adds skip conditions for SRIOV disable/enable tests on PCI-only NICs |
| lisa/microsoft/testsuites/network/networksettings.py | Adds synthetic NIC checks and updates to use is_pci_module_enabled and pci_device_name properties |
| lisa/microsoft/testsuites/network/common.py | Updates NIC selection logic to use is_pci_module_enabled instead of checking nic.lower |
| lisa/microsoft/testsuites/xdp/common.py | Replaces nic.lower with nic.pci_device_name for proper PCI device name retrieval |
| lisa/microsoft/testsuites/power/common.py | Updates calls from get_lower_nics() to get_pci_nics() |
| lisa/microsoft/testsuites/performance/common.py | Updates calls from get_lower_nics() to get_pci_nics() |
| lisa/microsoft/testsuites/core/provisioning.py | Updates calls from get_lower_nics() to get_pci_nics() and fixes string formatting |
| lisa/microsoft/testsuites/xfstests/xfstesting.py | Reorganizes imports to follow PEP 8 (stdlib/third-party before local imports) |
| lisa/microsoft/testsuites/ltp/ltpsuite.py | Reorganizes imports to follow PEP 8 |
| lisa/microsoft/testsuites/kselftest/kselftest-suite.py | Reorganizes imports to follow PEP 8 |
| for nic in self.nics.values(): | ||
| if nic.is_pci_only_nic: | ||
| pci_nics.append(nic.name) | ||
| else: |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method appends nic.lower for all non-PCI-only NICs, including synthetic NICs that have no lower device. This results in empty strings being added to the list. The logic should only append when there's an actual PCI device. Consider: if nic.is_pci_only_nic: pci_nics.append(nic.name) followed by elif nic.lower: pci_nics.append(nic.lower) to avoid adding empty strings for synthetic NICs without PCI devices.
| else: | |
| elif nic.lower: |
| f"Nic set was {self.nics.keys()} and only found info for " | ||
| f"{found_nics}" |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The string split across lines breaks PEP 8 convention unnecessarily. The original single-line f-string was under the typical 88-100 character limit for modern Python style. Consider reverting to a single line for better readability.
| canonicalize=True, | ||
| sudo=sudo, | ||
| no_error_log=no_error_log, | ||
| ) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new Readlink tool lacks test coverage. Consider adding unit tests to verify the tool's behavior for valid symbolic links, broken links, and the no_error_log parameter functionality, especially since this tool is now used in critical NIC discovery paths.
| def is_mana_driver_enabled(self) -> bool: | ||
| return self._node.tools[KernelConfig].is_enabled("CONFIG_MICROSOFT_MANA") | ||
|
|
||
| def _discover_standalone_pci_nics(self, lspci: Lspci) -> None: |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new _discover_standalone_pci_nics method, which is central to the PR's functionality, lacks integration test coverage. Consider adding tests that verify standalone PCI NIC discovery works correctly for PCI-only NIC configurations.
fd5635a to
e24ed5f
Compare
No description provided.