From fa0a52c957f6601bf5e0ec1838ad08a86c5edc0e Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Tue, 13 Jun 2023 15:05:27 +0800 Subject: [PATCH 1/2] fix policysnapshot hash --- .../placement/v1beta1/policysnapshot_types.go | 2 + .../placement_controller.go | 101 ++++++++--- .../placement_controller_test.go | 167 ++++++++++++++++-- 3 files changed, 227 insertions(+), 43 deletions(-) diff --git a/apis/placement/v1beta1/policysnapshot_types.go b/apis/placement/v1beta1/policysnapshot_types.go index 07eb2e597..ed2395f5b 100644 --- a/apis/placement/v1beta1/policysnapshot_types.go +++ b/apis/placement/v1beta1/policysnapshot_types.go @@ -16,6 +16,8 @@ const ( // PolicySnapshotNameFmt is clusterPolicySnapshot name format: {CRPName}-{PolicySnapshotIndex}. PolicySnapshotNameFmt = "%s-%d" + + NumberOfClustersAnnotation = fleetPrefix + "numberOfClusters" ) // +genclient diff --git a/pkg/controllers/clusterresourceplacement/placement_controller.go b/pkg/controllers/clusterresourceplacement/placement_controller.go index a77f4058d..173107b82 100644 --- a/pkg/controllers/clusterresourceplacement/placement_controller.go +++ b/pkg/controllers/clusterresourceplacement/placement_controller.go @@ -345,9 +345,11 @@ func (r *Reconciler) updatePlacementAppliedCondition(placement *fleetv1alpha1.Cl // It creates corresponding clusterPolicySnapshot and clusterResourceSnapshot if needed and updates the status based on // clusterPolicySnapshot status and work status. // If the error type is ErrUnexpectedBehavior, the controller will skip the reconciling. -func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement) (ctrl.Result, error) { +func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1.ClusterResourcePlacement) (ctrl.Result, error) { crpKObj := klog.KObj(crp) - policyHash, err := generatePolicyHash(crp.Spec.Policy) + schedulingPolicy := *crp.Spec.Policy // will exclude the numberOfClusters + schedulingPolicy.NumberOfClusters = nil + policyHash, err := generatePolicyHash(&schedulingPolicy) if err != nil { klog.ErrorS(err, "Failed to generate policy hash of crp", "clusterResourcePlacement", crpKObj) return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err) @@ -361,57 +363,97 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.Cluster // mark the last policy snapshot as inactive if it is different from what we have now if latestPolicySnapshot != nil && string(latestPolicySnapshot.Spec.PolicyHash) != policyHash && - latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] == strconv.FormatBool(true) { + latestPolicySnapshot.Labels[fleetv1.IsLatestSnapshotLabel] == strconv.FormatBool(true) { // set the latest label to false first to make sure there is only one or none active policy snapshot - latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] = strconv.FormatBool(false) + latestPolicySnapshot.Labels[fleetv1.IsLatestSnapshotLabel] = strconv.FormatBool(false) if err := r.Client.Update(ctx, latestPolicySnapshot); err != nil { klog.ErrorS(err, "Failed to set the isLatestSnapshot label to false", "clusterPolicySnapshot", klog.KObj(latestPolicySnapshot)) return ctrl.Result{}, controller.NewAPIServerError(err) } } - if latestPolicySnapshot == nil || string(latestPolicySnapshot.Spec.PolicyHash) != policyHash { + if latestPolicySnapshot != nil && string(latestPolicySnapshot.Spec.PolicyHash) == policyHash { + if err := r.ensureLatestPolicySnapshot(ctx, crp, latestPolicySnapshot); err != nil { + return ctrl.Result{}, err + } + } else { // create a new policy snapshot latestPolicySnapshotIndex++ - latestPolicySnapshot = &fleetv1beta1.ClusterPolicySnapshot{ + latestPolicySnapshot = &fleetv1.ClusterPolicySnapshot{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, crp.Name, latestPolicySnapshotIndex), + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, crp.Name, latestPolicySnapshotIndex), Labels: map[string]string{ - fleetv1beta1.CRPTrackingLabel: crp.Name, - fleetv1beta1.IsLatestSnapshotLabel: strconv.FormatBool(true), - fleetv1beta1.PolicyIndexLabel: strconv.Itoa(latestPolicySnapshotIndex), + fleetv1.CRPTrackingLabel: crp.Name, + fleetv1.IsLatestSnapshotLabel: strconv.FormatBool(true), + fleetv1.PolicyIndexLabel: strconv.Itoa(latestPolicySnapshotIndex), }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: crp.Spec.Policy, + Spec: fleetv1.PolicySnapshotSpec{ + Policy: &schedulingPolicy, PolicyHash: []byte(policyHash), }, } + policySnapshotKObj := klog.KObj(latestPolicySnapshot) if err := controllerutil.SetControllerReference(crp, latestPolicySnapshot, r.Scheme); err != nil { - klog.ErrorS(err, "Failed to create set owner reference", "clusterPolicySnapshot", klog.KObj(latestPolicySnapshot)) + klog.ErrorS(err, "Failed to create set owner reference", "clusterPolicySnapshot", policySnapshotKObj) // should never happen return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err) } + // make sure each policySnapshot should always have the annotation if CRP is selectN type + if crp.Spec.Policy != nil && + crp.Spec.Policy.PlacementType == fleetv1.PickNPlacementType && + crp.Spec.Policy.NumberOfClusters != nil { + latestPolicySnapshot.Annotations = map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(int(*crp.Spec.Policy.NumberOfClusters)), + } + } + if err := r.Client.Create(ctx, latestPolicySnapshot); err != nil { - klog.ErrorS(err, "Failed to create new clusterPolicySnapshot", "clusterPolicySnapshot", klog.KObj(latestPolicySnapshot)) + klog.ErrorS(err, "Failed to create new clusterPolicySnapshot", "clusterPolicySnapshot", policySnapshotKObj) return ctrl.Result{}, controller.NewAPIServerError(err) } - } else if latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] != strconv.FormatBool(true) { + } + + // create clusterResourceSnapshot + // update the status based on the latestPolicySnapshot status + // update the status based on the work + return ctrl.Result{}, nil +} + +// ensureLatestPolicySnapshot ensures the latest policySnapshot has the isLatest label and the numberOfClusters are updated. +func (r *Reconciler) ensureLatestPolicySnapshot(ctx context.Context, crp *fleetv1.ClusterResourcePlacement, latest *fleetv1.ClusterPolicySnapshot) error { + needUpdate := false + if latest.Labels[fleetv1.IsLatestSnapshotLabel] != strconv.FormatBool(true) { // When latestPolicySnapshot.Spec.PolicyHash == policyHash, // It could happen when the controller just sets the latest label to false for the old snapshot, and fails to // create a new policy snapshot. // And then the customers revert back their policy to the old one again. // In this case, the "latest" snapshot without isLatest label has the same policy hash as the current policy. - latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] = strconv.FormatBool(true) - if err := r.Client.Update(ctx, latestPolicySnapshot); err != nil { - klog.ErrorS(err, "Failed to set the isLatestSnapshot label to true", "clusterPolicySnapshot", klog.KObj(latestPolicySnapshot)) - return ctrl.Result{}, controller.NewAPIServerError(err) + latest.Labels[fleetv1.IsLatestSnapshotLabel] = strconv.FormatBool(true) + needUpdate = true + } + + if crp.Spec.Policy != nil && + crp.Spec.Policy.PlacementType == fleetv1.PickNPlacementType && + crp.Spec.Policy.NumberOfClusters != nil { + oldCount, err := parseNumberOfClustersFromAnnotation(latest) + if err != nil { + return controller.NewUnexpectedBehaviorError(err) + } + newCount := int(*crp.Spec.Policy.NumberOfClusters) + if oldCount != newCount { + latest.Annotations[fleetv1.NumberOfClustersAnnotation] = strconv.Itoa(newCount) + needUpdate = true } } - // create clusterResourceSnapshot - // update the status based on the latestPolicySnapshot status - // update the status based on the work - return ctrl.Result{}, nil + if !needUpdate { + return nil + } + if err := r.Client.Update(ctx, latest); err != nil { + klog.ErrorS(err, "Failed to update the clusterPolicySnapshot", "clusterPolicySnapshot", klog.KObj(latest)) + return controller.NewAPIServerError(err) + } + return nil } // lookupLatestClusterPolicySnapshot finds the latest snapshots and its policy index. @@ -480,7 +522,18 @@ func parsePolicyIndexFromLabel(s *fleetv1beta1.ClusterPolicySnapshot) (int, erro return v, nil } -func generatePolicyHash(policy *fleetv1beta1.PlacementPolicy) (string, error) { +func parseNumberOfClustersFromAnnotation(s *fleetv1.ClusterPolicySnapshot) (int, error) { + n := s.Annotations[fleetv1.NumberOfClustersAnnotation] + v, err := strconv.Atoi(n) + if err != nil { + klog.ErrorS(err, "Failed to parse the numberOfClusterAnnotation", "clusterPolicySnapshot", klog.KObj(s), "numberOfClusterAnnotation", n) + // should never happen + return -1, err + } + return v, nil +} + +func generatePolicyHash(policy *fleetv1.PlacementPolicy) (string, error) { jsonBytes, err := json.Marshal(policy) if err != nil { return "", err diff --git a/pkg/controllers/clusterresourceplacement/placement_controller_test.go b/pkg/controllers/clusterresourceplacement/placement_controller_test.go index 554efcc40..b74c63645 100644 --- a/pkg/controllers/clusterresourceplacement/placement_controller_test.go +++ b/pkg/controllers/clusterresourceplacement/placement_controller_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "strconv" "testing" "github.com/google/go-cmp/cmp" @@ -71,7 +72,9 @@ func clusterResourcePlacementForTest() *fleetv1beta1.ClusterResourcePlacement { } func TestHandleUpdate(t *testing.T) { - jsonBytes, err := json.Marshal(placementPolicyForTest()) + wantPolicy := placementPolicyForTest() + wantPolicy.NumberOfClusters = nil + jsonBytes, err := json.Marshal(wantPolicy) if err != nil { t.Fatalf("failed to create the policy hash: %v", err) } @@ -129,9 +132,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, @@ -157,9 +163,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, @@ -182,9 +191,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, @@ -306,9 +318,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, @@ -382,9 +397,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, @@ -420,8 +438,8 @@ func TestHandleUpdate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), Labels: map[string]string{ - fleetv1beta1.PolicyIndexLabel: "1", - fleetv1beta1.CRPTrackingLabel: testName, + fleetv1.PolicyIndexLabel: "1", + fleetv1.CRPTrackingLabel: testName, }, OwnerReferences: []metav1.OwnerReference{ { @@ -432,17 +450,125 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, }, - wantPolicySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ + wantPolicySnapshots: []fleetv1.ClusterPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0), + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), + Labels: map[string]string{ + fleetv1.PolicyIndexLabel: "0", + fleetv1.IsLatestSnapshotLabel: "false", + fleetv1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + }, + Spec: fleetv1.PolicySnapshotSpec{ + // Policy is not specified. + PolicyHash: unspecifiedPolicyHash, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 1), + Labels: map[string]string{ + fleetv1.PolicyIndexLabel: "1", + fleetv1.IsLatestSnapshotLabel: "true", + fleetv1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, + }, + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, + PolicyHash: policyHash, + }, + }, + }, + }, + { + name: "crp policy has not been changed and only the numberOfCluster is changed", + policySnapshots: []fleetv1.ClusterPolicySnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), + Labels: map[string]string{ + fleetv1.PolicyIndexLabel: "0", + fleetv1.IsLatestSnapshotLabel: "false", + fleetv1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + }, + Spec: fleetv1.PolicySnapshotSpec{ + // Policy is not specified. + PolicyHash: unspecifiedPolicyHash, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 1), + Labels: map[string]string{ + fleetv1.PolicyIndexLabel: "1", + fleetv1.CRPTrackingLabel: testName, + fleetv1.IsLatestSnapshotLabel: "true", + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(1), + }, + }, + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, + PolicyHash: policyHash, + }, + }, + }, + wantPolicySnapshots: []fleetv1.ClusterPolicySnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), Labels: map[string]string{ fleetv1beta1.PolicyIndexLabel: "0", fleetv1beta1.IsLatestSnapshotLabel: "false", @@ -480,9 +606,12 @@ func TestHandleUpdate(t *testing.T) { Kind: "ClusterResourcePlacement", }, }, + Annotations: map[string]string{ + fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + }, }, - Spec: fleetv1beta1.PolicySnapshotSpec{ - Policy: placementPolicyForTest(), + Spec: fleetv1.PolicySnapshotSpec{ + Policy: wantPolicy, PolicyHash: policyHash, }, }, From 724bd8227813387d9aff8faf878ee17f86cfe56d Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Thu, 15 Jun 2023 11:04:49 +0800 Subject: [PATCH 2/2] address comments --- .../placement/v1beta1/policysnapshot_types.go | 1 + .../placement_controller.go | 55 +++--- .../placement_controller_test.go | 176 +++++++++++++----- 3 files changed, 166 insertions(+), 66 deletions(-) diff --git a/apis/placement/v1beta1/policysnapshot_types.go b/apis/placement/v1beta1/policysnapshot_types.go index ed2395f5b..7ff78402e 100644 --- a/apis/placement/v1beta1/policysnapshot_types.go +++ b/apis/placement/v1beta1/policysnapshot_types.go @@ -17,6 +17,7 @@ const ( // PolicySnapshotNameFmt is clusterPolicySnapshot name format: {CRPName}-{PolicySnapshotIndex}. PolicySnapshotNameFmt = "%s-%d" + // NumberOfClustersAnnotation is the annotation that indicates how many clusters should be selected for selectN placement type. NumberOfClustersAnnotation = fleetPrefix + "numberOfClusters" ) diff --git a/pkg/controllers/clusterresourceplacement/placement_controller.go b/pkg/controllers/clusterresourceplacement/placement_controller.go index 173107b82..088d09187 100644 --- a/pkg/controllers/clusterresourceplacement/placement_controller.go +++ b/pkg/controllers/clusterresourceplacement/placement_controller.go @@ -345,7 +345,7 @@ func (r *Reconciler) updatePlacementAppliedCondition(placement *fleetv1alpha1.Cl // It creates corresponding clusterPolicySnapshot and clusterResourceSnapshot if needed and updates the status based on // clusterPolicySnapshot status and work status. // If the error type is ErrUnexpectedBehavior, the controller will skip the reconciling. -func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1.ClusterResourcePlacement) (ctrl.Result, error) { +func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement) (ctrl.Result, error) { crpKObj := klog.KObj(crp) schedulingPolicy := *crp.Spec.Policy // will exclude the numberOfClusters schedulingPolicy.NumberOfClusters = nil @@ -363,9 +363,9 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1.ClusterResou // mark the last policy snapshot as inactive if it is different from what we have now if latestPolicySnapshot != nil && string(latestPolicySnapshot.Spec.PolicyHash) != policyHash && - latestPolicySnapshot.Labels[fleetv1.IsLatestSnapshotLabel] == strconv.FormatBool(true) { + latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] == strconv.FormatBool(true) { // set the latest label to false first to make sure there is only one or none active policy snapshot - latestPolicySnapshot.Labels[fleetv1.IsLatestSnapshotLabel] = strconv.FormatBool(false) + latestPolicySnapshot.Labels[fleetv1beta1.IsLatestSnapshotLabel] = strconv.FormatBool(false) if err := r.Client.Update(ctx, latestPolicySnapshot); err != nil { klog.ErrorS(err, "Failed to set the isLatestSnapshot label to false", "clusterPolicySnapshot", klog.KObj(latestPolicySnapshot)) return ctrl.Result{}, controller.NewAPIServerError(err) @@ -378,32 +378,32 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1.ClusterResou } else { // create a new policy snapshot latestPolicySnapshotIndex++ - latestPolicySnapshot = &fleetv1.ClusterPolicySnapshot{ + latestPolicySnapshot = &fleetv1beta1.ClusterPolicySnapshot{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, crp.Name, latestPolicySnapshotIndex), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, crp.Name, latestPolicySnapshotIndex), Labels: map[string]string{ - fleetv1.CRPTrackingLabel: crp.Name, - fleetv1.IsLatestSnapshotLabel: strconv.FormatBool(true), - fleetv1.PolicyIndexLabel: strconv.Itoa(latestPolicySnapshotIndex), + fleetv1beta1.CRPTrackingLabel: crp.Name, + fleetv1beta1.IsLatestSnapshotLabel: strconv.FormatBool(true), + fleetv1beta1.PolicyIndexLabel: strconv.Itoa(latestPolicySnapshotIndex), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: &schedulingPolicy, PolicyHash: []byte(policyHash), }, } policySnapshotKObj := klog.KObj(latestPolicySnapshot) if err := controllerutil.SetControllerReference(crp, latestPolicySnapshot, r.Scheme); err != nil { - klog.ErrorS(err, "Failed to create set owner reference", "clusterPolicySnapshot", policySnapshotKObj) + klog.ErrorS(err, "Failed to set owner reference", "clusterPolicySnapshot", policySnapshotKObj) // should never happen return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err) } // make sure each policySnapshot should always have the annotation if CRP is selectN type if crp.Spec.Policy != nil && - crp.Spec.Policy.PlacementType == fleetv1.PickNPlacementType && + crp.Spec.Policy.PlacementType == fleetv1beta1.PickNPlacementType && crp.Spec.Policy.NumberOfClusters != nil { latestPolicySnapshot.Annotations = map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(int(*crp.Spec.Policy.NumberOfClusters)), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(int(*crp.Spec.Policy.NumberOfClusters)), } } @@ -420,29 +420,30 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1.ClusterResou } // ensureLatestPolicySnapshot ensures the latest policySnapshot has the isLatest label and the numberOfClusters are updated. -func (r *Reconciler) ensureLatestPolicySnapshot(ctx context.Context, crp *fleetv1.ClusterResourcePlacement, latest *fleetv1.ClusterPolicySnapshot) error { +func (r *Reconciler) ensureLatestPolicySnapshot(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement, latest *fleetv1beta1.ClusterPolicySnapshot) error { needUpdate := false - if latest.Labels[fleetv1.IsLatestSnapshotLabel] != strconv.FormatBool(true) { + if latest.Labels[fleetv1beta1.IsLatestSnapshotLabel] != strconv.FormatBool(true) { // When latestPolicySnapshot.Spec.PolicyHash == policyHash, // It could happen when the controller just sets the latest label to false for the old snapshot, and fails to // create a new policy snapshot. // And then the customers revert back their policy to the old one again. // In this case, the "latest" snapshot without isLatest label has the same policy hash as the current policy. - latest.Labels[fleetv1.IsLatestSnapshotLabel] = strconv.FormatBool(true) + latest.Labels[fleetv1beta1.IsLatestSnapshotLabel] = strconv.FormatBool(true) needUpdate = true } if crp.Spec.Policy != nil && - crp.Spec.Policy.PlacementType == fleetv1.PickNPlacementType && + crp.Spec.Policy.PlacementType == fleetv1beta1.PickNPlacementType && crp.Spec.Policy.NumberOfClusters != nil { oldCount, err := parseNumberOfClustersFromAnnotation(latest) if err != nil { + klog.ErrorS(err, "Failed to parse the numberOfClusterAnnotation", "clusterPolicySnapshot", klog.KObj(latest)) return controller.NewUnexpectedBehaviorError(err) } newCount := int(*crp.Spec.Policy.NumberOfClusters) if oldCount != newCount { - latest.Annotations[fleetv1.NumberOfClustersAnnotation] = strconv.Itoa(newCount) + latest.Annotations[fleetv1beta1.NumberOfClustersAnnotation] = strconv.Itoa(newCount) needUpdate = true } } @@ -478,6 +479,7 @@ func (r *Reconciler) lookupLatestClusterPolicySnapshot(ctx context.Context, crp if len(snapshotList.Items) == 1 { policyIndex, err := parsePolicyIndexFromLabel(&snapshotList.Items[0]) if err != nil { + klog.ErrorS(err, "Failed to parse the policy index label", "clusterPolicySnapshot", klog.KObj(&snapshotList.Items[0])) return nil, -1, controller.NewUnexpectedBehaviorError(err) } return &snapshotList.Items[0], policyIndex, nil @@ -501,6 +503,7 @@ func (r *Reconciler) lookupLatestClusterPolicySnapshot(ctx context.Context, crp for i := range snapshotList.Items { policyIndex, err := parsePolicyIndexFromLabel(&snapshotList.Items[i]) if err != nil { + klog.ErrorS(err, "Failed to parse the policy index label", "clusterPolicySnapshot", klog.KObj(&snapshotList.Items[i])) return nil, -1, controller.NewUnexpectedBehaviorError(err) } if lastPolicyIndex < policyIndex { @@ -511,29 +514,33 @@ func (r *Reconciler) lookupLatestClusterPolicySnapshot(ctx context.Context, crp return &snapshotList.Items[index], lastPolicyIndex, nil } +// parsePolicyIndexFromLabel returns error when parsing the label which should never return error in production. func parsePolicyIndexFromLabel(s *fleetv1beta1.ClusterPolicySnapshot) (int, error) { indexLabel := s.Labels[fleetv1beta1.PolicyIndexLabel] v, err := strconv.Atoi(indexLabel) if err != nil { - klog.ErrorS(err, "Failed to parse the policy index label", "clusterPolicySnapshot", klog.KObj(s), "policyIndexLabel", indexLabel) - // should never happen return -1, err } + if v < 0 { + return -1, fmt.Errorf("policy index should not be negative: %d", v) + } return v, nil } -func parseNumberOfClustersFromAnnotation(s *fleetv1.ClusterPolicySnapshot) (int, error) { - n := s.Annotations[fleetv1.NumberOfClustersAnnotation] +// parseNumberOfClustersFromAnnotation returns error when parsing the annotation which should never return error in production. +func parseNumberOfClustersFromAnnotation(s *fleetv1beta1.ClusterPolicySnapshot) (int, error) { + n := s.Annotations[fleetv1beta1.NumberOfClustersAnnotation] v, err := strconv.Atoi(n) if err != nil { - klog.ErrorS(err, "Failed to parse the numberOfClusterAnnotation", "clusterPolicySnapshot", klog.KObj(s), "numberOfClusterAnnotation", n) - // should never happen return -1, err } + if v < 0 { + return -1, fmt.Errorf("numberOfCluster should not be negative: %d", v) + } return v, nil } -func generatePolicyHash(policy *fleetv1.PlacementPolicy) (string, error) { +func generatePolicyHash(policy *fleetv1beta1.PlacementPolicy) (string, error) { jsonBytes, err := json.Marshal(policy) if err != nil { return "", err diff --git a/pkg/controllers/clusterresourceplacement/placement_controller_test.go b/pkg/controllers/clusterresourceplacement/placement_controller_test.go index b74c63645..fe9eb2474 100644 --- a/pkg/controllers/clusterresourceplacement/placement_controller_test.go +++ b/pkg/controllers/clusterresourceplacement/placement_controller_test.go @@ -133,10 +133,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -164,10 +164,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -192,10 +192,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -319,10 +319,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -398,10 +398,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -438,8 +438,8 @@ func TestHandleUpdate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), Labels: map[string]string{ - fleetv1.PolicyIndexLabel: "1", - fleetv1.CRPTrackingLabel: testName, + fleetv1beta1.PolicyIndexLabel: "1", + fleetv1beta1.CRPTrackingLabel: testName, }, OwnerReferences: []metav1.OwnerReference{ { @@ -451,23 +451,23 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, }, }, - wantPolicySnapshots: []fleetv1.ClusterPolicySnapshot{ + wantPolicySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0), Labels: map[string]string{ - fleetv1.PolicyIndexLabel: "0", - fleetv1.IsLatestSnapshotLabel: "false", - fleetv1.CRPTrackingLabel: testName, + fleetv1beta1.PolicyIndexLabel: "0", + fleetv1beta1.IsLatestSnapshotLabel: "false", + fleetv1beta1.CRPTrackingLabel: testName, }, OwnerReferences: []metav1.OwnerReference{ { @@ -479,18 +479,18 @@ func TestHandleUpdate(t *testing.T) { }, }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ // Policy is not specified. PolicyHash: unspecifiedPolicyHash, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 1), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), Labels: map[string]string{ - fleetv1.PolicyIndexLabel: "1", - fleetv1.IsLatestSnapshotLabel: "true", - fleetv1.CRPTrackingLabel: testName, + fleetv1beta1.PolicyIndexLabel: "1", + fleetv1beta1.IsLatestSnapshotLabel: "true", + fleetv1beta1.CRPTrackingLabel: testName, }, OwnerReferences: []metav1.OwnerReference{ { @@ -502,10 +502,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -514,14 +514,14 @@ func TestHandleUpdate(t *testing.T) { }, { name: "crp policy has not been changed and only the numberOfCluster is changed", - policySnapshots: []fleetv1.ClusterPolicySnapshot{ + policySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0), Labels: map[string]string{ - fleetv1.PolicyIndexLabel: "0", - fleetv1.IsLatestSnapshotLabel: "false", - fleetv1.CRPTrackingLabel: testName, + fleetv1beta1.PolicyIndexLabel: "0", + fleetv1beta1.IsLatestSnapshotLabel: "false", + fleetv1beta1.CRPTrackingLabel: testName, }, OwnerReferences: []metav1.OwnerReference{ { @@ -533,18 +533,18 @@ func TestHandleUpdate(t *testing.T) { }, }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ // Policy is not specified. PolicyHash: unspecifiedPolicyHash, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 1), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), Labels: map[string]string{ - fleetv1.PolicyIndexLabel: "1", - fleetv1.CRPTrackingLabel: testName, - fleetv1.IsLatestSnapshotLabel: "true", + fleetv1beta1.PolicyIndexLabel: "1", + fleetv1beta1.CRPTrackingLabel: testName, + fleetv1beta1.IsLatestSnapshotLabel: "true", }, OwnerReferences: []metav1.OwnerReference{ { @@ -556,19 +556,19 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(1), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(1), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, }, }, - wantPolicySnapshots: []fleetv1.ClusterPolicySnapshot{ + wantPolicySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ { ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(fleetv1.PolicySnapshotNameFmt, testName, 0), + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0), Labels: map[string]string{ fleetv1beta1.PolicyIndexLabel: "0", fleetv1beta1.IsLatestSnapshotLabel: "false", @@ -607,10 +607,10 @@ func TestHandleUpdate(t *testing.T) { }, }, Annotations: map[string]string{ - fleetv1.NumberOfClustersAnnotation: strconv.Itoa(3), + fleetv1beta1.NumberOfClustersAnnotation: strconv.Itoa(3), }, }, - Spec: fleetv1.PolicySnapshotSpec{ + Spec: fleetv1beta1.PolicySnapshotSpec{ Policy: wantPolicy, PolicyHash: policyHash, }, @@ -655,6 +655,13 @@ func TestHandleUpdate(t *testing.T) { } func TestHandleUpdate_failure(t *testing.T) { + wantPolicy := placementPolicyForTest() + wantPolicy.NumberOfClusters = nil + jsonBytes, err := json.Marshal(wantPolicy) + if err != nil { + t.Fatalf("failed to create the policy hash: %v", err) + } + policyHash := []byte(fmt.Sprintf("%x", sha256.Sum256(jsonBytes))) tests := []struct { name string policySnapshots []fleetv1beta1.ClusterPolicySnapshot @@ -766,6 +773,91 @@ func TestHandleUpdate_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)", + policySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0), + Labels: map[string]string{ + fleetv1beta1.PolicyIndexLabel: "-1", + fleetv1beta1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + }, + }, + }, + }, + }, + }, + { + // 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", + policySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), + Labels: map[string]string{ + fleetv1beta1.PolicyIndexLabel: "1", + fleetv1beta1.IsLatestSnapshotLabel: "true", + fleetv1beta1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + Annotations: map[string]string{ + fleetv1beta1.NumberOfClustersAnnotation: "invalid", + }, + }, + Spec: fleetv1beta1.PolicySnapshotSpec{ + Policy: wantPolicy, + PolicyHash: policyHash, + }, + }, + }, + }, + { + // 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)", + policySnapshots: []fleetv1beta1.ClusterPolicySnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 1), + Labels: map[string]string{ + fleetv1beta1.PolicyIndexLabel: "1", + fleetv1beta1.CRPTrackingLabel: testName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + Name: testName, + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + APIVersion: fleetAPIVersion, + Kind: "ClusterResourcePlacement", + }, + }, + Annotations: map[string]string{ + fleetv1beta1.NumberOfClustersAnnotation: "-123", + }, + }, + Spec: fleetv1beta1.PolicySnapshotSpec{ + Policy: wantPolicy, + PolicyHash: policyHash, + }, + }, + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {