Refactor ENI limit struct #1035
Conversation
e8156b6 to
5516566
Compare
5516566 to
09eb00b
Compare
09eb00b to
db01b8c
Compare
8e4a6ae to
0cb3723
Compare
Based on changes in aws/amazon-vpc-cni-k8s#1035
0106208 to
8754f63
Compare
Based on changes in aws/amazon-vpc-cni-k8s#1035
0cefc6a to
be4f230
Compare
be4f230 to
289afa4
Compare
There was a problem hiding this comment.
Code seems fine, assuming we want to do this at all. It seems like it might be better to just call DescribeInstanceTypes at CNI startup?
(Maybe keep manuallyAddedLimits, although that just escalates an arms-race - since then we need env vars to override the hardcoded limits, in which case just have the env vars and remove manuallyAddedLimits.)
Makefile
Outdated
| # Generate eni-max-pods.txt file for EKS AMI | ||
| generate-max-pods: GOOS= | ||
| generate-max-pods: | ||
| GENERATE_MAX_PODS=true go run scripts/gen_vpc_ip_limits.go |
There was a problem hiding this comment.
Too fancy. Just (re)generate both output files always.
There was a problem hiding this comment.
Hmm... The issue here is that the eni-max-pods.txt file actually belongs to the EKS AMI package... I just added the functionality here to generate it, using the same code that already generated the CNI limits. Since these are flags to the Kubelet on startup, it's not as easy to fetch the limits there.
There was a problem hiding this comment.
It's just a loop over the results of a single API call - any benefit isn't worth carrying extra unused code. Either we invoke (and therefore exercise) both code paths all the time, or the code path that this repo doesn't care about should be removed. Every conditional multiplies your entire test matrix - fighting complexity is everybody's responsibility!
I haven't dug deeply enough to understand how this glues into the AMI build process, but afaics it's one of:
- we don't need eni-max-pods.txt from this binary: remove the code branch
- we need to generate eni-max-pods.txt from this binary during AMI build: generate it always, and .gitignore it from this repo (with a comment).
- we need to pull the already-generated eni-max-pods.txt from this repo during AMI build: generate it always and commit it to this repo (with a comment).
There was a problem hiding this comment.
@anguslees We already have a dependency the other way, where the CNI will download the debug script from the EKS AMI repo. I've changed this PR to use int and to generate both files, checking in misc/eni-max-pods.txt.
289afa4 to
028a7a0
Compare
| ipLimit, ok := InstanceIPsAvailable[cache.instanceType] | ||
| // GetENIIPv4Limit return IP address limit per ENI based on EC2 instance type | ||
| func (cache *EC2InstanceMetadataCache) GetENIIPv4Limit() (int, error) { | ||
| eniLimits, ok := InstanceNetworkingLimits[cache.instanceType] | ||
| if !ok { | ||
| log.Errorf("Failed to get ENI IP limit due to unknown instance type %s", cache.instanceType) |
There was a problem hiding this comment.
This code doesn't fallback to DescribeInstanceTypes ?
Do we want to add that in this PR or a followup? (I'm ok with either - this PR doesn't make it worse)
There was a problem hiding this comment.
Well, this is really a bit hacky, but in GetENILimit() we add the missing instance type to the InstanceNetworkingLimits map, so as long as that function is called first, it always works. That said, we should probably move that part out to a separate function. I'll make a follow up PR.
028a7a0 to
c3571e7
Compare
c3571e7 to
0816db0
Compare
Based on changes in aws/amazon-vpc-cni-k8s#1035
Description of changes:
Refactored the two static ENI limit lists to a map from instance type to
InstanceTypeLimits. This is a preparation for adding additional limits. Also added some better logging when generating the file and the option to generate a matchingeni-max-pods.txtfile for the EKS AMI repo by usingmake generate-max-pods.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.