Skip to content

✨ ROSA: Reconcile ROSAMachinePool fields#4804

Merged
k8s-ci-robot merged 5 commits intokubernetes-sigs:mainfrom
muraee:reconcile-rosa-machinepool
Mar 1, 2024
Merged

✨ ROSA: Reconcile ROSAMachinePool fields#4804
k8s-ci-robot merged 5 commits intokubernetes-sigs:mainfrom
muraee:reconcile-rosa-machinepool

Conversation

@muraee
Copy link
Copy Markdown
Contributor

@muraee muraee commented Feb 21, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:
reconcile changes to the ROSAMachinePool fields

Add 2 new fields

  • Taints
  • TuningConfigs

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 #

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • includes emojis
  • adds unit tests
  • adds or updates e2e tests

Release note:

reconcile ROSAMachinePool fields

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 21, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2024
@muraee muraee changed the title ROSA: reconcile ROSAMachinePool fields ✨ ROSA: reconcile ROSAMachinePool fields Feb 21, 2024
@muraee muraee force-pushed the reconcile-rosa-machinepool branch 2 times, most recently from 713e7eb to aa768e4 Compare February 22, 2024 09:35
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024
@muraee muraee force-pushed the reconcile-rosa-machinepool branch from aa768e4 to 4930814 Compare February 28, 2024 13:49
@muraee muraee changed the title ✨ ROSA: reconcile ROSAMachinePool fields ✨ ROSA: Reconcile ROSAMachinePool fields Feb 28, 2024
stevekuznetsov
stevekuznetsov reviewed Feb 28, 2024
View reviewed changes
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024
@muraee muraee force-pushed the reconcile-rosa-machinepool branch 2 times, most recently from da45677 to ddfd1ce Compare February 29, 2024 12:37
serngawy
serngawy reviewed Feb 29, 2024
View reviewed changes
@muraee muraee mentioned this pull request Feb 29, 2024
Merged
5 tasks
@muraee
reconcile rosamachinepool fields
006a40e 
- add Taints field
- add TuningConfigs field
@muraee
add /finalizers rbac
9f69479 
This is required for when the managment cluster has OwnerReferencesPermissionEnforcement admission controller enabled.
@muraee muraee force-pushed the reconcile-rosa-machinepool branch from 8e9a0b5 to 1d3df4a Compare February 29, 2024 14:49
@muraee muraee force-pushed the reconcile-rosa-machinepool branch from 1d3df4a to a481de3 Compare February 29, 2024 14:49
stevekuznetsov
stevekuznetsov reviewed Feb 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Since you've already factored nodePoolBuilder() and nodePoolToRosaMachinePoolSpec() really well for unit testing, would you mind adding one for them?

@muraee muraee force-pushed the reconcile-rosa-machinepool branch from a16b1b1 to 610691b Compare February 29, 2024 16:35
@muraee
Copy link
Copy Markdown
Contributor Author

muraee commented Feb 29, 2024

@stevekuznetsov added, we can add more test cases later.

@stevekuznetsov
Copy link
Copy Markdown
Contributor

stevekuznetsov commented Feb 29, 2024

Sounds good, the round-trip unit test is what I had in mind.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 29, 2024
vincepri
vincepri reviewed Mar 1, 2024
View reviewed changes
exp/api/v1beta2/rosamachinepool_types.go Outdated
Comment on lines +90 to +93
// +kubebuilder:validation:Required
//
// The taint key to be applied to a node.
Key string `json:"key"`
Copy link
Copy Markdown
Member

@vincepri vincepri Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually have the +kubebuilder tags after the comment, like:

Suggested change
// +kubebuilder:validation:Required
//
// The taint key to be applied to a node.
Key string `json:"key"`
// The taint key to be applied to a node.
//
// +kubebuilder:validation:Required
Key string `json:"key"`

vincepri
vincepri reviewed Mar 1, 2024
View reviewed changes
controlplane/rosa/controllers/rosacontrolplane_controller.go
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinepools,verbs=get;list;watch
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes,verbs=get;list;watch;update;patch;delete
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes/finalizers,verbs=update
Copy link
Copy Markdown
Member

@vincepri vincepri Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Copy Markdown
Contributor Author

@muraee muraee Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is needed when OwnerReferencesPermissionEnforcement plugin is enabled

https://sdk.operatorframework.io/docs/faqs/#after-deploying-my-operator-why-do-i-see-errors-like-is-forbidden-cannot-set-blockownerdeletion-if-an-ownerreference-refers-to-a-resource-you-cant-set-finalizers-on-

vincepri
vincepri reviewed Mar 1, 2024
View reviewed changes
controlplane/rosa/controllers/rosacontrolplane_controller.go Outdated
Comment on lines +303 to +305
// TODO: expose in status
rosaScope.Error(err, "rosacontrolplane.spec.network.machineCIDR invalid", networkSpec.MachineCIDR)
return ctrl.Result{}, nil
Copy link
Copy Markdown
Member

@vincepri vincepri Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this to a validation webhook, and just return the error here (backoff loop) or set a condition?

Copy link
Copy Markdown
Contributor

@stevekuznetsov stevekuznetsov Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an error when the data provided by the user is incorrect would only lead to infinite retry by the controller and not result in a good outcome. It seems universally correct to expose states like this in status so that the user knows that they need to update the object's .spec and so that the controller does not do extra reconciliation loops that cannot possibly result in a better outcome.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 1, 2024
@muraee
add validation webhook
3defa28 
- cleanup *types.go
- report erros in conditions
@muraee muraee force-pushed the reconcile-rosa-machinepool branch from 99b3515 to 3defa28 Compare March 1, 2024 17:52
vincepri
vincepri approved these changes Mar 1, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2024
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov, vincepri

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit f0a5ecf into kubernetes-sigs:main Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ankitasw Ankitasw Awaiting requested review from Ankitasw

3 more reviewers

@serngawy serngawy serngawy left review comments

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@vincepri vincepri vincepri approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vincepri vincepri

@stevekuznetsov stevekuznetsov

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@muraee @stevekuznetsov @k8s-ci-robot @serngawy @vincepri