Skip to content

✨ KCP: Extend rollout logic for in-place updates #12840

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
Merged
merged 2 commits into from
Oct 14, 2025
Merged

✨ KCP: Extend rollout logic for in-place updates #12840

merged 2 commits into from
Oct 14, 2025

Conversation

@sbueringer
Copy link
Member

@sbueringer sbueringer commented Oct 9, 2025 •
edited
Loading

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Oct 9, 2025
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 9, 2025
@sbueringer sbueringer added the area/provider/control-plane-kubeadm Issues or PRs related to KCP label Oct 9, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Oct 9, 2025
@sbueringer sbueringer mentioned this pull request Oct 9, 2025
Open
66 tasks
@sbueringer
Copy link
Member Author

No worries, this still contains #12817. This PR is much smaller :) (but still needs some polishing)

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main

@sbueringer sbueringer force-pushed the pr-kcp-rollout-logic branch from 09f230e to 0f0f15e Compare October 10, 2025 07:00
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 10, 2025
@sbueringer sbueringer force-pushed the pr-kcp-rollout-logic branch from 0f0f15e to 6cee4f8 Compare October 10, 2025 07:03
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main

@sbueringer sbueringer force-pushed the pr-kcp-rollout-logic branch from 6cee4f8 to 41b6683 Compare October 10, 2025 07:36
@sbueringer sbueringer changed the title [WIP] ✨ KCP: Extend rollout logic for in-place updates ✨ KCP: Extend rollout logic for in-place updates Oct 10, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2025
@sbueringer
Copy link
Member Author

Ready for review

/assign @fabriziopandini

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main

@k8s-ci-robot 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 Oct 10, 2025
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main

alexander-demicev
alexander-demicev reviewed Oct 10, 2025
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

thanks a lot, this is a solid start for introducing in-place update logic!

fabriziopandini
fabriziopandini reviewed Oct 13, 2025
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.

Nice work, only one question from my side

controlplane/kubeadm/internal/filters.go
Comment on lines +68 to +72
// If a Machine is marked for deletion it is not eligible for in-place update.
if _, ok := machine.Annotations[clusterv1.DeleteMachineAnnotation]; ok {
res.EligibleForInPlaceUpdate = false
}

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should expand use cases where a machine is not eligible for in-place:

  • what if the machines has the manual remediation label? (it is another way to mark for deletion)
  • what about if unhealthy in the MHC sense? (most likely it will be remediated before upgrading in place)
  • what about if the machine has some control plane component unhealthy (TBD)
  • what if the machine is already updating in place (as a safeguard to to trigger update in place twice)

Copy link
Member Author

@sbueringer sbueringer Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this func should be responsible for all of this. I think it would mean pulling in code from various parts of the code base (remediation, preflight checks, condition calculation, ...) into the UpToDate func

I added a note in the umbrella issue so we can come back to this comment in 2-3 PRs? (once we have a better understanding how the overall logic for in-place looks like).

Copy link
Member

Choose a reason for hiding this comment

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

ACK, let's get back to this later

@fabriziopandini
Copy link
Member

/lgtm
/approve

/hold feel free to unhold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 13, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 45e8b2a732bbe46ba1f5361de00134d60b846372

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Oct 13, 2025
@sbueringer
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2025
@k8s-ci-robot k8s-ci-robot merged commit 7847c39 into kubernetes-sigs:main Oct 14, 2025
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Oct 14, 2025
@sbueringer sbueringer deleted the pr-kcp-rollout-logic branch October 14, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@richardcase richardcase Awaiting requested review from richardcase

@sivchari sivchari Awaiting requested review from sivchari

+1 more reviewer

@alexander-demicev alexander-demicev alexander-demicev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@fabriziopandini fabriziopandini

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

v1.12

Development

Successfully merging this pull request may close these issues.

4 participants

@sbueringer @fabriziopandini @k8s-ci-robot @alexander-demicev