feat: Reduce machine inventory size by filtering irrelevant network interfaces - #4703
feat: Reduce machine inventory size by filtering irrelevant network interfaces#4703terickson-nvidia wants to merge 3 commits into
Conversation
Summary by CodeRabbit
WalkthroughThe hardware registration flow now filters auxiliary network interfaces. It removes interfaces with the ChangesAuxiliary Interface Filtering
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/scout/src/register.rs (1)
249-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table for interface classification cases.
These tests invoke
is_auxiliary_interfacerepeatedly with different inputs and expected results. Replace the separate cases with onevalue_scenarios!table. Keep the mixed-listretaintest standalone because it tests filtering integration.As per coding guidelines, use a table whenever two or more tests invoke the same operation with different inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/scout/src/register.rs` around lines 249 - 316, Replace the individual classification tests around is_auxiliary_interface with one value_scenarios! table containing each interface fixture and its expected boolean result. Preserve coverage for DPU PFs, regular host NICs, sentinel-MAC SFs, virtfn VFs, combined VF cases, and missing pci_properties; keep the mixed-list retain test standalone.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/scout/src/register.rs`:
- Around line 34-38: Update the is_vf_path calculation in register.rs to inspect
only the PCI portion of p.path and identify a virtfn* path component rather than
matching arbitrary text. Preserve physical interfaces such as paths ending in
/net/virtfn0, and add a regression case confirming that /net/virtfn0 remains
registered.
---
Nitpick comments:
In `@crates/scout/src/register.rs`:
- Around line 249-316: Replace the individual classification tests around
is_auxiliary_interface with one value_scenarios! table containing each interface
fixture and its expected boolean result. Preserve coverage for DPU PFs, regular
host NICs, sentinel-MAC SFs, virtfn VFs, combined VF cases, and missing
pci_properties; keep the mixed-list retain test standalone.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7dedca36-d711-43dd-90e6-60b506a0e3a4
📒 Files selected for processing (1)
crates/scout/src/register.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/scout/src/register.rs (1)
310-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven classification test.
is_auxiliary_interfaceis a total operation, and the test module invokes it with several different inputs. Addinterface_named_virtfn_with_real_mac_is_keptto the same table as the existing PF, SF, VF, and missing-property cases. Usevalue_scenarios!, or a direct table when the macro would obscure the cases.This removes repeated setup and keeps expected classifications together.
As per coding guidelines: “Use a table whenever two or more tests invoke the same operation with different inputs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/scout/src/register.rs` around lines 310 - 319, Consolidate the standalone interface_named_virtfn_with_real_mac_is_kept test into the existing table-driven classification test for is_auxiliary_interface, alongside the PF, SF, VF, and missing-property scenarios. Reuse the table’s established setup and add this input with an expected non-auxiliary result, using value_scenarios! or a direct table without obscuring the cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/scout/src/register.rs`:
- Around line 310-319: Consolidate the standalone
interface_named_virtfn_with_real_mac_is_kept test into the existing table-driven
classification test for is_auxiliary_interface, alongside the PF, SF, VF, and
missing-property scenarios. Reuse the table’s established setup and add this
input with an expected non-auxiliary result, using value_scenarios! or a direct
table without obscuring the cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 623008b6-3985-42be-b5d3-257be6869740
📒 Files selected for processing (1)
crates/scout/src/register.rs
thossain-nv
changed the title
2 participants
For better scaling, this PR reduces the amount of network interface data transmitted and persisted by filtering out interfaces that are not useful to the host inventory model. It reuses the existing filtering approach for InfiniBand interfaces by excluding interfaces that have
virtfnPCI path that indicates a virtual function (VF) regardless of mac addressThe filter is applied in scout. BMC-facing interfaces are already excluded by the existing filter in
enumerate_hardware_inner()before they reach scout's filter.Related issues
#2936
Type of Change
Breaking Changes
Testing
Additional Notes
The migration path for existing machines that already have oversized network interface inventory would be to trigger re-discovery.