Add flexibility in pod density for non-VPC CNI nodes (#1032)

* Add support for kubeletConfiguration.maxPods Signed-off-by: Michael Irwin <mikesir87@gmail.com> * Revert "Add support for kubeletConfiguration.maxPods" This reverts commit 6b52220. * Add aws-eni-limited-pod-density flag and default to true Signed-off-by: Michael Irwin <mikesir87@gmail.com> When set to false, it's currently setting maxPods value to 110, the default for kubelet. Instead of using the defaults, explicitly setting it lets us use that value in calculations for instance sizing, etc. * PR feedback adjustments Signed-off-by: Michael Irwin <mikesir87@gmail.com> * Update pkg/cloudprovider/aws/instancetype.go Co-authored-by: Ellis Tarn <ellistarn@gmail.com> * Completion of PR feedback due to function renaming Signed-off-by: Michael Irwin <mikesir87@gmail.com> * Don't modify global ctx during test execution Signed-off-by: Michael Irwin <mikesir87@gmail.com> * PR feedback Signed-off-by: Michael Irwin <mikesir87@gmail.com> Co-authored-by: Ellis Tarn <ellistarn@gmail.com>
aws · Jan 19, 2022 · f62c089 · f62c089
1 parent 60db550
commit f62c089
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 26 deletions.
diff --git a/pkg/cloudprovider/aws/instancetype.go b/pkg/cloudprovider/aws/instancetype.go
@@ -16,7 +16,6 @@ package aws
 
 import (
 	"fmt"
-
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/vpc"
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -26,6 +25,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"knative.dev/pkg/ptr"
 )
 
 // EC2VMAvailableMemoryFactor assumes the EC2 VM will consume <7.25% of the memory of a given machine
@@ -34,6 +34,7 @@ const EC2VMAvailableMemoryFactor = .925
 type InstanceType struct {
 	ec2.InstanceTypeInfo
 	AvailableOfferings []cloudprovider.Offering
+	MaxPods            *int32
 }
 
 func (i *InstanceType) Name() string {
@@ -70,10 +71,10 @@ func (i *InstanceType) Memory() *resource.Quantity {
 }
 
 func (i *InstanceType) Pods() *resource.Quantity {
-	// The number of pods per node is calculated using the formula:
-	// max number of ENIs * (IPv4 Addresses per ENI -1) + 2
-	// https://github.com/awslabs/amazon-eks-ami/blob/master/files/eni-max-pods.txt#L20
-	return resources.Quantity(fmt.Sprint(*i.NetworkInfo.MaximumNetworkInterfaces*(*i.NetworkInfo.Ipv4AddressesPerInterface-1) + 2))
+	if i.MaxPods != nil {
+		return resources.Quantity(fmt.Sprint(ptr.Int32Value(i.MaxPods)))
+	}
+	return resources.Quantity(fmt.Sprint(i.eniLimitedPods()))
 }
 
 func (i *InstanceType) AWSPodENI() *resource.Quantity {
@@ -120,15 +121,17 @@ func (i *InstanceType) AWSNeurons() *resource.Quantity {
 }
 
 // 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
+// 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 {
 	overhead := v1.ResourceList{
 		v1.ResourceCPU: *resource.NewMilliQuantity(
 			100, // system-reserved
 			resource.DecimalSI),
 		v1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dMi",
 			// kube-reserved
-			((11*i.Pods().Value())+255)+
+			((11*i.eniLimitedPods())+255)+
 				// system-reserved
 				100+
 				// eviction threshold https://github.com/kubernetes/kubernetes/blob/ea0764452222146c47ec826977f49d7001b0ea8c/pkg/kubelet/apis/config/v1beta1/defaults_linux.go#L23
@@ -157,3 +160,10 @@ func (i *InstanceType) Overhead() v1.ResourceList {
 	}
 	return overhead
 }
+
+// The number of pods per node is calculated using the formula:
+// max number of ENIs * (IPv4 Addresses per ENI -1) + 2
+// https://github.com/awslabs/amazon-eks-ami/blob/master/files/eni-max-pods.txt#L20
+func (i *InstanceType) eniLimitedPods() int64 {
+	return *i.NetworkInfo.MaximumNetworkInterfaces*(*i.NetworkInfo.Ipv4AddressesPerInterface-1) + 2
+}
diff --git a/pkg/cloudprovider/aws/instancetypes.go b/pkg/cloudprovider/aws/instancetypes.go
@@ -17,6 +17,8 @@ package aws
 import (
 	"context"
 	"fmt"
+	"github.com/aws/karpenter/pkg/utils/injection"
+	"knative.dev/pkg/ptr"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -85,6 +87,9 @@ func (p *InstanceTypeProvider) Get(ctx context.Context, provider *v1alpha1.AWS)
 			instanceType.AvailableOfferings = offerings
 			result = append(result, instanceType)
 		}
+		if !injection.GetOptions(ctx).AWSENILimitedPodDensity {
+			instanceType.MaxPods = ptr.Int32(110)
+		}
 	}
 	return result, nil
 }

diff --git a/pkg/cloudprovider/aws/launchtemplate.go b/pkg/cloudprovider/aws/launchtemplate.go
@@ -229,7 +229,7 @@ func sortedKeys(m map[string]string) []string {
 }
 
 // getUserData returns the exact same string for equivalent input,
-// even if elements of those inputs are in differing orders,
+// even if elements of those inputs are in differeing orders,
 // guaranteeing it won't cause spurious hash differences.
 func (p *LaunchTemplateProvider) getUserData(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType, additionalLabels map[string]string) (string, error) {
 	var containerRuntimeArg string
@@ -259,6 +259,12 @@ exec > >(tee /var/log/user-data.log|logger -t user-data -s 2>/dev/console) 2>&1
 	nodeTaintsArgs := p.getNodeTaintArgs(constraints)
 	kubeletExtraArgs := strings.Trim(strings.Join([]string{nodeLabelArgs, nodeTaintsArgs.String()}, " "), " ")
 
+	if !injection.GetOptions(ctx).AWSENILimitedPodDensity {
+		userData.WriteString(` \
+    --use-max-pods=false`)
+		kubeletExtraArgs += " --max-pods=110"
+	}
+
 	if len(kubeletExtraArgs) > 0 {
 		userData.WriteString(fmt.Sprintf(` \
     --kubelet-extra-args '%s'`, kubeletExtraArgs))

diff --git a/pkg/cloudprovider/aws/suite_test.go b/pkg/cloudprovider/aws/suite_test.go
@@ -51,6 +51,7 @@ import (
 )
 
 var ctx context.Context
+var opts options.Options
 var env *test.Environment
 var launchTemplateCache *cache.Cache
 var securityGroupCache *cache.Cache
@@ -67,10 +68,11 @@ func TestAPIs(t *testing.T) {
 
 var _ = BeforeSuite(func() {
 	env = test.NewEnvironment(ctx, func(e *test.Environment) {
-		opts := options.Options{
-			ClusterName:           "test-cluster",
-			ClusterEndpoint:       "https://test-cluster",
-			AWSNodeNameConvention: string(options.IPName),
+		opts = options.Options{
+			ClusterName:             "test-cluster",
+			ClusterEndpoint:         "https://test-cluster",
+			AWSNodeNameConvention:   string(options.IPName),
+			AWSENILimitedPodDensity: true,
 		}
 		Expect(opts.Validate()).To(Succeed(), "Failed to validate options")
 		ctx = injection.WithOptions(ctx, opts)
@@ -471,15 +473,38 @@ var _ = Describe("Allocation", func() {
 				))
 			})
 		})
-		Context("Kubelet Args", func() {
-			It("should specify the --dns-cluster-ip flag when clusterDNSIP is set", func() {
-				provisioner.Spec.KubeletConfiguration.ClusterDNS = []string{"10.0.10.100"}
-				pod := ExpectProvisioned(ctx, env.Client, selectionController, provisioners, ProvisionerWithProvider(provisioner, provider), test.UnschedulablePod())[0]
-				ExpectScheduled(ctx, env.Client, pod)
+		Context("User Data", func() {
+			It("should not specify --use-max-pods=false when using ENI-based pod density", func() {
+				opts.AWSENILimitedPodDensity = true
+				localCtx := injection.WithOptions(ctx, opts)
+				pod := ExpectProvisioned(localCtx, env.Client, selectionController, provisioners, ProvisionerWithProvider(provisioner, provider), test.UnschedulablePod())[0]
+				ExpectScheduled(localCtx, env.Client, pod)
+				Expect(fakeEC2API.CalledWithCreateLaunchTemplateInput.Cardinality()).To(Equal(1))
+				input := fakeEC2API.CalledWithCreateLaunchTemplateInput.Pop().(*ec2.CreateLaunchTemplateInput)
+				userData, _ := base64.StdEncoding.DecodeString(*input.LaunchTemplateData.UserData)
+				Expect(string(userData)).NotTo(ContainSubstring("--use-max-pods=false"))
+			})
+			It("should specify --use-max-pods=false when not using ENI-based pod density", func() {
+				opts.AWSENILimitedPodDensity = false
+				localCtx := injection.WithOptions(ctx, opts)
+				pod := ExpectProvisioned(localCtx, env.Client, selectionController, provisioners, ProvisionerWithProvider(provisioner, provider), test.UnschedulablePod())[0]
+				ExpectScheduled(localCtx, env.Client, pod)
 				Expect(fakeEC2API.CalledWithCreateLaunchTemplateInput.Cardinality()).To(Equal(1))
 				input := fakeEC2API.CalledWithCreateLaunchTemplateInput.Pop().(*ec2.CreateLaunchTemplateInput)
 				userData, _ := base64.StdEncoding.DecodeString(*input.LaunchTemplateData.UserData)
-				Expect(string(userData)).To(ContainSubstring("--dns-cluster-ip '10.0.10.100'"))
+				Expect(string(userData)).To(ContainSubstring("--use-max-pods=false"))
+				Expect(string(userData)).To(ContainSubstring("--max-pods=110"))
+			})
+			Context("Kubelet Args", func() {
+				It("should specify the --dns-cluster-ip flag when clusterDNSIP is set", func() {
+					provisioner.Spec.KubeletConfiguration.ClusterDNS = []string{"10.0.10.100"}
+					pod := ExpectProvisioned(ctx, env.Client, selectionController, provisioners, ProvisionerWithProvider(provisioner, provider), test.UnschedulablePod())[0]
+					ExpectScheduled(ctx, env.Client, pod)
+					Expect(fakeEC2API.CalledWithCreateLaunchTemplateInput.Cardinality()).To(Equal(1))
+					input := fakeEC2API.CalledWithCreateLaunchTemplateInput.Pop().(*ec2.CreateLaunchTemplateInput)
+					userData, _ := base64.StdEncoding.DecodeString(*input.LaunchTemplateData.UserData)
+					Expect(string(userData)).To(ContainSubstring("--dns-cluster-ip '10.0.10.100'"))
+				})
 			})
 		})
 		Context("Metadata Options", func() {

diff --git a/pkg/utils/env/env.go b/pkg/utils/env/env.go
@@ -42,3 +42,17 @@ func WithDefaultString(key string, def string) string {
 	}
 	return val
 }
+
+// WithDefaultBool returns the boolean value of the supplied environment variable or, if not present,
+// the supplied default value.
+func WithDefaultBool(key string, def bool) bool {
+	val, ok := os.LookupEnv(key)
+	if !ok {
+		return def
+	}
+	parsedVal, err := strconv.ParseBool(val)
+	if err != nil {
+		return def
+	}
+	return parsedVal
+}
diff --git a/pkg/utils/options/options.go b/pkg/utils/options/options.go
@@ -40,6 +40,7 @@ func MustParse() Options {
 	flag.IntVar(&opts.KubeClientQPS, "kube-client-qps", env.WithDefaultInt("KUBE_CLIENT_QPS", 200), "The smoothed rate of qps to kube-apiserver")
 	flag.IntVar(&opts.KubeClientBurst, "kube-client-burst", env.WithDefaultInt("KUBE_CLIENT_BURST", 300), "The maximum allowed burst of queries to the kube-apiserver")
 	flag.StringVar(&opts.AWSNodeNameConvention, "aws-node-name-convention", env.WithDefaultString("AWS_NODE_NAME_CONVENTION", string(IPName)), "The node naming convention used by the AWS cloud provider. DEPRECATION WARNING: this field may be deprecated at any time")
+	flag.BoolVar(&opts.AWSENILimitedPodDensity, "aws-eni-limited-pod-density", env.WithDefaultBool("AWS_ENI_LIMITED_POD_DENSITY", true), "Indicates whether new nodes should use ENI-based pod density")
 	flag.Parse()
 	if err := opts.Validate(); err != nil {
 		panic(err)
@@ -49,14 +50,15 @@ func MustParse() Options {
 
 // Options for running this binary
 type Options struct {
-	ClusterName           string
-	ClusterEndpoint       string
-	MetricsPort           int
-	HealthProbePort       int
-	WebhookPort           int
-	KubeClientQPS         int
-	KubeClientBurst       int
-	AWSNodeNameConvention string
+	ClusterName             string
+	ClusterEndpoint         string
+	MetricsPort             int
+	HealthProbePort         int
+	WebhookPort             int
+	KubeClientQPS           int
+	KubeClientBurst         int
+	AWSNodeNameConvention   string
+	AWSENILimitedPodDensity bool
 }
 
 func (o Options) Validate() (err error) {