feat: rename ExpectedHostNic to ExpectedInterface - #4390
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (34)
Summary by CodeRabbit
WalkthroughExpected-machine host NIC declarations are generalized to ChangesExpected interface migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RPCConversion
participant ExpectedMachineModel
participant Database
participant DHCPReconciliation
CLI->>RPCConversion: Parse --interfaces or --host_nics
RPCConversion->>ExpectedMachineModel: Convert ExpectedInterface values
ExpectedMachineModel->>Database: Persist interfaces in host_nics JSON column
DHCPReconciliation->>Database: Find machine by interface MAC
Database-->>DHCPReconciliation: Return ExpectedInterface declarations
DHCPReconciliation->>ExpectedMachineModel: Apply role, allocation, segment, and primary settings
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/rpc/build.rsast-grep timed out on this file Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rpc/build.rs (1)
634-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename-only field attribute is safe today, but only because
ExpectedMachinenever derivesDeserialize.
#[serde(rename = "interfaces")]onExpectedMachine.host_nicsrenames the JSON key outright — it does notaliasthe legacy"host_nics"key. This is safe right now becauseExpectedMachineonly derivesserde::Serialize(line 633), notDeserialize, so nothing can ever feed a raw"host_nics"-keyed JSON payload into this type directly; all legacy-JSON compatibility is instead handled one layer up inExpectedMachineJson/ExpectedMachineDataviaalias = "host_nics".If
Deserializeis ever added toExpectedMachinein a future change, this rename would silently stop accepting legacy"host_nics"payloads at that layer. Consider a short comment here noting the invariant so a future contributor doesn't addDeserializewithout also adding the alias.🤖 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/build.rs` around lines 634 - 637, Add a concise comment beside the ExpectedMachine.host_nics serde rename in the field_attribute configuration, documenting that ExpectedMachine currently only derives Serialize and that adding Deserialize requires an alias for the legacy "host_nics" key. Do not change the rename behavior or the existing ExpectedMachineJson/ExpectedMachineData compatibility handling.
🤖 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/rpc/build.rs`:
- Around line 634-637: Add a concise comment beside the
ExpectedMachine.host_nics serde rename in the field_attribute configuration,
documenting that ExpectedMachine currently only derives Serialize and that
adding Deserialize requires an alias for the legacy "host_nics" key. Do not
change the rename behavior or the existing
ExpectedMachineJson/ExpectedMachineData compatibility handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4def9189-2679-4f17-b4dd-299f7995c6a2
📒 Files selected for processing (34)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/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/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_dhcp.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-db/src/machine_interface/tests.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/predicted_machine_interface.rscrates/api-model/src/site_explorer/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/src/model/expected_machine.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rs
|
@coderabbitai Thanks for the eyes. Updated. PTAL. Resolve if good, thanks! |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR diff summary =='
git diff --stat origin/main...HEAD
echo
echo '== Changed Rust files =='
git diff --name-only origin/main...HEAD -- 'crates/**/*.rs'
echo
echo '== Compatibility-sensitive diff =='
git diff --unified=80 origin/main...HEAD -- \
crates/api-model/src/expected_machine.rs \
crates/rpc/build.rs \
crates/rpc/src/protos/mod.rs \
crates/rpc/src/model/expected_machine.rs \
crates/admin-cli/src/expected_machines/common.rs \
crates/admin-cli/src/expected_machines/add/args.rs \
crates/admin-cli/src/expected_machines/patch/args.rs \
crates/admin-cli/src/rpc.rs
echo
echo '== Relevant tests and call sites =='
rg -n -C 3 --glob '*.rs' \
'alias = "host_nics"|visible_alias = "host_nics"|interfaces\(\)|interfaces_mut\(\)|replace_interfaces|replace_host_nics|ExpectedHostNic|ExpectedInterface' \
crates/api-model crates/rpc crates/admin-cli | head -n 900
echo
echo '== CodeRabbit review comments on PR 4390 =='
gh api repos/NVIDIA/infra-controller/pulls/4390/comments \
--paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]" or .user.login == "coderabbitai") | {id, node_id, path, line, body, html_url}'Length of output: 144632
I did not run the test suite in this sandbox, but the targeted compatibility tests and static implementation review are consistent with the stated guarantees. Resolving CodeRabbit comments. 🐇 ✅ [resolve] You are interacting with an AI system. |
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/admin-cli/src/expected_machines/patch/args.rs (1)
248-261: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError message omits
sku-idandrack-idfrom the list of valid selectors.The guard checks 11 fields, but the message only names 8 of them. An operator relying on this text to understand which flags satisfy the "at least one" requirement won't see
sku-id/rack-idlisted, even though setting either alone passes the check.As per path instructions, "Review CLI changes for clap behavior, actionable operator-facing error messages...".🐛 Proposed fix to align the message with the guard
- return Err(CarbideCliError::GenericError("One of the following options must be specified: bmc-username and bmc-password or chassis-serial-number or fallback-dpu-serial-number or bmc-ip-address or dpu-policy or bmc-ip-allocation or dpf-enabled or interfaces".to_string())); + return Err(CarbideCliError::GenericError("one of the following options must be specified: bmc-username and bmc-password or chassis-serial-number or fallback-dpu-serial-number or sku-id or rack-id or bmc-ip-address or dpu-policy or bmc-ip-allocation or dpf-enabled or interfaces".to_string()));🤖 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/patch/args.rs` around lines 248 - 261, Update the GenericError message in the validation guard to include the sku-id and rack-id selectors alongside the other accepted options, matching all fields checked by the guard while preserving the existing validation behavior.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.
Outside diff comments:
In `@crates/admin-cli/src/expected_machines/patch/args.rs`:
- Around line 248-261: Update the GenericError message in the validation guard
to include the sku-id and rack-id selectors alongside the other accepted
options, matching all fields checked by the guard while preserving the existing
validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: af85e725-6ecf-4834-aa2b-527c267291c7
📒 Files selected for processing (34)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/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/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_dhcp.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-db/src/machine_interface/tests.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/predicted_machine_interface.rscrates/api-model/src/site_explorer/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/src/model/expected_machine.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/api-model/src/machine/mod.rs
- crates/admin-cli/src/boot_interface/candidates/cmd.rs
- crates/api-model/src/predicted_machine_interface.rs
- crates/admin-cli/src/boot_interface/set/cmd.rs
- crates/api-model/src/site_explorer/mod.rs
- crates/api-core/src/tests/sku.rs
- crates/site-explorer/tests/integration/site_explorer.rs
|
@coderabbitai Thanks for the eyes. Updated -- added |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Focused diff =='
git diff --unified=40 origin/main...HEAD -- \
crates/admin-cli/src/expected_machines/patch/args.rs \
crates/admin-cli/src/expected_machines/tests.rs
echo
echo '== Target file outline =='
ast-grep outline crates/admin-cli/src/expected_machines/patch/args.rs --items all
echo
echo '== Validation implementation and selector declarations =='
rg -n -C 12 --glob '*.rs' \
'fn validate\s*\(|ArgGroup|sku-id|rack-id|interfaces|host_nics|Specify at least' \
crates/admin-cli/src/expected_machines/patch/args.rs \
crates/admin-cli/src/expected_machines/tests.rs
echo
echo '== Patch argument parse/validation test call sites =='
rg -n -C 5 --glob '*.rs' \
'expected-machine.*patch|Args::validate|validate_patch|sku-id|rack-id' \
crates/admin-cli/src/expected_machinesLength of output: 72340 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Patch argument declarations around SKU and rack selectors =='
sed -n '120,185p' crates/admin-cli/src/expected_machines/patch/args.rs
echo
echo '== Rack-ID patch parsing and validation coverage =='
rg -n -C 8 --glob '*.rs' \
'rack_id|rack-id|validate_patch.*rack|patch.*rack' \
crates/admin-cli/src/expected_machines/tests.rs \
crates/admin-cli/src/expected_machines/patch/args.rs
echo
echo '== Full required ArgGroup selector list =='
sed -n '40,58p' crates/admin-cli/src/expected_machines/patch/args.rsLength of output: 7877
🐇 ✅ [resolve] You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/rpc/src/model/expected_machine.rs (1)
304-369: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the needless per-interface
.clone()—expected_machineis already owned here.Since this
Fromimpl consumesexpected_machineby value, cloning every retained (non-HostBmc) interface before converting it is unnecessary —into_iter()moves them instead. This is a hot conversion path (invoked on every outboundExpectedMachineread/list response), so avoiding the clone is a small but genuine win. As per coding guidelines, "Avoid needless.clone()calls; prefer borrowing, moving withinto_iter, ordering struct fields to enable moves, or usingCowwhere values may be borrowed or owned."♻️ Proposed refactor using owned iteration instead of cloning
impl From<ExpectedMachine> for rpc::forge::ExpectedMachine { fn from(expected_machine: ExpectedMachine) -> Self { let has_stored_host_bmc = expected_machine .data .interfaces .iter() .any(|interface| interface.role.is_host_bmc()); let bmc_ip_allocation = expected_machine .compatibility_bmc_ip_allocation() .map(rpc::forge::BmcIpAllocationType::from) .map(|allocation| allocation as i32); - let mut interfaces = expected_machine - .data - .interfaces - .iter() - .filter(|interface| !interface.role.is_host_bmc()) - .map(|x| x.clone().into()) - .collect::<Vec<_>>(); - if has_stored_host_bmc { - interfaces.push(expected_machine.effective_host_bmc().into()); - } + let effective_host_bmc = has_stored_host_bmc + .then(|| rpc::forge::ExpectedInterface::from(expected_machine.effective_host_bmc())); + let interfaces = expected_machine + .data + .interfaces + .into_iter() + .filter(|interface| !interface.role.is_host_bmc()) + .map(Into::into) + .chain(effective_host_bmc) + .collect::<Vec<_>>(); rpc::forge::ExpectedMachine { id: expected_machine.id.map(|u| crate::common::Uuid { value: u.to_string(), }),🤖 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/expected_machine.rs` around lines 304 - 369, Update the interfaces collection in From<ExpectedMachine> for rpc::forge::ExpectedMachine to consume expected_machine.data.interfaces with into_iter() instead of iter(), then convert retained non-HostBmc interfaces directly without cloning. Preserve the existing filtering and effective_host_bmc append 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/admin-cli/src/expected_machines/patch/args.rs`:
- Around line 258-260: Update the validation error returned by the argument
check in the surrounding validation method to begin with a lowercase phrase,
preserving the existing options and wording otherwise; ensure the message has no
trailing period.
---
Nitpick comments:
In `@crates/rpc/src/model/expected_machine.rs`:
- Around line 304-369: Update the interfaces collection in From<ExpectedMachine>
for rpc::forge::ExpectedMachine to consume expected_machine.data.interfaces with
into_iter() instead of iter(), then convert retained non-HostBmc interfaces
directly without cloning. Preserve the existing filtering and effective_host_bmc
append 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: 0df6fa51-2e4e-4ee2-8223-10abf9f7be66
📒 Files selected for processing (34)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/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/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/machine_dhcp.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-db/src/machine_interface/tests.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/predicted_machine_interface.rscrates/api-model/src/site_explorer/mod.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/src/model/expected_machine.rscrates/rpc/src/protos/mod.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/zero_dpu.rs
`ExpectedMachine.host_nics` started as a host-only list, but now describes `Host`, `DpuOs`, `DpuBmc`, and `HostBmc` interfaces. The old name no longer matches the data. So this makes `ExpectedInterface` and `interfaces` the Rust, configuration, and admin CLI names. Existing `host_nics` input and `--host_nics` commands keep working as aliases, while the protobuf names, field numbers, wire bytes, and physical `expected_machines.host_nics` column stay unchanged. This is a no-op for existing configs and databases, and it does not change `fixed_ip`, role, or allocation behavior. The compatibility tests cover both JSON and CLI spellings, protobuf and ProtoJSON details, and the JSONB round trip. This supports NVIDIA#4104 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Updated -- the owned conversion now consumes |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed hunks for expected-machine RPC conversion =='
git diff --unified=50 origin/main...HEAD -- crates/rpc/src/model/expected_machine.rs
echo
echo '== Outline of candidate conversion module =='
ast-grep outline crates/rpc/src/model/expected_machine.rs --items all
echo
echo '== Owned ExpectedMachine conversion and interface iteration =='
rg -n -C 16 --glob '*.rs' \
'impl From<.*ExpectedMachine.*> for rpc::forge::ExpectedMachine|impl From<ExpectedMachine|data\.interfaces|interfaces\.into_iter|interfaces\.iter\(\).*clone|host_nics' \
crates/rpc/src/model/expected_machine.rs
echo
echo '== Focused conversion test references =='
rg -n -C 8 --glob '*.rs' \
'canonical.*serial|interfaces.*clone|ExpectedMachine.*into|effective_host_bmc|replace_interfaces' \
crates/rpc/src/model/expected_machine.rsLength of output: 83987
I did not run the test suite in this sandbox. 🐇 ✅ [resolve] You are interacting with an AI system. |
|
✅ Action performedFull review finished. |
ExpectedMachine.host_nicsstarted as a host-only list, but now describesHost,DpuOs,DpuBmc, andHostBmcinterfaces. The old name no longer matches the data.So this makes
ExpectedInterfaceandinterfacesthe Rust, configuration, and admin CLI names. Existinghost_nicsinput and--host_nicscommands keep working as aliases, while the protobuf names, field numbers, wire bytes, and physicalexpected_machines.host_nicscolumn stay unchanged.This is a no-op for existing configs and databases, and it does not change
fixed_ip, role, or allocation behavior. The compatibility tests cover old, new, and mixed JSON/configuration spellings, both CLI spellings, protobuf and ProtoJSON details, and the JSONB round trip.Related issues
This supports #4104
Part of #3491.
Type of Change
Breaking Changes
Testing
The focused compatibility tests cover canonical
interfacesinput, legacyhost_nicsinput, both spellings across different machines in the same file, ambiguous same-machine duplicates,--interfaces,--host_nics, omitted-versus-present list handling, exact protobuf bytes and descriptor names, and the existing JSONB column. The fullcargo make format-nightly,cargo make clippy, andcargo make carbide-lintsgates pass on currentmain.Additional Notes
There is no database migration or protobuf schema change. The physical
expected_machines.host_nicscolumn, protobufExpectedHostNicmessage,host_nicsfield numbers, and ProtoJSON names stay unchanged.Documentation is tracked separately by #4151 so technical-writing review does not block this code change. That docs-only PR will use
interfacesin current examples and call outhost_nicsas a supported compatibility alias.Closes #4104