docs: Managed host decommissioning design doc#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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe pull request adds a complete design for NICo managed-host decommissioning, covering lifecycle states, hardware and control-plane cleanup, public RPCs, retry semantics, ignored BMC handling, deletion behavior, and verification requirements. It also clarifies Scout release correlation wording in a host-ingestion playbook. ChangesMachine decommissioning workflow
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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.
2 participants
This is the design doc for #1969 - decommissioning a managed host. I did my best to include people's concerns, suggestions are still very much welcome