Updating generic registry to return UID of the deleted resource #45600

nikhiljindal · 2017-05-10T15:17:34Z

cc @kubernetes/sig-api-machinery-pr-reviews @smarterclayton

Updating apiserver to return UID of the deleted resource. Clients can use this UID to verify that the resource was deleted or waiting for finalizers.

k8s-reviewable · 2017-05-10T15:17:41Z

This change is

nikhiljindal · 2017-05-10T15:27:14Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -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 {


This is an API change. We will now return a metav1.Status object where we were returning a StatusError object earlier. We should have always been returning a StatusError only in case of errors.
This was the only place where errors.FromObject was being used even when status is StatusSuccess: https://github.com/kubernetes/kubernetes/search?q=errors.FromObject.

So the fix should be fine but wanted to highlight it as an important change.

I need to think about the implications of this.

soltysh · 2017-05-11T13:37:58Z

/lgtm

nikhiljindal · 2017-05-11T18:58:27Z

cc @kubernetes/api-reviewers since it is an api change

nikhiljindal · 2017-05-11T22:23:39Z

Rebased and added another commit with auto-generated swagger docs.

soltysh · 2017-05-12T13:25:54Z

/lgtm

smarterclayton · 2017-05-12T14:55:49Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

+	if err != nil {
+		return nil, err
+	}
+	details := &metav1.StatusDetails{UID: accessor.GetUID()}


You should set name here as well.

And the other attributes.

smarterclayton · 2017-05-12T14:56:14Z

Removing label, have additional comments.

nikhiljindal · 2017-05-13T01:04:18Z

Fixed the code to set name and other attributes as well

smarterclayton · 2017-05-14T00:05:38Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

+	if err != nil {
+		return nil, err
+	}
+	gvk := obj.GetObjectKind().GroupVersionKind()


This isn't going tone correct - you need to use the GVR passed to the store (Kind on StatusDetails really means Resource, this was a problem we created for ourselves)

ok I see now that we set Kind to resource at another place:

kubernetes/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go

Line 102 in 6defc30

Kind: qualifiedResource.Resource,

.
That is definitely wrong. We should add another Resource field and deprecate kind. Should at least add a comment in API docs saying value of Kind field is resource.

Updated the code to set Kind to store.QualifiedResource.Resource.

nikhiljindal · 2017-05-14T22:44:10Z

Rebased and updated code as per comments.
PTAL.

smarterclayton · 2017-05-15T02:22:20Z

/lgtm

thanks

k8s-github-robot · 2017-05-15T02:22:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhiljindal, smarterclayton, soltysh

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~federation/OWNERS~~ [nikhiljindal,smarterclayton]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiserver/OWNERS~~ [nikhiljindal,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

nikhiljindal · 2017-05-15T06:12:37Z

Rebased and ran hack/update-federation-api-reference-docs.sh again to fix failing Jenkins verification

nikhiljindal · 2017-05-15T16:04:08Z

@k8s-bot node e2e test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-github-robot · 2017-05-15T20:25:45Z

Automatic merge from submit-queue (batch tested with PRs 41331, 45591, 45600, 45176, 45658)

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 10, 2017

k8s-github-robot assigned soltysh May 10, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 10, 2017

nikhiljindal mentioned this pull request May 10, 2017

kubectl delete should wait for resource to be deleted before returning #42594

Closed

4 tasks

nikhiljindal commented May 10, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2017

nikhiljindal assigned deads2k May 11, 2017

nikhiljindal force-pushed the waitForDel branch from ed410b0 to 6349523 Compare May 11, 2017 22:20

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 11, 2017

nikhiljindal assigned smarterclayton May 11, 2017

nikhiljindal force-pushed the waitForDel branch from 6349523 to 187cf3d Compare May 11, 2017 23:58

nikhiljindal mentioned this pull request May 12, 2017

Add federation update script to update-all.sh #41866

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017

smarterclayton reviewed May 12, 2017

View reviewed changes

smarterclayton removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017

nikhiljindal force-pushed the waitForDel branch from 187cf3d to 9ebebd2 Compare May 13, 2017 01:02

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2017

smarterclayton reviewed May 14, 2017

View reviewed changes

nikhiljindal force-pushed the waitForDel branch from 9ebebd2 to 1233502 Compare May 14, 2017 22:42

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2017

nikhiljindal force-pushed the waitForDel branch from 1233502 to b8de15f Compare May 15, 2017 00:11

smarterclayton added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2017

nikhiljindal added 3 commits May 14, 2017 23:09

Updating generic registry to return UID while deleting the object

44fc88c

Autogenerated proto changes

a1ffc8c

Autogenerated swagger changes

4c828ee

nikhiljindal force-pushed the waitForDel branch from b8de15f to 4c828ee Compare May 15, 2017 06:11

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017

k8s-github-robot merged commit a4307eb into kubernetes:master May 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating generic registry to return UID of the deleted resource #45600

Updating generic registry to return UID of the deleted resource #45600

nikhiljindal commented May 10, 2017 •

edited

Loading

k8s-reviewable commented May 10, 2017

nikhiljindal May 10, 2017 •

edited

Loading

smarterclayton May 12, 2017

soltysh commented May 11, 2017

nikhiljindal commented May 11, 2017

nikhiljindal commented May 11, 2017

soltysh commented May 12, 2017

smarterclayton May 12, 2017

smarterclayton May 12, 2017

nikhiljindal May 13, 2017

smarterclayton commented May 12, 2017

nikhiljindal commented May 13, 2017

smarterclayton May 14, 2017

nikhiljindal May 14, 2017

nikhiljindal commented May 14, 2017

smarterclayton commented May 15, 2017

k8s-github-robot commented May 15, 2017

nikhiljindal commented May 15, 2017

nikhiljindal commented May 15, 2017

k8s-github-robot commented May 15, 2017

Updating generic registry to return UID of the deleted resource #45600

Updating generic registry to return UID of the deleted resource #45600

Conversation

nikhiljindal commented May 10, 2017 • edited Loading

k8s-reviewable commented May 10, 2017

nikhiljindal May 10, 2017 • edited Loading

Choose a reason for hiding this comment

smarterclayton May 12, 2017

Choose a reason for hiding this comment

soltysh commented May 11, 2017

nikhiljindal commented May 11, 2017

nikhiljindal commented May 11, 2017

soltysh commented May 12, 2017

smarterclayton May 12, 2017

Choose a reason for hiding this comment

smarterclayton May 12, 2017

Choose a reason for hiding this comment

nikhiljindal May 13, 2017

Choose a reason for hiding this comment

smarterclayton commented May 12, 2017

nikhiljindal commented May 13, 2017

smarterclayton May 14, 2017

Choose a reason for hiding this comment

nikhiljindal May 14, 2017

Choose a reason for hiding this comment

nikhiljindal commented May 14, 2017

smarterclayton commented May 15, 2017

k8s-github-robot commented May 15, 2017

nikhiljindal commented May 15, 2017

nikhiljindal commented May 15, 2017

k8s-github-robot commented May 15, 2017

nikhiljindal commented May 10, 2017 •

edited

Loading

nikhiljindal May 10, 2017 •

edited

Loading