Skip to content

fix(nodeadm): calculate max pods on default network card#2229

Closed
mselim00 wants to merge 1 commit intoawslabs:mainfrom
mselim00:al2023-max-pods
Closed

fix(nodeadm): calculate max pods on default network card#2229
mselim00 wants to merge 1 commit intoawslabs:mainfrom
mselim00:al2023-max-pods

Conversation

@mselim00
Copy link
Copy Markdown
Contributor

@mselim00 mselim00 commented Apr 24, 2025

Issue #, if available:

Description of changes:

At the moment there's a gap between the max pods value from eni-max-pods.txt and the fallback calculator for multicard instance types.

Both use the equation max # network interfaces * (max # ipv4 addresses per interface -1) + 2 for the max pods value, but the text file is calculated using the max number of network interfaces for the default network card, whereas the fallback uses the total maximum number of network interfaces for the instance. To take an example, consider p4de.24xlarge

$ aws ec2 describe-instance-types --region=us-west-2 --instance-type=p4d.24xlarge | jq '.InstanceTypes[].NetworkInfo'
{
  "NetworkPerformance": "4x 100 Gigabit",
  "MaximumNetworkInterfaces": 60,
  "MaximumNetworkCards": 4,
  "DefaultNetworkCardIndex": 0,
  "NetworkCards": [
    {
      "NetworkCardIndex": 0,
      "NetworkPerformance": "100 Gigabit",
      "MaximumNetworkInterfaces": 15,
      "BaselineBandwidthInGbps": 100,
      "PeakBandwidthInGbps": 100
    },
    {
      "NetworkCardIndex": 1,
      "NetworkPerformance": "100 Gigabit",
      "MaximumNetworkInterfaces": 15,
      "BaselineBandwidthInGbps": 100,
      "PeakBandwidthInGbps": 100
    },
    {
      "NetworkCardIndex": 2,
      "NetworkPerformance": "100 Gigabit",
      "MaximumNetworkInterfaces": 15,
      "BaselineBandwidthInGbps": 100,
      "PeakBandwidthInGbps": 100
    },
    {
      "NetworkCardIndex": 3,
      "NetworkPerformance": "100 Gigabit",
      "MaximumNetworkInterfaces": 15,
      "BaselineBandwidthInGbps": 100,
      "PeakBandwidthInGbps": 100
    }
  ],
  "Ipv4AddressesPerInterface": 50,
  "Ipv6AddressesPerInterface": 50,
  "Ipv6Supported": true,
  "EnaSupport": "required",
  "EfaSupported": true,
  "EfaInfo": {
    "MaximumEfaInterfaces": 4
  },
  "EncryptionInTransitSupported": true,
  "EnaSrdSupported": false
}

eni-max-pods.txt shows 737, which follows from NetworkCards[DefaultNetworkCardIndex].MaximumNetworkInterfaces * (Ipv4AddressesPerInterface - 1) + 2 = 15 * (50 - 1) + 2 = 737. On the other hand, the calculator fallback currently uses MaximumNetworkInterfaces * (Ipv4AddressesPerInterface - 1) + 2 = 60 * (50 - 1) + 2 = 2,942.

The max pods value is mainly intended to account for the fact that the VPC CNI only allocates pods using the first network interface. Since this calculator is only used if no corresponding value was found in the text file for the instance type, the existing math could also lead to a major discrepancy between AMI versions for new instance types.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

Added a unit test for a mock multi-card instance type.

See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.

@mselim00 mselim00 requested a review from cartermckinnon April 24, 2025 04:26
@cartermckinnon
Copy link
Copy Markdown
Contributor

The max pods value is mainly intended to account for the fact that the VPC CNI only allocates pods using the first network interface.

@jaydeokar is working on removing this limitation, so we can't bake this assumption in.

@mselim00
Copy link
Copy Markdown
Contributor Author

mselim00 commented Oct 2, 2025

Absorbed into #2446

The VPC CNI has yet to implement support for allocating on multiple ENIs, and even when it does, the fallback should line up with the default values and allow custom configurations for those using that version + feature of the VPC CNI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants