feat: remove cpu limit default #1163
Merged
Conversation
This file contains 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
Signed-off-by: Ricardo Ribeiro <j.ribeiro.fafe@gmail.com>
NissesSenap
approved these changes
Jul 13, 2023
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.
Currently, CPU limit can't be removed.
The getResources function, which creates the base for resources, sets the requests and limits for both memory and CPU.
If one defines the container's limits not to include a CPU limit, the reconciler merges the base resources with the overrides defined in the CR using a structural merge patch, which doesn't remove fields present in base but missing from the overrides. So, there's no way we can remove CPU limits.
Now, this should not be yet another discussion on whether we should or not specify CPU limits, but to allow those who need to remove CPU limits to do so.
Possible solutions:
Remove the CPU limit from base: this is a breaking change, as users not defining the CPU limit would have to define it if needed. Also, having a CPU limit by default is a good thing if users don't want to explicitly remove it.
Look for the grafana container defined in the CR and if the CR defines limits, but leaves out the CPU limit, don't add it to the base (ie. define the base with only a memory limit). This is not a breaking change because if users don't define limits, these will still be defined as they are now. The only change is, if the CR defines limits only containing a memory limit, then only the memory limit is set, not the CPU limit.
This is an implementation of the first solution.
Related: #1161