From f437b4d6c9755c30dc2e57a79c79e73bbda2fff3 Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Sat, 11 Jan 2020 21:46:27 -0500 Subject: [PATCH] feat(comp): Speed up completion of charts The completion of charts was using 'helm search repo' which can be quite slow as it must parse the entire yaml of every repo cache file. Using completion for a chart name can end up triggering multiple calls to 'helm search'; this makes the user experience poor, as there is a delay of over a second at every press. This commit creates a cache file for each repo which contains the list of charts for that repo. The completion logic then uses this new cache file directly and obtains the chart names very quickly. With only the stable repo configured, this optimization makes the completion of charts about 85 times faster, going from 1.2 seconds to 0.014 seconds; such a difference gives a much better user experience when completing chart names. On the other hand, adding the creation of the chart list cache file to 'helm repo update' or 'helm repo add' is pretty much negligible compared to the downloading of the index file. It is also worth noting that when more repos are configured, 'helm search repo' only becomes slower, while the completion logic that uses the new chart list cache file will not be affected as it only looks for the single relevant repo file. Signed-off-by: Marc Khouzam --- cmd/helm/repo_add_test.go | 16 +++++++++++- cmd/helm/repo_remove.go | 7 +++++- cmd/helm/repo_remove_test.go | 11 +++++++-- cmd/helm/root.go | 16 ++++++++++-- pkg/helmpath/home.go | 11 ++++++++- pkg/repo/chartrepo.go | 13 +++++++++- pkg/repo/index_test.go | 47 ++++++++++++++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 8 deletions(-) diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go index 4dbd0f435..d1b9bc0fc 100644 --- a/cmd/helm/repo_add_test.go +++ b/cmd/helm/repo_add_test.go @@ -19,6 +19,7 @@ package main import ( "fmt" "io/ioutil" + "os" "path/filepath" "sync" "testing" @@ -26,6 +27,8 @@ import ( "sigs.k8s.io/yaml" "helm.sh/helm/v3/internal/test/ensure" + "helm.sh/helm/v3/pkg/helmpath" + "helm.sh/helm/v3/pkg/helmpath/xdg" "helm.sh/helm/v3/pkg/repo" "helm.sh/helm/v3/pkg/repo/repotest" ) @@ -55,7 +58,8 @@ func TestRepoAdd(t *testing.T) { } defer ts.Stop() - repoFile := filepath.Join(ensure.TempDir(t), "repositories.yaml") + rootDir := ensure.TempDir(t) + repoFile := filepath.Join(rootDir, "repositories.yaml") const testRepoName = "test-name" @@ -65,6 +69,7 @@ func TestRepoAdd(t *testing.T) { noUpdate: true, repoFile: repoFile, } + os.Setenv(xdg.CacheHomeEnvVar, rootDir) if err := o.run(ioutil.Discard); err != nil { t.Error(err) @@ -79,6 +84,15 @@ func TestRepoAdd(t *testing.T) { t.Errorf("%s was not successfully inserted into %s", testRepoName, repoFile) } + idx := filepath.Join(helmpath.CachePath("repository"), helmpath.CacheIndexFile(testRepoName)) + if _, err := os.Stat(idx); os.IsNotExist(err) { + t.Errorf("Error cache index file was not created for repository %s", testRepoName) + } + idx = filepath.Join(helmpath.CachePath("repository"), helmpath.CacheChartsFile(testRepoName)) + if _, err := os.Stat(idx); os.IsNotExist(err) { + t.Errorf("Error cache charts file was not created for repository %s", testRepoName) + } + o.noUpdate = false if err := o.run(ioutil.Discard); err != nil { diff --git a/cmd/helm/repo_remove.go b/cmd/helm/repo_remove.go index c1ecb1829..ea7a5db24 100644 --- a/cmd/helm/repo_remove.go +++ b/cmd/helm/repo_remove.go @@ -76,7 +76,12 @@ func (o *repoRemoveOptions) run(out io.Writer) error { } func removeRepoCache(root, name string) error { - idx := filepath.Join(root, helmpath.CacheIndexFile(name)) + idx := filepath.Join(root, helmpath.CacheChartsFile(name)) + if _, err := os.Stat(idx); err == nil { + os.Remove(idx) + } + + idx = filepath.Join(root, helmpath.CacheIndexFile(name)) if _, err := os.Stat(idx); os.IsNotExist(err) { return nil } else if err != nil { diff --git a/cmd/helm/repo_remove_test.go b/cmd/helm/repo_remove_test.go index 3fdcbe9c3..85c76bb92 100644 --- a/cmd/helm/repo_remove_test.go +++ b/cmd/helm/repo_remove_test.go @@ -63,10 +63,13 @@ func TestRepoRemove(t *testing.T) { } idx := filepath.Join(rootDir, helmpath.CacheIndexFile(testRepoName)) - mf, _ := os.Create(idx) mf.Close() + idx2 := filepath.Join(rootDir, helmpath.CacheChartsFile(testRepoName)) + mf, _ = os.Create(idx2) + mf.Close() + b.Reset() if err := rmOpts.run(b); err != nil { @@ -77,7 +80,11 @@ func TestRepoRemove(t *testing.T) { } if _, err := os.Stat(idx); err == nil { - t.Errorf("Error cache file was not removed for repository %s", testRepoName) + t.Errorf("Error cache index file was not removed for repository %s", testRepoName) + } + + if _, err := os.Stat(idx2); err == nil { + t.Errorf("Error cache chart file was not removed for repository %s", testRepoName) } f, err := repo.LoadFile(repoFile) diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 3405299fd..bba3ff643 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -139,13 +139,25 @@ __helm_zsh_comp_nospace() { __helm_list_charts() { __helm_debug "${FUNCNAME[0]}: c is $c words[c] is ${words[c]}" - local repo url file out=() nospace=0 wantFiles=$1 + local prefix chart repo url file out=() nospace=0 wantFiles=$1 # Handle completions for repos for repo in $(__helm_get_repos); do if [[ "${cur}" =~ ^${repo}/.* ]]; then # We are doing completion from within a repo - out=$(eval $(__helm_binary_name) search repo ${cur} 2>/dev/null | \cut -f1 | \grep ^${cur}) + local cacheFile=$(eval $(__helm_binary_name) env 2>/dev/null | \grep HELM_REPOSITORY_CACHE | \cut -d= -f2 | \sed s/\"//g)/${repo}-charts.txt + if [ -f "$cacheFile" ]; then + # Get the list of charts from the cached file + prefix=${cur#${repo}/} + for chart in $(\grep ^$prefix $cacheFile); do + out+=(${repo}/${chart}) + done + else + # If there is no cached list file, fallback to helm search, which is much slower + # This will happen after the caching feature is first installed but before the user + # does a 'helm repo update' to generate the cached list file. + out=$(eval $(__helm_binary_name) search repo ${cur} 2>/dev/null | \cut -f1 | \grep ^${cur}) + fi nospace=0 elif [[ ${repo} =~ ^${cur}.* ]]; then # We are completing a repo name diff --git a/pkg/helmpath/home.go b/pkg/helmpath/home.go index 0b0f110a5..b3f14c558 100644 --- a/pkg/helmpath/home.go +++ b/pkg/helmpath/home.go @@ -25,10 +25,19 @@ func CachePath(elem ...string) string { return lp.cachePath(elem...) } // DataPath returns the path where Helm stores data. func DataPath(elem ...string) string { return lp.dataPath(elem...) } -// CacheIndex returns the path to an index for the given named repository. +// CacheIndexFile returns the path to an index for the given named repository. func CacheIndexFile(name string) string { if name != "" { name += "-" } return name + "index.yaml" } + +// CacheChartsFile returns the path to a text file listing all the charts +// within the given named repository. +func CacheChartsFile(name string) string { + if name != "" { + name += "-" + } + return name + "charts.txt" +} diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index c8d0d6a3d..38b6b8fb0 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -133,10 +133,21 @@ func (r *ChartRepository) DownloadIndexFile() (string, error) { return "", err } - if _, err := loadIndex(index); err != nil { + indexFile, err := loadIndex(index) + if err != nil { return "", err } + // Create the chart list file in the cache directory + var charts strings.Builder + for name := range indexFile.Entries { + fmt.Fprintln(&charts, name) + } + chartsFile := filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name)) + os.MkdirAll(filepath.Dir(chartsFile), 0755) + ioutil.WriteFile(chartsFile, []byte(charts.String()), 0644) + + // Create the index file in the cache directory fname := filepath.Join(r.CachePath, helmpath.CacheIndexFile(r.Config.Name)) os.MkdirAll(filepath.Dir(fname), 0755) return fname, ioutil.WriteFile(fname, index, 0644) diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index c830a339e..ab998ba54 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -17,14 +17,19 @@ limitations under the License. package repo import ( + "bufio" + "bytes" "io/ioutil" "net/http" "os" + "path/filepath" + "sort" "strings" "testing" "helm.sh/helm/v3/pkg/cli" "helm.sh/helm/v3/pkg/getter" + "helm.sh/helm/v3/pkg/helmpath" "helm.sh/helm/v3/pkg/chart" ) @@ -178,6 +183,18 @@ func TestDownloadIndexFile(t *testing.T) { t.Fatalf("Index %q failed to parse: %s", testfile, err) } verifyLocalIndex(t, i) + + // Check that charts file is also created + idx = filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name)) + if _, err := os.Stat(idx); err != nil { + t.Fatalf("error finding created charts file: %#v", err) + } + + b, err = ioutil.ReadFile(idx) + if err != nil { + t.Fatalf("error reading charts file: %#v", err) + } + verifyLocalChartsFile(t, b, i) }) t.Run("should not decode the path in the repo url while downloading index", func(t *testing.T) { @@ -224,6 +241,18 @@ func TestDownloadIndexFile(t *testing.T) { t.Fatalf("Index %q failed to parse: %s", testfile, err) } verifyLocalIndex(t, i) + + // Check that charts file is also created + idx = filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name)) + if _, err := os.Stat(idx); err != nil { + t.Fatalf("error finding created charts file: %#v", err) + } + + b, err = ioutil.ReadFile(idx) + if err != nil { + t.Fatalf("error reading charts file: %#v", err) + } + verifyLocalChartsFile(t, b, i) }) } @@ -322,6 +351,24 @@ func verifyLocalIndex(t *testing.T, i *IndexFile) { } } +func verifyLocalChartsFile(t *testing.T, chartsContent []byte, indexContent *IndexFile) { + var expected, real []string + for chart := range indexContent.Entries { + expected = append(expected, chart) + } + sort.Strings(expected) + + scanner := bufio.NewScanner(bytes.NewReader(chartsContent)) + for scanner.Scan() { + real = append(real, scanner.Text()) + } + sort.Strings(real) + + if strings.Join(expected, " ") != strings.Join(real, " ") { + t.Errorf("Cached charts file content unexpected. Expected:\n%s\ngot:\n%s", expected, real) + } +} + func TestIndexDirectory(t *testing.T) { dir := "testdata/repository" index, err := IndexDirectory(dir, "http://localhost:8080")