Skip to content

Commit 7619bdb

Browse files
jchen6585Joseph Chen
jchen6585
and
Joseph Chen
authored
Subnet Discovery - Unfilled ENI fix (#2954)
Co-authored-by: Joseph Chen <[email protected]>
1 parent 0e3d4b1 commit 7619bdb

3 files changed

Lines changed: 91 additions & 52 deletions

File tree

‎pkg/ipamd/datastore/data_store.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -973,8 +973,9 @@ func (e *ENI) hasPods() bool {
973973
return e.AssignedIPv4Addresses() != 0
974974
}
975975

976-
// GetENINeedsIP finds an ENI in the datastore that needs more IP addresses allocated
977-
func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENI {
976+
// GetAllocatableENIs finds ENIs in the datastore that needs more IP addresses allocated
977+
func (ds *DataStore) GetAllocatableENIs(maxIPperENI int, skipPrimary bool) []*ENI {
978+
var enis []*ENI
978979
ds.lock.Lock()
979980
defer ds.lock.Unlock()
980981
for _, eni := range ds.eniPool {
@@ -985,10 +986,10 @@ func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENI {
985986
if len(eni.AvailableIPv4Cidrs) < maxIPperENI {
986987
ds.log.Debugf("Found ENI %s that has less than the maximum number of IP/Prefixes addresses allocated: cur=%d, max=%d",
987988
eni.ID, len(eni.AvailableIPv4Cidrs), maxIPperENI)
988-
return eni
989+
enis = append(enis, eni)
989990
}
990991
}
991-
return nil
992+
return enis
992993
}
993994

994995
// RemoveUnusedENIFromStore removes a deletable ENI from the data store.

‎pkg/ipamd/ipamd.go‎

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -915,6 +915,7 @@ func (c *IPAMContext) tryAssignCidrs() (increasedPool bool, err error) {
915915
// For an ENI, try to fill in missing IPs on an existing ENI.
916916
// PRECONDITION: isDatastorePoolTooLow returned true
917917
func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) {
918+
918919
// If WARM_IP_TARGET is set, only proceed if we are short of target
919920
short, _, warmIPTargetsDefined := c.datastoreTargetState(nil)
920921
if warmIPTargetsDefined && short == 0 {
@@ -928,45 +929,50 @@ func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) {
928929
}
929930

930931
// Find an ENI where we can add more IPs
931-
eni := c.dataStore.GetENINeedsIP(c.maxIPsPerENI, c.useCustomNetworking)
932-
if eni != nil && len(eni.AvailableIPv4Cidrs) < c.maxIPsPerENI {
933-
currentNumberOfAllocatedIPs := len(eni.AvailableIPv4Cidrs)
934-
// Try to allocate all available IPs for this ENI
935-
resourcesToAllocate := min((c.maxIPsPerENI - currentNumberOfAllocatedIPs), toAllocate)
936-
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
937-
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
938-
log.Warnf("failed to allocate all available IP addresses on ENI %s, err: %v", eni.ID, err)
939-
// Try to just get one more IP
940-
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
932+
enis := c.dataStore.GetAllocatableENIs(c.maxIPsPerENI, c.useCustomNetworking)
933+
for _, eni := range enis {
934+
if len(eni.AvailableIPv4Cidrs) < c.maxIPsPerENI {
935+
currentNumberOfAllocatedIPs := len(eni.AvailableIPv4Cidrs)
936+
// Try to allocate all available IPs for this ENI
937+
resourcesToAllocate := min((c.maxIPsPerENI - currentNumberOfAllocatedIPs), toAllocate)
938+
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
941939
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
942-
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
943-
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IP addresses on ENI %s, err ", eni.ID))
940+
log.Warnf("failed to allocate all available IP addresses on ENI %s, err: %v", eni.ID, err)
941+
// Try to just get one more IP
942+
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
943+
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
944+
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
945+
if c.useSubnetDiscovery && containsInsufficientCIDRsOrSubnetIPs(err) {
946+
continue
947+
}
948+
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IP addresses on ENI %s, err ", eni.ID))
949+
}
944950
}
945-
}
946951

947-
var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
948-
if containsPrivateIPAddressLimitExceededError(err) {
949-
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +
950-
"Returning without an error here since we will verify the actual state by calling EC2 to see what addresses have already assigned to this ENI.")
951-
// This call to EC2 is needed to verify which IPs got attached to this ENI.
952-
ec2ip4s, err = c.awsClient.GetIPv4sFromEC2(eni.ID)
953-
if err != nil {
954-
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
955-
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
956-
}
957-
} else {
958-
if output == nil {
959-
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
960-
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
961-
}
952+
var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
953+
if containsPrivateIPAddressLimitExceededError(err) {
954+
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +
955+
"Returning without an error here since we will verify the actual state by calling EC2 to see what addresses have already assigned to this ENI.")
956+
// This call to EC2 is needed to verify which IPs got attached to this ENI.
957+
ec2ip4s, err = c.awsClient.GetIPv4sFromEC2(eni.ID)
958+
if err != nil {
959+
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
960+
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
961+
}
962+
} else {
963+
if output == nil {
964+
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
965+
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
966+
}
962967

963-
ec2Addrs := output.AssignedPrivateIpAddresses
964-
for _, ec2Addr := range ec2Addrs {
965-
ec2ip4s = append(ec2ip4s, &ec2.NetworkInterfacePrivateIpAddress{PrivateIpAddress: aws.String(aws.StringValue(ec2Addr.PrivateIpAddress))})
968+
ec2Addrs := output.AssignedPrivateIpAddresses
969+
for _, ec2Addr := range ec2Addrs {
970+
ec2ip4s = append(ec2ip4s, &ec2.NetworkInterfacePrivateIpAddress{PrivateIpAddress: aws.String(aws.StringValue(ec2Addr.PrivateIpAddress))})
971+
}
966972
}
973+
c.addENIsecondaryIPsToDataStore(ec2ip4s, eni.ID)
974+
return true, nil
967975
}
968-
c.addENIsecondaryIPsToDataStore(ec2ip4s, eni.ID)
969-
return true, nil
970976
}
971977
return false, nil
972978
}
@@ -1015,8 +1021,8 @@ func (c *IPAMContext) tryAssignPrefixes() (increasedPool bool, err error) {
10151021
toAllocate := c.getPrefixesNeeded()
10161022
// Returns an ENI which has space for more prefixes to be attached, but this
10171023
// ENI might not suffice the WARM_IP_TARGET/WARM_PREFIX_TARGET
1018-
eni := c.dataStore.GetENINeedsIP(c.maxPrefixesPerENI, c.useCustomNetworking)
1019-
if eni != nil {
1024+
enis := c.dataStore.GetAllocatableENIs(c.maxPrefixesPerENI, c.useCustomNetworking)
1025+
for _, eni := range enis {
10201026
currentNumberOfAllocatedPrefixes := len(eni.AvailableIPv4Cidrs)
10211027
resourcesToAllocate := min((c.maxPrefixesPerENI - currentNumberOfAllocatedPrefixes), toAllocate)
10221028
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
@@ -1026,9 +1032,13 @@ func (c *IPAMContext) tryAssignPrefixes() (increasedPool bool, err error) {
10261032
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
10271033
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
10281034
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
1035+
if c.useSubnetDiscovery && containsInsufficientCIDRsOrSubnetIPs(err) {
1036+
continue
1037+
}
10291038
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IPv4 prefix on ENI %s, err: %v", eni.ID, err))
10301039
}
10311040
}
1041+
10321042
var ec2Prefixes []*ec2.Ipv4PrefixSpecification
10331043
if containsPrivateIPAddressLimitExceededError(err) {
10341044
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +

‎pkg/ipamd/ipamd_test.go‎

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"time"
2626

2727
"github.com/aws/aws-sdk-go/aws"
28+
"github.com/aws/aws-sdk-go/aws/awserr"
2829
"github.com/aws/aws-sdk-go/service/ec2"
2930
"github.com/golang/mock/gomock"
3031
"github.com/samber/lo"
@@ -468,32 +469,37 @@ func getDummyENIMetadataWithV6Prefix() awsutils.ENIMetadata {
468469

469470
func TestIncreaseIPPoolDefault(t *testing.T) {
470471
_ = os.Unsetenv(envCustomNetworkCfg)
471-
testIncreaseIPPool(t, false, false)
472+
testIncreaseIPPool(t, false, false, false)
473+
}
474+
475+
func TestIncreaseIPPoolSubnetDiscoveryUnfilledENI(t *testing.T) {
476+
_ = os.Unsetenv(envCustomNetworkCfg)
477+
testIncreaseIPPool(t, false, false, true)
472478
}
473479

474480
func TestIncreaseIPPoolCustomENI(t *testing.T) {
475481
_ = os.Setenv(envCustomNetworkCfg, "true")
476482
_ = os.Setenv("MY_NODE_NAME", myNodeName)
477-
testIncreaseIPPool(t, true, false)
483+
testIncreaseIPPool(t, true, false, false)
478484
}
479485

480486
// Testing that the ENI will be allocated on non schedulable node when the AWS_MANAGE_ENIS_NON_SCHEDULABLE is set to `true`
481487
func TestIncreaseIPPoolCustomENIOnNonSchedulableNode(t *testing.T) {
482488
_ = os.Setenv(envCustomNetworkCfg, "true")
483489
_ = os.Setenv(envManageENIsNonSchedulable, "true")
484490
_ = os.Setenv("MY_NODE_NAME", myNodeName)
485-
testIncreaseIPPool(t, true, true)
491+
testIncreaseIPPool(t, true, true, false)
486492
}
487493

488494
// Testing that the ENI will NOT be allocated on non schedulable node when the AWS_MANAGE_ENIS_NON_SCHEDULABLE is not set
489495
func TestIncreaseIPPoolCustomENIOnNonSchedulableNodeDefault(t *testing.T) {
490496
_ = os.Unsetenv(envManageENIsNonSchedulable)
491497
_ = os.Setenv(envCustomNetworkCfg, "true")
492498
_ = os.Setenv("MY_NODE_NAME", myNodeName)
493-
testIncreaseIPPool(t, true, true)
499+
testIncreaseIPPool(t, true, true, false)
494500
}
495501

496-
func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool) {
502+
func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool, subnetDiscovery bool) {
497503
m := setup(t)
498504
defer m.ctrl.Finish()
499505
ctx := context.Background()
@@ -506,11 +512,15 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
506512
warmENITarget: 1,
507513
networkClient: m.network,
508514
useCustomNetworking: UseCustomNetworkCfg(),
515+
useSubnetDiscovery: UseSubnetDiscovery(),
509516
manageENIsNonScheduleable: ManageENIsOnNonSchedulableNode(),
510517
primaryIP: make(map[string]string),
511518
terminating: int32(0),
512519
}
513520
mockContext.dataStore = testDatastore()
521+
if subnetDiscovery {
522+
mockContext.dataStore.AddENI(primaryENIid, primaryDevice, true, false, false)
523+
}
514524

515525
primary := true
516526
notPrimary := false
@@ -564,13 +574,14 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
564574
if unschedulabeNode {
565575
val, exist := os.LookupEnv(envManageENIsNonSchedulable)
566576
if exist && val == "true" {
567-
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
577+
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
568578
} else {
569-
assertAllocationExternalCalls(false, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
579+
assertAllocationExternalCalls(false, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
570580
}
571-
581+
} else if subnetDiscovery {
582+
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, true)
572583
} else {
573-
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
584+
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
574585
}
575586

576587
if mockContext.useCustomNetworking {
@@ -609,14 +620,18 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
609620
mockContext.increaseDatastorePool(ctx)
610621
}
611622

612-
func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMocks, sg []*string, podENIConfig *eniconfigscheme.ENIConfigSpec, eni2 string, eniMetadata []awsutils.ENIMetadata) {
623+
func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMocks, sg []*string, podENIConfig *eniconfigscheme.ENIConfigSpec, eni2 string, eniMetadata []awsutils.ENIMetadata, subnetDiscovery bool) {
613624
callCount := 0
614625
if shouldCall {
615626
callCount = 1
616627
}
617628

618629
if useENIConfig {
619630
m.awsutils.EXPECT().AllocENI(true, sg, podENIConfig.Subnet, 14).Times(callCount).Return(eni2, nil)
631+
} else if subnetDiscovery {
632+
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 14).Times(callCount).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
633+
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Times(callCount).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
634+
m.awsutils.EXPECT().AllocENI(false, nil, "", 14).Times(callCount).Return(eni2, nil)
620635
} else {
621636
m.awsutils.EXPECT().AllocENI(false, nil, "", 14).Times(callCount).Return(eni2, nil)
622637
}
@@ -627,15 +642,20 @@ func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMo
627642

628643
func TestIncreasePrefixPoolDefault(t *testing.T) {
629644
_ = os.Unsetenv(envCustomNetworkCfg)
630-
testIncreasePrefixPool(t, false)
645+
testIncreasePrefixPool(t, false, false)
646+
}
647+
648+
func TestIncreasePrefixPoolSubnetDiscoveryUnfilledENI(t *testing.T) {
649+
_ = os.Unsetenv(envCustomNetworkCfg)
650+
testIncreasePrefixPool(t, false, true)
631651
}
632652

633653
func TestIncreasePrefixPoolCustomENI(t *testing.T) {
634654
_ = os.Setenv(envCustomNetworkCfg, "true")
635-
testIncreasePrefixPool(t, true)
655+
testIncreasePrefixPool(t, true, false)
636656
}
637657

638-
func testIncreasePrefixPool(t *testing.T, useENIConfig bool) {
658+
func testIncreasePrefixPool(t *testing.T, useENIConfig, subnetDiscovery bool) {
639659
m := setup(t)
640660
defer m.ctrl.Finish()
641661
ctx := context.Background()
@@ -650,13 +670,17 @@ func testIncreasePrefixPool(t *testing.T, useENIConfig bool) {
650670
warmPrefixTarget: 1,
651671
networkClient: m.network,
652672
useCustomNetworking: UseCustomNetworkCfg(),
673+
useSubnetDiscovery: UseSubnetDiscovery(),
653674
manageENIsNonScheduleable: ManageENIsOnNonSchedulableNode(),
654675
primaryIP: make(map[string]string),
655676
terminating: int32(0),
656677
enablePrefixDelegation: true,
657678
}
658679

659680
mockContext.dataStore = testDatastorewithPrefix()
681+
if subnetDiscovery {
682+
mockContext.dataStore.AddENI(primaryENIid, primaryDevice, true, false, false)
683+
}
660684

661685
primary := true
662686
testAddr1 := ipaddr01
@@ -677,6 +701,10 @@ func testIncreasePrefixPool(t *testing.T, useENIConfig bool) {
677701

678702
if useENIConfig {
679703
m.awsutils.EXPECT().AllocENI(true, sg, podENIConfig.Subnet, 1).Return(eni2, nil)
704+
} else if subnetDiscovery {
705+
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
706+
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
707+
m.awsutils.EXPECT().AllocENI(false, nil, "", 1).Return(eni2, nil)
680708
} else {
681709
m.awsutils.EXPECT().AllocENI(false, nil, "", 1).Return(eni2, nil)
682710
}

0 commit comments

Comments
 (0)