OSAC-2675: Design for BareMetalInstanceTypes - #119
Conversation
|
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 an OSAC-1201 design for tenant-discoverable bare-metal instance types, including public/private APIs, protobuf schemas, host-label-based provisioning, fulfillment-service storage and controllers, security, observability, testing, and rollout behavior. ChangesBare Metal Instance Types
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Tenant
participant FulfillmentService
participant BareMetalFulfillmentOperator
participant InventoryBackend
Tenant->>FulfillmentService: List and select BareMetalInstanceType
Tenant->>FulfillmentService: Request BareMetalInstance
FulfillmentService->>BareMetalFulfillmentOperator: Resolve instance_type
BareMetalFulfillmentOperator->>InventoryBackend: FindFreeHost using host_label_selector
InventoryBackend-->>BareMetalFulfillmentOperator: Return matching host
BareMetalFulfillmentOperator-->>FulfillmentService: Report provisioning status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AI Design Review: EP-119Score: 8/8 | Verdict: PASS
Verdict: A well-structured, thorough enhancement proposal that follows established OSAC patterns, provides detailed proto schemas and workflow descriptions, and addresses error handling, version skew, and security comprehensively—minor gaps in template conformance (missing User Stories section) and a field requiredness ambiguity do not undermine the design's overall quality. Feedback: Clarify the validation semantics for the Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@enhancements/baremetal-instance-types/README.md`:
- Around line 199-214: The BareMetalInstanceSpec proposal must define a single
compatibility contract for instance_type rather than relying on proto3
requiredness. Update the BareMetalInstance integration documentation and
corresponding sections around the operator behavior to specify explicit
validation and precedence: use catalog-item-only provisioning only when
instance_type is absent, and reject or hold requests that require label
selection when the operator does not support instance_type.
- Around line 185-190: Update BareMetalAcceleratorSpec and its README example
consistently so dual accelerators can be represented: add a validated count
field to the schema and use it for the documented A100 example, or model
accelerators as repeated entries. Ensure the chosen representation is valid in
the schema and matches the example’s API usage.
- Around line 126-128: Update the Operational Impact statements in the README to
remove the claim that Fulfillment Service downtime never affects in-flight
provisioning. Either narrow the guarantee to listing and new instance creation,
or document and implement a durably persisted/cached resolved selector with
explicit recovery behavior across reconciliation and operator restart.
- Around line 76-81: Update the Tenant Usage Workflow and the related
ClusterTemplateNodeSet documentation to designate one canonical field for
cluster node-set selection. Explicitly document how legacy host_type and
bare_metal_instance_type inputs translate to that field, including precedence
when multiple fields are supplied, and ensure the client and catalog-item
guidance uses the same contract.
- Around line 143-160: Add explicit validation invariants for
BareMetalInstanceTypeSpec, BareMetalHardwareSpec, and the related CPU, memory,
storage, accelerator, and network-port fields: require non-empty host_label,
enforce positive capacities and valid core counts, and constrain architecture,
device, and interface values to supported enums. Apply the same validation rules
to selector fields referenced around the additional section, and ensure
malformed data is rejected as promised by the security documentation.
- Around line 410-420: Add a blank line immediately after each Open Questions
subsection heading in the README, including “1. Label Format and Namespace,” “2.
Label Validation at Type Creation Time,” and “3. Integration Simplification
Strategy,” while leaving the surrounding question content unchanged.
- Around line 217-226: Define the canonical selector contract before documenting
the label-based flow: reconcile the proposed {"label": host_label} usage with
the existing FindFreeHost match-expression keys, including hostType and
backend-specific mappings. Explicitly specify the supported selector key and its
exact translation in both OpenStack and Metal3, then update the affected README
sections so the API and backend behavior are consistent.
- Around line 275-284: Update the Security Considerations section to specify
Fulfillment Service-owned authentication and per-RPC authorization, including
method-specific permissions for listing, selecting, creating, updating, and
deleting BareMetalInstanceTypes. Document tenant filtering for global versus
tenant-scoped types and validation of referenced resources during create
operations, and state that these controls are enforced in the Fulfillment
Service rather than relying solely on annotations, RBAC, or OPA policies.
- Around line 306-309: Update the host-selection flow described under
“Idempotency Guarantees” to make claiming crash-safe across retries: introduce a
durable idempotency key or lease lookup that recovers an existing assignment
before calling FindFreeHost or AssignHost, and persist or make immutable the
resolved host_label for each BareMetalInstance so retries reuse the original
host.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 15ddb21a-0c12-481c-ac14-a9e67a0b9621
📒 Files selected for processing (1)
enhancements/baremetal-instance-types/README.md
ajamias
changed the title
|
@ajamias: This pull request references OSAC-1201 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
AI Design Review: EP-119Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design document that introduces BareMetalInstanceType resources with a clean label-based host selection model, comprehensive failure handling, strong security controls, and full backward compatibility — all built on established OSAC patterns. Feedback: Consider adding a formal User Stories section per the template to make the enhancement more accessible to product reviewers who scan for role-based narratives. The open question on label format (free-form vs. namespaced) should be resolved before implementation since it affects the proto schema, inventory client matchExpression format, and operational documentation — leaning toward namespaced labels (e.g., 'osac.openshift.io/hardware-profile=gpu-large') would prevent collisions with other inventory host labels. Finally, consider briefly addressing whether host_label should support multi-label selection in the future, as the current single-string design would require a schema change to support composite hardware profiles. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
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 `@enhancements/baremetal-instance-types/README.md`:
- Around line 232-233: Update the operator authorization table to explicitly
permit the private Get/read RPC used in the fulfillment workflow for retrieving
BareMetalInstanceType and host_label, or document that the operator performs
this read through the public API. Keep the existing Create, Update, Delete, and
Signal permissions unchanged and ensure the authorization guidance matches the
workflow steps.
- Around line 343-349: The idempotent host-claiming design requires inventory
assignment support that is absent from the current AssignHost contract. Update
AssignHost and every inventory backend implementation to accept the
BareMetalInstance ID as an idempotency key and return the existing assignment
for repeated keys, or introduce and use a durable assignment lookup before
creating a new claim; ensure the crash-before-status-persist path cannot
double-claim a host.
- Around line 162-166: Update the documented gpu-large CPU example to include a
positive threads_per_core value, ensuring it satisfies BareMetalCPUSpec
validation; do not leave the field at its default zero value.
- Around line 280-295: Define validation on the ClusterTemplate and
ClusterCatalogItem create paths so every ClusterTemplateNodeSet has at least one
non-empty field between instance_type and host_type. Reject requests before
persistence or publication when both are empty, while preserving the documented
precedence of instance_type over host_type.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d4b51ee2-35f1-40c9-aa6e-2025c4e5eda3
📒 Files selected for processing (1)
enhancements/baremetal-instance-types/README.md
AI Design Review: EP-119Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows established OSAC patterns, provides complete proto schemas with validation, handles edge cases (idempotency, crash recovery, version skew) rigorously, and defines a comprehensive test strategy across all tiers. Feedback: The document is strong overall. Two minor improvements: (1) add formal 'User Stories' in the 'As a [role], I want [action] so that [goal]' format under Motivation to match the template structure — the workflow descriptions are excellent but the user-story framing helps reviewers quickly validate that all personas' needs are met. (2) Consider resolving the label format open question (free-form vs. namespaced) before merge, as it affects the proto schema definition and inventory client integration — deferring it risks a breaking change later. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
Oh, this isn't the PRD - will take a closer look!
ajamias
changed the title
|
@ajamias: This pull request references OSAC-2675 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
|
||
| ### Goals | ||
|
|
||
| - Reuse the existing resource management patterns for consistency with OSAC's architecture |
There was a problem hiding this comment.
What does this mean? I think it would be better to state the use case directly rather than referencing other implementation that might change in the future or the reader might not be familiar with.
| - Reuse the existing resource management patterns for consistency with OSAC's architecture | ||
| - Support both direct BareMetalInstance and Cluster creation, integrating their catalog items with hardware types | ||
| - Enable Cloud Provider Admins to define BareMetalInstanceTypes with host label selectors that the BMaaS operator uses to claim matching inventory hosts during provisioning | ||
| - Maintain backward compatibility with existing catalog_item-based bare metal instance creation |
There was a problem hiding this comment.
Not sure I understand this. You will continue to create bare metal instances through catalog items so what does this compatibility really require?
There was a problem hiding this comment.
We talked about this and I think the goal should be to have the type be mandatory. That can be achieved in multiple steps to make sure CI and such stays green, but the goal of this proposal should be to require users to select a valid instance type.
| - Storage inventory beyond basic local storage metadata | ||
| - Multi-backend inventory collision handling (single backend per deployment) | ||
| - Automatic discovery and creation of BareMetalInstanceTypes from inventory backends | ||
| - Complex lifecycle management with deprecation workflows |
There was a problem hiding this comment.
I would call out that this is lifecycle management of instance types
| Operator->>Inventory: FindFreeHost(matchExpressions={"hostType":"gpu-large"}) | ||
| Inventory-->>Operator: Return matching host | ||
| Operator->>Inventory: AssignHost(host, labels) | ||
| Operator->>FS: Update BareMetalInstance status (provisioned) |
There was a problem hiding this comment.
This isn't really provisioned yet, right?
| | RPC | Service | Authorization | | ||
| |-----|---------|---------------| | ||
| | List, Get | Public | Any authenticated tenant; results filtered by TenancyLogic to globally-visible types plus caller's tenant-scoped types | | ||
| | Get | Private | Operator service account; used during BareMetalInstance provisioning to retrieve the `host_label` | |
There was a problem hiding this comment.
I don't think we want to introduce this. I would rather see whatever is required to do provisioning be in the BareMetalInstance CR.
BMF operator's API is the CR. I don't think it should also have to query the database to do its job.
| | Create, Update, Delete | Private | Cloud Provider Admin role enforced via AttributionLogic; tenant service accounts cannot call private RPCs | | ||
| | Signal | Private | Operator service account only; enforced by private API transport | | ||
|
|
||
| **Tenant filtering:** Globally-visible BareMetalInstanceTypes (empty tenant annotation) are returned to all authenticated callers. Tenant-scoped types are returned only to the owning tenant. This filtering is applied by the public server's TenancyLogic before returning results — not delegated to the caller or to OPA alone. |
There was a problem hiding this comment.
Do we think instance types will ever be scoped to a tenant?
There was a problem hiding this comment.
I don't see the need for tenant-scoped instance types, they are derived from the inventory so they are defined by the system (and end-up in "shared" tenant?). Maybe we would need to restrict tenants to use some instance types in the future, but that should be an RBAC rule, no? I think it's out of scope for the moment.
|
|
||
| **Tenant Isolation Requirements:** | ||
| All BareMetalInstanceType resources include required tenant isolation metadata: | ||
| - `osac.openshift.io/tenant`: Tenant scoping (often empty for globally-visible types) |
There was a problem hiding this comment.
Are these osac DB metadata fields? They look like kubernetes labels or annotations, but we're not creating a CR.
There was a problem hiding this comment.
According to Claude:
Based on my research in the fulfillment-service code, here's how the tenant system actually works:
The osac.openshift.io/tenant Annotation
1. Storage: The tenant value is stored in a tenant column in the database, not just as an annotation
2. Values:
- "shared" - Globally visible to all authenticated users
- "system" - Only visible to system/admin users
- "<tenant-name>" - Only visible to users belonging to that specific tenant
- Empty/missing - This is the key part!
What Happens When the Annotation is Empty or Missing
From the addTenancyFilter logic in the DAO layer:
1. Admin users (who have universal tenant access): See everything regardless of tenant value - no filtering is applied
2. Regular tenant users: The filtering logic is tenant = any($tenant_list) where $tenant_list includes:
- The user's specific tenant(s)
- Always includes "shared" (via SharedTenants.Union(result) in DetermineVisibleTenants)
This means:
- Empty/null tenant = NOT visible to regular users, only to admins
- "shared" tenant = Visible to all authenticated users
- Specific tenant = Only visible to users in that tenant (plus admins)
For BareMetalInstanceTypes
So when we say "globally-visible" with "empty tenant annotation", that actually means:
- Only admins can see them (Cloud Provider Admins)
- Regular tenant users cannot see them
If we want regular tenant users to see BareMetalInstanceTypes, we need to set the tenant to "shared", not empty!
|
|
||
| ### 2. Label Validation at Type Creation Time | ||
|
|
||
| **Question:** Should the fulfillment-service (or operator) validate that the `host_label` on a new BareMetalInstanceType matches at least one actual inventory host at creation time? This would catch coordination errors early but requires the API server to query the inventory backend at write time. |
There was a problem hiding this comment.
I don't think it's currently possible for fulfillment-service to do this. We should leave it to BMF operator I think.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
enhancements/baremetal-instance-types/README.md (3)
148-152: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate MatchExpression operator/value combinations.
The schema allows
InorNotInwith no values and allows values forExistsorDoesNotExist, despite claiming malformed selectors are rejected. Add cross-field validation: require non-empty values forIn/NotInand forbid values forExists/DoesNotExist.Also applies to: 321-321
🤖 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 `@enhancements/baremetal-instance-types/README.md` around lines 148 - 152, Update the MatchExpression schema validation to enforce operator/value combinations: require at least one value when operator is In or NotIn, and require values to be empty when operator is Exists or DoesNotExist. Use the schema’s cross-field validation mechanism while preserving the existing key and operator constraints.
144-152: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReconcile MatchExpression with the inventory selector contract.
host_labelis a repeated Kubernetes-styleMatchExpression, butFindFreeHostis documented as acceptingmap[string]string; the two representations cannot be passed directly. The example also useshardware-profile, while the backend contract requires canonicalhostType. Define the conversion, supported operators, multi-value semantics, and backend translation—or change the schema/API to one canonical representation.#!/bin/bash set -euo pipefail rg -n 'FindFreeHost|matchExpressions|hostType|resource_class|osac.openshift.io/instance-type' \ enhancements/bare-metal-fulfillment enhancements/baremetal-instance-types/README.mdAlso applies to: 232-245
🤖 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 `@enhancements/baremetal-instance-types/README.md` around lines 144 - 152, Reconcile the MatchExpression schema and examples with the FindFreeHost inventory-selector contract by choosing one canonical representation. If retaining MatchExpression, document and implement its conversion to map[string]string, including supported operators, multi-value semantics, and translation of hardware-profile to canonical hostType; otherwise change host_label and related examples/API usage to use the map representation directly.
349-354: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftExpose the crash-recovery lookup through the inventory contract.
Recovery step 3 requires finding an existing assignment by
bareMetalInstanceID, but the documentedinventory.Clientonly exposesFindFreeHostand explicitly claims no interface changes are needed. Add a backend-neutral lookup such asFindAssignedHost, or define how the operator invokes backend-native recovery logic; otherwise crash-before-persist recovery is not implementable.🤖 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 `@enhancements/baremetal-instance-types/README.md` around lines 349 - 354, The documented inventory contract must expose crash-recovery lookup by bareMetalInstanceID. Update the inventory.Client interface near FindFreeHost to add a backend-neutral FindAssignedHost operation, or explicitly define an equivalent operator-level mechanism, then ensure both OpenStack and Metal3 implementations support querying their existing assignment markers and returning the assigned host ID.
🤖 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 `@enhancements/baremetal-instance-types/README.md`:
- Line 37: Update the wording in the bare-metal hardware selection description
to hyphenate “bare-metal” when it is used as a compound adjective.
- Line 279: Align the “Provisioning Label Lookup” documentation and the
authorization table’s provisioning entries on a single host-label resolution
path: either remove the Fulfillment Service private Get/RPC dependency and
describe provisioning as using persisted BareMetalInstance host_label
matchExpressions, or explicitly document the lookup’s timing and purpose. Update
the related outage and recovery guarantees consistently.
---
Duplicate comments:
In `@enhancements/baremetal-instance-types/README.md`:
- Around line 148-152: Update the MatchExpression schema validation to enforce
operator/value combinations: require at least one value when operator is In or
NotIn, and require values to be empty when operator is Exists or DoesNotExist.
Use the schema’s cross-field validation mechanism while preserving the existing
key and operator constraints.
- Around line 144-152: Reconcile the MatchExpression schema and examples with
the FindFreeHost inventory-selector contract by choosing one canonical
representation. If retaining MatchExpression, document and implement its
conversion to map[string]string, including supported operators, multi-value
semantics, and translation of hardware-profile to canonical hostType; otherwise
change host_label and related examples/API usage to use the map representation
directly.
- Around line 349-354: The documented inventory contract must expose
crash-recovery lookup by bareMetalInstanceID. Update the inventory.Client
interface near FindFreeHost to add a backend-neutral FindAssignedHost operation,
or explicitly define an equivalent operator-level mechanism, then ensure both
OpenStack and Metal3 implementations support querying their existing assignment
markers and returning the assigned host ID.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49f3b18a-6672-450a-9928-c49d73fcfadd
📒 Files selected for processing (1)
enhancements/baremetal-instance-types/README.md
AI Design Review: EP-119Score: 7/8 | Verdict: PASS
Verdict: A strong, well-detailed design that follows OSAC patterns and provides deep technical specificity, held back from a perfect score by a missing User Stories section and unaddressed cross-cutting dimensions (Installation, Documentation, UI, Tenant Onboarding). Feedback: Add a formal User Stories section under Motivation with 'As a [role], I want...' stories for Cloud Provider Admin, Cloud Infrastructure Admin, and Tenant User — the Workflow Description covers these actors well but the template requires explicit user stories. Address the cross-cutting dimensions from osac-dimensions.md that are currently silent: state whether Installation changes, Documentation, and UI support are in scope or explicitly deferred for this milestone. Fix the proto-vs-example inconsistency where the YAML example shows accelerator 'count: 2' but BareMetalAcceleratorSpec lacks a count field. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
tchughesiv
left a comment
tchughesiv
left a comment
There was a problem hiding this comment.
Thanks for this proposal! Per the naming convention in CONTRIBUTING.md, new enhancement directories must follow OSAC-NNNN-feature-slug/ (with lowercase prd.md/design.md), where OSAC-NNNN is the Jira Feature-level key.
This PR adds enhancements/baremetal-instance-types/, which doesn't match that pattern yet, and will fail the check-ep-naming CI check once this branch is rebased onto latest main (the check merged in #133, after this PR was opened).
Could you rename enhancements/baremetal-instance-types/ → enhancements/OSAC-1201-baremetal-instance-types/ (and update the tracking-link in the frontmatter if needed)? Happy to help if you have questions about the convention.
AI Design Review: EP-119Score: 5/8 | Verdict: PASS
Verdict: A well-structured design that follows OSAC architectural patterns and provides detailed proto schemas with specific implementation logic, but is held back by a proto/example inconsistency, a backward compatibility contradiction around mandatory instance_type, unaddressed UI and Documentation dimensions, and fully deferred graduation criteria. Feedback: Fix the BareMetalAcceleratorSpec proto to include the 'count' field shown in the YAML example, and resolve the tension between instance_type being required (min_len=1 validation) and the stated incremental adoption path from catalog_item — either make instance_type optional with a oneof or document the breaking change explicitly. Address the UI dimension (what UI changes are needed for BareMetalInstanceType listing/selection, or explicitly defer to a later milestone) and the Documentation dimension. Add provisional graduation criteria with measurable conditions even if the target release is TBD. Critical (0)None. Important (6)
Suggestions (4)
Review costModel: claude-opus-4-6 |
- Use string reference for instance_type field in BareMetalInstanceSpec (follows catalog_item pattern) - Remove custom BareMetalLabelSelector in favor of simple map<string,string> host_selector - Clarify controller architecture: fulfillment-service resolves instance_type and sets CRD hostType - Update flow to show proper separation: protobuf API → controller mapping → existing CRD schema - Remove incorrect references to embedded selectors - controller resolves at creation time - Document that existing CRD schema remains unchanged - Focus on hostType field mapping without involving Selector.HostSelector Addresses architectural alignment based on actual fulfillment-service controller patterns. Assisted-by: Claude Code <noreply@anthropic.com>
3243d85 to
6c9f40c
Compare
AI Design Review: EP-119Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows OSAC architectural patterns and provides deep implementation detail; the only significant gap is deferred graduation criteria which prevents a perfect score on testability. Feedback: The graduation criteria section needs concrete, measurable conditions rather than deferring to a future release milestone — specify what 'done' looks like (e.g., 'all CRUD operations pass e2e, label-based provisioning succeeds on both OpenStack and Metal3 backends, no regressions in existing bare metal tests'). Fix the internal inconsistency between the Go code snippets and proto definitions: the code references Critical (0)None. Important (3)
Suggestions (4)
Review costModel: claude-opus-4-6 |
Networking EP alignment — BareMetalInstanceType needs network port name + roleWe're working on the networking EPs (PR #107) which define how tenants attach resources to subnets. Our design currently uses What's needed for networking to workThe current message BareMetalNetworkPortSpec {
string name = 1; // e.g., "data-0", "mgmt-0" — unique identifier within the type
string role = 2; // e.g., "fabric", "management", "storage", "lifecycle"
string type = 3; // e.g., Ethernet, InfiniBand (existing)
string speed = 4; // e.g., 1Gbps, 100Gbps (existing)
}Why Why
The role lets the system make intelligent defaults (e.g., "pick the first fabric port when the tenant doesn't specify") and enforce safety rules (e.g., "reject lifecycle ports in tenant requests") regardless of service type. CaaS usageCaaS cluster Additional conventions
We'll update our networking EPs (PR #107) to reference |
…uture BMaaS design: reverted to HostType for interface validation (v0.2). Added "Future: BareMetalInstanceType Integration" section documenting the migration plan when PR osac-project#119 lands with enhanced network_ports. HostType is the system-level resource that exists today and works for both CaaS and BMaaS. BareMetalInstanceType will be the tenant-facing catalog once it's available. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…uture BMaaS design: reverted to HostType for interface validation (v0.2). Added "Future: BareMetalInstanceType Integration" section documenting the migration plan when PR osac-project#119 lands with enhanced network_ports. HostType is the system-level resource that exists today and works for both CaaS and BMaaS. BareMetalInstanceType will be the tenant-facing catalog once it's available. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Add network port role documentation (fabric, management, storage, lifecycle) - Document ordering conventions for default port resolution - Clarify that lifecycle ports are excluded from tenant validation - Support for BMaaS interface naming (data-0, mgmt-0) and CaaS fabric resolution - Fix host_label_selector → host_selector reference consistency Addresses networking EP coordination feedback for unified hardware type catalog. Assisted-by: Claude Code <noreply@anthropic.com>
|
|
||
| // New logic (resolved from BareMetalInstanceType) | ||
| instanceType, err := resolveBareMetalInstanceType(ctx, bareMetalInstance.Spec.InstanceType) | ||
| object.Spec.HostType = instanceType.Spec.HostSelector["hostType"] |
There was a problem hiding this comment.
This was called host_label_selector in the private API above. And it was a separate struct (BareMetalLabelSelector) with an entire map in it.
Should this reflect that setup or am I misunderstanding something?
Also what are we doing with the entries in this map that are not hostType? Are we also passing those to the BareMetalInstance.Spec.Selector field?
There was a problem hiding this comment.
Also by extension, why have separate fields on BareMetalInstance? Why not just use Selector for everything if the user is adding a map here?
There was a problem hiding this comment.
I'll correct the inconsistency, and yes the entries in the map that are not hostType will be passed to the BareMetalInstance.Spec.Selector field
| | Backend | `"hostType"` translation | | ||
| |---------|--------------------------| | ||
| | OpenStack (`openstack.go`) | Ironic node `ResourceClass` field | | ||
| | Metal3 (`metal3.go`) | `BareMetalHost` label `osac.openshift.io/instance-type` | |
There was a problem hiding this comment.
Should we prepend this same namespace for all the fields in BareMetalInstance.Spec.Selector? I would want to treat everything in the instanceType.Spec.HostSelector map the same way or just make a separate field.
|
@danmanor I'm fine with adding the fields you need, but note that by design this type and the hardware it describes is not guaranteed to be exact. We can document the potential issues that could occur if the type doesn't match the actual hardware, but I'm curious how precisely these things need to match up? The "name" field in particular ... does that need to reference something real on the host? How do you plan to know which discovered interface on the host matches the named ones in the instance type? For now we can model these fields, but as with all the others, we're not going to do anything other than display them just yet. |
- Remove canonical hostType requirement and allow backends to use native label formats - Labels pass through directly from BareMetalInstanceType to inventory backend - Update examples to show Metal3 namespaced labels vs simple backend labels - Resolve Open Question 1 about label format and namespace approach Assisted-by: Claude Code <noreply@anthropic.com>
AI Design Review: EP-119Score: 6/8 | Verdict: PASS
Verdict: A solid design that follows OSAC architectural patterns and provides detailed proto schemas, but is held back by a backward compatibility contradiction between the required instance_type field and the claimed incremental adoption path, plus missing coverage of UI and Documentation dimensions. Feedback: Fix the backward compatibility gap: either make instance_type optional in the proto (allowing catalog_item-only creation to continue working) or update the Upgrade Strategy to acknowledge that instance_type becomes mandatory, breaking the incremental adoption claim. Add a count field to BareMetalAcceleratorSpec or remove count from the YAML example — the proto and examples must be consistent. Address the UI and Documentation cross-cutting dimensions explicitly, even if just to defer them to a later milestone. Critical (0)None. Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
| FS-->>CloudProvider: BareMetalInstanceType created | ||
| ``` | ||
|
|
||
| Cloud Infrastructure Admins apply a label (e.g., `"gpu-large"`) to all inventory hosts that share that hardware profile. Cloud Provider Admins then create a corresponding BareMetalInstanceType in OSAC with the same label and the hardware metadata that describes those hosts. These two steps must be coordinated out-of-band — OSAC does not validate that the label on a BareMetalInstanceType matches any actual hosts at creation time. |
There was a problem hiding this comment.
nit: Could they apply more than one label? I think Nick has an example below with a BareMetalInstanceType with a host_label_selector that specifies two.
There was a problem hiding this comment.
Clarified to say "one or more labels"
… add EP ref - Remove Open Questions §11: reviewers confirmed the BMaaS operator never transitions to FAILED post-provisioning, so with metering starting at provisioning complete the question is moot - Add BareMetalInstanceType EP (PR osac-project#119 / OSAC-2675) cross-reference to dependencies per carbonin's suggestion Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Just one nit. Looks like something just got missed in some of the churn
|
|
||
| ## Summary | ||
|
|
||
| This enhancement introduces BareMetalInstanceType resources that provide a discoverable hardware type catalog for bare metal infrastructure provisioning. Cloud Provider Admins define BareMetalInstanceTypes via the OSAC API, specifying hardware metadata and a host selector using the canonical `hostType` key. Cloud Infrastructure Admins label inventory hosts to classify them by hardware profile. During provisioning, the fulfillment-service controller resolves the BareMetalInstanceType reference and sets the `hostType` in the CRD, which the bare-metal-fulfillment-operator uses for host selection. |
There was a problem hiding this comment.
specifying hardware metadata and a host selector using the canonical
hostTypekey
Is this still the case?
There was a problem hiding this comment.
I updated it so it uses the labels instead of a dedicated hostType
AI Design Review: EP-119Score: 7/8 | Verdict: PASS
Verdict: A well-structured design that follows OSAC patterns and provides substantial implementation detail, held back from a perfect score by proto schema inconsistencies (missing accelerator count field, backward-incompatible instance_type validation) and an under-specified catalog item integration. Feedback: Fix the proto/example discrepancy: BareMetalAcceleratorSpec needs a 'count' field to match the YAML example, or the example should show repeated entries instead. Make 'instance_type' an 'optional string' (or use a oneof with catalog_item) so existing clients that only set catalog_item aren't broken by the min_len=1 validation — this directly contradicts the backward compatibility guarantee in the Upgrade section. Describe what 'Enhanced to work with BareMetalInstanceType selection' means for BareMetalInstanceCatalogItems: what fields change, how does a catalog item reference an instance type, and what happens to existing catalog items that use opaque bareMetalInstanceType strings? Critical (0)None. Important (4)
Suggestions (4)
Review costModel: claude-opus-4-6 |
The fulfillment-service sets host selector labels (Spec.Selector.HostSelector) not the hostType field. Updated all sections that incorrectly referenced setting hostType in the CRD. Removed canonical hostType key validation requirement since labels pass through without transformation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Austin Jamias <ajamias@redhat.com>
bda9fd9 to
0d5afa0
Compare
AI Design Review: EP-119Score: 7/8 | Verdict: PASS
Verdict: A well-structured design document that follows OSAC architectural patterns, provides comprehensive proto schemas with validation, and includes a concrete multi-level test plan, but falls short on cross-cutting dimension coverage — Documentation, UI, and Installation dimensions are neither addressed nor explicitly deferred. Feedback: Address the silent cross-cutting dimension gaps: explicitly state whether UI work, user-facing documentation, and osac-installer changes are in scope or deferred (with a tracking reference). Fix the proto/YAML inconsistency in BareMetalAcceleratorSpec — the YAML example includes 'count: 2' but the proto schema has no count field; clarify whether multiple accelerators of the same type are represented as repeated entries or via a count field. Elaborate on the BareMetalInstanceCatalogItems modification mentioned in API Extensions — it is listed as a modified service but the implementation details section does not describe the changes. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajamias, carbonin, tzumainn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…uture BMaaS design: reverted to HostType for interface validation (v0.2). Added "Future: BareMetalInstanceType Integration" section documenting the migration plan when PR osac-project#119 lands with enhanced network_ports. HostType is the system-level resource that exists today and works for both CaaS and BMaaS. BareMetalInstanceType will be the tenant-facing catalog once it's available. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
… add EP ref - Remove Open Questions §11: reviewers confirmed the BMaaS operator never transitions to FAILED post-provisioning, so with metering starting at provisioning complete the question is moot - Add BareMetalInstanceType EP (PR osac-project#119 / OSAC-2675) cross-reference to dependencies per carbonin's suggestion Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
7 participants
Introduces the BareMetalInstanceType design document covering:
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit