refactor(api): clean separation of host DPU policy from observed device mode#3453
Open
chet wants to merge 1 commit into
Open
refactor(api): clean separation of host DPU policy from observed device mode#3453chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
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:
WalkthroughThis change replaces legacy DPU modes with host and site DPU policies, preserves RPC and configuration compatibility, distinguishes policy from observed BlueField operating mode, and applies resolved policies to Site Explorer ingestion, CLI/API wiring, machine registration, persistence, and tests. ChangesHost DPU policy migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant ExpectedMachine
participant SiteExplorer
participant BlueField
participant Database
Config->>ExpectedMachine: provide site and per-host policy
ExpectedMachine->>SiteExplorer: resolve effective HostDpuPolicy
SiteExplorer->>BlueField: inspect and optionally set BlueFieldOperatingMode
SiteExplorer->>Database: persist host, DPU, and boot-interface state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/site-explorer/src/lib.rs (1)
3342-3342: 📐 Maintainability & Code Quality | 🔵 TrivialLog field renamed from
host_dpu_modetohost_dpu_policy.Structured
logfmtsearches keyed on the oldhost_dpu_modefield (dashboards/alerts) will silently stop matching after this rename. Worth a quick check that no saved queries or alert rules reference the old key before merging.🤖 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/site-explorer/src/lib.rs` at line 3342, Check the structured logging configuration around the host DPU field and update any dashboards, saved queries, or alert rules that still reference host_dpu_mode to use host_dpu_policy, preserving the renamed log field in the implementation.
🤖 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/site-explorer/src/lib.rs`:
- Line 3342: Check the structured logging configuration around the host DPU
field and update any dashboards, saved queries, or alert rules that still
reference host_dpu_mode to use host_dpu_policy, preserving the renamed log field
in the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 882d20b3-a647-4fc0-927a-40286cb382c4
📒 Files selected for processing (2)
crates/api-model/src/expected_machine.rscrates/site-explorer/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
@coderabbitai Hey! Just making sure the full_review didn't surface any findings? Looks like it didn't? Lemme know. Thanks! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 3453 --repo NVIDIA/infra-controller --json comments,reviews --jq '.reviews[] | {author: .author.login, body: .body, state: .state}'
echo "---review comments---"
gh api repos/NVIDIA/infra-controller/pulls/3453/comments --jq '.[] | {user: .user.login, path: .path, body: .body}'Length of output: 3377
No actionable/major issues were raised on the ヽ(・∀・)ﾉ |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3453.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-07-14 04:20:07 UTC | Commit: ab33263 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/admin-cli/src/expected_machines/add/args.rs`:
- Around line 167-174: The dpu_policy argument preserves the legacy flag name
but not its legacy value strings. Update HostDpuPolicy’s clap value metadata to
alias each historical DPU mode value to its corresponding current value,
ensuring old --dpu-mode invocations still parse; alternatively remove the help
text claim that legacy values remain accepted.
In `@crates/admin-cli/src/expected_machines/show/cmd.rs`:
- Around line 160-169: Update the dpu_policy_display conversion in the
expected-machine rendering flow so a successfully decoded
HostDpuPolicy::Unspecified value is explicitly converted to
HostDpuPolicy::Manage before calling to_possible_value(). Preserve the existing
fallback to Manage for absent or invalid wire values and keep the kebab-case
name generation unchanged.
In `@crates/api-core/src/tests/common/api_fixtures/site_explorer.rs`:
- Around line 1501-1505: Update the zero-DPU handling around config.dpus and
data.dpu_policy so it does not claim to preserve every explicit policy when
HostDpuPolicy::Manage cannot distinguish an omitted value from an explicit
Manage value. Either track whether dpu_policy was explicitly provided before
rewriting the default, or narrow the nearby comment and behavior to preserve
only explicitly configured non-Manage policies.
🪄 Autofix (Beta)
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: 3f38556b-6fae-437c-b998-67c9f44f0619
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (42)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/config.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsdocs/glossary.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/manuals/vpc/flat_vpcs_zero_dpu.mddocs/observability/core_metrics.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/ingesting-hosts.mdrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/site-explorer/src/lib.rs
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/admin-cli/src/expected_machines/show/cmd.rs (1)
160-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill doesn't normalize
UnspecifiedtoManage.
try_from(0)succeeds and returnsSome(HostDpuPolicy::Unspecified), so.unwrap_or(Manage)only covers the "field absent" case, not an explicit wire0. IfUnspecifiedis a skippedValueEnumvariant (typical for proto defaults),to_possible_value()returnsNoneand the table cell renders empty instead of "manage" — the same gap flagged on the predecessordpu_mode_display/DpuModeversion of this code.🐛 Proposed fix
let dpu_policy_display = expected_machine .dpu_mode .and_then(|i| ::rpc::forge::HostDpuPolicy::try_from(i).ok()) + .filter(|policy| *policy != ::rpc::forge::HostDpuPolicy::Unspecified) .unwrap_or(::rpc::forge::HostDpuPolicy::Manage) .to_possible_value() .map(|pv| pv.get_name().to_owned()) .unwrap_or_default();🤖 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/admin-cli/src/expected_machines/show/cmd.rs` around lines 160 - 169, Update the dpu_policy_display conversion to normalize both an absent value and an explicitly decoded HostDpuPolicy::Unspecified to HostDpuPolicy::Manage before calling to_possible_value(). Preserve the existing kebab-case name extraction and default behavior for other valid policies.crates/admin-cli/src/expected_machines/add/args.rs (1)
167-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConfirm
HostDpuPolicycarries clap value aliases for the legacy strings.
visible_alias = "dpu-mode"only preserves the old flag name; it does nothing for the old value strings. The help text claims "legacy--dpu-modeflag and values remain accepted," anddocs/manuals/vpc/flat_vpcs_zero_dpu.mdrepeats the same claim fordpu-mode/nic-mode/no-dpu. UnlessHostDpuPolicy's#[derive(ValueEnum)](defined outside this file, likely incrates/rpc) attaches#[value(alias = "nic-mode")]/#[value(alias = "no-dpu")]/#[value(alias = "dpu-mode")]per variant,--dpu-mode nic-modewill fail to parse — breaking the PR's stated backward-compatibility guarantee for legacy CLI values. A prior review round on this same code shape flagged exactly this gap as unresolved.#!/bin/bash set -euo pipefail rg -n --hidden -S 'enum HostDpuPolicy' crates -g '*.rs' -A 25 rg -n --hidden -S '#\[value\(alias' crates -g '*.rs'🤖 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/admin-cli/src/expected_machines/add/args.rs` around lines 167 - 174, Update the HostDpuPolicy ValueEnum definition to attach clap value aliases for each legacy value: nic-mode, no-dpu, and dpu-mode, mapped to their corresponding current variants. Ensure parsing accepts these values through the legacy --dpu-mode flag while preserving the current canonical values and the dpu_policy argument declaration.
🧹 Nitpick comments (1)
crates/admin-cli/src/expected_machines/tests.rs (1)
500-599: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an
addregression case for the legacy--dpu-modealiasThe parser still accepts
--dpu-modeonadd, but the table only exercises the legacy values onpatch. Add alegacy add --dpu-mode ...scenario to keep the documented alias covered.🤖 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/admin-cli/src/expected_machines/tests.rs` around lines 500 - 599, Add a regression scenario to parse_dpu_policy_to_its_variant in crates/admin-cli/src/expected_machines/tests.rs (lines 500-599) covering add with the legacy --dpu-mode alias and asserting it maps to the corresponding HostDpuPolicy variant; no direct change is required in docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md (lines 117-133).
🤖 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/admin-cli/src/expected_machines/add/args.rs`:
- Around line 167-174: Update the HostDpuPolicy ValueEnum definition to attach
clap value aliases for each legacy value: nic-mode, no-dpu, and dpu-mode, mapped
to their corresponding current variants. Ensure parsing accepts these values
through the legacy --dpu-mode flag while preserving the current canonical values
and the dpu_policy argument declaration.
In `@crates/admin-cli/src/expected_machines/show/cmd.rs`:
- Around line 160-169: Update the dpu_policy_display conversion to normalize
both an absent value and an explicitly decoded HostDpuPolicy::Unspecified to
HostDpuPolicy::Manage before calling to_possible_value(). Preserve the existing
kebab-case name extraction and default behavior for other valid policies.
---
Nitpick comments:
In `@crates/admin-cli/src/expected_machines/tests.rs`:
- Around line 500-599: Add a regression scenario to
parse_dpu_policy_to_its_variant in
crates/admin-cli/src/expected_machines/tests.rs (lines 500-599) covering add
with the legacy --dpu-mode alias and asserting it maps to the corresponding
HostDpuPolicy variant; no direct change is required in
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
(lines 117-133).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77a51f7f-bb40-4bf2-ab5a-3b83ac9bb987
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (42)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/config.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsdocs/glossary.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/manuals/vpc/flat_vpcs_zero_dpu.mddocs/observability/core_metrics.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/ingesting-hosts.mdrest-api/proto/core/src/v1/nico_nico.proto
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/rpc/proto/forge.proto`:
- Around line 6006-6034: Keep ExpectedMachine.dpu_mode declared with the
existing forge.DpuMode type instead of changing it to forge.HostDpuPolicy, while
preserving the compatible enum values and aliases. If the type change is
required, add the repository’s explicit breaking-change waiver for that field so
the buf breaking check accepts it.
In `@crates/site-explorer/src/lib.rs`:
- Around line 1274-1289: The primary boot-interface lookup currently occurs
after the HostDpuPolicy::Ignore early path, so ignored hosts do not refresh
last_explored.boot_interface_mac. Move the boot_interfaces lookup and update
logic before the Ignore check, alongside the existing nic_boot_interfaces
handling, while preserving the Ignore path’s remaining behavior.
In `@crates/site-explorer/src/metrics.rs`:
- Around line 48-49: Update the documentation for the blocker in the metrics
definition to explicitly identify the host policy as Manage, replacing the
broader “not Ignore” wording. Keep the description clear that it applies when
managed DPUs are expected and does not imply UseAsNic is invalid.
🪄 Autofix (Beta)
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: 77a51f7f-bb40-4bf2-ab5a-3b83ac9bb987
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (42)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/config.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsdocs/glossary.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/manuals/vpc/flat_vpcs_zero_dpu.mddocs/observability/core_metrics.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/ingesting-hosts.mdrest-api/proto/core/src/v1/nico_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/site-explorer/src/lib.rs (1)
1274-1298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignore-policy hosts still skip the primary boot-interface refresh (past review comment only half-fixed).A prior review flagged that
HostDpuPolicy::Ignore(previouslyDpuMode) skips the block that refreshes the host's boot-interface hint, and asked to hoist it above the earlycontinue— matching thenic_boot_interfacestreatment. This diff hoistsnic_boot_interfaces.extend(...)(Line 1251) above theIgnorefast-path, but the primary boot-interface computation (fetch_host_primary_interface_mac→boot_interfaces.push(...), Lines 1572-1627) still lives after theIgnorecontinue(Line 1298), so it never runs forIgnorehosts.Consequence:
db::explored_endpoints::set_boot_interfaceis never called forIgnorehosts, soexplored_endpoints.boot_interface_mac/boot_interface_idare never (re)populated, and the nextexplore_endpoint(...)call loses its "last known boot interface" hint for these hosts — contradicting the stated intent ("including a zero-DPU host whose primary boots from a plain NIC") that now only actually holds fornic_boot_interfaces.Hoist the primary-interface lookup above the
Ignorecontinue too (it can safely use an empty DPU list, sinceIgnorehosts never have managed DPUs), symmetric to thenic_boot_interfacesfix.🐛 Proposed fix outline
nic_boot_interfaces.extend(ep.report.complete_boot_interfaces()); + // Resolve/record the primary boot interface before the `Ignore` + // fast-path continue, so zero-DPU-by-policy hosts still get their + // `explored_endpoints.boot_interface_mac`/`boot_interface_id` hint + // refreshed for the next site-explorer iteration. + let declared_primary = expected_explored_endpoint_index + .matched_expected_machine(&ep.address) + .and_then(|expected| expected.data.declared_primary_mac()); + if let Some(mac_address) = ep + .report + .fetch_host_primary_interface_mac(&[], declared_primary) + && let Some(interface_id) = ep.report.find_interface_id_for_mac(mac_address) + { + boot_interfaces.push(( + ep.address, + MachineBootInterface { mac_address, interface_id: interface_id.to_string() }, + )); + } + let host_dpu_policy = effective_policy(&ep.address); if matches!(host_dpu_policy, HostDpuPolicy::Ignore) { ... continue; }Note: the later (non-
Ignore) primary-interface block still needs to run with the realdpus_explored_for_host, so this hoisted call is additive, not a full replacement — dedupe carefully to avoid double-pushing for non-Ignorehosts.Also applies to: 1563-1627
🤖 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/site-explorer/src/lib.rs` around lines 1274 - 1298, Move the primary-interface lookup using fetch_host_primary_interface_mac above the HostDpuPolicy::Ignore early continue, passing an empty DPU list and recording its result in boot_interfaces so Ignore hosts refresh their boot-interface hint. Retain the existing lookup for non-Ignore hosts with dpus_explored_for_host, but guard or structure the flow to avoid duplicate boot-interface entries.
🧹 Nitpick comments (1)
crates/rpc/src/model/site_explorer.rs (1)
181-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated enum mapping.
Both conversions re-implement the identical
Dpu/Nicmapping (one viaas i32, the other via.into()). Extracting a singleFrom<model::BlueFieldOperatingMode> for rpc::site_explorer::BlueFieldOperatingModeimpl removes the duplication and keeps both call sites in sync if a variant is ever added.♻️ Proposed consolidation
+impl From<BlueFieldOperatingMode> for rpc::site_explorer::BlueFieldOperatingMode { + fn from(mode: BlueFieldOperatingMode) -> Self { + match mode { + BlueFieldOperatingMode::Dpu => rpc::site_explorer::BlueFieldOperatingMode::Dpu, + BlueFieldOperatingMode::Nic => rpc::site_explorer::BlueFieldOperatingMode::Nic, + } + } +} + impl From<ExploredMlxDevice> for rpc::site_explorer::ExploredMlxDevice { fn from(device: ExploredMlxDevice) -> Self { rpc::site_explorer::ExploredMlxDevice { ... - nic_mode: device.nic_mode.map(|m| match m { - BlueFieldOperatingMode::Nic => { - rpc::site_explorer::BlueFieldOperatingMode::Nic as i32 - } - BlueFieldOperatingMode::Dpu => { - rpc::site_explorer::BlueFieldOperatingMode::Dpu as i32 - } - }), + nic_mode: device.nic_mode.map(|m| rpc::site_explorer::BlueFieldOperatingMode::from(m) as i32), } } } impl From<ComputerSystemAttributes> for rpc::site_explorer::ComputerSystemAttributes { fn from(attributes: ComputerSystemAttributes) -> Self { rpc::site_explorer::ComputerSystemAttributes { - nic_mode: attributes.nic_mode.map(|a| match a { - BlueFieldOperatingMode::Nic => { - rpc::site_explorer::BlueFieldOperatingMode::Nic.into() - } - BlueFieldOperatingMode::Dpu => { - rpc::site_explorer::BlueFieldOperatingMode::Dpu.into() - } - }), + nic_mode: attributes.nic_mode.map(|a| rpc::site_explorer::BlueFieldOperatingMode::from(a).into()), } } }🤖 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/rpc/src/model/site_explorer.rs` around lines 181 - 206, Extract the duplicated BlueFieldOperatingMode conversion into a single From<model::BlueFieldOperatingMode> for rpc::site_explorer::BlueFieldOperatingMode implementation. Update the nic_mode mappings in the device and ComputerSystemAttributes conversions to reuse this conversion, preserving the existing Nic and Dpu mappings and numeric output.
🤖 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/site-explorer/src/lib.rs`:
- Around line 1274-1298: Move the primary-interface lookup using
fetch_host_primary_interface_mac above the HostDpuPolicy::Ignore early continue,
passing an empty DPU list and recording its result in boot_interfaces so Ignore
hosts refresh their boot-interface hint. Retain the existing lookup for
non-Ignore hosts with dpus_explored_for_host, but guard or structure the flow to
avoid duplicate boot-interface entries.
---
Nitpick comments:
In `@crates/rpc/src/model/site_explorer.rs`:
- Around line 181-206: Extract the duplicated BlueFieldOperatingMode conversion
into a single From<model::BlueFieldOperatingMode> for
rpc::site_explorer::BlueFieldOperatingMode implementation. Update the nic_mode
mappings in the device and ComputerSystemAttributes conversions to reuse this
conversion, preserving the existing Nic and Dpu mappings and numeric output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f09cdc3b-0ecf-4670-adc2-c9e63e5530ca
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (49)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/libredfish/conv.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (24)
- crates/api-db/src/expected_machine/tests.rs
- crates/api-core/src/cfg/README.md
- crates/api-core/src/tests/machine_interfaces.rs
- crates/admin-cli/src/rpc.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/machine-a-tron/src/api_client.rs
- crates/machine-a-tron/src/machine_a_tron.rs
- crates/site-explorer/src/machine_creator.rs
- crates/site-explorer/src/metrics.rs
- crates/admin-cli/src/expected_machines/show/cmd.rs
- crates/api-db/src/expected_machine.rs
- crates/machine-controller/src/handler.rs
- crates/api-core/src/tests/sku.rs
- crates/site-explorer/tests/integration/zero_dpu.rs
- crates/api-db/src/machine_interface.rs
- crates/api-core/src/cfg/file.rs
- crates/site-explorer/src/config.rs
- crates/admin-cli/src/expected_machines/patch/args.rs
- crates/rpc/src/model/expected_machine.rs
- crates/api-core/src/tests/common/api_fixtures/site_explorer.rs
- crates/admin-cli/src/expected_machines/add/args.rs
- crates/api-model/src/expected_machine.rs
- crates/admin-cli/src/expected_machines/tests.rs
- crates/site-explorer/tests/integration/site_explorer.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/site-explorer/src/lib.rs (1)
1274-1299: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignorehosts still skip boot-interface refresh. Thecontinuehere still bypasses thedeclared_primary/fetch_host_primary_interface_mac/boot_interfaces.push(...)path, soexplored_endpoints.boot_interface_*is never refreshed for zero-DPU hosts even when a primary NIC is resolvable. Hoist the boot-interface lookup above this fast-path, or make the asymmetry explicit ifIgnoreshould not update that hint.🤖 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/site-explorer/src/lib.rs` around lines 1274 - 1299, The HostDpuPolicy::Ignore fast path bypasses boot-interface refresh for zero-DPU hosts. Update the surrounding exploration flow so declared_primary and fetch_host_primary_interface_mac run before this continue, and boot_interfaces.push updates explored_endpoints.boot_interface_* when the primary NIC is resolvable; otherwise explicitly preserve the intended no-refresh behavior.
🤖 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.
Outside diff comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 1274-1299: The HostDpuPolicy::Ignore fast path bypasses
boot-interface refresh for zero-DPU hosts. Update the surrounding exploration
flow so declared_primary and fetch_host_primary_interface_mac run before this
continue, and boot_interfaces.push updates explored_endpoints.boot_interface_*
when the primary NIC is resolvable; otherwise explicitly preserve the intended
no-refresh behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dbfef57c-f48a-403c-b63e-ac7072c20ed0
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (49)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/libredfish/conv.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-core/src/cfg/README.md (1)
313-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSlightly confusing repetition of
dpu_modein the legacy-compat note.The sentence "The legacy
dpu_modefield anddpu_mode/nic_mode/no_dpuvalues remain accepted during deserialization" usesdpu_modetwice with two different meanings (the legacy field name vs. the legacy value alias formanage), which can read as a typo/duplication rather than two distinct facts.✏️ Suggested rewording for clarity
-| `dpu_policy` | `Option<HostDpuPolicy>` | — | Site-wide policy for DPU hardware: `manage`, `use_as_nic`, or `ignore`. Per-host `use_as_nic` and `ignore` override it; per-host `manage` inherits it for backward compatibility. When omitted, the site default is `manage`. The legacy `dpu_mode` field and `dpu_mode` / `nic_mode` / `no_dpu` values remain accepted during deserialization. | +| `dpu_policy` | `Option<HostDpuPolicy>` | — | Site-wide policy for DPU hardware: `manage`, `use_as_nic`, or `ignore`. Per-host `use_as_nic` and `ignore` override it; per-host `manage` inherits it for backward compatibility. When omitted, the site default is `manage`. The legacy field name `dpu_mode`, and its legacy enum values `dpu_mode` / `nic_mode` / `no_dpu`, remain accepted during deserialization. |As per path instructions, "Review Markdown for correctness, clarity, spelling, grammar, working links, and whether commands/examples are realistic and safe."
🤖 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-core/src/cfg/README.md` at line 313, Clarify the legacy-compatibility note in the dpu_policy documentation by distinguishing the legacy dpu_mode field from the accepted legacy value aliases, avoiding repeated ambiguous use of dpu_mode while preserving all listed compatibility behavior.Source: Path instructions
crates/site-explorer/tests/integration/site_explorer.rs (1)
2658-2659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the duplicated local imports
In each regression test, the function-local
useonly needsHostDpuPolicy;ExpectedMachine,ExpectedMachineData, andBlueFieldOperatingModeare already in module scope. Keeping the local import to the new symbol reduces noise without changing behavior.🤖 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/site-explorer/tests/integration/site_explorer.rs` around lines 2658 - 2659, Update the function-local imports in each regression test to import only HostDpuPolicy. Remove ExpectedMachine, ExpectedMachineData, and BlueFieldOperatingMode from those local use statements, relying on their existing module-scope imports without changing test behavior.
🤖 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-core/src/cfg/README.md`:
- Line 313: Clarify the legacy-compatibility note in the dpu_policy
documentation by distinguishing the legacy dpu_mode field from the accepted
legacy value aliases, avoiding repeated ambiguous use of dpu_mode while
preserving all listed compatibility behavior.
In `@crates/site-explorer/tests/integration/site_explorer.rs`:
- Around line 2658-2659: Update the function-local imports in each regression
test to import only HostDpuPolicy. Remove ExpectedMachine, ExpectedMachineData,
and BlueFieldOperatingMode from those local use statements, relying on their
existing module-scope imports without changing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 42625445-91a6-4f77-beab-644890b074f2
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (49)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/libredfish/conv.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (42)
- crates/admin-cli/src/expected_machines/update/mod.rs
- crates/api-core/src/tests/machine_interfaces.rs
- crates/rpc/build.rs
- crates/api-db/src/expected_machine/tests.rs
- crates/api-model/src/test_support/dpu.rs
- crates/admin-cli/src/rpc.rs
- crates/api-db/src/expected_machine.rs
- crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
- crates/site-explorer/src/endpoint_explorer.rs
- crates/api-core/src/tests/common/api_fixtures/site_explorer.rs
- crates/admin-cli/src/expected_machines/show/cmd.rs
- crates/api-model/src/test_support/machine_snapshot.rs
- crates/rpc/proto/forge.proto
- crates/machine-a-tron/src/api_client.rs
- crates/admin-cli/src/expected_machines/patch/mod.rs
- crates/site-explorer/src/machine_creator.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/machine-a-tron/src/machine_a_tron.rs
- crates/preingestion-manager/src/lib.rs
- crates/redfish/src/libredfish/conv.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/site-explorer/src/metrics.rs
- crates/machine-controller/src/handler.rs
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/api-core/src/tests/sku.rs
- crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs
- crates/api-core/tests/integration/explored_mlx_devices.rs
- crates/api-core/src/tests/preingestion_dpu_nic_mode.rs
- crates/site-explorer/src/config.rs
- crates/rpc/src/protos/mod.rs
- crates/admin-cli/src/expected_machines/common.rs
- crates/api-db/src/machine_interface.rs
- crates/api-core/src/cfg/file.rs
- crates/admin-cli/src/expected_machines/add/args.rs
- crates/api-model/src/expected_machine.rs
- crates/site-explorer/src/bmc_endpoint_explorer.rs
- crates/admin-cli/src/expected_machines/patch/args.rs
- crates/rpc/src/model/site_explorer.rs
- crates/api-model/src/site_explorer/mod.rs
- crates/admin-cli/src/expected_machines/tests.rs
- crates/rpc/src/model/expected_machine.rs
- crates/site-explorer/src/lib.rs
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/rpc/src/model/site_explorer.rs (1)
500-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a table-driven check for the enum conversion.
Express the
Dpu/Nicconversion cases throughcheck_casesorvalue_scenarios!rather than separate assertions. As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs, errors, or observable results.”🤖 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/rpc/src/model/site_explorer.rs` around lines 500 - 511, Replace the separate BlueFieldOperatingMode conversion assertions in the existing test with a table-driven check using check_cases or value_scenarios!, covering both Dpu and Nic inputs and their expected rpc::site_explorer::BlueFieldOperatingMode outputs. Keep the existing as_str_name assertion unchanged.Source: Coding guidelines
crates/api-model/src/expected_machine.rs (1)
514-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock canonical policy serialization across both Serde boundaries.
The compatibility suites validate legacy input but do not protect canonical output with serialize→deserialize round trips.
crates/api-model/src/expected_machine.rs#L514-L567: verify model policies serialize asmanage,use_as_nic, andignore, including default/omittedExpectedMachinebehavior.crates/rpc/src/model/expected_machine.rs#L453-L476: verify the RPC alias emits the same canonical strings and round-trips each variant.As per path instructions, conversion and Serde compatibility tests must include round-trips and boundary/unset behavior.
🤖 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/expected_machine.rs` around lines 514 - 567, The compatibility tests around host_dpu_policy_deserializes_new_and_legacy_values and expected_machine_deserializes_new_and_legacy_policy_fields must also lock canonical serialization. In crates/api-model/src/expected_machine.rs lines 514-567, add serialize→deserialize round trips for every HostDpuPolicy variant, assert canonical strings manage, use_as_nic, and ignore, and cover omitted/default ExpectedMachine behavior. In crates/rpc/src/model/expected_machine.rs lines 453-476, add equivalent canonical-string assertions and round trips for the RPC alias; no other conversion behavior needs changing.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/api-model/src/expected_machine.rs`:
- Around line 514-567: The compatibility tests around
host_dpu_policy_deserializes_new_and_legacy_values and
expected_machine_deserializes_new_and_legacy_policy_fields must also lock
canonical serialization. In crates/api-model/src/expected_machine.rs lines
514-567, add serialize→deserialize round trips for every HostDpuPolicy variant,
assert canonical strings manage, use_as_nic, and ignore, and cover
omitted/default ExpectedMachine behavior. In
crates/rpc/src/model/expected_machine.rs lines 453-476, add equivalent
canonical-string assertions and round trips for the RPC alias; no other
conversion behavior needs changing.
In `@crates/rpc/src/model/site_explorer.rs`:
- Around line 500-511: Replace the separate BlueFieldOperatingMode conversion
assertions in the existing test with a table-driven check using check_cases or
value_scenarios!, covering both Dpu and Nic inputs and their expected
rpc::site_explorer::BlueFieldOperatingMode outputs. Keep the existing
as_str_name assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 36f001c5-464e-44ca-b40e-04e074bf3cde
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (49)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/libredfish/conv.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
Contributor
|
✅ Action performedFull review finished. |
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/machine-a-tron/src/api_client.rs (1)
526-530: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
Fromconversion.This duplicates the
HostDpuPolicy -> rpc::forge::DpuModemapping already defined incrates/rpc/src/model/expected_machine.rs. Userpc::forge::DpuMode::from(policy) as i32to prevent mapping drift.Proposed refactor
- dpu_mode: dpu_policy.map(|policy| match policy { - HostDpuPolicy::Manage => rpc::forge::DpuMode::DpuMode as i32, - HostDpuPolicy::UseAsNic => rpc::forge::DpuMode::NicMode as i32, - HostDpuPolicy::Ignore => rpc::forge::DpuMode::NoDpu as i32, - }), + dpu_mode: dpu_policy.map(|policy| rpc::forge::DpuMode::from(policy) as i32),🤖 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/machine-a-tron/src/api_client.rs` around lines 526 - 530, Replace the inline match in the dpu_mode mapping with the shared HostDpuPolicy conversion, using rpc::forge::DpuMode::from(policy) as i32 within the existing dpu_policy.map closure. Preserve the current optional mapping behavior.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/tests/integration/zero_dpu.rs`:
- Around line 369-370: Update the comment describing the zero-DPU scenario near
register_zero_dpu_expected_machine to match the helper’s seeded
HostDpuPolicy::Ignore value, replacing the incorrect UseAsNic reference. Keep
the test behavior and helper unchanged.
---
Nitpick comments:
In `@crates/machine-a-tron/src/api_client.rs`:
- Around line 526-530: Replace the inline match in the dpu_mode mapping with the
shared HostDpuPolicy conversion, using rpc::forge::DpuMode::from(policy) as i32
within the existing dpu_policy.map closure. Preserve the current optional
mapping behavior.
🪄 Autofix (Beta)
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: 4e1fea5f-f8d2-437a-9aa8-ee227e38159d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (54)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/replace_all/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/Cargo.tomlcrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/boot_interface.rscrates/redfish/src/libredfish/conv.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
chet
force-pushed
the
gh-issue-3447
branch
2 times, most recently
from
July 16, 2026 05:30
2fec248 to
0681024
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/site-explorer/src/lib.rs (1)
1620-1645: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWarning message mislabels the gate as "not
Ignore" — it's actuallyManage-specific.The surrounding comment (L1621-1634) correctly explains the effective policy is
Manage, but the emittedtracing::warn!text says "its DPU policy is notIgnore" — which is also true forUseAsNic, a policy that does not trigger this blocker. The suggested remediation only offersdpu_policy = "ignore", butuse_as_nicachieves the same zero-DPU opt-in and should be mentioned too.✏️ Suggested wording fix
- "cannot identify managed host: site explorer sees no DPUs on this host and its DPU policy is not `Ignore`; declare `dpu_policy = \"ignore\"` to ingest as zero-DPU", + "cannot identify managed host: site explorer sees no DPUs on this host and its DPU policy resolves to `Manage` (the default); declare `dpu_policy = \"ignore\"` or `\"use_as_nic\"` to ingest as zero-DPU",🤖 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/site-explorer/src/lib.rs` around lines 1620 - 1645, Update the tracing::warn! message in the host DPU policy branch guarded by host_dpu_policy.expects_managed_dpus() so it identifies the blocking policy as Manage-specific rather than saying the policy is merely not Ignore. Include both Ignore and UseAsNic as valid zero-DPU opt-in alternatives in the remediation text.crates/site-explorer/src/metrics.rs (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBoth sites describe the zero-DPU gate as "not
Ignore" instead of naming the actual gated policy,Manage.UseAsNicis also "notIgnore" yet is never gated byNoDpuReportedByHost(onlyManage, viaexpects_managed_dpus(), triggers it) — the imprecise wording risks steering operators towardignorewhenuse_as_nicmay be the correct choice. The accurate phrasing already exists incrates/site-explorer/tests/integration/site_explorer.rs#L395-398.
crates/site-explorer/src/metrics.rs#L35-52: reword theNoDpuReportedByHostdoc comment to say the effective policy "resolves toManage(the default)" rather than "is notIgnore".crates/site-explorer/src/lib.rs#L1620-1645: reword thetracing::warn!message at L1639 to nameManageexplicitly, and mention bothignoreanduse_as_nicas valid zero-DPU opt-ins.🤖 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/site-explorer/src/metrics.rs` at line 1, Update the NoDpuReportedByHost documentation in metrics.rs to state that the policy resolves to Manage by default, replacing the imprecise “not Ignore” wording. Update the tracing::warn! message in the relevant lib.rs handling to name Manage explicitly and identify both ignore and use_as_nic as valid zero-DPU opt-ins, matching the established integration-test phrasing.
♻️ Duplicate comments (1)
crates/site-explorer/src/metrics.rs (1)
35-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc still says "not
Ignore" — should say "Manage" (the actual gate).
UseAsNicis also "notIgnore", yetcrates/site-explorer/src/lib.rsonly firesNoDpuReportedByHostwhenhost_dpu_policy.expects_managed_dpus()is true (i.e.,Manage). This is the exact concern raised in a prior round on this same range, which was marked addressed — but the wording here is unchanged. Compare with the corrected doc incrates/site-explorer/tests/integration/site_explorer.rs("resolves toManage").✏️ Suggested wording fix
- /// Host BMC reports no BlueField PCIe devices but the host policy is not - /// `Ignore`. We expect DPUs but didn't + /// Host BMC reports no BlueField PCIe devices and the effective host + /// policy resolves to `Manage` (the default). We expect DPUs but didn't /// find any -- likely a misconfiguration or DPU-discovery bug.🤖 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/site-explorer/src/metrics.rs` around lines 35 - 52, Update the NoDpuReportedByHost documentation to say the host policy resolves to `Manage`, replacing the inaccurate “not `Ignore`” wording. Keep the explanation aligned with the expects_managed_dpus() gate and preserve the existing rationale about missing expected DPUs.
🧹 Nitpick comments (1)
crates/api-core/src/cfg/file.rs (1)
4223-4241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s scenario macros for these mapping tests.
Replace the handwritten case loops with
scenarios!for fallible Figment/serde operations andvalue_scenarios!for total mappings.
crates/api-core/src/cfg/file.rs#L4223-L4241: express policy parsing cases withscenarios!.crates/api-core/src/cfg/file.rs#L4249-L4275: express layered-precedence cases withscenarios!.crates/api-core/src/cfg/file.rs#L4280-L4303: express serialization cases withscenarios!.crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs#L203-L254: expressoperating_as_niccases withvalue_scenarios!.As per coding guidelines, mapping tests must use carbide-test-support scenario helpers. As per path instructions, formatting and serde-boundary logic should use the repository’s table-driven test conventions.
🤖 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-core/src/cfg/file.rs` around lines 4223 - 4241, The mapping tests currently use handwritten case loops instead of repository scenario helpers. In crates/api-core/src/cfg/file.rs ranges 4223-4241, 4249-4275, and 4280-4303, replace the policy parsing, layered-precedence, and serialization case tables with scenarios! while preserving their existing fallible Figment/serde assertions; in crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs range 203-254, replace the operating_as_nic mapping cases with value_scenarios! for the total mapping.Sources: Coding guidelines, 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.
Outside diff comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 1620-1645: Update the tracing::warn! message in the host DPU
policy branch guarded by host_dpu_policy.expects_managed_dpus() so it identifies
the blocking policy as Manage-specific rather than saying the policy is merely
not Ignore. Include both Ignore and UseAsNic as valid zero-DPU opt-in
alternatives in the remediation text.
In `@crates/site-explorer/src/metrics.rs`:
- Line 1: Update the NoDpuReportedByHost documentation in metrics.rs to state
that the policy resolves to Manage by default, replacing the imprecise “not
Ignore” wording. Update the tracing::warn! message in the relevant lib.rs
handling to name Manage explicitly and identify both ignore and use_as_nic as
valid zero-DPU opt-ins, matching the established integration-test phrasing.
---
Duplicate comments:
In `@crates/site-explorer/src/metrics.rs`:
- Around line 35-52: Update the NoDpuReportedByHost documentation to say the
host policy resolves to `Manage`, replacing the inaccurate “not `Ignore`”
wording. Keep the explanation aligned with the expects_managed_dpus() gate and
preserve the existing rationale about missing expected DPUs.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4223-4241: The mapping tests currently use handwritten case loops
instead of repository scenario helpers. In crates/api-core/src/cfg/file.rs
ranges 4223-4241, 4249-4275, and 4280-4303, replace the policy parsing,
layered-precedence, and serialization case tables with scenarios! while
preserving their existing fallible Figment/serde assertions; in
crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs range 203-254, replace the
operating_as_nic mapping cases with value_scenarios! for the total mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a852967-5061-4698-9c28-c9e3a60f47bc
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (54)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/replace_all/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/Cargo.tomlcrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/boot_interface.rscrates/redfish/src/libredfish/conv.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/admin-cli/src/expected_machines/add/args.rs`:
- Around line 169-175: Update the help text for the dpu_policy argument to state
that explicit manage selects normal DPU management, while omitting the option
inherits the site-wide [site_explorer] dpu_policy setting. Keep the existing
descriptions for use-as-nic, ignore, and legacy --dpu-mode mappings unchanged.
In `@crates/admin-cli/src/expected_machines/patch/args.rs`:
- Line 194: Update the help text for the per-host DPU policy argument in the
argument definition containing the help string so `manage` is described as
explicitly managing DPUs, not inheriting site policy. Clarify that only an unset
value preserves or inherits the existing site/per-host policy, while retaining
the descriptions for `use-as-nic`, `ignore`, and legacy `--dpu-mode` mappings.
In `@crates/api-model/src/machine_boot_interface.rs`:
- Around line 29-33: Revise the documentation near MachineBootInterface and
from_parts so it does not claim interface_id can be used alone after a
transition. Describe the fallback only when both interface_id and mac_address
remain available, unless the model is first changed to support an optional MAC
or separate identifier type.
🪄 Autofix (Beta)
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: fbaa2f4e-22f7-45bc-a6f1-4f7ae1d433b2
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (54)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/replace_all/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/Cargo.tomlcrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/boot_interface.rscrates/redfish/src/libredfish/conv.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/site-explorer/src/lib.rs (1)
3476-3524: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not treat an unknown mode as satisfying
UseAsNic.When
host_dpu_policyisUseAsNicbutbluefield_operating_mode()returnsNone, this function returnsOk(true). The caller can then register a zero-DPU host without confirming or applying NIC mode, leaving an attached BlueField operating as a DPU. Block ingestion or explicitly apply the requested NIC mode instead of reporting successful convergence.🤖 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/site-explorer/src/lib.rs` around lines 3476 - 3524, The None branch of the bluefield_operating_mode match must not report success when host_dpu_policy is UseAsNic. Update this branch to block ingestion or invoke set_nic_mode with BlueFieldOperatingMode::Nic, and only preserve the existing Ok(true) behavior for policies where an unknown observed mode is acceptable.
🤖 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/api-model/src/expected_machine.rs`:
- Around line 78-92: The flattened dpu_policy field needs map-shaped
serialization to match its custom deserialization. Add a serialize_with handler
for dpu_policy alongside deserialize_host_dpu_policy that emits the canonical
field structure, and add coverage for ExpectedMachineData serialization
confirming dpu_policy is encoded successfully under its canonical name.
---
Outside diff comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 3476-3524: The None branch of the bluefield_operating_mode match
must not report success when host_dpu_policy is UseAsNic. Update this branch to
block ingestion or invoke set_nic_mode with BlueFieldOperatingMode::Nic, and
only preserve the existing Ok(true) behavior for policies where an unknown
observed mode is acceptable.
🪄 Autofix (Beta)
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: 42beb26b-6bb1-4da4-a718-95884b3db9e3
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockrest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (54)
crates/admin-cli/src/expected_machines/add/args.rscrates/admin-cli/src/expected_machines/common.rscrates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/replace_all/mod.rscrates/admin-cli/src/expected_machines/show/cmd.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/test_support/fixture_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/preingestion_dpu_nic_mode.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/explored_mlx_devices.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/machine_interface.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/machine_snapshot.rscrates/bmc-explorer/src/computer_system.rscrates/machine-a-tron/Cargo.tomlcrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/machine-controller/src/handler.rscrates/preingestion-manager/src/lib.rscrates/redfish/src/boot_interface.rscrates/redfish/src/libredfish/conv.rscrates/rpc/proto/forge.protocrates/rpc/src/model/expected_machine.rscrates/rpc/src/model/site_explorer.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/config.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/metrics.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rsrest-api/proto/core/src/v1/nico_nico.proto
Contributor
Author
|
Good catch: this is a real pre-existing behavior, not a regression from this refactor. origin/main has the same DpuMode::NicMode plus observed None path returning Ok(true); this PR maps it 1:1 to HostDpuPolicy::UseAsNic. Fixing it needs a separate behavioral change because BF4 intentionally has no observable mode and is allowed to pair, while returning false here would incorrectly drive a reset without having issued a mode change. I am leaving this PR behavior-compatible and deferring unknown-mode reconciliation to a focused follow-up with direct- and fallback-match coverage. |
7 tasks
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
chet
force-pushed
the
gh-issue-3447
branch
3 times, most recently
from
July 17, 2026 06:20
74935d1 to
98ebf04
Compare
wminckler
approved these changes
Jul 17, 2026
…ce mode
`HostDpuPolicy::{Manage, Nic, Ignore}` is now the policy vocabulary across per-host and site configuration, the API model, clients, and site-explorer. Observed hardware state is `BlueFieldOperatingMode::{Dpu, Nic}`, so intent no longer shares a type name with what a card reports.
This remains backwards compatible across the configuration, wire, storage, and generated-client boundaries:
- `ExpectedMachine.dpu_mode` remains protobuf field 16 with the original `DpuMode` symbols and numeric values. RPC code translates that stable shell directly into `HostDpuPolicy`.
- Missing, unspecified, and unknown wire values retain the historical `Manage` default and site-policy inheritance behavior.
- Configuration and admin JSON serialize the canonical `manage`, `nic`, and `ignore` values while accepting the previous `use_as_nic` value and legacy `dpu_mode` key and values. Provider-local normalization also preserves global < site < environment precedence when old and new keys are mixed.
- The CLI exposes `--dpu-policy manage|nic|ignore` while accepting the previous `use-as-nic` value and legacy `--dpu-mode dpu-mode|nic-mode|no-dpu` spellings.
- Old generated clients and old servers continue to interoperate because clients send only the unchanged field 16 compatibility surface.
- The protobuf and Redfish `NicMode` boundary retains its symbols and numeric meanings; model code uses `BlueFieldOperatingMode` for the observed state.
- The existing PostgreSQL column, type, and stored labels remain unchanged.
`Ignore` hosts with a declared primary NIC also refresh a stale boot-interface pair before their early exit, keeping zero-DPU boot targeting aligned with the other policy paths.
Tests updated!
This supports NVIDIA#3447
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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.
HostDpuPolicy::{Manage, Nic, Ignore}is now the policy vocabulary across per-host and site configuration, the API model, clients, and site-explorer. Observed hardware state isBlueFieldOperatingMode::{Dpu, Nic}, so intent no longer shares a type name with what a card reports.This remains backwards compatible across the configuration, wire, storage, and generated-client boundaries:
ExpectedMachine.dpu_modefield 16 and the originalDpuModesymbols and numeric values. RPC code translates that stable compatibility shell directly intoHostDpuPolicy.Managedefault and site-policy inheritance behavior.manage,nic, andignorevalues while continuing to accept the previoususe_as_nicvalue and legacydpu_modekey and values.--dpu-policy manage|nic|ignore, accepts the previoususe-as-nicvalue and legacy--dpu-mode dpu-mode|nic-mode|no-dpuspellings, and sends only the unchanged field 16 surface so new clients continue to work with old servers.NicModeboundary retains its symbols and numeric meanings; model code usesBlueFieldOperatingModefor observed state.registered_zero_dpu_for_nic_modemetric label remains unchanged.There is deliberately no additive field 19 and no permanent dual-write compatibility window.
The
Ignorediscovery path also refreshes a stale boot-interface pair from a declared primary host NIC when one is available. This keeps zero-DPU hosts on the same primary-interface behavior as the other policy paths.Validation includes focused model, configuration, admin CLI, RPC wire/descriptor, PostgreSQL decoding, and metric-label compatibility tests; the exact configuration-documentation regression test; nightly formatting; full workspace Clippy; custom Carbide lints; and independent reviews.
Documentation is split into #3473 for separate technical-writer review and must merge after this code PR.
Tests updated!
This supports #3447