allow a deletestrategy to opt-out of GC #48354
Conversation
k8s-ci-robot
added
sig/api-machinery
k8s-github-robot
added
size/S
| @@ -39,6 +39,7 @@ type GarbageCollectionPolicy string | |||
| const ( | |||
| DeleteDependents GarbageCollectionPolicy = "DeleteDependents" | |||
| OrphanDependents GarbageCollectionPolicy = "OrphanDependents" | |||
| Unsupported GarbageCollectionPolicy = "Unsupported" | |||
There was a problem hiding this comment.
Would you expect the garbage collector to delete a resource that doesn't support garbage collection as part of a cascade?
There was a problem hiding this comment.
Would you expect the garbage collector to delete a resource that doesn't support garbage collection as part of a cascade?
No. Not supporting GC means not supporting GC. The ownerrefs would not be respected and the GC chain is lost when getting to that node in the graph.
There was a problem hiding this comment.
please add a comment what Unsupported means.
There was a problem hiding this comment.
No. Not supporting GC means not supporting GC.
With this implementation, the Unsupported resource won't break the background deletion chain.
There was a problem hiding this comment.
With this implementation, the Unsupported resource won't break the background deletion chain.
It does because the two sets of ignores match. I suppose if someone modified one and not the other they'd have a problem. We could expose GC-ability through discovery, but that feels separate.
There was a problem hiding this comment.
Also, it seems if someone deletes an "unsupported" resource with either "Orphan" or "foreground deletion", the object will be deleted immediately.
I don't think so. We enumerate the types here: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/garbagecollector/graph_builder.go#L235-L253, so they are never observed in the graph as being in or out. They are just missing and never acted upon.
There was a problem hiding this comment.
I see. All "unsupported" resources should also be in the "ignored" list of GC. Could you say that explicitly in the comment of Unsupported? The system will behave strangely if the "two sets of ignores" don't match.
There was a problem hiding this comment.
I see. All "unsupported" resources should also be in the "ignored" list of GC. Could you say that explicitly in the comment of Unsupported? The system will behave strangely if the "two sets of ignores" don't match.
I can, but do you think this should be exposed via discovery? I think there's a good argument to do that.
There was a problem hiding this comment.
I think so. It's no.2 in #26120 (comment). CRD should be able to opt-out GC as well.
There was a problem hiding this comment.
I think so. It's no.2 in #26120 (comment). CRD should be able to opt-out GC as well.
Let's work towards that instead.
deads2k
force-pushed
the
gc-01-deletenever
branch
from
a8c52bd
to
4cb7c7a
Compare
sttts
added
release-note-none
| @@ -637,6 +637,10 @@ func shouldUpdateFinalizerOrphanDependents(e *Store, accessor metav1.Object, opt | |||
| shouldOrphan = false | |||
| // Get default orphan policy from this REST object type | |||
| if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok { | |||
| if gcStrategy.DefaultGarbageCollectionPolicy() == rest.Unsupported { | |||
| // return true to force clear the GC policy if needed, and false to indicate that we should NOT orphan | |||
| return true, false | |||
There was a problem hiding this comment.
nit: shouldUpdate not necessary.
Can't we drop this shouldUpdate value from both funcs and add some more clever logic to shouldUpdateFinalizers to detect changes in the finalizers? With you change this gets even more complicated.
|
The overall approach lgtm. Some nits and possible simplification. |
deads2k
force-pushed
the
gc-01-deletenever
branch
from
4cb7c7a
to
39d54c3
Compare
k8s-github-robot
added
size/L
deads2k
force-pushed
the
gc-01-deletenever
branch
from
39d54c3
to
e2060ed
Compare
|
@sttts simplified in another commit. See if you prefer it. |
| if !shouldUpdate1 && !shouldUpdate2 { | ||
| return false, oldFinalizers | ||
| } | ||
| // deletionFinalizers returns the deletion finalizers we should set on the object. The cost |
There was a problem hiding this comment.
better move the optimization comment down where we call updateForGracefulDeletionAndFinalizers.
|
@deads2k code-wise it's much better with the 2nd commit. Wondering what the performance consequences are: one etcd GET more per deletion of non-graceful objects? |
Added back that bool. If some complains about garbage, we'll point them at the etcd get. Better? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm cancel Maybe squash before. |
k8s-ci-robot
removed
the
lgtm
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, sttts Associated issue requirement bypassed by: sttts The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
deads2k
force-pushed
the
gc-01-deletenever
branch
from
701d1b3
to
312fb1e
Compare
|
squash, reapplying label. |
deads2k
added
the
lgtm
|
Automatic merge from submit-queue (batch tested with PRs 47784, 47793, 48334, 48435, 48354) |
Not all resources should be GC-able and we implemented an ignore list to handle this, but at the storage layer they could still set finalizers, they just hung in a stuck state forever. This updates the strategy to allow a resource to indicate that they shouldn't be GCed.
@kubernetes/sig-api-machinery-misc