Merge pull request #180 from wanyufe/main

Update failuredomain regexp and prevent race condition while removing failure domain

Author: rejoshed

controllers/cloudstackfailuredomain_controller.go

@@ -31,6 +31,11 @@ import (
 	csCtrlrUtils "sigs.k8s.io/cluster-api-provider-cloudstack/controllers/utils"
 )
 
+const (
+	conditionTypeReady   = "Ready"
+	conditionStatusFalse = "False"
+)
+
 // CloudStackFailureDomainReconciler is the k8s controller manager's interface to reconcile a CloudStackFailureDomain.
 // This is primarily to adapt to k8s.
 type CloudStackFailureDomainReconciler struct {
@@ -122,6 +127,7 @@ func (r *CloudStackFailureDomainReconciliationRunner) ReconcileDelete() (ctrl.Re
 
 	return r.RunReconciliationStages(
 		r.GetAllMachinesInFailureDomain,
+		r.RequeueIfClusterNotReady,
 		r.RequeueIfMachineCannotBeRemoved,
 		r.ClearMachines,
 		r.DeleteOwnedObjects(
@@ -148,6 +154,21 @@ func (r *CloudStackFailureDomainReconciliationRunner) GetAllMachinesInFailureDom
 	return ctrl.Result{}, nil
 }
 
+// RequeueIfClusterNotReady check cluster to see if there is any rolling update going on.
+func (r *CloudStackFailureDomainReconciliationRunner) RequeueIfClusterNotReady() (ctrl.Result, error) {
+	if len(r.Machines) > 0 {
+		if !r.CAPICluster.DeletionTimestamp.IsZero() {
+			return ctrl.Result{}, nil
+		}
+		for _, condition := range r.CAPICluster.Status.Conditions {
+			if condition.Type == conditionTypeReady && condition.Status == conditionStatusFalse {
+				return r.RequeueWithMessage("cluster status not ready,")
+			}
+		}
+	}
+	return ctrl.Result{}, nil
+}
+
 // RequeueIfMachineCannotBeRemoved checks for each machine to confirm it is not risky to remove it.
 func (r *CloudStackFailureDomainReconciliationRunner) RequeueIfMachineCannotBeRemoved() (ctrl.Result, error) {
 	// check CAPI machines for CloudStack machines found.

controllers/cloudstackfailuredomain_controller_test.go

@@ -21,10 +21,12 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/utils/pointer"
 	infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-cloudstack/pkg/cloud"
 	dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta2"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -47,84 +49,93 @@ var _ = Describe("CloudStackFailureDomainReconciler", func() {
 					arg1.(*infrav1.CloudStackZoneSpec).Network.ID = "SomeID"
 					arg1.(*infrav1.CloudStackZoneSpec).Network.Type = cloud.NetworkTypeShared
 				}).MinTimes(1)
-
 		})
 
-		It("Should set failure domain Status.Ready to true.", func() {
-			assertFailureDomainCreated()
-		})
 		It("Should delete failure domain if no VM under this failure domain.", func() {
-			assertFailureDomainCreated()
+			Eventually(func() bool {
+				return getFailuredomainStatus(dummies.CSFailureDomain1)
+			}, timeout).WithPolling(pollInterval).Should(BeTrue())
+
 			Ω(k8sClient.Delete(ctx, dummies.CSFailureDomain1))
 
-			assertFailureDomainNotExisted()
+			tempfd := &infrav1.CloudStackFailureDomain{}
+			Eventually(func() bool {
+				key := client.ObjectKeyFromObject(dummies.CSFailureDomain1)
+				if err := k8sClient.Get(ctx, key, tempfd); err != nil {
+					return errors.IsNotFound(err)
+				}
+				return false
+			}, timeout).WithPolling(pollInterval).Should(BeTrue())
 		})
+
 		DescribeTable("Should function in different replicas conditions",
-			func(shouldDeleteVM bool, specReplicas, statusReplicas, statusReadyReplicas *int32, statusReady *bool) {
-				assertFailureDomainCreated()
+			func(shouldDeleteVM bool, specReplicas, statusReplicas, statusReadyReplicas *int32, statusReady *bool, controlPlaneReady bool) {
+				Eventually(func() bool {
+					return getFailuredomainStatus(dummies.CSFailureDomain1)
+				}, timeout).WithPolling(pollInterval).Should(BeTrue())
+
 				setCSMachineOwnerCRD(dummies.CSMachineOwner, specReplicas, statusReplicas, statusReadyReplicas, statusReady)
 				setCAPIMachineAndCSMachineCRDs(dummies.CSMachine1, dummies.CAPIMachine)
 				setMachineOwnerReference(dummies.CSMachine1, dummies.CSMachineOwnerReference)
 				labelMachineFailuredomain(dummies.CSMachine1, dummies.CSFailureDomain1)
 
+				if !controlPlaneReady {
+					Eventually(func() error {
+						ph, err := patch.NewHelper(dummies.CAPICluster, k8sClient)
+						Ω(err).ShouldNot(HaveOccurred())
+						dummies.CAPICluster.Status.Conditions = []clusterv1.Condition{
+							{
+								Type:   "Ready",
+								Status: "False",
+							},
+						}
+						return ph.Patch(ctx, dummies.CAPICluster, patch.WithStatusObservedGeneration{})
+					}, timeout).Should(Succeed())
+				}
+
 				Ω(k8sClient.Delete(ctx, dummies.CSFailureDomain1))
 
 				CAPIMachine := &clusterv1.Machine{}
-				Eventually(func() bool {
-					key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CAPIMachine.Name}
-					if shouldDeleteVM {
+				if shouldDeleteVM {
+					Eventually(func() bool {
+						key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CAPIMachine.Name}
 						if err := k8sClient.Get(ctx, key, CAPIMachine); err != nil {
 							return errors.IsNotFound(err)
 						}
-					} else {
+						return false
+					}, timeout).WithPolling(pollInterval).Should(BeTrue())
+				} else {
+					Consistently(func() bool {
+						key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CAPIMachine.Name}
 						if err := k8sClient.Get(ctx, key, CAPIMachine); err == nil {
 							return CAPIMachine.DeletionTimestamp.IsZero()
 						}
-					}
-					return false
-				}, timeout).WithPolling(pollInterval).Should(BeTrue())
+						return false
+					}, timeout).WithPolling(pollInterval).Should(BeTrue())
+				}
+
 			},
 			// should delete - simulate owner is kubeadmcontrolplane
-			Entry("Should delete machine if spec.replicas > 1", true, int32Pointer(2), int32Pointer(2), int32Pointer(2), boolPointer(true)),
+			Entry("Should delete machine if spec.replicas > 1", true, pointer.Int32(2), pointer.Int32(2), pointer.Int32(2), pointer.Bool(true), true),
 			// should delete - simulate owner is etcdadmcluster
-			Entry("Should delete machine if status.readyReplica does not exist", true, int32Pointer(2), int32Pointer(2), nil, boolPointer(true)),
+			Entry("Should delete machine if status.readyReplica does not exist", true, pointer.Int32(2), pointer.Int32(2), nil, pointer.Bool(true), true),
 			// should delete - simulate owner is machineset
-			Entry("Should delete machine if status.ready does not exist", true, int32Pointer(2), int32Pointer(2), int32Pointer(2), nil),
+			Entry("Should delete machine if status.ready does not exist", true, pointer.Int32(2), pointer.Int32(2), pointer.Int32(2), nil, true),
 			// should not delete if condition not met
-			Entry("Should return error if status.replicas < spec.replicas", false, int32Pointer(2), int32Pointer(1), int32Pointer(1), boolPointer(true)),
-			Entry("Should return error if spec.replicas < 2", false, int32Pointer(1), int32Pointer(1), int32Pointer(1), boolPointer(true)),
-			Entry("Should return error if status.ready is false", false, int32Pointer(2), int32Pointer(2), int32Pointer(2), boolPointer(false)),
-			Entry("Should return error if status.readyReplicas <> status.replicas", false, int32Pointer(2), int32Pointer(2), int32Pointer(1), boolPointer(true)),
+			Entry("Should not delete machine if cluster control plane not ready", false, pointer.Int32(2), pointer.Int32(2), pointer.Int32(2), pointer.Bool(true), false),
+			Entry("Should not delete machine if status.replicas < spec.replicas", false, pointer.Int32(2), pointer.Int32(1), pointer.Int32(1), pointer.Bool(true), true),
+			Entry("Should not delete machine if spec.replicas < 2", false, pointer.Int32(1), pointer.Int32(1), pointer.Int32(1), pointer.Bool(true), true),
+			Entry("Should not delete machine if status.ready is false", false, pointer.Int32(2), pointer.Int32(2), pointer.Int32(2), pointer.Bool(false), true),
+			Entry("Should not delete machine if status.readyReplicas <> status.replicas", false, pointer.Int32(2), pointer.Int32(2), pointer.Int32(1), pointer.Bool(true), true),
 		)
 	})
 })
 
-func assertFailureDomainCreated() {
-	tempfd := &infrav1.CloudStackFailureDomain{}
-	Eventually(func() bool {
-		key := client.ObjectKeyFromObject(dummies.CSFailureDomain1)
-		if err := k8sClient.Get(ctx, key, tempfd); err == nil {
-			return tempfd.Status.Ready
-		}
-		return false
-	}, timeout).WithPolling(pollInterval).Should(BeTrue())
-}
-
-func assertFailureDomainNotExisted() {
+func getFailuredomainStatus(failureDomain *infrav1.CloudStackFailureDomain) bool {
 	tempfd := &infrav1.CloudStackFailureDomain{}
-	Eventually(func() bool {
-		key := client.ObjectKeyFromObject(dummies.CSFailureDomain1)
-		if err := k8sClient.Get(ctx, key, tempfd); err != nil {
-			return true
-		}
-		return false
-	}, timeout).WithPolling(pollInterval).Should(BeTrue())
-}
-
-func boolPointer(b bool) *bool {
-	return &b
-}
-
-func int32Pointer(b int32) *int32 {
-	return &b
+	key := client.ObjectKeyFromObject(failureDomain)
+	if err := k8sClient.Get(ctx, key, tempfd); err == nil {
+		return tempfd.Status.Ready
+	}
+	return false
 }

controllers/cloudstackmachine_controller.go

@@ -99,6 +99,7 @@ func (reconciler *CloudStackMachineReconciler) Reconcile(ctx context.Context, re
 
 func (r *CloudStackMachineReconciliationRunner) Reconcile() (retRes ctrl.Result, reterr error) {
 	return r.RunReconciliationStages(
+		r.DeleteMachineIfFailuredomainNotExist,
 		r.GetObjectByName("placeholder", r.IsoNet,
 			func() string { return r.IsoNetMetaName(r.FailureDomain.Spec.Zone.Network.Name) }),
 		r.RunIf(func() bool { return r.FailureDomain.Spec.Zone.Network.Type == cloud.NetworkTypeIsolated },
@@ -156,6 +157,29 @@ func (r *CloudStackMachineReconciliationRunner) SetFailureDomainOnCSMachine() (r
 	return ctrl.Result{}, nil
 }
 
+// DeleteMachineIfFailuredomainNotExist delete CAPI machine if machine is deployed in a failuredomain that does not exist anymore.
+func (r *CloudStackMachineReconciliationRunner) DeleteMachineIfFailuredomainNotExist() (retRes ctrl.Result, reterr error) {
+	if r.CAPIMachine.Spec.FailureDomain == nil {
+		return ctrl.Result{}, nil
+	}
+	capiAssignedFailuredomainName := *r.CAPIMachine.Spec.FailureDomain
+	exist := false
+	for _, fd := range r.CSCluster.Spec.FailureDomains {
+		if capiAssignedFailuredomainName == fd.Name {
+			exist = true
+			break
+		}
+	}
+	if !exist {
+		r.Log.Info("CAPI Machine in non-existent failuredomain. Deleting associated Machine.", "csMachine", r.ReconciliationSubject.GetName(), "failuredomain", capiAssignedFailuredomainName)
+		if err := r.K8sClient.Delete(r.RequestCtx, r.CAPIMachine); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
+	return ctrl.Result{}, nil
+}
+
 // GetOrCreateVMInstance gets or creates a VM instance.
 // Implicitly it also fetches its bootstrap secret in order to create said instance.
 func (r *CloudStackMachineReconciliationRunner) GetOrCreateVMInstance() (retRes ctrl.Result, reterr error) {

go.mod

@@ -17,7 +17,6 @@ require (
 	golang.org/x/text v0.3.7
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
 	k8s.io/api v0.23.0
-	k8s.io/apiextensions-apiserver v0.23.0
 	k8s.io/apimachinery v0.23.0
 	k8s.io/client-go v0.23.0
 	k8s.io/klog/v2 v2.30.0