feat: enable work generator to process apply strategy updates + DiffReported condition #1016
Conversation
| @@ -700,15 +703,16 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee | |||
| } else { | |||
| // we already checked the label in fetchAllResourceSnapShots function so no need to check again | |||
| resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot) | |||
| if workResourceIndex == resourceIndex { | |||
| // no need to do anything if the work is generated from the same resource/override snapshots | |||
| if workResourceIndex == resourceIndex && equality.Semantic.DeepEqual(existingWork.Spec.ApplyStrategy, newWork.Spec.ApplyStrategy) { | |||
There was a problem hiding this comment.
should we also check applyStrategy in the areAllWorkSynced() function?
There was a problem hiding this comment.
Ah, this is actually a tricky one; to avoid apply strategy inconsistencies the refresh has to be handled before the check. I will some edits
There was a problem hiding this comment.
do we still need this after we did the update of the applyStrategy first regardless?
| It("can update work status", func() { | ||
| work := retrieveWork(memberClusterNamespaceName, fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName)) | ||
|
|
||
| diffReportedCond := &metav1.Condition{ | ||
| Type: string(placementv1beta1.WorkConditionTypeDiffReported), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(workapplier.ManifestProcessingReportDiffResultTypeNoDiffFound), | ||
| ObservedGeneration: work.GetGeneration(), | ||
| } | ||
| refreshWorkStatus(work, nil, nil, diffReportedCond, nil) | ||
| }) |
There was a problem hiding this comment.
we won't need this (which simulated a behavior of another controller) if we explicitly set the applied to be false ourself.
There was a problem hiding this comment.
Hi Ryan! Part of the code tests the status populator, which is why the condition is being set (summarizing all DiffReported conditions from work objects).
| if updateErr := errs.Wait(); updateErr != nil { | ||
| return false, false, updateErr | ||
| } | ||
|
|
||
| // the hash256 function can can handle empty list https://go.dev/play/p/_4HW17fooXM |
There was a problem hiding this comment.
| // the hash256 function can can handle empty list https://go.dev/play/p/_4HW17fooXM | |
| // the hash256 function can handle empty list https://go.dev/play/p/_4HW17fooXM |
| if err != nil { | ||
| return err | ||
| } | ||
| if updated { |
There was a problem hiding this comment.
this will revert the applied condition on the binding. It seems that it's fine but please think through.
| @@ -700,15 +703,16 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee | |||
| } else { | |||
| // we already checked the label in fetchAllResourceSnapShots function so no need to check again | |||
| resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot) | |||
| if workResourceIndex == resourceIndex { | |||
| // no need to do anything if the work is generated from the same resource/override snapshots | |||
| if workResourceIndex == resourceIndex && equality.Semantic.DeepEqual(existingWork.Spec.ApplyStrategy, newWork.Spec.ApplyStrategy) { | |||
There was a problem hiding this comment.
do we still need this after we did the update of the applyStrategy first regardless?
| if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] && | ||
| existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] { | ||
| klog.V(2).InfoS("Work is associated with the desired resource/override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation], | ||
| klog.V(2).InfoS("Work is associated with the desired resource/override snapshots and apply strategy", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation], |
| // In the case where ReportDiff apply strategy is used, work applier will no longer update | ||
| // Applied condition on Work objects; since existing Applied conditions will all become stale, | ||
| // this function will return a False Applied condition. |
There was a problem hiding this comment.
do we still need this comment?
| // in the status, as appropriate; the DiffReported condition will not be updated. | ||
| // b) If the currently active apply strategy is ReportDiff, the work generator will update | ||
| // the DiffReported condition in the status, plus the details about diffed placements; | ||
| // the Applied condition will always be set to False, despite the fact that the work applier |
There was a problem hiding this comment.
where is " the Applied condition will always be set to False" done explicitly?
| wantUpdated bool | ||
| wantApplyStrategy *fleetv1beta1.ApplyStrategy | ||
| }{ | ||
| { |
There was a problem hiding this comment.
add a diff test case? the work has something but the binding has something else?
| updateRolloutStartedGeneration(&clusterResourceBinding) | ||
| }) | ||
|
|
||
| It("should update apply strategy on the work object", func() { |
There was a problem hiding this comment.
why not continue to check the binding status too?
| Eventually(applyStrategyUpdatedActual, timeout, interval).Should(Succeed(), "Failed to create expected work object") | ||
| }) | ||
|
|
||
| AfterAll(func() { |
There was a problem hiding this comment.
can we not reuse this AfterAll between those ITs?
Description of your changes
This PR:
a) fixes the issue where work generator will not refresh apply strategies; and
b) enables work generator to process
DiffReportedconditionI have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
E2E tests can only be added when the new work applier is enabled.