Reuse existing Helm release values during rad upgrade kubernetes#11361
Reuse existing Helm release values during rad upgrade kubernetes#11361nicolejms wants to merge 14 commits intoradius-project:mainfrom
Conversation
- Changed pkg/cli/helm/cluster.go line 598 to use LastDeployed instead of FirstDeployed - Updated Test_Helm_GetRadiusRevisions_Success to use different timestamps for different revisions - Added comprehensive test Test_Helm_GetRadiusRevisions_MultipleUpgradesWithDifferentTimestamps - All tests pass successfully Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com>
…utput Fix revision timestamps in `rad rollback kubernetes --list-revisions` to show the list of all install times per version.
* Initial plan * Fix: Use LastDeployed instead of FirstDeployed for revision timestamps - Changed pkg/cli/helm/cluster.go line 598 to use LastDeployed instead of FirstDeployed - Updated Test_Helm_GetRadiusRevisions_Success to use different timestamps for different revisions - Added comprehensive test Test_Helm_GetRadiusRevisions_MultipleUpgradesWithDifferentTimestamps - All tests pass successfully Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com> * Merge pull request #2 from nicolejms/copilot/fix-revision-timestamp-output Fix revision timestamps in `rad rollback kubernetes --list-revisions` to show the list of all install times per version. * Merge branch 'main' into main * Merge branch 'main' into main * Merge branch 'radius-project:main' into main * Merge branch 'radius-project:main' into main * Merge branch 'radius-project:main' into main * Merge branch 'radius-project:main' into main * Merge branch 'radius-project:main' into main * new agent to investigate issues * Merge branch 'main' of https://github.com/nicolejms/radius * Initial plan * Add ReuseValues flag to Helm upgrade to preserve existing values Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com> * Add test and documentation for value preservation in upgrades Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com> * Address code review feedback with expanded test documentation Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com> * Add integration test to verify ReuseValues preserves existing Helm values Co-authored-by: nicolejms <101607760+nicolejms@users.noreply.github.com> * Merge remote-tracking branch 'origin/main' into copilot/fix-issue-11218
nicolejms
requested a deployment
to
external-contributor-approval
GitHub Actions
Waiting
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure rad upgrade kubernetes preserves existing Helm release values during upgrades (instead of resetting to chart defaults), and updates CLI help text and tests to reflect/validate that behavior.
Changes:
- Set
ReuseValues=truefor Helm upgrades inHelmClientImpl.RunHelmUpgrade. - Add a new test intended to validate value preservation/merging during upgrade.
- Update
rad upgrade kubernetescommand help text/examples to state that existing values are preserved and merged with--set/--set-file.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/cli/helm/helmclient.go | Enables Helm upgrade value reuse via ReuseValues=true. |
| pkg/cli/helm/helmclient_test.go | Adds a test intended to assert upgrade value preservation/merge behavior. |
| pkg/cli/cmd/upgrade/kubernetes/kubernetes.go | Updates CLI long description/examples to describe preserved/merged Helm values on upgrade. |
| // TestHelmClient_UpgradeReusesValues verifies that RunHelmUpgrade preserves existing values | ||
| // by using ReuseValues=true. This test validates the fix for issue #11218 where upgrades | ||
| // were resetting values to chart defaults. | ||
| func TestHelmClient_UpgradeReusesValues(t *testing.T) { | ||
| t.Run("upgrade preserves existing values and merges new values", func(t *testing.T) { | ||
| // This is an integration test that validates the actual behavior of RunHelmUpgrade | ||
| // with ReuseValues=true by simulating an upgrade with Helm's in-memory storage. | ||
|
|
||
| // Test data representing values from previous install/upgrade | ||
| existingValues := map[string]interface{}{ | ||
| "global": map[string]interface{}{ | ||
| "azureWorkloadIdentity": map[string]interface{}{ | ||
| "enabled": true, | ||
| }, | ||
| "imageRegistry": "myregistry.azurecr.io", | ||
| }, | ||
| "database": map[string]interface{}{ | ||
| "enabled": true, | ||
| }, | ||
| } | ||
|
|
||
| // New values being applied in this upgrade | ||
| newValues := map[string]interface{}{ | ||
| "global": map[string]interface{}{ | ||
| "imageTag": "0.48.0", | ||
| }, | ||
| } | ||
|
|
||
| // Expected merged result: existing values preserved + new values applied | ||
| expectedValues := map[string]interface{}{ | ||
| "global": map[string]interface{}{ | ||
| "azureWorkloadIdentity": map[string]interface{}{ | ||
| "enabled": true, | ||
| }, | ||
| "imageRegistry": "myregistry.azurecr.io", | ||
| "imageTag": "0.48.0", | ||
| }, | ||
| "database": map[string]interface{}{ | ||
| "enabled": true, | ||
| }, | ||
| } | ||
|
|
||
| // Create an in-memory Helm configuration for testing (similar to Helm's own tests) | ||
| registryClient, err := registry.NewClient() | ||
| require.NoError(t, err, "Failed to create registry client") | ||
|
|
||
| cfg := &helm.Configuration{ | ||
| Releases: storage.Init(driver.NewMemory()), | ||
| KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard}, | ||
| Capabilities: chartutil.DefaultCapabilities, | ||
| RegistryClient: registryClient, | ||
| Log: func(format string, v ...interface{}) {}, | ||
| } | ||
|
|
||
| // Create and store an initial release with existing values | ||
| initialRelease := &release.Release{ | ||
| Name: "test-release", | ||
| Namespace: "test-namespace", | ||
| Version: 1, | ||
| Config: existingValues, | ||
| Chart: &chart.Chart{ | ||
| Metadata: &chart.Metadata{ | ||
| Name: "test-chart", | ||
| Version: "1.0.0", | ||
| }, | ||
| }, | ||
| Info: &release.Info{ | ||
| Status: release.StatusDeployed, | ||
| }, | ||
| } | ||
| err = cfg.Releases.Create(initialRelease) | ||
| require.NoError(t, err, "Failed to create initial release") | ||
|
|
||
| // Create a chart with new values | ||
| upgradeChart := &chart.Chart{ | ||
| Metadata: &chart.Metadata{ | ||
| Name: "test-chart", | ||
| Version: "1.1.0", | ||
| }, | ||
| Values: newValues, | ||
| } |
There was a problem hiding this comment.
This test sets upgradeChart.Values to contain only the incremental changes, but in the real codepath helmChart.Values comes from the loaded chart (defaults) and is then mutated by --set/--set-file. That means upgrades will still pass a map containing many default keys, which can overwrite existing release values even with ReuseValues=true. Update the test to reflect the real scenario (chart defaults present) and assert that only CLI overrides are applied on top of the existing release config.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| The upgrade process preserves your existing Helm chart values (such as Azure Workload Identity settings, | ||
| database configuration, and custom container registries). The upgrade merges these existing values with | ||
| any new values specified via --set or --set-file flags, allowing you to override specific settings | ||
| without losing your configuration. |
There was a problem hiding this comment.
The command help claims upgrades preserve existing Helm values and only override what’s provided via --set/--set-file, but the current implementation passes helmChart.Values (chart defaults + parsed CLI flags) into Helm upgrade. Unless the upgrade path is changed to pass only user overrides (or an empty map), this documentation is likely inaccurate and may mislead users.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @@ -61,9 +67,18 @@ rad workspace show | |||
| rad workspace switch myworkspace | |||
| rad upgrade kubernetes | |||
|
|
|||
| # Upgrade Radius with custom configuration | |||
| # Upgrade Radius and override a specific value | |||
| # All other existing values from the previous installation are preserved | |||
| rad upgrade kubernetes --set key=value | |||
|
|
|||
| # Example: If you installed with Azure Workload Identity enabled: | |||
| # rad install kubernetes --set global.azureWorkloadIdentity.enabled=true | |||
| # Then upgrade without repeating the flag - the setting is preserved: | |||
| rad upgrade kubernetes | |||
|
|
|||
| # You can still override specific values during upgrade: | |||
| rad upgrade kubernetes --set global.imageTag=0.48 | |||
|
|
|||
There was a problem hiding this comment.
The examples state that existing values are "automatically preserved" on upgrade, but with the current Helm upgrade invocation this is only true if the values map passed to Helm contains just explicit overrides. Either adjust the implementation to match this behavior or soften the wording in the examples to avoid promising value preservation that may not occur.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| upgradeClient := helm.NewUpgrade(helmConf) | ||
| upgradeClient.Namespace = namespace | ||
| upgradeClient.Wait = wait | ||
| upgradeClient.Timeout = upgradeTimeout | ||
| upgradeClient.Recreate = true | ||
| upgradeClient.ReuseValues = true | ||
|
|
||
| return upgradeClient.Run(releaseName, helmChart, helmChart.Values) | ||
| } |
There was a problem hiding this comment.
Setting upgradeClient.ReuseValues = true won’t preserve prior user configuration as long as you continue to pass helmChart.Values (which includes the chart’s default values plus any --set mutations) into upgradeClient.Run(...). Those defaults will be treated as explicit overrides and can still overwrite the previous release’s config. Consider separating CLI-provided overrides from chart defaults (e.g., build a fresh values map from --set/--set-file and pass only that), or pass an empty values map when no overrides are supplied so reuse-values can actually take effect.
nicolejms
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11361 +/- ##
==========================================
+ Coverage 51.10% 51.12% +0.02%
==========================================
Files 699 699
Lines 44067 44080 +13
==========================================
+ Hits 22519 22537 +18
+ Misses 19401 19395 -6
- Partials 2147 2148 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Today when you upgrade kubernetes, the existing helm values which were used to deploy are ignored. While there is a workaround where you can pass in the helm values, it's counter-intuitive (and hard to troubleshoot failures) to upgrade and lose all existing helm settings. This PR just adds an option to reuse existing values using a built in helm option:
upgradeClient.ReuseValues = true
I reviewed the change with Will Tsai and he agrees this should be the default behavior.
Type of change
Fixes: #11218
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: