fix: handle PickFixed CRP in eviction controller (Azure#998)

apis/placement/v1alpha1/disruptionbudget_types.go

@@ -44,8 +44,7 @@ type PlacementDisruptionBudgetSpec struct {
 	// If a percentage is specified, Fleet will calculate the corresponding absolute values
 	// as follows:
 	// * if the linked Placement object is of the PickFixed placement type,
-	//   the percentage is against the number of clusters specified in the placement (i.e., the
-	//   length of ClusterNames field in the placement policy);
+	//   we don't perform any calculation because eviction is not allowed for PickFixed CRP.
 	// * if the linked Placement object is of the PickAll placement type, MaxUnavailable cannot
 	//   be specified since we cannot derive the total number of clusters selected.
 	// * if the linked Placement object is of the PickN placement type,
@@ -74,8 +73,7 @@ type PlacementDisruptionBudgetSpec struct {
 	// If a percentage is specified, Fleet will calculate the corresponding absolute values
 	// as follows:
 	// * if the linked Placement object is of the PickFixed placement type,
-	//   the percentage is against the number of clusters specified in the placement (i.e., the
-	//   length of ClusterNames field in the placement policy);
+	//   we don't perform any calculation because eviction is not allowed for PickFixed CRP.
 	// * if the linked Placement object is of the PickAll placement type, MinAvailable can be
 	//   specified but only as an integer since we cannot derive the total number of clusters selected.
 	// * if the linked Placement object is of the PickN placement type,

apis/placement/v1alpha1/eviction_types.go

@@ -37,6 +37,10 @@ import (
 // (e.g., the CRP object or the placement appears later). To fix the situation, re-create the
 // Eviction object.
 //
+// Note: Eviction of resources from a cluster propagated by a PickFixed CRP is not allowed.
+// If the user wants to remove resources from a cluster propagated by a PickFixed CRP simply
+// remove the cluster name from cluster names field from the CRP spec.
+//
 // Executed evictions might be kept around for a while for auditing purposes; the Fleet controllers might
 // have a TTL set up for such objects and will garbage collect them automatically. For further
 // information, see the Fleet documentation.

pkg/controllers/clusterresourceplacementeviction/controller.go

@@ -91,6 +91,11 @@ func (r *Reconciler) validateEviction(ctx context.Context, eviction *placementv1
 		markEvictionInvalid(eviction, condition.EvictionInvalidDeletingCRPMessage)
 		return validationResult, nil
 	}
+	if crp.Spec.Policy.PlacementType == placementv1beta1.PickFixedPlacementType {
+		klog.V(2).InfoS(condition.EvictionInvalidPickFixedCRPMessage, "clusterResourcePlacementEviction", eviction.Name, "clusterResourcePlacement", eviction.Spec.PlacementName)
+		markEvictionInvalid(eviction, condition.EvictionInvalidPickFixedCRPMessage)
+		return validationResult, nil
+	}
 	validationResult.crp = &crp
 
 	var crbList placementv1beta1.ClusterResourceBindingList
@@ -256,13 +261,9 @@ func isEvictionAllowed(bindings []placementv1beta1.ClusterResourceBinding, crp p
 
 	var desiredBindings int
 	placementType := crp.Spec.Policy.PlacementType
-	switch placementType {
-	case placementv1beta1.PickNPlacementType:
+	// we don't know the desired bindings for PickAll and we won't evict a binding for PickFixed CRP.
+	if placementType == placementv1beta1.PickNPlacementType {
 		desiredBindings = int(*crp.Spec.Policy.NumberOfClusters)
-	case placementv1beta1.PickFixedPlacementType:
-		desiredBindings = len(crp.Spec.Policy.ClusterNames)
-	default:
-		// we don't know the desired bindings for PickAll.
 	}
 
 	var disruptionsAllowed int

pkg/controllers/clusterresourceplacementeviction/controller_test.go

@@ -135,6 +135,31 @@ func TestValidateEviction(t *testing.T) {
 			},
 			wantErr: nil,
 		},
+		{
+			name:     "invalid eviction - pickFixed CRP",
+			eviction: buildTestEviction(testEvictionName, testCRPName, testClusterName),
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: testCRPName,
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+					},
+				},
+			},
+			wantValidationResult: &evictionValidationResult{
+				isValid: false,
+			},
+			wantEvictionInvalidCondition: &metav1.Condition{
+				Type:               string(placementv1alpha1.PlacementEvictionConditionTypeValid),
+				Status:             metav1.ConditionFalse,
+				ObservedGeneration: 1,
+				Reason:             condition.ClusterResourcePlacementEvictionInvalidReason,
+				Message:            condition.EvictionInvalidPickFixedCRPMessage,
+			},
+			wantErr: nil,
+		},
 		{
 			name:     "invalid eviction - multiple CRBs for same cluster",
 			eviction: buildTestEviction(testEvictionName, testCRPName, testClusterName),

pkg/utils/condition/condition.go

@@ -165,6 +165,9 @@ const (
 	// EvictionInvalidDeletingCRPMessage is the message string of invalid eviction condition when CRP is deleting.
 	EvictionInvalidDeletingCRPMessage = "Found deleting ClusterResourcePlacement targeted by eviction"
 
+	// EvictionInvalidPickFixedCRPMessage is the message string of invalid eviction condition when CRP placement type is PickFixed.
+	EvictionInvalidPickFixedCRPMessage = "Found ClusterResourcePlacement with PickFixed placement type targeted by eviction"
+
 	// EvictionInvalidMissingCRBMessage is the message string of invalid eviction condition when CRB is missing.
 	EvictionInvalidMissingCRBMessage = "Failed to find scheduler decision for placement in cluster targeted by eviction"