Don't rewrite manifests when values don't change #2457
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
This was referenced Feb 10, 2022
briandowns
reviewed
Feb 10, 2022
briandowns
reviewed
Feb 10, 2022
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond
force-pushed
the
helmchart_conditional_rewrite
branch
from
eeedd2c to
d7e319f
Compare
briandowns
approved these changes
Feb 10, 2022
| chart.Spec.Set[k] = intstr.FromString(v) | ||
| val := intstr.FromString(v) | ||
| if cur, ok := chart.Spec.Set[k]; ok { | ||
| curBytes, _ := cur.MarshalJSON() |
There was a problem hiding this comment.
why do we ignore the error check here?
There was a problem hiding this comment.
The new value is guaranteed to be valid since we just generated it, and if the current value is invalid and returns an error we'll handle comparing the empty byte slice to the new value properly anyway despite the error.
galal-hussein
approved these changes
Feb 11, 2022
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
Don't rewrite manifests when values don't change.
RKE2 injects cluster configuration into HelmChart manifests on startup, including into user-provided HelmCharts, by rewriting the files on disk. These manifests were rewritten on startup every time, even if cluster configuration values did not change. This rewrite causes the Deploy controller to re-apply the manifest, and in turn the Helm controller to trigger a helm upgrade of the affected chart - even if there was not in fact any change to apply.
These no-op helm upgrades could cause issues if they triggered hooks in the Helm chart, or resulted in the Helm release getting stuck pending due to simultaneous upgrades to cluster infrastructure triggered by the restart.
Types of Changes
bugfix
Verification
Linked Issues
Further Comments
Note that manifests for packaged components will still get rewritten every time, as they are copied out of the runtime image every startup.