Skip to content

Commit

Permalink
Subnet Discovery - Unfilled ENI fix
Browse files Browse the repository at this point in the history
  • Loading branch information
Joseph Chen committed Jun 21, 2024
1 parent 0e3d4b1 commit 660c41f
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 52 deletions.
9 changes: 5 additions & 4 deletions pkg/ipamd/datastore/data_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -973,8 +973,9 @@ func (e *ENI) hasPods() bool {
return e.AssignedIPv4Addresses() != 0
}

// GetENINeedsIP finds an ENI in the datastore that needs more IP addresses allocated
func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENI {
// GetAllocatableENIs finds ENIs in the datastore that needs more IP addresses allocated
func (ds *DataStore) GetAllocatableENIs(maxIPperENI int, skipPrimary bool) []*ENI {
var enis []*ENI
ds.lock.Lock()
defer ds.lock.Unlock()
for _, eni := range ds.eniPool {
Expand All @@ -985,10 +986,10 @@ func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENI {
if len(eni.AvailableIPv4Cidrs) < maxIPperENI {
ds.log.Debugf("Found ENI %s that has less than the maximum number of IP/Prefixes addresses allocated: cur=%d, max=%d",
eni.ID, len(eni.AvailableIPv4Cidrs), maxIPperENI)
return eni
enis = append(enis, eni)
}
}
return nil
return enis
}

// RemoveUnusedENIFromStore removes a deletable ENI from the data store.
Expand Down
80 changes: 45 additions & 35 deletions pkg/ipamd/ipamd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,7 @@ func (c *IPAMContext) tryAssignCidrs() (increasedPool bool, err error) {
// For an ENI, try to fill in missing IPs on an existing ENI.
// PRECONDITION: isDatastorePoolTooLow returned true
func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) {

// If WARM_IP_TARGET is set, only proceed if we are short of target
short, _, warmIPTargetsDefined := c.datastoreTargetState(nil)
if warmIPTargetsDefined && short == 0 {
Expand All @@ -928,45 +929,50 @@ func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) {
}

// Find an ENI where we can add more IPs
eni := c.dataStore.GetENINeedsIP(c.maxIPsPerENI, c.useCustomNetworking)
if eni != nil && len(eni.AvailableIPv4Cidrs) < c.maxIPsPerENI {
currentNumberOfAllocatedIPs := len(eni.AvailableIPv4Cidrs)
// Try to allocate all available IPs for this ENI
resourcesToAllocate := min((c.maxIPsPerENI - currentNumberOfAllocatedIPs), toAllocate)
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
log.Warnf("failed to allocate all available IP addresses on ENI %s, err: %v", eni.ID, err)
// Try to just get one more IP
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
enis := c.dataStore.GetAllocatableENIs(c.maxIPsPerENI, c.useCustomNetworking)
for _, eni := range enis {
if len(eni.AvailableIPv4Cidrs) < c.maxIPsPerENI {
currentNumberOfAllocatedIPs := len(eni.AvailableIPv4Cidrs)
// Try to allocate all available IPs for this ENI
resourcesToAllocate := min((c.maxIPsPerENI - currentNumberOfAllocatedIPs), toAllocate)
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IP addresses on ENI %s, err ", eni.ID))
log.Warnf("failed to allocate all available IP addresses on ENI %s, err: %v", eni.ID, err)
// Try to just get one more IP
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
if c.useSubnetDiscovery && containsInsufficientCIDRsOrSubnetIPs(err) {
continue
}
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IP addresses on ENI %s, err ", eni.ID))
}
}
}

var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
if containsPrivateIPAddressLimitExceededError(err) {
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +
"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.")
// This call to EC2 is needed to verify which IPs got attached to this ENI.
ec2ip4s, err = c.awsClient.GetIPv4sFromEC2(eni.ID)
if err != nil {
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
}
} else {
if output == nil {
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
}
var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
if containsPrivateIPAddressLimitExceededError(err) {
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +
"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.")
// This call to EC2 is needed to verify which IPs got attached to this ENI.
ec2ip4s, err = c.awsClient.GetIPv4sFromEC2(eni.ID)
if err != nil {
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
}
} else {
if output == nil {
ipamdErrInc("increaseIPPoolGetENIaddressesFailed")
return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation")
}

ec2Addrs := output.AssignedPrivateIpAddresses
for _, ec2Addr := range ec2Addrs {
ec2ip4s = append(ec2ip4s, &ec2.NetworkInterfacePrivateIpAddress{PrivateIpAddress: aws.String(aws.StringValue(ec2Addr.PrivateIpAddress))})
ec2Addrs := output.AssignedPrivateIpAddresses
for _, ec2Addr := range ec2Addrs {
ec2ip4s = append(ec2ip4s, &ec2.NetworkInterfacePrivateIpAddress{PrivateIpAddress: aws.String(aws.StringValue(ec2Addr.PrivateIpAddress))})
}
}
c.addENIsecondaryIPsToDataStore(ec2ip4s, eni.ID)
return true, nil
}
c.addENIsecondaryIPsToDataStore(ec2ip4s, eni.ID)
return true, nil
}
return false, nil
}
Expand Down Expand Up @@ -1015,8 +1021,8 @@ func (c *IPAMContext) tryAssignPrefixes() (increasedPool bool, err error) {
toAllocate := c.getPrefixesNeeded()
// Returns an ENI which has space for more prefixes to be attached, but this
// ENI might not suffice the WARM_IP_TARGET/WARM_PREFIX_TARGET
eni := c.dataStore.GetENINeedsIP(c.maxPrefixesPerENI, c.useCustomNetworking)
if eni != nil {
enis := c.dataStore.GetAllocatableENIs(c.maxPrefixesPerENI, c.useCustomNetworking)
for _, eni := range enis {
currentNumberOfAllocatedPrefixes := len(eni.AvailableIPv4Cidrs)
resourcesToAllocate := min((c.maxPrefixesPerENI - currentNumberOfAllocatedPrefixes), toAllocate)
output, err := c.awsClient.AllocIPAddresses(eni.ID, resourcesToAllocate)
Expand All @@ -1026,9 +1032,13 @@ func (c *IPAMContext) tryAssignPrefixes() (increasedPool bool, err error) {
output, err = c.awsClient.AllocIPAddresses(eni.ID, 1)
if err != nil && !containsPrivateIPAddressLimitExceededError(err) {
ipamdErrInc("increaseIPPoolAllocIPAddressesFailed")
if c.useSubnetDiscovery && containsInsufficientCIDRsOrSubnetIPs(err) {
continue
}
return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IPv4 prefix on ENI %s, err: %v", eni.ID, err))
}
}

var ec2Prefixes []*ec2.Ipv4PrefixSpecification
if containsPrivateIPAddressLimitExceededError(err) {
log.Debug("AssignPrivateIpAddresses returned PrivateIpAddressLimitExceeded. This can happen if the data store is out of sync." +
Expand Down
54 changes: 41 additions & 13 deletions pkg/ipamd/ipamd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/golang/mock/gomock"
"github.com/samber/lo"
Expand Down Expand Up @@ -468,32 +469,37 @@ func getDummyENIMetadataWithV6Prefix() awsutils.ENIMetadata {

func TestIncreaseIPPoolDefault(t *testing.T) {
_ = os.Unsetenv(envCustomNetworkCfg)
testIncreaseIPPool(t, false, false)
testIncreaseIPPool(t, false, false, false)
}

func TestIncreaseIPPoolSubnetDiscoveryUnfilledENI(t *testing.T) {
_ = os.Unsetenv(envCustomNetworkCfg)
testIncreaseIPPool(t, false, false, true)
}

func TestIncreaseIPPoolCustomENI(t *testing.T) {
_ = os.Setenv(envCustomNetworkCfg, "true")
_ = os.Setenv("MY_NODE_NAME", myNodeName)
testIncreaseIPPool(t, true, false)
testIncreaseIPPool(t, true, false, false)
}

// Testing that the ENI will be allocated on non schedulable node when the AWS_MANAGE_ENIS_NON_SCHEDULABLE is set to `true`
func TestIncreaseIPPoolCustomENIOnNonSchedulableNode(t *testing.T) {
_ = os.Setenv(envCustomNetworkCfg, "true")
_ = os.Setenv(envManageENIsNonSchedulable, "true")
_ = os.Setenv("MY_NODE_NAME", myNodeName)
testIncreaseIPPool(t, true, true)
testIncreaseIPPool(t, true, true, false)
}

// Testing that the ENI will NOT be allocated on non schedulable node when the AWS_MANAGE_ENIS_NON_SCHEDULABLE is not set
func TestIncreaseIPPoolCustomENIOnNonSchedulableNodeDefault(t *testing.T) {
_ = os.Unsetenv(envManageENIsNonSchedulable)
_ = os.Setenv(envCustomNetworkCfg, "true")
_ = os.Setenv("MY_NODE_NAME", myNodeName)
testIncreaseIPPool(t, true, true)
testIncreaseIPPool(t, true, true, false)
}

func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool) {
func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool, subnetDiscovery bool) {
m := setup(t)
defer m.ctrl.Finish()
ctx := context.Background()
Expand All @@ -506,11 +512,15 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
warmENITarget: 1,
networkClient: m.network,
useCustomNetworking: UseCustomNetworkCfg(),
useSubnetDiscovery: UseSubnetDiscovery(),
manageENIsNonScheduleable: ManageENIsOnNonSchedulableNode(),
primaryIP: make(map[string]string),
terminating: int32(0),
}
mockContext.dataStore = testDatastore()
if subnetDiscovery {
mockContext.dataStore.AddENI(primaryENIid, primaryDevice, true, false, false)
}

primary := true
notPrimary := false
Expand Down Expand Up @@ -564,13 +574,14 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
if unschedulabeNode {
val, exist := os.LookupEnv(envManageENIsNonSchedulable)
if exist && val == "true" {
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
} else {
assertAllocationExternalCalls(false, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
assertAllocationExternalCalls(false, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
}

} else if subnetDiscovery {
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, true)
} else {
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata)
assertAllocationExternalCalls(true, useENIConfig, m, sg, podENIConfig, eni2, eniMetadata, false)
}

if mockContext.useCustomNetworking {
Expand Down Expand Up @@ -609,14 +620,18 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool, unschedulabeNode bool)
mockContext.increaseDatastorePool(ctx)
}

func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMocks, sg []*string, podENIConfig *eniconfigscheme.ENIConfigSpec, eni2 string, eniMetadata []awsutils.ENIMetadata) {
func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMocks, sg []*string, podENIConfig *eniconfigscheme.ENIConfigSpec, eni2 string, eniMetadata []awsutils.ENIMetadata, subnetDiscovery bool) {
callCount := 0
if shouldCall {
callCount = 1
}

if useENIConfig {
m.awsutils.EXPECT().AllocENI(true, sg, podENIConfig.Subnet, 14).Times(callCount).Return(eni2, nil)
} else if subnetDiscovery {
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 14).Times(callCount).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Times(callCount).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
m.awsutils.EXPECT().AllocENI(false, nil, "", 14).Times(callCount).Return(eni2, nil)
} else {
m.awsutils.EXPECT().AllocENI(false, nil, "", 14).Times(callCount).Return(eni2, nil)
}
Expand All @@ -627,15 +642,20 @@ func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMo

func TestIncreasePrefixPoolDefault(t *testing.T) {
_ = os.Unsetenv(envCustomNetworkCfg)
testIncreasePrefixPool(t, false)
testIncreasePrefixPool(t, false, false)
}

func TestIncreasePrefixPoolSubnetDiscoveryUnfilledENI(t *testing.T) {
_ = os.Unsetenv(envCustomNetworkCfg)
testIncreasePrefixPool(t, false, true)
}

func TestIncreasePrefixPoolCustomENI(t *testing.T) {
_ = os.Setenv(envCustomNetworkCfg, "true")
testIncreasePrefixPool(t, true)
testIncreasePrefixPool(t, true, false)
}

func testIncreasePrefixPool(t *testing.T, useENIConfig bool) {
func testIncreasePrefixPool(t *testing.T, useENIConfig, subnetDiscovery bool) {
m := setup(t)
defer m.ctrl.Finish()
ctx := context.Background()
Expand All @@ -650,13 +670,17 @@ func testIncreasePrefixPool(t *testing.T, useENIConfig bool) {
warmPrefixTarget: 1,
networkClient: m.network,
useCustomNetworking: UseCustomNetworkCfg(),
useSubnetDiscovery: UseSubnetDiscovery(),
manageENIsNonScheduleable: ManageENIsOnNonSchedulableNode(),
primaryIP: make(map[string]string),
terminating: int32(0),
enablePrefixDelegation: true,
}

mockContext.dataStore = testDatastorewithPrefix()
if subnetDiscovery {
mockContext.dataStore.AddENI(primaryENIid, primaryDevice, true, false, false)
}

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

if useENIConfig {
m.awsutils.EXPECT().AllocENI(true, sg, podENIConfig.Subnet, 1).Return(eni2, nil)
} else if subnetDiscovery {
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
m.awsutils.EXPECT().AllocIPAddresses(primaryENIid, 1).Return(nil, awserr.New("InsufficientFreeAddressesInSubnet", "", errors.New("err")))
m.awsutils.EXPECT().AllocENI(false, nil, "", 1).Return(eni2, nil)
} else {
m.awsutils.EXPECT().AllocENI(false, nil, "", 1).Return(eni2, nil)
}
Expand Down

0 comments on commit 660c41f

Please sign in to comment.