Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Subnet Discovery - Unfilled ENI fix #2954

Merged
merged 1 commit into from
Jun 21, 2024
+91 −52
Merged

Subnet Discovery - Unfilled ENI fix #2954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions pkg/ipamd/datastore/data_store.go
View file
Open in desktop
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
Open in desktop
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) {
jchen6585 marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Loading