Merge pull request #3364 from whitewindmills/patch-util

use patch helper instead of jsonpatch

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 len(patchBytes) == 0 {
-	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) {
 		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 len(patchBytes) == 0 {
-	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) {
 		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")

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
 }

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 {
+	for _, tt := range tests {
-		tc := test
+		t.Run(tt.name, func(t *testing.T) {
-		t.Run(test.name, func(t *testing.T) {
+			patch, err := GenMergePatch(testObj, tt.modifyFunc())
-			patch, err := GenMergePatch(testObj, tc.modifyFunc())
+			if err != nil && tt.expectErr == false {
-			if err != nil && tc.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 {
+			if string(patch) != tt.expectedPatch {
-				t.Fatalf("want patch: %s, but got :%s", tc.expectedPatch, string(patch))
+				t.Fatalf("want patch: %s, but got :%s", tt.expectedPatch, string(patch))
 			}
 		})
 	}