From 7233538008489c189d09bb042fbabca97d9cdbaf Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Thu, 14 Jul 2022 11:40:20 -0700 Subject: [PATCH 1/7] revert timestamp updates to object if non-managed fields do not change add short-circuiting logic for long comaprison replace timestamps rather than doing a full managed fields deepcopy add guard --- .../apimachinery/pkg/conversion/deep_equal.go | 11 ++ .../forked/golang/reflect/deep_equal.go | 60 ++++-- .../forked/golang/reflect/deep_equal_test.go | 30 ++- .../handlers/fieldmanager/equality.go | 109 +++++++++++ .../apiserver/pkg/endpoints/handlers/patch.go | 2 +- .../pkg/endpoints/handlers/update.go | 9 + .../integration/apiserver/apply/apply_test.go | 179 ++++++++++++++++++ 7 files changed, 378 insertions(+), 22 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go diff --git a/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go b/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go index f21abe1e53a..25b2923f223 100644 --- a/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go @@ -34,3 +34,14 @@ func EqualitiesOrDie(funcs ...interface{}) Equalities { } return e } + +// Performs a shallow copy of the equalities map +func (e Equalities) Copy() Equalities { + result := Equalities{reflect.Equalities{}} + + for key, value := range e.Equalities { + result.Equalities[key] = value + } + + return result +} diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go index fb6b054792b..e25247100e7 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go @@ -100,7 +100,8 @@ func makeUsefulPanic(v reflect.Value) { // Tests for deep equality using reflected types. The map argument tracks // comparisons that have already been seen, which allows short circuiting on // recursive types. -func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, depth int) bool { +// equateNilAndEmpty controls whether empty maps/slices are equivalent to nil +func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, equateNilAndEmpty bool, depth int) bool { defer makeUsefulPanic(v1) if !v1.IsValid() || !v2.IsValid() { @@ -150,17 +151,24 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, // We don't need to check length here because length is part of // an array's type, which has already been filtered for. for i := 0; i < v1.Len(); i++ { - if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, depth+1) { + if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, equateNilAndEmpty, depth+1) { return false } } return true case reflect.Slice: - if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { - return false - } - if v1.IsNil() || v1.Len() == 0 { - return true + if equateNilAndEmpty { + if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { + return false + } + + if v1.IsNil() || v1.Len() == 0 { + return true + } + } else { + if v1.IsNil() != v2.IsNil() { + return false + } } if v1.Len() != v2.Len() { return false @@ -169,7 +177,7 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, return true } for i := 0; i < v1.Len(); i++ { - if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, depth+1) { + if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, equateNilAndEmpty, depth+1) { return false } } @@ -178,22 +186,28 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, if v1.IsNil() || v2.IsNil() { return v1.IsNil() == v2.IsNil() } - return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, depth+1) - case reflect.Pointer: - return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, depth+1) + return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, equateNilAndEmpty, depth+1) + case reflect.Ptr: + return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, equateNilAndEmpty, depth+1) case reflect.Struct: for i, n := 0, v1.NumField(); i < n; i++ { - if !e.deepValueEqual(v1.Field(i), v2.Field(i), visited, depth+1) { + if !e.deepValueEqual(v1.Field(i), v2.Field(i), visited, equateNilAndEmpty, depth+1) { return false } } return true case reflect.Map: - if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { - return false - } - if v1.IsNil() || v1.Len() == 0 { - return true + if equateNilAndEmpty { + if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { + return false + } + if v1.IsNil() || v1.Len() == 0 { + return true + } + } else { + if v1.IsNil() != v2.IsNil() { + return false + } } if v1.Len() != v2.Len() { return false @@ -202,7 +216,7 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, return true } for _, k := range v1.MapKeys() { - if !e.deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, depth+1) { + if !e.deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, equateNilAndEmpty, depth+1) { return false } } @@ -232,6 +246,14 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, // Unexported field members cannot be compared and will cause an informative panic; you must add an Equality // function for these types. func (e Equalities) DeepEqual(a1, a2 interface{}) bool { + return e.deepEqual(a1, a2, true) +} + +func (e Equalities) DeepEqualWithNilDifferentFromEmpty(a1, a2 interface{}) bool { + return e.deepEqual(a1, a2, false) +} + +func (e Equalities) deepEqual(a1, a2 interface{}, equateNilAndEmpty bool) bool { if a1 == nil || a2 == nil { return a1 == a2 } @@ -240,7 +262,7 @@ func (e Equalities) DeepEqual(a1, a2 interface{}) bool { if v1.Type() != v2.Type() { return false } - return e.deepValueEqual(v1, v2, make(map[visit]bool), 0) + return e.deepValueEqual(v1, v2, make(map[visit]bool), equateNilAndEmpty, 0) } func (e Equalities) deepValueDerive(v1, v2 reflect.Value, visited map[visit]bool, depth int) bool { diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go index bcb08b27eae..6222817615c 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go @@ -16,6 +16,10 @@ func TestEqualities(t *testing.T) { type Baz struct { Y Bar } + type Zap struct { + A []int + B map[string][]int + } err := e.AddFuncs( func(a, b int) bool { return a+1 == b @@ -32,10 +36,12 @@ func TestEqualities(t *testing.T) { X int } - table := []struct { + type Case struct { a, b interface{} equal bool - }{ + } + + table := []Case{ {1, 2, true}, {2, 1, false}, {"foo", "fo", false}, @@ -70,6 +76,26 @@ func TestEqualities(t *testing.T) { t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) } } + + // Cases which hinge upon implicit nil/empty map/slice equality + implicitTable := []Case{ + {map[string][]int{}, map[string][]int(nil), true}, + {[]int{}, []int(nil), true}, + {map[string][]int{"foo": nil}, map[string][]int{"foo": {}}, true}, + {Zap{A: nil, B: map[string][]int{"foo": nil}}, Zap{A: []int{}, B: map[string][]int{"foo": {}}}, true}, + } + + for _, item := range implicitTable { + if e, a := item.equal, e.DeepEqual(item.a, item.b); e != a { + t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) + } + } + + for _, item := range implicitTable { + if e, a := !item.equal, e.DeepEqualWithNilDifferentFromEmpty(item.a, item.b); e != a { + t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) + } + } } func TestDerivatives(t *testing.T) { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go new file mode 100644 index 00000000000..74cc27fd149 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go @@ -0,0 +1,109 @@ +/* +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 fieldmanager + +import ( + "context" + "fmt" + "reflect" + + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" + "k8s.io/apimachinery/pkg/runtime" +) + +var ignoreTimestampEqualities = func() conversion.Equalities { + var eqs = equality.Semantic.Copy() + + err := eqs.AddFunc( + func(a, b metav1.ManagedFieldsEntry) bool { + // Two objects' managed fields are equivalent if, ignoring timestamp, + // the objects are deeply equal. + a.Time = nil + b.Time = nil + return reflect.DeepEqual(a, b) + }, + ) + + if err != nil { + panic(err) + } + + return eqs +}() + +// IgnoreManagedFieldsTimestampsTransformer reverts timestamp updates +// if the non-managed parts of the object are equivalent +func IgnoreManagedFieldsTimestampsTransformer( + _ context.Context, + newObj runtime.Object, + oldObj runtime.Object, +) (runtime.Object, error) { + // If managedFields modulo timestamps are unchanged + // and + // rest of object is unchanged + // then + // revert any changes to timestamps in managed fields + // (to prevent spurious ResourceVersion bump) + // + // Procecure: + // Do a quicker check to see if just managed fields modulo timestamps are + // unchanged. If so, then do the full, slower check. + // + // In most cases which actually update the object, the managed fields modulo + // timestamp check will fail, and we will be able to return early. + // + // In other cases, the managed fields may be exactly the same, + // except for timestamp, but the objects are the different. This is the + // slow path which checks the full object. + oldAccessor, err := meta.Accessor(oldObj) + if err != nil { + return nil, fmt.Errorf("failed to acquire accessor for oldObj: %v", err) + } + + accessor, err := meta.Accessor(newObj) + if err != nil { + return nil, fmt.Errorf("failed to acquire accessor for newObj: %v", err) + } + + oldManagedFields := oldAccessor.GetManagedFields() + newManagedFields := accessor.GetManagedFields() + + // This condition ensures the managed fields are always compared first. If + // this check fails, the if statement will short circuit. If the check + // succeeds the slow path is taken which compares entire objects. + if ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) && + ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { + + // Remove any changed timestamps, so that timestamp is not the only + // change seen by etcd. + // + // newManagedFields is known to be exactly pairwise equal to + // oldManagedFields except for timestamps. + // + // Simply replace possibly changed new timestamps with their old values. + for idx := 0; idx < len(oldManagedFields); idx++ { + newManagedFields[idx].Time = oldManagedFields[idx].Time + } + + accessor.SetManagedFields(newManagedFields) + return newObj, nil + } + return newObj, nil +} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index 6ee4904bf4f..50f6005ab83 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -660,7 +660,7 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti } wasCreated := false - p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer) + p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) requestFunc := func() (runtime.Object, error) { // Pass in UpdateOptions to override UpdateStrategy.AllowUpdateOnCreate options := patchToUpdateOptions(p.options) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index 1c8f140175d..cb0ba5d7de8 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -191,6 +191,15 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa }) } + // Ignore changes that only affect managed fields + // timestamps. FieldManager can't know about changes + // like normalized fields, defaulted fields and other + // mutations. + // Only makes sense when SSA field manager is being used + if scope.FieldManager != nil { + transformers = append(transformers, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) + } + createAuthorizerAttributes := authorizer.AttributesRecord{ User: userInfo, ResourceRequest: true, diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index a59585a8c53..b206fc4c5cf 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -249,6 +249,185 @@ func TestNoOpUpdateSameResourceVersion(t *testing.T) { } } +func getRV(obj runtime.Object) (string, error) { + acc, err := meta.Accessor(obj) + if err != nil { + return "", err + } + return acc.GetResourceVersion(), nil +} + +// TestNoSemanticUpdateAppleSameResourceVersion makes sure that APPLY requests which makes no semantic changes +// will not change the resource version (no write to etcd is done) +// +// Some of the non-semantic changes are: +// - Applying an atomic struct that removes a default +// - Changing Quantity or other fields that are normalized +func TestNoSemanticUpdateApplySameResourceVersion(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + client, closeFn := setup(t) + defer closeFn() + + ssBytes := []byte(`{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": { + "name": "nginx", + "labels": {"app": "nginx"} + }, + "spec": { + "serviceName": "nginx", + "selector": { "matchLabels": {"app": "nginx"}}, + "template": { + "metadata": { + "labels": {"app": "nginx"} + }, + "spec": { + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": { + "limits": {"memory": "2048Mi"} + } + }] + } + }, + "volumeClaimTemplates": [{ + "metadata": {"name": "nginx"}, + "spec": { + "accessModes": ["ReadWriteOnce"], + "resources": {"requests": {"storage": "1Gi"}} + } + }] + } + }`) + + obj, err := client.AppsV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + rvCreated, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1200 * time.Millisecond) + + obj, err = client.AppsV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + rvApplied, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + if rvApplied != rvCreated { + t.Fatal("ResourceVersion changed after apply") + } +} + +// TestNoSemanticUpdateAppleSameResourceVersion makes sure that PUT requests which makes no semantic changes +// will not change the resource version (no write to etcd is done) +// +// Some of the non-semantic changes are: +// - Applying an atomic struct that removes a default +// - Changing Quantity or other fields that are normalized +func TestNoSemanticUpdatePutSameResourceVersion(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + client, closeFn := setup(t) + defer closeFn() + + ssBytes := []byte(`{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": { + "name": "nginx", + "labels": {"app": "nginx"} + }, + "spec": { + "serviceName": "nginx", + "selector": { "matchLabels": {"app": "nginx"}}, + "template": { + "metadata": { + "labels": {"app": "nginx"} + }, + "spec": { + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": { + "limits": {"memory": "2048Mi"} + } + }] + } + }, + "volumeClaimTemplates": [{ + "metadata": {"name": "nginx"}, + "spec": { + "accessModes": ["ReadWriteOnce"], + "resources": { "requests": { "storage": "1Gi"}} + } + }] + } + }`) + + obj, err := client.AppsV1().RESTClient().Post(). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + rvCreated, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1200 * time.Millisecond) + + obj, err = client.AppsV1().RESTClient().Put(). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + rvApplied, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + if rvApplied != rvCreated { + t.Fatal("ResourceVersion changed after similar PUT") + } +} + // TestCreateOnApplyFailsWithUID makes sure that PATCH requests with the apply content type // will not create the object if it doesn't already exist and it specifies a UID func TestCreateOnApplyFailsWithUID(t *testing.T) { From 40343793f7b9787b2d4b88f0a0439ce9e538075a Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 1 Mar 2022 14:24:12 -0800 Subject: [PATCH 2/7] benchmark and metrics for new timestamp transformer comparison add proper metrics rename & improve documentation for path metric dimension --- .../handlers/fieldmanager/equality.go | 53 ++++++- .../pkg/endpoints/metrics/metrics.go | 17 ++ .../apiserver/timestamp_transformer_test.go | 149 ++++++++++++++++++ 3 files changed, 216 insertions(+), 3 deletions(-) create mode 100644 test/integration/apiserver/timestamp_transformer_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go index 74cc27fd149..ddc0ec8c965 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go @@ -20,12 +20,14 @@ import ( "context" "fmt" "reflect" + "time" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/endpoints/metrics" ) var ignoreTimestampEqualities = func() conversion.Equalities { @@ -54,7 +56,20 @@ func IgnoreManagedFieldsTimestampsTransformer( _ context.Context, newObj runtime.Object, oldObj runtime.Object, -) (runtime.Object, error) { +) (res runtime.Object, err error) { + outcome := "unequal_objects_fast" + start := time.Now() + err = nil + res = nil + + defer func() { + if err != nil { + outcome = "error" + } + + metrics.RecordTimestampComparisonLatency(outcome, time.Since(start)) + }() + // If managedFields modulo timestamps are unchanged // and // rest of object is unchanged @@ -85,12 +100,41 @@ func IgnoreManagedFieldsTimestampsTransformer( oldManagedFields := oldAccessor.GetManagedFields() newManagedFields := accessor.GetManagedFields() + if len(oldManagedFields) != len(newManagedFields) { + // Return early if any managed fields entry was added/removed. + // We want to retain user expectation that even if they write to a field + // whose value did not change, they will still result as the field + // manager at the end. + return newObj, nil + } else if len(newManagedFields) == 0 { + // This transformation only makes sense when managedFields are + // non-empty + return newObj, nil + } + + // This transformation only makes sense if the managed fields has at least one + // changed timestamp; and are otherwise equal. Return early if there are no + // changed timestamps. + allTimesUnchanged := true + for i, e := range newManagedFields { + if !e.Time.Equal(oldManagedFields[i].Time) { + allTimesUnchanged = false + break + } + } + + if allTimesUnchanged { + return newObj, nil + } + // This condition ensures the managed fields are always compared first. If // this check fails, the if statement will short circuit. If the check // succeeds the slow path is taken which compares entire objects. - if ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) && - ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { + if !ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) { + return newObj, nil + } + if ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { // Remove any changed timestamps, so that timestamp is not the only // change seen by etcd. // @@ -103,7 +147,10 @@ func IgnoreManagedFieldsTimestampsTransformer( } accessor.SetManagedFields(newManagedFields) + outcome = "equal_objects" return newObj, nil } + + outcome = "unequal_objects_slow" return newObj, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index 4f851bb4a3b..ee754812881 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -238,6 +238,18 @@ var ( []string{"source", "status"}, ) + requestTimestampComparisonDuration = compbasemetrics.NewHistogramVec( + &compbasemetrics.HistogramOpts{ + Name: "apiserver_request_timestamp_comparison_time", + Help: "Time taken for comparison of old vs new objects in UPDATE or PATCH requests", + Buckets: []float64{0.0001, 0.0003, 0.001, 0.003, 0.01, 0.03, 0.1, 0.3, 1.0, 5.0}, + StabilityLevel: compbasemetrics.ALPHA, + }, + // Path the code takes to reach a conclusion: + // i.e. unequalObjectsFast, unequalObjectsSlow, equalObjectsSlow + []string{"code_path"}, + ) + metrics = []resettableCollector{ deprecatedRequestGauge, requestCounter, @@ -256,6 +268,7 @@ var ( requestFilterDuration, requestAbortsTotal, requestPostTimeoutTotal, + requestTimestampComparisonDuration, } // these are the valid request methods which we report in our metrics. Any other request methods @@ -366,6 +379,10 @@ func RecordFilterLatency(ctx context.Context, name string, elapsed time.Duration requestFilterDuration.WithContext(ctx).WithLabelValues(name).Observe(elapsed.Seconds()) } +func RecordTimestampComparisonLatency(codePath string, elapsed time.Duration) { + requestTimestampComparisonDuration.WithLabelValues(codePath).Observe(elapsed.Seconds()) +} + func RecordRequestPostTimeout(source string, status string) { requestPostTimeoutTotal.WithLabelValues(source, status).Inc() } diff --git a/test/integration/apiserver/timestamp_transformer_test.go b/test/integration/apiserver/timestamp_transformer_test.go new file mode 100644 index 00000000000..bdc564e7da8 --- /dev/null +++ b/test/integration/apiserver/timestamp_transformer_test.go @@ -0,0 +1,149 @@ +/* +Copyright 2020 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 apiserver + +import ( + "context" + "encoding/json" + "math/rand" + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + k8sfuzz "k8s.io/apimachinery/pkg/api/apitesting/fuzzer" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + k8stest "k8s.io/kubernetes/pkg/api/testing" +) + +func convertToUnstructured(b *testing.B, obj runtime.Object) runtime.Object { + converter := fieldmanager.DeducedTypeConverter{} + typed, err := converter.ObjectToTyped(obj) + require.NoError(b, err) + res, err := converter.TypedToObject(typed) + require.NoError(b, err) + return res +} + +func doBench(b *testing.B, useUnstructured bool, shortCircuit bool) { + var ( + expectedLarge runtime.Object + actualLarge runtime.Object + expectedSmall runtime.Object + actualSmall runtime.Object + ) + + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + seed := rand.Int63() + fuzzer := k8sfuzz.FuzzerFor(k8stest.FuzzerFuncs, rand.NewSource(seed), codecs) + fuzzer.NilChance(0) + + fuzzer.MaxDepth(1000).NilChance(0.2).NumElements(2, 15) + pod := &v1.Pod{} + fuzzer.Fuzz(pod) + + fuzzer.NilChance(0.2).NumElements(10, 100).MaxDepth(10) + deployment := &v1.Endpoints{} + fuzzer.Fuzz(deployment) + + bts, err := json.Marshal(deployment) + require.NoError(b, err) + b.Logf("Small (Deployment): %v bytes", len(bts)) + bts, err = json.Marshal(pod) + require.NoError(b, err) + b.Logf("Large (Pod): %v bytes", len(bts)) + + expectedLarge = deployment + expectedSmall = pod + + if useUnstructured { + expectedSmall = convertToUnstructured(b, expectedSmall) + expectedLarge = convertToUnstructured(b, expectedLarge) + } + + actualLarge = expectedLarge.DeepCopyObject() + actualSmall = expectedSmall.DeepCopyObject() + + if shortCircuit { + // Modify managed fields of the compared objects to induce a short circuit + now := metav1.Now() + extraEntry := &metav1.ManagedFieldsEntry{ + Manager: "sidecar_controller", + Operation: metav1.ManagedFieldsOperationApply, + APIVersion: "apps/v1", + Time: &now, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:metadata":{"f:labels":{"f:sidecar_version":{}}},"f:spec":{"f:template":{"f:spec":{"f:containers":{"k:{\"name\":\"sidecar\"}":{".":{},"f:image":{},"f:name":{}}}}}}}`), + }, + } + + largeMeta, err := meta.Accessor(actualLarge) + require.NoError(b, err) + largeMeta.SetManagedFields(append(largeMeta.GetManagedFields(), *extraEntry)) + + smallMeta, err := meta.Accessor(actualSmall) + require.NoError(b, err) + smallMeta.SetManagedFields(append(smallMeta.GetManagedFields(), *extraEntry)) + } + + b.ResetTimer() + + b.Run("Large", func(b2 *testing.B) { + for i := 0; i < b2.N; i++ { + if _, err := fieldmanager.IgnoreManagedFieldsTimestampsTransformer( + context.TODO(), + actualLarge, + expectedLarge, + ); err != nil { + b2.Fatal(err) + } + } + }) + + b.Run("Small", func(b2 *testing.B) { + for i := 0; i < b2.N; i++ { + if _, err := fieldmanager.IgnoreManagedFieldsTimestampsTransformer( + context.TODO(), + actualSmall, + expectedSmall, + ); err != nil { + b2.Fatal(err) + } + } + }) +} + +func BenchmarkIgnoreManagedFieldsTimestampTransformerStructuredShortCircuit(b *testing.B) { + doBench(b, false, true) +} + +func BenchmarkIgnoreManagedFieldsTimestampTransformerStructuredWorstCase(b *testing.B) { + doBench(b, false, false) +} + +func BenchmarkIgnoreManagedFieldsTimestampTransformerUnstructuredShortCircuit(b *testing.B) { + doBench(b, true, true) +} + +func BenchmarkIgnoreManagedFieldsTimestampTransformerUnstructuredWorstCase(b *testing.B) { + doBench(b, true, false) +} From 076051135d16b70c08d8d6382e73ef983614240f Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Thu, 14 Jul 2022 11:28:10 -0700 Subject: [PATCH 3/7] add envar to disable non semantic updates feature enabled by default. can easily be changed in backports --- .../handlers/fieldmanager/equality.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go index ddc0ec8c965..d0aad18c2ff 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go @@ -19,7 +19,9 @@ package fieldmanager import ( "context" "fmt" + "os" "reflect" + "strconv" "time" "k8s.io/apimachinery/pkg/api/equality" @@ -28,6 +30,24 @@ import ( "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/endpoints/metrics" + "k8s.io/klog/v2" +) + +func determineIgnoreNonSemanticUpdatesEnabled() bool { + if ignoreNonSemanticUpdatesString, exists := os.LookupEnv("KUBE_APISERVER_IGNORE_NON_SEMANTIC_UPDATES"); exists { + if ret, err := strconv.ParseBool(ignoreNonSemanticUpdatesString); err == nil { + return ret + } else { + klog.Errorf("failed to parse envar KUBE_APISERVER_IGNORE_NON_SEMANTIC_UPDATES: %v", err) + } + } + + // enabled by default + return true +} + +var ( + ignoreNonSemanticUpdatesEnabled = determineIgnoreNonSemanticUpdatesEnabled() ) var ignoreTimestampEqualities = func() conversion.Equalities { @@ -57,6 +77,10 @@ func IgnoreManagedFieldsTimestampsTransformer( newObj runtime.Object, oldObj runtime.Object, ) (res runtime.Object, err error) { + if !ignoreNonSemanticUpdatesEnabled { + return newObj, nil + } + outcome := "unequal_objects_fast" start := time.Now() err = nil From 2c996344f57217e628e510a0370b953909fd53ae Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Wed, 20 Jul 2022 08:57:42 -0700 Subject: [PATCH 4/7] optimize nil and empty case for parity with other branch --- .../third_party/forked/golang/reflect/deep_equal.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go index e25247100e7..cc5d236bf84 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go @@ -208,6 +208,18 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, if v1.IsNil() != v2.IsNil() { return false } + + // Optimize nil and empty cases + // Two lists that are BOTH nil are equal + // No need to check v2 is nil since v1.IsNil == v2.IsNil from above + if v1.IsNil() { + return true + } + + // Two lists that are both empty and both non nil are equal + if v1.Len() == 0 || v2.Len() == 0 { + return true + } } if v1.Len() != v2.Len() { return false From 48786d90da794bd1a0ef588f6393a0dddffcc400 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Wed, 20 Jul 2022 08:58:28 -0700 Subject: [PATCH 5/7] guard usage of timestamp transformer under fieldManager non nil not strictly necessary for correctness, but it is not needed unless SSA is enabled --- .../src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index 50f6005ab83..a0a8fb6ca77 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -659,8 +659,13 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti return obj, nil } + transformers := []rest.TransformFunc{p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer} + if scope.FieldManager != nil { + transformers = append(transformers, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) + } + wasCreated := false - p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) + p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, transformers...) requestFunc := func() (runtime.Object, error) { // Pass in UpdateOptions to override UpdateStrategy.AllowUpdateOnCreate options := patchToUpdateOptions(p.options) From a4819996a860066dfba5383e4fb6db2a243ad03e Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Wed, 20 Jul 2022 10:18:42 -0700 Subject: [PATCH 6/7] optimize nil and empty also for slices brings to parity with maps --- .../forked/golang/reflect/deep_equal.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go index cc5d236bf84..511e625b63d 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go @@ -169,6 +169,18 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, if v1.IsNil() != v2.IsNil() { return false } + + // Optimize nil and empty cases + // Two lists that are BOTH nil are equal + // No need to check v2 is nil since v1.IsNil == v2.IsNil from above + if v1.IsNil() { + return true + } + + // Two lists that are both empty and both non nil are equal + if v1.Len() == 0 || v2.Len() == 0 { + return true + } } if v1.Len() != v2.Len() { return false @@ -210,13 +222,13 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, } // Optimize nil and empty cases - // Two lists that are BOTH nil are equal + // Two maps that are BOTH nil are equal // No need to check v2 is nil since v1.IsNil == v2.IsNil from above if v1.IsNil() { return true } - // Two lists that are both empty and both non nil are equal + // Two maps that are both empty and both non nil are equal if v1.Len() == 0 || v2.Len() == 0 { return true } From c2cbc460f2c5be07b0f5762005642625b06b6a75 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <351783+alexzielenski@users.noreply.github.com> Date: Wed, 20 Jul 2022 10:45:21 -0700 Subject: [PATCH 7/7] use more apt name for flag --- .../handlers/fieldmanager/equality.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go index d0aad18c2ff..366d88fcc50 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go @@ -33,12 +33,12 @@ import ( "k8s.io/klog/v2" ) -func determineIgnoreNonSemanticUpdatesEnabled() bool { - if ignoreNonSemanticUpdatesString, exists := os.LookupEnv("KUBE_APISERVER_IGNORE_NON_SEMANTIC_UPDATES"); exists { - if ret, err := strconv.ParseBool(ignoreNonSemanticUpdatesString); err == nil { +func determineAvoidNoopTimestampUpdatesEnabled() bool { + if avoidNoopTimestampUpdatesString, exists := os.LookupEnv("KUBE_APISERVER_AVOID_NOOP_SSA_TIMESTAMP_UPDATES"); exists { + if ret, err := strconv.ParseBool(avoidNoopTimestampUpdatesString); err == nil { return ret } else { - klog.Errorf("failed to parse envar KUBE_APISERVER_IGNORE_NON_SEMANTIC_UPDATES: %v", err) + klog.Errorf("failed to parse envar KUBE_APISERVER_AVOID_NOOP_SSA_TIMESTAMP_UPDATES: %v", err) } } @@ -47,10 +47,10 @@ func determineIgnoreNonSemanticUpdatesEnabled() bool { } var ( - ignoreNonSemanticUpdatesEnabled = determineIgnoreNonSemanticUpdatesEnabled() + avoidNoopTimestampUpdatesEnabled = determineAvoidNoopTimestampUpdatesEnabled() ) -var ignoreTimestampEqualities = func() conversion.Equalities { +var avoidTimestampEqualities = func() conversion.Equalities { var eqs = equality.Semantic.Copy() err := eqs.AddFunc( @@ -77,7 +77,7 @@ func IgnoreManagedFieldsTimestampsTransformer( newObj runtime.Object, oldObj runtime.Object, ) (res runtime.Object, err error) { - if !ignoreNonSemanticUpdatesEnabled { + if !avoidNoopTimestampUpdatesEnabled { return newObj, nil } @@ -154,11 +154,11 @@ func IgnoreManagedFieldsTimestampsTransformer( // This condition ensures the managed fields are always compared first. If // this check fails, the if statement will short circuit. If the check // succeeds the slow path is taken which compares entire objects. - if !ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) { + if !avoidTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) { return newObj, nil } - if ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { + if avoidTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { // Remove any changed timestamps, so that timestamp is not the only // change seen by etcd. //