gh comments

aws · Nov 29, 2023 · 5e9a4e9 · 5e9a4e9
1 parent 3d2152d
commit 5e9a4e9
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 40 deletions.
diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go
@@ -308,10 +308,15 @@ func (c *CloudProvider) instanceToNodeClaim(i *instance.Instance, instanceType *
 			}
 		}
 		resourceFilter := func(n v1.ResourceName, v resource.Quantity) bool {
-			if !i.EFAEnabled && n == v1beta1.ResourceEFA {
+			if resources.IsZero(v) {
 				return false
 			}
-			return !resources.IsZero(v)
+			// The nodeclaim should only advertise an EFA resource if it was requested. EFA network interfaces are only
+			// added to the launch template if they're requested, otherwise the instance is launched with a normal ENI.
+			if n == v1beta1.ResourceEFA {
+				return i.EFAEnabled
+			}
+			return true
 		}
 		nodeClaim.Status.Capacity = functional.FilterMap(instanceType.Capacity, resourceFilter)
 		nodeClaim.Status.Allocatable = functional.FilterMap(instanceType.Allocatable(), resourceFilter)

diff --git a/pkg/providers/amifamily/resolver.go b/pkg/providers/amifamily/resolver.go
@@ -131,6 +131,11 @@ func (r Resolver) Resolve(ctx context.Context, nodeClass *v1beta1.EC2NodeClass,
 	}
 	var resolvedTemplates []*LaunchTemplate
 	for amiID, instanceTypes := range mappedAMIs {
+		// In order to support reserved ENIs for CNI custom networking setups,
+		// we need to pass down the max-pods calculation to the kubelet.
+		// This requires that we resolve a unique launch template per max-pods value.
+		// Similarly, instance types configured with EfAs require unique launch templates depending on the number of
+		// EFAs they support.
 		type launchTemplateParams struct {
 			efaCount int
 			maxPods  int
@@ -145,41 +150,10 @@ func (r Resolver) Resolve(ctx context.Context, nodeClass *v1beta1.EC2NodeClass,
 				maxPods: int(instanceType.Capacity.Pods().Value()),
 			}
 		})
-		// In order to support reserved ENIs for CNI custom networking setups,
-		// we need to pass down the max-pods calculation to the kubelet.
-		// This requires that we resolve a unique launch template per max-pods value.
 		for params, instanceTypes := range paramsToInstanceTypes {
-			kubeletConfig := &corev1beta1.KubeletConfiguration{}
-			if nodeClaim.Spec.Kubelet != nil {
-				if err := mergo.Merge(kubeletConfig, nodeClaim.Spec.Kubelet); err != nil {
-					return nil, err
-				}
-			}
-			if kubeletConfig.MaxPods == nil {
-				kubeletConfig.MaxPods = lo.ToPtr(int32(params.maxPods))
-			}
-			resolved := &LaunchTemplate{
-				Options: options,
-				UserData: amiFamily.UserData(
-					r.defaultClusterDNS(options, kubeletConfig),
-					append(nodeClaim.Spec.Taints, nodeClaim.Spec.StartupTaints...),
-					options.Labels,
-					options.CABundle,
-					instanceTypes,
-					nodeClass.Spec.UserData,
-				),
-				BlockDeviceMappings: nodeClass.Spec.BlockDeviceMappings,
-				MetadataOptions:     nodeClass.Spec.MetadataOptions,
-				DetailedMonitoring:  aws.BoolValue(nodeClass.Spec.DetailedMonitoring),
-				AMIID:               amiID,
-				InstanceTypes:       instanceTypes,
-				EFACount:            params.efaCount,
-			}
-			if len(resolved.BlockDeviceMappings) == 0 {
-				resolved.BlockDeviceMappings = amiFamily.DefaultBlockDeviceMappings()
-			}
-			if resolved.MetadataOptions == nil {
-				resolved.MetadataOptions = amiFamily.DefaultMetadataOptions()
+			resolved, err := r.resolveLaunchTemplate(nodeClass, nodeClaim, instanceTypes, amiFamily, amiID, params.maxPods, params.efaCount, options)
+			if err != nil {
+				return nil, err
 			}
 			resolvedTemplates = append(resolvedTemplates, resolved)
 		}
@@ -229,3 +203,40 @@ func (r Resolver) defaultClusterDNS(opts *Options, kubeletConfig *corev1beta1.Ku
 	newKubeletConfig.ClusterDNS = []string{opts.KubeDNSIP.String()}
 	return newKubeletConfig
 }
+
+func (r Resolver) resolveLaunchTemplate(nodeClass *v1beta1.EC2NodeClass, nodeClaim *corev1beta1.NodeClaim, instanceTypes []*cloudprovider.InstanceType,
+	amiFamily AMIFamily, amiID string, maxPods int, efaCount int, options *Options) (*LaunchTemplate, error) {
+	kubeletConfig := &corev1beta1.KubeletConfiguration{}
+	if nodeClaim.Spec.Kubelet != nil {
+		if err := mergo.Merge(kubeletConfig, nodeClaim.Spec.Kubelet); err != nil {
+			return nil, err
+		}
+	}
+	if kubeletConfig.MaxPods == nil {
+		kubeletConfig.MaxPods = lo.ToPtr(int32(maxPods))
+	}
+	resolved := &LaunchTemplate{
+		Options: options,
+		UserData: amiFamily.UserData(
+			r.defaultClusterDNS(options, kubeletConfig),
+			append(nodeClaim.Spec.Taints, nodeClaim.Spec.StartupTaints...),
+			options.Labels,
+			options.CABundle,
+			instanceTypes,
+			nodeClass.Spec.UserData,
+		),
+		BlockDeviceMappings: nodeClass.Spec.BlockDeviceMappings,
+		MetadataOptions:     nodeClass.Spec.MetadataOptions,
+		DetailedMonitoring:  aws.BoolValue(nodeClass.Spec.DetailedMonitoring),
+		AMIID:               amiID,
+		InstanceTypes:       instanceTypes,
+		EFACount:            efaCount,
+	}
+	if len(resolved.BlockDeviceMappings) == 0 {
+		resolved.BlockDeviceMappings = amiFamily.DefaultBlockDeviceMappings()
+	}
+	if resolved.MetadataOptions == nil {
+		resolved.MetadataOptions = amiFamily.DefaultMetadataOptions()
+	}
+	return resolved, nil
+}
diff --git a/pkg/providers/instance/types.go b/pkg/providers/instance/types.go
@@ -55,7 +55,7 @@ func NewInstance(out *ec2.Instance) *Instance {
 		SubnetID: aws.StringValue(out.SubnetId),
 		Tags:     lo.SliceToMap(out.Tags, func(t *ec2.Tag) (string, string) { return aws.StringValue(t.Key), aws.StringValue(t.Value) }),
 		EFAEnabled: lo.ContainsBy(out.NetworkInterfaces, func(ni *ec2.InstanceNetworkInterface) bool {
-			return ni != nil && lo.FromPtr(ni.InterfaceType) == "efa"
+			return ni != nil && lo.FromPtr(ni.InterfaceType) == ec2.NetworkInterfaceTypeEfa
 		}),
 	}
 

diff --git a/pkg/providers/launchtemplate/launchtemplate.go b/pkg/providers/launchtemplate/launchtemplate.go
@@ -293,7 +293,7 @@ func (p *Provider) generateNetworkInterface(options *amifamily.LaunchTemplate) [
 				AssociatePublicIpAddress: lo.Ternary(options.EFACount == 1, options.AssociatePublicIPAddress, nil),
 				NetworkCardIndex:         lo.ToPtr(int64(i)),
 				DeviceIndex:              lo.ToPtr(lo.Ternary[int64](i == 0, 0, 1)),
-				InterfaceType:            lo.ToPtr("efa"),
+				InterfaceType:            lo.ToPtr(ec2.NetworkInterfaceTypeEfa),
 				Groups:                   lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }),
 			}
 		})

diff --git a/test/suites/integration/extended_resources_test.go b/test/suites/integration/extended_resources_test.go
@@ -221,6 +221,9 @@ var _ = Describe("Extended Resources", func() {
 				Effect: v1.TaintEffectNoSchedule,
 			},
 		}
+		// Only select private subnets since instances with multiple network instances at launch won't get a public IP.
+		nodeClass.Spec.SubnetSelectorTerms[0].Tags["Name"] = "*Private*"
+
 		numPods := 1
 		dep := test.Deployment(test.DeploymentOptions{
 			Replicas: int32(numPods),
@@ -236,10 +239,10 @@ var _ = Describe("Extended Resources", func() {
 				},
 				ResourceRequirements: v1.ResourceRequirements{
 					Requests: v1.ResourceList{
-						"vpc.amazonaws.com/efa": resource.MustParse("1"),
+						"vpc.amazonaws.com/efa": resource.MustParse("2"),
 					},
 					Limits: v1.ResourceList{
-						"vpc.amazonaws.com/efa": resource.MustParse("1"),
+						"vpc.amazonaws.com/efa": resource.MustParse("2"),
 					},
 				},
 			},