-
Notifications
You must be signed in to change notification settings - Fork 983
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
Check DoNotEvict after filtering evictable pods to ensure termination can complete. #1294
Conversation
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 7d00e1e 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6203106ef4a6030008a3d54e |
pod := test.Pod(test.PodOptions{ | ||
NodeName: node.Name, | ||
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{v1alpha5.DoNotEvictPodAnnotationKey: "true"}}, | ||
}) |
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.
Consider testing static pods and a pod that tolerates NoSchedule
, should just be two more pods creation lines, and adding to the respective lines below.
Co-authored-by: Nick Tran <[email protected]>
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.
Thanks for the fix! Looks good!
1. Issue, if available:
#1166
2. Description of changes:
Previously, the
do-not-evict
annotation could block termination logic, even if the node was unreachable. This change modifies thedo-not-evict
logic to only apply to evictable pods. Currently, evictable pods excludeThis means that the
do-not-evict
annotation will no longer work for pods in the above categories. Given that we already did not attempt to evict these pods, the semantic holds. However, previously, pods in these categories with thedo-not-evict
annotation could block node termination.3. How was this change tested?
Reproduced
Fixed
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.