From 3065b390dd16e5a30f3722097b79fa297042bc7d Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Mon, 15 Dec 2025 20:46:03 +0800 Subject: [PATCH 1/3] fix: bug fixes in rollout controller and work-generator Signed-off-by: Zhiying Lin --- pkg/controllers/rollout/controller.go | 4 +- pkg/controllers/workgenerator/controller.go | 7 + .../workgenerator/controller_test.go | 170 ++++++++++++++++++ 3 files changed, 180 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go index e383508c2..150ac88e4 100644 --- a/pkg/controllers/rollout/controller.go +++ b/pkg/controllers/rollout/controller.go @@ -145,7 +145,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim } // find the master resourceSnapshot. - masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.UncachedReader, placementKey) + // Use the cached client so that rollout controller and binding controller have the same view of the + // resourceSnapshots. + masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.Client, placementKey) if err != nil { klog.ErrorS(err, "Failed to find the masterResourceSnapshot for the placement", "placement", placementObjRef) diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index f334e86c1..545f793fa 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -673,6 +673,13 @@ func (r *Reconciler) syncApplyStrategy( // areAllWorkSynced checks if all the works are synced with the resource binding. func areAllWorkSynced(existingWorks map[string]*fleetv1beta1.Work, resourceBinding fleetv1beta1.BindingObj, _, _ string) bool { + // If there is no existing work, they are not synced. + // Even for the case where the resource snapshot has no selected resources, + // there should be one work created for the empty resource list. + if len(existingWorks) == 0 { + return false + } + // TODO: check resourceOverrideSnapshotHash and clusterResourceOverrideSnapshotHash after all the work has the ParentResourceOverrideSnapshotHashAnnotation and ParentClusterResourceOverrideSnapshotHashAnnotation resourceSnapshotName := resourceBinding.GetBindingSpec().ResourceSnapshotName for _, work := range existingWorks { diff --git a/pkg/controllers/workgenerator/controller_test.go b/pkg/controllers/workgenerator/controller_test.go index b6fff71c3..168930f5e 100644 --- a/pkg/controllers/workgenerator/controller_test.go +++ b/pkg/controllers/workgenerator/controller_test.go @@ -3751,6 +3751,176 @@ func TestSyncApplyStrategy(t *testing.T) { } } +func TestAreAllWorkSynced(t *testing.T) { + tests := map[string]struct { + existingWorks map[string]*fleetv1beta1.Work + resourceBinding fleetv1beta1.BindingObj + want bool + }{ + "returns false when no existing works": { + existingWorks: map[string]*fleetv1beta1.Work{}, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-1", + }, + }, + want: false, + }, + "returns true when all works are synced with annotation": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + }, + }, + }, + "work2": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work2", + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-1", + }, + }, + want: true, + }, + "returns false when works have different snapshot names": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + }, + }, + }, + "work2": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work2", + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-2", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-1", + }, + }, + want: false, + }, + "returns true when works are synced via label construction (fallback)": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "test-placement", + }, + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "test-placement-1-snapshot", + }, + }, + want: true, + }, + "returns false when label construction fallback fails": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "2", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "test-placement", + }, + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "test-placement-1-snapshot", + }, + }, + want: false, + }, + "returns false when works have no annotation and invalid label": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "invalid", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "test-placement", + }, + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "test-placement-1-snapshot", + }, + }, + want: false, + }, + "returns true for ResourceBinding (namespaced) with annotation": { + existingWorks: map[string]*fleetv1beta1.Work{ + "work1": { + ObjectMeta: metav1.ObjectMeta{ + Name: "work1", + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "test-snapshot-1", + }, + }, + }, + }, + resourceBinding: &fleetv1beta1.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + Namespace: "test-namespace", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "test-snapshot-1", + }, + }, + want: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := areAllWorkSynced(tt.existingWorks, tt.resourceBinding, "", "") + if got != tt.want { + t.Errorf("areAllWorkSynced() = %v, want %v", got, tt.want) + } + }) + } +} + func TestShouldIgnoreWork(t *testing.T) { tests := map[string]struct { enqueueCRP bool From ca29c4b34b7b7c0f7e3e3be8b8a86b312a7e81d2 Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Tue, 16 Dec 2025 16:09:03 +0800 Subject: [PATCH 2/3] address comments Signed-off-by: Zhiying Lin --- pkg/controllers/rollout/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go index 150ac88e4..9c53abad0 100644 --- a/pkg/controllers/rollout/controller.go +++ b/pkg/controllers/rollout/controller.go @@ -145,7 +145,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim } // find the master resourceSnapshot. - // Use the cached client so that rollout controller and binding controller have the same view of the + // Use the cached client so that rollout controller and work-generator have the same view of the // resourceSnapshots. masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.Client, placementKey) if err != nil { From 47fb1ddff178f2b5db551d487889f0643318370e Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Tue, 16 Dec 2025 16:20:14 +0800 Subject: [PATCH 3/3] fix the comment Signed-off-by: Zhiying Lin --- pkg/controllers/rollout/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go index 9c53abad0..0854abb5c 100644 --- a/pkg/controllers/rollout/controller.go +++ b/pkg/controllers/rollout/controller.go @@ -146,7 +146,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim // find the master resourceSnapshot. // Use the cached client so that rollout controller and work-generator have the same view of the - // resourceSnapshots. + // resourceSnapshots in order to reduce the possibility of missing resourceSnapshots in work-generator. masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.Client, placementKey) if err != nil { klog.ErrorS(err, "Failed to find the masterResourceSnapshot for the placement",