🌱 Add missing Fields to RosaControlPlane #4780

serngawy · 2024-02-06T22:57:47Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adding the missing RosaControlPlan API fields;

Autoscaling
ComputeMachineType
ServiceCIDR
PodCIDR
HostPrefix

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
issue

Special notes for your reviewer:

Checklist:

Release note:

None

k8s-ci-robot · 2024-02-06T22:57:56Z

Hi @serngawy. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

serngawy · 2024-02-06T23:22:25Z

/cc @stevekuznetsov

serngawy · 2024-02-06T23:23:17Z

/cc @muraee

k8s-ci-robot · 2024-02-06T23:23:19Z

@serngawy: GitHub didn't allow me to request PR reviews from the following users: muraee.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @muraee

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/test-infra repository.

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

api/v1beta1/conversion_test.go

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

controlplane/rosa/controllers/rosacontrolplane_controller.go

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

controlplane/rosa/controllers/rosacontrolplane_controller.go

vincepri · 2024-02-12T17:03:12Z

/ok-to-test
/assign @muraee

vkareh · 2024-02-16T19:14:16Z

I think we should hold this PR until the OCM/ROSA team has had a chance to review.
/cc @pvasant @robpblake

k8s-ci-robot · 2024-02-16T19:14:18Z

@vkareh: GitHub didn't allow me to request PR reviews from the following users: pvasant, robpblake.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I think we should hold this PR until the OCM/ROSA team has had a chance to review.
/cc @pvasant @robpblake

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/test-infra repository.

robpblake · 2024-02-20T15:29:07Z

config/crd/bases/controlplane.cluster.x-k8s.io_rosacontrolplanes.yaml

+                type: string
+              autoscaling:
+                description: Autoscaling specifies auto scaling behaviour for this
+                  NodePool.


I believe the general preference is to refer to MachinePool and not NodePool. There is no customer-visible mention of NodePool within ROSA CLI.

robpblake · 2024-02-20T15:29:14Z

config/crd/bases/controlplane.cluster.x-k8s.io_rosacontrolplanes.yaml

-                  the cluster, for example "10.0.0.0/16".
-                type: string
+              network:
+                description: Network config for the ROSA-HCP cluster.


Should be ROSA HCP without the -.

serngawy · 2024-02-20T20:00:05Z

/test pull-cluster-api-provider-aws-apidiff-main

muraee · 2024-02-21T08:59:51Z

@serngawy I think you need to rebase your branch.

stevekuznetsov · 2024-02-21T14:05:09Z

Ideally this lands after #4765

serngawy · 2024-02-21T14:49:18Z

Ideally this lands after #4765

I don't mind @stevekuznetsov but if it will takes more time to merge your PR, I would rather merge this one as there is another PR here waiting this one.

k8s-ci-robot · 2024-02-28T08:44:23Z

PR needs rebase.

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/test-infra repository.

stevekuznetsov · 2024-02-28T20:08:35Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+	// +optional
+	InstanceType string `json:"instanceType,omitempty"`
+
+	// Autoscaling specifies auto scaling behaviour for this MachinePool.


This GoDoc need to explain what MachinePool the content is for. Note that this spec field is on the ROSAControlPlane object.

stevekuznetsov · 2024-02-28T20:09:37Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+
+	// The instance type to use, for example `r5.xlarge`. Machine type ref; https://aws.amazon.com/ec2/instance-types/
+	// +optional
+	InstanceType string `json:"instanceType,omitempty"`


Is there a reason we do not want to support all the fields from the ROSAMachinePool here?

I believe we give the options for the default MP here, otherwise the user the need to define the RosaMP

stevekuznetsov · 2024-02-28T20:10:21Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+	ServiceCIDR string `json:"serviceCIDR,omitempty"`
+
+	// Network host prefix which is defaulted to `23` if not specified.
+	// +kubebuilder:default:=23


Suggested change

// +kubebuilder:default:=23

// +kubebuilder:default=23

Seems like either := or = works but we use = exclusively elsewhere.

stevekuznetsov · 2024-02-28T20:10:33Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+
+	// The CNI network type default is OVNKubernetes.
+	// +kubebuilder:validation:Enum=OVNKubernetes;Other
+	// +kubebuilder:default:=OVNKubernetes


Suggested change

// +kubebuilder:default:=OVNKubernetes

// +kubebuilder:default=OVNKubernetes

Signed-off-by: melserngawy <melserng@redhat.com>

stevekuznetsov

/lgtm
/approve

k8s-ci-robot · 2024-02-28T22:18:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~config/crd/bases/OWNERS~~ [stevekuznetsov]
~~controlplane/rosa/OWNERS~~ [stevekuznetsov]
~~templates/OWNERS~~ [stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 6, 2024

k8s-ci-robot requested a review from cnmcavoy February 6, 2024 22:57

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2024

k8s-ci-robot requested a review from fiunchinho February 6, 2024 22:57

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2024

serngawy force-pushed the fields branch from 1dff6dd to 4160ad0 Compare February 6, 2024 22:58

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2024

serngawy force-pushed the fields branch from 4160ad0 to 0b12a65 Compare February 6, 2024 23:16

k8s-ci-robot requested a review from stevekuznetsov February 6, 2024 23:22

muraee reviewed Feb 7, 2024

View reviewed changes

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go Outdated Show resolved Hide resolved

muraee reviewed Feb 7, 2024

View reviewed changes

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go Outdated Show resolved Hide resolved

muraee reviewed Feb 7, 2024

View reviewed changes

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go Outdated Show resolved Hide resolved

serngawy force-pushed the fields branch from 0b12a65 to 9fb56a5 Compare February 7, 2024 17:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2024

stevekuznetsov reviewed Feb 7, 2024

View reviewed changes

api/v1beta1/conversion_test.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Feb 7, 2024

View reviewed changes

muraee reviewed Feb 8, 2024

View reviewed changes

controlplane/rosa/controllers/rosacontrolplane_controller.go Outdated Show resolved Hide resolved

serngawy force-pushed the fields branch 2 times, most recently from f7a1fe1 to 2e45c68 Compare February 9, 2024 13:29

k8s-ci-robot assigned muraee Feb 12, 2024

xiangjingli mentioned this pull request Feb 16, 2024

✨Add missing Fields to RosaControlPlane - tags, etcdEncryption #4795

Closed

5 tasks

robpblake reviewed Feb 20, 2024

View reviewed changes

serngawy force-pushed the fields branch from 1439581 to 1041f02 Compare February 20, 2024 16:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2024

xiangjingli mentioned this pull request Feb 20, 2024

✨Add missing Fields to RosaControlPlane - tags, etcdEncryption #4802

Closed

5 tasks

serngawy force-pushed the fields branch from 1041f02 to e692d6a Compare February 21, 2024 13:56

serngawy force-pushed the fields branch 2 times, most recently from e864d26 to 950477a Compare February 22, 2024 18:32

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024

stevekuznetsov reviewed Feb 28, 2024

View reviewed changes

serngawy force-pushed the fields branch 3 times, most recently from a1d179b to ab57062 Compare February 28, 2024 21:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024

Add missing Fields to RosaControlPlan

3b6ea0f

Signed-off-by: melserngawy <melserng@redhat.com>

serngawy force-pushed the fields branch from ab57062 to 3b6ea0f Compare February 28, 2024 22:14

stevekuznetsov approved these changes Feb 28, 2024

View reviewed changes

k8s-ci-robot assigned stevekuznetsov Feb 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024

k8s-ci-robot merged commit 4e748e2 into kubernetes-sigs:main Feb 28, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add missing Fields to RosaControlPlane #4780

🌱 Add missing Fields to RosaControlPlane #4780

serngawy commented Feb 6, 2024

k8s-ci-robot commented Feb 6, 2024

serngawy commented Feb 6, 2024

serngawy commented Feb 6, 2024

k8s-ci-robot commented Feb 6, 2024

vincepri commented Feb 12, 2024

vkareh commented Feb 16, 2024

k8s-ci-robot commented Feb 16, 2024

robpblake Feb 20, 2024

serngawy Feb 20, 2024

robpblake Feb 20, 2024

serngawy Feb 20, 2024

serngawy commented Feb 20, 2024

muraee commented Feb 21, 2024

stevekuznetsov commented Feb 21, 2024

serngawy commented Feb 21, 2024

k8s-ci-robot commented Feb 28, 2024

stevekuznetsov Feb 28, 2024

stevekuznetsov Feb 28, 2024

serngawy Feb 28, 2024

stevekuznetsov Feb 28, 2024

stevekuznetsov Feb 28, 2024

stevekuznetsov Feb 28, 2024

stevekuznetsov left a comment

k8s-ci-robot commented Feb 28, 2024

	// +kubebuilder:default:=OVNKubernetes
	// +kubebuilder:default=OVNKubernetes

🌱 Add missing Fields to RosaControlPlane #4780

🌱 Add missing Fields to RosaControlPlane #4780

Conversation

serngawy commented Feb 6, 2024

k8s-ci-robot commented Feb 6, 2024

serngawy commented Feb 6, 2024

serngawy commented Feb 6, 2024

k8s-ci-robot commented Feb 6, 2024

vincepri commented Feb 12, 2024

vkareh commented Feb 16, 2024

k8s-ci-robot commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serngawy commented Feb 20, 2024

muraee commented Feb 21, 2024

stevekuznetsov commented Feb 21, 2024

serngawy commented Feb 21, 2024

k8s-ci-robot commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 28, 2024