fix: consider pod density in memory overhead calc (aws#1960)

* fix: consider pod density in memory overhead calc

* fix: control memory overhead calc by AMI family

AL2 behaves differently than Bottlerocket, so we need to account for the
difference in the instance type calculation.

* test: test pod memory overhead calculation

* test: simplify tests

* test: use more realistic EC2 numbers in fake API

pkg/cloudprovider/aws/amifamily/al2.go

@@ -93,3 +93,7 @@ func (a AL2) EphemeralBlockDevice() *string {
 func (a AL2) EphemeralBlockDeviceOverhead() resource.Quantity {
 	return resource.MustParse("5Gi")
 }
+
+func (a AL2) ENILimitedMemoryOverhead() bool {
+	return true
+}

pkg/cloudprovider/aws/amifamily/bottlerocket.go

@@ -85,3 +85,7 @@ func (b Bottlerocket) EphemeralBlockDevice() *string {
 func (b Bottlerocket) EphemeralBlockDeviceOverhead() resource.Quantity {
 	return resource.MustParse("5Gi")
 }
+
+func (b Bottlerocket) ENILimitedMemoryOverhead() bool {
+	return false
+}

pkg/cloudprovider/aws/amifamily/resolver.go

@@ -76,6 +76,7 @@ type AMIFamily interface {
 	DefaultMetadataOptions() *v1alpha1.MetadataOptions
 	EphemeralBlockDevice() *string
 	EphemeralBlockDeviceOverhead() resource.Quantity
+	ENILimitedMemoryOverhead() bool
 }
 
 // New constructs a new launch template Resolver

pkg/cloudprovider/aws/amifamily/ubuntu.go

@@ -67,3 +67,7 @@ func (u Ubuntu) EphemeralBlockDevice() *string {
 func (u Ubuntu) EphemeralBlockDeviceOverhead() resource.Quantity {
 	return resource.MustParse("5Gi")
 }
+
+func (u Ubuntu) ENILimitedMemoryOverhead() bool {
+	return true
+}

pkg/cloudprovider/aws/fake/ec2api.go

@@ -404,7 +404,7 @@ func (e *EC2API) DescribeInstanceTypesPagesWithContext(_ context.Context, _ *ec2
 				},
 				NetworkInfo: &ec2.NetworkInfo{
 					MaximumNetworkInterfaces:  aws.Int64(4),
-					Ipv4AddressesPerInterface: aws.Int64(60),
+					Ipv4AddressesPerInterface: aws.Int64(15),
 				},
 			},
 			{

pkg/cloudprovider/aws/instancetype.go

@@ -86,8 +86,6 @@ func (i *InstanceType) Resources() v1.ResourceList {
 
 // Overhead computes overhead for https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/#node-allocatable
 // using calculations copied from https://github.com/bottlerocket-os/bottlerocket#kubernetes-settings.
-// While this doesn't calculate the correct overhead for non-ENI-limited nodes, we're using this approach until further
-// analysis can be performed
 func (i *InstanceType) Overhead() v1.ResourceList {
 	return i.overhead
 }
@@ -233,6 +231,13 @@ func (i *InstanceType) awsNeurons() *resource.Quantity {
 
 func (i *InstanceType) computeOverhead(vmMemOverhead float64) v1.ResourceList {
 	memory := i.memory()
+	pods := i.pods()
+	amiFamily := amifamily.GetAMIFamily(i.provider.AMIFamily, &amifamily.Options{})
+	memoryOverheadPods := pods.Value()
+	if amiFamily.ENILimitedMemoryOverhead() {
+		memoryOverheadPods = i.eniLimitedPods()
+	}
+
 	overhead := v1.ResourceList{
 		v1.ResourceCPU: *resource.NewMilliQuantity(
 			100, // system-reserved
@@ -241,13 +246,13 @@ func (i *InstanceType) computeOverhead(vmMemOverhead float64) v1.ResourceList {
 			// vm-overhead
 			(int64(math.Ceil(float64(memory.Value())*vmMemOverhead/1024/1024)))+
 				// kube-reserved
-				((11*i.eniLimitedPods())+255)+
+				((11*memoryOverheadPods)+255)+
 				// system-reserved
 				100+
 				// eviction threshold https://github.com/kubernetes/kubernetes/blob/ea0764452222146c47ec826977f49d7001b0ea8c/pkg/kubelet/apis/config/v1beta1/defaults_linux.go#L23
 				100,
 		)),
-		v1.ResourceEphemeralStorage: amifamily.GetAMIFamily(i.provider.AMIFamily, &amifamily.Options{}).EphemeralBlockDeviceOverhead(),
+		v1.ResourceEphemeralStorage: amiFamily.EphemeralBlockDeviceOverhead(),
 	}
 	// kube-reserved Computed from
 	// https://github.com/bottlerocket-os/bottlerocket/pull/1388/files#diff-bba9e4e3e46203be2b12f22e0d654ebd270f0b478dd34f40c31d7aa695620f2fR611

pkg/cloudprovider/aws/suite_test.go

@@ -395,6 +395,50 @@ var _ = Describe("Allocation", func() {
 					Expect(resources.Pods().Value()).ToNot(BeNumerically("==", 110))
 				}
 			})
+			Context("AL2", func() {
+				It("should calculate memory overhead based on eni limited pods when ENI limited", func() {
+					opts.AWSENILimitedPodDensity = true
+					opts.VMMemoryOverhead = 0 // cutting a factor out of the equation
+					provider.AMIFamily = &v1alpha1.AMIFamilyAL2
+					instanceInfo, err := instanceTypeProvider.getInstanceTypes(ctx, provider)
+					Expect(err).To(BeNil())
+					it := NewInstanceType(injection.WithOptions(ctx, opts), instanceInfo["m5.xlarge"], 0, provider, nil)
+					overhead := it.Overhead()
+					Expect(overhead.Memory().String()).To(Equal("1093Mi"))
+				})
+				It("should calculate memory overhead based on eni limited pods when not ENI limited", func() {
+					opts.AWSENILimitedPodDensity = false
+					opts.VMMemoryOverhead = 0 // cutting a factor out of the equation
+					provider.AMIFamily = &v1alpha1.AMIFamilyAL2
+					instanceInfo, err := instanceTypeProvider.getInstanceTypes(ctx, provider)
+					Expect(err).To(BeNil())
+					it := NewInstanceType(injection.WithOptions(ctx, opts), instanceInfo["m5.xlarge"], 0, provider, nil)
+					overhead := it.Overhead()
+					Expect(overhead.Memory().String()).To(Equal("1093Mi"))
+				})
+			})
+			Context("Bottlerocket", func() {
+				It("should calculate memory overhead based on eni limited pods when ENI limited", func() {
+					opts.AWSENILimitedPodDensity = true
+					opts.VMMemoryOverhead = 0 // cutting a factor out of the equation
+					provider.AMIFamily = &v1alpha1.AMIFamilyBottlerocket
+					instanceInfo, err := instanceTypeProvider.getInstanceTypes(ctx, provider)
+					Expect(err).To(BeNil())
+					it := NewInstanceType(injection.WithOptions(ctx, opts), instanceInfo["m5.xlarge"], 0, provider, nil)
+					overhead := it.Overhead()
+					Expect(overhead.Memory().String()).To(Equal("1093Mi"))
+				})
+				It("should calculate memory overhead based on max pods when not ENI limited", func() {
+					opts.AWSENILimitedPodDensity = false
+					opts.VMMemoryOverhead = 0 // cutting a factor out of the equation
+					provider.AMIFamily = &v1alpha1.AMIFamilyBottlerocket
+					instanceInfo, err := instanceTypeProvider.getInstanceTypes(ctx, provider)
+					Expect(err).To(BeNil())
+					it := NewInstanceType(injection.WithOptions(ctx, opts), instanceInfo["m5.xlarge"], 0, provider, nil)
+					overhead := it.Overhead()
+					Expect(overhead.Memory().String()).To(Equal("1665Mi"))
+				})
+			})
 		})
 		Context("Insufficient Capacity Error Cache", func() {
 			It("should launch instances of different type on second reconciliation attempt with Insufficient Capacity Error Cache fallback", func() {