From ec70bc5b2d8ab522f49326757fe7f61f7827deea Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Thu, 21 Nov 2024 15:58:31 -0500 Subject: [PATCH 01/11] feat: Add updateStrategy option in the state file with 'reinstall'/'reinstallIfForbidden' choices to uninstall and apply the specific release(s) (if forbidden to update) Signed-off-by: Simon Bouchard --- docs/index.md | 2 + pkg/app/app.go | 25 ++-- pkg/app/app_diff_test.go | 24 ++++ pkg/app/app_test.go | 102 ++++++++++++++- pkg/app/run.go | 36 +++++- ...on_changed_selected_release_with_reinstall | 42 ++++++ .../install-with-upgrade-with-reinstall/log | 74 +++++++++++ .../log | 74 +++++++++++ pkg/state/state.go | 120 ++++++++++++++++-- 9 files changed, 472 insertions(+), 27 deletions(-) create mode 100644 pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall create mode 100644 pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log create mode 100644 pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstall/log diff --git a/docs/index.md b/docs/index.md index 211dbf8e..d771d7fb 100644 --- a/docs/index.md +++ b/docs/index.md @@ -329,6 +329,8 @@ releases: reuseValues: false # set `false` to uninstall this release on sync. (default true) installed: true + # when set to "reinstall", an uninstall will be performed before the update + updateStrategy: "" # restores previous state in case of failed release (default false) atomic: true # when true, cleans up any new resources created during a failed release (default false) diff --git a/pkg/app/app.go b/pkg/app/app.go index 65751681..614c9bf7 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) { SuppressOutputLineRegex: c.SuppressOutputLineRegex(), } - infoMsg, releasesToBeUpdated, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) + infoMsg, releasesToBeUpdated, releasesToBeReinstalled, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) if len(errs) > 0 { return false, false, errs } @@ -1407,13 +1407,19 @@ 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] - if !uninstalled && !updated { + _, reinstalled := releasesToBeReinstalled[id] + if !uninstalled && !updated && !reinstalled { releasesWithNoChange[id] = release } } @@ -1478,7 +1484,7 @@ Do you really want to apply? } // We upgrade releases by traversing the DAG - if len(releasesToBeUpdated) > 0 { + if len(releasesToBeUpdated) > 0 || len(releasesToBeReinstalled) > 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 @@ -1487,6 +1493,9 @@ 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 @@ -1525,7 +1534,7 @@ Do you really want to apply? a.Logger.Warnf("warn: %v\n", err) } } - if releasesToBeDeleted == nil && releasesToBeUpdated == nil { + if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesToBeReinstalled == nil { return true, false, nil } @@ -1609,8 +1618,8 @@ Do you really want to delete? func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) { var ( - infoMsg *string - updated, deleted map[string]state.ReleaseSpec + infoMsg *string + updated, reinstalled, deleted map[string]state.ReleaseSpec ) ok, errs := a.withNeeds(r, c, true, func(st *state.HelmState) []error { @@ -1642,12 +1651,12 @@ func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) ctx: r.ctx, Ask: r.Ask, } - infoMsg, updated, deleted, errs = filtered.diff(true, c.DetailedExitcode(), c, opts) + infoMsg, updated, reinstalled, deleted, errs = filtered.diff(true, c.DetailedExitcode(), c, opts) return errs }) - return infoMsg, ok, len(deleted) > 0 || len(updated) > 0, errs + return infoMsg, ok, len(deleted) > 0 || len(updated) > 0 || len(reinstalled) > 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 d9f7b691..8b104f87 100644 --- a/pkg/app/app_diff_test.go +++ b/pkg/app/app_diff_test.go @@ -415,4 +415,28 @@ releases: }, }) }) + + t.Run("show diff on changed selected release with reinstall", func(t *testing.T) { + check(t, testcase{ + helmfile: ` +releases: +- name: a + chart: incubator/raw + namespace: default + updateStrategy: reinstall +- name: b + chart: incubator/raw + namespace: default +`, + selectors: []string{"name=a"}, + lists: map[exectest.ListKey]string{ + {Filter: "^a$", Flags: listFlags("default", "default")}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default +`, + }, + diffed: []exectest.Release{ + {Name: "a", Flags: []string{"--kube-context", "default", "--namespace", "default", "--reset-values"}}, + }, + }) + }) } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 435ed7cf..fb2dd8fd 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -3073,6 +3073,106 @@ 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 + // + { + name: "install-with-upgrade-with-reinstall", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: baz + chart: stable/mychart3 + disableValidationOnInstall: true + updateStrategy: reinstall +- name: foo + chart: stable/mychart1 + disableValidationOnInstall: true + needs: + - bar +- name: bar + chart: stable/mychart2 + disableValidation: true + updateStrategy: reinstall +`, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + {Name: "foo", Chart: "stable/mychart1", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{ + {Filter: "^foo$", Flags: listFlags("", "default")}: ``, + {Filter: "^bar$", Flags: listFlags("", "default")}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +bar 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart2-3.1.0 3.1.0 default +`, + {Filter: "^baz$", Flags: listFlags("", "default")}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 default +`, + }, + upgraded: []exectest.Release{ + {Name: "baz", Flags: []string{"--kube-context", "default"}}, + {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, + }, + // + // install with upgrade and --skip-diff-on-install with reinstall + // + { + name: "install-with-upgrade-with-skip-diff-on-install-with-reinstall", + loc: location(), + skipDiffOnInstall: true, + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: baz + chart: stable/mychart3 + disableValidationOnInstall: true + updateStrategy: reinstall +- name: foo + chart: stable/mychart1 + disableValidationOnInstall: true + needs: + - bar +- name: bar + chart: stable/mychart2 + disableValidation: true + updateStrategy: reinstall +`, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{ + {Filter: "^foo$", Flags: listFlags("", "default")}: ``, + {Filter: "^bar$", Flags: listFlags("", "default")}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +bar 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart2-3.1.0 3.1.0 default +`, + {Filter: "^baz$", Flags: listFlags("", "default")}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 default +`, + }, + upgraded: []exectest.Release{ + {Name: "baz", Flags: []string{"--kube-context", "default"}}, + {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, + }, + // // upgrades // { @@ -3769,7 +3869,7 @@ releases: } for flagIdx := range wantDeletes[relIdx].Flags { if wantDeletes[relIdx].Flags[flagIdx] != helm.Deleted[relIdx].Flags[flagIdx] { - t.Errorf("releaes[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Deleted[relIdx].Flags[flagIdx], wantDeletes[relIdx].Flags[flagIdx]) + t.Errorf("releases[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Deleted[relIdx].Flags[flagIdx], wantDeletes[relIdx].Flags[flagIdx]) } } } diff --git a/pkg/app/run.go b/pkg/app/run.go index 2beab11a..f1cc19d6 100644 --- a/pkg/app/run.go +++ b/pkg/app/run.go @@ -137,12 +137,13 @@ 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, []error) { +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) { 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 @@ -154,6 +155,10 @@ 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{} @@ -170,7 +175,7 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig } if len(fatalErrs) > 0 { - return nil, nil, nil, fatalErrs + return nil, nil, nil, nil, fatalErrs } releasesToBeDeleted := map[string]state.ReleaseSpec{} @@ -180,6 +185,14 @@ 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 @@ -187,25 +200,36 @@ 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 { - releasesToBeUpdated[id] = release + // 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 + } } } // sync only when there are changes - if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 { + if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 && len(releasesToBeReinstalled) == 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 + return msg, nil, 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() { @@ -220,5 +244,5 @@ func (r *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfig %s `, strings.Join(names, "\n")) - return &infoMsg, releasesToBeUpdated, releasesToBeDeleted, nil + return &infoMsg, releasesToBeUpdated, releasesToBeReinstalled, releasesToBeDeleted, nil } diff --git a/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall new file mode 100644 index 00000000..0d9d039d --- /dev/null +++ b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall @@ -0,0 +1,42 @@ +processing file "helmfile.yaml" in directory "." +changing working directory to "/path/to" +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: namespace: default + 5: updateStrategy: reinstall + 6: - name: b + 7: chart: incubator/raw + 8: namespace: default + 9: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: namespace: default + 5: updateStrategy: reinstall + 6: - name: b + 7: chart: incubator/raw + 8: namespace: default + 9: + +merged environment: &{default map[] map[]} +1 release(s) matching name=a found in helmfile.yaml + +processing 1 groups of releases in this order: +GROUP RELEASES +1 default/default/a + +processing releases in group 1/1: default/default/a +changing working directory back to "/path/to" diff --git a/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log new file mode 100644 index 00000000..4967a798 --- /dev/null +++ b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log @@ -0,0 +1,74 @@ +processing file "helmfile.yaml" in directory "." +changing working directory to "/path/to" +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: updateStrategy: reinstall + 6: - name: foo + 7: chart: stable/mychart1 + 8: disableValidationOnInstall: true + 9: needs: +10: - bar +11: - name: bar +12: chart: stable/mychart2 +13: disableValidation: true +14: updateStrategy: reinstall +15: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: updateStrategy: reinstall + 6: - name: foo + 7: chart: stable/mychart1 + 8: disableValidationOnInstall: true + 9: needs: +10: - bar +11: - name: bar +12: chart: stable/mychart2 +13: disableValidation: true +14: updateStrategy: reinstall +15: + +merged environment: &{default map[] map[]} +3 release(s) found in helmfile.yaml + +Affected releases are: + bar (stable/mychart2) REINSTALLED + baz (stable/mychart3) REINSTALLED + foo (stable/mychart1) UPDATED + +invoking preapply hooks for 2 groups of releases in this order: +GROUP RELEASES +1 default//foo +2 default//baz, default//bar + +invoking preapply hooks for releases in group 1/2: default//foo +invoking preapply hooks for releases in group 2/2: default//baz, default//bar +processing 2 groups of releases in this order: +GROUP RELEASES +1 default//baz, default//bar +2 default//foo + +processing releases in group 1/2: default//baz, default//bar +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 +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-reinstall/log new file mode 100644 index 00000000..4967a798 --- /dev/null +++ b/pkg/app/testdata/testapply/install-with-upgrade-with-skip-diff-on-install-with-reinstall/log @@ -0,0 +1,74 @@ +processing file "helmfile.yaml" in directory "." +changing working directory to "/path/to" +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: updateStrategy: reinstall + 6: - name: foo + 7: chart: stable/mychart1 + 8: disableValidationOnInstall: true + 9: needs: +10: - bar +11: - name: bar +12: chart: stable/mychart2 +13: disableValidation: true +14: updateStrategy: reinstall +15: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: updateStrategy: reinstall + 6: - name: foo + 7: chart: stable/mychart1 + 8: disableValidationOnInstall: true + 9: needs: +10: - bar +11: - name: bar +12: chart: stable/mychart2 +13: disableValidation: true +14: updateStrategy: reinstall +15: + +merged environment: &{default map[] map[]} +3 release(s) found in helmfile.yaml + +Affected releases are: + bar (stable/mychart2) REINSTALLED + baz (stable/mychart3) REINSTALLED + foo (stable/mychart1) UPDATED + +invoking preapply hooks for 2 groups of releases in this order: +GROUP RELEASES +1 default//foo +2 default//baz, default//bar + +invoking preapply hooks for releases in group 1/2: default//foo +invoking preapply hooks for releases in group 2/2: default//baz, default//bar +processing 2 groups of releases in this order: +GROUP RELEASES +1 default//baz, default//bar +2 default//foo + +processing releases in group 1/2: default//baz, default//bar +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 +foo stable/mychart1 0s + +changing working directory back to "/path/to" diff --git a/pkg/state/state.go b/pkg/state/state.go index ef93ce67..cfb41e45 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -273,6 +273,8 @@ type ReleaseSpec struct { Force *bool `yaml:"force,omitempty"` // Installed, when set to true, `delete --purge` the release Installed *bool `yaml:"installed,omitempty"` + // UpdateStrategy, when set, indicate the strategy to use to update the release + UpdateStrategy string `yaml:"updateStrategy,omitempty"` // Atomic, when set to true, restore previous state in case of a failed install/upgrade attempt Atomic *bool `yaml:"atomic,omitempty"` // CleanupOnFail, when set to true, the --cleanup-on-fail helm flag is passed to the upgrade command @@ -457,6 +459,7 @@ type SetValue struct { // AffectedReleases hold the list of released that where updated, deleted, or in error type AffectedReleases struct { Upgraded []*ReleaseSpec + Reinstalled []*ReleaseSpec Deleted []*ReleaseSpec Failed []*ReleaseSpec DeleteFailed []*ReleaseSpec @@ -775,6 +778,26 @@ 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 { @@ -1026,20 +1049,93 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } m.Unlock() } - } else if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { - m.Lock() - affectedReleases.Failed = append(affectedReleases.Failed, release) - m.Unlock() - relErr = newReleaseFailedError(release, err) + } else if release.UpdateStrategy == "reinstall" || release.UpdateStrategy == "reinstallIfForbidden" { + 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() + relErr = 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 + if !syncSuccessful { + st.logger.Debugf("reinstallIfForbidden update strategy - reinstalling...") + installed, err := st.isReleaseInstalled(context, helm, *release) + if err != nil { + relErr = 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) + relErr = newReleaseFailedError(release, err) + } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, release) + relErr = newReleaseFailedError(release, err) + } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, release) + relErr = 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() + relErr = 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 { - 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) + if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { + m.Lock() + affectedReleases.Failed = append(affectedReleases.Failed, release) + m.Unlock() + relErr = newReleaseFailedError(release, err) } else { - release.installedVersion = installedVersion + 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 + } } } From 98eb14c0243f147d3399daecd92538281bddf49d Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Mon, 14 Apr 2025 21:37:41 -0400 Subject: [PATCH 02/11] Fix unit tests related to the new updateStrategy feature Signed-off-by: Simon Bouchard --- ...on_changed_selected_release_with_reinstall | 31 ------------- .../install-with-upgrade-with-reinstall/log | 45 +------------------ .../log | 45 +------------------ pkg/state/temp_test.go | 12 ++--- 4 files changed, 10 insertions(+), 123 deletions(-) diff --git a/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall index 0d9d039d..66b419fa 100644 --- a/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall +++ b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release_with_reinstall @@ -1,36 +1,5 @@ processing file "helmfile.yaml" in directory "." changing working directory to "/path/to" -first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= -first-pass uses: &{default map[] map[]} -first-pass rendering output of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: a - 3: chart: incubator/raw - 4: namespace: default - 5: updateStrategy: reinstall - 6: - name: b - 7: chart: incubator/raw - 8: namespace: default - 9: - -first-pass produced: &{default map[] map[]} -first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} -vals: -map[] -defaultVals:[] -second-pass rendering result of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: a - 3: chart: incubator/raw - 4: namespace: default - 5: updateStrategy: reinstall - 6: - name: b - 7: chart: incubator/raw - 8: namespace: default - 9: - merged environment: &{default map[] map[]} 1 release(s) matching name=a found in helmfile.yaml diff --git a/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log index 4967a798..39f26151 100644 --- a/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log +++ b/pkg/app/testdata/testapply/install-with-upgrade-with-reinstall/log @@ -1,48 +1,5 @@ processing file "helmfile.yaml" in directory "." changing working directory to "/path/to" -first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= -first-pass uses: &{default map[] map[]} -first-pass rendering output of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: baz - 3: chart: stable/mychart3 - 4: disableValidationOnInstall: true - 5: updateStrategy: reinstall - 6: - name: foo - 7: chart: stable/mychart1 - 8: disableValidationOnInstall: true - 9: needs: -10: - bar -11: - name: bar -12: chart: stable/mychart2 -13: disableValidation: true -14: updateStrategy: reinstall -15: - -first-pass produced: &{default map[] map[]} -first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} -vals: -map[] -defaultVals:[] -second-pass rendering result of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: baz - 3: chart: stable/mychart3 - 4: disableValidationOnInstall: true - 5: updateStrategy: reinstall - 6: - name: foo - 7: chart: stable/mychart1 - 8: disableValidationOnInstall: true - 9: needs: -10: - bar -11: - name: bar -12: chart: stable/mychart2 -13: disableValidation: true -14: updateStrategy: reinstall -15: - merged environment: &{default map[] map[]} 3 release(s) found in helmfile.yaml @@ -64,6 +21,8 @@ GROUP RELEASES 2 default//foo processing releases in group 1/2: default//baz, default//bar +reinstallIfForbidden update strategy - reinstalling... +reinstallIfForbidden update strategy - reinstalling... processing releases in group 2/2: default//foo getting deployed release version failed: Failed to get the version for: mychart1 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-reinstall/log index 4967a798..39f26151 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-reinstall/log @@ -1,48 +1,5 @@ processing file "helmfile.yaml" in directory "." changing working directory to "/path/to" -first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= -first-pass uses: &{default map[] map[]} -first-pass rendering output of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: baz - 3: chart: stable/mychart3 - 4: disableValidationOnInstall: true - 5: updateStrategy: reinstall - 6: - name: foo - 7: chart: stable/mychart1 - 8: disableValidationOnInstall: true - 9: needs: -10: - bar -11: - name: bar -12: chart: stable/mychart2 -13: disableValidation: true -14: updateStrategy: reinstall -15: - -first-pass produced: &{default map[] map[]} -first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} -vals: -map[] -defaultVals:[] -second-pass rendering result of "helmfile.yaml.part.0": - 0: - 1: releases: - 2: - name: baz - 3: chart: stable/mychart3 - 4: disableValidationOnInstall: true - 5: updateStrategy: reinstall - 6: - name: foo - 7: chart: stable/mychart1 - 8: disableValidationOnInstall: true - 9: needs: -10: - bar -11: - name: bar -12: chart: stable/mychart2 -13: disableValidation: true -14: updateStrategy: reinstall -15: - merged environment: &{default map[] map[]} 3 release(s) found in helmfile.yaml @@ -64,6 +21,8 @@ GROUP RELEASES 2 default//foo processing releases in group 1/2: default//baz, default//bar +reinstallIfForbidden update strategy - reinstalling... +reinstallIfForbidden update strategy - reinstalling... processing releases in group 2/2: default//foo getting deployed release version failed: Failed to get the version for: mychart1 diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index f0eed882..5b0d4d13 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-54f5f6cdb5", + want: "foo-values-788ff4f7f7", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-6bc8f7944b", + want: "foo-values-757ff75d94", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-dcffdcb8", + want: "foo-values-7b89b8c7cb", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-6d4c6fd548", + want: "foo-values-5fb75677fd", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-76974767c8", + want: "bar-values-847b48bc58", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-77bd9cc6fb", + want: "myns-foo-values-6fff5b8664", }) for id, n := range ids { From 0e23aefe10f670a52cf8791d46d72e16cf720ada Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Thu, 24 Apr 2025 18:58:16 -0400 Subject: [PATCH 03/11] Fix unit tests related to the new updateStrategy feature Signed-off-by: Simon Bouchard --- pkg/app/app_test.go | 10 +++++----- pkg/state/temp_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index fb2dd8fd..8fd985bc 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -3097,9 +3097,9 @@ releases: `, }, diffs: map[exectest.DiffKey]error{ - {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, - {Name: "foo", Chart: "stable/mychart1", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, - {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --reset-values --detailed-exitcode"}: helmexec.ExitError{Code: 2}, + {Name: "foo", Chart: "stable/mychart1", Flags: "--disable-validation --kube-context default --reset-values --detailed-exitcode"}: helmexec.ExitError{Code: 2}, + {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --reset-values --detailed-exitcode"}: helmexec.ExitError{Code: 2}, }, lists: map[exectest.ListKey]string{ {Filter: "^foo$", Flags: listFlags("", "default")}: ``, @@ -3148,8 +3148,8 @@ releases: `, }, diffs: map[exectest.DiffKey]error{ - {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, - {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --detailed-exitcode --reset-values"}: helmexec.ExitError{Code: 2}, + {Name: "baz", Chart: "stable/mychart3", Flags: "--kube-context default --reset-values --detailed-exitcode"}: helmexec.ExitError{Code: 2}, + {Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation --kube-context default --reset-values --detailed-exitcode"}: helmexec.ExitError{Code: 2}, }, lists: map[exectest.ListKey]string{ {Filter: "^foo$", Flags: listFlags("", "default")}: ``, diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index 5b0d4d13..03da0c09 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-788ff4f7f7", + want: "foo-values-57cf65699f", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-757ff75d94", + want: "foo-values-78ccf5c7cb", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-7b89b8c7cb", + want: "foo-values-f94dd98bb", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-5fb75677fd", + want: "foo-values-6d977c9b54", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-847b48bc58", + want: "bar-values-5bc44889dd", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-6fff5b8664", + want: "myns-foo-values-589f97d65d", }) for id, n := range ids { From aaef5c98f8aea0ce77c6e903438a4febdf8e13d8 Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Fri, 25 Apr 2025 15:51:50 -0400 Subject: [PATCH 04/11] Resolve linter issue due to cognitive complexity Signed-off-by: Simon Bouchard --- pkg/state/state.go | 146 +++++++++++++++++++++++---------------------- 1 file changed, 76 insertions(+), 70 deletions(-) diff --git a/pkg/state/state.go b/pkg/state/state.go index cfb41e45..8216b806 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1050,76 +1050,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme m.Unlock() } } else if release.UpdateStrategy == "reinstall" || release.UpdateStrategy == "reinstallIfForbidden" { - 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() - relErr = 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 - if !syncSuccessful { - st.logger.Debugf("reinstallIfForbidden update strategy - reinstalling...") - installed, err := st.isReleaseInstalled(context, helm, *release) - if err != nil { - relErr = 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) - relErr = newReleaseFailedError(release, err) - } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, release) - relErr = newReleaseFailedError(release, err) - } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, release) - relErr = 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() - relErr = 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 - } - } - } + relErr = st.performSyncOrReinstallOfRelease(affectedReleases, helm, context, release, chart, m, flags...) } else { if err := helm.SyncRelease(context, release.Name, chart, release.Namespace, flags...); err != nil { m.Lock() @@ -1181,6 +1112,81 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme return nil } +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 { + 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 + } + } + } + + return nil +} + func (st *HelmState) listReleases(context helmexec.HelmContext, helm helmexec.Interface, release *ReleaseSpec) (string, error) { flags := st.kubeConnectionFlags(release) if release.Namespace != "" { From 89e98c4c71e17f64e8e51bf6af1eee53981b8d1a Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Mon, 28 Apr 2025 09:42:42 -0400 Subject: [PATCH 05/11] Updated index.md to describe the possible values of updateStrategy Signed-off-by: Simon Bouchard --- docs/index.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/index.md b/docs/index.md index d771d7fb..95937a9d 100644 --- a/docs/index.md +++ b/docs/index.md @@ -329,7 +329,9 @@ releases: reuseValues: false # set `false` to uninstall this release on sync. (default true) installed: true - # when set to "reinstall", an uninstall will be performed before the update + # Defines the strategy to use when updating. Possible values are: + # - "reinstall": Performs an uninstall before the update, ensuring a clean replacement. + # - "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) atomic: true From c10bb1c4e03156d0923ab88860cc4e28e85ce4cc Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Tue, 29 Jul 2025 18:02:20 -0400 Subject: [PATCH 06/11] Add validation of updateStrategy parameter and unit test Signed-off-by: Simon Bouchard --- pkg/app/app_test.go | 91 ++++++++++++++++++++++++++++ pkg/app/desired_state_file_loader.go | 13 ++++ pkg/state/create.go | 10 +++ pkg/state/state.go | 4 ++ 4 files changed, 118 insertions(+) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 5380f225..b935c2a8 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -220,6 +220,97 @@ releases: } } +func TestUpdateStrategyParamValidation(t *testing.T) { + cases := []struct { + files map[string]string + updateStrategy string + isValid bool + }{ + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: reinstall +`}, + "reinstall", + true}, + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: reinstallIfForbidden +`}, + "reinstallIfForbidden", + true}, + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: +`}, + "", + true}, + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: foo +`}, + "foo", + false}, + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: reinstal +`}, + "reinstal", + false}, + {map[string]string{ + "/path/to/helmfile.yaml": `releases: +- name: zipkin + chart: stable/zipkin + updateStrategy: reinstall1 +`}, + "reinstall1", + false}, + } + + for idx, c := range cases { + fs := testhelper.NewTestFs(c.files) + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + Logger: newAppTestLogger(), + Namespace: "", + Env: "default", + FileOrDir: "helmfile.yaml", + } + + expectNoCallsToHelm(app) + + app = injectFs(app, fs) + + err := app.ForEachState( + Noop, + false, + SetFilter(true), + ) + + if c.isValid && err != nil { + t.Errorf("[case: %d] Unexpected error for valid case: %v", idx, err) + } else if !c.isValid { + var invalidUpdateStrategy state.InvalidUpdateStrategyError + invalidUpdateStrategy.UpdateStrategy = c.updateStrategy + if err == nil { + t.Errorf("[case: %d] Expected error for invalid case", idx) + } else if !strings.Contains(err.Error(), invalidUpdateStrategy.Error()) { + t.Errorf("[case: %d] Unexpected error returned for invalid case\ngot: %v\nexpected underlying error: %s", idx, err, invalidUpdateStrategy.Error()) + } + } + } +} + func TestVisitDesiredStatesWithReleasesFiltered_Issue1008_MissingNonDefaultEnvInBase(t *testing.T) { files := map[string]string{ "/path/to/base.yaml": ` diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 90986023..3e149ab1 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "path/filepath" + "slices" "dario.cat/mergo" "github.com/helmfile/vals" @@ -285,6 +286,18 @@ func (ld *desiredStateLoader) load(env, overrodeEnv *environment.Environment, ba } } + // Validate updateStrategy value if set in the releases + for i := range finalState.Releases { + if finalState.Releases[i].UpdateStrategy != "" { + if !slices.Contains(state.ValidUpdateStrategyValues, finalState.Releases[i].UpdateStrategy) { + return nil, &state.StateLoadError{ + Msg: fmt.Sprintf("failed to read %s", finalState.FilePath), + Cause: &state.InvalidUpdateStrategyError{UpdateStrategy: finalState.Releases[i].UpdateStrategy}, + } + } + } + } + finalState.OrginReleases = finalState.Releases return finalState, nil } diff --git a/pkg/state/create.go b/pkg/state/create.go index b175faf9..7874b9dc 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -26,6 +26,8 @@ const ( DefaultHCLFileExtension = ".hcl" ) +var ValidUpdateStrategyValues = []string{UpdateStrategyReinstall, UpdateStrategyReinstallIfForbidden} + type StateLoadError struct { Msg string Cause error @@ -43,6 +45,14 @@ func (e *UndefinedEnvError) Error() string { return fmt.Sprintf("environment \"%s\" is not defined", e.Env) } +type InvalidUpdateStrategyError struct { + UpdateStrategy string +} + +func (e *InvalidUpdateStrategyError) Error() string { + return fmt.Sprintf("updateStrategy %q is invalid, valid values are: %s or not set", e.UpdateStrategy, strings.Join(ValidUpdateStrategyValues, ", ")) +} + type StateCreator struct { logger *zap.SugaredLogger diff --git a/pkg/state/state.go b/pkg/state/state.go index 9b121851..d451105e 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -43,6 +43,10 @@ const ( // This is used by an interim solution to make the urfave/cli command report to the helmfile internal about that the // --timeout flag is missingl EmptyTimeout = -1 + + // Valid enum for updateStrategy values + UpdateStrategyReinstall = "reinstall" + UpdateStrategyReinstallIfForbidden = "reinstallIfForbidden" ) // ReleaseSetSpec is release set spec From 5d100dfbef03b5c0fc49cbc25db8d4415a3dc61e Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Tue, 29 Jul 2025 19:50:16 -0400 Subject: [PATCH 07/11] Updated unit test Signed-off-by: Simon Bouchard --- pkg/state/temp_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index 8cfc3c10..7e082d51 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-5859bbd547", + want: "foo-values-67dc97cbcb", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-56876c54b8", + want: "foo-values-75d7c4758c", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-5ccd486f8b", + want: "foo-values-685f8cf685", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-6f477bcd58", + want: "foo-values-75597d9c57", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-86d77b4c7b", + want: "bar-values-7b77df65ff", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-68d8d6f46", + want: "myns-foo-values-85f979545c", }) for id, n := range ids { From e3c0d3b770f11b01e252d44585acb5eaa9c57ed5 Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Sat, 2 Aug 2025 16:21:27 -0400 Subject: [PATCH 08/11] 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) { From 94de61b9511c80eec2daec88afe5fc645c54e0cd Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Sat, 2 Aug 2025 16:47:09 -0400 Subject: [PATCH 09/11] Display affected releases that were reinstalled Signed-off-by: Simon Bouchard --- pkg/state/state.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/state/state.go b/pkg/state/state.go index 76ce618b..d4b98f73 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -3735,6 +3735,28 @@ func (ar *AffectedReleases) DisplayAffectedReleases(logger *zap.SugaredLogger) { } logger.Info(tbl.String()) } + if len(ar.Reinstalled) > 0 { + logger.Info("\nREINSTALLED RELEASES:") + tbl, _ := prettytable.NewTable(prettytable.Column{Header: "NAME"}, + prettytable.Column{Header: "NAMESPACE", MinWidth: 6}, + prettytable.Column{Header: "CHART", MinWidth: 6}, + prettytable.Column{Header: "VERSION", MinWidth: 6}, + prettytable.Column{Header: "DURATION", AlignRight: true}, + ) + tbl.Separator = " " + for _, release := range ar.Reinstalled { + modifiedChart, modErr := hideChartCredentials(release.Chart) + if modErr != nil { + logger.Warn("Could not modify chart credentials, %v", modErr) + continue + } + err := tbl.AddRow(release.Name, release.Namespace, modifiedChart, release.installedVersion, release.duration.Round(time.Second)) + if err != nil { + logger.Warn("Could not add row, %v", err) + } + } + logger.Info(tbl.String()) + } if len(ar.Deleted) > 0 { logger.Info("\nDELETED RELEASES:") tbl, _ := prettytable.NewTable(prettytable.Column{Header: "NAME"}, From eca333c0eb98f0df75f50242a6f377da600dc990 Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Tue, 5 Aug 2025 11:24:26 -0400 Subject: [PATCH 10/11] Make sure to add --wait when deleting a release to be reinstalled due to reinstallIfForbidden Signed-off-by: Simon Bouchard --- pkg/state/state.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/state/state.go b/pkg/state/state.go index d4b98f73..ce5a87cf 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1138,6 +1138,8 @@ func (st *HelmState) performSyncOrReinstallOfRelease(affectedReleases *AffectedR if release.Namespace != "" { args = append(args, "--namespace", release.Namespace) } + deleteWaitFlag := true + release.DeleteWait = &deleteWaitFlag args = st.appendDeleteWaitFlags(args, release) deletionFlags := st.appendConnectionFlags(args, release) m.Lock() From 422ac1ecef4eaa62e3d0defb2c1863c237b7bcd8 Mon Sep 17 00:00:00 2001 From: Simon Bouchard Date: Fri, 19 Sep 2025 20:19:41 -0400 Subject: [PATCH 11/11] Apply suggestions from Copilot code review Signed-off-by: Simon Bouchard --- pkg/state/state.go | 2 +- pkg/state/state_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/state/state.go b/pkg/state/state.go index 95461dbc..8a3fbc4d 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1111,7 +1111,7 @@ func (st *HelmState) performSyncOrReinstallOfRelease(affectedReleases *AffectedR 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") + st.logger.Debugf("update strategy - sync failed not due to Forbidden updates") m.Lock() affectedReleases.Failed = append(affectedReleases.Failed, release) m.Unlock() diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index d651baa6..a7c1da9d 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -1730,17 +1730,17 @@ func TestHelmState_SyncReleasesAffectedReleasesWithReinstallIfForbidden(t *testi 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) + t.Errorf("HelmState.SyncReleases() 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) + t.Errorf("HelmState.SyncReleases() 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) + t.Errorf("HelmState.SyncReleases() 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) + t.Errorf("HelmState.SyncReleases() deleted failed for [%s] = %v, want %v", tt.name, affectedReleases.Deleted, tt.wantAffected.Deleted) } }) }