Repository navigation
OSAC-2135: Design — CaaS Bare-Metal Worker Node Provisioning - #198
Conversation
|
@rccrdpccl: This pull request references OSAC-2135 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. |
|
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:
WalkthroughThe design specifies on-demand CaaS bare-metal worker provisioning through the fulfillment-service private API. It defines resource creation, image selection, Agent correlation, NodePool binding, scaling, cleanup, tenancy, status, recovery, and validation. ChangesCaaS bare-metal worker provisioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterOrderController
participant InfraEnv
participant FulfillmentServicePrivateAPI
participant Agents
participant HyperShiftNodePools
ClusterOrderController->>InfraEnv: create shared discovery configuration
ClusterOrderController->>FulfillmentServicePrivateAPI: create BareMetalInstance with disk_image
InfraEnv->>Agents: discover worker hardware
Agents->>ClusterOrderController: provide MAC address
ClusterOrderController->>HyperShiftNodePools: bind matching worker Agent
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design document that follows all OSAC architectural patterns, provides deep implementation detail with proto schemas and controller phase decomposition, covers all lifecycle operations with specific failure handling, and includes a concrete multi-level test plan — one of the strongest OSAC designs reviewed. Feedback: Fix the RBAC section contradiction: the implementation details require patch permission on agents in the agent-install.openshift.io API group, but the RBAC section claims no new permissions are needed — enumerate the specific ClusterRole changes required. Reframe implementation-focused goals (e.g., 'Reuse the existing ClusterOrder controller reconciliation pattern') as user-visible outcomes. Consider adding more detail on how in-memory worker phase state is rebuilt from live CR and Agent state on controller restart — the current description says it happens but not how ambiguous states (e.g., a BMI in Running but no matching Agent yet) are resolved. Critical (0)None. Important (2)
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: 13
🧹 Nitpick comments (1)
enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md (1)
444-475: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd failure-window and contract tests.
The test plan does not cover a lost Create response, a BMI stuck in
Deleting, ambiguous or cross-cluster MAC matches, zero or multiple DiskImages, reserved-label writes, URL limits, or old-service capability gating. Add these cases 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 `@enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md` around lines 444 - 475, Add failure-window and contract coverage to the Test Plan for lost Create responses, BMIs stuck in Deleting, ambiguous or cross-cluster MAC matches, zero and multiple DiskImages, reserved-label writes, URL-length limits, and old-service capability gating. Place the cases under the appropriate Unit, Integration, or E2E sections and require them before rollout.
🤖 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-caas-bare-metal-worker-provisioning/design.md`:
- Line 428: Update the MAC-based Agent correlation algorithm to require a unique
candidate matching the current worker’s Agent namespace, InfraEnv or
ClusterDeployment, and osac.openshift.io/cluster-order ownership. Reject zero or
multiple matches and perform no Delete call unless exactly one candidate
satisfies all scope constraints; add tests covering these failure cases.
- Around line 126-134: The scale-up capacity calculation must not count workers
in `Failed` phase toward `current`; update the controller’s
desired-versus-current worker logic so failed slots are replaced and the desired
`Ready` capacity is restored, while preserving the existing treatment of
`Deleting` workers.
- Around line 319-327: Update the public BareMetalInstances Create and Update
validation to reject any request containing the reserved
osac.openshift.io/managed-by label key, regardless of its value. Keep visibility
filtering in BareMetalInstances.List consistent with this reserved-key contract
so tenant-owned resources cannot be hidden by assigning another value.
- Around line 352-362: The security design must not leave discovery ignition
pull secrets exposed in immutable, plaintext user_data. Update the user_data
handling to use an approved secret reference or envelope-encryption mechanism,
restrict and redact reads and logs, and expire or remove the credential material
after provisioning where supported; revise the surrounding Security
Considerations to describe the resulting protections.
- Around line 114-117: Update the discovery ignition fetch in the InfraEnv
polling flow after status.bootArtifacts.discoveryIgnitionURL is populated:
validate that the URL uses HTTPS and an allowed external destination, configure
connection, read, and total timeouts, disable unsafe redirects, and stream-read
with a strict 64 KiB maximum. Reject invalid schemes, destinations, redirects,
timeouts, or oversized responses, and do not log the fetched ignition content.
- Around line 163-171: Update the worker deletion flow around
BareMetalInstances.Delete and status.workers so the worker record remains in a
Deleting state after the delete request, allowing reconciliation retries while
deletion is pending or fails. Remove the record only after the private API
confirms terminal deletion and BMaaS cleanup; apply the same retention and retry
behavior to the break-glass procedure.
- Around line 329-333: Before implementing the MAC correlation workflow, define
the canonical BMI status MAC field and its type, normalization rules, readiness
semantics, and API version gate, replacing the TBD status.host.mac_address
reference in the design. Gate controller matching on that contract and add
producer-consumer coverage alongside the controller matching test.
- Around line 199-211: Replace the proposed ClusterOrderStatus.Workers
ObjectReference-only design with a durable, authoritative worker record
containing the fulfillment-service BMI ID and reconciliation/lifecycle state, or
explicitly define the authoritative BMI CR, ID mapping, ownership, and watch
contract. Align the workers[] description and schema so they do not conflict,
and ensure reconciliation can rebuild identical worker mappings and state after
restart from persisted status and authoritative resources rather than in-memory
state.
- Around line 499-503: The Version Skew Strategy must require
fulfillment-service capability validation before the controller creates BMIs
using source_type "disk_image". Replace the cosmetic upgrade-window guidance
with a fail-closed requirement: upgrade fulfillment-service first or
simultaneously, and prevent BMI creation and tenant operations when the required
capability or visibility filtering is unavailable.
- Around line 277-289: Update the RHCOS DiskImage resolution design to require
exactly one matching provider-global image: filter for AVAILABLE lifecycle,
empty tenant metadata, Linux guest OS family, amd64 architecture, and the target
OCP version label; fail when zero or multiple images match. Define and test the
NodePool.spec.release.image parser against every supported release-image format,
including z-stream variants.
- Around line 291-317: Update the BMI creation flow around
BareMetalInstances.Create to be idempotent when responses or status updates are
lost: use a deterministic idempotency key or enforce the generated worker name
as unique, and reconcile AlreadyExists by retrieving and reusing the existing
BMI. Define the gRPC deadline and bounded retry behavior, then add a test
covering a successful Create followed by a lost response/status update and
reconciliation of the existing BMI.
- Around line 381-385: Update the RBAC / Tenancy design section to define the
exact Role or ClusterRole and RoleBinding for the target namespace, including
patch access to agents in the agent-install.openshift.io API group and the
permissions needed to watch Agent resources. Add an RBAC test covering these
permissions and bindings.
- Around line 335-337: Update the “Minimum MCE Version” section to replace the
inconsistent post-v2.55.0 and assisted-service 5.0.0+ prerequisites with one
exact released MCE bundle version known to contain both MGMT-24903 fixes. Add an
installation compatibility check that validates the deployed MCE version meets
this minimum before proceeding.
---
Nitpick comments:
In `@enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md`:
- Around line 444-475: Add failure-window and contract coverage to the Test Plan
for lost Create responses, BMIs stuck in Deleting, ambiguous or cross-cluster
MAC matches, zero and multiple DiskImages, reserved-label writes, URL-length
limits, and old-service capability gating. Place the cases under the appropriate
Unit, Integration, or E2E sections and require them before rollout.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: ae07ffdd-d4a2-4357-bd1b-198661d88b86
📒 Files selected for processing (1)
enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md
| #### Minimum MCE Version | ||
|
|
||
| The MGMT-24903 fix (persistent-boot day-2 installs) is merged to assisted-service master ([PR #10717](https://github.com/openshift/assisted-service/pull/10717), 2026-07-29) and assisted-installer-agent master ([PR #1568](https://github.com/openshift/assisted-installer-agent/pull/1568), 2026-07-30). The fix is not yet in a tagged release (post-v2.55.0). The design requires a MCE version shipping these commits. Without them, workers fail to install because `osImageURL` is stripped from the ignition config. The controller does not implement a workaround — the deployment prerequisite documentation must specify the minimum MCE version. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '320,345p;510,535p' enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md
printf '\n--- related MCE/version references ---\n'
rg -n -i 'MCE|assisted-service|assisted-installer-agent|5\.0\.0|v2\.55\.0|minimum' enhancements/OSAC-2135-caas-bare-metal-worker-provisioningRepository: osac-project/enhancement-proposals
Length of output: 10000
🌐 Web query:
OpenShift Multi-Cluster Engine release containing assisted-service PR 10717 and assisted-installer-agent PR 1568, and the corresponding MCE bundle/version
💡 Result:
Based on the provided search results, there is no evidence of a specific OpenShift Multi-Cluster Engine (MCE) release that explicitly bundles or corresponds to an assisted-service pull request (PR) 10717 or an assisted-installer-agent PR 1568. The current PR numbering in the openshift/assisted-service and openshift/assisted-installer-agent repositories does not indicate that these specific PR numbers have been merged or associated with a released MCE version in the context of the provided data [1][2][3][4]. The assisted-installer-agent and assisted-service repositories utilize continuous integration and development workflows where PRs are merged independently [5][6][7][8][9][10][11][12][13][14]. MCE bundles are managed through standard Red Hat release processes, and while the assisted-installer-agent is a component of the MCE operator (often referred to as Central Infrastructure Management or CIM) [15][16], specific PR numbers are rarely used to identify MCE bundle versions in public documentation [15][17]. The provided information does not link the requested PR numbers to any MCE bundle or version.
Citations:
- 1: openshift/assisted-service@0657bd1...9346359
- 2: openshift/assisted-service@edf2311...6b16108
- 3: openshift/assisted-service@7efb52f...974f740
- 4: MGMT-21810: Support TNF cluster installation using kube-api openshift/assisted-service#8085
- 5: OCPBUGS-31423: Try to pull installer image before installing openshift/assisted-installer-agent#679
- 6: NO-ISSUE: Update assisted-service's version openshift/assisted-installer-agent#997
- 7: MGMT-12186: Add MCE operator plugin openshift/assisted-service#5203
- 8: MGMT-16452: Change MCE subscription to use the default channel openshift/assisted-service#5843
- 9: MGMT-14975: Update MCE operator installation to use stable-2.3 channel openshift/assisted-service#5297
- 10: MGMT-17353: Debug pod left in ImagePullBackOff after install in disconnected environment openshift/assisted-service#6333
- 11: NO-ISSUE: [master] Bump OCP versions: 4.16, 4.17 openshift/assisted-service#6676
- 12: MGMT-18505: Fix installation from a 4.17 hub with converged flow openshift/assisted-service#6639
- 13: OCPBUGS-14405: Avoid panic if pull secret contains non-string openshift/assisted-service#5267
- 14: MGMT-16517: Add Env Var for On-Prem Data Deployment Type openshift/assisted-service#5874
- 15: https://www.redhat.com/en/blog/how-to-add-a-worker-to-an-imported-ocp-cluster-using-mce
- 16: http://schmaustech.blogspot.com/search/label/multicluster-engine
- 17: https://github.com/borball/ocp-cluster-scaling
🏁 Script executed:
set -eu
for spec in \
'openshift/assisted-service/pulls/10717' \
'openshift/assisted-installer-agent/pulls/1568'
do
echo "--- $spec ---"
curl -fsSL "https://api.github.com/repos/$spec" |
jq '{number,state,title,merged_at,merge_commit_sha,head:.head.sha,base:.base.ref,html_url}'
done
for repo in openshift/assisted-service openshift/assisted-installer-agent; do
echo "--- tags: $repo ---"
curl -fsSL -H 'Accept: application/vnd.github+json' \
"https://api.github.com/repos/$repo/tags?per_page=100" |
jq -r '.[] | [.name,.commit.sha] | `@tsv`' | head -40
done
echo "--- MCE repositories matching repository metadata ---"
curl -fsSL 'https://api.github.com/search/repositories?q=multicluster-engine+org:redhat-open-cluster-management&per_page=20' |
jq -r '.items[] | [.full_name,.html_url,.archived] | `@tsv`'Repository: osac-project/enhancement-proposals
Length of output: 662
🌐 Web query:
site:github.com/openshift/assisted-service/pull/10717 MGMT-24903
💡 Result:
I couldn’t retrieve the contents of PR #10717 or confirm its association with MGMT-24903 from indexed results. The pull request is here:
https://github.com/openshift/assisted-service/pull/10717
GitHub’s PR API documents the endpoint format for retrieving PR details. [1] (docs.github.com)
Citations:
🌐 Web query:
site:github.com/openshift/assisted-installer-agent/pull/1568 MGMT-24903
💡 Result:
PR #1568, “MGMT-24903: Preserve encapsulated MachineConfig in filtered ignition,” was merged on July 30, 2026. It preserves /etc/ignition-machine-config-encapsulated.json so the service can obtain spec.osImageURL for persistent-boot day-2 installations when the release image is unavailable. It added unit and regression tests; all 17 checks passed. [1] (github.com)
Citations:
🌐 Web query:
OpenShift MCE release notes assisted-service 5.0.0 MGMT-24903 osImageURL
💡 Result:
The issue identifier MGMT-24903 in the OpenShift assisted-service (related to OpenShift MCE/Agent-based installation) is associated with two specific technical improvements implemented in recent releases: 1. Preserve encapsulated MachineConfig in filtered ignition: This fix ensures that MachineConfig data is preserved during ignition filtering [1][2][3][4][5]. 2. Fall back to CoreOS image from worker ignition for day-2 persistent-boot: This improvement addresses persistent boot issues during day-2 operations by enabling a fallback mechanism to the CoreOS image provided in the worker ignition [3][6][7]. The query for osImageURL appears in related OpenShift technical contexts (such as OCPNODE-4518, which involves blocking runc on RHEL 10 via OSImageURL stream class inspection), but it is not the primary subject of MGMT-24903 [8][7][9][10]. The MGMT-24903 identifier specifically tracks the ignition and boot-related fixes mentioned above in the assisted-installer and agent-installer components [1][3][7].
Citations:
- 1: https://origin-release.apps.ci.l2s4.p1.openshiftapps.com/changelog?from=5.0.0-0.okd-scos-nightly-2026-07-24-032429&to=5.0.0-0.okd-scos-nightly-2026-07-31-152724
- 2: https://origin-release.apps.ci.l2s4.p1.openshiftapps.com/releasestream/5.0.0-0.okd-scos-nightly/release/5.0.0-0.okd-scos-nightly-2026-07-31-152724
- 3: https://origin-release.apps.ci.l2s4.p1.openshiftapps.com/releasestream/5.0.0-0.okd-scos-nightly/release/5.0.0-0.okd-scos-nightly-2026-08-03-032724
- 4: https://amd64.ocp.releases.ci.openshift.org/releasetag/5.0.0-0.ci-2026-07-30-173831
- 5: https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/releasetag/4.23.0-0.ci-2026-07-31-000539
- 6: https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/releasetag/4.23.0-0.ci-2026-08-02-061239?from=4.22.0-0.ci-2026-08-02-005640
- 7: https://origin-release.apps.ci.l2s4.p1.openshiftapps.com/releasetag/5.0.0-0.okd-scos-nightly-2026-08-03-152724
- 8: https://origin-release.ci.openshift.org/releasetag/5.0.0-0.okd-scos-nightly-2026-07-29-152429
- 9: https://amd64.origin.releases.ci.openshift.org/releasestream/5.0.0-0.okd-scos-nightly/release/5.0.0-0.okd-scos-nightly-2026-07-28-152429
- 10: https://amd64.ocp.releases.ci.openshift.org/releasetag/5.0.0-0.ci-2026-07-26-230915
🏁 Script executed:
set -u
for repo in \
https://github.com/openshift/assisted-service.git \
https://github.com/openshift/assisted-installer-agent.git
do
echo "--- $repo PR refs ---"
git ls-remote "$repo" \
'refs/pull/10717/*' 'refs/pull/1568/*' 2>&1 || true
echo "--- $repo version tags ---"
git ls-remote --tags "$repo" 'refs/tags/v2.55*' 'refs/tags/v5.0*' 2>&1 |
tail -30
doneRepository: osac-project/enhancement-proposals
Length of output: 888
Pin the minimum released MCE bundle.
The document uses inconsistent prerequisites (post-v2.55.0 and assisted-service 5.0.0+) and does not identify an MCE bundle containing both MGMT-24903 fixes. Specify the exact supported MCE version and add an installation compatibility check.
🤖 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/OSAC-2135-caas-bare-metal-worker-provisioning/design.md` around
lines 335 - 337, Update the “Minimum MCE Version” section to replace the
inconsistent post-v2.55.0 and assisted-service 5.0.0+ prerequisites with one
exact released MCE bundle version known to contain both MGMT-24903 fixes. Add an
installation compatibility check that validates the deployed MCE version meets
this minimum before proceeding.
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-caas-bare-metal-worker-provisioning/design.md`:
- Line 130: Align the failed-worker behavior described in the controller
capacity calculation with the terminal failure policy: either exclude automatic
replacement for workers in Failed phase, or define a persisted retry-attempt
limit with bounded backoff and repeated-failure tests. Update the lifecycle and
failure table consistently, including the cleanup and status.workers behavior.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 9581afa6-51c7-423f-b207-372ee82f5fd0
📒 Files selected for processing (1)
enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A strong, implementation-ready design that follows all OSAC patterns, provides deep technical detail validated by a PoC, covers all lifecycle operations with comprehensive failure handling, and includes a concrete test plan at all levels — scoring 8/8. Feedback: The design is well-structured and thorough. Two minor improvements: (1) Reword goals 1 and 3 to be user-visible outcomes rather than implementation choices — e.g., 'Workers provision on-demand when a cluster requests bare-metal nodes' instead of 'Reuse the existing ClusterOrder controller reconciliation pattern.' (2) Consider adding a brief Terminology section defining CaaS, BMI, InfraEnv, and Agent upfront for reviewers less familiar with the assisted-service stack — the networking EP's terminology section is the reference pattern. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: Exceptionally well-crafted design document that follows all OSAC patterns, provides deep implementation specificity across all lifecycle operations, thoroughly addresses failure modes and risks, and includes a comprehensive multi-level test plan. Feedback: This design is ready for merge. Minor polish opportunities: (1) the Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design that follows all OSAC conventions, covers all lifecycle operations in detail, specifies concrete proto changes and failure handling, and includes a well-structured test plan — the strongest area is the depth of workflow description and failure mode analysis. Feedback: The retry attempt count reconstruction from Kubernetes events (counting WorkerFailed events per worker index on restart) is fragile: events have a default TTL and can be garbage-collected, which would reset the retry counter and potentially allow unlimited retries for a persistently failing worker slot. Consider persisting the attempt count in the ClusterOrder status (e.g., per-worker annotation or a count field in the workers list) or documenting the accepted risk with a fallback behavior. Additionally, define the values for the Critical (0)None. Important (1)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
d562722 to
4322c8a
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design document that follows all OSAC architectural patterns, provides deep implementation detail with specific error codes and backoff values, clearly scopes the work with specific non-goals and three rejected alternatives, and includes a concrete multi-level test plan. Feedback: The design is ready for merge. Two minor improvements: (1) Reframe Goal 1 as a user-visible outcome rather than an implementation choice — e.g., 'Provision and manage bare-metal workers through the existing fulfillment-service BMI lifecycle' instead of 'Reuse the existing ClusterOrder controller reconciliation pattern.' (2) Consider adding a brief note to the test plan about success criteria for the E2E tests (e.g., 'all lifecycle operations complete within X minutes, no orphaned BMIs after deletion') to give reviewers confidence in what 'passing' looks like at the E2E level. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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-caas-bare-metal-worker-provisioning/design.md`:
- Line 125: Update the RBAC section associated with the Agent correlation and
watch flow to define the exact Role or ClusterRole and corresponding RoleBinding
for agent-install.openshift.io Agents. Grant get, list, watch, and patch on
agents, and ensure the design specifies testing the complete binding rather than
only patch access.
- Line 431: Update the BMI creation phase and its reconciliation logic to remain
idempotent when BareMetalInstances.Create succeeds but its response or status
update is lost: define and consistently use an idempotency key or unique worker
name, handle AlreadyExists by retrieving and adopting the existing BMI, and add
a test covering this lost-response failure path.
- Around line 418-420: Update the “Minimum MCE Version” section to name one
released MCE bundle that contains both assisted-service PR `#10717` and
assisted-installer-agent PR `#1568`, rather than separate component versions.
Define an installation-time compatibility check in the provisioning flow and
block worker provisioning when the deployed MCE bundle is below that exact
supported release.
- Around line 168-175: Separate teardown from replacement by introducing a
teardown-specific worker phase instead of using Failed when Agent unbinding
times out, and ensure automatic BMI replacement only handles genuine
provisioning failures. Update the scale-down and deletion flows around the
Failed handling, unbinding-timeout assignment, and status.workers removal so
teardown requires a drained node and unbound Agent before calling
BareMetalInstances.Delete, then retains the status entry until the BMI reaches
terminal deletion; apply the same guard to manual removal.
- Around line 258-270: Update the YAML example’s currentWorkers value from 5 to
3 so it counts only the three Ready workers and excludes the two Failed workers,
while leaving desiredWorkers and readyWorkers unchanged.
- Around line 411-416: Update the Agent-to-BMI correlation algorithm to require
an authoritative match for the expected ClusterDeployment and InfraEnv in
addition to namespace, ClusterOrder ownership, and MAC. Reject zero or multiple
candidates before any binding or BMI deletion side effect, including the flow at
the referenced deletion logic, and add coverage for stale, cross-installation,
and ambiguous matches.
- Around line 120-121: The design must define how BareMetalInstances.Create
handles spec.user_data: either specify a supported Secret-reference format and
the API/controller resolution path before BMI creation, or pass the ignition
content inline while enforcing the 64 KB limit. Update the Secret and BMI
provisioning flow to match the chosen contract, and add an integration test
covering host boot with the resulting user data.
- Line 119: The design must define how worker architecture is handled
consistently with DiskImage resolution: either restrict provisioning to amd64
and explicitly reject other resourceClass architectures, or derive the worker
architecture from nodeRequests[].resourceClass, resolve the matching
architecture-specific DiskImage, and add tests for each supported architecture.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: dd6dc1e1-773f-4246-a974-1d00db382576
📒 Files selected for processing (1)
enhancements/OSAC-2135-caas-bare-metal-worker-provisioning/design.md
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design document that follows all OSAC architectural patterns, provides deep technical specificity (Go structs, proto references, backoff tables, failure mode matrix), has clear scope boundaries with real alternatives, and includes a concrete three-level test plan — scoring 8/8 with no weaknesses severe enough to reduce any criterion. Feedback: Two minor improvements: (1) Add a formal Terminology section defining key terms (shared InfraEnv, system tenant, discovery ignition, MAC correlation, MinHealthyDuration) — the networking EP sets the bar here, and while your usage is consistent, an upfront glossary helps reviewers orient faster. (2) Consider replacing the N/A graduation criteria with a concrete readiness checklist (e.g., 'all CRUD lifecycle operations pass E2E, worker retry converges within 3 attempts for transient failures, no orphaned BMIs after cluster deletion') since the test plan already implies these conditions. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Mostly I think we need to ensure this stays decoupled from ironic/BMO. I don't think there's any reason to be talking about those concepts here.
|
|
||
| ## Proposal | ||
|
|
||
| The ClusterOrder controller in osac-operator gains a new reconciliation phase for bare-metal worker management. When a ClusterOrder's `nodeRequests` reference bare-metal resource classes, the controller: |
There was a problem hiding this comment.
Should it check for a BareMetalInstanceType matching the resource class? Or are these different things?
There was a problem hiding this comment.
reworded to clarify
There was a problem hiding this comment.
Where did you change this? I'm still not sure what this sentence as written means exactly.
| 3. The controller reads the InfraEnv's `status.bootArtifacts.discoveryIgnitionURL` and fetches the discovery ignition content. The ignition is architecture-neutral (the `assisted-installer-agent` image is a multi-arch manifest), so the same InfraEnv serves hosts of any architecture. | ||
| 4. For each bare-metal worker requested, the controller calls `BareMetalInstances.Create` on the private API with: `spec.catalog_item` resolved from the resource class, `spec.image` set to the resolved RHCOS DiskImage ID (see RHCOS DiskImage Resolution), `spec.user_data` set to the fetched ignition content inline (the `user_data` field accepts raw first-boot data up to 64KB; the PoC measured 15KB), `spec.network_attachments` built from the Cluster's `ClusterNetworkAttachment` (subnet + security groups) and the node set's HostType (fabric interface), and `metadata.tenant = "system"` (see System Tenant Isolation). The network attachment mapping is a pass-through: the controller reads the Cluster's `ClusterNetworkAttachment` for the subnet and security group references, resolves the fabric interface name from the node set's HostType definition (first interface with role `fabric`), and constructs a `BareMetalNetworkAttachment` with `primary: true`. BMaaS handles the physical networking — moving the host to the tenant subnet VLAN and assigning an IP via fabric DHCP — as part of BMI provisioning (dependency: OSAC-1437). If the host fails to join the tenant network, the agent will not register on the expected subnet, and the existing `AgentRegistrationTimeout` handles this failure mode. API and ingress VIPs are provisioned by the existing AAP template (MetalLB LoadBalancer Services) and are not managed by this controller. | ||
| 5. The controller updates ClusterOrder status with the BMI references in `workers[]`. | ||
| 6. BMaaS allocates a host, writes the qcow2 to disk via Ironic, and boots with the discovery ignition. The host registers as an Agent with assisted-service. |
There was a problem hiding this comment.
I don't think you should mention Ironic here. In theory this should be backend agnostic.
| 2. The controller removes `Failed` workers first — deletes their dead BMIs and removes their `status.workers` entries. If more removals are needed after clearing all failed slots, the controller decreases NodePool `.spec.replicas` by the remaining excess. | ||
| 3. CAPI's MachineDeployment controller (used by HyperShift's default Replace upgrade type) manages MachineSets, which select Machines for deletion. CaaS does not control the selection order. | ||
| 4. CAPI drains each selected node, then the AgentMachine controller unbinds the Agent (clears `ClusterDeploymentName`, removes labels and ignition refs). | ||
| 5. Because BMH resources exist, the Agent enters `UnbindingPendingUserAction`. The BMH agent controller triggers Ironic deprovision (clears `bmh.Spec.Image`, removes the `detached` annotation). |
There was a problem hiding this comment.
This will not necessarily be true and the BMH, if it does exist, won't be related to the agent directly (it won't have the infraenv lable).
The BMH is an implementation detail of BMaaS. I think this whole section needs to be reworked.
When the host is unbound by CAPI agent controller it will sit in unbinding-pending-user-action and it will remain there. So the next step needs to be deleting the BareMetalInstance.
I don't remember, but does this allow CAPI to properly drain and remove the node? I didn't test this in the PcC.
There was a problem hiding this comment.
So it's possible that assisted will move through "reclaim" in this case, but this all needs to be investigated more. Generally I'd look closely anywhere you're making assumptions about ironic, BMH, etc. This should work with any backend we implement behind the BMaaS API.
|
|
||
| The InfraEnv has no `clusterRef` and no `sshAuthorizedKey` — it generates unbound discovery ignition that is not scoped to any cluster. Agent-to-cluster binding happens explicitly in the correlation phase (step 9), where the controller sets `clusterDeploymentName` on each Agent after MAC-based matching. | ||
|
|
||
| **Why a shared InfraEnv:** The discovery ignition is architecture-neutral (`assisted-installer-agent` is a multi-arch manifest) and does not vary by cluster or tenant. The `cpuArchitecture` field on InfraEnv only affects ISO/kernel/rootfs URLs in `status.bootArtifacts`, not the ignition content — and this design uses the ignition-only flow, not ISO download. The InfraEnv's pull secret is a platform-level credential (Cloud Provider Admin's registry credentials for pulling the discovery agent image), separate from the per-cluster pull secret used for OCP release images. The existing OSAC deployment already uses a single `infraenv` in the `hardware-inventory` namespace with a platform-level `pull-secret`. |
There was a problem hiding this comment.
and does not vary by cluster or tenant.
This is only true if we never have to provide static networking, right? Are we sure that's the case?
There was a problem hiding this comment.
good call, we changed approach to 1 infraenv per cluster to simplify all this
|
|
||
| #### RHCOS DiskImage Resolution | ||
|
|
||
| The controller resolves the RHCOS boot image via a pre-registered DiskImage resource (dependency: OSAC-2540 DiskImage, OSAC-1270 BMI DiskImage integration). The Cloud Infrastructure Admin registers RHCOS qcow2 images as provider-global DiskImages with guest OS family (`linux`) and architecture (`amd64`), and applies a CaaS-specific label `osac.openshift.io/ocp-version: "4.22"` to enable version-based lookup. This label is a CaaS convention — the DiskImage resource itself (OSAC-2540) has no OCP version field, since version-based lookup is a CaaS-specific need. If richer metadata is needed (e.g., multiple image variants per version, automated registration), a dedicated `ClusterDiskImage` resource could wrap DiskImage with CaaS-specific fields. Labeling is sufficient for this design. |
There was a problem hiding this comment.
The Cloud Infrastructure Admin registers RHCOS qcow2 images
@adriengentil we agreed to only support OCI images, right? Do we know how are they getting an RHCOS OCI image to use with this flow before BMO supports bootc?
|
|
||
| The controller resolves the RHCOS boot image via a pre-registered DiskImage resource (dependency: OSAC-2540 DiskImage, OSAC-1270 BMI DiskImage integration). The Cloud Infrastructure Admin registers RHCOS qcow2 images as provider-global DiskImages with guest OS family (`linux`) and architecture (`amd64`), and applies a CaaS-specific label `osac.openshift.io/ocp-version: "4.22"` to enable version-based lookup. This label is a CaaS convention — the DiskImage resource itself (OSAC-2540) has no OCP version field, since version-based lookup is a CaaS-specific need. If richer metadata is needed (e.g., multiple image variants per version, automated registration), a dedicated `ClusterDiskImage` resource could wrap DiskImage with CaaS-specific fields. Labeling is sufficient for this design. | ||
|
|
||
| The controller reads `NodePool.spec.release.image`, extracts the OCP major.minor version (e.g., `4.22` from `ocp-release:4.22.5-x86_64`), and resolves the worker architecture from the node set's `BareMetalInstanceType` (which defines the HostType and its CPU architecture). It then queries for provider-global DiskImages matching both the `osac.openshift.io/ocp-version` label and the resolved architecture. This design targets `amd64` only; other architectures require Cloud Infrastructure Admin to register the corresponding RHCOS DiskImages. |
There was a problem hiding this comment.
Will this always be a tag? What if it's a digest in a mirror or something? Or is it not an image at all?
There was a problem hiding this comment.
good call, addressed in the design proposal
|
|
||
| CaaS-managed BMIs are created under the builtin `system` tenant via the private API. The `system` tenant is excluded from `DetermineVisibleTenants`, so these BMIs are invisible to all regular users without any additional filtering. The private API bypasses tenant-scoped OPA policies because it operates with system-level credentials. Ownership is traceable via the `osac.openshift.io/owner-reference` annotation linking each BMI to its parent ClusterOrder (which belongs to the real tenant). | ||
|
|
||
| The discovery ignition contains the InfraEnv's pull secret and the assisted-service endpoint URL (both platform-level, not cluster-specific). It is passed inline as `user_data` on each BMI (max 64KB; PoC measured 15KB). The `user_data` field is immutable (enforced by the proto `IMMUTABLE` field behavior annotation). The ignition comes from the shared platform-level InfraEnv and is not cluster-scoped — agent-to-cluster binding is enforced by the MAC correlation algorithm, not by the ignition content. |
There was a problem hiding this comment.
The discovery ignition contains the InfraEnv's pull secret and the assisted-service endpoint URL (both platform-level, not cluster-specific)
Is the pull-secret platform level? How does that work in practice? Does the cloud admin use their pull secret? Won't that get into the cluster in a way the tenant user can see?
There was a problem hiding this comment.
Yes, the cloud admin should use their pull secret for the discovery phase to pull the initial image. The discovery ignition is in the BMI's user_data, which is system owned so not readable.
There was a problem hiding this comment.
Sure, the user can't see the user data, but they will be able to pull the secret out of the cluster once it is finished installing. In openshift-config namespace (or something like that)
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows all OSAC architectural patterns, provides deep implementation detail (Go structs, retry tables, correlation algorithms), covers all lifecycle operations with concrete failure handling, and includes a specific multi-level test plan — one of the stronger designs in the enhancement-proposals corpus. Feedback: Two minor dimension gaps worth addressing: (1) explicitly state whether osac-installer Helm charts need changes for the new controller configuration or InfraEnv RBAC, or mark Installation as N/A with a sentence explaining why; (2) add a brief note on inventory capacity planning — even if sizing is the admin's responsibility, a recommendation or link to guidance helps operators. The MAC address field path (TBD pending OSAC-2308/OSAC-3254) is properly flagged as a dependency but should be updated with the concrete field path once that work lands, before this design is considered implementation-ready. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
074cf7c to
85be2ac
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that replaces a fragile static pool with on-demand BMI provisioning, grounded in a validated PoC, with clear dependency tracking, clean architectural separation, and comprehensive test coverage across all three levels. Feedback: The design is strong and implementation-ready once its three blocking dependencies land. Two minor improvements: (1) The Catalog Item Resolution section introduces a ClusterTemplate metadata addition ( Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
85be2ac to
32f8461
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-architected design validated by a working PoC, with clean separation of concerns, sound isolation decisions, and comprehensive failure handling — strong across all dimensions. Feedback: The PR description is stale relative to the design document: it describes label-based visibility filtering ('managed-by: caas') and CaaS-specific DiskImage labels ('osac.openshift.io/ocp-version'), while the actual design uses system tenant isolation and typed ClusterVersion references, which are architecturally superior — update the PR body to match the current design to avoid confusing reviewers. Consider whether the future DiskImage automation path (extracting RHCOS from release payloads) can be scoped into the same release to reduce the day-1 operational burden of the manual 5-step registration process. The dependency on the exact MAC field path in BMI status (OSAC-2308/OSAC-3254) is well-documented but consider defining a placeholder interface now to unblock controller development in parallel. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-validated design that replaces static BareMetalPool pre-boot with on-demand BMI provisioning via a clean controller separation, system tenant isolation, and MAC-based agent correlation — all backed by PoC validation and comprehensive failure handling. Feedback: The design is strong across all dimensions. Two items worth addressing before implementation: (1) the PR description still references 'managed-by: caas label-based visibility filtering' while the design correctly uses system tenant isolation — update the PR body to match the final design to avoid reviewer confusion. (2) Consider making the MinHealthyDuration (1 hour) for attemptCount reset configurable via a controller flag or ConfigMap, since different environments may have different stability baselines, and a hardcoded value will require a code change to tune in production. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
bd35334 to
aa3fa56
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design backed by PoC validation, with clean architectural separation (dedicated reconciler, system tenant isolation, categorized retry), comprehensive test coverage across all tiers, and tightly scoped goals with explicitly gated dependencies. Feedback: The catalog item resolution mechanism is the weakest link — describing it as a 'template parameter' passthrough without specifying the actual plumbing (how it reaches ClusterOrder.nodeRequests, whether it needs CRD changes or is carried as an annotation) leaves an implementation gap that could cause surprises. Consider adding a concrete example of the data flow from ClusterTemplate YAML through to the BareMetalWorkerReconciler's access path. The indefinite retry with no circuit breaker is a deliberate choice but could benefit from an operator-facing annotation (e.g., Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Yeah, thanks. Looks good from my side. Thank you @rccrdpccl ! |
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design document validated by a PoC, with clean architectural patterns (separate controller, system tenant isolation, MAC-scoped correlation), comprehensive failure handling, and honestly identified blocking dependencies. Feedback: The PR description is inconsistent with the design: it describes label-based visibility filtering via a 'managed-by: caas' label requiring fulfillment-service changes, but the design actually uses system tenant isolation requiring no public API changes. Update the PR description to match the design's approach to avoid reviewer confusion. Consider adding a brief discussion of rate limiting for BMI creation API calls in large-cluster scenarios (e.g., 50+ workers) and whether there are practical upper bounds on worker count per cluster. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
- Add OSAC-1437 as blocking dependency with network attachment pass-through (ClusterNetworkAttachment → BareMetalNetworkAttachment) - Switch to shared platform-level InfraEnv (architecture-neutral ignition, platform-level pull secret, no clusterRef) - Replace managed-by label with system tenant isolation - Replace corev1.ObjectReference with WorkerStatus struct (phase, attemptCount, failure details, nextRetryTime) - Add aggregate counts: desiredWorkers, currentWorkers, readyWorkers - Infinite retry with escalating backoff, no PermanentlyFailed state - Clarify DiskImage source_type owned by OSAC-1270 - Add OSAC-1604 alignment note for tenant-visible status Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
- Clarify DiskImage architecture resolved from BareMetalInstanceType - Fix RBAC section to enumerate Agent permissions (get/list/watch/patch) - Fix currentWorkers example (3 not 5, excludes Failed) - Simplify ignition flow: inline passthrough via user_data, no intermediate Secret - Separate Failed (provisioning retry) from Unbinding/Deleting (teardown) — teardown failures do not trigger replacement - Add BMI creation idempotency: list-before-create with ownership label, note unique constraint alternative - Renumber provisioning steps (1-9) after Secret removal Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
- Remove all Ironic/BMH/Metal3 references — design treats BMaaS as a black box, interacts only via private API - Rework scale-down: watch for unbinding-pending-user-action (not *-unbound), then delete BMI directly - Revert to per-cluster InfraEnv (clusterRef, ownerReference) for simplicity and to avoid pull secret scope / static networking concerns; note shared InfraEnv as future pooling optimization - Clarify DiskImage resolution: driven by ClusterVersion.spec.version, no release image pullspec parsing. Document manual OCI packaging steps and future automation path via release image introspection - Clarify resourceClass → HostType resolution - Fix step numbering throughout Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
- Separate BM-specific logic into BareMetalWorkerReconciler, keeping ClusterOrder controller generic (ready for future VM worker support) - Both controllers watch the same ClusterOrder CR; shared status.workers[] partitioned by kind field - Add BMaaS-owned worker lifecycle as evaluated alternative - Update goals, test plan, and references throughout Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
- Replace label-based DiskImage lookup with typed DiskImageReference on ClusterVersionSpec (per OSAC-1330 type-safe references) - Reference validation at ClusterVersion creation time, deletion protection, no ambiguity (eliminates RHCOSImageAmbiguous condition) - Update manual registration flow: link DiskImage to ClusterVersion via --disk-image flag on CLI - Flag cluster upgrade dependency as a risk (PRD stage) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
e90598f to
0c838b1
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that replaces a fragile static pool with on-demand provisioning using validated patterns (PoC-backed, controller-runtime, system tenant isolation); the multi-node-set NodePool mapping and shared-status patch strategy are the main areas needing clarification before implementation. Feedback: The design should clarify how multiple node sets (e.g., 'compute' + 'gpu' with different BareMetalInstanceTypes) map to NodePools — the current text references a single NodePool and CAPI's random Machine selection, which won't correctly handle per-node-set scale-down if each node set should scale independently. Commit to a specific strategy for the shared status.workers[] patch (server-side apply with field ownership would be cleanest) rather than leaving it as 'either/or'. The management-state annotation check (osac.openshift.io/management-state → skip reconciliation when Unmanaged) is an established osac-operator pattern that the BareMetalWorkerReconciler section does not mention — ensure it is included for consistency with other controllers. Critical (0)None. Important (4)
Suggestions (4)
Review costModel: claude-opus-4-6 |
0c838b1 to
205f4d7
Compare
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, implementation-ready design document that covers all required template sections with concrete technical detail, validated by a PoC, with explicit dependency tracking and well-reasoned architectural decisions. Feedback: The PR description mentions label-based visibility filtering ('managed-by: caas' label) for tenant isolation, but the design document correctly uses the system tenant approach instead — update the PR description to match the final design to avoid reviewer confusion. The MAC address field path is marked 'TBD' pending OSAC-2308/OSAC-3254; consider adding an Open Questions section for this since it affects the controller's core correlation logic. The shared status.workers[] between two controllers warrants a brief note on whether a Server-Side Apply field manager strategy would be preferable to the described patchStatusWithRetry approach, as SSA provides stronger field-ownership guarantees for multi-controller status updates. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
…sistency fixes ClusterNodeSet: replace HostType with BareMetalInstanceTypeReference. Add nodeSet field to WorkerStatus for multi-node-set scaling. Own disk_image field on ClusterVersionSpec (with upgrade path). Add OSAC-1330 to dependency table. User flow: unified Setup Flow with before/after CLI commands and explanations for DiskImage, BareMetalInstanceType, and template changes. BMI Create field-source table in provisioning step 4. Networking: use ClusterNetworkAttachment, add Network Attachment Enrichment section, read subnet from ClusterOrder CRD. Installer/Enclave: document impact on importAgents, assisted image service, pool playbooks, and Wizard configuration. Fixes: stale InfraEnv reference, gRPC timeout pattern, RBAC for infraenvs, worker_type metric values, patchStatusWithRetry note, InfraEnv deletion recovery, OSAC-3266 name uniqueness for BMI creation idempotency. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
205f4d7 to
8ecdf71
Compare
AI Design Review: EP-198Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-structured design that demonstrates strong feasibility (PoC-validated), clear scope boundaries, and concrete testability, held back from a top score only by an underspecified dual-controller status write strategy and the lack of a circuit breaker on infinite retries. Feedback: Concretize the dual-controller status write strategy: specify whether the BareMetalWorkerReconciler uses Server-Side Apply with a distinct field manager or a dedicated status patch path that avoids clobbering ClusterOrder controller fields — the current 'must be extended' language leaves a real race condition unresolved. Consider adding an operator-configurable maximum attempt count (or circuit breaker) for worker retries to prevent resource waste on persistent misconfigurations; indefinite retry with 30m cap is reasonable for transient failures but burns hosts repeatedly when the root cause is a wrong DiskImage or broken network. Finally, define a contract or interface boundary for the MAC address field on BMI status (even if the exact proto field is TBD) so integration tests can be written against a stable expectation. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Just some minor comments. Seems most of the real issues are resolved.
|
|
||
| ```bash | ||
| osac-admin create diskimage rhcos-4.18 \ | ||
| --source-ref quay.io/osac/rhcos:4.18.0 \ |
There was a problem hiding this comment.
I'm guessing this is a placeholder or are we actually going to publish these?
There was a problem hiding this comment.
yep, it's just to give an idea but we're not planning to publish these in this Design at least
|
|
||
| #### Cluster Deletion | ||
|
|
||
| On ClusterOrder deletion, the worker reconciler does not need to explicitly orchestrate scale-down. Deleting the HostedCluster cascades through HyperShift (deletes all NodePools) → CAPI (drains nodes, deletes Machines) → CAPA (unbinds Agents). The worker reconciler reacts to Agents entering `unbinding-pending-user-action` and cleans up Agent CRs and BMIs through the normal scale-down watch (steps 5-8). The ClusterOrder's finalizer holds until all `status.workers[]` entries are cleaned up. The InfraEnv CR is garbage collected via its ownerReference to the ClusterOrder. |
There was a problem hiding this comment.
BMI's should still be deleted though, right?
| } | ||
| ``` | ||
|
|
||
| The system-owned catalog item is a deployment prerequisite — created during OSAC installation with unlocked parameters so the CaaS controller can set image, user_data, and network_attachments freely. The `BareMetalInstanceType` referenced in the `ClusterNodeSet` determines which host hardware profile is allocated by BMaaS. |
There was a problem hiding this comment.
created during OSAC installation by who? The install scripts or one of the admins?
There was a problem hiding this comment.
explicitly added a proposal on how we can add it
- Backend decoupling: drop Metal3/Ironic examples in host labeling; state CaaS does not assume a specific BMaaS backend (carbonin) - Cluster deletion: clarify BMIs are actively deleted by the ClusterOrder finalizer via BareMetalInstances.Delete — the HyperShift/CAPI/CAPA cascade cleans up K8s objects and Agent CRs only, BMIs are unknown to HyperShift (carbonin r3786801576) - System-owned catalog item: specify it is created by automation (installer seed + controller reconcile), not by an admin, gated on CaaS deployed + BMaaS integrated + a BareMetalInstanceType registered (carbonin r3786851230) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
AI Design Review: EP-198Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design with strong PoC validation, clear dependency tracking, sound architectural separation, and comprehensive test coverage — the two unresolved implementation details (catalog item seeding mechanism, status patch strategy) are minor and appropriate to defer to implementation. Feedback: Two open items should be resolved before implementation begins: (1) Decide whether the system-owned BareMetalInstanceCatalogItem is seeded by the installer chart or reconciled by the controller — the 'either way' framing is fine for design review but the implementation PR will need a clear answer, and the choice affects the installer's bare-metal integration toggle. (2) Resolve the status patch strategy for shared status.workers[] between the two controllers — optimistic concurrency handles correctness, but deciding between extending patchStatusWithRetry vs a separate patch path determines the code structure and testing approach. Additionally, consider adding explicit test cases for retry/backoff behavior (e.g., verify escalating backoff caps, verify attemptCount reset after MinHealthyDuration) since the retry logic is a key correctness property of the design. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, rccrdpccl 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 |
- Use ClusterNetworkAttachment (proto-aligned) instead of ComputeInstance's NetworkAttachment type — different semantics for cluster vs per-NIC - Add hook stability contract (document variables, compatibility test) - CUDN naming uses ClusterOrder UID (not cluster_name) for uniqueness - Reject non-empty SecurityGroupRefs in Phase 1 - Add NAD readiness wait and VM sizing validation - Clarify Phase 1/2 boundary: triggers, migration path, production-grade - Add status.workers[] for CAPK VM inventory visibility (same pattern as BMI workers in PR osac-project#198) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
6 participants
Design: CaaS Bare-Metal Worker Node Provisioning
Jira: OSAC-2135
PRD: prd.md
Summary
Replaces the static BareMetalPool-based agent pre-boot pool with on-demand bare-metal worker provisioning. The ClusterOrder controller creates individual BareMetalInstances via the fulfillment-service private API, each booting a pre-registered RHCOS DiskImage with cluster-specific discovery ignition. Agents register, are correlated to BMIs via MAC address, and join the HyperShift-managed cluster as worker nodes. CaaS-managed BMIs are hidden from tenant APIs via label-based visibility filtering.
Requesting Review On
osac.openshift.io/ocp-version) on DiskImage resources for version-based lookup. This label is not part of the DiskImage PRD — alignment needed.managed-by: caaslabel. Requires fulfillment-service changes (baremetal_instances_server.go).cluster_infraAAP step and the existing pool-based flow are removed entirely — no coexistence period. Upgrade procedure includes AAP job queue drain.source_typevalue (disk_image): Proto schema change onBareMetalInstanceImage— version skew implications documented.Documents
design.md— technical design documentHow to Review
Summary by CodeRabbit
New Features
Documentation