GC: update required verbs for deletable resources, allow list of ignored resources to be customized#45897
Conversation
|
@ncdc nicely done! code lgtm. |
There was a problem hiding this comment.
Rename to DefaultIgnoredResources()?
|
Last two commits lgtm except for a nit. |
a1cce10 to
d31fedd
Compare
|
lgtm for the last two commits. |
d31fedd to
a52aa5d
Compare
a52aa5d to
a2a9dfa
Compare
There was a problem hiding this comment.
Pretty sure that we use a live "get" to confirm existence and "patch" to orphan resources.
There was a problem hiding this comment.
So you want to make sure that for any resource we might want to gc or orphan, that we can get/list/watch/patch/delete?
There was a problem hiding this comment.
So you want to make sure that for any resource we might want to gc or orphan, that we can get/list/watch/patch/delete?
Yes
There was a problem hiding this comment.
I just looked and we also use "update" to remove the finalizer (there is a todo to switch to patch when strategic merge supports removing an entry from a slice of a base type). I'll make the appropriate changes.
There was a problem hiding this comment.
I think this should be option on the config struct contained in the controller context. It's logically config information even if its never exposed via flags. You can set the default where its created.
There was a problem hiding this comment.
|
@deads2k updated |
|
You'll need to define a serializeable GroupResource, but yes.
…On Tue, May 23, 2017 at 8:55 AM, Andy Goldstein ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/kube-controller-manager/app/core.go
<#45897 (comment)>
:
> @@ -189,7 +189,14 @@ func startGarbageCollectorController(ctx ControllerContext) (bool, error) {
metaOnlyClientPool := dynamic.NewClientPool(config, restMapper, dynamic.LegacyAPIPathResolverFunc)
config.ContentConfig = dynamic.ContentConfig()
clientPool := dynamic.NewClientPool(config, restMapper, dynamic.LegacyAPIPathResolverFunc)
- garbageCollector, err := garbagecollector.NewGarbageCollector(metaOnlyClientPool, clientPool, restMapper, deletableGroupVersionResources, ctx.InformerFactory)
+ garbageCollector, err := garbagecollector.NewGarbageCollector(
+ metaOnlyClientPool,
+ clientPool,
+ restMapper,
+ deletableGroupVersionResources,
+ garbagecollector.DefaultIgnoredResources(),
@deads2k <https://github.com/deads2k> like this?
https://gist.github.com/ncdc/cc366247ebda5b434a3da0668637613d
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45897 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH2BSrhogtV9OlyToKN5AOHUoJiZ2VhDks5r8tcrgaJpZM4Nc1y6>
.
|
What do you mean / when / where? |
Down inside KubeControllerManagerConfiguration |
|
I didn't make it part of |
|
@deads2k if it needs to be in |
|
Updated, now with GCIgnoredResources living in the |
|
lgtm please squash before merge. |
The garbage collector controller currently needs to list, watch, get, patch, update, and delete resources. Update the criteria for deletable resources to reflect this.
Allow the list of resources the garbage collector controller should ignore to be customizable, so downstream integrators can add their own resources to the list, if necessary.
99d91a5 to
d1a0384
Compare
|
Squashed |
|
lgtm |
|
@k8s-bot pull-kubernetes-kubemark-e2e-gce test this |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this (took too long to delete some namespaces). |
|
/lgtm |
|
/approve based on reviewers signing off |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ncdc, smarterclayton DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 46149, 45897, 46293, 46296, 46194) |
|
@ncdc: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
The garbage collector controller currently needs to list, watch, get,
patch, update, and delete resources. Update the criteria for
deletable resources to reflect this.
Also allow the list of resources the garbage collector controller should
ignore to be customizable, so downstream integrators can add their own
resources to the list, if necessary.
cc @caesarxuchao @deads2k @smarterclayton @mfojtik @liggitt @sttts @kubernetes/sig-api-machinery-pr-reviews