Namespace deletion uses discovery + dynamic client #21400

derekwaynecarr · 2016-02-17T17:43:06Z

The namespace controller:

takes a list of group version resources supported based on the discovery client
during deletion, uses dynamic client to remove those resources
- does DELETE_COLLECTION if supported, otherwise falls back to LIST/DELETE by name.
- verifies that no items exist for each collection for each resource.

This means developers no longer need to remember to clean-up their resource as part of namespace lifecycle.

derekwaynecarr · 2016-02-17T18:20:26Z

see: #20351 (comment)

derekwaynecarr · 2016-02-23T08:27:54Z

@lavalamp @thockin @bgrant0607 - this PR updates the namespace controller to use the discovery client to find the list of supported resources, and the dynamic client to then delete those resources. this should eliminate developer errors where developers had forgot to do this manually. assuming this is stable, it may be worth our while to have it in 1.2.

/cc @smarterclayton @kubernetes/rh-cluster-infra

derekwaynecarr · 2016-02-23T16:37:41Z

@liggitt - can you take a look as well?

k8s-bot · 2016-02-23T16:55:49Z

GCE e2e test build/test passed for commit f3b8b739d0ef19fe6c0fce50128077eb4b85bfea.

lavalamp · 2016-02-23T17:53:07Z

@krousey Can you take a look?

krousey · 2016-02-23T17:54:55Z

@lavalamp sure. Is this 1.2 critical?

lavalamp · 2016-02-23T18:00:55Z

It's 1.2 nice-to-have.

derekwaynecarr · 2016-02-23T18:01:54Z

Looks like my last commit broke a unit test, investigating.

@lavalamp @krousey I agree its a nice-to-have for 1.2. I always worry that people forget to handle deletion issues as things move around API groups, and this just eliminates that worry from the project.

derekwaynecarr · 2016-02-23T19:01:36Z

Recording here so I do not forget:

To support re-using the same controller to delete downstream OpenShift specific resources or in federated API server use cases, I will need to let a namespace controller have a finalizer token that it is associated with passed in during creation.

k8s-bot · 2016-02-23T19:15:56Z

GCE e2e test build/test passed for commit 05556c62977dc0c5d867d64c96040c2cebf0b390.

k8s-bot · 2016-02-23T19:17:52Z

GCE e2e test build/test passed for commit 788e65c7fda85d321ee96f667168487d16a7f1bb.

krousey · 2016-02-23T19:18:58Z

pkg/client/typed/dynamic/client_pool.go

+	if len(groupVersion.Group) == 0 {
+		return "/api"
+	}
+	return "/apis"


I know it's not, but shouldn't this be in the discovery into?

opened issue: #21916

lavalamp · 2016-02-29T21:26:32Z

I finally got a chance to go through this. @derekwaynecarr sorry it took me so long.

k8s-github-robot · 2016-02-29T22:05:20Z

PR needs rebase

derekwaynecarr · 2016-03-03T04:23:45Z

@lavalamp - rebased, and updated per code review requests. PTAL

k8s-bot · 2016-03-03T05:07:45Z

GCE e2e build/test passed for commit 773cf73.

eparis · 2016-03-03T17:52:36Z

@k8s-bot test this, issue #22435

k8s-bot · 2016-03-03T18:36:42Z

GCE e2e build/test passed for commit beace2d.

lavalamp · 2016-03-03T21:45:29Z

pkg/controller/namespace/namespace_controller_utils.go

+	// this is strange, but we need to special case for both MethodNotSupported and NotFound errors
+	// TODO: https://github.com/kubernetes/kubernetes/issues/22413
+	// we have a resource returned in the discovery API that supports no top-level verbs:
+	//  /apis/extensions/v1beta1/namespaces/default/replicationcontrollers


So, rather than this special case here, you may want to special case just this and keep it out of the resource list entirely. That path only exists to mount a subresource. We won't ever make anything like it again.

lavalamp · 2016-03-03T22:50:25Z

So I have a bunch of nitpicky things that I said and some I didn't say. But if you're confident that this works, I'm willing to hold my nose and LGTM.

derekwaynecarr · 2016-03-03T23:07:11Z

@lavalamp - I'm confident it works, as for holding noses, if we are fine holding our nose on ReplicationControllerDummy (#22413), I think this smells a little better than what we had before and vote to merge for 1.2.

lavalamp · 2016-03-03T23:56:53Z

LGTM thanks!

k8s-github-robot · 2016-03-04T04:55:21Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-03-04T05:32:48Z

GCE e2e build/test passed for commit beace2d.

k8s-github-robot · 2016-03-04T05:33:01Z

Automatic merge from submit-queue

…overy Auto commit by PR queue bot

googlebot added the cla: yes label Feb 17, 2016

k8s-github-robot assigned lavalamp Feb 17, 2016

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 17, 2016

derekwaynecarr force-pushed the namespace_deletion_discovery branch 3 times, most recently from 92c57fd to c41330b Compare February 23, 2016 06:59

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 23, 2016

derekwaynecarr force-pushed the namespace_deletion_discovery branch 2 times, most recently from 3e40b48 to 5ba3797 Compare February 23, 2016 08:22

derekwaynecarr changed the title ~~WIP: Namespace deletion uses discovery + dynamic client~~ Namespace deletion uses discovery + dynamic client Feb 23, 2016

derekwaynecarr added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 23, 2016

derekwaynecarr force-pushed the namespace_deletion_discovery branch 2 times, most recently from ed20247 to f3b8b73 Compare February 23, 2016 16:28

derekwaynecarr force-pushed the namespace_deletion_discovery branch 2 times, most recently from 05556c6 to 788e65c Compare February 23, 2016 18:47

krousey reviewed Feb 23, 2016
View reviewed changes

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

derekwaynecarr mentioned this pull request Mar 2, 2016

(WIP) Delete empty directories when namespaces are deleted openshift/origin#7344

Closed

Add ability to manage a pool of dynamic clients

3bda581

derekwaynecarr force-pushed the namespace_deletion_discovery branch from 41c4797 to 601096f Compare March 3, 2016 04:23

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

derekwaynecarr added 2 commits March 2, 2016 23:34

Move namespace controller to use discovery and dynamic client

41057b0

Add e2e tests to verify more resources are deleted

773cf73

derekwaynecarr force-pushed the namespace_deletion_discovery branch from 601096f to 773cf73 Compare March 3, 2016 04:36

Fix incorrect logging string that could cause confusion

beace2d

eparis mentioned this pull request Mar 3, 2016

Jenkins is regularly getting stuck never completing #22435

Closed

lavalamp reviewed Mar 3, 2016
View reviewed changes

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2016

k8s-github-robot pushed a commit that referenced this pull request Mar 4, 2016

Merge pull request #21400 from derekwaynecarr/namespace_deletion_disc…

258eac5

…overy Auto commit by PR queue bot

k8s-github-robot merged commit 258eac5 into kubernetes:master Mar 4, 2016

derekwaynecarr mentioned this pull request Mar 9, 2016

Add e2e test to verify pods are removed on namespace deletion #21567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespace deletion uses discovery + dynamic client #21400

Namespace deletion uses discovery + dynamic client #21400

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

lavalamp commented Feb 23, 2016

krousey commented Feb 23, 2016

lavalamp commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

krousey Feb 23, 2016

derekwaynecarr Feb 24, 2016

lavalamp commented Feb 29, 2016

k8s-github-robot commented Feb 29, 2016

derekwaynecarr commented Mar 3, 2016

k8s-bot commented Mar 3, 2016

eparis commented Mar 3, 2016

k8s-bot commented Mar 3, 2016

lavalamp Mar 3, 2016

lavalamp commented Mar 3, 2016

derekwaynecarr commented Mar 3, 2016

lavalamp commented Mar 3, 2016

k8s-github-robot commented Mar 4, 2016

k8s-bot commented Mar 4, 2016

k8s-github-robot commented Mar 4, 2016

Namespace deletion uses discovery + dynamic client #21400

Namespace deletion uses discovery + dynamic client #21400

Conversation

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

lavalamp commented Feb 23, 2016

krousey commented Feb 23, 2016

lavalamp commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

derekwaynecarr commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

krousey Feb 23, 2016

Choose a reason for hiding this comment

derekwaynecarr Feb 24, 2016

Choose a reason for hiding this comment

lavalamp commented Feb 29, 2016

k8s-github-robot commented Feb 29, 2016

derekwaynecarr commented Mar 3, 2016

k8s-bot commented Mar 3, 2016

eparis commented Mar 3, 2016

k8s-bot commented Mar 3, 2016

lavalamp Mar 3, 2016

Choose a reason for hiding this comment

lavalamp commented Mar 3, 2016

derekwaynecarr commented Mar 3, 2016

lavalamp commented Mar 3, 2016

k8s-github-robot commented Mar 4, 2016

k8s-bot commented Mar 4, 2016

k8s-github-robot commented Mar 4, 2016