Introduce a minimum target for ENI IPs by asheldon · Pull Request #612 · aws/amazon-vpc-cni-k8s

asheldon · 2019-09-10T00:09:12Z

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.

edmorley

Hi! I spotted some typos in passing :-)

README.md

ipamd/ipamd.go

ipamd/datastore/data_store.go

asheldon · 2019-10-17T18:36:25Z

Thanks, @edmorley!

jaypipes

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.

asheldon · 2019-10-23T15:44:27Z

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.

jaypipes · 2019-10-25T12:11:47Z

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,
-jay

jaypipes · 2019-10-25T12:11:47Z

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,
-jay

jaypipes

Couple minor suggestions and a request for a couple new tests, but overall great stuff, @asheldon

jaypipes · 2019-10-25T12:20:51Z

README.md

+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.


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)

Please let me know if the new text looks good to you.

jaypipes · 2019-10-25T12:28:56Z

ipamd/datastore/data_store.go

 }

-func (ds *DataStore) getDeletableENI(warmIPTarget int) *ENIIPPool {
+// IsRequiredForMinimumWarmIPTarget determines if this ENI is necessary to fulfill whatever MINIMUM_IP_TARGET is


s/fulfill whatever MINIMUM_IP_TARGET/fulfill whatever is greater, MINIMUM_IP_TARGET or WARM_IP_TARGET/

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.

ipamd/datastore/data_store_test.go

nckturner

Thanks, I think this would be a helpful parameter to have 👍

ipamd/datastore/data_store.go

ipamd/datastore/data_store_test.go

asheldon · 2019-10-27T02:35:03Z

Thanks for the feedback, @nckturner! I believe all your concerns have been addressed.

mogren

Thanks a lot @asheldon, this is a good feature. Just one extra check we should do.

ipamd/ipamd.go

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 <[email protected]>

mogren

Thanks a lot @asheldon! LGTM

jaypipes

Excellent work, @asheldon

jaypipes · 2019-10-28T14:54:19Z

README.md

+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).


bueno. 👍

jaypipes · 2019-10-28T14:55:13Z

ipamd/datastore/data_store_test.go

+	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.")


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 <[email protected]> (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 <[email protected]>

mogren added the enhancement label Sep 10, 2019

mogren self-requested a review September 10, 2019 19:50

edmorley reviewed Oct 15, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

ipamd/ipamd.go Outdated Show resolved Hide resolved

ipamd/ipamd.go Outdated Show resolved Hide resolved

ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

jaypipes reviewed Oct 23, 2019

View reviewed changes

jaypipes suggested changes Oct 25, 2019

View reviewed changes

asheldon requested a review from jaypipes October 26, 2019 07:31

nckturner reviewed Oct 26, 2019

View reviewed changes

ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

ipamd/datastore/data_store_test.go Show resolved Hide resolved

ipamd/datastore/data_store_test.go Outdated Show resolved Hide resolved

mogren suggested changes Oct 28, 2019

View reviewed changes

ipamd/ipamd.go Show resolved Hide resolved

asheldon and others added 2 commits October 27, 2019 21:48

Fix multiple incorrect references to MINIMUM_IP_TARGET

9546d60

Co-Authored-By: Ed Morley <[email protected]>

mogren approved these changes Oct 28, 2019

View reviewed changes

nckturner approved these changes Oct 28, 2019

View reviewed changes

jaypipes approved these changes Oct 28, 2019

View reviewed changes

jaypipes merged commit 1726afd into aws:master Oct 28, 2019

asheldon mentioned this pull request Oct 28, 2019

Fix Readme.md formatting around MINIMUM_IP_TARGET #683

Merged

mogren mentioned this pull request Oct 28, 2019

Cherry pick fixes over to 1.6 branch from master #684

Merged

Conversation

asheldon commented Sep 10, 2019

Uh oh!

edmorley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asheldon commented Oct 17, 2019

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

asheldon commented Oct 23, 2019

Uh oh!

jaypipes commented Oct 25, 2019

Uh oh!

jaypipes commented Oct 25, 2019

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

asheldon Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

jaypipes Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

asheldon Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nckturner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asheldon commented Oct 27, 2019

Uh oh!

mogren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mogren left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

jaypipes Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants