feat: Managed power shelf decommissioning - #4680
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:
Summary by CodeRabbit
WalkthroughThe change adds managed power-shelf decommissioning and permanent deletion. It adds lifecycle states, database tracking, controller cleanup, Forge RPCs, Flow integration, authorization, Admin CLI commands, documentation, and integration tests. ChangesPower shelf lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant FlowGrpcClient
participant ForgeApi
participant PowerShelfHandler
participant PowerShelfController
participant Database
participant BMCAndDHCPSystems
AdminCLI->>FlowGrpcClient: DecommissionPowerShelf(PowerShelfId)
FlowGrpcClient->>ForgeApi: DecommissionPowerShelf(PowerShelfId)
ForgeApi->>PowerShelfHandler: record decommission request
PowerShelfHandler->>Database: set decommission_requested
PowerShelfController->>Database: transition to Preparing
PowerShelfController->>BMCAndDHCPSystems: suppress BMC and verify DHCP release
PowerShelfController->>Database: transition to Decommissioned
AdminCLI->>ForgeApi: DeleteDecommissionedPowerShelf(PowerShelfId)
ForgeApi->>PowerShelfHandler: delete decommissioned shelf
PowerShelfHandler->>Database: remove interfaces, suppressions, and shelf
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/api-core/src/tests/power_shelf_state_controller/decommissioning.rs (2)
238-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the retained boot record cleanup.
These assertions cover the shelf row, the interfaces, and both suppression subsystems. They do not cover retained boot records.
db::machine_interface::deletewrites aretained_boot_interfacesrow only for an interface that carries aboot_interface_id.seed_pmc_endpointat Lines 74-85 does not set that column, so thedb::retained_boot_interface::take_by_maccall at Line 191 ofcrates/api-core/src/handlers/power_shelf.rsis a no-op in this test. Removing that call would not fail the suite, even though the PR objectives state that permanent deletion removes retained boot records.Set
boot_interface_idin the fixture and assert the retained record is absent after deletion.💚 Proposed fixture and assertion change
let interface_id: MachineInterfaceId = sqlx::query_scalar( "INSERT INTO machine_interfaces (power_shelf_id, association_type, segment_id, mac_address, - primary_interface, hostname, interface_type) - VALUES ($1, 'PowerShelf', $2, $3, false, 'pmc', 'Bmc') + primary_interface, hostname, interface_type, boot_interface_id) + VALUES ($1, 'PowerShelf', $2, $3, false, 'pmc', 'Bmc', 'NIC.Slot.1-1') RETURNING id", )Then add the assertion after Line 261:
assert!( db::bmc_suppression::find(&pool, pmc_mac, BmcSuppressionSubsystem::Dhcp) .await? .is_none() ); + assert!( + db::retained_boot_interface::find_by_mac(conn.as_mut(), pmc_mac) + .await? + .is_none() + ); Ok(())Confirm the exact
retained_boot_interfaceread helper name before applying.🤖 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/power_shelf_state_controller/decommissioning.rs` around lines 238 - 263, Update the decommissioning test fixture created by seed_pmc_endpoint to assign a boot_interface_id, ensuring interface deletion creates a retained boot record. After delete_decommissioned_power_shelf completes, use the existing db::retained_boot_interface read helper to assert that the retained record for the fixture’s boot interface or MAC is absent, alongside the existing cleanup assertions.
104-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
NotFoundpath.This test pins the
FailedPreconditioncontract for both RPCs. Neither new handler has coverage for an unknownPowerShelfId, which returnsCarbideError::NotFoundErrorat Lines 110-113 and Lines 163-166 ofcrates/api-core/src/handlers/power_shelf.rs.The
NotFoundcode is part of the operator-facing contract for the CLI and the Flow client. A test prevents a silent regression toInternalorFailedPrecondition.If you add these cases, use
scenarios!withOutcomeover the shelf-identifier input, because the same two operations would then run with different inputs.As per coding guidelines: "Use a table whenever two or more tests invoke the same operation with different inputs, but keep genuinely distinct tests standalone."
🤖 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/power_shelf_state_controller/decommissioning.rs` around lines 104 - 130, Extend decommission_requires_ready_power_shelf coverage to include unknown PowerShelfId inputs for both decommission_power_shelf and delete_decommissioned_power_shelf, asserting Code::NotFound. Refactor the shared operation cases into scenarios! with Outcome over the shelf identifier, while retaining the existing initializing-shelf FailedPrecondition assertions.Source: Coding guidelines
crates/api-core/src/handlers/power_shelf.rs (1)
145-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the BMC MAC resolution rule onto
PowerShelf.
power_shelf_bmc_macduplicatesbmc_macincrates/power-shelf-controller/src/decommissioning.rs(Lines 33-43). Both apply the same rule: preferbmc_info.mac, then fall back tobmc_mac_address. The two copies differ only in the failure type.The rule is a property of
PowerShelf. Define it once as a method on the model incrates/api-model, then let each caller adapt theOptionto its own error type. This prevents the two copies from drifting when BMC identity handling changes.As per coding guidelines: "When behavior primarily operates on one type, define it as a method on that type; use free functions only for multi-type logic or utilities with no natural owner."
♻️ Proposed consolidation
Add the method to
PowerShelfincrates/api-model/src/power_shelf/mod.rs:impl PowerShelf { /// Resolves the PMC MAC, preferring the explored BMC info over the /// directly recorded address. pub fn bmc_mac(&self) -> Option<mac_address::MacAddress> { self.bmc_info .as_ref() .and_then(|info| info.mac) .or(self.bmc_mac_address) } }Then remove the local helper:
-fn power_shelf_bmc_mac(power_shelf: &PowerShelf) -> Option<mac_address::MacAddress> { - power_shelf - .bmc_info - .as_ref() - .and_then(|info| info.mac) - .or(power_shelf.bmc_mac_address) -}Update the two call sites at Lines 168 and 194 to
power_shelf.bmc_mac(), and havecrates/power-shelf-controller/src/decommissioning.rsmap theOptiontoStateHandlerError::MissingData.🤖 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/power_shelf.rs` around lines 145 - 151, Move the BMC MAC resolution rule into a `PowerShelf::bmc_mac` method in the model, preferring `bmc_info.mac` and falling back to `bmc_mac_address`. Remove the local `power_shelf_bmc_mac` helper and update its call sites to use the method; likewise update `decommissioning.rs` to call `PowerShelf::bmc_mac()` and adapt the returned `Option` to `StateHandlerError::MissingData`.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-core/src/auth/internal_rbac_rules.rs`:
- Around line 728-731: Update the DeleteDecommissionedPowerShelf permission
grant in the RBAC rules to remove Flow, leaving only ForgeAdminCLI and
Machineatron as authorized principals.
In `@crates/api-db/src/power_shelf.rs`:
- Around line 343-356: Update set_decommission_requested to make the Ready-state
validation atomic with the update: lock the selected row or add the expected
controller_state_version to the UPDATE predicate and bind that expected version.
Ensure the update succeeds only when the row still matches the validated state,
while preserving the existing error handling and return behavior.
In `@crates/api-model/src/power_shelf/mod.rs`:
- Around line 326-328: The SLA mapping currently disables overdue detection for
active decommissioning states. Update the match handling in the power-shelf
state SLA definition so Preparing and VerifyingDhcpRelease use bounded SLA
policies, while Decommissioned continues using StateSla::no_sla(); add
table-driven rows covering both active states and their expected policy.
In
`@docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md`:
- Around line 15-17: The power-shelf lifecycle contract is missing from the
decommission and delete documentation. Update the canonical generated sources
under crates/admin-cli/src/power_shelf/ to document the Ready precondition,
asynchronous decommission sequence, waits, errors, retries, and
Decommissioned-only deletion cleanup order; regenerate both
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md:15-17
and
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.md:15-17,
then validate the rendered output.
---
Nitpick comments:
In `@crates/api-core/src/handlers/power_shelf.rs`:
- Around line 145-151: Move the BMC MAC resolution rule into a
`PowerShelf::bmc_mac` method in the model, preferring `bmc_info.mac` and falling
back to `bmc_mac_address`. Remove the local `power_shelf_bmc_mac` helper and
update its call sites to use the method; likewise update `decommissioning.rs` to
call `PowerShelf::bmc_mac()` and adapt the returned `Option` to
`StateHandlerError::MissingData`.
In `@crates/api-core/src/tests/power_shelf_state_controller/decommissioning.rs`:
- Around line 238-263: Update the decommissioning test fixture created by
seed_pmc_endpoint to assign a boot_interface_id, ensuring interface deletion
creates a retained boot record. After delete_decommissioned_power_shelf
completes, use the existing db::retained_boot_interface read helper to assert
that the retained record for the fixture’s boot interface or MAC is absent,
alongside the existing cleanup assertions.
- Around line 104-130: Extend decommission_requires_ready_power_shelf coverage
to include unknown PowerShelfId inputs for both decommission_power_shelf and
delete_decommissioned_power_shelf, asserting Code::NotFound. Refactor the shared
operation cases into scenarios! with Outcome over the shelf identifier, while
retaining the existing initializing-shelf FailedPrecondition assertions.
🪄 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: 5638f7c0-c109-4604-ab42-73c9c7f6fbd6
⛔ 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 (30)
crates/admin-cli/src/power_shelf/decommission/args.rscrates/admin-cli/src/power_shelf/decommission/cmd.rscrates/admin-cli/src/power_shelf/decommission/mod.rscrates/admin-cli/src/power_shelf/delete_decommissioned/args.rscrates/admin-cli/src/power_shelf/delete_decommissioned/cmd.rscrates/admin-cli/src/power_shelf/delete_decommissioned/mod.rscrates/admin-cli/src/power_shelf/mod.rscrates/admin-cli/src/power_shelf/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/tests/power_shelf_state_controller/decommissioning.rscrates/api-core/src/tests/power_shelf_state_controller/mod.rscrates/api-db/migrations/20260806120000_power_shelf_decommission_requested.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/power_shelf.rscrates/api-model/src/power_shelf/mod.rscrates/power-shelf-controller/src/decommissioning.rscrates/power-shelf-controller/src/handler.rscrates/power-shelf-controller/src/io.rscrates/power-shelf-controller/src/lib.rscrates/power-shelf-controller/src/ready.rscrates/rpc/proto/forge.protodocs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.mddocs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.mddocs/manuals/nico-admin-cli/commands/power-shelf/power-shelf.mdrest-api/flow/internal/nicoapi/grpc.gorest-api/flow/internal/nicoapi/grpc_test.gorest-api/flow/internal/nicoapi/mod.gorest-api/proto/core/src/v1/nico_nico.proto
💤 Files with no reviewable changes (1)
- rest-api/flow/internal/nicoapi/mod.go
| x.perm( | ||
| "DeleteDecommissionedPowerShelf", | ||
| vec![ForgeAdminCLI, Machineatron, Flow], | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find Flow callers of the two new power-shelf lifecycle RPCs.
set -uo pipefail
echo "=== Go references in rest-api/flow ==="
rg -n -C 3 'DeleteDecommissionedPowerShelf|DecommissionPowerShelf' rest-api/flow
echo "=== Go references repository-wide ==="
rg -n --type=go -C 2 'DeleteDecommissionedPowerShelf'
echo "=== Flow interface declarations ==="
fd -t f 'mod.go' rest-api/flow --exec rg -n -C 4 'PowerShelf'Repository: NVIDIA/infra-controller
Length of output: 7703
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== RBAC rule context ==="
sed -n '700,745p' crates/api-core/src/auth/internal_rbac_rules.rs
sed -n '285,315p' crates/api-core/src/auth/internal_rbac_rules.rs
echo "=== All repository references to DeleteDecommissionedPowerShelf ==="
rg -n -C 3 'DeleteDecommissionedPowerShelf' . || true
echo "=== All repository references to power-shelf delete operations ==="
rg -n -C 2 'DeletePowerShelf|AdminForceDeletePowerShelf|DecommissionPowerShelf' . || true
echo "=== RPC declarations and implementations ==="
rg -n -C 4 'rpc (DeleteDecommissionedPowerShelf|DeletePowerShelf|AdminForceDeletePowerShelf|DecommissionPowerShelf)|DeleteDecommissionedPowerShelf\(' . || trueRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Non-generated application references ==="
rg -n --glob '!**/gen/**' --glob '!**/*.proto' \
'DeleteDecommissionedPowerShelf|delete_decommissioned_power_shelf|deleteDecommissionedPowerShelf|decommissioned power shelf' \
crates rest-api || true
echo "=== Flow references, excluding mocks and generated code ==="
rg -n --glob '!**/gen/**' \
'DeleteDecommissionedPowerShelf|delete_decommissioned_power_shelf|DeletePowerShelf|AdminForceDeletePowerShelf' \
rest-api/flow || true
echo "=== Power-shelf deletion implementations and authorization checks ==="
rg -n -C 4 --glob '*.rs' \
'delete_decommissioned|DeleteDecommissioned|admin_force_delete_power_shelf|delete_power_shelf|PowerShelf.*delete|delete.*PowerShelf' \
crates || trueRepository: NVIDIA/infra-controller
Length of output: 27312
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Permanent deletion handler ==="
sed -n '135,225p' crates/api-core/src/handlers/power_shelf.rs
echo "=== RBAC enforcement references ==="
rg -n -C 4 \
'internal_rbac_rules|check_permission|permission|authorize|authorization' \
crates/api-core/src/auth crates/api-core/src | head -n 220
echo "=== Direct application callers of the permanent-deletion API ==="
rg -n --glob '*.rs' --glob '!**/gen/**' \
'delete_decommissioned_power_shelf\(' crates rest-api | \
grep -v 'fn delete_decommissioned_power_shelf' || trueRepository: NVIDIA/infra-controller
Length of output: 20935
Restrict DeleteDecommissionedPowerShelf to admin principals.
Only the admin CLI and tests call this permanent-deletion RPC. Remove Flow from the grant list. The RPC deletes credentials, interfaces, retained boot records, suppression records, and the shelf.
🤖 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 728 - 731,
Update the DeleteDecommissionedPowerShelf permission grant in the RBAC rules to
remove Flow, leaving only ForgeAdminCLI and Machineatron as authorized
principals.
| ## DESCRIPTION | ||
|
|
||
| Start decommissioning a managed power shelf |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target files ---'
for f in \
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md \
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.md
do
if [ -f "$f" ]; then
wc -l "$f"
cat -n "$f"
else
printf 'MISSING %s\n' "$f"
fi
done
printf '%s\n' '--- related power-shelf documentation and source references ---'
rg -n -i \
'decommission(ed|ing)?|power shelf|power_shelf|PMC reset|DHCP|boot record|suppression|credential|interface' \
docs crates rest-api helm helm-prereqs deploy README.md \
--glob '*.md' --glob '*.rs' --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.toml' \
| head -n 400Repository: NVIDIA/infra-controller
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate generated-doc sources ---'
rg -n -i \
'power-shelf-decommission|power-shelf-delete-decommissioned|delete-decommissioned|Start decommissioning a managed power shelf|Permanently delete a decommissioned managed power shelf' \
. --glob '!docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md' \
--glob '!docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.md' \
| head -n 300
printf '%s\n' '--- power-shelf file inventory ---'
git ls-files | rg -i 'power.?shelf|power_shelf|nico-admin-cli' | head -n 300Repository: NVIDIA/infra-controller
Length of output: 22610
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CLI command definitions ---'
for f in \
crates/admin-cli/src/power_shelf/mod.rs \
crates/admin-cli/src/power_shelf/decommission/mod.rs \
crates/admin-cli/src/power_shelf/decommission/args.rs \
crates/admin-cli/src/power_shelf/decommission/cmd.rs \
crates/admin-cli/src/power_shelf/delete_decommissioned/mod.rs \
crates/admin-cli/src/power_shelf/delete_decommissioned/args.rs \
crates/admin-cli/src/power_shelf/delete_decommissioned/cmd.rs
do
printf '\n### %s\n' "$f"
cat -n "$f"
done
printf '%s\n' '--- state-machine documentation ---'
wc -l docs/architecture/state_machines/power_shelf.md
cat -n docs/architecture/state_machines/power_shelf.md
printf '%s\n' '--- controller outlines ---'
ast-grep outline crates/power-shelf-controller/src/decommissioning.rs
ast-grep outline crates/power-shelf-controller/src/deleting.rs
ast-grep outline crates/api-core/src/handlers/power_shelf.rsRepository: NVIDIA/infra-controller
Length of output: 23436
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- decommission/delete implementation and tests ---'
for f in \
crates/power-shelf-controller/src/decommissioning.rs \
crates/power-shelf-controller/src/deleting.rs \
crates/api-core/src/tests/power_shelf_state_controller/decommissioning.rs \
crates/api-core/tests/integration/power_shelf_delete.rs \
crates/api-db/migrations/20260806120000_power_shelf_decommission_requested.sql
do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
wc -l "$f"
cat -n "$f"
fi
doneRepository: NVIDIA/infra-controller
Length of output: 28547
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- lifecycle symbols and side effects ---'
rg -n -i \
'Ready|Decommissioned|Decommissioning|Deleting|PMC|reset|DHCP|acknowledge|boot|suppression|credential|interface|retry|failed|error|transition|decommission_requested' \
crates/power-shelf-controller/src \
crates/api-core/src/handlers/power_shelf.rs \
crates/api-core/src/tests/power_shelf_state_controller \
crates/api-core/tests/integration/power_shelf_delete.rs \
docs/architecture/state_machines/power_shelf.md \
| head -n 700
printf '%s\n' '--- all power-shelf state declarations/usages ---'
rg -n \
'enum PowerShelfState|PowerShelfState::|power_shelf_state|decommission' \
crates/api-model crates/api-db crates/api-core/src/handlers/power_shelf.rs \
crates/power-shelf-controller/src \
| head -n 700Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- API lifecycle handlers ---'
cat -n crates/api-core/src/handlers/power_shelf.rs | sed -n '95,215p'
printf '%s\n' '--- database deletion implementation ---'
rg -n 'fn final_delete|final_delete|take_by_mac|delete_many|delete_bmc_root_credentials_by_mac' \
crates/api-db crates/api-core/src/handlers crates --glob '*.rs' \
| head -n 120
printf '%s\n' '--- relevant deletion helpers ---'
for f in \
crates/api-db/src/power_shelf.rs \
crates/api-db/src/machine_interface.rs \
crates/api-db/src/retained_boot_interface.rs \
crates/api-db/src/bmc_suppression.rs \
crates/api-core/src/handlers/credential.rs
do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
rg -n -A 45 -B 8 'final_delete|take_by_mac|delete_many|delete_bmc_root_credentials_by_mac' "$f" || true
fi
done
printf '%s\n' '--- controller state model and ready transition ---'
rg -n -A 35 -B 10 \
'enum PowerShelfControllerState|Decommissioned|decommission_requested|handle_ready' \
crates/api-model/src/power_shelf crates/power-shelf-controller/src/ready.rs \
crates/power-shelf-controller/src/handler.rs crates/api-db/src/power_shelf.rs \
| head -n 500Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
handler = Path("crates/api-core/src/handlers/power_shelf.rs").read_text()
decommissioning = Path("crates/power-shelf-controller/src/decommissioning.rs").read_text()
checks = {
"decommission requires Ready": "must be Ready to be decommissioned" in handler,
"decommission request is persisted": "set_decommission_requested" in handler,
"waits for Site Explorer acknowledgement": "waiting for Site Explorer suppression acknowledgement" in decommissioning,
"performs BMC factory reset": "bmc_reset_to_defaults" in decommissioning,
"suppresses DHCP and records interface deletion": (
"BmcSuppressionSubsystem::Dhcp" in decommissioning
and "machine_interface::record_deletion" in decommissioning
),
"waits for DHCP acknowledgement": "WaitingForBmcDhcpAcknowledgement" in decommissioning,
"reaches Decommissioned after DHCP acknowledgement": (
"PowerShelfControllerState::Decommissioned" in decommissioning
),
"deletion requires Decommissioned": "must be Decommissioned before deletion" in handler,
"credentials are deleted before the transaction": (
"delete_bmc_root_credentials_by_mac" in handler
and handler.index("delete_bmc_root_credentials_by_mac")
< handler.index("db::machine_interface::find_by_power_shelf_id")
),
"deletion removes interfaces and retained boot records": (
"machine_interface::delete" in handler
and "retained_boot_interface::take_by_mac" in handler
),
"deletion removes both suppression records": (
handler.count("db::bmc_suppression::delete_many") >= 2
),
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
# Show the source ordering that determines the retry-safe boundary.
start = handler.index("pub(crate) async fn delete_decommissioned_power_shelf")
end = handler.find("\npub(crate)", start + 1)
if end == -1:
end = min(len(handler), start + 5000)
print("\n--- delete handler operation order ---")
for i, line in enumerate(handler[start:end].splitlines(), 1):
if any(term in line for term in (
"find_by_id", "require_decommissioned", "delete_bmc_root",
"begin", "machine_interface::delete", "retained_boot_interface",
"bmc_suppression::delete_many", "final_delete", "commit",
)):
print(f"{i:03}: {line.strip()}")
PYRepository: NVIDIA/infra-controller
Length of output: 1530
Document the managed power-shelf lifecycle contract.
- Document the
Readyprecondition and asynchronous sequence:Ready→Preparing→ Site Explorer suppression acknowledgement → BMC factory reset → BMC DHCP suppression acknowledgement →Decommissioned. - Document waits, errors, and retry behavior.
- Document that deletion requires
Decommissionedand removes BMC credentials, interfaces, retained boot-interface records, suppression records, and the shelf row. State that credential deletion occurs before database cleanup and that later failures leave the shelf available for retry. - Update the canonical generated sources under
crates/admin-cli/src/power_shelf/, regenerate both pages, and validate the rendered output.
📍 Affects 2 files
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md#L15-L17(this comment)docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.md#L15-L17
🤖 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/power-shelf/power-shelf-decommission.md`
around lines 15 - 17, The power-shelf lifecycle contract is missing from the
decommission and delete documentation. Update the canonical generated sources
under crates/admin-cli/src/power_shelf/ to document the Ready precondition,
asynchronous decommission sequence, waits, errors, retries, and
Decommissioned-only deletion cleanup order; regenerate both
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-decommission.md:15-17
and
docs/manuals/nico-admin-cli/commands/power-shelf/power-shelf-delete-decommissioned.md:15-17,
then validate the rendered output.
Source: Coding guidelines
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:27:03 UTC | Commit: e2b56ec |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4680.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/power_shelf.rs (1)
356-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
query_scalarfor the single returned column.
PowerShelfIdsupports both SQLx scalar decoding andFromRow, soquery_asis valid.query_scalarmapsRETURNING iddirectly and matches existing usage for scalarPowerShelfIdresults.🤖 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/power_shelf.rs` around lines 356 - 359, Update the query construction in the PowerShelf update flow to use SQLx’s query_scalar API for the single returned id column instead of query_as::<_, PowerShelfId>. Preserve the existing bindings and fetch_optional behavior, and continue returning the scalar PowerShelfId result.Source: 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.
Nitpick comments:
In `@crates/api-db/src/power_shelf.rs`:
- Around line 356-359: Update the query construction in the PowerShelf update
flow to use SQLx’s query_scalar API for the single returned id column instead of
query_as::<_, PowerShelfId>. Preserve the existing bindings and fetch_optional
behavior, and continue returning the scalar PowerShelfId result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f1f9538-5a65-44d0-bcc8-30848f120e74
📒 Files selected for processing (3)
crates/api-core/src/handlers/power_shelf.rscrates/api-db/migrations/20260806120001_power_shelf_decommission_requested.sqlcrates/api-db/src/power_shelf.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/handlers/power_shelf.rs
| Ok(Response::new(())) | ||
| } | ||
|
|
||
| fn require_decommissioned(power_shelf: &PowerShelf) -> Result<(), CarbideError> { |
There was a problem hiding this comment.
This helper func seems overdone. Dont think it adds much and it only has one caller.
| ))) | ||
| } | ||
|
|
||
| fn power_shelf_bmc_mac(power_shelf: &PowerShelf) -> Option<mac_address::MacAddress> { |
There was a problem hiding this comment.
ditto here: this helper function is not adding much
| let mut txn = api.txn_begin().await?; | ||
| let power_shelf = db_power_shelf::find_by_id(&mut txn, &power_shelf_id) | ||
| .await? | ||
| .ok_or_else(|| CarbideError::NotFoundError { | ||
| kind: "power_shelf", | ||
| id: power_shelf_id.to_string(), | ||
| })?; | ||
| require_decommissioned(&power_shelf)?; |
There was a problem hiding this comment.
Isnt this a duplicate of lines 165-172 from above?
| // Secret-store operations cannot participate in the database transaction. | ||
| // Delete the credential first so a failure leaves the terminal shelf | ||
| // available for a safe retry. | ||
| if let Some(bmc_mac) = bmc_mac { |
There was a problem hiding this comment.
Shouldnt we return an error if the bmc mac of a powershelf cant be found?
| db::retained_boot_interface::take_by_mac(&mut txn, interface.mac_address, None).await?; | ||
| } | ||
|
|
||
| if let Some(bmc_mac) = power_shelf_bmc_mac(&power_shelf) { |
There was a problem hiding this comment.
we are extracting the bmc mac from the powershelf twice in this func --we have already done this above in line 179
| } | ||
|
|
||
| /// Find all machine interfaces associated with a power shelf. | ||
| pub async fn find_by_power_shelf_id( |
There was a problem hiding this comment.
Is this helper function overkill? Its a very thin wrapper.
There was a problem hiding this comment.
I'm gonna leave it in but change it to follow the shape of the function above
| reprovisioning_state: ReProvisioningState, | ||
| }, | ||
| /// Site Explorer is being suppressed before the destructive reset. | ||
| Preparing, |
There was a problem hiding this comment.
I think this state's name needs to give an operator better context on the semantic nature of the operations being done. How about something like SuppressSiteExplorer.
| /// Site Explorer is being suppressed before the destructive reset. | ||
| Preparing, | ||
| /// The controller is resetting the PMC and waiting for DHCP release. | ||
| VerifyingDhcpRelease { | ||
| verifying_state: PowerShelfVerifyingDhcpReleaseState, | ||
| }, | ||
| /// Terminal state: the power shelf has been removed from managed service. | ||
| Decommissioned, |
There was a problem hiding this comment.
Doesnt it make more sense to have a top level Decommission state and then have these three as sub-states
|
|
||
| if state.decommission_requested { | ||
| let mut txn = ctx.services.db_pool.begin().await?; | ||
| db_power_shelf::clear_decommission_requested(txn.as_mut(), *power_shelf_id).await?; |
There was a problem hiding this comment.
Do you need to care about clearing this flag if the powershelf object is going to be deleted from the table?
There was a problem hiding this comment.
Doesn't hurt, just following the design pattern for these flags.
|
|
||
| use crate::context::PowerShelfStateHandlerContextObjects; | ||
|
|
||
| fn bmc_mac(power_shelf: &PowerShelf) -> Result<mac_address::MacAddress, StateHandlerError> { |
There was a problem hiding this comment.
nit: this seems like a very thin wrapper. keep it if you think if its helpful, but it might be overkill
| verifying_state, | ||
| PowerShelfVerifyingDhcpReleaseState::FactoryResetBmc | ||
| ) { | ||
| return handle_factory_reset_bmc(&power_shelf.id, power_shelf, ctx).await; |
There was a problem hiding this comment.
IMO this is where we should delete the credentials for the PMC from vault--after doing a factory reset
| }, | ||
| ) | ||
| .await?; | ||
| db::machine_interface::record_deletion(&mut txn).await?; |
There was a problem hiding this comment.
why do we want to delete the interface here? Doesnt it make sense to do that as part of the delete decomissioned powershelf logic?
There was a problem hiding this comment.
This was a trick that I used to have the kea dhcp server restart. It doesn't actually delete the interface yet. I'll comment to make that more clear.
| })?; | ||
|
|
||
| let mut txn = ctx.services.db_pool.begin().await?; | ||
| db::bmc_suppression::upsert( |
There was a problem hiding this comment.
Should this be in a separate step? If for some reason, we fail to update the db, and re-execute this state, we will keep trying to factory reset the BMC in a loop.
| reason: format!("power shelf {power_shelf_id} is being decommissioned"), | ||
| }, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Im having a hard time following this logic. Arent we doing the same thing in in lines 106-115 in the handler for FactoryResetBMC?
There was a problem hiding this comment.
This is for suppressing site explorer (BmcSuppressionSubsystem::SiteExplorer), lines 106-115 is for DHCP. These need to be done at different times.
|
Why do we want to do this in two steps: 1) request decomission and 2) delete decomissioned powershelf? Wouldnt it make more sense to add the logic in (2) as the final/teminal step in the state machine once decomission is requested (1). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/src/managed_host.rs (1)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept a generic database reader.
This helper only reads data. Accept
impl DbReader<'_>so callers can use a pool or an existing transaction without requiring a mutablePgConnection.Proposed change
pub async fn has_instance_assigned_host_in_rack( - txn: &mut PgConnection, + txn: impl DbReader<'_>, rack_id: &RackId, ) -> Result<bool, DatabaseError> {As per coding guidelines, “For read-only database functions, accept
impl DbReaderso callers can use a pool without unnecessary transactions.”🤖 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 has_instance_assigned_host_in_rack to accept an impl DbReader<'_> instead of &mut PgConnection, and adjust its query usage as needed to work with the generic reader while preserving the existing read-only result behavior.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-db/src/managed_host.rs`:
- Around line 90-102: Serialize decommissioning and instance allocation by
acquiring the same rack-scoped or advisory lock in decommission_power_shelf and
batch_allocate_instances before checking or modifying related rows. Preserve the
existing DPU-machine query while ensuring both transactions hold the lock
through commit, and add a concurrent integration test proving allocation cannot
commit between the decommission check and update.
---
Nitpick comments:
In `@crates/api-db/src/managed_host.rs`:
- Around line 86-89: Update has_instance_assigned_host_in_rack to accept an impl
DbReader<'_> instead of &mut PgConnection, and adjust its query usage as needed
to work with the generic reader while preserving the existing read-only result
behavior.
🪄 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: 5ff5e426-0110-44f9-8032-a8334233335b
📒 Files selected for processing (3)
crates/api-core/src/handlers/power_shelf.rscrates/api-core/src/tests/power_shelf_state_controller/decommissioning.rscrates/api-db/src/managed_host.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/handlers/power_shelf.rs
| let query = r#" | ||
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| 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) | ||
| ) | ||
| "#; | ||
| sqlx::query_scalar(query) | ||
| .bind(rack_id) | ||
| .bind(MachineType::Dpu.id_prefix()) | ||
| .fetch_one(txn) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the transaction isolation used by the API lifecycle operation.
rg -n -C 5 'async fn txn_begin|fn txn_begin|txn_begin\(' crates/api-core/src crates/api-db/src || true
# Trace instance allocation and writes to the instances relation.
rg -n -C 8 -P 'INSERT\s+INTO\s+instances|allocate.*instance|create.*instance|instances.*machine_id|machine_id.*instances' \
crates/api-core/src crates/api-db/src || true
# Confirm the ordering of the decommission precondition and state update.
rg -n -C 8 'has_instance_assigned_host_in_rack|set_decommission_requested' \
crates/api-core/src/handlers crates/api-db/src || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- decommission call sites ---'
rg -n -C 20 'has_instance_assigned_host_in_rack|set_decommission_requested|decommission_power_shelf' \
crates/api-core/src crates/api-db/src \
| head -n 1200
printf '%s\n' '--- transaction creation and isolation ---'
rg -n -C 12 'begin\(\)|Transaction|IsolationLevel|REPEATABLE|SERIALIZABLE|FOR UPDATE|FOR NO KEY UPDATE|advisory_xact_lock' \
crates/api-core/src crates/api-db/src \
| head -n 1600
printf '%s\n' '--- instance allocation implementation ---'
rg -n -C 18 'pub async fn allocate_instance|async fn allocate_instance|INSERT INTO instances|insert_instance|create_instance' \
crates/api-core/src crates/api-db/src \
| head -n 2000Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact files ---'
rg -l 'has_instance_assigned_host_in_rack|set_decommission_requested|decommission_power_shelf' \
crates/api-core/src crates/api-db/src
printf '%s\n' '--- exact decommission references ---'
rg -n -C 35 'has_instance_assigned_host_in_rack|set_decommission_requested' \
crates/api-core/src/handlers crates/api-core/src/power_shelf* crates/api-db/src/managed_host.rs \
2>/dev/null | head -n 700
printf '%s\n' '--- allocation batch definition and transaction boundaries ---'
rg -n -C 35 'batch_allocate_instances|pub\(crate\) async fn batch_allocate_instances|async fn batch_allocate_instances' \
crates/api-core/src/instance/mod.rs crates/api-core/src/handlers/instance.rs \
| head -n 1000
printf '%s\n' '--- migration constraints and triggers for instances/machines/power shelves ---'
rg -n -i -C 8 'CREATE TABLE (instances|machines|power_shelves)|CREATE UNIQUE|FOREIGN KEY.*machine|REFERENCES machines|decommission_requested|instance.*machine' \
migrations crates 2>/dev/null | head -n 1600Repository: NVIDIA/infra-controller
Length of output: 12178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transaction helper ---'
rg -n -C 20 'txn_begin|IsolationLevel|SET TRANSACTION' crates/api-core/src || true
printf '%s\n' '--- batch allocation implementation ---'
rg -n -C 25 'batch_allocate_instances' crates/api-core/src/instance/mod.rs crates/api-core/src/handlers/instance.rs || true
printf '%s\n' '--- instance writes in allocation modules ---'
rg -n -C 18 'INSERT INTO instances|instances::create|instance::create|insert.*instance' \
crates/api-core/src/instance crates/api-core/src/handlers/instance.rs crates/api-db/src \
| head -n 1800 || true
printf '%s\n' '--- power shelf row locking and update ---'
rg -n -C 25 'pub async fn find_by_id|async fn find_by_id|set_decommission_requested|FOR UPDATE|FOR NO KEY UPDATE' \
crates/api-db/src/power_shelf.rs crates/api-core/src/handlers/power_shelf.rs || true
printf '%s\n' '--- relevant schema objects ---'
rg -n -i -C 12 'CREATE TABLE .*instances|CREATE TABLE .*machines|CREATE TABLE .*power_shelves|REFERENCES machines|REFERENCES power_shelves|decommission_requested' \
migrations crates/api-db 2>/dev/null | head -n 1800 || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- allocation transaction and host locks ---'
sed -n '1350,1660p' crates/api-core/src/instance/mod.rs
rg -n -C 12 'FOR UPDATE|lock.*machine|machine.*lock|batch.*lock' \
crates/api-core/src/instance/mod.rs crates/api-db/src/machine.rs crates/api-db/src/instance.rs
printf '%s\n' '--- transaction helper definition ---'
rg -n 'txn_begin' crates/api-core/src --glob '*.rs' | head -n 80
while IFS=: read -r file line rest; do
[ -n "${file:-}" ] || continue
start=$((line - 20)); [ "$start" -lt 1 ] && start=1
end=$((line + 45))
sed -n "${start},${end}p" "$file"
done < <(rg -n 'fn txn_begin' crates/api-core/src --glob '*.rs' | head -n 10)
printf '%s\n' '--- power shelf query/update definitions ---'
rg -n -C 30 'find_by_id|set_decommission_requested|FOR UPDATE|FOR NO KEY UPDATE' \
crates/api-db/src/power_shelf.rs
printf '%s\n' '--- schema file names and relevant constraints ---'
git ls-files | rg '(^|/)(migrations?|schema|.*\\.sql$)' | head -n 300Repository: NVIDIA/infra-controller
Length of output: 35595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transaction helper definition ---'
sed -n '3728,3765p' crates/api-core/src/api.rs
printf '%s\n' '--- power shelf database operations ---'
rg -n 'pub async fn find_by_id|pub async fn set_decommission_requested|async fn find_by_id|async fn set_decommission_requested' \
crates/api-db/src/power_shelf.rs
sed -n '1,220p' crates/api-db/src/power_shelf.rs | grep -n -A45 -B8 \
-E 'find_by_id|set_decommission_requested' || true
printf '%s\n' '--- decommission concurrency tests ---'
rg -n -i -C 8 'concurr|race|parallel|spawn|decommission_power_shelf|allocate_instance' \
crates/api-core/src/tests/power_shelf_state_controller/decommissioning.rs \
crates/api-core/src/tests/instance* \
| head -n 1200 || trueRepository: NVIDIA/infra-controller
Length of output: 50380
Serialize power-shelf decommission with instance allocation.
decommission_power_shelf checks instances before updating power_shelves, while batch_allocate_instances locks only machines. An allocation can commit after the check and before decommission commits. Use a shared rack or advisory lock in both transactions, and add a concurrent integration 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-db/src/managed_host.rs` around lines 90 - 102, Serialize
decommissioning and instance allocation by acquiring the same rack-scoped or
advisory lock in decommission_power_shelf and batch_allocate_instances before
checking or modifying related rows. Preserve the existing DPU-machine query
while ensuring both transactions hold the lock through commit, and add a
concurrent integration test proving allocation cannot commit between the
decommission check and update.
For the use case where we want to decommission a machine before it is physically removed from the site, we can't automatically delete the records from NICo because NICo would just start automatically ingesting them again*. We want to pause it just before (2) so the operator can confirm with the datacenter that the machines have been removed. Once that's happened they can then run (2). *we could get around this by having an option to not delete the suppression entries but that still leaves a manual second step for later to remove the suppressions, which is easy to forget about and significantly reduces visibility. |
3 participants
Summary
Adds managed power-shelf decommissioning, including Core API support, state-controller orchestration, Flow integration, and admin CLI commands.
Decommissioning workflow
API and CLI
DeleteDecommissionedPowerShelfRPC and authorization rules.Decommissioned.nico-admin-cli power-shelf decommissionnico-admin-cli power-shelf delete-decommissionedFlow integration
Decommissioned.Validation
I have no managed power shelf 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.
Type of Change
Breaking Changes
Testing
Will need manual testing.