-
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
Support graceful deletion of resources #5085
Support graceful deletion of resources #5085
Conversation
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. |
Addresses #1535 |
49a4b1a
to
f1737d2
Compare
assigning WIP PR to author until it becomes ready for review. |
I would love to review my own code :)
|
f1737d2
to
0f77292
Compare
0f77292
to
f8f4afc
Compare
As if I don't have enough PRs to review, assigning to myself for now to vet the approach. |
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:
|
Agree
|
Ok, so GracePeriod is just one number. Does it specify the wind-down period, or the post-wind-down deletion TTL? |
Same right now. As discussed in 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.
|
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? |
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 -----
|
Sounds like a good plan @smarterclayton. I'm looking forward to this PR being complete. |
f8f4afc
to
d84845c
Compare
d84845c
to
5749b63
Compare
@smarterclayton - thanks, this should help me make progress once this merges. let me know if we need help on reviews. |
b68c404
to
e9f7c4d
Compare
Any reviews helpful. |
{ | ||
"kind": "DeleteOptions", | ||
"apiVersion": "v1beta1", | ||
"gracePeriod": 0%s |
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.
gracePeriodSeconds.
What's the %s
for?
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.
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.
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 |
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.
BeforeDelete tests...
Minor nit in godoc, this LGTM. Would love to get this merged today ;-) |
e9f7c4d
to
dfa8e7d
Compare
Comments all addressed (requests for forbearance included in some places :)) - ptal. |
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.
dfa8e7d
to
428d226
Compare
Sure, thanks! |
Waiting for green |
Support graceful deletion of resources
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).
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.