Don't store no-op updates in etcd.

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

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 {

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
 		},
 	)

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()

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)