Unassign IPs over warm IP target #412
Conversation
| @@ -883,6 +886,34 @@ func (cache *EC2InstanceMetadataCache) AllocIPAddresses(eniID string, numIPs int | |||
| return nil | |||
| } | |||
|
|
|||
| // DeallocIPAddresses allocates numIPs of IP address on an ENI | |||
| func (cache *EC2InstanceMetadataCache) DeallocIPAddresses(eniID string, ips []string) error { | |||
| ctx := context.Background() | |||
There was a problem hiding this comment.
Currently just using a dummy context, but in a future change we should pipe context from higher up so that we can take advantage of cancellation.
| @@ -348,177 +352,254 @@ func (c *IPAMContext) StartNodeIPPoolManager() { | |||
| } | |||
|
|
|||
| func (c *IPAMContext) updateIPPoolIfRequired() { | |||
| c.retryAllocENIIP() | |||
There was a problem hiding this comment.
This logic was moved into increaseIPPool, since that's what its doing, and if WARM_IP_TARGET is set, then its not always a retry.
| log.Debugf("Found ENI %s that has less than the maximum number of IP addresses allocated: cur=%d, max=%d", | ||
| eni.ID, len(eni.IPv4Addresses), maxIPPerENI) | ||
|
|
||
| err = c.awsClient.AllocIPAddresses(eni.ID, maxIPPerENI-int64(len(eni.IPv4Addresses))) |
There was a problem hiding this comment.
Here is the IP allocation logic for each ENI. We attempt to allocate the remaining IPs on the ENI, keep them around until the next decreaseIPPool (which is called if we are over WARM_IP_TARGET), and then they will be unassigned if they are still not used. We only get here if we have less than WARM_IP_TARGET, so this doesn't continuously assign and unassign IPs.
| if eni != nil { | ||
| log.Debugf("Attempt again to allocate IP address for ENI :%s", eni.ID) | ||
| var err error | ||
| if warmIPTargetDefined { |
There was a problem hiding this comment.
See line 587 to see where the IP allocation logic moved for cases where ENIs should have more assigned IPs than they currently have.
| @@ -850,17 +929,25 @@ func getWarmIPTarget() int { | |||
| return noWarmIPTarget | |||
| } | |||
|
|
|||
| func (c *IPAMContext) getCurWarmIPTarget() (int64, bool) { | |||
| func (c *IPAMContext) ipTargetState() (short int, over int, enabled bool) { | |||
There was a problem hiding this comment.
This now returns short (the number of IPs that we are short of the target), over (the number of IPs that we are over the target) and enabled. I thought it made more sense than using a negative number to represent short, but maybe its more confusing. Appreciate feedback.
When the plugin's IP address pool goes beyond WARM_IP_TARGET, it will unassign excess IP addresses in order to reduce the total number of allocated IP addresses.
nckturner
force-pushed
the
remove-ips-over-warm-ip-target
branch
from
9084865 to
935e26d
Compare
| total, used := c.dataStore.GetStats() | ||
| log.Debugf("Successfully decreased IP pool") | ||
| logPoolStats(int64(total), int64(used), c.currentMaxAddrsPerENI, c.maxAddrsPerENI) | ||
| } | ||
|
|
||
| // findFreeableIPs finds and returns IPs that are not assigned to Pods but are attached | ||
| // to ENIs on the node. | ||
| func (c *IPAMContext) findFreeableIPs(eni string) ([]string, error) { |
There was a problem hiding this comment.
Would be nice with a unit test for this one.
|
|
||
| // Deallocate IPs from the instance if they aren't used by pods. | ||
| if err := c.awsClient.DeallocIPAddresses(eniID, ips); err != nil { | ||
| log.Debugf(fmt.Sprintf("Failed to decrease IP pool by removing IPs %v from ENI %s: %s", ips, eniID, err)) |
There was a problem hiding this comment.
What happens if we try to free 5 IPs and the call fails. Could 3 have been freed? Might be handled by the reconcile loop.
| @@ -3,58 +3,63 @@ module github.com/aws/amazon-vpc-cni-k8s | |||
| go 1.12 | |||
|
|
|||
| require ( | |||
There was a problem hiding this comment.
What is the actual dependency change? How easy is this PR to cherry pick over to the release-1.4 branch?
| github.com/davecgh/go-spew v1.1.1 | ||
| github.com/docker/distribution v2.6.2+incompatible | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/deckarep/golang-set v1.7.1 |
There was a problem hiding this comment.
golang-set is the only dependency I know of that actually changed (was added).
|
Not sure what happened. But I couldn't find any references to |
|
Superseded by #461 |
This is a change to the behavior of WARM_IP_TARGET. When the plugin's IP address pool goes beyond WARM_IP_TARGET, it will unassign excess IP addresses in order to reduce the total number of allocated IP addresses.
The way it does this is when the number of available IPs is short of WARM_IP_TARGET, we allocate the remaining IPs on the ENI. Every 30 seconds, if IPs remain ununsed, the
decreaseIPPoolfunction will deallocate IPs left over on ENIs down to WARM_IP_TARGET.Issue #, if available:
Helps with concerns like #314 where total IP usage in a VPC is a problem
Description of changes:
increaseIPPool(),retryAllocENIIP().c.decreaseIPPool(decreaseIPPoolInterval), so that it does not run as frequently (now every 30 seconds) to allow for IPs to be used by kubelet before being unassigned and removed from the pool.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.