feat(rack): coordinate power shelf, compute , switch firmware upgrades across rack components - #4089
Open
vinodchitraliNVIDIA wants to merge 1 commit into
Open
feat(rack): coordinate power shelf, compute , switch firmware upgrades across rack components#4089vinodchitraliNVIDIA wants to merge 1 commit into
vinodchitraliNVIDIA wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds rack firmware reprovisioning for power shelves, activity-driven switch reprovisioning, persisted firmware status, controller state transitions, rack-scoped progress evaluation, configuration wiring, and database-backed tests. ChangesRack firmware reprovisioning
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
vinodchitraliNVIDIA
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/tests/common/api_fixtures/mod.rs`:
- Line 1658: Update the controller service configuration in the fixture to use
the rack firmware reprovisioning value from overrides.config instead of
hard-coding false. Preserve the existing default when no override is provided,
so tests can explicitly enable the feature through their configuration.
In `@crates/power-shelf-controller/src/ready.rs`:
- Around line 85-98: Update the unknown-initiator branch in the Ready state
handler to clear power_shelf_reprovisioning_requested before returning the Error
transition. Match the request-clearing behavior of the flag-disabled and
no-relevant-activities early-return branches, while preserving the existing
warning and Error cause.
In `@crates/switch-controller/src/ready.rs`:
- Around line 60-73: Update the invalid-initiator branch in the ready-state
reprovisioning flow to call clear_switch_reprovisioning_requested before
transitioning to SwitchControllerState::Error. Match the cleanup ordering and
behavior used by the other Error transitions in the reprovisioning flow, while
preserving the existing warning and error cause.
In `@crates/switch-controller/src/reprovisioning.rs`:
- Around line 40-101: Update nvos_update_requested to use the same
empty-activities-as-all-phases behavior as should_run, while preserving its NVOS
activity matching for non-empty lists. Verify next_state_after_firmware enters
WaitingForNVOSUpgrade for an empty request, and add a table-driven regression
case in the switch state controller tests covering this flow.
🪄 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: a8a5100e-d95e-4cc8-a06d-39f7881d9778
📒 Files selected for processing (28)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/error_state.rscrates/api-core/src/tests/power_shelf_state_controller/maintenance.rscrates/api-core/src/tests/power_shelf_state_controller/mod.rscrates/api-core/src/tests/power_shelf_state_controller/reprovisioning.rscrates/api-core/src/tests/rack_state_controller/handler.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-db/migrations/20260724120000_power_shelf_reprovisioning_requested.sqlcrates/api-db/src/power_shelf.rscrates/api-db/src/switch.rscrates/api-model/src/power_shelf/mod.rscrates/api-model/src/power_shelf/slas.rscrates/api-model/src/switch/mod.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/handler.rscrates/power-shelf-controller/src/io.rscrates/power-shelf-controller/src/lib.rscrates/power-shelf-controller/src/ready.rscrates/power-shelf-controller/src/reprovisioning.rscrates/rack-controller/src/maintenance.rscrates/rpc/src/model/power_shelf.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/reprovisioning.rs
| component_manager: test_component_manager.clone(), | ||
| credential_manager: credential_manager.clone(), | ||
| per_object_metrics_registry: per_object_metrics_registry.clone(), | ||
| rack_firmware_reprovisioning_enabled: false, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor the test configuration override.
This fixture builds config from overrides.config, but the controller service hard-codes false, so callers cannot enable rack firmware reprovisioning through the test environment. Enabled-path integration tests will silently exercise the disabled behavior.
Proposed fix
- rack_firmware_reprovisioning_enabled: false,
+ rack_firmware_reprovisioning_enabled: config
+ .power_shelf_state_controller
+ .rack_firmware_reprovisioning_enabled,📝 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
| rack_firmware_reprovisioning_enabled: false, | |
| rack_firmware_reprovisioning_enabled: config | |
| .power_shelf_state_controller | |
| .rack_firmware_reprovisioning_enabled, |
🤖 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/common/api_fixtures/mod.rs` at line 1658, Update
the controller service configuration in the fixture to use the rack firmware
reprovisioning value from overrides.config instead of hard-coding false.
Preserve the existing default when no override is provided, so tests can
explicitly enable the feature through their configuration.
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
2 times, most recently
from
July 27, 2026 13:54
5d07975 to
7989ad3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
from
July 27, 2026 16:02
7989ad3 to
189bb51
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/tests/switch_state_controller/mod.rs (1)
920-964: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the direct NMXC continuation.
The table omits
FirmwareUpgrade + ConfigureNmxCluster. That exercises the separate NMXC fallback after firmware completion; a regression there passes every current case. Add an expectation forWaitingForNMXCConfigure.Proposed test case
+ Check { + scenario: "firmware plus NMXC advances to WaitingForNMXCConfigure", + input: CaseInput { + activities: vec![ + model::rack::MaintenanceActivity::FirmwareUpgrade { + firmware_version: None, + components: vec![], + force_update: false, + }, + model::rack::MaintenanceActivity::ConfigureNmxCluster, + ], + }, + expect: Expect::ReProvisioning(ReProvisioningState::WaitingForNMXCConfigure), + },As per coding guidelines, table-driven tests should cover input-to-output mappings.
🤖 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/switch_state_controller/mod.rs` around lines 920 - 964, Add a table-driven case to test_switch_waiting_for_rack_firmware_upgrade_next_state_by_activities covering FirmwareUpgrade plus ConfigureNmxCluster, and assert that it produces ReProvisioningState::WaitingForNMXCConfigure. Keep the existing cases unchanged.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/tests/switch_state_controller/mod.rs`:
- Around line 920-964: Add a table-driven case to
test_switch_waiting_for_rack_firmware_upgrade_next_state_by_activities covering
FirmwareUpgrade plus ConfigureNmxCluster, and assert that it produces
ReProvisioningState::WaitingForNMXCConfigure. Keep the existing cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: af91b540-0a5c-493c-b11f-9da120fc90e7
📒 Files selected for processing (28)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/error_state.rscrates/api-core/src/tests/power_shelf_state_controller/maintenance.rscrates/api-core/src/tests/power_shelf_state_controller/mod.rscrates/api-core/src/tests/power_shelf_state_controller/reprovisioning.rscrates/api-core/src/tests/rack_state_controller/handler.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-db/migrations/20260724120000_power_shelf_reprovisioning_requested.sqlcrates/api-db/src/power_shelf.rscrates/api-db/src/switch.rscrates/api-model/src/power_shelf/mod.rscrates/api-model/src/power_shelf/slas.rscrates/api-model/src/switch/mod.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/handler.rscrates/power-shelf-controller/src/io.rscrates/power-shelf-controller/src/lib.rscrates/power-shelf-controller/src/ready.rscrates/power-shelf-controller/src/reprovisioning.rscrates/rack-controller/src/maintenance.rscrates/rpc/src/model/power_shelf.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/reprovisioning.rs
🚧 Files skipped from review as they are similar to previous changes (27)
- crates/api-core/src/cfg/README.md
- crates/api-db/migrations/20260724120000_power_shelf_reprovisioning_requested.sql
- crates/rpc/src/model/power_shelf.rs
- crates/api-core/src/cfg/file.rs
- crates/api-core/src/tests/power_shelf_state_controller/maintenance.rs
- crates/power-shelf-controller/src/io.rs
- crates/api-core/src/tests/power_shelf_state_controller/error_state.rs
- crates/power-shelf-controller/src/lib.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/power-shelf-controller/src/handler.rs
- crates/api-db/src/switch.rs
- crates/api-model/src/power_shelf/slas.rs
- crates/api-core/src/setup.rs
- crates/power-shelf-controller/src/ready.rs
- crates/api-model/src/switch/mod.rs
- crates/power-shelf-controller/src/context.rs
- crates/api-core/src/test_support/default_config.rs
- crates/switch-controller/src/reprovisioning.rs
- crates/api-core/src/tests/power_shelf_state_controller/reprovisioning.rs
- crates/api-core/src/tests/power_shelf_state_controller/mod.rs
- crates/power-shelf-controller/src/reprovisioning.rs
- crates/api-core/src/handlers/component_manager.rs
- crates/switch-controller/src/ready.rs
- crates/api-db/src/power_shelf.rs
- crates/api-model/src/power_shelf/mod.rs
- crates/api-core/src/tests/rack_state_controller/handler.rs
- crates/rack-controller/src/maintenance.rs
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
from
July 27, 2026 16:37
189bb51 to
179d8ce
Compare
Route state-controller firmware requests for power shelves through rack maintenance, alongside machines and switches. Persist per-device reprovisioning requests and firmware status so rack orchestration can synchronize component progress and terminal outcomes. Add the power-shelf reprovisioning state machine behind an opt-in configuration flag, including completion, failure, stale-cycle, and rack-error handling. Tighten switch reprovisioning behavior and expand rack maintenance coordination to handle mixed component scopes, retries, and status propagation. Add the database migration, model and RPC wiring, controller configuration, and integration coverage for rack, switch, and power-shelf firmware workflows. Signed-off-by: Vinod Chitrali <vchitrali@nvidia.com>
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
from
July 27, 2026 16:37
179d8ce to
241f0a0
Compare
zhaozhongn
approved these changes
Jul 27, 2026
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
2 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.
Route state-controller firmware requests for power shelves through rack maintenance, alongside machines and switches. Persist per-device reprovisioning requests and firmware status so rack orchestration can synchronize component progress and terminal outcomes.
Add the power-shelf reprovisioning state machine behind an opt-in configuration flag, including completion, failure, stale-cycle, and rack-error handling. Tighten switch reprovisioning behavior and expand rack maintenance coordination to handle mixed component scopes, retries, and status propagation.
Add the database migration, model and RPC wiring, controller configuration, and integration coverage for rack, switch, and power-shelf firmware workflows.
Related issues
#4088
Type of Change
Breaking Changes
Testing
Additional Notes
NONE