-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: use only evictionHard for allocatable capacity calculation #8565
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
fix: use only evictionHard for allocatable capacity calculation #8565
Conversation
✅ Deploy Preview for karpenter-docs-prod ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
grosser
left a comment
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 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 PR, lets see if CI passes
|
Preview deployment ready! Preview URL: https://pr-8565.d18coufmbnnaag.amplifyapp.com Built from commit |
|
@DerekFrank |
DerekFrank
left a comment
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.
/karpenter snapshot
|
Snapshot successfully published to |
|
@moko-poi Looks like there are still CI issues. You should be able to run them locally: https://github.com/aws/karpenter-provider-aws/blob/main/Makefile#L52 |
|
@DerekFrank Thanks for the reminder. I've already run the tests locally and pushed the fixes. The CI should pass now. Please let me know if there are any remaining issues. |
Pull Request Test Coverage Report for Build 20976572512Details
💛 - Coveralls |
|
@moko-poi Could you rebase so we can get this merged? |
0fcc0e5 to
06e5403
Compare
|
@DerekFrank Rebased! Ready for review. |
|
@DerekFrank @grosser @sumukha-radhakrishna |
06e5403 to
07e8b64
Compare
jigisha620
left a comment
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.
LGTM 🚀
Fixes #8407
Description
This PR fixes the incorrect eviction threshold calculation in Karpenter's allocatable memory computation. Previously, Karpenter used
max(evictionSoft, evictionHard)to determine how much memory to reserve for eviction thresholds, but this is inconsistent with kubelet behavior.According to the Kubernetes documentation, only
evictionHardthresholds should impact allocatable capacity, whileevictionSoftthresholds are warning-only and should not reserve memory.Changes:
evictionThreshold()function inpkg/providers/instancetype/types.goto use onlyevictionHardvaluesevictionHardvalues instead of the maximum of both thresholdsExample:
evictionSoft: 1Gi, evictionHard: 500Mi→ reserves 1Gi (incorrect)evictionSoft: 1Gi, evictionHard: 500Mi→ reserves 500Mi (matches kubelet)How was this change tested?
max(evictionSoft, evictionHard)logicevictionHardvalues are used for allocatable capacity calculationDoes 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.