-
Notifications
You must be signed in to change notification settings - Fork 170
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
docs: add RFC on making expiration forceful #1303
Conversation
Hey all, I figure this is a long shot, but as you all were up-votes on aws/karpenter-provider-aws#1884, I wanted to see if any of you had thoughts on this RFC, and know the use-cases for Graceful Expiration. @andrebikeev @agrrh @karma-git @funes79 @FernandoMiguel @duboisf @diranged @creosonic @gbvanrenswoude @universam1 @philslab-ninja @round-corgi @mmukarram @grandich |
Thanks for the ping. |
@@ -0,0 +1,64 @@ | |||
# Forceful Expiration - v0.37+ |
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.
Would calling Graceful "Launch before Terminate" and Forceful as "Terminate and launch" make more sense? Both flow is still graceful they just order the node creation differently. AWS already use these terminology https://docs.aws.amazon.com/autoscaling/ec2/userguide/instance-maintenance-policy-overview-and-considerations.html
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 think there's value in having one-word terms to map to the phrase, but I do agree those are pretty clear. There's another mechanism in between called Eventual that's not called out here that is harder to explain in those terms as well.
|
||
***S4** is a point of no return. The node will be terminated within its TGP.* | ||
|
||
**T4 (S4 → S5):** Once the node has no running drainable pods or the TGP timeout is reached, Karpenter directly terminates the underlying instance, initiating forceful termination and cleanup of daemons and networking applications. |
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.
there seems to be a design issue here.
when using controllers like ebs csi plugin, which work as daemonset, if you drain it before removing the EBS volumes, the volumes wont be detached.
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.
Wouldn't deletion of pods that asked for the volumes kick off the CSI del to remove those volumes?
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 ebs-csi-node
pod tolerates Karpenter's disruption taint and is drained early, this could result in volumes still being attached. This shouldn't happen in this specific case since it has a preStop hook that waits for all volumes to be detached, though this can still be problematic if its terminationGracePeriodSeconds is exceeded before all volumes are detached. This should also apply once the instance is terminating if graceful node termination has been configured (though there's currently an issue since Karpenter removes the Node object before the instance is terminated).
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.
this is a current issue. blocking many Orgs.
kubernetes-sigs/aws-ebs-csi-driver#1955 (comment)
So whatever we design here needs to take in consideration who currently Karpenter works and plan for the future, or I fear we may make the issue worst by assuming certain things
|
||
## 🔑 Adding Forceful Expiration | ||
|
||
### Option 1 - Make Expiration Forceful - Recommended |
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 like option 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.
Overall lgtm. I only have questions for myself
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GnatorX, njtran The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
For those I've pinged, hoping to close out the RFC this week. Please let me know if you have questions. |
/lgtm |
Pull Request Test Coverage Report for Build 9589288784Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Fixes #N/A
Description
This proposes making Expiration forceful, allowing cluster admins to have a guaranteed way to understand the maximum node lifetime of the node.
How was this change tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.