From 2e97793840440a1122fa86289edc08105a76792e Mon Sep 17 00:00:00 2001 From: Wojciech Tyczynski Date: Tue, 9 Feb 2016 14:06:52 +0100 Subject: [PATCH] Don't store no-op updates in etcd. --- pkg/registry/generic/etcd/etcd.go | 9 +++ pkg/registry/generic/etcd/etcd_test.go | 59 +++++++++++++++++++ pkg/registry/serviceaccount/etcd/etcd_test.go | 2 +- pkg/storage/etcd/etcd_helper.go | 35 ++++++++++- pkg/storage/etcd/etcd_helper_test.go | 1 - 5 files changed, 102 insertions(+), 4 deletions(-) diff --git a/pkg/registry/generic/etcd/etcd.go b/pkg/registry/generic/etcd/etcd.go index d83180dba39..a976d61af91 100644 --- a/pkg/registry/generic/etcd/etcd.go +++ b/pkg/registry/generic/etcd/etcd.go @@ -254,6 +254,15 @@ func (e *Etcd) Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool creating := false out := e.NewFunc() err = e.Storage.GuaranteedUpdate(ctx, key, out, true, func(existing runtime.Object, res storage.ResponseMeta) (runtime.Object, *uint64, error) { + // Since we return 'obj' from this function and it can be modified outside this + // function, we are resetting resourceVersion to the initial value here. + // + // TODO: In fact, we should probably return a DeepCopy of obj in all places. + err := e.Storage.Versioner().UpdateObject(obj, nil, resourceVersion) + if err != nil { + return nil, nil, err + } + version, err := e.Storage.Versioner().ObjectResourceVersion(existing) if err != nil { return nil, nil, err diff --git a/pkg/registry/generic/etcd/etcd_test.go b/pkg/registry/generic/etcd/etcd_test.go index 0c82c7f862e..a0d09dd987e 100644 --- a/pkg/registry/generic/etcd/etcd_test.go +++ b/pkg/registry/generic/etcd/etcd_test.go @@ -339,6 +339,65 @@ func TestEtcdUpdate(t *testing.T) { } +func TestNoOpUpdates(t *testing.T) { + server, registry := NewTestGenericEtcdRegistry(t) + defer server.Terminate(t) + + newPod := func() *api.Pod { + return &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Namespace: api.NamespaceDefault, + Name: "foo", + Labels: map[string]string{"prepare_create": "true"}, + }, + Spec: api.PodSpec{NodeName: "machine"}, + } + } + + var err error + var createResult runtime.Object + if createResult, err = registry.Create(api.NewDefaultContext(), newPod()); err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + createdPod, err := registry.Get(api.NewDefaultContext(), "foo") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + var updateResult runtime.Object + if updateResult, _, err = registry.Update(api.NewDefaultContext(), newPod()); err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + // Check whether we do not return empty result on no-op update. + if !reflect.DeepEqual(createResult, updateResult) { + t.Errorf("no-op update should return a correct value, got: %#v", updateResult) + } + + updatedPod, err := registry.Get(api.NewDefaultContext(), "foo") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + createdMeta, err := meta.Accessor(createdPod) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + updatedMeta, err := meta.Accessor(updatedPod) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if createdMeta.GetResourceVersion() != updatedMeta.GetResourceVersion() { + t.Errorf("no-op update should be ignored and not written to etcd") + } +} + +// TODO: Add a test to check no-op update if we have object with ResourceVersion +// already stored in etcd. Currently there is no easy way to store object with +// ResourceVersion in etcd. + type testPodExport struct{} func (t testPodExport) Export(obj runtime.Object, exact bool) error { diff --git a/pkg/registry/serviceaccount/etcd/etcd_test.go b/pkg/registry/serviceaccount/etcd/etcd_test.go index bb7d198d21c..2cf2aaadcaa 100644 --- a/pkg/registry/serviceaccount/etcd/etcd_test.go +++ b/pkg/registry/serviceaccount/etcd/etcd_test.go @@ -70,7 +70,7 @@ func TestUpdate(t *testing.T) { // updateFunc func(obj runtime.Object) runtime.Object { object := obj.(*api.ServiceAccount) - // TODO: Update this serviceAccount + object.Secrets = []api.ObjectReference{{}} return object }, ) diff --git a/pkg/storage/etcd/etcd_helper.go b/pkg/storage/etcd/etcd_helper.go index 255e59d19c2..191c4bd6197 100644 --- a/pkg/storage/etcd/etcd_helper.go +++ b/pkg/storage/etcd/etcd_helper.go @@ -191,6 +191,24 @@ func (h *etcdHelper) Set(ctx context.Context, key string, obj, out runtime.Objec if ctx == nil { glog.Errorf("Context is nil") } + + version := uint64(0) + if h.versioner != nil { + var err error + if version, err = h.versioner.ObjectResourceVersion(obj); err != nil { + return errors.New("couldn't get resourceVersion from object") + } + if version != 0 { + // We cannot store object with resourceVersion in etcd, we need to clear it here. + if err := h.versioner.UpdateObject(obj, nil, 0); err != nil { + return errors.New("resourceVersion cannot be set on objects store in etcd") + } + } + } + // TODO: If versioner is nil, then we may end up with having ResourceVersion set + // in the object and this will be incorrect ResourceVersion. We should fix it by + // requiring "versioner != nil" at the constructor level for 1.3 milestone. + var response *etcd.Response data, err := runtime.Encode(h.codec, obj) if err != nil { @@ -200,7 +218,7 @@ func (h *etcdHelper) Set(ctx context.Context, key string, obj, out runtime.Objec create := true if h.versioner != nil { - if version, err := h.versioner.ObjectResourceVersion(obj); err == nil && version != 0 { + if version != 0 { create = false startTime := time.Now() opts := etcd.SetOptions{ @@ -558,6 +576,16 @@ func (h *etcdHelper) GuaranteedUpdate(ctx context.Context, key string, ptrToType ttl = *newTTL } + // Since update object may have a resourceVersion set, we need to clear it here. + if h.versioner != nil { + if err := h.versioner.UpdateObject(ret, meta.Expiration, 0); err != nil { + return errors.New("resourceVersion cannot be set on objects store in etcd") + } + } + // TODO: If versioner is nil, then we may end up with having ResourceVersion set + // in the object and this will be incorrect ResourceVersion. We should fix it by + // requiring "versioner != nil" at the constructor level for 1.3 milestone. + data, err := runtime.Encode(h.codec, ret) if err != nil { return err @@ -580,7 +608,10 @@ func (h *etcdHelper) GuaranteedUpdate(ctx context.Context, key string, ptrToType } if string(data) == origBody { - return nil + // If we don't send an update, we simply return the currently existing + // version of the object. + _, _, err := h.extractObj(res, nil, ptrToType, ignoreNotFound, false) + return err } startTime := time.Now() diff --git a/pkg/storage/etcd/etcd_helper_test.go b/pkg/storage/etcd/etcd_helper_test.go index a7d63025f58..7a892b34034 100644 --- a/pkg/storage/etcd/etcd_helper_test.go +++ b/pkg/storage/etcd/etcd_helper_test.go @@ -373,7 +373,6 @@ func TestSetNilOutParam(t *testing.T) { server := etcdtesting.NewEtcdTestClientServer(t) defer server.Terminate(t) helper := newEtcdHelper(server.Client, testapi.Default.Codec(), etcdtest.PathPrefix()) - helper.versioner = nil err := helper.Set(context.TODO(), "/some/key", obj, nil, 3) if err != nil { t.Errorf("Unexpected error %#v", err)