Merge pull request #29520 from hongchaodeng/serr

Automatic merge from submit-queue

storage error: precondition failure should return invalid object error

In introducing the preconditions by @caesarxuchao , if check preconditions failed, it returns resource version conflict error. This is the wrong error to return, and instead it should return invalid object error. We need to separate these two types of errors.
See the implementation in etcd3 [https://github.com/kubernetes/kubernetes/blob/master/pkg/storage/etcd3/store.go#L467].

Also renaming "ErrCodeResourceVersionConflicts" to "ErrCodeVersionConflicts" for simpler reading.

pkg/api/errors/storage/storage.go

@@ -65,7 +65,7 @@ func InterpretCreateError(err error, qualifiedResource unversioned.GroupResource
 // operation into the appropriate API error.
 func InterpretUpdateError(err error, qualifiedResource unversioned.GroupResource, name string) error {
 	switch {
-	case storage.IsTestFailed(err), storage.IsNodeExist(err):
+	case storage.IsTestFailed(err), storage.IsNodeExist(err), storage.IsInvalidObj(err):
 		return errors.NewConflict(qualifiedResource, name, err)
 	case storage.IsUnreachable(err):
 		return errors.NewServerTimeout(qualifiedResource, "update", 2) // TODO: make configurable or handled at a higher level
@@ -86,7 +86,7 @@ func InterpretDeleteError(err error, qualifiedResource unversioned.GroupResource
 		return errors.NewNotFound(qualifiedResource, name)
 	case storage.IsUnreachable(err):
 		return errors.NewServerTimeout(qualifiedResource, "delete", 2) // TODO: make configurable or handled at a higher level
-	case storage.IsTestFailed(err), storage.IsNodeExist(err):
+	case storage.IsTestFailed(err), storage.IsNodeExist(err), storage.IsInvalidObj(err):
 		return errors.NewConflict(qualifiedResource, name, err)
 	case storage.IsInternalError(err):
 		return errors.NewInternalError(err)

pkg/storage/errors.go

@@ -107,7 +107,7 @@ func IsUnreachable(err error) bool {
 
 // IsTestFailed returns true if and only if err is a write conflict.
 func IsTestFailed(err error) bool {
-	return isErrCode(err, ErrCodeResourceVersionConflicts, ErrCodeInvalidObj)
+	return isErrCode(err, ErrCodeResourceVersionConflicts)
 }
 
 // IsInvalidUID returns true if and only if err is invalid UID error
@@ -115,16 +115,12 @@ func IsInvalidObj(err error) bool {
 	return isErrCode(err, ErrCodeInvalidObj)
 }
 
-func isErrCode(err error, codes ...int) bool {
+func isErrCode(err error, code int) bool {
 	if err == nil {
 		return false
 	}
 	if e, ok := err.(*StorageError); ok {
-		for _, code := range codes {
+		return e.Code == code
-			if e.Code == code {
-				return true
-			}
-		}
 	}
 	return false
 }

pkg/storage/etcd/etcd_helper.go

@@ -150,7 +150,7 @@ func (h *etcdHelper) Create(ctx context.Context, key string, obj, out runtime.Ob
 	return err
 }
 
-func checkPreconditions(preconditions *storage.Preconditions, out runtime.Object) error {
+func checkPreconditions(key string, preconditions *storage.Preconditions, out runtime.Object) error {
 	if preconditions == nil {
 		return nil
 	}
@@ -159,7 +159,8 @@ func checkPreconditions(preconditions *storage.Preconditions, out runtime.Object
 		return storage.NewInternalErrorf("can't enforce preconditions %v on un-introspectable object %v, got error: %v", *preconditions, out, err)
 	}
 	if preconditions.UID != nil && *preconditions.UID != objMeta.UID {
-		return etcd.Error{Code: etcd.ErrorCodeTestFailed, Message: fmt.Sprintf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *preconditions.UID, objMeta.UID)}
+		errMsg := fmt.Sprintf("Precondition failed: UID in precondition: %v, UID in object meta: %v", preconditions.UID, objMeta.UID)
+		return storage.NewInvalidObjError(key, errMsg)
 	}
 	return nil
 }
@@ -195,7 +196,7 @@ func (h *etcdHelper) Delete(ctx context.Context, key string, out runtime.Object,
 		if err != nil {
 			return toStorageErr(err, key, 0)
 		}
-		if err := checkPreconditions(preconditions, obj); err != nil {
+		if err := checkPreconditions(key, preconditions, obj); err != nil {
 			return toStorageErr(err, key, 0)
 		}
 		index := uint64(0)
@@ -471,7 +472,7 @@ func (h *etcdHelper) GuaranteedUpdate(ctx context.Context, key string, ptrToType
 		if err != nil {
 			return toStorageErr(err, key, 0)
 		}
-		if err := checkPreconditions(preconditions, obj); err != nil {
+		if err := checkPreconditions(key, preconditions, obj); err != nil {
 			return toStorageErr(err, key, 0)
 		}
 		meta := storage.ResponseMeta{}

pkg/storage/etcd/etcd_helper_test.go

@@ -460,7 +460,7 @@ func TestGuaranteedUpdateUIDMismatch(t *testing.T) {
 	err = helper.GuaranteedUpdate(context.TODO(), "/some/key", podPtr, true, storage.NewUIDPreconditions("B"), storage.SimpleUpdate(func(in runtime.Object) (runtime.Object, error) {
 		return obj, nil
 	}))
-	if !storage.IsTestFailed(err) {
+	if !storage.IsInvalidObj(err) {
 		t.Fatalf("Expect a Test Failed (write conflict) error, got: %v", err)
 	}
 }
@@ -499,7 +499,7 @@ func TestDeleteUIDMismatch(t *testing.T) {
 		t.Fatalf("Unexpected error %#v", err)
 	}
 	err = helper.Delete(context.TODO(), "/some/key", obj, storage.NewUIDPreconditions("B"))
-	if !storage.IsTestFailed(err) {
+	if !storage.IsInvalidObj(err) {
 		t.Fatalf("Expect a Test Failed (write conflict) error, got: %v", err)
 	}
 }