Merge pull request #1109 from shiv-amz/optimize_ec2_describe_instances

Optimize node controller describe instances calls
kubernetes · Feb 25, 2025 · c583035 · c583035
2 parents 24f674d + acdfc87
commit c583035
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 18 deletions.
diff --git a/pkg/providers/v1/aws.go b/pkg/providers/v1/aws.go
@@ -857,6 +857,10 @@ func (c *Cloud) NodeAddressesByProviderID(ctx context.Context, providerID string
 		return nil, err
 	}
 
+	return c.getInstanceNodeAddress(instance)
+}
+
+func (c *Cloud) getInstanceNodeAddress(instance *ec2.Instance) ([]v1.NodeAddress, error) {
 	var addresses []v1.NodeAddress
 
 	for _, family := range c.cfg.Global.NodeIPFamilies {
@@ -993,8 +997,11 @@ func (c *Cloud) InstanceTypeByProviderID(ctx context.Context, providerID string)
 	if err != nil {
 		return "", err
 	}
+	return c.getInstanceType(instance), nil
+}
 
-	return aws.StringValue(instance.InstanceType), nil
+func (c *Cloud) getInstanceType(instance *ec2.Instance) string {
+	return aws.StringValue(instance.InstanceType)
 }
 
 // InstanceType returns the type of the node with the specified nodeName.
@@ -1034,13 +1041,14 @@ func (c *Cloud) GetZoneByProviderID(ctx context.Context, providerID string) (clo
 	if err != nil {
 		return cloudprovider.Zone{}, err
 	}
+	return c.getInstanceZone(instance), nil
+}
 
-	zone := cloudprovider.Zone{
+func (c *Cloud) getInstanceZone(instance *ec2.Instance) cloudprovider.Zone {
+	return cloudprovider.Zone{
 		FailureDomain: *(instance.Placement.AvailabilityZone),
 		Region:        c.region,
 	}
-
-	return zone, nil
 }
 
 // GetZoneByNodeName implements Zones.GetZoneByNodeName

diff --git a/pkg/providers/v1/aws_fakes.go b/pkg/providers/v1/aws_fakes.go
@@ -199,6 +199,7 @@ type FakeEC2Impl struct {
 // DescribeInstances returns fake instance descriptions
 func (ec2i *FakeEC2Impl) DescribeInstances(request *ec2.DescribeInstancesInput) ([]*ec2.Instance, error) {
 	matches := []*ec2.Instance{}
+	var matchedInstances []string
 	for _, instance := range ec2i.aws.instances {
 		if request.InstanceIds != nil {
 			if instance.InstanceId == nil {
@@ -230,8 +231,10 @@ func (ec2i *FakeEC2Impl) DescribeInstances(request *ec2.DescribeInstancesInput)
 			}
 		}
 		matches = append(matches, instance)
+		matchedInstances = append(matchedInstances, *instance.InstanceId)
 	}
 
+	ec2i.aws.countCall("ec2", "DescribeInstances", strings.Join(matchedInstances, ","))
 	return matches, nil
 }
 

diff --git a/pkg/providers/v1/instances_v2.go b/pkg/providers/v1/instances_v2.go
@@ -22,6 +22,8 @@ package aws
 
 import (
 	"context"
+	"fmt"
+	"k8s.io/cloud-provider-aws/pkg/providers/v1/variant"
 	"strconv"
 
 	v1 "k8s.io/api/core/v1"
@@ -118,25 +120,43 @@ func (c *Cloud) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloudprov
 	if err != nil {
 		return nil, err
 	}
-
-	instanceType, err := c.InstanceTypeByProviderID(ctx, providerID)
+	instanceID, err := KubernetesInstanceID(providerID).MapToAWSInstanceID()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to map provider ID to AWS instance ID for node %s: %w", node.Name, err)
 	}
 
-	zone, err := c.GetZoneByProviderID(ctx, providerID)
-	if err != nil {
-		return nil, err
-	}
+	var (
+		instanceType  string
+		zone          cloudprovider.Zone
+		nodeAddresses []v1.NodeAddress
+	)
+	if variant.IsVariantNode(string(instanceID)) {
+		instanceType, err = c.InstanceTypeByProviderID(ctx, providerID)
+		if err != nil {
+			return nil, err
+		}
 
-	nodeAddresses, err := c.NodeAddressesByProviderID(ctx, providerID)
-	if err != nil {
-		return nil, err
-	}
+		zone, err = c.GetZoneByProviderID(ctx, providerID)
+		if err != nil {
+			return nil, err
+		}
 
-	instanceID, err := KubernetesInstanceID(providerID).MapToAWSInstanceID()
-	if err != nil {
-		return nil, err
+		nodeAddresses, err = c.NodeAddressesByProviderID(ctx, providerID)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		instance, err := c.getInstanceByID(string(instanceID))
+		if err != nil {
+			return nil, fmt.Errorf("failed to get instance by ID %s: %w", instanceID, err)
+		}
+
+		instanceType = c.getInstanceType(instance)
+		zone = c.getInstanceZone(instance)
+		nodeAddresses, err = c.getInstanceNodeAddress(instance)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get node addresses for instance %s: %w", instanceID, err)
+		}
 	}
 
 	additionalLabels, err := c.getAdditionalLabels(ctx, zone.FailureDomain, string(instanceID), instanceType, zone.Region, node.Labels)

diff --git a/pkg/providers/v1/instances_v2_test.go b/pkg/providers/v1/instances_v2_test.go
@@ -284,6 +284,23 @@ func TestInstanceMetadata(t *testing.T) {
 
 		mockedTopologyManager.AssertNumberOfCalls(t, "GetNodeTopology", 1)
 	})
+
+	t.Run("Should limit ec2:DescribeInstances calls to a single request per instance", func(t *testing.T) {
+		instance := makeInstance("i-00000000000001234", "192.168.0.1", "1.2.3.4", "instance-same.ec2.internal", "instance-same.ec2.external", nil, true)
+		c, awsServices := mockInstancesResp(&instance, []*ec2.Instance{&instance})
+		node := &v1.Node{
+			Spec: v1.NodeSpec{
+				ProviderID: fmt.Sprintf("aws:///us-west-2c/%s", *instance.InstanceId),
+			},
+		}
+		instanceMetadataDescribeInstances := fmt.Sprintf("%s:%s:%s", "ec2", "DescribeInstances", *instance.InstanceId)
+		delete(awsServices.callCounts, instanceMetadataDescribeInstances)
+		_, err := c.InstanceMetadata(context.TODO(), node)
+		if err != nil {
+			t.Errorf("Should not error getting InstanceMetadata: %s", err)
+		}
+		assert.Equal(t, awsServices.callCounts[instanceMetadataDescribeInstances], 1)
+	})
 }
 
 func getCloudWithMockedDescribeInstances(instanceExists bool, instanceState string) *Cloud {