mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-21 09:12:13 +02:00
* fix(state): prefetch shared remote charts instead of serializing sync/diff (#2741) PR #2662 fixed a Windows chart-download race (#768) by wrapping the entire helm upgrade/diff operation in a per-chart+version lock, not just the download. Since sync/apply/diff never set ForceDownload, releases sharing a remote chart end up fully serialized even at high --concurrency. Add ChartPrepareOptions.PrefetchSharedRemoteCharts: PrepareCharts groups selected releases by chart+version, and force-downloads (once) any chart used by 2+ releases that also resolve to identical acquisition flags (--verify/--keyring/--plain-http/--insecure-skip-tls-verify/--devel) and to a configured repository (or OCI ref) - not a bare \"dir/chart\"-shaped local path. That materializes release.ChartPath, which lets withChartOperationLock's existing ChartPath != \"\" guard skip the lock, restoring concurrency without touching the #768 protection for charts that aren't prefetched. Also add chartFetchFlags to give forcedDownloadChart's \`helm fetch\` the same verify/keyring/TLS flags flagsForUpgrade already applies, closing a parity gap that predates this change (affects lint/unittest/pull too). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Thomas Hanser <gh@toms.place> * fix(state): exclude verify-enabled charts from shared-chart prefetch, use NUL-delimited flag signature Copilot review on #2741's PR flagged two issues in the shared-chart prefetch added there: 1. forcedDownloadChart untars a shared chart into a local directory, but flagsForUpgrade unconditionally re-adds --verify for the later `helm upgrade` regardless of ChartPath. Helm's VerifyChart only accepts a packaged .tgz/provenance pair, not an unpacked directory, so upgrading a prefetched chart with verify enabled would fail. Exclude --verify from prefetch eligibility entirely rather than trying to suppress the later flag - those releases just keep the pre-existing serialized-lock behavior, unaffected by this feature. 2. The per-key flag-agreement signature joined flags with a space, which isn't injective: a keyring path containing a space and a flag-like token could collide with a different keyring plus a real flag, silently deduplicating releases with different acquisition settings. Join with NUL instead, which can't appear in an OS argument. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Thomas Hanser <gh@toms.place> * fix(state): apply -chart override before shared-chart grouping Copilot flagged that PrepareCharts grouped releases by release.Chart before prepareChartForRelease applied st.OverrideChart (the -chart CLI flag), so distinct original charts that all resolve to the same overridden chart were never recognized as shared and missed the prefetch. Apply the override once upfront, before the grouping loop reads release.Chart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Thomas Hanser <gh@toms.place> --------- Signed-off-by: Thomas Hanser <gh@toms.place> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Thomas Hanser
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
228c8fe24f
commit
2cb878d5a0
+117
-8
@@ -15,6 +15,7 @@ import (
|
||||
"path/filepath"
|
||||
"regexp"
|
||||
"runtime"
|
||||
"slices"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
@@ -1526,11 +1527,15 @@ func filterReleasesForBuild(releases []ReleaseSpec) []ReleaseSpec {
|
||||
|
||||
type ChartPrepareOptions struct {
|
||||
ForceDownload bool
|
||||
SkipRepos bool
|
||||
SkipDeps bool
|
||||
SkipRefresh bool
|
||||
SkipResolve bool
|
||||
SkipCleanup bool
|
||||
// PrefetchSharedRemoteCharts forces a single materialized download for any remote chart
|
||||
// (chart+version) used by more than one selected release, so those releases stop being
|
||||
// serialized by withChartOperationLock during sync/diff. See issue #2741.
|
||||
PrefetchSharedRemoteCharts bool
|
||||
SkipRepos bool
|
||||
SkipDeps bool
|
||||
SkipRefresh bool
|
||||
SkipResolve bool
|
||||
SkipCleanup bool
|
||||
// SkipSchemaValidation configures chartify to pass --skip-schema-validation to helm-template run by it.
|
||||
SkipSchemaValidation bool
|
||||
// Validate configures chartify to pass --validate to helm-template run by it.
|
||||
@@ -2043,6 +2048,25 @@ func (st *HelmState) processLocalChart(normalizedChart, dir string, release *Rel
|
||||
return chartPath, nil
|
||||
}
|
||||
|
||||
// chartFetchFlags builds the `helm fetch` flags for a remote chart download, mirroring
|
||||
// the flags flagsForUpgrade applies for chart acquisition (version, verify, keyring,
|
||||
// TLS/plain-http) so a prefetched chart behaves identically to one helm would have
|
||||
// downloaded itself during `helm upgrade`. See issue #2741.
|
||||
func (st *HelmState) chartFetchFlags(release *ReleaseSpec) []string {
|
||||
var flags []string
|
||||
flags = st.appendChartVersionFlags(flags, release)
|
||||
|
||||
// non-OCI chart should be verified here, matching flagsForUpgrade.
|
||||
if !st.IsOCIChart(release.Chart) {
|
||||
flags = st.appendVerifyFlags(flags, release)
|
||||
flags = st.appendKeyringFlags(flags, release)
|
||||
}
|
||||
|
||||
flags = st.appendChartDownloadFlags(flags, release)
|
||||
|
||||
return flags
|
||||
}
|
||||
|
||||
// forcedDownloadChart handles forced chart downloads.
|
||||
// Locks are acquired during download and released immediately after.
|
||||
// A per-chart+version mutex serializes downloads within the process so that
|
||||
@@ -2100,8 +2124,7 @@ func (st *HelmState) forcedDownloadChart(chartName, dir string, release *Release
|
||||
}
|
||||
|
||||
// Download the chart
|
||||
var fetchFlags []string
|
||||
fetchFlags = st.appendChartVersionFlags(fetchFlags, release)
|
||||
fetchFlags := st.chartFetchFlags(release)
|
||||
fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath)
|
||||
if err := helm.Fetch(chartName, fetchFlags...); err != nil {
|
||||
lockResult.Release(st.logger)
|
||||
@@ -2258,10 +2281,78 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
|
||||
releases = filterReleasesForBuild(releases)
|
||||
}
|
||||
|
||||
// Apply -chart override here, before the shared-chart grouping below reads
|
||||
// release.Chart. It was previously applied later inside prepareChartForRelease,
|
||||
// which meant grouping ran on each release's original chart and never noticed
|
||||
// that all of them resolve to the same overridden chart.
|
||||
if st.OverrideChart != "" {
|
||||
for i := range releases {
|
||||
releases[i].Chart = st.OverrideChart
|
||||
}
|
||||
}
|
||||
|
||||
// Initialize the chartify temp dir tracker before concurrent workers start,
|
||||
// so that all workers share a single tracker instance. See issue #1799.
|
||||
st.chartifyTempDirs = &chartifyTempDirTracker{}
|
||||
|
||||
// Issue #2741: releases sharing a remote chart+version are otherwise serialized by
|
||||
// withChartOperationLock for the whole sync/diff, not just the download. Force a
|
||||
// single materialized download per shared chart key here so release.ChartPath gets
|
||||
// populated and withChartOperationLock's ChartPath != "" guard skips the lock for them.
|
||||
// Charts unique to one release are left alone to preserve current behavior.
|
||||
sharedChartKeys := map[ChartCacheKey]bool{}
|
||||
if opts.PrefetchSharedRemoteCharts {
|
||||
type prefetchStats struct {
|
||||
count int
|
||||
flagSigs map[string]bool
|
||||
sampleFlags []string
|
||||
}
|
||||
stats := map[ChartCacheKey]*prefetchStats{}
|
||||
for i := range releases {
|
||||
release := &releases[i]
|
||||
key := st.getChartCacheKey(release)
|
||||
s, ok := stats[key]
|
||||
if !ok {
|
||||
s = &prefetchStats{flagSigs: map[string]bool{}}
|
||||
stats[key] = s
|
||||
}
|
||||
s.count++
|
||||
flags := st.chartFetchFlags(release)
|
||||
// NUL can't appear in an OS argument, so this join is injective —
|
||||
// unlike a space-joined signature, it can't collide two different
|
||||
// flag slices (e.g. a keyring path containing a space and a
|
||||
// flag-like token) into the same string.
|
||||
s.flagSigs[strings.Join(flags, "\x00")] = true
|
||||
s.sampleFlags = flags
|
||||
}
|
||||
for key, s := range stats {
|
||||
// The download cache (checkChartCache/addToChartCache) is keyed by
|
||||
// chart+version alone, not by acquisition flags. If releases sharing a
|
||||
// chart+version disagree on --verify/--keyring/--plain-http/
|
||||
// --insecure-skip-tls-verify/--devel, whichever release's worker wins the
|
||||
// download race would silently decide those settings for the others. Only
|
||||
// prefetch when every release sharing this key resolves to identical flags;
|
||||
// otherwise fall back to today's per-release fetch (still correct, just not
|
||||
// deduplicated).
|
||||
//
|
||||
// Only chart strings that resolve to a configured repository (or an OCI
|
||||
// reference) are unambiguously remote. A bare "dir/chart"-shaped string with
|
||||
// no matching `repositories:` entry is conventionally a local chart path
|
||||
// (e.g. "charts/frontend") and must be left to the existing local-directory
|
||||
// resolution, not force-fetched as if it were a registry chart.
|
||||
//
|
||||
// --verify is also disqualifying: forcedDownloadChart untars the chart, and
|
||||
// flagsForUpgrade later re-adds --verify for the upgrade itself (it can't
|
||||
// tell the chart was already verified at fetch time). Helm's VerifyChart only
|
||||
// accepts a packaged .tgz/provenance pair, not an unpacked directory, so
|
||||
// upgrading a prefetched chart with verify enabled would fail. Leave verified
|
||||
// charts on the existing serialized remote-chart path instead.
|
||||
if s.count > 1 && len(s.flagSigs) == 1 && !slices.Contains(s.sampleFlags, "--verify") && st.isPrefetchEligibleChart(key.Chart) {
|
||||
sharedChartKeys[key] = true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var prepareChartInfoMutex sync.Mutex
|
||||
|
||||
prepareChartInfo := make(map[PrepareChartKey]string, len(releases))
|
||||
@@ -2284,7 +2375,11 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
|
||||
},
|
||||
func(workerIndex int) {
|
||||
for release := range jobQueue {
|
||||
result := st.prepareChartForRelease(release, helm, dir, helmfileCommand, opts, workerIndex)
|
||||
releaseOpts := opts
|
||||
if sharedChartKeys[st.getChartCacheKey(release)] {
|
||||
releaseOpts.ForceDownload = true
|
||||
}
|
||||
result := st.prepareChartForRelease(release, helm, dir, helmfileCommand, releaseOpts, workerIndex)
|
||||
results <- result
|
||||
}
|
||||
},
|
||||
@@ -6096,6 +6191,20 @@ func (st *HelmState) IsOCIChart(chart string) bool {
|
||||
return repo.OCI
|
||||
}
|
||||
|
||||
// isPrefetchEligibleChart returns true if chart unambiguously refers to a
|
||||
// remote source: an OCI reference, or a "repo/chart" string whose repo prefix
|
||||
// matches a configured `repositories:` entry. Used by PrepareCharts to decide
|
||||
// which shared chart keys are safe to force-download (see issue #2741) -
|
||||
// local chart paths conventionally shaped like "dir/chart" (e.g.
|
||||
// "charts/frontend") must not be mistaken for registry references.
|
||||
func (st *HelmState) isPrefetchEligibleChart(chart string) bool {
|
||||
if strings.HasPrefix(chart, "oci://") {
|
||||
return true
|
||||
}
|
||||
repo, _ := st.GetRepositoryAndNameFromChartName(chart)
|
||||
return repo != nil
|
||||
}
|
||||
|
||||
// NeedsRepoUpdate returns true if there are any repositories that require `helm repo update`.
|
||||
// OCI repositories don't need `helm repo update` as they use `helm registry login` instead.
|
||||
func (st *HelmState) NeedsRepoUpdate() bool {
|
||||
|
||||
Reference in New Issue
Block a user