Repository: helm/helm
Pull request #6809 (2019-10-29 03:15:45)

feat(comp): Speed up completion of charts
I feel that this improvement in performance when listing charts makes it much more useful to use shell completion when wanting to find a chart.

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.

EDIT: please see https://github.com/helm/helm/pull/6809#issuecomment-560977848 below for a description of the reworked PR.

~~This commit makes the completion logic access the correct repo cache file directly and only extract the chart names from it.~~

~~With only the stable repo configured, this optimization makes the completion of charts about 10 times faster, going from 1.1 seconds (which is quite slow for a user doing completion) to 0.14 seconds; such a difference give a much better user experience.~~

If other repos are configured, `helm search repo` only become slower, while the new completion logic will not be affected as it only looks for the relevant single repo file.

**Notes for maintainers**:

~~Can a maintainer tell me their opinion on the approach of using the cache file by:
1- extracting the cache directory by looking for HELM_REPOSITORY_CACHE in the output of `helm env`
2- greping for chart names (using `\egrep -e '^  [a-zA-Z]'` on the `$repo-index.yaml` cache file~~

~~I would have liked something a little more straightforward, but nothing better jumped out at me.~~

This change was tested for backwards-compatibility using the acceptance-testing repo

marckhouzam: Although technically not a bug fix, this is only a 2-line change that makes the completion of charts much nicer.  The change is safe as it can only affect completion.

I wonder if this could be considered for the 3.0 release?

The user experience would be much nicer because, when completing the name of a chart, (e.g. `helm install myrelease stable/ngi<TAB>` a user may press `<TAB>` multiple times; without this fix, each `<TAB>` will take around 2 seconds or more, which defeats the purpose of asking the computer to complete the word faster than if the user had typed it.
marckhouzam: I reworked this PR in a much less hacky fashion.

This new attempt has helm create a new cache file for each repo which contains the list of charts for that repo.  Helm will do this whenever it downloads the repo index file.

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 (e.g., `helm install nginx stable/ng<TAB>`).

On the other hand, adding the creation of the new chart-list cache file to 'helm update' or 'helm add' is pretty much negligible compared to the time spent parsing the index files.
marckhouzam: I've just added tests for this change and also fixed the fact that I had forgotten to remove the new cache file when doing a `helm repo remove`
marckhouzam: This last forced push was just to rebase and to correct the commit message.  New tests still pending.

Patch:
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/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)


Test patch:
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_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/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")