Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 Prioritize Machine with remediate-machine anotation when selecting the next machine to be remediated #11495

Merged
merged 4 commits into from
Jan 23, 2025
Merged

🌱 Prioritize Machine with remediate-machine anotation when selecting the next machine to be remediated #11495

merged 4 commits into from
Jan 23, 2025
+163 −4

Conversation

Karthik-K-N
Copy link
Contributor

What this PR does / why we need it:

This PR contains the changes to priorities the machine with cluster.x-k8s.io/remediate-machine annotation

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 #11385

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Nov 28, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 28, 2024
@Karthik-K-N Karthik-K-N mentioned this pull request Nov 28, 2024
Closed
@fabriziopandini fabriziopandini changed the title 🌱 Priorities machine with remidiation anotation for remidiation 🌱 Priorities machine with remediate-machine anotation when selecting the next machine to be remediated Dec 2, 2024
fabriziopandini
fabriziopandini reviewed Dec 2, 2024
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@Karthik-K-N thanks for taking up this work!
Let me know if you want some additional clarification

controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/controllers/remediation_test.go Outdated Show resolved Hide resolved
@Karthik-K-N
Copy link
Contributor Author

Thanks for the clarification, I will update it accordingly. Thanks

@Karthik-K-N
Copy link
Contributor Author

Updated with changes for KCP as suggested, Will do for remaining soon.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2024
@Karthik-K-N
Copy link
Contributor Author

Karthik-K-N commented Dec 17, 2024 •
edited
Loading

Made changes for machine set controller as well, Please take a look when you get some time. Thanks.

@enxebre
Copy link
Member

enxebre commented Dec 17, 2024 •
edited
Loading

can we update the annotation description as well?

current:

// RemediateMachineAnnotation is the annotation used to mark machines that should be remediated by MachineHealthCheck reconciler.

I think it should be:

// RemediateMachineAnnotation request the MachineHealthCheck reconciler to mark a Machine as unhealthy. CAPI builtin remediation will prioritize Machines with the annotation to be remediated.

@Karthik-K-N
Copy link
Contributor Author

RemediateMachineAnnotation

Updated. Thank you.

Danil-Grigorev
Danil-Grigorev reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c4a693a66713a3cdeda6cdfce2fef3b14a938fa4

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2025
@sbueringer
Copy link
Member

Sorry for the delay. @fabriziopandini should this be rebased after #11716 is merged?

@sbueringer
Copy link
Member

@Karthik-K-N Sorry for the delay. #11716 will be merged in a bit, can you please rebase this PR afterwards? Thank you!!

@Karthik-K-N
Copy link
Contributor Author

I have rebased this PR with main. Please take a look, Thank you.

@sbueringer
Copy link
Member

/assign @fabriziopandini
Maybe you can do a first round

@fabriziopandini
Copy link
Member

/lgtm

@sbueringer or @chrischdim. PTAL for the final pass

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 86a852680b25faa3f8c1d9a0489dc1a3bfa27944

chrischdi
chrischdi reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

one thing, otherwise good to go from my side :-)

internal/controllers/machineset/machineset_controller.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2025
@sbueringer sbueringer added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. area/provider/control-plane-kubeadm Issues or PRs related to KCP area/machineset Issues or PRs related to machinesets labels Jan 23, 2025
@k8s-ci-robot k8s-ci-robot removed do-not-merge/needs-area PR is missing an area label labels Jan 23, 2025
@sbueringer sbueringer changed the title 🌱 Priorities machine with remediate-machine anotation when selecting the next machine to be remediated 🌱 Prioritize Machine with remediate-machine anotation when selecting the next machine to be remediated Jan 23, 2025
sbueringer
sbueringer reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some nits

internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
internal/controllers/machineset/machineset_controller_test.go Show resolved Hide resolved
@sbueringer
Copy link
Member

Thank you!

/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 Jan 23, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 178d8167058d411f9493cc820d989160c1b6462f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

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 Jan 23, 2025
@k8s-ci-robot k8s-ci-robot merged commit a090145 into kubernetes-sigs:main Jan 23, 2025
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.10 milestone Jan 23, 2025
cprivitere pushed a commit to cprivitere/cluster-api that referenced this pull request Jan 23, 2025
@Karthik-K-N @cprivitere
🌱 Prioritize Machine with remediate-machine anotation when selecting …
93fd899 
…the next machine to be remediated (kubernetes-sigs#11495)

* Prioritise machine with remidiation anotation for remidiation

* Changes to prioritise remediation for machine set controller

* Update RemediateMachineAnnotation description

* Address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@chrischdi chrischdi chrischdi left review comments

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@Danil-Grigorev Danil-Grigorev Awaiting requested review from Danil-Grigorev

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

@Danil-Grigorev Danil-Grigorev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/machineset Issues or PRs related to machinesets area/provider/control-plane-kubeadm Issues or PRs related to KCP cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

Consider cluster.x-k8s.io/remediate-machine annotated when selecting the machine to be remediate
7 participants
@Karthik-K-N @enxebre @k8s-ci-robot @sbueringer @fabriziopandini @chrischdi @Danil-Grigorev