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 Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (49)
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:
WalkthroughThe change replaces configured DPU modes with ChangesHost DPU policy migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExpectedMachine
participant SiteExplorer
participant BlueFieldEndpoint
participant BmcEndpointExplorer
ExpectedMachine->>SiteExplorer: resolve HostDpuPolicy
SiteExplorer->>BlueFieldEndpoint: inspect BlueFieldOperatingMode
SiteExplorer->>SiteExplorer: apply Ignore, UseAsNic, or Manage behavior
SiteExplorer->>BmcEndpointExplorer: configure target operating mode when needed
BmcEndpointExplorer->>BlueFieldEndpoint: set NIC or DPU mode
🚥 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
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/admin-cli/src/expected_machines/show/cmd.rs (1)
215-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
carbide-test-support's table-driven macros for consistency.This new test loops over a plain tuple array instead of
check_values/value_scenarios!, unlike every other DPU-policy test added in this PR (crates/api-model/src/expected_machine.rs,crates/rpc/src/model/expected_machine.rs).♻️ Proposed refactor
#[cfg(test)] mod tests { - use rpc::forge::HostDpuPolicy; + use carbide_test_support::{Check, check_values}; + use rpc::forge::HostDpuPolicy; use super::dpu_policy_display; #[test] fn dpu_policy_display_uses_canonical_policy_vocabulary() { - for (wire_value, expected) in [ - (None, "manage"), - (Some(HostDpuPolicy::Unspecified as i32), "manage"), - (Some(HostDpuPolicy::Manage as i32), "manage"), - (Some(HostDpuPolicy::UseAsNic as i32), "use-as-nic"), - (Some(HostDpuPolicy::Ignore as i32), "ignore"), - (Some(i32::MAX), "manage"), - ] { - assert_eq!(dpu_policy_display(wire_value), expected); - } + check_values( + [ + Check { scenario: "absent wire value", input: None, expect: "manage" }, + Check { scenario: "unspecified sentinel", input: Some(HostDpuPolicy::Unspecified as i32), expect: "manage" }, + Check { scenario: "manage", input: Some(HostDpuPolicy::Manage as i32), expect: "manage" }, + Check { scenario: "use as nic", input: Some(HostDpuPolicy::UseAsNic as i32), expect: "use-as-nic" }, + Check { scenario: "ignore", input: Some(HostDpuPolicy::Ignore as i32), expect: "ignore" }, + Check { scenario: "unknown discriminant", input: Some(i32::MAX), expect: "manage" }, + ], + dpu_policy_display, + ); } }As per coding guidelines, "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results. In Rust tests, prefer carbide-test-support's scenarios!, value_scenarios!, check_cases, and check_values appropriately."
🤖 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 215 - 234, Update the test dpu_policy_display_uses_canonical_policy_vocabulary to use carbide-test-support’s table-driven value_scenarios! or check_values macro, matching the patterns used by the other DPU-policy tests. Preserve all existing wire-value and expected-output cases, including unspecified, unknown, and None values.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/site_explorer.rs`:
- Around line 396-398: Update the doc comment above the relevant integration
test to state that hosts are skipped only when ExpectedMachine resolves to
HostDpuPolicy::Manage, while Ignore and UseAsNic bypass the strict zero-DPU
gate. Preserve the existing warning and NoDpuReportedByHost metric details.
---
Nitpick comments:
In `@crates/admin-cli/src/expected_machines/show/cmd.rs`:
- Around line 215-234: Update the test
dpu_policy_display_uses_canonical_policy_vocabulary to use
carbide-test-support’s table-driven value_scenarios! or check_values macro,
matching the patterns used by the other DPU-policy tests. Preserve all existing
wire-value and expected-output cases, including unspecified, unknown, and None
values.
🪄 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: ad1df007-a6e8-4564-be50-8a496d4c12d3
⛔ 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
…ce mode
`HostDpuPolicy::{Manage, UseAsNic, 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 stays backwards compatible at the input, wire, storage, and generated-client boundaries:
- Legacy `dpu_mode` fields, flags, and values continue to deserialize.
- The protobuf `DpuMode` and `NicMode` types, field names, symbols, and numeric meanings remain unchanged as compatibility boundaries.
- New Rust callers use policy- and observed-state aliases at those protobuf boundaries; existing generated Rust and Go clients retain their legacy names.
- The existing PostgreSQL column, type, and stored labels remain unchanged.
- Per-host `Manage` and omitted policies preserve the existing site-policy inheritance behavior.
- Observed mode continues to serialize as `DpuMode` and `NicMode`.
The canonical configuration and CLI vocabulary is now `dpu_policy` with `manage`, `use_as_nic`, and `ignore`.
Tests updated!
This supports NVIDIA#3447
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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
1 participant
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, UseAsNic, 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 stays backwards compatible at the input, wire, storage, and generated-client boundaries:
dpu_modefields, flags, and values continue to deserialize.DpuModeandNicModetypes, field names, symbols, and numeric meanings remain unchanged as compatibility boundaries.Manageand omitted policies preserve the existing site-policy inheritance behavior.DpuModeandNicMode.The canonical configuration and CLI vocabulary is now
dpu_policywithmanage,use_as_nic, andignore.Documentation is split into #3472 for separate technical-writer review.
Tests updated!
This supports #3447