Remove handling for orphaned VSphereMachines

Signed-off-by: killianmuldoon <[email protected]>
kubernetes-sigs · Oct 13, 2023 · c242fe5 · c242fe5
1 parent a1db12e
commit c242fe5
Show file tree

Hide file tree

Showing 4 changed files with 163 additions and 46 deletions.
diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go
@@ -121,10 +121,6 @@ func (r *clusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		}
 	}()
 
-	if err := r.setOwnerRefsOnVsphereMachines(ctx, clusterContext); err != nil {
-		return reconcile.Result{}, errors.Wrapf(err, "failed to set owner refs on VSphereMachine objects")
-	}
-
 	// Handle deleted clusters
 	if !vsphereCluster.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, clusterContext)
@@ -158,12 +154,6 @@ func (r *clusterReconciler) reconcileDelete(ctx context.Context, clusterCtx *cap
 		log := log.WithValues("VSphereMachine", klog.KObj(vsphereMachine))
 		ctx := ctrl.LoggerInto(ctx, log)
 
-		// If the VSphereMachine is not owned by the CAPI Machine object because the machine object was deleted
-		// before setting the owner references, then proceed with the deletion of the VSphereMachine object.
-		// This is required until CAPI has a solution for https://github.com/kubernetes-sigs/cluster-api/issues/5483
-		if !clusterutilv1.IsOwnedByObject(vsphereMachine, clusterCtx.VSphereCluster) || len(vsphereMachine.OwnerReferences) != 1 {
-			continue
-		}
 		machineDeletionCount++
 		// Remove the finalizer since VM creation wouldn't proceed
 		log.Info("Removing finalizer from VSphereMachine")
@@ -528,37 +518,6 @@ func (r *clusterReconciler) isControlPlaneInitialized(ctx context.Context, clust
 	return conditions.IsTrue(clusterCtx.Cluster, clusterv1.ControlPlaneInitializedCondition)
 }
 
-func (r *clusterReconciler) setOwnerRefsOnVsphereMachines(ctx context.Context, clusterCtx *capvcontext.ClusterContext) error {
-	vsphereMachines, err := infrautilv1.GetVSphereMachinesInCluster(ctx, r.Client, clusterCtx.Cluster.Namespace, clusterCtx.Cluster.Name)
-	if err != nil {
-		return errors.Wrapf(err,
-			"unable to list VSphereMachines part of VSphereCluster %s/%s", clusterCtx.VSphereCluster.Namespace, clusterCtx.VSphereCluster.Name)
-	}
-
-	var patchErrors []error
-	for _, vsphereMachine := range vsphereMachines {
-		patchHelper, err := patch.NewHelper(vsphereMachine, r.Client)
-		if err != nil {
-			patchErrors = append(patchErrors, err)
-			continue
-		}
-
-		vsphereMachine.SetOwnerReferences(clusterutilv1.EnsureOwnerRef(
-			vsphereMachine.OwnerReferences,
-			metav1.OwnerReference{
-				APIVersion: clusterCtx.VSphereCluster.APIVersion,
-				Kind:       clusterCtx.VSphereCluster.Kind,
-				Name:       clusterCtx.VSphereCluster.Name,
-				UID:        clusterCtx.VSphereCluster.UID,
-			}))
-
-		if err := patchHelper.Patch(ctx, vsphereMachine); err != nil {
-			patchErrors = append(patchErrors, err)
-		}
-	}
-	return kerrors.NewAggregate(patchErrors)
-}
-
 func (r *clusterReconciler) reconcileClusterModules(ctx context.Context, clusterCtx *capvcontext.ClusterContext) (reconcile.Result, error) {
 	if feature.Gates.Enabled(feature.NodeAntiAffinity) {
 		return r.clusterModuleReconciler.Reconcile(ctx, clusterCtx)

diff --git a/controllers/vspheremachine_controller.go b/controllers/vspheremachine_controller.go
@@ -242,6 +242,21 @@ func (r *machineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		return reconcile.Result{}, nil
 	}
 
+	// If the VSphereCluster has been previously set as an ownerReference remove it. This may have been set in an older
+	// version of CAPV to prevent VSphereMachines from being orphaned, but is no longer needed.
+	// For more info see: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/issues/2054
+	// TODO (killianmuldoon): Decide when this can be removed.
+	machineContext.GetVSphereMachine().SetOwnerReferences(
+		clusterutilv1.RemoveOwnerRef(
+			machineContext.GetVSphereMachine().GetOwnerReferences(),
+			metav1.OwnerReference{
+				Name:       cluster.Spec.InfrastructureRef.Name,
+				APIVersion: cluster.Spec.InfrastructureRef.APIVersion,
+				Kind:       cluster.Spec.InfrastructureRef.Kind,
+			},
+		),
+	)
+
 	// Fetch the VSphereCluster and update the machine context
 	machineContext, err = r.VMService.FetchVSphereCluster(ctx, cluster, machineContext)
 	if err != nil {
@@ -255,6 +270,7 @@ func (r *machineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		ctrlutil.AddFinalizer(machineContext.GetVSphereMachine(), infrav1.MachineFinalizer)
 		return reconcile.Result{}, nil
 	}
+
 	// Handle non-deleted machines
 	return r.reconcileNormal(ctx, machineContext)
 }

diff --git a/controllers/vspheremachine_controller_test.go b/controllers/vspheremachine_controller_test.go
@@ -17,15 +17,19 @@ limitations under the License.
 package controllers
 
 import (
+	"testing"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	capiutil "sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
 )
@@ -64,7 +68,7 @@ var _ = Describe("VsphereMachineReconciler", func() {
 			},
 			Spec: clusterv1.ClusterSpec{
 				InfrastructureRef: &corev1.ObjectReference{
-					APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha4",
+					APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 					Kind:       "VSphereCluster",
 					Name:       "vsphere-test1",
 				},
@@ -78,7 +82,7 @@ var _ = Describe("VsphereMachineReconciler", func() {
 				Namespace: testNs.Name,
 				OwnerReferences: []metav1.OwnerReference{
 					{
-						APIVersion: "cluster.x-k8s.io/v1alpha4",
+						APIVersion: "cluster.x-k8s.io/v1beta1",
 						Kind:       "Cluster",
 						Name:       capiCluster.Name,
 						UID:        "blah",
@@ -101,7 +105,7 @@ var _ = Describe("VsphereMachineReconciler", func() {
 			Spec: clusterv1.MachineSpec{
 				ClusterName: capiCluster.Name,
 				InfrastructureRef: corev1.ObjectReference{
-					APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha4",
+					APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 					Kind:       "VSphereMachine",
 					Name:       "vsphere-machine-1",
 				},
@@ -205,3 +209,142 @@ var _ = Describe("VsphereMachineReconciler", func() {
 		})
 	})
 })
+
+func Test_machineReconciler_Metadata(t *testing.T) {
+	g := NewWithT(t)
+	ns, err := testEnv.CreateNamespace(ctx, "vsphere-machine-reconciler")
+	g.Expect(err).NotTo(HaveOccurred())
+
+	defer func() {
+		if err := testEnv.Delete(ctx, ns); err != nil {
+			g.Expect(err).NotTo(HaveOccurred())
+		}
+	}()
+	capiCluster := &clusterv1.Cluster{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Cluster",
+			APIVersion: clusterv1.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "cluster1",
+			Namespace: ns.Name,
+		},
+		Spec: clusterv1.ClusterSpec{
+			InfrastructureRef: &corev1.ObjectReference{
+				APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+				Kind:       "VSphereCluster",
+				Name:       "vsphere-test1",
+			},
+		},
+	}
+
+	vSphereCluster := &infrav1.VSphereCluster{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "VSphereCluster",
+			APIVersion: infrav1.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "vsphere-test1",
+			Namespace: ns.Name,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: "cluster.x-k8s.io/v1beta1",
+					Kind:       "Cluster",
+					Name:       capiCluster.Name,
+					UID:        "blah",
+				},
+			},
+		},
+		Spec: infrav1.VSphereClusterSpec{},
+	}
+
+	capiMachine := &clusterv1.Machine{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Machine",
+			APIVersion: clusterv1.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "machine1",
+			Namespace:  ns.Name,
+			Finalizers: []string{clusterv1.MachineFinalizer},
+			Labels: map[string]string{
+				clusterv1.ClusterNameLabel: capiCluster.Name,
+			},
+		},
+		Spec: clusterv1.MachineSpec{
+			ClusterName: capiCluster.Name,
+			InfrastructureRef: corev1.ObjectReference{
+				APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+				Kind:       "VSphereMachine",
+				Name:       "vsphere-machine-1",
+			},
+		},
+	}
+	vSphereMachine := &infrav1.VSphereMachine{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "VSphereMachine",
+			APIVersion: infrav1.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "vsphere-machine-1",
+			Namespace: ns.Name,
+			Labels: map[string]string{
+				clusterv1.ClusterNameLabel:         capiCluster.Name,
+				clusterv1.MachineControlPlaneLabel: "",
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: clusterv1.GroupVersion.String(),
+					Kind:       "Machine",
+					Name:       capiMachine.Name,
+					UID:        "blah",
+				},
+				// This ownerReference should be removed by the reconciler as it's no longer needed.
+				// These ownerReferences were previously added by CAPV to prevent machines becoming orphaned.
+				{
+					APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+					Kind:       "VSphereCluster",
+					Name:       vSphereCluster.Name,
+					UID:        "blah",
+				},
+			},
+		},
+		Spec: infrav1.VSphereMachineSpec{
+			VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{
+				Template: "ubuntu-k9s-1.19",
+				Network: infrav1.NetworkSpec{
+					Devices: []infrav1.NetworkDeviceSpec{
+						{NetworkName: "network-1", DHCP4: true},
+					},
+				},
+			},
+		},
+	}
+
+	t.Run("Should set finalizer and remove unnecessary ownerReference", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Create the Machine, Cluster, VSphereCluster object and expect the Reconcile and Deployment to be created
+		g.Expect(testEnv.Create(ctx, vSphereCluster)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, capiCluster)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, capiMachine)).To(Succeed())
+
+		g.Expect(testEnv.Create(ctx, vSphereMachine)).To(Succeed())
+
+		key := client.ObjectKey{Namespace: vSphereMachine.Namespace, Name: vSphereMachine.Name}
+		defer func() {
+			err := testEnv.Delete(ctx, vSphereMachine)
+			g.Expect(err).ToNot(HaveOccurred())
+		}()
+
+		// Make sure the VSphereMachine has a finalizer and the correct ownerReferences.
+		g.Eventually(func() bool {
+			if err := testEnv.Get(ctx, key, vSphereMachine); err != nil {
+				return false
+			}
+			return ctrlutil.ContainsFinalizer(vSphereMachine, infrav1.MachineFinalizer) &&
+				capiutil.HasOwner(vSphereMachine.GetOwnerReferences(), clusterv1.GroupVersion.String(), []string{"Machine"}) &&
+				!capiutil.HasOwner(vSphereMachine.GetOwnerReferences(), infrav1.GroupVersion.String(), []string{"VSphereCluster"})
+		}, timeout).Should(BeTrue())
+	})
+}
diff --git a/test/e2e/ownerreference_test.go b/test/e2e/ownerreference_test.go
@@ -122,8 +122,7 @@ var (
 			return framework.HasExactOwners(owners, clusterClassOwner)
 		},
 		"VSphereMachine": func(owners []metav1.OwnerReference) error {
-			// The vSphereCluster takes ownership of all vSphereMachines in addition to the core Machine.
-			return framework.HasExactOwners(owners, vSphereClusterOwner, machineController)
+			return framework.HasExactOwners(owners, machineController)
 		},
 		"VSphereMachineTemplate": func(owners []metav1.OwnerReference) error {
 			// The vSphereMachineTemplate can be owned by the Cluster or the ClusterClass.