feat(rack): add custom attributes to rack profiles - #4152
Conversation
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Summary by CodeRabbit
WalkthroughChangesRack profiles and compute, switch, and power-shelf capabilities now support custom attributes. Configuration merging preserves per-key provider precedence; RMS descriptors inherit rack and role attributes, generated fields take precedence, and startup emits structured collision warnings. Tests cover parsing, merging, validation, conversion boundaries, and RMS requests. Rack profile attribute contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigProviders
participant CarbideConfig
participant RuntimePrelude
participant RMSNodeDescriptor
ConfigProviders->>CarbideConfig: merge rack profile attributes
RuntimePrelude->>CarbideConfig: inspect rack profile overrides
RuntimePrelude->>RuntimePrelude: emit structured warnings
CarbideConfig->>RMSNodeDescriptor: provide rack and role attributes
RMSNodeDescriptor->>RMSNodeDescriptor: apply generated field precedence
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rack/src/rms_node_type.rs (1)
182-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared vendor/product_family "presence" logic.
The trimmed, non-blank presence check for
vendorandproduct_familyhere duplicates the identical rule already implemented innode_identity_for_profile(Lines 283-285 for vendor, Lines 272-276 for product family). If the blank-string definition ever changes in one spot, these warnings could silently misreport what the actual descriptor construction does.♻️ Proposed shared helper
+fn present_str(value: Option<&str>) -> Option<&str> { + value.map(str::trim).filter(|value| !value.is_empty()) +} + fn node_identity_for_profile( profile: &RackProfile, role: RmsNodeRole, ) -> Result<RmsNodeIdentity, NodeDescriptorError> { - let Some(product_family) = profile - .product_family - .as_ref() - .map(|family| family.as_str()) - .filter(|family| !family.is_empty()) - else { + let Some(product_family) = present_str(profile.product_family.as_ref().map(|f| f.as_str())) + else { return Err(NodeDescriptorError::MissingProductFamily); }; let (vendor, role_attributes) = vendor_and_attributes_for_role(profile, role); - let Some(vendor) = vendor.map(str::trim).filter(|vendor| !vendor.is_empty()) else { + let Some(vendor) = present_str(vendor) else { return Err(NodeDescriptorError::VendorMissing { role: role.label() }); };let named_attribute_keys = [ (KEY_ROLE, true), - ( - KEY_VENDOR, - vendor - .map(str::trim) - .is_some_and(|vendor| !vendor.is_empty()), - ), - ( - KEY_PRODUCT_FAMILY, - profile - .product_family - .as_ref() - .is_some_and(|family| !family.as_str().is_empty()), - ), + (KEY_VENDOR, present_str(vendor).is_some()), + ( + KEY_PRODUCT_FAMILY, + present_str(profile.product_family.as_ref().map(|f| f.as_str())).is_some(), + ), ];🤖 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/src/rms_node_type.rs` around lines 182 - 224, Extract the trimmed, non-blank presence checks for vendor and product_family into a shared helper or reusable presence logic, then reuse it in this named_attribute_keys construction and node_identity_for_profile. Preserve the existing treatment of missing, whitespace-only, and populated values so override warnings match descriptor construction.
🤖 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/rack/src/rms_node_type.rs`:
- Around line 182-224: Extract the trimmed, non-blank presence checks for vendor
and product_family into a shared helper or reusable presence logic, then reuse
it in this named_attribute_keys construction and node_identity_for_profile.
Preserve the existing treatment of missing, whitespace-only, and populated
values so override warnings match descriptor construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ec53768-89fe-4db0-8819-e24afdc8874c
📒 Files selected for processing (8)
crates/api-core/src/bootstrap.rscrates/api-core/src/setup.rscrates/api-core/src/tests/rack_state_controller/handler.rscrates/api-core/tests/integration/expected_rack.rscrates/api-model/src/rack_type.rscrates/rack/src/rms_node_type.rscrates/rpc/src/model/rack_type.rscrates/site-explorer/tests/integration/machine_creator.rs
|
NICo already has "config overload" (i.e. far too many configurations possible) - would this not be better served by APIs instead? Must they be config? |
Agreed on the config overload. Right now rack profile is used as the source for NICo to RMS RPCs for hardware type resolution. In the short term, using rack profiles in the config seems to be more straightforward to the users compared to using APIs - the source config still needs to come from the operator. But yes, if the benefits are clear, we can move rack profiles to the DB and manage them via APIs in the future. |
Rack profiles currently provide predefined fields. They cannot carry optional key-value attributes/metadata that may be used to identify specific hardware variants or select workflows.
This PR adds attributes that can be configured at rack and component-role levels:
Attributes use the following precedence, from lowest to highest priority:
The value must be
stringtyped. Thevendorandproduct_familyattributes do not populate the corresponding named rack-profile fields.Related issues
Resolves #4146
Type of Change
Breaking Changes
Testing