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

jawnsy · 2016-02-16T15:04:02Z

This change resolves #7032

liggitt · 2016-02-16T15:13:04Z

before this would work, we'd need the new namespace controller from upstream that uses deletecollection, and to update the origin namespace controller to use deletecollection

jawnsy · 2016-02-19T21:34:27Z

Hey Jordan,

We did some research and found that 300k empty directories takes 254MB in memory and 215MB on disk (etcd). Even though this solution isn't perfect, we'd like to get this change into Origin as soon as possible. This means we'll diverge from upstream Kubernetes, but that should be okay since eventually Kubernetes will switch to etcd 3.x which doesn't have this problem.

Can you review this request? I tested by making the same changes in upstream Kubernetes and saw that the changes work, for the most part (just excluding the kinds like pods that aren't using the etcd registry.)

cc @derekwaynecarr @abhgupta

Jonathan

liggitt · 2016-02-19T21:47:55Z

that doesn't seem like a big enough impact to justify carrying this

derekwaynecarr · 2016-02-19T22:05:09Z

unit tests?

liggitt · 2016-02-19T22:08:30Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/storage/etcd/etcd_helper.go

@@ -264,6 +264,20 @@ func (h *etcdHelper) Delete(ctx context.Context, key string, out runtime.Object)
 }

 // Implements storage.Interface.
+func (h *etcdHelper) DeleteDir(ctx context.Context, directory string) error {
+	if ctx == nil {
+		glog.Errorf("Context is nil")


return an error, don't log

@liggitt Wouldn't this make the behaviour of DeleteDir different from the rest of the methods, e.g. Watch?

you're right... the other places are weird, too

if ctx is required, we should return an error
if it's not required, we shouldn't log an error

derekwaynecarr · 2016-02-19T22:09:31Z

read about how to name commits when making upstream changes:

origin/tools/rebasehelpers/commitchecker/validate.go

Line 34 in 273458a

UPSTREAM: <carry>: A carried kube change

https://github.com/openshift/origin/blob/master/HACKING.md#cherry-picking-an-upstream-commit-into-origin

derekwaynecarr · 2016-02-19T22:15:03Z

@liggitt - so I think its fair to have @jawnsy determine what kinds in openshift would actually benefit from the change looking at the code that is actually in the code base today. As you note, this helps with upstream, but does not yet fix the dangling pod and service folders. Nothing downstream uses delete collection, so maybe he can get a PR to start moving the ball forward on that in parallel since I do not anticipate we will get the dynamic client in time for 3.2 since it has not yet merged in 1.2 kube.

I agree there is no sense in carrying the patch if its ignored by a large number of downstream resources.

jawnsy · 2016-03-01T23:19:06Z

I was struggling to answer @derekwaynecarr's question for a few days, so @liggitt took mercy on me and wrote some quick Go code to determine which resources need to be updated.

Here's the code for posterity (dropped into master.go in the GetRestStorage() func after we build the storage map):

    for resource, handler := range storage {
        _, isLister := handler.(rest.Lister)
        _, isDeleter := handler.(rest.Deleter)
        _, isGracefulDeleter := handler.(rest.GracefulDeleter)
        _, isCollectionDeleter := handler.(rest.CollectionDeleter)
        if isLister && (isDeleter || isGracefulDeleter) && !isCollectionDeleter {
            fmt.Println(resource)
        }
    }
    os.Exit(1)

This outputs:

roles
clusterRoleBindings
projects
imageStreamTags
oAuthAuthorizeTokens
roleBindings
oAuthAccessTokens
clusterRoles

This is the list of things that have to be listed and deleted one-by-one. However, roles, clusterRoleBindings, projects, imageStreamTags, roleBindings, and clusterRoles are all virtual (they're backed by other resources) so we don't have to worry about them.

Thus the remaining real resources that need to be updated are oAuthAuthorizeTokens and oAuthAccessTokens to use the new generic etcd storage mechanism.

@abhgupta how should we proceed given the above?

liggitt · 2016-03-01T23:31:30Z

Also note that both of those types are not namespaced, so would not factor into per-namespace empty directory counts

abhgupta · 2016-03-02T00:56:50Z

So, for all of the project scoped resources, the current PR will be able to delete their leftover directories in etcd. Is my understanding correct?

Also, since this PR is not being submitted/accepted upstream K8s, what is the cost of carrying this patch in origin and do we have consensus on carrying this patch in origin?

liggitt · 2016-03-02T01:04:01Z

No, that just means that all the namespaced resources have a deletecollection API method. The controller that cleans up origin resources has not been updated to use deletecollection yet. The controller that cleans up kube resources uses deletecollection for most, but not all kube resources.

abhgupta · 2016-03-02T17:39:09Z

Alright, so it doesn't really make sense to have this patch at our end till we have the namespaced resources in origin being cleaned up via the deletecollection API call.

Agreed?

derekwaynecarr · 2016-03-02T19:30:26Z

This should make kube 1.2 if I ever get a chance to finally finish the last code review updates:
kubernetes/kubernetes#21400

It will move to use delete collection for all things that support it upstream - which is everything but services. After the next rebase, we need to assess using this controller in origin for origin resources or updating the existing controller to call delete collection for origin resources. I think it should support reuse, but we cannot use discovery in OpenShift to find its namespaced resources yet so it would be a hard coded list of group kinds that get passed into controller.

liggitt · 2016-03-02T19:36:09Z

everything but services

and pods, for graceful deletion purposes, right?

derekwaynecarr · 2016-03-16T15:43:57Z

@liggitt - pods now use delete collection.

per the latest rebase, using the upstream namespace controller to delete downstream OpenShift resources is listed as a p2 #7975

openshift-bot · 2016-03-16T19:05:33Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

derekwaynecarr · 2016-03-18T19:38:33Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/registry/generic/etcd/etcd.go

+
+	// Try to delete the directory, which will only succeed if the directory is empty.
+	// We can ignore errors since the cost of an empty directory is relatively small.
+	e.Storage.DeleteDir(ctx, e.KeyRootFunc(ctx));


can we not call this when list options has provided a field selector or label selector?

delete collection lets you scope the set of items to delete....

We should only call this when the ctx has no namespace.

do you mean when it has a namespace?

Yes, that is correct. We only want to delete namespaced resources. The comment was probably a typo.

sorry, yes, it was a typo.

derekwaynecarr · 2016-03-18T19:44:21Z

https://github.com/openshift/origin/blob/master/pkg/project/controller/controller.go#L100

^^^

I am less concerned about changing the existing controller to call delete collection on a per call basis.

liggitt · 2016-03-24T21:02:30Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/registry/generic/etcd/etcd.go

@@ -534,12 +534,22 @@ func (e *Etcd) DeleteCollection(ctx api.Context, options *api.DeleteOptions, lis
 		}()
 	}
 	wg.Wait()
+
+	if listOptions.LabelSelector == nil && listOptions.FieldSelector == nil && api.NamespaceValue(ctx) != "" {


move this to the success case (just before we return listObj, nil below)

these can be nil or the result of labels.Everything(), etc... change to

anyLabels := listOptions.LabelSelector == nil || listOptions.LabelSelector.Empty() anyFields := listOptions.FieldSelector == nil || listOptions.FieldSelector.Empty() if anyLabels && anyFields { ... }

Updated the PR, thanks for the review!

Signed-off-by: Jonathan Yu <jawnsy@redhat.com>

…tion Whenever DeleteCollection is invoked, we attempt to delete the parent directory using DeleteDir, which will only succeed if the directory is, in fact, empty. This prevents empty directories from collecting and consuming memory. Signed-off-by: Jonathan Yu <jawnsy@redhat.com>

jawnsy · 2016-05-13T19:37:01Z

We are going to withdraw this PR for now, as given the current plan and estimated growth rate, we do not anticipate the extra memory usage to be problematic. We can always reopen this if it becomes necessary later.

jawnsy changed the title ~~(WORK-IN-PROGRESS) Delete empty directories~~ Delete empty directories when namespaces are deleted Feb 19, 2016

liggitt reviewed Feb 19, 2016
View reviewed changes

jawnsy changed the title ~~Delete empty directories when namespaces are deleted~~ (WIP) Delete empty directories when namespaces are deleted Feb 29, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2016

derekwaynecarr reviewed Mar 18, 2016
View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2016

liggitt reviewed Mar 24, 2016
View reviewed changes

Jonathan Yu added 2 commits March 29, 2016 15:52

UPSTREAM: <carry>: Add new interface for deleting directories via etcd

243c04a

Signed-off-by: Jonathan Yu <jawnsy@redhat.com>

jawnsy self-assigned this Apr 26, 2016

jawnsy closed this May 13, 2016

derekwaynecarr mentioned this pull request Jun 24, 2016

etcd 2: directory cleanup needed after namespace deletion? kubernetes/kubernetes#27307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jawnsy commented Feb 16, 2016

liggitt commented Feb 16, 2016

jawnsy commented Feb 19, 2016

liggitt commented Feb 19, 2016

derekwaynecarr commented Feb 19, 2016

liggitt Feb 19, 2016

jawnsy Mar 24, 2016

liggitt Mar 24, 2016

derekwaynecarr commented Feb 19, 2016

derekwaynecarr commented Feb 19, 2016

jawnsy commented Mar 1, 2016

liggitt commented Mar 1, 2016

abhgupta commented Mar 2, 2016

liggitt commented Mar 2, 2016

abhgupta commented Mar 2, 2016

derekwaynecarr commented Mar 2, 2016

liggitt commented Mar 2, 2016

derekwaynecarr commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

derekwaynecarr Mar 18, 2016

derekwaynecarr Mar 18, 2016

liggitt Mar 18, 2016

abhgupta Mar 18, 2016

derekwaynecarr Mar 21, 2016

derekwaynecarr commented Mar 18, 2016

liggitt Mar 24, 2016

liggitt Mar 24, 2016

jawnsy Mar 24, 2016

jawnsy commented May 13, 2016

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

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

Conversation

jawnsy commented Feb 16, 2016

liggitt commented Feb 16, 2016

jawnsy commented Feb 19, 2016

liggitt commented Feb 19, 2016

derekwaynecarr commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Feb 19, 2016

derekwaynecarr commented Feb 19, 2016

jawnsy commented Mar 1, 2016

liggitt commented Mar 1, 2016

abhgupta commented Mar 2, 2016

liggitt commented Mar 2, 2016

abhgupta commented Mar 2, 2016

derekwaynecarr commented Mar 2, 2016

liggitt commented Mar 2, 2016

derekwaynecarr commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

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

derekwaynecarr commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawnsy commented May 13, 2016