fix: refactor the work applier code to reduce conditional checking + address an issue in work applier where some errors might not get properly surfaced in ReportDiff mode (#183)

Author: michaelawyu

pkg/controllers/workapplier/availability_tracker.go

@@ -48,7 +48,7 @@ func (r *Reconciler) trackInMemberClusterObjAvailability(ctx context.Context, bu
 
 	doWork := func(pieces int) {
 		bundle := bundles[pieces]
-		if !isManifestObjectApplied(bundle.applyResTyp) {
+		if !isManifestObjectApplied(bundle.applyOrReportDiffResTyp) {
 			// The manifest object in the bundle has not been applied yet. No availability check
 			// is needed.
 			bundle.availabilityResTyp = ManifestProcessingAvailabilityResultTypeSkipped

pkg/controllers/workapplier/availability_tracker_test.go

@@ -1047,74 +1047,74 @@ func TestTrackInMemberClusterObjAvailability(t *testing.T) {
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 0,
 					},
-					gvr:                &utils.DeploymentGVR,
-					inMemberClusterObj: toUnstructured(t, availableDeploy),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
+					gvr:                     &utils.DeploymentGVR,
+					inMemberClusterObj:      toUnstructured(t, availableDeploy),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
 				},
 				// A failed to get applied service.
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 1,
 					},
-					gvr:                &utils.ServiceGVR,
-					inMemberClusterObj: nil,
-					applyResTyp:        ManifestProcessingApplyResultTypeFailedToApply,
+					gvr:                     &utils.ServiceGVR,
+					inMemberClusterObj:      nil,
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeFailedToApply,
 				},
 				// An unavailable daemon set.
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 2,
 					},
-					gvr:                &utils.DaemonSetGVR,
-					inMemberClusterObj: toUnstructured(t, unavailableDaemonSet),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
+					gvr:                     &utils.DaemonSetGVR,
+					inMemberClusterObj:      toUnstructured(t, unavailableDaemonSet),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
 				},
 				// An untrackable job.
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 3,
 					},
-					gvr:                &utils.JobGVR,
-					inMemberClusterObj: toUnstructured(t, untrackableJob),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
+					gvr:                     &utils.JobGVR,
+					inMemberClusterObj:      toUnstructured(t, untrackableJob),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
 				},
 			},
 			wantBundles: []*manifestProcessingBundle{
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 0,
 					},
-					gvr:                &utils.DeploymentGVR,
-					inMemberClusterObj: toUnstructured(t, availableDeploy),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
-					availabilityResTyp: ManifestProcessingAvailabilityResultTypeAvailable,
+					gvr:                     &utils.DeploymentGVR,
+					inMemberClusterObj:      toUnstructured(t, availableDeploy),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
+					availabilityResTyp:      ManifestProcessingAvailabilityResultTypeAvailable,
 				},
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 1,
 					},
-					gvr:                &utils.ServiceGVR,
-					inMemberClusterObj: nil,
-					applyResTyp:        ManifestProcessingApplyResultTypeFailedToApply,
-					availabilityResTyp: ManifestProcessingAvailabilityResultTypeSkipped,
+					gvr:                     &utils.ServiceGVR,
+					inMemberClusterObj:      nil,
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeFailedToApply,
+					availabilityResTyp:      ManifestProcessingAvailabilityResultTypeSkipped,
 				},
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 2,
 					},
-					gvr:                &utils.DaemonSetGVR,
-					inMemberClusterObj: toUnstructured(t, unavailableDaemonSet),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
-					availabilityResTyp: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+					gvr:                     &utils.DaemonSetGVR,
+					inMemberClusterObj:      toUnstructured(t, unavailableDaemonSet),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
+					availabilityResTyp:      ManifestProcessingAvailabilityResultTypeNotYetAvailable,
 				},
 				{
 					id: &fleetv1beta1.WorkResourceIdentifier{
 						Ordinal: 3,
 					},
-					gvr:                &utils.JobGVR,
-					inMemberClusterObj: toUnstructured(t, untrackableJob),
-					applyResTyp:        ManifestProcessingApplyResultTypeApplied,
-					availabilityResTyp: ManifestProcessingAvailabilityResultTypeNotTrackable,
+					gvr:                     &utils.JobGVR,
+					inMemberClusterObj:      toUnstructured(t, untrackableJob),
+					applyOrReportDiffResTyp: ApplyOrReportDiffResTypeApplied,
+					availabilityResTyp:      ManifestProcessingAvailabilityResultTypeNotTrackable,
 				},
 			},
 		},

pkg/controllers/workapplier/backoff.go

@@ -60,7 +60,7 @@ const (
 )
 
 const (
-	processingResultStrTpl = "%s,%s,%s"
+	processingResultStrTpl = "%s,%s"
 )
 
 // RequeueMultiStageWithExponentialBackoffRateLimiter is a rate limiter that allows requeues of various
@@ -287,7 +287,7 @@ func computeProcessingResultHash(work *fleetv1beta1.Work, bundles []*manifestPro
 	// The order of manifests is stable in a bundle.
 	processingResults := make([]string, 0, len(bundles))
 	for _, bundle := range bundles {
-		processingResults = append(processingResults, fmt.Sprintf(processingResultStrTpl, bundle.applyResTyp, bundle.availabilityResTyp, bundle.reportDiffResTyp))
+		processingResults = append(processingResults, fmt.Sprintf(processingResultStrTpl, bundle.applyOrReportDiffResTyp, bundle.availabilityResTyp))
 	}
 
 	processingResHash, err := resource.HashOf(processingResults)