From ce51051846db7deef18fd6a988584f5ca54de020 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 02:05:25 +0000 Subject: [PATCH 1/4] Initial plan From e06d02fcab7a20596ffee46768662c53929789c3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 02:14:58 +0000 Subject: [PATCH 2/4] fix: preserve CRP scheduled status on non-scheduling updates --- pkg/controllers/placement/controller.go | 14 --- pkg/controllers/placement/controller_test.go | 92 +++++++++++++++++-- .../placement/placement_status_test.go | 7 +- 3 files changed, 89 insertions(+), 24 deletions(-) diff --git a/pkg/controllers/placement/controller.go b/pkg/controllers/placement/controller.go index e6a80a899..eca209642 100644 --- a/pkg/controllers/placement/controller.go +++ b/pkg/controllers/placement/controller.go @@ -457,17 +457,6 @@ func (r *Reconciler) ensureLatestPolicySnapshot(ctx context.Context, placementOb latest.SetLabels(labels) needUpdate = true } - placementGeneration, err := annotations.ExtractObservedPlacementGenerationFromPolicySnapshot(latest) - if err != nil { - klog.ErrorS(err, "Failed to parse the placement generation from the annotations", "policySnapshot", latestKObj) - return controller.NewUnexpectedBehaviorError(err) - } - if placementGeneration != placementObj.GetGeneration() { - annotations := latest.GetAnnotations() - annotations[fleetv1beta1.CRPGenerationAnnotation] = strconv.FormatInt(placementObj.GetGeneration(), 10) - latest.SetAnnotations(annotations) - needUpdate = true - } // Handle NumberOfClusters annotation for selectN type placements placementSpec := placementObj.GetPlacementSpec() @@ -798,9 +787,6 @@ func buildScheduledCondition(placementObj fleetv1beta1.PlacementObj, latestSched if scheduledCondition == nil || // defensive check and not needed for now as the policySnapshot should be immutable. scheduledCondition.ObservedGeneration < latestSchedulingPolicySnapshot.GetGeneration() || - // We have numberOfCluster annotation added on the placement and it won't change the placement generation. - // So that we need to compare the placement observedCRPGeneration reported by the scheduler. - latestSchedulingPolicySnapshot.GetPolicySnapshotStatus().ObservedCRPGeneration < placementObj.GetGeneration() || scheduledCondition.Status == metav1.ConditionUnknown { return metav1.Condition{ Status: metav1.ConditionUnknown, diff --git a/pkg/controllers/placement/controller_test.go b/pkg/controllers/placement/controller_test.go index 026621671..d52ac0be2 100644 --- a/pkg/controllers/placement/controller_test.go +++ b/pkg/controllers/placement/controller_test.go @@ -322,6 +322,70 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, wantLatestSnapshotIndex: 0, }, + { + name: "crp policy has no change and generation alone does not update the policy snapshot", + policy: placementPolicyForTest(), + revisionHistoryLimit: &singleRevisionLimit, + policySnapshots: []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(3), + fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration - 1), + }, + }, + Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ + Policy: testPolicy, + PolicyHash: policyHash, + }, + }, + }, + wantPolicySnapshots: []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(3), + fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration - 1), + }, + }, + Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ + Policy: testPolicy, + PolicyHash: policyHash, + }, + }, + }, + wantLatestSnapshotIndex: 0, + }, { name: "crp policy has changed and there is no active snapshot", // It happens when last reconcile loop fails after setting the latest label to false and @@ -593,7 +657,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration), + fleetv1beta1.CRPGenerationAnnotation: "2", }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -710,7 +774,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration), + fleetv1beta1.CRPGenerationAnnotation: "2", }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -785,10 +849,12 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { tests := []struct { name string policySnapshots []fleetv1beta1.ClusterSchedulingPolicySnapshot + wantErr bool }{ { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "existing active policy snapshot does not have policyIndex label", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -811,6 +877,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "existing active policy snapshot has an invalid policyIndex label", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -834,6 +901,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "no active policy snapshot exists and policySnapshot with invalid policyIndex label", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -872,6 +940,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "multiple active policy snapshot exist", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -912,6 +981,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "no active policy snapshot exists and policySnapshot with invalid policyIndex label (negative value)", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -934,6 +1004,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "active policy snapshot exists and policySnapshot with invalid numberOfClusters annotation", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -966,6 +1037,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { { // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "no active policy snapshot exists and policySnapshot with invalid numberOfClusters annotation (negative)", + wantErr: true, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -995,8 +1067,8 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { }, }, { - // Should never hit this case unless there is a bug in the controller or customers manually modify the clusterPolicySnapshot. name: "active policy snapshot exists and policySnapshot without crp generation annotation", + wantErr: false, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ @@ -1046,11 +1118,17 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { Recorder: record.NewFakeRecorder(10), } _, err := r.getOrCreateSchedulingPolicySnapshot(ctx, crp, 1) - if err == nil { // if error is nil - t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want err") + if tc.wantErr { + if err == nil { // if error is nil + t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want err") + } + if !errors.Is(err, controller.ErrUnexpectedBehavior) { + t.Errorf("getOrCreateClusterResourceSnapshot() got %v, want %v type", err, controller.ErrUnexpectedBehavior) + } + return } - if !errors.Is(err, controller.ErrUnexpectedBehavior) { - t.Errorf("getOrCreateClusterResourceSnapshot() got %v, want %v type", err, controller.ErrUnexpectedBehavior) + if err != nil { + t.Fatalf("getOrCreateClusterResourceSnapshot() = %v, want nil", err) } }) } diff --git a/pkg/controllers/placement/placement_status_test.go b/pkg/controllers/placement/placement_status_test.go index 361af9a1f..e35bf36b4 100644 --- a/pkg/controllers/placement/placement_status_test.go +++ b/pkg/controllers/placement/placement_status_test.go @@ -428,7 +428,7 @@ func TestSetPlacementStatusForClusterResourcePlacement(t *testing.T) { }, }, { - name: "scheduler does not report the latest status for policy snapshot (annotation change)", + name: "placement generation changes without scheduling snapshot changes keeps the last scheduling result", latestPolicySnapshot: &fleetv1beta1.ClusterSchedulingPolicySnapshot{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testCRPName, 0), @@ -476,9 +476,10 @@ func TestSetPlacementStatusForClusterResourcePlacement(t *testing.T) { ObservedResourceIndex: "0", Conditions: []metav1.Condition{ { - Status: metav1.ConditionUnknown, + Status: metav1.ConditionTrue, Type: string(fleetv1beta1.ClusterResourcePlacementScheduledConditionType), - Reason: condition.SchedulingUnknownReason, + Reason: "Scheduled", + Message: "message", ObservedGeneration: crpGeneration, LastTransitionTime: metav1.NewTime(currentTime), }, From 90e341b435156334a26b90e594d3059ca89c5534 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 02:16:32 +0000 Subject: [PATCH 3/4] test: clarify placement snapshot generation coverage --- pkg/controllers/placement/controller_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/placement/controller_test.go b/pkg/controllers/placement/controller_test.go index d52ac0be2..de8e11525 100644 --- a/pkg/controllers/placement/controller_test.go +++ b/pkg/controllers/placement/controller_test.go @@ -133,6 +133,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { t.Fatalf("failed to create the policy hash: %v", err) } unspecifiedPolicyHash := []byte(fmt.Sprintf("%x", sha256.Sum256(jsonBytes))) + stalePlacementGeneration := strconv.Itoa(placementGeneration - 1) tests := []struct { name string policy *fleetv1beta1.PlacementPolicy @@ -346,7 +347,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration - 1), + fleetv1beta1.CRPGenerationAnnotation: stalePlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -375,7 +376,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration - 1), + fleetv1beta1.CRPGenerationAnnotation: stalePlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -1067,6 +1068,8 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { }, }, { + // This can happen for older snapshots created before the annotation existed; we now tolerate it + // because same-policy reconciles no longer require rewriting the CRP generation annotation. name: "active policy snapshot exists and policySnapshot without crp generation annotation", wantErr: false, policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{ From ea3bfa85d970bc511da343d307662b343054e17f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 02:18:41 +0000 Subject: [PATCH 4/4] test: tighten placement scheduling coverage --- pkg/controllers/placement/controller_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/placement/controller_test.go b/pkg/controllers/placement/controller_test.go index de8e11525..e6b8d16f6 100644 --- a/pkg/controllers/placement/controller_test.go +++ b/pkg/controllers/placement/controller_test.go @@ -133,7 +133,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { t.Fatalf("failed to create the policy hash: %v", err) } unspecifiedPolicyHash := []byte(fmt.Sprintf("%x", sha256.Sum256(jsonBytes))) - stalePlacementGeneration := strconv.Itoa(placementGeneration - 1) + previousPlacementGeneration := strconv.Itoa(placementGeneration - 1) tests := []struct { name string policy *fleetv1beta1.PlacementPolicy @@ -347,7 +347,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: stalePlacementGeneration, + fleetv1beta1.CRPGenerationAnnotation: previousPlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -376,7 +376,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: stalePlacementGeneration, + fleetv1beta1.CRPGenerationAnnotation: previousPlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -603,7 +603,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: "2", + fleetv1beta1.CRPGenerationAnnotation: previousPlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -658,7 +658,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) { }, Annotations: map[string]string{ fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), - fleetv1beta1.CRPGenerationAnnotation: "2", + fleetv1beta1.CRPGenerationAnnotation: previousPlacementGeneration, }, }, Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{ @@ -1120,7 +1120,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { Scheme: scheme, Recorder: record.NewFakeRecorder(10), } - _, err := r.getOrCreateSchedulingPolicySnapshot(ctx, crp, 1) + got, err := r.getOrCreateSchedulingPolicySnapshot(ctx, crp, 1) if tc.wantErr { if err == nil { // if error is nil t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want err") @@ -1133,6 +1133,12 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) { if err != nil { t.Fatalf("getOrCreateClusterResourceSnapshot() = %v, want nil", err) } + if got == nil { + t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want policy snapshot") + } + if got.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel] != strconv.FormatBool(true) { + t.Fatalf("policy snapshot latest label = %q, want %q", got.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel], strconv.FormatBool(true)) + } }) } }