Skip to content

Preserve CRP scheduled status across non-scheduling spec updates #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Preserve CRP scheduled status across non-scheduling spec updates #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce51051
Initial plan
Copilot May 29, 2026
e06d02f
fix: preserve CRP scheduled status on non-scheduling updates
Copilot May 29, 2026
90e341b
test: clarify placement snapshot generation coverage
Copilot May 29, 2026
ea3bfa8
test: tighten placement scheduling coverage
Copilot May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions pkg/controllers/placement/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down
105 changes: 96 additions & 9 deletions pkg/controllers/placement/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
previousPlacementGeneration := strconv.Itoa(placementGeneration - 1)
tests := []struct {
name string
policy *fleetv1beta1.PlacementPolicy
Expand Down Expand Up @@ -322,6 +323,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: previousPlacementGeneration,
},
},
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: previousPlacementGeneration,
},
},
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
Expand Down Expand Up @@ -538,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{
Expand Down Expand Up @@ -593,7 +658,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
Annotations: map[string]string{
fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3),
fleetv1beta1.CRPGenerationAnnotation: strconv.Itoa(placementGeneration),
fleetv1beta1.CRPGenerationAnnotation: previousPlacementGeneration,
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Expand Down Expand Up @@ -710,7 +775,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{
Expand Down Expand Up @@ -785,10 +850,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{
Expand All @@ -811,6 +878,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{
Expand All @@ -834,6 +902,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{
Expand Down Expand Up @@ -872,6 +941,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{
Expand Down Expand Up @@ -912,6 +982,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{
Expand All @@ -934,6 +1005,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{
Expand Down Expand Up @@ -966,6 +1038,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{
Expand Down Expand Up @@ -995,8 +1068,10 @@ 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.
// 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{
{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -1045,12 +1120,24 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot_failure(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}
_, err := r.getOrCreateSchedulingPolicySnapshot(ctx, crp, 1)
if err == nil { // if error is nil
t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want err")
got, err := r.getOrCreateSchedulingPolicySnapshot(ctx, crp, 1)
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 err != nil {
t.Fatalf("getOrCreateClusterResourceSnapshot() = %v, want nil", err)
}
if got == nil {
t.Fatal("getOrCreateClusterResourceSnapshot() = nil, want policy snapshot")
}
if !errors.Is(err, controller.ErrUnexpectedBehavior) {
t.Errorf("getOrCreateClusterResourceSnapshot() got %v, want %v type", err, controller.ErrUnexpectedBehavior)
if got.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel] != strconv.FormatBool(true) {
t.Fatalf("policy snapshot latest label = %q, want %q", got.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel], strconv.FormatBool(true))
}
})
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/controllers/placement/placement_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
},
Expand Down