Updated default kubelet reserved memory calculation to match the calculation used by amazon-eks-ami #2443
Conversation
brianpursley
force-pushed
the
new-default-memory-calc
branch
from
e99f523
to
ea846d2
Compare
pkg/nodebootstrap/reserved.go
Outdated
| if i.MaxPodsPerNode > 0 { | ||
| mib = 11*i.MaxPodsPerNode + minimumMemoryToReserve | ||
| } else { | ||
| // As a fallback, if MaxPodsPerNode is 0, use the original calculation, which is a progressive calculation based on total memory. |
There was a problem hiding this comment.
Do we need the fallback? I don't see any types with MaxPodsPerNode: 0
EDIT: I see you addressed this in the description. But I wonder if it's really worth keeping the old code around, as far as I can tell the AMI fails if there's no MaxPods.
There was a problem hiding this comment.
You probably saw this, but in the diff, it is collapsed. Some examples of instance types with no max pods are:
- m5ad.8xlarge
- m5ad.16xlarge
I wasn't really quite sure what to make of this, as these seem to not be available when I created an EC2 instance via the console. They don't exist in the list.
I was hesitant to completely remove the old calc in case those instance types need to be supported.
As a test, I just tried to create a cluster using m5ad.8xlarge, and it failed, so I guess those instance types won't work anyway, so I think you're probably right, the fallback is unnecessary and can be removed.
I'll update the PR to remove it.
There was a problem hiding this comment.
Ah yeah true, I was checking for a missing line MaxPodsPerNode:.
There was a problem hiding this comment.
Ok, those instance types don't exist apparently (anymore?). Now I'm wondering why they show up at all.
There was a problem hiding this comment.
I pushed a new update that removes the fallback and related field which is no longer used.
I also discovered a problem (with this new PR) where there are mixed instance types and it takes the minimum across all types. I had not updated that branch of the code, so it was using the fallback, and the unit test still passed because it was using the fallback calc.
ptal and see what you think now.
brianpursley
force-pushed
the
new-default-memory-calc
branch
from
49b46fb
to
9880f77
Compare
…ased on pod density (11*maxPodsPerNode+255) instead of total memory. See https://github.com/awslabs/amazon-eks-ami/blob/21426e27e3845319dbca92e7df32e5c4b984a1d1/files/bootstrap.sh#L163 This commit introduces a new MaxPodsPerNode property on InstanceTypeInfo and populates it using the data from maxpods.go so that it will be available for calculating the default memory to reserve. It removes the Memory property from InstanceTypeInfo because it is no longer needed. This commit also updates related unit tests. It fixes some unit tests which reference instance types which no longer exist and adds a check to make sure instance types which are expected to exist actually exist, and those which are not expected to exist do not exist.
brianpursley
force-pushed
the
new-default-memory-calc
branch
from
9880f77
to
6bf3afb
Compare
Description
The reserved memory calculation was changed in amazon-eks-ami to be based on pod density
(11*maxPodsPerNode+255) instead of total memory.
See https://github.com/awslabs/amazon-eks-ami/blob/21426e27e3845319dbca92e7df32e5c4b984a1d1/files/bootstrap.sh#L163
This commit introduces a new MaxPodsPerNode property on InstanceTypeInfo and populates it
using the data from maxpods.go so that it will be available for calculating the default memory
to reserve. It removes the Memory property from InstanceTypeInfo because it is no longer needed.
This commit also updates related unit tests. It fixes some unit tests which reference instance types which no
longer exist and adds a check to make sure instance types which are expected to exist actually exist, and those
which are not expected to exist do not exist.
Fixes #2395
Checklist
README.md, or theuserdocsdirectory)area/nodegroup), target version (e.g.version/0.12.0) and kind (e.g.kind/improvement)