feat(api): make managed boot-interface updates declarative - #4434
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
Summary by CodeRabbit
WalkthroughThe PR moves managed-host boot-interface operations to transactional desired state and machine-controller reconciliation. It adds reconciliation status to RPC, CLI, and web responses, introduces ChangesBoot-interface reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant API
participant Database
participant MachineController
participant Redfish
CLI->>API: submit primary interface and force_reconcile
API->>Database: commit primary row and desired target
API->>MachineController: enqueue reconciliation
MachineController->>Redfish: apply boot-interface target
MachineController-->>API: report observation and reconciliation state
API-->>CLI: return reconciliation status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/rpc/build.rsast-grep timed out on this file Comment |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4434.docs.buildwithfern.com/infra-controller |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-31 15:01:41 UTC | Commit: ef586b9 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-core/src/handlers/bmc_endpoint_explorer.rs (1)
647-676: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated managed-host commit sequence.
This managed branch and the one in
machine_setup(Lines 579-595) are near-identical: resolve the target, convert withMachineBootInterfaceTarget::from, callforce_set, commit, then enqueue reconciliation. Only the log message and the response type differ.A future change to this sequence must be applied twice. A missed site would silently diverge the two admin actions. Extract one private helper that performs the resolve-persist-commit-enqueue steps and returns whether it handled the request. Each caller then keeps only its own log message and response construction.
🤖 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/bmc_endpoint_explorer.rs` around lines 647 - 676, Extract the duplicated managed-host sequence from the BMC endpoint handler and the managed branch in machine_setup into one private helper. Have the helper resolve the target, build MachineBootInterfaceTarget, persist with force_set, commit the transaction, enqueue reconciliation, and return whether it handled the request. Keep each caller’s existing log message and response construction outside the helper, using the boolean result to control them.crates/admin-cli/src/managed_host/tests.rs (1)
251-253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive cases for the new reconciliation controls.
These assertions verify only false defaults. They do not verify that
--force-reconcileparses as true or that--rebootremains accepted and maps toforce_reconcilein the request conversions. Add table-driven cases for the default, explicit--force-reconcile, and legacy--rebootpaths.As per coding guidelines, use table-driven tests when multiple cases call the same operation with different inputs. Based on learnings, the
admin-clicrate is not included in the CI test job, so explicit coverage is important.Also applies to: 272-274
🤖 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/managed_host/tests.rs` around lines 251 - 253, Add table-driven coverage around the managed-host argument parsing tests, using cases for defaults, explicit --force-reconcile, and legacy --reboot. Assert the resulting force_reconcile and reboot values, and verify request conversion maps --reboot to force_reconcile while retaining legacy acceptance.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 51-54: Update the --reboot help text in
crates/admin-cli/src/boot_interface/set/args.rs lines 51-54,
crates/admin-cli/src/managed_host/set_primary_dpu/args.rs lines 45-49, and
crates/admin-cli/src/managed_host/set_primary_interface/args.rs lines 46-50 to
use version-neutral wording: describe it as a deprecated compatibility alias and
recommend --force-reconcile for current servers. Keep the request conversion
behavior unchanged.
---
Nitpick comments:
In `@crates/admin-cli/src/managed_host/tests.rs`:
- Around line 251-253: Add table-driven coverage around the managed-host
argument parsing tests, using cases for defaults, explicit --force-reconcile,
and legacy --reboot. Assert the resulting force_reconcile and reboot values, and
verify request conversion maps --reboot to force_reconcile while retaining
legacy acceptance.
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 647-676: Extract the duplicated managed-host sequence from the BMC
endpoint handler and the managed branch in machine_setup into one private
helper. Have the helper resolve the target, build MachineBootInterfaceTarget,
persist with force_set, commit the transaction, enqueue reconciliation, and
return whether it handled the request. Keep each caller’s existing log message
and response construction outside the helper, using the boolean result to
control them.
🪄 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: ab0d60ea-b4c3-4a67-9f13-5abd34d1d2f2
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/api-core/src/handlers/machine_boot_interfaces.rs (1)
67-79: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the ineffective search flags.
db::machine::finddoes not useinclude_dpusorinclude_predicted_host, so these fields do not change this query. UseMachineSearchConfig::default()here.🤖 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_boot_interfaces.rs` around lines 67 - 79, Update the MachineSearchConfig passed to db::machine::find_one in the machine snapshot query to use MachineSearchConfig::default() directly, removing the ineffective include_dpus and include_predicted_host overrides.crates/api-core/src/tests/set_primary_interface.rs (1)
227-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that a forced no-change request still enqueues the controller.
This block proves that
force_reconcile: trueadvances the desired version and writes no Redfish. It does not prove that the controller is woken. That matters here: the handler skips every interface write when the primary is unchanged, so the enqueue is the only mechanism that turns the forced generation into real convergence. A regression that dropped the wakeup on this path would leave the new generation pending forever and would not fail any current test.Add a queue assertion around the forced call, in the style already used at lines 422-432.
♻️ Proposed additional assertion
let timepoint = env.redfish_sim.timepoint(); + sqlx::query("DELETE FROM machine_state_controller_queued_objects WHERE object_id = $1") + .bind(host_id.to_string()) + .execute(&env.pool) + .await?; env.api .set_primary_interface(tonic::Request::new(forge::SetPrimaryInterfaceRequest { host_machine_id: Some(host_id), interface_id: Some(promote_id), force_reconcile: true, ..Default::default() })) .await?; let forced = db::machine_desired_boot_interface::get(&env.pool, &host_id) .await? .expect("the forced request should retain the desired target"); assert_eq!(forced.value, promote_target); assert_eq!(forced.version, desired.version.increment()); + let forced_is_queued: bool = sqlx::query_scalar( + "SELECT EXISTS ( + SELECT 1 + FROM machine_state_controller_queued_objects + WHERE object_id = $1 + )", + ) + .bind(host_id.to_string()) + .fetch_one(&env.pool) + .await?; + assert!( + forced_is_queued, + "a forced generation must still wake the controller", + ); assert!( env.redfish_sim .actions_since(&timepoint) .all_hosts() .is_empty(), "force_reconcile should schedule controller work, not write Redfish directly", );🤖 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/set_primary_interface.rs` around lines 227 - 247, Extend the forced no-change request test around set_primary_interface to capture the controller queue state before the call and assert afterward that a controller work item was enqueued, following the existing assertion pattern used near lines 422–432. Keep the current desired-version and Redfish-action assertions unchanged, and ensure the assertion verifies the wakeup specifically for the force_reconcile path.crates/rpc/proto/forge.proto (1)
9499-9508: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReplace
machine_statewith a typed message in both mirroredReconciliationdefinitions.ManagedHostState::to_string()produces composite values such asBootConfiguring/FailedandAssigned/Ready, so a single enum cannot represent the full state. Preserve field number7and update both proto files together.🤖 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/proto/forge.proto` around lines 9499 - 9508, Replace the string field machine_state with a typed message representing the composite ManagedHostState values, preserving field number 7 and defining the message with the required state components. Apply the same change to both mirrored Reconciliation definitions in crates/rpc/proto/forge.proto (lines 9499-9508) and rest-api/proto/core/src/v1/nico_nico.proto (lines 9517-9526), keeping the schemas synchronized.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 236-252: Move the boot_interface_reconciliation_eligible check out
of the unconditional flow and into each if let Some((machine_id,
boot_interface)) = managed_boot_interface_target(...) branch. Only invoke it
after a managed boot-interface target exists, preserving the direct-Redfish
fallback for host-type machines without a managed_host snapshot and avoiding
snapshot lookup errors for unmanaged hosts.
---
Nitpick comments:
In `@crates/api-core/src/handlers/machine_boot_interfaces.rs`:
- Around line 67-79: Update the MachineSearchConfig passed to
db::machine::find_one in the machine snapshot query to use
MachineSearchConfig::default() directly, removing the ineffective include_dpus
and include_predicted_host overrides.
In `@crates/api-core/src/tests/set_primary_interface.rs`:
- Around line 227-247: Extend the forced no-change request test around
set_primary_interface to capture the controller queue state before the call and
assert afterward that a controller work item was enqueued, following the
existing assertion pattern used near lines 422–432. Keep the current
desired-version and Redfish-action assertions unchanged, and ensure the
assertion verifies the wakeup specifically for the force_reconcile path.
In `@crates/rpc/proto/forge.proto`:
- Around line 9499-9508: Replace the string field machine_state with a typed
message representing the composite ManagedHostState values, preserving field
number 7 and defining the message with the required state components. Apply the
same change to both mirrored Reconciliation definitions in
crates/rpc/proto/forge.proto (lines 9499-9508) and
rest-api/proto/core/src/v1/nico_nico.proto (lines 9517-9526), keeping the
schemas synchronized.
🪄 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: dbf8ac22-dc00-4a2e-bd03-d6ca4f16cdad
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-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 (5)
crates/admin-cli/src/managed_host/tests.rs (2)
243-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a distinct DPU identifier for the DPU scenarios.
The DPU rows pass
TEST_MACHINE_IDas both the host argument and thetargetargument, soargvrepeats the same machine id twice. The test still parses, but the positional roles become indistinguishable to a reader. A separateTEST_DPU_IDconstant makes each position self-describing. Consider also a scenario that supplies--force-reconcileand--reboottogether, sincehandle_setcombines them withargs.force_reconcile || args.reboot.Also applies to: 271-281
🤖 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/managed_host/tests.rs` around lines 243 - 244, Update the DPU scenarios in the managed-host argument construction to use a distinct TEST_DPU_ID for the target positional argument instead of TEST_MACHINE_ID, while preserving TEST_MACHINE_ID as the host argument. Add a DPU scenario covering both --force-reconcile and --reboot together to exercise the combined handle_set condition.
269-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the clap error text instead of discarding it.
.map_err(drop)reduces every parse failure toErr(()). All six scenarios expect success, so a future regression in the argument definitions reports an empty error and gives no diagnostic. Map the error to its message so the failure output names the offending argument. This matters here because theadmin-clicrate is not part of the CI test job, so local runs are the only signal.♻️ Proposed change
- .map_err(drop) + .map_err(|error| error.to_string())Based on learnings: the
admin-clicrate is not included in the CI test job, so mismatched assertions and lost diagnostics can slip through without CI coverage.🤖 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/managed_host/tests.rs` around lines 269 - 270, Update the argument parsing error handling in the test setup around the .map_err(drop) call to preserve the clap error message instead of converting failures to Err(()). Map each parse error into a diagnostic string or equivalent message so assertion failures identify the offending argument while retaining the existing success expectations for all six scenarios.Source: Learnings
crates/api-core/src/tests/boot_interface_resolution.rs (3)
186-223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the Ready-host-with-instance branch.
The handler gates enqueueing on two conditions:
matches!(machine.current_state(), ManagedHostState::Ready) && instance.is_none()(seecrates/api-core/src/handlers/managed_host.rs). This test only exercises the state half by moving the machine toAssigned. A host that stays inReadywhile an instance row exists still must not be enqueued, and that branch is currently unverified. Consider a second scenario that attaches an instance and leaves the state atReady.🤖 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/boot_interface_resolution.rs` around lines 186 - 223, The test currently covers only the non-Ready state; add a second scenario in the boot-order test that keeps the machine in ManagedHostState::Ready while creating or attaching an instance row for the host. Invoke set_dpu_first_boot_order and assert no Redfish actions occur and machine_state_controller_queued_objects remains empty, covering the instance.is_some() branch of the handler condition.Source: Path instructions
351-354: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAssert the full desired target, not only the MAC.
The comment above this test states that the
HostInbandrow "resolves an exact managed target", but the assertion checks onlymac_address(). The interface row read at Lines 324-334 also carriesboot_interface_id, so the test can build aMachineBootInterfaceTargetand compare it in full, matching the other tests in this file.🤖 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/boot_interface_resolution.rs` around lines 351 - 354, Update the assertion in the zero-DPU request test to compare the complete desired boot target, not just desired.value.mac_address(). Build the expected MachineBootInterfaceTarget using the resolved in-band interface data, including boot_interface_id, and compare it with desired.value while preserving the existing persistence check.
113-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the controller-queue SQL into local test helpers.
The literal table name
machine_state_controller_queued_objectsand the same two statements appear in several tests in this file (clear at Lines 113-116 and 193-196, count at Lines 132-137 and 214-219). Two small helpers remove the duplication and keep a single place to update if the table or the object-id encoding changes.♻️ Proposed helpers
async fn clear_controller_queue( pool: &sqlx::PgPool, machine_id: &MachineId, ) -> Result<(), sqlx::Error> { sqlx::query("DELETE FROM machine_state_controller_queued_objects WHERE object_id = $1") .bind(machine_id.to_string()) .execute(pool) .await?; Ok(()) } async fn queued_count(pool: &sqlx::PgPool, machine_id: &MachineId) -> Result<i64, sqlx::Error> { sqlx::query_scalar( "SELECT count(*) FROM machine_state_controller_queued_objects WHERE object_id = $1", ) .bind(machine_id.to_string()) .fetch_one(pool) .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/boot_interface_resolution.rs` around lines 113 - 116, Extract the repeated controller-queue SQL into local async test helpers, such as clear_controller_queue and queued_count, accepting the pool and MachineId and preserving the existing Result types and object-ID encoding. Replace both DELETE statements and both count queries in the tests with these helpers, leaving other test 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/admin-cli/src/managed_host/tests.rs`:
- Around line 243-244: Update the DPU scenarios in the managed-host argument
construction to use a distinct TEST_DPU_ID for the target positional argument
instead of TEST_MACHINE_ID, while preserving TEST_MACHINE_ID as the host
argument. Add a DPU scenario covering both --force-reconcile and --reboot
together to exercise the combined handle_set condition.
- Around line 269-270: Update the argument parsing error handling in the test
setup around the .map_err(drop) call to preserve the clap error message instead
of converting failures to Err(()). Map each parse error into a diagnostic string
or equivalent message so assertion failures identify the offending argument
while retaining the existing success expectations for all six scenarios.
In `@crates/api-core/src/tests/boot_interface_resolution.rs`:
- Around line 186-223: The test currently covers only the non-Ready state; add a
second scenario in the boot-order test that keeps the machine in
ManagedHostState::Ready while creating or attaching an instance row for the
host. Invoke set_dpu_first_boot_order and assert no Redfish actions occur and
machine_state_controller_queued_objects remains empty, covering the
instance.is_some() branch of the handler condition.
- Around line 351-354: Update the assertion in the zero-DPU request test to
compare the complete desired boot target, not just desired.value.mac_address().
Build the expected MachineBootInterfaceTarget using the resolved in-band
interface data, including boot_interface_id, and compare it with desired.value
while preserving the existing persistence check.
- Around line 113-116: Extract the repeated controller-queue SQL into local
async test helpers, such as clear_controller_queue and queued_count, accepting
the pool and MachineId and preserving the existing Result types and object-ID
encoding. Replace both DELETE statements and both count queries in the tests
with these helpers, leaving other test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 320ca1c3-5cea-4405-bf77-55a00f59796f
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (25)
- crates/admin-cli/src/boot_interface/candidates/cmd.rs
- crates/admin-cli/src/boot_interface/mod.rs
- crates/api-web/templates/machine_detail.html
- docs/observability/core_metrics.md
- crates/rpc/build.rs
- crates/api-web/src/tests/managed_host.rs
- crates/admin-cli/src/managed_host/set_primary_dpu/args.rs
- crates/admin-cli/src/boot_interface/set/cmd.rs
- crates/api-core/src/tests/set_primary_dpu.rs
- crates/api-web/src/machine.rs
- crates/api-db/src/machine_desired_boot_interface.rs
- crates/admin-cli/src/managed_host/set_primary_interface/args.rs
- crates/api-core/tests/integration/machine_boot_interfaces.rs
- crates/api-web/src/explored_endpoint.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-db/src/machine_interface/tests.rs
- crates/api-core/src/handlers/utils.rs
- crates/admin-cli/src/boot_interface/set/args.rs
- crates/api-core/src/handlers/machine_boot_interfaces.rs
- crates/api-db/src/machine_interface.rs
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/api-core/src/tests/set_primary_interface.rs
- crates/rpc/proto/forge.proto
- crates/admin-cli/src/boot_interface/show/cmd.rs
- crates/api-core/src/handlers/managed_host.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/admin-cli/src/boot_interface/set/cmd.rs (1)
43-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
force_reconcilecomputation.
force_reconcile: args.force_reconcile || args.rebootis the only place that decides whether reboot alone triggers reconciliation. No test in this file exerciseshandle_set's request construction for the four combinations of--rebootand--force-reconcile.admin-cliis not part of the CI test job, so a regression here (for example, swapping||for&&) would not be caught automatically.Extract the request-building logic into a small pure function so it can be unit tested without a live
ApiClient.♻️ Suggested refactor to make the logic testable
- #[allow(deprecated)] // Keep `--reboot` functional when this CLI calls an older server. - let request = forgerpc::SetPrimaryInterfaceRequest { - host_machine_id: Some(args.machine), - interface_id: Some(interface_id), - reboot: args.reboot, - force_reconcile: args.force_reconcile || args.reboot, - }; + let request = build_set_primary_interface_request(args.machine, interface_id, args.reboot, args.force_reconcile); api_client.0.set_primary_interface(request).await?; Ok(()) } + +#[allow(deprecated)] // Keep `--reboot` functional when this CLI calls an older server. +fn build_set_primary_interface_request( + machine: MachineId, + interface_id: MachineInterfaceId, + reboot: bool, + force_reconcile: bool, +) -> forgerpc::SetPrimaryInterfaceRequest { + forgerpc::SetPrimaryInterfaceRequest { + host_machine_id: Some(machine), + interface_id: Some(interface_id), + reboot, + force_reconcile: force_reconcile || reboot, + } +}Based on learnings, "Because the admin-cli crate is not included in the CI test job for this repo, CI will not automatically catch regressions originating in crates/admin-cli/**." and "be extra diligent with test and assertion updates."
🤖 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/boot_interface/set/cmd.rs` around lines 43 - 50, Extract the request construction currently inside handle_set into a small pure helper that accepts the relevant arguments and returns SetPrimaryInterfaceRequest, preserving force_reconcile as args.force_reconcile || args.reboot. Add unit tests covering all four reboot/force-reconcile combinations, asserting the resulting request fields, and have handle_set call the helper before invoking set_primary_interface.Source: Learnings
crates/api-core/src/handlers/managed_host.rs (1)
69-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the two
#[allow(deprecated)]attributes. Both sites suppress the deprecation warning for readingrequest.reboot, and neither states why. The repository style guide asks for a strong justification on every#[allow(...)]. The test atcrates/api-core/src/tests/set_primary_interface.rsLine 131 already carries such a comment, so the production sites are the inconsistent ones. Without a note, the attribute is likely to outlive the deprecated field and hide a future genuine warning.
crates/api-core/src/handlers/managed_host.rs#L69-L70: add a one-line comment stating thatrebootis read only as a compatibility alias forforce_reconcile.crates/api-core/src/handlers/managed_host.rs#L99-L100: add the same comment.As per coding guidelines: "Enable all Clippy lints, treat warnings as errors, and avoid `#[allow(...)]` unless there is a strong justification."♻️ Proposed change
- #[allow(deprecated)] + // `reboot` is read only as the deprecated compatibility alias for + // `force_reconcile`. Remove this allow with the proto field. + #[allow(deprecated)] let force_reconcile = request.force_reconcile || request.reboot;🤖 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/managed_host.rs` around lines 69 - 70, Document both #[allow(deprecated)] attributes in crates/api-core/src/handlers/managed_host.rs at lines 69-70 and 99-100 with the same one-line comment stating that request.reboot is read only as a compatibility alias for force_reconcile; make no other changes.Source: Coding guidelines
crates/api-core/src/handlers/bmc_endpoint_explorer.rs (1)
583-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the intentional divergence between the two handlers explicit.
set_dpu_first_boot_orderresolves its managed target throughmanaged_boot_interface_target, which rejects an unresolvable target withInvalidArgument.machine_setupinstead inlinesfilter(has_managed_boot_target).zip(...), so an unresolvable target silently falls through to the direct-Redfish path. Both behaviours are covered by tests and are deliberate, but the inlined form duplicates the ownership predicate and does not state why it tolerates a missing target. A future change could easily unify the two shapes and break the target-less BIOS-setup path.Extract the shared ownership check and record the intent at this call site.
♻️ Suggested shape
+ // `machine_setup` tolerates a missing target: a managed host whose candidate + // data cannot name a boot NIC still needs the target-less BIOS setup pass. + // `set_dpu_first_boot_order` rejects that case instead, because a boot-order + // change without a target is meaningless. let managed_target = owning_machine_id .filter(has_managed_boot_target) .zip(resolve_admin_boot_interface_target( None, desired.as_ref(), candidates.as_ref(), entered_mac, ));🤖 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/bmc_endpoint_explorer.rs` around lines 583 - 591, In the machine_setup handler, extract the has_managed_boot_target ownership check from the inline filter/zip expression into a clearly named local or helper, then reuse it when resolving the managed target. Add a concise comment at this call site documenting that, unlike set_dpu_first_boot_order and managed_boot_interface_target, an unresolvable managed target intentionally falls through to the direct-Redfish path.crates/api-core/src/tests/boot_interface_resolution.rs (1)
132-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a test-support accessor for the controller queue. Nine sites across three test files hand-write SQL against
machine_state_controller_queued_objectsand encode the machine id withhost_id.to_string(). The shared root cause is a missing test-support helper, so every test now depends on an internal queue table name and on the id's string encoding. A rename of that table or a change to the key type breaks all three files at once.Add one helper pair to the shared test environment, for example
env.queued_object_count(machine_id)andenv.clear_queued_objects(machine_id), then replace the inline SQL.
crates/api-core/src/tests/boot_interface_resolution.rs#L132-L141: replace the queued-count query with the helper; apply the same change to the reset at Lines 113-116 and to the sites at Lines 193-196 and 214-219.crates/api-core/src/tests/set_primary_dpu.rs#L211-L220: replace theEXISTSquery with the helper; apply the same change to the reset at Lines 173-176.crates/api-core/src/tests/set_primary_interface.rs#L428-L437: replace theEXISTSquery with the helper; apply the same change to the sites at Lines 415-418, 458-461, and 480-489.🤖 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/boot_interface_resolution.rs` around lines 132 - 141, Extract shared test-environment helpers for queued-object counting and clearing, such as queued_object_count and clear_queued_objects, so tests no longer access the internal queue table or encode IDs directly. Update crates/api-core/src/tests/boot_interface_resolution.rs at lines 132-141, 113-116, 193-196, and 214-219; crates/api-core/src/tests/set_primary_dpu.rs at lines 211-220 and 173-176; and crates/api-core/src/tests/set_primary_interface.rs at lines 428-437, 415-418, 458-461, and 480-489 to use the helpers, preserving each test’s existing assertions and reset 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/rpc/build.rs`:
- Around line 346-349: Remove the nested type_attribute targeting
forge.GetMachineBootInterfacesResponse.Reconciliation from the build
configuration, leaving the parent rule as the sole source of its
serde::Serialize derive.
---
Nitpick comments:
In `@crates/admin-cli/src/boot_interface/set/cmd.rs`:
- Around line 43-50: Extract the request construction currently inside
handle_set into a small pure helper that accepts the relevant arguments and
returns SetPrimaryInterfaceRequest, preserving force_reconcile as
args.force_reconcile || args.reboot. Add unit tests covering all four
reboot/force-reconcile combinations, asserting the resulting request fields, and
have handle_set call the helper before invoking set_primary_interface.
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 583-591: In the machine_setup handler, extract the
has_managed_boot_target ownership check from the inline filter/zip expression
into a clearly named local or helper, then reuse it when resolving the managed
target. Add a concise comment at this call site documenting that, unlike
set_dpu_first_boot_order and managed_boot_interface_target, an unresolvable
managed target intentionally falls through to the direct-Redfish path.
In `@crates/api-core/src/handlers/managed_host.rs`:
- Around line 69-70: Document both #[allow(deprecated)] attributes in
crates/api-core/src/handlers/managed_host.rs at lines 69-70 and 99-100 with the
same one-line comment stating that request.reboot is read only as a
compatibility alias for force_reconcile; make no other changes.
In `@crates/api-core/src/tests/boot_interface_resolution.rs`:
- Around line 132-141: Extract shared test-environment helpers for queued-object
counting and clearing, such as queued_object_count and clear_queued_objects, so
tests no longer access the internal queue table or encode IDs directly. Update
crates/api-core/src/tests/boot_interface_resolution.rs at lines 132-141,
113-116, 193-196, and 214-219; crates/api-core/src/tests/set_primary_dpu.rs at
lines 211-220 and 173-176; and
crates/api-core/src/tests/set_primary_interface.rs at lines 428-437, 415-418,
458-461, and 480-489 to use the helpers, preserving each test’s existing
assertions and reset 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: ece8d194-862b-4be2-ac2b-71df8ebf3d16
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
|
CodeRabbit review disposition for 8552217: Addressed:
Declined after verification:
Verification: nightly format, full workspace Clippy, custom carbide lints, generated Go tests, CLI parsing/reporting tests, web rendering, reconciliation unit tests, endpoint resolver tests, and the focused SQLx behavior regressions all pass. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/api-db/src/machine_interface/tests.rs (1)
187-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the BMC writer with a
lock_timeoutso a regression fails fast.This step proves that
find_by_machine_id_for_updateleaves BMC rows unlocked. The host writer below setsSET LOCAL lock_timeout = '100ms', but this transaction does not. If theinterface_type != 'Bmc'filter ever regresses, thisUPDATEblocks on the lock held bylock_txnand the test hangs until the harness kills it, instead of reporting a clear failure.Set the same timeout here and let the successful update carry the meaning.
♻️ Proposed change to bound the BMC writer
let mut bmc_writer = pool.begin().await?; + sqlx::query("SET LOCAL lock_timeout = '100ms'") + .execute(bmc_writer.as_mut()) + .await?; sqlx::query("UPDATE machine_interfaces SET hostname = hostname WHERE id = $1") .bind(bmc_interface_id) .execute(bmc_writer.as_mut()) - .await?; + .await + .expect("a BMC interface row must not be locked by find_by_machine_id_for_update"); bmc_writer.commit().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 187 - 192, Set the BMC transaction’s local lock timeout immediately after pool.begin() in the test transaction before the UPDATE, matching the host writer’s 100ms timeout. Keep the existing successful UPDATE and commit flow so the bounded transaction verifies BMC rows remain unlocked.crates/rpc/proto/forge.proto (1)
9500-9506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a typed
machine_statefield, and keep both protos aligned. Both files declaremachine_stateas astringnext to the correctly typedStateenum. The producer derives the value from the RustManagedHostStateenum, so the value set is finite. This is a consistency judgement, not a defect; the surrounding response message already uses strings for other descriptive fields.
crates/rpc/proto/forge.proto#L9500-L9506: decide between a typed enum and the currentstring. Keep thestringif the field is intended purely for operator display.rest-api/proto/core/src/v1/nico_nico.proto#L9517-L9524: apply the identical declaration so the mirror does not drift.As per path instructions for
crates/rpc/proto/**: "using typed fields/enums for reconciliation state rather than stringly-typed values".🤖 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/proto/forge.proto` around lines 9500 - 9506, Replace the stringly-typed machine_state declarations with the appropriate ManagedHostState-derived enum type in both crates/rpc/proto/forge.proto lines 9500-9506 and rest-api/proto/core/src/v1/nico_nico.proto lines 9517-9524, keeping the field number and both mirrored declarations aligned.Source: Path instructions
crates/api-core/src/handlers/machine_boot_interfaces.rs (1)
231-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that
failurecan reference a superseded generation.
failurederives fromactive_reconciliation, which is populated whenevermachine_stateisBootConfiguring, regardless of whetherreconciling_versionmatchesdesired_boot_interface.version. Meanwhilereconciliation_state(viaboot_interface_reconciliation_state) only reportsFailedwhen the active work matches the current desired generation. As a result, a response can reportreconciliation_state: Pendingtogether with a non-emptyfailurethat actually belongs to a stale, already-superseded generation.The docstring at Lines 234-236 states this is intentional, and the test
reconciliation_status_keeps_stale_observation_and_active_failure_detailsconfirms it. The tradeoff is reasonable for troubleshooting, butcrates/api-web/templates/machine_detail.html(Lines 74-91) rendersFailureunconditionally whenever it isSome, without indicating that it may not belong to the current desired version. An operator scanning the page could read "State: Pending" plus a visible "Failure" row and assume the current attempt is failing.Consider exposing enough information for the display layer to qualify the failure (for example, comparing
reconciling_versiontodesired_versionin the template, or adding a short note such as "(previous generation)" when they differ).{% if let Some(failure) = reconciliation.failure %} <tr><th>Failure</th><td class="cell-error"> {{ failure }} {% if reconciliation.reconciling_version != reconciliation.desired_version %} <br><small>(reported for a superseded generation: {{ reconciliation.reconciling_version }})</small> {% endif %} </td></tr> {% endif %}🤖 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_boot_interfaces.rs` around lines 231 - 271, Update the machine detail template’s reconciliation failure display to distinguish failures from superseded generations. When rendering reconciliation.failure, compare reconciling_version with desired_version and add a clear previous-generation qualifier, including the superseded version when available; keep the existing failure output unchanged for the current generation.crates/api-core/src/tests/boot_interface_resolution.rs (1)
100-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPromote the controller-queue helpers to shared test support. The table name
machine_state_controller_queued_objectsand its two access patterns (delete pending work, assert pending work) are now hard-coded across three test files. A schema rename would require edits in each of them, and the assertions already diverge betweencount(*)andEXISTS.
crates/api-core/src/tests/boot_interface_resolution.rs#L100-L123: moveclear_controller_queueandcontroller_queue_countinto the sharedapi_fixtures/test_supportmodule and re-export them, so the raw SQL lives in one place.crates/api-core/src/tests/set_primary_interface.rs#L227-L230: replace the inlinedDELETEand each inlinedEXISTSquery (also at Lines 248-257, 433-436, 446-455, 476-479, 498-507) with the shared helpers.crates/api-core/src/tests/set_primary_dpu.rs#L173-L176: replace the inlinedDELETEand theEXISTSquery at Lines 211-220 with the shared helpers.🤖 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/boot_interface_resolution.rs` around lines 100 - 123, Promote clear_controller_queue and controller_queue_count from boot_interface_resolution.rs into the shared api_fixtures/test_support module and re-export them. In crates/api-core/src/tests/set_primary_interface.rs at lines 227-230, 248-257, 433-436, 446-455, 476-479, and 498-507, replace the inline DELETE and EXISTS queries with these helpers; make the same replacement in crates/api-core/src/tests/set_primary_dpu.rs at lines 173-176 and 211-220. Keep all raw SQL for machine_state_controller_queued_objects centralized in the shared helpers.
🤖 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/tests/set_primary_dpu.rs`:
- Around line 163-169: Update the comment above the SQL mutation to explain that
the stale interface is reassigned to surviving_dpu_id, keeping the host
DPU-backed while stale_dpu_id loses all interfaces; explicitly preserve this
shape for the Admin-segment guard and discourage replacing it with NULL.
---
Nitpick comments:
In `@crates/api-core/src/handlers/machine_boot_interfaces.rs`:
- Around line 231-271: Update the machine detail template’s reconciliation
failure display to distinguish failures from superseded generations. When
rendering reconciliation.failure, compare reconciling_version with
desired_version and add a clear previous-generation qualifier, including the
superseded version when available; keep the existing failure output unchanged
for the current generation.
In `@crates/api-core/src/tests/boot_interface_resolution.rs`:
- Around line 100-123: Promote clear_controller_queue and controller_queue_count
from boot_interface_resolution.rs into the shared api_fixtures/test_support
module and re-export them. In crates/api-core/src/tests/set_primary_interface.rs
at lines 227-230, 248-257, 433-436, 446-455, 476-479, and 498-507, replace the
inline DELETE and EXISTS queries with these helpers; make the same replacement
in crates/api-core/src/tests/set_primary_dpu.rs at lines 173-176 and 211-220.
Keep all raw SQL for machine_state_controller_queued_objects centralized in the
shared helpers.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 187-192: Set the BMC transaction’s local lock timeout immediately
after pool.begin() in the test transaction before the UPDATE, matching the host
writer’s 100ms timeout. Keep the existing successful UPDATE and commit flow so
the bounded transaction verifies BMC rows remain unlocked.
In `@crates/rpc/proto/forge.proto`:
- Around line 9500-9506: Replace the stringly-typed machine_state declarations
with the appropriate ManagedHostState-derived enum type in both
crates/rpc/proto/forge.proto lines 9500-9506 and
rest-api/proto/core/src/v1/nico_nico.proto lines 9517-9524, keeping the field
number and both mirrored declarations aligned.
🪄 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: b0d2f887-399e-4792-a7e2-3e0a75b6652c
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-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 (3)
crates/admin-cli/src/boot_interface/show/cmd.rs (1)
356-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
.ok()when discarding formatting results.Replace the new
let _ = writeln!(...)calls withwriteln!(...).ok(). This follows the repository convention for intentionally discarded fallible results.As per coding guidelines, “Discard fallible results with
.ok()rather than assigning them to an unused variable.”🤖 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/boot_interface/show/cmd.rs` around lines 356 - 377, Update the new output-writing calls in the boot-interface display block to discard each writeln! result with .ok() instead of binding it to _. Preserve the existing messages, formatting, and output order.Source: Coding guidelines
crates/api-core/src/handlers/managed_host.rs (1)
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the reconciliation-eligibility predicate.
This handler computes eligibility as
Readystate plus no attached instance.crates/api-core/src/handlers/bmc_endpoint_explorer.rslines 236-256 encodes the same rule inboot_interface_reconciliation_eligible. Two copies of one safety gate can diverge when the lifecycle rule changes.A shared helper next to
enqueue_boot_interface_reconciliationincrates/api-core/src/handlers/utils.rswould keep the rule in one place. This handler can pass its already-loadedmachinestate and instance lookup result, so the helper does not need to re-read the row.🤖 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/managed_host.rs` around lines 231 - 233, Centralize the reconciliation eligibility rule in a shared helper near enqueue_boot_interface_reconciliation in utils.rs, accepting the already-loaded machine state and instance lookup result. Update managed_host.rs and bmc_endpoint_explorer.rs to call this helper instead of duplicating the Ready-state and no-instance predicate, preserving the existing eligibility behavior.crates/api-core/src/tests/boot_interface_resolution.rs (1)
101-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the controller-queue test helpers instead of repeating the SQL. The same
machine_state_controller_queued_objectsclear-and-assert scaffolding appears in three files in three different shapes: typed helpers in one file, and inlineDELETEplusSELECT EXISTSstatements in the other two. When the queue table or itsobject_idencoding changes, every copy must be updated.
crates/api-core/src/tests/boot_interface_resolution.rs#L101-L123: moveclear_controller_queueandcontroller_queue_countinto the shared test-support module used by these tests, and import them here.crates/api-core/src/tests/set_primary_dpu.rs#L175-L226: replace the inlineDELETEandSELECT EXISTSstatements with the shared helpers.crates/api-core/src/tests/set_primary_interface.rs#L227-L261: replace the inlineDELETEandSELECT EXISTSstatements with the shared helpers, and apply the same change to the repeated blocks at lines 446-455 and 498-507.🤖 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/boot_interface_resolution.rs` around lines 101 - 123, Centralize the controller-queue SQL helpers: move clear_controller_queue and controller_queue_count from crates/api-core/src/tests/boot_interface_resolution.rs:101-123 into the shared test-support module, then import and use them in crates/api-core/src/tests/boot_interface_resolution.rs:101-123. Replace the inline DELETE and SELECT EXISTS logic with these helpers in crates/api-core/src/tests/set_primary_dpu.rs:175-226 and crates/api-core/src/tests/set_primary_interface.rs:227-261, including the repeated blocks at lines 446-455 and 498-507 in the latter file.
🤖 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/admin-cli/src/boot_interface/show/cmd.rs`:
- Around line 356-377: Update the new output-writing calls in the boot-interface
display block to discard each writeln! result with .ok() instead of binding it
to _. Preserve the existing messages, formatting, and output order.
In `@crates/api-core/src/handlers/managed_host.rs`:
- Around line 231-233: Centralize the reconciliation eligibility rule in a
shared helper near enqueue_boot_interface_reconciliation in utils.rs, accepting
the already-loaded machine state and instance lookup result. Update
managed_host.rs and bmc_endpoint_explorer.rs to call this helper instead of
duplicating the Ready-state and no-instance predicate, preserving the existing
eligibility behavior.
In `@crates/api-core/src/tests/boot_interface_resolution.rs`:
- Around line 101-123: Centralize the controller-queue SQL helpers: move
clear_controller_queue and controller_queue_count from
crates/api-core/src/tests/boot_interface_resolution.rs:101-123 into the shared
test-support module, then import and use them in
crates/api-core/src/tests/boot_interface_resolution.rs:101-123. Replace the
inline DELETE and SELECT EXISTS logic with these helpers in
crates/api-core/src/tests/set_primary_dpu.rs:175-226 and
crates/api-core/src/tests/set_primary_interface.rs:227-261, including the
repeated blocks at lines 446-455 and 498-507 in the latter file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d0c1eea-f4db-457b-8a64-6c7b0822a16c
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-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: 4
🧹 Nitpick comments (4)
crates/api-core/src/handlers/utils.rs (1)
135-161: 🩺 Stability & Availability | 🔵 TrivialConfirm an alert exists for the wakeup-failure counter.
The design here is sound. The desired generation is already committed, so swallowing the enqueue error correctly keeps an otherwise successful operator request successful, and the periodic scan converges the machine.
One operational consequence follows from that choice. When the enqueue fails, no caller and no operator sees an error. The only signal is
carbide_state_handler_wakeup_failures_totalwithtrigger="boot_interface_intent". The machine then waits for the next periodic scan, which lengthens the observed reconciliation latency.Please confirm an alert or dashboard panel covers this counter, so a sustained enqueue failure does not stay invisible until an operator notices slow boot-interface convergence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/utils.rs` around lines 135 - 161, Confirm that operational monitoring covers carbide_state_handler_wakeup_failures_total with trigger="boot_interface_intent", including an alert or dashboard panel for sustained enqueue failures. Update the relevant monitoring configuration or documentation rather than changing enqueue_boot_interface_reconciliation’s error-swallowing behavior.crates/api-core/tests/integration/machine_boot_interfaces.rs (1)
175-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected reconciliation state, not merely a non-default one.
This PR introduces four meaningful states:
Pending,Converging,Converged, andFailed.assert_ne!(..., Unspecified)accepts all four. A handler that classifies a freshly ingested, never-reconciled host asFailedwould satisfy this assertion, and that misclassification is exactly the kind of defect this test should catch. Themachine_statenon-empty check has the same limitation.The fixture state is deterministic here.
initbuilds the managed host and the test never runs a machine-controller iteration, so the persisted desired generation has nothing verified against it. The expected classification is thereforePending.Compare
crates/api-core/src/tests/boot_interface_resolution.rsLine 735-738, which pins the exactdesired_versionfor a predicted host. This test can be equally precise.♻️ Proposed tightening
- assert_ne!( - reconciliation.reconciliation_state, - ReconciliationState::Unspecified as i32, - "a desired generation should have a reconciliation classification" - ); - assert!( - !reconciliation.machine_state.is_empty(), - "the managed-host state should explain where reconciliation is waiting" - ); + assert_eq!( + reconciliation.reconciliation_state, + ReconciliationState::Pending as i32, + "an ingested host with an unverified desired generation is Pending" + ); + assert_eq!( + reconciliation.machine_state, + ManagedHostState::Ready.to_string(), + "the managed-host state should explain where reconciliation is waiting" + );If the fixture's progression is genuinely non-deterministic, narrow the assertion to the acceptable subset instead, so
Failedis rejected.🤖 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/tests/integration/machine_boot_interfaces.rs` around lines 175 - 183, Update the assertions in the reconciliation test to require the deterministic expected state, `ReconciliationState::Pending`, rather than merely rejecting `Unspecified`. Also replace the non-empty `machine_state` check with an exact expected value if the fixture defines one; otherwise assert only the documented acceptable subset that excludes `Failed`, preserving the test’s coverage of the initial, never-reconciled host.crates/api-core/src/tests/boot_interface_resolution.rs (2)
101-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one controller-queue test helper across the three test files. The root cause is that the new helper pair is private to one test module, so every other assertion site re-encodes the same three coupled facts: the
machine_state_controller_queued_objectstable name, theobject_idcolumn, and themachine_id.to_string()binding. Six sites now depend on that schema detail. When the queue table is renamed orobject_idchanges type, five of the six sites fail to compile in unrelated files, and any site that is missed silently asserts against a stale contract.Promote the helpers to the shared test-support module used by these tests, then call them everywhere. Prefer returning
boolfrom anis_queuedhelper so the call sites read as intent rather than SQL.
crates/api-core/src/tests/boot_interface_resolution.rs#L101-L123: moveclear_controller_queueandcontroller_queue_countinto the shared test-support module, and add anis_queuedwrapper over the count.crates/api-core/src/tests/set_primary_dpu.rs#L213-L226: replace theSELECT EXISTSstatement with the sharedis_queuedhelper, and replace theDELETEat Line 175-178 withclear_controller_queue.crates/api-core/src/tests/set_primary_interface.rs#L248-L257: replace theSELECT EXISTSstatement with the sharedis_queuedhelper.crates/api-core/src/tests/set_primary_interface.rs#L446-L455: replace theSELECT EXISTSstatement with the sharedis_queuedhelper.crates/api-core/src/tests/set_primary_interface.rs#L498-L507: replace theSELECT EXISTSstatement with the sharedis_queuedhelper, and replace theDELETEstatements at Line 227-230, 433-436, and 476-479 withclear_controller_queue.🤖 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/boot_interface_resolution.rs` around lines 101 - 123, Centralize clear_controller_queue and controller_queue_count in the shared test-support module, add an is_queued wrapper returning bool, and update all listed sites to use these helpers instead of inline SQL. In crates/api-core/src/tests/boot_interface_resolution.rs#L101-123 move the helpers and add is_queued; in crates/api-core/src/tests/set_primary_dpu.rs#L213-226 replace SELECT EXISTS with is_queued and the specified DELETE with clear_controller_queue; in crates/api-core/src/tests/set_primary_interface.rs#L248-257, `#L446-455`, and `#L498-507` replace each SELECT EXISTS with is_queued and all specified DELETE statements with clear_controller_queue.
193-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvery negative enqueue test falsifies only one term of a two-term predicate.
set_primary_interface_coregates enqueueing onmatches!(machine.current_state(), ManagedHostState::Ready) && instance.is_none()(crates/api-core/src/handlers/managed_host.rs, around Line 232). Both negative tests reach the not-enqueued outcome by forcingcontroller_statetoAssignedthrough raw SQL, and neither host owns an instance. Theinstance.is_none()term is therefore never the reason a request is withheld, in any test in this cohort. Removing that term from the handler would keep both tests green while allowing aReadyhost that still serves a tenant instance to be enqueued for boot reconciliation.
crates/api-core/src/tests/boot_interface_resolution.rs#L193-L232: add a case that leavescontroller_stateatReady, attaches an instance to the host, and asserts the desired generation advances while the host stays unqueued.crates/api-core/src/tests/set_primary_interface.rs#L476-L537: keep theAssignedcase, and additionally assert theReady-with-instance path so the two terms are covered independently rather than only in combination.🤖 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/boot_interface_resolution.rs` around lines 193 - 232, Add a Ready-host-with-instance negative enqueue case in crates/api-core/src/tests/boot_interface_resolution.rs lines 193-232, asserting the desired generation advances while the host remains unqueued; retain the existing Assigned case. Extend the related coverage in crates/api-core/src/tests/set_primary_interface.rs lines 476-537 with the same Ready-with-instance assertions, so set_primary_interface_core’s ManagedHostState::Ready and instance.is_none() predicate terms are tested independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/boot_interface/mod.rs`:
- Around line 67-70: Update the command help text in the managed-host
set-primary-interface description to clarify that machine-controller reconciles
the BMC when the host becomes eligible, rather than implying immediate
convergence after the desired commit.
In `@crates/api-core/src/handlers/machine_boot_interfaces.rs`:
- Around line 231-271: Update boot_interface_reconciliation_status so the
failure field is suppressed when boot_interface_reconciliation_state returns
Converged for the desired version. Compute or reuse the reconciliation state
before constructing BootInterfaceReconciliationStatus, and retain failure only
for non-Converged outcomes, including superseded-generation failures.
- Around line 322-480: Add a table-driven case covering a matching observed
version alongside a same-version leftover Failed BootConfiguring state,
asserting the reconciliation status is Converged and does not retain stale
failure details. Extend
reconciliation_status_keeps_stale_observation_and_active_failure_details or add
a focused test, using the existing check_values pattern and symbols such as
boot_interface_reconciliation_status and BootInterfaceReconciliationState.
In `@crates/api-web/src/machine.rs`:
- Around line 524-527: Update the observed_at mapping in the machine page data
construction to use the existing to_time(...) formatter instead of
Timestamp::to_string(). Preserve the "-" fallback for missing timestamps and
keep the resulting display consistent with the other DateTime<Utc> fields.
---
Nitpick comments:
In `@crates/api-core/src/handlers/utils.rs`:
- Around line 135-161: Confirm that operational monitoring covers
carbide_state_handler_wakeup_failures_total with
trigger="boot_interface_intent", including an alert or dashboard panel for
sustained enqueue failures. Update the relevant monitoring configuration or
documentation rather than changing enqueue_boot_interface_reconciliation’s
error-swallowing behavior.
In `@crates/api-core/src/tests/boot_interface_resolution.rs`:
- Around line 101-123: Centralize clear_controller_queue and
controller_queue_count in the shared test-support module, add an is_queued
wrapper returning bool, and update all listed sites to use these helpers instead
of inline SQL. In
crates/api-core/src/tests/boot_interface_resolution.rs#L101-123 move the helpers
and add is_queued; in crates/api-core/src/tests/set_primary_dpu.rs#L213-226
replace SELECT EXISTS with is_queued and the specified DELETE with
clear_controller_queue; in
crates/api-core/src/tests/set_primary_interface.rs#L248-257, `#L446-455`, and
`#L498-507` replace each SELECT EXISTS with is_queued and all specified DELETE
statements with clear_controller_queue.
- Around line 193-232: Add a Ready-host-with-instance negative enqueue case in
crates/api-core/src/tests/boot_interface_resolution.rs lines 193-232, asserting
the desired generation advances while the host remains unqueued; retain the
existing Assigned case. Extend the related coverage in
crates/api-core/src/tests/set_primary_interface.rs lines 476-537 with the same
Ready-with-instance assertions, so set_primary_interface_core’s
ManagedHostState::Ready and instance.is_none() predicate terms are tested
independently.
In `@crates/api-core/tests/integration/machine_boot_interfaces.rs`:
- Around line 175-183: Update the assertions in the reconciliation test to
require the deterministic expected state, `ReconciliationState::Pending`, rather
than merely rejecting `Unspecified`. Also replace the non-empty `machine_state`
check with an exact expected value if the fixture defines one; otherwise assert
only the documented acceptable subset that excludes `Failed`, preserving the
test’s coverage of the initial, never-reconciled host.
🪄 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: 794158b8-f375-45f8-a47c-dd138caca043
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
@coderabbitai Thanks -- I verified the latest review-body findings and am leaving them unchanged. The CLI-doc generator is not wired into pre-commit or CI and, in the current environment, rewrites 581 tracked pages plus unrelated stale commands because the checked-in snapshot and pandoc output have drifted; committing that churn or a partial generated snapshot would be inappropriate here, while the new commands are already covered in the boot-interfaces operator guide. Lowercase machine is the dominant NotFoundError convention across api-core, so changing this new call site to the exceptional uppercase spelling would reduce consistency. machine_state is already documented and exposed as operator-facing status alongside the typed reconciliation enum, so the extra non-parsing warning is optional prose rather than a correctness gap. Finally, Ready-plus-attached-instance coverage is an additional synthetic branch test, not required for the focused Assigned-host authorization behavior in this PR. |
|
Tip For best results, initiate chat on the files or code changes.
The CLI documentation update would create unrelated generated-file churn. The boot-interfaces operator guide covers the new commands. The lowercase
The Ready-plus-attached-instance case is outside this PR’s focused Assigned-host authorization coverage. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rpc/proto/forge.proto (1)
9544-9546: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
machine_stateis stringly-typed in both proto copies. The newReconciliationmessage models a finite managed-host state as astring. The path instructions for core gRPC protobuf definitions require typed enums or messages for finite reconciliation states. Both copies must change together, and changing them after release breaks the wire and JSON contract for CLI, web, and REST consumers.
crates/rpc/proto/forge.proto#L9544-L9546: replacestring machine_state = 7with a typed enum covering the managed-host states, and update the handler that populates it.rest-api/proto/core/src/v1/nico_nico.proto#L9562-L9564: apply the identical change so the mirrored definition stays byte-compatible withforge.proto.As per path instructions, core gRPC protobuf definitions must "use typed enums/messages for finite reconciliation states rather than stringly-typed values".
🤖 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/proto/forge.proto` around lines 9544 - 9546, The Reconciliation message uses a string for the finite machine state; replace machine_state in crates/rpc/proto/forge.proto#L9544-L9546 and rest-api/proto/core/src/v1/nico_nico.proto#L9562-L9564 with the same typed enum covering all managed-host states, keeping both definitions byte-compatible. Update the handler that populates Reconciliation.machine_state to assign the enum values.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/rpc/proto/forge.proto`:
- Around line 9544-9546: The Reconciliation message uses a string for the finite
machine state; replace machine_state in crates/rpc/proto/forge.proto#L9544-L9546
and rest-api/proto/core/src/v1/nico_nico.proto#L9562-L9564 with the same typed
enum covering all managed-host states, keeping both definitions byte-compatible.
Update the handler that populates Reconciliation.machine_state to assign the
enum values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fbebe366-3000-4edf-95cc-71c1f4519a13
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-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/src/machine_desired_boot_interface.rs (1)
420-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider narrowing the pair-preservation step to the forced path.
The logic is correct. The
IfChangedmode returns early at Line 436 wheneverrequest_is_satisfiedholds, so thefilter/map_orat Lines 439-442 can only substitute the stored value inSetMode::Force. A reader must trace both branches to establish that.Scoping the substitution to the forced path makes the invariant local and removes the redundant second
request_is_satisfiedevaluation. This is a readability preference only; keep the current form if you prefer one uniform expression.♻️ Optional: make the forced-path intent explicit
- let row = load_for_update(txn, machine_id).await?; - let current_machine_version = row.machine_version; - let current = row.decode(machine_id)?; - if mode == SetMode::IfChanged - && let Some(current) = current.as_ref() - && request_is_satisfied(¤t.value, target) - { - return Ok(current.clone()); - } - - let target = current - .as_ref() - .filter(|current| request_is_satisfied(¤t.value, target)) - .map_or(target, |current| ¤t.value); + let row = load_for_update(txn, machine_id).await?; + let current_machine_version = row.machine_version; + let current = row.decode(machine_id)?; + let satisfied = current + .as_ref() + .filter(|current| request_is_satisfied(¤t.value, target)); + // A satisfied request never weakens the stored value: `IfChanged` keeps the + // existing generation, `Force` re-publishes the stronger stored target. + if let Some(current) = satisfied { + if mode == SetMode::IfChanged { + return Ok(current.clone()); + } + } + let target = satisfied.map_or(target, |current| ¤t.value);🤖 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_desired_boot_interface.rs` around lines 420 - 442, In set_with_mode, narrow the stored-value substitution after the IfChanged early return to the SetMode::Force path, avoiding the second request_is_satisfied evaluation. Preserve the existing behavior of returning the current version for satisfied IfChanged requests and retaining the requested target otherwise.crates/api-core/src/handlers/managed_host.rs (1)
288-297: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider gating the wakeup on an advanced desired generation.
db::machine_desired_boot_interface::setusesSetMode::IfChanged, so it can leave the version untouched when the promoted row already matches the stored target. The repair path exercised bytest_set_primary_interface_repairs_dpu_host_with_no_admin_primaryreaches this state: the primary flag moves, but the desired target does not. The handler then still callsenqueue_boot_interface_reconciliation, which schedules a controller pass with no pending generation. The result is one redundant reconciliation, not a correctness fault.Both
setandforce_setreturnVersioned<MachineBootInterfaceTarget>. Compare that version against the locked pre-write value and enqueue only when it advanced.♻️ Sketch of the version-gated enqueue
- if force_reconcile { - db::machine_desired_boot_interface::force_set(&mut txn, &host_machine_id, &boot_target) - .await?; - } else { - db::machine_desired_boot_interface::set(&mut txn, &host_machine_id, &boot_target).await?; - } + let previous_version = db::machine_desired_boot_interface::lock(&mut txn, &host_machine_id) + .await? + .map(|desired| desired.version); + let persisted = if force_reconcile { + db::machine_desired_boot_interface::force_set(&mut txn, &host_machine_id, &boot_target) + .await? + } else { + db::machine_desired_boot_interface::set(&mut txn, &host_machine_id, &boot_target).await? + }; + let generation_advanced = previous_version != Some(persisted.version); txn.commit().await?; - enqueue_boot_interface_reconciliation(api, host_machine_id, should_enqueue).await; + enqueue_boot_interface_reconciliation(api, host_machine_id, should_enqueue && generation_advanced) + .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/managed_host.rs` around lines 288 - 297, Capture the locked pre-write desired boot-interface version before calling `db::machine_desired_boot_interface::set` or `force_set`, then compare it with the returned `Versioned<MachineBootInterfaceTarget>` version. Update the handler’s enqueue logic after `txn.commit()` to call `enqueue_boot_interface_reconciliation` only when the version advanced, while preserving the existing write behavior for both `force_reconcile` branches.
🤖 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/managed_host.rs`:
- Around line 288-297: Capture the locked pre-write desired boot-interface
version before calling `db::machine_desired_boot_interface::set` or `force_set`,
then compare it with the returned `Versioned<MachineBootInterfaceTarget>`
version. Update the handler’s enqueue logic after `txn.commit()` to call
`enqueue_boot_interface_reconciliation` only when the version advanced, while
preserving the existing write behavior for both `force_reconcile` branches.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 420-442: In set_with_mode, narrow the stored-value substitution
after the IfChanged early return to the SetMode::Force path, avoiding the second
request_is_satisfied evaluation. Preserve the existing behavior of returning the
current version for satisfied IfChanged requests and retaining the requested
target otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6aa96dc-3366-4999-a188-e5bb3fdc5eec
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai Thanks -- I verified both final review-body nits and am leaving them unchanged. In set_with_mode, request_is_satisfied is a cheap pure predicate and the single target-selection expression keeps the pair-preservation rule uniform; the proposed mode-specific nesting adds control flow without changing behavior. More importantly, enqueueing after a primary-row repair is intentional even when the desired version is unchanged: that existing generation may still be pending or unobserved, or its earlier wake may have been lost, and the idempotent queue wake ensures machine-controller re-evaluates the persisted intent. Gating solely on version advancement would remove that recovery path. |
|
Tip For best results, initiate chat on the files or code changes.
The shared After a primary-row repair, the handler must enqueue reconciliation even when the desired version is unchanged. The queue wake re-evaluates persisted intent when the generation is pending, unobserved, or was not processed after an earlier wake. 🐇 ✓ ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/api-core/tests/integration/machine_boot_interfaces.rs (1)
175-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the generated enum accessor for this assertion.
proststores the field asi32and generatesreconciliation_state(). Compare its result withReconciliationState::Pending. Unknown values map to the enum default; the accessor does not reject them.🤖 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/tests/integration/machine_boot_interfaces.rs` around lines 175 - 179, Update the assertion around reconciliation_state to call the generated reconciliation_state() accessor and compare its enum result directly with ReconciliationState::Pending, removing the manual i32 cast while preserving the existing assertion message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/boot_interface/show/cmd.rs`:
- Around line 154-171: Add table-driven value_scenarios! tests for
ReconciliationReport::from covering a recognized reconciliation state and an
unknown numeric state. Assert recognized values use the state name and unknown
values produce the Unknown(...) fallback, while preserving the existing
report-field mappings.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 487-509: The find_by_machine_id_for_update function must not rely
on SELECT ... ORDER BY id FOR UPDATE to acquire locks in ID order. First
retrieve the non-BMC interface IDs in ascending order, then issue a separate FOR
UPDATE query for each ID sequentially before constructing the snapshots,
preserving the existing result ordering and error handling.
---
Nitpick comments:
In `@crates/api-core/tests/integration/machine_boot_interfaces.rs`:
- Around line 175-179: Update the assertion around reconciliation_state to call
the generated reconciliation_state() accessor and compare its enum result
directly with ReconciliationState::Pending, removing the manual i32 cast while
preserving the existing assertion message.
🪄 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: 3b219389-3ece-4411-984e-27101bf30e27
⛔ 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 (27)
crates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_interface/args.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/boot_interface_resolution.rscrates/api-core/src/tests/set_primary_dpu.rscrates/api-core/src/tests/set_primary_interface.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/machine.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/machine_detail.htmlcrates/rpc/build.rscrates/rpc/proto/forge.protodocs/observability/core_metrics.mdrest-api/proto/core/src/v1/nico_nico.proto
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Today `SetPrimaryInterface`, `SetPrimaryDpu`, and the managed boot-order helpers can update Redfish while the database change is still in flight. That leaves two sources of truth and makes an admin request responsible for orchestration work that already belongs to `machine-controller`. So, this commits the selected primary interface, network ownership, and versioned `machine_desired_boot_interfaces` target in one lock-ordered transaction, then wakes `machine-controller` after commit when a Ready, unassigned host can reconcile it. Assigned hosts keep the request pending, and managed BMC endpoints use the same desired-state path while unowned endpoints and DPUs retain direct Redfish behavior. `force_reconcile` creates a fresh desired generation without making the request wait for Redfish or a reboot. The deprecated `reboot` field remains an alias, and clients populate both fields so mixed-version deployments keep the old behavior when they reach an older server. `GetMachineBootInterfaces` now reports `Pending`, `Converging`, `Converged`, or `Failed` reconciliation state, which gives the CLI and web UI one server-derived view of progress. This supports NVIDIA#4247 Tests updated! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
3 participants
Historically,
SetPrimaryInterface,SetPrimaryDpu,machine_setup, andset_dpu_first_boot_orderhave mixed database updates with synchronous Redfish work. If Redfish succeeds and a later database write fails, the BMC and NICo can disagree, and operators have no single place to see whether the desired boot interface has actually converged.So, this makes the database the authority for managed-host boot-interface changes. The primary interface, admin address ownership, network versions, and versioned
machine_desired_boot_interfacestarget now commit in one lock-ordered transaction; only after commit do we wakemachine-controllerto reconcile Redfish. Ready hosts without an instance can start immediately, while Assigned hosts keep the desired generation pending until they are eligible.force_reconcilelets an operator ask for a fresh generation even when the target did not change. The deprecatedrebootfield remains an alias, and newer clients populate both fields so they still work with an older API server.GetMachineBootInterfacesnow reportsPending,Converging,Converged, orFailedthrough typed protobuf values, which gives the CLI and web UI one server-derived view of progress. Unowned endpoints and DPUs still use direct Redfish; only managed host BMC endpoints enter the desired-state flow.Related issues
This supports #4247
Type of Change
Breaking Changes
Testing
Additional Notes
Validated with
cargo make format-nightly,cargo make clippy,cargo make carbide-lints, focused SQLx tests across the database/API/web boundaries, CLI serialization tests, generated Go protobuf tests, and the metric catalogue check.The checked-in Go protobuf diff is mechanically large because
protocregenerated its descriptor and index tables for the typed nested reconciliation enum; the authored schema change is 61 lines. Hardware-backed manual testing was not available in this environment.Closes #4247