feat(machine-controller): add DPU UEFI credential rotation - #4462
Merged
spydaNVIDIA merged 2 commits intoAug 1, 2026
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe change adds DPU UEFI password management through the Forge API and CLI. It adds DPU-specific rotation states, gates, Redfish handling, controller convergence, quarantine handling, and integration coverage. ChangesDPU UEFI management
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI as Admin CLI
participant Forge as Forge API
participant UefiHandler as UEFI handler
participant RedfishClientPool
AdminCLI->>Forge: SetDpuUefiPassword request
Forge->>UefiHandler: resolve DPU and credential
UefiHandler->>RedfishClientPool: rotate_uefi_password
RedfishClientPool-->>UefiHandler: apply password through Redfish
UefiHandler-->>Forge: return staging response
Forge-->>AdminCLI: report deferred commit
sequenceDiagram
participant MachineController
participant DpuUefiRotation
participant RotationGate
participant RedfishClientPool
MachineController->>DpuUefiRotation: select eligible DPU
DpuUefiRotation->>RotationGate: query DPU target version
RotationGate-->>DpuUefiRotation: return rotation target
MachineController->>DpuUefiRotation: converge RotatingDpuUefi
DpuUefiRotation->>RedfishClientPool: apply password and restart DPU
RedfishClientPool-->>DpuUefiRotation: return Redfish result
DpuUefiRotation-->>MachineController: return to Ready or quarantine
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-31 21:31:45 UTC | Commit: 04d4087 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/redfish/src/libredfish/mod.rs (1)
334-339: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn an error for an empty candidate list.
libredfishv0.46.1 has noRedfishError::MissingCredentialsvariant. ReturnRedfishClientCreationError::MissingArgument("current_password_candidates".to_owned())for an empty list, and replaceexpectwith a non-panicking fallback. Do not replace it withunreachable!.🤖 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/redfish/src/libredfish/mod.rs` around lines 334 - 339, Update the candidate-handling flow around the final error return in the password-rotation method to explicitly return RedfishClientCreationError::MissingArgument for an empty current-password candidate list. Replace last_err.expect with a non-panicking fallback that returns an appropriate existing RedfishClientCreationError when no error was captured, without using unreachable!, while preserving the existing RedfishError path when last_err is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/dpu/set_uefi_password/args.rs`:
- Around line 30-31: Update the set-uefi-password help example to use the
standard canonical UUID placeholder 12345678-1234-5678-90ab-cdef01234567 instead
of concatenating the fm100d prefix with it.
In `@crates/api-core/src/handlers/uefi.rs`:
- Around line 571-604: Preserve the result returned by the DPU branch’s
uefi_setup call instead of discarding it, and only invoke
record_device_converged with CredentialRotationType::DpuUefi when the result is
Some, indicating the credential update was applied. Treat Ok(None) as skipped
and return successfully without writing a convergence record; retain the
existing error handling for failed setup and convergence recording.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6949-6991: Update handle_rotating_dpu_uefi so a missing
dpu_bmc_mac logs the selected DPU and transitions to ManagedHostState::Ready
instead of returning StateHandlerError::MissingData. Keep this early return
adjacent to the existing missing-DPU guard and ensure no rotation or device
action occurs on this path.
- Around line 6995-7026: Update the credential resolution in the block following
current_site_uefi_target so resolve_site_uefi_credentials uses the captured
target rather than re-reading the current site target. Pass target into the
helper or directly read CredentialKey::dpu_uefi_site_default(target), ensuring
the credential matches the rotation state recorded for that target.
---
Nitpick comments:
In `@crates/redfish/src/libredfish/mod.rs`:
- Around line 334-339: Update the candidate-handling flow around the final error
return in the password-rotation method to explicitly return
RedfishClientCreationError::MissingArgument for an empty current-password
candidate list. Replace last_err.expect with a non-panicking fallback that
returns an appropriate existing RedfishClientCreationError when no error was
captured, without using unreachable!, while preserving the existing RedfishError
path when last_err is present.
🪄 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: 9fed9802-f066-42ec-be1e-95530a446486
⛔ 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 (25)
crates/admin-cli/src/dpu/mod.rscrates/admin-cli/src/dpu/set_uefi_password/args.rscrates/admin-cli/src/dpu/set_uefi_password/cmd.rscrates/admin-cli/src/dpu/set_uefi_password/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/handlers/uefi.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpu_uefi_rotation.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/dpu_uefi_rotation.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/main.rscrates/redfish/src/libredfish/mod.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+30
to
+31
| Set the UEFI password for a DPU by machine ID: | ||
| $ nico-admin-cli dpu set-uefi-password --query fm100d12345678-1234-5678-90ab-cdef01234567 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the machine ID placeholder in the help example.
The value fm100d12345678-1234-5678-90ab-cdef01234567 concatenates a prefix with the canonical UUID placeholder. Operators copy help examples verbatim, so this string must be a valid machine ID. Use the standard UUID placeholder.
📝 Proposed fix for the placeholder
Set the UEFI password for a DPU by machine ID:
- $ nico-admin-cli dpu set-uefi-password --query fm100d12345678-1234-5678-90ab-cdef01234567
+ $ nico-admin-cli dpu set-uefi-password --query 12345678-1234-5678-90ab-cdef01234567As per coding guidelines: "Use realistic, consistent placeholder values instead of angle brackets: UUIDs as '12345678-1234-5678-90ab-cdef01234567'".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Set the UEFI password for a DPU by machine ID: | |
| $ nico-admin-cli dpu set-uefi-password --query fm100d12345678-1234-5678-90ab-cdef01234567 | |
| Set the UEFI password for a DPU by machine ID: | |
| $ nico-admin-cli dpu set-uefi-password --query 12345678-1234-5678-90ab-cdef01234567 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/admin-cli/src/dpu/set_uefi_password/args.rs` around lines 30 - 31,
Update the set-uefi-password help example to use the standard canonical UUID
placeholder 12345678-1234-5678-90ab-cdef01234567 instead of concatenating the
fm100d prefix with it.
Source: Coding guidelines
Comment on lines
+6949
to
+6991
| /// Converge one DPU's UEFI (BIOS setup) password to the staged site-wide | ||
| /// `dpu_uefi` target in `ManagedHostState::RotatingDpuUefi`, then return to | ||
| /// `Ready`. The Ready entry guard already picked `dpu_machine_id` as the DPU to | ||
| /// converge this cycle (force-requested, or lagging with rotation enabled). | ||
| /// | ||
| /// Single-tick, fire-and-record (matching the DPU ingestion path): stage the | ||
| /// target through the DPU's `Bios/Settings` (authenticating with the | ||
| /// current-version credential via | ||
| /// [`dpu_uefi_rotation::dpu_uefi_current_candidates`], not the empty/factory | ||
| /// assumption), issue the DPU restart that commits it, then record convergence. | ||
| /// Crash-safe and idempotent -- `rotating_to_version` is staged before the | ||
| /// restart and re-running just re-applies the same target and re-issues the | ||
| /// restart. A device-level failure is quarantined with exponential backoff and | ||
| /// returns to `Ready`, so the DPU never wedges the host in this state; the | ||
| /// passive gate then skips the DPU until the window elapses. A missing DPU (gone | ||
| /// from the snapshot) or missing BMC MAC returns to `Ready` without acting. | ||
| async fn handle_rotating_dpu_uefi( | ||
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | ||
| state: &ManagedHostStateSnapshot, | ||
| dpu_machine_id: MachineId, | ||
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | ||
| use db::credential_rotation::CredentialRotationType::DpuUefi; | ||
|
|
||
| let db_pool = ctx.services.db_pool.clone(); | ||
|
|
||
| // The entry guard selected this DPU from the same snapshot; if it is somehow | ||
| // gone now, there is nothing to converge -- return to Ready rather than error. | ||
| let Some(dpu) = state.dpu_snapshots.iter().find(|d| d.id == dpu_machine_id) else { | ||
| tracing::warn!( | ||
| %dpu_machine_id, | ||
| "RotatingDpuUefi selected a DPU no longer present on the host; returning to Ready" | ||
| ); | ||
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready)); | ||
| }; | ||
|
|
||
| // A known DPU BMC MAC keys the dpu_uefi rotation bookkeeping; without it the | ||
| // device can be neither tracked nor reached (the entry guard likewise never | ||
| // selects such a DPU). | ||
| let dpu_bmc_mac = | ||
| dpu_uefi_rotation::dpu_bmc_mac(dpu).ok_or(StateHandlerError::MissingData { | ||
| object_id: dpu.id.to_string(), | ||
| missing: "bmc_mac", | ||
| })?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the missing-BMC-MAC path to match its own documented contract.
The doc comment for handle_rotating_dpu_uefi states: "A missing DPU (gone from the snapshot) or missing BMC MAC returns to Ready without acting." The "missing DPU" branch above (lines 6976-6982) does this correctly. The BMC MAC extraction does not: .ok_or(StateHandlerError::MissingData {...})? returns an Err, not a transition to Ready.
should_rotate_dpu_uefi in dpu_uefi_rotation.rs lets a force-requested DPU enter RotatingDpuUefi even when it has no BMC MAC (only the passive path checks dpu_bmc_mac). When that happens, this function errors out before the if forced { clear_uefi_credential_rotation_requested } logic in either branch of the later match runs. The one-shot force request is never cleared, so the operator's force-converge request re-selects the same DPU on every Ready sweep. The host cannot make progress on this DPU, unlike the documented safe behavior.
Add an early return that matches the "missing DPU" guard style, so the promise in the doc comment holds.
🛡️ Proposed fix to honor the documented "returns to Ready without acting" contract
- let dpu_bmc_mac =
- dpu_uefi_rotation::dpu_bmc_mac(dpu).ok_or(StateHandlerError::MissingData {
- object_id: dpu.id.to_string(),
- missing: "bmc_mac",
- })?;
+ let Some(dpu_bmc_mac) = dpu_uefi_rotation::dpu_bmc_mac(dpu) else {
+ // A force request can select a DPU before its BMC MAC is discovered.
+ // Match the "missing DPU" guard above: return to Ready without acting
+ // rather than erroring out and wedging the host on this DPU.
+ tracing::warn!(
+ %dpu_machine_id,
+ "RotatingDpuUefi selected a DPU with no known BMC MAC; returning to Ready without acting"
+ );
+ return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready));
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Converge one DPU's UEFI (BIOS setup) password to the staged site-wide | |
| /// `dpu_uefi` target in `ManagedHostState::RotatingDpuUefi`, then return to | |
| /// `Ready`. The Ready entry guard already picked `dpu_machine_id` as the DPU to | |
| /// converge this cycle (force-requested, or lagging with rotation enabled). | |
| /// | |
| /// Single-tick, fire-and-record (matching the DPU ingestion path): stage the | |
| /// target through the DPU's `Bios/Settings` (authenticating with the | |
| /// current-version credential via | |
| /// [`dpu_uefi_rotation::dpu_uefi_current_candidates`], not the empty/factory | |
| /// assumption), issue the DPU restart that commits it, then record convergence. | |
| /// Crash-safe and idempotent -- `rotating_to_version` is staged before the | |
| /// restart and re-running just re-applies the same target and re-issues the | |
| /// restart. A device-level failure is quarantined with exponential backoff and | |
| /// returns to `Ready`, so the DPU never wedges the host in this state; the | |
| /// passive gate then skips the DPU until the window elapses. A missing DPU (gone | |
| /// from the snapshot) or missing BMC MAC returns to `Ready` without acting. | |
| async fn handle_rotating_dpu_uefi( | |
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | |
| state: &ManagedHostStateSnapshot, | |
| dpu_machine_id: MachineId, | |
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | |
| use db::credential_rotation::CredentialRotationType::DpuUefi; | |
| let db_pool = ctx.services.db_pool.clone(); | |
| // The entry guard selected this DPU from the same snapshot; if it is somehow | |
| // gone now, there is nothing to converge -- return to Ready rather than error. | |
| let Some(dpu) = state.dpu_snapshots.iter().find(|d| d.id == dpu_machine_id) else { | |
| tracing::warn!( | |
| %dpu_machine_id, | |
| "RotatingDpuUefi selected a DPU no longer present on the host; returning to Ready" | |
| ); | |
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready)); | |
| }; | |
| // A known DPU BMC MAC keys the dpu_uefi rotation bookkeeping; without it the | |
| // device can be neither tracked nor reached (the entry guard likewise never | |
| // selects such a DPU). | |
| let dpu_bmc_mac = | |
| dpu_uefi_rotation::dpu_bmc_mac(dpu).ok_or(StateHandlerError::MissingData { | |
| object_id: dpu.id.to_string(), | |
| missing: "bmc_mac", | |
| })?; | |
| /// Converge one DPU's UEFI (BIOS setup) password to the staged site-wide | |
| /// `dpu_uefi` target in `ManagedHostState::RotatingDpuUefi`, then return to | |
| /// `Ready`. The Ready entry guard already picked `dpu_machine_id` as the DPU to | |
| /// converge this cycle (force-requested, or lagging with rotation enabled). | |
| /// | |
| /// Single-tick, fire-and-record (matching the DPU ingestion path): stage the | |
| /// target through the DPU's `Bios/Settings` (authenticating with the | |
| /// current-version credential via | |
| /// [`dpu_uefi_rotation::dpu_uefi_current_candidates`], not the empty/factory | |
| /// assumption), issue the DPU restart that commits it, then record convergence. | |
| /// Crash-safe and idempotent -- `rotating_to_version` is staged before the | |
| /// restart and re-running just re-applies the same target and re-issues the | |
| /// restart. A device-level failure is quarantined with exponential backoff and | |
| /// returns to `Ready`, so the DPU never wedges the host in this state; the | |
| /// passive gate then skips the DPU until the window elapses. A missing DPU (gone | |
| /// from the snapshot) or missing BMC MAC returns to `Ready` without acting. | |
| async fn handle_rotating_dpu_uefi( | |
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | |
| state: &ManagedHostStateSnapshot, | |
| dpu_machine_id: MachineId, | |
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | |
| use db::credential_rotation::CredentialRotationType::DpuUefi; | |
| let db_pool = ctx.services.db_pool.clone(); | |
| // The entry guard selected this DPU from the same snapshot; if it is somehow | |
| // gone now, there is nothing to converge -- return to Ready rather than error. | |
| let Some(dpu) = state.dpu_snapshots.iter().find(|d| d.id == dpu_machine_id) else { | |
| tracing::warn!( | |
| %dpu_machine_id, | |
| "RotatingDpuUefi selected a DPU no longer present on the host; returning to Ready" | |
| ); | |
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready)); | |
| }; | |
| // A known DPU BMC MAC keys the dpu_uefi rotation bookkeeping; without it the | |
| // device can be neither tracked nor reached (the entry guard likewise never | |
| // selects such a DPU). | |
| let Some(dpu_bmc_mac) = dpu_uefi_rotation::dpu_bmc_mac(dpu) else { | |
| // A force request can select a DPU before its BMC MAC is discovered. | |
| // Match the "missing DPU" guard above: return to Ready without acting | |
| // rather than erroring out and wedging the host on this DPU. | |
| tracing::warn!( | |
| %dpu_machine_id, | |
| "RotatingDpuUefi selected a DPU with no known BMC MAC; returning to Ready without acting" | |
| ); | |
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready)); | |
| }; |
🤖 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/machine-controller/src/handler.rs` around lines 6949 - 6991, Update
handle_rotating_dpu_uefi so a missing dpu_bmc_mac logs the selected DPU and
transitions to ManagedHostState::Ready instead of returning
StateHandlerError::MissingData. Keep this early return adjacent to the existing
missing-DPU guard and ensure no rotation or device action occurs on this path.
Comment on lines
+6995
to
+7026
| let target = current_site_uefi_target(&db_pool, DpuUefi).await?; | ||
|
|
||
| // The device's tracked current version selects the first authentication | ||
| // candidate; its prior attempt count sizes the backoff on failure. | ||
| let (current_version, prior_attempts) = { | ||
| let mut conn = db_pool.acquire().await?; | ||
| match db::credential_rotation::device_rotation_status(&mut conn, DpuUefi, dpu_bmc_mac) | ||
| .await | ||
| .map_err(|e| { | ||
| StateHandlerError::GenericError(eyre!("read dpu uefi rotation status: {e}")) | ||
| })? { | ||
| Some(status) => ( | ||
| status.current_version.and_then(|v| u32::try_from(v).ok()), | ||
| status.rotate_attempts, | ||
| ), | ||
| None => (None, 0), | ||
| } | ||
| }; | ||
|
|
||
| // Resolve the ordered current-password candidates and the new (target) | ||
| // password before touching the device; scope the credential reader so it is | ||
| // not held across the mutable-context DPU restart below. | ||
| let (candidates, new_password) = { | ||
| let reader = ctx.services.redfish_client_pool.credential_reader(); | ||
| let candidates = | ||
| dpu_uefi_rotation::dpu_uefi_current_candidates(reader, current_version, target).await?; | ||
| let Credentials::UsernamePassword { | ||
| password: new_password, | ||
| .. | ||
| } = resolve_site_uefi_credentials(&db_pool, reader, DpuUefi).await?; | ||
| (candidates, new_password) | ||
| }; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e rs . crates/api-db/src | xargs rg -n -A 15 'fn mark_device_rotating_to_version|fn promote_rotating_to_current'Repository: NVIDIA/infra-controller
Length of output: 3076
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate symbols ---'
rg -n -A 35 -B 15 \
'current_site_uefi_target|resolve_site_uefi_credentials|mark_device_rotating_to_version|promote_rotating_to_current|dpu_uefi_current_candidates' \
crates/machine-controller/src/handler.rs crates/api-db/src crates -g '*.rs' | head -n 1200
printf '%s\n' '--- handler context ---'
sed -n '6900,7105p' crates/machine-controller/src/handler.rsRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
handler = Path("crates/machine-controller/src/handler.rs").read_text()
rotation = Path("crates/api-db/src/credential_rotation.rs").read_text()
def between(text, start, end):
a = text.index(start)
b = text.index(end, a)
return text[a:b]
resolve = between(
handler,
"async fn resolve_site_uefi_credentials(",
"async fn handle_host_uefi_setup(",
)
dpu_flow = between(
handler,
"async fn handle_rotating_dpu_uefi(",
"\nasync fn ",
)
mark = between(
rotation,
"pub async fn mark_device_rotating_to_version(",
"pub async fn promote_rotating_to_current(",
)
promote = between(
rotation,
"pub async fn promote_rotating_to_current(",
"pub async fn ",
)
assert "current_site_uefi_target(db_pool, credential_type).await?" in resolve
assert "CredentialKey::dpu_uefi_site_default(version)" in resolve
assert "let target = current_site_uefi_target(&db_pool, DpuUefi).await?;" in dpu_flow
assert "dpu_uefi_current_candidates(reader, current_version, target)" in dpu_flow
assert "resolve_site_uefi_credentials(&db_pool, reader, DpuUefi)" in dpu_flow
assert "target as i32" in dpu_flow
assert "rotating_to_version = EXCLUDED.rotating_to_version" in mark
assert "current_version = rotating_to_version" in promote
assert "AND rotating_to_version IS NOT NULL" in promote
print("captured target and independently re-read credential target are both present")
print("rotation persistence promotes the staged value without a target-version guard")
PYRepository: NVIDIA/infra-controller
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
handler = Path("crates/machine-controller/src/handler.rs").read_text()
rotation = Path("crates/api-db/src/credential_rotation.rs").read_text()
def between(text, start, end):
a = text.index(start)
b = text.index(end, a)
return text[a:b]
resolve = between(
handler,
"async fn resolve_site_uefi_credentials(",
"async fn handle_host_uefi_setup(",
)
dpu_flow = between(
handler,
"async fn handle_rotating_dpu_uefi(",
"\nasync fn ",
)
mark = between(
rotation,
"pub async fn mark_device_rotating_to_version(",
"pub async fn promote_rotating_to_current(",
)
promote = between(
rotation,
"pub async fn promote_rotating_to_current(",
"pub async fn ",
)
checks = [
("helper re-reads target", "current_site_uefi_target(db_pool, credential_type).await?", resolve),
("helper uses target credential key", "CredentialKey::dpu_uefi_site_default(version)", resolve),
("flow captures target", "let target = current_site_uefi_target(&db_pool, DpuUefi).await?;", dpu_flow),
("flow passes target to candidates", "dpu_uefi_current_candidates(reader, current_version, target)", dpu_flow),
("flow re-reads helper", "resolve_site_uefi_credentials(&db_pool, reader, DpuUefi)", dpu_flow),
("flow stages target", "target as i32", dpu_flow),
("mark updates staged version", "rotating_to_version = EXCLUDED.rotating_to_version", mark),
("promote copies staged version", "current_version = rotating_to_version", promote),
("promote requires staged version", "AND rotating_to_version IS NOT NULL", promote),
]
for name, needle, haystack in checks:
print(f"{'PASS' if needle in haystack else 'FAIL'}: {name}")
if needle not in haystack:
print(f" missing: {needle}")
PYRepository: NVIDIA/infra-controller
Length of output: 552
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("crates/api-db/src/credential_rotation.rs").read_text()
start = source.index("pub async fn promote_rotating_to_current(")
body_start = source.index("{", start)
next_fn = source.find("\npub async fn ", body_start)
promote = source[body_start:] if next_fn == -1 else source[body_start:next_fn]
assert "current_version = rotating_to_version" in promote
assert "AND rotating_to_version IS NOT NULL" in promote
print("promote_rotating_to_current copies the staged version without a target-version guard")
PYRepository: NVIDIA/infra-controller
Length of output: 246
Resolve the DPU UEFI credential from the captured target.
If the site target advances before resolve_site_uefi_credentials runs, the device receives the newer credential while rotation state records the older target. Read CredentialKey::dpu_uefi_site_default(target) directly, or pass target into the helper.
🤖 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/machine-controller/src/handler.rs` around lines 6995 - 7026, Update
the credential resolution in the block following current_site_uefi_target so
resolve_site_uefi_credentials uses the captured target rather than re-reading
the current site target. Pass target into the helper or directly read
CredentialKey::dpu_uefi_site_default(target), ensuring the credential matches
the rotation state recorded for that target.
spydaNVIDIA
force-pushed
the
dpu_uefi_credential_rotation
branch
from
July 31, 2026 22:18
04d4087 to
f21586b
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4462.docs.buildwithfern.com/infra-controller |
spydaNVIDIA
force-pushed
the
dpu_uefi_credential_rotation
branch
from
July 31, 2026 22:21
f21586b to
e704b29
Compare
spydaNVIDIA
changed the title
Contributor
Author
|
@coderabbitai full review please |
Contributor
|
✅ Action performedFull review finished. |
spydaNVIDIA
enabled auto-merge (squash)
July 31, 2026 22:23
aswaroop-nv
approved these changes
Jul 31, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/machine-controller/src/handler/dpu_uefi_rotation.rs`:
- Around line 228-258: Update the credential resolution in the block using
current_version and target so resolve_site_uefi_credentials reads credentials
for the already captured target, passing target if the helper supports a
target-version parameter or otherwise retrieving that exact version directly.
Ensure dpu_uefi_current_candidates, the staged rotating_to_version, and the
password applied to the device all use the same captured target.
- Around line 214-224: Update the BMC MAC extraction in the DPU UEFI rotation
handler to handle a missing MAC like the existing missing-DPU guard: clear the
one-shot forced rotation request when applicable, transition the state back to
Ready, and return without performing rotation. Replace the MissingData error
path around dpu_bmc_mac while preserving normal processing when a MAC is
present.
In `@crates/rpc/proto/forge.proto`:
- Around line 452-455: The comments for SetDpuUefiPassword in
crates/rpc/proto/forge.proto lines 452-455 and
rest-api/proto/core/src/v1/nico_nico.proto lines 457-460 incorrectly state that
the DPU restarts during the RPC. Update both comments to say the UEFI password
is staged and takes effect on the next DPU restart, without changing the RPC
definitions.
- Around line 6206-6212: Make dpu_id and machine_query mutually exclusive by
placing both fields in a oneof in SetDpuUefiPasswordRequest in
crates/rpc/proto/forge.proto and rest-api/proto/core/src/v1/nico_nico.proto,
preserving field numbers 1 and 2. Regenerate protobuf bindings and update all
generated callers and request construction to use the oneof representation.
🪄 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: 52a29d99-bbc3-40c1-b72c-96bd11ea0850
⛔ 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 (42)
crates/admin-cli/src/dpu/mod.rscrates/admin-cli/src/dpu/set_uefi_password/args.rscrates/admin-cli/src/dpu/set_uefi_password/cmd.rscrates/admin-cli/src/dpu/set_uefi_password/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-db/migrations/20260717120004_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260729120000_switch_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260730120001_power_shelf_bmc_credential_rotation_requested.sqlcrates/api-db/src/power_shelf.rscrates/api-db/src/switch.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/power_shelf/mod.rscrates/api-model/src/switch/mod.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpu_uefi_rotation.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/dpu_uefi_rotation.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/main.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/rotating_bmc.rscrates/redfish/src/libredfish/mod.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (21)
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/machine-controller/src/config/mod.rs
- crates/admin-cli/src/dpu/mod.rs
- crates/admin-cli/src/dpu/set_uefi_password/cmd.rs
- crates/admin-cli/src/dpu/set_uefi_password/args.rs
- crates/machine-controller/src/io.rs
- crates/machine-controller/tests/integration/main.rs
- crates/api-core/src/cfg/README.md
- crates/admin-cli/src/dpu/set_uefi_password/mod.rs
- crates/api-core/src/setup.rs
- crates/api-model/src/machine/slas.rs
- crates/api-core/src/handlers/uefi_credential_rotation.rs
- crates/machine-controller/src/handler/host_boot_config.rs
- crates/machine-controller/src/context.rs
- crates/api-model/src/machine/mod.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-core/src/handlers/uefi.rs
- crates/api-core/src/api.rs
- crates/machine-controller/tests/integration/env.rs
- crates/machine-controller/tests/integration/dpu_uefi_rotation.rs
- crates/redfish/src/libredfish/mod.rs
Comment on lines
+228
to
+258
| let target = current_site_uefi_target(&db_pool, DpuUefi).await?; | ||
|
|
||
| // The device's tracked current version selects the first authentication | ||
| // candidate; its prior attempt count sizes the backoff on failure. | ||
| let (current_version, prior_attempts) = { | ||
| let mut conn = db_pool.acquire().await?; | ||
| match db::credential_rotation::device_rotation_status(&mut conn, DpuUefi, dpu_bmc_mac) | ||
| .await | ||
| .map_err(|e| { | ||
| StateHandlerError::GenericError(eyre!("read dpu uefi rotation status: {e}")) | ||
| })? { | ||
| Some(status) => ( | ||
| status.current_version.and_then(|v| u32::try_from(v).ok()), | ||
| status.rotate_attempts, | ||
| ), | ||
| None => (None, 0), | ||
| } | ||
| }; | ||
|
|
||
| // Resolve the ordered current-password candidates and the new (target) | ||
| // password before touching the device; scope the credential reader so it is | ||
| // not held across the mutable-context DPU restart below. | ||
| let (candidates, new_password) = { | ||
| let reader = ctx.services.redfish_client_pool.credential_reader(); | ||
| let candidates = dpu_uefi_current_candidates(reader, current_version, target).await?; | ||
| let Credentials::UsernamePassword { | ||
| password: new_password, | ||
| .. | ||
| } = resolve_site_uefi_credentials(&db_pool, reader, DpuUefi).await?; | ||
| (candidates, new_password) | ||
| }; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the DPU UEFI credential from the captured target.
target is captured once at line 228 via current_site_uefi_target and used for dpu_uefi_current_candidates (line 252) and for staging rotating_to_version (line 270). resolve_site_uefi_credentials (line 256) is called separately with no target argument. If the site-wide target advances between these two reads, the device receives the newer credential while the rotation-state row records convergence to the older captured target. This mismatches the DB's tracked version against the password actually applied to the device.
Read the credential for the captured target directly, or pass target into the helper, so the staged version and the applied password always agree.
resolve_site_uefi_credentials's definition is not included in this batch. Please confirm whether it accepts a target-version parameter, or search the codebase for its current implementation.
#!/bin/bash
# Description: Locate resolve_site_uefi_credentials and current_site_uefi_target definitions.
rg -n -A 20 'async fn resolve_site_uefi_credentials|async fn current_site_uefi_target' crates/machine-controller/src🤖 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/machine-controller/src/handler/dpu_uefi_rotation.rs` around lines 228
- 258, Update the credential resolution in the block using current_version and
target so resolve_site_uefi_credentials reads credentials for the already
captured target, passing target if the helper supports a target-version
parameter or otherwise retrieving that exact version directly. Ensure
dpu_uefi_current_candidates, the staged rotating_to_version, and the password
applied to the device all use the same captured target.
Comment on lines
+6206
to
+6212
| message SetDpuUefiPasswordRequest { | ||
| // The DPU machine to set the UEFI password on. | ||
| common.MachineId dpu_id = 1; | ||
| // UUID, IP address, hostname or MAC address resolving to the DPU machine | ||
| // (preferred over dpu_id). | ||
| optional string machine_query = 2; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate definitions ---'
rg -n -C 12 'message SetDpuUefiPasswordRequest|machine_query|SetDpuUefiPassword' crates/rpc/proto/forge.proto rest-api/proto/core/src/v1/nico_nico.proto 2>/dev/null || true
printf '%s\n' '--- all handler and caller references ---'
rg -n -C 5 'SetDpuUefiPassword(Request|Response)?|machine_query' --glob '!**/target/**' --glob '!**/node_modules/**' .Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- implementation and request construction ---'
rg -n -C 10 'SetDpuUefiPassword|set_dpu_uefi_password|machine_query|dpu_id' \
crates --glob '*.rs' --glob '!**/target/**' | head -n 400
printf '%s\n' '--- CLI argument definition and conversion ---'
sed -n '1,220p' crates/admin-cli/src/dpu/set_uefi_password/args.rs
printf '%s\n' '--- generated Rust request shape ---'
rg -n -C 12 'pub struct SetDpuUefiPasswordRequest|SetDpuUefiPasswordRequest \{' crates --glob '*.rs' --glob '!**/target/**'
printf '%s\n' '--- proto source relationship ---'
rg -n 'nico_nico.proto|forge.proto|include|prost' Cargo.toml crates rest-api --glob '*.toml' --glob '*.rs' --glob '*.mk' --glob 'build.rs' | head -n 300Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- files that reference the RPC outside generated code ---'
rg -l 'SetDpuUefiPassword|set_dpu_uefi_password' . \
--glob '!**/gen/**' --glob '!**/target/**' --glob '!*.pb.go'
printf '%s\n' '--- exact implementation references ---'
rg -n -i -C 8 'set.?dpu.?uefi|dpu.?uefi.?password' \
--glob '!**/gen/**' --glob '!**/target/**' --glob '!*.pb.go' \
--glob '*.rs' --glob '*.go' --glob '*.ts' --glob '*.proto' | head -n 500
printf '%s\n' '--- working-tree summary ---'
git status --short
git diff --statRepository: NVIDIA/infra-controller
Length of output: 560
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- UEFI API handler ---'
sed -n '1,280p' crates/api-core/src/handlers/uefi.rs
printf '%s\n' '--- API routing and RPC conversion references ---'
rg -n -C 12 'SetDpuUefiPassword|set_dpu|machine_query|dpu_id' \
crates/api-core/src/api.rs crates/api-core/src/handlers/uefi.rs \
crates/admin-cli/src/dpu/set_uefi_password/cmd.rs
printf '%s\n' '--- request conversion tests ---'
rg -n -C 10 'SetDpuUefiPassword|machine_query|dpu_id' \
crates/api-core --glob '*.rs' | head -n 500Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
proto_paths = [
Path("crates/rpc/proto/forge.proto"),
Path("rest-api/proto/core/src/v1/nico_nico.proto"),
]
pattern = re.compile(
r"message SetDpuUefiPasswordRequest\s*\{(?P<body>.*?)\n\}",
re.S,
)
field_pattern = re.compile(
r"(?P<type>common\.MachineId|optional string)\s+"
r"(?P<name>dpu_id|machine_query)\s*=\s*(?P<number>[12])\s*;"
)
print("--- contract verifier ---")
contracts = []
for path in proto_paths:
text = path.read_text()
match = pattern.search(text)
assert match, f"missing request in {path}"
fields = [(m["type"], m["name"], int(m["number"])) for m in field_pattern.finditer(match["body"])]
assert fields == [
("common.MachineId", "dpu_id", 1),
("optional string", "machine_query", 2),
], (path, fields)
contracts.append(fields)
print(f"{path}: independent fields {fields}")
assert contracts[0] == contracts[1]
handler = Path("crates/api-core/src/handlers/uefi.rs").read_text()
start = handler.index("pub(crate) async fn set_dpu_uefi_password")
body = handler[start:]
branch = re.search(
r"let machine_id = if let Some\(query\) = request\.machine_query \{.*?"
r"\n\s*\} else \{\s*"
r"convert_and_log_machine_id\(request\.dpu_id\.as_ref\(\)\)\?",
body,
re.S,
)
assert branch, "DPU selector resolution branch not found"
print("handler: machine_query is resolved first; dpu_id is used only in the else branch")
# Model the branch for the conflicting-input case without executing repository code.
def selected(dpu_id, machine_query):
return machine_query if machine_query is not None else dpu_id
assert selected("dpu-A", "query-for-dpu-B") == "query-for-dpu-B"
print("conflicting selectors: dpu_id='dpu-A', machine_query='query-for-dpu-B' -> query-for-dpu-B")
PY
printf '%s\n' '--- existing oneof conventions ---'
rg -n -C 5 'oneof ' crates/rpc/proto/forge.proto rest-api/proto/core/src/v1/nico_nico.proto | head -n 160Repository: NVIDIA/infra-controller
Length of output: 9925
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
proto_paths = [
Path("crates/rpc/proto/forge.proto"),
Path("rest-api/proto/core/src/v1/nico_nico.proto"),
]
pattern = re.compile(
r"message SetDpuUefiPasswordRequest\s*\{(?P<body>.*?)\n\}",
re.S,
)
field_pattern = re.compile(
r"(?P<type>common\.MachineId|optional string)\s+"
r"(?P<name>dpu_id|machine_query)\s*=\s*(?P<number>[12])\s*;"
)
print("--- contract verifier ---")
contracts = []
for path in proto_paths:
text = path.read_text()
match = pattern.search(text)
assert match, f"missing request in {path}"
fields = [(m["type"], m["name"], int(m["number"])) for m in field_pattern.finditer(match["body"])]
assert fields == [
("common.MachineId", "dpu_id", 1),
("optional string", "machine_query", 2),
], (path, fields)
contracts.append(fields)
print(f"{path}: independent fields {fields}")
assert contracts[0] == contracts[1]
handler = Path("crates/api-core/src/handlers/uefi.rs").read_text()
start = handler.index("pub(crate) async fn set_dpu_uefi_password")
body = handler[start:]
branch = re.search(
r"let machine_id = if let Some\(query\) = request\.machine_query \{.*?"
r"\n\s*\} else \{\s*"
r"convert_and_log_machine_id\(request\.dpu_id\.as_ref\(\)\)\?",
body,
re.S,
)
assert branch, "DPU selector resolution branch not found"
print("handler: machine_query is resolved first; dpu_id is used only in the else branch")
def selected(dpu_id, machine_query):
return machine_query if machine_query is not None else dpu_id
assert selected("dpu-A", "query-for-dpu-B") == "query-for-dpu-B"
print("conflicting selectors: dpu_id='dpu-A', machine_query='query-for-dpu-B' -> query-for-dpu-B")
PY
printf '%s\n' '--- existing oneof conventions ---'
rg -n -C 5 'oneof ' crates/rpc/proto/forge.proto rest-api/proto/core/src/v1/nico_nico.proto | head -n 160Repository: NVIDIA/infra-controller
Length of output: 9925
Make DPU target selectors mutually exclusive.
When both fields are supplied, set_dpu_uefi_password resolves machine_query and ignores dpu_id. Declare dpu_id and machine_query in one oneof in both protobuf contracts, preserve field numbers 1 and 2, and update generated callers.
📍 Affects 2 files
crates/rpc/proto/forge.proto#L6206-L6212(this comment)rest-api/proto/core/src/v1/nico_nico.proto#L6207-L6213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/rpc/proto/forge.proto` around lines 6206 - 6212, Make dpu_id and
machine_query mutually exclusive by placing both fields in a oneof in
SetDpuUefiPasswordRequest in crates/rpc/proto/forge.proto and
rest-api/proto/core/src/v1/nico_nico.proto, preserving field numbers 1 and 2.
Regenerate protobuf bindings and update all generated callers and request
construction to use the oneof representation.
Source: Path instructions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/redfish/src/libredfish/mod.rs (1)
311-336: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn an error when no password candidate exists.
current_password_candidatescan be empty. In that case,last_err.expect(...)panics. Validate the slice before the loop and return a typedRedfishClientCreationErrorinstead.🤖 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/redfish/src/libredfish/mod.rs` around lines 311 - 336, Update the password-rotation method containing current_password_candidates and last_err to handle an empty candidate slice before entering the loop. Return an appropriate typed RedfishClientCreationError for the empty-input case, and retain the existing candidate iteration and final error behavior when candidates are present.
♻️ Duplicate comments (2)
crates/machine-controller/src/handler/dpu_uefi_rotation.rs (2)
214-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe missing-BMC-MAC path still contradicts the documented contract.
The doc comment at lines 193-194 promises that a missing BMC MAC returns to
Readywithout acting. Lines 217-224 returnStateHandlerError::MissingDatainstead.should_rotate_dpu_uefiat line 83 selects a force-requested DPU without checking for a MAC, so this path is reachable. The error short-circuits bothclear_uefi_credential_rotation_requestedcall sites, so the one-shot request survives and the entry guard re-selects the same DPU on every Ready sweep.This was raised on an earlier commit and marked addressed in commit e704b29; the current code still uses the error path.
🛡️ Proposed fix to honor the documented contract
- let dpu_bmc_mac = dpu - .status - .bmc_info - .mac - .ok_or(StateHandlerError::MissingData { - object_id: dpu.id.to_string(), - missing: "bmc_mac", - })?; + let Some(dpu_bmc_mac) = dpu.status.bmc_info.mac else { + // A force request can select a DPU before its BMC MAC is discovered. + // Match the missing-DPU guard above and clear the one-shot request so + // the guard does not re-select this DPU on every sweep. + tracing::warn!( + %dpu_machine_id, + "RotatingDpuUefi selected a DPU with no known BMC MAC; returning to Ready without acting" + ); + if dpu.uefi_credential_rotation_requested { + let mut txn = db_pool.begin().await?; + db::machine::clear_uefi_credential_rotation_requested(&mut txn, dpu_machine_id).await?; + return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready).with_txn(txn)); + } + return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready)); + };🤖 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/machine-controller/src/handler/dpu_uefi_rotation.rs` around lines 214 - 224, Update the missing-BMC-MAC handling in the DPU UEFI rotation handler to return the DPU to Ready without performing rotation or propagating StateHandlerError::MissingData. Ensure the one-shot request is cleared through the existing clear_uefi_credential_rotation_requested call path so should_rotate_dpu_uefi does not select the same DPU on subsequent Ready sweeps.
247-258: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResolve the new password from the captured
target.Line 228 captures
target. Lines 252 and 270 use it for the candidate walk and forrotating_to_version. Line 256 callsresolve_site_uefi_credentials, which reads the site target again. If the site target advances between the two reads, the DPU receives the newer password while the rotation row records the older capturedtarget.Pass
targetinto the helper, or readCredentialKey::dpu_uefi_site_default(target)directly, so the staged version and the applied password always agree. The same skew exists inset_rotating_host_uefi_password.🤖 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/machine-controller/src/handler/dpu_uefi_rotation.rs` around lines 247 - 258, Update the credential resolution in the DPU UEFI rotation flow to use the already captured target version consistently, passing target into resolve_site_uefi_credentials or directly reading CredentialKey::dpu_uefi_site_default(target) instead of rereading the site target. Apply the same fix in set_rotating_host_uefi_password so the applied password matches the staged rotation version.
🧹 Nitpick comments (1)
crates/machine-controller/tests/integration/dpu_uefi_rotation.rs (1)
121-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the unused return value.
run_until_readyreturns the iteration count, and all three call sites discard it. Returning()removes theiteration - 1bookkeeping and the trailing12literal, which currently duplicates the loop bound.♻️ Proposed simplification
-async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) -> usize { - for iteration in 1..=12 { +const READY_ITERATION_BUDGET: usize = 12; + +async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) { + for _ in 0..READY_ITERATION_BUDGET { if matches!(mh.host.machine().await.state.value, ManagedHostState::Ready) { - return iteration - 1; + return; } env.run_single_iteration().await; } assert!( matches!(mh.host.machine().await.state.value, ManagedHostState::Ready), "DPU UEFI rotation FSM did not return to Ready within the iteration budget, got {:?}", mh.host.machine().await.state.value, ); - 12 }🤖 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/machine-controller/tests/integration/dpu_uefi_rotation.rs` around lines 121 - 134, Update run_until_ready to return (), remove the iteration-count returns and redundant bookkeeping, and adjust all three call sites to use it without discarding a value. Preserve the existing readiness assertion, iteration loop, and timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/power-shelf-controller/src/context.rs`:
- Line 45: Update the rustdoc comment near the shared instance description by
removing the stray period immediately after the documentation marker, so the
sentence begins with “The same shared instance...” and renders with normal
spacing and punctuation.
In `@rest-api/proto/core/src/v1/nico_nico.proto`:
- Around line 6207-6218: Update SetDpuUefiPasswordRequest to place dpu_id and
machine_query in a oneof so exactly one DPU selector can be provided, then
update the request handler to reject an empty machine_query. Review and
regenerate the affected protobuf client/server bindings and adjust validation or
call sites for the new oneof-generated API.
---
Outside diff comments:
In `@crates/redfish/src/libredfish/mod.rs`:
- Around line 311-336: Update the password-rotation method containing
current_password_candidates and last_err to handle an empty candidate slice
before entering the loop. Return an appropriate typed RedfishClientCreationError
for the empty-input case, and retain the existing candidate iteration and final
error behavior when candidates are present.
---
Duplicate comments:
In `@crates/machine-controller/src/handler/dpu_uefi_rotation.rs`:
- Around line 214-224: Update the missing-BMC-MAC handling in the DPU UEFI
rotation handler to return the DPU to Ready without performing rotation or
propagating StateHandlerError::MissingData. Ensure the one-shot request is
cleared through the existing clear_uefi_credential_rotation_requested call path
so should_rotate_dpu_uefi does not select the same DPU on subsequent Ready
sweeps.
- Around line 247-258: Update the credential resolution in the DPU UEFI rotation
flow to use the already captured target version consistently, passing target
into resolve_site_uefi_credentials or directly reading
CredentialKey::dpu_uefi_site_default(target) instead of rereading the site
target. Apply the same fix in set_rotating_host_uefi_password so the applied
password matches the staged rotation version.
---
Nitpick comments:
In `@crates/machine-controller/tests/integration/dpu_uefi_rotation.rs`:
- Around line 121-134: Update run_until_ready to return (), remove the
iteration-count returns and redundant bookkeeping, and adjust all three call
sites to use it without discarding a value. Preserve the existing readiness
assertion, iteration loop, and timeout behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df710547-b702-4075-a132-c26fee45492d
⛔ 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 (42)
crates/admin-cli/src/dpu/mod.rscrates/admin-cli/src/dpu/set_uefi_password/args.rscrates/admin-cli/src/dpu/set_uefi_password/cmd.rscrates/admin-cli/src/dpu/set_uefi_password/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-db/migrations/20260717120004_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260729120000_switch_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260730120001_power_shelf_bmc_credential_rotation_requested.sqlcrates/api-db/src/power_shelf.rscrates/api-db/src/switch.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/power_shelf/mod.rscrates/api-model/src/switch/mod.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpu_uefi_rotation.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/dpu_uefi_rotation.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/main.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/rotating_bmc.rscrates/redfish/src/libredfish/mod.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+6207
to
+6218
| message SetDpuUefiPasswordRequest { | ||
| // The DPU machine to set the UEFI password on. | ||
| common.MachineId dpu_id = 1; | ||
| // UUID, IP address, hostname or MAC address resolving to the DPU machine | ||
| // (preferred over dpu_id). | ||
| optional string machine_query = 2; | ||
| } | ||
|
|
||
| message SetDpuUefiPasswordResponse { | ||
| // A DPU stages the change through Redfish BIOS settings and schedules no job, | ||
| // so there is nothing to poll and no job id is returned. | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require exactly one DPU target.
dpu_id and machine_query can both be present. A conflicting query silently takes precedence and can stage credentials and restart the wrong DPU.
Use a oneof for the two selectors. Reject an empty machine_query in the handler.
Proposed contract change
message SetDpuUefiPasswordRequest {
- common.MachineId dpu_id = 1;
+ oneof target {
+ common.MachineId dpu_id = 1;
+ string machine_query = 2;
+ }
// UUID, IP address, hostname or MAC address resolving to the DPU machine
- // (preferred over dpu_id).
- optional string machine_query = 2;
}As per path instructions, review protobuf definitions for validation implications and generated-client impact.
🤖 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 `@rest-api/proto/core/src/v1/nico_nico.proto` around lines 6207 - 6218, Update
SetDpuUefiPasswordRequest to place dpu_id and machine_query in a oneof so
exactly one DPU selector can be provided, then update the request handler to
reject an empty machine_query. Review and regenerate the affected protobuf
client/server bindings and adjust validation or call sites for the new
oneof-generated API.
Source: Path instructions
spydaNVIDIA
force-pushed
the
dpu_uefi_credential_rotation
branch
from
July 31, 2026 23:29
e704b29 to
b168f61
Compare
spydaNVIDIA
force-pushed
the
dpu_uefi_credential_rotation
branch
from
July 31, 2026 23:30
b168f61 to
cf7f227
Compare
chet
approved these changes
Jul 31, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/machine-controller/tests/integration/dpu_uefi_rotation.rs (3)
145-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove each credential manager into
EnvBuilder.Each
cmvalue is unused aftercm.clone(). Passcmdirectly towith_credential_managerat every site.
crates/machine-controller/tests/integration/dpu_uefi_rotation.rs#L145-L150: replacecm.clone()withcm.crates/machine-controller/tests/integration/dpu_uefi_rotation.rs#L210-L215: replacecm.clone()withcm.crates/machine-controller/tests/integration/dpu_uefi_rotation.rs#L250-L255: replacecm.clone()withcm.crates/machine-controller/tests/integration/dpu_uefi_rotation.rs#L320-L325: replacecm.clone()withcm.As per coding guidelines, Rust changes must be Clippy-clean and avoid needless clones.
🤖 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/machine-controller/tests/integration/dpu_uefi_rotation.rs` around lines 145 - 150, Remove the needless credential-manager clones by passing each cm directly to EnvBuilder::with_credential_manager in crates/machine-controller/tests/integration/dpu_uefi_rotation.rs at lines 145-150, 210-215, 250-255, and 320-325; no direct change is needed elsewhere.Source: Coding guidelines
62-93: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd a multi-DPU rotation test.
This helper creates a host with one DPU. The tests cannot detect a controller that starts rotation for all eligible DPUs in one cycle.
Create a fixture with at least two lagging DPUs. After the Ready sweep, assert that the controller selected one DPU and left the other DPU unconverged.
As per path instructions, prefer findings about behavior and missing tests over style-only comments.
🤖 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/machine-controller/tests/integration/dpu_uefi_rotation.rs` around lines 62 - 93, Add a multi-DPU fixture alongside ready_host_with_dpu that creates at least two lagging DPUs, then add an integration test covering the Ready sweep and UEFI rotation flow. Assert that exactly one eligible DPU is selected for rotation while the other remains unconverged, using each DPU’s observable rotation/convergence state and existing test harness helpers.Source: Path instructions
181-197: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify the DPU restart action.
The
current_version == Some(1)assertion verifies database bookkeeping only. A regression that records convergence after staging the password but does not issue the DPU restart would still pass this test.Assert through the Redfish simulator that the selected DPU restart action occurred before convergence is recorded.
As per path instructions, prefer findings about behavior and missing tests over style-only comments.
🤖 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/machine-controller/tests/integration/dpu_uefi_rotation.rs` around lines 181 - 197, Extend the integration test around run_until_ready and the subsequent convergence assertions to verify through the Redfish simulator that the selected DPU restart action was issued. Assert the restart occurred before checking or recording convergence, using the simulator’s existing action-observation API and preserving the current database bookkeeping assertions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/machine-controller/tests/integration/dpu_uefi_rotation.rs`:
- Around line 145-150: Remove the needless credential-manager clones by passing
each cm directly to EnvBuilder::with_credential_manager in
crates/machine-controller/tests/integration/dpu_uefi_rotation.rs at lines
145-150, 210-215, 250-255, and 320-325; no direct change is needed elsewhere.
- Around line 62-93: Add a multi-DPU fixture alongside ready_host_with_dpu that
creates at least two lagging DPUs, then add an integration test covering the
Ready sweep and UEFI rotation flow. Assert that exactly one eligible DPU is
selected for rotation while the other remains unconverged, using each DPU’s
observable rotation/convergence state and existing test harness helpers.
- Around line 181-197: Extend the integration test around run_until_ready and
the subsequent convergence assertions to verify through the Redfish simulator
that the selected DPU restart action was issued. Assert the restart occurred
before checking or recording convergence, using the simulator’s existing
action-observation API and preserving the current database bookkeeping
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ede61e35-766a-4590-a93e-34766fe878ff
⛔ 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 (42)
crates/admin-cli/src/dpu/mod.rscrates/admin-cli/src/dpu/set_uefi_password/args.rscrates/admin-cli/src/dpu/set_uefi_password/cmd.rscrates/admin-cli/src/dpu/set_uefi_password/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-db/migrations/20260717120004_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260729120000_switch_bmc_credential_rotation_requested.sqlcrates/api-db/migrations/20260730120001_power_shelf_bmc_credential_rotation_requested.sqlcrates/api-db/src/power_shelf.rscrates/api-db/src/switch.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/power_shelf/mod.rscrates/api-model/src/switch/mod.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpu_uefi_rotation.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/dpu_uefi_rotation.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/main.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/rotating_bmc.rscrates/redfish/src/libredfish/mod.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (41)
- crates/api-core/src/cfg/file.rs
- crates/machine-controller/src/io.rs
- crates/switch-controller/src/rotating_bmc.rs
- crates/machine-controller/tests/integration/bmc_rotation.rs
- crates/machine-controller/src/config/mod.rs
- crates/api-core/src/tests/switch_state_controller/bmc_rotation.rs
- crates/api-db/migrations/20260729120000_switch_bmc_credential_rotation_requested.sql
- crates/api-model/src/power_shelf/mod.rs
- crates/machine-controller/src/handler/rotation.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/machine-controller/src/handler/host_boot_config.rs
- crates/api-db/src/switch.rs
- crates/admin-cli/src/dpu/set_uefi_password/cmd.rs
- crates/admin-cli/src/dpu/mod.rs
- crates/power-shelf-controller/src/rotating_bmc.rs
- crates/api-db/src/power_shelf.rs
- crates/admin-cli/src/dpu/set_uefi_password/mod.rs
- crates/api-core/src/cfg/README.md
- crates/machine-controller/tests/integration/env.rs
- crates/power-shelf-controller/src/context.rs
- crates/api-model/src/switch/mod.rs
- crates/api-model/src/machine/slas.rs
- crates/switch-controller/src/context.rs
- crates/machine-controller/src/handler/host_uefi_rotation.rs
- crates/api-core/src/setup.rs
- crates/machine-controller/tests/integration/main.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-core/src/api.rs
- crates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rs
- crates/machine-controller/src/context.rs
- crates/api-db/migrations/20260717120004_bmc_credential_rotation_requested.sql
- crates/rpc/proto/forge.proto
- crates/api-core/src/handlers/uefi_credential_rotation.rs
- crates/redfish/src/libredfish/mod.rs
- crates/api-db/migrations/20260730120001_power_shelf_bmc_credential_rotation_requested.sql
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/machine-controller/src/handler.rs
- crates/admin-cli/src/dpu/set_uefi_password/args.rs
- crates/api-core/src/handlers/uefi.rs
- crates/machine-controller/src/handler/dpu_uefi_rotation.rs
- crates/api-model/src/machine/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend Ready-only UEFI rotation to DPUs with a dedicated RotatingDpuUefi state that converges one DPU per cycle (stage BIOS settings -> DPU restart -> record), keyed by the DPU BMC MAC and reusing the site-wide uefi_rotation_enabled flag, per-machine force flag, versioned credential candidates, and backoff/quarantine bookkeeping. Add SetDpuUefiPassword RPC +
dpu set-uefi-passwordCLI + RBAC for the direct-on-device path, and confirm TriggerUefiCredentialRotation resolves DPU BMC MACs.Related issues
#367
Type of Change
Breaking Changes
Testing
Additional Notes