Skip to content

feat(nvos): support password rotation#3706

Open
jayzhudev wants to merge 11 commits into
NVIDIA:mainfrom
jayzhudev:nvos/password-rotation-04
Open

feat(nvos): support password rotation#3706
jayzhudev wants to merge 11 commits into
NVIDIA:mainfrom
jayzhudev:nvos/password-rotation-04

Conversation

@jayzhudev

@jayzhudev jayzhudev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

This continues #3409 and completes end-to-end NVOS password rotation support.

The main flow is:

  • Store each site-wide switch target as an immutable, versioned secret.
  • Verify both writer and effective credential readback before publishing the target.
  • Atomically publish the target and enqueue live switches for reconciliation.
  • Stage each per-switch target and attempt before calling component-manager.
  • Use RMS job IDs as transient observation handles.
  • Retry the same current-to-target request after unknown, failed, or missing jobs.
  • Persist and verify the new per-switch credential before promoting the exact target and attempt.

Passwords remain in the configured credential backend. The database stores versions, attempt metadata, transient job IDs, and redacted failure information.

Expected-switch validates NVOS credential pairs and supports updating the current credential used for reconciliation. If a confirmed per-switch credential is missing, the controller restores it from the last confirmed versioned target instead of falling back to a stale bootstrap password. Support for admin-cli is included for NVOS password rotation and credential-only expected-switch updates.

Related issues

Supports #2041 and #3522.

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required

Manual validation with real switches

Validated NVOS password rotation against two real GB200 NVLink switches. Device identities, addresses, usernames, passwords, fingerprints, and rack IDs are removed.

Scenarios

  1. Single-switch rotation: Ingested Switch A, established target version 0, then completed 10 sequential rotations through version 10. Direct credential authentication succeeded after every convergence.
  2. Late join with a different password: Added Switch B after version 10 existed and verified its distinct bootstrap credential before ingestion. NICo and RMS converged only Switch B to version 10 while Switch A remained converged. Both accepted the target; Switch B's old password returned HTTP 401, proving device mutation.
  3. Controller recovery: Stopped NICo while Switch B held a queued controller claim. NICo reclaimed it after normal stale-claim expiry and completed convergence. This covered recovery before mutation dispatch, not restart during an active RMS job.
  4. Two-switch soak: Completed 20 more site-wide rotations across both switches, versions 11 through 30, without skipping or superseding a target. Runtime was 8 minutes (2026-07-19T19:03:59Z to 2026-07-19T19:11:59Z).

For each soak iteration, the test recorded the exact target before publication, published it through admin CLI, waited for aggregate convergence, authenticated it directly on both switches, promoted the tracked current credential while retaining the previous password, then authenticated the current credential on both switches.

Results

Scenario Result
Single-switch sequence 10/10 rotations converged through version 10
Late-joining switch Caught up to version 10; old password rejected with HTTP 401
Controller stale-claim recovery Reclaimed and converged successfully
Two-switch soak 20/20 targets converged, versions 11 through 30
Soak evidence Result
Admin CLI rotation RPCs 20/20 successful
NICo controller reconciliation transitions 40
RMS per-switch requests and completed jobs 40/40
Direct target-credential authentication 40/40 HTTP 200
Direct current-credential authentication 42/42 HTTP 200 (2 preflight, 40 post-convergence)
BMC authentication checks 82/82 HTTP 200
Recovery-journal failures 0
Rotation failures / process panics 0 / 0
Runtime 8 minutes

Final state was target version 30 with both switches converged at version 30, pending=0, quarantined=0, and complete=true. No staged target, job ID, or rotation error remained. Both switches accepted the final current credential; NICo's current and target records matched, while the previous record remained distinct for recovery.

Sanitized logs

NICo final-target publication and reconciliation:

time=2026-07-19T19:11:38.410991215Z component=nico-api rpc=RotateCredential grpc_status=0
component=switch_controller switch=[REDACTED] message="NVOS password reconciliation pending; transitioning to Configuring"
component=switch_controller switch=[REDACTED] message="NVOS password reconciliation pending; transitioning to Configuring"
time=2026-07-19T19:12:01.781432629Z component=nico-api rpc=GetCredentialRotationStatus grpc_status=0

RMS final-target processing for both switches:

time=2026-07-19T19:11:41.384222Z message="processing switch password rotation request" switch=[REDACTED]
time=2026-07-19T19:11:42.438184Z message="processing switch password rotation request" switch=[REDACTED]
time=2026-07-19T19:11:43.241833Z message="started NVUE revision apply for switch password rotation" switch=[REDACTED]
time=2026-07-19T19:11:44.252225Z message="started NVUE revision apply for switch password rotation" switch=[REDACTED]
time=2026-07-19T19:11:47.716380Z message="saved applied switch password revision for reboot persistence" switch=[REDACTED]
time=2026-07-19T19:11:47.716459Z message="switch password update job completed" switch=[REDACTED]
time=2026-07-19T19:11:52.969675Z message="saved applied switch password revision for reboot persistence" switch=[REDACTED]
time=2026-07-19T19:11:52.969784Z message="switch password update job completed" switch=[REDACTED]

Recovery journal completion:

timestamp=2026-07-19T19:11:38Z iteration=20 state=published target_version=30
timestamp=2026-07-19T19:11:59Z iteration=20 state=converged target_version=30
timestamp=2026-07-19T19:11:59Z state=complete iterations=20

Automated validation with simulation

Ran CLI-driven NVOS password rotation through NICo, component-manager, RMS, and nine simulated switches.

Tested scenarios:

  • Initial target publication and fleet convergence at version 0.
  • Normal fleet rotation to a new target.
  • Partial authentication failure: 8 of 9 switches converged while one rejected both known passwords. NICo retained its staged target; restoring a known password allowed 9 of 9 convergence.
  • Total authentication failure: all 9 switches rejected both known passwords. NICo retained every staged target; restoring known passwords allowed full convergence.
  • Lost submission response without mutation: NICo retained the target without a job ID, retried the same transition, and converged.
  • Lost RMS job record: NICo observed the missing job, re-dispatched the same staged transition, and converged.
  • Password changed but response lost: the switch already accepted the target while NICo still recorded the prior version. RMS recognized the target on retry, and NICo converged without losing either known credential.
  • Paused RMS job: The operation remained pending across reconciliation passes without losing credential state. Removing the pause allowed convergence to complete.
  • Multiple sequential targets: versions 0 through 7 converged without losing staged or confirmed credential state.
  • Concurrent admin requests: two simultaneous rotations published distinct versions through CAS; the fleet converged directly to the latest target without overwriting either versioned secret.
  • NICo and RMS restart recovery: mixed per-switch password states survived service restart and converged to the published target.
  • Mixed-version recovery: one switch lagging the rest of the fleet converged to the next published target.
  • Public status reporting: partial failure, total failure, pending recovery, and final completion counts matched simulated hardware state.
  • Direct verification: RMS authenticated the final target credential against all 9 simulated switches.

Final result:

  • target_version=7
  • converged=9
  • pending=0
  • quarantined=0
  • complete=true
  • Final target credential accepted by all simulated switches.

Restart testing covered recovery between reconciliation passes and explicit lost-job state. It did not kill RMS at the exact instant of an active NVUE password apply.

@jayzhudev jayzhudev self-assigned this Jul 18, 2026
@jayzhudev
jayzhudev requested a review from a team as a code owner July 18, 2026 08:53
@jayzhudev jayzhudev added the rack lifecycle Issues that relate to managing the lifecycle of a full rack (compute, switches and powershelves) label Jul 18, 2026
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • New Features

    • Added site-wide NVOS administrator password rotation with asynchronous progress tracking and recovery.
    • Added CLI guidance and documentation for initiating and monitoring NVOS rotations.
    • Added validation requiring NVOS usernames and passwords to be provided together and non-empty.
    • Added sparse expected-switch updates that preserve fields not included in a patch.
  • Bug Fixes

    • Improved rotation safety, credential verification, retry handling, and device convergence.
    • Clarified error reporting for unavailable backends, rejected requests, and missing rotation targets.

Walkthrough

NVOS credential rotation is enabled end to end, including version-zero target staging, verified credential readback, repeat-safe component-manager dispatch, retry-fenced database state, and switch-controller reconciliation. Expected-switch updates now support validated sparse patches.

Changes

NVOS credential rotation

Layer / File(s) Summary
Backend and credential contracts
crates/secrets/..., crates/component-manager/..., crates/api-core/src/handlers/component_manager.rs
Credential writers now support direct persistence readback. NVOS password rotation uses repeat-safe ensure semantics, explicit pre-dispatch rejection, and unavailable mapping for unknown outcomes.
Target publication and expected-switch patches
crates/api-core/src/handlers/credential_rotation.rs, crates/api-core/src/handlers/expected_switch.rs, crates/rpc/src/forge_api_client.rs, crates/admin-cli/...
NVOS targets support version-zero publication and verified staging. Expected-switch CLI updates send sparse masks, validate paired NVOS credentials, merge omitted fields, and preserve existing values.
Retry-safe rotation state
crates/api-db/src/credential_rotation.rs, crates/api-core/src/errors.rs
Rotation attempts, job identifiers, failures, timestamps, and completion promotion are fenced by target and attempt state. Credentialless NVOS switches remain pending in status calculations.
Switch-controller convergence
crates/switch-controller/..., crates/api-core/src/tests/switch_state_controller/..., crates/api-core/tests/integration/credential_rotation.rs
Ready switches trigger NVOS reconciliation, restore and verify credentials, dispatch or retry backend jobs, and persist completed target credentials with integration coverage.
Operational documentation
docs/index.yml, docs/operations/nvos-password-rotation.md
The operations guide documents prerequisites, target publication, monitoring, recovery behavior, failure handling, and credential retention.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AdminCLI
  participant API
  participant CredentialStore
  participant SwitchController
  participant ComponentManager
  AdminCLI->>API: publish NVOS target
  API->>CredentialStore: stage versioned secret
  API->>SwitchController: enqueue switch reconciliation
  SwitchController->>CredentialStore: resolve current credentials
  SwitchController->>ComponentManager: ensure current-to-target rotation
  ComponentManager-->>SwitchController: return job status
  SwitchController->>CredentialStore: persist completed credentials
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: NVOS password rotation support.
Description check ✅ Passed The description is directly aligned with the implemented NVOS rotation flow, validation, tests, and docs.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/api-core/src/handlers/expected_switch.rs (1)

112-120: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Persist NVOS credentials for add and replace-all mutations.

These handlers validate and store the credential pair in the database but never update SwitchNvosAdmin. Existing switches can therefore retain stale credentials indefinitely, particularly after replace-all. Apply the same credential-store write/readback contract used by update, with explicit retry or compensation for partial bulk failures.

As per path instructions, API changes must preserve transaction safety and schema/API compatibility.

Also applies to: 313-321

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/api-core/src/handlers/expected_switch.rs` around lines 112 - 120,
Update the add and replace-all handlers around db_expected_switch::create and
txn.commit to persist validated NVOS credentials through the same
credential-store write/readback contract used by update. Ensure SwitchNvosAdmin
is updated only after the database mutation succeeds, and add explicit retry or
compensation so partial replace-all failures cannot leave database and NVOS
credentials inconsistent. Preserve transaction safety and existing schema/API
behavior.

Source: Path instructions

🧹 Nitpick comments (2)
crates/api-core/src/tests/expected_switch.rs (1)

462-475: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Verify persistence through direct writer readback.

get_credentials may resolve through an effective reader, cache, or fallback. Use get_credentials_from_writer here so this test proves that update_expected_switch persisted the credential to the configured writer.

Proposed fix
     let credentials = env
         .api
         .credential_manager
-        .get_credentials(&CredentialKey::SwitchNvosAdmin { bmc_mac_address })
+        .get_credentials_from_writer(&CredentialKey::SwitchNvosAdmin {
+            bmc_mac_address,
+        })
         .await
         .expect("unable to read NVOS admin credentials");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/api-core/src/tests/expected_switch.rs` around lines 462 - 475, Update
the credential readback in the expected-switch test to call CredentialManager’s
get_credentials_from_writer method instead of get_credentials. Keep the existing
CredentialKey::SwitchNvosAdmin lookup, await, error message, and assertions
unchanged so the test validates persistence in the configured writer.
crates/admin-cli/src/expected_switch/update/cmd.rs (1)

133-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Table-drive the sparse merge contract.

This single case misses the independent omitted/provided behavior for identifiers, BMC and NVOS credential pairs, metadata, rack ID, IPs, and credential retention. Cover those axes with check_cases or scenarios!.

As per coding guidelines, functions mapping inputs to observable outputs should use table-driven tests.

🤖 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/expected_switch/update/cmd.rs` around lines 133 - 155,
Expand the test coverage for merge_update_request with table-driven cases using
check_cases or scenarios!, covering independent omitted/provided behavior for
identifiers, BMC and NVOS credential pairs, metadata, rack ID, IP addresses, and
credential retention. Replace the single
merge_update_request_preserves_endpoint_fields_for_credential_only_update case
with cases that assert each sparse-merge outcome while preserving the existing
expected request values.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/admin-cli/src/credential/rotate/args.rs`:
- Around line 34-35: Update the credential rotation help example for the NVOS
target password to use the prescribed shell-safe placeholder “mypassword”
instead of the unquoted password containing “!”. Keep the command structure and
copy-pasteable formatting unchanged.

In `@crates/admin-cli/src/expected_switch/update/cmd.rs`:
- Around line 33-36: Replace the client-side read-modify-write sequence around
get_expected_switch, merge_update_request, and update_expected_switch with an
atomic server-side sparse patch, or pass the fetched revision and enforce
compare-and-swap rejection for stale updates. Ensure concurrent changes are not
overwritten by stale fields while preserving the requested patch semantics.

In `@crates/api-core/src/handlers/component_manager.rs`:
- Around line 78-79: Update the ComponentManagerError-to-gRPC mapping so
RejectedBeforeDispatch remains a definitive non-retryable response while
OperationOutcomeUnknown uses a distinct retry-preserving status or structured
status detail. Revise the associated status-code table to document this
distinction consistently with the implementation.

In `@crates/component-manager/src/rms.rs`:
- Around line 2063-2068: Update the RackManagerError::ApiInvocationError mapping
for tonic::Code::Unimplemented to return the existing Unsupported error
classification instead of RejectedBeforeDispatch. Preserve target-specific
rejection handling for other failures and ensure staged rotations can defer this
unsupported RMS capability.

In `@crates/switch-controller/src/configuring.rs`:
- Around line 82-96: Reorder the configuring flow so
reconcile_nvos_password_rotation runs its no-op gates for missing
component-manager, unsupported rotation, and an uninitialized or absent
published target before ensure_nvos_admin_credentials is called. Keep credential
resolution only on actionable rotation paths, preserve existing state outcomes,
and add coverage for unsupported and uninitialized-target cases while ensuring
ConfiguringState::RotateOsPassword recovers safely.

---

Outside diff comments:
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 112-120: Update the add and replace-all handlers around
db_expected_switch::create and txn.commit to persist validated NVOS credentials
through the same credential-store write/readback contract used by update. Ensure
SwitchNvosAdmin is updated only after the database mutation succeeds, and add
explicit retry or compensation so partial replace-all failures cannot leave
database and NVOS credentials inconsistent. Preserve transaction safety and
existing schema/API behavior.

---

Nitpick comments:
In `@crates/admin-cli/src/expected_switch/update/cmd.rs`:
- Around line 133-155: Expand the test coverage for merge_update_request with
table-driven cases using check_cases or scenarios!, covering independent
omitted/provided behavior for identifiers, BMC and NVOS credential pairs,
metadata, rack ID, IP addresses, and credential retention. Replace the single
merge_update_request_preserves_endpoint_fields_for_credential_only_update case
with cases that assert each sparse-merge outcome while preserving the existing
expected request values.

In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 462-475: Update the credential readback in the expected-switch
test to call CredentialManager’s get_credentials_from_writer method instead of
get_credentials. Keep the existing CredentialKey::SwitchNvosAdmin lookup, await,
error message, and assertions unchanged so the test validates persistence in the
configured writer.
🪄 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: 7f0e6878-fa9f-4313-b1a2-3796897a7584

📥 Commits

Reviewing files that changed from the base of the PR and between d538254 and 158baba.

📒 Files selected for processing (30)
  • crates/admin-cli/src/credential/common.rs
  • crates/admin-cli/src/credential/rotate/args.rs
  • crates/admin-cli/src/credential/tests.rs
  • crates/admin-cli/src/expected_switch/update/cmd.rs
  • crates/api-core/src/credentials/bmc_session_manager.rs
  • crates/api-core/src/errors.rs
  • crates/api-core/src/handlers/component_manager.rs
  • crates/api-core/src/handlers/credential.rs
  • crates/api-core/src/handlers/credential_rotation.rs
  • crates/api-core/src/handlers/expected_switch.rs
  • crates/api-core/src/secrets/mod.rs
  • crates/api-core/src/tests/expected_switch.rs
  • crates/api-core/src/tests/switch_state_controller/mod.rs
  • crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs
  • crates/api-core/tests/integration/credential_rotation.rs
  • crates/api-db/src/credential_rotation.rs
  • crates/component-manager/src/component_manager.rs
  • crates/component-manager/src/config.rs
  • crates/component-manager/src/error.rs
  • crates/component-manager/src/mock.rs
  • crates/component-manager/src/nv_switch_manager.rs
  • crates/component-manager/src/rms.rs
  • crates/secrets/src/credentials.rs
  • crates/secrets/src/forge_vault.rs
  • crates/secrets/src/memory_credentials.rs
  • crates/secrets/src/test_support/credentials.rs
  • crates/switch-controller/src/configuring.rs
  • crates/switch-controller/src/lib.rs
  • crates/switch-controller/src/nvos_password_rotation.rs
  • crates/switch-controller/src/ready.rs

Comment thread crates/admin-cli/src/credential/rotate/args.rs Outdated
Comment thread crates/admin-cli/src/expected_switch/update/cmd.rs Outdated
Comment thread crates/api-core/src/handlers/component_manager.rs Outdated
Comment thread crates/component-manager/src/rms.rs
Comment thread crates/switch-controller/src/configuring.rs Outdated
Add direct writer readback for security-sensitive persistence checks.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Persist staged targets and backend attempts with CAS updates.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Send repeat-safe current-to-target requests through RMS.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Apply sparse patches and verify credential-store writes.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Retain current and target credentials across repeat-safe retries.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Reconcile in Configuring and reschedule drift from Ready.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Publish versioned targets and report fleet convergence.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Exercise partial, failed, lost-job, and target-active retries.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
@jayzhudev
jayzhudev force-pushed the nvos/password-rotation-04 branch from 158baba to e69fc6c Compare July 18, 2026 23:04

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
crates/api-core/src/tests/expected_switch.rs (1)

129-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover empty NVOS credential values in the table.

Add (Some(""), Some("password")) and (Some("admin"), Some("")) cases to exercise the new non-empty validation branches.

As per coding guidelines, validation input variants should use table-driven tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/api-core/src/tests/expected_switch.rs` around lines 129 - 137, Add
table-driven cases to the credential validation test’s input array, alongside
the existing nvos_username/nvos_password combinations: one with an empty
username and non-empty password, and one with a non-empty username and empty
password. Mark both cases invalid and preserve the existing table-driven
structure.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 32-61: Replace the raw update-mask constants and ALL in
crates/rpc/src/forge_api_client.rs:32-61 with an ExpectedSwitchUpdateField enum
implementing Display and FromStr. Update forge API client functions in
crates/rpc/src/forge_api_client.rs:64-170 to use typed masks internally and
serialize fields only at the metadata boundary. In
crates/api-core/src/handlers/expected_switch.rs:33-125, parse masks into
HashSet<ExpectedSwitchUpdateField> and match enum variants during merging.

In `@crates/switch-controller/src/nvos_password_rotation.rs`:
- Around line 549-563: Update nvos_password_rotation.rs in the current_version
eligibility check to detect validated expected-switch username or password
changes after confirmation, not only password rotation. In configuring.rs, apply
the explicit expected-switch credential override policy so configured username
and password values replace confirmed/existing values when provided. In
crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs, add
coverage for post-convergence username-only and password-only updates; these
sites require test changes validating credential-only reconciliation.
- Around line 167-187: Update the NVOS rotation flow around
persist_nvos_admin_credentials and record_device_rotation_succeeded so failures
after target credential persistence cannot leave recovery using the old
current_version. Persist a durable completion-pending state or otherwise retain
the staged target credential before promotion, and make
ensure_nvos_admin_credentials recovery preserve credentials matching that target
until database promotion completes.

---

Nitpick comments:
In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 129-137: Add table-driven cases to the credential validation
test’s input array, alongside the existing nvos_username/nvos_password
combinations: one with an empty username and non-empty password, and one with a
non-empty username and empty password. Mark both cases invalid and preserve the
existing table-driven structure.
🪄 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: 95d41731-4c54-463c-ad84-00d5e30c1f98

📥 Commits

Reviewing files that changed from the base of the PR and between 158baba and e69fc6c.

📒 Files selected for processing (31)
  • crates/admin-cli/src/credential/common.rs
  • crates/admin-cli/src/credential/rotate/args.rs
  • crates/admin-cli/src/credential/tests.rs
  • crates/admin-cli/src/expected_switch/update/cmd.rs
  • crates/api-core/src/credentials/bmc_session_manager.rs
  • crates/api-core/src/errors.rs
  • crates/api-core/src/handlers/component_manager.rs
  • crates/api-core/src/handlers/credential_rotation.rs
  • crates/api-core/src/handlers/expected_switch.rs
  • crates/api-core/src/secrets/mod.rs
  • crates/api-core/src/tests/expected_switch.rs
  • crates/api-core/src/tests/switch_state_controller/mod.rs
  • crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs
  • crates/api-core/tests/integration/credential_rotation.rs
  • crates/api-db/src/credential_rotation.rs
  • crates/api-db/src/expected_switch.rs
  • crates/component-manager/src/component_manager.rs
  • crates/component-manager/src/config.rs
  • crates/component-manager/src/error.rs
  • crates/component-manager/src/mock.rs
  • crates/component-manager/src/nv_switch_manager.rs
  • crates/component-manager/src/rms.rs
  • crates/rpc/src/forge_api_client.rs
  • crates/secrets/src/credentials.rs
  • crates/secrets/src/forge_vault.rs
  • crates/secrets/src/memory_credentials.rs
  • crates/secrets/src/test_support/credentials.rs
  • crates/switch-controller/src/configuring.rs
  • crates/switch-controller/src/lib.rs
  • crates/switch-controller/src/nvos_password_rotation.rs
  • crates/switch-controller/src/ready.rs
🚧 Files skipped from review as they are similar to previous changes (16)
  • crates/switch-controller/src/lib.rs
  • crates/api-core/src/tests/switch_state_controller/mod.rs
  • crates/secrets/src/forge_vault.rs
  • crates/component-manager/src/component_manager.rs
  • crates/api-core/src/errors.rs
  • crates/api-core/src/secrets/mod.rs
  • crates/api-core/src/credentials/bmc_session_manager.rs
  • crates/secrets/src/memory_credentials.rs
  • crates/admin-cli/src/credential/common.rs
  • crates/secrets/src/credentials.rs
  • crates/api-core/src/handlers/credential_rotation.rs
  • crates/component-manager/src/mock.rs
  • crates/api-core/tests/integration/credential_rotation.rs
  • crates/component-manager/src/nv_switch_manager.rs
  • crates/component-manager/src/rms.rs
  • crates/api-db/src/credential_rotation.rs

Comment thread crates/rpc/src/forge_api_client.rs Outdated
Comment thread crates/switch-controller/src/nvos_password_rotation.rs
Comment thread crates/switch-controller/src/nvos_password_rotation.rs
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/rpc/src/forge_api_client.rs (1)

215-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle an empty update mask before constructing the request.

expected_switch_update_mask(Default::default()) returns an empty vector, but this serializes it as "". The handler then attempts to parse an empty field and rejects the request. Return a no-op or an explicit local validation error instead.

Proposed no-op behavior
     ) -> Result<(), Status> {
+        if update_mask.is_empty() {
+            return Ok(());
+        }
+
         let update_mask = update_mask
🤖 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/src/forge_api_client.rs` around lines 215 - 228, Handle an empty
update_mask at the start of expected_switch_update_mask before parsing or
constructing the tonic request, returning success as a no-op. Preserve the
existing serialization, validation error, metadata insertion, and request flow
for non-empty masks.
🧹 Nitpick comments (1)
crates/rpc/src/forge_api_client.rs (1)

376-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise every update-field wire mapping.

This checks only NvosUsername; mismatched Display/FromStr arms for the other variants would pass unnoticed. Use check_values with every (variant, wire_name) pair and verify both directions.

As per coding guidelines, functions mapping inputs to outputs should use table-driven tests with check_values when appropriate.

🤖 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/src/forge_api_client.rs` around lines 376 - 383, Update
expected_switch_update_field_round_trips to use the table-driven check_values
helper with every ExpectedSwitchUpdateField variant and its wire-name string,
validating both Display and FromStr directions for each pair while retaining the
unknown-value error assertion.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 215-228: Handle an empty update_mask at the start of
expected_switch_update_mask before parsing or constructing the tonic request,
returning success as a no-op. Preserve the existing serialization, validation
error, metadata insertion, and request flow for non-empty masks.

---

Nitpick comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 376-383: Update expected_switch_update_field_round_trips to use
the table-driven check_values helper with every ExpectedSwitchUpdateField
variant and its wire-name string, validating both Display and FromStr directions
for each pair while retaining the unknown-value error assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f1dcf2bc-865e-440b-8d63-3544397ec925

📥 Commits

Reviewing files that changed from the base of the PR and between e69fc6c and 61b757a.

📒 Files selected for processing (5)
  • crates/api-core/src/handlers/expected_switch.rs
  • crates/api-core/src/tests/expected_switch.rs
  • crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs
  • crates/rpc/src/forge_api_client.rs
  • crates/switch-controller/src/configuring.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/api-core/src/tests/expected_switch.rs
  • crates/api-core/src/handlers/expected_switch.rs
  • crates/switch-controller/src/configuring.rs

Reject target publication when any live switch lacks a usable current
credential source. Report setup gaps through admin CLI before a site-wide
rotation can remain partially converged.

Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
@jayzhudev
jayzhudev requested a review from polarweasel as a code owner July 19, 2026 22:58
@github-actions

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/api-core/tests/integration/credential_rotation.rs (1)

362-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise each credential source independently.

The mixed fixture proves the combined case, but not expected-switch-only and credential-store-only admission independently. Split these into table-driven cases, retaining the mixed case only if it covers separate behavior.

As per coding guidelines, “Use a table whenever two or more tests call the same operation with different inputs.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/api-core/tests/integration/credential_rotation.rs` around lines 362 -
402, The test nvos_rotation_accepts_expected_or_stored_current_credentials
currently validates only the mixed credential-source case. Replace it with
table-driven cases that independently cover expected-switch-only and
credential-store-only admission, while retaining the mixed case only if it
verifies distinct behavior; parameterize the shared setup and rotation operation
so each credential source is exercised separately.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/api-core/tests/integration/credential_rotation.rs`:
- Around line 362-402: The test
nvos_rotation_accepts_expected_or_stored_current_credentials currently validates
only the mixed credential-source case. Replace it with table-driven cases that
independently cover expected-switch-only and credential-store-only admission,
while retaining the mixed case only if it verifies distinct behavior;
parameterize the shared setup and rotation operation so each credential source
is exercised separately.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 56e8d2b6-c62c-4627-bb62-aa5ec3ca083b

📥 Commits

Reviewing files that changed from the base of the PR and between 61b757a and 64a3b1f.

📒 Files selected for processing (5)
  • crates/api-core/src/handlers/credential_rotation.rs
  • crates/api-core/tests/integration/credential_rotation.rs
  • crates/api-db/src/credential_rotation.rs
  • docs/index.yml
  • docs/operations/nvos-password-rotation.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/api-core/src/handlers/credential_rotation.rs
  • crates/api-db/src/credential_rotation.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rack lifecycle Issues that relate to managing the lifecycle of a full rack (compute, switches and powershelves)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant