Repository navigation
[OSAC-2135] PRD: CaaS Bare Metal Worker Node Provisioning - #181
forgeSmith-bot wants to merge 11 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PRD defines on-demand CaaS bare-metal worker provisioning. It covers provisioning, boot configuration, host correlation, tenant isolation, cleanup, resource selection, user stories, assumptions, dependencies, exclusions, and provenance metadata. ChangesBare-metal provisioning requirements
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
|
PRD has been revised based on feedback. Please review the updated version. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/OSAC-2135/prd.md`:
- Line 84: Update prd.md so the file ends with exactly one trailing newline,
satisfying the MD047 markdownlint requirement.
- Line 1: Update the compound adjective usage in the PRD, including the title
and the referenced lines, to consistently hyphenate “bare-metal” whenever it
modifies a noun, while preserving the surrounding wording.
- Line 24: Expand the MAC Address Correlation requirement in the PRD to define
the complete matching contract: whether BMaaS exposes one canonical MAC or
multiple values, how CaaS normalizes and compares them with Assisted Installer
agent reports, and the deterministic behavior for missing, duplicate, or changed
MACs. Apply the same clarification to the corresponding repeated requirement.
- Line 27: Clarify the “Race Prevention” requirement by selecting one
authoritative InfraEnv scope and identity key. State explicitly whether node
pools within the same cluster share an InfraEnv, and define when the InfraEnv is
created and deleted so exactly one instance is maintained for the chosen scope.
- Around line 62-64: Update the BMaaS completion-boundary requirements around
“Boundary of Failure” to define an observable completion signal after image
write, ignition delivery, boot, and network readiness are verified. Explicitly
classify failures in those prerequisites as BMaaS-owned, and only classify a
missing agent registration as a CaaS software failure after the defined signal
is emitted.
- Around line 26-32: Clarify the lifecycle scope in the PRD by distinguishing
explicit cluster or node-pool deprovisioning from automated day-2 workload
scaling. Define BareMetalInstance cleanup as applying only to explicit
decommission or scale-down operations, and state that automated workload-driven
scaling remains out of scope unless it is intended to be included.
🪄 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: Pro Plus
Run ID: 17565d09-aa4f-45c7-831f-f0f9fd6bb27f
📒 Files selected for processing (1)
enhancements/OSAC-2135/prd.md
|
PRD has been revised based on feedback. Please review the updated version. |
|
PRD has been revised based on feedback. Please review the updated version. |
| - **Exposed Values:** BMaaS must expose all physical MAC addresses of the host's network interfaces as a structured list within the `BareMetalInstance` status, clearly identifying the primary/boot interface MAC address as the canonical reference. | ||
| - **Normalization and Comparison:** CaaS retrieves this list of MAC addresses, normalizes all values to lowercase, colon-separated format (e.g., `aa:bb:cc:dd:ee:ff`), and compares them against the list of interface MAC addresses reported by the Assisted Installer agent's system discovery reports. | ||
| - **Deterministic Edge-Case Behavior:** | ||
| - *Missing MACs:* If the `BareMetalInstance` status lacks MAC address information, CaaS will pause provisioning, mark the node's reconcile status as `AwaitingHardwareDiscovery`, and retry with an exponential backoff. | ||
| - *Duplicate MACs:* If a MAC address matches multiple `BareMetalInstance` resources or multiple registered agents, CaaS will trigger a high-severity alert, isolate the conflicting hosts/agents in a quarantined state, and fail the affected `ClusterOrder` with a clear validation error. | ||
| - *Changed MACs:* The MAC-to-agent mapping contract is immutable once successfully established. Any subsequent changes in the reported MAC addresses of an active instance will trigger a node reconciliation failure, prompting a replacement of the node. |
There was a problem hiding this comment.
Design details don't belong in the PRD
There was a problem hiding this comment.
Also, I think that if we're having an InfraEnv per cluster then there's no need for matching MAC addresses, but again this is for the design doc
| - *Duplicate MACs:* If a MAC address matches multiple `BareMetalInstance` resources or multiple registered agents, CaaS will trigger a high-severity alert, isolate the conflicting hosts/agents in a quarantined state, and fail the affected `ClusterOrder` with a clear validation error. | ||
| - *Changed MACs:* The MAC-to-agent mapping contract is immutable once successfully established. Any subsequent changes in the reported MAC addresses of an active instance will trigger a node reconciliation failure, prompting a replacement of the node. | ||
| - **Tenant Isolation & Security:** Complete exclusion of CaaS-managed `BareMetalInstances` and underlying `ComputeImages` from tenant-facing APIs and UI consoles. | ||
| - **Automatic Lifecycle Cleanup:** Deletion of `BareMetalInstance` resources upon explicit, administrator-initiated cluster decommissioning or manual node pool scale-down operations. This triggers a mandatory, automated, blocking host cleanup (including deep disk wipe, network interface reset, and credentials removal) performed by BMaaS before the host can be returned to the general active inventory pool. This lifecycle cleanup applies exclusively to these manual, administrator-driven operations and does not cover automated workload-driven scaling. |
There was a problem hiding this comment.
The details of cleanup are handled by BMaaS and should not be detailed here
| - *Changed MACs:* The MAC-to-agent mapping contract is immutable once successfully established. Any subsequent changes in the reported MAC addresses of an active instance will trigger a node reconciliation failure, prompting a replacement of the node. | ||
| - **Tenant Isolation & Security:** Complete exclusion of CaaS-managed `BareMetalInstances` and underlying `ComputeImages` from tenant-facing APIs and UI consoles. | ||
| - **Automatic Lifecycle Cleanup:** Deletion of `BareMetalInstance` resources upon explicit, administrator-initiated cluster decommissioning or manual node pool scale-down operations. This triggers a mandatory, automated, blocking host cleanup (including deep disk wipe, network interface reset, and credentials removal) performed by BMaaS before the host can be returned to the general active inventory pool. This lifecycle cleanup applies exclusively to these manual, administrator-driven operations and does not cover automated workload-driven scaling. | ||
| - **Race Prevention:** Allocation of exactly one isolated `InfraEnv` per cluster, uniquely scoped and keyed by the Cluster UUID as its identity key, to prevent cross-tenant agent registration races. Node pools within the same cluster share this single `InfraEnv`. The `InfraEnv` is created immediately during initial cluster bootstrap (prior to any worker node provisioning) and is deleted only during the final cluster decommissioning phase, after all associated nodes have been successfully deprovisioned and cleaned up. This guarantees that exactly one `InfraEnv` instance exists per cluster lifecycle scope. |
There was a problem hiding this comment.
InfraEnv per cluster is design details
|
|
||
| ### Cloud Infrastructure Admin | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want every deprovisioned bare-metal host to undergo a guaranteed, blocking cleanup (including deep disk wipe and network reset) by the BMaaS layer before being returned to the general inventory pool, so that I can prevent security leaks and configuration drift between different tenants. |
There was a problem hiding this comment.
Tenant boundaries are the cloud provider admin's responsibility
|
|
||
| - As a Cloud Infrastructure Admin, I want every deprovisioned bare-metal host to undergo a guaranteed, blocking cleanup (including deep disk wipe and network reset) by the BMaaS layer before being returned to the general inventory pool, so that I can prevent security leaks and configuration drift between different tenants. | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want to ensure that no Assisted Installer, agent, or cluster-specific terminology is exposed within the BMaaS private APIs, so that the BMaaS service remains a clean, generic bare-metal-as-a-service provider. |
There was a problem hiding this comment.
This isn't exposed to users and therefore isn't a user story
|
|
||
| ## Dependencies | ||
|
|
||
| - **MAC Address Status Exposure:** BMaaS must expose all physical MAC addresses of the host's interfaces as a structured list in the `BareMetalInstance` status subresource, clearly identifying the primary/boot interface MAC address as the canonical reference, to support the CaaS MAC normalization and matching contract `[Jira: OSAC-2308]`. |
| - **BareMetalInstanceType Definition:** The `BareMetalInstanceType` specifications and schema definitions must be finalized and available `[PR #59]`. | ||
| - **User Data Pass-through:** BMaaS private API must support the ingestion and pass-through of ignition configurations in the `BareMetalInstance` spec. | ||
|
|
||
| ## Risks & Mitigations |
There was a problem hiding this comment.
Mitigations contain design details
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Good problem framing and persona coverage. Three things to fix:
- In Scope has design leakage
Issue: MAC normalization format, exponential backoff, AwaitingHardwareDiscovery/BareMetalInstanceReady conditions, and InfraEnv lifecycle are implementation details.
Action: Reframe as user-observable outcomes: "CaaS correlates provisioned hosts to cluster agents", "hosts are cleaned up on decommission." Move the mechanics to the design doc.
- Remove the Risks section
Issue: prd_template.md has 6 sections (Problem Statement, In Scope, Out of Scope, User Stories, Assumptions, Dependencies).
Action: Remove this section. Risks belongs in the design doc.
- Assumptions contains API contracts
Issue: The BareMetalInstanceReady condition spec and failure-ownership breakdown are design-level interface contracts, not product assumptions.
Action: Remove this section.
|
PRD has been revised based on feedback. Please review the updated version. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: forgeSmith-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
PRD has been revised based on feedback. Please review the updated version. |
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 `@enhancements/OSAC-2135/prd.md`:
- Line 28: The PRD’s ClusterOrder resourceClass definition lacks a deterministic
BMaaS mapping and rejection behavior. Update the Resource Definition section to
specify that each nodeRequests[].resourceClass maps to the corresponding
BareMetalInstanceSpec.instance_type.name, whose host_label_selector/match_labels
are passed to BMaaS; explicitly require unsupported or unavailable resourceClass
values to be rejected rather than mapped to another hardware class.
🪄 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: Pro Plus
Run ID: 283e32cd-fff6-4f4a-86ca-3cd74f60387a
📒 Files selected for processing (1)
enhancements/OSAC-2135/prd.md
|
PRD has been revised based on feedback. Please review the updated version. |
|
|
||
| ### Cloud Infrastructure Admin | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want every deprovisioned bare-metal host to undergo a guaranteed, blocking cleanup (including deep disk wipe and network reset) by the BMaaS layer before being returned to the general inventory pool, so that I can prevent security leaks and configuration drift between different tenants. |
There was a problem hiding this comment.
Tenant boundaries are the cloud provider admin's responsibility
|
|
||
| - As a Cloud Infrastructure Admin, I want every deprovisioned bare-metal host to undergo a guaranteed, blocking cleanup (including deep disk wipe and network reset) by the BMaaS layer before being returned to the general inventory pool, so that I can prevent security leaks and configuration drift between different tenants. | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want to ensure that no Assisted Installer, agent, or cluster-specific terminology is exposed within the BMaaS private APIs, so that the BMaaS service remains a clean, generic bare-metal-as-a-service provider. |
There was a problem hiding this comment.
This isn't exposed to users and therefore isn't a user story
|
@eshulman2: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
PRD has been revised based on feedback. Please review the updated version. |
|
@eshulman2: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
PRD has been revised based on feedback. Please review the updated version. |
|
@eshulman2: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
tchughesiv
left a comment
tchughesiv
left a comment
There was a problem hiding this comment.
@eranco74 any idea why we're not seeing provenance details at the end of the file?
|
PRD has been revised based on feedback. Please review the updated version. |
| |-------------|-------| | ||
| | Author(s) | CaaS and BMaaS Product Teams | | ||
| | Jira | OSAC-2135 | | ||
| | Date | 2026-08-02 | |
There was a problem hiding this comment.
add a field called test with the value true
|
@eshulman2: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
PRD has been revised based on feedback. Please review the updated version. |
mhrivnak
left a comment
mhrivnak
left a comment
There was a problem hiding this comment.
I'm not sure this PRD should exist. The jira issue describes work that looks like a change of internal implementation details. That explains why this PRD in its current form is fairly confusing. I read it twice trying to find a narrative or cohesive theme, then went to jira and found that it's really a refactor. I think we should probably skip the PRD and not try to force this change to be describes in terms of user stories, and just go straight to design.
|
Forge reviewed the feedback but the regenerated PRD was unchanged. The feedback may require manual revision, or it may have already been addressed in a previous revision. |
|
Closing in favor of #185 |
7 participants
PRD for OSAC-2135
The PRD document is in
enhancements/OSAC-2135/prd.mdon this branch.Review the file changes for the latest version. Leave comments on this PR to provide feedback — Forge will regenerate the PRD and push updated commits.
Summary by CodeRabbit