From 9a2ff7802ff84fcc260c0eff940fa0af3ea91137 Mon Sep 17 00:00:00 2001 From: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com> Date: Thu, 16 Jan 2020 21:43:17 +0100 Subject: [PATCH] fix(helm): sort hooks by kind for equal weight Use the same install order for hooks as for normal resources (non-hooks) for hooks with equal weight. This makes resource handling more consistent and helps, when there are hook consisting of several resources like e.g. a service account and a job using this service account. The sort functions are changed from an in place search to an out of place sort to avoid inout parameters. Closes #7416. Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com> --- pkg/action/hooks.go | 3 +- pkg/releaseutil/kind_sorter.go | 78 +++++++++++++++++++------ pkg/releaseutil/kind_sorter_test.go | 53 ++++++++++++++++- pkg/releaseutil/manifest_sorter.go | 2 +- pkg/releaseutil/manifest_sorter_test.go | 2 +- 5 files changed, 114 insertions(+), 24 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index a161f9377..40c1ffdb6 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -38,7 +38,8 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, } } - sort.Sort(hookByWeight(executingHooks)) + // hooke are pre-ordered by kind, so keep order stable + sort.Stable(hookByWeight(executingHooks)) for _, h := range executingHooks { // Set default delete policy to before-hook-creation diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go index 92ffa03f2..3f47ea55d 100644 --- a/pkg/releaseutil/kind_sorter.go +++ b/pkg/releaseutil/kind_sorter.go @@ -16,7 +16,11 @@ limitations under the License. package releaseutil -import "sort" +import ( + "sort" + + "helm.sh/helm/v3/pkg/release" +) // KindSortOrder is an ordering of Kinds. type KindSortOrder []string @@ -99,46 +103,84 @@ var UninstallOrder KindSortOrder = []string{ "Namespace", } -// sortByKind does an in-place sort of manifests by Kind. +// sort manifests by kind (out of place sort) // // Results are sorted by 'ordering', keeping order of items with equal kind/priority -func sortByKind(manifests []Manifest, ordering KindSortOrder) []Manifest { - ks := newKindSorter(manifests, ordering) +func manifestsSortedByKind(manifests []Manifest, ordering KindSortOrder) []Manifest { + k := make([]string, len(manifests)) + for i, h := range manifests { + k[i] = h.Head.Kind + } + ks := newKindSorter(k, ordering) sort.Stable(ks) - return ks.manifests + + // apply permutation + sorted := make([]Manifest, len(manifests)) + for i, p := range ks.permutation { + sorted[i] = manifests[p] + } + return sorted +} + +// sort hooks by kind (out of place sort) +// +// Results are sorted by 'ordering', keeping order of items with equal kind/priority +func hooksSortedByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook { + k := make([]string, len(hooks)) + for i, h := range hooks { + k[i] = h.Kind + } + + ks := newKindSorter(k, ordering) + sort.Stable(ks) + + // apply permutation + sorted := make([]*release.Hook, len(hooks)) + for i, p := range ks.permutation { + sorted[i] = hooks[p] + } + return sorted } type kindSorter struct { - ordering map[string]int - manifests []Manifest + permutation []int + ordering map[string]int + kinds []string } -func newKindSorter(m []Manifest, s KindSortOrder) *kindSorter { +func newKindSorter(kinds []string, s KindSortOrder) *kindSorter { o := make(map[string]int, len(s)) for v, k := range s { o[k] = v } + p := make([]int, len(kinds)) + for i := range p { + p[i] = i + } return &kindSorter{ - manifests: m, - ordering: o, + permutation: p, + kinds: kinds, + ordering: o, } } -func (k *kindSorter) Len() int { return len(k.manifests) } +func (k *kindSorter) Len() int { return len(k.kinds) } -func (k *kindSorter) Swap(i, j int) { k.manifests[i], k.manifests[j] = k.manifests[j], k.manifests[i] } +func (k *kindSorter) Swap(i, j int) { + k.permutation[i], k.permutation[j] = k.permutation[j], k.permutation[i] +} func (k *kindSorter) Less(i, j int) bool { - a := k.manifests[i] - b := k.manifests[j] - first, aok := k.ordering[a.Head.Kind] - second, bok := k.ordering[b.Head.Kind] + a := k.kinds[k.permutation[i]] + b := k.kinds[k.permutation[j]] + first, aok := k.ordering[a] + second, bok := k.ordering[b] if !aok && !bok { // if both are unknown then sort alphabetically by kind, keep original order if same kind - if a.Head.Kind != b.Head.Kind { - return a.Head.Kind < b.Head.Kind + if a != b { + return a < b } return first < second } diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go index 4747e8252..777813667 100644 --- a/pkg/releaseutil/kind_sorter_test.go +++ b/pkg/releaseutil/kind_sorter_test.go @@ -19,6 +19,8 @@ package releaseutil import ( "bytes" "testing" + + "helm.sh/helm/v3/pkg/release" ) func TestKindSorter(t *testing.T) { @@ -175,7 +177,7 @@ func TestKindSorter(t *testing.T) { t.Fatalf("Expected %d names in order, got %d", want, got) } defer buf.Reset() - for _, r := range sortByKind(manifests, test.order) { + for _, r := range manifestsSortedByKind(manifests, test.order) { buf.WriteString(r.Name) } if got := buf.String(); got != test.expected { @@ -236,7 +238,7 @@ func TestKindSorterKeepOriginalOrder(t *testing.T) { var buf bytes.Buffer t.Run(test.description, func(t *testing.T) { defer buf.Reset() - for _, r := range sortByKind(manifests, test.order) { + for _, r := range manifestsSortedByKind(manifests, test.order) { buf.WriteString(r.Name) } if got := buf.String(); got != test.expected { @@ -257,7 +259,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) { } manifests := []Manifest{unknown, namespace} - sortByKind(manifests, InstallOrder) + manifests = manifestsSortedByKind(manifests, InstallOrder) expectedOrder := []Manifest{namespace, unknown} for i, manifest := range manifests { @@ -266,3 +268,48 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) { } } } + +// test hook sorting with a small subset of kinds, since it uses the same algorithm as manifestsSortedByKind +func TestKindSorterForHooks(t *testing.T) { + hooks := []*release.Hook{ + { + Name: "i", + Kind: "ClusterRole", + }, + { + Name: "j", + Kind: "ClusterRoleBinding", + }, + { + Name: "c", + Kind: "LimitRange", + }, + { + Name: "a", + Kind: "Namespace", + }, + } + + for _, test := range []struct { + description string + order KindSortOrder + expected string + }{ + {"install", InstallOrder, "acij"}, + {"uninstall", UninstallOrder, "jica"}, + } { + var buf bytes.Buffer + t.Run(test.description, func(t *testing.T) { + if got, want := len(test.expected), len(hooks); got != want { + t.Fatalf("Expected %d names in order, got %d", want, got) + } + defer buf.Reset() + for _, r := range hooksSortedByKind(hooks, test.order) { + buf.WriteString(r.Name) + } + if got := buf.String(); got != test.expected { + t.Errorf("Expected %q, got %q", test.expected, got) + } + }) + } +} diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index 24b0c3c95..454977b00 100644 --- a/pkg/releaseutil/manifest_sorter.go +++ b/pkg/releaseutil/manifest_sorter.go @@ -108,7 +108,7 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, ordering } } - return result.hooks, sortByKind(result.generic, ordering), nil + return hooksSortedByKind(result.hooks, ordering), manifestsSortedByKind(result.generic, ordering), nil } // sort takes a manifestFile object which may contain multiple resource definition diff --git a/pkg/releaseutil/manifest_sorter_test.go b/pkg/releaseutil/manifest_sorter_test.go index 0d2d6660a..29b0d85cf 100644 --- a/pkg/releaseutil/manifest_sorter_test.go +++ b/pkg/releaseutil/manifest_sorter_test.go @@ -219,7 +219,7 @@ metadata: } } - sorted = sortByKind(sorted, InstallOrder) + sorted = manifestsSortedByKind(sorted, InstallOrder) for i, m := range generic { if m.Content != sorted[i].Content { t.Errorf("Expected %q, got %q", m.Content, sorted[i].Content)