Generate rke2-canal resource bounds from template values rather than hardcoding. #359
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
|
Thanks! Please sign-off all of your commits for DCO! |
manuelbuil
approved these changes
Jul 13, 2023
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
AJMansfield
force-pushed
the
canal-resources
branch
from
dc564df
to
3c298f3
Compare
Done! Also, updated the PR comment to follow the new PR template I saw was being pushed through. |
rbrtbnfgl
approved these changes
Jul 14, 2023
github-actions bot
pushed a commit
that referenced
this pull request
Jul 14, 2023
…hardcoding. (#359) * use template values to set resource bounds * packageVersion increment Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
|
@AJMansfield Could you create a PR in rke2 so that it starts using the new chart? Something like this PR rancher/rke2@33caf61. No hurry, we are already in code freeze for the July release. |
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.
Proposed Changes
This PR adds additional
resourceshelm value configuration options torke2-canal. It replaces the previously-hardcoded 250m CPU request on thecalico-nodecontainer with a helm-configured request, and also adds the option to theflannel-nodecontainer.I've added the remaining unused resource bound keys as comments for documentation purposes (aside from the
calico.resources.requests.cpukey that's set by default to preserve the existing 250m bound). I don't know if the bounds the comments suggest are reasonable for the same median use-case as that 250m bound, but I don't figure it matters much; anyone configuring them should be doing their own profiling on their own hardware anyway.Types of Changes
This is a non-breaking change to enhance existing functionality with the addition of a standard feature.
Verification
This patch does not result in any changes to the template output from helm, unless the new configuration keys it adds are overridden; the default rendered output manifests is byte-for-byte identical to the pre-patch version.
The code pattern used in the added code is the standard way to implement resource bounds configuration in helm, as seen already in use in other packages in this repository such as
rke2-multusorrke2-coredns.Further Comments
For my specific use case, this change makes it possible for me to scale the default request down, and make it a better fit for the small 4-core nodes I'm running. With this change I can just add a
HelmChartConfigwith the appropriate resource bounds keys to my RKE2 server manifests folder (i.e./var/lib/rancher/rke2/server/manifests/canal-resources.yaml), rather than needing Kustomize or some other workaround.