From 6b5d500363b78f31b351b01c1799e25e9d9bdccd Mon Sep 17 00:00:00 2001 From: Jian Zhang Date: Thu, 12 Mar 2026 09:19:57 +0800 Subject: [PATCH] Fix: Trigger Helm upgrade when storage labels change even if manifests are identical --- internal/operator-controller/applier/helm.go | 19 +++++-- .../operator-controller/applier/helm_test.go | 50 ++++++++++++++++++- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index ab6e58a36..38d95a6a4 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -117,7 +117,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return false, "", err } - rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post) + rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post, storageLabels) if err != nil { return false, "", fmt.Errorf("failed to get release state using server-side dry-run: %w", err) } @@ -289,7 +289,7 @@ func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExt }, helmclient.AppendInstallPostRenderer(post)) } -func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) { +func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer, storageLabels map[string]string) (*release.Release, *release.Release, string, error) { currentRelease, err := cl.Get(ext.GetName()) if errors.Is(err, driver.ErrReleaseNotFound) { desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error { @@ -318,12 +318,25 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE relState := StateUnchanged if desiredRelease.Manifest != currentRelease.Manifest || currentRelease.Info.Status == release.StatusFailed || - currentRelease.Info.Status == release.StatusSuperseded { + currentRelease.Info.Status == release.StatusSuperseded || + releaseLabelsChanged(currentRelease.Labels, storageLabels) { relState = StateNeedsUpgrade } return currentRelease, desiredRelease, relState, nil } +// releaseLabelsChanged checks if any of the storage labels have changed. +// This ensures we upgrade the release when bundle metadata changes, +// even if the rendered manifests are identical. +func releaseLabelsChanged(currentLabels, desiredLabels map[string]string) bool { + for key, desiredValue := range desiredLabels { + if currentValue, exists := currentLabels[key]; !exists || currentValue != desiredValue { + return true + } + } + return false +} + type postrenderer struct { labels map[string]string cascade postrender.PostRenderer diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 25fad4d66..917702711 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -622,11 +622,16 @@ func TestApply_Upgrade(t *testing.T) { t.Run("fails during upgrade reconcile (StateUnchanged)", func(t *testing.T) { // make sure desired and current are the same this time - testDesiredRelease := *testCurrentRelease + // Current release must have the same labels as storageLabels to trigger StateUnchanged + testCurrentWithLabels := &release.Release{ + Info: &release.Info{Status: release.StatusDeployed}, + Labels: testStorageLabels, + } + testDesiredRelease := *testCurrentWithLabels mockAcg := &mockActionGetter{ reconcileErr: errors.New("failed reconciling charts"), - currentRel: testCurrentRelease, + currentRel: testCurrentWithLabels, desiredRel: &testDesiredRelease, } mockPf := &mockPreflight{} @@ -666,6 +671,47 @@ func TestApply_Upgrade(t *testing.T) { require.True(t, installSucceeded) require.Empty(t, installStatus) }) + + t.Run("triggers upgrade when storage labels change but manifest is unchanged", func(t *testing.T) { + // Current release has old labels but same manifest as desired + testCurrentWithLabels := &release.Release{ + Info: &release.Info{Status: release.StatusDeployed}, + Labels: map[string]string{"bundle-version": "v1.0.0"}, + Manifest: validManifest, // Same manifest as desired - only labels differ + } + // Desired release has same manifest as current (no manifest change) + testDesiredRelease := &release.Release{ + Info: &release.Info{Status: release.StatusDeployed}, + Manifest: validManifest, + } + + mockAcg := &mockActionGetter{ + currentRel: testCurrentWithLabels, + desiredRel: testDesiredRelease, + // Set reconcileErr to ensure test fails if reconcile path is taken instead of upgrade + reconcileErr: errors.New("reconcile should not be called when labels change"), + } + + helmApplier := applier.Helm{ + ActionClientGetter: mockAcg, + HelmChartProvider: DummyHelmChartProvider, + HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, + Manager: &mockManagedContentCacheManager{ + cache: &mockManagedContentCache{}, + }, + } + + // Use new storage labels that differ from current release labels + newStorageLabels := map[string]string{"bundle-version": "v2.0.0"} + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, newStorageLabels) + + // If the code correctly triggers upgrade (not reconcile) when labels change, + // the test succeeds. If it incorrectly takes the reconcile path, reconcileErr + // will cause the test to fail. + require.NoError(t, err) + require.True(t, installSucceeded) + require.Empty(t, installStatus) + }) } func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {