From a79f6f1353572a1916393f0b68605719a10451f8 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 3 Mar 2021 09:45:21 -0600 Subject: [PATCH] =?UTF-8?q?=E2=9A=A0=EF=B8=8F=20Change=20client.Patch=20to?= =?UTF-8?q?=20take=20client.Object=20for=20performance=20(#1395)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ Add a fast path for client.Object in merge-patch Most uses of merge-patch will be for built-in types or kubebuilder- generated custom resources which have accessors for ResourceVersion. Using these with DeepCopyObject() is simple and fast. name old time/op new time/op delta MergeFrom/NoOptions 91.6µs ± 4% 107.3µs ±26% ~ (p=0.417 n=7+10) MergeFrom/NoOptions-2 112µs ±18% 88µs ±11% -21.15% (p=0.000 n=10+9) MergeFrom/WithOptimisticLock 163µs ± 3% 121µs ± 3% -25.66% (p=0.000 n=10+8) MergeFrom/WithOptimisticLock-2 137µs ± 4% 101µs ± 4% -26.28% (p=0.000 n=10+10) name old alloc/op new alloc/op delta MergeFrom/NoOptions 20.3kB ± 0% 20.3kB ± 0% ~ (all equal) MergeFrom/NoOptions-2 20.3kB ± 0% 20.3kB ± 0% ~ (all equal) MergeFrom/WithOptimisticLock 34.1kB ± 0% 26.7kB ± 0% -21.89% (p=0.000 n=10+10) MergeFrom/WithOptimisticLock-2 34.2kB ± 0% 26.7kB ± 0% -21.89% (p=0.000 n=10+8) name old allocs/op new allocs/op delta MergeFrom/NoOptions 359 ± 0% 359 ± 0% ~ (all equal) MergeFrom/NoOptions-2 359 ± 0% 359 ± 0% ~ (all equal) MergeFrom/WithOptimisticLock 579 ± 0% 390 ± 0% -32.64% (p=0.000 n=10+10) MergeFrom/WithOptimisticLock-2 579 ± 0% 390 ± 0% -32.64% (p=0.000 n=10+10) * ⚠ Change client.Patch to take client.Object This allows us to use metav1 accessors to speed up the merge-patch implementation. --- pkg/client/interfaces.go | 2 +- pkg/client/patch.go | 53 +++++------ pkg/client/patch_test.go | 95 +++++++++++++++++++ .../controllerutil/controllerutil.go | 4 +- 4 files changed, 121 insertions(+), 33 deletions(-) create mode 100644 pkg/client/patch_test.go diff --git a/pkg/client/interfaces.go b/pkg/client/interfaces.go index 09636968f1..4dc6eb79a0 100644 --- a/pkg/client/interfaces.go +++ b/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? diff --git a/pkg/client/patch.go b/pkg/client/patch.go index c32a06c06d..260db896fb 100644 --- a/pkg/client/patch.go +++ b/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) diff --git a/pkg/client/patch_test.go b/pkg/client/patch_test.go new file mode 100644 index 0000000000..7f86d2357a --- /dev/null +++ b/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) + } + }) +} diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index 462781bd37..07bf8e632d 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/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.