Support collection deletion in apiserver. #18290

wojtek-t · 2015-12-07T13:34:11Z

What does it give us:

in scalability tests, namespace deletion takes ~3m instead of ~30m
the API is pretty generic, although the first implementation is not the ideal one - this should work much better in etcd v3 api

@derekwaynecarr @smarterclayton @lavalamp @gmarek

wojtek-t · 2015-12-07T13:34:54Z

@smarterclayton @derekwaynecarr - can you please take a first look? If you're fine with the approach, I will extend test, etc.

k8s-github-robot · 2015-12-07T13:35:54Z

Labelling this PR as size/L

k8s-bot · 2015-12-07T14:04:46Z

GCE e2e test build/test passed for commit 2cb31c4a80b0ef08ac4ba19f3b6172fc69df9404.

derekwaynecarr · 2015-12-07T14:45:17Z

pkg/apiserver/resthandler.go

+		if admit != nil && admit.Handles(admission.Delete) {
+			userInfo, _ := api.UserFrom(ctx)
+
+			err = admit.Admit(admission.NewAttributesRecord(nil, scope.Kind.Kind, namespace, "", scope.Resource.Resource, scope.Subresource, admission.Delete, userInfo))


I am wondering if admission control should make it clearer when you are doing a delete over an entire collection rather than just DELETE + kind + (optional namespace). It's probably fine as is, but will mull on it some.

I'm far from being an expert in admission. I did something that was the easiest to do, but if you think we should do something different, I'm happy to change it.

This would require (for instance) admission controllers that track resource cluster usage to also track bulk deletion and implement the same logic that the list call does. Generally we want admission controllers to operate on concrete objects. At a minimum the list Options have to be passed in.

@smarterclayton - not sure if I understood. Why do you want to pass ListOptions? Do you want to have it based on selectors?
Does it have to be part of this PR (I have almost zero knowledge about admission control).

derekwaynecarr · 2015-12-07T14:53:20Z

I am fine with the approach.

wojtek-t · 2015-12-07T15:01:16Z

@derekwaynecarr - thanks!

I will cleanup this PR, add some tests, etc. and will let you know (probably tomorrow).

timothysc · 2015-12-07T22:18:39Z

pkg/api/rest/rest.go

@@ -125,6 +125,13 @@ func (w GracefulDeleteAdapter) Delete(ctx api.Context, name string, options *api
 	return w.Deleter.Delete(ctx, name)
 }

+// CollectionDeleter is an object that can delete a collection
+// of RESTful resources.
+type CollectionDeleter interface {


could we mark as TODO for future cleanup.

Sorry - I don't understand this comment. What you would like to cleanup with it?

Aren't we planning on changing this post etcd3?

I don't think so. I think the interface should remain as it is here.

What we would like to cleanup is the implementation of DeleteCollection in pkg/registry - but there is already a TODO there.

Will review first thing tomorrow morning.

On Tuesday, December 8, 2015, Timothy St. Clair notifications@github.com
wrote:

In pkg/api/rest/rest.go
#18290 (comment)
:

@@ -125,6 +125,13 @@ func (w GracefulDeleteAdapter) Delete(ctx api.Context, name string, options *api
return w.Deleter.Delete(ctx, name)
}

+// CollectionDeleter is an object that can delete a collection
+// of RESTful resources.
+type CollectionDeleter interface {

sgtm

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/18290/files#r46977051.

@derekwaynecarr - great thanks!

k8s-bot · 2015-12-08T09:37:35Z

GCE e2e build/test failed for commit e643cb3452a884b70e676257dea136e0889e1d1d.

k8s-bot · 2015-12-08T11:27:30Z

GCE e2e build/test failed for commit ab750984381e741fe5d09771fa12552b19f16c04.

k8s-github-robot · 2015-12-08T12:02:10Z

Labelling this PR as size/XXL

k8s-bot · 2015-12-08T13:12:50Z

GCE e2e build/test failed for commit 576c57997b0638e522b1011308becc44e4675911.

wojtek-t · 2015-12-08T13:57:52Z

@derekwaynecarr @smarterclayton - I cleaned up this PR and it is ready for review
The e2e failures are not related to this PR - we have huge problems with our e2e now and are trying to fix them - will rerun once the problem is solved.

PTAL

derekwaynecarr · 2015-12-10T00:06:12Z

@wojtek-t - so you are only looking to make the namespace controller use this for Events in this PR? want to make sure I get the initial scope, or if you plan to extend this for other resource types?

derekwaynecarr · 2015-12-10T00:17:20Z

@deads2k - just a heads up, but this PR would allow for DELETE /api/v1/namespace/foo/pods/ to do a DELETE_ALL, which I think is fine for our policy concerns, but wanted to give you a heads up.

wojtek-t · 2015-12-10T08:48:40Z

@derekwaynecarr - thanks a lot for looking into this PR

@wojtek-t - so you are only looking to make the namespace controller use this for Events in this PR? want to make sure I get the initial scope, or if you plan to extend this for other resource types?

Yes - in this PR I'm targeting to change only Events client. This is because @caesarxuchao is working on auto-generating the whole client code, so we will get all other resources for free. (I think other resources are not that urgent - if you think they are, I'm happy to add them, but i would prefer doing it in a separate PR).

I applied all comments - PTAL

k8s-bot · 2015-12-10T09:15:42Z

GCE e2e test build/test passed for commit f28bb68.

deads2k · 2015-12-10T13:44:28Z

@deads2k - just a heads up, but this PR would allow for DELETE /api/v1/namespace/foo/pods/ to do a DELETE_ALL, which I think is fine for our policy concerns, but wanted to give you a heads up.

It's fine for policy, but will it affect things that are watching for deletion event? I seem to recall that etcd didn't send deletion events for individual items removed. Sorry to be lazy; I'd dig through the pull, but its a little large.

wojtek-t · 2015-12-10T13:52:00Z

It's fine for policy, but will it affect things that are watching for deletion event?

@deads2k This problem is solved - watches work fine with it.

wojtek-t · 2015-12-10T15:14:41Z

@derekwaynecarr - PTAL

derekwaynecarr · 2015-12-10T15:37:42Z

@deads2k - yeah, this is basically letting you do a DELETE of the collection to the API server, and then the API server right now makes N calls to etcd so watch behavior is preserved, but we can get around qps throttling to the API server.

@wojtek-t - lgtm, thanks!

wojtek-t · 2015-12-10T15:39:09Z

@derekwaynecarr - thanks a lot for review!

k8s-github-robot · 2015-12-10T17:56:14Z

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

k8s-bot · 2015-12-10T18:26:06Z

GCE e2e test build/test passed for commit f28bb68.

wojtek-t · 2015-12-10T18:39:24Z

@k8s-bot unit test this please

k8s-github-robot · 2015-12-11T14:01:51Z

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

k8s-bot · 2015-12-11T14:33:41Z

GCE e2e test build/test passed for commit f28bb68.

k8s-github-robot · 2015-12-11T15:17:15Z

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

k8s-bot · 2015-12-11T15:47:24Z

GCE e2e build/test failed for commit f28bb68.

wojtek-t · 2015-12-11T16:02:09Z

@k8s-bot e2e test this please

k8s-bot · 2015-12-11T16:39:41Z

GCE e2e test build/test passed for commit f28bb68.

Support collection deletion in apiserver.

bgrant0607 · 2016-02-12T06:09:33Z

Ref #10217

@gnufied

Automatic merge from submit-queue (batch tested with PRs 18390, 18389, 18290, 18377, 18385). UPSTREAM: 56288: Add list of pods that use a volume to multiattach events Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1511097 /assign @gnufied @rootfs Origin-commit: 84cbae163e58ec7dcef6569e89898c6ad8a207e6

googlebot added the cla: yes label Dec 7, 2015

k8s-github-robot assigned lavalamp Dec 7, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2015

wojtek-t mentioned this pull request Dec 7, 2015

WIP Modify REST framework to support deleting all resources in a namespace #7372

Closed

wojtek-t assigned derekwaynecarr and unassigned lavalamp Dec 7, 2015

derekwaynecarr reviewed Dec 7, 2015
View reviewed changes

timothysc reviewed Dec 7, 2015
View reviewed changes

wojtek-t force-pushed the fast_namespace_deletion branch 2 times, most recently from e643cb3 to ab75098 Compare December 8, 2015 09:25

wojtek-t force-pushed the fast_namespace_deletion branch from ab75098 to 576c579 Compare December 8, 2015 11:59

wojtek-t closed this Dec 8, 2015

wojtek-t reopened this Dec 8, 2015

k8s-github-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 8, 2015

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 8, 2015

wojtek-t changed the title ~~[WIP] Support collection deletion in apiserver.~~ Support collection deletion in apiserver. Dec 8, 2015

wojtek-t force-pushed the fast_namespace_deletion branch from 576c579 to 831c887 Compare December 9, 2015 10:27

Support collection deletion in apiserver.

f28bb68

wojtek-t force-pushed the fast_namespace_deletion branch from 831c887 to f28bb68 Compare December 10, 2015 08:46

derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2015

j3ffml added a commit that referenced this pull request Dec 11, 2015

Merge pull request #18290 from wojtek-t/fast_namespace_deletion

f6686ba

Support collection deletion in apiserver.

j3ffml merged commit f6686ba into kubernetes:master Dec 11, 2015

wojtek-t deleted the fast_namespace_deletion branch December 15, 2015 14:00

caesarxuchao mentioned this pull request Dec 15, 2015

Update client-generator #18700

Merged

davidopp mentioned this pull request Jan 10, 2016

Namespaces should delete fast enough is flaky in parallel #19026

Closed

liggitt mentioned this pull request Feb 10, 2016

Use a different verb for delete collection #21005

Merged

Support collection deletion in apiserver. #18290

Support collection deletion in apiserver. #18290

Conversation

wojtek-t commented Dec 7, 2015

wojtek-t commented Dec 7, 2015

k8s-github-robot commented Dec 7, 2015

k8s-bot commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Dec 7, 2015

wojtek-t commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Dec 8, 2015

k8s-bot commented Dec 8, 2015

k8s-github-robot commented Dec 8, 2015

k8s-bot commented Dec 8, 2015

wojtek-t commented Dec 8, 2015

derekwaynecarr commented Dec 10, 2015

derekwaynecarr commented Dec 10, 2015

wojtek-t commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

deads2k commented Dec 10, 2015

wojtek-t commented Dec 10, 2015

wojtek-t commented Dec 10, 2015

derekwaynecarr commented Dec 10, 2015

wojtek-t commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

wojtek-t commented Dec 10, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

wojtek-t commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

bgrant0607 commented Feb 12, 2016