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.

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

Merged
merged 2 commits into from
Oct 28, 2019
Merged

Introduce a minimum target for ENI IPs #612

merged 2 commits into from
Oct 28, 2019

Conversation

asheldon
Copy link
Contributor

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.

@mogren mogren self-requested a review September 10, 2019 19:50
Copy link
Contributor

@edmorley edmorley left a 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 :-)

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
@asheldon
Copy link
Contributor Author

Thanks, @edmorley!

Copy link
Contributor

@jaypipes jaypipes left a 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.

@asheldon
Copy link
Contributor Author

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
Copy link
Contributor

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

1 similar comment
@jaypipes
Copy link
Contributor

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

Copy link
Contributor

@jaypipes jaypipes left a 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.
Copy link
Contributor

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)

Copy link
Contributor Author

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.

@@ -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
Copy link
Contributor

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/

Copy link
Contributor Author

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.

ipamd/datastore/data_store_test.go Show resolved Hide resolved
@asheldon asheldon requested a review from jaypipes October 26, 2019 07:31
Copy link
Contributor

@nckturner nckturner left a 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 👍

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
@asheldon
Copy link
Contributor Author

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

Copy link
Contributor

@mogren mogren left a 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.

ipamd/ipamd.go Show resolved Hide resolved
asheldon and others added 2 commits October 27, 2019 21:48
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>
Copy link
Contributor

@mogren mogren left a 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

Copy link
Contributor

@jaypipes jaypipes left a 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).
Copy link
Contributor

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.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tests. ✔️

@jaypipes jaypipes merged commit 1726afd into aws:master Oct 28, 2019
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this pull request Oct 28, 2019
* 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)
mogren pushed a commit that referenced this pull request Oct 28, 2019
* 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)
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this pull request Dec 15, 2019
* 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>
jaypipes pushed a commit that referenced this pull request Dec 19, 2019
* 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>
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this pull request Apr 20, 2020
* 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>
mogren pushed a commit that referenced this pull request Apr 20, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants