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

Support graceful deletion of resources #5085

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

smarterclayton
Copy link
Contributor

This commit adds support for graceful deletion of resources, although it does not enable that on any resource yet (follow up pulls will handle that).

DELETE /pods/foo
{
  "kind": "DeleteOptions",
  "gracePeriodSeconds": 300
}

If grace period is empty, delete follows the default rules for the resource type. If a delete is already in progress and gracePeriod is shorter than the existing period, the grace period will be shortened. Will return a 202 accepted. If gracePeriod is zero, delete is immediate.

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@smarterclayton
Copy link
Contributor Author

Addresses #1535

@smarterclayton smarterclayton force-pushed the allow_graceful_delete branch 4 times, most recently from 49a4b1a to f1737d2 Compare March 5, 2015 05:28
@erictune erictune assigned erictune and smarterclayton and unassigned erictune Mar 5, 2015
@erictune
Copy link
Member

erictune commented Mar 5, 2015

assigning WIP PR to author until it becomes ready for review.

@smarterclayton
Copy link
Contributor Author

I would love to review my own code :)

On Mar 5, 2015, at 10:50 AM, Eric Tune notifications@github.com wrote:

Assigned #5085 to @smarterclayton.


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

As if I don't have enough PRs to review, assigning to myself for now to vet the approach.

@bgrant0607
Copy link
Member

I don't have a strong preference of delete vs. a stop/control subresource. I think we'll need a control subresource eventually, but that doesn't invalidate the delete approach.

I do think we need to support multiple stages:

  • Start graceful wind-down, with a client-specified reason and timeout, with the timeout potentially capped by the system depending on QoS or other policy.
  • The fact that the object is winding down either needs to be recorded in spec or metadata (as in this PR), so that whatever controller is responsible can continue whatever shutdown process is required.
  • Hard kill after the timeout.
  • Set object TTL to some fixed duration, common to all objects, to ensure the object is observable.

@smarterclayton
Copy link
Contributor Author

Agree

On Mar 12, 2015, at 12:30 AM, Brian Grant notifications@github.com wrote:

I don't have a strong preference of delete vs. a stop/control subresource. I think we'll need a control subresource eventually, but that doesn't invalidate the delete approach.

I do think we need to support multiple stages:

Start graceful wind-down, with a client-specified reason and timeout, with the timeout potentially capped by the system depending on QoS or other policy.
The fact that the object is winding down either needs to be recorded in spec or metadata (as in this PR), so that whatever controller is responsible can continue whatever shutdown process is required.
Hard kill after the timeout.
Set object TTL to some fixed duration, common to all objects, to ensure the object is observable.

Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

Ok, so GracePeriod is just one number. Does it specify the wind-down period, or the post-wind-down deletion TTL?

@smarterclayton
Copy link
Contributor Author

Same right now. As discussed in
December, the ttl is "when do I want this name released". We made the argument that retrieval post name release had to be via UID or via a "deleted items" queue. Ideally, preservation post deletion would make this an administrative policy type decision.

I'm more worried about not releasing the name and its impact on the user experience (scripting is unpredictable). If the Kubelet can mark early release (wind down complete) then it can also make the deletion immediate, which allows the deletion to propagate to other clients and watchers. Propogating DELETE at the moment the pod is truly done feels most consistent with the expectations of the rest of the system.

On Mar 13, 2015, at 1:23 AM, Brian Grant notifications@github.com wrote:

Ok, so GracePeriod is just one number. Does it specify the wind-down period, or the post-wind-down deletion TTL?


Reply to this email directly or view it on GitHub.

@satoshi75nakamoto
Copy link
Contributor

I agree with @smarterclayton that propagating the DELETE at the moment the pod is truly done feels most consistent. Is there any documentation to support this modification?

@smarterclayton
Copy link
Contributor Author

No, I'll probably work to get the Kubelet to delete the pod when graceful completes (if it hasn't passed the grace period), get more feedback, then write doc.

----- Original Message -----

I agree with @smarterclayton that propagating the DELETE at the moment the
pod is truly done feels most consistent. Is there any documentation to
support this modification?


Reply to this email directly or view it on GitHub:
#5085 (comment)

@satoshi75nakamoto
Copy link
Contributor

Sounds like a good plan @smarterclayton. I'm looking forward to this PR being complete.

@derekwaynecarr
Copy link
Member

@smarterclayton - thanks, this should help me make progress once this merges. let me know if we need help on reviews.

@smarterclayton smarterclayton force-pushed the allow_graceful_delete branch from b68c404 to e9f7c4d Compare March 18, 2015 21:17
@smarterclayton
Copy link
Contributor Author

Any reviews helpful.

{
"kind": "DeleteOptions",
"apiVersion": "v1beta1",
"gracePeriod": 0%s
Copy link
Member

Choose a reason for hiding this comment

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

gracePeriodSeconds.

What's the %s for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra data added to all requests. Sprintf adds extra data if you send a value and there's no format, so the substitution, while empty, is necessary.

On Mar 18, 2015, at 8:35 PM, Brian Grant notifications@github.com wrote:

In test/integration/auth_test.go:

@@ -169,6 +169,14 @@ var aEndpoints string = }

+var deleteNow string = `
+{

  • "kind": "DeleteOptions",
  • "apiVersion": "v1beta1",
  • "gracePeriod": 0%s
    gracePeriodSeconds.

What's the %s for?


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

Mostly looks good. Just a few questions and comments.

CheckGracefulDelete(obj runtime.Object, options *api.DeleteOptions) bool
}

// BeforeGracefulDelete tests whether the object can be gracefully deleted. If graceful is set the object
Copy link
Member

Choose a reason for hiding this comment

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

BeforeDelete tests...

@derekwaynecarr
Copy link
Member

Minor nit in godoc, this LGTM. Would love to get this merged today ;-)

@smarterclayton smarterclayton force-pushed the allow_graceful_delete branch from e9f7c4d to dfa8e7d Compare March 19, 2015 14:49
@smarterclayton
Copy link
Contributor Author

Comments all addressed (requests for forbearance included in some places :)) - ptal.

@derekwaynecarr
Copy link
Member

rebase needed.

@nikhiljindal - do you mind if i take over this PR to review?

This commit adds support to core resources to enable deferred deletion
of resources.  Clients may optionally specify a time period after which
resources must be deleted via an object sent with their DELETE. That
object may define an optional grace period in seconds, or allow the
default "preferred" value for a resource to be used. Once the object
is marked as pending deletion, the deletionTimestamp field will be set
and an etcd TTL will be in place.

Clients should assume resources that have deletionTimestamp set will
be deleted at some point in the future.  Other changes will come later
to enable graceful deletion on a per resource basis.
@nikhiljindal
Copy link
Contributor

Sure, thanks!

@derekwaynecarr
Copy link
Member

Waiting for green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants