Skip to content

✨ MD: Implement CanUpdateMachineSet #12965

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

✨ MD: Implement CanUpdateMachineSet #12965

wants to merge 2 commits into from
+1,554 −212

Conversation

@sbueringer
Copy link
Member

@sbueringer sbueringer commented Nov 7, 2025

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 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 7, 2025
@sbueringer sbueringer changed the title ✨ MD: Implement CanUpdateMachineSet [WIP] ✨ MD: Implement CanUpdateMachineSet Nov 7, 2025
@k8s-ci-robot k8s-ci-robot requested a review from elmiko November 7, 2025 08:32
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 7, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 requested a review from g-gaston November 7, 2025 08:32
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2025
@sbueringer sbueringer mentioned this pull request Nov 7, 2025
Open
74 tasks
@sbueringer sbueringer added the area/machinedeployment Issues or PRs related to machinedeployments label Nov 7, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Nov 7, 2025
@sbueringer sbueringer force-pushed the pr-can-update-machineset branch 2 times, most recently from 5896543 to 7c325f8 Compare November 7, 2025 08:44
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 7, 2025
@sbueringer sbueringer force-pushed the pr-can-update-machineset branch 5 times, most recently from 4178ca8 to 6813157 Compare November 7, 2025 09:37
@sbueringer
Copy link
Member Author

sbueringer commented Nov 7, 2025

/test pull-cluster-api-e2e-main-gke

@sbueringer sbueringer mentioned this pull request Nov 7, 2025
Closed
@sbueringer sbueringer force-pushed the pr-can-update-machineset branch from 6813157 to 3c99995 Compare November 7, 2025 12:01
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 7, 2025
@sbueringer sbueringer force-pushed the pr-can-update-machineset branch from 3c99995 to d02f2b2 Compare November 7, 2025 12:20
@sbueringer
Copy link
Member Author

sbueringer commented Nov 7, 2025

/test pull-cluster-api-e2e-main-gke

@sbueringer sbueringer changed the title [WIP] ✨ MD: Implement CanUpdateMachineSet ✨ MD: Implement CanUpdateMachineSet Nov 7, 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 Nov 7, 2025
@sbueringer
Copy link
Member Author

sbueringer commented Nov 7, 2025

/test pull-cluster-api-e2e-main-gke

sbueringer
sbueringer commented Nov 7, 2025
View reviewed changes
internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go
Comment on lines +277 to +284
// Cleanup fields that are not the responsibility of the in-place update extension.
// Remove in-place mutable fields.
cleanedUpPatchedMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpPatchedMachineSet.Spec.Template)
cleanedUpDesiredMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpDesiredMachineSet.Spec.Template)
// Set refs equal.
cleanedUpPatchedMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef
cleanedUpPatchedMachineSet.Spec.Template.Spec.InfrastructureRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.InfrastructureRef
return compare.Diff(cleanedUpPatchedMachineSet, cleanedUpDesiredMachineSet)
Copy link
Member Author

@sbueringer sbueringer Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@fabriziopandini Let's talk about this on Monday

I think we should filter out fields here that are not relevant for the diff and we should share a util across here, KCP and the regular MD rollout decision as far as possible.

An alternative would be to explicitly only compare Version / FailureDomain but I think that has the risk that we forget to add new fields here in the future. In that case I would prefer to get a full rollout instead of an in-place rollout that doesn't work

Note to myself:

  • Also update KCP + add test coverage for this logic in both places

@sbueringer
Copy link
Member Author

sbueringer commented Nov 7, 2025

/test pull-cluster-api-e2e-main-gke

@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elmiko elmiko Awaiting requested review from elmiko

@g-gaston g-gaston Awaiting requested review from g-gaston

Assignees

No one assigned

Labels

area/machinedeployment Issues or PRs related to machinedeployments cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sbueringer @k8s-ci-robot