feat(rack): coordinate power shelf, compute , switch firmware upgrades across rack components#4089
feat(rack): coordinate power shelf, compute , switch firmware upgrades across rack components#4089vinodchitraliNVIDIA wants to merge 1 commit into
Conversation
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.
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) | ~90 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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, |
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.
| 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.
| if !req.initiator.starts_with("rack-") { | ||
| tracing::warn!( | ||
| initiator = %req.initiator, | ||
| "Unknown initiator for power shelf reprovisioning request", | ||
| ); | ||
| return Ok(StateHandlerOutcome::transition( | ||
| PowerShelfControllerState::Error { | ||
| cause: format!( | ||
| "unknown initiator for power shelf reprovisioning request: {}", | ||
| req.initiator | ||
| ), | ||
| }, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the reprovisioning request before transitioning to Error.
Unlike the two sibling early-return branches (flag-disabled, no-relevant-activities), this branch leaves power_shelf_reprovisioning_requested intact. If an operator manually clears the resulting Error state, the stale request — still carrying the same unrecognized initiator — will be re-evaluated on the next Ready pass and immediately send the power shelf back into Error, producing an unrecoverable loop without direct DB surgery.
🔧 Proposed fix: clear the request before transitioning to Error
if !req.initiator.starts_with("rack-") {
tracing::warn!(
initiator = %req.initiator,
"Unknown initiator for power shelf reprovisioning request",
);
+ let mut txn = ctx.services.db_pool.begin().await?;
+ db_power_shelf::clear_power_shelf_reprovisioning_requested(
+ txn.as_mut(),
+ *power_shelf_id,
+ )
+ .await?;
return Ok(StateHandlerOutcome::transition(
PowerShelfControllerState::Error {
cause: format!(
"unknown initiator for power shelf reprovisioning request: {}",
req.initiator
),
},
- ));
+ )
+ .with_txn(txn));
}📝 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.
| if !req.initiator.starts_with("rack-") { | |
| tracing::warn!( | |
| initiator = %req.initiator, | |
| "Unknown initiator for power shelf reprovisioning request", | |
| ); | |
| return Ok(StateHandlerOutcome::transition( | |
| PowerShelfControllerState::Error { | |
| cause: format!( | |
| "unknown initiator for power shelf reprovisioning request: {}", | |
| req.initiator | |
| ), | |
| }, | |
| )); | |
| } | |
| if !req.initiator.starts_with("rack-") { | |
| tracing::warn!( | |
| initiator = %req.initiator, | |
| "Unknown initiator for power shelf reprovisioning request", | |
| ); | |
| let mut txn = ctx.services.db_pool.begin().await?; | |
| db_power_shelf::clear_power_shelf_reprovisioning_requested( | |
| txn.as_mut(), | |
| *power_shelf_id, | |
| ) | |
| .await?; | |
| return Ok(StateHandlerOutcome::transition( | |
| PowerShelfControllerState::Error { | |
| cause: format!( | |
| "unknown initiator for power shelf reprovisioning request: {}", | |
| req.initiator | |
| ), | |
| }, | |
| ) | |
| .with_txn(txn)); | |
| } |
🤖 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/power-shelf-controller/src/ready.rs` around lines 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.
| if !req.initiator.starts_with("rack-") { | ||
| tracing::warn!( | ||
| initiator = %req.initiator, | ||
| "Unknown initiator for switch reprovisioning request", | ||
| ); | ||
| return Ok(StateHandlerOutcome::transition( | ||
| SwitchControllerState::ReProvisioning { | ||
| reprovisioning_state: ReProvisioningState::WaitingForRackFirmwareUpgrade, | ||
| SwitchControllerState::Error { | ||
| cause: format!( | ||
| "unknown initiator for switch reprovisioning request: {}", | ||
| req.initiator | ||
| ), | ||
| }, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalid-initiator path leaves a stale switch_reprovisioning_requested.
Every other transition into Error in this reprovisioning flow (e.g. reprovisioning.rs lines 204-214, 262-272) clears the persisted request via clear_switch_reprovisioning_requested before erroring out. This branch skips that step, so the switch enters Error while still carrying the invalid request. If the switch is later recovered to Ready, handle_ready will immediately re-evaluate the same stale, invalid request and bounce right back into Error.
🐛 Proposed fix
if !req.initiator.starts_with("rack-") {
tracing::warn!(
initiator = %req.initiator,
"Unknown initiator for switch reprovisioning request",
);
+ let mut txn = ctx.services.db_pool.begin().await?;
+ db_switch::clear_switch_reprovisioning_requested(txn.as_mut(), *switch_id).await?;
return Ok(StateHandlerOutcome::transition(
SwitchControllerState::Error {
cause: format!(
"unknown initiator for switch reprovisioning request: {}",
req.initiator
),
},
- ));
+ )
+ .with_txn(txn));
}📝 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.
| if !req.initiator.starts_with("rack-") { | |
| tracing::warn!( | |
| initiator = %req.initiator, | |
| "Unknown initiator for switch reprovisioning request", | |
| ); | |
| return Ok(StateHandlerOutcome::transition( | |
| SwitchControllerState::ReProvisioning { | |
| reprovisioning_state: ReProvisioningState::WaitingForRackFirmwareUpgrade, | |
| SwitchControllerState::Error { | |
| cause: format!( | |
| "unknown initiator for switch reprovisioning request: {}", | |
| req.initiator | |
| ), | |
| }, | |
| )); | |
| } | |
| if !req.initiator.starts_with("rack-") { | |
| tracing::warn!( | |
| initiator = %req.initiator, | |
| "Unknown initiator for switch reprovisioning request", | |
| ); | |
| let mut txn = ctx.services.db_pool.begin().await?; | |
| db_switch::clear_switch_reprovisioning_requested(txn.as_mut(), *switch_id).await?; | |
| return Ok(StateHandlerOutcome::transition( | |
| SwitchControllerState::Error { | |
| cause: format!( | |
| "unknown initiator for switch reprovisioning request: {}", | |
| req.initiator | |
| ), | |
| }, | |
| ) | |
| .with_txn(txn)); | |
| } |
🤖 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/switch-controller/src/ready.rs` around lines 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.
| fn should_run(activities: &[MaintenanceActivity], activity: &MaintenanceActivity) -> bool { | ||
| activities.is_empty() || activities.iter().any(|a| a.same_kind(activity)) | ||
| } | ||
|
|
||
| fn nvos_update_requested(activities: &[MaintenanceActivity]) -> bool { | ||
| activities | ||
| .iter() | ||
| .any(|activity| matches!(activity, MaintenanceActivity::NvosUpdate { .. })) | ||
| } | ||
|
|
||
| fn configure_nmx_cluster_requested(activities: &[MaintenanceActivity]) -> bool { | ||
| should_run(activities, &MaintenanceActivity::ConfigureNmxCluster) | ||
| } | ||
|
|
||
| fn firmware_upgrade_requested(activities: &[MaintenanceActivity]) -> bool { | ||
| should_run( | ||
| activities, | ||
| &MaintenanceActivity::FirmwareUpgrade { | ||
| firmware_version: None, | ||
| components: vec![], | ||
| force_update: false, | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| /// First ReProvisioning sub-state to enter from Ready, based on the request | ||
| /// activities. Empty activities means all phases. Returns `None` when the | ||
| /// activities list has no switch-relevant wait phases. | ||
| pub(crate) fn first_reprovisioning_state( | ||
| request: &SwitchReprovisionRequest, | ||
| ) -> Option<ReProvisioningState> { | ||
| let activities = &request.activities; | ||
| if firmware_upgrade_requested(activities) { | ||
| return Some(ReProvisioningState::WaitingForRackFirmwareUpgrade); | ||
| } | ||
| if nvos_update_requested(activities) { | ||
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | ||
| } | ||
| if configure_nmx_cluster_requested(activities) { | ||
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// Next ReProvisioning sub-state after firmware completes. | ||
| fn next_state_after_firmware(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | ||
| let activities = &request.activities; | ||
| if nvos_update_requested(activities) { | ||
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | ||
| } | ||
| if configure_nmx_cluster_requested(activities) { | ||
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// Next ReProvisioning sub-state after NVOS completes. | ||
| fn next_state_after_nvos(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | ||
| configure_nmx_cluster_requested(&request.activities) | ||
| .then_some(ReProvisioningState::WaitingForNMXCConfigure) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
nvos_update_requested breaks the "empty activities = all phases" contract.
should_run (line 40-42) treats an empty activities list as "run every phase," and firmware_upgrade_requested/configure_nmx_cluster_requested both build on it. nvos_update_requested instead does a bare .any() over the slice, which returns false for an empty list — contradicting the doc comment on first_reprovisioning_state ("Empty activities means all phases"). Concretely, with an empty activity list: firmware completes → next_state_after_firmware checks nvos_update_requested (false) → skips straight to NMXC, silently dropping the NVOS upgrade phase for a "reprovision everything" request. No test in crates/api-core/src/tests/switch_state_controller/mod.rs exercises the empty-list case, so this regression would go unnoticed.
🐛 Proposed fix
fn nvos_update_requested(activities: &[MaintenanceActivity]) -> bool {
- activities
- .iter()
- .any(|activity| matches!(activity, MaintenanceActivity::NvosUpdate { .. }))
+ should_run(
+ activities,
+ &MaintenanceActivity::NvosUpdate {
+ config_json: String::new(),
+ },
+ )
}Consider also adding a table-driven test case with an empty activity list asserting the NVOS phase is entered, per the repo's table-driven-test convention for .rs files.
📝 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.
| fn should_run(activities: &[MaintenanceActivity], activity: &MaintenanceActivity) -> bool { | |
| activities.is_empty() || activities.iter().any(|a| a.same_kind(activity)) | |
| } | |
| fn nvos_update_requested(activities: &[MaintenanceActivity]) -> bool { | |
| activities | |
| .iter() | |
| .any(|activity| matches!(activity, MaintenanceActivity::NvosUpdate { .. })) | |
| } | |
| fn configure_nmx_cluster_requested(activities: &[MaintenanceActivity]) -> bool { | |
| should_run(activities, &MaintenanceActivity::ConfigureNmxCluster) | |
| } | |
| fn firmware_upgrade_requested(activities: &[MaintenanceActivity]) -> bool { | |
| should_run( | |
| activities, | |
| &MaintenanceActivity::FirmwareUpgrade { | |
| firmware_version: None, | |
| components: vec![], | |
| force_update: false, | |
| }, | |
| ) | |
| } | |
| /// First ReProvisioning sub-state to enter from Ready, based on the request | |
| /// activities. Empty activities means all phases. Returns `None` when the | |
| /// activities list has no switch-relevant wait phases. | |
| pub(crate) fn first_reprovisioning_state( | |
| request: &SwitchReprovisionRequest, | |
| ) -> Option<ReProvisioningState> { | |
| let activities = &request.activities; | |
| if firmware_upgrade_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForRackFirmwareUpgrade); | |
| } | |
| if nvos_update_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | |
| } | |
| if configure_nmx_cluster_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | |
| } | |
| None | |
| } | |
| /// Next ReProvisioning sub-state after firmware completes. | |
| fn next_state_after_firmware(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | |
| let activities = &request.activities; | |
| if nvos_update_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | |
| } | |
| if configure_nmx_cluster_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | |
| } | |
| None | |
| } | |
| /// Next ReProvisioning sub-state after NVOS completes. | |
| fn next_state_after_nvos(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | |
| configure_nmx_cluster_requested(&request.activities) | |
| .then_some(ReProvisioningState::WaitingForNMXCConfigure) | |
| } | |
| fn should_run(activities: &[MaintenanceActivity], activity: &MaintenanceActivity) -> bool { | |
| activities.is_empty() || activities.iter().any(|a| a.same_kind(activity)) | |
| } | |
| fn nvos_update_requested(activities: &[MaintenanceActivity]) -> bool { | |
| should_run( | |
| activities, | |
| &MaintenanceActivity::NvosUpdate { | |
| config_json: String::new(), | |
| }, | |
| ) | |
| } | |
| fn configure_nmx_cluster_requested(activities: &[MaintenanceActivity]) -> bool { | |
| should_run(activities, &MaintenanceActivity::ConfigureNmxCluster) | |
| } | |
| fn firmware_upgrade_requested(activities: &[MaintenanceActivity]) -> bool { | |
| should_run( | |
| activities, | |
| &MaintenanceActivity::FirmwareUpgrade { | |
| firmware_version: None, | |
| components: vec![], | |
| force_update: false, | |
| }, | |
| ) | |
| } | |
| /// First ReProvisioning sub-state to enter from Ready, based on the request | |
| /// activities. Empty activities means all phases. Returns `None` when the | |
| /// activities list has no switch-relevant wait phases. | |
| pub(crate) fn first_reprovisioning_state( | |
| request: &SwitchReprovisionRequest, | |
| ) -> Option<ReProvisioningState> { | |
| let activities = &request.activities; | |
| if firmware_upgrade_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForRackFirmwareUpgrade); | |
| } | |
| if nvos_update_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | |
| } | |
| if configure_nmx_cluster_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | |
| } | |
| None | |
| } | |
| /// Next ReProvisioning sub-state after firmware completes. | |
| fn next_state_after_firmware(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | |
| let activities = &request.activities; | |
| if nvos_update_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNVOSUpgrade); | |
| } | |
| if configure_nmx_cluster_requested(activities) { | |
| return Some(ReProvisioningState::WaitingForNMXCConfigure); | |
| } | |
| None | |
| } | |
| /// Next ReProvisioning sub-state after NVOS completes. | |
| fn next_state_after_nvos(request: &SwitchReprovisionRequest) -> Option<ReProvisioningState> { | |
| configure_nmx_cluster_requested(&request.activities) | |
| .then_some(ReProvisioningState::WaitingForNMXCConfigure) | |
| } |
🤖 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/switch-controller/src/reprovisioning.rs` around lines 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.
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