From 251b3a38afb7900c26a9bebf39a95b4a4ec2d3fd Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 25 Oct 2024 13:51:53 -0700 Subject: [PATCH] make sure the binding is already sycned --- apis/placement/v1beta1/commons.go | 7 +- pkg/controllers/workgenerator/controller.go | 38 +++++++--- .../controller_integration_test.go | 74 ++++++++++++++++++- 3 files changed, 104 insertions(+), 15 deletions(-) diff --git a/apis/placement/v1beta1/commons.go b/apis/placement/v1beta1/commons.go index 321ce3131..e805051a0 100644 --- a/apis/placement/v1beta1/commons.go +++ b/apis/placement/v1beta1/commons.go @@ -50,12 +50,15 @@ const ( // The format is {workPrefix}-configMap-uuid WorkNameWithConfigEnvelopeFmt = "%s-configmap-%s" - // ParentClusterResourceOverrideSnapshotHashAnnotation is the label to work that contains the hash of the parent cluster resource override snapshot list. + // ParentClusterResourceOverrideSnapshotHashAnnotation is the annotation to work that contains the hash of the parent cluster resource override snapshot list. ParentClusterResourceOverrideSnapshotHashAnnotation = fleetPrefix + "parent-cluster-resource-override-snapshot-hash" - // ParentResourceOverrideSnapshotHashAnnotation is the label to work that contains the hash of the parent resource override snapshot list. + // ParentResourceOverrideSnapshotHashAnnotation is the annotation to work that contains the hash of the parent resource override snapshot list. ParentResourceOverrideSnapshotHashAnnotation = fleetPrefix + "parent-resource-override-snapshot-hash" + // ParentResourceSnapshotNameAnnotation is the annotation applied to work that contains the name of the master resource snapshot that generates the work. + ParentResourceSnapshotNameAnnotation = fleetPrefix + "parent-resource-snapshot-name" + // ParentResourceSnapshotIndexLabel is the label applied to work that contains the index of the resource snapshot that generates the work. ParentResourceSnapshotIndexLabel = fleetPrefix + "parent-resource-snapshot-index" diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index 6ffd43e1e..90e3aba92 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -136,10 +136,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques workUpdated := false overrideSucceeded := false - // Reset the conditions and failed placements. - for i := condition.OverriddenCondition; i < condition.TotalCondition; i++ { - resourceBinding.RemoveCondition(string(i.ResourceBindingConditionType())) - } resourceBinding.Status.FailedPlacements = nil // list all the corresponding works works, syncErr := r.listAllWorksAssociated(ctx, &resourceBinding) @@ -147,7 +143,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques // generate and apply the workUpdated works if we have all the works overrideSucceeded, workUpdated, syncErr = r.syncAllWork(ctx, &resourceBinding, works, cluster) } - + // Reset the conditions and failed placements. + for i := condition.OverriddenCondition; i < condition.TotalCondition; i++ { + resourceBinding.RemoveCondition(string(i.ResourceBindingConditionType())) + } if overrideSucceeded { overrideReason := condition.OverriddenSucceededReason overrideMessage := "Successfully applied the override rules on the resources" @@ -392,9 +391,9 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be if err != nil { if errors.Is(err, errResourceSnapshotNotFound) { // the resourceIndex is deleted but the works might still be up to date with the binding. - if areAllWorkSynced(existingWorks, resourceBinding.Spec.ResourceSnapshotName, resourceOverrideSnapshotHash, clusterResourceOverrideSnapshotHash) { + if areAllWorkSynced(existingWorks, resourceBinding, resourceOverrideSnapshotHash, clusterResourceOverrideSnapshotHash) { klog.V(2).InfoS("All the works are synced with the resourceBinding even if the resource snapshot index is removed", "resourceBinding", resourceBindingRef) - return false, false, nil + return true, false, nil } return false, false, controller.NewUserError(err) } @@ -505,10 +504,25 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be } // areAllWorkSynced checks if all the works are synced with the resource binding. -func areAllWorkSynced(existingWorks map[string]*fleetv1beta1.Work, resourceSnapshotIndex, _, _ string) bool { +func areAllWorkSynced(existingWorks map[string]*fleetv1beta1.Work, resourceBinding *fleetv1beta1.ClusterResourceBinding, _, _ string) bool { + syncedCondition := resourceBinding.GetCondition(string(fleetv1beta1.ResourceBindingWorkSynchronized)) + if !condition.IsConditionStatusTrue(syncedCondition, resourceBinding.Generation) { + // The binding has to be synced first before we can check the works + return false + } // TODO: check resourceOverrideSnapshotHash and clusterResourceOverrideSnapshotHash after all the work has the ParentResourceOverrideSnapshotHashAnnotation and ParentClusterResourceOverrideSnapshotHashAnnotation + resourceSnapshotName := resourceBinding.Spec.ResourceSnapshotName for _, work := range existingWorks { - if work.GetLabels()[fleetv1beta1.ParentResourceSnapshotIndexLabel] != resourceSnapshotIndex { + recordedName, exist := work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] + if !exist { + // TODO: remove this block after all the work has the ParentResourceSnapshotNameAnnotation + // the parent resource snapshot name is not recorded in the work, we need to construct it from the labels + crpName := resourceBinding.Labels[fleetv1beta1.CRPTrackingLabel] + index, _ := labels.ExtractResourceSnapshotIndexFromWork(work) + recordedName = fmt.Sprintf(fleetv1beta1.ResourceSnapshotNameFmt, crpName, index) + } + if recordedName != resourceSnapshotName { + klog.V(2).InfoS("The work is not synced with the resourceBinding", "work", klog.KObj(work), "resourceBinding", klog.KObj(resourceBinding), "annotationExist", exist, "recordedName", recordedName, "resourceSnapshotName", resourceSnapshotName) return false } } @@ -576,6 +590,7 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre fleetv1beta1.EnvelopeNamespaceLabel: envelopeObj.GetNamespace(), }, Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: resourceBinding.Spec.ResourceSnapshotName, fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: resourceOverrideSnapshotHash, fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: clusterResourceOverrideSnapshotHash, }, @@ -604,6 +619,7 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre } work := workList.Items[0] work.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = resourceSnapshot.Labels[fleetv1beta1.ResourceIndexLabel] + work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = resourceBinding.Spec.ResourceSnapshotName work.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = resourceOverrideSnapshotHash work.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = clusterResourceOverrideSnapshotHash work.Spec.Workload.Manifests = manifest @@ -624,6 +640,7 @@ func generateSnapshotWorkObj(workName string, resourceBinding *fleetv1beta1.Clus fleetv1beta1.ParentResourceSnapshotIndexLabel: resourceSnapshot.Labels[fleetv1beta1.ResourceIndexLabel], }, Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: resourceBinding.Spec.ResourceSnapshotName, fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: resourceOverrideSnapshotHash, fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: clusterResourceOverrideSnapshotHash, }, @@ -679,8 +696,9 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee } klog.V(2).InfoS("Work is already associated with the desired resourceSnapshot but still not having the right override snapshots", "resourceIndex", resourceIndex, "work", workObj, "resourceSnapshot", resourceSnapshotObj) } - // need to copy the new work to the existing work, only 4 possible changes: + // need to copy the new work to the existing work, only 5 possible changes: existingWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] + existingWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] existingWork.Spec.Workload.Manifests = newWork.Spec.Workload.Manifests diff --git a/pkg/controllers/workgenerator/controller_integration_test.go b/pkg/controllers/workgenerator/controller_integration_test.go index f5b6f2491..ee3c34ff5 100644 --- a/pkg/controllers/workgenerator/controller_integration_test.go +++ b/pkg/controllers/workgenerator/controller_integration_test.go @@ -214,7 +214,7 @@ var _ = Describe("Test Work Generator Controller", func() { By(fmt.Sprintf("work %s is created in %s", work.Name, work.Namespace)) }) - It("Should handle the case that the snapshot is deleted", func() { + It("Should handle the case that the binding is deleted", func() { // generate master resource snapshot masterSnapshot := generateResourceSnapshot(1, 1, 0, [][]byte{ testResourceCRD, testNameSpace, testResource, @@ -310,6 +310,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "1", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -366,7 +367,7 @@ var _ = Describe("Test Work Generator Controller", func() { verifyBindingStatusSyncedNotApplied(binding, false, false) }) - Context("Test Bound ClusterResourceBinding with failed to apply manifests", func() { + Context("Test Bound ClusterResourceBinding with manifests go from not applied to available", func() { work := placementv1beta1.Work{} BeforeEach(func() { // check the binding status till the bound condition is true @@ -403,6 +404,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "1", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -452,6 +454,65 @@ var _ = Describe("Test Work Generator Controller", func() { // check the binding status that it should be marked as available true eventually verifyBindStatusAvail(binding, false) }) + + It("Should continue to update the binding status even if the master resource snapshot is deleted after the work is synced", func() { + // delete the snapshot after the work is synced with binding + Expect(k8sClient.Delete(ctx, masterSnapshot)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{})) + // mark the work applied which should trigger a reconcile loop and copy the status from the work to the binding + markWorkApplied(&work) + // check the binding status that it should be marked as applied true eventually + verifyBindStatusAppliedNotAvailable(binding, false) + // delete the ParentResourceSnapshotNameAnnotation after the work is synced with binding + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName), Namespace: memberClusterNamespaceName}, &work)).Should(Succeed()) + delete(work.Annotations, placementv1beta1.ParentResourceSnapshotNameAnnotation) + Expect(k8sClient.Update(ctx, &work)).Should(Succeed()) + // mark the work available which should trigger a reconcile loop and copy the status from the work to the binding even if the work has no annotation + markWorkAvailable(&work) + // check the binding status that it should be marked as available true eventually + verifyBindStatusAvail(binding, false) + }) + + It("Should mark the binding as failed to sync if the master resource snapshot does not exist and the work do not sync ", func() { + // delete the snapshot after the work is synced with binding + Expect(k8sClient.Delete(ctx, masterSnapshot)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{})) + // mark the work applied which should trigger a reconcile loop and copy the status from the work to the binding + markWorkApplied(&work) + // check the binding status that it should be marked as applied true eventually + verifyBindStatusAppliedNotAvailable(binding, false) + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) + // update the resource snapshot to the next version that doesn't exist + binding.Spec.ResourceSnapshotName = "next" + Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) + updateRolloutStartedGeneration(&binding) + // check the binding status that it should be marked as override succeed but not synced + Eventually(func() string { + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) + wantStatus := placementv1beta1.ResourceBindingStatus{ + Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, + { + Type: string(placementv1beta1.ResourceBindingOverridden), + Status: metav1.ConditionTrue, + Reason: condition.OverrideNotSpecifiedReason, + ObservedGeneration: binding.GetGeneration(), + }, + { + Type: string(placementv1beta1.ResourceBindingWorkSynchronized), + Status: metav1.ConditionFalse, + Reason: condition.SyncWorkFailedReason, + ObservedGeneration: binding.GetGeneration(), + }, + }, + FailedPlacements: nil, + } + return cmp.Diff(wantStatus, binding.Status, cmpConditionOption) + }, timeout, interval).Should(BeEmpty(), fmt.Sprintf("binding(%s) mismatch (-want +got)", binding.Name)) + }) }) }) @@ -504,6 +565,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "1", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -547,6 +609,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.EnvelopeNamespaceLabel: "app", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -629,6 +692,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "2", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -671,6 +735,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.EnvelopeNamespaceLabel: "app", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -815,6 +880,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentBindingLabel: binding.Name, }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -889,6 +955,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentBindingLabel: binding.Name, }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, }, @@ -1187,6 +1254,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "1", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: croHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: roHash, }, @@ -1337,7 +1405,6 @@ var _ = Describe("Test Work Generator Controller", func() { // binding should have a finalizer Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) Expect(len(binding.Finalizers)).Should(Equal(1)) - Eventually(func() string { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) wantStatus := placementv1beta1.ResourceBindingStatus{ @@ -1419,6 +1486,7 @@ var _ = Describe("Test Work Generator Controller", func() { placementv1beta1.ParentResourceSnapshotIndexLabel: "1", }, Annotations: map[string]string{ + placementv1beta1.ParentResourceSnapshotNameAnnotation: binding.Spec.ResourceSnapshotName, placementv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: emptyHash, placementv1beta1.ParentResourceOverrideSnapshotHashAnnotation: emptyHash, },