feat(nodeadm): support custom max pods expressions#2446
feat(nodeadm): support custom max pods expressions#2446mselim00 merged 3 commits intoawslabs:mainfrom
Conversation
3e18a56 to
074c0bd
Compare
|
/ci |
2ce2935 to
0c89741
Compare
|
/ci |
7e3bb0b to
66dfb8a
Compare
| #!/usr/bin/env bash | ||
| set -o errexit | ||
| cd amazon-eks-ami/ | ||
| make generate-instance-info |
There was a problem hiding this comment.
im glad we're owning this ourselves now, can you also remove nodeadm/internal/kubelet/eni-max-pods.txt and remove that cp from the workflow flow?
There was a problem hiding this comment.
I was planning to keep the VPC CNI generated one for this PR to utilize as sanity check in a test (computed value from instanceInfo = old val) and then clean it up in a follow up. if you're convinced it's consistent then I can do that cleanup now 😅
There was a problem hiding this comment.
i did a quick check, transforming the jsonl to txt with this to do a diff
jq -r '"\(.instanceType) \(.defaultMaxENIs * (.ipv4AddressesPerInterface - 1) + 2)"'only to realize the diff i got was just the manually added entries from https://github.com/aws/amazon-vpc-cni-k8s/blob/a49107121e4c0337372dbe18d65f15d761973ebf/scripts/gen_vpc_ip_limits.go#L202-L374
There was a problem hiding this comment.
good catch, I added in those overrides to ensure consistent behavior. also temporarily added a test to do the same and removed it, along with eni-max-pods.txt in 2311f7e
8923128 to
f896e80
Compare
fletcherw
left a comment
There was a problem hiding this comment.
looks good from my perspective, up to you if you want to get Nick's approval as well.
|
hopefully the CI's not still flaky /ci |
568f76d to
c8d576d
Compare
Issue #, if available:
Fixes #2201
Resolves #2276
Closes #782
Closes #953
Fixes #1698
Closes #2045
Description of changes:
Adds a new kubelet configuration to the
NodeConfigto allow insertion of aMaxPodsExpressionwhich is evaluated using cel-go to an int32 for use in the final kubelet configuration. The behavior is largely unchanged if an expression is not set, including the lack of the use of the cel evaluator.There are a few internal changes to how the
maxPodsvalue is calculated, but these are tested in all the relevant ways to ensure they do not become external. The most obvious of these changes is that the pre-computed value is split up into its constituent parts (# ENIs and # IPs) to be extracted from a JSON lines file and computed at runtime.There's one included change in the default behavior, which is to calculate the standard max pods value on the default network card rather than for all network cards on the instance. This only impacts users using an instance type not in the pre-existing
eni_max_pods.txtfile or the newinstance-info.jsonlas this calculation is only done as a fallback. This change is a fix to prior inconsistent behavior to better line up with the values in the file cache. Some more details in #2229, which is superseded by this PR.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Testing Done
See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.