Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize PatchResponseFromRaw #2432

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions pkg/webhook/admission/defaulter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@

import (
"context"
"encoding/json"
"fmt"
"net/http"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

admissionv1 "k8s.io/api/admission/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
)
Expand Down Expand Up @@ -66,3 +72,124 @@
d.Replica = 2
}
}

/*
*
BEFORE (without isByteArrayEqual)
goos: darwin
goarch: arm64
pkg: sigs.k8s.io/controller-runtime/pkg/webhook/admission
BenchmarkHandle
BenchmarkHandle/benchmark_handle_function
BenchmarkHandle/benchmark_handle_function-10 22878 50394 ns/op

AFTER (with isByteArrayEqual)
goos: darwin
goarch: arm64
pkg: sigs.k8s.io/controller-runtime/pkg/webhook/admission
BenchmarkHandle
BenchmarkHandle/benchmark_handle_function
BenchmarkHandle/benchmark_handle_function-10 44620 26178 ns/op
*/
func BenchmarkHandle(b *testing.B) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two benchmarks are not representative, as you are only benchmarking the "objects are equal case" and not the "objects are not equal", in which case your change worsens performance

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your prompt reply @alvaroaleman
I've add more benchmark tests regarding equal case and not equal case.

BenchmarkPatchResponseFromRawAndIsByteArrayEqual/benchmark_PatchResponseFromRaw_function-10         	   52774	     22749 ns/op
BenchmarkPatchResponseFromRawAndIsByteArrayEqual/benchmark_isByteArrayEqual_function-10             	 5252599	       230.7 ns/op

To add more details to this:

  1. By benchmarking, comparing two byte arrays takes far less time than the recursive reflect.DeepEqual, which is about 1/100 according to my above benchmark
  2. Most of the Apiserver's patch/update requests will still get same object after going through all mutating webhooks. as mutating webhooks most of the time take effect at the beginning of a Pod's lifecycle (or other resources lifecycle)
  3. In our production cluster, this change "reducing recursive reflect.DeepEqual" can save more than 40% cpu time for webhook.

pod := createPodForBenchmark()

byteArray, err := json.Marshal(pod)
if err != nil {
fmt.Println("error marshalling pod")
}

pa := &podAnnotator{}
handler := WithCustomDefaulter(admissionScheme, pod, pa)

b.Run("benchmark handle function", func(b *testing.B) {

Check failure on line 105 in pkg/webhook/admission/defaulter_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary leading newline (whitespace)

b.ResetTimer()
for i := 0; i < b.N; i++ {

Check failure on line 108 in pkg/webhook/admission/defaulter_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary leading newline (whitespace)

// handler.Handle invokes PatchResponseFromRaw function, which is the func to be benchmarked.
handler.Handle(context.TODO(), Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Operation: admissionv1.Update,
Object: runtime.RawExtension{
Raw: byteArray,
},
},
})
}
})
}

// podAnnotator does nothing to a pod.
// it tries to imitate the behavior that pod is unchanged after going through mutating webhook.
type podAnnotator struct{}

func (a *podAnnotator) Default(ctx context.Context, obj runtime.Object) error {

Check failure on line 127 in pkg/webhook/admission/defaulter_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary leading newline (whitespace)

pod, ok := obj.(*corev1.Pod)
if !ok {
return fmt.Errorf("expected a Pod but got a %T", obj)
}

if pod.Annotations == nil {
pod.Annotations = map[string]string{}
}

return nil
}

// Create a dummy pod for benchmarking patch
func createPodForBenchmark() *corev1.Pod {
pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "test-pod-name",
Labels: map[string]string{
"appname0": "app-name",
"appname1": "app-name",
"appname2": "app-name",
"appname3": "app-name",
"appname4": "app-name",
"appname5": "app-name",
"appname6": "app-name",
},
Annotations: map[string]string{
"annoKey": "annoValue",
"annoKey1": "annoValue",
"annoKey2": "annoValue",
"annoKey3": "annoValue",
"annoKey4": "annoValue",
"annoKey5": "annoValue",
"annoKey6": "annoValue",
},
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "main",
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("25Gi"),
},
},
},
{
Name: "sidecar",
Env: []corev1.EnvVar{{Name: "sampleEnv", Value: "true"}},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("100m"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("2Gi"),
},
},
},
},
},
}
return pod
}
24 changes: 24 additions & 0 deletions pkg/webhook/admission/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,17 @@
// The original object should be passed in as raw bytes to avoid the roundtripping problem
// described in https://github.com/kubernetes-sigs/kubebuilder/issues/510.
func PatchResponseFromRaw(original, current []byte) Response {
if isByteArrayEqual(original, current) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isByteArrayEqual(original, current) {
if bytes.Equal(original, current) {

return Response{
Patches: []jsonpatch.JsonPatchOperation{},
AdmissionResponse: admissionv1.AdmissionResponse{
Allowed: true,
PatchType: func() *admissionv1.PatchType {
return nil
}(),
},
}
}
patches, err := jsonpatch.CreatePatch(original, current)
if err != nil {
return Errored(http.StatusInternalServerError, err)
Expand All @@ -105,6 +116,19 @@
}
}

// isByteArrayEqual takes 2 byte arrays and returns if they are equal.
func isByteArrayEqual(a, b []byte) bool {
if len(a) != len(b) {
return false
}
for i, _ := range a {

Check failure on line 124 in pkg/webhook/admission/response.go

View workflow job for this annotation

GitHub Actions / lint

File is not `gofmt`-ed with `-s` (gofmt)
if a[i] != b[i] {
return false
}
}
return true
}

// validationResponseFromStatus returns a response for admitting a request with provided Status object.
func validationResponseFromStatus(allowed bool, status metav1.Status) Response {
resp := Response{
Expand Down
38 changes: 38 additions & 0 deletions pkg/webhook/admission/response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
package admission

import (
"encoding/json"
"errors"
"fmt"
"net/http"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -247,3 +250,38 @@
})
})
})

/*
*
BEFORE (without isByteArrayEqual)
goos: darwin
goarch: arm64
pkg: sigs.k8s.io/controller-runtime/pkg/webhook/admission
BenchmarkPatchResponseFromRaw
BenchmarkPatchResponseFromRaw/benchmark_PatchResponseFromRaw_function
BenchmarkPatchResponseFromRaw/benchmark_PatchResponseFromRaw_function-10 52371 22572 ns/op

AFTER (with isByteArrayEqual)
goos: darwin
goarch: arm64
pkg: sigs.k8s.io/controller-runtime/pkg/webhook/admission
BenchmarkPatchResponseFromRaw
BenchmarkPatchResponseFromRaw/benchmark_PatchResponseFromRaw_function
BenchmarkPatchResponseFromRaw/benchmark_PatchResponseFromRaw_function-10 5078506 235.4 ns/op
*/
func BenchmarkPatchResponseFromRaw(b *testing.B) {
pod := createPodForBenchmark()

byteArray, err := json.Marshal(pod)
if err != nil {
fmt.Println("error marshalling pod")
}

b.Run("benchmark PatchResponseFromRaw function", func(b *testing.B) {

Check failure on line 280 in pkg/webhook/admission/response_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary leading newline (whitespace)

b.ResetTimer()
for i := 0; i < b.N; i++ {
PatchResponseFromRaw(byteArray, byteArray)
}
})
}
Loading