feat: handle namespace resource selector change CRPS (#240)

Author: Arvindthiru

apis/placement/v1beta1/clusterresourceplacement_types.go

@@ -1287,6 +1287,17 @@ const (
 	//   clusters, or an error has occurred.
 	// * Unknown: Fleet has not finished processing the diff reporting yet.
 	ClusterResourcePlacementDiffReportedConditionType ClusterResourcePlacementConditionType = "ClusterResourcePlacementDiffReported"
+
+	// ClusterResourcePlacementStatusSyncedConditionType indicates whether Fleet has successfully
+	// created or updated the ClusterResourcePlacementStatus object in the target namespace when
+	// StatusReportingScope is NamespaceAccessible.
+	//
+	// It can have the following condition statuses:
+	// * True: Fleet has successfully created or updated the ClusterResourcePlacementStatus object
+	//   in the target namespace.
+	// * False: Fleet has failed to create or update the ClusterResourcePlacementStatus object
+	//   in the target namespace.
+	ClusterResourcePlacementStatusSyncedConditionType ClusterResourcePlacementConditionType = "ClusterResourcePlacementStatusSynced"
 )
 
 // ResourcePlacementConditionType defines a specific condition of a resource placement object.

pkg/controllers/placement/controller.go

@@ -142,6 +142,19 @@ func (r *Reconciler) handleUpdate(ctx context.Context, placementObj fleetv1beta1
 		}
 	}
 
+	// Validate namespace selector consistency for NamespaceAccessible CRPs.
+	if isNamespaceAccessibleCRP(placementObj) {
+		isValid, err := r.validateNamespaceSelectorConsistency(ctx, placementObj)
+		if err != nil {
+			klog.V(2).ErrorS(err, "Namespace resource selector validation failed for NamespaceAccessible CRP", "placement", placementKObj)
+			return ctrl.Result{}, err
+		}
+		if !isValid {
+			klog.V(2).InfoS("Invalid Namespace resource selector specified for NamespaceAccessible CRP, stopping reconciliation", "placement", placementKObj)
+			return ctrl.Result{}, nil
+		}
+	}
+
 	// validate the resource selectors first before creating any snapshot
 	envelopeObjCount, selectedResources, selectedResourceIDs, err := r.selectResourcesForPlacement(placementObj)
 	if err != nil {
@@ -160,16 +173,17 @@ func (r *Reconciler) handleUpdate(ctx context.Context, placementObj fleetv1beta1
 		}
 		placementObj.SetConditions(scheduleCondition)
 
-		// Sync ClusterResourcePlacementStatus object if StatusReportingScope is NamespaceAccessible
-		if syncErr := r.syncClusterResourcePlacementStatus(ctx, placementObj); syncErr != nil {
-			klog.ErrorS(syncErr, "Failed to sync ClusterResourcePlacementStatus", "placement", placementKObj)
-			return ctrl.Result{}, syncErr
-		}
-
 		if updateErr := r.Client.Status().Update(ctx, placementObj); updateErr != nil {
 			klog.ErrorS(updateErr, "Failed to update the status", "placement", placementKObj)
 			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(updateErr)
 		}
+		klog.V(2).InfoS("Updated the placement status with scheduled condition", "placement", placementKObj)
+
+		if isNamespaceAccessibleCRP(placementObj) {
+			if err := r.handleNamespaceAccessibleCRP(ctx, placementObj); err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 
 		// no need to retry faster, the user needs to fix the resource selectors
 		return ctrl.Result{RequeueAfter: controllerResyncPeriod}, nil
@@ -212,18 +226,18 @@ func (r *Reconciler) handleUpdate(ctx context.Context, placementObj fleetv1beta1
 		return ctrl.Result{}, err
 	}
 
-	// Sync ClusterResourcePlacementStatus object if StatusReportingScope is NamespaceAccessible
-	if err := r.syncClusterResourcePlacementStatus(ctx, placementObj); err != nil {
-		klog.ErrorS(err, "Failed to sync ClusterResourcePlacementStatus", "placement", placementKObj)
-		return ctrl.Result{}, err
-	}
-
 	if err := r.Client.Status().Update(ctx, placementObj); err != nil {
 		klog.ErrorS(err, "Failed to update the status", "placement", placementKObj)
 		return ctrl.Result{}, err
 	}
 	klog.V(2).InfoS("Updated the placement status", "placement", placementKObj)
 
+	if isNamespaceAccessibleCRP(placementObj) {
+		if err := r.handleNamespaceAccessibleCRP(ctx, placementObj); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	// We skip checking the last resource condition (available) because it will be covered by checking isRolloutCompleted func.
 	for i := condition.RolloutStartedCondition; i < condition.TotalCondition-1; i++ {
 		var oldCond, newCond *metav1.Condition

pkg/controllers/placement/controller_integration_test.go

@@ -39,7 +39,7 @@ import (
 	"github.com/kubefleet-dev/kubefleet/pkg/utils"
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/resource"
-	statussyncutils "github.com/kubefleet-dev/kubefleet/test/utils/crpstatussync"
+	"github.com/kubefleet-dev/kubefleet/test/utils/crpstatussync"
 	metricsUtils "github.com/kubefleet-dev/kubefleet/test/utils/metrics"
 )
 
@@ -2189,26 +2189,31 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 		})
 
 		It("Should handle missing target namespace", func() {
-			By("Validate CRP keeps retrying due to missing namespace (status remain nil)")
+			By("Validate CRP status includes StatusSynced condition")
 			wantCRP := &placementv1beta1.ClusterResourcePlacement{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       testCRPName,
 					Finalizers: []string{placementv1beta1.PlacementCleanupFinalizer},
 				},
 				Spec: crp.Spec,
+				Status: placementv1beta1.PlacementStatus{
+					ObservedResourceIndex: "0",
+					Conditions: []metav1.Condition{
+						{
+							Status: metav1.ConditionUnknown,
+							Type:   string(placementv1beta1.ClusterResourcePlacementScheduledConditionType),
+							Reason: condition.SchedulingUnknownReason,
+						},
+						{
+							Status: metav1.ConditionFalse,
+							Type:   string(placementv1beta1.ClusterResourcePlacementStatusSyncedConditionType),
+							Reason: condition.StatusSyncFailedReason,
+						},
+					},
+				},
 			}
-			retrieveAndValidateClusterResourcePlacement(testCRPName, wantCRP)
-			// Ensure that CRP status remains nil as the controller keeps retrying.
-			Consistently(func() error {
-				gotCRP := &placementv1beta1.ClusterResourcePlacement{}
-				if err := k8sClient.Get(ctx, types.NamespacedName{Name: testCRPName}, gotCRP); err != nil {
-					return err
-				}
-				if diff := cmp.Diff(wantCRP, gotCRP, cmpCRPOptions); diff != "" {
-					return fmt.Errorf("clusterResourcePlacement mismatch (-want, +got) :\n%s", diff)
-				}
-				return nil
-			}, consistentlyTimeout, interval).Should(Succeed(), "ClusterResourcePlacement status must be nil")
+			retrieveAndValidateClusterResourcePlacement(crp.Name, wantCRP)
+
 			// Namespace doesn't exist, so no CRPS should be created - sanity check.
 			By("Ensure no ClusterResourcePlacementStatus is created in the nonexistent namespace")
 			Consistently(func() bool {
@@ -2250,7 +2255,7 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 						{
 							Group:   corev1.GroupName,
 							Version: "v1",
-							Kind:    "InvalidKind", // Invalid kind to trigger user error
+							Kind:    "InvalidKind", // Invalid kind to trigger user error.
 							Name:    "invalid-resource",
 						},
 					},
@@ -2268,7 +2273,7 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 				},
 			}
 			Expect(k8sClient.Delete(ctx, crp)).Should(Succeed())
-			retrieveAndValidateCRPDeletion(gotCRP)
+			retrieveAndValidateCRPDeletion(crp)
 
 			// Need to manually clean up CRPS in test environment https://book.kubebuilder.io/reference/envtest#testing-considerations.
 			By("Deleting crps")
@@ -2293,8 +2298,8 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 			Expect(k8sClient.Delete(ctx, namespace)).Should(Succeed())
 		})
 
-		It("Should handle invalid resource selector", func() {
-			By("Validate CRP status")
+		It("Should handle invalid resource selector and create CRPS", func() {
+			By("Validate CRP status includes both Scheduled and StatusSynced conditions")
 			wantCRP := &placementv1beta1.ClusterResourcePlacement{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       testCRPName,
@@ -2303,20 +2308,25 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 				Spec: crp.Spec,
 				Status: placementv1beta1.PlacementStatus{
 					Conditions: []metav1.Condition{
+						// ObservedResourceIndex is not set because resource selector is invalid.
 						{
 							Status: metav1.ConditionFalse,
 							Type:   string(placementv1beta1.ClusterResourcePlacementScheduledConditionType),
 							Reason: condition.InvalidResourceSelectorsReason,
 						},
+						{
+							Status: metav1.ConditionTrue,
+							Type:   string(placementv1beta1.ClusterResourcePlacementStatusSyncedConditionType),
+							Reason: condition.StatusSyncSucceededReason,
+						},
 					},
 				},
 			}
-			gotCRP = retrieveAndValidateClusterResourcePlacement(crp.Name, wantCRP)
+			retrieveAndValidateClusterResourcePlacement(crp.Name, wantCRP)
 
-			By("Validate CRPS status", func() {
-				crpsMatchesActual := statussyncutils.CRPSStatusMatchesCRPActual(ctx, k8sClient, testCRPName, namespaceName)
-				Eventually(crpsMatchesActual, eventuallyTimeout, interval).Should(Succeed(), "ClusterResourcePlacementStatus should match expected structure and CRP status")
-			})
+			By("Validate CRPS is created and matches CRP status")
+			crpsMatchesActual := crpstatussync.CRPSStatusMatchesCRPActual(ctx, k8sClient, testCRPName, namespaceName)
+			Eventually(crpsMatchesActual, eventuallyTimeout, interval).Should(Succeed(), "ClusterResourcePlacementStatus should be created and match expected structure")
 		})
 	})
 })