From a5ba8fe2738e85f99f294fd04cb49113b690b925 Mon Sep 17 00:00:00 2001 From: Sagar Muchhal Date: Wed, 27 Oct 2021 23:55:22 -0700 Subject: [PATCH] Adds logic for deletion of orphaned VSphereMachine This patch adds the logic for the deletion of orphaned vspheremachine objects. Orphaned VSphereMachine objects are those who do not have the CAPI Machine objects set as their owners. This situation could arise if the owner CAPI Machine gets deleted before claiming the VSphereMachine object as its dependent object. There is a CAPI bug which is supposed to solve this issue, but in the meanwhile, if the VSPhereMachine does not have any other owner references apart from the VSphereCluster object, then we delete those objects during VSphereCluster deletion loop. Signed-off-by: Sagar Muchhal --- controllers/vspherecluster_controller.go | 66 ++++++++- controllers/vspherecluster_controller_test.go | 129 +++++++++++++++++- 2 files changed, 190 insertions(+), 5 deletions(-) diff --git a/controllers/vspherecluster_controller.go b/controllers/vspherecluster_controller.go index 39ce37bd37..a7f45c283d 100644 --- a/controllers/vspherecluster_controller.go +++ b/controllers/vspherecluster_controller.go @@ -29,6 +29,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" clusterutilv1 "sigs.k8s.io/cluster-api/util" @@ -201,6 +202,10 @@ func (r clusterReconciler) Reconcile(ctx goctx.Context, req ctrl.Request) (_ ctr } }() + if err := setOwnerRefsOnVsphereMachines(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(clusterContext) @@ -213,14 +218,38 @@ func (r clusterReconciler) Reconcile(ctx goctx.Context, req ctrl.Request) (_ ctr func (r clusterReconciler) reconcileDelete(ctx *context.ClusterContext) (reconcile.Result, error) { ctx.Logger.Info("Reconciling VSphereCluster delete") - vsphereMachines, err := infrautilv1.GetVSphereMachinesInCluster(ctx, ctx.Client, ctx.VSphereCluster.Namespace, ctx.VSphereCluster.Name) + vsphereMachines, err := infrautilv1.GetVSphereMachinesInCluster(ctx, ctx.Client, ctx.Cluster.Namespace, ctx.Cluster.Name) if err != nil { return reconcile.Result{}, errors.Wrapf(err, "unable to list VSphereMachines part of VSphereCluster %s/%s", ctx.VSphereCluster.Namespace, ctx.VSphereCluster.Name) } - if len(vsphereMachines) > 0 { - ctx.Logger.Info("Waiting for VSphereMachines to be deleted", "count", len(vsphereMachines)) + machineDeletionCount := 0 + var deletionErrors []error + for _, vsphereMachine := range vsphereMachines { + // 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, ctx.VSphereCluster) && len(vsphereMachine.OwnerReferences) == 1 { + machineDeletionCount++ + // Remove the finalizer since VM creation wouldn't proceed + r.Logger.Info("Removing finalizer from VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name) + ctrlutil.RemoveFinalizer(vsphereMachine, infrav1.MachineFinalizer) + if err := r.Client.Update(ctx, vsphereMachine); err != nil { + return reconcile.Result{}, err + } + if err := r.Client.Delete(ctx, vsphereMachine); err != nil && !apierrors.IsNotFound(err) { + ctx.Logger.Error(err, "Failed to delete for VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name) + deletionErrors = append(deletionErrors, err) + } + } + } + if len(deletionErrors) > 0 { + return reconcile.Result{}, kerrors.NewAggregate(deletionErrors) + } + + if len(vsphereMachines)-machineDeletionCount > 0 { + ctx.Logger.Info("Waiting for VSphereMachines to be deleted", "count", len(vsphereMachines)-machineDeletionCount) return reconcile.Result{RequeueAfter: 10 * time.Second}, nil } @@ -498,6 +527,37 @@ func (r clusterReconciler) isControlPlaneInitialized(ctx *context.ClusterContext return conditions.IsTrue(ctx.Cluster, clusterv1.ControlPlaneInitializedCondition) } +func setOwnerRefsOnVsphereMachines(ctx *context.ClusterContext) error { + vsphereMachines, err := infrautilv1.GetVSphereMachinesInCluster(ctx, ctx.Client, ctx.Cluster.Namespace, ctx.Cluster.Name) + if err != nil { + return errors.Wrapf(err, + "unable to list VSphereMachines part of VSphereCluster %s/%s", ctx.VSphereCluster.Namespace, ctx.VSphereCluster.Name) + } + + var patchErrors []error + for _, vsphereMachine := range vsphereMachines { + patchHelper, err := patch.NewHelper(vsphereMachine, ctx.Client) + if err != nil { + patchErrors = append(patchErrors, err) + continue + } + + vsphereMachine.SetOwnerReferences(clusterutilv1.EnsureOwnerRef( + vsphereMachine.OwnerReferences, + metav1.OwnerReference{ + APIVersion: ctx.VSphereCluster.APIVersion, + Kind: ctx.VSphereCluster.Kind, + Name: ctx.VSphereCluster.Name, + UID: ctx.VSphereCluster.UID, + })) + + if err := patchHelper.Patch(ctx, vsphereMachine); err != nil { + patchErrors = append(patchErrors, err) + } + } + return kerrors.NewAggregate(patchErrors) +} + // controlPlaneMachineToCluster is a handler.ToRequestsFunc to be used // to enqueue requests for reconciliation for VSphereCluster to update // its status.apiEndpoints field. diff --git a/controllers/vspherecluster_controller_test.go b/controllers/vspherecluster_controller_test.go index a417ca6aa6..41ccd69c2a 100644 --- a/controllers/vspherecluster_controller_test.go +++ b/controllers/vspherecluster_controller_test.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + clusterutil1v1 "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" @@ -162,7 +163,7 @@ var _ = Describe("ClusterReconciler", func() { }, Spec: clusterv1.ClusterSpec{ InfrastructureRef: &corev1.ObjectReference{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha4", + APIVersion: infrav1.GroupVersion.String(), Kind: "VsphereCluster", Name: "vsphere-test1", }, @@ -247,7 +248,7 @@ var _ = Describe("ClusterReconciler", func() { }, Spec: clusterv1.ClusterSpec{ InfrastructureRef: &corev1.ObjectReference{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha4", + APIVersion: infrav1.GroupVersion.String(), Kind: "VsphereCluster", Name: "vsphere-test1", }, @@ -385,8 +386,132 @@ var _ = Describe("ClusterReconciler", func() { }, timeout).Should(BeTrue()) }) }) + + Context("For VSphereMachines belonging to the cluster", func() { + + var ( + namespace string + testNs *corev1.Namespace + ) + + BeforeEach(func() { + var err error + testNs, err = testEnv.CreateNamespace(ctx, "vsm-owner-ref") + Expect(err).NotTo(HaveOccurred()) + namespace = testNs.Name + }) + + AfterEach(func() { + Expect(testEnv.Delete(ctx, testNs)).To(Succeed()) + }) + + It("sets owner references to those machines", func() { + // Create the VSphereCluster object + instance := &infrav1.VSphereCluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test1-", + Namespace: namespace, + }, + Spec: infrav1.VSphereClusterSpec{ + Server: testEnv.Simulator.ServerURL().Host, + }, + } + Expect(testEnv.Create(ctx, instance)).To(Succeed()) + + capiCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "capi-test1-", + Namespace: namespace, + }, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "VsphereCluster", + Name: instance.Name, + }, + }, + } + Expect(testEnv.Create(ctx, capiCluster)).To(Succeed()) + + // Make sure the VSphereCluster exists. + key := client.ObjectKey{Namespace: namespace, Name: instance.Name} + Eventually(func() error { + return testEnv.Get(ctx, key, instance) + }, timeout).Should(BeNil()) + + machineCount := 3 + for i := 0; i < machineCount; i++ { + Expect(createVsphereMachine(ctx, testEnv, namespace, capiCluster.Name)).To(Succeed()) + } + + Eventually(func() bool { + ph, err := patch.NewHelper(instance, testEnv) + Expect(err).ShouldNot(HaveOccurred()) + instance.OwnerReferences = append(instance.OwnerReferences, metav1.OwnerReference{ + Kind: "Cluster", + APIVersion: clusterv1.GroupVersion.String(), + Name: capiCluster.Name, + UID: "blah", + }) + Expect(ph.Patch(ctx, instance, patch.WithStatusObservedGeneration{})).ShouldNot(HaveOccurred()) + return true + }, timeout).Should(BeTrue()) + + By("checking for presence of VSphereMachine objects") + Eventually(func() int { + machines := &infrav1.VSphereMachineList{} + if err := testEnv.List(ctx, machines, client.InNamespace(namespace), + client.MatchingLabels(map[string]string{clusterv1.ClusterLabelName: capiCluster.Name})); err != nil { + return -1 + } + return len(machines.Items) + }, timeout).Should(Equal(machineCount)) + + By("checking VSphereMachine owner refs") + Eventually(func() int { + machines := &infrav1.VSphereMachineList{} + if err := testEnv.List(ctx, machines, client.InNamespace(namespace), + client.MatchingLabels(map[string]string{clusterv1.ClusterLabelName: capiCluster.Name})); err != nil { + return 0 + } + ownerRefSet := 0 + for _, m := range machines.Items { + if len(m.OwnerReferences) >= 1 && clusterutil1v1.HasOwnerRef(m.OwnerReferences, metav1.OwnerReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: instance.Kind, + Name: instance.Name, + UID: instance.UID, + }) { + ownerRefSet++ + } + } + return ownerRefSet + }, timeout).Should(Equal(machineCount)) + }) + }) }) +func createVsphereMachine(ctx context.Context, env *helpers.TestEnvironment, namespace, clusterName string) error { + vsphereMachine := &infrav1.VSphereMachine{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-vsp", + Namespace: namespace, + Labels: map[string]string{clusterv1.ClusterLabelName: clusterName}, + }, + Spec: infrav1.VSphereMachineSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + Template: "ubuntu-k9s-1.19", + Network: infrav1.NetworkSpec{ + Devices: []infrav1.NetworkDeviceSpec{ + {NetworkName: "network-1", DHCP4: true}, + }, + }, + }, + }, + } + return env.Create(ctx, vsphereMachine) +} + func TestClusterReconciler_ReconcileDeploymentZones(t *testing.T) { server := "vcenter123.foo.com" g := NewWithT(t)