test: improve the join and leave tests (#73)

Author: zhiying-lin

pkg/scheduler/watchers/membercluster/controller_integration_test.go

@@ -24,6 +24,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -512,19 +513,31 @@ var _ = Describe("scheduler member cluster source controller", Serial, Ordered,
 		})
 	})
 
-	Context("ready cluster has left", func() {
+	Context("leaving clusters should not trigger rescheduling until the member cluster object is fully deleted", func() {
 		BeforeAll(func() {
 			Consistently(noKeyEnqueuedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Workqueue is not empty")
 
 			// Retrieve the cluster.
 			memberCluster := &clusterv1beta1.MemberCluster{}
 			Expect(hubClient.Get(ctx, types.NamespacedName{Name: clusterName1}, memberCluster)).To(Succeed(), "Failed to get member cluster")
-
-			// Update the spec as leave.
+			// Delete the cluster to simulate leaving which will not delete the MC as it has finalizers.
 			Expect(hubClient.Delete(ctx, memberCluster)).To(Succeed(), "Failed to delete member cluster")
 		})
 
-		It("should enqueue all CRPs for cluster left (case 1b)", func() {
+		It("should not enqueue all CRPs for cluster leaving", func() {
+			Consistently(noKeyEnqueuedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Workqueue is not empty")
+		})
+
+		It("remove the finalizer", func() {
+			// Retrieve the cluster.
+			memberCluster := &clusterv1beta1.MemberCluster{}
+			Expect(hubClient.Get(ctx, types.NamespacedName{Name: clusterName1}, memberCluster)).To(Succeed(), "Failed to get member cluster")
+			// Remove the finalizer from cluster.
+			controllerutil.RemoveFinalizer(memberCluster, placementv1beta1.MemberClusterFinalizer)
+			Expect(hubClient.Update(ctx, memberCluster)).Should(Succeed(), "Failed to update member cluster taints")
+		})
+
+		It("should enqueue all CRPs for cluster left (case 2c)", func() {
 			Eventually(allKeysEnqueuedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Keys are not enqueued as expected")
 			Consistently(allKeysEnqueuedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Keys are not enqueued as expected")
 		})

pkg/scheduler/watchers/membercluster/suite_test.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
@@ -62,11 +63,13 @@ var (
 
 var (
 	newMemberCluster = func(name string) *clusterv1beta1.MemberCluster {
-		return &clusterv1beta1.MemberCluster{
+		memberCluster := &clusterv1beta1.MemberCluster{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: name,
 			},
 		}
+		controllerutil.AddFinalizer(memberCluster, placementv1beta1.MemberClusterFinalizer)
+		return memberCluster
 	}
 
 	newCRP = func(name string, policy *placementv1beta1.PlacementPolicy) *placementv1beta1.ClusterResourcePlacement {
@@ -143,7 +146,9 @@ func setupResources() {
 }
 
 var _ = BeforeSuite(func() {
-	klog.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
+	logger := zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter))
+	klog.SetLogger(logger)
+	ctrl.SetLogger(logger)
 
 	ctx, cancel = context.WithCancel(context.TODO())
 

pkg/scheduler/watchers/membercluster/watcher.go

@@ -80,7 +80,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	//     a) the cluster's setup (e.g., its labels) or status (e.g., resource/non-resource properties),
 	//        has changed; and/or
 	//     b) an unexpected development (e.g., agents failing, network partition, etc.) has occurred.
-	//     c) the cluster, which may or may not have resources placed on it, has left the fleet (deleting).
+	//     c) the cluster, which may or may not have resources placed on it, has left the fleet (deleted).
 	//
 	// Among the cases,
 	//
@@ -123,9 +123,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	memberClusterGetErr := r.Client.Get(ctx, memberClusterKey, memberCluster)
 	switch {
 	case errors.IsNotFound(memberClusterGetErr):
-		// On very unlikely occasions, it could happen that the member cluster is deleted
-		// before this controller gets a chance to process it, it may happen when a member cluster
-		// leaves the fleet. In such cases, this controller will request the scheduler to check
+		// This could happen when the member cluster is deleted. In this case, controller will request the scheduler to check
 		// all CRPs just in case.
 		isMemberClusterMissing = true
 	case memberClusterGetErr != nil:
@@ -185,10 +183,9 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 			return false
 		},
 		DeleteFunc: func(e event.DeleteEvent) bool {
-			// Ignore deletion events; the removal of a cluster is first signaled by adding a deleteTimeStamp,
-			// which is an update event
-			klog.V(3).InfoS("Ignoring delete events for member cluster objects", "eventObject", klog.KObj(e.Object))
-			return false
+			// We only notify the scheduler when a member cluster is deleted which means the member agent has finished the leaving process.
+			klog.V(2).InfoS("Member cluster object is deleted", "eventObject", klog.KObj(e.Object))
+			return true
 		},
 		UpdateFunc: func(e event.UpdateEvent) bool {
 			// Check if the update event is valid.
@@ -208,9 +205,9 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 			clusterKObj := klog.KObj(newCluster)
 			// The cluster is being deleted.
-			if oldCluster.GetDeletionTimestamp().IsZero() && !newCluster.GetDeletionTimestamp().IsZero() {
-				klog.V(2).InfoS("A member cluster is leaving the fleet", "memberCluster", clusterKObj)
-				return true
+			if !newCluster.GetDeletionTimestamp().IsZero() {
+				klog.V(2).InfoS("A member cluster is leaving the fleet, ignore until it is left", "memberCluster", clusterKObj)
+				return false
 			}
 
 			// Capture label changes.

test/e2e/join_and_leave_test.go

@@ -72,7 +72,7 @@ var _ = Describe("Test member cluster join and leave flow", Ordered, Serial, fun
 			},
 			{
 				Group:   placementv1beta1.GroupVersion.Group,
-				Kind:    "ClusterResourceEnvelope",
+				Kind:    placementv1beta1.ClusterResourceEnvelopeKind,
 				Version: placementv1beta1.GroupVersion.Version,
 				Name:    testClusterResourceEnvelope.Name,
 			},
@@ -212,12 +212,20 @@ var _ = Describe("Test member cluster join and leave flow", Ordered, Serial, fun
 			checkIfAllMemberClustersHaveLeft()
 		})
 
-		It("should update CRP status to not placing any resources since all clusters are left", func() {
+		It("Should update CRP status to not placing any resources since all clusters are left", func() {
 			// resourceQuota is enveloped so it's not trackable yet
 			crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, nil, nil, "0", false)
 			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
 		})
 
+		It("Valdiating if the resources are still on all member clusters", func() {
+			for idx := range allMemberClusters {
+				memberCluster := allMemberClusters[idx]
+				workResourcesPlacedActual := checkAllResourcesPlacement(memberCluster)
+				Consistently(workResourcesPlacedActual, 3*consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to place work resources on member cluster %s", memberCluster.ClusterName)
+			}
+		})
+
 		It("Should be able to rejoin the cluster", func() {
 			By("rejoin all the clusters without deleting the CRP")
 			setAllMemberClustersToJoin()

test/e2e/scheduler_watchers_test.go

@@ -589,6 +589,17 @@ var _ = Describe("responding to specific member cluster changes", func() {
 			Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to update CRP status as expected")
 		})
 
+		It("can mark the cluster as leaving", func() {
+			markMemberClusterAsLeaving(fakeClusterName1ForWatcherTests)
+		})
+
+		It("should not remove the leaving cluster from the scheduling decision", func() {
+			targetClusterNames := allMemberClusterNames
+			targetClusterNames = append(targetClusterNames, fakeClusterName1ForWatcherTests)
+			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), targetClusterNames, nil, "0")
+			Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+		})
+
 		It("can mark the cluster as left", func() {
 			markMemberClusterAsLeft(fakeClusterName1ForWatcherTests)
 		})
@@ -1515,6 +1526,17 @@ var _ = Describe("responding to specific member cluster changes", func() {
 			Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to update CRP status as expected")
 		})
 
+		It("can mark the cluster as leaving", func() {
+			markMemberClusterAsLeaving(fakeClusterName1ForWatcherTests)
+		})
+
+		It("should not remove the leaving cluster from the scheduling decision", func() {
+			targetClusterNames := allMemberClusterNames
+			targetClusterNames = append(targetClusterNames, fakeClusterName1ForWatcherTests)
+			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), targetClusterNames, nil, "0")
+			Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+		})
+
 		It("can mark the cluster as left", func() {
 			markMemberClusterAsLeft(fakeClusterName1ForWatcherTests)
 		})

test/e2e/utils_test.go

@@ -166,8 +166,8 @@ func markMemberClusterAsUnhealthy(name string) {
 	}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to mark member cluster as unhealthy")
 }
 
-// markMemberClusterAsLeft marks the specified member cluster as left.
-func markMemberClusterAsLeft(name string) {
+// markMemberClusterAsLeaving marks the specified member cluster as leaving.
+func markMemberClusterAsLeaving(name string) {
 	mcObj := &clusterv1beta1.MemberCluster{}
 	Eventually(func() error {
 		// Add a custom deletion blocker finalizer to the member cluster.
@@ -177,9 +177,27 @@ func markMemberClusterAsLeft(name string) {
 
 		mcObj.Finalizers = append(mcObj.Finalizers, customDeletionBlockerFinalizer)
 		return hubClient.Update(ctx, mcObj)
-	}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to mark member cluster as left")
+	}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to add finalizer")
+
+	Expect(hubClient.Delete(ctx, mcObj)).To(Succeed(), "Failed to mark member cluster as leaving")
+}
+
+// markMemberClusterAsLeft deletes the specified member cluster.
+func markMemberClusterAsLeft(name string) {
+	mcObj := &clusterv1beta1.MemberCluster{}
+	Eventually(func() error {
+		// remove finalizer to the member cluster.
+		if err := hubClient.Get(ctx, types.NamespacedName{Name: name}, mcObj); err != nil {
+			return err
+		}
+		if len(mcObj.Finalizers) > 0 {
+			mcObj.Finalizers = []string{}
+			return hubClient.Update(ctx, mcObj)
+		}
+		return nil
+	}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove finalizer")
 
-	Expect(hubClient.Delete(ctx, mcObj)).To(Succeed(), "Failed to delete member cluster")
+	Expect(hubClient.Delete(ctx, mcObj)).To(SatisfyAny(Succeed(), utils.NotFoundMatcher{}), "Failed to delete member cluster")
 }
 
 // setAllMemberClustersToJoin creates a MemberCluster object for each member cluster.