-
Notifications
You must be signed in to change notification settings - Fork 749
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.
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
Introduce a minimum target for ENI IPs #612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I spotted some typos in passing :-)
Thanks, @edmorley! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @asheldon, thanks for your contribution! Unfortunately, I'm not understanding how MINIMUM_IP_TARGET
is different from WARM_IP_TARGET
. When using WARM_IP_TARGET
, the "2 x IPs usage" is not an issue, because the plugin will not pre-allocate a whole new ENI and associated IP addresses like the behaviour is with WARM_ENI_TARGET
.
Hi @jaypipes. MINIMUM_IP_TARGET is for pre-scaling, WARM_IP_TARGET is for dynamic scaling. Suppose I have a cluster and know I will deploy approximately 30 pods to each node. What I've observed in practice is that people set WARM_IP_TARGET to 30 in this case to ensure there are enough IPs allocated up-front by the CNI. Then, when their 30 pods are running, the CNI allocates an additional 30 IPs to the node that are never used. This accelerates IP exhaustion in the relevant subnets. If instead they set the proposed MINIMUM_IP_TARGET to 30 and WARM_IP_TARGET to 2, they would deploy their 30 expected pods and the CNI would allocate an additional 2 IPs. This still provides elasticity, but uses roughly half as many IPs as the prior behavior (32 vs 60 IPs). This also improves reliability of the EKS cluster by reducing the number of calls necessary to allocate or deallocate private IPs, which may be throttled, especially at scaling-related times. |
Ahh... I see now, thank you for the excellent explanation. OK, I will re-review with that understanding. Cheers, |
1 similar comment
Ahh... I see now, thank you for the excellent explanation. OK, I will re-review with that understanding. Cheers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor suggestions and a request for a couple new tests, but overall great stuff, @asheldon
README.md
Outdated
For example, if `MINIMUM_IP_TARGET` is set to 10, then `ipamD` attempts to prevent the total number of IP addresses from falling | ||
below 10 at all times. If the elastic network interfaces on the node are unable to provide this many addresses, `ipamD` attempts | ||
to allocate more interfaces until `MINIMUM_IP_TARGET` IP addresses exist in total. | ||
If both `WARM_IP_TARGET` and `MINIMUM_IP_TARGET` are set, `ipamD` will attempt to meet both constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go ahead and add the explanation you gave to me into the docs here. I think it was a very clear explanation of the difference, and that difference kind of gets lost in the wording used above (basically, the difference is between "free" and "total", but it's not as clear as when you described it with an example in your reply to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if the new text looks good to you.
ipamd/datastore/data_store.go
Outdated
@@ -331,7 +331,19 @@ func (ds *DataStore) isRequiredForWarmIPTarget(warmIPTarget int, eni *ENIIPPool) | |||
return otherWarmIPs < warmIPTarget | |||
} | |||
|
|||
func (ds *DataStore) getDeletableENI(warmIPTarget int) *ENIIPPool { | |||
// IsRequiredForMinimumWarmIPTarget determines if this ENI is necessary to fulfill whatever MINIMUM_IP_TARGET is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/fulfill whatever MINIMUM_IP_TARGET/fulfill whatever is greater, MINIMUM_IP_TARGET or WARM_IP_TARGET/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be a misleading change to make since this method considers only the minimum IP target and the prior method, 'isRequiredForWarmIPTarget', is responsible for the warm IP target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this would be a helpful parameter to have 👍
Thanks for the feedback, @nckturner! I believe all your concerns have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @asheldon, this is a good feature. Just one extra check we should do.
Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value.
Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @asheldon! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, @asheldon
allocated up front by the CNI, then 30 pods are deployed to the node, the CNI will allocate an additional 30 IPs, for | ||
a total of 60, accelerating IP exhaustion in the relevant subnets. If instead `MINIMUM_IP_TARGET` is set to 30 and | ||
`WARM_IP_TARGET` to 2, after the 30 pods are deployed the CNI would allocate an additional 2 IPs. This still provides | ||
elasticity, but uses roughly half as many IPs as using WARM_IP_TARGET alone (32 IPs vs 60 IPs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bueno. 👍
secondRemovedEni := ds.RemoveUnusedENIFromStore(noWarmIPTarget, 2) | ||
assert.Contains(t, []string{"eni-2", "eni-3"}, secondRemovedEni) | ||
|
||
assert.NotEqual(t, removedEni, secondRemovedEni, "The two removed ENIs should not be the same ENI.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good tests. ✔️
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com> (cherry picked from commit 1726afd)
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com> (cherry picked from commit 1726afd)
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com>
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com>
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com>
* Introduce a minimum target for ENI IPs Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value. * Fix multiple incorrect references to MINIMUM_IP_TARGET Co-Authored-By: Ed Morley <501702+edmorley@users.noreply.github.com>
Adds a MINIMUM_IP_TARGET environment variable to inform the AWS CNI that a particular number of total IPs is anticipated for use with a particular node. This is useful to ensure a sufficient supply of IPs on a node up-front without the 2x IP usage overhead of setting WARM_IP_TARGET to the same value.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.