From 8d7c79a323b65a89b8e9bcb78d545ebf188e738a Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 7 Nov 2019 10:05:30 +0900 Subject: [PATCH 1/4] fix: release not found on uninstall through sync/apply The recent addition of the DAG support(`needs`) and the fixes on it broke the delete-on-sync functionality. And there were two more bugs. One is that it was not correctly running `helm delete` when needed and the another is that it was failing when `--selector` is specified and the releases to delete by sync found, but nothing actually got deleted. This fixes all of them. Fixes #941 --- pkg/app/app.go | 123 +++++++++++++++++++++++++++------------- pkg/state/state.go | 44 ++++++++------ pkg/state/state_test.go | 12 ++-- 3 files changed, 116 insertions(+), 63 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index e275ba09..d21efc16 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -701,7 +701,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, []error) { changedReleases, planningErrs = st.DiffReleases(helm, c.Values(), c.Concurrency(), detailedExitCode, c.SuppressSecrets(), false, diffOpts) var err error - deletingReleases, err = st.DetectReleasesToBeDeleted(helm, st.Releases) + deletingReleases, err = st.DetectReleasesToBeDeletedForSync(helm, st.Releases) if err != nil { planningErrs = append(planningErrs, err) } @@ -835,34 +835,38 @@ func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error affectedReleases := state.AffectedReleases{} - var deletingReleases []state.ReleaseSpec + var toSync []state.ReleaseSpec if len(st.Selectors) > 0 { var err error - deletingReleases, err = st.GetFilteredReleases() + toSync, err = st.GetFilteredReleases() if err != nil { return false, []error{err} } - if len(deletingReleases) == 0 { + if len(toSync) == 0 { return false, nil } } else { - deletingReleases = st.Releases + toSync = st.Releases } - releasesToBeDeleted := map[string]state.ReleaseSpec{} - for _, r := range deletingReleases { + toDelete, err := st.DetectReleasesToBeDeleted(helm, toSync) + if err != nil { + return false, []error{err} + } + + releasesToDelete := map[string]state.ReleaseSpec{} + for _, r := range toDelete { id := state.ReleaseToID(&r) - releasesToBeDeleted[id] = r + releasesToDelete[id] = r } - names := make([]string, len(deletingReleases)) - for i, r := range deletingReleases { + names := make([]string, len(toSync)) + for i, r := range toSync { names[i] = fmt.Sprintf(" %s (%s)", r.Name, r.Chart) } var errs []error - var any bool msg := fmt.Sprintf(`Affected releases are: %s @@ -875,12 +879,12 @@ Do you really want to delete? if !interactive || interactive && r.askForConfirmation(msg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) - if len(releasesToBeDeleted) > 0 { - deleted, deletionErrs := withDAG(st, helm, a.Logger, true, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { + if len(releasesToDelete) > 0 { + _, deletionErrs := withDAG(st, helm, a.Logger, true, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { var rs []state.ReleaseSpec for _, r := range subst.Releases { - if _, ok := releasesToBeDeleted[state.ReleaseToID(&r)]; ok { + if _, ok := releasesToDelete[state.ReleaseToID(&r)]; ok { rs = append(rs, r) } } @@ -890,15 +894,13 @@ Do you really want to delete? return subst.DeleteReleases(&affectedReleases, helm, c.Concurrency(), purge) })) - any = any || deleted - if deletionErrs != nil && len(deletionErrs) > 0 { errs = append(errs, deletionErrs...) } } } affectedReleases.DisplayAffectedReleases(c.Logger()) - return any, errs + return true, errs } func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { @@ -919,43 +921,91 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { return false, errs } - var syncedReleases []state.ReleaseSpec + var toSync []state.ReleaseSpec if len(st.Selectors) > 0 { var err error - syncedReleases, err = st.GetFilteredReleases() + toSync, err = st.GetFilteredReleases() if err != nil { return false, []error{err} } - if len(syncedReleases) == 0 { + if len(toSync) == 0 { return false, nil } } else { - syncedReleases = st.Releases + toSync = st.Releases } - releasesToBeSynced := map[string]state.ReleaseSpec{} - for _, r := range syncedReleases { + toDelete, err := st.DetectReleasesToBeDeletedForSync(helm, toSync) + if err != nil { + return false, []error{err} + } + + releasesToDelete := map[string]state.ReleaseSpec{} + for _, r := range toDelete { id := state.ReleaseToID(&r) - releasesToBeSynced[id] = r + releasesToDelete[id] = r } - names := make([]string, len(syncedReleases)) - for i, r := range syncedReleases { - names[i] = fmt.Sprintf(" %s (%s)", r.Name, r.Chart) + var toUpdate []state.ReleaseSpec + for _, r := range toSync { + if _, deleted := releasesToDelete[state.ReleaseToID(&r)]; !deleted { + toUpdate = append(toUpdate, r) + } } + releasesToUpdate := map[string]state.ReleaseSpec{} + for _, r := range toUpdate { + id := state.ReleaseToID(&r) + releasesToUpdate[id] = r + } + + names := []string{} + for _, r := range releasesToUpdate { + names = append(names, fmt.Sprintf(" %s (%s) UPDATED", r.Name, r.Chart)) + } + for _, r := range releasesToDelete { + names = append(names, fmt.Sprintf(" %s (%s) DELETED", r.Name, r.Chart)) + } + // Make the output deterministic for testing purpose + sort.Strings(names) + + infoMsg := fmt.Sprintf(`Affected releases are: +%s +`, strings.Join(names, "\n")) + + a.Logger.Info(infoMsg) + var errs []error - var any bool r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) - if len(releasesToBeSynced) > 0 { - synced, syncErrs := withDAG(st, helm, a.Logger, false, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { + if len(releasesToDelete) > 0 { + _, deletionErrs := withDAG(st, helm, a.Logger, true, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { var rs []state.ReleaseSpec for _, r := range subst.Releases { - if _, ok := releasesToBeSynced[state.ReleaseToID(&r)]; ok { + if _, ok := releasesToDelete[state.ReleaseToID(&r)]; ok { + rs = append(rs, r) + } + } + + subst.Releases = rs + + return subst.DeleteReleasesForSync(&affectedReleases, helm, c.Concurrency()) + })) + + if deletionErrs != nil && len(deletionErrs) > 0 { + errs = append(errs, deletionErrs...) + } + } + + if len(releasesToUpdate) > 0 { + _, syncErrs := withDAG(st, helm, a.Logger, false, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { + var rs []state.ReleaseSpec + + for _, r := range subst.Releases { + if _, ok := releasesToUpdate[state.ReleaseToID(&r)]; ok { rs = append(rs, r) } } @@ -968,14 +1018,12 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { return subst.SyncReleases(&affectedReleases, helm, c.Values(), c.Concurrency(), opts) })) - any = any || synced - if syncErrs != nil && len(syncErrs) > 0 { errs = append(errs, syncErrs...) } } affectedReleases.DisplayAffectedReleases(c.Logger()) - return any, errs + return true, errs } func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { @@ -1022,10 +1070,9 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { } var errs []error - var any bool if len(releasesToBeTemplated) > 0 { - synced, templateErrs := withDAG(st, helm, a.Logger, false, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { + _, templateErrs := withDAG(st, helm, a.Logger, false, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { var rs []state.ReleaseSpec for _, r := range subst.Releases { @@ -1043,13 +1090,11 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { return subst.TemplateReleases(helm, c.OutputDir(), c.Values(), args, c.Concurrency(), opts) })) - any = any || synced - if templateErrs != nil && len(templateErrs) > 0 { errs = append(errs, templateErrs...) } } - return any, errs + return true, errs } func fileExistsAt(path string) bool { diff --git a/pkg/state/state.go b/pkg/state/state.go index 2f0d781e..5a41241a 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -381,7 +381,7 @@ func (st *HelmState) isReleaseInstalled(context helmexec.HelmContext, helm helme return false, nil } -func (st *HelmState) DetectReleasesToBeDeleted(helm helmexec.Interface, releases []ReleaseSpec) ([]ReleaseSpec, error) { +func (st *HelmState) DetectReleasesToBeDeletedForSync(helm helmexec.Interface, releases []ReleaseSpec) ([]ReleaseSpec, error) { detected := []ReleaseSpec{} for i := range releases { release := releases[i] @@ -400,6 +400,23 @@ func (st *HelmState) DetectReleasesToBeDeleted(helm helmexec.Interface, releases return detected, nil } +func (st *HelmState) DetectReleasesToBeDeleted(helm helmexec.Interface, releases []ReleaseSpec) ([]ReleaseSpec, error) { + detected := []ReleaseSpec{} + for i := range releases { + release := releases[i] + + installed, err := st.isReleaseInstalled(st.createHelmContext(&release, 0), helm, release) + if err != nil { + return nil, err + } else if installed { + // Otherwise `release` messed up(https://github.com/roboll/helmfile/issues/554) + r := release + detected = append(detected, r) + } + } + return detected, nil +} + type SyncOpts struct { Set []string } @@ -462,6 +479,9 @@ func (st *HelmState) DeleteReleasesForSync(affectedReleases *AffectedReleases, h var args []string if isHelm3() { args = []string{} + if release.Namespace != "" { + args = append(args, "--namespace", release.Namespace) + } } else { args = []string{"--purge"} } @@ -1136,13 +1156,8 @@ func (st *HelmState) ReleaseStatuses(helm helmexec.Interface, workerLimit int) [ } // DeleteReleases wrapper for executing helm delete on the releases -// This function traverses the DAG of the releases in the reverse order, so that the releases that are NOT depended by any others are deleted first. func (st *HelmState) DeleteReleases(affectedReleases *AffectedReleases, helm helmexec.Interface, concurrency int, purge bool) []error { return st.scatterGatherReleases(helm, concurrency, func(release ReleaseSpec, workerIndex int) error { - if !release.Desired() { - return nil - } - st.ApplyOverrides(&release) flags := []string{} @@ -1155,20 +1170,13 @@ func (st *HelmState) DeleteReleases(affectedReleases *AffectedReleases, helm hel } context := st.createHelmContext(&release, workerIndex) - installed, err := st.isReleaseInstalled(context, helm, release) - if err != nil { + if err := helm.DeleteRelease(context, release.Name, flags...); err != nil { + affectedReleases.Failed = append(affectedReleases.Failed, &release) return err + } else { + affectedReleases.Deleted = append(affectedReleases.Deleted, &release) + return nil } - if installed { - if err := helm.DeleteRelease(context, release.Name, flags...); err != nil { - affectedReleases.Failed = append(affectedReleases.Failed, &release) - return err - } else { - affectedReleases.Deleted = append(affectedReleases.Deleted, &release) - return nil - } - } - return nil }) } diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 8c2ad549..caa2c6fb 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -1926,7 +1926,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(true), installed: false, purge: false, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{}}}, }, { name: "desired but not installed (purge=true)", @@ -1934,7 +1934,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(true), installed: false, purge: true, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{"--purge"}}}, }, { name: "installed but filtered (purge=false)", @@ -1942,7 +1942,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(false), installed: true, purge: false, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{}}}, }, { name: "installed but filtered (purge=true)", @@ -1950,7 +1950,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(false), installed: true, purge: true, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{"--purge"}}}, }, { name: "not installed, and filtered (purge=false)", @@ -1958,7 +1958,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(false), installed: false, purge: false, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{}}}, }, { name: "not installed, and filtered (purge=true)", @@ -1966,7 +1966,7 @@ func TestHelmState_Delete(t *testing.T) { desired: boolValue(false), installed: false, purge: true, - deleted: []exectest.Release{}, + deleted: []exectest.Release{{Name: "releaseA", Flags: []string{"--purge"}}}, }, { name: "with tiller args", From 77082cef58e895aa7274798dad8dfd142f7e3831 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 7 Nov 2019 19:53:27 +0900 Subject: [PATCH 2/4] fix regression that `--namespace` breaks delete/destroy and possibly sync/apply as well The problem was that `--namespace NS` had been not taken into account while deleting releases, that resulted in releases that should be deleted are not deleted. --- pkg/app/app.go | 29 +++++++++++------------------ pkg/state/state.go | 32 ++++++++++++++++++++++++++++---- pkg/state/state_run.go | 4 ++-- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index d21efc16..aa545480 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -451,7 +451,7 @@ func printBatches(batches [][]state.Release) string { } func withDAG(templated *state.HelmState, helm helmexec.Interface, logger *zap.SugaredLogger, reverse bool, converge func(*state.HelmState, helmexec.Interface) (bool, []error)) (bool, []error) { - batches, err := state.PlanReleases(templated.Releases, templated.Selectors, reverse) + batches, err := templated.PlanReleases(reverse) if err != nil { return false, []error{err} } @@ -679,7 +679,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, []error) { Set: c.Set(), } - allReleases := st.Releases + allReleases := st.GetReleasesWithOverrides() var changedReleases []state.ReleaseSpec var deletingReleases []state.ReleaseSpec @@ -688,7 +688,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, []error) { // TODO Better way to detect diff on only filtered releases { if len(st.Selectors) > 0 { - releases, err := st.GetFilteredReleases() + releases, err := st.GetSelectedReleasesWithOverrides() if err != nil { return false, []error{err} } @@ -835,19 +835,12 @@ func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error affectedReleases := state.AffectedReleases{} - var toSync []state.ReleaseSpec - - if len(st.Selectors) > 0 { - var err error - toSync, err = st.GetFilteredReleases() - if err != nil { - return false, []error{err} - } - if len(toSync) == 0 { - return false, nil - } - } else { - toSync = st.Releases + toSync, err := st.GetSelectedReleasesWithOverrides() + if err != nil { + return false, []error{err} + } + if len(toSync) == 0 { + return false, nil } toDelete, err := st.DetectReleasesToBeDeleted(helm, toSync) @@ -925,7 +918,7 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { if len(st.Selectors) > 0 { var err error - toSync, err = st.GetFilteredReleases() + toSync, err = st.GetSelectedReleasesWithOverrides() if err != nil { return false, []error{err} } @@ -1047,7 +1040,7 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { if len(st.Selectors) > 0 { var err error - templatedReleases, err = st.GetFilteredReleases() + templatedReleases, err = st.GetSelectedReleasesWithOverrides() if err != nil { return false, []error{err} } diff --git a/pkg/state/state.go b/pkg/state/state.go index 5a41241a..3b93becb 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1226,7 +1226,31 @@ func (st *HelmState) Clean() []error { return nil } -func MarkFilteredReleases(releases []ReleaseSpec, selectors []string) ([]Release, error) { +func (st *HelmState) GetReleasesWithOverrides() []ReleaseSpec { + var rs []ReleaseSpec + for _, r := range st.Releases { + var spec ReleaseSpec + spec = r + st.ApplyOverrides(&spec) + rs = append(rs, spec) + } + return rs +} + +func (st *HelmState) SelectReleasesWithOverrides() ([]Release, error) { + rs, err := markFilteredReleases(st.GetReleasesWithOverrides(), st.Selectors) + if err != nil { + return nil, err + } + for _, r := range rs { + spec := r.ReleaseSpec + st.ApplyOverrides(&spec) + r.ReleaseSpec = spec + } + return rs, nil +} + +func markFilteredReleases(releases []ReleaseSpec, selectors []string) ([]Release, error) { var filteredReleases []Release filters := []ReleaseFilter{} for _, label := range selectors { @@ -1266,8 +1290,8 @@ func MarkFilteredReleases(releases []ReleaseSpec, selectors []string) ([]Release return filteredReleases, nil } -func (st *HelmState) GetFilteredReleases() ([]ReleaseSpec, error) { - filteredReleases, err := MarkFilteredReleases(st.Releases, st.Selectors) +func (st *HelmState) GetSelectedReleasesWithOverrides() ([]ReleaseSpec, error) { + filteredReleases, err := st.SelectReleasesWithOverrides() if err != nil { return nil, err } @@ -1282,7 +1306,7 @@ func (st *HelmState) GetFilteredReleases() ([]ReleaseSpec, error) { // FilterReleases allows for the execution of helm commands against a subset of the releases in the helmfile. func (st *HelmState) FilterReleases() error { - releases, err := st.GetFilteredReleases() + releases, err := st.GetSelectedReleasesWithOverrides() if err != nil { return err } diff --git a/pkg/state/state_run.go b/pkg/state/state_run.go index 9c7a2f53..e074d64f 100644 --- a/pkg/state/state_run.go +++ b/pkg/state/state_run.go @@ -108,8 +108,8 @@ type PlanOpts struct { Reverse bool } -func PlanReleases(releases []ReleaseSpec, selectors []string, reverse bool) ([][]Release, error) { - marked, err := MarkFilteredReleases(releases, selectors) +func (st *HelmState) PlanReleases(reverse bool) ([][]Release, error) { + marked, err := st.SelectReleasesWithOverrides() if err != nil { return nil, err } From e2e4e8440d2e713e9afcfa1c6851bb029f34e81a Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 7 Nov 2019 20:48:55 +0900 Subject: [PATCH 3/4] fix the bug that resulted in `helmfile sync` not delete releases with `--namespace` --- pkg/app/app.go | 19 ++++++------------- pkg/state/state.go | 5 ----- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index aa545480..4e0a732f 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -914,19 +914,12 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { return false, errs } - var toSync []state.ReleaseSpec - - if len(st.Selectors) > 0 { - var err error - toSync, err = st.GetSelectedReleasesWithOverrides() - if err != nil { - return false, []error{err} - } - if len(toSync) == 0 { - return false, nil - } - } else { - toSync = st.Releases + toSync, err := st.GetSelectedReleasesWithOverrides() + if err != nil { + return false, []error{err} + } + if len(toSync) == 0 { + return false, nil } toDelete, err := st.DetectReleasesToBeDeletedForSync(helm, toSync) diff --git a/pkg/state/state.go b/pkg/state/state.go index 3b93becb..f6dd690f 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1242,11 +1242,6 @@ func (st *HelmState) SelectReleasesWithOverrides() ([]Release, error) { if err != nil { return nil, err } - for _, r := range rs { - spec := r.ReleaseSpec - st.ApplyOverrides(&spec) - r.ReleaseSpec = spec - } return rs, nil } From a136c46c0665d2cb748e4ddade62ccb3b3fa0923 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 7 Nov 2019 21:23:36 +0900 Subject: [PATCH 4/4] fix the bug that `helmfile template` is unable to render anything when `--namespace` is specified --- pkg/app/app.go | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 4e0a732f..2db35c90 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1029,40 +1029,33 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { return false, errs } - var templatedReleases []state.ReleaseSpec - - if len(st.Selectors) > 0 { - var err error - templatedReleases, err = st.GetSelectedReleasesWithOverrides() - if err != nil { - return false, []error{err} - } - if len(templatedReleases) == 0 { - return false, nil - } - } else { - templatedReleases = st.Releases + toRender, err := st.GetSelectedReleasesWithOverrides() + if err != nil { + return false, []error{err} + } + if len(toRender) == 0 { + return false, nil } - releasesToBeTemplated := map[string]state.ReleaseSpec{} - for _, r := range templatedReleases { + releasesToRender := map[string]state.ReleaseSpec{} + for _, r := range toRender { id := state.ReleaseToID(&r) - releasesToBeTemplated[id] = r + releasesToRender[id] = r } - names := make([]string, len(templatedReleases)) - for i, r := range templatedReleases { + names := make([]string, len(toRender)) + for i, r := range toRender { names[i] = fmt.Sprintf(" %s (%s)", r.Name, r.Chart) } var errs []error - if len(releasesToBeTemplated) > 0 { + if len(releasesToRender) > 0 { _, templateErrs := withDAG(st, helm, a.Logger, false, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { var rs []state.ReleaseSpec for _, r := range subst.Releases { - if _, ok := releasesToBeTemplated[state.ReleaseToID(&r)]; ok { + if _, ok := releasesToRender[state.ReleaseToID(&r)]; ok { rs = append(rs, r) } }