feat: honor a declared primary NIC in the explored boot default#2721
Conversation
NVIDIA#2657 made a declared ExpectedHostNic.primary authoritative in machine_interfaces, the managed store every real boot path reads. This closes the matching gap in the explored default: the boot interface site-explorer records in explored_endpoints before any machine owns the endpoint, which a preingestion admin action (machine_setup, Restore) reads. That default came from an automatic, DPU-centric pick that filters to DPU host-PF MACs, so it could never name an integrated NIC -- a host that declared an integrated NIC as primary got a DPU NIC (or nothing) as its explored default. Now the declaration wins there too. fetch_host_primary_interface_mac takes the declared primary and returns it when this report has that NIC as a full pair (any type -- integrated or DPU host-PF); absent a declaration, today's lowest-PCI DPU-PF pick stands. The site-explorer caller feeds it the matched expected machine's declared primary, reusing NVIDIA#2657's declared_primary_mac(). The explored default and the managed store now agree on the declaration across the ownership handoff. - fetch_host_primary_interface_mac gains a declared_primary arg; a declared NIC present in the report wins whatever its type, via the existing find_interface_id_for_mac (which scans every system ethernet interface). - The exploration loop passes the matched expected machine's declared primary; the explored-default capture (id resolve / last-known-good) is untouched. - No declaration -> the explored default is unchanged from today. Tests extend test_fetch_host_primary_interface_mac: a declared DPU host-PF wins over the lowest-PCI pick, a declared integrated NIC becomes the default, a declared MAC absent from the report falls back, and the no-declaration pick is unchanged. Part of NVIDIA#2662 (epic NVIDIA#2660). Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
Walkthrough
ChangesDeclared Primary MAC Override in Boot Interface Selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai PTAL, thank you! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 2721 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions 2>/dev/null | head -100 || echo "Cannot access repo"
gh pr diff 2721 --repo NVIDIA/infra-controller 2>/dev/null | head -600 || echo "Cannot fetch diff"Length of output: 7900 🏁 Script executed: #!/bin/bash
# Fetch PR files list
gh pr view 2721 --repo NVIDIA/infra-controller --json files --jq '.files[].path' 2>/dev/null || echo "Cannot list files"Length of output: 317 🏁 Script executed: #!/bin/bash
# Read find_interface_id_for_mac implementation and any other relevant context
rg -n "find_interface_id_for_mac" --type rust -C5Length of output: 6800
// find_interface_id_for_mac (line 169-174)
.find(|e| e.mac_address == Some(mac))
.and_then(|e| e.id.as_deref().filter(|id| !id.is_empty())) // empty id → NoneSo in This is deliberately correct — /// A declared `ExpectedHostNic.primary` wins when this report has that NIC
/// *as a full pair* (MAC present in `ethernet_interfaces` with a non-empty
/// Redfish interface id), whatever its type ...
Everything else looks production-ready: the if-let chain is idiomatic, the fallback path is byte-for-byte preserved, and the signature change is fully contained within the updated callers. No blocking concerns. 🐇 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
What
The Store-A / site-explorer counterpart to #2657: a declared
ExpectedHostNic.primarynow wins in the explored boot default too -- the boot interface site-explorer records inexplored_endpointsbefore any machine owns the endpoint, which a preingestion admin action (machine_setup,set_dpu_first_boot_order, Restore) reads.fetch_host_primary_interface_mac(api-model/src/site_explorer/mod.rs) filtered NICs to DPU host-PF MACs, then picked lowest-PCI -- so it structurally couldn't name an integrated NIC. A host declaring an integrated NIC as primary got a DPU NIC (or nothing) as its explored default.Now the picker takes a
declared_primary: Option<MacAddress>and returns it when this report has that NIC as a full pair (reusing the existingfind_interface_id_for_mac, which scans every system ethernet interface -- integrated NICs included), whatever the NIC type. The site-explorer caller (lib.rs) feeds it the matched expected machine's declared primary viaExpectedMachineData::declared_primary_mac()(the resolver from #2657). Absent a declaration, today's lowest-PCI DPU-PF pick stands, byte-for-byte.Net: the explored default and the managed store (#2657) agree on the declaration across the ownership handoff, completing "declared primary is load-bearing everywhere."
Notes
fetch_host_primary_interface_macfrom its DPU-only assumption; the only caller islib.rs(the exploration loop) and the existing test, both updated. The no-declaration path is unchanged, so DPU-PF-assuming behavior is preserved.Test plan
cargo clippy --all-features --testsclean over the touched crates;cargo +nightly-2026-06-16 fmt --all.test_fetch_host_primary_interface_mac: declared DPU host-PF wins over the lowest-PCI pick; declared integrated NIC becomes the default (the headline case the DPU-only pick could never produce); a declared MAC absent from the report falls back to the DPU pick; no declaration → unchanged.Part of #2662 (epic #2660).