feat: support IP allocation policies for expected interfaces#3991
Conversation
|
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, allocation policies, captured state, and reservation provenance through CLI, RPC, database, DHCP, discovery, Site Explorer, and cleanup workflows. Lock ordering, ambiguity validation, atomic reconciliation, and prepared deletion flows are also expanded. 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 identity and reconcile reservations
DhcpDiscovery->>ApiDB: Resolve captured policy and allocation state
SiteExplorer->>ApiDB: Reload current 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
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
crates/api-core/src/dhcp/discover.rs (1)
626-636: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the redundant rebinding into the original
let.Line 636 exists only to acquire mutability that the
if/elseat 626-630 could have declared directly. Threading the binding through a barelet mut x = x;obscures ownership for no benefit.♻️ Proposed simplification
- let predicted_interface = if prediction_needs_refresh { + let mut predicted_interface = if prediction_needs_refresh { db::predicted_machine_interface::find_by_mac_address(&mut txn, parsed_mac).await? } else { predicted_interface }; let is_primary_nic = host_nic .as_ref() .filter(|interface| interface.role == ExpectedInterfaceRole::Host) .map(ExpectedHostNic::initial_primary_interface); - - let mut predicted_interface = predicted_interface;🤖 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/dhcp/discover.rs` around lines 626 - 636, Make the original predicted_interface binding mutable in the if/else assignment, then remove the redundant `let mut predicted_interface = predicted_interface;` rebinding. Preserve the existing refresh and reuse behavior while updating only the binding declaration near the prediction logic.crates/api-db/src/expected_machine/test_allocation_state_migration.rs (1)
166-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider folding the three prediction snapshot lookups into a table-driven check.
Lines 166-210 repeat the same query three times, differing only in the bound MAC and the expected
(snapshot, captured)pair. A singlecheck_values/check_casestable keyed by MAC would remove the duplication and make the classification matrix (claimed / unclaimed-duplicate / unclaimed-BMC-collision) readable at a glance.As per coding guidelines, "Use table-driven tests for functions mapping inputs to outputs or errors."
🤖 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/test_allocation_state_migration.rs` around lines 166 - 210, Replace the three repeated `sqlx::query_as` assertions in the migration test with a table-driven `check_cases` collection containing each MAC and its expected `(Option<Value>, bool)` result, then iterate through it to execute the shared query and assertions. Preserve the existing expected host snapshot, duplicate, and BMC-collision values while making the classification matrix explicit.Source: Coding guidelines
crates/admin-cli/src/expected_machines/common.rs (1)
139-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the tri-state contract of the new optional
host_nicsfield at its definition. Making this field optional introduced three distinct meanings — omitted, explicitly empty, explicitly populated — that are recorded nowhere, forcing every reader of a consuming call site to trace theOptionback throughpatch_expected_machineandreplace_all_expected_machinesto recover the intent.
crates/admin-cli/src/expected_machines/common.rs#L139-L140: add a doc comment onhost_nicsstating thatNone(omitted) preserves the stored list onupdate,Some(vec![])clears it, andreplace-allcollapsesNoneto an empty list.crates/admin-cli/src/expected_machines/update/mod.rs#L41-L45: add a brief note that an omittedhost_nicsdeliberately preserves the server value, so this command's "result matches the file" claim does not hold for that field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/expected_machines/common.rs` around lines 139 - 140, Document the tri-state behavior of the optional host_nics field at its definition: None means omitted and preserves the stored list on update, Some(empty) clears it, and replace-all converts None to an empty list. Also add a brief note in update/mod.rs near the command’s result-matches-file claim explaining that omitted host_nics intentionally preserves the server value, so that claim excludes this field; update both crates/admin-cli/src/expected_machines/common.rs:139-140 and crates/admin-cli/src/expected_machines/update/mod.rs:41-45.crates/rpc/src/lib.rs (1)
580-697: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the two adapters into one macro.
The role and IP-allocation adapters are structurally identical — untagged input enum,
Optionshort-circuit, alias normalization, canonical string emission — differing only in variant names and aliases. A small declarative macro would keep future enums (and future variants) from drifting between the two copies. Entirely optional if the team prefers the explicit form.🤖 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, Optionally replace the duplicated ExpectedInterfaceRole and ExpectedInterfaceIpAllocation adapter implementations with a small declarative macro that generates their input enum and serialize_optional/deserialize_optional methods. Parameterize the macro with the enum type, input name, variants, canonical names, and accepted aliases, while preserving numeric inputs, null handling, unknown-value errors, and canonical string serialization.crates/api-core/src/handlers/machine.rs (1)
758-801: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePreparation covers interfaces the request may never delete.
affected_interface_idsis assembled unconditionally, so whendelete_interfacesis false the batch still takes exclusive segment and row locks on every associated interface and then discards the prepared handles. Gating the association-derived ids onrequest.delete_interfaces(BMC ids already are gated) would narrow the lock footprint of force-delete without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/machine.rs` around lines 758 - 801, Only add machine-associated interface IDs to affected_interface_ids when request.delete_interfaces is enabled, while preserving the existing BMC-interface gating and preparation flow. Update the association lookup near prepare_deletes_with_exclusive_segments so no interface locks or prepared handles are created when interface deletion is disabled.crates/site-explorer/tests/integration/machine_creator.rs (1)
450-461: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
.clone()by capturing the persisted record.
db::expected_machine::createreturns the persistedExpectedMachine(confirmed by the sibling usage inreconcile.rs:let mut expected_machine = db::expected_machine::create(&mut txn, ...).await?;). Cloning here just to keep a copy for the fixture is unnecessary.♻️ Proposed fix
- let expected_machine = expected_machine(managed_host); let mut txn = env.pool.begin().await.unwrap(); - db::expected_machine::create(&mut txn, expected_machine.clone()) - .await - .unwrap(); + let expected_machine = db::expected_machine::create(&mut txn, expected_machine(managed_host)) + .await + .unwrap(); txn.commit().await.unwrap();As per coding guidelines, "Avoid needless
.clone()calls; borrow values, consume iterators withinto_iter(), reorder struct initialization when possible, or useCowwhere values may be borrowed or owned."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/site-explorer/tests/integration/machine_creator.rs` around lines 450 - 461, Update the fixture setup around db::expected_machine::create to capture its returned persisted ExpectedMachine in expected_machine, rather than cloning the input before the call. Preserve using that returned value in ExploredHostFixture after the transaction commits.Source: Coding guidelines
crates/api-db/src/machine_interface/tests.rs (1)
235-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe lock-wait probe busy-polls the database.
yield_now()gives no back-off, so this loop issuespg_stat_activitycounts as fast as the runtime will schedule it for up to five seconds — measurable CPU and connection churn in CI for every test that uses it. A short sleep keeps the intent identical at a fraction of the cost.♻️ Proposed back-off
if waiting > 0 { return; } - tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(25)).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-db/src/machine_interface/tests.rs` around lines 235 - 257, Update wait_for_advisory_lock_wait to replace the tight-loop tokio::task::yield_now() polling with a short tokio sleep between pg_stat_activity checks, while preserving the five-second timeout and immediate return once waiting is greater than zero.crates/api-db/src/machine_interface.rs (2)
1615-1666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider collapsing the duplicated snapshot SQL.
The
for_updateand read-only variants differ only by the trailingFOR UPDATE; the sizeable projection (including the three-clauseassociatedexpression) is copied verbatim. Aconcat!of a shared body with the optional suffix keeps the two definitions from drifting apart on the next projection change.🤖 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 1615 - 1666, Refactor expected_interface_snapshot so the shared SELECT projection and WHERE clause are defined once, with the FOR UPDATE suffix appended only when for_update is true. Preserve the existing query behavior and DatabaseError handling while removing the duplicated SQL definitions.
5163-5183: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOne query per machine where a single array bind would do.
find_ids_by_machine_associationsissues N round trips for N machine ids.WHERE machine_id = ANY($1) OR attached_dpu_machine_id = ANY($1)collapses this into one statement and removes the manual sort/dedup of the concatenated results (PostgresORDER BY idalready yields them ordered). Worth doing if force-cleanup can ever be handed a sizeable machine set.🤖 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 5163 - 5183, Update find_ids_by_machine_associations to execute one SQL query for all machine_ids, binding the slice as an array and filtering with ANY for both association columns. Preserve ORDER BY id, remove the per-machine loop and manual sort/dedup, and return the single query’s results directly.
🤖 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 1492-1548: Wrap the fallible work in the BatchOperation::Create
arm of the operation_result match in an async block and await it, so conversion,
validation, locking, and creation errors are returned through operation_result.
Preserve the existing Create behavior while ensuring failures reach the shared
rollback_or_log("expected-machine atomic batch failure") path, matching
BatchOperation::Update.
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 538-580: The expected-switch replacement flow must release static
reservations belonging to switches removed from the configuration before
db_expected_switch::clear(). Identify removed switches by comparing previous
with replacements, then mirror the release logic used by the expected-machine
removal path for their materialized bmc_ip_address and nvos_ip_address
reservations, preserving existing reconciliation for retained or newly added
switches.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 4454-4458: Update load_and_lock_all_admin_segments so
lock_network_segments_exclusive acquires the segment locks before
load_prelocked_admin_segments reads the rows. Keep the same segment_ids,
transaction connection, error propagation, and return value, ensuring validation
and snapshot loading occur only after the exclusive locks are held.
---
Nitpick comments:
In `@crates/admin-cli/src/expected_machines/common.rs`:
- Around line 139-140: Document the tri-state behavior of the optional host_nics
field at its definition: None means omitted and preserves the stored list on
update, Some(empty) clears it, and replace-all converts None to an empty list.
Also add a brief note in update/mod.rs near the command’s result-matches-file
claim explaining that omitted host_nics intentionally preserves the server
value, so that claim excludes this field; update both
crates/admin-cli/src/expected_machines/common.rs:139-140 and
crates/admin-cli/src/expected_machines/update/mod.rs:41-45.
In `@crates/api-core/src/dhcp/discover.rs`:
- Around line 626-636: Make the original predicted_interface binding mutable in
the if/else assignment, then remove the redundant `let mut predicted_interface =
predicted_interface;` rebinding. Preserve the existing refresh and reuse
behavior while updating only the binding declaration near the prediction logic.
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 758-801: Only add machine-associated interface IDs to
affected_interface_ids when request.delete_interfaces is enabled, while
preserving the existing BMC-interface gating and preparation flow. Update the
association lookup near prepare_deletes_with_exclusive_segments so no interface
locks or prepared handles are created when interface deletion is disabled.
In `@crates/api-db/src/expected_machine/test_allocation_state_migration.rs`:
- Around line 166-210: Replace the three repeated `sqlx::query_as` assertions in
the migration test with a table-driven `check_cases` collection containing each
MAC and its expected `(Option<Value>, bool)` result, then iterate through it to
execute the shared query and assertions. Preserve the existing expected host
snapshot, duplicate, and BMC-collision values while making the classification
matrix explicit.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 1615-1666: Refactor expected_interface_snapshot so the shared
SELECT projection and WHERE clause are defined once, with the FOR UPDATE suffix
appended only when for_update is true. Preserve the existing query behavior and
DatabaseError handling while removing the duplicated SQL definitions.
- Around line 5163-5183: Update find_ids_by_machine_associations to execute one
SQL query for all machine_ids, binding the slice as an array and filtering with
ANY for both association columns. Preserve ORDER BY id, remove the per-machine
loop and manual sort/dedup, and return the single query’s results directly.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 235-257: Update wait_for_advisory_lock_wait to replace the
tight-loop tokio::task::yield_now() polling with a short tokio sleep between
pg_stat_activity checks, while preserving the five-second timeout and immediate
return once waiting is greater than zero.
In `@crates/rpc/src/lib.rs`:
- Around line 580-697: Optionally replace the duplicated ExpectedInterfaceRole
and ExpectedInterfaceIpAllocation adapter implementations with a small
declarative macro that generates their input enum and
serialize_optional/deserialize_optional methods. Parameterize the macro with the
enum type, input name, variants, canonical names, and accepted aliases, while
preserving numeric inputs, null handling, unknown-value errors, and canonical
string serialization.
In `@crates/site-explorer/tests/integration/machine_creator.rs`:
- Around line 450-461: Update the fixture setup around
db::expected_machine::create to capture its returned persisted ExpectedMachine
in expected_machine, rather than cloning the input before the call. Preserve
using that returned value in ExploredHostFixture after the transaction commits.
🪄 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: 97df67d8-4648-4c19-8858-5e2006842da9
⛔ 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
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (15)
crates/site-explorer/tests/integration/machine_creator.rs (2)
741-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode the nullable
expected_interfacecolumn asOption<Json<...>>.
expected_interfaceis nullable, so decoding straight intoJson<ExpectedHostNic>turns a regression that leaves the columnNULLinto an opaque decode error rather than a readable assertion failure. The sibling tests at lines 954-961 and 1027-1034 already use theOptionform.♻️ Proposed refactor
- let saved_interface = sqlx::query_scalar::< - _, - sqlx::types::Json<model::expected_machine::ExpectedHostNic>, - >("SELECT expected_interface FROM machine_interfaces WHERE id = $1") - .bind(interface.id) - .fetch_one(&mut *txn) - .await?; + let saved_interface: Option<sqlx::types::Json<ExpectedHostNic>> = + sqlx::query_scalar("SELECT expected_interface FROM machine_interfaces WHERE id = $1") + .bind(interface.id) + .fetch_one(&mut *txn) + .await?; + let saved_interface = + saved_interface.expect("the fixed policy should be captured before association");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/site-explorer/tests/integration/machine_creator.rs` around lines 741 - 748, Update the query in the test around saved_interface to decode expected_interface as Option<Json<model::expected_machine::ExpectedHostNic>>, matching the sibling tests. Preserve the existing fixed_ip assertion while ensuring a NULL column produces an assertion failure rather than a decode error.
450-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStore the row returned by
db::expected_machine::create, not the pre-insert value.The fixture discards
create's return value and keeps the locally constructedexpected_machine. Sincecreate_managed_hostre-fetches the rowFOR UPDATEand compares identity, any field the database normalizes or assigns would make the fixture diverge from the persisted row and surface as an opaqueFailedPrecondition.crates/api-core/src/tests/machine_states.rs(lines 526-528) already rebinds to the returned row; aligning here keeps the two fixtures on one contract.♻️ Proposed refactor
- let expected_machine = expected_machine(managed_host); let mut txn = env.pool.begin().await.unwrap(); - db::expected_machine::create(&mut txn, expected_machine.clone()) + let expected_machine = db::expected_machine::create(&mut txn, expected_machine(managed_host)) .await .unwrap(); txn.commit().await.unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/site-explorer/tests/integration/machine_creator.rs` around lines 450 - 462, In the fixture setup around db::expected_machine::create, rebind expected_machine to the row returned by create instead of retaining the pre-insert value. Continue cloning or passing this persisted result into ExploredHostFixture so it reflects database-normalized or assigned fields.crates/api-db/src/machine_interface/tests.rs (1)
235-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a small delay to the advisory-lock poll loop.
yield_now()provides no backpressure, so this helper issuesCOUNT(*)againstpg_stat_activityas fast as the runtime permits while contending for pool connections with the very transactions under test. A short sleep detects the wait just as promptly at a fraction of the cost, and this helper is now on the hot path of six concurrency tests.♻️ 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/api-db/src/machine_interface/tests.rs` around lines 235 - 257, Update wait_for_advisory_lock_wait to replace the unbounded tokio::task::yield_now polling with a short tokio sleep between pg_stat_activity queries, preserving the existing timeout and successful return behavior.crates/api-db/src/machine_interface.rs (3)
1838-1853: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe guarded pre-read does not protect the final
UPDATEinreplace_expected_interface.The
SELECT ... FOR UPDATEfilters associated rows out, so for an associated row it returnsNoneand takes no row lock at all — after which the unconditionalUPDATEat line 1910 still overwritesexpected_interface. Today every caller pre-checks association (the deliberate exception being the legacy-finalization path at line 1786), so this is a hardening gap rather than a live defect, but the asymmetry withclear_expected_interface_if_anonymous, whose final statement does carry the anonymity predicate, invites a future regression.Either mirror the predicate here or state in the doc comment that associated rewrites are the caller's responsibility.
Also applies to: 1910-1921
🤖 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 1838 - 1853, Update replace_expected_interface, including its final UPDATE, so the anonymity/association predicate is enforced by the write itself rather than only by the guarded SELECT; alternatively, explicitly document in the function’s doc comment that callers must prevent associated rewrites. Prefer mirroring the existing SELECT predicate in the UPDATE while preserving the transaction and expected-interface behavior.
1620-1643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo hand-copied SQL bodies differing only by
FOR UPDATE.The
associatedexpression is now repeated verbatim in four places in this file (here twice, plus lines 1841-1845 and 1930-1934, 1975-1979). Consider a singleconst ASSOCIATED_PREDICATE: &strand a conditional" FOR UPDATE"suffix so the definition of "associated" cannot drift between the locking and non-locking readers.[scratchpad_end_marker_none]♻️ Sketch
- let query = if for_update { - "SELECT - id, - ... - WHERE mac_address = $1 - FOR UPDATE" - } else { - "SELECT - id, - ... - WHERE mac_address = $1" - }; + let query = format!( + "SELECT id, segment_id, expected_interface, expected_interface_captured, + {ASSOCIATED_PREDICATE} AS associated + FROM machine_interfaces + WHERE mac_address = $1{}", + if for_update { " FOR UPDATE" } else { "" } + );(
DatabaseError::querytakes the statement text, so aStringworks with a borrow at the call sites.)🤖 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 1620 - 1643, Consolidate the duplicated associated SQL expression used by the query-building code around the visible `for_update` branch into one shared `ASSOCIATED_PREDICATE` constant, and construct a single base query with a conditional `FOR UPDATE` suffix. Reuse the same predicate for the related query definitions in this file so locking and non-locking readers cannot diverge.
735-735: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
newly_createdreturn valuecrates/api-db/src/machine_interface.rs:728-763— bothfind_or_create_machine_interface_innerandvalidate_existing_mac_and_create_innerthread this flag through only to discard it at the call sites. Returning justMachineInterfaceSnapshotwould simplify the internal API.🤖 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 735, Remove the unused newly_created boolean from the return type and implementations of find_or_create_machine_interface_inner and validate_existing_mac_and_create_inner, returning only MachineInterfaceSnapshot. Update all callers and destructuring to consume the snapshot directly while preserving the existing machine-interface creation and validation behavior.crates/site-explorer/tests/integration/reconcile.rs (1)
426-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a positive counterpart for the retention path.
try_reconcile_expected_machine_bmcswallows every failure into awarn!, so this assertion also holds if the helper aborted early for an unrelated reason. A sibling case that leaves the policy atRetainedand asserts the address flips toStaticwould prove the mechanism is actually exercised, not merely inert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/site-explorer/tests/integration/reconcile.rs` around lines 426 - 438, Add a sibling integration test covering the retained-policy path through try_reconcile_expected_machine_bmc: keep the expected machine’s BMC IP allocation as Retained, run reconciliation, then query the interface addresses and assert the allocation type becomes Static. Follow the existing transaction and setup pattern so the assertion verifies reconciliation occurred rather than passing after an early failure.crates/site-explorer/src/lib.rs (2)
3570-3577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree distinct failure causes emit the identical log line.
"Site-explorer expected-machine BMC reconcile skipped"is used for lock acquisition failure, lookup failure, and preallocation/retain failure. The message literal should stay stable, so add a discriminating structured field rather than varying the text — the guidelines already reservereasonfor exactly this.As per coding guidelines, use "stable semantic logging field names:
error,grpc_status_code,http_status,reason, …".♻️ Suggested shape
if let Err(error) = db::expected_machine::lock_config_mutations_shared(txn.as_pgconn()).await { tracing::warn!( %error, %bmc_mac_address, + reason = "lock_config_mutations", "Site-explorer expected-machine BMC reconcile skipped" ); return; }The same applies to
try_reconcile_expected_switch_addresses, which repeats one message across four sites.Also applies to: 3589-3596, 3622-3629
🤖 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 3570 - 3577, Add a structured `reason` field to every warning using the stable “Site-explorer expected-machine BMC reconcile skipped” message, assigning distinct values for lock acquisition, lookup, and preallocation/retain failures. Apply the same treatment to all warning sites in `try_reconcile_expected_switch_addresses`, covering each of its four failure paths, without changing the message literal.Source: Coding guidelines
2035-2061: 🚀 Performance & Scalability | 🔵 TrivialSerial per-entry transactions will dominate this phase as the site grows.
Each expected machine now costs one transaction for the BMC reconcile plus one per fixed-IP host NIC, all sequential. The
update_explored_endpoints_preallocatephase latency already instruments this, so it is observable rather than invisible — worth watching the metric at scale and, if it grows, either batching the reconcile per machine or bounding concurrency with a semaphore the way the exploration probes are bounded.🤖 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, Update the update_explored_endpoints_preallocate phase so expected-machine BMC reconciliation and fixed-IP NIC preallocation do not execute as unbounded serial transactions. Reuse the existing bounded-concurrency semaphore pattern from exploration probes, or batch each machine’s reconciliation work, while preserving switch reconciliation behavior and the phase’s existing instrumentation.crates/api-db/src/bmc_metadata.rs (1)
68-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead-only lookup should accept
impl DbReader.
find_interface_by_bmc_ipperforms no writes and takes no locks, yet it requires&mut PgConnection, which forces read-only callers into a transaction handle. Generifying the shared helper overDbReaderkeeps the_for_updatevariant on&mut PgConnectionwhile making the plain variant usable from read paths.As per coding guidelines: "Keep database calls in
api-dbas bare functions accepting model values, while keeping model definitions inapi-model. Useimpl DbReaderfor read-only database functions."🤖 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 68 - 83, Update find_interface_by_bmc_ip and the shared find_interface_by_bmc_ip_with_query helper to accept impl DbReader for read-only access, while preserving the _for_update variant’s &mut PgConnection signature for locking behavior. Keep the existing query and return contract unchanged.Source: Coding guidelines
crates/api-db/migrations/20260722120000_expected_interface_allocation_state.sql (1)
49-50: 🧹 Nitpick | 🔵 TrivialPlan the rollout window for the unqualified rewrites and the index build.
Two observations for deployment, not code defects:
- Line 49-50 rewrites every
predicted_machine_interfacesrow (the second full-table pass in this migration), and Lines 56-60 rewrite a large share ofmachine_interfaces. Both run inside the migration transaction, so row locks and bloat persist until commit.- Lines 71-72 take
ACCESS EXCLUSIVEonexpected_machinesfor the duration of the build. That table is small, so this should be brief; the note is only so the combined migration duration is budgeted against the API-server startup gate.The
jsonb_path_opsopclass choice is correct for thehost_nics @> …containment lookups added inapi-db, and plainCREATE INDEXis the right call for a transactional migration.The Squawk
require-concurrent-index-creationhint is not applicable:CONCURRENTLYis incompatible withsqlx::migrate!()'s transactional DDL, which is this repository's established convention.Also applies to: 71-72
🤖 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 49 - 50, The review identifies deployment planning considerations only; no code changes are required. Keep the full-table updates, transactional index creation, and existing jsonb_path_ops choice unchanged, and ensure the rollout window accounts for migration locks, bloat, index-build duration, and the API-server startup gate.Source: Linters/SAST tools
crates/api-db/src/expected_machine.rs (2)
133-139: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument that this containment predicate is representation-coupled.
host_nics @> [{"mac_address": "<Display>"}]matches on the textual JSON value, so it silently returns no rows if a writer ever persists a MAC in a different case or separator style. Note the contrast withvalidate_identity_macs_available(Line 250) and the migration, which both cast tomacaddrand are representation-independent.The coupling is currently sound because reads and writes share one
ExpectedHostNicserde, and the textual form is what lets the newjsonb_path_opsGIN index serve this lookup. A short comment stating that invariant would prevent a future serde change from turning a lookup miss into a silent ingestion failure.🤖 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 133 - 139, Add a concise comment immediately above the mac_address JSON construction or query in the expected-machine lookup explaining that the JSON containment predicate compares textual MAC representations, relies on shared ExpectedHostNic serialization for reads and writes, and must remain aligned with the jsonb_path_ops index; contrast this with macaddr-cast validation only if needed to clarify the invariant.
242-254: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIdentity validation is a per-record full scan with lateral expansion.
The
jsonb_array_elements(host_nics)+(… ->> 'mac_address')::macaddrpredicate cannot use the newjsonb_path_opsGIN index, so each invocation is a sequential scan overexpected_machineswith per-row array expansion. Callers run it in a loop — one call per model increate_expected_machine_records,create_missing_from, and per changed machine inapply_atomic_batch_updates— making a large import O(batch × rows × nics).Consider a single set-based query per batch, or an index-friendly containment predicate over the candidate MACs, so bulk imports do not scale quadratically.
🤖 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 242 - 254, The identity validation query should avoid per-record JSON expansion and repeated full-table scans. Update the expected-machine validation flow around this query and its callers in create_expected_machine_records, create_missing_from, and apply_atomic_batch_updates to perform batch-level validation, or replace the jsonb_array_elements predicate with an index-friendly containment check over candidate MAC addresses while preserving the existing identity-matching behavior.crates/rpc/src/lib.rs (1)
580-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the two identical adapters into one macro.
The role and allocation implementations differ only in their alias tables; the input enums,
Optionhandling, numeric conversion, and error shaping are byte-for-byte duplicates. A small declarative macro taking the type plus its alias/label pairs would keep future enums from copying another sixty lines.🤖 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 - 696, Replace the duplicated ExpectedInterfaceRole and ExpectedInterfaceIpAllocation adapters with one declarative macro parameterized by the enum type and alias/serialized-label mappings. Have the macro generate the shared input enum, deserialize_optional, and serialize_optional logic while preserving numeric conversion, Option handling, and existing unknown-value errors; invoke it separately with each enum’s current aliases and labels.crates/api-db/src/machine_interface_address.rs (1)
184-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the two retain helpers onto one implementation.
retain_expected_machine_dhcp_address_for_familydiffers fromretain_dhcp_address_for_familyonly by also setting the provenance marker. Expressing the promotion once, parameterised by ownership, keeps the two SQL predicates from drifting apart later.♻️ Suggested consolidation
async fn retain_dhcp_address_for_family_owned( txn: &mut PgConnection, interface_id: MachineInterfaceId, family: IpAddressFamily, expected_machine_owned: bool, ) -> Result<bool, DatabaseError> { let query = "UPDATE machine_interface_addresses SET allocation_type = $3, expected_machine_preallocation = expected_machine_preallocation OR $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_owned) .execute(txn) .await .map(|result| result.rows_affected() > 0) .map_err(|error| DatabaseError::query(query, error)) }🤖 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 a shared helper, such as retain_dhcp_address_for_family_owned, parameterized by whether the address is expected-machine-owned. Use one UPDATE statement that promotes DHCP to Static and preserves or sets expected_machine_preallocation via the ownership parameter, while keeping both public helpers’ 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.
Inline comments:
In `@crates/api-core/src/dhcp/discover.rs`:
- Around line 825-830: Update persist_expected_interface_if_missing to avoid
issuing a write when expected_interface_captured is already true by adding that
flag to the statement predicate; preserve the existing behavior of marking the
interface captured, including when host_nic is None.
In `@crates/api-core/src/handlers/expected_machine.rs`:
- Around line 825-837: Update the conflict error construction in the second pass
over machines to report the changed machine identified by
changed_owners[&mac_address], rather than the unchanged machine’s
machine.bmc_mac_address. Preserve the existing conflict detection and error
behavior while using the recorded changed-machine counterpart in the message.
- Around line 1045-1056: Update the interface resolution in the expected-machine
handler around expected_interface_for_mac to propagate a lookup failure as an
InvalidArgument response instead of panicking via expect. Preserve the existing
MAC-based resolution and sorting behavior, and use the handler’s established
error-return path.
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 363-377: Update update_expected_switch to release prior
expected_machine_preallocation entries when the new switch payload clears
bmc_ip_address or nvos_ip_address, before reconciling the remaining static
interfaces. Ensure retained addresses continue through
resolve_and_lock_expected_switch_static_interfaces and
reconcile_expected_switch_static_interfaces unchanged; if updates are
intentionally one-way instead, add an inline note documenting that contract.
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 341-349: Update the interface filtering in
delete_prepared_machine_interfaces() to reuse the same association predicate as
find_ids_by_machine_associations(), including attached_dpu_machine_id alongside
machine_id while still excluding BMC interfaces. Ensure association-only
interfaces are included in both deletion and the reported results.
In `@crates/api-core/src/tests/expected_machine.rs`:
- Around line 110-162: The advisory-lock wait helpers busy-poll the database and
can contend with the task being observed. In
crates/api-core/src/tests/expected_machine.rs#L110-L162, update both
wait_for_expected_machine_advisory_lock and wait_for_advisory_lock_wait to sleep
for 10 ms between probes instead of calling yield_now; make the same replacement
in wait_for_advisory_lock_waiters in
crates/api-core/src/tests/machine_dhcp.rs#L96-L118.
In `@crates/api-db/src/bmc_metadata.rs`:
- Around line 155-159: The enrich-then-associate flow resolves its write target
without locking, allowing ownership changes before
capture_expected_interface_before_association or associate_bmc_interface runs.
In crates/api-db/src/bmc_metadata.rs:155-159, replace
machine_interface::find_one with a locking single-interface lookup or fall back
to find_interface_by_bmc_ip_for_update when bmc_info.ip is available. In
crates/api-db/src/bmc_metadata.rs:222-234, use
find_interface_by_bmc_ip_for_update whenever the resolved machine_interface_id
will be reused for a write, including the persist path, so cached IDs cannot
bypass the locked association flow.
In `@crates/api-db/src/expected_machine.rs`:
- Around line 149-166: Update the ambiguity errors in the shown ExpectedMachine
MAC-resolution logic to use DatabaseError::internal instead of
DatabaseError::InvalidArgument for both the BMC-and-nested conflict and multiple
nested interface declarations. Preserve the existing messages and successful
resolution behavior, matching find_interface_by_bmc_ip_with_query and
find_by_nvos_mac_address.
In `@crates/api-db/src/expected_switch.rs`:
- Around line 374-384: The row-resolution logic in delete_by_id/delete_by_mac
and the site-explorer hardware-truth write must preserve their prior tolerant
behavior: when find returns no expected-switch row, return Ok(()) without
applying MAC locks or propagating NotFoundError. Update both affected sites in
crates/api-db/src/expected_switch.rs (anchor lines 374-384 and sibling lines
450-456), while retaining normal processing when a row is found.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 4110-4114: Update the FailedPrecondition messages in the visible
reconciliation error and the additional occurrences around the related Loaded
and Previously messages to begin with lowercase words, while preserving the
remaining text and existing no-trailing-period format.
- Around line 3540-3547: Update the shared
unique_instance_discovery_interface_id helper to sort and deduplicate the
supplied MachineInterfaceId values before performing its length-based ambiguity
check. Preserve the existing unambiguous, missing, and ambiguous outcomes, and
apply this in the helper so both regular and locking query variants handle
duplicate joined rows consistently.
---
Nitpick comments:
In
`@crates/api-db/migrations/20260722120000_expected_interface_allocation_state.sql`:
- Around line 49-50: The review identifies deployment planning considerations
only; no code changes are required. Keep the full-table updates, transactional
index creation, and existing jsonb_path_ops choice unchanged, and ensure the
rollout window accounts for migration locks, bloat, index-build duration, and
the API-server startup gate.
In `@crates/api-db/src/bmc_metadata.rs`:
- Around line 68-83: Update find_interface_by_bmc_ip and the shared
find_interface_by_bmc_ip_with_query helper to accept impl DbReader for read-only
access, while preserving the _for_update variant’s &mut PgConnection signature
for locking behavior. Keep the existing query and return contract unchanged.
In `@crates/api-db/src/expected_machine.rs`:
- Around line 133-139: Add a concise comment immediately above the mac_address
JSON construction or query in the expected-machine lookup explaining that the
JSON containment predicate compares textual MAC representations, relies on
shared ExpectedHostNic serialization for reads and writes, and must remain
aligned with the jsonb_path_ops index; contrast this with macaddr-cast
validation only if needed to clarify the invariant.
- Around line 242-254: The identity validation query should avoid per-record
JSON expansion and repeated full-table scans. Update the expected-machine
validation flow around this query and its callers in
create_expected_machine_records, create_missing_from, and
apply_atomic_batch_updates to perform batch-level validation, or replace the
jsonb_array_elements predicate with an index-friendly containment check over
candidate MAC addresses while preserving the existing identity-matching
behavior.
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 a shared helper, such as
retain_dhcp_address_for_family_owned, parameterized by whether the address is
expected-machine-owned. Use one UPDATE statement that promotes DHCP to Static
and preserves or sets expected_machine_preallocation via the ownership
parameter, while keeping both public helpers’ return behavior unchanged.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 1838-1853: Update replace_expected_interface, including its final
UPDATE, so the anonymity/association predicate is enforced by the write itself
rather than only by the guarded SELECT; alternatively, explicitly document in
the function’s doc comment that callers must prevent associated rewrites. Prefer
mirroring the existing SELECT predicate in the UPDATE while preserving the
transaction and expected-interface behavior.
- Around line 1620-1643: Consolidate the duplicated associated SQL expression
used by the query-building code around the visible `for_update` branch into one
shared `ASSOCIATED_PREDICATE` constant, and construct a single base query with a
conditional `FOR UPDATE` suffix. Reuse the same predicate for the related query
definitions in this file so locking and non-locking readers cannot diverge.
- Line 735: Remove the unused newly_created boolean from the return type and
implementations of find_or_create_machine_interface_inner and
validate_existing_mac_and_create_inner, returning only MachineInterfaceSnapshot.
Update all callers and destructuring to consume the snapshot directly while
preserving the existing machine-interface creation and validation behavior.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 235-257: Update wait_for_advisory_lock_wait to replace the
unbounded tokio::task::yield_now polling with a short tokio sleep between
pg_stat_activity queries, preserving the existing timeout and successful return
behavior.
In `@crates/rpc/src/lib.rs`:
- Around line 580-696: Replace the duplicated ExpectedInterfaceRole and
ExpectedInterfaceIpAllocation adapters with one declarative macro parameterized
by the enum type and alias/serialized-label mappings. Have the macro generate
the shared input enum, deserialize_optional, and serialize_optional logic while
preserving numeric conversion, Option handling, and existing unknown-value
errors; invoke it separately with each enum’s current aliases and labels.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3570-3577: Add a structured `reason` field to every warning using
the stable “Site-explorer expected-machine BMC reconcile skipped” message,
assigning distinct values for lock acquisition, lookup, and preallocation/retain
failures. Apply the same treatment to all warning sites in
`try_reconcile_expected_switch_addresses`, covering each of its four failure
paths, without changing the message literal.
- Around line 2035-2061: Update the update_explored_endpoints_preallocate phase
so expected-machine BMC reconciliation and fixed-IP NIC preallocation do not
execute as unbounded serial transactions. Reuse the existing bounded-concurrency
semaphore pattern from exploration probes, or batch each machine’s
reconciliation work, while preserving switch reconciliation behavior and the
phase’s existing instrumentation.
In `@crates/site-explorer/tests/integration/machine_creator.rs`:
- Around line 741-748: Update the query in the test around saved_interface to
decode expected_interface as
Option<Json<model::expected_machine::ExpectedHostNic>>, matching the sibling
tests. Preserve the existing fixed_ip assertion while ensuring a NULL column
produces an assertion failure rather than a decode error.
- Around line 450-462: In the fixture setup around db::expected_machine::create,
rebind expected_machine to the row returned by create instead of retaining the
pre-insert value. Continue cloning or passing this persisted result into
ExploredHostFixture so it reflects database-normalized or assigned fields.
In `@crates/site-explorer/tests/integration/reconcile.rs`:
- Around line 426-438: Add a sibling integration test covering the
retained-policy path through try_reconcile_expected_machine_bmc: keep the
expected machine’s BMC IP allocation as Retained, run reconciliation, then query
the interface addresses and assert the allocation type becomes Static. Follow
the existing transaction and setup pattern so the assertion verifies
reconciliation occurred rather than passing after an early failure.
🪄 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: 34868109-f906-4f26-a448-eaa052498a26
⛔ 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
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
crates/rpc/src/lib.rs (1)
580-636: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo structurally identical adapters — consider collapsing the boilerplate.
ExpectedInterfaceRoleandExpectedInterfaceIpAllocationdiffer only in their alias tables and canonical names; the untagged input enum, theOptionunwrap, the numerictry_from, and the serializer shape are byte-for-byte equivalent. A single declarative macro taking the enum path plus(canonical_name, aliases…) => Variantrows would keep the alias tables visible while eliminating ~50 duplicated lines, and would guarantee the next enum added toExpectedHostNicinherits identical semantics rather than a hand-copied approximation.Deferrable — the current form is correct and readable. Flagging it because a third copy is the point at which the drift becomes expensive.
Also applies to: 638-696
🤖 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 - 636, Consolidate the duplicated ExpectedInterfaceRole and ExpectedInterfaceIpAllocation serde adapters into one declarative macro that accepts the enum type and canonical-name/alias mappings. Reuse the macro for both implementations, preserving their numeric handling, optional values, validation, canonical serialization names, and distinct alias tables.crates/api-core/src/handlers/expected_switch.rs (1)
338-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
NotFoundidentifier can render empty.When neither
expected_switch_idnorbmc_mac_addresssurvives conversion,unwrap_or_default()yields an emptyid, producing aNotFoundmessage with no subject. IfExpectedSwitchRequestpermits both selectors to be absent, rejecting that shape asInvalidArgumentbefore the lookup would give operators an actionable message instead.🤖 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_switch.rs` around lines 338 - 345, Update the expected-switch lookup flow around the NotFoundError construction to validate that ExpectedSwitchRequest provides at least one usable selector before querying. If both expected_switch_id and bmc_mac_address are absent or unusable, return InvalidArgument with an actionable message; otherwise preserve the existing lookup and NotFound behavior.crates/api-core/src/handlers/expected_machine.rs (1)
883-951: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
transitioned_macsparameter where it is consumed as a retained set.
release_removed_fixed_reservationsforwardstransitioned_macsintoclear_removed_expected_interface_snapshots(txn, previous, retained_macs), where the value is negated (!retained_macs.contains(...)). The two names describe opposite intents at the same call boundary, which makes the delete/delete_all/replace_all call sites (empty set vs. full declaration set) harder to reason about. A single name such asstill_declared_macswould remove the ambiguity without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/expected_machine.rs` around lines 883 - 951, Rename the consumed `transitioned_macs` parameter and its corresponding argument in `release_removed_fixed_reservations` to a retained-set name such as `still_declared_macs`, including the call to `clear_removed_expected_interface_snapshots`. Preserve the existing set contents and all reservation-release behavior; only clarify the parameter naming at this boundary.crates/api-core/src/handlers/machine.rs (1)
341-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
remove()error path.interface_idsis derived fromprepared_interfaces, soremove()cannot returnNonehere. Drain the matching entries directly withextract_ifto make the invariant explicit.🤖 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, Replace the interface_ids collection and subsequent remove loop with direct draining via extract_if on prepared_interfaces, selecting non-Bmc interfaces belonging to machine_id and passing each extracted prepared entry to db::machine_interface::delete_prepared. Remove the unreachable FailedPrecondition error handling while preserving transaction usage and deletion behavior.crates/api-core/src/tests/machine_dhcp.rs (1)
96-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree byte-identical advisory-lock wait helpers, each coupled to the SQL text of the lock helper. All three poll
pg_stat_activitywithquery LIKE '%pg_advisory_xact_lock%'. If a DB lock helper ever switches topg_advisory_xact_lock_sharedor wraps the call, every one of these silently stops matching and the affected tests degrade into 5-second timeouts rather than reporting the real problem. Extracting one shared helper (and preferring thepg_blocking_pids-based predicate already used incrates/api-core/src/tests/expected_machine.rs, which does not depend on query text) removes both the duplication and the coupling.
crates/api-core/src/tests/machine_dhcp.rs#L96-L118: move this parameterised waiter into the shared test-support module and drop thequery LIKEpredicate in favour of blocker-pid scoping.crates/api-core/src/tests/machine_discovery.rs#L85-L107: delete the local copy and call the shared helper.crates/site-explorer/src/machine_creator.rs#L1568-L1590: delete the local copy and call the shared helper.🤖 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 - 118, Extract the parameterized wait_for_advisory_lock_waiters helper from crates/api-core/src/tests/machine_dhcp.rs#L96-L118 into the shared test-support module, replacing the query-text predicate with the existing pg_blocking_pids-based scoping used in crates/api-core/src/tests/expected_machine.rs. Delete the duplicate local helper and call the shared helper from crates/api-core/src/tests/machine_discovery.rs#L85-L107 and crates/site-explorer/src/machine_creator.rs#L1568-L1590.crates/site-explorer/src/machine_creator.rs (1)
705-725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrediction paths hand-roll the expected-interface lookup instead of reusing
expected_interface_for_mac. Both sites searchmachine_data.host_nicsby MAC and then patchprimarythemselves, diverging from the association paths (Lines 631-638, 763-770) that callExpectedMachineData::expected_interface_for_mac— and diverging from each other, since only one of the two applies therole.is_host()guard. A single helper that returns the normalised declaration and then applies the settled boot choice keeps the stored snapshot consistent across every ingestion path.
crates/site-explorer/src/machine_creator.rs#L705-L725: replace the manualhost_nicssearch withexpected_interface_for_mac(mac_address)and apply theis_primaryoverride on the result.crates/site-explorer/src/machine_creator.rs#L828-L846: do the same fordeclared_mac, so therole.is_host()handling matches the sibling site rather than unconditionally forcingprimary = Some(true).🤖 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 705 - 725, Update crates/site-explorer/src/machine_creator.rs:705-725 and 828-846 to replace each manual host_nics MAC lookup with ExpectedMachineData::expected_interface_for_mac, then apply the settled boot choice only through the helper’s normalized result and existing role.is_host() behavior; use mac_address at 705-725 and declared_mac at 828-846, ensuring both prediction paths produce the same expected-interface snapshot.crates/api-model/src/expected_machine.rs (1)
255-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
primarynormalized in one place.ExpectedHostNic::primaryis public, andexpected_interface_for_mac()can return DPU interfaces with a rawprimaryvalue that disagrees withinitial_primary_interface(). Hide the field behind the accessor, or normalizeprimaryfor every role when building the snapshot so callers cannot observe two different answers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-model/src/expected_machine.rs` around lines 255 - 265, Ensure ExpectedHostNic::primary has a single normalized source of truth: make the field private and route reads through initial_primary_interface(), or normalize it for every role when constructing snapshots returned by expected_interface_for_mac(). Prevent callers from observing a raw primary value that conflicts with initial_primary_interface(), while preserving the existing Host, DpuOs, and DpuBmc semantics.crates/api-db/src/bmc_metadata.rs (1)
232-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
%shorthand for the MAC field and drop the duplicate read.
MacAddressimplementsDisplay, so?emits a Debug rendering that is inconsistent with the surrounding logfmt fields; the already-boundbmc_mac_addresslocal also makes the second field access redundant.As per coding guidelines: "Use logfmt-compatible structured logging … use native shorthand,
%forDisplay, and?forDebug."♻️ Proposed logging fix
tracing::info!( caller = %caller, machine_id = %machine_id, - mac_address = ?bmc_interface_owner.mac_address, + mac_address = %bmc_mac_address, "Enriching BMC information", );🤖 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 232 - 244, Update the tracing::info! call in the bmc_interface_owner branch to log the existing bmc_mac_address local with `%` instead of rereading bmc_interface_owner.mac_address with `?`; leave the remaining enrichment assignments unchanged.Source: Coding guidelines
crates/api-db/src/expected_machine.rs (1)
205-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort
identity_macswithsort_unstable().MacAddressalready implementsOrd, so ordering directly makes the lock-order invariant explicit and avoids allocating aStringper key.🤖 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 205 - 224, Update lock_identity_macs to sort identity_macs directly with sort_unstable(), relying on MacAddress’s Ord implementation; remove the sort_by_key(ToString::to_string) call while preserving deduplication and lock acquisition order.Source: Coding guidelines
crates/api-db/src/machine_interface_address.rs (1)
523-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the lock preconditions on these two public deletion helpers.
Unlike the neighbouring additions (
ensure_available_for_interface,find_for_interface_for_update,release_expected_machine_retained_addresses), these two bypassdelete_address_allocations_with_lock_orderand issue an unguardedDELETE. They are safe today only because their callers inmachine_interface.rs(replace_expected_interface,clear_expected_interface_if_anonymous) already hold the MAC, fixed-address, and interface-row locks. Stating that contract in the doc comments keeps a future caller from reintroducing the lock-order inversion this PR removes elsewhere.♻️ Proposed documentation
/// Delete a static address only when ExpectedMachine reconciliation created it. +/// +/// Callers must already hold the interface MAC, fixed-address, and interface +/// row locks; this helper deliberately performs no locking of its own. pub async fn delete_expected_machine_preallocation_by_interface_and_address(🤖 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 523 - 568, Update the doc comments for delete_expected_machine_preallocation_by_interface_and_address and delete_expected_machine_preallocation_or_legacy_by_interface_and_address to state that callers must already hold the MAC, fixed-address, and interface-row locks before invoking these helpers. Preserve the existing deletion semantics and clarify that the helpers intentionally perform an unguarded DELETE under those lock preconditions.
🤖 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/machine_discovery.rs`:
- Around line 315-340: Sort and deduplicate the accumulated segment_ids and
fixed_allocations before invoking lock_network_segments_exclusive and
lock_static_address_keys_after_segment_locks. Preserve the existing per-MAC
collection logic, but establish one deterministic ordering for both lock sets so
concurrent discoveries acquire overlapping locks consistently.
In `@crates/site-explorer/src/lib.rs`:
- Around line 2054-2060: Update update_explored_endpoints and the corresponding
reconciliation path around try_reconcile_expected_switch_addresses so
ExpectedSwitch records with no declared static address are identified before
acquiring the write lock or opening a per-switch write transaction. Reload and
filter the relevant rows first, then invoke reconciliation only for switches
requiring changes, while preserving existing address reconciliation behavior for
applicable records.
---
Nitpick comments:
In `@crates/api-core/src/handlers/expected_machine.rs`:
- Around line 883-951: Rename the consumed `transitioned_macs` parameter and its
corresponding argument in `release_removed_fixed_reservations` to a retained-set
name such as `still_declared_macs`, including the call to
`clear_removed_expected_interface_snapshots`. Preserve the existing set contents
and all reservation-release behavior; only clarify the parameter naming at this
boundary.
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 338-345: Update the expected-switch lookup flow around the
NotFoundError construction to validate that ExpectedSwitchRequest provides at
least one usable selector before querying. If both expected_switch_id and
bmc_mac_address are absent or unusable, return InvalidArgument with an
actionable message; otherwise preserve the existing lookup and NotFound
behavior.
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 341-357: Replace the interface_ids collection and subsequent
remove loop with direct draining via extract_if on prepared_interfaces,
selecting non-Bmc interfaces belonging to machine_id and passing each extracted
prepared entry to db::machine_interface::delete_prepared. Remove the unreachable
FailedPrecondition error handling while preserving transaction usage and
deletion behavior.
In `@crates/api-core/src/tests/machine_dhcp.rs`:
- Around line 96-118: Extract the parameterized wait_for_advisory_lock_waiters
helper from crates/api-core/src/tests/machine_dhcp.rs#L96-L118 into the shared
test-support module, replacing the query-text predicate with the existing
pg_blocking_pids-based scoping used in
crates/api-core/src/tests/expected_machine.rs. Delete the duplicate local helper
and call the shared helper from
crates/api-core/src/tests/machine_discovery.rs#L85-L107 and
crates/site-explorer/src/machine_creator.rs#L1568-L1590.
In `@crates/api-db/src/bmc_metadata.rs`:
- Around line 232-244: Update the tracing::info! call in the bmc_interface_owner
branch to log the existing bmc_mac_address local with `%` instead of rereading
bmc_interface_owner.mac_address with `?`; leave the remaining enrichment
assignments unchanged.
In `@crates/api-db/src/expected_machine.rs`:
- Around line 205-224: Update lock_identity_macs to sort identity_macs directly
with sort_unstable(), relying on MacAddress’s Ord implementation; remove the
sort_by_key(ToString::to_string) call while preserving deduplication and lock
acquisition order.
In `@crates/api-db/src/machine_interface_address.rs`:
- Around line 523-568: Update the doc comments for
delete_expected_machine_preallocation_by_interface_and_address and
delete_expected_machine_preallocation_or_legacy_by_interface_and_address to
state that callers must already hold the MAC, fixed-address, and interface-row
locks before invoking these helpers. Preserve the existing deletion semantics
and clarify that the helpers intentionally perform an unguarded DELETE under
those lock preconditions.
In `@crates/api-model/src/expected_machine.rs`:
- Around line 255-265: Ensure ExpectedHostNic::primary has a single normalized
source of truth: make the field private and route reads through
initial_primary_interface(), or normalize it for every role when constructing
snapshots returned by expected_interface_for_mac(). Prevent callers from
observing a raw primary value that conflicts with initial_primary_interface(),
while preserving the existing Host, DpuOs, and DpuBmc semantics.
In `@crates/rpc/src/lib.rs`:
- Around line 580-636: Consolidate the duplicated ExpectedInterfaceRole and
ExpectedInterfaceIpAllocation serde adapters into one declarative macro that
accepts the enum type and canonical-name/alias mappings. Reuse the macro for
both implementations, preserving their numeric handling, optional values,
validation, canonical serialization names, and distinct alias tables.
In `@crates/site-explorer/src/machine_creator.rs`:
- Around line 705-725: Update
crates/site-explorer/src/machine_creator.rs:705-725 and 828-846 to replace each
manual host_nics MAC lookup with
ExpectedMachineData::expected_interface_for_mac, then apply the settled boot
choice only through the helper’s normalized result and existing role.is_host()
behavior; use mac_address at 705-725 and declared_mac at 828-846, ensuring both
prediction paths produce the same expected-interface snapshot.
🪄 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: f4e26a71-1e80-4f47-9a2a-479743628ba6
⛔ 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>
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.Configuration examples
Expected Machine manifests remain JSON today, so these examples use the current
expected_machines.jsoninput rather than TOML. A single machine can mix every supported role and allocation policy:{ "expected_machines": [ { "bmc_mac_address": "02:00:00:00:00:01", "bmc_username": "root", "bmc_password": "default-password", "chassis_serial_number": "SERIAL-1", "host_nics": [ { "mac_address": "02:00:00:00:00:10", "role": "host", "ip_allocation": "dynamic", "primary": true }, { "mac_address": "02:00:00:00:00:20", "role": "dpu_os", "ip_allocation": "fixed", "fixed_ip": "192.0.2.20" }, { "mac_address": "02:00:00:00:00:21", "role": "dpu_bmc", "ip_allocation": "retained" } ] } ] }Dynamicuses the DHCP relay or link segment and keeps a normal lease.Fixedrequiresfixed_ip, which selects its segment by prefix containment.Retainedmust omitfixed_ip; NICo retains the address selected through DHCP.network_segment_typeremains optional and only narrows or verifies the selected segment.Existing host reservations remain valid without either new field:
{ "host_nics": [ { "mac_address": "02:00:00:00:00:30", "fixed_ip": "192.0.2.30" } ] }Omitting
rolemeanshost; omittingip_allocationinfersfixedwhenfixed_ipis present anddynamicotherwise.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 check-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-db admin_segment_snapshot_is_loaded_after_exclusive_lock --lib -- --nocapturecargo test -p carbide-api-db bmc_metadata::tests --libcargo test -p carbide-api-db expected_switch::tests --libcargo test -p carbide-api-db machine_interface::tests --libcargo test -p carbide-api-core expected_interface -- --test-threads=1cargo test -p carbide-api-core tests::expected_machine --libcargo test -p carbide-api-core test_replace_all_expected_switches --lib -- --nocapturecargo 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.