add more checks

Author: Arvind Thirumurugan

pkg/controllers/clusterresourceplacementeviction/controller.go

@@ -169,7 +169,7 @@ func (r *Reconciler) executeEviction(ctx context.Context, validationResult *evic
 		return nil
 	}
 
-	if !isPlacementPresent(evictionTargetBinding) {
+	if !evictionutils.IsPlacementPresent(evictionTargetBinding) {
 		klog.V(2).InfoS("No resources have been placed for ClusterResourceBinding in target cluster",
 			"clusterResourcePlacementEviction", eviction.Name, "clusterResourceBinding", evictionTargetBinding.Name, "targetCluster", eviction.Spec.ClusterName)
 		markEvictionNotExecuted(eviction, condition.EvictionBlockedMissingPlacementMessage)
@@ -220,21 +220,6 @@ func (r *Reconciler) executeEviction(ctx context.Context, validationResult *evic
 	return nil
 }
 
-// isPlacementPresent checks to see if placement on target cluster could be present.
-func isPlacementPresent(binding *placementv1beta1.ClusterResourceBinding) bool {
-	if binding.Spec.State == placementv1beta1.BindingStateBound {
-		return true
-	}
-	if binding.Spec.State == placementv1beta1.BindingStateUnscheduled {
-		currentAnnotation := binding.GetAnnotations()
-		previousState, exist := currentAnnotation[placementv1beta1.PreviousBindingStateAnnotation]
-		if exist && placementv1beta1.BindingState(previousState) == placementv1beta1.BindingStateBound {
-			return true
-		}
-	}
-	return false
-}
-
 // isEvictionAllowed calculates if eviction allowed based on available bindings and spec specified in placement disruption budget.
 func isEvictionAllowed(bindings []placementv1beta1.ClusterResourceBinding, crp placementv1beta1.ClusterResourcePlacement, db placementv1beta1.ClusterResourcePlacementDisruptionBudget) (bool, int) {
 	availableBindings := 0

pkg/utils/eviction/eviction.go

@@ -20,3 +20,18 @@ func IsEvictionInTerminalState(eviction *placementv1beta1.ClusterResourcePlaceme
 	}
 	return false
 }
+
+// IsPlacementPresent checks to see if placement on target cluster could be present.
+func IsPlacementPresent(binding *placementv1beta1.ClusterResourceBinding) bool {
+	if binding.Spec.State == placementv1beta1.BindingStateBound {
+		return true
+	}
+	if binding.Spec.State == placementv1beta1.BindingStateUnscheduled {
+		currentAnnotation := binding.GetAnnotations()
+		previousState, exist := currentAnnotation[placementv1beta1.PreviousBindingStateAnnotation]
+		if exist && placementv1beta1.BindingState(previousState) == placementv1beta1.BindingStateBound {
+			return true
+		}
+	}
+	return false
+}

pkg/utils/eviction/eviction_test.go

@@ -83,7 +83,65 @@ func TestIsEvictionInTerminalState(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if got := IsEvictionInTerminalState(tt.eviction); got != tt.want {
-				t.Errorf("IsEvictionInTerminalState test failed got = %v, want %v", got, tt.want)
+				t.Errorf("IsEvictionInTerminalState test failed got = %v, want = %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestIsPlacementPresent(t *testing.T) {
+	tests := []struct {
+		name    string
+		binding *placementv1beta1.ClusterResourceBinding
+		want    bool
+	}{
+		{
+			name: "Bound binding - placement present",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				Spec: placementv1beta1.ResourceBindingSpec{
+					State: placementv1beta1.BindingStateBound,
+				},
+			},
+			want: true,
+		},
+		{
+			name: "Unscheduled binding with previous state bound - placement present",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				Spec: placementv1beta1.ResourceBindingSpec{
+					State: placementv1beta1.BindingStateUnscheduled,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						placementv1beta1.PreviousBindingStateAnnotation: string(placementv1beta1.BindingStateBound),
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "Unscheduled binding with no previous state - placement not present",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				Spec: placementv1beta1.ResourceBindingSpec{
+					State: placementv1beta1.BindingStateUnscheduled,
+				},
+			},
+			want: false,
+		},
+		{
+			name: "Scheduled binding - placement not present",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				Spec: placementv1beta1.ResourceBindingSpec{
+					State: placementv1beta1.BindingStateScheduled,
+				},
+			},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := IsPlacementPresent(tt.binding); got != tt.want {
+				t.Errorf("IsPlacementPresent test failed got = %v, want = %v", got, tt.want)
 			}
 		})
 	}

tools/draincluster/main.go

@@ -64,11 +64,16 @@ func main() {
 		ClusterResourcePlacementResourcesMap: make(map[string][]placementv1beta1.ResourceIdentifier),
 	}
 
-	if err = drainCluster.cordonCluster(ctx); err != nil {
+	uncordonCluster := toolsutils.UncordonCluster{
+		HubClient:   hubClient,
+		ClusterName: *clusterName,
+	}
+
+	if err = drainCluster.cordon(ctx); err != nil {
 		log.Fatalf("failed to cordon member cluster %s: %v", drainCluster.ClusterName, err)
 	}
 
-	isDrainSuccessful, err := drainCluster.drainCluster(ctx)
+	isDrainSuccessful, err := drainCluster.drain(ctx)
 	if err != nil {
 		log.Fatalf("failed to drain member cluster %s: %v", drainCluster.ClusterName, err)
 	}
@@ -78,19 +83,24 @@ func main() {
 	} else {
 		log.Printf("drain was not successful for cluster %s, some evictions were not successfully executed", *clusterName)
 		log.Printf("retrying drain to evict the resources that were not successfully removed")
-		isDrainRetrySuccessful, err := drainCluster.drainCluster(ctx)
+		// reset ClusterResourcePlacementResourcesMap for retry.
+		drainCluster.ClusterResourcePlacementResourcesMap = map[string][]placementv1beta1.ResourceIdentifier{}
+		isDrainRetrySuccessful, err := drainCluster.drain(ctx)
 		if err != nil {
 			log.Fatalf("failed to drain cluster again %s: %v", drainCluster.ClusterName, err)
 		}
 		if isDrainRetrySuccessful {
 			log.Printf("drain retry was successful for cluster %s", *clusterName)
 		} else {
-			log.Printf("drain retry was not successful for cluster %s, some evictions were not successfully executed", *clusterName)
+			if err = uncordonCluster.Uncordon(ctx); err != nil {
+				log.Fatalf("failed to uncordon cluster %s: %v", *clusterName, err)
+			}
+			log.Fatalf("drain retry was not successful for cluster %s, some evictions were not successfully executed", *clusterName)
 		}
 	}
 }
 
-func (d *DrainCluster) cordonCluster(ctx context.Context) error {
+func (d *DrainCluster) cordon(ctx context.Context) error {
 	// add taint to member cluster to ensure resources aren't scheduled on it.
 	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		var mc clusterv1beta1.MemberCluster
@@ -117,20 +127,44 @@ func (d *DrainCluster) cordonCluster(ctx context.Context) error {
 	})
 }
 
-func (d *DrainCluster) drainCluster(ctx context.Context) (bool, error) {
+func (d *DrainCluster) drain(ctx context.Context) (bool, error) {
 	var crpList placementv1beta1.ClusterResourcePlacementList
 	if err := d.hubClient.List(ctx, &crpList); err != nil {
 		return false, fmt.Errorf("failed to list cluster resource placements: %v", err)
 	}
 
 	// find all unique CRP names for which eviction needs to occur.
 	for i := range crpList.Items {
-		crpStatus := crpList.Items[i].Status
-		for j := range crpStatus.PlacementStatuses {
-			placementStatus := crpStatus.PlacementStatuses[j]
-			if placementStatus.ClusterName == d.ClusterName {
-				d.ClusterResourcePlacementResourcesMap[crpList.Items[i].Name] = resourcePropagatedByCRP(crpStatus.SelectedResources, placementStatus.FailedPlacements)
-				break
+		// list all bindings for the CRP.
+		crp := crpList.Items[i]
+		var crbList placementv1beta1.ClusterResourceBindingList
+		if err := d.hubClient.List(ctx, &crbList, client.MatchingLabels{placementv1beta1.CRPTrackingLabel: crp.Name}); err != nil {
+			return false, fmt.Errorf("failed to list cluster resource bindings for CRP %s: %v", crp.Name, err)
+		}
+		var targetBinding *placementv1beta1.ClusterResourceBinding
+		for j := range crbList.Items {
+			crb := crbList.Items[j]
+			if crb.Spec.TargetCluster == d.ClusterName {
+				targetBinding = &crb
+			}
+		}
+		if targetBinding != nil && targetBinding.DeletionTimestamp == nil {
+			// check if placement is present.
+			if !evictionutils.IsPlacementPresent(targetBinding) {
+				return false, fmt.Errorf("placement is not present in cluster %s for CRP %s, please retry drain once resources are propagated to the cluster", d.ClusterName, crp.Name)
+			}
+			isCRPStatusUpdated := false
+			crpStatus := crp.Status
+			for j := range crpStatus.PlacementStatuses {
+				placementStatus := crpStatus.PlacementStatuses[j]
+				if placementStatus.ClusterName == d.ClusterName {
+					isCRPStatusUpdated = true
+					d.ClusterResourcePlacementResourcesMap[crpList.Items[i].Name] = resourcePropagatedByCRP(crpStatus.SelectedResources, placementStatus.FailedPlacements)
+					break
+				}
+			}
+			if !isCRPStatusUpdated {
+				return false, fmt.Errorf("failed to determine all resources propagated to cluster %s for CRP %s", d.ClusterName, crp.Name)
 			}
 		}
 	}

tools/uncordoncluster/README.md

@@ -4,7 +4,7 @@ To uncordon a member cluster connected to a fleet, you can use the `uncordon-clu
 uncordon a member cluster that has been cordoned using the `cordon-cluster` tool. 
 
 ```
-go run tools/cordoncluster/main.go --hubClusterContext <hub-cluster-context> --clusterName <memberClusterName>
+go run tools/uncordoncluster/main.go --hubClusterContext <hub-cluster-context> --clusterName <memberClusterName>
 ```
 
 the tool currently is a go program that also takes the hub cluster context and the member cluster name as arguments.

tools/uncordoncluster/main.go

@@ -6,8 +6,6 @@ import (
 	"log"
 
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/util/retry"
 
 	clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1"
 	placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
@@ -41,30 +39,13 @@ func main() {
 		log.Fatalf("failed to create hub cluster client: %v", err)
 	}
 
-	var mc clusterv1beta1.MemberCluster
-	if err = hubClient.Get(ctx, types.NamespacedName{Name: *clusterName}, &mc); err != nil {
-		log.Fatalf("failed to get member cluster %s: %v", *clusterName, err)
+	uncordonCluster := toolsutils.UncordonCluster{
+		HubClient:   hubClient,
+		ClusterName: *clusterName,
 	}
 
-	// remove existing taints from member cluster.
-	err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		var mc clusterv1beta1.MemberCluster
-		if err := hubClient.Get(ctx, types.NamespacedName{Name: *clusterName}, &mc); err != nil {
-			return err
-		}
-
-		if mc.Spec.Taints == nil {
-			return nil
-		}
-
-		// remove all taints from member cluster.
-		mc.Spec.Taints = nil
-
-		return hubClient.Update(ctx, &mc)
-	})
-
-	if err != nil {
-		log.Fatalf("failed to remove taints from member cluster %s: %v", *clusterName, err)
+	if err = uncordonCluster.Uncordon(ctx); err != nil {
+		log.Fatalf("failed to uncordon cluster %s: %v", *clusterName, err)
 	}
 
 	log.Printf("uncordoned member cluster %s", *clusterName)

tools/utils/common.go

@@ -1,8 +1,14 @@
 package utils
 
 import (
+	"context"
 	"os"
 
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
+
+	clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1"
+
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/clientcmd"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -12,6 +18,29 @@ var (
 	kubeConfigPath = os.Getenv("KUBECONFIG")
 )
 
+type UncordonCluster struct {
+	HubClient   client.Client
+	ClusterName string
+}
+
+func (u *UncordonCluster) Uncordon(ctx context.Context) error {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var mc clusterv1beta1.MemberCluster
+		if err := u.HubClient.Get(ctx, types.NamespacedName{Name: u.ClusterName}, &mc); err != nil {
+			return err
+		}
+
+		if mc.Spec.Taints == nil {
+			return nil
+		}
+
+		// remove all taints from member cluster.
+		mc.Spec.Taints = nil
+
+		return u.HubClient.Update(ctx, &mc)
+	})
+}
+
 func GetClusterClientFromClusterContext(clusterContext string, scheme *runtime.Scheme) (client.Client, error) {
 	clusterConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
 		&clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeConfigPath},