feat(nico-api): define rack profile, component manager and RMS ts globally - #4211
Conversation
…ts globally The rack profile, component manager and RMS sections only existed in per-site config, so every site running rack management had to restate all of them. Move a deployment-wide baseline into the global config so sites only override what actually differs. rack_profiles carries the standard NVL72 bills of materials for GB200 and GB300. The component manager backends are rms, which is both the recommended backend for rack trays and what nico-api already defaults to. The state controller flags are on because state-controlled dispatch is the recommended mode; the field default of false is the legacy direct-dispatch path, which a site can still select. The rack profile and component manager sections are coupled: an rms backend requires a non-empty rack_profiles and configuration loading fails without one. Setting [rms].api_url here enables the component manager fleet-wide rather than only supplying a default. Its client is constructed lazily and never verifies connectivity, so a site that does not run RMS builds one too, and its rack operations fail against an unreachable endpoint instead of reporting that no component manager is configured. Sites can still point elsewhere through their own [rms].api_url. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
WalkthroughAdds NVL72 rack profile expectations and configures the component manager to use RMS backends and state controllers for compute trays, NV switches, and power shelves, with TLS enforced for the RMS endpoint. ChangesRack profile and component-manager configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@helm/charts/nico-api/files/carbide-api-config.toml`:
- Around line 146-154: Update the [component_manager] defaults so RMS is not
applied fleet-wide without availability protection: preserve a per-site non-RMS
backend override or add a preflight/rollout gate before enabling the RMS
backends and state controllers. Verify the rendered Helm configuration and
upgrade behavior, and require testing in non-critical environments before
production rollout.
🪄 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: 07c7d11a-7c4b-4569-8c7d-6712afa53ee0
📒 Files selected for processing (1)
helm/charts/nico-api/files/carbide-api-config.toml
| # Defining this section is what enables the component manager. | ||
| [component_manager] | ||
| compute_tray_backend = "rms" | ||
| nv_switch_backend = "rms" | ||
| power_shelf_backend = "rms" | ||
|
|
||
| nv_switch_use_state_controller = true | ||
| power_shelf_use_state_controller = true | ||
| compute_tray_use_state_controller = true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not make RMS the fleet-wide backend without an availability gate.
These settings route compute trays, NV switches, and power shelves through RMS for every rendered deployment. Sites without a working RMS service will start with configured backends but fail operations against the endpoint. Preserve a per-site non-RMS override or add a preflight/rollout gate before applying this global default.
As per coding guidelines, treat NICo as experimental software and test deployments in non-critical environments before production use. As per path instructions, verify the rendered Helm configuration and upgrade behavior before rollout.
🤖 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 `@helm/charts/nico-api/files/carbide-api-config.toml` around lines 146 - 154,
Update the [component_manager] defaults so RMS is not applied fleet-wide without
availability protection: preserve a per-site non-RMS backend override or add a
preflight/rollout gate before enabling the RMS backends and state controllers.
Verify the rendered Helm configuration and upgrade behavior, and require testing
in non-critical environments before production rollout.
Sources: Coding guidelines, Path instructions
| count = 6 | ||
|
|
||
| # Defining this section is what enables the component manager. | ||
| [component_manager] |
There was a problem hiding this comment.
Major: unconditional enablement is a one-way door for non-RMS sites. The PR description itself notes that a site not running RMS builds a client anyway and its rack operations "fail against an unreachable endpoint instead of reporting that no component manager is configured." Site TOML can override values but cannot un-define this section once the global config declares it — the only opt-out is the undocumented [rms] api_url = "" escape hatch (setup.rs treats an empty URL as unset), which still leaves component-manager enabled with rms backends and no client. Suggest gating these sections behind chart values instead of shipping them unconditionally — #4218 adds exactly that shape for [rms] (same file, so we need to reconcile the two before either merges; happy to combine them).
|
|
||
| [rms] | ||
| api_url = "https://rms-api-server.rack-manager.svc.cluster.local:8801" | ||
| enforce_tls = true |
There was a problem hiding this comment.
Major: this default doesn't match the rack-manager chart as deployed. The RMS chart ships tls.enabled: false / allowInsecure: true (plain HTTP on 8801), and the config validated on RMS-consuming sites was http://rms-api-server.rack-manager:8801 with enforce_tls = false. A site following both charts' defaults gets a TLS handshake failure on every rack operation. Preferring TLS is the right instinct, but either match the RMS chart as it ships today, or document the RMS-side TLS prerequisite in this comment block.
There was a problem hiding this comment.
Correction after reading the Launchpad RMS runbook: the current validated deployment IS mTLS (apiServer.tls.enabled + caEnabled: true), so https + enforce_tls = true is the right direction — my forged plain-HTTP reference was the older pattern. However this block is still incomplete against that deployment: caEnabled RMS requires a client certificate, and without root_ca_path/client_cert/client_key the handshake fails. The validated trio is nico-api's own SPIFFE mount (already present in every pod, no new mounts): root_ca_path = "/var/run/secrets/spiffe.io/ca.crt", client_cert = ".../tls.crt", client_key = ".../tls.key". #4218 now ships exactly that as its values defaults — happy to converge the two PRs.
There was a problem hiding this comment.
Second correction, now fully resolved: I claimed the missing cert paths would fail the handshake — wrong. nv-rms-client's RmsClientConfig auto-discovers nico-api's SPIFFE mount (/var/run/secrets/spiffe.io/{tls.crt,tls.key,ca.crt}) whenever the [rms] paths are unset, so this PR's bare api_url + enforce_tls = true block is sufficient against the standard mTLS RMS deployment. Apologies for the noise. One behavior worth knowing: if the SPIFFE files were ever absent, the client logs a warning and silently disables TLS enforcement rather than failing — there's no hard-fail mode. My TLS concern on this PR is withdrawn; the remaining discussion is only the unconditional-enablement question above.
| rack_hardware_topology = "gb300_nvl72r1_c2g4_topology" | ||
|
|
||
| [rack_profiles.NVL72_GB300.rack_capabilities.compute] | ||
| vendor = "Lenovo" |
There was a problem hiding this comment.
Medium: OEM-specific vendors as deployment-wide defaults. vendor = "Lenovo" (and LiteOn for power shelves) are per-OEM facts, not properties of the GB300 NVL72 platform. Startup validation checks vendor fields for RMS-enabled roles, so a non-Lenovo GB300 site inherits a default that validates cleanly and then mismatches at component discovery/linking — harder to debug than a missing profile. Since Figment merges site over global per-key, sites can override, but that expectation should be stated here — or name the profile for what it is (e.g. NVL72_GB300_Lenovo) so a site with different hardware knows it must define its own.
4 participants
The rack profile, component manager and RMS sections only existed in per-site config, so every site running rack management had to restate all of them. Move a deployment-wide baseline into the global config so sites only override what actually differs.
rack_profiles carries the standard NVL72 bills of materials for GB200 and GB300. The component manager backends are rms, which is both the recommended backend for rack trays and what nico-api already defaults to. The state controller flags are on because state-controlled dispatch is the recommended mode; the field default of false is the legacy direct-dispatch path, which a site can still select.
The rack profile and component manager sections are coupled: an rms backend requires a non-empty rack_profiles and configuration loading fails without one.
Setting [rms].api_url here enables the component manager fleet-wide rather than only supplying a default. Its client is constructed lazily and never verifies connectivity, so a site that does not run RMS builds one too, and its rack operations fail against an unreachable endpoint instead of reporting that no component manager is configured. Sites can still point elsewhere through their own [rms].api_url.
Related issues
#3299
Type of Change
Breaking Changes
Testing
Additional Notes