Skip to content

Optimize ENI slot reservation for non-supported instance type #3250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phuhung273
Copy link

@phuhung273 phuhung273 commented Apr 6, 2025

What type of PR is this?
improvement

Which issue does this PR fix?:
Close #3094

What does this PR do / Why do we need it?:
Do not reserve ENI slot for non-supported instance type

Testing done on this change:
make unit-test

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades? Has updating a running cluster been tested?:
Can the team show me how to test this case ?

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:

Optimize ENI slot reservation for non-supported instance type

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@phuhung273 phuhung273 requested a review from a team as a code owner April 6, 2025 14:10
@phuhung273 phuhung273 changed the title optimize ENI slot reservation for non-supported instance type Optimize ENI slot reservation for non-supported instance type Apr 6, 2025
@phuhung273 phuhung273 marked this pull request as draft April 6, 2025 14:17
@phuhung273 phuhung273 marked this pull request as ready for review April 7, 2025 05:06
@jayanthvn jayanthvn requested a review from Copilot May 27, 2025 01:38
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR prevents ENI slot reservation on instance types that don’t support ENI trunking by introducing a check in the IPAMD logic and wiring it through AWS utils and tests.

  • Add lists of non-supported ENI trunking instance types/families and implement IsENITrunkingSupported
  • Update hasRoomForEni to account for trunking support
  • Extend mocks and unit tests in both ipamd and awsutils packages to cover the new trunking flag

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/ipamd/ipamd.go Added IsENITrunkingSupported check in hasRoomForEni
pkg/ipamd/ipamd_test.go Expanded test cases (testIncreaseIPPool) to include trunking
pkg/awsutils/no_eni_trunking_instance_types.go Defined noENITrunkingInstanceTypes and noENITrunkingInstanceFamilies
pkg/awsutils/awsutils.go Implemented IsENITrunkingSupported using new slices
pkg/awsutils/mocks/awsutils_mocks.go Added mock recorder and implementation for IsENITrunkingSupported
pkg/awsutils/awsutils_test.go Added unit tests for IsENITrunkingSupported
Comments suppressed due to low confidence (3)

pkg/ipamd/ipamd_test.go:514

  • The parameter name 'unschedulabeNode' is misspelled and may confuse readers. Consider renaming it to 'unschedulableNode'.
func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool, subnetDiscovery bool, eniTrunking bool) {

pkg/ipamd/ipamd.go:2236

  • Consider adding unit tests for hasRoomForEni to verify its behavior when ENI trunking is supported versus not supported, ensuring the trunkEni offset is applied correctly.
if c.awsClient.IsENITrunkingSupported() && c.enablePodENI && c.dataStore.GetTrunkENI() == "" {

pkg/ipamd/ipamd_test.go:668

  • [nitpick] This test sets MY_NODE_NAME but doesn’t unset it; environment state may leak between tests. Consider using defer os.Unsetenv("MY_NODE_NAME") or explicitly calling os.Unsetenv in teardown.
func TestIncreasePrefixPoolDefault(t *testing.T) {

@phuhung273 phuhung273 force-pushed the non-supported-eni-trunking-instance-type branch from 92b3109 to d8372ce Compare July 19, 2025 04:04
@phuhung273
Copy link
Author

Hi @jayanthvn @yash97, do you think this PR reasonable to go ahead ?

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.

VPC CNI shouldn't reserve a ENI for trunk ENI if the instance doesn't support ENI trunking
1 participant