From 44fc88cecd5ab175fe7907eb7b975f0a00cb2305 Mon Sep 17 00:00:00 2001 From: nikhiljindal Date: Tue, 9 May 2017 18:27:52 -0700 Subject: [PATCH] Updating generic registry to return UID while deleting the object --- .../apimachinery/pkg/apis/meta/v1/types.go | 5 ++ .../apiserver/pkg/endpoints/handlers/rest.go | 6 +- .../pkg/endpoints/handlers/rest_test.go | 59 +++++++++++++++++++ .../pkg/registry/generic/registry/store.go | 15 ++++- .../registry/generic/registry/store_test.go | 26 ++++++++ 5 files changed, 108 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index f14efc8dfe7..b0a83583914 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -425,6 +425,11 @@ type StatusDetails struct { // More info: http://releases.k8s.io/HEAD/docs/devel/api-conventions.md#types-kinds // +optional Kind string `json:"kind,omitempty" protobuf:"bytes,3,opt,name=kind"` + // UID of the resource. + // (when there is a single resource which can be described). + // More info: http://kubernetes.io/docs/user-guide/identifiers#uids + // +optional + UID types.UID `json:"uid,omitempty" protobuf:"bytes,6,opt,name=uid,casttype=k8s.io/apimachinery/pkg/types.UID"` // The Causes array includes more details associated with the StatusReason // failure. Not all StatusReasons may provide detailed causes. // +optional diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index a10410e3f99..7e2c972bc77 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go @@ -1059,7 +1059,7 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco type resultFunc func() (runtime.Object, error) // finishRequest makes a given resultFunc asynchronous and handles errors returned by the response. -// Any api.Status object returned is considered an "error", which interrupts the normal response flow. +// An api.Status object with status != success is considered an "error", which interrupts the normal response flow. func finishRequest(timeout time.Duration, fn resultFunc) (result runtime.Object, err error) { // these channels need to be buffered to prevent the goroutine below from hanging indefinitely // when the select statement reads something other than the one the goroutine sends on. @@ -1083,7 +1083,9 @@ func finishRequest(timeout time.Duration, fn resultFunc) (result runtime.Object, select { case result = <-ch: if status, ok := result.(*metav1.Status); ok { - return nil, errors.FromObject(status) + if status.Status != metav1.StatusSuccess { + return nil, errors.FromObject(status) + } } return result, nil case err = <-errCh: diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go index 98ec1da996b..918089fd2ad 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go @@ -597,3 +597,62 @@ func TestParseTimeout(t *testing.T) { t.Errorf("10s timeout produced: %v", d) } } + +func TestFinishRequest(t *testing.T) { + exampleObj := &example.Pod{} + exampleErr := fmt.Errorf("error") + successStatusObj := &metav1.Status{Status: metav1.StatusSuccess, Message: "success message"} + errorStatusObj := &metav1.Status{Status: metav1.StatusFailure, Message: "error message"} + testcases := []struct { + timeout time.Duration + fn resultFunc + expectedObj runtime.Object + expectedErr error + }{ + { + // Expected obj is returned. + timeout: time.Second, + fn: func() (runtime.Object, error) { + return exampleObj, nil + }, + expectedObj: exampleObj, + expectedErr: nil, + }, + { + // Expected error is returned. + timeout: time.Second, + fn: func() (runtime.Object, error) { + return nil, exampleErr + }, + expectedObj: nil, + expectedErr: exampleErr, + }, + { + // Successful status object is returned as expected. + timeout: time.Second, + fn: func() (runtime.Object, error) { + return successStatusObj, nil + }, + expectedObj: successStatusObj, + expectedErr: nil, + }, + { + // Error status object is converted to StatusError. + timeout: time.Second, + fn: func() (runtime.Object, error) { + return errorStatusObj, nil + }, + expectedObj: nil, + expectedErr: apierrors.FromObject(errorStatusObj), + }, + } + for i, tc := range testcases { + obj, err := finishRequest(tc.timeout, tc.fn) + if (err == nil && tc.expectedErr != nil) || (err != nil && tc.expectedErr == nil) || (err != nil && tc.expectedErr != nil && err.Error() != tc.expectedErr.Error()) { + t.Errorf("%d: unexpected err. expected: %v, got: %v", i, tc.expectedErr, err) + } + if !apiequality.Semantic.DeepEqual(obj, tc.expectedObj) { + t.Errorf("%d: unexpected obj. expected %#v, got %#v", tc.expectedObj, obj) + } + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 697f7f25737..a6867426c5d 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -992,7 +992,20 @@ func (e *Store) finalizeDelete(obj runtime.Object, runHooks bool) (runtime.Objec } return obj, nil } - return &metav1.Status{Status: metav1.StatusSuccess}, nil + // Return information about the deleted object, which enables clients to + // verify that the object was actually deleted and not waiting for finalizers. + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + details := &metav1.StatusDetails{ + Name: accessor.GetName(), + Group: e.QualifiedResource.Group, + Kind: e.QualifiedResource.Resource, // Yes we set Kind field to resource. + UID: accessor.GetUID(), + } + status := &metav1.Status{Status: metav1.StatusSuccess, Details: details} + return status, nil } // Watch makes a matcher for the given label and field, and calls diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index 5f92a4a4b5e..d589d57256b 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -1542,3 +1542,29 @@ func newTestGenericStoreRegistry(t *testing.T, scheme *runtime.Scheme, hasCacheE Storage: s, } } + +func TestFinalizeDelete(t *testing.T) { + // Verify that it returns the expected Status. + _, s := NewTestGenericStoreRegistry(t) + obj := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "random-uid"}, + } + result, err := s.finalizeDelete(obj, false) + if err != nil { + t.Fatalf("unexpected err: %s", err) + } + returnedObj := result.(*metav1.Status) + + expectedObj := &metav1.Status{ + Status: metav1.StatusSuccess, + Details: &metav1.StatusDetails{ + Name: "foo", + Group: s.QualifiedResource.Group, + Kind: s.QualifiedResource.Resource, + UID: "random-uid", + }, + } + if !apiequality.Semantic.DeepEqual(expectedObj, returnedObj) { + t.Errorf("unexpected obj. expected %#v, got %#v", expectedObj, returnedObj) + } +}