feat: support IP allocation policies for expected interfaces#3991
Conversation
|
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 (58)
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:
WalkthroughExpected-machine interfaces now carry explicit roles and allocation policies through CLI, RPC, database, DHCP, discovery, Site Explorer, and cleanup workflows. The changes also add reservation provenance, ordered locking, ambiguity validation, atomic reconciliation, and prepared interface deletion. ChangesExpected interface lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant ExpectedMachineHandler
participant ApiDB
participant DhcpDiscovery
participant SiteExplorer
AdminCLI->>ExpectedMachineHandler: Submit expected interface policy
ExpectedMachineHandler->>ApiDB: Validate identities and reconcile reservations
DhcpDiscovery->>ApiDB: Resolve captured interface policy and allocation state
SiteExplorer->>ApiDB: Reload expected configuration before preallocation
ApiDB-->>DhcpDiscovery: Return interface and address state
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
🔐 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-23 17:25:23 UTC | Commit: c10be0b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
2051-2058: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPer-NIC DB round trip now happens even for dynamic-allocation interfaces.
try_preallocate_onepreviously required a concreteip: IpAddr, so this loop must have filtered onnic.fixed_ip.is_some()before calling it. That guard has been removed:try_preallocate_expected_interfacenow unconditionally opens a transaction, runs the lockedfind_by_host_mac_addressquery, and only then checksfixed_ip. Every host NIC usingDynamic/Retainedallocation (the common case) now incurs a full txn+query+lock on every SiteExplorer iteration, across the whole fleet, for no reason — since the caller already hasexpected_machine.data.host_nicsin memory withfixed_ippopulated.Restore the cheap in-memory filter before paying for the DB round trip; re-validation under lock still happens inside
try_preallocate_expected_interfacefor NICs that do declare a fixed IP, so TOCTOU protection is unaffected.🐎 Proposed fix to skip needless transactions for dynamic NICs
for nic in &expected_machine.data.host_nics { + if nic.fixed_ip.is_none() { + continue; + } try_preallocate_expected_interface( &self.database_connection, nic.mac_address, self.config.retained_boot_interface_window, ) .await; }🤖 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 2051 - 2058, In the loop over expected_machine.data.host_nics, skip NICs whose fixed_ip is absent before calling try_preallocate_expected_interface. Preserve the existing database-backed revalidation inside try_preallocate_expected_interface for NICs with a concrete fixed IP.
🧹 Nitpick comments (1)
crates/site-explorer/src/lib.rs (1)
4250-4317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the file's established table-driven test pattern here.
expected_data_interfaces_are_not_redfish_scan_candidatescovers six input combinations with hand-rolledassert!/assert!(!...)calls, but this same file already usescheck_cases/check_values/value_scenarios!for exactly this "pure input → bool" shape (e.g.bmc_reset_results_update_legacy_gauge_event_and_log,test_find_host_pf_mac_address). Table-driving this test would make the six scenarios self-documenting and easier to extend.check_values( [ Check { scenario: "unowned data on underlay, unknown mac", input: (unknown_mac, underlay_segment, InterfaceType::Data, false), expect: true }, Check { scenario: "unowned data on underlay, expected data mac", input: (expected_data_mac, underlay_segment, InterfaceType::Data, false), expect: false }, Check { scenario: "owned data on underlay", input: (unknown_mac, underlay_segment, InterfaceType::Data, true), expect: false }, Check { scenario: "bmc on underlay, expected data mac, owned", input: (expected_data_mac, underlay_segment, InterfaceType::Bmc, true), expect: true }, Check { scenario: "bmc on host-inband", input: (unknown_mac, host_inband_segment, InterfaceType::Bmc, false), expect: true }, Check { scenario: "data on host-inband", input: (unknown_mac, host_inband_segment, InterfaceType::Data, false), expect: false }, ], |(mac, segment, itype, owned)| scannable(&interface(mac, segment, itype, owned)), );As per coding guidelines, "For tests, prefer table-driven scenarios or explicit grouped cases (such as
scenarios!,value_scenarios!,check_cases, andcheck_values) to cover input variants, especially for parsers, validators, and conversions."🤖 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 4250 - 4317, Refactor expected_data_interfaces_are_not_redfish_scan_candidates to use the file’s established check_values table-driven pattern instead of six standalone assertions. Define named Check scenarios for each existing MAC, segment, interface-type, and ownership combination, then evaluate them through scannable and interface while preserving all expected boolean results.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.
Outside diff comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 2051-2058: In the loop over expected_machine.data.host_nics, skip
NICs whose fixed_ip is absent before calling try_preallocate_expected_interface.
Preserve the existing database-backed revalidation inside
try_preallocate_expected_interface for NICs with a concrete fixed IP.
---
Nitpick comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 4250-4317: Refactor
expected_data_interfaces_are_not_redfish_scan_candidates to use the file’s
established check_values table-driven pattern instead of six standalone
assertions. Define named Check scenarios for each existing MAC, segment,
interface-type, and ownership combination, then evaluate them through scannable
and interface while preserving all expected boolean results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26a16d52-706c-4325-acd8-0c88fbf7ff44
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/site-explorer/src/lib.rs (2)
4250-4317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer table-driven assertions for this predicate.
expected_data_interfaces_are_not_redfish_scan_candidatesexercises six combinations ofis_bmc/segment/owned/expectedvia a flat series ofassert!/assert!(!...)calls. This module already importscheck_cases/value_scenarios!for exactly this kind of validator coverage elsewhere in the file (e.g.test_find_host_pf_mac_address).♻️ Sketch using check_values
+ use carbide_test_support::{Check, check_values}; + #[test] fn expected_data_interfaces_are_not_redfish_scan_candidates() { ... - assert!(scannable(&interface(unknown_mac, underlay_segment, InterfaceType::Data, false))); - assert!(!scannable(&interface(expected_data_mac, underlay_segment, InterfaceType::Data, false))); - ... + check_values( + [ + Check { scenario: "unowned unknown data on underlay", input: (unknown_mac, underlay_segment, InterfaceType::Data, false), expect: true }, + Check { scenario: "unowned expected data on underlay", input: (expected_data_mac, underlay_segment, InterfaceType::Data, false), expect: false }, + // ... remaining cases + ], + |(mac, seg, ty, owned)| scannable(&interface(mac, seg, ty, owned)), + ); }As per coding guidelines, "For tests, prefer table-driven scenarios or explicit grouped cases (such as
scenarios!,value_scenarios!,check_cases, andcheck_values) to cover input variants, especially for parsers, validators, and conversions."🤖 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 4250 - 4317, Refactor expected_data_interfaces_are_not_redfish_scan_candidates into a table-driven test using the existing check_cases, check_values, or value_scenarios! helpers. Represent the six combinations of interface type, segment, ownership, MAC expectation, and expected scannability as grouped scenarios, while preserving each case’s current outcome.Source: Coding guidelines
2051-2058: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the per-NIC round trip for non-fixed interfaces.
try_preallocate_expected_interfaceis invoked unconditionally for everyhost_nicon every reconciliation pass, but the loop already holdsnic: &ExpectedHostNicin memory — aDynamic/RetainedNIC'sfixed_ipisNoneand known here without a DB call. Each invocation still pays for a fulltxn.begin(), thefind_by_host_mac_addressjsonb lookup, and an implicit rollback before discovering there's nothing to do. At scale (many machines × NICs, run every iteration) this is a steady stream of wasted transactions.⚡ Proposed fix: filter by `fixed_ip` before the expensive call
for nic in &expected_machine.data.host_nics { - try_preallocate_expected_interface( - &self.database_connection, - nic.mac_address, - self.config.retained_boot_interface_window, - ) - .await; + if nic.fixed_ip.is_some() { + try_preallocate_expected_interface( + &self.database_connection, + nic.mac_address, + self.config.retained_boot_interface_window, + ) + .await; + } }🤖 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 2051 - 2058, In the reconciliation loop over expected_machine.data.host_nics, invoke try_preallocate_expected_interface only when nic.fixed_ip is present; skip Dynamic/Retained NICs with fixed_ip set to None before entering the database-backed call, while preserving the existing arguments and await behavior for fixed interfaces.
🤖 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`:
- Around line 4250-4317: Refactor
expected_data_interfaces_are_not_redfish_scan_candidates into a table-driven
test using the existing check_cases, check_values, or value_scenarios! helpers.
Represent the six combinations of interface type, segment, ownership, MAC
expectation, and expected scannability as grouped scenarios, while preserving
each case’s current outcome.
- Around line 2051-2058: In the reconciliation loop over
expected_machine.data.host_nics, invoke try_preallocate_expected_interface only
when nic.fixed_ip is present; skip Dynamic/Retained NICs with fixed_ip set to
None before entering the database-backed call, while preserving the existing
arguments and await behavior for fixed interfaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b3ce8801-09e2-4a0e-a7aa-d363a774b688
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 seconds. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3991.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/handlers/expected_machine.rs (1)
238-259: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd chassis-serial validation to the single update path
The
updatehandler only checks duplicate DPU serials before converting the request, whileaddand batch writes enforceCHASSIS_SERIAL_REGEX. A malformedchassis_serial_numbercan still pass through this RPC. Reuse the shared parser here or apply the same regex beforetry_into()so all write paths enforce the same constraint.🤖 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/handlers/expected_machine.rs` around lines 238 - 259, The single-machine update flow around request conversion lacks chassis serial validation. In the update handler, validate request.chassis_serial_number using the shared chassis-serial parser or the same CHASSIS_SERIAL_REGEX enforcement used by add and batch writes before request.try_into(), while preserving the existing invalid-argument error 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.
Inline comments:
In `@crates/api-core/src/handlers/expected_machine.rs`:
- Around line 579-582: Update the InvalidArgument message in the
expected-machine BMC MAC validation path to begin with a lowercase phrase, while
preserving the existing interpolation and no-trailing-period format.
In
`@crates/api-db/migrations/20260722120000_expected_interface_allocation_state.sql`:
- Around line 4-8: Update the migration adding expected_machine_preallocation to
backfill existing NULL values to false before relying on the default, then
enforce the non-NULL invariant with a NOT NULL constraint. Keep the false
default for future inserts.
---
Outside diff comments:
In `@crates/api-core/src/handlers/expected_machine.rs`:
- Around line 238-259: The single-machine update flow around request conversion
lacks chassis serial validation. In the update handler, validate
request.chassis_serial_number using the shared chassis-serial parser or the same
CHASSIS_SERIAL_REGEX enforcement used by add and batch writes before
request.try_into(), while preserving the existing invalid-argument error
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: 3568b2f2-58fc-416a-9b4b-d575e6d07418
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (26)
- crates/admin-cli/src/expected_machines/common.rs
- crates/admin-cli/src/expected_machines/update/mod.rs
- crates/api-core/src/handlers/expected_power_shelf.rs
- crates/rpc/build.rs
- crates/api-core/src/tests/expected_switch.rs
- crates/admin-cli/src/rpc.rs
- crates/api-core/src/handlers/expected_switch.rs
- crates/site-explorer/src/machine_creator.rs
- crates/machine-a-tron/src/machine_a_tron.rs
- crates/api-db/src/network_segment.rs
- crates/rpc/src/lib.rs
- crates/admin-cli/src/expected_machines/tests.rs
- crates/rpc/proto/forge.proto
- crates/api-core/src/handlers/machine_interface_address.rs
- crates/api-db/src/machine_interface_address.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-db/src/machine_interface/tests.rs
- crates/rpc/src/model/expected_machine.rs
- crates/api-core/tests/integration/expected_power_shelf_static_address.rs
- crates/api-db/src/expected_machine.rs
- crates/api-model/src/expected_machine.rs
- crates/site-explorer/src/lib.rs
- crates/api-core/src/tests/expected_machine.rs
- crates/api-core/src/dhcp/discover.rs
- crates/api-core/src/tests/machine_dhcp.rs
- crates/api-db/src/machine_interface.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/expected_machine.rs`:
- Around line 205-207: Update the DatabaseError::InvalidArgument message in the
ExpectedMachine identity-conflict branch to begin with lowercase text while
preserving the existing interpolation and wording. Keep the message free of a
trailing period, consistent with the neighboring error messages in the same
helper.
🪄 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: 5c93563e-64ea-476f-a6d7-5f11d52ccc53
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-db/migrations/20260722120000_expected_interface_allocation_state.sql (1)
12-13: 🗄️ Data Integrity & Integration | 🔵 TrivialThe Squawk
require-concurrent-index-creationwarning is a false positive in this repository:sqlx::migrate!()runs migrations in a transaction, andCREATE INDEX CONCURRENTLYis incompatible with transactional DDL. PlainCREATE INDEXis the established convention here, so no change is warranted.That said, given
expected_machinesis written on the operator/reconciliation path, do confirm the table is small enough that a brief write lock during index build is acceptable in production; if not, this index may warrant an out-of-band concurrent build outside the transactional migration.🤖 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-db/migrations/20260722120000_expected_interface_allocation_state.sql` around lines 12 - 13, No code change is warranted: retain the plain CREATE INDEX statement because sqlx::migrate!() runs migrations transactionally and CREATE INDEX CONCURRENTLY is incompatible with transactional DDL. Confirm that expected_machines is small enough for the brief write lock during index creation; only pursue an out-of-band concurrent build if production sizing makes that lock unacceptable.Sources: Learnings, Linters/SAST tools
crates/api-core/src/handlers/expected_machine.rs (1)
433-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rejecting duplicate
fixed_ipvalues acrosshost_nicshere.Interface validation catches duplicate MACs but not two distinct NICs declaring the same
fixed_ip. That collision currently escapes validation and only surfaces downstream inreconcile_configured_interfacesas a genericIP address … is already allocatedDatabaseError, which is opaque to the operator. A smallHashSet<IpAddr>guard alongside the existingseen_macscheck would fail fast with an actionable message.🤖 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/handlers/expected_machine.rs` around lines 433 - 478, The validate_expected_interfaces function should reject duplicate fixed_ip values across distinct host_nics before reconciliation. Add a HashSet<IpAddr> alongside seen_macs, insert each present fixed_ip during interface validation, and return CarbideError::InvalidArgument with an actionable host_nics-specific message when insertion detects a duplicate.
🤖 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/handlers/expected_machine.rs`:
- Around line 433-478: The validate_expected_interfaces function should reject
duplicate fixed_ip values across distinct host_nics before reconciliation. Add a
HashSet<IpAddr> alongside seen_macs, insert each present fixed_ip during
interface validation, and return CarbideError::InvalidArgument with an
actionable host_nics-specific message when insertion detects a duplicate.
In
`@crates/api-db/migrations/20260722120000_expected_interface_allocation_state.sql`:
- Around line 12-13: No code change is warranted: retain the plain CREATE INDEX
statement because sqlx::migrate!() runs migrations transactionally and CREATE
INDEX CONCURRENTLY is incompatible with transactional DDL. Confirm that
expected_machines is small enough for the brief write lock during index
creation; only pursue an out-of-band concurrent build if production sizing makes
that lock unacceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66ab5277-f8e4-4d01-9909-6f0b9a337ad8
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/expected_machine.rs (1)
93-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHost-MAC disambiguation reads cleanly.
The cross-result
top_level/nestedaccounting mirrors the write-time invariant enforced byvalidate_identity_macs_available, and theFOR SHARElock is held for the caller's preallocation window as intended. One observation: a BMC-only match deliberately yieldsOk(None)here and is routed to the legacy BMC path downstream — worth a one-line inline note so a future reader does not mistake it for a missed case.🤖 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-db/src/expected_machine.rs` around lines 93 - 145, Add a concise inline comment in find_by_host_mac_address explaining that a BMC-only match intentionally returns Ok(None) so the caller can use the legacy BMC path. Keep the existing matching logic and return behavior unchanged.
🤖 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-db/src/expected_machine.rs`:
- Around line 93-145: Add a concise inline comment in find_by_host_mac_address
explaining that a BMC-only match intentionally returns Ok(None) so the caller
can use the legacy BMC path. Keep the existing matching logic and return
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16a0ffeb-7ec3-406f-882e-702703f22859
⛔ 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 (29)
crates/admin-cli/src/expected_machines/common.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/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/expected_machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-model/src/expected_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
crates/api-core/src/tests/expected_switch.rs (2)
499-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the lock-timeout condition rather than any error.
result.is_err()is satisfied by any failure — a pool error or an unrelatedDatabaseErrorwould keep this test green while proving nothing about lock retention. Bind the assertion to Postgreslock_not_available(SQLSTATE55P03) so the test fails loudly if the contention mechanism changes.♻️ Suggested assertion tightening
- assert!( - result.is_err(), - "ExpectedSwitch replacement did not hold its {name} MAC lock" - ); + let error = result.expect_err(&format!( + "ExpectedSwitch replacement did not hold its {name} MAC lock" + )); + assert!( + format!("{error:?}").contains("55P03"), + "expected a lock timeout for the {name} MAC, got: {error:?}" + );Prefer matching the concrete
DatabaseErrorvariant and itssqlx::Error::Database(db).code()if that is reachable from this module, rather than string matching.🤖 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/tests/expected_switch.rs` around lines 499 - 514, Strengthen the assertion in the loop testing lock retention by requiring the `lock_expected_machine_interface_macs` result to be a `sqlx::Error::Database` whose database error code is PostgreSQL `55P03` (`lock_not_available`). Reject pool errors and unrelated database errors while preserving the existing rollback flow.
29-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the two lock-wait helpers and add a poll interval.
The two helpers differ only in
EXISTSversuscount(*) >= n, i.e. the first is the second withexpected_waiters = 1. Additionally,yield_now()provides no backoff, so each helper issuespg_stat_activityround-trips as fast as the pool permits for up to five seconds, occupying a pooled connection and contending with the very tasks under observation. A shortsleepis the idiomatic cadence for this pattern.♻️ Proposed consolidation
-async fn wait_for_expected_switch_mac_lock(pool: &sqlx::PgPool, blocker_pid: i32) { - tokio::time::timeout(std::time::Duration::from_secs(5), async { - loop { - let waiting = sqlx::query_scalar::<_, bool>( - "SELECT EXISTS ( - SELECT 1 - FROM pg_stat_activity - WHERE datname = current_database() - AND wait_event_type = 'Lock' - AND wait_event = 'advisory' - AND $1::integer = ANY(pg_blocking_pids(pid)) - )", - ) - .bind(blocker_pid) - .fetch_one(pool) - .await - .expect("inspect ExpectedSwitch MAC lock wait"); - if waiting { - return; - } - tokio::task::yield_now().await; - } - }) - .await - .expect("ExpectedSwitch replacement did not reach the MAC lock wait"); -} - -async fn wait_for_expected_switch_write_lock_waiters( - pool: &sqlx::PgPool, - blocker_pid: i32, - expected_waiters: i64, -) { - tokio::time::timeout(std::time::Duration::from_secs(5), async { +async fn wait_for_advisory_lock_waiters( + pool: &sqlx::PgPool, + blocker_pid: i32, + expected_waiters: i64, + context: &str, +) { + tokio::time::timeout(std::time::Duration::from_secs(5), async { loop { let waiters = sqlx::query_scalar::<_, i64>( "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock' AND wait_event = 'advisory' AND $1::integer = ANY(pg_blocking_pids(pid))", ) .bind(blocker_pid) .fetch_one(pool) .await - .expect("inspect ExpectedSwitch write lock waiters"); + .expect("inspect advisory lock waiters"); if waiters >= expected_waiters { return; } - tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(10)).await; } }) .await - .expect("ExpectedSwitch creates did not reach the write lock wait"); + .unwrap_or_else(|_| panic!("did not reach the advisory lock wait: {context}")); }🤖 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/tests/expected_switch.rs` around lines 29 - 83, Consolidate wait_for_expected_switch_mac_lock and wait_for_expected_switch_write_lock_waiters into one helper parameterized by expected_waiters, using a count query with the existing blocker and advisory-lock predicates; update callers so the MAC-lock case passes 1. Replace tokio::task::yield_now() in the polling loop with a short tokio::time::sleep interval before retrying, while preserving the five-second timeout and existing error messages or equivalent context.crates/api-core/src/tests/machine_dhcp.rs (1)
96-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo divergent advisory-lock wait helpers, one of them name-colliding. This PR introduces advisory-lock synchronization scaffolding independently in two test modules, with different detection strategies and a shared function name that does not share a signature. The
machine_dhcp.rsvariant matches waiters by SQL text (query LIKE '%pg_advisory_xact_lock%') and a global count, so it cannot attribute a waiter to the intended blocker and quietly stops matching if a lock is ever taken through a helper whose SQL text differs — for example a shared-mode or wrapped variant. Theexpected_machine.rsvariant keys on a pid andpg_blocking_pids, which is the precise formulation. Consolidating on the pid-based approach would remove the duplication and make the weaker call sites attribute-correct; both currently pass only because#[crate::sqlx_test]isolates each test in its own database.
crates/api-core/src/tests/machine_dhcp.rs#L96-L123: replace the SQL-text/global-count predicate with the pid-basedpg_blocking_pidsform, and either renamewait_for_advisory_lock_waitor align its signature with the identically named helper inexpected_machine.rsso the two cannot be confused; the multi-waiter call sites at lines 2002 and 2012 then need the blocker or waiter pid threaded through instead of a bare count.crates/api-core/src/tests/expected_machine.rs#L110-L162: promote these two helpers into the shared test-support module (alongside the othercommontest fixtures) somachine_dhcp.rsconsumes them rather than carrying a second implementation.♻️ Sketch of the shared, pid-attributed helper
/// Wait until `waiting_pid`'s backend is parked on an advisory lock held by /// `blocker_pid`. Attribution by pid keeps the predicate independent of the /// SQL text used to take the lock. pub async fn wait_for_advisory_lock_wait(pool: &sqlx::PgPool, blocker_pid: i32) { tokio::time::timeout(std::time::Duration::from_secs(5), async { loop { let waiting = sqlx::query_scalar::<_, bool>( "SELECT EXISTS ( SELECT 1 FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock' AND wait_event = 'advisory' AND $1::integer = ANY(pg_blocking_pids(pid)) )", ) .bind(blocker_pid) .fetch_one(pool) .await .expect("inspect advisory lock wait"); if waiting { return; } tokio::task::yield_now().await; } }) .await .expect("request did not reach the advisory lock wait"); }🤖 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/tests/machine_dhcp.rs` around lines 96 - 123, Consolidate the advisory-lock wait helpers into shared test support: in crates/api-core/src/tests/expected_machine.rs lines 110-162, promote the pid-attributed helpers for reuse; in crates/api-core/src/tests/machine_dhcp.rs lines 96-123, remove the SQL-text/global-count implementation and consume the shared helpers, aligning or renaming wait_for_advisory_lock_wait to avoid the signature collision. Update machine_dhcp.rs multi-waiter call sites at lines 2002 and 2012 to pass blocker or waiter PIDs rather than a bare count; no separate implementation should remain in machine_dhcp.rs.crates/api-db/src/predicted_machine_interface.rs (1)
124-137: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider asserting the conditional UPDATE actually won the race.
The statement is guarded by
AND NOT expected_interface_captured, yet the result is discarded and the caller-supplied declaration is returned unconditionally. Today the MAC advisory lock taken incapture_expected_interface_for_discoveryserializes writers, so a zero-row outcome is unreachable; if that lock discipline is ever relaxed, this silently reports a capture that did not happen. Inspectingrows_affected()(and returning the stored value on 0) would make the invariant self-enforcing rather than lock-dependent.🤖 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-db/src/predicted_machine_interface.rs` around lines 124 - 137, Update the conditional UPDATE in the expected-interface capture method to inspect its rows_affected result. If no row was updated, return the persisted interface value instead of unconditionally returning the caller-supplied declaration; otherwise preserve the current captured-value return behavior.crates/site-explorer/src/machine_creator.rs (1)
1283-1298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare the id as a
Uuid, not aString.
ExpectedMachineIngestionIdentitystringifiesexpected_machine.idsolely to obtain equality, allocating on every ingestion attempt for a type that is alreadyCopy + Eq.♻️ Suggested simplification
-struct ExpectedMachineIngestionIdentity { - id: Option<String>, +struct ExpectedMachineIngestionIdentity { + id: Option<uuid::Uuid>, bmc_mac_address: MacAddress, has_rack_id: bool, } @@ - id: expected_machine.id.map(|id| id.to_string()), + id: expected_machine.id,The struct can then derive
Copyas well.🤖 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/machine_creator.rs` around lines 1283 - 1298, Update ExpectedMachineIngestionIdentity to store expected_machine.id as its native Uuid-compatible type instead of converting it to String in From<&ExpectedMachine>. Preserve the existing optional-ID equality semantics and derive Copy alongside the existing traits, since all fields are copyable.crates/site-explorer/src/lib.rs (1)
2035-2061: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate the per-entity reconcile calls on the snapshot, as the host-NIC loop already does.
try_reconcile_expected_machine_bmcandtry_reconcile_expected_switch_addressesare invoked unconditionally for every expected machine and every expected switch. Each call opens its own transaction, takes the shared configuration lock, and re-reads the row — even when the snapshot shows neither a fixed address nor a retention policy, i.e. when the helper is guaranteed to be a no-op. On a large site that is one extra transaction plus advisory lock per entity per pass.The host-NIC loop three lines below already establishes the right pattern with
if nic.fixed_ip.is_none() { continue; }; the helpers still re-read under lock, so a cheap snapshot guard can only skip work that would have been a no-op.⚡ Suggested guard
- try_reconcile_expected_machine_bmc( - &self.database_connection, - expected_machine.bmc_mac_address, - self.config.retained_boot_interface_window, - ) - .await; + if expected_machine.data.bmc_ip_address.is_some() + || expected_machine + .data + .bmc_ip_allocation + .retains_dynamic_ip(false) + { + try_reconcile_expected_machine_bmc( + &self.database_connection, + expected_machine.bmc_mac_address, + self.config.retained_boot_interface_window, + ) + .await; + }An equivalent
bmc_ip_address.is_some() || nvos_ip_address.is_some()guard applies to the expected-switch loop.🤖 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 2035 - 2061, Gate the calls to try_reconcile_expected_machine_bmc and try_reconcile_expected_switch_addresses using the snapshot before invoking them: skip machines when neither bmc_ip_address nor nvos_ip_address is present, and skip switches unless bmc_ip_address or nvos_ip_address is present. Preserve the existing host-NIC fixed_ip guard and reconciliation behavior for entities that pass these checks.crates/api-db/src/machine.rs (1)
854-951: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo near-identical lock helpers; consider extracting the shared plumbing.
Both functions normalize the id set, bind stringified ids, map
55P03to a retryableFailedPrecondition, and verify the locked set matches the request. Only the lock strength (FOR KEY SHAREvsFOR UPDATE), the message, and the extraForceDeletioncheck differ. A small private helper taking the lock clause plus the message would remove the duplicated error mapping and keep future callers honest about theNOWAITcontract.Otherwise the semantics read correctly:
NOWAITavoids waiting behind a force-delete that will later need the caller's allocator/interface locks, and55P03is indeedlock_not_available.🤖 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-db/src/machine.rs` around lines 854 - 951, Extract the duplicated machine-id normalization, string binding, NOWAIT query execution, 55P03 error mapping, and locked-set validation from lock_interface_association_targets and lock_force_deletion_targets into a small private helper. Parameterize the helper with the lock clause and retry message, while keeping each public function’s distinct query result handling and ForceDeletion validation unchanged.crates/api-core/src/handlers/expected_machine.rs (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comments left behind by the validation/creation centralization. Both sites still describe the pre-refactor topology, where validation was insert-only and
replace_alldelegated per-entry work toadd.
crates/api-core/src/handlers/expected_machine.rs#L134-L138: drop "prior to insert" and the add/import-only sharing claim;validate_expected_machinenow serves the add, update, replace-all, and batch paths.crates/api-core/src/handlers/expected_machine.rs#L376-L377: stop referencing [add]; describe the inline parse/validate/release/recreate flow and the fact that reconciled replacements materialize reservations here, with deferred materialization retained only for brand-new identities.🤖 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/handlers/expected_machine.rs` around lines 134 - 138, The doc comment for validate_expected_machine must describe its use across add, update, replace-all, and batch paths rather than only insert and add/import flows. Also update the replace-all documentation at crates/api-core/src/handlers/expected_machine.rs#L376-L377 to describe its inline parse, validate, release, and recreate process, including immediate reservation materialization for reconciled replacements and deferred materialization only for brand-new identities.crates/api-db/src/machine_interface.rs (2)
1029-1044: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest candidate emptiness directly rather than through the rendered type string.
Line 1038 uses
!candidate_segment_types.is_empty()as a proxy for "there were candidate segments". It is correct today only because everyNetworkSegmentTyperenders to a non-empty string; the predicate silently inverts if that ever stops holding. Capture the count before theinto_iter()and assert on it.♻️ Proposed clarification
if let Some(network_segment_type) = expected_network_segment_type { + let had_candidates = !candidate_segments.is_empty(); let candidate_segment_types = candidate_segments .iter() .map(|segment| segment.config.segment_type.to_string()) .join(", "); let matching_segments = candidate_segments .into_iter() .filter(|segment| segment.config.segment_type == network_segment_type) .collect::<Vec<_>>(); - if matching_segments.is_empty() && !candidate_segment_types.is_empty() { + if matching_segments.is_empty() && had_candidates {🤖 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-db/src/machine_interface.rs` around lines 1029 - 1044, In the expected network segment type branch, capture candidate_segments’ count before consuming it with into_iter(), then use that count to determine whether candidates existed when matching_segments is empty. Keep candidate_segment_types solely for constructing the error message and preserve the existing filtering and return behavior.
1357-1366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
retains_dynamic_ip()predicate here too.This branch compares against
ExpectedInterfaceIpAllocation::Retaineddirectly, while the sibling path incapture_expected_interface_before_association(line 2067) asksresolved_ip_allocation().retains_dynamic_ip(). Two encodings of one rule will drift the moment another retaining variant appears. Prefer the named predicate in both places.♻️ Proposed alignment
- if expected_interface.resolved_ip_allocation() == ExpectedInterfaceIpAllocation::Retained { + if expected_interface + .resolved_ip_allocation() + .retains_dynamic_ip() + {🤖 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-db/src/machine_interface.rs` around lines 1357 - 1366, Update the conditional surrounding retain_expected_machine_dhcp_address_for_family in the shown machine-interface flow to use resolved_ip_allocation().retains_dynamic_ip() instead of directly comparing with ExpectedInterfaceIpAllocation::Retained. Align this check with capture_expected_interface_before_association and preserve the existing IPv4/IPv6 retention loop.crates/api-core/src/tests/machine_discovery.rs (1)
85-107: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a short sleep instead of
yield_nowin the polling loop.
tokio::task::yield_now()reschedules immediately, so this helper issuespg_stat_activityqueries as fast as the runtime allows for up to five seconds. Atokio::time::sleep(Duration::from_millis(10))keeps the same detection latency at a fraction of the database churn, which matters when the suite runs many of these concurrency tests.♻️ Proposed refactor
- tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(10)).await;🤖 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/tests/machine_discovery.rs` around lines 85 - 107, Update the polling loop in wait_for_advisory_lock_wait to replace tokio::task::yield_now() with a 10-millisecond tokio time sleep, preserving the existing timeout and advisory-lock detection 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.
Inline comments:
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 428-450: Update replace_all_expected_switches to reuse the
static-address allocation locking and preallocated-interface materialization
performed by update_expected_switch before inserting replacements through
db_expected_switch::create(). Ensure both bmc_ip_address and nvos_ip_address
allocations are covered, while preserving the existing MAC lock and replacement
flow.
- Around line 244-259: Update replace_all_expected_switches to perform the same
BMC/NVOS static-IP reconciliation as update_expected_switch: lock static
addresses before replacing rows and invoke update_preallocated_machine_interface
for affected interfaces after recreation. Preserve the existing MAC locking and
bulk replacement flow while ensuring fixed-address allocation and
machine-interface state remain synchronized.
In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 280-319: Update the expected_switch fixture closure to use move
capture, ensuring the referenced nvos_mac value is captured by value and the
closure remains reusable within both tokio::spawn tasks. Keep the existing
fixture fields and call sites unchanged.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 2290-2313: The doc comment for the function returning
ExistingPreallocationResult still describes the removed boolean contract. Update
that comment to document Missing, Unchanged, and Assigned, including the
conditions that produce each outcome, and remove references to Ok(true) or
Ok(false).
---
Nitpick comments:
In `@crates/api-core/src/handlers/expected_machine.rs`:
- Around line 134-138: The doc comment for validate_expected_machine must
describe its use across add, update, replace-all, and batch paths rather than
only insert and add/import flows. Also update the replace-all documentation at
crates/api-core/src/handlers/expected_machine.rs#L376-L377 to describe its
inline parse, validate, release, and recreate process, including immediate
reservation materialization for reconciled replacements and deferred
materialization only for brand-new identities.
In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 499-514: Strengthen the assertion in the loop testing lock
retention by requiring the `lock_expected_machine_interface_macs` result to be a
`sqlx::Error::Database` whose database error code is PostgreSQL `55P03`
(`lock_not_available`). Reject pool errors and unrelated database errors while
preserving the existing rollback flow.
- Around line 29-83: Consolidate wait_for_expected_switch_mac_lock and
wait_for_expected_switch_write_lock_waiters into one helper parameterized by
expected_waiters, using a count query with the existing blocker and
advisory-lock predicates; update callers so the MAC-lock case passes 1. Replace
tokio::task::yield_now() in the polling loop with a short tokio::time::sleep
interval before retrying, while preserving the five-second timeout and existing
error messages or equivalent context.
In `@crates/api-core/src/tests/machine_dhcp.rs`:
- Around line 96-123: Consolidate the advisory-lock wait helpers into shared
test support: in crates/api-core/src/tests/expected_machine.rs lines 110-162,
promote the pid-attributed helpers for reuse; in
crates/api-core/src/tests/machine_dhcp.rs lines 96-123, remove the
SQL-text/global-count implementation and consume the shared helpers, aligning or
renaming wait_for_advisory_lock_wait to avoid the signature collision. Update
machine_dhcp.rs multi-waiter call sites at lines 2002 and 2012 to pass blocker
or waiter PIDs rather than a bare count; no separate implementation should
remain in machine_dhcp.rs.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 85-107: Update the polling loop in wait_for_advisory_lock_wait to
replace tokio::task::yield_now() with a 10-millisecond tokio time sleep,
preserving the existing timeout and advisory-lock detection behavior.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 1029-1044: In the expected network segment type branch, capture
candidate_segments’ count before consuming it with into_iter(), then use that
count to determine whether candidates existed when matching_segments is empty.
Keep candidate_segment_types solely for constructing the error message and
preserve the existing filtering and return behavior.
- Around line 1357-1366: Update the conditional surrounding
retain_expected_machine_dhcp_address_for_family in the shown machine-interface
flow to use resolved_ip_allocation().retains_dynamic_ip() instead of directly
comparing with ExpectedInterfaceIpAllocation::Retained. Align this check with
capture_expected_interface_before_association and preserve the existing
IPv4/IPv6 retention loop.
In `@crates/api-db/src/machine.rs`:
- Around line 854-951: Extract the duplicated machine-id normalization, string
binding, NOWAIT query execution, 55P03 error mapping, and locked-set validation
from lock_interface_association_targets and lock_force_deletion_targets into a
small private helper. Parameterize the helper with the lock clause and retry
message, while keeping each public function’s distinct query result handling and
ForceDeletion validation unchanged.
In `@crates/api-db/src/predicted_machine_interface.rs`:
- Around line 124-137: Update the conditional UPDATE in the expected-interface
capture method to inspect its rows_affected result. If no row was updated,
return the persisted interface value instead of unconditionally returning the
caller-supplied declaration; otherwise preserve the current captured-value
return behavior.
In `@crates/site-explorer/src/lib.rs`:
- Around line 2035-2061: Gate the calls to try_reconcile_expected_machine_bmc
and try_reconcile_expected_switch_addresses using the snapshot before invoking
them: skip machines when neither bmc_ip_address nor nvos_ip_address is present,
and skip switches unless bmc_ip_address or nvos_ip_address is present. Preserve
the existing host-NIC fixed_ip guard and reconciliation behavior for entities
that pass these checks.
In `@crates/site-explorer/src/machine_creator.rs`:
- Around line 1283-1298: Update ExpectedMachineIngestionIdentity to store
expected_machine.id as its native Uuid-compatible type instead of converting it
to String in From<&ExpectedMachine>. Preserve the existing optional-ID equality
semantics and derive Copy alongside the existing traits, since all fields are
copyable.
🪄 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: a5873b46-284c-4598-9f1b-194026164972
⛔ 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 (56)
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/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/machine_interface.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/switch.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/switch.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-core/tests/integration/power_shelf_delete.rscrates/api-core/tests/integration/static_address_management.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/bmc_metadata.rscrates/api-db/src/bmc_metadata/tests.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/test_allocation_state_migration.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-db/src/predicted_machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/predicted_machine_interface.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
crates/api-db/src/bmc_metadata.rs (1)
227-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
%over?for aDisplayvalue.
MacAddressimplementsDisplay;?emits Debug formatting, which is noisier in logfmt output.♻️ Proposed tweak
- mac_address = ?bmc_interface_owner.mac_address, + mac_address = %bmc_interface_owner.mac_address,As per coding guidelines: "record dynamic values as structured fields and use native shorthand,
%forDisplay, and?forDebug."🤖 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-db/src/bmc_metadata.rs` around lines 227 - 232, Update the mac_address structured field in the tracing::info! call within the BMC enrichment flow to use % formatting instead of ? because MacAddress implements Display; leave the other fields and message unchanged.Source: Coding guidelines
crates/api-core/src/tests/expected_machine.rs (1)
175-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the typed lock helper over hand-rolled advisory-lock key SQL.
Three tests reproduce the
hashtextextended('expected_machine_interface.' || $1::text, 0)key derivation inline (here, Lines 234-241, and Lines 393-400), whiletest_update_and_add_serialize_identity_changesalready usesdb::machine_interface::lock_expected_machine_interface_macsat Line 333. Duplicating the formula couples these tests to a private DB implementation detail: if the key derivation ever changes, the blocker grabs an unrelated key and the tests degrade into timeouts rather than meaningful assertions. Using the helper uniformly keeps the intent and the production key in one place.♻️ Suggested change
- sqlx::query( - "SELECT pg_advisory_xact_lock( - hashtextextended('expected_machine_interface.' || $1::text, 0) - )", - ) - .bind(interface_mac) - .execute(&mut *blocker) - .await?; + db::machine_interface::lock_expected_machine_interface_macs(&mut blocker, [interface_mac]) + .await?;🤖 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/tests/expected_machine.rs` around lines 175 - 182, Replace the inline pg_advisory_xact_lock SQL in the affected expected-machine tests with the existing db::machine_interface::lock_expected_machine_interface_macs helper, matching the usage in test_update_and_add_serialize_identity_changes. Pass the relevant interface MAC and blocker transaction through the helper so all three tests use the shared production lock-key derivation.crates/rpc/src/lib.rs (1)
580-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCollapse the duplicate enum adapters.
ExpectedInterfaceRoleandExpectedInterfaceIpAllocationeach repeat the same untagged input enum plus optional serialize/deserialize helpers; a small declarative macro would centralize the shared flow and reduce drift. Also make theError::customcalls explicit on both paths for consistency.🤖 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/lib.rs` around lines 580 - 697, Consolidate the duplicated ExpectedInterfaceRole and ExpectedInterfaceIpAllocation adapters into a small declarative macro that generates their untagged input type and optional serialize_optional/deserialize_optional helpers, while preserving each enum’s accepted names, aliases, and output strings. Ensure the generated numeric and serialization error conversions use explicit serde::de::Error::custom and serde::ser::Error::custom calls consistently.crates/api-db/src/machine_interface.rs (1)
703-703: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
newly_createdflag. Every caller here drops it, including the DPU path, so the widened tuple return is dead API surface. Keep the creation-state logic local if it is only needed for future logging or metrics.🤖 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-db/src/machine_interface.rs` at line 703, Remove the unused newly_created value from the tuple produced by the relevant mapping flow around the interface extraction, changing the return type to expose only interface. Update all callers, including the DPU path, to consume the narrowed result while keeping any creation-state tracking local where needed for future logging or metrics.crates/site-explorer/src/machine_creator.rs (2)
1568-1590: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the
yield_now()busy-spin with a short sleep.The loop re-issues a
pg_stat_activityquery as fast as the runtime will schedule it, burning CPU and a pool connection for up to five seconds. A small sleep gives the waiting transaction time to register without the spin.♻️ Proposed refactor
if waiting > 0 { return; } - tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(10)).await;🤖 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/machine_creator.rs` around lines 1568 - 1590, Update wait_for_advisory_lock_wait so the polling loop uses a short Tokio sleep instead of tokio::task::yield_now(). Preserve the existing query, timeout, and immediate return once waiting is greater than zero.
1520-1522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort MAC addresses directly.
sort_by_key(ToString::to_string)allocates aStringper entry and ties the ordering to the display format.MacAddressalready orders by its raw bytes, sosort_unstable()(orsort_unstable_by_key(|mac| mac.bytes())) is clearer and cheaper.🤖 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/machine_creator.rs` around lines 1520 - 1522, Update the MAC address sorting in the visible collection-building flow to use MacAddress’s native ordering via sort_unstable() (or its raw-byte key), instead of sort_by_key(ToString::to_string). Keep the subsequent deduplication and return behavior unchanged.crates/api-core/src/tests/machine_discovery.rs (1)
566-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
panic!over synthesising anio::Errorfor an unmet test expectation.Wrapping the message in
std::io::Error::otherthen boxing it obscures a plain assertion failure and produces a less legible report. The same let-else shape elsewhere in this PR (crates/api-db/src/machine_interface/tests.rs) usespanic!directly.♻️ Suggested adjustment
let [host_interface] = host_interfaces.as_slice() else { - return Err(std::io::Error::other(format!( - "expected one proactive host interface, got {}", - host_interfaces.len() - )) - .into()); + panic!( + "expected one proactive host interface, got {}", + host_interfaces.len() + ); };🤖 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/tests/machine_discovery.rs` around lines 566 - 572, Replace the io::Error construction in the host_interfaces let-else branch with a direct panic! carrying the existing expectation message. Keep the successful single-interface destructuring unchanged and remove the unnecessary error conversion and boxing.crates/api-core/src/tests/switch.rs (1)
712-721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo new tests leave their final transaction to the implicit drop. Every other verification block added in this PR ends with an explicit
txn.rollback().await?(for examplecrates/api-core/tests/integration/power_shelf_delete.rs). Relying on drop is functionally equivalent under sqlx but breaks the local convention and hides the intent to discard.
crates/api-core/src/tests/switch.rs#L712-L721: addtxn.rollback().await?;after the final address assertion, beforeOk(()).crates/api-db/src/machine_interface/tests.rs#L1708-L1717: addtxn.rollback().await?;after theInvalidArgumentassertion, matching the explicitcommit/rollbackcalls in the earlier blocks of the same test.🤖 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/tests/switch.rs` around lines 712 - 721, Add explicit transaction rollback before returning from the verification block in switch.rs, after the final matching-address assertion. Also add the same rollback after the InvalidArgument assertion in machine_interface/tests.rs; both sites should call the existing txn rollback method before Ok(()) to match the surrounding transaction convention.crates/api-db/src/machine_interface/tests.rs (1)
154-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree overlapping segment builders now coexist in this module.
create_static_assignments_segment,create_test_segment, and the newcreate_managed_segmentall wrapnetwork_segment::persistwith aNewNetworkSegmentliteral, differing only in name, prefix list, segment type, and reserved count. A single builder taking those four inputs — with thin named wrappers where call sites benefit from the intent — would keep the fixture surface from growing further as more segment shapes are needed.🤖 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-db/src/machine_interface/tests.rs` around lines 154 - 187, Consolidate create_static_assignments_segment, create_test_segment, and create_managed_segment around one shared network-segment builder accepting name, prefix list, segment type, and reserved count. Preserve each helper’s existing fixture behavior through thin intent-specific wrappers where needed, and update their call sites to use the shared implementation without duplicating the NewNetworkSegment construction.crates/api-db/src/machine_interface_address.rs (1)
184-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
retain_dhcp_address_for_familyand its ExpectedMachine twin differ by one assignment.Two functions carry the same predicate, the same four bindings, and the same idempotency contract; only the
expected_machine_preallocation = trueclause differs. Consider a single private implementation parameterised by provenance, with the two public names as thin wrappers, so the predicate cannot drift between them.♻️ Suggested consolidation
+async fn retain_dhcp_address_for_family_inner( + txn: &mut PgConnection, + interface_id: MachineInterfaceId, + family: IpAddressFamily, + expected_machine_preallocation: bool, +) -> Result<bool, DatabaseError> { + let query = "UPDATE machine_interface_addresses + SET allocation_type = $3, + expected_machine_preallocation = $5 + WHERE interface_id = $1 + AND family(address) = $2 + AND allocation_type = $4"; + sqlx::query(query) + .bind(interface_id) + .bind(family.pg_family()) + .bind(AllocationType::Static) + .bind(AllocationType::Dhcp) + .bind(expected_machine_preallocation) + .execute(txn) + .await + .map(|result| result.rows_affected() > 0) + .map_err(|error| DatabaseError::query(query, error)) +}Note that this changes the non-ExpectedMachine path to write
falseexplicitly rather than leaving the column untouched; confirm that is acceptable before adopting, otherwise keep the two statements and extract only the shared predicate as a constant.🤖 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-db/src/machine_interface_address.rs` around lines 184 - 234, Consolidate retain_dhcp_address_for_family and retain_expected_machine_dhcp_address_for_family through one private helper parameterized by provenance, keeping both public functions as thin wrappers. Reuse the shared predicate, bindings, execution, and result handling; have the helper explicitly set expected_machine_preallocation according to the parameter, and verify that writing false for the non-ExpectedMachine path is acceptable before applying it.crates/api-core/src/handlers/machine.rs (2)
383-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
dpu_machines.len() != 1guard is dead in this branch.This branch is only reached when
host_machineisNone, and in that casedpu_machineswas constructed asvec![machine]. The meaningful check is thefind_host_by_dpu_machine_idre-read. Retaining the length test is defensible as defence against future refactoring of the branch above, but a one-line comment stating that intent would spare the next reader the same trace.🤖 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/handlers/machine.rs` around lines 383 - 393, The dpu_machines.len() != 1 check in the force-deletion validation is defensive rather than meaningful for the current host_machine-none branch. Add a concise comment immediately above that condition explaining it protects against future refactoring, while preserving the existing find_host_by_dpu_machine_id re-read and behavior.
341-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
ok_or_elsearm here is unreachable.
interface_idsis collected fromprepared_interfacesimmediately above and nothing mutates the map in between, soremovealways returnsSome. TheFailedPreconditionmessage therefore documents a state that cannot occur, which is misleading next to the two genuinely reachable checks at Lines 809 and 884. Consider draining directly.♻️ Suggested simplification
- for interface_id in interface_ids { - let prepared = prepared_interfaces.remove(&interface_id).ok_or_else(|| { - CarbideError::FailedPrecondition(format!( - "machine interface {interface_id} changed while preparing force deletion; retry the request", - )) - })?; - db::machine_interface::delete_prepared(prepared, txn.as_pgconn()).await?; - } + for interface_id in interface_ids { + let Some(prepared) = prepared_interfaces.remove(&interface_id) else { + continue; + }; + db::machine_interface::delete_prepared(prepared, txn.as_pgconn()).await?; + }🤖 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/handlers/machine.rs` around lines 341 - 357, Update the interface deletion loop around prepared_interfaces and interface_ids to drain or otherwise remove the matching entries directly, eliminating the unreachable ok_or_else FailedPrecondition branch. Preserve the existing filtering for non-BMC interfaces belonging to machine_id and continue passing each prepared interface to db::machine_interface::delete_prepared.crates/api-core/src/tests/machine_dhcp.rs (1)
96-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe advisory-lock wait helper is now copied into three modules and the copies have already diverged. All three poll
pg_stat_activityforpg_advisory_xact_lockwaiters under a five-second timeout, but they differ in wait strategy (yield_nowversus a 10 ms sleep), in parameterisation (waiter count versus a fixed one), and in failure message. Extracting one shared fixture would keep the synchronisation semantics — the linchpin of every new concurrency test in this PR — defined in a single place.
crates/api-core/src/tests/machine_dhcp.rs#L96-L122: promote this parameterised version (waiter count plus thesleep-based backoff) into a shared test helper and re-export it; drop the local definition.crates/api-core/src/tests/machine_discovery.rs#L85-L107: delete the local copy and call the shared helper.crates/api-db/src/machine_interface/tests.rs#L235-L257: delete the local copy and call the shared helper. Note this crate isapi-db, so the fixture likely belongs in a shared test-support crate rather than inapi-core.🤖 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/tests/machine_dhcp.rs` around lines 96 - 122, Centralize the parameterized advisory-lock waiter helper using the implementation in crates/api-core/src/tests/machine_dhcp.rs:96-122 as the basis, changing it to use the 10 ms sleep backoff, then re-export it from shared test support. Remove the local definition and call the shared helper in crates/api-core/src/tests/machine_dhcp.rs:96-122, crates/api-core/src/tests/machine_discovery.rs:85-107, and crates/api-db/src/machine_interface/tests.rs:235-257; place the fixture in a shared test-support crate accessible to both api-core and api-db, while preserving the waiter-count parameter and five-second timeout.
🤖 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-db/migrations/20260722120000_expected_interface_allocation_state.sql`:
- Around line 13-40: Update the declarations CTE and its downstream filtering to
exclude MAC addresses matching expected_machines.bmc_mac_address, so overlapping
host NIC/BMC entries are not backfilled into predicted_machine_interfaces;
preserve the existing duplicate nested-declaration exclusion and update the
lookup logic using the existing expected_machines symbol.
In `@crates/api-db/src/bmc_metadata.rs`:
- Around line 155-199: In the BMC-IP resolution branch of the interface
selection logic, replace find_interface_by_bmc_ip with
find_interface_by_bmc_ip_for_update so the relevant machine interface rows are
locked before capture_expected_interface_before_association and
associate_bmc_interface execute. Preserve the existing not-found handling and
tuple construction.
In `@crates/api-db/src/expected_switch.rs`:
- Around line 49-64: In the ExpectedSwitch ambiguity handling, replace the
DatabaseError::InvalidArgument classification with DatabaseError::internal,
matching find_interface_by_bmc_ip_with_query for multiple owners while
preserving the existing error message and branch behavior.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 5068-5079: Remove the unnecessary .bind(interface_id) call from
record_machine_interface_deletion, since its UPDATE query has no placeholders.
Keep the existing execution and error mapping unchanged.
In `@crates/site-explorer/tests/integration/machine_creator.rs`:
- Around line 1001-1012: Check the result returned by the UPDATE query in the
stale-policy fixture setup and assert that exactly one row was affected before
committing the transaction. Update the query flow around the machine_interfaces
update, preserving the existing bindings and commit behavior while making a
missing target row fail the test.
---
Nitpick comments:
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 383-393: The dpu_machines.len() != 1 check in the force-deletion
validation is defensive rather than meaningful for the current host_machine-none
branch. Add a concise comment immediately above that condition explaining it
protects against future refactoring, while preserving the existing
find_host_by_dpu_machine_id re-read and behavior.
- Around line 341-357: Update the interface deletion loop around
prepared_interfaces and interface_ids to drain or otherwise remove the matching
entries directly, eliminating the unreachable ok_or_else FailedPrecondition
branch. Preserve the existing filtering for non-BMC interfaces belonging to
machine_id and continue passing each prepared interface to
db::machine_interface::delete_prepared.
In `@crates/api-core/src/tests/expected_machine.rs`:
- Around line 175-182: Replace the inline pg_advisory_xact_lock SQL in the
affected expected-machine tests with the existing
db::machine_interface::lock_expected_machine_interface_macs helper, matching the
usage in test_update_and_add_serialize_identity_changes. Pass the relevant
interface MAC and blocker transaction through the helper so all three tests use
the shared production lock-key derivation.
In `@crates/api-core/src/tests/machine_dhcp.rs`:
- Around line 96-122: Centralize the parameterized advisory-lock waiter helper
using the implementation in crates/api-core/src/tests/machine_dhcp.rs:96-122 as
the basis, changing it to use the 10 ms sleep backoff, then re-export it from
shared test support. Remove the local definition and call the shared helper in
crates/api-core/src/tests/machine_dhcp.rs:96-122,
crates/api-core/src/tests/machine_discovery.rs:85-107, and
crates/api-db/src/machine_interface/tests.rs:235-257; place the fixture in a
shared test-support crate accessible to both api-core and api-db, while
preserving the waiter-count parameter and five-second timeout.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 566-572: Replace the io::Error construction in the host_interfaces
let-else branch with a direct panic! carrying the existing expectation message.
Keep the successful single-interface destructuring unchanged and remove the
unnecessary error conversion and boxing.
In `@crates/api-core/src/tests/switch.rs`:
- Around line 712-721: Add explicit transaction rollback before returning from
the verification block in switch.rs, after the final matching-address assertion.
Also add the same rollback after the InvalidArgument assertion in
machine_interface/tests.rs; both sites should call the existing txn rollback
method before Ok(()) to match the surrounding transaction convention.
In `@crates/api-db/src/bmc_metadata.rs`:
- Around line 227-232: Update the mac_address structured field in the
tracing::info! call within the BMC enrichment flow to use % formatting instead
of ? because MacAddress implements Display; leave the other fields and message
unchanged.
In `@crates/api-db/src/machine_interface_address.rs`:
- Around line 184-234: Consolidate retain_dhcp_address_for_family and
retain_expected_machine_dhcp_address_for_family through one private helper
parameterized by provenance, keeping both public functions as thin wrappers.
Reuse the shared predicate, bindings, execution, and result handling; have the
helper explicitly set expected_machine_preallocation according to the parameter,
and verify that writing false for the non-ExpectedMachine path is acceptable
before applying it.
In `@crates/api-db/src/machine_interface.rs`:
- Line 703: Remove the unused newly_created value from the tuple produced by the
relevant mapping flow around the interface extraction, changing the return type
to expose only interface. Update all callers, including the DPU path, to consume
the narrowed result while keeping any creation-state tracking local where needed
for future logging or metrics.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 154-187: Consolidate create_static_assignments_segment,
create_test_segment, and create_managed_segment around one shared
network-segment builder accepting name, prefix list, segment type, and reserved
count. Preserve each helper’s existing fixture behavior through thin
intent-specific wrappers where needed, and update their call sites to use the
shared implementation without duplicating the NewNetworkSegment construction.
In `@crates/rpc/src/lib.rs`:
- Around line 580-697: Consolidate the duplicated ExpectedInterfaceRole and
ExpectedInterfaceIpAllocation adapters into a small declarative macro that
generates their untagged input type and optional
serialize_optional/deserialize_optional helpers, while preserving each enum’s
accepted names, aliases, and output strings. Ensure the generated numeric and
serialization error conversions use explicit serde::de::Error::custom and
serde::ser::Error::custom calls consistently.
In `@crates/site-explorer/src/machine_creator.rs`:
- Around line 1568-1590: Update wait_for_advisory_lock_wait so the polling loop
uses a short Tokio sleep instead of tokio::task::yield_now(). Preserve the
existing query, timeout, and immediate return once waiting is greater than zero.
- Around line 1520-1522: Update the MAC address sorting in the visible
collection-building flow to use MacAddress’s native ordering via sort_unstable()
(or its raw-byte key), instead of sort_by_key(ToString::to_string). Keep the
subsequent deduplication and return behavior unchanged.
🪄 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: e905561f-929c-47df-b103-519afc7745c7
⛔ 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 (58)
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/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/expected_power_shelf.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/machine_interface.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/switch.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/switch.rscrates/api-core/tests/integration/expected_power_shelf_static_address.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-core/tests/integration/power_shelf_delete.rscrates/api-core/tests/integration/static_address_management.rscrates/api-db/migrations/20260722120000_expected_interface_allocation_state.sqlcrates/api-db/src/bmc_metadata.rscrates/api-db/src/bmc_metadata/tests.rscrates/api-db/src/expected_machine.rscrates/api-db/src/expected_machine/test_allocation_state_migration.rscrates/api-db/src/expected_machine/tests.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rscrates/api-db/src/predicted_machine_interface.rscrates/api-model/src/expected_machine.rscrates/api-model/src/machine/mod.rscrates/api-model/src/predicted_machine_interface.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-controller/src/boot_interface.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/expected_machine.rscrates/site-explorer/Cargo.tomlcrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/machine_creator.rscrates/site-explorer/tests/integration/reconcile.rsrest-api/proto/core/src/v1/nico_nico.proto
As it stood, `ExpectedMachine.host_nics` could reserve a host NIC with `fixed_ip`, but every entry was treated as a host data interface. There was no way to say "this is the DPU OS/BMC" or choose `Dynamic`, `Fixed`, or `Retained` for that interface. So, add `Host`, `DpuOs`, and `DpuBmc` roles plus an allocation policy for every entry. Fixed addresses find their segment by prefix containment, while DHCP uses the relay/link segment; `network_segment_type` is an optional check for either path. Retained DHCP allocations use the existing `Static` representation once an address has been selected. The protobuf fields are optional, and old `host_nics` payloads still infer `Fixed` from `fixed_ip` and `Dynamic` otherwise. ExpectedMachine updates only reconcile never-associated reservations, so managed, preserved, and operator-owned interfaces are left alone. `host_nics` also gets a `jsonb_path_ops` GIN index now that DHCP and Site Explorer hit that lookup regularly. Tests cover the old payloads, every role/policy combination, reservation transitions, DHCPv4/v6 retention, and the managed-state boundaries. `HostBmc`, DPU loopbacks, and the `ExpectedInterface`/`interfaces` rename stay in separate follow-ups. This supports NVIDIA#3990 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 Action performedFull review triggered. |
1 participant
As it stood,
ExpectedMachine.host_nicscould reserve a host NIC withfixed_ip, but every entry was treated as a host data interface. There was no way to say "this is the DPU OS/BMC" or chooseDynamic,Fixed, orRetainedfor that interface.So, add
Host,DpuOs, andDpuBmcroles plus an allocation policy for every entry. Fixed addresses find their segment by prefix containment, while DHCP uses the relay/link segment;network_segment_typeis an optional check for either path. Retained DHCP allocations use the existingStaticrepresentation once an address has been selected.The protobuf fields are optional, and old
host_nicspayloads still inferFixedfromfixed_ipandDynamicotherwise. ExpectedMachine updates only reconcile never-associated reservations, so managed, preserved, and operator-owned interfaces are left alone.host_nicsalso gets ajsonb_path_opsGIN index now that DHCP and Site Explorer hit that lookup regularly.Tests cover the old payloads, every role/policy combination, reservation transitions, DHCPv4/v6 retention, and the managed-state boundaries.
HostBmc, DPU loopbacks, and theExpectedInterface/interfacesrename stay in separate follow-ups.Related issues
This supports #3990
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-workspace-depscargo check --tests -p carbide-api-db -p carbide-site-explorer -p carbide-api-corecargo test -p carbide-api-db --lib -- --test-threads=1cargo test -p carbide-api-core --libcargo test -p carbide-site-explorer --libcargo test -p carbide-rpc --features model --lib expected_interfacecargo test -p nico-admin-cli expected_machine_json_preserves_host_nics_field_presencecargo test -p carbide-api-db expected_interface -- --test-threads=1cargo test -p carbide-api-db force_delet --lib -- --test-threads=1cargo test -p carbide-api-db host_mac_lookup_rejects_duplicate --lib -- --test-threads=1cargo test -p carbide-api-core expected_interface -- --test-threads=1cargo test -p carbide-api-core machine_admin_force_delete:: --lib -- --test-threads=1cargo test -p carbide-api-core machine_discovery:: --lib -- --test-threads=1cargo test -p carbide-api-core expected_switch:: --lib -- --test-threads=1cargo test -p carbide-api-core older_client --lib -- --test-threads=1cargo test -p carbide-api-core legacy_expected_machine --lib -- --test-threads=1cargo test -p carbide-api-core --test integration test_force_delete_power_shelf_deletes_interfaces -- --test-threads=1cargo test -p carbide-site-explorer machine_creator::tests --lib -- --test-threads=1cargo test -p carbide-site-explorer --test integration test_machine_creator_clears_removed_dpu_bmc_policy_before_association -- --test-threads=1cargo test -p carbide-site-explorer --test integration test_site_explorer_reconcile_ -- --test-threads=1Regenerated the REST Core protobufs and verified that a second generation pass left the source and generated diff byte-for-byte unchanged.
Additional Notes
host_nicsGIN index.expected_machine_preallocation = NULLintentionally identifies writes from before ownership tracking or from older binaries during a rolling upgrade. Current writers storetrueorfalse.Retaineduses the existingStaticdatabase representation; it does not add another allocation type.ExpectedHostNic/host_nicsrename is Rename ExpectedHostNic to ExpectedInterface #4104, and the docs-only CLI update is Document Expected Interface IP Allocation Policies #4151.