MachineDeployment refs and ownerRefs are not upgraded after CAPI upgrade with apiVersion bump

[sbueringer]

What steps did you take and what happened:
I ran the clusterctl upgrade e2e test locally (v1alpha4=>v1beta1) and the refs in MD were not upgraded from v1alpha4 to v1beta1.

Some details:

Refs are initially in-memory bumped to v1beta1 through:

cluster-api/internal/controllers/machinedeployment/machinedeployment_controller.go

Lines 215 to 217 in d7bf8df

	if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, cluster, d.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
	return ctrl.Result{}, err
	}

At some later time we overwrite the in-memory modified version of MD by getting the current one from the server:

cluster-api/internal/controllers/machinedeployment/machinedeployment_sync.go

Lines 133 to 136 in d7bf8df

	err = r.updateMachineDeployment(ctx, d, func(innerDeployment *clusterv1.MachineDeployment) {
	mdutil.SetDeploymentRevision(d, msCopy.Annotations[clusterv1.RevisionAnnotation])
	})
	return msCopy, err

This drops all the changes we made in the meantime and thus the refs stay on v1alpha4 (or on v1alpha3 depends on with which version the MD was initially created)

Example in CI: https://storage.googleapis.com/kubernetes-jenkins/logs/periodic-cluster-api-e2e-main/1542205506544734208/artifacts/clusters/clusterctl-upgrade-f4s62o/resources/clusterctl-upgrade/MachineDeployment/clusterctl-upgrade-rlxhz4-md-0.yaml

We need to fix this in a minor CAPI release before we will be able to drop v1alpha3 from our CRDs in a subsequent release!

Additionally we have to verify that after a CAPI upgrade with apiVersion bumps all ownerReferences have been upgraded.

See ownerRef to v1alpha4 Cluster here: https://storage.googleapis.com/kubernetes-jenkins/logs/periodic-cluster-api-e2e-main/1542205506544734208/artifacts/clusters/clusterctl-upgrade-f4s62o/resources/clusterctl-upgrade/MachineDeployment/clusterctl-upgrade-rlxhz4-md-0.yaml

If we don't do this and just drop the version we run into all kind of fun deadlocks (at the latest when trying to delete a Cluster/MD/...)

P.S. Found this issue when playing around with clusterctl CRD migration and trying to drop v1alpha3 from our CRDs

Environment:

• Cluster-api version:
• minikube/kind version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

/kind bug
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

[sbueringer]

Duplicate of an issue I created last year (#5470) :(

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

Duplicate of an issue I created last year (#5470) :(

/close

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]

/reopen

Realized this issue is more comprehensive as it also mentions ownerRefs.
More details about the ref issue can be found in #5470

[k8s-ci-robot]

@sbueringer: Reopened this issue.

In response to this:

/reopen

Realized this issue is more comprehensive as it also mentions ownerRefs.
More details about the ref issue can be found in #5470

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]

@sbueringer: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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]

/assign @killianmuldoon
(as you were assigned to the other one)

[sbueringer]

Sorry for the back and forth.

I created this issue for ownerRefs: #7224

I've done some additional research on refs and will open a separate issue to address the apiVersion-bump-in-refs topic.

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

Sorry for the back and forth.

I created this issue for ownerRefs: #7224

I've done some additional research on refs and will open a separate issue to address the apiVersion-bump-in-refs topic.

/close

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]

/reopen

Let's use this issue to track the fix for the ref bump in MD and also the implementation for MachinePool

• MD: ⚠️ Remove PopulateDefaultsMachineDeployment and fix how MD controller set revision #7992
• MP: 🌱 MachinePool: automatically bump apiVersions like in our other controllers #7995

/assign

/milestone v1.4

[k8s-ci-robot]

@sbueringer: Reopened this issue.

In response to this:

/reopen

Let's use this issue to track the fix for MD and also the implementation for MachinePool

• MD: ⚠️ Remove PopulateDefaultsMachineDeployment and fix how MD controller set revision #7992
• MP: 🌱 MachinePool: automatically bump apiVersions like in our other controllers #7995

/assign

/milestone v1.4

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.