feat: placement controller does not create resource snapshot when External rollout strategy (#465)

Author: britaniar

pkg/controllers/placement/controller.go

@@ -213,31 +213,12 @@ func (r *Reconciler) handleUpdate(ctx context.Context, placementObj fleetv1beta1
 		return ctrl.Result{}, err
 	}
 
-	createResourceSnapshotRes, latestResourceSnapshot, err := r.ResourceSnapshotResolver.GetOrCreateResourceSnapshot(ctx, placementObj, envelopeObjCount,
-		&fleetv1beta1.ResourceSnapshotSpec{SelectedResources: selectedResources}, int(revisionLimit))
+	createResourceSnapshotRes, latestResourceSnapshot, selectedResourceIDs, err := r.handleResourceSnapshotByStrategy(
+		ctx, placementObj, envelopeObjCount, selectedResources, selectedResourceIDs, int(revisionLimit))
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// We don't requeue the request here immediately so that placement can keep tracking the rollout status.
-	if createResourceSnapshotRes.RequeueAfter > 0 {
-		latestResourceSnapshotKObj := klog.KObj(latestResourceSnapshot)
-		// We cannot create the resource snapshot immediately because of the resource snapshot creation interval.
-		// Rebuild the seletedResourceIDs using the latestResourceSnapshot.
-		latestResourceSnapshotIndex, err := labels.ExtractResourceIndexFromResourceSnapshot(latestResourceSnapshot)
-		if err != nil {
-			klog.ErrorS(err, "Failed to extract the resource index from the resourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj)
-			return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err)
-		}
-		placementKey := controller.GetObjectKeyFromNamespaceName(placementObj.GetNamespace(), placementObj.GetName())
-		selectedResourceIDs, err = controller.CollectResourceIdentifiersUsingMasterResourceSnapshot(ctx, r.Client, placementKey, latestResourceSnapshot, strconv.Itoa(latestResourceSnapshotIndex))
-		if err != nil {
-			klog.ErrorS(err, "Failed to collect resource identifiers from the resourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj)
-			return ctrl.Result{}, err
-		}
-		klog.V(2).InfoS("Fetched the selected resources from the lastestResourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj, "generation", placementObj.GetGeneration())
-	}
-
 	// isScheduleFullfilled is to indicate whether we need to requeue the placement request to track the rollout status.
 	isScheduleFullfilled, err := r.setPlacementStatus(ctx, placementObj, selectedResourceIDs, latestSchedulingPolicySnapshot, latestResourceSnapshot)
 	if err != nil {
@@ -315,6 +296,55 @@ func (r *Reconciler) handleUpdate(ctx context.Context, placementObj fleetv1beta1
 	return ctrl.Result{RequeueAfter: controllerResyncPeriod}, nil
 }
 
+// handleResourceSnapshotByStrategy handles resource snapshot resolution based on rollout strategy.
+// For External rollout strategy, it only fetches the existing snapshot (can be nil).
+// For other strategies, it creates or gets a resource snapshot and may update selectedResourceIDs if requeue is needed.
+func (r *Reconciler) handleResourceSnapshotByStrategy(
+	ctx context.Context,
+	placementObj fleetv1beta1.PlacementObj,
+	envelopeObjCount int,
+	selectedResources []fleetv1beta1.ResourceContent,
+	selectedResourceIDs []fleetv1beta1.ResourceIdentifier,
+	revisionHistoryLimit int,
+) (ctrl.Result, fleetv1beta1.ResourceSnapshotObj, []fleetv1beta1.ResourceIdentifier, error) {
+	placementKObj := klog.KObj(placementObj)
+	placementSpec := placementObj.GetPlacementSpec()
+
+	// For External rollout strategy, the placement controller should not create new resource snapshots.
+	// The external controller (e.g., UpdateRun controller) is responsible for creating them.
+	if placementSpec.Strategy.Type == fleetv1beta1.ExternalRolloutStrategyType {
+		// latestResourceSnapshot is nil for External strategy - the external controller will create it.
+		klog.V(2).InfoS("Using external rollout strategy, skipping resource snapshot creation", "placement", placementKObj)
+		return ctrl.Result{}, nil, selectedResourceIDs, nil
+	}
+
+	createResourceSnapshotRes, latestResourceSnapshot, err := r.ResourceSnapshotResolver.GetOrCreateResourceSnapshot(ctx, placementObj, envelopeObjCount,
+		&fleetv1beta1.ResourceSnapshotSpec{SelectedResources: selectedResources}, revisionHistoryLimit)
+	if err != nil {
+		return ctrl.Result{}, nil, selectedResourceIDs, err
+	}
+
+	// We don't requeue the request here immediately so that placement can keep tracking the rollout status.
+	if createResourceSnapshotRes.RequeueAfter > 0 {
+		latestResourceSnapshotKObj := klog.KObj(latestResourceSnapshot)
+		// We cannot create the resource snapshot immediately because of the resource snapshot creation interval.
+		// Rebuild the selectedResourceIDs using the latestResourceSnapshot.
+		latestResourceSnapshotIndex, err := labels.ExtractResourceIndexFromResourceSnapshot(latestResourceSnapshot)
+		if err != nil {
+			klog.ErrorS(err, "Failed to extract the resource index from the resourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj)
+			return ctrl.Result{}, nil, selectedResourceIDs, controller.NewUnexpectedBehaviorError(err)
+		}
+		placementKey := controller.GetObjectKeyFromNamespaceName(placementObj.GetNamespace(), placementObj.GetName())
+		selectedResourceIDs, err = controller.CollectResourceIdentifiersUsingMasterResourceSnapshot(ctx, r.Client, placementKey, latestResourceSnapshot, strconv.Itoa(latestResourceSnapshotIndex))
+		if err != nil {
+			klog.ErrorS(err, "Failed to collect resource identifiers from the resourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj)
+			return ctrl.Result{}, nil, selectedResourceIDs, err
+		}
+		klog.V(2).InfoS("Fetched the selected resources from the latestResourceSnapshot", "placement", placementKObj, "resourceSnapshot", latestResourceSnapshotKObj, "generation", placementObj.GetGeneration())
+	}
+	return createResourceSnapshotRes, latestResourceSnapshot, selectedResourceIDs, nil
+}
+
 func (r *Reconciler) getOrCreateSchedulingPolicySnapshot(ctx context.Context, placementObj fleetv1beta1.PlacementObj, revisionHistoryLimit int) (fleetv1beta1.PolicySnapshotObj, error) {
 	placementKObj := klog.KObj(placementObj)
 	placementSpec := placementObj.GetPlacementSpec()
@@ -571,7 +601,12 @@ func (r *Reconciler) setPlacementStatus(
 	scheduledCondition := buildScheduledCondition(placementObj, latestSchedulingPolicySnapshot)
 	placementObj.SetConditions(scheduledCondition)
 	// set ObservedResourceIndex from the latest resource snapshot's resource index label, before we set Synchronized, Applied conditions.
-	placementStatus.ObservedResourceIndex = latestResourceSnapshot.GetLabels()[fleetv1beta1.ResourceIndexLabel]
+	// For External rollout strategy, latestResourceSnapshot can be nil if no snapshot has been created yet by the external controller.
+	if latestResourceSnapshot != nil {
+		placementStatus.ObservedResourceIndex = latestResourceSnapshot.GetLabels()[fleetv1beta1.ResourceIndexLabel]
+	} else {
+		placementStatus.ObservedResourceIndex = ""
+	}
 
 	// When scheduledCondition is unknown, appliedCondition should be unknown too.
 	// Note: If the scheduledCondition is failed, it means the placement requirement cannot be satisfied fully. For example,

pkg/controllers/placement/controller_integration_test.go

@@ -2029,8 +2029,14 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 			By("Check clusterSchedulingPolicySnapshot")
 			gotPolicySnapshot = checkClusterSchedulingPolicySnapshot()
 
-			By("Check clusterResourceSnapshot")
-			gotResourceSnapshot = checkClusterResourceSnapshot()
+			By("Check that no clusterResourceSnapshot is created (External rollout strategy)")
+			// For External rollout strategy, the placement controller should NOT create resource snapshots.
+			// The external controller is responsible for creating them.
+			resourceSnapshotList := &placementv1beta1.ClusterResourceSnapshotList{}
+			Consistently(func() int {
+				Expect(k8sClient.List(ctx, resourceSnapshotList, client.MatchingLabels{placementv1beta1.PlacementTrackingLabel: crp.Name})).Should(Succeed())
+				return len(resourceSnapshotList.Items)
+			}, consistentlyTimeout, interval).Should(Equal(0), "Resource snapshot should not be created for External rollout strategy")
 
 			By("Validate CRP status")
 			wantCRP := &placementv1beta1.ClusterResourcePlacement{
@@ -2040,7 +2046,7 @@ var _ = Describe("Test ClusterResourcePlacement Controller", func() {
 				},
 				Spec: crp.Spec,
 				Status: placementv1beta1.PlacementStatus{
-					ObservedResourceIndex: "0",
+					ObservedResourceIndex: "",
 					Conditions: []metav1.Condition{
 						{
 							Status: metav1.ConditionUnknown,

pkg/controllers/placement/controller_test.go

@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"strconv"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
@@ -2390,3 +2391,211 @@ func TestDetermineRolloutStateForPlacementWithExternalRolloutStrategy(t *testing
 		})
 	}
 }
+
+func TestHandleResourceSnapshotByStrategy(t *testing.T) {
+	tests := []struct {
+		name                    string
+		crp                     *fleetv1beta1.ClusterResourcePlacement
+		existingSnapshots       []client.Object
+		selectedResources       []fleetv1beta1.ResourceContent
+		selectedResourceIDs     []fleetv1beta1.ResourceIdentifier
+		snapshotResolverConfig  *controller.ResourceSnapshotConfig // optional Config for the resolver
+		wantSnapshot            bool
+		wantSnapshotName        string
+		wantSelectedResourceIDs []fleetv1beta1.ResourceIdentifier
+		wantRequeueAfter        bool // true if we expect RequeueAfter > 0
+		wantErr                 bool
+	}{
+		{
+			name: "External rollout strategy with no existing snapshot",
+			crp: &fleetv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       testCRPName,
+					Generation: 1,
+				},
+				Spec: fleetv1beta1.PlacementSpec{
+					ResourceSelectors: []fleetv1beta1.ResourceSelectorTerm{
+						{
+							Group:   corev1.GroupName,
+							Version: "v1",
+							Kind:    "Namespace",
+						},
+					},
+					Strategy: fleetv1beta1.RolloutStrategy{
+						Type: fleetv1beta1.ExternalRolloutStrategyType,
+					},
+				},
+			},
+			existingSnapshots:       []client.Object{},
+			selectedResources:       []fleetv1beta1.ResourceContent{},
+			selectedResourceIDs:     []fleetv1beta1.ResourceIdentifier{{Kind: "Namespace", Name: "test"}},
+			wantSnapshot:            false,
+			wantSnapshotName:        "",
+			wantSelectedResourceIDs: []fleetv1beta1.ResourceIdentifier{{Kind: "Namespace", Name: "test"}},
+			wantRequeueAfter:        false,
+			wantErr:                 false,
+		},
+		{
+			name: "RollingUpdate strategy creates new snapshot when none exists",
+			crp: &fleetv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       testCRPName,
+					Generation: 1,
+				},
+				Spec: fleetv1beta1.PlacementSpec{
+					ResourceSelectors: []fleetv1beta1.ResourceSelectorTerm{
+						{
+							Group:   corev1.GroupName,
+							Version: "v1",
+							Kind:    "Namespace",
+						},
+					},
+					Strategy: fleetv1beta1.RolloutStrategy{
+						Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
+					},
+				},
+			},
+			existingSnapshots: []client.Object{},
+			selectedResources: []fleetv1beta1.ResourceContent{
+				{
+					RawExtension: runtime.RawExtension{
+						Raw: []byte(`{"apiVersion":"v1","kind":"Namespace","metadata":{"name":"test-ns"}}`),
+					},
+				},
+			},
+			selectedResourceIDs:     []fleetv1beta1.ResourceIdentifier{{Kind: "Namespace", Name: "test-ns"}},
+			wantSnapshot:            true,
+			wantSnapshotName:        fmt.Sprintf(fleetv1beta1.ResourceSnapshotNameFmt, testCRPName, 0),
+			wantSelectedResourceIDs: []fleetv1beta1.ResourceIdentifier{{Kind: "Namespace", Name: "test-ns"}},
+			wantRequeueAfter:        false,
+			wantErr:                 false,
+		},
+		{
+			name: "RollingUpdate strategy with different hash triggers requeue when interval configured",
+			crp: &fleetv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       testCRPName,
+					Generation: 1,
+				},
+				Spec: fleetv1beta1.PlacementSpec{
+					ResourceSelectors: []fleetv1beta1.ResourceSelectorTerm{
+						{
+							Group:   corev1.GroupName,
+							Version: "v1",
+							Kind:    "Namespace",
+						},
+					},
+					Strategy: fleetv1beta1.RolloutStrategy{
+						Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
+					},
+				},
+			},
+			existingSnapshots: []client.Object{
+				&fleetv1beta1.ClusterResourceSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              fmt.Sprintf(fleetv1beta1.ResourceSnapshotNameFmt, testCRPName, 0),
+						CreationTimestamp: metav1.Now(),
+						Labels: map[string]string{
+							fleetv1beta1.PlacementTrackingLabel: testCRPName,
+							fleetv1beta1.IsLatestSnapshotLabel:  strconv.FormatBool(true),
+							fleetv1beta1.ResourceIndexLabel:     "0",
+						},
+						Annotations: map[string]string{
+							fleetv1beta1.ResourceGroupHashAnnotation:         "old-hash-different-from-new",
+							fleetv1beta1.NumberOfResourceSnapshotsAnnotation: "1",
+						},
+					},
+					Spec: fleetv1beta1.ResourceSnapshotSpec{
+						SelectedResources: []fleetv1beta1.ResourceContent{
+							{
+								RawExtension: runtime.RawExtension{
+									Raw: []byte(`{"apiVersion":"v1","kind":"Namespace","metadata":{"name":"old-ns"}}`),
+								},
+							},
+						},
+					},
+				},
+			},
+			selectedResources: []fleetv1beta1.ResourceContent{
+				{
+					RawExtension: runtime.RawExtension{
+						Raw: []byte(`{"apiVersion":"v1","kind":"Namespace","metadata":{"name":"new-ns"}}`),
+					},
+				},
+			},
+			selectedResourceIDs:    []fleetv1beta1.ResourceIdentifier{{Kind: "Namespace", Name: "new-ns"}},
+			snapshotResolverConfig: controller.NewResourceSnapshotConfig(15*time.Second, 10*time.Second),
+			wantSnapshot:           true,
+			wantSnapshotName:       fmt.Sprintf(fleetv1beta1.ResourceSnapshotNameFmt, testCRPName, 0),
+			// When requeue is triggered, selectedResourceIDs are rebuilt from the existing snapshot
+			wantSelectedResourceIDs: []fleetv1beta1.ResourceIdentifier{
+				{
+					Group:   "",
+					Version: "v1",
+					Kind:    "Namespace",
+					Name:    "old-ns",
+				},
+			},
+			wantRequeueAfter: true,
+			wantErr:          false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			scheme := serviceScheme(t)
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(scheme).
+				WithObjects(tc.existingSnapshots...).
+				Build()
+			resolver := controller.NewResourceSnapshotResolver(fakeClient, scheme)
+			if tc.snapshotResolverConfig != nil {
+				resolver.Config = tc.snapshotResolverConfig
+			}
+			r := Reconciler{
+				Client:                   fakeClient,
+				Scheme:                   scheme,
+				ResourceSnapshotResolver: resolver,
+			}
+
+			gotResult, gotSnapshot, gotSelectedResourceIDs, gotErr := r.handleResourceSnapshotByStrategy(
+				context.Background(), tc.crp, 0, tc.selectedResources, tc.selectedResourceIDs, 10)
+
+			if (gotErr != nil) != tc.wantErr {
+				t.Errorf("handleResourceSnapshotByStrategy() error = %v, wantErr %v", gotErr, tc.wantErr)
+				return
+			}
+
+			if tc.wantSnapshot {
+				if gotSnapshot == nil {
+					t.Errorf("handleResourceSnapshotByStrategy() gotSnapshot = nil, want non-nil")
+					return
+				}
+				if gotSnapshot.GetName() != tc.wantSnapshotName {
+					t.Errorf("handleResourceSnapshotByStrategy() gotSnapshot.Name = %v, want %v", gotSnapshot.GetName(), tc.wantSnapshotName)
+				}
+			} else {
+				if gotSnapshot != nil {
+					t.Errorf("handleResourceSnapshotByStrategy() gotSnapshot = %v, want nil", gotSnapshot.GetName())
+				}
+			}
+
+			if diff := cmp.Diff(tc.wantSelectedResourceIDs, gotSelectedResourceIDs); diff != "" {
+				t.Errorf("handleResourceSnapshotByStrategy() selectedResourceIDs mismatch (-want, +got):\n%s", diff)
+			}
+
+			// Verify RequeueAfter behavior
+			gotRequeueAfter := gotResult.RequeueAfter > 0
+			if gotRequeueAfter != tc.wantRequeueAfter {
+				t.Errorf("handleResourceSnapshotByStrategy() gotResult.RequeueAfter > 0 = %v, want %v", gotRequeueAfter, tc.wantRequeueAfter)
+			}
+
+			// For External strategy, we always expect no requeue
+			if tc.crp.Spec.Strategy.Type == fleetv1beta1.ExternalRolloutStrategyType {
+				if gotResult.RequeueAfter != 0 {
+					t.Errorf("handleResourceSnapshotByStrategy() gotResult.RequeueAfter = %v, want 0 for External strategy", gotResult.RequeueAfter)
+				}
+			}
+		})
+	}
+}