Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

helm: optimise repository index loading #685

Merged
merged 1 commit into from
Apr 26, 2022
Merged

helm: optimise repository index loading #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions api/v1beta2/artifact_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,24 @@ 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
}
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
Comment on lines +71 to +74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if in == nil {
return false
}
return in.Checksum == checksum
return in != nil && in.Checksum == checksum

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I do not use this for nil checks is that when logic changes, there is a chance someone accidentally omits the in != nil check, and introduces a panic.

}

// ArtifactDir returns the artifact dir path in the form of
// '<kind>/<namespace>/<name>'.
func ArtifactDir(kind, namespace, name string) string {
Expand Down
22 changes: 17 additions & 5 deletions controllers/helmrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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
}

Expand All @@ -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
}
Expand Down
5 changes: 3 additions & 2 deletions internal/helm/repository/chart_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down