feat(scale-up-fabric): support RMS v2 fabric manager configuration#4158
Open
jayzhudev wants to merge 1 commit into
Open
feat(scale-up-fabric): support RMS v2 fabric manager configuration#4158jayzhudev wants to merge 1 commit into
jayzhudev wants to merge 1 commit into
Conversation
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
added
the
rack lifecycle
Contributor
WalkthroughChangesRMS v2 fabric manager workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MaintenanceHandler
participant CertificateWorkflow
participant FabricWorkflow
participant RMS
participant StatusVerification
MaintenanceHandler->>CertificateWorkflow: submit V2 certificate jobs
CertificateWorkflow->>FabricWorkflow: transition after completion
FabricWorkflow->>RMS: submit rack-wide fabric configuration
RMS-->>FabricWorkflow: return asynchronous job_id
FabricWorkflow->>RMS: poll job status
RMS-->>FabricWorkflow: return completion state
FabricWorkflow->>StatusVerification: verify completed configuration
StatusVerification->>RMS: retrieve fabric and switch statuses
RMS-->>StatusVerification: return observed statuses
StatusVerification-->>MaintenanceHandler: persist status and complete maintenance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jayzhudev
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.
🧹 Nitpick comments (3)
crates/api-core/src/tests/rack_state_controller/handler.rs (1)
3681-3722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the returned node IDs, not just counts. The node payloads include IDs, so these assertions should compare the sorted IDs from each request against
switch_ids; count-only checks still pass for duplicated or wrong switches. Apply the same check tostatus_nodesandfabric_manager_status_nodes.🤖 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/rack_state_controller/handler.rs` around lines 3681 - 3722, The assertions in the test should validate node identities rather than only lengths. In the test block containing configure_nodes, status_nodes, and fabric_manager_status_nodes, extract each payload’s node IDs, sort them, and compare them with the sorted switch_ids for all three requests while preserving the existing request-count assertions.crates/rack-controller/src/maintenance.rs (1)
1704-1714: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the RMS client-resolution preamble.
This "prefer the NMX-configure client, fall back to the service client" sequence now appears verbatim at lines 1554-1563 and here, alongside the older
if let/elsevariants at lines 536-553, 2698-2714 and 2845-2861. A single helper returning the resolved client (with theArcclone where the caller must outlive a later&mut ctxborrow, as this call site requires for line 1785) would collapse five near-copies and make the lifetime distinction deliberate rather than incidental.Entirely optional — the current code is correct as written.
🤖 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/rack-controller/src/maintenance.rs` around lines 1704 - 1714, Extract the repeated RMS client-resolution logic into a shared helper, including NMX-configured client preference and service-client fallback. Update the variants around this preamble and the older if-let branches to use the helper, preserving an owned Arc clone where callers such as the current maintenance flow need the client to outlive a later mutable ctx borrow.crates/rack-controller/src/fabric_manager.rs (1)
671-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress this as a
check_valuestable rather than a hand-rolled loop.
observe_primary_casealready reduces the fallible call to a totalPrimaryObservation, so this is a textbook input→output mapping.check_valuesis imported at line 465 and used bytest_fabric_manager_status_from_entryin this same module; aligning keeps scenario reporting uniform across the suite.As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs or errors. Use
scenarios!withOutcomefor fallible operations,value_scenarios!for total operations, and directcheck_cases/check_valueswhen macros obscure intent."♻️ Proposed refactor to the established table-driven form
#[test] fn observed_primary_switch_classifies_status_responses() { let second_id = SwitchId::new(SwitchIdSource::Tpm, [2; 32], SwitchType::NvLink).to_string(); - for (scenario, input, expect) in [ - ( - "one enabled switch is the observed primary", - PrimaryResponseCase::Selected, - PrimaryObservation::Primary(second_id), - ), - ( - "RMS failure can converge", - PrimaryResponseCase::RmsFailure, - PrimaryObservation::Retryable, - ), - ( - "invalid RMS status violates the contract", - PrimaryResponseCase::InvalidStatus, - PrimaryObservation::Terminal, - ), - ( - "missing fabric status violates the contract", - PrimaryResponseCase::MissingFabricStatus, - PrimaryObservation::Terminal, - ), - ( - "unexpected switch violates the contract", - PrimaryResponseCase::UnexpectedSwitch, - PrimaryObservation::Terminal, - ), - ( - "duplicate switch violates the contract", - PrimaryResponseCase::DuplicateSwitch, - PrimaryObservation::Terminal, - ), - ( - "per-switch inspection failure can recover", - PrimaryResponseCase::InspectionFailure, - PrimaryObservation::Retryable, - ), - ( - "omitted switch violates the contract", - PrimaryResponseCase::OmittedSwitch, - PrimaryObservation::Terminal, - ), - ( - "no enabled switch can converge", - PrimaryResponseCase::NoPrimary, - PrimaryObservation::Retryable, - ), - ( - "multiple enabled switches can converge", - PrimaryResponseCase::MultiplePrimaries, - PrimaryObservation::Retryable, - ), - ( - "invalid primary ID violates the contract", - PrimaryResponseCase::InvalidPrimaryId, - PrimaryObservation::Terminal, - ), - ] { - assert_eq!(observe_primary_case(input), expect, "{scenario}"); - } + check_values( + [ + Check { + scenario: "one enabled switch is the observed primary", + input: PrimaryResponseCase::Selected, + expect: PrimaryObservation::Primary(second_id), + }, + Check { + scenario: "RMS failure can converge", + input: PrimaryResponseCase::RmsFailure, + expect: PrimaryObservation::Retryable, + }, + Check { + scenario: "invalid RMS status violates the contract", + input: PrimaryResponseCase::InvalidStatus, + expect: PrimaryObservation::Terminal, + }, + Check { + scenario: "missing fabric status violates the contract", + input: PrimaryResponseCase::MissingFabricStatus, + expect: PrimaryObservation::Terminal, + }, + Check { + scenario: "unexpected switch violates the contract", + input: PrimaryResponseCase::UnexpectedSwitch, + expect: PrimaryObservation::Terminal, + }, + Check { + scenario: "duplicate switch violates the contract", + input: PrimaryResponseCase::DuplicateSwitch, + expect: PrimaryObservation::Terminal, + }, + Check { + scenario: "per-switch inspection failure can recover", + input: PrimaryResponseCase::InspectionFailure, + expect: PrimaryObservation::Retryable, + }, + Check { + scenario: "omitted switch violates the contract", + input: PrimaryResponseCase::OmittedSwitch, + expect: PrimaryObservation::Terminal, + }, + Check { + scenario: "no enabled switch can converge", + input: PrimaryResponseCase::NoPrimary, + expect: PrimaryObservation::Retryable, + }, + Check { + scenario: "multiple enabled switches can converge", + input: PrimaryResponseCase::MultiplePrimaries, + expect: PrimaryObservation::Retryable, + }, + Check { + scenario: "invalid primary ID violates the contract", + input: PrimaryResponseCase::InvalidPrimaryId, + expect: PrimaryObservation::Terminal, + }, + ], + observe_primary_case, + ); }🤖 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/rack-controller/src/fabric_manager.rs` around lines 671 - 733, Replace the hand-rolled loop in observed_primary_switch_classifies_status_responses with the imported check_values table-driven assertion, preserving all existing scenario inputs, expected PrimaryObservation values, and scenario labels. Keep the test focused on the total observe_primary_case input-to-output mapping and follow the check_values usage established by test_fabric_manager_status_from_entry.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/src/tests/rack_state_controller/handler.rs`:
- Around line 3681-3722: The assertions in the test should validate node
identities rather than only lengths. In the test block containing
configure_nodes, status_nodes, and fabric_manager_status_nodes, extract each
payload’s node IDs, sort them, and compare them with the sorted switch_ids for
all three requests while preserving the existing request-count assertions.
In `@crates/rack-controller/src/fabric_manager.rs`:
- Around line 671-733: Replace the hand-rolled loop in
observed_primary_switch_classifies_status_responses with the imported
check_values table-driven assertion, preserving all existing scenario inputs,
expected PrimaryObservation values, and scenario labels. Keep the test focused
on the total observe_primary_case input-to-output mapping and follow the
check_values usage established by test_fabric_manager_status_from_entry.
In `@crates/rack-controller/src/maintenance.rs`:
- Around line 1704-1714: Extract the repeated RMS client-resolution logic into a
shared helper, including NMX-configured client preference and service-client
fallback. Update the variants around this preamble and the older if-let branches
to use the helper, preserving an owned Arc clone where callers such as the
current maintenance flow need the client to outlive a later mutable ctx borrow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 458ee8ce-7f6e-46a3-ab4b-eaef103da77f
📒 Files selected for processing (7)
crates/api-core/src/cfg/README.mdcrates/api-core/src/tests/rack_state_controller/handler.rscrates/api-model/src/rack.rscrates/rack-controller/src/config.rscrates/rack-controller/src/fabric_manager.rscrates/rack-controller/src/maintenance.rscrates/rack/src/rms_client.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
1 participant
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.
Add support for the
librms v0.10.0ConfigureScaleUpFabricManagerV2async RPC.The V1 requires NICo to coordinate multiple RMS RPCs: disable the cluster state across all switches, query switch tray indexes to select a primary, synchronously configure that primary, and then query fabric status separately. V2 moves this coordination into RMS as a scale-up fabric-wide desired-state reconciliation job.
With the V2 RPC, NICo gains a stronger completion contract: RMS selects the primary, reconciles the scale-up fabric, waits for NMX-C to reach configured state, and verifies final switch and topology readback before completing the job. NICo submits the group of switches and desired topology, polls the job, then reads and persists the RMS-selected primary and per-switch Fabric Manager status.
V1 remains the default for compatibility.
To enable V2 under the RMS configuration:
If unset,
v1is used.Related issues
Resolves #1923
Type of Change
Breaking Changes
V1 remains the default, so existing deployments retain current behavior.
Testing
Manual Testing
Validated against two switches using RMS and the V2 selector.
GetScaleUpFabricStatusafter completion and persisted the observed primary and Fabric Manager status.BatchSetScaleUpFabricStatecalls were observed.Selected redacted logs:
Trimmed completed job response:
{ "executionState": "JOB_EXECUTION_STATE_COMPLETED", "stateDescription": "Completed", "result": { "topology_type": "gb200_nvl36r1_c2g4_topology", "switches": [ { "node_id": "<primary-switch-id>", "enabled": true, "fabric_manager_status": "ok" }, { "node_id": "<non-primary-switch-id>", "enabled": false, "fabric_manager_status": "" } ] } }Persisted NICo state:
Repeat reconciliation evidence:
Additional notes:
REFUSED_STREAMduring final readback. RMS rolled both switches back to disabled; retrying the same desired state passed.Simulation Testing
Tested scenarios:
scale_up_fabric_manager_api_version = "v2"and verified selection of the V2 workflow.BatchSetScaleUpFabricState(false)and device-info preparation calls.ConfigureScaleUpFabricManagerV2 request containing the selected topology, all rack switches, and no primary-switch override.GetJobStatus, and continued only after job completion.GetScaleUpFabricStatusafter completion, accepted the sole primary selected by RMS, and replaced the previously stored primary.