-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Graceful deletion bumps object's generation #27269
Conversation
Cc @erictune |
What about when we remove a finalizer? Should that also bump the generation? |
@caesarxuchao will let you review-- is this the only place this change is needed? |
We haven't discussed bumping generation during finalizer removal - I'm not sure it's needed. |
The changes LGTM, but give me some time thinking if we need to do this in other places. |
This will bump generation for resources that otherwise don't set it. Is generation even initialized for all resources? |
It's a plain int64, so it's initialized to 0. |
Generic etcd registry code does generation bumps for all resource type On Tue, Jun 14, 2016 at 10:24 AM, Brian Grant notifications@github.com
|
@bgrant0607 - does this require release note? |
@gmarek Re. release note: The question to ask is "would some users care"? In this case, the answer is "yes". This is not a breaking change, but is a visible change in behavior. |
The resources that implement generation initialize it to 1 in PrepareForCreate. For instance:
The code that bumps generation is in PrepareForUpdate and is specific to each resource, because it's only bumped when a change to desired state that affects controller behavior is made. For instance:
|
OK-- I don't want to distribute this logic, it should be done in one place. On Wed, Jun 15, 2016 at 10:33 AM, Brian Grant notifications@github.com
|
@@ -89,5 +89,6 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx api.Context, obj runtime.Obje | |||
now := unversioned.NewTime(unversioned.Now().Add(time.Second * time.Duration(*options.GracePeriodSeconds))) | |||
objectMeta.DeletionTimestamp = &now | |||
objectMeta.DeletionGracePeriodSeconds = options.GracePeriodSeconds | |||
objectMeta.Generation = objectMeta.Generation + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually doing it here is not enough. We set DeletionTimestamp here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@lavalamp @bgrant0607 - I added ">0" guards. Is this enough, as I also rather keep it in a single place - amount of boilerplate in the code is already too much, so I'd prefer not to add more. |
LGTM. I'll leave time for @lavalamp @bgrant0607 making more comments. |
@gmarek I'm fine with the guards, thanks. However, please add comments to explain where generation needs to be bumped and why, and why it doesn't need to be bumped in some places (e.g., where the object was already being deleted). BeforeDelete and markAsDeleting should cross-ref each other, also. If it's subtle enough that it wasn't obvious to you, it won't be obvious to others. |
Will do. |
678fe42
to
e0bae1a
Compare
Done. @bgrant0607 PTAL |
GCE e2e build/test passed for commit 678fe42f0dcdd82ab0fe140662b6b15d45dbec5a. |
Can this result in double generation bumps? |
// take any nontrivial actions, hence it's behavior changes. Thus we need to bump object's | ||
// Generation (if set). This handles generation bump during graceful deletion. For bump | ||
// caused by the finalizer-driven cascading deletion go to pkg/registry/generic/registry/store.go | ||
if objectMeta.Generation > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the strategy updates the generation this could potentially bump twice. I'm not sure we'd want the strategy to bump generation, but something we should all remember in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton do you foresee the damage caused by bumping generation twice? Controllers will just acknowledge it sees the latest one, so they don't care how many times the generation get bumped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why are we doing this. We want controllers to observe objects that are marked as deleted so I would say that a bumped generation in case of deletionTimestamp != nil is a feature rather than a bug? |
This looks ok to me. I don't think the risk is very high in general because delete should not allow mutation beyond this code path. |
It's mainly for controllers. We want to have an invariant that as long as an object has the same generation it's behavior is also the same. Setting deletion timestamp will mean that controllers will stop taking any meaningful actions, which means is starts behaving differently. @bgrant0607 |
Deletion is a change to spec (intent), roughly. |
Oh nevermind, I got this the other way around. Agreed that deletion should bump generation. |
// setting DeletionTimestamp to something != nil. Object that's being deleted shouldn't | ||
// take any nontrivial actions, hence it's behavior changes. Thus we need to bump object's | ||
// Generation (if set). This handles generation bump during graceful deletion. For bump | ||
// caused by the finalizer-driven cascading deletion go to pkg/registry/generic/registry/store.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the indention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Sorry.
@caesarxuchao PTAL |
GCE e2e build/test passed for commit 638f4e1. |
LGTM. Thank you! |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 638f4e1. |
Automatic merge from submit-queue |
Fix. #24935
cc @caesarxuchao