fix the code

Author: Ryan Zhang

pkg/controllers/clusterresourceplacement/controller.go

@@ -969,6 +969,7 @@ func (r *Reconciler) listSortedResourceSnapshots(ctx context.Context, placementO
 }
 
 // setPlacementStatus returns if there is a cluster scheduled by the scheduler.
+// it returns true if the cluster schedule succeeded, false otherwise.
 func (r *Reconciler) setPlacementStatus(
 	ctx context.Context,
 	placementObj fleetv1beta1.PlacementObj,
@@ -997,11 +998,8 @@ func (r *Reconciler) setPlacementStatus(
 		// TODO: need to track whether we have deleted the resources for the last decisions.
 		// The undeleted resources on these old clusters could lead to failed synchronized or applied condition.
 		// Today, we only track the resources progress if the same cluster is selected again.
-
-		// For now, only CRP supports placement statuses - namespace-scoped placements would need different handling
-		if crp, ok := placementObj.(*fleetv1beta1.ClusterResourcePlacement); ok {
-			crp.Status.PlacementStatuses = []fleetv1beta1.ResourcePlacementStatus{}
-		}
+		klog.V(2).InfoS("Resetting the resource placement status since scheduled condition is unknown", "placement", klog.KObj(placementObj))
+		placementStatus.PlacementStatuses = []fleetv1beta1.ResourcePlacementStatus{}
 		return false, nil
 	}
 
@@ -1032,43 +1030,47 @@ func (r *Reconciler) setPlacementStatus(
 
 	// For clusters that failed to get scheduled, set a resource placement status with the
 	// failed to schedule condition for each of them.
-	// This is currently CRP-specific functionality
-	if crp, ok := placementObj.(*fleetv1beta1.ClusterResourcePlacement); ok {
-		allRPS = appendFailedToScheduleResourcePlacementStatuses(allRPS, unselected, failedToScheduleClusterCount, crp)
-
-		crp.Status.PlacementStatuses = allRPS
+	allRPS = appendFailedToScheduleResourcePlacementStatuses(allRPS, unselected, failedToScheduleClusterCount, placementObj)
+	placementStatus.PlacementStatuses = allRPS
+	klog.V(2).InfoS("Updated placement status for each individual cluster", "selectedNoCluster", len(selected), "unselectedNoCluster", len(unselected), "failedToScheduleClusterCount", failedToScheduleClusterCount, "placement", klog.KObj(placementObj))
 
-		// Prepare the conditions for the CRP object itself.
-
-		if len(selected) == 0 {
-			// There is no selected cluster at all. It could be that there is no matching cluster
-			// given the current scheduling policy; there remains a corner case as well where a cluster
-			// has been selected before (with resources being possibly applied), but has now
-			// left the fleet. To address this corner case, Fleet here will remove all lingering
-			// conditions (any condition type other than CRPScheduled).
+	// Prepare the conditions for the placement object itself.
+	policy := placementObj.GetPlacementSpec().Policy
+	if len(selected) == 0 {
+		// There is no selected cluster at all. It could be that there is no matching cluster
+		// given the current scheduling policy; there remains a corner case as well where a cluster
+		// has been selected before (with resources being possibly applied), but has now
+		// left the fleet. To address this corner case, Fleet here will remove all lingering
+		// conditions (any condition type other than CRPScheduled).
 
-			// Note that the scheduled condition has been set earlier in this method.
-			crp.Status.Conditions = []metav1.Condition{*crp.GetCondition(string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType))}
-			return false, nil
-		}
+		// Note that the scheduled condition has been set earlier in this method.
+		placementStatus.Conditions = []metav1.Condition{*placementObj.GetCondition(getPlacementScheduledConditionType(placementObj))}
+		return isPolicySelectingNoClusters(policy), nil
+	}
 
-		if crp.Spec.Strategy.Type == fleetv1beta1.ExternalRolloutStrategyType {
-			// For external rollout strategy, if clusters observe different resource snapshot versions,
-			// we set RolloutStarted to Unknown without any other conditions since we do not know exactly which version is rolling out.
-			// We also need to reset ObservedResourceIndex and selectedResources.
-			rolloutStartedUnknown, err := r.determineRolloutStateForPlacementWithExternalRolloutStrategy(ctx, placementObj, selected, allRPS, selectedResourceIDs)
-			if err != nil || rolloutStartedUnknown {
-				return true, err
-			}
+	if placementObj.GetPlacementSpec().Strategy.Type == fleetv1beta1.ExternalRolloutStrategyType {
+		// For external rollout strategy, if clusters observe different resource snapshot versions,
+		// we set RolloutStarted to Unknown without any other conditions since we do not know exactly which version is rolling out.
+		// We also need to reset ObservedResourceIndex and selectedResources.
+		rolloutStartedUnknown, err := r.determineRolloutStateForPlacementWithExternalRolloutStrategy(ctx, placementObj, selected, allRPS, selectedResourceIDs)
+		if err != nil || rolloutStartedUnknown {
+			return true, err
 		}
-
-		setCRPConditions(crp, allRPS, rpsSetCondTypeCounter, expectedCondTypes)
 	}
-
+	setCRPConditions(placementObj, allRPS, rpsSetCondTypeCounter, expectedCondTypes)
+	klog.V(2).InfoS("Updated placement status for the entire placement", "placement", klog.KObj(placementObj))
 	return true, nil
 }
 
-// TODO: This currently only works for cluster-scoped placements due to condition type differences
+// isPolicySelectingNoClusters checks if the given placement policy is selecting no clusters by design.
+func isPolicySelectingNoClusters(policy *fleetv1beta1.PlacementPolicy) bool {
+	if policy == nil {
+		return false
+	}
+	return (policy.PlacementType == fleetv1beta1.PickNPlacementType && *policy.NumberOfClusters == 0) || (policy.PlacementType == fleetv1beta1.PickFixedPlacementType && len(policy.ClusterNames) == 0)
+}
+
+// determineRolloutStateForPlacementWithExternalRolloutStrategy checks the rollout state for the placement with external rollout strategy.
 func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrategy(
 	ctx context.Context,
 	placementObj fleetv1beta1.PlacementObj,
@@ -1100,7 +1102,7 @@ func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrateg
 		placementStatus.ObservedResourceIndex = ""
 		placementStatus.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
 		placementObj.SetConditions(metav1.Condition{
-			Type:               string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
+			Type:               getPlacementRolloutStartedConditionType(placementObj),
 			Status:             metav1.ConditionUnknown,
 			Reason:             condition.RolloutControlledByExternalControllerReason,
 			Message:            "Rollout is controlled by an external controller and different resource snapshot versions are observed across clusters",
@@ -1121,7 +1123,7 @@ func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrateg
 		placementStatus.ObservedResourceIndex = ""
 		placementStatus.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
 		placementObj.SetConditions(metav1.Condition{
-			Type:               string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
+			Type:               getPlacementRolloutStartedConditionType(placementObj),
 			Status:             metav1.ConditionUnknown,
 			Reason:             condition.RolloutControlledByExternalControllerReason,
 			Message:            "Rollout is controlled by an external controller and no resource snapshot name is observed across clusters, probably rollout has not started yet",
@@ -1163,7 +1165,7 @@ func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrateg
 			klog.V(2).InfoS("Placement has External rollout strategy and some cluster is in RolloutStarted Unknown state, set RolloutStarted condition to Unknown",
 				"clusterName", allRPS[i].ClusterName, "observedResourceIndex", observedResourceIndex, "placement", klog.KObj(placementObj))
 			placementObj.SetConditions(metav1.Condition{
-				Type:               string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
+				Type:               getPlacementRolloutStartedConditionType(placementObj),
 				Status:             metav1.ConditionUnknown,
 				Reason:             condition.RolloutControlledByExternalControllerReason,
 				Message:            fmt.Sprintf("Rollout is controlled by an external controller and cluster %s is in RolloutStarted Unknown state", allRPS[i].ClusterName),
@@ -1179,6 +1181,28 @@ func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrateg
 	return false, nil
 }
 
+// TODO: determine the actual condition type for RP
+// getPlacementScheduledConditionType returns the appropriate scheduled condition type based on the placement type.
+func getPlacementScheduledConditionType(placementObj fleetv1beta1.PlacementObj) string {
+	if placementObj.GetNamespace() == "" {
+		// Cluster-scoped placement
+		return string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType)
+	}
+	// Namespace-scoped placement
+	return string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType)
+}
+
+// TODO: determine the actual condition type for RP
+// getPlacementRolloutStartedConditionType returns the appropriate rollout started condition type based on the placement type.
+func getPlacementRolloutStartedConditionType(placementObj fleetv1beta1.PlacementObj) string {
+	if placementObj.GetNamespace() == "" {
+		// Cluster-scoped placement
+		return string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType)
+	}
+	// Namespace-scoped placement
+	return string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType)
+}
+
 func buildScheduledCondition(placementObj fleetv1beta1.PlacementObj, latestSchedulingPolicySnapshot fleetv1beta1.PolicySnapshotObj) metav1.Condition {
 	scheduledCondition := latestSchedulingPolicySnapshot.GetCondition(string(fleetv1beta1.PolicySnapshotScheduled))
 
@@ -1191,15 +1215,15 @@ func buildScheduledCondition(placementObj fleetv1beta1.PlacementObj, latestSched
 		scheduledCondition.Status == metav1.ConditionUnknown {
 		return metav1.Condition{
 			Status:             metav1.ConditionUnknown,
-			Type:               string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType),
+			Type:               getPlacementScheduledConditionType(placementObj),
 			Reason:             condition.SchedulingUnknownReason,
 			Message:            "Scheduling has not completed",
 			ObservedGeneration: placementObj.GetGeneration(),
 		}
 	}
 	return metav1.Condition{
 		Status:             scheduledCondition.Status,
-		Type:               string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType),
+		Type:               getPlacementScheduledConditionType(placementObj),
 		Reason:             scheduledCondition.Reason,
 		Message:            scheduledCondition.Message,
 		ObservedGeneration: placementObj.GetGeneration(),

pkg/controllers/clusterresourceplacement/placement_status_test.go

@@ -962,7 +962,6 @@ func TestSetPlacementStatus(t *testing.T) {
 				},
 			},
 		},
-		// TODO special handling when selected cluster is 0
 		{
 			name: "the placement scheduling succeeded when numberOfClusters is 0",
 			policy: &fleetv1beta1.PlacementPolicy{
@@ -1039,7 +1038,7 @@ func TestSetPlacementStatus(t *testing.T) {
 					},
 				},
 			},
-			want: false,
+			want: true,
 			wantStatus: &fleetv1beta1.PlacementStatus{
 				SelectedResources:     selectedResources,
 				ObservedResourceIndex: "0",
@@ -1055,6 +1054,124 @@ func TestSetPlacementStatus(t *testing.T) {
 				PlacementStatuses: []fleetv1beta1.ResourcePlacementStatus{},
 			},
 		},
+
+		{
+			name: "the placement scheduling succeeded when numberOfClusters is 0 but user actually wants to select some",
+			policy: &fleetv1beta1.PlacementPolicy{
+				PlacementType:    fleetv1beta1.PickNPlacementType,
+				NumberOfClusters: ptr.To(int32(3)),
+			},
+			latestPolicySnapshot: &fleetv1beta1.ClusterSchedulingPolicySnapshot{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testCRPName, 0),
+					Labels: map[string]string{
+						fleetv1beta1.PolicyIndexLabel:       "0",
+						fleetv1beta1.IsLatestSnapshotLabel:  "true",
+						fleetv1beta1.PlacementTrackingLabel: testCRPName,
+					},
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							Name:               testCRPName,
+							BlockOwnerDeletion: ptr.To(true),
+							Controller:         ptr.To(true),
+							APIVersion:         fleetAPIVersion,
+							Kind:               "ClusterResourcePlacement",
+						},
+					},
+					Annotations: map[string]string{
+						fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(0),
+					},
+					Generation: 1,
+				},
+				Status: fleetv1beta1.SchedulingPolicySnapshotStatus{
+					ObservedCRPGeneration: crpGeneration,
+					Conditions: []metav1.Condition{
+						{
+							Status:             metav1.ConditionFalse,
+							Type:               string(fleetv1beta1.PolicySnapshotScheduled),
+							Reason:             "Scheduled not met",
+							Message:            "message",
+							ObservedGeneration: 1,
+						},
+					},
+					ClusterDecisions: []fleetv1beta1.ClusterDecision{
+						{
+							ClusterName: "member-1",
+							Selected:    false,
+							Reason:      "filtered",
+						},
+						{
+							ClusterName: "member-2",
+							Selected:    false,
+							Reason:      "filtered",
+						},
+					},
+				},
+			},
+			latestResourceSnapshot: &fleetv1beta1.ClusterResourceSnapshot{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf(fleetv1beta1.ResourceSnapshotNameFmt, testCRPName, 0),
+					Labels: map[string]string{
+						fleetv1beta1.ResourceIndexLabel:     "0",
+						fleetv1beta1.PlacementTrackingLabel: testCRPName,
+						fleetv1beta1.IsLatestSnapshotLabel:  "true",
+					},
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							Name:               testCRPName,
+							BlockOwnerDeletion: ptr.To(true),
+							Controller:         ptr.To(true),
+							APIVersion:         fleetAPIVersion,
+							Kind:               "ClusterResourcePlacement",
+						},
+					},
+					Annotations: map[string]string{
+						fleetv1beta1.ResourceGroupHashAnnotation:         "hash",
+						fleetv1beta1.NumberOfResourceSnapshotsAnnotation: "0",
+					},
+				},
+			},
+			want: false,
+			wantStatus: &fleetv1beta1.PlacementStatus{
+				SelectedResources:     selectedResources,
+				ObservedResourceIndex: "0",
+				Conditions: []metav1.Condition{
+					{
+						Status:             metav1.ConditionFalse,
+						Type:               string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType),
+						Reason:             "Scheduled not met",
+						ObservedGeneration: crpGeneration,
+						LastTransitionTime: metav1.NewTime(currentTime),
+					},
+				},
+				PlacementStatuses: []fleetv1beta1.ResourcePlacementStatus{
+					{
+						Conditions: []metav1.Condition{
+							{
+								Status:             metav1.ConditionFalse,
+								Type:               string(fleetv1beta1.ResourceScheduledConditionType),
+								Reason:             condition.ResourceScheduleFailedReason,
+								ObservedGeneration: crpGeneration,
+								LastTransitionTime: metav1.NewTime(currentTime),
+								Message:            "filtered",
+							},
+						},
+					},
+					{
+						Conditions: []metav1.Condition{
+							{
+								Status:             metav1.ConditionFalse,
+								Type:               string(fleetv1beta1.ResourceScheduledConditionType),
+								Reason:             condition.ResourceScheduleFailedReason,
+								ObservedGeneration: crpGeneration,
+								LastTransitionTime: metav1.NewTime(currentTime),
+								Message:            "filtered",
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "the placement is completed with clusterResourceBindings and works",
 			policy: &fleetv1beta1.PlacementPolicy{

pkg/controllers/rollout/controller.go

@@ -386,7 +386,6 @@ func (r *Reconciler) pickBindingsToRoll(
 		bindingSpec := binding.GetBindingSpec()
 		switch bindingSpec.State {
 		case placementv1beta1.BindingStateUnscheduled:
-			// Use updated interface-based bindingutils functions
 			if bindingutils.HasBindingFailed(binding) {
 				klog.V(2).InfoS("Found a failed to be ready unscheduled binding", "placement", placementKObj, "binding", bindingKObj)
 			} else if !bindingutils.IsBindingDiffReported(binding) {
@@ -501,6 +500,7 @@ func determineBindingsToUpdate(
 	readyBindings, canBeReadyBindings, canBeUnavailableBindings []placementv1beta1.BindingObj,
 ) ([]toBeUpdatedBinding, []toBeUpdatedBinding) {
 	toBeUpdatedBindingList := make([]toBeUpdatedBinding, 0)
+	// TODO: Fix the bug that we don't shrink to zero when there are bindings that are not
 	// calculate the max number of bindings that can be unavailable according to user specified maxUnavailable
 	maxNumberToRemove := calculateMaxToRemove(placementObj, targetNumber, readyBindings, canBeUnavailableBindings)
 	// we can still update the bindings that are failed to apply already regardless of the maxNumberToRemove

pkg/scheduler/framework/framework.go

@@ -384,7 +384,7 @@ var removeFinalizerAndUpdate = func(ctx context.Context, hubClient client.Client
 	controllerutil.RemoveFinalizer(binding, placementv1beta1.SchedulerBindingCleanupFinalizer)
 	err := hubClient.Update(ctx, binding, &client.UpdateOptions{})
 	if err == nil {
-		klog.V(2).InfoS("Removed scheduler CRB cleanup finalizer", "binding", klog.KObj(binding))
+		klog.V(2).InfoS("Removed scheduler binding cleanup finalizer", "binding", klog.KObj(binding))
 	}
 	return err
 }

test/e2e/placement_pickn_test.go

@@ -492,7 +492,7 @@ var _ = Describe("placing resources using a CRP of PickN placement", func() {
 		})
 	})
 
-	Context("downscaling to zero", Ordered, func() {
+	FContext("downscaling to zero", Ordered, func() {
 		crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
 
 		BeforeAll(func() {