-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Added evictionHard
, kubeReserved
and podsPerCore
to Provisioner
#2444
feat: Added evictionHard
, kubeReserved
and podsPerCore
to Provisioner
#2444
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
if e.KubeletConfig != nil && e.KubeletConfig.MaxPods != nil { | ||
userData.WriteString(" \\\n--use-max-pods false") | ||
kubeletExtraArgs += fmt.Sprintf(" --max-pods=%d", ptr.Int32Value(e.KubeletConfig.MaxPods)) | ||
} else if !e.AWSENILimitedPodDensity { | ||
userData.WriteString(" \\\n--use-max-pods false") | ||
kubeletExtraArgs += " --max-pods=110" | ||
} | ||
if e.KubeletConfig != nil && e.KubeletConfig.PodsPerCore != nil { | ||
kubeletExtraArgs += fmt.Sprintf(" --pods-per-core=%d", ptr.Int32Value(e.KubeletConfig.PodsPerCore)) |
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.
@bwagner5 Wasn't there some weird issue earlier this year with the eks bootstrap script and setting values using an =
sign?
E.g.
--pods-per-core 50
vs --pods-per-core=50
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.
Found the old issue, #1520 , it's only the bootstrap args that it causes problems with.
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.
^^ yep, kubelet args will work fine w/ =
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
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.
nice! A few comments but mainly one issue in the podsPerCore calc.
Does it make sense to support soft eviction thresholds as well? |
It probably does, I figured we could decouple these for now, but we should probably follow-up this PR with support for |
6a2077b
to
0d9cf51
Compare
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
2b5e0d9
to
6766f31
Compare
6766f31
to
cfe75cb
Compare
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 #2129
Description
evictionHard
,kubeReserved
andpodsPerCore
in Provisioner specHow was this change tested?
make test
TEST_FILTER="integration" make e2etests
Does this change impact docs?
Release Note
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.