From aaf608fa583984878cbc9063aa23cc7d2901f992 Mon Sep 17 00:00:00 2001 From: michaelawyu Date: Wed, 18 Dec 2024 06:17:03 +0800 Subject: [PATCH] Minor changes --- .../workapplier/availability_tracker.go | 6 +- pkg/controllers/workapplier/controller.go | 7 +- .../workapplier/drift_detection_takeover.go | 93 +++++++------------ pkg/controllers/workapplier/utils.go | 5 +- 4 files changed, 42 insertions(+), 69 deletions(-) diff --git a/pkg/controllers/workapplier/availability_tracker.go b/pkg/controllers/workapplier/availability_tracker.go index f0ed40c03..d5b2a9326 100644 --- a/pkg/controllers/workapplier/availability_tracker.go +++ b/pkg/controllers/workapplier/availability_tracker.go @@ -20,7 +20,7 @@ import ( "go.goms.io/fleet/pkg/utils/controller" ) -func (r *Reconciler) trackInMemberClusterObjAvailability(ctx context.Context, bundles []*manifestProcessingBundle, workRef *klog.ObjectRef) { +func (r *Reconciler) trackInMemberClusterObjAvailability(ctx context.Context, bundles []*manifestProcessingBundle, workRef klog.ObjectRef) { // Track the availability of all the applied objects in the member cluster in parallel. // // This is concurrency-safe as the bundles slice has been pre-allocated. @@ -46,12 +46,12 @@ func (r *Reconciler) trackInMemberClusterObjAvailability(ctx context.Context, bu bundle.availabilityResTyp = ManifestProcessingAvailabilityResultTypeFailed klog.ErrorS(err, "Failed to track the availability of the applied object in the member cluster", - "work", *workRef, "GVR", *bundle.gvr, "inMemberClusterObj", klog.KObj(bundle.inMemberClusterObj)) + "work", workRef, "GVR", *bundle.gvr, "inMemberClusterObj", klog.KObj(bundle.inMemberClusterObj)) return } bundle.availabilityResTyp = availabilityResTyp klog.V(2).InfoS("Tracked availability of a resource", - "work", *workRef, "GVR", *bundle.gvr, "inMemberClusterObj", klog.KObj(bundle.inMemberClusterObj), + "work", workRef, "GVR", *bundle.gvr, "inMemberClusterObj", klog.KObj(bundle.inMemberClusterObj), "availabilityResTyp", availabilityResTyp) } diff --git a/pkg/controllers/workapplier/controller.go b/pkg/controllers/workapplier/controller.go index e38167a08..4db9364d6 100644 --- a/pkg/controllers/workapplier/controller.go +++ b/pkg/controllers/workapplier/controller.go @@ -280,7 +280,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.processManifests(ctx, bundles, work, expectedAppliedWorkOwnerRef) // Track the availability information. - r.trackInMemberClusterObjAvailability(ctx, bundles, &workRef) + r.trackInMemberClusterObjAvailability(ctx, bundles, workRef) trackWorkApplyLatencyMetric(work) @@ -322,11 +322,8 @@ func (r *Reconciler) preProcessManifests( defer cancel() doWork := func(pieces int) { + // At this moment the bundles are just created. bundle := bundles[pieces] - if bundle.applyErr != nil { - // Skip a manifest if it cannot be processed. - return - } gvr, manifestObj, err := r.decodeManifest(bundle.manifest) // Build the identifier. Note that this would return an identifier even if the decoding diff --git a/pkg/controllers/workapplier/drift_detection_takeover.go b/pkg/controllers/workapplier/drift_detection_takeover.go index 9cbabdbaf..b6735d3d3 100644 --- a/pkg/controllers/workapplier/drift_detection_takeover.go +++ b/pkg/controllers/workapplier/drift_detection_takeover.go @@ -89,7 +89,9 @@ func (r *Reconciler) diffBetweenManifestAndInMemberClusterObjects( case fleetv1beta1.ComparisonOptionTypePartialComparison: return r.partialDiffBetweenManifestAndInMemberClusterObjects(ctx, gvr, manifestObj, inMemberClusterObj) case fleetv1beta1.ComparisonOptionTypeFullComparison: - return fullDiffBetweenManifestAndInMemberClusterObjects(manifestObj, inMemberClusterObj) + // For the full comparison, Fleet compares directly the JSON representations of the + // manifest object and the object in the member cluster. + return preparePatchDetails(manifestObj, inMemberClusterObj) default: return nil, fmt.Errorf("an invalid comparison option is specified") } @@ -115,64 +117,8 @@ func (r *Reconciler) partialDiffBetweenManifestAndInMemberClusterObjects( // imply that running an actual apply op would lead to unexpected changes, which signifies // the presence of partial drifts (drifts in managed fields). - // Discard certain fields from both objects before comparison. - inMemberClusterObjCopy := discardFieldsIrrelevantInComparisonFrom(inMemberClusterObj) - appliedObjCopy := discardFieldsIrrelevantInComparisonFrom(appliedObj) - - // Marshal the objects into JSON. - inMemberClusterObjCopyJSONBytes, err := inMemberClusterObjCopy.MarshalJSON() - if err != nil { - return nil, fmt.Errorf("failed to marshal the object from the member cluster into JSON: %w", err) - } - appliedObjJSONBytes, err := appliedObjCopy.MarshalJSON() - if err != nil { - return nil, fmt.Errorf("failed to marshal the object returned by the dry-run apply op into JSON: %w", err) - } - - // Compare the JSON representations. - patch, err := jsondiff.CompareJSON(appliedObjJSONBytes, inMemberClusterObjCopyJSONBytes) - if err != nil { - return nil, fmt.Errorf("failed to compare the JSON representations of the the object from the member cluster and the object returned by the dry-run apply op: %w", err) - } - - details, err := organizeJSONPatchIntoFleetPatchDetails(patch, appliedObjCopy.Object, inMemberClusterObj.Object) - if err != nil { - return nil, fmt.Errorf("failed to organize the JSON patch into Fleet patch details: %w", err) - } - return details, nil -} - -func fullDiffBetweenManifestAndInMemberClusterObjects( - manifestObj, inMemberClusterObj *unstructured.Unstructured, -) ([]fleetv1beta1.PatchDetail, error) { - // Fleet calculates the full diff between two objects by directly comparing their JSON - // representations. - - // Discard certain fields from both objects before comparison. - manifestObjCopy := discardFieldsIrrelevantInComparisonFrom(manifestObj) - inMemberClusterObjCopy := discardFieldsIrrelevantInComparisonFrom(inMemberClusterObj) - - // Marshal the objects into JSON. - manifestObjJSONBytes, err := manifestObjCopy.MarshalJSON() - if err != nil { - return nil, fmt.Errorf("failed to marshal the manifest object into JSON: %w", err) - } - inMemberClusterObjJSONBytes, err := inMemberClusterObjCopy.MarshalJSON() - if err != nil { - return nil, fmt.Errorf("failed to marshal the object from the member cluster into JSON: %w", err) - } - - // Compare the JSON representations. - patch, err := jsondiff.CompareJSON(manifestObjJSONBytes, inMemberClusterObjJSONBytes) - if err != nil { - return nil, fmt.Errorf("failed to compare the JSON representations of the manifest object and the object from the member cluster: %w", err) - } - - details, err := organizeJSONPatchIntoFleetPatchDetails(patch, manifestObjCopy.Object, inMemberClusterObj.Object) - if err != nil { - return nil, fmt.Errorf("failed to organize JSON patch operations into Fleet patch details: %w", err) - } - return details, nil + // Prepare the patch details. + return preparePatchDetails(appliedObj, inMemberClusterObj) } func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap, inMemberClusterObjMap map[string]interface{}) ([]fleetv1beta1.PatchDetail, error) { @@ -263,3 +209,32 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap return details, nil } + +func preparePatchDetails(srcObj, destObj *unstructured.Unstructured) ([]fleetv1beta1.PatchDetail, error) { + // Discard certain fields from both objects before comparison. + srcObjCopy := discardFieldsIrrelevantInComparisonFrom(srcObj) + destObjCopy := discardFieldsIrrelevantInComparisonFrom(destObj) + + // Marshal the objects into JSON. + srcObjJSONBytes, err := srcObjCopy.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("failed to marshal the source object into JSON: %w", err) + } + + destObjJSONBytes, err := destObjCopy.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("failed to marshal the destination object into JSON: %w", err) + } + + // Compare the JSON representations. + patch, err := jsondiff.CompareJSON(srcObjJSONBytes, destObjJSONBytes) + if err != nil { + return nil, fmt.Errorf("failed to compare the JSON representations of the source and destination objects: %w", err) + } + + details, err := organizeJSONPatchIntoFleetPatchDetails(patch, srcObjCopy.Object, destObjCopy.Object) + if err != nil { + return nil, fmt.Errorf("failed to organize JSON patch operations into Fleet patch details: %w", err) + } + return details, nil +} diff --git a/pkg/controllers/workapplier/utils.go b/pkg/controllers/workapplier/utils.go index 1f0f8c110..63eaddc5d 100644 --- a/pkg/controllers/workapplier/utils.go +++ b/pkg/controllers/workapplier/utils.go @@ -473,8 +473,9 @@ func shouldPerformPreApplyDriftDetection(manifestObj, inMemberClusterObj *unstru // * Fleet reports that the manifest has been applied before (i.e., inMemberClusterObj exists); and // * The apply strategy dictates that an apply op should only run if there is no // detected drift; and - // * The hash of the manifest object is consistently with that bookkept in the live object - // annotations (i.e., the manifest object has been applied before). + // * The hash of the manifest object is consistent with the last applied manifest object hash + // annotation on the corresponding resource in the member cluster (i.e., the same manifest + // object has been applied before). if applyStrategy.WhenToApply != fleetv1beta1.WhenToApplyTypeIfNotDrifted || inMemberClusterObj == nil { // A shortcut to save some overhead. return false, nil