bug fix

Author: zhiying-lin

pkg/controllers/clusterresourceplacement/controller.go

@@ -116,9 +116,11 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.Cluster
 
 func (r *Reconciler) getOrCreateClusterSchedulingPolicySnapshot(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement) (*fleetv1beta1.ClusterSchedulingPolicySnapshot, error) {
 	crpKObj := klog.KObj(crp)
-	schedulingPolicy := *crp.Spec.Policy // will exclude the numberOfClusters
-	schedulingPolicy.NumberOfClusters = nil
-	policyHash, err := generatePolicyHash(&schedulingPolicy)
+	schedulingPolicy := crp.Spec.Policy
+	if schedulingPolicy != nil {
+		schedulingPolicy.NumberOfClusters = nil // will exclude the numberOfClusters
+	}
+	policyHash, err := generatePolicyHash(schedulingPolicy)
 	if err != nil {
 		klog.ErrorS(err, "Failed to generate policy hash of crp", "clusterResourcePlacement", crpKObj)
 		return nil, controller.NewUnexpectedBehaviorError(err)
@@ -168,7 +170,7 @@ func (r *Reconciler) getOrCreateClusterSchedulingPolicySnapshot(ctx context.Cont
 			},
 		},
 		Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-			Policy:     &schedulingPolicy,
+			Policy:     schedulingPolicy,
 			PolicyHash: []byte(policyHash),
 		},
 	}

pkg/controllers/clusterresourceplacement/controller_test.go

@@ -85,7 +85,6 @@ func clusterResourcePlacementForTest() *fleetv1beta1.ClusterResourcePlacement {
 			Name: testName,
 		},
 		Spec: fleetv1beta1.ClusterResourcePlacementSpec{
-			Policy: placementPolicyForTest(),
 			ResourceSelectors: []fleetv1beta1.ClusterResourceSelector{
 				{
 					Group:   corev1.GroupName,
@@ -101,9 +100,9 @@ func clusterResourcePlacementForTest() *fleetv1beta1.ClusterResourcePlacement {
 }
 
 func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
-	wantPolicy := placementPolicyForTest()
-	wantPolicy.NumberOfClusters = nil
-	jsonBytes, err := json.Marshal(wantPolicy)
+	testPolicy := placementPolicyForTest()
+	testPolicy.NumberOfClusters = nil
+	jsonBytes, err := json.Marshal(testPolicy)
 	if err != nil {
 		t.Fatalf("failed to create the policy hash: %v", err)
 	}
@@ -115,13 +114,15 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 	unspecifiedPolicyHash := []byte(fmt.Sprintf("%x", sha256.Sum256(jsonBytes)))
 	tests := []struct {
 		name                    string
+		policy                  *fleetv1beta1.PlacementPolicy
 		revisionHistoryLimit    *int32
 		policySnapshots         []fleetv1beta1.ClusterSchedulingPolicySnapshot
 		wantPolicySnapshots     []fleetv1beta1.ClusterSchedulingPolicySnapshot
 		wantLatestSnapshotIndex int // index of the wantPolicySnapshots array
 	}{
 		{
-			name: "new clusterResourcePolicy and no existing policy snapshots owned by my-crp",
+			name:   "new clusterResourcePolicy and no existing policy snapshots owned by my-crp",
+			policy: placementPolicyForTest(),
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -168,15 +169,67 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
 			},
 			wantLatestSnapshotIndex: 1,
 		},
+		{
+			name: "new clusterResourcePolicy (unspecified policy) and no existing policy snapshots owned by my-crp",
+			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "another-crp-1",
+						Labels: map[string]string{
+							fleetv1beta1.PolicyIndexLabel:      "1",
+							fleetv1beta1.IsLatestSnapshotLabel: "true",
+							fleetv1beta1.CRPTrackingLabel:      "another-crp",
+						},
+					},
+				},
+			},
+			wantPolicySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "another-crp-1",
+						Labels: map[string]string{
+							fleetv1beta1.PolicyIndexLabel:      "1",
+							fleetv1beta1.IsLatestSnapshotLabel: "true",
+							fleetv1beta1.CRPTrackingLabel:      "another-crp",
+						},
+					},
+				},
+				// new policy snapshot owned by the my-crp
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0),
+						Labels: map[string]string{
+							fleetv1beta1.PolicyIndexLabel:      "0",
+							fleetv1beta1.IsLatestSnapshotLabel: "true",
+							fleetv1beta1.CRPTrackingLabel:      testName,
+						},
+						OwnerReferences: []metav1.OwnerReference{
+							{
+								Name:               testName,
+								BlockOwnerDeletion: pointer.Bool(true),
+								Controller:         pointer.Bool(true),
+								APIVersion:         fleetAPIVersion,
+								Kind:               "ClusterResourcePlacement",
+							},
+						},
+					},
+					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
+						PolicyHash: unspecifiedPolicyHash,
+					},
+				},
+			},
+			wantLatestSnapshotIndex: 1,
+		},
 		{
 			name:                 "new clusterResourcePolicy with invalidRevisionLimit and no existing policy snapshots owned by my-crp",
+			policy:               placementPolicyForTest(),
 			revisionHistoryLimit: &invalidRevisionLimit,
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
@@ -224,7 +277,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -233,6 +286,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 		},
 		{
 			name:                 "crp policy has no change",
+			policy:               placementPolicyForTest(),
 			revisionHistoryLimit: &singleRevisionLimit,
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
@@ -257,7 +311,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -285,7 +339,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -296,6 +350,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 			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
 			// before creating a new policy snapshot.
+			policy:               placementPolicyForTest(),
 			revisionHistoryLimit: &multipleRevisionLimit,
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
@@ -391,7 +446,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -400,6 +455,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 		},
 		{
 			name:                 "crp policy has changed and there is an active snapshot",
+			policy:               placementPolicyForTest(),
 			revisionHistoryLimit: &singleRevisionLimit,
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
@@ -449,15 +505,16 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
 			},
 			wantLatestSnapshotIndex: 0,
 		},
 		{
-			name: "crp policy has been changed and reverted back and there is no active snapshot",
+			name:   "crp policy has been changed and reverted back and there is no active snapshot",
+			policy: placementPolicyForTest(),
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -503,7 +560,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -554,7 +611,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -564,6 +621,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 		{
 			name: "crp policy has not been changed and only the numberOfCluster is changed",
 			// cause no new policy snapshot is created, it does not trigger the history limit check.
+			policy:               placementPolicyForTest(),
 			revisionHistoryLimit: &singleRevisionLimit,
 			policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
 				{
@@ -611,7 +669,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -662,7 +720,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 						},
 					},
 					Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
-						Policy:     wantPolicy,
+						Policy:     testPolicy,
 						PolicyHash: policyHash,
 					},
 				},
@@ -674,6 +732,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx := context.Background()
 			crp := clusterResourcePlacementForTest()
+			crp.Spec.Policy = tc.policy
 			crp.Spec.RevisionHistoryLimit = tc.revisionHistoryLimit
 			objects := []client.Object{crp}
 			for i := range tc.policySnapshots {