Skip to content
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

Merged
merged 1 commit into from
Jul 1, 2016

Conversation

gmarek
Copy link
Contributor

@gmarek gmarek commented Jun 13, 2016

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 13, 2016
@bgrant0607
Copy link
Member

Cc @erictune

@gmarek
Copy link
Contributor Author

gmarek commented Jun 13, 2016

@k8s-bot unit test this issue: #IGNORE
no test results <- cc @fejta

@lavalamp
Copy link
Member

What about when we remove a finalizer? Should that also bump the generation?

@lavalamp lavalamp assigned caesarxuchao and unassigned lavalamp and bgrant0607 Jun 13, 2016
@lavalamp
Copy link
Member

@caesarxuchao will let you review-- is this the only place this change is needed?

@gmarek
Copy link
Contributor Author

gmarek commented Jun 13, 2016

We haven't discussed bumping generation during finalizer removal - I'm not sure it's needed.

@caesarxuchao
Copy link
Member

caesarxuchao commented Jun 14, 2016

The changes LGTM, but give me some time thinking if we need to do this in other places.

@bgrant0607
Copy link
Member

This will bump generation for resources that otherwise don't set it.

Is generation even initialized for all resources?

cc @janetkuo @pwittrock

@gmarek
Copy link
Contributor Author

gmarek commented Jun 14, 2016

It's a plain int64, so it's initialized to 0.

@lavalamp
Copy link
Member

Generic etcd registry code does generation bumps for all resource type
already, afaik.

On Tue, Jun 14, 2016 at 10:24 AM, Brian Grant notifications@github.com
wrote:

This will bump generation for resources that otherwise don't set it.

Is generation even initialized for all resources?

cc @janetkuo https://github.com/janetkuo @pwittrock
https://github.com/pwittrock


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#27269 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAnglrKan_B1qs-wOZruIehWEqfWAqQqks5qLuPrgaJpZM4I0Rd_
.

@gmarek
Copy link
Contributor Author

gmarek commented Jun 15, 2016

@k8s-bot test this issue: #27344

@gmarek
Copy link
Contributor Author

gmarek commented Jun 15, 2016

@bgrant0607 - does this require release note?

@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 15, 2016
@bgrant0607
Copy link
Member

@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.

@bgrant0607
Copy link
Member

@lavalamp @gmarek

The resources that implement generation initialize it to 1 in PrepareForCreate. For instance:

ingress.Generation = 1

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:

if !reflect.DeepEqual(newDeployment.Spec, oldDeployment.Spec) ||

@lavalamp
Copy link
Member

OK-- I don't want to distribute this logic, it should be done in one place.
Are you satisfied if we add a if generation > 0 { ... } guard around this
logic but keep it here?

On Wed, Jun 15, 2016 at 10:33 AM, Brian Grant notifications@github.com
wrote:

@lavalamp https://github.com/lavalamp @gmarek
https://github.com/gmarek

The resources that implement generation initialize it to 1 in
PrepareForCreate. For instance:

ingress.Generation = 1

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:

if !reflect.DeepEqual(newDeployment.Spec, oldDeployment.Spec) ||


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27269 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAnglt4eaWjWXtEpMt6dyaOCawqTiNqkks5qMDdSgaJpZM4I0Rd_
.

@@ -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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@gmarek
Copy link
Contributor Author

gmarek commented Jun 16, 2016

@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.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 16, 2016
@caesarxuchao
Copy link
Member

LGTM. I'll leave time for @lavalamp @bgrant0607 making more comments.

@bgrant0607
Copy link
Member

@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.

@gmarek
Copy link
Contributor Author

gmarek commented Jun 20, 2016

Will do.

@gmarek gmarek force-pushed the controllerRef branch 3 times, most recently from 678fe42 to e0bae1a Compare June 23, 2016 12:13
@gmarek
Copy link
Contributor Author

gmarek commented Jun 23, 2016

Done. @bgrant0607 PTAL

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

GCE e2e build/test passed for commit 678fe42f0dcdd82ab0fe140662b6b15d45dbec5a.

@smarterclayton smarterclayton changed the title Setting deletion timestamp bumps object's generation Graceful deletion bumps object's generation Jun 23, 2016
@smarterclayton
Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xmichalis
Copy link
Contributor

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?

@smarterclayton
Copy link
Contributor

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.

@gmarek
Copy link
Contributor Author

gmarek commented Jun 23, 2016

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

@smarterclayton
Copy link
Contributor

Deletion is a change to spec (intent), roughly.

@0xmichalis
Copy link
Contributor

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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Sorry.

@gmarek
Copy link
Contributor Author

gmarek commented Jul 1, 2016

@caesarxuchao PTAL

@k8s-bot
Copy link

k8s-bot commented Jul 1, 2016

GCE e2e build/test passed for commit 638f4e1.

@caesarxuchao
Copy link
Member

LGTM. Thank you!

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2016
@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Jul 1, 2016

GCE e2e build/test passed for commit 638f4e1.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0e24db8 into kubernetes:master Jul 1, 2016
@gmarek gmarek deleted the controllerRef branch August 30, 2016 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants