adding a note to minimum ip target #2739
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdn5126
reviewed
Jan 2, 2024
README.md
Outdated
| @@ -333,6 +334,8 @@ elasticity, but uses roughly half as many IPs as using WARM_IP_TARGET alone (32 | |||
| This also improves the 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. | |||
|
|
|||
| **NOTE!** If MINIMUM_IP_TARGET is set WARM_ENI_TARGET will be ignored, please utilize WARM_IP_TARGET instead. Please also referring `WARM_ENI_TARGET`. | |||
There was a problem hiding this comment.
format nit: "NOTE! If MINIMUM_IP_TARGET is set, WARM_ENI_TARGET will be ignored. Please utilize WARM_IP_TARGET instead. Please also refer to WARM_ENI_TARGET."
There was a problem hiding this comment.
Nit: referring -> refer
I don't think we need -> Please also refer to WARM_ENI_TARGET... Lets also add a note in best practices guide..
There was a problem hiding this comment.
Let's also add that if MINIMUM_IP_TARGET is set, but WARM_IP_TARGET is not set, WARM_IP_TARGET is assumed to be 0, which will prevent future ENIs from being allocated. We should strongly recommend that WARM_IP_TARGET is configured when MINIMUM_IP_TARGET is
There was a problem hiding this comment.
Updated the content, can you check again? Thanks.
haouc
force-pushed
the
master
branch
2 times, most recently
from
6ce456b to
ef02ac5
Compare
jayanthvn
reviewed
Jan 2, 2024
README.md
Outdated
| @@ -333,6 +334,10 @@ elasticity, but uses roughly half as many IPs as using WARM_IP_TARGET alone (32 | |||
| This also improves the 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. | |||
|
|
|||
| **NOTE!** | |||
| 1. If `MINIMUM_IP_TARGET` is set, `WARM_ENI_TARGET` will be ignored. Please utilize `WARM_IP_TARGET` instead. | |||
| 2. If `MINIMUM_IP_TARGET` is set and `WARM_IP_TARGET` is not set, `WARM_IP_TARGET` is assumed to be 0, which will prevent future ENIs from being allocated. It is strongly recommended that `WARM_IP_TARGET` should be set greater than 0 when `MINIMUM_IP_TARGET` is set. | |||
There was a problem hiding this comment.
We can further clarify this...something like..
If `MINIMUM_IP_TARGET` is set and `WARM_IP_TARGET` is not set, `WARM_IP_TARGET` is assumed to be 0, leading to the number of IPs attached to the node will be the value of `MINIMUM_IP_TARGET`. This configuration will prevent future ENIs/IPs from being allocated. It is strongly recommended that `WARM_IP_TARGET` should be set greater than 0 when `MINIMUM_IP_TARGET` is set.
44b46d5 to
4416ca3
Compare
jayanthvn
reviewed
Jan 2, 2024
README.md
Outdated
| @@ -333,6 +334,10 @@ elasticity, but uses roughly half as many IPs as using WARM_IP_TARGET alone (32 | |||
| This also improves the 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. | |||
|
|
|||
| **NOTE!** | |||
| 1. If `MINIMUM_IP_TARGET` is set, `WARM_ENI_TARGET` will be ignored. Please utilize `WARM_IP_TARGET` instead. | |||
| 2. If `MINIMUM_IP_TARGET` is set and `WARM_IP_TARGET` is not set, `WARM_IP_TARGET` is assumed to be 0, which leads to the number of IPs attached to the node will be the value of `MINIMUM_IP_TARGET`. This configuration will prevent future ENIs from being allocated. It is strongly recommended that `WARM_IP_TARGET` should be set greater than 0 when `MINIMUM_IP_TARGET` is set. | |||
There was a problem hiding this comment.
Minor nit : "will prevent future ENIs" -> "will prevent future ENIs/IPs"
But rest lgtm
jayanthvn
previously approved these changes
Jan 2, 2024
jayanthvn
approved these changes
Jan 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
documentation
Which issue does this PR fix:
Clarifying the best practice to setup warm target parameters.
What does this PR do / Why do we need it:
Customers may be confused and impacted if the variables are set incorrectly.
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades? Has updating a running cluster been tested?:
Does this change require updates to the CNI daemonset config files to work?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.