Skip to content

Commit

Permalink
make sure the binding is already sycned
Browse files Browse the repository at this point in the history
  • Loading branch information
Ryan Zhang committed Oct 25, 2024
1 parent ddb950a commit 251b3a3
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 15 deletions.
7 changes: 5 additions & 2 deletions apis/placement/v1beta1/commons.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
38 changes: 28 additions & 10 deletions pkg/controllers/workgenerator/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,18 +136,17 @@ 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)
if syncErr == nil {
// 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"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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
Expand All @@ -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,
},
Expand Down Expand Up @@ -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
Expand Down
74 changes: 71 additions & 3 deletions pkg/controllers/workgenerator/controller_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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))
})
})
})

Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
},
Expand Down

0 comments on commit 251b3a3

Please sign in to comment.