adjust the code

Signed-off-by: Ryan Zhang <[email protected]>

Author: Ryan Zhang

apis/placement/v1beta1/commons.go

@@ -109,6 +109,7 @@ const (
 	ParentNamespaceLabel = fleetPrefix + "parent-placement-namespace"
 
 	// CRPGenerationAnnotation indicates the generation of the CRP from which an object is derived or last updated.
+	// TODO: rename this variable
 	CRPGenerationAnnotation = fleetPrefix + "CRP-generation"
 
 	// EnvelopeConfigMapAnnotation indicates the configmap is an envelope configmap containing resources we need to apply to the member cluster instead of the configMap itself.

pkg/controllers/clusterresourceplacement/controller.go

@@ -97,9 +97,9 @@ func appendFailedToScheduleResourcePlacementStatuses(
 	return allRPS
 }
 
-// determineExpectedCRPAndResourcePlacementStatusCondType determines the expected condition types for the CRP and resource placement statuses
+// determineExpectedPlacementAndResourcePlacementStatusCondType determines the expected condition types for the CRP and resource placement statuses
 // given the currently in-use apply strategy.
-func determineExpectedCRPAndResourcePlacementStatusCondType(placementObj fleetv1beta1.PlacementObj) []condition.ResourceCondition {
+func determineExpectedPlacementAndResourcePlacementStatusCondType(placementObj fleetv1beta1.PlacementObj) []condition.ResourceCondition {
 	placementSpec := placementObj.GetPlacementSpec()
 	switch {
 	case placementSpec.Strategy.ApplyStrategy == nil:
@@ -113,6 +113,7 @@ func determineExpectedCRPAndResourcePlacementStatusCondType(placementObj fleetv1
 
 // appendScheduledResourcePlacementStatuses appends the resource placement statuses for the
 // scheduled clusters to the list of all resource placement statuses.
+// it returns the updated list of resource placement statuses.
 func (r *Reconciler) appendScheduledResourcePlacementStatuses(
 	ctx context.Context,
 	allRPS []fleetv1beta1.ResourcePlacementStatus,
@@ -196,8 +197,9 @@ func (r *Reconciler) appendScheduledResourcePlacementStatuses(
 	return allRPS, rpsSetCondTypeCounter, nil
 }
 
-// setCRPConditions sets the CRP conditions based on the resource placement statuses.
-func setCRPConditions(
+// TODO: make this work with RP
+// setPlacementConditions currently sets the CRP conditions based on the resource placement statuses.
+func setPlacementConditions(
 	placementObj fleetv1beta1.PlacementObj,
 	allRPS []fleetv1beta1.ResourcePlacementStatus,
 	rpsSetCondTypeCounter [condition.TotalCondition][condition.TotalConditionStatus]int,

pkg/controllers/clusterresourceplacement/placement_status.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/clusterresourceplacement/placement_status_test.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 ready yet.
 	// 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/controllers/rollout/controller.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
 }
@@ -763,7 +763,7 @@ func (f *framework) updatePolicySnapshotStatusFromBindings(
 	policyRef := klog.KObj(policy)
 
 	// Retrieve the corresponding CRP generation.
-	observedCRPGeneration, err := annotations.ExtractObservedCRPGenerationFromPolicySnapshot(policy)
+	observedCRPGeneration, err := annotations.ExtractObservedPlacementGenerationFromPolicySnapshot(policy)
 	if err != nil {
 		klog.ErrorS(err, "Failed to retrieve CRP generation from annotation", "clusterSchedulingPolicySnapshot", policyRef)
 		return controller.NewUnexpectedBehaviorError(err)
@@ -1344,7 +1344,7 @@ func (f *framework) updatePolicySnapshotStatusForPickFixedPlacementType(
 	policyRef := klog.KObj(policy)
 
 	// Retrieve the corresponding CRP generation.
-	observedCRPGeneration, err := annotations.ExtractObservedCRPGenerationFromPolicySnapshot(policy)
+	observedCRPGeneration, err := annotations.ExtractObservedPlacementGenerationFromPolicySnapshot(policy)
 	if err != nil {
 		klog.ErrorS(err, "Failed to retrieve CRP generation from annotation", "clusterSchedulingPolicySnapshot", policyRef)
 		return controller.NewUnexpectedBehaviorError(err)

pkg/scheduler/framework/framework.go

@@ -42,34 +42,44 @@ func ExtractNumOfClustersFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotOb
 	return numOfClusters, nil
 }
 
-// ExtractObservedCRPGenerationFromPolicySnapshot extracts the observed CRP generation from policySnapshot.
-func ExtractObservedCRPGenerationFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotObj) (int64, error) {
-	crpGenerationStr, ok := policy.GetAnnotations()[fleetv1beta1.CRPGenerationAnnotation]
-	if !ok {
-		return 0, fmt.Errorf("cannot find annotation %s", fleetv1beta1.CRPGenerationAnnotation)
+// ExtractSubindexFromResourceSnapshot is a helper function to extract subindex from ResourceSnapshot objects.
+func ExtractSubindexFromResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
+	annotations := snapshot.GetAnnotations()
+	if annotations == nil {
+		return false, 0, nil
 	}
 
-	// Cast the annotation to an integer; throw an error if the cast cannot be completed or the value is negative.
-	observedCRPGeneration, err := strconv.Atoi(crpGenerationStr)
-	if err != nil || observedCRPGeneration < 0 {
-		return 0, fmt.Errorf("invalid annotation %s: %s is not a valid value: %w", fleetv1beta1.CRPGenerationAnnotation, crpGenerationStr, err)
+	subindexStr, exists := annotations[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
+	if !exists || subindexStr == "" {
+		return false, 0, nil
 	}
 
-	return int64(observedCRPGeneration), nil
+	subindex, err = strconv.Atoi(subindexStr)
+	if err != nil {
+		return false, 0, fmt.Errorf("invalid subindex annotation value %q: %w", subindexStr, err)
+	}
+
+	if subindex < 0 {
+		return false, 0, fmt.Errorf("subindex cannot be negative: %d", subindex)
+	}
+
+	return true, subindex, nil
 }
 
-// ExtractSubindexFromClusterResourceSnapshot extracts the subindex value from the annotations of a clusterResourceSnapshot.
-func ExtractSubindexFromClusterResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
-	subindexStr, ok := snapshot.GetAnnotations()[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
+// ExtractObservedPlacementGenerationFromPolicySnapshot extracts the observed placement generation from policySnapshot.
+func ExtractObservedPlacementGenerationFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotObj) (int64, error) {
+	placementGenerationStr, ok := policy.GetAnnotations()[fleetv1beta1.CRPGenerationAnnotation]
 	if !ok {
-		return false, -1, nil
+		return 0, fmt.Errorf("cannot find annotation %s", fleetv1beta1.CRPGenerationAnnotation)
 	}
-	subindex, err = strconv.Atoi(subindexStr)
-	if err != nil || subindex < 0 {
-		return true, -1, fmt.Errorf("invalid annotation %s: %s is invalid: %w", fleetv1beta1.SubindexOfResourceSnapshotAnnotation, subindexStr, err)
+
+	// Cast the annotation to an integer; throw an error if the cast cannot be completed or the value is negative.
+	observedplacementGeneration, err := strconv.Atoi(placementGenerationStr)
+	if err != nil || observedplacementGeneration < 0 {
+		return 0, fmt.Errorf("invalid annotation %s: %s is not a valid value: %w", fleetv1beta1.CRPGenerationAnnotation, placementGenerationStr, err)
 	}
 
-	return true, subindex, nil
+	return int64(observedplacementGeneration), nil
 }
 
 // ExtractNumberOfResourceSnapshotsFromResourceSnapshot extracts the number of clusterResourceSnapshots in a group from the master clusterResourceSnapshot.
@@ -125,27 +135,3 @@ func ParseResourceGroupHashFromAnnotation(resourceSnapshot fleetv1beta1.Resource
 	}
 	return v, nil
 }
-
-// ExtractSubindexFromResourceSnapshot is a helper function to extract subindex from ResourceSnapshot objects.
-func ExtractSubindexFromResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
-	annotations := snapshot.GetAnnotations()
-	if annotations == nil {
-		return false, 0, nil
-	}
-
-	subindexStr, exists := annotations[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
-	if !exists || subindexStr == "" {
-		return false, 0, nil
-	}
-
-	subindex, err = strconv.Atoi(subindexStr)
-	if err != nil {
-		return false, 0, fmt.Errorf("invalid subindex annotation value %q: %w", subindexStr, err)
-	}
-
-	if subindex < 0 {
-		return false, 0, fmt.Errorf("subindex cannot be negative: %d", subindex)
-	}
-
-	return true, subindex, nil
-}