🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7591

sbueringer · 2022-11-22T13:18:12Z

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

Kudos to @fabriziopandini for the initial 80% of the PR :)

The expected behavior after this PR is roughly:

KCP: Machines will now have:
- cluster.x-k8s.io/cluster-name: cluster.Name
- cluster.x-k8s.io/control-plane: ""
- cluster.x-k8s.io/control-plane-name: kcp.Name
  - also add to pre-existing
- Should not be adopted to Cluster if Machines have CP or MS label
MachineSet:
- cluster.x-k8s.io/cluster-name: cluster.Name
- cluster.x-k8s.io/deployment-name: md.Name
  - also add to pre-existing
- Should not be adopted to Cluster if MachineSet has MD label
Machine:
- cluster.x-k8s.io/cluster-name: cluster.Name
- cluster.x-k8s.io/deployment-name: md.Name
  - also add to pre-existing
- cluster.x-k8s.io/set-name: ms.Name
  - also add to pre-existing
- Should not be adopted to Cluster if Machines have CP or MS label

(all also manually verified)

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

api/v1beta1/machine_types.go

sbueringer · 2022-11-22T13:25:53Z

controlplane/kubeadm/internal/controllers/controller.go

+	// which were created before the `cluster.x-k8s.io/control-plane-name` label was introduced.
+	// NOTE: Changes will be applied to the Machines in reconcileControlPlaneConditions.
+	// NOTE: cluster.x-k8s.io/control-plane is already set at this stage (it is used when reading controlPlane.Machines).
+	// TODO(sbueringer): Drop the following code with v1.4 after all existing Machines are guaranteed to have the new label.


Will open a follow-up issue to track this for the v1.4 milestone

Note: Turns out we can't just drop it with v1.4 as we have no guarantees that everyone is upgrading v1.2 => v1.3 => v1.4

I would like to get rid of the logic at some point though. Just to keep the complexity of our already complex code as low as possible. Maybe we can drop it 1-2 years for now? (with some disclaimer in the corresponding .0 release notes)

I think this isn't only for adding the label on upgrade but also for ensuring the label gets re-applied if someone removes it or changes its value.
So I will drop the todo

I'm fine with keeping this, although the complexity that we have in the MD controller is really concerning already.

I wonder if the approach "what if a user deletes this label / ownerRef / field / ..." is something that we can sustain. There is just data that we have to be able to rely on otherwise all bets are off, e.g. what if a user:

removes the cluster label or

while we are now safe against users removing the cluster.x-k8s.io/control-plane-name label I think we have no chance if users remove the cluster.x-k8s.io/control-plane label.

To be clear, I'm fine with keeping this logic, just wondering how sustainable it is in addition to the already concerning complexity we have having to be able to deal with all kind of edge cases where users manually remove/sabotage our data.

(dropped the TODOs)

sbueringer · 2022-11-22T13:37:15Z

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

internal/controllers/machinedeployment/machinedeployment_controller.go

sbueringer · 2022-11-23T09:30:27Z

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

sbueringer · 2022-11-23T09:54:48Z

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

internal/controllers/machinedeployment/machinedeployment_controller.go

internal/controllers/machineset/machineset_controller.go

sbueringer · 2022-11-23T10:44:23Z

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

sbueringer · 2022-11-24T05:00:12Z

/cherry-pick release-1.2

k8s-infra-cherrypick-robot · 2022-11-24T05:00:14Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2

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.

sbueringer · 2022-11-24T05:00:32Z

/cherry-pick release-1.3

k8s-infra-cherrypick-robot · 2022-11-24T05:00:33Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.3

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.

sbueringer · 2022-11-24T05:00:36Z

/retest

sbueringer · 2022-11-24T05:02:46Z

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

sbueringer · 2022-11-24T05:51:57Z

Did the following additional validation:

compared YAMLs of KCP / MD / MS / Machines of quickstart (classy & non-classy) and clusterctl upgrade tests on main vs. this PR
The labels on MachineSets and Machines were all there as expected
There were no side-effects on ownerRefs or spec.template labels/selectors on any of the objects
The clusterctl upgrade test showed that the labels are also added to existing objects and that it is done without additional rollouts

Co-authored-by: fabriziopandini <fpandini@vmware.com> Signed-off-by: Stefan Büringer buringerst@vmware.com

sbueringer · 2022-11-24T05:53:06Z

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

sbueringer · 2022-11-24T05:53:29Z

@fabriziopandini This should now be ready for merge

I think merging this PR would also unblock one of Killians PRs.

killianmuldoon · 2022-11-24T09:50:16Z

/hold

There's a bug in #7606 which might be related to this PR, so I'd like to get to the bottom of that first.

killianmuldoon · 2022-11-24T14:49:31Z

/remove-hold

killianmuldoon

/lgtm

Thanks for tackling this!

fabriziopandini

/lgtm
/approve

k8s-ci-robot · 2022-11-28T10:05:25Z

[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:

~~OWNERS~~ [fabriziopandini]

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

k8s-infra-cherrypick-robot · 2022-11-28T10:18:45Z

@sbueringer: #7591 failed to apply on top of branch "release-1.2":

Applying: Improve Machine adoption
Using index info to reconstruct a base tree...
M	api/v1beta1/machine_types.go
M	controlplane/kubeadm/internal/controllers/controller.go
M	controlplane/kubeadm/internal/controllers/helpers.go
M	controlplane/kubeadm/internal/controllers/helpers_test.go
M	internal/controllers/machine/machine_controller.go
M	internal/controllers/machinedeployment/machinedeployment_controller.go
M	internal/controllers/machinedeployment/machinedeployment_controller_test.go
M	internal/controllers/machinedeployment/machinedeployment_sync.go
M	internal/controllers/machinedeployment/mdutil/util.go
M	internal/controllers/machineset/machineset_controller.go
Falling back to patching base and 3-way merge...
Auto-merging internal/controllers/machineset/machineset_controller.go
Auto-merging internal/controllers/machinedeployment/mdutil/util.go
CONFLICT (content): Merge conflict in internal/controllers/machinedeployment/mdutil/util.go
Auto-merging internal/controllers/machinedeployment/machinedeployment_sync.go
Auto-merging internal/controllers/machinedeployment/machinedeployment_controller_test.go
Auto-merging internal/controllers/machinedeployment/machinedeployment_controller.go
Auto-merging internal/controllers/machine/machine_controller.go
Auto-merging controlplane/kubeadm/internal/controllers/helpers_test.go
Auto-merging controlplane/kubeadm/internal/controllers/helpers.go
Auto-merging controlplane/kubeadm/internal/controllers/controller.go
Auto-merging api/v1beta1/machine_types.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Improve Machine adoption
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

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.

k8s-infra-cherrypick-robot · 2022-11-28T10:19:21Z

@sbueringer: new pull request created: #7637

In response to this:

/cherry-pick release-1.3

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2022

k8s-ci-robot requested review from CecileRobertMichon and JoelSpeed November 22, 2022 13:18

sbueringer commented Nov 22, 2022

View reviewed changes

api/v1beta1/machine_types.go Show resolved Hide resolved

killianmuldoon mentioned this pull request Nov 22, 2022

Ensure owner references are correctly re-reconciled #7575

Closed

sbueringer commented Nov 22, 2022

View reviewed changes

sbueringer changed the title ~~Improve Machine adoption~~ 🐛 Fix Machine adoption for KCP/MachineSet-owned Machines Nov 22, 2022

sbueringer commented Nov 23, 2022

View reviewed changes

internal/controllers/machinedeployment/machinedeployment_controller.go Outdated Show resolved Hide resolved

sbueringer mentioned this pull request Nov 23, 2022

🌱 MD reconciler: improve integration test #7596

Merged

sbueringer force-pushed the pr-adoption branch 3 times, most recently from ba962a0 to 7a5b3e8 Compare November 23, 2022 09:47

fabriziopandini reviewed Nov 23, 2022

View reviewed changes

internal/controllers/machinedeployment/machinedeployment_controller.go Outdated Show resolved Hide resolved

internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-adoption branch 2 times, most recently from 5e513d5 to 10f016c Compare November 23, 2022 10:44

killianmuldoon mentioned this pull request Nov 23, 2022

🌱 Ensure infra and bootstrap objects are owned by Machines #7593

Merged

sbueringer force-pushed the pr-adoption branch from 10f016c to 6be714a Compare November 24, 2022 05:01

Improve Machine adoption

4f536d8

Co-authored-by: fabriziopandini <fpandini@vmware.com> Signed-off-by: Stefan Büringer buringerst@vmware.com

sbueringer force-pushed the pr-adoption branch from 6be714a to 4f536d8 Compare November 24, 2022 05:52

sbueringer mentioned this pull request Nov 24, 2022

Use suffixes consistently in API constants #7611

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2022

killianmuldoon approved these changes Nov 24, 2022

View reviewed changes

k8s-ci-robot assigned killianmuldoon Nov 24, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2022

fabriziopandini reviewed Nov 28, 2022

View reviewed changes

k8s-ci-robot assigned fabriziopandini Nov 28, 2022

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

k8s-ci-robot merged commit 0a17fa2 into kubernetes-sigs:main Nov 28, 2022

k8s-ci-robot added this to the v1.4 milestone Nov 28, 2022

k8s-infra-cherrypick-robot mentioned this pull request Nov 28, 2022

[release-1.3] 🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7637

Merged

This was referenced Nov 28, 2022

[release-1.2] 🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7638

Closed

[release-1.2] 🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7640

Merged

sbueringer deleted the pr-adoption branch November 28, 2022 11:33

killianmuldoon mentioned this pull request Dec 8, 2022

MachineSet and ControlPlanes with longer names no longer reconcile #7710

Open

raafatseif mentioned this pull request Nov 16, 2023

RKE2 control plane machines should be adopted after restore rancher/cluster-api-provider-rke2#200

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7591

🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7591

sbueringer commented Nov 22, 2022 •

edited

Loading

sbueringer Nov 22, 2022

sbueringer Nov 23, 2022

fabriziopandini Nov 23, 2022

sbueringer Nov 23, 2022 •

edited

Loading

sbueringer commented Nov 22, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 24, 2022

k8s-infra-cherrypick-robot commented Nov 24, 2022

sbueringer commented Nov 24, 2022

k8s-infra-cherrypick-robot commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

killianmuldoon commented Nov 24, 2022

killianmuldoon commented Nov 24, 2022

killianmuldoon left a comment

fabriziopandini left a comment

k8s-ci-robot commented Nov 28, 2022

k8s-infra-cherrypick-robot commented Nov 28, 2022

k8s-infra-cherrypick-robot commented Nov 28, 2022

🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7591

🐛 Fix Machine adoption for KCP/MachineSet-owned Machines #7591

Conversation

sbueringer commented Nov 22, 2022 • edited Loading

sbueringer Nov 22, 2022

Choose a reason for hiding this comment

sbueringer Nov 23, 2022

Choose a reason for hiding this comment

fabriziopandini Nov 23, 2022

Choose a reason for hiding this comment

sbueringer Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer commented Nov 22, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 23, 2022

sbueringer commented Nov 24, 2022

k8s-infra-cherrypick-robot commented Nov 24, 2022

sbueringer commented Nov 24, 2022

k8s-infra-cherrypick-robot commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

sbueringer commented Nov 24, 2022

killianmuldoon commented Nov 24, 2022

killianmuldoon commented Nov 24, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 28, 2022

k8s-infra-cherrypick-robot commented Nov 28, 2022

k8s-infra-cherrypick-robot commented Nov 28, 2022

sbueringer commented Nov 22, 2022 •

edited

Loading

sbueringer Nov 23, 2022 •

edited

Loading