From e3c0d3b770f11b01e252d44585acb5eaa9c57ed5 Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Sat, 2 Aug 2025 16:21:27 -0400 Subject: [PATCH] Removed 'reinstall' update strategy option to only have reinstallIfForbidden, cleanup of pre-sync changes, adapted unit tests Signed-off-by: Simon Bouchard --- docs/index.md | 3 +- pkg/app/app.go | 25 +-- pkg/app/app_diff_test.go | 2 +- pkg/app/app_test.go | 31 ++-- pkg/app/run.go | 36 +---- .../log | 10 +- .../log | 10 +- pkg/exectest/helm.go | 26 +++- pkg/state/create.go | 2 +- pkg/state/state.go | 145 +++++++----------- pkg/state/state_test.go | 106 +++++++++++++ 11 files changed, 227 insertions(+), 169 deletions(-) rename pkg/app/testdata/testapply/{install-with-upgrade-with-reinstall => install-with-upgrade-with-reinstallifforbidden}/log (80%) rename pkg/app/testdata/testapply/{install-with-upgrade-with-skip-diff-on-install-with-reinstall => install-with-upgrade-with-skip-diff-on-install-with-reinstallifforbidden}/log (80%) diff --git a/docs/index.md b/docs/index.md index 976f6086..ea883a7e 100644 --- a/docs/index.md +++ b/docs/index.md @@ -326,8 +326,7 @@ releases: reuseValues: false # set `false` to uninstall this release on sync. (default true) installed: true - # Defines the strategy to use when updating. Possible values are: - # - "reinstall": Performs an uninstall before the update, ensuring a clean replacement. + # Defines the strategy to use when updating. Possible value is: # - "reinstallIfForbidden": Performs an uninstall before the update only if the update is forbidden (e.g., due to permission issues or conflicts). updateStrategy: "" # restores previous state in case of failed release (default false) diff --git a/pkg/app/app.go b/pkg/app/app.go index adc6b180..19024459 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1392,7 +1392,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { TakeOwnership: c.TakeOwnership(), } - infoMsg, releasesToBeUpdated, releasesToBeReinstalled, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) + infoMsg, releasesToBeUpdated, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) if len(errs) > 0 { return false, false, errs } @@ -1407,19 +1407,13 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { toUpdate = append(toUpdate, r) } - for _, r := range releasesToBeReinstalled { - toDelete = append(toDelete, r) - toUpdate = append(toUpdate, r) - } - releasesWithNoChange := map[string]state.ReleaseSpec{} for _, r := range toApplyWithNeeds { release := r id := state.ReleaseToID(&release) _, uninstalled := releasesToBeDeleted[id] _, updated := releasesToBeUpdated[id] - _, reinstalled := releasesToBeReinstalled[id] - if !uninstalled && !updated && !reinstalled { + if !uninstalled && !updated { releasesWithNoChange[id] = release } } @@ -1484,7 +1478,7 @@ Do you really want to apply? } // We upgrade releases by traversing the DAG - if len(releasesToBeUpdated) > 0 || len(releasesToBeReinstalled) > 0 { + if len(releasesToBeUpdated) > 0 { _, updateErrs := withDAG(st, helm, a.Logger, state.PlanOptions{SelectedReleases: toUpdate, Reverse: false, SkipNeeds: true, IncludeTransitiveNeeds: c.IncludeTransitiveNeeds()}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { var rs []state.ReleaseSpec @@ -1493,9 +1487,6 @@ Do you really want to apply? if r2, ok := releasesToBeUpdated[state.ReleaseToID(&release)]; ok { rs = append(rs, r2) } - if r2, ok := releasesToBeReinstalled[state.ReleaseToID(&release)]; ok { - rs = append(rs, r2) - } } subst.Releases = rs @@ -1534,7 +1525,7 @@ Do you really want to apply? a.Logger.Warnf("warn: %v\n", err) } } - if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesToBeReinstalled == nil { + if releasesToBeDeleted == nil && releasesToBeUpdated == nil { return true, false, nil } @@ -1618,8 +1609,8 @@ Do you really want to delete? func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) { var ( - infoMsg *string - updated, reinstalled, deleted map[string]state.ReleaseSpec + infoMsg *string + updated, deleted map[string]state.ReleaseSpec ) ok, errs := a.withNeeds(r, c, true, func(st *state.HelmState) []error { @@ -1652,12 +1643,12 @@ func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) ctx: r.ctx, Ask: r.Ask, } - infoMsg, updated, reinstalled, deleted, errs = filtered.diff(true, c.DetailedExitcode(), c, opts) + infoMsg, updated, deleted, errs = filtered.diff(true, c.DetailedExitcode(), c, opts) return errs }) - return infoMsg, ok, len(deleted) > 0 || len(updated) > 0 || len(reinstalled) > 0, errs + return infoMsg, ok, len(deleted) > 0 || len(updated) > 0, errs } func (a *App) lint(r *Run, c LintConfigProvider) (bool, []error, []error) { diff --git a/pkg/app/app_diff_test.go b/pkg/app/app_diff_test.go index 8b104f87..3f353e92 100644 --- a/pkg/app/app_diff_test.go +++ b/pkg/app/app_diff_test.go @@ -423,7 +423,7 @@ releases: - name: a chart: incubator/raw namespace: default - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden - name: b chart: incubator/raw namespace: default diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 773b1914..21a41a3c 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -230,9 +230,9 @@ func TestUpdateStrategyParamValidation(t *testing.T) { "/path/to/helmfile.yaml": `releases: - name: zipkin chart: stable/zipkin - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden `}, - "reinstall", + "reinstallIfForbidden", true}, {map[string]string{ "/path/to/helmfile.yaml": `releases: @@ -3164,10 +3164,10 @@ baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 defau concurrency: 1, }, // - // install with upgrade with reinstall + // install with upgrade with reinstallIfForbidden // { - name: "install-with-upgrade-with-reinstall", + name: "install-with-upgrade-with-reinstallIfForbidden", loc: location(), files: map[string]string{ "/path/to/helmfile.yaml": ` @@ -3175,7 +3175,7 @@ releases: - name: baz chart: stable/mychart3 disableValidationOnInstall: true - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden - name: foo chart: stable/mychart1 disableValidationOnInstall: true @@ -3184,7 +3184,7 @@ releases: - name: bar chart: stable/mychart2 disableValidation: true - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden `, }, diffs: map[exectest.DiffKey]error{ @@ -3206,18 +3206,14 @@ baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 defau {Name: "bar", Flags: []string{"--kube-context", "default"}}, {Name: "foo", Flags: []string{"--kube-context", "default"}}, }, - deleted: []exectest.Release{ - // These releases have updateStrategy=reinstall which will be both uninstalled and installed - {Name: "baz", Flags: []string{}}, - {Name: "bar", Flags: []string{}}, - }, + deleted: []exectest.Release{}, concurrency: 1, }, // - // install with upgrade and --skip-diff-on-install with reinstall + // install with upgrade and --skip-diff-on-install with reinstallIfForbidden // { - name: "install-with-upgrade-with-skip-diff-on-install-with-reinstall", + name: "install-with-upgrade-with-skip-diff-on-install-with-reinstallIfForbidden", loc: location(), skipDiffOnInstall: true, files: map[string]string{ @@ -3226,7 +3222,7 @@ releases: - name: baz chart: stable/mychart3 disableValidationOnInstall: true - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden - name: foo chart: stable/mychart1 disableValidationOnInstall: true @@ -3235,7 +3231,7 @@ releases: - name: bar chart: stable/mychart2 disableValidation: true - updateStrategy: reinstall + updateStrategy: reinstallIfForbidden `, }, diffs: map[exectest.DiffKey]error{ @@ -3256,11 +3252,6 @@ baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 defau {Name: "bar", Flags: []string{"--kube-context", "default"}}, {Name: "foo", Flags: []string{"--kube-context", "default"}}, }, - deleted: []exectest.Release{ - // These releases have updateStrategy=reinstall which will be both uninstalled and installed - {Name: "baz", Flags: []string{}}, - {Name: "bar", Flags: []string{}}, - }, concurrency: 1, }, // diff --git a/pkg/app/run.go b/pkg/app/run.go index f1cc19d6..2beab11a 100644 --- a/pkg/app/run.go +++ b/pkg/app/run.go @@ -137,13 +137,12 @@ func (r *Run) Repos(c ReposConfigProvider) error { return r.ctx.SyncReposOnce(r.state, r.helm) } -func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfigProvider, diffOpts *state.DiffOpts) (*string, map[string]state.ReleaseSpec, map[string]state.ReleaseSpec, map[string]state.ReleaseSpec, []error) { +func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfigProvider, diffOpts *state.DiffOpts) (*string, map[string]state.ReleaseSpec, map[string]state.ReleaseSpec, []error) { st := r.state helm := r.helm var changedReleases []state.ReleaseSpec var deletingReleases []state.ReleaseSpec - var reinstallingReleases []state.ReleaseSpec var planningErrs []error // TODO Better way to detect diff on only filtered releases @@ -155,10 +154,6 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig if err != nil { planningErrs = append(planningErrs, err) } - reinstallingReleases, err = st.DetectReleasesToBeReinstalledForSync(helm, st.Releases) - if err != nil { - planningErrs = append(planningErrs, err) - } } fatalErrs := []error{} @@ -175,7 +170,7 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig } if len(fatalErrs) > 0 { - return nil, nil, nil, nil, fatalErrs + return nil, nil, nil, fatalErrs } releasesToBeDeleted := map[string]state.ReleaseSpec{} @@ -185,14 +180,6 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig releasesToBeDeleted[id] = release } - releasesWithReinstalled := map[string]state.ReleaseSpec{} - for _, r := range reinstallingReleases { - release := r - id := state.ReleaseToID(&release) - releasesWithReinstalled[id] = release - } - - releasesToBeReinstalled := map[string]state.ReleaseSpec{} releasesToBeUpdated := map[string]state.ReleaseSpec{} for _, r := range changedReleases { release := r @@ -200,36 +187,25 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig // If `helm-diff` detected changes but it is not being `helm delete`ed, we should run `helm upgrade` if _, ok := releasesToBeDeleted[id]; !ok { - // Is the release with "reinstall" update strategy - if _, ok := releasesWithReinstalled[id]; ok { - // Make sure we wait on the delete in the case of a reinstall - deleteWait := true - release.DeleteWait = &deleteWait - releasesToBeReinstalled[id] = release - } else { - releasesToBeUpdated[id] = release - } + releasesToBeUpdated[id] = release } } // sync only when there are changes - if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 && len(releasesToBeReinstalled) == 0 { + if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 { var msg *string if c.DetailedExitcode() { // TODO better way to get the logger m := "No affected releases" msg = &m } - return msg, nil, nil, nil, nil + return msg, nil, nil, nil } names := []string{} for _, r := range releasesToBeUpdated { names = append(names, fmt.Sprintf(" %s (%s) UPDATED", r.Name, r.Chart)) } - for _, r := range releasesToBeReinstalled { - names = append(names, fmt.Sprintf(" %s (%s) REINSTALLED", r.Name, r.Chart)) - } for _, r := range releasesToBeDeleted { releaseToBeDeleted := fmt.Sprintf(" %s (%s) DELETED", r.Name, r.Chart) if c.Color() { @@ -244,5 +220,5 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig %s `, strings.Join(names, "\n")) - return &infoMsg, releasesToBeUpdated, releasesToBeReinstalled, releasesToBeDeleted, nil + return &infoMsg, releasesToBeUpdated, releasesToBeDeleted, nil } diff --git a/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstallifforbidden/log similarity index 80% rename from pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log rename to pkg/app/testdata/testapply/install-with-upgrade-with-reinstallifforbidden/log index 39f26151..22fc346b 100644 --- a/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log +++ b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstallifforbidden/log @@ -4,8 +4,8 @@ merged environment: &{default map[] map[]} 3 release(s) found in helmfile.yaml Affected releases are: - bar (stable/mychart2) REINSTALLED - baz (stable/mychart3) REINSTALLED + bar (stable/mychart2) UPDATED + baz (stable/mychart3) UPDATED foo (stable/mychart1) UPDATED invoking preapply hooks for 2 groups of releases in this order: @@ -21,13 +21,15 @@ GROUP RELEASES 2 default//foo processing releases in group 1/2: default//baz, default//bar -reinstallIfForbidden update strategy - reinstalling... -reinstallIfForbidden update strategy - reinstalling... +update strategy - sync success +update strategy - sync success processing releases in group 2/2: default//foo getting deployed release version failed: Failed to get the version for: mychart1 UPDATED RELEASES: NAME NAMESPACE CHART VERSION DURATION +baz stable/mychart3 3.1.0 0s +bar stable/mychart2 3.1.0 0s foo stable/mychart1 0s changing working directory back to "/path/to" diff --git a/pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstall/log b/pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstallifforbidden/log similarity index 80% rename from pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstall/log rename to pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstallifforbidden/log index 39f26151..22fc346b 100644 --- a/pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstall/log +++ b/pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstallifforbidden/log @@ -4,8 +4,8 @@ merged environment: &{default map[] map[]} 3 release(s) found in helmfile.yaml Affected releases are: - bar (stable/mychart2) REINSTALLED - baz (stable/mychart3) REINSTALLED + bar (stable/mychart2) UPDATED + baz (stable/mychart3) UPDATED foo (stable/mychart1) UPDATED invoking preapply hooks for 2 groups of releases in this order: @@ -21,13 +21,15 @@ GROUP RELEASES 2 default//foo processing releases in group 1/2: default//baz, default//bar -reinstallIfForbidden update strategy - reinstalling... -reinstallIfForbidden update strategy - reinstalling... +update strategy - sync success +update strategy - sync success processing releases in group 2/2: default//foo getting deployed release version failed: Failed to get the version for: mychart1 UPDATED RELEASES: NAME NAMESPACE CHART VERSION DURATION +baz stable/mychart3 3.1.0 0s +bar stable/mychart2 3.1.0 0s foo stable/mychart1 0s changing working directory back to "/path/to" diff --git a/pkg/exectest/helm.go b/pkg/exectest/helm.go index 9daaf0ec..6206e86f 100644 --- a/pkg/exectest/helm.go +++ b/pkg/exectest/helm.go @@ -56,9 +56,10 @@ type Release struct { } type Affected struct { - Upgraded []*Release - Deleted []*Release - Failed []*Release + Upgraded []*Release + Reinstalled []*Release + Deleted []*Release + Failed []*Release } func (helm *Helm) UpdateDeps(chart string) error { @@ -107,7 +108,24 @@ func (helm *Helm) RegistryLogin(name, username, password, caFile, certFile, keyF return nil } func (helm *Helm) SyncRelease(context helmexec.HelmContext, name, chart, namespace string, flags ...string) error { - if strings.Contains(name, "error") { + if strings.Contains(name, "forbidden") { + releaseExists := false + for _, release := range helm.Releases { + if release.Name == name { + releaseExists = true + } + } + releaseDeleted := false + for _, release := range helm.Deleted { + if release.Name == name { + releaseDeleted = true + } + } + // Only fail if the release is present in the helm.Releases to simulate a forbidden update if it exists + if releaseExists && !releaseDeleted { + return fmt.Errorf("cannot patch %q with kind StatefulSet: StatefulSet.apps %q is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden", name, name) + } + } else if strings.Contains(name, "error") { return errors.New("error") } helm.sync(helm.ReleasesMutex, func() { diff --git a/pkg/state/create.go b/pkg/state/create.go index 1e18aae9..28244255 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -26,7 +26,7 @@ const ( DefaultHCLFileExtension = ".hcl" ) -var ValidUpdateStrategyValues = []string{UpdateStrategyReinstall, UpdateStrategyReinstallIfForbidden} +var ValidUpdateStrategyValues = []string{UpdateStrategyReinstallIfForbidden} type StateLoadError struct { Msg string diff --git a/pkg/state/state.go b/pkg/state/state.go index 908d5c61..76ce618b 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -45,7 +45,6 @@ const ( EmptyTimeout = -1 // Valid enum for updateStrategy values - UpdateStrategyReinstall = "reinstall" UpdateStrategyReinstallIfForbidden = "reinstallIfForbidden" ) @@ -790,26 +789,6 @@ func (st *HelmState) DetectReleasesToBeDeletedForSync(helm helmexec.Interface, r return deleted, nil } -func (st *HelmState) DetectReleasesToBeReinstalledForSync(helm helmexec.Interface, releases []ReleaseSpec) ([]ReleaseSpec, error) { - reinstalled := []ReleaseSpec{} - for i := range releases { - release := releases[i] - - if release.Desired() && (release.UpdateStrategy == "reinstall" || release.UpdateStrategy == "reinstallIfForbidden") { - installed, err := st.isReleaseInstalled(st.createHelmContext(&release, 0), helm, release) - if err != nil { - return nil, err - } - if installed { - // Otherwise `release` messed up(https://github.com/roboll/helmfile/issues/554) - r := release - reinstalled = append(reinstalled, r) - } - } - } - return reinstalled, nil -} - func (st *HelmState) DetectReleasesToBeDeleted(helm helmexec.Interface, releases []ReleaseSpec) ([]ReleaseSpec, error) { detected := []ReleaseSpec{} for i := range releases { @@ -1061,7 +1040,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } m.Unlock() } - } else if release.UpdateStrategy == "reinstall" || release.UpdateStrategy == "reinstallIfForbidden" { + } else if release.UpdateStrategy == UpdateStrategyReinstallIfForbidden { relErr = st.performSyncOrReinstallOfRelease(affectedReleases, helm, context, release, chart, m, flags...) } else { if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { @@ -1125,77 +1104,71 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } func (st *HelmState) performSyncOrReinstallOfRelease(affectedReleases *AffectedReleases, helm helmexec.Interface, context helmexec.HelmContext, release *ReleaseSpec, chart string, m *sync.Mutex, flags ...string) *ReleaseError { - syncSuccessful := false - if release.UpdateStrategy == "reinstallIfForbidden" { - if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { - st.logger.Debugf("reinstallIfForbidden update strategy - sync failed: %s", err.Error()) - // Only fail if a different error than forbidden updates - if !strings.Contains(err.Error(), "Forbidden: updates") { - st.logger.Debugf("reinstallIfForbidden update strategy - sync failed not due to Fobidden updates") - m.Lock() - affectedReleases.Failed = append(affectedReleases.Failed, release) - m.Unlock() - return newReleaseFailedError(release, err) - } - } else { - st.logger.Debugf("reinstallIfForbidden update strategy - sync success") - syncSuccessful = true - m.Lock() - affectedReleases.Upgraded = append(affectedReleases.Upgraded, release) - m.Unlock() - installedVersion, err := st.getDeployedVersion(context, helm, release) - if err != nil { // err is not really impacting so just log it - st.logger.Debugf("getting deployed release version failed: %v", err) - } else { - release.installedVersion = installedVersion - } - } - } - // Don't attempt to reinstall if a sync was successful or needed a forced reinstall - if !syncSuccessful { - st.logger.Debugf("reinstallIfForbidden update strategy - reinstalling...") - installed, err := st.isReleaseInstalled(context, helm, *release) - if err != nil { - return newReleaseFailedError(release, err) - } - if installed { - var args []string - if release.Namespace != "" { - args = append(args, "--namespace", release.Namespace) - } - args = st.appendDeleteWaitFlags(args, release) - deletionFlags := st.appendConnectionFlags(args, release) - m.Lock() - if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync"); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, release) - return newReleaseFailedError(release, err) - } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, release) - return newReleaseFailedError(release, err) - } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, release) - return newReleaseFailedError(release, err) - } - m.Unlock() - } - if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { + if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { + st.logger.Debugf("update strategy - sync failed: %s", err.Error()) + // Only fail if a different error than forbidden updates + if !strings.Contains(err.Error(), "Forbidden: updates") { + st.logger.Debugf("update strategy - sync failed not due to Fobidden updates") m.Lock() affectedReleases.Failed = append(affectedReleases.Failed, release) m.Unlock() return newReleaseFailedError(release, err) - } else { - m.Lock() - affectedReleases.Reinstalled = append(affectedReleases.Reinstalled, release) - m.Unlock() - installedVersion, err := st.getDeployedVersion(context, helm, release) - if err != nil { // err is not really impacting so just log it - st.logger.Debugf("getting deployed release version failed: %v", err) - } else { - release.installedVersion = installedVersion - } } + } else { + st.logger.Debugf("update strategy - sync success") + m.Lock() + affectedReleases.Upgraded = append(affectedReleases.Upgraded, release) + m.Unlock() + installedVersion, err := st.getDeployedVersion(context, helm, release) + if err != nil { // err is not really impacting so just log it + st.logger.Debugf("update strategy - getting deployed release version failed: %v", err) + } else { + release.installedVersion = installedVersion + } + return nil } + st.logger.Infof("Failed to sync due to forbidden updates, attempting to reinstall %q allowed by update strategy", release.Name) + installed, err := st.isReleaseInstalled(context, helm, *release) + if err != nil { + return newReleaseFailedError(release, err) + } + if installed { + var args []string + if release.Namespace != "" { + args = append(args, "--namespace", release.Namespace) + } + args = st.appendDeleteWaitFlags(args, release) + deletionFlags := st.appendConnectionFlags(args, release) + m.Lock() + if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync"); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, release) + return newReleaseFailedError(release, err) + } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, release) + return newReleaseFailedError(release, err) + } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, release) + return newReleaseFailedError(release, err) + } + m.Unlock() + } + if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { + m.Lock() + affectedReleases.Failed = append(affectedReleases.Failed, release) + m.Unlock() + return newReleaseFailedError(release, err) + } else { + m.Lock() + affectedReleases.Reinstalled = append(affectedReleases.Reinstalled, release) + m.Unlock() + installedVersion, err := st.getDeployedVersion(context, helm, release) + if err != nil { // err is not really impacting so just log it + st.logger.Debugf("update strategy - getting deployed release version failed: %v", err) + } else { + release.installedVersion = installedVersion + } + } return nil } diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 4834ba1c..7bbeab46 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -1618,6 +1618,112 @@ func TestHelmState_SyncReleasesAffectedRealeases(t *testing.T) { } } +func TestHelmState_SyncReleasesAffectedReleasesWithReinstallIfForbidden(t *testing.T) { + no := false + tests := []struct { + name string + releases []ReleaseSpec + installed []bool + wantAffected exectest.Affected + }{ + { + name: "2 new", + releases: []ReleaseSpec{ + { + Name: "releaseNameFoo-forbidden", + Chart: "foo", + UpdateStrategy: "reinstallIfForbidden", + }, + { + Name: "releaseNameBar-forbidden", + Chart: "foo", + UpdateStrategy: "reinstallIfForbidden", + }, + }, + wantAffected: exectest.Affected{ + Upgraded: []*exectest.Release{ + {Name: "releaseNameFoo-forbidden", Flags: []string{}}, + {Name: "releaseNameBar-forbidden", Flags: []string{}}, + }, + Reinstalled: nil, + Deleted: nil, + Failed: nil, + }, + }, + { + name: "1 removed, 1 new, 1 reinstalled first new", + releases: []ReleaseSpec{ + { + Name: "releaseNameFoo-forbidden", + Chart: "foo", + UpdateStrategy: "reinstallIfForbidden", + }, + { + Name: "releaseNameBar", + Chart: "foo", + UpdateStrategy: "reinstallIfForbidden", + Installed: &no, + }, + { + Name: "releaseNameFoo-forbidden", + Chart: "foo", + UpdateStrategy: "reinstallIfForbidden", + }, + }, + installed: []bool{true, true, true}, + wantAffected: exectest.Affected{ + Upgraded: []*exectest.Release{ + {Name: "releaseNameFoo-forbidden", Flags: []string{}}, + }, + Reinstalled: []*exectest.Release{ + {Name: "releaseNameFoo-forbidden", Flags: []string{}}, + }, + Deleted: []*exectest.Release{ + {Name: "releaseNameBar", Flags: []string{}}, + }, + Failed: nil, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + state := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Releases: tt.releases, + }, + logger: logger, + valsRuntime: valsRuntime, + RenderedValues: map[string]any{}, + } + helm := &exectest.Helm{ + Lists: map[exectest.ListKey]string{}, + } + //simulate the release is already installed + for i, release := range tt.releases { + if tt.installed != nil && tt.installed[i] { + helm.Lists[exectest.ListKey{Filter: "^" + release.Name + "$", Flags: "--uninstalling --deployed --failed --pending"}] = release.Name + } + } + + affectedReleases := AffectedReleases{} + if err := state.SyncReleases(&affectedReleases, helm, []string{}, 1); err != nil { + if !testEq(affectedReleases.Failed, tt.wantAffected.Failed) { + t.Errorf("HelmState.SynchAffectedRelease() error failed for [%s] = %v, want %v", tt.name, affectedReleases.Failed, tt.wantAffected.Failed) + } //else expected error + } + if !testEq(affectedReleases.Upgraded, tt.wantAffected.Upgraded) { + t.Errorf("HelmState.SynchAffectedRelease() upgrade failed for [%s] = %v, want %v", tt.name, affectedReleases.Upgraded, tt.wantAffected.Upgraded) + } + if !testEq(affectedReleases.Reinstalled, tt.wantAffected.Reinstalled) { + t.Errorf("HelmState.SynchAffectedRelease() reinstalled failed for [%s] = %v, want %v", tt.name, affectedReleases.Reinstalled, tt.wantAffected.Reinstalled) + } + if !testEq(affectedReleases.Deleted, tt.wantAffected.Deleted) { + t.Errorf("HelmState.SynchAffectedRelease() deleted failed for [%s] = %v, want %v", tt.name, affectedReleases.Deleted, tt.wantAffected.Deleted) + } + }) + } +} + func testEq(a []*ReleaseSpec, b []*exectest.Release) bool { // If one is nil, the other must also be nil. if (a == nil) != (b == nil) {