OSAC-1421: Tenant-managed cluster node_sets - #108
openshift-merge-bot[bot] merged 1 commit into
Conversation
WalkthroughThe design and PRD now define a five-step adapter-driven provisioning wizard, scoped catalog overlays, tenant-composed cluster ChangesWizard specification updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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 |
AI EP Review: EP-108Score: 8/10 | Verdict: PASS
Verdict: The PRD update is well-structured with clear user-observable capabilities and focused scope, held back slightly by absent business justification for the design changes and minor design leakage (internal function/proto references) in what should be a user-facing document. Feedback: Move the three internal implementation references out of prd.md into design.md: 'applyFieldDefinitions' (line referencing fulfillment server-side behavior), 'cluster_type.proto', and 'Per ClusterNodeSet in cluster_type.proto' — restate these as user-observable behaviors instead (e.g., 'fulfillment may still apply the catalog default server-side when defined' without naming the internal function). Add a brief rationale in the PRD for why cluster node_sets shifted from template-driven to tenant-composed — what user problem or product constraint motivated this change? This strengthens the WHY for reviewers evaluating the design shift. Critical (0)None. Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-108Score: 8/8 | Verdict: PASS
Verdict: A well-structured design revision that simplifies cluster node_sets to tenant-composed rows, extends catalog overlay to General basics fields, resolves open optionality decisions, and adds a thorough component test plan — all changes are consistent across PRD and design doc with clear API contracts. Feedback: The resolveGeneralFields optional adapter method appears alongside the existing static generalFields property, but the design doesn't specify resolution priority or when to use one vs the other — a one-line note on fallback behavior would prevent implementer confusion. Consider specifying the UX for a failed HostTypes.List load on the cluster Configuration step (empty table? retry button? inline error?), since the test plan references 'picker hook error' generically but the design doesn't describe this failure mode. The server-side applyFieldDefinitions fallback for omitted basics fields is mentioned once — confirm with the fulfillment team that this contract is stable, since the wizard's omit-when-blank behavior depends on it. Critical (0)None. Important (1)
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: 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/cluster-and-vm-provisioning-wizard/prd.md`:
- Around line 94-99: The validation matrix for the wizard currently implies
read-only fields still use validation_schema, which conflicts with the
field-definition contract. Update the PRD entry around the wizard field mapping
so validation only applies to editable fields, and ensure any field with
editable: false is either excluded from Yup validation or required to have a
default. Reference the matching-entry rules for Label, Editable, Default, and
Validation in the provisioning wizard section when making the fix.
🪄 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: 9f8dcca2-59b7-49b0-b53d-a720d4900914
📒 Files selected for processing (2)
enhancements/cluster-and-vm-provisioning-wizard/design.mdenhancements/cluster-and-vm-provisioning-wizard/prd.md
|
+1 looks good to me as well |
|
@vladikr: 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. |
|
|
||
| **Cluster Networking specifics:** `spec.network.pod_cidr` and `spec.network.service_cidr` are optional — omit from payload when empty. Yup validates format only when a value is present. | ||
|
|
||
| **Step validation:** Next is always enabled. On click, run the step Yup schema, `setTouched` for all step fields, surface inline errors for untouched fields, and show an alert if invalid; do not advance until the step passes. | ||
|
|
||
| ### API Extensions | ||
|
|
||
| No API extensions. The wizard consumes existing `ComputeInstanceCatalogItems`, `ClusterCatalogItems`, `InstanceTypes`, networking list APIs (`GET /api/fulfillment/v1/virtual_networks`, `.../subnets`, `.../security_groups`), `ClusterTemplates`, `HostTypes`, and create APIs. Server-side catalog validation (`catalog_item_validation.go`) is unchanged. | ||
| No API extensions to create payloads. The wizard consumes existing `ComputeInstanceCatalogItems`, `ClusterCatalogItems`, `InstanceTypes`, networking list APIs (`GET /api/fulfillment/v1/virtual_networks`, `.../subnets`, `.../security_groups`), `HostTypes.List` (`GET /api/fulfillment/v1/host_types`), and create APIs. Server-side catalog validation (`catalog_item_validation.go` / `applyFieldDefinitions`) still applies catalog `field_definitions` on create when the client omits a field the wizard left blank. The wizard does **not** use `ClusterTemplates.Get` for Configuration `node_sets`. |
There was a problem hiding this comment.
if the tenant user won't put any value in the nodeset then it will be taken by the BE from the catalog item. And if the catalog item don't have nodeset as well, it will be taken from the template default. So why not showing it in the UI?
like any other editable/non editable field we show
There was a problem hiding this comment.
Why do we need two levels of defaults? Why not just have default values in the catalog item?
If we want to show default values for node sets we need to define what happens if it contains a none existing host type.
There was a problem hiding this comment.
We agreed to wait for API so let's just merge this now as this was implementation for 0.1
Concern: UI duplicating backend default-resolution logicThe current design has the UI reimplementing the backend's default-resolution pipeline — reading The This will keep accumulating inconsistencies as new fields or overlay rules are added. Suggestion: backend-owned default resolutionInstead of the UI maintaining a parallel overlay implementation, consider a "resolve defaults" endpoint — e.g.
The UI becomes a renderer: call resolve on catalog selection, populate the form with what comes back, mark fields editable or read-only based on the response metadata. No client-side overlay logic, no need to know about templates or Benefits:
This could be a v2 improvement — not necessarily blocking this PR — but worth considering before the UI overlay logic grows further. |
… main pass These 3 directories were missed by the original OSAC-2870 naming cleanup (osac-project#139/osac-project#144) specifically because they had active, unmerged PRs against them at the time the plan was drafted, so their Jira keys and directory names were still moving targets: - storage-control-plane-osac-2872 -> OSAC-2872-storage-control-plane (Jira key existed, but was still on an open PR (osac-project#134) that hadn't merged to main yet when osac-project#139 was planned/built) - cluster-and-vm-provisioning-wizard -> OSAC-1421-cluster-and-vm-provisioning-wizard (key OSAC-1421 has been in the doc's tracking-link since June; PR osac-project#108 was open against it at audit time) - metering-and-usage-tracking -> OSAC-985-metering-and-usage-tracking (key OSAC-985 has been in the doc's tracking-link for weeks; PRs osac-project#131 and osac-project#143 were open against it at audit time) Updated the one cross-reference found repo-wide pointing at the old cluster-and-vm-provisioning-wizard path (in OSAC-1319-bare-metal-instance-ui/design.md). No cross-references found for the other two. Note: PR osac-project#131 (open, adds a new metering-and-usage-tracking/design.md) will need to retarget to the new path when it rebases, since it adds a file git has no rename history for -- flagging this on that PR separately. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Replace template-driven node_sets with tenant-composed rows: host type dropdown from HostTypes.List, size per row, map key = host type id, unique host types only, no catalog defaults in v1. Assisted-by: Claude Code <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
5662f32 to
ac10ca0
Compare
AI EP Review: EP-108Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear user-observable capabilities, detailed field specifications, and well-resolved open decisions; the only gap is that the business justification for the node_sets approach shift is not explicitly stated in the changed sections. Feedback: The shift from template-driven to tenant-composed node_sets is a significant design decision — add a sentence in the summary or §2.1.1 notes explaining why this better serves tenants (e.g., flexibility, no dependency on pre-configured templates). Remove the 'applyFieldDefinitions' function name from §2.1.2 and the 'ClusterNodeSet in cluster_type.proto' reference from §2.1.6 — these are server-side implementation details; describe the payload contract in user-facing terms only. Two open '?' fields remain unresolved in the cluster table (pod_cidr, service_cidr, boot_disk.size_gib) — consider resolving them in this revision to avoid blocking implementation. Critical (0)None. Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md`:
- Line 73: Update adapter.getReviewSections() so the Review step redacts the
pull_secret value rather than exposing the raw secret, while preserving the
existing wizard field labels and values for other fields. Add coverage verifying
the raw pull_secret never appears in the rendered Review sections.
- Line 88: Resolve the inconsistency between tenant-cleared optional basics
fields and fulfillment defaults by choosing and documenting an explicit
suppression/null contract or fallback-to-default semantics. Update
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md lines 88 and
101, enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md lines
292-293, and enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md
line 101 so the applyFieldDefinitions behavior, acceptance language, and payload
tests all reflect the chosen backend behavior.
In `@enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md`:
- Around line 244-247: Resolve the requiredness contradiction by marking
spec.boot_disk.size_gib, spec.network.pod_cidr, and spec.network.service_cidr as
optional in the acceptance criteria and removing their entries from §5 Open
Decisions. Keep the claim that all requiredness decisions are resolved.
🪄 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: a8830ca3-94e0-4b7a-b478-b7f54a3d7898
📒 Files selected for processing (2)
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md
AI Design Review: EP-108Score: 7/8 | Verdict: PASS
Verdict: Strong UI design revision with excellent implementation specificity and test coverage; held back from a perfect score only by silent gaps on E2E testing and documentation cross-cutting dimensions. Feedback: Address the E2E testing and Documentation dimensions from osac-dimensions.md — either describe what's needed (e.g., Cypress E2E scenarios in osac-test-infra, user-facing doc updates for the new wizard) or explicitly defer them with a rationale. This is the only gap preventing a top score. Consider also noting what happens when the HostTypes.List returns an empty list (no available host types) — the current design specifies 'at least one row required' validation but doesn't address the case where the host type picker has no options to offer. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Now that we have API this will change, but for now we should get this merged to match the implementation we have for 0.1
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, danmanor, ElayAharoni 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 |
batzionb
changed the title
|
@batzionb: This pull request references OSAC-1421 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. |
… main pass These 3 directories were missed by the original OSAC-2870 naming cleanup (osac-project#139/osac-project#144) specifically because they had active, unmerged PRs against them at the time the plan was drafted, so their Jira keys and directory names were still moving targets: - storage-control-plane-osac-2872 -> OSAC-2872-storage-control-plane (Jira key existed, but was still on an open PR (osac-project#134) that hadn't merged to main yet when osac-project#139 was planned/built) - cluster-and-vm-provisioning-wizard -> OSAC-1421-cluster-and-vm-provisioning-wizard (key OSAC-1421 has been in the doc's tracking-link since June; PR osac-project#108 was open against it at audit time) - metering-and-usage-tracking -> OSAC-985-metering-and-usage-tracking (key OSAC-985 has been in the doc's tracking-link for weeks; PRs osac-project#131 and osac-project#143 were open against it at audit time) Updated the one cross-reference found repo-wide pointing at the old cluster-and-vm-provisioning-wizard path (in OSAC-1319-bare-metal-instance-ui/design.md). No cross-references found for the other two. Note: PR osac-project#131 (open, adds a new metering-and-usage-tracking/design.md) will need to retarget to the new path when it rebases, since it adds a file git has no rename history for -- flagging this on that PR separately. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
… main pass These 3 directories were missed by the original OSAC-2870 naming cleanup (osac-project#139/osac-project#144) specifically because they had active, unmerged PRs against them at the time the plan was drafted, so their Jira keys and directory names were still moving targets: - storage-control-plane-osac-2872 -> OSAC-2872-storage-control-plane (Jira key existed, but was still on an open PR (osac-project#134) that hadn't merged to main yet when osac-project#139 was planned/built) - cluster-and-vm-provisioning-wizard -> OSAC-1421-cluster-and-vm-provisioning-wizard (key OSAC-1421 has been in the doc's tracking-link since June; PR osac-project#108 was open against it at audit time) - metering-and-usage-tracking -> OSAC-985-metering-and-usage-tracking (key OSAC-985 has been in the doc's tracking-link for weeks; PRs osac-project#131 and osac-project#143 were open against it at audit time) Updated the one cross-reference found repo-wide pointing at the old cluster-and-vm-provisioning-wizard path (in OSAC-1319-bare-metal-instance-ui/design.md). No cross-references found for the other two. Note: PR osac-project#131 (open, adds a new metering-and-usage-tracking/design.md) will need to retarget to the new path when it rebases, since it adds a file git has no rename history for -- flagging this on that PR separately. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
6 participants
Summary
Updates the cluster and VM provisioning wizard PRD and design for tenant-managed
spec.node_setson the Configuration step:ClusterTemplate.spec.node_setshost_type(dropdown fromHostTypes.List) andsizeonly (ClusterNodeSet)field_definitionsdefaults forspec.node_setsdo not apply — table starts empty on catalog selectionJira
OSAC-1421
Files
enhancements/cluster-and-vm-provisioning-wizard/prd.md— §2.1.1, §2.1.2, new §2.1.6, acceptance criteria, §5enhancements/cluster-and-vm-provisioning-wizard/design.md— Cluster Configuration specifics, test plan, sequence diagramTest plan
ClusterNodeSet:host_type+size)Made with Cursor
Summary by CodeRabbit