Skip to content

Commit 1ce7939

Browse files
JvD-EricssonJvD-Ericsson
committed
Improve helm dependency update performance
What this PR does / why we need it: This PR was created to improve performance of the dependency update command by skipping unnecessary downloading and loading of index files that have already been downloaded and loaded I believe this would close refs #9865 Signed-off-by: Jeff van Dam <[email protected]>
1 parent b2d5235 commit 1ce7939

File tree

5 files changed

+67
-41
lines changed

5 files changed

+67
-41
lines changed

‎internal/resolver/resolver.go

+31-13
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,23 @@ func New(chartpath, cachepath string, registryClient *registry.Client) *Resolver
5252
}
5353

5454
// Resolve resolves dependencies and returns a lock file with the resolution.
55-
func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string) (*chart.Lock, error) {
55+
func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string) (*chart.Lock, map[string]string, error) {
5656

5757
// Now we clone the dependencies, locking as we go.
5858
locked := make([]*chart.Dependency, len(reqs))
5959
missing := []string{}
60+
loadedIndexFiles := make(map[string]*repo.IndexFile)
61+
urls := make(map[string]string)
6062
for i, d := range reqs {
6163
constraint, err := semver.NewConstraint(d.Version)
6264
if err != nil {
63-
return nil, errors.Wrapf(err, "dependency %q has an invalid version/constraint format", d.Name)
65+
return nil, nil, errors.Wrapf(err, "dependency %q has an invalid version/constraint format", d.Name)
6466
}
6567

6668
if d.Repository == "" {
6769
// Local chart subfolder
6870
if _, err := GetLocalPath(filepath.Join("charts", d.Name), r.chartpath); err != nil {
69-
return nil, err
71+
return nil, nil, err
7072
}
7173

7274
locked[i] = &chart.Dependency{
@@ -80,12 +82,12 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
8082

8183
chartpath, err := GetLocalPath(d.Repository, r.chartpath)
8284
if err != nil {
83-
return nil, err
85+
return nil, nil, err
8486
}
8587

8688
ch, err := loader.LoadDir(chartpath)
8789
if err != nil {
88-
return nil, err
90+
return nil, nil, err
8991
}
9092

9193
v, err := semver.NewVersion(ch.Metadata.Version)
@@ -123,14 +125,26 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
123125
var ok bool
124126
found := true
125127
if !registry.IsOCI(d.Repository) {
126-
repoIndex, err := repo.LoadIndexFile(filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName)))
127-
if err != nil {
128-
return nil, errors.Wrapf(err, "no cached repository for %s found. (try 'helm repo update')", repoName)
128+
filepath := filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName))
129+
var repoIndex *repo.IndexFile
130+
131+
// Store previously loaded index files in a map. If repositories share the
132+
// same index file there is no need to reload the same file again. This
133+
// improves performance.
134+
if indexFile, loaded := loadedIndexFiles[filepath]; !loaded {
135+
var err error
136+
repoIndex, err = repo.LoadIndexFile(filepath)
137+
loadedIndexFiles[filepath] = repoIndex
138+
if err != nil {
139+
return nil, nil, errors.Wrapf(err, "no cached repository for %s found. (try 'helm repo update')", repoName)
140+
}
141+
} else {
142+
repoIndex = indexFile
129143
}
130144

131145
vs, ok = repoIndex.Entries[d.Name]
132146
if !ok {
133-
return nil, errors.Errorf("%s chart not found in repo %s", d.Name, d.Repository)
147+
return nil, nil, errors.Errorf("%s chart not found in repo %s", d.Name, d.Repository)
134148
}
135149
found = false
136150
} else {
@@ -152,7 +166,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
152166
ref := fmt.Sprintf("%s/%s", strings.TrimPrefix(d.Repository, fmt.Sprintf("%s://", registry.OCIScheme)), d.Name)
153167
tags, err := r.registryClient.Tags(ref)
154168
if err != nil {
155-
return nil, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository)
169+
return nil, nil, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository)
156170
}
157171

158172
vs = make(repo.ChartVersions, len(tags))
@@ -173,6 +187,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
173187
Repository: d.Repository,
174188
Version: version,
175189
}
190+
176191
// The version are already sorted and hence the first one to satisfy the constraint is used
177192
for _, ver := range vs {
178193
v, err := semver.NewVersion(ver.Version)
@@ -183,6 +198,9 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
183198
}
184199
if constraint.Check(v) {
185200
found = true
201+
if len(ver.URLs) > 0 {
202+
urls[ver.Name] = ver.URLs[0]
203+
}
186204
locked[i].Version = v.Original()
187205
break
188206
}
@@ -193,19 +211,19 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
193211
}
194212
}
195213
if len(missing) > 0 {
196-
return nil, errors.Errorf("can't get a valid version for repositories %s. Try changing the version constraint in Chart.yaml", strings.Join(missing, ", "))
214+
return nil, nil, errors.Errorf("can't get a valid version for repositories %s. Try changing the version constraint in Chart.yaml", strings.Join(missing, ", "))
197215
}
198216

199217
digest, err := HashReq(reqs, locked)
200218
if err != nil {
201-
return nil, err
219+
return nil, nil, err
202220
}
203221

204222
return &chart.Lock{
205223
Generated: time.Now(),
206224
Digest: digest,
207225
Dependencies: locked,
208-
}, nil
226+
}, urls, nil
209227
}
210228

211229
// HashReq generates a hash of the dependencies.

‎internal/resolver/resolver_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func TestResolve(t *testing.T) {
144144
r := New("testdata/chartpath", "testdata/repository", registryClient)
145145
for _, tt := range tests {
146146
t.Run(tt.name, func(t *testing.T) {
147-
l, err := r.Resolve(tt.req, repoNames)
147+
l, _, err := r.Resolve(tt.req, repoNames)
148148
if err != nil {
149149
if tt.err {
150150
return

‎pkg/downloader/chart_downloader.go

+9-16
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
"github.com/pkg/errors"
2828

2929
"helm.sh/helm/v3/internal/fileutil"
30-
"helm.sh/helm/v3/internal/urlutil"
3130
"helm.sh/helm/v3/pkg/getter"
3231
"helm.sh/helm/v3/pkg/helmpath"
3332
"helm.sh/helm/v3/pkg/provenance"
@@ -184,11 +183,11 @@ func (c *ChartDownloader) getOciURI(ref, version string, u *url.URL) (*url.URL,
184183
//
185184
// A version is a SemVer string (1.2.3-beta.1+f334a6789).
186185
//
187-
// - For fully qualified URLs, the version will be ignored (since URLs aren't versioned)
188-
// - For a chart reference
189-
// * If version is non-empty, this will return the URL for that version
190-
// * If version is empty, this will return the URL for the latest version
191-
// * If no version can be found, an error is returned
186+
// - For fully qualified URLs, the version will be ignored (since URLs aren't versioned)
187+
// - For a chart reference
188+
// - If version is non-empty, this will return the URL for that version
189+
// - If version is empty, this will return the URL for the latest version
190+
// - If no version can be found, an error is returned
192191
func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, error) {
193192
u, err := url.Parse(ref)
194193
if err != nil {
@@ -378,19 +377,13 @@ func (c *ChartDownloader) scanReposForURL(u string, rf *repo.File) (*repo.Entry,
378377
}
379378

380379
idxFile := filepath.Join(c.RepositoryCache, helmpath.CacheIndexFile(r.Config.Name))
381-
i, err := repo.LoadIndexFile(idxFile)
380+
yamlFile, err := os.ReadFile(idxFile)
382381
if err != nil {
383382
return nil, errors.Wrap(err, "no cached repo found. (try 'helm repo update')")
384383
}
385-
386-
for _, entry := range i.Entries {
387-
for _, ver := range entry {
388-
for _, dl := range ver.URLs {
389-
if urlutil.Equal(u, dl) {
390-
return rc, nil
391-
}
392-
}
393-
}
384+
file := string(yamlFile[:])
385+
if strings.Contains(file, u) {
386+
return rc, nil
394387
}
395388
}
396389
// This means that there is no repo file for the given URL.

‎pkg/downloader/manager.go

+23-8
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func (m *Manager) Build() error {
142142
}
143143

144144
// Now we need to fetch every package here into charts/
145-
return m.downloadAll(lock.Dependencies)
145+
return m.downloadAll(lock.Dependencies, nil)
146146
}
147147

148148
// Update updates a local charts directory.
@@ -192,13 +192,13 @@ func (m *Manager) Update() error {
192192

193193
// Now we need to find out which version of a chart best satisfies the
194194
// dependencies in the Chart.yaml
195-
lock, err := m.resolve(req, repoNames)
195+
lock, urls, err := m.resolve(req, repoNames)
196196
if err != nil {
197197
return err
198198
}
199199

200200
// Now we need to fetch every package here into charts/
201-
if err := m.downloadAll(lock.Dependencies); err != nil {
201+
if err := m.downloadAll(lock.Dependencies, urls); err != nil {
202202
return err
203203
}
204204

@@ -231,7 +231,7 @@ func (m *Manager) loadChartDir() (*chart.Chart, error) {
231231
// resolve takes a list of dependencies and translates them into an exact version to download.
232232
//
233233
// This returns a lock file, which has all of the dependencies normalized to a specific version.
234-
func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) (*chart.Lock, error) {
234+
func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) (*chart.Lock, map[string]string, error) {
235235
res := resolver.New(m.ChartPath, m.RepositoryCache, m.RegistryClient)
236236
return res.Resolve(req, repoNames)
237237
}
@@ -240,7 +240,7 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string)
240240
//
241241
// It will delete versions of the chart that exist on disk and might cause
242242
// a conflict.
243-
func (m *Manager) downloadAll(deps []*chart.Dependency) error {
243+
func (m *Manager) downloadAll(deps []*chart.Dependency, urls map[string]string) error {
244244
repos, err := m.loadChartRepositories()
245245
if err != nil {
246246
return err
@@ -313,7 +313,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
313313

314314
// Any failure to resolve/download a chart should fail:
315315
// https://github.com/helm/helm/issues/1439
316-
churl, username, password, insecureskiptlsverify, passcredentialsall, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos)
316+
churl, username, password, insecureskiptlsverify, passcredentialsall, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos, urls)
317317
if err != nil {
318318
saveError = errors.Wrapf(err, "could not find %s", churl)
319319
break
@@ -502,6 +502,7 @@ func (m *Manager) ensureMissingRepos(repoNames map[string]string, deps []*chart.
502502

503503
var ru []*repo.Entry
504504

505+
Outer:
505506
for _, dd := range deps {
506507

507508
// If the chart is in the local charts directory no repository needs
@@ -529,6 +530,14 @@ func (m *Manager) ensureMissingRepos(repoNames map[string]string, deps []*chart.
529530

530531
repoNames[dd.Name] = rn
531532

533+
// If repository is already present don't add to array. This will skip
534+
// unnecessary index file downloading improving performance.
535+
for _, item := range ru {
536+
if item.URL == dd.Repository {
537+
continue Outer
538+
}
539+
}
540+
532541
// Assuming the repository is generally available. For Helm managed
533542
// access controls the repository needs to be added through the user
534543
// managed system. This path will work for public charts, like those
@@ -703,7 +712,7 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
703712
// repoURL is the repository to search
704713
//
705714
// If it finds a URL that is "relative", it will prepend the repoURL.
706-
func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) {
715+
func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository, urls map[string]string) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) {
707716
if registry.IsOCI(repoURL) {
708717
return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, "", "", "", nil
709718
}
@@ -735,7 +744,13 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*
735744
return
736745
}
737746
}
738-
url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters)
747+
748+
if _, ok := urls[name]; ok {
749+
url = urls[name]
750+
} else {
751+
url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters)
752+
}
753+
739754
if err == nil {
740755
return url, username, password, false, false, "", "", "", err
741756
}

‎pkg/downloader/manager_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func TestFindChartURL(t *testing.T) {
8484
version := "0.1.0"
8585
repoURL := "http://example.com/charts"
8686

87-
churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos)
87+
churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos, make(map[string]string))
8888
if err != nil {
8989
t.Fatal(err)
9090
}
@@ -109,7 +109,7 @@ func TestFindChartURL(t *testing.T) {
109109
version = "1.2.3"
110110
repoURL = "https://example-https-insecureskiptlsverify.com"
111111

112-
churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos)
112+
churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos, make(map[string]string))
113113
if err != nil {
114114
t.Fatal(err)
115115
}
@@ -255,7 +255,7 @@ func TestDownloadAll(t *testing.T) {
255255
if err := os.MkdirAll(filepath.Join(chartPath, "tmpcharts"), 0755); err != nil {
256256
t.Fatal(err)
257257
}
258-
if err := m.downloadAll([]*chart.Dependency{signDep, localDep}); err != nil {
258+
if err := m.downloadAll([]*chart.Dependency{signDep, localDep}, make(map[string]string)); err != nil {
259259
t.Error(err)
260260
}
261261

0 commit comments

Comments
 (0)