diff --git a/pkg/ipamd/datastore/data_store.go b/pkg/ipamd/datastore/data_store.go index ba49b98bc3..02d5cd21f0 100644 --- a/pkg/ipamd/datastore/data_store.go +++ b/pkg/ipamd/datastore/data_store.go @@ -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 { @@ -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. diff --git a/pkg/ipamd/ipamd.go b/pkg/ipamd/ipamd.go index cb11da3811..ca3c0c3306 100644 --- a/pkg/ipamd/ipamd.go +++ b/pkg/ipamd/ipamd.go @@ -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 { @@ -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 } @@ -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) @@ -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." + diff --git a/pkg/ipamd/ipamd_test.go b/pkg/ipamd/ipamd_test.go index 277e06d6d3..d522334257 100644 --- a/pkg/ipamd/ipamd_test.go +++ b/pkg/ipamd/ipamd_test.go @@ -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" @@ -468,13 +469,18 @@ 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` @@ -482,7 +488,7 @@ 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 @@ -490,10 +496,10 @@ 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() @@ -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 @@ -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 { @@ -609,7 +620,7 @@ 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 @@ -617,6 +628,10 @@ func assertAllocationExternalCalls(shouldCall bool, useENIConfig bool, m *testMo 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) } @@ -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() @@ -650,6 +670,7 @@ 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), @@ -657,6 +678,9 @@ func testIncreasePrefixPool(t *testing.T, useENIConfig bool) { } mockContext.dataStore = testDatastorewithPrefix() + if subnetDiscovery { + mockContext.dataStore.AddENI(primaryENIid, primaryDevice, true, false, false) + } primary := true testAddr1 := ipaddr01 @@ -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) }