docs: Decommissioning design docs#3659
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
WalkthroughThe PR adds shared and resource-specific NICo decommissioning designs for racks, hosts, switches, and power shelves, covering lifecycle states, orchestration, cleanup, retries, deletion, and verification. It also tightens one Scout version troubleshooting statement. ChangesManaged resource decommissioning
Scout version playbook
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Flow
participant Temporal
participant Core
Operator->>Flow: DecommissionRack request
Flow->>Temporal: create durable task and frozen plan
Temporal->>Core: start host decommissioning
Core-->>Temporal: report host completion
Temporal->>Core: start switch decommissioning
Core-->>Temporal: report switch completion
Temporal->>Core: start power-shelf decommissioning
Core-->>Temporal: report rack completion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3659.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
docs/design/machine-decommissioning.md (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the document with the established
docs/designheading hierarchy.
Status,Summary, and the following primary sections are currently unnumbered H2s. Based on learnings, use numbered H1 primary sections and reserve H2/H3 for subsections.🤖 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/design/machine-decommissioning.md` around lines 1 - 7, Update the heading hierarchy in the Machine Decommissioning document: make the document’s primary sections, including Status, Summary, and subsequent top-level sections, numbered H1 headings, and convert their nested content to appropriately numbered H2/H3 subsections while preserving the existing section order and content.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.
Inline comments:
In `@docs/design/machine-decommissioning.md`:
- Line 143: Update the Markdown text near “Decommissioning the host” to
hyphenate “vendor-specific” wherever it appears in that sentence, preserving the
rest of the documentation unchanged.
- Line 120: Update the Preparing-to-DeconfiguringHost criterion to require all
managed BMC identities, including DPU BMC MACs, to be resolved and persisted
before cleanup begins and ignore entries are inserted, rather than checking only
the host BMC MAC.
- Around line 219-224: Reorder the machine decommissioning procedure so
`suppress_dhcp` is persisted for all BMCs before revoking leases or invalidating
the DHCP record cache. Ensure DHCP offers and renewals honor the suppression
state, then perform lease revocation, cache invalidation, and the
`Decommissioned` transition.
- Around line 274-280: Revise the force-skip behavior in
ResumeMachineDecommissioning to prevent bypassing cleanup invariants: either
remove force-skip or define its request contract with authorization and
auditing, restrict it to an explicit safe-substate allowlist, and add a hard
gate preventing advancement to Decommissioned until hardware cleanup and
credential removal are verified.
- Around line 289-295: Revise the machine decommissioning design so final
deletion is a resumable saga rather than one atomic transaction: retain the
machine and ignored_bmc_macs rows plus explicit deletion progress while
idempotently cleaning DPF, DHCP/DNS, exploration, and other external resources.
Commit database deletion only after all external cleanup succeeds, and resume
safely after failures without recreating managed-host records prematurely.
- Around line 135-139: Update the Decommissioning/Preparing documentation to
define how an authorized failure is aborted or rolled back to Ready after
suppress_site_explorer and BMC ignore entries are written. Specify the required
cleanup, safety invariants, and handling for permanently missing credentials or
artifacts; if rollback is unavailable, document the manual recovery procedure
and authorization requirements instead.
- Line 121: Update the DeconfiguringHost-to-DeconfiguringDPUs transition
documentation to require converge-and-verify success for every host-cleanup
operation, including restoring in-band BMC/IPMI policy and removing NIC/SuperNIC
lockdown, alongside the already listed lockdown, UEFI, BIOS, and reboot
operations.
---
Nitpick comments:
In `@docs/design/machine-decommissioning.md`:
- Around line 1-7: Update the heading hierarchy in the Machine Decommissioning
document: make the document’s primary sections, including Status, Summary, and
subsequent top-level sections, numbered H1 headings, and convert their nested
content to appropriately numbered H2/H3 subsections while preserving the
existing section order and content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c30aa504-94b9-4b3c-99b2-974a2f3dac77
📒 Files selected for processing (1)
docs/design/machine-decommissioning.md
| rpc ResumeMachineDecommissioning(ResumeMachineDecommissioningRequest) | ||
| returns (DecommissionMachineResponse); | ||
| ``` | ||
|
|
||
| This API clears an exhausted retry delay and asks the controller to run the current substate again; it does not skip a failed criterion or choose a later state by default. | ||
|
|
||
| Allow the operator to optionally force skip the currently failing state if it's not important for handoff. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not let force-skip bypass cleanup invariants.
The force-skip behavior has no request contract or safe-state allowlist. If it can advance to Decommissioned, it contradicts the invariant requiring verified hardware cleanup and credential removal. Either remove it or define authorization, auditing, allowed substates, and a hard terminal-state safety gate.
🧰 Tools
🪛 LanguageTool
[style] ~280-~280: Consider an alternative for the often overused word ‘important’.
Context: ...the currently failing state if it's not important for handoff. ### Final deletion ```pr...
(NOT_IMPORTANT)
🤖 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/design/machine-decommissioning.md` around lines 274 - 280, Revise the
force-skip behavior in ResumeMachineDecommissioning to prevent bypassing cleanup
invariants: either remove force-skip or define its request contract with
authorization and auditing, restrict it to an explicit safe-substate allowlist,
and add a hard gate preventing advancement to Decommissioned until hardware
cleanup and credential removal are verified.
| The request requires the canonical host ID. It is accepted only from exactly `Decommissioned`. It deletes the host, associated DPUs, interfaces, explored endpoints, observations, measurements, health | ||
| records, DNS/DHCP state, allocation state, and DPF/extension resources. | ||
|
|
||
| The `expected_machines` row is deliberately preserved. | ||
|
|
||
| The final transaction removes the machine rows and corresponding | ||
| `ignored_bmc_macs` rows. If the BMC is still reachable, the expected-machine entry drives normal discovery and ingestion from the beginning. If the machine is absent, no managed-host record is recreated. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Model final deletion as a resumable saga, not an atomic transaction.
Database rows cannot be committed atomically with DPF, DHCP/DNS, exploration, or other external resources. If external cleanup fails after the database commit, resources remain without a managed-host record. Retain ignore rows and deletion progress until idempotent external cleanup succeeds, then commit database deletion last.
🤖 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/design/machine-decommissioning.md` around lines 289 - 295, Revise the
machine decommissioning design so final deletion is a resumable saga rather than
one atomic transaction: retain the machine and ignored_bmc_macs rows plus
explicit deletion progress while idempotently cleaning DPF, DHCP/DNS,
exploration, and other external resources. Commit database deletion only after
all external cleanup succeeds, and resume safely after failures without
recreating managed-host records prematurely.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
docs/design/machine-decommissioning.md (1)
3-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the document with the numbered primary-heading convention.
docs/design/architecture documents use one title H1 followed by numbered primary sections as H1 headings, with H2/H3 subsections. This document currently uses H2 headings for primary sections such asStatus,Summary, andTerminology and invariants, which will make navigation and generated tables of contents inconsistent.Based on learnings, architecture design documents under
docs/design/should use numbered H1 primary sections.🤖 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/design/machine-decommissioning.md` around lines 3 - 24, Update the primary section headings in the machine decommissioning design document to follow the docs/design convention: add a document title as the sole unnumbered H1, convert Status, Summary, and Terminology and invariants to numbered H1 headings, and preserve any subsection hierarchy beneath them using H2/H3 headings.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.
Inline comments:
In `@docs/design/machine-decommissioning.md`:
- Around line 278-280: Constrain the proposed force-skip option in the
decommissioning API: define which substates may be skipped, require explicit
authorization and audit logging, and ensure skipped cleanup cannot produce a
normal Decommissioned result by using a distinct incomplete or quarantined
outcome. If these safeguards cannot be specified, remove the optional skip
behavior.
- Line 143: Update the sentence beginning “Decommissioning the host” to
hyphenate “vendor-specific.”
- Around line 121-149: Update the DeconfiguringHost-to-DeconfiguringDPUs
transition criteria to explicitly require restoration of the in-band BMC/IPMI
policy and removal of NIC/SuperNIC lockdown from host-visible devices, matching
the cleanup operations documented under Decommissioning/DeconfiguringHost. Keep
the existing lockdown, UEFI password, BIOS reset, and reboot requirements
intact.
- Around line 289-295: Update the machine decommissioning design around the
final transaction to separate database changes from external cleanup, defining
an idempotent saga or outbox with durable per-resource progress. Specify retry
behavior for failed DHCP, DNS, DPF, Site Explorer, and other external
operations, plus compensation or reconciliation before database deletion and
safe recovery after crashes, while preserving the expected_machines behavior.
- Around line 248-250: Update the Site Explorer flow described in
“machine-decommissioning” to prevent endpoints from proceeding after “Preparing”
inserts their ignore-table row: add a synchronization boundary or recheck the
current ignore state immediately before each side effect, including Redfish
authentication, credential rotation, inventory persistence, power control, and
managed-host creation. Preserve filtering for already-ignored endpoints while
ensuring in-flight exploration cannot perform any of these operations after the
row is inserted.
- Around line 333-334: Update the machine decommissioning design to name the
rejection path for stale machine certificates on machine-authenticated APIs,
covering decommissioning, terminal completion, and deletion. Explicitly state
whether this validation gate executes before the machine record is removed.
---
Nitpick comments:
In `@docs/design/machine-decommissioning.md`:
- Around line 3-24: Update the primary section headings in the machine
decommissioning design document to follow the docs/design convention: add a
document title as the sole unnumbered H1, convert Status, Summary, and
Terminology and invariants to numbered H1 headings, and preserve any subsection
hierarchy beneath them using H2/H3 headings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70989f90-70d3-44be-a05e-b6d741eb4b5a
📒 Files selected for processing (1)
docs/design/machine-decommissioning.md
| This API clears an exhausted retry delay and asks the controller to run the current substate again; it does not skip a failed criterion or choose a later state by default. | ||
|
|
||
| Allow the operator to optionally force skip the currently failing state if it's not important for handoff. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not leave force-skip semantics unconstrained.
An optional skip can bypass required hardware cleanup and credential removal, conflicting with invariants 5–6 and allowing Decommissioned to represent an incompletely sanitized machine. Define allowed substates, explicit authorization and audit requirements, and a distinct incomplete/quarantined outcome—or remove the skip option.
🧰 Tools
🪛 LanguageTool
[style] ~280-~280: Consider an alternative for the often overused word ‘important’.
Context: ...the currently failing state if it's not important for handoff. ### Final deletion ```pr...
(NOT_IMPORTANT)
🤖 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/design/machine-decommissioning.md` around lines 278 - 280, Constrain the
proposed force-skip option in the decommissioning API: define which substates
may be skipped, require explicit authorization and audit logging, and ensure
skipped cleanup cannot produce a normal Decommissioned result by using a
distinct incomplete or quarantined outcome. If these safeguards cannot be
specified, remove the optional skip behavior.
| The request requires the canonical host ID. It is accepted only from exactly `Decommissioned`. It deletes the host, associated DPUs, interfaces, explored endpoints, observations, measurements, health | ||
| records, DNS/DHCP state, allocation state, and DPF/extension resources. | ||
|
|
||
| The `expected_machines` row is deliberately preserved. | ||
|
|
||
| The final transaction removes the machine rows and corresponding | ||
| `ignored_bmc_macs` rows. If the BMC is still reachable, the expected-machine entry drives normal discovery and ingestion from the beginning. If the machine is absent, no managed-host record is recreated. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Separate database atomicity from external cleanup.
A database transaction cannot atomically include DHCP, DNS, DPF, Site Explorer, and other external resources. Failures between external deletion and the database commit can leave orphaned state or a rediscoverable machine with incomplete cleanup. Specify an idempotent saga/outbox with durable progress and retry or compensation behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/machine-decommissioning.md` around lines 289 - 295, Update the
machine decommissioning design around the final transaction to separate database
changes from external cleanup, defining an idempotent saga or outbox with
durable per-resource progress. Specify retry behavior for failed DHCP, DNS, DPF,
Site Explorer, and other external operations, plus compensation or
reconciliation before database deletion and safe recovery after crashes, while
preserving the expected_machines behavior.
| - stale machine certificates cannot call machine-authenticated APIs during | ||
| decommissioning, after terminal completion, or after deletion. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the design doc
sed -n '300,380p' docs/design/machine-decommissioning.md
# Search for references to certificate rejection, revocation, denylist, and credential epochs
rg -n --hidden --glob '!.git' -e 'stale machine certificates|certificate revocation|denylist|credential epoch|machine-authenticated APIs|decommissioning|terminal completion|deletion' docs .Repository: NVIDIA/infra-controller
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact section around the cited lines with line numbers.
nl -ba docs/design/machine-decommissioning.md | sed -n '320,350p'
# Find any nearby explanation of how machine certificates are invalidated.
rg -n -C 3 -e 'stale machine certificates|revok|denylist|epoch|terminal completion|deletion|machine-authenticated APIs|machine identity|certificate' docs/design/machine-decommissioning.md
# Search the docs for other references to machine certificate invalidation / auth checks.
rg -n -C 2 -e 'machine certificate|machine-authenticated|credential epoch|revocation|denylist|revoke|decommissioning' docsRepository: NVIDIA/infra-controller
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the cited range with line numbers using awk.
awk 'NR>=320 && NR<=350 { printf "%d:%s\n", NR, $0 }' docs/design/machine-decommissioning.md
# Search for nearby wording about how stale certificates are rejected.
rg -n -C 4 -e 'stale machine certificates|machine-authenticated APIs|revocation|denylist|credential epoch|deletion|terminal completion' docs/design/machine-decommissioning.md
# Search the docs tree for any concrete mechanism named for stale-machine rejection.
rg -n -C 2 -e 'credential epoch|certificate revocation|denylist|machine certificate|machine-authenticated|stale certificate' docsRepository: NVIDIA/infra-controller
Length of output: 6657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the machine identity verification doc for certificate lifecycle/rejection behavior.
awk 'NR>=1 && NR<=260 { printf "%d:%s\n", NR, $0 }' docs/manuals/machine_identity_verification.md
# Search for concrete enforcement terms across the repo.
rg -n -C 3 -e 'revoke|revocation|denylist|epoch|expired certificate|machine certificate|machine-authenticated APIs|stale machine certificates|mTLS' docs src .Repository: NVIDIA/infra-controller
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the machine identity docs around certificate lifecycle, rotation, and expiry.
awk 'NR>=1 && NR<=260 { printf "%d:%s\n", NR, $0 }' docs/configuration/machine_identity.md | sed -n '1,260p'
# Pull only the sections in the verification manual that mention machine certs and admin inspection.
rg -n -C 4 -e 'certificate|revok|expiry|rotate|mTLS|machine-authenticated|stale' docs/manuals/machine_identity_verification.md docs/configuration/machine_identity.md
# Check whether the codebase uses certificate-expiry or another explicit auth gate for machine APIs.
rg -n -C 2 -e 'client_certificate_expiry_unix_epoch_secs|machine-authenticated|machine certificate|expire|revok' crates rest-api docsRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the machine identity config docs around the sections that define signing keys and certificate behavior.
awk 'NR>=120 && NR<=240 { printf "%d:%s\n", NR, $0 }' docs/configuration/machine_identity.md
# Inspect the auth code for machine-certificate handling and any explicit expiry check.
rg -n -C 4 -e 'client_certificate_expiry_unix_epoch_secs|ClientCert|machine certificate|mTLS|expired|expiry|x509|spiffe' crates/api-core/src/auth.rs crates/api-core/srcRepository: NVIDIA/infra-controller
Length of output: 50382
Name the stale-cert rejection path. docs/design/machine-decommissioning.md:333-334 The design should state how machine-authenticated APIs reject stale machine certificates after decommissioning, terminal completion, and deletion, and whether that gate runs before the machine record 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 `@docs/design/machine-decommissioning.md` around lines 333 - 334, Update the
machine decommissioning design to name the rejection path for stale machine
certificates on machine-authenticated APIs, covering decommissioning, terminal
completion, and deletion. Explicitly state whether this validation gate executes
before the machine record is removed.
| This state performs the control-plane cutover: | ||
|
|
||
| 1. Revoke the existing BMC DHCP leases and release their current address | ||
| allocations. |
There was a problem hiding this comment.
The design does not yet guarantee that a decommissioned BMC has dropped its old address or is ready for fresh ingestion. It resets BMC credentials, but neither that nor deleting the server-side lease/allocation makes the BMC stop using its address. Releasing the allocation makes it eligible for assignment to another MAC, potentially resulting in two devices using the same IP (this exact problem is in the Github issue.)
I think you should define a converge-and-verify handoff for every host and DPU BMC while working credentials still exist: Disable DHCP service for the BMC first, clear the BMC’s DHCP client state using a BMC reset or factory reset if that fails and observe it return to a fresh DHCPDISCOVER state. Separately verify that the old address is no longer in use. The old allocation must remain quarantined and must not be reassigned until that succeeds, or until the maximum remaining lease lifetime has expired with an appropriate safety check. Fresh re-ingestion should then explicitly remove suppression and verify a new discovery and lease cycle.
|
|
||
| Decommissioning starts only when the managed host is in `Ready`. NICo removes the configurations and credentials it placed on the host and its DPUs, installs a vanilla BFB on every managed DPU, and then ends in the terminal `Decommissioned` state. | ||
|
|
||
| During the decommissioning process, the BMC MACs are added to an ignore table: at the beginning of decommissioning, we tell site explorer to skip exploring these BMCs, and at the end of decommissioning we additionally tell the DHCP server to stop leasing (and revoke) IPs to these BMCs. |
There was a problem hiding this comment.
@nvaprado Note that this will make the BMC unreachable after the lease time expires until the next environment picks up the machine so if you actually needed to do something on it after "decommissioning", then you couldn't since the BMC wouldn't have an IP address.
NICo can also send a NAK instead of just dropping the request so the BMC will release the IP during the next renewal (vs expiry); as long as it behaves properly.
| deletion (different from force-delete). Final deletion removes the machine and its associated state, but | ||
| does not touch the `expected_machines` entry. It also removes the BMC ignore entries. | ||
| Consequently, hardware that is still connected to the site is automatically discovered and | ||
| ingested again. Hardware that has been physically removed stays absent. The site manager can remove the machine from `expected_machines` after the process is completed. |
There was a problem hiding this comment.
So the operator needs to be very sure that the host is gone before "final deletion" or that they actually want the host to be re-ingested?
There was a problem hiding this comment.
Ideally yes, but yeah it would likely be better to let the operator specify whether NICO should remove the MACs from the ignore table right away or just leave it. Leaving the ignored MACs to be manually deleted at some time in the future could lead to forgotten/stale entries. I was a bit worried about a scenario where maybe a few months after a machine is removed from a site, the machine is added back but doesn't ingest because the operator forgot that the MAC addresses were left sitting in the ignore table. Site explorer + DHCP logging the MACs they skip should hopefully be sufficient for visibility on that though.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/design/decommissioning/rack-decommissioning.md (1)
163-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefine the meaning of
max_parallel: 0.The document says components may run in parallel, but every default stage uses
max_parallel: 0without defining whether zero means unlimited, disabled, or invalid. Make the configuration explicit—prefer a named unlimited value or a positive bound—and document the framework semantics so the default cannot deadlock or accidentally overload Core.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/decommissioning/rack-decommissioning.md` around lines 163 - 183, Clarify the `max_parallel` semantics in the Default Rack Decommissioning configuration: replace each `max_parallel: 0` under the staged component rules with an explicit named unlimited value or a positive bound, and document the framework behavior for that value so execution cannot deadlock or overload Core.
🤖 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 `@docs/design/decommissioning/decommissioning-workflow.md`:
- Around line 145-154: Update the decommissioning workflow’s Discovery
suppression section to require a generation or lock barrier that invalidates and
aborts any in-flight Site Explorer pass before it can authenticate, rotate
credentials, persist inventory, or create resources. Ensure decommissioning
waits for that barrier to complete before hardware cleanup begins, while
retaining the existing ignore-table and suppress_site_explorer behavior.
- Around line 130-143: Define a single atomic, idempotent decommissioning claim
across Core and Flow: in docs/design/decommissioning/decommissioning-workflow.md
lines 130-143, specify that Core atomically transitions a resource from Ready to
Preparing, while Flow persists and reuses a per-resource idempotency key and
duplicate or concurrent requests return the existing claim rather than starting
another operation. In docs/design/decommissioning/rack-decommissioning.md lines
256-259, change retry handling to use this idempotent claim instead of
performing a bare state read followed by a start.
In `@docs/design/decommissioning/managed-switch-decommissioning.md`:
- Around line 90-99: Normalize the terminal transition target to Deleted in both
transition tables: update the Decommissioned row in
docs/design/decommissioning/managed-switch-decommissioning.md lines 90-99 and
docs/design/decommissioning/managed-power-shelf-decommissioning.md lines 86-94.
Keep the existing deletion criteria and transition behavior unchanged.
---
Nitpick comments:
In `@docs/design/decommissioning/rack-decommissioning.md`:
- Around line 163-183: Clarify the `max_parallel` semantics in the Default Rack
Decommissioning configuration: replace each `max_parallel: 0` under the staged
component rules with an explicit named unlimited value or a positive bound, and
document the framework behavior for that value so execution cannot deadlock or
overload Core.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9ebc802-0d38-490a-81fc-dca1d49c2152
📒 Files selected for processing (5)
docs/design/decommissioning/decommissioning-workflow.mddocs/design/decommissioning/managed-host-decommissioning.mddocs/design/decommissioning/managed-power-shelf-decommissioning.mddocs/design/decommissioning/managed-switch-decommissioning.mddocs/design/decommissioning/rack-decommissioning.md
| ### Transition criteria | ||
|
|
||
| | From | To | Required criteria | | ||
| | --- | --- | --- | | ||
| | `Ready` | `Decommissioning/Preparing` | The request is authorized; the switch is exactly `Ready`; its rack is known; no managed host on the rack is in use; and no maintenance, reprovisioning, rack firmware, or other exclusive operation is active. | | ||
| | `Preparing` | `ResettingNVOS` | The BMC MAC and `expected_switches` record exist; Site Explorer is suppressed for the BMC; BMC and NVOS credentials are readable; and pending switch operations are cleared. | | ||
| | `ResettingNVOS` | `RemovingManagedCredentials` | NICo-managed NVOS configuration is removed, the NVOS password is reset to its neutral value, and both outcomes are verified. | | ||
| | `RemovingManagedCredentials` | `Finalizing` | The BMC credential reset is verified, and NICo-held BMC and NVOS credential material and rotation markers are absent. | | ||
| | `Finalizing` | `Decommissioned` | BMC DHCP is suppressed, the current BMC lease is revoked, and the DHCP record cache is invalidated. | | ||
| | `Decommissioned` | deleted | `DeleteDecommissionedSwitch` is authorized; associated switch state and the BMC ignore row are removed; `expected_switches` remains. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the terminal state to Deleted.
Both transition tables use lowercase deleted, while their state models and diagrams use Deleted.
docs/design/decommissioning/managed-switch-decommissioning.md#L90-L99: change the target state toDeleted.docs/design/decommissioning/managed-power-shelf-decommissioning.md#L86-L94: change the target state toDeleted.
📍 Affects 2 files
docs/design/decommissioning/managed-switch-decommissioning.md#L90-L99(this comment)docs/design/decommissioning/managed-power-shelf-decommissioning.md#L86-L94
🤖 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/design/decommissioning/managed-switch-decommissioning.md` around lines
90 - 99, Normalize the terminal transition target to Deleted in both transition
tables: update the Decommissioned row in
docs/design/decommissioning/managed-switch-decommissioning.md lines 90-99 and
docs/design/decommissioning/managed-power-shelf-decommissioning.md lines 86-94.
Keep the existing deletion criteria and transition behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/decommissioning/managed-switch-decommissioning.md`:
- Around line 215-217: Update the managed-switch decommissioning bullet
beginning “a BMC request” to close the inline-code span after DHCPNAK, ensuring
the following text renders as normal Markdown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 31949028-8235-4536-9087-be7d78301102
📒 Files selected for processing (5)
docs/design/decommissioning/decommissioning-workflow.mddocs/design/decommissioning/managed-host-decommissioning.mddocs/design/decommissioning/managed-power-shelf-decommissioning.mddocs/design/decommissioning/managed-switch-decommissioning.mddocs/design/decommissioning/rack-decommissioning.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/design/decommissioning/decommissioning-workflow.md
- docs/design/decommissioning/rack-decommissioning.md
polarweasel
left a comment
polarweasel
left a comment
There was a problem hiding this comment.
Approving because the one change to published docs is a good one. If you want me to review the design docs before they're final, let me know!
4 participants
This is the design doc for #1969 - decommissioning managed components from NICo. I did my best to address people's concerns, suggestions are still very much welcome