feat: discover host NICs through nv-redfish adapter ports - #4534
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe exploration flow retrieves network-adapter ports from linked chassis for eligible Lenovo systems. It stores port MAC addresses on network adapters, deduplicates report MACs, serializes the new field through API and RPC models, and adds database, simulator, and integration coverage. ChangesNetwork adapter port discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BMC
participant RedfishExplorer
participant ChassisExplorer
participant AdapterExplorer
participant EndpointReport
BMC->>RedfishExplorer: return system chassis links and Ethernet interfaces
RedfishExplorer->>ChassisExplorer: request ports for eligible linked chassis
ChassisExplorer->>BMC: fetch network adapters and ports
BMC-->>AdapterExplorer: return port resources
AdapterExplorer-->>ChassisExplorer: return deduplicated port MAC addresses
ChassisExplorer-->>EndpointReport: attach MACs to network adapters
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chet
changed the title
chet
marked this pull request as ready for review
August 4, 2026 20:30
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/bmc-explorer/tests/integration/network_adapter_port_explore.rs (1)
44-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd malformed and duplicate MAC test cases.
The implementation skips malformed port MAC values and deduplicates repeated valid values. The current table does not exercise either recovery path. Add cases that assert malformed input does not fail exploration and repeated MAC values produce one port-derived interface.
As per coding guidelines, “Prefer table-driven tests, using
scenarios!,value_scenarios!,check_cases, orcheck_valuesfor grouped input variants and thorough coverage.”🤖 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/bmc-explorer/tests/integration/network_adapter_port_explore.rs` around lines 44 - 100, Extend the table-driven cases in the port exploration test to cover a malformed port MAC that still allows exploration to succeed without producing an invalid interface, and repeated valid MAC values that result in exactly one port-derived interface. Use the existing case structure and assertions, or the project’s established scenario helpers such as scenarios! or check_cases, while preserving the current standard-data precedence coverage.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/bmc-explorer/tests/integration/network_adapter_port_explore.rs`:
- Around line 44-100: Extend the table-driven cases in the port exploration test
to cover a malformed port MAC that still allows exploration to succeed without
producing an invalid interface, and repeated valid MAC values that result in
exactly one port-derived interface. Use the existing case structure and
assertions, or the project’s established scenario helpers such as scenarios! or
check_cases, while preserving the current standard-data precedence coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de949f59-dfbd-4d39-8008-d6715c850b77
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rs
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 7, 2026 07:14
4797a02 to
937a485
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4534.docs.buildwithfern.com/infra-controller |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-07 07:16:40 UTC | Commit: 937a485 |
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 7, 2026 07:52
937a485 to
e61689e
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ
|
10 tasks
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 7, 2026 17:46
e61689e to
24a19cc
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
10 tasks
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 7, 2026 23:22
24a19cc to
594639f
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/bmc-explorer/src/lib.rs (1)
1190-1253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
value_scenarios!for this total predicate test.
should_use_network_adapter_port_fallbackmaps inputs to a boolean and cannot fail. Replace the manualCaseloop withvalue_scenarios!to follow the repository test convention.As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs or errors; use
scenarios!for fallible operations,value_scenarios!for total operations, and direct helpers when macros obscure intent.”🤖 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/bmc-explorer/src/lib.rs` around lines 1190 - 1253, The should_use_network_adapter_port_fallback test is a total input-to-boolean predicate but currently uses a manual Case loop. Replace the Case definition and iteration in lenovo_network_adapter_port_fallback_is_narrow with the repository’s value_scenarios! macro, preserving all existing input combinations, expected results, linked chassis construction, and failure labels.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/site-explorer/src/redfish.rs`:
- Line 282: Update the vendor handling around is_lenovo_xcc to retain and
inspect the vendor detected by service_root.vendor() before converting it to
BMCVendor, setting the flag for RedfishVendor::Lenovo while keeping LenovoAMI
and LenovoGB300 distinct. Add a LibRedfish test covering vendor: None with a
Lenovo XCC host.
---
Nitpick comments:
In `@crates/bmc-explorer/src/lib.rs`:
- Around line 1190-1253: The should_use_network_adapter_port_fallback test is a
total input-to-boolean predicate but currently uses a manual Case loop. Replace
the Case definition and iteration in
lenovo_network_adapter_port_fallback_is_narrow with the repository’s
value_scenarios! macro, preserving all existing input combinations, expected
results, linked chassis construction, and failure labels.
🪄 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: 85c4d70e-7e5c-4ddf-8e22-56c75f4bd00f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
Cargo.tomlcrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/health/src/collectors/entity_metrics.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 7, 2026 23:41
594639f to
f872999
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bmc-explorer/src/computer_system.rs`:
- Around line 132-139: The Ethernet MAC fallback gates must ignore disabled
interfaces. In crates/bmc-explorer/src/computer_system.rs:132-139, update
has_usable_ethernet_mac_address to require
interface.interface_enabled().unwrap_or(true) alongside a parseable MAC; in
crates/site-explorer/src/redfish.rs:1128-1140, update
should_fetch_network_adapter_ports to require interface.interface_enabled !=
Some(false). Preserve the existing fallback behavior for enabled or unspecified
interfaces.
🪄 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: cdf9df47-482d-41fd-99b2-06c184778d73
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (18)
crates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/bmc-explorer/src/computer_system.rs (1)
132-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBoth Lenovo fallback gates ignore disabled interfaces, so a placeholder MAC silently disables the fallback.
has_usable_ethernet_mac_addressandshould_fetch_network_adapter_portsboth treat any parseable MAC as usable, without checkinginterface_enabled. A disabled interface that reports a parseable placeholder MAC (for example00:00:00:00:00:00) makes both gates report a usable System MAC, so the Lenovo network-adapter-port fallback never triggers even though the value is not a real host MAC. This gap was raised in a past review with a concrete reproduction and remains unresolved.
crates/bmc-explorer/src/computer_system.rs#L132-L139: requireinterface.interface_enabled().unwrap_or(true)alongside a parseable MAC inhas_usable_ethernet_mac_address.crates/site-explorer/src/redfish.rs#L1131-L1143: requireinterface.interface_enabled != Some(false)alongsideinterface.mac_address.is_some()inshould_fetch_network_adapter_ports.Add a disabled-interface-with-placeholder-MAC scenario to the existing test tables in both files (
lenovo_network_adapter_port_fallback_is_narrowandnetwork_adapter_port_fallback_gate_cases) to lock in the fix.🤖 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/bmc-explorer/src/computer_system.rs` around lines 132 - 139, The Lenovo fallback gates must ignore disabled interfaces with placeholder MACs. In crates/bmc-explorer/src/computer_system.rs:132-139, update has_usable_ethernet_mac_address to require interface.interface_enabled().unwrap_or(true) alongside a parseable MAC; in crates/site-explorer/src/redfish.rs:1131-1143, update should_fetch_network_adapter_ports to require interface.interface_enabled != Some(false) alongside a present MAC. Add disabled-interface placeholder-MAC cases to lenovo_network_adapter_port_fallback_is_narrow and network_adapter_port_fallback_gate_cases.
🤖 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.
Duplicate comments:
In `@crates/bmc-explorer/src/computer_system.rs`:
- Around line 132-139: The Lenovo fallback gates must ignore disabled interfaces
with placeholder MACs. In crates/bmc-explorer/src/computer_system.rs:132-139,
update has_usable_ethernet_mac_address to require
interface.interface_enabled().unwrap_or(true) alongside a parseable MAC; in
crates/site-explorer/src/redfish.rs:1131-1143, update
should_fetch_network_adapter_ports to require interface.interface_enabled !=
Some(false) alongside a present MAC. Add disabled-interface placeholder-MAC
cases to lenovo_network_adapter_port_fallback_is_narrow and
network_adapter_port_fallback_gate_cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 806d9c8b-166a-4531-9fef-9a921d0d1733
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (18)
crates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 10, 2026 18:48
183cbbe to
15f4727
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ
|
Contributor
Author
|
@coderabbitai full_review if you can, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-model/src/site_explorer/mod.rs (1)
2565-2594: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for
all_mac_addresses.This test maps an
EndpointExplorationReportinput to MAC-address output. Usevalue_scenarios!orcheck_valuesso additional ordering and duplicate cases are simple to add.As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs or errors.”
🤖 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/api-model/src/site_explorer/mod.rs` around lines 2565 - 2594, Refactor all_mac_addresses_combines_system_and_adapter_inventory_without_duplicates into a table-driven test using the repository’s value_scenarios! or check_values helper. Represent each EndpointExplorationReport input and expected MAC-address output as a scenario, preserving the current ordering and duplicate-removal assertion while making additional cases easy to add.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/api-model/src/site_explorer/mod.rs`:
- Around line 2565-2594: Refactor
all_mac_addresses_combines_system_and_adapter_inventory_without_duplicates into
a table-driven test using the repository’s value_scenarios! or check_values
helper. Represent each EndpointExplorationReport input and expected MAC-address
output as a scenario, preserving the current ordering and duplicate-removal
assertion while making additional cases easy to add.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c68cb694-8f42-4023-86c2-416201892d7b
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (18)
crates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
bcavnvidia
approved these changes
Aug 10, 2026
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 10, 2026 21:49
15f4727 to
f0864f2
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-model/src/site_explorer/mod.rs (1)
2565-2595: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the test to actually exercise chassis MAC chaining.
The test seeds
system_macandadapter_macin bothsystems[0].ethernet_interfacesandchassis[0].network_adapters[0].port_mac_addresses. Because the two sources contain the exact same MACs, the assertionassert_eq!(report.all_mac_addresses(), vec![system_mac, adapter_mac])passes even if the.chain(...)call overself.chassisis removed fromall_mac_addresses. The test name states it verifies combining system and adapter inventory, but it only proves deduplication, not that adapter MACs are actually included.Use a MAC that exists only in
port_mac_addressesand not in anyethernet_interfaces, so the assertion fails if the chassis chain is dropped.🧪 Proposed fix to make the test meaningful
fn all_mac_addresses_combines_system_and_adapter_inventory_without_duplicates() { let system_mac = "02:aa:bb:cc:dd:01".parse().unwrap(); let adapter_mac = "94:6d:ae:53:cb:9b".parse().unwrap(); + let unique_adapter_mac = "02:aa:bb:cc:dd:02".parse().unwrap(); let report = EndpointExplorationReport { systems: vec![ComputerSystem { ethernet_interfaces: vec![ EthernetInterface { mac_address: Some(system_mac), ..Default::default() }, EthernetInterface { mac_address: Some(adapter_mac), ..Default::default() }, ], ..Default::default() }], chassis: vec![Chassis { network_adapters: vec![NetworkAdapter { - port_mac_addresses: vec![system_mac, adapter_mac], + port_mac_addresses: vec![adapter_mac, unique_adapter_mac], ..Default::default() }], ..Default::default() }], ..Default::default() }; - assert_eq!(report.all_mac_addresses(), vec![system_mac, adapter_mac]); + assert_eq!( + report.all_mac_addresses(), + vec![system_mac, adapter_mac, unique_adapter_mac] + ); }🤖 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/api-model/src/site_explorer/mod.rs` around lines 2565 - 2595, Update the test all_mac_addresses_combines_system_and_adapter_inventory_without_duplicates so at least one MAC address exists only in chassis[0].network_adapters[0].port_mac_addresses and is absent from systems[0].ethernet_interfaces. Adjust the expected vector to include that chassis-only address while preserving duplicate coverage, ensuring the assertion fails if all_mac_addresses removes the self.chassis chain.
🤖 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/api-model/src/site_explorer/mod.rs`:
- Around line 2565-2595: Update the test
all_mac_addresses_combines_system_and_adapter_inventory_without_duplicates so at
least one MAC address exists only in
chassis[0].network_adapters[0].port_mac_addresses and is absent from
systems[0].ethernet_interfaces. Adjust the expected vector to include that
chassis-only address while preserving duplicate coverage, ensuring the assertion
fails if all_mac_addresses removes the self.chassis chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7925a129-99c7-4d09-8bf3-7c4ec2370e5b
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
crates/api-db/migrations/20260810143726_index_explored_endpoint_port_macs.sqlcrates/api-db/src/explored_endpoints.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
Site Explorer's default nv-redfish path can now find an ordinary host NIC when the System EthernetInterfaces collection has no usable MACs but the chassis NetworkAdapter Ports do. The fallback keeps System interfaces authoritative, prefers standard Port MACs before Lenovo OEM data, and fetches Port links independently so one failed member does not hide valid siblings. This supports NVIDIA#4469 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-4469-nv-redfish
branch
from
August 11, 2026 00:04
f0864f2 to
c346085
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/bmc-explorer/src/network_adapter.rs (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth the nv-redfish and legacy libredfish port-MAC extraction paths dedup MAC addresses with the same
if !result.contains(&mac_address) { result.push(mac_address); }pattern, and neither path has a test that proves this guard actually collapses a repeated MAC into a single entry. The referenced STYLE_GUIDE.md documentation explicitly requires conversion tests for "populated, duplicate, and empty port MAC lists"; the "duplicate" case is missing at both sites.
crates/bmc-explorer/src/network_adapter.rs#L172-219: add an integration test case (incrates/bmc-explorer/tests/integration/network_adapter_port_explore.rs) where two ports on the same adapter (or one port'sAssociatedMACAddresseslist) report the same MAC address, and assert the adapter'sport_mac_addressescontains exactly one entry.crates/site-explorer/src/redfish.rs#L1145-1201: add the equivalent case forfetch_network_adapter_port_mac_addresses, asserting a single deduplicated entry when the simulator returns a repeated MAC across ports.As per path instructions, STYLE_GUIDE.md states: "add or update focused conversion tests for populated, duplicate, and empty port MAC lists."
🤖 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/bmc-explorer/src/network_adapter.rs` at line 1, Add focused duplicate-MAC conversion tests for both network adapter paths: the integration coverage around the port exploration flow in network_adapter_port_explore.rs and the fetch_network_adapter_port_mac_addresses test flow in redfish.rs. Configure repeated MAC data across ports or within AssociatedMACAddresses, then assert port_mac_addresses contains exactly one entry; preserve existing populated and empty cases.Source: Path instructions
172-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a duplicate-MAC test case for
port_mac_addresses.
port_mac_addressesdedups MAC addresses across ports withif !result.contains(&mac_address) { result.push(mac_address); }, but no test exercises two ports (or one port'sAssociatedMACAddresseslist) reporting the same MAC. Add a case to the integration suite that supplies a duplicate MAC across two ports and asserts a single entry in the result.This finding is expanded in the consolidated comment covering the parallel implementation in
crates/site-explorer/src/redfish.rs.
As per path instructions, STYLE_GUIDE.md states: "add or update focused conversion tests for populated, duplicate, and empty port MAC lists."🤖 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/bmc-explorer/src/network_adapter.rs` around lines 172 - 219, Add a focused integration test for NetworkAdapter::port_mac_addresses that provides the same MAC through two distinct Port resources and asserts the returned vector contains that MAC exactly once. Keep the test alongside the existing populated/empty port MAC conversion tests and preserve first-seen ordering.Source: Path instructions
🤖 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/bmc-explorer/src/network_adapter.rs`:
- Line 1: Add focused duplicate-MAC conversion tests for both network adapter
paths: the integration coverage around the port exploration flow in
network_adapter_port_explore.rs and the fetch_network_adapter_port_mac_addresses
test flow in redfish.rs. Configure repeated MAC data across ports or within
AssociatedMACAddresses, then assert port_mac_addresses contains exactly one
entry; preserve existing populated and empty cases.
- Around line 172-219: Add a focused integration test for
NetworkAdapter::port_mac_addresses that provides the same MAC through two
distinct Port resources and asserts the returned vector contains that MAC
exactly once. Keep the test alongside the existing populated/empty port MAC
conversion tests and preserve first-seen ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a3749a7b-773b-4848-afbb-e0ba92b9c16d
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
crates/api-db/migrations/20260810143726_index_explored_endpoint_port_macs.sqlcrates/api-db/src/explored_endpoints.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/network_adapter.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/integration/bluefield3_explore.rscrates/bmc-explorer/tests/integration/main.rscrates/bmc-explorer/tests/integration/network_adapter_port_explore.rscrates/bmc-mock/src/test_support/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/site_explorer.protocrates/rpc/src/model/site_explorer.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/redfish.rsrest-api/proto/core/src/v1/site_explorer_nico.proto
bcavnvidia
approved these changes
Aug 11, 2026
Sinck
pushed a commit
to Sinck/infra-controller
that referenced
this pull request
Aug 11, 2026
10 tasks
nv-dmendoza
pushed a commit
that referenced
this pull request
Aug 11, 2026
> [!IMPORTANT] > This PR cherry-picks two commits into `release/v2.1`: > - c79d042 (#4534) -- discover host NICs through nv-redfish adapter ports > - e9c13b3 (#4788) -- verify pull request secret-scan ranges Site Explorer's default nv-redfish path can now find an ordinary host NIC when the System EthernetInterfaces collection has no usable MACs but the chassis NetworkAdapter Ports do. The fallback keeps System interfaces authoritative, prefers standard Port MACs before Lenovo OEM data, and fetches Port links independently so one failed member does not hide valid siblings. #4788 rides along because it fixes the secret-scan failure this branch hits. `copy-pr-bot` can rewrite its synthetic PR branch, so `github.event.before` is unusable and TruffleHog falls back to scanning the whole repository -- on this branch that surfaced 117 unrelated findings and failed `REST Secret Scan with TruffleHog` (and with it the `rest-ci-pass` rollup) on every recent `release/v2.1` PR, merged ones included. The resolver computes the current PR's merge-base and head for both Core and REST synthetic scans, so the scan is scoped to this PR's own commits. ## Related issues This supports #4469 and #4786 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) `cargo test -p bmc-explorer` (30 passed, including the new `network_adapter_port_explore` suite) and `cargo test -p carbide-site-explorer 'redfish::tests::' --lib` (6 passed) pass on the branch. `bash .github/ci/test-resolve-pr-scan-range.sh` passes, and both workflow files still parse. ## Additional Notes **#4534 -- three conflicts, all visibility drift.** `release/v2.1` does not have #4662 (`refactor(bmc/console): adopt style guide rules for pub/module visibility`), so `bmc-explorer` here still uses `pub` where `main` uses `pub(crate)`. `chassis.rs` and `network_adapter.rs` keep this branch's `pub` and take only the new `fetch_network_adapter_ports` method and `ports` field; #4662 is style-only and is intentionally not pulled in. `bluefield3_explore.rs` conflicted because #4534's new test anchors directly after `explore_bluefield3_ignores_invalid_system_interface_mac`, which belongs to a different commit that is not on this branch -- only #4534's own `explore_bluefield3_preserves_oem_mode_and_base_mac` is added here. **#4788 -- one conflict in `.github/workflows/ci.yaml`,** because this branch already carries #4715's earlier Core-only workaround. Resolved to keep release-specific workflow behavior: #4788's resolver now backs synthetic PR scans in both Core (`ci.yaml`) and REST (`rest-ci.yml`), the existing Core tag-scan branch is preserved, and the resolver plus its focused test come along. The surrounding CI steps in that hunk (`check-ci-permissions.sh`, `check-core-ci-permissions.sh`, `check-stale-ci-permissions.sh`, `check-ci-concurrency.sh`, `test_check_ci_gate.py`, `check_ci_gate.py`) are from other commits and reference scripts that do not exist on this branch, so they are deliberately left out -- every script referenced by `ci.yaml` here resolves. **Migration is additive and safe.** `20260810143726_index_explored_endpoint_port_macs.sql` sorts after this branch's latest migration (`20260731143022`), so it appends without disturbing existing checksums. It drops and recreates `explored_endpoints_mac_addresses_idx`, and that index's pre-state is byte-identical on `release/v2.1` and `main` (both from `20260708172302_squash_snapshot.sql`), so the rebuild behaves the same here. **No dependency bump needed.** The adapter-port support this relies on is already on the branch -- `nv-redfish` is pinned at `0.14.2`, matching `main` (via #4785).
This was referenced Aug 13, 2026
chet
added a commit
that referenced
this pull request
Aug 14, 2026
…4968) Lenovo XCC can report usable onboard `ComputerSystem.EthernetInterfaces` while exposing an installed ConnectX NIC only through a linked chassis `NetworkAdapter.Port`. The adapter-Port fetch treated any usable System MAC as proof that inventory was complete, so the declared NoDpu boot NIC never reached `predicted_machine_interfaces`. So, this leaves the existing no-usable-System-MAC fallback intact and adds a separate supplemental case inside the same verified Lenovo + `ComputerSystem.Links.Chassis` boundary. That case collects adapter Ports even when usable System interfaces exist. Site Explorer still uses System interfaces as Host candidates; it adds a Port MAC alongside them only when that MAC was actually reported by hardware and `ExpectedMachine` declares it as a Host interface. It does not synthesize `EthernetInterfaces`, treat the declaration as an override, or use a Port ID as a boot-interface ID. A refreshed report can now add that MAC to an existing predicted host, keep its boot target MAC-only, and leave already-managed hosts alone. Retained boot metadata is consulted only when a predicted host has no primary, so an unrelated stale record cannot replace a settled primary. This means verified Lenovo XCC hosts fetch Ports from the explicitly linked chassis on each exploration. That extra inventory work stays behind the vendor + linked-chassis boundary because a generic chassis Port is not necessarily a host/PXE interface. Tests added! ## Related issues This supports #4952 ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ```bash cargo test -p bmc-explorer -p carbide-site-explorer --lib make core/tests TEST_ARGS="-p carbide-site-explorer --test integration zero_dpu::test_ -- --nocapture" cargo make clippy carbide-lints --all-targets --all-features cargo make format-nightly cargo make check-licenses cargo make check-bans cargo xtask check-workspace-deps taplo fmt --check crates/site-explorer/Cargo.toml ``` ## Additional Notes This is the caller-side follow-up to #4469 and #4534, using the adapter-Port inventory exposed by [NVIDIA/nv-redfish#180](NVIDIA/nv-redfish#180). Signed-off-by: Chet Nichols III <chetn@nvidia.com>
10 tasks
nv-dmendoza
pushed a commit
that referenced
this pull request
Aug 14, 2026
> [!IMPORTANT] > This PR cherry-picks commit 1badba5 (#4968) into `release/v2.1`. Lenovo XCC can report usable onboard `ComputerSystem.EthernetInterfaces` while exposing an installed ConnectX NIC only through a linked chassis `NetworkAdapter.Port`. The adapter-Port fetch treated any System MAC as proof that inventory was complete, so the declared NoDpu boot NIC never reached `predicted_machine_interfaces`. So, this keeps the verified Lenovo + `ComputerSystem.Links.Chassis` boundary, but collects adapter Ports as supplemental inventory even when System interfaces exist. Site Explorer still uses System interfaces as Host candidates; it adds a Port MAC alongside them only when that MAC was actually reported by hardware and `ExpectedMachine` declares it as a Host interface. It does not synthesize `EthernetInterfaces`, treat the declaration as an override, or use a Port ID as a boot-interface ID. A refreshed report can now add that MAC to an existing predicted host, keep its boot target MAC-only, and leave already-managed hosts alone. Retained boot metadata is consulted only when a predicted host has no primary, so an unrelated stale record cannot replace a settled primary. ## Related issues This supports #4952 ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) `cargo test -p bmc-explorer -p carbide-site-explorer --lib` and the `zero_dpu` integration tests pass on the branch. ## Additional Notes **One conflict, in `crates/bmc-explorer/src/lib.rs`.** It was the test module's `use super::{...}` list: the incoming side also imports `should_fetch_bf4_chassis_except_irot_nic`, which #4968 does not add and this branch does not have -- it comes from the BlueField-4 IRoT chassis work on `main`. Resolved to this branch's existing names plus #4968's new `should_fetch_supplemental_network_adapter_ports`; nothing else references the BF4 helper here. The rest of the pick applied cleanly. **Prerequisite is already on this branch.** #4968 builds on the adapter-Port fetch from #4534, which landed here in #4832, so `fetch_network_adapter_ports` is present and this is a true follow-on rather than a partial backport. `crates/site-explorer` picks up an `axum` dev dependency; it resolves against this branch's existing workspace pin (`0.8.4`), so no `Cargo.toml` workspace change was needed.
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
2 participants
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.
This pulls together NVIDIA/nv-redfish#180, NVIDIA/nv-redfish#185, and NVIDIA/libredfish#118.
As it stood, Site Explorer's default
nv-redfishpath could only match a host NIC from the SystemEthernetInterfacescollection. Lenovo XCC can leave that collection without a usable MAC while reporting the physical address throughNetworkAdapterPorts, which meant the NIC was in Redfish but never made it into host matching.So, this teaches the default exploration path to fetch those adapter ports when the platform supports the fallback, the
ComputerSystemexplicitly links the chassis, and there is no usable System MAC. An interface only blocks the fallback when it is enabled (or omitsInterfaceEnabled) and has a parseable MAC -- a disabled placeholder no longer hides the linked Port data.The Port MACs stay on their
NetworkAdapterinventory whileEndpointExplorationReport::all_mac_addresses()exposes them to host matching. Persisted endpoint lookup indexes that inventory too, so early PXE and the other MAC-based lookup paths can find the host before it is owned. We still don't fabricate System interfaces or treatPort.Idas a boot selector, which keeps the workaround pretty tightly scoped.Related issues
This supports #4469
Type of Change
Breaking Changes
Testing
cargo test -p bmc-explorercargo test -p carbide-site-explorer 'redfish::tests::' --libcargo test -p carbide-site-explorer default_nvredfish_mode_does_not_apply_lenovo_fallback_to_generic_ami --libcargo test -p carbide-api-model all_mac_addresses --libcargo test -p carbide-api-db find_by_mac_address_includes_adapter_ports --lib -- --nocaptureAdditional Notes
Standard
AssociatedMACAddressesdata wins when it contains a usable MAC. Lenovo'sPhysicalPortMacAddressis only read as a fallback, and Port links are fetched independently so one failed member does not hide valid siblings or fail the rest of BMC exploration.Closes #4469