Add finalizer for CksMachine Controller

Author: vishesh92

controllers/cks_cluster_controller.go

@@ -44,7 +44,6 @@ type CksClusterReconciliationRunner struct {
 }
 
 // CksClusterReconciler is the k8s controller manager's interface to reconcile a CloudStackCluster.
-// This is primarily to adapt to k8s.
 type CksClusterReconciler struct {
 	csCtrlrUtils.ReconcilerBase
 }

controllers/cks_machine_controller.go

@@ -21,30 +21,33 @@ import (
 	"fmt"
 
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3"
 	csCtrlrUtils "sigs.k8s.io/cluster-api-provider-cloudstack/controllers/utils"
 )
 
-// RBAC permissions for CloudStackCluster.
+const CksMachineFinalizer = "cksMachine.infrastructure.cluster.x-k8s.io"
+
+// RBAC permissions for CloudStackMachines.
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines,verbs=get;list;watch
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines/status,verbs=get
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines/finalizers,verbs=update
 
-// CksMachineReconciliationRunner is a ReconciliationRunner with extensions specific to CloudStackClusters.
+// CksMachineReconciliationRunner is a ReconciliationRunner with extensions specific to CloudStackMachines.
 // The runner does the actual reconciliation.
 type CksMachineReconciliationRunner struct {
 	*csCtrlrUtils.ReconciliationRunner
 	FailureDomain         *infrav1.CloudStackFailureDomain
 	ReconciliationSubject *infrav1.CloudStackMachine
 }
 
-// CksMachineReconciler is the k8s controller manager's interface to reconcile a CloudStackCluster.
-// This is primarily to adapt to k8s.
+// CksMachineReconciler is the k8s controller manager's interface to reconcile CloudStackMachines with a CKS cluster.
 type CksMachineReconciler struct {
 	csCtrlrUtils.ReconcilerBase
 }
 
-// Initialize a new CloudStackCluster reconciliation runner with concrete types and initialized member fields.
+// Initialize a new CloudStackMachines reconciliation runner with concrete types and initialized member fields.
 func NewCksMachineReconciliationRunner() *CksMachineReconciliationRunner {
 	// Set concrete type and init pointers.
 	runner := &CksMachineReconciliationRunner{ReconciliationSubject: &infrav1.CloudStackMachine{}}
@@ -64,7 +67,7 @@ func (reconciler *CksMachineReconciler) Reconcile(ctx context.Context, req ctrl.
 	return r.RunBaseReconciliationStages()
 }
 
-// Reconcile actually reconciles the CloudStackCluster.
+// Reconcile actually reconciles the CloudStackMachine.
 func (r *CksMachineReconciliationRunner) Reconcile() (res ctrl.Result, reterr error) {
 	if !r.CSCluster.Spec.SyncWithACS {
 		return ctrl.Result{}, nil
@@ -77,6 +80,9 @@ func (r *CksMachineReconciliationRunner) Reconcile() (res ctrl.Result, reterr er
 		return r.RequeueWithMessage("InstanceID is not set")
 	}
 
+	// Prevent premature deletion.
+	controllerutil.AddFinalizer(r.ReconciliationSubject, CksMachineFinalizer)
+
 	res, err := r.AsFailureDomainUser(&r.FailureDomain.Spec)()
 	if r.ShouldReturn(res, err) {
 		return res, err
@@ -86,18 +92,22 @@ func (r *CksMachineReconciliationRunner) Reconcile() (res ctrl.Result, reterr er
 	if err != nil {
 		return r.RequeueWithMessage(fmt.Sprintf("Adding VM to CloudStack CKS cluster failed. error: %s", err.Error()))
 	}
+	r.Log.Info("Assigned VM to CKS")
 	return ctrl.Result{}, nil
 
 }
 
-// ReconcileDelete cleans up resources used by the cluster and finally removes the CloudStackCluster's finalizers.
+// ReconcileDelete cleans up resources used by the cluster and finally removes the CloudStackMachine's finalizers.
 func (r *CksMachineReconciliationRunner) ReconcileDelete() (ctrl.Result, error) {
+	r.Log.Info("Removing VM from CKS")
 	if r.ReconciliationSubject.Spec.InstanceID != nil && *r.ReconciliationSubject.Spec.InstanceID != "" {
 		err := r.CSUser.RemoveVMFromCksCluster(r.CSCluster, r.ReconciliationSubject)
 		if err != nil {
 			return r.RequeueWithMessage(fmt.Sprintf("Removing VM from CloudStack CKS cluster failed. error: %s", err.Error()))
 		}
 	}
+	r.Log.Info("Removed VM from CKS")
+	controllerutil.RemoveFinalizer(r.ReconciliationSubject, CksMachineFinalizer)
 	return ctrl.Result{}, nil
 }
 

controllers/cks_machine_controller_test.go

@@ -75,7 +75,7 @@ var _ = Describe("CksCloudStackMachineReconciler", func() {
 				key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CSMachine1.Name}
 				if err := k8sClient.Get(ctx, key, tempMachine); err == nil {
 					if tempMachine.Status.Ready == true {
-						return len(tempMachine.ObjectMeta.Finalizers) > 0
+						return len(tempMachine.ObjectMeta.Finalizers) > 1
 					}
 				}
 				return false
@@ -94,7 +94,7 @@ var _ = Describe("CksCloudStackMachineReconciler", func() {
 
 			mockCloudClient.EXPECT().AddVMToCksCluster(gomock.Any(), gomock.Any()).MinTimes(1).Return(nil)
 
-			mockCloudClient.EXPECT().DestroyVMInstance(gomock.Any()).Times(1).Return(nil)
+			mockCloudClient.EXPECT().DestroyVMInstance(gomock.Any()).MinTimes(1).Return(nil)
 			mockCloudClient.EXPECT().RemoveVMFromCksCluster(
 				gomock.Any(), gomock.Any()).MinTimes(1).Return(nil)
 			// Have to do this here or the reconcile call to GetOrCreateVMInstance may happen too early.
@@ -106,7 +106,7 @@ var _ = Describe("CksCloudStackMachineReconciler", func() {
 				key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CSMachine1.Name}
 				if err := k8sClient.Get(ctx, key, tempMachine); err == nil {
 					if tempMachine.Status.Ready == true {
-						return true
+						return len(tempMachine.ObjectMeta.Finalizers) > 1
 					}
 				}
 				return false
@@ -122,7 +122,6 @@ var _ = Describe("CksCloudStackMachineReconciler", func() {
 				}
 				return false
 			}, timeout).WithPolling(pollInterval).Should(BeTrue())
-
 		})
 	})