ECOPROJECT-3992 | feat: sizer - support hosted control plane nodes#970
Merged
openshift-merge-bot[bot] merged 1 commit intokubev2v:mainfrom Mar 19, 2026
Merged
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:
📝 WalkthroughWalkthroughAdds a new boolean field Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler as "API Handler\n(inbound mapper)"
participant Mapper as "Service Mapper\n(ClusterRequirementsRequestForm)"
participant SizerSvc as "Sizing Service"
participant External as "External Sizer"
Client->>APIHandler: POST ClusterRequirementsRequest (hostedControlPlane?)
APIHandler->>Mapper: map request -> form (propagate HostedControlPlane)
Mapper->>SizerSvc: CalculateClusterRequirements(form)
alt hostedControlPlane == true
SizerSvc->>SizerSvc: effectiveCP = 0\nincludeControlPlane = false
else
SizerSvc->>SizerSvc: effectiveCP = ControlPlaneNodeCount\nincludeControlPlane = true
end
SizerSvc->>External: POST /api/v1/size/custom (payload uses effectiveCP)
External-->>SizerSvc: sizer response
SizerSvc-->>APIHandler: transformed sizing result (controlPlaneNodes, workerNodes, total)
APIHandler-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/sizer_test.go`:
- Around line 357-391: The test currently leaves ControlPlaneNodeCount nil so it
doesn't exercise the new "bypass on HostedControlPlane" validation branch;
modify the test's request to set ControlPlaneNodeCount to an otherwise-invalid
value (e.g., cp := -1; request.ControlPlaneNodeCount = &cp) while keeping
hostedTrue := true so that handlers.CalculateAssessmentClusterRequirements (and
the ClusterRequirementsRequest) demonstrates that hostedControlPlane=true skips
the handler-side check and still returns 200.
In `@internal/service/sizer_test.go`:
- Around line 299-315: The test currently only asserts the transformed response;
add assertions that the outbound HTTP payload sent to the mock created by
createTestSizerServer (the request hitting /api/v1/size/custom) omits the
control-plane machine set and any control-plane workload when
request.HostedControlPlane is true — capture the incoming request body in the
test server handler, decode/inspect the JSON payload and assert the controlPlane
machineSet/workload fields are absent or empty before calling
sizerService.CalculateClusterRequirements (refer to the test's
request.HostedControlPlane, createTestSizerServer, and the /api/v1/size/custom
handler to locate where to capture and assert the outbound payload).
In `@internal/service/sizer.go`:
- Line 271: Normalize the effective control-plane node count once and reuse it
everywhere instead of recalculating or leaking the raw
req.ControlPlaneNodeCount: compute a single variable (e.g., effectiveCP :=
effectiveControlPlaneNodeCount(req)) before building the payload and before
calling transformSizerResponse, pass effectiveCP into buildSizerPayload (or
ensure buildSizerPayload uses the provided effectiveCP) and use effectiveCP in
any tracing/logging instead of req.ControlPlaneNodeCount; update references
around transformSizerResponse, buildSizerPayload, includeControlPlane and the
trace/span recording so they all use the same normalized effectiveCP value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0502a39d-40c7-4010-8ce6-fa84ad513aca
📒 Files selected for processing (9)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.go
ronenav
force-pushed
the
3992-hosted-control-plane
branch
from
8b1f63b to
a095a95
Compare
Collaborator
|
/retest |
ronenav
force-pushed
the
3992-hosted-control-plane
branch
from
a095a95 to
57d5719
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/sizer_test.go`:
- Around line 333-336: The test currently checks shapes but not scheduling;
update the assertions to verify the workload is scheduled only on the "worker"
machineset by asserting sizerPayload.Workloads[0].MachineSetNames (or the field
that lists target machinesets on the Workload struct) has length 1 and contains
exactly "worker" (e.g. Expect(...).To(HaveLen(1)) and
Expect(...).To(ContainElement("worker")) or an equality assertion to
[]string{"worker"}), so the hosted-control-plane contract is locked to the
worker-only placement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e132fe04-d815-412e-89c1-cd4aa914778a
📒 Files selected for processing (9)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.go
ronenav
force-pushed
the
3992-hosted-control-plane
branch
from
57d5719 to
cad296c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/service/sizer_test.go (1)
333-337:⚠️ Potential issue | 🟡 MinorAdd explicit assertion for worker-only workload placement.
The test validates shape, but not that the workload targets only the
workermachineset.🔧 Suggested assertion
Expect(sizerPayload.Workloads).To(HaveLen(1)) Expect(sizerPayload.Workloads[0].Name).To(Equal("vm-workload")) + Expect(sizerPayload.Workloads[0].UsesMachines).To(Equal([]string{"worker"}))As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/sizer_test.go` around lines 333 - 337, The test currently checks sizerPayload.MachineSets and sizerPayload.Workloads but doesn't assert that the workload is targeted only at the "worker" machineset; update the test to explicitly assert the workload's placement/target references the worker set by examining sizerPayload.Workloads[0] (for example its Placement, TargetMachineSet, or AllowedMachineSets field) and asserting it equals or contains only "worker" (and assert no other targets are present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/service/sizer_test.go`:
- Around line 333-337: The test currently checks sizerPayload.MachineSets and
sizerPayload.Workloads but doesn't assert that the workload is targeted only at
the "worker" machineset; update the test to explicitly assert the workload's
placement/target references the worker set by examining
sizerPayload.Workloads[0] (for example its Placement, TargetMachineSet, or
AllowedMachineSets field) and asserting it equals or contains only "worker" (and
assert no other targets are present).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff5853ed-6371-4024-89b4-18b843ad559d
📒 Files selected for processing (9)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.go
nirarg
reviewed
Mar 16, 2026
ronenav
force-pushed
the
3992-hosted-control-plane
branch
from
cad296c to
f45ff4d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/service/sizer_test.go (1)
333-337: 🧹 Nitpick | 🔵 TrivialStrengthen payload assertion for worker-only scheduling.
The test verifies the payload structure but should also assert that the workload is scheduled only on
"worker"machines to fully lock the hosted-control-plane contract.🔧 Suggested assertion
Expect(sizerPayload.MachineSets).To(HaveLen(1)) Expect(sizerPayload.MachineSets[0].Name).To(Equal("worker")) Expect(sizerPayload.Workloads).To(HaveLen(1)) Expect(sizerPayload.Workloads[0].Name).To(Equal("vm-workload")) + Expect(sizerPayload.Workloads[0].UsesMachines).To(Equal([]string{"worker"}))As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/sizer_test.go` around lines 333 - 337, Add an assertion that the workload is scheduled exclusively to the "worker" machines by checking the workload scheduling field used in the payload (e.g., sizerPayload.Workloads[0].MachineSetNames or the workload's NodeSelector/Affinity field) and assert it equals/contains only "worker" (for example expect the slice to equal []string{"worker"} or the NodeSelector to be map[string]string{"node-role.kubernetes.io/worker": "true"}); update the test around sizerPayload.Workloads[0] where you already assert the name to include this exclusive-scheduling check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/service/sizer_test.go`:
- Around line 333-337: Add an assertion that the workload is scheduled
exclusively to the "worker" machines by checking the workload scheduling field
used in the payload (e.g., sizerPayload.Workloads[0].MachineSetNames or the
workload's NodeSelector/Affinity field) and assert it equals/contains only
"worker" (for example expect the slice to equal []string{"worker"} or the
NodeSelector to be map[string]string{"node-role.kubernetes.io/worker": "true"});
update the test around sizerPayload.Workloads[0] where you already assert the
name to include this exclusive-scheduling check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f2778be-3c26-459c-bcff-3e0842b4f701
📒 Files selected for processing (9)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.go
Signed-off-by: Ronen Avraham <ravraham@redhat.com>
ronenav
force-pushed
the
3992-hosted-control-plane
branch
from
f45ff4d to
c5e87ce
Compare
nirarg
approved these changes
Mar 19, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When hostedControlPlane is true, omit control plane machine set and workload from the sizer payload and return 0 control plane nodes.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests