From 5a26151e3e4463ed16d549755cb9d0e606dc199a Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Mon, 27 Apr 2020 12:43:08 +0100 Subject: [PATCH 1/8] reduce number of helm commands for validation to number of chart/version pairs to validate --- internal/app/release.go | 102 +++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 44 deletions(-) diff --git a/internal/app/release.go b/internal/app/release.go index d5a2ebe6..8f411db7 100644 --- a/internal/app/release.go +++ b/internal/app/release.go @@ -173,17 +173,39 @@ func validateReleaseCharts(s *state) error { apps = s.Apps } c := make(chan string, len(apps)) + + charts := make(map[string]map[string][]string) for app, r := range apps { - sem <- struct{}{} - wg.Add(1) - go func(r *release, app string) { - defer func() { - wg.Done() - <-sem - }() - r.validateChart(app, s, c) - }(r, app) + if charts[r.Chart] == nil { + charts[r.Chart] = make(map[string][]string) + } + + if charts[r.Chart][r.Version] == nil { + charts[r.Chart][r.Version] = make([]string, 0) + } + + if r.isConsideredToRun(s) { + charts[r.Chart][r.Version] = append(charts[r.Chart][r.Version], app) + } } + + for chart, versions := range charts { + for version, apps := range versions { + concattedApps := strings.Join(apps, ", ") + ch := chart + v := version + sem <- struct{}{} + wg.Add(1) + go func(apps, chart, version string) { + defer func() { + wg.Done() + <-sem + }() + validateChart(concattedApps, chart, version, s, c) + }(concattedApps, ch, v) + } + } + wg.Wait() close(c) for err := range c { @@ -201,45 +223,37 @@ func validateReleaseCharts(s *state) error { var versionExtractor = regexp.MustCompile(`[\n]version:\s?(.*)`) // validateChart validates if chart with the same name and version as specified in the DSF exists -func (r *release) validateChart(app string, s *state, c chan string) { +func validateChart(apps, chart, version string, s *state, c chan string) { + if isLocalChart(chart) { + cmd := helmCmd([]string{"inspect", "chart", chart}, "Validating [ "+chart+" ] chart's availability") - validateCurrentChart := true - if !r.isConsideredToRun(s) { - validateCurrentChart = false - } - if validateCurrentChart { - if isLocalChart(r.Chart) { - cmd := helmCmd([]string{"inspect", "chart", r.Chart}, "Validating [ "+r.Chart+" ] chart's availability") - - result := cmd.exec() - if result.code != 0 { - maybeRepo := filepath.Base(filepath.Dir(r.Chart)) - c <- "Chart [ " + r.Chart + " ] for app [" + app + "] can't be found. Inspection returned error: \"" + - strings.TrimSpace(result.errors) + "\" -- If this is not a local chart, add the repo [ " + maybeRepo + " ] in your helmRepos stanza." + result := cmd.exec() + if result.code != 0 { + maybeRepo := filepath.Base(filepath.Dir(chart)) + c <- "Chart [ " + chart + " ] for apps [" + apps + "] can't be found. Inspection returned error: \"" + + strings.TrimSpace(result.errors) + "\" -- If this is not a local chart, add the repo [ " + maybeRepo + " ] in your helmRepos stanza." + return + } + matches := versionExtractor.FindStringSubmatch(result.output) + if len(matches) == 2 { + v := strings.Trim(matches[1], `'"`) + if strings.Trim(version, `'"`) != v { + c <- "Chart [ " + chart + " ] with version [ " + version + " ] is specified for " + + "apps [" + apps + "] but the chart found at that path has version [ " + v + " ] which does not match." return } - matches := versionExtractor.FindStringSubmatch(result.output) - if len(matches) == 2 { - version := strings.Trim(matches[1], `'"`) - if strings.Trim(r.Version, `'"`) != version { - c <- "Chart [ " + r.Chart + " ] with version [ " + r.Version + " ] is specified for " + - "app [" + app + "] but the chart found at that path has version [ " + version + " ] which does not match." - return - } - } - - } else { - version := r.Version - if len(version) == 0 { - version = "*" - } - cmd := helmCmd([]string{"search", "repo", r.Chart, "--version", version, "-l"}, "Validating [ "+r.Chart+" ] chart's version [ "+r.Version+" ] availability") + } + } else { + v := version + if len(v) == 0 { + v = "*" + } + cmd := helmCmd([]string{"search", "repo", chart, "--version", v, "-l"}, "Validating [ "+chart+" ] chart's version [ "+version+" ] availability") - if result := cmd.exec(); result.code != 0 || strings.Contains(result.output, "No results found") { - c <- "Chart [ " + r.Chart + " ] with version [ " + r.Version + " ] is specified for " + - "app [" + app + "] but was not found. If this is not a local chart, define its helm repo in the helmRepo stanza in your DSF." - return - } + if result := cmd.exec(); result.code != 0 || strings.Contains(result.output, "No results found") { + c <- "Chart [ " + chart + " ] with version [ " + version + " ] is specified for " + + "apps [" + apps + "] but was not found. If this is not a local chart, define its helm repo in the helmRepo stanza in your DSF." + return } } } From 002e01a8eabdbfba34e1d8fa2edfcd5322f97fcf Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Mon, 27 Apr 2020 13:19:25 +0100 Subject: [PATCH 2/8] cache some helm command results --- internal/app/helm_helpers.go | 9 +++++++++ internal/app/release.go | 14 ++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/internal/app/helm_helpers.go b/internal/app/helm_helpers.go index 2666c4b0..1bf8692d 100644 --- a/internal/app/helm_helpers.go +++ b/internal/app/helm_helpers.go @@ -6,6 +6,7 @@ import ( "fmt" "net/url" "strings" + "sync" "github.com/Praqma/helmsman/internal/gcs" ) @@ -15,6 +16,8 @@ type helmRepo struct { Url string `json:"url"` } +var chartNameCache sync.Map + // helmCmd prepares a helm command to be executed func helmCmd(args []string, desc string) command { return command{ @@ -26,6 +29,11 @@ func helmCmd(args []string, desc string) command { // extractChartName extracts the Helm chart name from full chart name in the desired state. func extractChartName(releaseChart string) string { + chart, ok := chartNameCache.Load(releaseChart) + if ok { + return chart.(string) + } + cmd := helmCmd([]string{"show", "chart", releaseChart}, "Show chart information") result := cmd.exec() @@ -42,6 +50,7 @@ func extractChartName(releaseChart string) string { } } + chartNameCache.Store(releaseChart, name) return name } diff --git a/internal/app/release.go b/internal/app/release.go index 8f411db7..1966327b 100644 --- a/internal/app/release.go +++ b/internal/app/release.go @@ -262,9 +262,16 @@ func validateChart(apps, chart, version string, s *state, c chan string) { // If chart is local, returns the given release version func (r *release) getChartVersion() (string, string) { if isLocalChart(r.Chart) { + log.Info("Chart [ " + r.Chart + "] with version [ " + r.Version + " ] was found locally.") return r.Version, "" } - cmd := helmCmd([]string{"search", "repo", r.Chart, "--version", r.Version, "-o", "json"}, "Getting latest chart's version "+r.Chart+"-"+r.Version+"") + + if len(r.cachedVersion) > 0 { + log.Info("Non-local chart [ " + r.Chart + "] with version [ " + r.Version + " ] was found in cache.") + return r.cachedVersion, "" + } + + cmd := helmCmd([]string{"search", "repo", r.Chart, "--version", r.Version, "-o", "json"}, "Getting latest non-local chart's version "+r.Chart+"-"+r.Version+"") result := cmd.exec() if result.code != 0 { @@ -288,7 +295,10 @@ func (r *release) getChartVersion() (string, string) { } else if len(filteredChartVersions) > 1 { return "", "Multiple versions of chart [ " + r.Chart + " ] with version [ " + r.Version + " ] found in the helm repositories" } - return filteredChartVersions[0].Version, "" + + v := filteredChartVersions[0].Version + r.cachedVersion = v + return v, "" } // testRelease creates a Helm command to test a particular release. From 5898e6cd2a3e04990907228ffa5cf5371dd56192 Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Mon, 27 Apr 2020 15:01:26 +0100 Subject: [PATCH 3/8] fix appVersion --- internal/app/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/app/main.go b/internal/app/main.go index 4891d56b..baf8f2fe 100644 --- a/internal/app/main.go +++ b/internal/app/main.go @@ -6,7 +6,7 @@ import ( const ( helmBin = "helm" - appVersion = "v3.3.0" + appVersion = "v3.3.1-perf" tempFilesDir = ".helmsman-tmp" defaultContextName = "default" resourcePool = 10 From 0f62379b134fad6447ddc6f9f778b8e7afa7790a Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Tue, 28 Apr 2020 00:32:00 +0100 Subject: [PATCH 4/8] fix release performancce improvements --- Dockerfile | 56 ++++++--- internal/app/decision_maker.go | 126 +++++++++++++++++--- internal/app/decision_maker_test.go | 8 +- internal/app/helm_helpers.go | 13 +-- internal/app/main.go | 2 +- internal/app/release.go | 39 +++---- internal/app/release_test.go | 173 ++++++++-------------------- 7 files changed, 223 insertions(+), 194 deletions(-) diff --git a/Dockerfile b/Dockerfile index 4476c618..fda61e68 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,18 +4,44 @@ ARG GLOBAL_KUBE_VERSION="v1.14.10" ARG GLOBAL_HELM_VERSION="v3.1.1" ARG GLOBAL_HELM_DIFF_VERSION="v3.1.1" - -FROM golang:${GO_VERSION}-alpine${ALPINE_VERSION} as builder +### Helm Installer ### +FROM alpine:${ALPINE_VERSION} as helm-installer ARG GLOBAL_KUBE_VERSION ARG GLOBAL_HELM_VERSION ARG GLOBAL_HELM_DIFF_VERSION ENV KUBE_VERSION=$GLOBAL_KUBE_VERSION ENV HELM_VERSION=$GLOBAL_HELM_VERSION ENV HELM_DIFF_VERSION=$GLOBAL_HELM_DIFF_VERSION + +RUN apk add --update --no-cache ca-certificates git openssh ruby curl tar gzip make bash + +RUN curl -L https://storage.googleapis.com/kubernetes-release/release/${KUBE_VERSION}/bin/linux/amd64/kubectl -o /usr/local/bin/kubectl +RUN chmod +x /usr/local/bin/kubectl + +RUN curl -Lk https://get.helm.sh/helm-${HELM_VERSION}-linux-amd64.tar.gz | tar zxv -C /tmp +RUN mv /tmp/linux-amd64/helm /usr/local/bin/helm && rm -rf /tmp/linux-amd64 +RUN chmod +x /usr/local/bin/helm + +RUN helm plugin install https://github.com/hypnoglow/helm-s3.git +RUN helm plugin install https://github.com/nouney/helm-gcs +RUN helm plugin install https://github.com/databus23/helm-diff --version ${HELM_DIFF_VERSION} +RUN helm plugin install https://github.com/futuresimple/helm-secrets +RUN rm -r /tmp/helm-diff /tmp/helm-diff.tgz + +### Go Builder & Tester ### +FROM golang:${GO_VERSION}-alpine${ALPINE_VERSION} as builder + +RUN apk add --update --no-cache ca-certificates git openssh ruby bash make +RUN gem install hiera-eyaml --no-doc +RUN update-ca-certificates + +COPY --from=helm-installer /usr/local/bin/kubectl /usr/local/bin/kubectl +COPY --from=helm-installer /usr/local/bin/helm /usr/local/bin/helm +COPY --from=helm-installer /root/.cache/helm/plugins/ /root/.cache/helm/plugins/ +COPY --from=helm-installer /root/.local/share/helm/plugins/ /root/.local/share/helm/plugins/ + WORKDIR /go/src/github.com/Praqma/helmsman -COPY scripts/ /tmp/ -RUN sh /tmp/setup.sh \ - && apk --no-cache add dep + COPY . . RUN make test \ && LastTag=$(git describe --abbrev=0 --tags) \ @@ -25,14 +51,16 @@ RUN make test \ && if [ ${LT_SHA} != ${LC_SHA} ]; then TAG=latest-$(date +"%d%m%y"); fi \ && make build - +### Final Image ### FROM alpine:${ALPINE_VERSION} as base -ARG GLOBAL_KUBE_VERSION -ARG GLOBAL_HELM_VERSION -ARG GLOBAL_HELM_DIFF_VERSION -ENV KUBE_VERSION=$GLOBAL_KUBE_VERSION -ENV HELM_VERSION=$GLOBAL_HELM_VERSION -ENV HELM_DIFF_VERSION=$GLOBAL_HELM_DIFF_VERSION -COPY scripts/ /tmp/ -RUN sh /tmp/setup.sh + +RUN apk add --update --no-cache ca-certificates git openssh ruby curl bash +RUN gem install hiera-eyaml --no-doc +RUN update-ca-certificates + +COPY --from=helm-installer /usr/local/bin/kubectl /usr/local/bin/kubectl +COPY --from=helm-installer /usr/local/bin/helm /usr/local/bin/helm +COPY --from=helm-installer /root/.cache/helm/plugins/ /root/.cache/helm/plugins/ +COPY --from=helm-installer /root/.local/share/helm/plugins/ /root/.local/share/helm/plugins/ + COPY --from=builder /go/src/github.com/Praqma/helmsman/helmsman /bin/helmsman diff --git a/internal/app/decision_maker.go b/internal/app/decision_maker.go index 5254a707..30203ec2 100644 --- a/internal/app/decision_maker.go +++ b/internal/app/decision_maker.go @@ -44,8 +44,8 @@ func buildState(s *state) *currentState { if flags.contextOverride == "" { r.HelmsmanContext = getReleaseContext(r.Name, r.Namespace) } else { - log.Info("Overriding Helmsman context for " + r.Name + " as " + flags.contextOverride) r.HelmsmanContext = flags.contextOverride + log.Info("Overwrote Helmsman context for release [ " + r.Name + " ] to " + flags.contextOverride) } cs.releases[r.key()] = r }(r) @@ -60,18 +60,115 @@ func (cs *currentState) makePlan(s *state) *plan { wg := sync.WaitGroup{} sem := make(chan struct{}, resourcePool) + namesC := make(chan [2]string, len(s.Apps)) + versionsC := make(chan [4]string, len(s.Apps)) + + // We store the results of the helm commands + extractedChartNames := make(map[string]string) + extractedChartVersions := make(map[string]map[string]string) + + // We get the charts and versions with the expensive helm commands first. + // We can probably DRY this concurrency stuff up somehow. + // We can also definitely DRY this with validateReleaseChart. + // We should probably have a data structure earlier on that sorts this out properly. + // Ideally we'd have a pipeline of helm command tasks with several stages that can all come home if one of them fails. + // Is it better to fail early here? I am not sure. + + // Initialize the rejigged data structures + charts := make(map[string]map[string]bool) + for _, r := range s.Apps { + if charts[r.Chart] == nil { + charts[r.Chart] = make(map[string]bool) + } + + if extractedChartVersions[r.Chart] == nil { + extractedChartVersions[r.Chart] = make(map[string]string) + } + + if r.isConsideredToRun(s) { + charts[r.Chart][r.Version] = true + } + } + + // Concurrently extract chart names and versions + // I'm not fond of this concurrency pattern, it's just a continuation of what's already there. + // This seems like overkill somehow. Is it necessary to run all the helm commands concurrently? Does that speed it up? (Quick sanity check says: yes, it does) + for chart, versions := range charts { + sem <- struct{}{} + wg.Add(1) + go func(chart string) { + defer func() { + wg.Done() + <-sem + }() + namesC <- [2]string{chart, extractChartName(chart)} + }(chart) + + for version, shouldRun := range versions { + if !shouldRun { + continue + } + + sem <- struct{}{} + wg.Add(1) + go func(chart, version string) { + defer func() { + wg.Done() + <-sem + }() + + // even though I wrote it, lines like this are a code smell to me -- we need to rethink the concurrency as i mentioned above. + // ideally you just process all helm commands "in a background pool", they return channels, + // and we would be looping over select statements until we had the results of the helm commands we wanted. + n, m := getChartVersion(chart, version) + versionsC <- [4]string{chart, version, n, m} + }(chart, version) + } + } + + wg.Wait() + close(namesC) + close(versionsC) + + // One thing we could do here instead would be: + // instead of waiting for everything to come back, just start deciding for releases as their chart names and versions come through. + + for nameResult := range namesC { + // Is there a ... that can do this? I forget + c, n := nameResult[0], nameResult[1] + log.Info("Extracted chart name [ " + c + " ].") + extractedChartNames[c] = n + } + + for versionResult := range versionsC { + // Is there a ... that can do this? I forget + c, v, n, m := versionResult[0], versionResult[1], versionResult[2], versionResult[3] + if m != "" { + // Better to fail early and return here? + log.Error(m) + } else { + log.Info("Extracted chart version from chart [ " + c + " ] with version [ " + v + " ]: '" + n + "'") + extractedChartVersions[c][v] = n + } + } + + // Pass the extracted names and versions back to the apps to decide. + // We still have to run decide on all the apps, even the ones we previously filtered out when extracting names and versions. + // We can now proceed without trying lots of identical helm commands at the same time. for _, r := range s.Apps { + // To be honest, the helmCmd function should probably pass back a channel at this point, making the resource pool "global", for all helm commands. + // It would make more sense than parallelising *some of the workload* like we do here with r.checkChartDepUpdate(), leaving some helm commands outside the concurrent part. r.checkChartDepUpdate() sem <- struct{}{} wg.Add(1) - go func(r *release) { + go func(r *release, chartName, chartVersion string) { defer func() { wg.Done() <-sem }() - cs.decide(r, s, p) - }(r) + cs.decide(r, s, p, chartName, chartVersion) + }(r, extractedChartNames[r.Chart], extractedChartVersions[r.Chart][r.Version]) } wg.Wait() @@ -80,7 +177,7 @@ func (cs *currentState) makePlan(s *state) *plan { // decide makes a decision about what commands (actions) need to be executed // to make a release section of the desired state come true. -func (cs *currentState) decide(r *release, s *state, p *plan) { +func (cs *currentState) decide(r *release, s *state, p *plan, chartName, chartVersion string) { // check for presence in defined targets or groups if !r.isConsideredToRun(s) { p.addDecision("Release [ "+r.Name+" ] ignored", r.Priority, ignored) @@ -112,7 +209,7 @@ func (cs *currentState) decide(r *release, s *state, p *plan) { if ok := cs.releaseExists(r, helmStatusDeployed); ok { if !r.isProtected(cs, s) { - cs.inspectUpgradeScenario(r, p) // upgrade or move + cs.inspectUpgradeScenario(r, p, chartName, chartVersion) // upgrade or move } else { p.addDecision("Release [ "+r.Name+" ] in namespace [ "+r.Namespace+" ] is PROTECTED. Operations are not allowed on this release until "+ "you remove its protection.", r.Priority, noop) @@ -284,7 +381,10 @@ func (cs *currentState) cleanUntrackedReleases(s *state, p *plan) { // it will be purge deleted and installed in the same namespace using the new chart. // - If the release is NOT in the same namespace specified in the input, // it will be purge deleted and installed in the new namespace. -func (cs *currentState) inspectUpgradeScenario(r *release, p *plan) { +func (cs *currentState) inspectUpgradeScenario(r *release, p *plan, chartName, chartVersion string) { + if chartName == "" || chartVersion == "" { + return + } rs, ok := cs.releases[r.key()] if !ok { @@ -292,21 +392,15 @@ func (cs *currentState) inspectUpgradeScenario(r *release, p *plan) { } if r.Namespace == rs.Namespace { + r.Version = chartVersion - version, msg := r.getChartVersion() - if msg != "" { - log.Fatal(msg) - return - } - r.Version = version - - if extractChartName(r.Chart) == rs.getChartName() && r.Version != rs.getChartVersion() { + if chartName == rs.getChartName() && r.Version != rs.getChartVersion() { // upgrade r.diff() r.upgrade(p) p.addDecision("Release [ "+r.Name+" ] will be updated", r.Priority, change) - } else if extractChartName(r.Chart) != rs.getChartName() { + } else if chartName != rs.getChartName() { r.reInstall(p) p.addDecision("Release [ "+r.Name+" ] is desired to use a new chart [ "+r.Chart+ " ]. Delete of the current release will be planned and new chart will be installed in namespace [ "+ diff --git a/internal/app/decision_maker_test.go b/internal/app/decision_maker_test.go index 87813e07..13eae616 100644 --- a/internal/app/decision_maker_test.go +++ b/internal/app/decision_maker_test.go @@ -86,7 +86,7 @@ func Test_inspectUpgradeScenario(t *testing.T) { want decisionType }{ { - name: "inspectUpgradeScenario() - local chart with different chart name should change", + name: "makePlan() - local chart with different chart name should change", args: args{ r: &release{ Name: "release1", @@ -111,7 +111,9 @@ func Test_inspectUpgradeScenario(t *testing.T) { cs := currentState{releases: *tt.args.s} // Act - cs.inspectUpgradeScenario(tt.args.r, &outcome) + chartName := extractChartName(tt.args.r.Chart) + chartVersion, _ := getChartVersion(tt.args.r.Chart, tt.args.r.Version) + cs.inspectUpgradeScenario(tt.args.r, &outcome, chartName, chartVersion) got := outcome.Decisions[0].Type t.Log(outcome.Decisions[0].Description) @@ -211,7 +213,7 @@ func Test_decide(t *testing.T) { } outcome := plan{} // Act - cs.decide(tt.args.r, tt.args.s, &outcome) + cs.decide(tt.args.r, tt.args.s, &outcome, "", "") got := outcome.Decisions[0].Type t.Log(outcome.Decisions[0].Description) diff --git a/internal/app/helm_helpers.go b/internal/app/helm_helpers.go index 1bf8692d..56817f66 100644 --- a/internal/app/helm_helpers.go +++ b/internal/app/helm_helpers.go @@ -6,7 +6,6 @@ import ( "fmt" "net/url" "strings" - "sync" "github.com/Praqma/helmsman/internal/gcs" ) @@ -16,8 +15,6 @@ type helmRepo struct { Url string `json:"url"` } -var chartNameCache sync.Map - // helmCmd prepares a helm command to be executed func helmCmd(args []string, desc string) command { return command{ @@ -29,12 +26,7 @@ func helmCmd(args []string, desc string) command { // extractChartName extracts the Helm chart name from full chart name in the desired state. func extractChartName(releaseChart string) string { - chart, ok := chartNameCache.Load(releaseChart) - if ok { - return chart.(string) - } - - cmd := helmCmd([]string{"show", "chart", releaseChart}, "Show chart information") + cmd := helmCmd([]string{"show", "chart", releaseChart}, "Caching chart information for [ "+releaseChart+" ].") result := cmd.exec() if result.code != 0 { @@ -50,7 +42,6 @@ func extractChartName(releaseChart string) string { } } - chartNameCache.Store(releaseChart, name) return name } @@ -62,6 +53,8 @@ func getHelmVersion() string { if result.code != 0 { log.Fatal("While checking helm version: " + result.errors) } + + log.Verbose("Helm version " + result.output) return result.output } diff --git a/internal/app/main.go b/internal/app/main.go index baf8f2fe..4891d56b 100644 --- a/internal/app/main.go +++ b/internal/app/main.go @@ -6,7 +6,7 @@ import ( const ( helmBin = "helm" - appVersion = "v3.3.1-perf" + appVersion = "v3.3.0" tempFilesDir = ".helmsman-tmp" defaultContextName = "default" resourcePool = 10 diff --git a/internal/app/release.go b/internal/app/release.go index 1966327b..40965cf0 100644 --- a/internal/app/release.go +++ b/internal/app/release.go @@ -201,7 +201,7 @@ func validateReleaseCharts(s *state) error { wg.Done() <-sem }() - validateChart(concattedApps, chart, version, s, c) + validateChart(concattedApps, chart, version, c) }(concattedApps, ch, v) } } @@ -223,7 +223,7 @@ func validateReleaseCharts(s *state) error { var versionExtractor = regexp.MustCompile(`[\n]version:\s?(.*)`) // validateChart validates if chart with the same name and version as specified in the DSF exists -func validateChart(apps, chart, version string, s *state, c chan string) { +func validateChart(apps, chart, version string, c chan string) { if isLocalChart(chart) { cmd := helmCmd([]string{"inspect", "chart", chart}, "Validating [ "+chart+" ] chart's availability") @@ -260,45 +260,38 @@ func validateChart(apps, chart, version string, s *state, c chan string) { // getChartVersion fetches the lastest chart version matching the semantic versioning constraints. // If chart is local, returns the given release version -func (r *release) getChartVersion() (string, string) { - if isLocalChart(r.Chart) { - log.Info("Chart [ " + r.Chart + "] with version [ " + r.Version + " ] was found locally.") - return r.Version, "" - } - - if len(r.cachedVersion) > 0 { - log.Info("Non-local chart [ " + r.Chart + "] with version [ " + r.Version + " ] was found in cache.") - return r.cachedVersion, "" +func getChartVersion(chart, version string) (string, string) { + if isLocalChart(chart) { + log.Info("Chart [ " + chart + "] with version [ " + version + " ] was found locally.") + return version, "" } - cmd := helmCmd([]string{"search", "repo", r.Chart, "--version", r.Version, "-o", "json"}, "Getting latest non-local chart's version "+r.Chart+"-"+r.Version+"") + cmd := helmCmd([]string{"search", "repo", chart, "--version", version, "-o", "json"}, "Getting latest non-local chart's version "+chart+"-"+version+"") result := cmd.exec() if result.code != 0 { - return "", "Chart [ " + r.Chart + " ] with version [ " + r.Version + " ] is specified but not found in the helm repositories" + return "", "Chart [ " + chart + " ] with version [ " + version + " ] is specified but not found in the helm repositories" } - chartVersions := make([]chartVersion, 0) - if err := json.Unmarshal([]byte(result.output), &chartVersions); err != nil { + cv := make([]chartVersion, 0) + if err := json.Unmarshal([]byte(result.output), &cv); err != nil { log.Fatal(fmt.Sprint(err)) } filteredChartVersions := make([]chartVersion, 0) - for _, chart := range chartVersions { - if chart.Name == r.Chart { - filteredChartVersions = append(filteredChartVersions, chart) + for _, c := range cv { + if c.Name == chart { + filteredChartVersions = append(filteredChartVersions, c) } } if len(filteredChartVersions) < 1 { - return "", "Chart [ " + r.Chart + " ] with version [ " + r.Version + " ] is specified but not found in the helm repositories" + return "", "Chart [ " + chart + " ] with version [ " + version + " ] is specified but not found in the helm repositories" } else if len(filteredChartVersions) > 1 { - return "", "Multiple versions of chart [ " + r.Chart + " ] with version [ " + r.Version + " ] found in the helm repositories" + return "", "Multiple versions of chart [ " + chart + " ] with version [ " + version + " ] found in the helm repositories" } - v := filteredChartVersions[0].Version - r.cachedVersion = v - return v, "" + return filteredChartVersions[0].Version, "" } // testRelease creates a Helm command to test a particular release. diff --git a/internal/app/release_test.go b/internal/app/release_test.go index 8742ca63..34859542 100644 --- a/internal/app/release_test.go +++ b/internal/app/release_test.go @@ -457,10 +457,36 @@ func Test_inheritHooks(t *testing.T) { }) } } + +func createFullReleasePointer(chart, version string) *release { + return &release{ + Name: "", + Description: "", + Namespace: "", + Enabled: true, + Chart: chart, + Version: version, + ValuesFile: "", + ValuesFiles: []string{}, + SecretsFile: "", + SecretsFiles: []string{}, + Test: false, + Protected: false, + Wait: false, + Priority: 0, + Set: make(map[string]string), + SetString: make(map[string]string), + HelmFlags: []string{}, + NoHooks: false, + Timeout: 0, + } +} + func Test_validateReleaseCharts(t *testing.T) { type args struct { apps map[string]*release } + tests := []struct { name string targetFlag []string @@ -473,27 +499,7 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "", - Description: "", - Namespace: "", - Enabled: true, - Chart: os.TempDir() + "/helmsman-tests/myapp", - Version: "", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer(os.TempDir()+"/helmsman-tests/myapp", ""), }, }, want: false, @@ -502,27 +508,7 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "", - Description: "", - Namespace: "", - Enabled: true, - Chart: os.TempDir() + "/does-not-exist/myapp", - Version: "", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer(os.TempDir()+"/does-not-exist/myapp", ""), }, }, want: false, @@ -531,27 +517,7 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "", - Description: "", - Namespace: "", - Enabled: true, - Chart: os.TempDir() + "/helmsman-tests/dir-with space/myapp", - Version: "0.1.0", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), }, }, want: true, @@ -560,27 +526,7 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "", - Description: "", - Namespace: "", - Enabled: true, - Chart: "stable/prometheus", - Version: "9.5.2", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer("stable/prometheus", "9.5.2"), }, }, want: true, @@ -589,27 +535,7 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{"notThisOne"}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "app", - Description: "", - Namespace: "", - Enabled: true, - Chart: os.TempDir() + "/does-not-exist/myapp", - Version: "", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer(os.TempDir()+"/does-not-exist/myapp", ""), }, }, want: true, @@ -618,30 +544,23 @@ func Test_validateReleaseCharts(t *testing.T) { targetFlag: []string{"app"}, args: args{ apps: map[string]*release{ - "app": &release{ - Name: "app", - Description: "", - Namespace: "", - Enabled: true, - Chart: os.TempDir() + "/does-not-exist/myapp", - Version: "", - ValuesFile: "", - ValuesFiles: []string{}, - SecretsFile: "", - SecretsFiles: []string{}, - Test: false, - Protected: false, - Wait: false, - Priority: 0, - Set: make(map[string]string), - SetString: make(map[string]string), - HelmFlags: []string{}, - NoHooks: false, - Timeout: 0, - }, + "app": createFullReleasePointer(os.TempDir()+"/does-not-exist/myapp", ""), }, }, want: false, + }, { + name: "test case 7: multiple valid local apps with the same chart version", + targetFlag: []string{"app"}, + args: args{ + apps: map[string]*release{ + "app1": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), + "app2": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), + "app3": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), + "app4": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), + "app5": createFullReleasePointer(os.TempDir()+"/helmsman-tests/dir-with space/myapp", "0.1.0"), + }, + }, + want: true, }, } @@ -815,7 +734,7 @@ func Test_getChartVersion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Log(tt.want) - got, _ := tt.args.r.getChartVersion() + got, _ := getChartVersion(tt.args.r.Chart, tt.args.r.Version) if got != tt.want { t.Errorf("getChartVersion() = %v, want %v", got, tt.want) } From 3eb2a47aac4c75e7b3b01c4442eb72b5b88ede65 Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Tue, 12 May 2020 14:35:21 +0100 Subject: [PATCH 5/8] fix pr feedback and restrict plan to targetted apps --- internal/app/decision_maker.go | 15 +++++++++++---- internal/app/decision_maker_test.go | 2 +- internal/app/helm_helpers.go | 3 +-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/app/decision_maker.go b/internal/app/decision_maker.go index 30203ec2..56c1af4d 100644 --- a/internal/app/decision_maker.go +++ b/internal/app/decision_maker.go @@ -58,10 +58,17 @@ func buildState(s *state) *currentState { func (cs *currentState) makePlan(s *state) *plan { p := createPlan() + var apps map[string]*release + if len(s.TargetMap) > 0 { + apps = s.TargetApps + } else { + apps = s.Apps + } + wg := sync.WaitGroup{} sem := make(chan struct{}, resourcePool) - namesC := make(chan [2]string, len(s.Apps)) - versionsC := make(chan [4]string, len(s.Apps)) + namesC := make(chan [2]string, len(apps)) + versionsC := make(chan [4]string, len(apps)) // We store the results of the helm commands extractedChartNames := make(map[string]string) @@ -76,7 +83,7 @@ func (cs *currentState) makePlan(s *state) *plan { // Initialize the rejigged data structures charts := make(map[string]map[string]bool) - for _, r := range s.Apps { + for _, r := range apps { if charts[r.Chart] == nil { charts[r.Chart] = make(map[string]bool) } @@ -156,7 +163,7 @@ func (cs *currentState) makePlan(s *state) *plan { // Pass the extracted names and versions back to the apps to decide. // We still have to run decide on all the apps, even the ones we previously filtered out when extracting names and versions. // We can now proceed without trying lots of identical helm commands at the same time. - for _, r := range s.Apps { + for _, r := range apps { // To be honest, the helmCmd function should probably pass back a channel at this point, making the resource pool "global", for all helm commands. // It would make more sense than parallelising *some of the workload* like we do here with r.checkChartDepUpdate(), leaving some helm commands outside the concurrent part. r.checkChartDepUpdate() diff --git a/internal/app/decision_maker_test.go b/internal/app/decision_maker_test.go index 13eae616..d1f6a54d 100644 --- a/internal/app/decision_maker_test.go +++ b/internal/app/decision_maker_test.go @@ -86,7 +86,7 @@ func Test_inspectUpgradeScenario(t *testing.T) { want decisionType }{ { - name: "makePlan() - local chart with different chart name should change", + name: "inspectUpgradeScenario() - local chart with different chart name should change", args: args{ r: &release{ Name: "release1", diff --git a/internal/app/helm_helpers.go b/internal/app/helm_helpers.go index 56817f66..e0e90a4e 100644 --- a/internal/app/helm_helpers.go +++ b/internal/app/helm_helpers.go @@ -26,7 +26,7 @@ func helmCmd(args []string, desc string) command { // extractChartName extracts the Helm chart name from full chart name in the desired state. func extractChartName(releaseChart string) string { - cmd := helmCmd([]string{"show", "chart", releaseChart}, "Caching chart information for [ "+releaseChart+" ].") + cmd := helmCmd([]string{"show", "chart", releaseChart}, "Extracting chart information for [ "+releaseChart+" ].") result := cmd.exec() if result.code != 0 { @@ -54,7 +54,6 @@ func getHelmVersion() string { log.Fatal("While checking helm version: " + result.errors) } - log.Verbose("Helm version " + result.output) return result.output } From 09dae83f3fb7e36008ea7e69f985a2e93aa50048 Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Tue, 12 May 2020 14:44:20 +0100 Subject: [PATCH 6/8] fix variable name --- internal/app/release.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/app/release.go b/internal/app/release.go index 40965cf0..6579f65b 100644 --- a/internal/app/release.go +++ b/internal/app/release.go @@ -273,13 +273,13 @@ func getChartVersion(chart, version string) (string, string) { return "", "Chart [ " + chart + " ] with version [ " + version + " ] is specified but not found in the helm repositories" } - cv := make([]chartVersion, 0) - if err := json.Unmarshal([]byte(result.output), &cv); err != nil { + chartVersions := make([]chartVersion, 0) + if err := json.Unmarshal([]byte(result.output), &chartVersions); err != nil { log.Fatal(fmt.Sprint(err)) } filteredChartVersions := make([]chartVersion, 0) - for _, c := range cv { + for _, c := range chartVersions { if c.Name == chart { filteredChartVersions = append(filteredChartVersions, c) } From 903572033930aedeca828cca67f9eb47bd4fd6c4 Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Wed, 13 May 2020 12:09:06 +0100 Subject: [PATCH 7/8] revert target for plan --- internal/app/decision_maker.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/internal/app/decision_maker.go b/internal/app/decision_maker.go index 56c1af4d..30203ec2 100644 --- a/internal/app/decision_maker.go +++ b/internal/app/decision_maker.go @@ -58,17 +58,10 @@ func buildState(s *state) *currentState { func (cs *currentState) makePlan(s *state) *plan { p := createPlan() - var apps map[string]*release - if len(s.TargetMap) > 0 { - apps = s.TargetApps - } else { - apps = s.Apps - } - wg := sync.WaitGroup{} sem := make(chan struct{}, resourcePool) - namesC := make(chan [2]string, len(apps)) - versionsC := make(chan [4]string, len(apps)) + namesC := make(chan [2]string, len(s.Apps)) + versionsC := make(chan [4]string, len(s.Apps)) // We store the results of the helm commands extractedChartNames := make(map[string]string) @@ -83,7 +76,7 @@ func (cs *currentState) makePlan(s *state) *plan { // Initialize the rejigged data structures charts := make(map[string]map[string]bool) - for _, r := range apps { + for _, r := range s.Apps { if charts[r.Chart] == nil { charts[r.Chart] = make(map[string]bool) } @@ -163,7 +156,7 @@ func (cs *currentState) makePlan(s *state) *plan { // Pass the extracted names and versions back to the apps to decide. // We still have to run decide on all the apps, even the ones we previously filtered out when extracting names and versions. // We can now proceed without trying lots of identical helm commands at the same time. - for _, r := range apps { + for _, r := range s.Apps { // To be honest, the helmCmd function should probably pass back a channel at this point, making the resource pool "global", for all helm commands. // It would make more sense than parallelising *some of the workload* like we do here with r.checkChartDepUpdate(), leaving some helm commands outside the concurrent part. r.checkChartDepUpdate() From 0158490816a7c565f1b85b03bf4ebdf007f93a62 Mon Sep 17 00:00:00 2001 From: Fareed Dudhia Date: Thu, 14 May 2020 15:57:00 +0100 Subject: [PATCH 8/8] fix missing quote --- internal/app/helm_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/app/helm_helpers.go b/internal/app/helm_helpers.go index f7aa6053..975ccaa0 100644 --- a/internal/app/helm_helpers.go +++ b/internal/app/helm_helpers.go @@ -26,7 +26,7 @@ func helmCmd(args []string, desc string) command { // extractChartName extracts the Helm chart name from full chart name in the desired state. func extractChartName(releaseChart string) string { - cmd := helmCmd([]string{"show", "chart", releaseChart}, "Extracting chart information for [ "+releaseChart+" ]) + cmd := helmCmd([]string{"show", "chart", releaseChart}, "Extracting chart information for [ "+releaseChart+" ]") result := cmd.exec() if result.code != 0 {