From aa1f01ec7e6ebf29444b93de7c3cb65a1a54d47e Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Tue, 20 Aug 2019 15:09:42 -0700 Subject: [PATCH] Make sure no op updates don't affect the resource version --- .../handlers/fieldmanager/fieldmanager.go | 24 +++-- .../integration/apiserver/apply/apply_test.go | 100 ++++++++++++++++++ 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go index 466593dd4d1..66bdc7937b3 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go @@ -127,10 +127,6 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r return newObj, nil } apiVersion := fieldpath.APIVersion(f.groupVersion.String()) - manager, err = f.buildManagerInfo(manager, metav1.ManagedFieldsOperationUpdate) - if err != nil { - return nil, fmt.Errorf("failed to build manager identifier: %v", err) - } // TODO(apelisse) use the first return value when unions are implemented _, managed.Fields, err = f.updater.Update(liveObjTyped, newObjTyped, apiVersion, managed.Fields, manager) @@ -139,8 +135,24 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r } managed.Fields = f.stripFields(managed.Fields, manager) - // Update the time in the managedFieldsEntry for this operation - managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()} + // If the current operation took any fields from anything, it means the object changed, + // so update the timestamp of the managedFieldsEntry and merge with any previous updates from the same manager + if vs, ok := managed.Fields[manager]; ok { + delete(managed.Fields, manager) + + // Build a manager identifier which will only match previous updates from the same manager + manager, err = f.buildManagerInfo(manager, metav1.ManagedFieldsOperationUpdate) + if err != nil { + return nil, fmt.Errorf("failed to build manager identifier: %v", err) + } + + managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()} + if previous, ok := managed.Fields[manager]; ok { + managed.Fields[manager] = fieldpath.NewVersionedSet(vs.Set().Union(previous.Set()), vs.APIVersion(), vs.Applied()) + } else { + managed.Fields[manager] = vs + } + } if err := internal.EncodeObjectManagedFields(newObj, managed); err != nil { return nil, fmt.Errorf("failed to encode managed fields: %v", err) diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 491c5833f28..456d410b201 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -145,6 +145,106 @@ func TestApplyAlsoCreates(t *testing.T) { } } +// TestNoOpUpdateSameResourceVersion makes sure that PUT requests which change nothing +// will not change the resource version (no write to etcd is done) +func TestNoOpUpdateSameResourceVersion(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + _, client, closeFn := setup(t) + defer closeFn() + + podName := "no-op" + podResource := "pods" + podBytes := []byte(`{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": { + "name": "` + podName + `", + "labels": { + "a": "one", + "c": "two", + "b": "three" + } + }, + "spec": { + "containers": [{ + "name": "test-container-a", + "image": "test-image-one" + },{ + "name": "test-container-c", + "image": "test-image-two" + },{ + "name": "test-container-b", + "image": "test-image-three" + }] + } + }`) + + _, err := client.CoreV1().RESTClient().Post(). + Namespace("default"). + Resource(podResource). + Body(podBytes). + Do(). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1 * time.Second) + + createdObject, err := client.CoreV1().RESTClient().Get().Namespace("default").Resource(podResource).Name(podName).Do().Get() + if err != nil { + t.Fatalf("Failed to retrieve created object: %v", err) + } + + createdAccessor, err := meta.Accessor(createdObject) + if err != nil { + t.Fatalf("Failed to get meta accessor for created object: %v", err) + } + + createdBytes, err := json.MarshalIndent(createdObject, "\t", "\t") + if err != nil { + t.Fatalf("Failed to marshal created object: %v", err) + } + + // Test that we can put the same object and don't change the RV + _, err = client.CoreV1().RESTClient().Put(). + Namespace("default"). + Resource(podResource). + Name(podName). + Body(createdBytes). + Do(). + Get() + if err != nil { + t.Fatalf("Failed to apply no-op update: %v", err) + } + + updatedObject, err := client.CoreV1().RESTClient().Get().Namespace("default").Resource(podResource).Name(podName).Do().Get() + if err != nil { + t.Fatalf("Failed to retrieve updated object: %v", err) + } + + updatedAccessor, err := meta.Accessor(updatedObject) + if err != nil { + t.Fatalf("Failed to get meta accessor for updated object: %v", err) + } + + updatedBytes, err := json.MarshalIndent(updatedObject, "\t", "\t") + if err != nil { + t.Fatalf("Failed to marshal updated object: %v", err) + } + + if createdAccessor.GetResourceVersion() != updatedAccessor.GetResourceVersion() { + t.Fatalf("Expected same resource version to be %v but got: %v\nold object:\n%v\nnew object:\n%v", + createdAccessor.GetResourceVersion(), + updatedAccessor.GetResourceVersion(), + string(createdBytes), + string(updatedBytes), + ) + } +} + // 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) {