⚠️ Change client.Patch to take client.Object for performance

pkg/client/interfaces.go

@@ -39,7 +39,7 @@ type Patch interface {
 	// Type is the PatchType of the patch.
 	Type() types.PatchType
 	// Data is the raw data representing the patch.
-	Data(obj runtime.Object) ([]byte, error)
+	Data(obj Object) ([]byte, error)
 }
 
 // TODO(directxman12): is there a sane way to deal with get/delete options?

pkg/client/patch.go

@@ -20,8 +20,6 @@ import (
 	"fmt"
 
 	jsonpatch "github.com/evanphx/json-patch"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/json"
@@ -47,7 +45,7 @@ func (s *patch) Type() types.PatchType {
 }
 
 // Data implements Patch.
-func (s *patch) Data(obj runtime.Object) ([]byte, error) {
+func (s *patch) Data(obj Object) ([]byte, error) {
 	return s.data, nil
 }
 
@@ -87,7 +85,7 @@ type MergeFromOptions struct {
 }
 
 type mergeFromPatch struct {
-	from runtime.Object
+	from Object
 	opts MergeFromOptions
 }
 
@@ -97,13 +95,29 @@ func (s *mergeFromPatch) Type() types.PatchType {
 }
 
 // Data implements Patch.
-func (s *mergeFromPatch) Data(obj runtime.Object) ([]byte, error) {
-	originalJSON, err := json.Marshal(s.from)
+func (s *mergeFromPatch) Data(obj Object) ([]byte, error) {
+	original := s.from
+	modified := obj
+
+	if s.opts.OptimisticLock {
+		version := original.GetResourceVersion()
+		if len(version) == 0 {
+			return nil, fmt.Errorf("cannot use OptimisticLock, object %q does not have any resource version we can use", original)
+		}
+
+		original = original.DeepCopyObject().(Object)
+		original.SetResourceVersion("")
+
+		modified = modified.DeepCopyObject().(Object)
+		modified.SetResourceVersion(version)
+	}
+
+	originalJSON, err := json.Marshal(original)
 	if err != nil {
 		return nil, err
 	}
 
-	modifiedJSON, err := json.Marshal(obj)
+	modifiedJSON, err := json.Marshal(modified)
 	if err != nil {
 		return nil, err
 	}
@@ -113,37 +127,16 @@ func (s *mergeFromPatch) Data(obj runtime.Object) ([]byte, error) {
 		return nil, err
 	}
 
-	if s.opts.OptimisticLock {
-		dataMap := map[string]interface{}{}
-		if err := json.Unmarshal(data, &dataMap); err != nil {
-			return nil, err
-		}
-		fromMeta, ok := s.from.(metav1.Object)
-		if !ok {
-			return nil, fmt.Errorf("cannot use OptimisticLock, from object %q is not a valid metav1.Object", s.from)
-		}
-		resourceVersion := fromMeta.GetResourceVersion()
-		if len(resourceVersion) == 0 {
-			return nil, fmt.Errorf("cannot use OptimisticLock, from object %q does not have any resource version we can use", s.from)
-		}
-		u := &unstructured.Unstructured{Object: dataMap}
-		u.SetResourceVersion(resourceVersion)
-		data, err = json.Marshal(u)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return data, nil
 }
 
 // MergeFrom creates a Patch that patches using the merge-patch strategy with the given object as base.
-func MergeFrom(obj runtime.Object) Patch {
+func MergeFrom(obj Object) Patch {
 	return &mergeFromPatch{from: obj}
 }
 
 // MergeFromWithOptions creates a Patch that patches using the merge-patch strategy with the given object as base.
-func MergeFromWithOptions(obj runtime.Object, opts ...MergeFromOption) Patch {
+func MergeFromWithOptions(obj Object, opts ...MergeFromOption) Patch {
 	options := &MergeFromOptions{}
 	for _, opt := range opts {
 		opt.ApplyToMergeFrom(options)

pkg/client/patch_test.go

@@ -0,0 +1,95 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package client
+
+import (
+	"testing"
+
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+func BenchmarkMergeFrom(b *testing.B) {
+	cm1 := &corev1.ConfigMap{}
+	cm1.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))
+	cm1.ResourceVersion = "anything"
+
+	cm2 := cm1.DeepCopy()
+	cm2.Data = map[string]string{"key": "value"}
+
+	sts1 := &appsv1.StatefulSet{}
+	sts1.SetGroupVersionKind(appsv1.SchemeGroupVersion.WithKind("StatefulSet"))
+	sts1.ResourceVersion = "somesuch"
+
+	sts2 := sts1.DeepCopy()
+	sts2.Spec.Template.Spec.Containers = []corev1.Container{{
+		Resources: corev1.ResourceRequirements{
+			Requests: map[corev1.ResourceName]resource.Quantity{
+				corev1.ResourceCPU:    resource.MustParse("1m"),
+				corev1.ResourceMemory: resource.MustParse("1M"),
+			},
+		},
+		ReadinessProbe: &corev1.Probe{
+			Handler: corev1.Handler{
+				HTTPGet: &corev1.HTTPGetAction{},
+			},
+		},
+		Lifecycle: &corev1.Lifecycle{
+			PreStop: &corev1.Handler{
+				HTTPGet: &corev1.HTTPGetAction{},
+			},
+		},
+		SecurityContext: &corev1.SecurityContext{},
+	}}
+
+	b.Run("NoOptions", func(b *testing.B) {
+		cmPatch := MergeFrom(cm1)
+		if _, err := cmPatch.Data(cm2); err != nil {
+			b.Fatalf("expected no error, got %v", err)
+		}
+
+		stsPatch := MergeFrom(sts1)
+		if _, err := stsPatch.Data(sts2); err != nil {
+			b.Fatalf("expected no error, got %v", err)
+		}
+
+		b.ResetTimer()
+		for i := 0; i < b.N; i++ {
+			_, _ = cmPatch.Data(cm2)
+			_, _ = stsPatch.Data(sts2)
+		}
+	})
+
+	b.Run("WithOptimisticLock", func(b *testing.B) {
+		cmPatch := MergeFromWithOptions(cm1, MergeFromWithOptimisticLock{})
+		if _, err := cmPatch.Data(cm2); err != nil {
+			b.Fatalf("expected no error, got %v", err)
+		}
+
+		stsPatch := MergeFromWithOptions(sts1, MergeFromWithOptimisticLock{})
+		if _, err := stsPatch.Data(sts2); err != nil {
+			b.Fatalf("expected no error, got %v", err)
+		}
+
+		b.ResetTimer()
+		for i := 0; i < b.N; i++ {
+			_, _ = cmPatch.Data(cm2)
+			_, _ = stsPatch.Data(sts2)
+		}
+	})
+}

pkg/controller/controllerutil/controllerutil.go

@@ -249,8 +249,8 @@ func CreateOrPatch(ctx context.Context, c client.Client, obj client.Object, f Mu
 	}
 
 	// Create patches for the object and its possible status.
-	objPatch := client.MergeFrom(obj.DeepCopyObject())
-	statusPatch := client.MergeFrom(obj.DeepCopyObject())
+	objPatch := client.MergeFrom(obj.DeepCopyObject().(client.Object))
+	statusPatch := client.MergeFrom(obj.DeepCopyObject().(client.Object))
 
 	// Create a copy of the original object as well as converting that copy to
 	// unstructured data.