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 1/2] 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) From 671ceb5514900cbbd40fffc4691710f044aa73c4 Mon Sep 17 00:00:00 2001 From: Matthew Fisher Date: Thu, 30 Jan 2020 15:12:16 -0800 Subject: [PATCH 2/2] ref(kind_sorter): use an in-place sort for sortManifestsByKind, reduce code duplication Signed-off-by: Matthew Fisher --- pkg/releaseutil/kind_sorter.go | 84 +++++++------------------ pkg/releaseutil/kind_sorter_test.go | 22 +++++-- pkg/releaseutil/manifest_sorter.go | 2 +- pkg/releaseutil/manifest_sorter_test.go | 2 +- 4 files changed, 40 insertions(+), 70 deletions(-) diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go index 3f47ea55d..5b131b3b0 100644 --- a/pkg/releaseutil/kind_sorter.go +++ b/pkg/releaseutil/kind_sorter.go @@ -103,84 +103,42 @@ var UninstallOrder KindSortOrder = []string{ "Namespace", } -// sort manifests by kind (out of place sort) +// sort manifests by kind. // // Results are sorted by 'ordering', keeping order of items with equal kind/priority -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) +func sortManifestsByKind(manifests []Manifest, ordering KindSortOrder) []Manifest { + sort.SliceStable(manifests, func(i, j int) bool { + return lessByKind(manifests[i], manifests[j], manifests[i].Head.Kind, manifests[j].Head.Kind, ordering) + }) - // apply permutation - sorted := make([]Manifest, len(manifests)) - for i, p := range ks.permutation { - sorted[i] = manifests[p] - } - return sorted + return manifests } -// sort hooks by kind (out of place sort) +// sort hooks by kind, using an out-of-place sort to preserve the input parameters. // // 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 -} +func sortHooksByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook { + h := hooks + sort.SliceStable(h, func(i, j int) bool { + return lessByKind(h[i], h[j], h[i].Kind, h[j].Kind, ordering) + }) -type kindSorter struct { - permutation []int - ordering map[string]int - kinds []string + return h } -func newKindSorter(kinds []string, s KindSortOrder) *kindSorter { - o := make(map[string]int, len(s)) - for v, k := range s { - o[k] = v +func lessByKind(a interface{}, b interface{}, kindA string, kindB string, o KindSortOrder) bool { + ordering := make(map[string]int, len(o)) + for v, k := range o { + ordering[k] = v } - p := make([]int, len(kinds)) - for i := range p { - p[i] = i - } - return &kindSorter{ - permutation: p, - kinds: kinds, - ordering: o, - } -} - -func (k *kindSorter) Len() int { return len(k.kinds) } - -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.kinds[k.permutation[i]] - b := k.kinds[k.permutation[j]] - first, aok := k.ordering[a] - second, bok := k.ordering[b] + first, aok := ordering[kindA] + second, bok := ordering[kindB] if !aok && !bok { // if both are unknown then sort alphabetically by kind, keep original order if same kind - if a != b { - return a < b + if kindA != kindB { + return kindA < kindB } return first < second } diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go index 777813667..341f528a0 100644 --- a/pkg/releaseutil/kind_sorter_test.go +++ b/pkg/releaseutil/kind_sorter_test.go @@ -177,12 +177,18 @@ func TestKindSorter(t *testing.T) { t.Fatalf("Expected %d names in order, got %d", want, got) } defer buf.Reset() - for _, r := range manifestsSortedByKind(manifests, test.order) { + orig := manifests + for _, r := range sortManifestsByKind(manifests, test.order) { buf.WriteString(r.Name) } if got := buf.String(); got != test.expected { t.Errorf("Expected %q, got %q", test.expected, got) } + for i, manifest := range orig { + if manifest != manifests[i] { + t.Fatal("Expected input to sortManifestsByKind to stay the same") + } + } }) } } @@ -238,7 +244,7 @@ func TestKindSorterKeepOriginalOrder(t *testing.T) { var buf bytes.Buffer t.Run(test.description, func(t *testing.T) { defer buf.Reset() - for _, r := range manifestsSortedByKind(manifests, test.order) { + for _, r := range sortManifestsByKind(manifests, test.order) { buf.WriteString(r.Name) } if got := buf.String(); got != test.expected { @@ -259,7 +265,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) { } manifests := []Manifest{unknown, namespace} - manifests = manifestsSortedByKind(manifests, InstallOrder) + manifests = sortManifestsByKind(manifests, InstallOrder) expectedOrder := []Manifest{namespace, unknown} for i, manifest := range manifests { @@ -269,7 +275,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) { } } -// test hook sorting with a small subset of kinds, since it uses the same algorithm as manifestsSortedByKind +// test hook sorting with a small subset of kinds, since it uses the same algorithm as sortManifestsByKind func TestKindSorterForHooks(t *testing.T) { hooks := []*release.Hook{ { @@ -304,9 +310,15 @@ func TestKindSorterForHooks(t *testing.T) { t.Fatalf("Expected %d names in order, got %d", want, got) } defer buf.Reset() - for _, r := range hooksSortedByKind(hooks, test.order) { + orig := hooks + for _, r := range sortHooksByKind(hooks, test.order) { buf.WriteString(r.Name) } + for i, hook := range orig { + if hook != hooks[i] { + t.Fatal("Expected input to sortHooksByKind to stay the same") + } + } 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 454977b00..e83414500 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 hooksSortedByKind(result.hooks, ordering), manifestsSortedByKind(result.generic, ordering), nil + return sortHooksByKind(result.hooks, ordering), sortManifestsByKind(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 29b0d85cf..20d809317 100644 --- a/pkg/releaseutil/manifest_sorter_test.go +++ b/pkg/releaseutil/manifest_sorter_test.go @@ -219,7 +219,7 @@ metadata: } } - sorted = manifestsSortedByKind(sorted, InstallOrder) + sorted = sortManifestsByKind(sorted, InstallOrder) for i, m := range generic { if m.Content != sorted[i].Content { t.Errorf("Expected %q, got %q", m.Content, sorted[i].Content)