From b15e43251bbfdd8823c789760ca2be6836f21f66 Mon Sep 17 00:00:00 2001 From: whitewindmills Date: Tue, 4 Apr 2023 11:45:37 +0800 Subject: [PATCH] use patch helper instead of jsonpatch Signed-off-by: whitewindmills --- pkg/scheduler/scheduler.go | 66 ++++++++++++++++------------------- pkg/util/helper/patch.go | 9 +++-- pkg/util/helper/patch_test.go | 31 ++++++++++------ 3 files changed, 57 insertions(+), 49 deletions(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 59b8b4c7d..0298a4c64 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -8,7 +8,6 @@ import ( "reflect" "time" - jsonpatch "github.com/evanphx/json-patch/v5" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -505,19 +504,11 @@ func (s *Scheduler) patchScheduleResultForResourceBinding(oldBinding *workv1alph newBinding.Annotations[util.PolicyPlacementAnnotation] = placement newBinding.Spec.Clusters = scheduleResult - oldData, err := json.Marshal(oldBinding) + patchBytes, err := helper.GenMergePatch(oldBinding, newBinding) if err != nil { - return fmt.Errorf("failed to marshal the existing resource binding(%s/%s): %v", oldBinding.Namespace, oldBinding.Name, err) + return err } - newData, err := json.Marshal(newBinding) - if err != nil { - return fmt.Errorf("failed to marshal the new resource binding(%s/%s): %v", newBinding.Namespace, newBinding.Name, err) - } - patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) - if err != nil { - return fmt.Errorf("failed to create a merge patch: %v", err) - } - if "{}" == string(patchBytes) { + if len(patchBytes) == 0 { return nil } @@ -649,19 +640,11 @@ func (s *Scheduler) patchScheduleResultForClusterResourceBinding(oldBinding *wor newBinding.Annotations[util.PolicyPlacementAnnotation] = placement newBinding.Spec.Clusters = scheduleResult - oldData, err := json.Marshal(oldBinding) + patchBytes, err := helper.GenMergePatch(oldBinding, newBinding) if err != nil { - return fmt.Errorf("failed to marshal the existing cluster resource binding(%s): %v", oldBinding.Name, err) + return err } - newData, err := json.Marshal(newBinding) - if err != nil { - return fmt.Errorf("failed to marshal the new resource binding(%s): %v", newBinding.Name, err) - } - patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) - if err != nil { - return fmt.Errorf("failed to create a merge patch: %v", err) - } - if "{}" == string(patchBytes) { + if len(patchBytes) == 0 { return nil } @@ -727,27 +710,32 @@ func patchBindingStatusCondition(karmadaClient karmadaclientset.Interface, rb *w if newScheduledCondition.Status == metav1.ConditionTrue { updateRB.Status.SchedulerObservedGeneration = rb.Generation } + + if reflect.DeepEqual(rb.Status, updateRB.Status) { + return nil + } return patchBindingStatus(karmadaClient, rb, updateRB) } // patchBindingStatusWithAffinityName patches schedule status with affinityName of ResourceBinding when necessary. func patchBindingStatusWithAffinityName(karmadaClient karmadaclientset.Interface, rb *workv1alpha2.ResourceBinding, affinityName string) error { - klog.V(4).Infof("Begin to patch status with affinityName(%s) to ResourceBinding(%s/%s).", affinityName, rb.Namespace, rb.Name) + if rb.Status.SchedulerObservedAffinityName == affinityName { + return nil + } + klog.V(4).Infof("Begin to patch status with affinityName(%s) to ResourceBinding(%s/%s).", affinityName, rb.Namespace, rb.Name) updateRB := rb.DeepCopy() updateRB.Status.SchedulerObservedAffinityName = affinityName return patchBindingStatus(karmadaClient, rb, updateRB) } func patchBindingStatus(karmadaClient karmadaclientset.Interface, rb, updateRB *workv1alpha2.ResourceBinding) error { - // Short path, ignore patch if no change. - if reflect.DeepEqual(rb.Status, updateRB.Status) { - return nil - } - patchBytes, err := helper.GenMergePatch(rb, updateRB) if err != nil { - return fmt.Errorf("failed to create a merge patch: %v", err) + return err + } + if len(patchBytes) == 0 { + return nil } _, err = karmadaClient.WorkV1alpha2().ResourceBindings(rb.Namespace).Patch(context.TODO(), rb.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status") @@ -771,11 +759,19 @@ func patchClusterBindingStatusCondition(karmadaClient karmadaclientset.Interface if newScheduledCondition.Status == metav1.ConditionTrue { updateCRB.Status.SchedulerObservedGeneration = crb.Generation } + + if reflect.DeepEqual(crb.Status, updateCRB.Status) { + return nil + } return patchClusterResourceBindingStatus(karmadaClient, crb, updateCRB) } // patchClusterBindingStatusWithAffinityName patches schedule status with affinityName of ClusterResourceBinding when necessary. func patchClusterBindingStatusWithAffinityName(karmadaClient karmadaclientset.Interface, crb *workv1alpha2.ClusterResourceBinding, affinityName string) error { + if crb.Status.SchedulerObservedAffinityName == affinityName { + return nil + } + klog.V(4).Infof("Begin to patch status with affinityName(%s) to ClusterResourceBinding(%s).", affinityName, crb.Name) updateCRB := crb.DeepCopy() updateCRB.Status.SchedulerObservedAffinityName = affinityName @@ -783,14 +779,12 @@ func patchClusterBindingStatusWithAffinityName(karmadaClient karmadaclientset.In } func patchClusterResourceBindingStatus(karmadaClient karmadaclientset.Interface, crb, updateCRB *workv1alpha2.ClusterResourceBinding) error { - // Short path, ignore patch if no change. - if reflect.DeepEqual(crb.Status, updateCRB.Status) { - return nil - } - patchBytes, err := helper.GenMergePatch(crb, updateCRB) if err != nil { - return fmt.Errorf("failed to create a merge patch: %v", err) + return err + } + if len(patchBytes) == 0 { + return nil } _, err = karmadaClient.WorkV1alpha2().ClusterResourceBindings().Patch(context.TODO(), crb.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status") diff --git a/pkg/util/helper/patch.go b/pkg/util/helper/patch.go index 1e410e269..039ddb11a 100644 --- a/pkg/util/helper/patch.go +++ b/pkg/util/helper/patch.go @@ -11,7 +11,7 @@ import ( // original object to the modified object. // The merge patch format is primarily intended for use with the HTTP PATCH method // as a means of describing a set of modifications to a target resource's content. -func GenMergePatch(originalObj interface{}, modifiedObj interface{}) (patchBytes []byte, err error) { +func GenMergePatch(originalObj interface{}, modifiedObj interface{}) ([]byte, error) { originalBytes, err := json.Marshal(originalObj) if err != nil { return nil, fmt.Errorf("failed to marshal original object: %w", err) @@ -20,10 +20,15 @@ func GenMergePatch(originalObj interface{}, modifiedObj interface{}) (patchBytes if err != nil { return nil, fmt.Errorf("failed to marshal modified object: %w", err) } - patchBytes, err = jsonpatch.CreateMergePatch(originalBytes, modifiedBytes) + patchBytes, err := jsonpatch.CreateMergePatch(originalBytes, modifiedBytes) if err != nil { return nil, fmt.Errorf("failed to create a merge patch: %w", err) } + // It's unnecessary to patch. + if string(patchBytes) == "{}" { + return nil, nil + } + return patchBytes, nil } diff --git a/pkg/util/helper/patch_test.go b/pkg/util/helper/patch_test.go index 72418ec73..f64394e95 100644 --- a/pkg/util/helper/patch_test.go +++ b/pkg/util/helper/patch_test.go @@ -9,7 +9,7 @@ import ( ) func TestGenMergePatch(t *testing.T) { - testObj := workv1alpha2.ResourceBinding{ + testObj := &workv1alpha2.ResourceBinding{ TypeMeta: metav1.TypeMeta{Kind: "ResourceBinding", APIVersion: "work.karmada.io/v1alpha2"}, ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: workv1alpha2.ResourceBindingSpec{Clusters: []workv1alpha2.TargetCluster{{Name: "foo", Replicas: 20}}}, @@ -68,7 +68,7 @@ func TestGenMergePatch(t *testing.T) { modified := testObj.DeepCopy() return modified }, - expectedPatch: `{}`, + expectedPatch: "", expectErr: false, }, { @@ -77,22 +77,31 @@ func TestGenMergePatch(t *testing.T) { var invalid = 0 return invalid }, - expectedPatch: ``, // empty(nil) patch + expectedPatch: "", expectErr: true, }, + { + name: "update to empty annotations", + modifyFunc: func() interface{} { + modified := testObj.DeepCopy() + modified.Annotations = make(map[string]string, 0) + return modified + }, + expectedPatch: "", + expectErr: false, + }, } - for _, test := range tests { - tc := test - t.Run(test.name, func(t *testing.T) { - patch, err := GenMergePatch(testObj, tc.modifyFunc()) - if err != nil && tc.expectErr == false { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + patch, err := GenMergePatch(testObj, tt.modifyFunc()) + if err != nil && tt.expectErr == false { t.Fatalf("unexpect error, but got: %v", err) - } else if err == nil && tc.expectErr == true { + } else if err == nil && tt.expectErr == true { t.Fatalf("expect error, but got none") } - if string(patch) != tc.expectedPatch { - t.Fatalf("want patch: %s, but got :%s", tc.expectedPatch, string(patch)) + if string(patch) != tt.expectedPatch { + t.Fatalf("want patch: %s, but got :%s", tt.expectedPatch, string(patch)) } }) }