helm: optimise repository index loading

Avoid validating (and thus loading) indexes if the checksum already exists in storage.
In other words, if the YAML is identical to the Artifact in storage, the reconciliation should
be a no-op, and therefore can short-circuit long/heavy operations.

Co-authored-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>

api/v1beta2/artifact_types.go

@@ -56,8 +56,8 @@ type Artifact struct {
 	Size *int64 `json:"size,omitempty"`
 }
 
-// HasRevision returns true if the given revision matches the current Revision
-// of the Artifact.
+// HasRevision returns if the given revision matches the current Revision of
+// the Artifact.
 func (in *Artifact) HasRevision(revision string) bool {
 	if in == nil {
 		return false
@@ -65,6 +65,15 @@ func (in *Artifact) HasRevision(revision string) bool {
 	return in.Revision == revision
 }
 
+// HasChecksum returns if the given checksum matches the current Checksum of
+// the Artifact.
+func (in *Artifact) HasChecksum(checksum string) bool {
+	if in == nil {
+		return false
+	}
+	return in.Checksum == checksum
+}
+
 // ArtifactDir returns the artifact dir path in the form of
 // '<kind>/<namespace>/<name>'.
 func ArtifactDir(kind, namespace, name string) string {

controllers/helmrepository_controller.go

@@ -398,6 +398,8 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 			return sreconcile.ResultEmpty, e
 		}
 	}
+
+	// Fetch the repository index from remote.
 	checksum, err := newChartRepo.CacheIndex()
 	if err != nil {
 		e := &serror.Event{
@@ -410,6 +412,15 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 	}
 	*chartRepo = *newChartRepo
 
+	// Short-circuit based on the fetched index being an exact match to the
+	// stored Artifact. This prevents having to unmarshal the YAML to calculate
+	// the (stable) revision, which is a memory expensive operation.
+	if obj.GetArtifact().HasChecksum(checksum) {
+		*artifact = *obj.GetArtifact()
+		conditions.Delete(obj, sourcev1.FetchFailedCondition)
+		return sreconcile.ResultSuccess, nil
+	}
+
 	// Load the cached repository index to ensure it passes validation.
 	if err := chartRepo.LoadFromCache(); err != nil {
 		e := &serror.Event{
@@ -419,23 +430,24 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
-	defer chartRepo.Unload()
+	chartRepo.Unload()
 
 	// Mark observations about the revision on the object.
-	if !obj.GetArtifact().HasRevision(checksum) {
+	if !obj.GetArtifact().HasRevision(newChartRepo.Checksum) {
 		message := fmt.Sprintf("new index revision '%s'", checksum)
 		conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
 		conditions.MarkReconciling(obj, "NewRevision", message)
 	}
 
-	conditions.Delete(obj, sourcev1.FetchFailedCondition)
-
 	// Create potential new artifact.
 	*artifact = r.Storage.NewArtifactFor(obj.Kind,
 		obj.ObjectMeta.GetObjectMeta(),
 		chartRepo.Checksum,
 		fmt.Sprintf("index-%s.yaml", checksum))
 
+	// Delete any stale failure observation
+	conditions.Delete(obj, sourcev1.FetchFailedCondition)
+
 	return sreconcile.ResultSuccess, nil
 }
 
@@ -462,7 +474,7 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s
 		}
 	}()
 
-	if obj.GetArtifact().HasRevision(artifact.Revision) {
+	if obj.GetArtifact().HasRevision(artifact.Revision) && obj.GetArtifact().HasChecksum(artifact.Checksum) {
 		r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
 		return sreconcile.ResultSuccess, nil
 	}

internal/helm/repository/chart_repository.go

@@ -66,7 +66,8 @@ type ChartRepository struct {
 	// Index contains a loaded chart repository index if not nil.
 	Index *repo.IndexFile
 	// Checksum contains the SHA256 checksum of the loaded chart repository
-	// index bytes.
+	// index bytes. This is different from the checksum of the CachePath, which
+	// may contain unordered entries.
 	Checksum string
 
 	tlsConfig *tls.Config
@@ -87,7 +88,7 @@ type cacheInfo struct {
 	RecordIndexCacheMetric RecordMetricsFunc
 }
 
-// ChartRepositoryOptions is a function that can be passed to NewChartRepository
+// ChartRepositoryOption is a function that can be passed to NewChartRepository
 // to configure a ChartRepository.
 type ChartRepositoryOption func(*ChartRepository) error