feat: Managed switch decommissioning - #4679
Open
ericpretzel wants to merge 5 commits into
Open
Conversation
…mmissioning # Conflicts: # rest-api/proto/core/gen/v1/nico_nico.pb.go
Contributor
Summary by CodeRabbit
WalkthroughChangesManaged switch decommissioning
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant CoreAPI
participant SwitchController
participant NVOS
participant BMC
participant DHCPRecords
AdminCLI->>CoreAPI: request switch decommission
CoreAPI->>SwitchController: persist decommission request
SwitchController->>NVOS: reset and verify NVOS
NVOS-->>SwitchController: reset and DHCP results
SwitchController->>BMC: reset BMC
BMC-->>SwitchController: reset result
SwitchController->>DHCPRecords: verify DHCP suppression
SwitchController-->>CoreAPI: mark switch Decommissioned
sequenceDiagram
participant AdminCLI
participant CoreAPI
participant CredentialStore
participant Database
AdminCLI->>CoreAPI: delete decommissioned switch
CoreAPI->>CredentialStore: delete BMC and NVOS credentials
CredentialStore-->>CoreAPI: deletion results
CoreAPI->>Database: remove interfaces, suppression records, and switch
Database-->>CoreAPI: deletion result
CoreAPI-->>AdminCLI: return success or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
crates/api-core/src/tests/switch_state_controller/mod.rs (1)
159-170: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftTest the persistent cleanup contract.
The test verifies only removal of the
switchesrow. Add associated machine interfaces, retained boot interfaces, and DHCP suppression records to the fixture. Assert that the deletion RPC removes each record. A partial cleanup currently passes this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/switch_state_controller/mod.rs` around lines 159 - 170, Expand the deletion test around delete_decommissioned_switch to seed associated machine interfaces, retained boot interfaces, and DHCP suppression records for the fixture switch, then query each repository after the RPC and assert every record is removed. Keep the existing switches-row assertion and use the corresponding fixture identifiers to verify complete persistent cleanup.crates/api-model/src/switch/mod.rs (2)
603-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a round-trip case for
FactoryResetNvos { job_id: None }.The table covers the accepted-job form (
job_id: Some(...)) but not the pre-submission form.job_id: Noneis the state the controller persists first, andhandle_preparingtransitions directly into it, so it is the variant that must survive a restart. Add both directions to lock the wire form for thenullcase.💚 Suggested additional scenarios
Serialization table:
+ "decommissioning: NVOS reset not yet submitted" { + SwitchControllerState::Decommissioning { + decommissioning_state: SwitchDecommissioningState::FactoryResetNvos { + job_id: None, + }, + } => Yields( + r#"{"state":"decommissioning","decommissioning_state":{"state":"factoryresetnvos","job_id":null}}"# + .to_string(), + ), + } +Deserialization table:
+ "decommissioning NVOS reset with absent job_id" { + r#"{"state":"decommissioning","decommissioning_state":{"state":"factoryresetnvos"}}"# => Yields( + SwitchControllerState::Decommissioning { + decommissioning_state: SwitchDecommissioningState::FactoryResetNvos { + job_id: None, + }, + }, + ), + } +As per coding guidelines: "Prefer table-driven tests for functions mapping inputs to outputs or errors. Use
scenarios!withOutcomefor fallible operations".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-model/src/switch/mod.rs` around lines 603 - 641, Add table-driven round-trip scenarios for SwitchDecommissioningState::FactoryResetNvos with job_id: None in both serialization and deserialization test tables. Assert the wire representation uses "job_id":null, alongside the existing Some("reset-1") case, so the pre-submission state is preserved across restarts.Source: Coding guidelines
495-495: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an SLA for the active decommissioning substates.
Decommissioningis not a steady state. It submits an RMS factory-reset job, force-restarts NVOS, resets the BMC, and then waits for two DHCP suppression acknowledgements.handle_factory_reset_nvosand the two verification handlers returnStateHandlerOutcome::wait(...), so a switch can remain in a wait loop indefinitely if the RMS job never settles or the DHCP acknowledgement never arrives. Withno_sla(), that stall never surfaces through the SLA signal.
Decommissionedis genuinely terminal, so a blanket SLA would fire falsely. Match on the substate instead and apply an SLA only to the in-flight phases.♻️ Suggested per-substate SLA
- SwitchControllerState::Decommissioning { .. } => StateSla::no_sla(), + SwitchControllerState::Decommissioning { + decommissioning_state, + } => match decommissioning_state { + SwitchDecommissioningState::Decommissioned => StateSla::no_sla(), + _ => StateSla::with_sla( + std::time::Duration::from_secs(slas::DECOMMISSIONING), + time_in_state, + ), + },This requires a new
slas::DECOMMISSIONINGconstant sized for the NVOS reset plus both DHCP acknowledgement waits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-model/src/switch/mod.rs` at line 495, Update the Decommissioning SLA handling in the switch state mapping to match its substates rather than returning StateSla::no_sla() for every Decommissioning variant. Add slas::DECOMMISSIONING for the in-flight phases, including the NVOS reset and DHCP acknowledgement waits, while keeping the terminal Decommissioned substate on no_sla().crates/switch-controller/src/ready.rs (1)
50-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the entry into decommissioning.
The precedence and the atomicity are correct here: decommissioning outranks maintenance and reprovisioning, and the flag is cleared in the same transaction that carries the transition, so the request can neither be lost nor consumed twice.
The branch emits no log, unlike the maintenance branch at lines 62-66 and the reprovisioning branches at lines 74-77 and 91-95. Entering decommissioning starts an irreversible sequence of factory resets. Add a structured log line so operators can correlate the transition with the destructive calls that follow.
♻️ Suggested log line
if state.decommission_requested { + tracing::info!( + switch_id = %switch_id, + "Switch decommissioning requested; transitioning to Decommissioning" + ); let mut txn = ctx.services.db_pool.begin().await?; db_switch::clear_decommission_requested(&mut txn, *switch_id).await?;As per coding guidelines: "Emit logfmt-compatible structured logs. Tracing messages must be stable human-readable string literals; record dynamic values as structured fields instead of interpolating 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/switch-controller/src/ready.rs` around lines 50 - 59, In the decommission_requested branch of the ready-state handler, add a structured log entry immediately before starting the transaction and transitioning to Decommissioning. Use a stable human-readable message literal and record the switch identifier as a structured field, matching the logging pattern used by the maintenance and reprovisioning branches.Source: Coding guidelines
crates/switch-controller/src/decommissioning.rs (1)
133-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for the factory-reset job-status mapping.
This module has no test module. The integration test in
crates/api-core/src/tests/switch_state_controller/mod.rscovers onlyReady -> Preparing -> FactoryResetNvos { job_id: None }and the post-Decommissioneddeletion. TheSwitchFactoryResetStatematch at lines 150-169 is the decision point that determines whether a decommission progresses, waits, or demands manual intervention, and it is untested. The pull request notes the managed-switch path has not been exercised in a development environment, which raises the value of this coverage.The mapping from
SwitchFactoryResetStateplus an optional error string to aStateHandlerOutcomeorStateHandlerErroris a pure decision. Extract it into a small function and cover it withscenarios!, so the three branches and the error-suffix formatting are locked without a database or an RMS backend.The
MissingDatapaths inhandle_factory_reset_bmcandhandle_verify_dhcp_releaseare similarly cheap to cover once the switch fixture is available.As per path instructions: "Prefer findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments." As per coding guidelines: "Prefer table-driven tests for functions mapping inputs to outputs or errors. Use
scenarios!withOutcomefor fallible operations."🤖 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/switch-controller/src/decommissioning.rs` around lines 133 - 189, Extract the SwitchFactoryResetState match from handle_factory_reset_nvos into a pure decision function that maps state and optional error text to the existing outcome or error types, preserving pending, completed, failed, and suffix formatting behavior. Add table-driven scenarios! coverage using Outcome for all branches, including failed jobs with and without error text, without requiring database or RMS services. Also cover the MissingData paths in handle_factory_reset_bmc and handle_verify_dhcp_release if the existing switch fixture supports them.Sources: Coding guidelines, 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/auth/internal_rbac_rules.rs`:
- Around line 302-303: Update the DeleteDecommissionedSwitch permission entry in
the RBAC rule definitions to grant access only to ForgeAdminCLI. Remove Flow
from its caller list while leaving the DecommissionSwitch permission unchanged.
In `@crates/api-core/src/handlers/switch.rs`:
- Around line 385-396: Make the credential cleanup in the switch deletion flow
idempotent by treating a not-found response from Vault metadata deletion as
success, while still propagating other errors; update
delete_bmc_root_credentials_by_mac and the NVOS credential deletion path as
needed. Add a retry test covering partial cleanup, verifying the switch row and
remaining NVOS credential are deleted successfully when the BMC secret is
already absent.
In `@crates/switch-controller/src/decommissioning.rs`:
- Around line 179-188: The factory-reset submission flow around
batch_reset_switch_factory_default must prevent duplicate RMS resets when state
persistence fails after acceptance. Add switch-keyed idempotency or job
reconciliation that records and recovers the submission intent, preserving
OperationOutcomeUnknown semantics so retries do not resubmit an uncertain
operation. Add a failure-path test verifying that only one RMS reset is
submitted.
- Around line 112-131: Centralize the RMS backend validation currently performed
in handle_preparing into a shared helper, preserving the existing
missing-backend and non-RMS InvalidState errors. Invoke that helper from
handle_preparing, handle_factory_reset_nvos, handle_verify_nvos_dhcp_release,
and decommission_switch so persisted states and direct decommission operations
are validated before proceeding; if needed, add a typed NvSwitchManager
discriminator instead of duplicating raw name checks.
- Around line 191-220: The decommissioning flow must remain usable after
factory-reset credentials change. In handle_verify_nvos_dhcp_release
(crates/switch-controller/src/decommissioning.rs:191-220), defer full endpoint
resolution until ForceRestarting and use a MAC-only lookup or persisted MAC
while awaiting DHCP; ensure power_control receives post-reset credentials or
force-restarts before reset. In the retry path at
crates/switch-controller/src/decommissioning.rs:277-286, make retries after
bmc_reset_to_defaults idempotent and reset-aware so DHCP suppression or
transition-commit failures do not recreate the client with stale stored BMC
credentials.
In
`@docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md`:
- Around line 15-17: Update the managed-switch decommission documentation at
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md:15-17
to describe Ready-state and RMS-backend prerequisites, request persistence,
asynchronous progression, terminal outcome, and failure behavior; update
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-delete-decommissioned.md:16-18
to document the terminal Decommissioned prerequisite, irreversible deletion,
removed managed records, retry behavior, and unsupported paths, then regenerate
the CLI references from the canonical source.
In `@rest-api/flow/internal/nicoapi/grpc.go`:
- Around line 570-573: Update the error message returned by the
DecommissionSwitch call to begin with lower-case text, while preserving the
switchID context and wrapped err value.
In `@rest-api/flow/internal/task/componentmanager/nvswitch/nico/nico.go`:
- Around line 622-628: Update the state-formatting logic around the JSON
unmarshal to return raw when state.DecommissioningState.State is empty, before
constructing the "Decommissioning/" result. Preserve the existing
"Decommissioned" and non-empty decommissioning-state behavior, and add coverage
for {"state":"decommissioning"}.
---
Nitpick comments:
In `@crates/api-core/src/tests/switch_state_controller/mod.rs`:
- Around line 159-170: Expand the deletion test around
delete_decommissioned_switch to seed associated machine interfaces, retained
boot interfaces, and DHCP suppression records for the fixture switch, then query
each repository after the RPC and assert every record is removed. Keep the
existing switches-row assertion and use the corresponding fixture identifiers to
verify complete persistent cleanup.
In `@crates/api-model/src/switch/mod.rs`:
- Around line 603-641: Add table-driven round-trip scenarios for
SwitchDecommissioningState::FactoryResetNvos with job_id: None in both
serialization and deserialization test tables. Assert the wire representation
uses "job_id":null, alongside the existing Some("reset-1") case, so the
pre-submission state is preserved across restarts.
- Line 495: Update the Decommissioning SLA handling in the switch state mapping
to match its substates rather than returning StateSla::no_sla() for every
Decommissioning variant. Add slas::DECOMMISSIONING for the in-flight phases,
including the NVOS reset and DHCP acknowledgement waits, while keeping the
terminal Decommissioned substate on no_sla().
In `@crates/switch-controller/src/decommissioning.rs`:
- Around line 133-189: Extract the SwitchFactoryResetState match from
handle_factory_reset_nvos into a pure decision function that maps state and
optional error text to the existing outcome or error types, preserving pending,
completed, failed, and suffix formatting behavior. Add table-driven scenarios!
coverage using Outcome for all branches, including failed jobs with and without
error text, without requiring database or RMS services. Also cover the
MissingData paths in handle_factory_reset_bmc and handle_verify_dhcp_release if
the existing switch fixture supports them.
In `@crates/switch-controller/src/ready.rs`:
- Around line 50-59: In the decommission_requested branch of the ready-state
handler, add a structured log entry immediately before starting the transaction
and transitioning to Decommissioning. Use a stable human-readable message
literal and record the switch identifier as a structured field, matching the
logging pattern used by the maintenance and reprovisioning branches.
🪄 Autofix
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: d06300a0-bf27-4d30-9710-48ad41d1c800
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (29)
crates/admin-cli/src/managed_switch/decommission/args.rscrates/admin-cli/src/managed_switch/decommission/cmd.rscrates/admin-cli/src/managed_switch/decommission/mod.rscrates/admin-cli/src/managed_switch/delete_decommissioned/args.rscrates/admin-cli/src/managed_switch/delete_decommissioned/cmd.rscrates/admin-cli/src/managed_switch/delete_decommissioned/mod.rscrates/admin-cli/src/managed_switch/mod.rscrates/admin-cli/src/managed_switch/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/switch.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-db/migrations/20260806120000_switch_decommission_requested.sqlcrates/api-db/src/switch.rscrates/api-model/src/switch/mod.rscrates/rpc/proto/forge.protocrates/rpc/src/model/switch.rscrates/switch-controller/src/decommissioning.rscrates/switch-controller/src/handler.rscrates/switch-controller/src/io.rscrates/switch-controller/src/lib.rscrates/switch-controller/src/ready.rsdocs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.mddocs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-delete-decommissioned.mddocs/manuals/nico-admin-cli/commands/managed-switch/managed-switch.mdrest-api/flow/internal/nicoapi/grpc.gorest-api/flow/internal/task/componentmanager/nvswitch/nico/nico.gorest-api/flow/internal/task/componentmanager/nvswitch/nico/nico_test.gorest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+302
to
+303
| x.perm("DecommissionSwitch", vec![ForgeAdminCLI, Flow]); | ||
| x.perm("DeleteDecommissionedSwitch", vec![ForgeAdminCLI, Flow]); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 '\.DeleteDecommissionedSwitch\s*\(' rest-api/flow
rg -n -C 4 'DeleteDecommissionedSwitch' crates rest-api/flowRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- permission definitions ---'
sed -n '270,320p' crates/api-core/src/auth/internal_rbac_rules.rs
printf '%s\n' '--- exact symbol references ---'
rg -n -C 5 'DeleteDecommissionedSwitch|DecommissionSwitch' crates rest-api/flow || true
printf '%s\n' '--- Flow authorization and client method references ---'
rg -n -C 4 'Flow|flow' crates/api-core/src/auth/internal_rbac_rules.rs rest-api/flow || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- exact deletion symbol matches ---'
rg -n -F 'DeleteDecommissionedSwitch' crates rest-api/flow || true
printf '%s\n' '--- exact decommission symbol matches ---'
rg -n -F 'DecommissionSwitch' crates rest-api/flow || true
printf '%s\n' '--- permission block ---'
sed -n '294,308p' crates/api-core/src/auth/internal_rbac_rules.rs
printf '%s\n' '--- generated/API definitions that may expose the RPC ---'
rg -n -F 'DeleteDecommissionedSwitch' . --glob '!target/**' --glob '!node_modules/**' --glob '!*.lock' || trueRepository: NVIDIA/infra-controller
Length of output: 9727
Restrict permanent switch deletion to ForgeAdminCLI.
Flow calls DecommissionSwitch but has no DeleteDecommissionedSwitch caller. Remove Flow from this permission.
🤖 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/auth/internal_rbac_rules.rs` around lines 302 - 303,
Update the DeleteDecommissionedSwitch permission entry in the RBAC rule
definitions to grant access only to ForgeAdminCLI. Remove Flow from its caller
list while leaving the DecommissionSwitch permission unchanged.
Source: Path instructions
Contributor
Author
There was a problem hiding this comment.
this will be added later
Contributor
There was a problem hiding this comment.
@ericpretzel, acknowledged. The Flow permission can remain deferred until Flow implements DeleteDecommissionedSwitch.
Would you like me to open a follow-up issue to track the Flow caller and the related RBAC update?
🐇💭
You are interacting with an AI system.
Comment on lines
+112
to
+131
| fn handle_preparing( | ||
| switch_id: &SwitchId, | ||
| ctx: &StateHandlerContext<'_, SwitchStateHandlerContextObjects>, | ||
| ) -> Result<StateHandlerOutcome<SwitchControllerState>, StateHandlerError> { | ||
| let component_manager = ctx.services.component_manager.as_ref().ok_or_else(|| { | ||
| StateHandlerError::InvalidState(format!( | ||
| "switch {switch_id} requires the RMS component-manager backend for decommissioning" | ||
| )) | ||
| })?; | ||
| if component_manager.nv_switch.name() != "rms" { | ||
| return Err(StateHandlerError::InvalidState(format!( | ||
| "switch {switch_id} requires the RMS component-manager backend for decommissioning; configured backend is {}", | ||
| component_manager.nv_switch.name() | ||
| ))); | ||
| } | ||
|
|
||
| Ok(StateHandlerOutcome::transition(decommissioning( | ||
| SwitchDecommissioningState::FactoryResetNvos { job_id: None }, | ||
| ))) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the nv_switch backend trait and any typed backend discriminant.
set -euo pipefail
fd -t f -e rs . crates/component-manager --exec ast-grep outline {} --items all \; 2>/dev/null | head -100
# Find the `name()` definition on the NvSwitch manager trait and its implementors.
rg -nP --type=rust -C6 'fn\s+name\s*\(\s*&self\s*\)\s*->' crates/component-manager
# Find every site that compares the backend name to a literal.
rg -nP --type=rust -C3 'nv_switch\s*\.\s*name\s*\(\s*\)'Repository: NVIDIA/infra-controller
Length of output: 16784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Backend enum and trait ---'
cat -n crates/component-manager/src/nv_switch_manager.rs | sed -n '1,45p;145,170p'
printf '%s\n' '--- RMS backend implementation ---'
cat -n crates/component-manager/src/rms.rs | sed -n '1438,1460p'
printf '%s\n' '--- Decommissioning handlers ---'
cat -n crates/switch-controller/src/decommissioning.rs | sed -n '90,225p'
printf '%s\n' '--- API handler ---'
cat -n crates/api-core/src/handlers/switch.rs | sed -n '245,280p'
printf '%s\n' '--- ComponentManager ownership and construction ---'
rg -n -C4 --type rust 'component_manager|ComponentManagerConfig|NvSwitchBackend' crates/switch-controller crates/api-core crates/component-manager | head -240Repository: NVIDIA/infra-controller
Length of output: 30893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Component manager type and construction ---'
ast-grep outline crates/component-manager/src/component_manager.rs --items all
rg -n -C5 --type rust 'pub struct ComponentManager|struct ComponentManager|nv_switch:' crates/component-manager/src/component_manager.rs crates/api-core crates/switch-controller
printf '%s\n' '--- All NV-switch backend-name comparisons and constants ---'
rg -n -C3 --type rust 'name\(\)\s*[!=]=|BACKEND_NAME|Backend::Rms|NvSwitchBackend' crates/switch-controller crates/api-core crates/component-manager/src
printf '%s\n' '--- Decommission state persistence and handler context setup ---'
rg -n -C5 --type rust 'FactoryResetNvos|SwitchDecommissioningState|SwitchStateHandlerContextObjects|component_manager:' crates/switch-controller crates/api-core crates/state-controller crates/model 2>/dev/null | head -300Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Context ownership ---'
cat -n crates/switch-controller/src/context.rs | sed -n '1,140p'
printf '%s\n' '--- State serialization and persisted controller state ---'
rg -n -C4 --type rust 'controller_state|serde_json|SwitchControllerState|decommissioning_state' crates/model crates/db crates/switch-controller | head -220
printf '%s\n' '--- Decommissioning tests and backend setup ---'
rg -n -C6 --type rust 'handle_preparing|handle_factory_reset_nvos|decommission|MockNvSwitchManager|name\(\)' crates/switch-controller/src crates/api-core/src/tests | head -260Repository: NVIDIA/infra-controller
Length of output: 20487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Factory-reset implementations ---'
rg -n -C8 --type rust 'fn batch_reset_switch_factory_default|batch_reset_switch_factory_default' crates/component-manager/src/nsm.rs crates/component-manager/src/mock.rs crates/component-manager/src/rms.rs crates/component-manager/src/nv_switch_manager.rs
printf '%s\n' '--- State model location and persistence fields ---'
fd -t f -e rs . | rg '(^|/)(switch|model|db)(/|_)' | head -80
rg -n -C4 --type rust 'pub enum SwitchControllerState|SwitchDecommissioningState|controller_state.*json|controller_state.*value' . | head -220Repository: NVIDIA/infra-controller
Length of output: 43465
Centralize RMS backend validation before each decommissioning operation.
handle_preparing checks name() == "rms", but handle_factory_reset_nvos and handle_verify_nvos_dhcp_release check only for presence. A persisted FactoryResetNvos state can therefore resume with a non-RMS backend and reach an operation that the backend does not support. Reuse one shared backend check in these handlers and decommission_switch. The existing Backend enum is not exposed by NvSwitchManager; add a typed discriminator if the raw "rms" comparison is removed.
🤖 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/switch-controller/src/decommissioning.rs` around lines 112 - 131,
Centralize the RMS backend validation currently performed in handle_preparing
into a shared helper, preserving the existing missing-backend and non-RMS
InvalidState errors. Invoke that helper from handle_preparing,
handle_factory_reset_nvos, handle_verify_nvos_dhcp_release, and
decommission_switch so persisted states and direct decommission operations are
validated before proceeding; if needed, add a typed NvSwitchManager
discriminator instead of duplicating raw name checks.
Source: Coding guidelines
Comment on lines
+179
to
+188
| let job_id = component_manager | ||
| .nv_switch | ||
| .batch_reset_switch_factory_default(&[endpoint], tls_server_domain.as_deref()) | ||
| .await | ||
| .map_err(|error| external_error("failed to submit NVOS factory reset", error))?; | ||
| Ok(StateHandlerOutcome::transition(decommissioning( | ||
| SwitchDecommissioningState::FactoryResetNvos { | ||
| job_id: Some(job_id), | ||
| }, | ||
| ))) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the RMS factory-reset submission contract and any job-lookup-by-target API.
set -euo pipefail
rg -nP --type=rust -C10 'fn\s+batch_reset_switch_factory_default' crates
# Look for an API that lists or finds reset jobs for a given switch, which would enable recovery.
rg -nP --type=rust -C5 'fn\s+\w*(list|find|get)\w*(factory_reset|reset_job)\w*\s*\(' cratesRepository: NVIDIA/infra-controller
Length of output: 11284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RMS submission implementation ---'
sed -n '1928,1992p' crates/component-manager/src/rms.rs
printf '%s\n' '--- component-manager contract ---'
sed -n '458,520p' crates/component-manager/src/component_manager.rs
sed -n '210,270p' crates/component-manager/src/nv_switch_manager.rs
printf '%s\n' '--- decommissioning handler and state model references ---'
sed -n '125,205p' crates/switch-controller/src/decommissioning.rs
rg -n -C5 'FactoryResetNvos|job_id.*persist|Once accepted|transition\(' crates/switch-controller crates/api-model crates -g '*.rs' | head -n 240Repository: NVIDIA/infra-controller
Length of output: 31773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- reset request and response types ---'
rg -n -C8 'BatchResetSwitchFactoryDefault(Request|Response)|batch_reset_switch_factory_default' crates/component-manager/src crates/api-test-helper/src -g '*.rs'
printf '%s\n' '--- reset-job lookup APIs ---'
rg -n -i -C4 'factory.?reset|reset.?job|operation.?outcome.?unknown|idempot' crates/component-manager/src crates/switch-controller/src -g '*.rs' | head -n 320Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RMS outcome mapping ---'
sed -n '2190,2255p' crates/component-manager/src/rms.rs
printf '%s\n' '--- state outcome persistence path ---'
rg -n -C8 'struct StateHandlerOutcome|enum StateHandlerOutcome|impl.*StateHandlerOutcome|with_txn|outcome.*transition|persist.*state|update.*state' crates -g '*.rs' | head -n 260
printf '%s\n' '--- decommissioning state definition and persistence fields ---'
rg -n -C12 'enum SwitchDecommissioningState|FactoryResetNvos|decommissioning_state' crates -g '*.rs' | head -n 260Repository: NVIDIA/infra-controller
Length of output: 44749
Prevent automatic resubmission after an uncertain RMS reset submission.
If state persistence fails after RMS accepts the reset, the persisted state still has job_id: None. The next retry submits the destructive operation again. Component Manager explicitly maps this outcome to OperationOutcomeUnknown and forbids automatic resubmission because dispatch may have occurred. Add idempotency or job reconciliation keyed by the switch, with recovery semantics for the submission intent. Add a failure-path test that proves only one RMS reset is submitted.
🤖 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/switch-controller/src/decommissioning.rs` around lines 179 - 188, The
factory-reset submission flow around batch_reset_switch_factory_default must
prevent duplicate RMS resets when state persistence fails after acceptance. Add
switch-keyed idempotency or job reconciliation that records and recovers the
submission intent, preserving OperationOutcomeUnknown semantics so retries do
not resubmit an uncertain operation. Add a failure-path test verifying that only
one RMS reset is submitted.
Comment on lines
+191
to
+220
| async fn handle_verify_nvos_dhcp_release( | ||
| switch_id: &SwitchId, | ||
| verifying_state: &VerifyNvosDhcpReleaseState, | ||
| ctx: &mut StateHandlerContext<'_, SwitchStateHandlerContextObjects>, | ||
| ) -> Result<StateHandlerOutcome<SwitchControllerState>, StateHandlerError> { | ||
| let endpoint = resolve_switch_endpoint( | ||
| switch_id, | ||
| &ctx.services.db_pool, | ||
| &ctx.services.credential_manager, | ||
| ) | ||
| .await?; | ||
| match verifying_state { | ||
| VerifyNvosDhcpReleaseState::ForceRestarting => { | ||
| let component_manager = ctx.services.component_manager.as_ref().ok_or_else(|| { | ||
| StateHandlerError::InvalidState(format!( | ||
| "switch {switch_id} requires the RMS component-manager backend for decommissioning" | ||
| )) | ||
| })?; | ||
| let result = component_manager | ||
| .nv_switch | ||
| .power_control(std::slice::from_ref(&endpoint), PowerAction::ForceRestart) | ||
| .await | ||
| .map_err(|error| external_error("failed to force restart NVOS", error))? | ||
| .into_iter() | ||
| .next() | ||
| .ok_or_else(|| { | ||
| StateHandlerError::GenericError(eyre::eyre!( | ||
| "component manager returned no force-restart result for switch {switch_id}" | ||
| )) | ||
| })?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine how each post-reset call authenticates, and whether a cheaper NVOS MAC source exists.
set -euo pipefail
fd -t f 'endpoint.rs' crates/switch-controller --exec cat -n {}
rg -nP --type=rust -C15 'fn\s+resolve_switch_endpoint' crates
rg -nP --type=rust -C10 'fn\s+power_control' crates/component-manager
rg -nP --type=rust -C10 'fn\s+bmc_reset_to_defaults' crates
rg -nP --type=rust -C10 'fn\s+for_bmc_mac' crates
rg -nP --type=rust -C3 '\bnvos_mac\b' cratesRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- decommissioning relevant ranges ---'
cat -n crates/switch-controller/src/decommissioning.rs | sed -n '1,340p'
echo '--- decommissioning symbols and call sites ---'
rg -n -C5 --type rust \
'handle_verify_nvos_dhcp_release|VerifyNvosDhcpReleaseState|SwitchFactoryResetState|FactoryResetBmc|create_client|bmc_reset_to_defaults|suppress_dhcp|resolve_switch_endpoint' \
crates/switch-controller/src/decommissioning.rs
echo '--- RMS NV switch power control ---'
cat -n crates/component-manager/src/rms.rs | sed -n '1440,1585p'
echo '--- NV switch endpoint registration and credential use ---'
cat -n crates/component-manager/src/rms.rs | sed -n '840,935p'
cat -n crates/component-manager/src/nsm.rs | sed -n '55,105p'
echo '--- Redfish client construction and reset-related callers ---'
rg -n -C8 --type rust \
'fn\s+create_client|create_client\(|RedfishAuth::for_bmc_mac|FactoryResetBmc|bmc_reset_to_defaults' \
crates/switch-controller crates/redfish crates/component-managerRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- NVOS factory reset implementations ---'
rg -n -C12 --type rust \
'batch_reset_switch_factory_default|get_switch_factory_reset_job_status|factory.?reset|reset_switch_factory' \
crates/component-manager/src crates/api-core/src
echo '--- RMS request credential serialization and client boundary ---'
rg -n -C10 --type rust \
'BatchSetPowerStateRequest|batch_set_power_state|credentials_to_rms|build_switch_node_info|RmsApi' \
crates/component-manager/src/rms.rs crates/component-manager/src
echo '--- Redfish reset implementation and auth flow ---'
rg -n -C15 --type rust \
'bmc_reset_to_defaults|ResetToDefaults|reset.*defaults|RedfishAuth::Key|CredentialKey|auth_attempts|authenticate' \
crates/redfish/src crates/switch-controller/src
echo '--- Switch model MAC fields and endpoint-row query helpers ---'
rg -n -C8 --type rust \
'struct Switch\b|nvos_mac|bmc_info|find_switch_endpoints_by_ids|SwitchEndpointRow' \
crates/api-model crates/db crates/switch-controller/srcRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- exact symbol locations ---'
rg -l --type rust 'batch_reset_switch_factory_default|get_switch_factory_reset_job_status' crates/component-manager crates/api-core
rg -n --type rust --max-count 8 'batch_reset_switch_factory_default|get_switch_factory_reset_job_status' crates/component-manager crates/api-core
echo '--- redfish reset symbol locations ---'
rg -l --type rust 'bmc_reset_to_defaults' crates/redfish
rg -n --type rust --max-count 12 'bmc_reset_to_defaults' crates/redfish
echo '--- Redfish auth implementation locations ---'
rg -l --type rust 'impl RedfishClientPool|RedfishAuth::Key|CredentialKey' crates/redfish/src
rg -n --type rust --max-count 20 'RedfishAuth::Key|CredentialKey|create_client' crates/redfish/src/libredfish/implementation.rs crates/redfish/src/libredfish/auth.rs
echo '--- switch model and endpoint query locations ---'
rg -l --type rust 'pub struct Switch\b|struct SwitchEndpointRow|find_switch_endpoints_by_ids' crates/api-model crates/db crates/switch-controller
rg -n --type rust --max-count 12 'pub struct Switch\b|struct SwitchEndpointRow|find_switch_endpoints_by_ids' crates/api-model crates/db crates/switch-controller
echo '--- narrow credential assertions in relevant tests ---'
rg -n --type rust --max-count 20 -C3 \
'host_endpoint.*credentials|bmc_endpoint.*credentials|nvos_credentials|bmc_credentials|bmc_reset_to_defaults' \
crates/component-manager/src/rms.rs crates/redfish/src/libredfish/test_support.rsRepository: NVIDIA/infra-controller
Length of output: 3933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- RMS reset helpers ---'
cat -n crates/component-manager/src/rms.rs | sed -n '1935,2020p'
cat -n crates/component-manager/src/rms.rs | sed -n '2185,2230p'
cat -n crates/component-manager/src/rms.rs | sed -n '2350,2395p'
echo '--- Redfish client authentication ---'
cat -n crates/redfish/src/libredfish/implementation.rs | sed -n '68,125p'
cat -n crates/redfish/src/libredfish/auth.rs | sed -n '18,38p'
echo '--- switch model fields ---'
cat -n crates/api-model/src/switch/mod.rs | sed -n '1,180p'
echo '--- endpoint row and query definitions ---'
rg -l --type rust 'struct SwitchEndpointRow|find_switch_endpoints_by_ids' .
rg -n --type rust --max-count 20 'struct SwitchEndpointRow|find_switch_endpoints_by_ids' .Repository: NVIDIA/infra-controller
Length of output: 23312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
decom = Path("crates/switch-controller/src/decommissioning.rs").read_text()
rms = Path("crates/component-manager/src/rms.rs").read_text()
redfish = Path("crates/redfish/src/libredfish/implementation.rs").read_text()
auth = Path("crates/redfish/src/libredfish/auth.rs").read_text()
def require(label, pattern, text):
if not re.search(pattern, text, re.S):
raise SystemExit(f"missing invariant: {label}")
print(f"PASS: {label}")
require(
"completed NVOS reset enters force-restart state",
r"SwitchFactoryResetState::Completed.*?VerifyNvosDhcpReleaseState::ForceRestarting",
decom,
)
require(
"NVOS verify resolves endpoint before state match",
r"async fn handle_verify_nvos_dhcp_release.*?let endpoint = resolve_switch_endpoint\(.*?\)\.await\?.*?match verifying_state",
decom,
)
require(
"waiting state reads only the NVOS MAC from the resolved endpoint",
r"WaitingForDhcpAcknowledgement.*?dhcp_suppression_acknowledged\(endpoint\.nvos_mac",
decom,
)
require(
"force restart uses the resolved endpoint",
r"ForceRestarting.*?power_control\(std::slice::from_ref\(&endpoint\), PowerAction::ForceRestart\)",
decom,
)
require(
"BMC reset happens before suppression and transition",
r"bmc_reset_to_defaults\(\).*?let txn = suppress_dhcp.*?VerifyDhcpRelease",
decom,
)
require(
"BMC factory-reset state recreates client with BMC-MAC auth",
r"FactoryResetBmc.*?RedfishAuth::for_bmc_mac\(bmc_mac_address\)",
decom,
)
require(
"RMS switch node carries both credential sets",
r"bmc_credentials.*?credentials_to_rms\(&ep\.bmc_credentials\).*?nvos_credentials.*?credentials_to_rms\(&ep\.nvos_credentials\)",
rms,
)
require(
"Redfish key auth reads credentials before client creation",
r"RedfishAuth::Key\(credential_key\).*?get_credentials\(&credential_key\).*?Endpoint",
redfish,
)
require(
"for_bmc_mac selects BMC root credentials",
r"RedfishAuth::Key\(CredentialKey::BmcCredentials.*?BmcCredentialType::BmcRoot",
auth,
)
PY
echo '--- narrow endpoint row/query and existing MAC-only callers ---'
cat -n crates/api-db/src/switch.rs | sed -n '720,810p'
cat -n crates/switch-controller/src/maintenance.rs | sed -n '340,370p'Repository: NVIDIA/infra-controller
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
decom = Path("crates/switch-controller/src/decommissioning.rs").read_text()
rms = Path("crates/component-manager/src/rms.rs").read_text()
redfish = Path("crates/redfish/src/libredfish/implementation.rs").read_text()
auth = Path("crates/redfish/src/libredfish/auth.rs").read_text()
def section(text, start, end):
a = text.index(start)
b = text.index(end, a)
return text[a:b]
def ordered(label, text, *parts):
positions = []
cursor = 0
for part in parts:
pos = text.find(part, cursor)
if pos < 0:
raise SystemExit(f"missing invariant: {label}: {part!r}")
positions.append(pos)
cursor = pos + len(part)
print(f"PASS: {label} at offsets {positions}")
verify = section(
decom,
"async fn handle_verify_nvos_dhcp_release(",
"async fn handle_factory_reset_bmc(",
)
ordered(
"NVOS verify resolves endpoint before matching state",
verify,
"let endpoint = resolve_switch_endpoint(",
"match verifying_state",
)
waiting = section(
verify,
"VerifyNvosDhcpReleaseState::WaitingForDhcpAcknowledgement",
" }\n}",
)
ordered(
"waiting state uses only the NVOS MAC after endpoint resolution",
waiting,
"dhcp_suppression_acknowledged(endpoint.nvos_mac",
)
bmc = section(
decom,
"async fn handle_factory_reset_bmc(",
"async fn handle_verify_dhcp_release(",
)
ordered(
"BMC reset precedes suppression and transition",
bmc,
"bmc_reset_to_defaults()",
"let txn = suppress_dhcp",
"SwitchDecommissioningState::VerifyDhcpRelease",
)
ordered(
"BMC retry recreates client with BMC-MAC authentication",
bmc,
"create_client(",
"RedfishAuth::for_bmc_mac(bmc_mac_address)",
)
node = section(rms, "fn build_switch_node_info(", "/// Builds the host-only endpoint")
ordered(
"RMS switch node carries BMC and NVOS credentials",
node,
"credentials_to_rms(&ep.bmc_credentials)",
"credentials_to_rms(&ep.nvos_credentials)",
)
ordered(
"Redfish key auth reads credentials before endpoint creation",
redfish,
"RedfishAuth::Key(credential_key)",
"get_credentials(&credential_key)",
"let endpoint = Endpoint",
)
ordered(
"for_bmc_mac selects BMC root credentials",
auth,
"CredentialKey::BmcCredentials",
"BmcCredentialType::BmcRoot",
)
PY
echo '--- endpoint row/query and existing MAC-only callers ---'
cat -n crates/api-db/src/switch.rs | sed -n '720,810p'
cat -n crates/switch-controller/src/maintenance.rs | sed -n '340,370p'Repository: NVIDIA/infra-controller
Length of output: 6303
Make decommissioning resilient to credential changes after factory reset.
- After NVOS reset completion,
power_controlreceives the stored BMC and NVOS credentials. Provide post-reset credentials or perform the force restart before the reset. - If
bmc_reset_to_defaultssucceeds but DHCP suppression or the transition commit fails, the retry recreates the client with the same stored BMC credential. Make this retry idempotent and reset-aware. - Resolve the full endpoint only in
ForceRestarting. Use a MAC-only lookup, or persist the MAC, while waiting for DHCP acknowledgement.
📍 Affects 1 file
crates/switch-controller/src/decommissioning.rs#L191-L220(this comment)crates/switch-controller/src/decommissioning.rs#L277-L286
🤖 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/switch-controller/src/decommissioning.rs` around lines 191 - 220, The
decommissioning flow must remain usable after factory-reset credentials change.
In handle_verify_nvos_dhcp_release
(crates/switch-controller/src/decommissioning.rs:191-220), defer full endpoint
resolution until ForceRestarting and use a MAC-only lookup or persisted MAC
while awaiting DHCP; ensure power_control receives post-reset credentials or
force-restarts before reset. In the retry path at
crates/switch-controller/src/decommissioning.rs:277-286, make retries after
bmc_reset_to_defaults idempotent and reset-aware so DHCP suppression or
transition-commit failures do not recreate the client with stale stored BMC
credentials.
Comment on lines
+15
to
+17
| ## DESCRIPTION | ||
|
|
||
| Start decommissioning a managed switch |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Document the managed-switch decommissioning contract completely.
The command references omit required preconditions, asynchronous behavior, state transitions, errors, and permanent cleanup effects. Update the CLI help source or a canonical workflow page, then regenerate these references.
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md#L15-L17: document the Ready-state and RMS-backend requirements, request persistence, asynchronous progression, terminal outcome, and failure behavior.docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-delete-decommissioned.md#L16-L18: document the terminalDecommissionedrequirement, irreversible deletion, removed managed records, retry behavior, and unsupported paths.
As per coding guidelines, Markdown must document interface contracts and state-machine transitions completely. As per path instructions, review Markdown for technical correctness and operator usability.
📍 Affects 2 files
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md#L15-L17(this comment)docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-delete-decommissioned.md#L16-L18
🤖 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
`@docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md`
around lines 15 - 17, Update the managed-switch decommission documentation at
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-decommission.md:15-17
to describe Ready-state and RMS-backend prerequisites, request persistence,
asynchronous progression, terminal outcome, and failure behavior; update
docs/manuals/nico-admin-cli/commands/managed-switch/managed-switch-delete-decommissioned.md:16-18
to document the terminal Decommissioned prerequisite, irreversible deletion,
removed managed records, retry behavior, and unsupported paths, then regenerate
the CLI references from the canonical source.
Sources: Coding guidelines, Path instructions
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
🔐 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-08-07 02:25:28 UTC | Commit: b98027c |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4679.docs.buildwithfern.com/infra-controller |
10 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-db/src/managed_host.rs (2)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
DbReaderabstraction for this read-only query.
find_assigned_hosts_in_rackexecutes one read-only query but exposes a concrete&mut PgConnection. Use the establishedimpl DbReader<'_>form. The handler can still pass its transaction reader when it must keep the read andset_decommission_requestedin one transaction.As per coding guidelines, read-only functions in
crates/api-dbshould acceptimpl DbReaderso callers can use a pool without unnecessary transactions. The repository'sDbReadercontract supports connections, transactions, and pools. (raw.githubusercontent.com)Suggested signature
pub async fn find_assigned_hosts_in_rack( - txn: &mut PgConnection, + txn: impl DbReader<'_>, rack_id: &RackId,🤖 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/managed_host.rs` around lines 86 - 89, Update find_assigned_hosts_in_rack to accept the established impl DbReader<'_> abstraction instead of &mut PgConnection, preserving its read-only query behavior and return type. Keep the handler’s transaction reader usage intact so the read can remain in the same transaction as set_decommission_requested, while allowing pool-backed callers.Source: Coding guidelines
85-105: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd focused database coverage for the rack filter.
This query gates a decommission request. Add table-driven integration cases for an assigned non-DPU host in the same rack, a DPU assignment, an assignment in another rack, no assignment, and the intended soft-delete behavior. Assert both returned identifiers and ordering.
As per coding guidelines, use table-driven tests for Rust operations that map inputs to outputs or errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/managed_host.rs` around lines 85 - 105, Add table-driven integration coverage for find_assigned_hosts_in_rack, covering same-rack non-DPU assignments, DPU assignments, assignments in another rack, no assignments, and the expected soft-delete behavior. Assert the returned machine/instance identifiers and their ordering, using existing database fixtures and test conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/managed_host.rs`:
- Around line 90-102: Update the machine query in the decommission-check flow to
append FOR UPDATE OF m after its ordering clause, locking the selected machine
rows through the transaction until commit. Preserve the existing filters,
ordering, bindings, and fetch behavior.
---
Nitpick comments:
In `@crates/api-db/src/managed_host.rs`:
- Around line 86-89: Update find_assigned_hosts_in_rack to accept the
established impl DbReader<'_> abstraction instead of &mut PgConnection,
preserving its read-only query behavior and return type. Keep the handler’s
transaction reader usage intact so the read can remain in the same transaction
as set_decommission_requested, while allowing pool-backed callers.
- Around line 85-105: Add table-driven integration coverage for
find_assigned_hosts_in_rack, covering same-rack non-DPU assignments, DPU
assignments, assignments in another rack, no assignments, and the expected
soft-delete behavior. Assert the returned machine/instance identifiers and their
ordering, using existing database fixtures and test conventions.
🪄 Autofix
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: 7124e350-b03c-425e-81b9-90759f6d0804
📒 Files selected for processing (3)
crates/api-core/src/handlers/switch.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-db/src/managed_host.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-core/src/tests/switch_state_controller/mod.rs
- crates/api-core/src/handlers/switch.rs
Comment on lines
+90
to
+102
| sqlx::query_as( | ||
| r#" | ||
| SELECT m.id, i.id | ||
| FROM machines m | ||
| INNER JOIN instances i ON i.machine_id = m.id | ||
| WHERE m.rack_id = $1 | ||
| AND NOT starts_with(m.id, $2) | ||
| ORDER BY m.id | ||
| "#, | ||
| ) | ||
| .bind(rack_id) | ||
| .bind(MachineType::Dpu.id_prefix()) | ||
| .fetch_all(txn) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'find_assigned_hosts_in_rack|set_decommission_requested|INSERT INTO instances|UPDATE instances.*machine_id|FOR UPDATE|FOR NO KEY UPDATE|rack_id' \
crates/api-core crates/api-db --glob '*.rs' --glob '*.sql'Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target helper and caller ---'
rg -n -C 18 'find_assigned_hosts_in_rack|set_decommission_requested' \
crates/api-db/src/managed_host.rs crates/api-core/src/handlers/switch.rs
printf '%s\n' '--- instance assignment writes and allocation paths ---'
rg -l 'INSERT INTO instances|UPDATE instances|machine_id' crates/api-core crates/api-db \
--glob '*.rs' --glob '*.sql' |
sort -u |
while IFS= read -r file; do
if rg -n 'INSERT INTO instances|UPDATE instances|machine_id' "$file" >/dev/null; then
printf '\n### %s\n' "$file"
rg -n -C 12 'INSERT INTO instances|UPDATE instances|machine_id' "$file"
fi
done
printf '%s\n' '--- locking and transaction boundaries in candidate files ---'
rg -n -C 10 'FOR UPDATE|FOR NO KEY UPDATE|FOR SHARE|FOR KEY SHARE|pool\.begin|begin\(\)|commit\(\)|fetch_(one|optional|all)|execute' \
crates/api-core crates/api-db --glob '*.rs' --glob '*.sql' |
rg 'managed_host|instance|machine|rack|switch|FOR UPDATE|FOR NO KEY UPDATE|pool\.begin|begin\(\)|commit\(\)' \
--context 10Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- helper ---'
rg -n -C 25 'find_assigned_hosts_in_rack' crates/api-db/src/managed_host.rs crates/api-db/src
printf '%s\n' '--- switch caller ---'
rg -n -C 35 'set_decommission_requested|find_assigned_hosts_in_rack' \
crates/api-core/src/handlers/switch.rs crates/api-core/src
printf '%s\n' '--- production instance writes ---'
rg -n -C 18 'INSERT INTO instances|UPDATE instances|instances\s*\(' \
crates/api-core/src crates/api-db/src \
--glob '*.rs' --glob '*.sql' \
--glob '!tests/**' --glob '!**/tests/**' \
--glob '!**/*test*.rs' \
| head -n 1200
printf '%s\n' '--- production locking clauses ---'
rg -n -C 12 'FOR UPDATE|FOR NO KEY UPDATE|FOR SHARE|FOR KEY SHARE' \
crates/api-core/src crates/api-db/src \
--glob '*.rs' --glob '*.sql' \
--glob '!tests/**' --glob '!**/tests/**' \
--glob '!**/*test*.rs' \
| head -n 1200Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- allocation call graph ---'
rg -n -C 20 'batch_allocate_instances|allocate_instance|allocate\(|create_instance|db::instance::create|instance::create|machine_id.*instance|instance.*machine_id' \
crates/api-core/src/instance.rs crates/api-core/src/handlers/instance.rs crates/api-db/src/instance.rs
printf '%s\n' '--- instance creation definitions ---'
rg -n -C 30 'pub async fn (create|allocate)|async fn (create|allocate)|INSERT INTO instances' \
crates/api-db/src/instance.rs crates/api-core/src/instance.rs crates/api-core/src/handlers/instance.rs
printf '%s\n' '--- machine selection and locking in allocation files ---'
rg -n -C 18 'FOR UPDATE|FOR NO KEY UPDATE|FOR SHARE|lock|available|candidate|machine_id|rack_id' \
crates/api-core/src/instance.rs crates/api-core/src/handlers/instance.rs crates/api-db/src/instance.rs \
| head -n 1800Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- instance module files ---'
fd -t f 'instance' crates/api-core/src crates/api-db/src | sort
printf '%s\n' '--- allocation definitions ---'
rg -n -C 35 'pub async fn (allocate_instance|batch_allocate_instances)|async fn (allocate_instance|batch_allocate_instances)|db::machine::find_one|MachineSearchConfig|for_update' \
crates/api-core/src --glob '*.rs' \
| head -n 2200
printf '%s\n' '--- instance insert/create calls ---'
rg -n -C 25 'INSERT INTO instances|db::instance::create|instance::create|create_instance' \
crates/api-core/src crates/api-db/src --glob '*.rs' --glob '*.sql' \
| head -n 1800Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- files containing allocation functions ---'
rg -l 'allocate_instance|batch_allocate_instances' crates/api-core/src --glob '*.rs' | sort
printf '%s\n' '--- exact allocation symbols and nearby code ---'
for file in $(rg -l 'allocate_instance|batch_allocate_instances' crates/api-core/src --glob '*.rs' | sort); do
printf '\n### %s\n' "$file"
rg -n -C 12 'allocate_instance|batch_allocate_instances' "$file"
done
printf '%s\n' '--- exact instance inserts in production source ---'
rg -n -C 12 'INSERT INTO instances' crates/api-core/src --glob '*.rs' --glob '!tests/**' --glob '!**/tests/**'Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- batch allocation implementation ---'
sed -n '1340,1585p' crates/api-core/src/instance/mod.rs
printf '%s\n' '--- machine lookup and row-lock configuration in allocation implementation ---'
rg -n -C 18 'find_one|MachineSearchConfig|for_update|find_by_machine|create\(|insert|txn_begin|commit' \
crates/api-core/src/instance/mod.rs | head -n 1800
printf '%s\n' '--- database instance creation callers outside tests ---'
rg -n -C 18 'db::instance::create|instance::create|INSERT INTO instances' \
crates/api-core/src crates/api-db/src --glob '*.rs' --glob '!tests/**' --glob '!**/tests/**' \
| head -n 1600Repository: NVIDIA/infra-controller
Length of output: 50379
Serialize switch decommission checks with instance allocation
Instance allocation locks candidate machines rows before inserting into instances, but this query does not. Add FOR UPDATE OF m so the assignment transaction cannot commit between this check and set_decommission_requested; retain the locks until commit.
🤖 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/managed_host.rs` around lines 90 - 102, Update the machine
query in the decommission-check flow to append FOR UPDATE OF m after its
ordering clause, locking the selected machine rows through the transaction until
commit. Preserve the existing filters, ordering, bindings, and fetch behavior.
Source: Path instructions
spydaNVIDIA
reviewed
Aug 7, 2026
| } | ||
|
|
||
| let mut txn = api.txn_begin().await?; | ||
| let exists = sqlx::query_scalar::<_, SwitchId>( |
Contributor
There was a problem hiding this comment.
doesnt find_by_id below do this already?
spydaNVIDIA
reviewed
Aug 7, 2026
| txn: &mut PgConnection, | ||
| switch_id: SwitchId, | ||
| ) -> Result<Switch, CarbideError> { | ||
| let exists = sqlx::query_scalar::<_, SwitchId>( |
Contributor
There was a problem hiding this comment.
ditto here, I think we are already doing this in line 346?
spydaNVIDIA
reviewed
Aug 7, 2026
| switch.id | ||
| )) | ||
| })?; | ||
| let expected_switch = db::expected_switch::find_by_bmc_mac_address(txn, bmc_mac_address) |
Contributor
There was a problem hiding this comment.
We should be able to get this from switch.nvos_info instead
spydaNVIDIA
reviewed
Aug 7, 2026
| Ok(switch) | ||
| } | ||
|
|
||
| async fn switch_endpoint_macs( |
spydaNVIDIA
reviewed
Aug 7, 2026
|
|
||
| // Secret-store operations cannot participate in the database transaction. Do them first so a | ||
| // failure leaves the terminal switch row available for a safe retry. | ||
| let bmc_mac_address = endpoint_macs[0]; |
Contributor
There was a problem hiding this comment.
Why not just get this from the switch directly? switch.bmc_info/bmc_mac_addr
spydaNVIDIA
reviewed
Aug 7, 2026
| })?; | ||
|
|
||
| let mut txn = api.txn_begin().await?; | ||
| let switch = find_decommissioned_switch_for_update(&mut txn, switch_id).await?; |
Contributor
There was a problem hiding this comment.
We already lookup the switch earlier, why look it up again?
spydaNVIDIA
reviewed
Aug 7, 2026
| time_in_state, | ||
| ), | ||
| SwitchControllerState::Ready => StateSla::no_sla(), | ||
| SwitchControllerState::Decommissioning { .. } => StateSla::no_sla(), |
spydaNVIDIA
reviewed
Aug 7, 2026
| rpc DeleteSwitch(SwitchDeletionRequest) returns (SwitchDeletionResult); | ||
| // Starts managed-switch decommissioning. The switch must be Ready and the | ||
| // Component Manager switch backend must be RMS. | ||
| rpc DecommissionSwitch(common.SwitchId) returns (google.protobuf.Empty); |
Contributor
There was a problem hiding this comment.
should we create a req struct to accomodate future fields?
spydaNVIDIA
reviewed
Aug 7, 2026
| let _secret_version_metadata = vault_response.inspect_err(|err| { | ||
| record_vault_credentials_delete_error(err); | ||
| })?; | ||
| match vault_response { |
Contributor
There was a problem hiding this comment.
Why are we modifying this func in this MR?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds managed-switch decommissioning, including Core API support, state-controller orchestration, Flow integration, and admin CLI commands.
Decommissioning workflow
API and CLI
DeleteDecommissionedSwitchRPC and authorization rules.Decommissioned.nico-admin-cli managed-switch decommissionnico-admin-cli managed-switch delete-decommissionedFlow integration
Decommissionedon completion.Validation
I have no managed switch in my dev environment so this code path is untested. Will be able to test in other sites that do have racks once this is merged. It has very similar logic to that of managed host decommissioning which does successfully work in my dev environment, so hopefully it will need minimal changes to become ready.
Related issues
Closes #3824
Type of Change
Breaking Changes
Testing