diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 1c68c621c..2611cf257 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -43,17 +43,41 @@ const ( // of a Source's Artifact. // If True, the Source can be in an ArtifactOutdatedCondition. BuildFailedCondition string = "BuildFailed" + + // StorageOperationFailedCondition indicates a transient or persistent + // failure related to storage. If True, the reconciliation failed while + // performing some filesystem operation. + StorageOperationFailedCondition string = "StorageOperationFailed" ) const ( // URLInvalidReason signals that a given Source has an invalid URL. URLInvalidReason string = "URLInvalid" - // StorageOperationFailedReason signals a failure caused by a storage - // operation. - StorageOperationFailedReason string = "StorageOperationFailed" - // AuthenticationFailedReason signals that a Secret does not have the // required fields, or the provided credentials do not match. AuthenticationFailedReason string = "AuthenticationFailed" + + // DirCreationFailedReason signals a failure caused by a directory creation + // operation. + DirCreationFailedReason string = "DirectoryCreationFailed" + + // StatOperationFailedReason signals a failure caused by a stat operation on + // a path. + StatOperationFailedReason string = "StatOperationFailed" + + // ReadOperationFailedReason signals a failure caused by a read operation. + ReadOperationFailedReason string = "ReadOperationFailed" + + // AcquireLockFailedReason signals a failure in acquiring lock. + AcquireLockFailedReason string = "AcquireLockFailed" + + // InvalidPathReason signals a failure caused by an invalid path. + InvalidPathReason string = "InvalidPath" + + // ArchiveOperationFailedReason signals a failure in archive operation. + ArchiveOperationFailedReason string = "ArchiveOperationFailed" + + // SymlinkUpdateFailedReason signals a failure in updating a symlink. + SymlinkUpdateFailedReason string = "SymlinkUpdateFailed" ) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 7c8b40516..80f99e6ff 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -74,21 +74,24 @@ const maxConcurrentBucketFetches = 100 var bucketReadyCondition = summarize.Conditions{ Target: meta.ReadyCondition, Owned: []string{ - sourcev1.ArtifactOutdatedCondition, sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, }, Summarize: []string{ - sourcev1.ArtifactOutdatedCondition, sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ - sourcev1.ArtifactOutdatedCondition, sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -313,16 +316,19 @@ func (r *BucketReconciler) reconcile(ctx context.Context, obj *sourcev1.Bucket, // Create temp working dir tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-%s-", obj.Kind, obj.Namespace, obj.Name)) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to create temporary directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + e := &serror.Event{ + Err: fmt.Errorf("failed to create temporary working directory: %w", err), + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } defer func() { if err = os.RemoveAll(tmpDir); err != nil { ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary working directory") } }() + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) // Run the sub-reconcilers and build the result of reconciliation. var ( @@ -521,23 +527,29 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("failed to stat source path: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.StatOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } else if !f.IsDir() { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("source path '%s' is not a directory", dir), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.InvalidPathReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Ensure artifact directory exists and acquire lock if err := r.Storage.MkdirAll(artifact); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) if err != nil { @@ -550,10 +562,12 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. // Archive directory to storage if err := r.Storage.Archive(&artifact, dir, nil); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("unable to archive artifact to storage: %s", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ArchiveOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } r.annotatedEventLogf(ctx, obj, map[string]string{ "revision": artifact.Revision, @@ -566,12 +580,13 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. // Update symlink on a "best effort" basis url, err := r.Storage.Symlink(artifact, "latest.tar.gz") if err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason, "failed to update status URL symlink: %s", err) } if url != "" { obj.Status.URL = url } + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) return sreconcile.ResultSuccess, nil } diff --git a/controllers/bucket_controller_test.go b/controllers/bucket_controller_test.go index 8f783e629..0732f1f2b 100644 --- a/controllers/bucket_controller_test.go +++ b/controllers/bucket_controller_test.go @@ -947,6 +947,9 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.StatOperationFailedReason, "failed to stat source path"), + }, }, { name: "Dir path is not a directory", @@ -963,6 +966,9 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "is not a directory"), + }, }, } diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 5564b836a..6161d0412 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -63,6 +63,7 @@ var gitRepositoryReadyCondition = summarize.Conditions{ sourcev1.FetchFailedCondition, sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, @@ -72,6 +73,7 @@ var gitRepositoryReadyCondition = summarize.Conditions{ sourcev1.SourceVerifiedCondition, sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -79,6 +81,7 @@ var gitRepositoryReadyCondition = summarize.Conditions{ sourcev1.FetchFailedCondition, sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -213,16 +216,19 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G // Create temp dir for Git clone tmpDir, err := util.TempDirForObj("", obj) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to create temporary directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + e := &serror.Event{ + Err: fmt.Errorf("failed to create temporary working directory: %w", err), + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } defer func() { if err = os.RemoveAll(tmpDir); err != nil { ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary working directory") } }() + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) // Run the sub-reconcilers and build the result of reconciliation. var ( @@ -322,7 +328,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return error as the world as observed may change return sreconcile.ResultEmpty, e } @@ -338,7 +344,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, Err: fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return error as the contents of the secret may change return sreconcile.ResultEmpty, e } @@ -358,7 +364,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), Reason: sourcev1.GitOperationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Do not return err as recovery without changes is impossible return sreconcile.ResultEmpty, e } @@ -372,7 +378,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, Err: fmt.Errorf("failed to checkout and determine revision: %w", err), Reason: sourcev1.GitOperationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e } @@ -429,15 +435,17 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { e := &serror.Event{ - Err: fmt.Errorf("failed to stat target path: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Err: fmt.Errorf("failed to stat target artifact path: %w", err), + Reason: sourcev1.StatOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } else if !f.IsDir() { e := &serror.Event{ Err: fmt.Errorf("invalid target path: '%s' is not a directory", dir), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.InvalidPathReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -445,8 +453,9 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, if err := r.Storage.MkdirAll(artifact); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) @@ -472,10 +481,12 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Archive directory to storage if err := r.Storage.Archive(&artifact, dir, SourceIgnoreFilter(ps, nil)); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("unable to archive artifact to storage: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ArchiveOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } r.AnnotatedEventf(obj, map[string]string{ "revision": artifact.Revision, @@ -489,12 +500,13 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Update symlink on a "best effort" basis url, err := r.Storage.Symlink(artifact, "latest.tar.gz") if err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason, "failed to update status URL symlink: %s", err) } if url != "" { obj.Status.URL = url } + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) return sreconcile.ResultSuccess, nil } @@ -520,7 +532,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, Err: fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err), Reason: "IllegalPath", } - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "IllegalPath", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -531,7 +543,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, Err: fmt.Errorf("could not get resource for include '%s': %w", incl.GitRepositoryRef.Name, err), Reason: "NotFound", } - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NotFound", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, err } @@ -541,7 +553,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, Err: fmt.Errorf("no artifact available for include '%s'", incl.GitRepositoryRef.Name), Reason: "NoArtifact", } - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NoArtifact", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -551,7 +563,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), Reason: "CopyFailure", } - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "CopyFailure", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } artifacts[i] = dep.GetArtifact().DeepCopy() @@ -598,7 +610,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj Err: fmt.Errorf("PGP public keys secret error: %w", err), Reason: "VerificationError", } - conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, meta.FailedReason, e.Err.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -612,7 +624,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj Err: fmt.Errorf("signature verification of commit '%s' failed: %w", commit.Hash.String(), err), Reason: "InvalidCommitSignature", } - conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, meta.FailedReason, e.Err.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) // Return error in the hope the secret changes return sreconcile.ResultEmpty, e } diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 8117e8d7c..59d17ea16 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -812,11 +812,17 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { name: "Target path does not exists", dir: "testdata/git/foo", wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.StatOperationFailedReason, "failed to stat target artifact path"), + }, }, { name: "Target path is not a directory", dir: "testdata/git/repository/foo.txt", wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "invalid target path"), + }, }, } artifactSize := func(g *WithT, artifactURL string) *int64 { @@ -1172,7 +1178,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, meta.FailedReason, "signature verification of commit 'shasum' failed: failed to verify commit with any of the given key rings"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: failed to verify commit with any of the given key rings"), }, }, { @@ -1188,7 +1194,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, meta.FailedReason, "PGP public keys secret error: secrets \"none-existing\" not found"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerificationError", "PGP public keys secret error: secrets \"none-existing\" not found"), }, }, { diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 7ea13ac60..5e9d5b2b8 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -72,6 +72,7 @@ var helmChartReadyCondition = summarize.Conditions{ sourcev1.BuildFailedCondition, sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, @@ -80,6 +81,7 @@ var helmChartReadyCondition = summarize.Conditions{ sourcev1.BuildFailedCondition, sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -87,6 +89,7 @@ var helmChartReadyCondition = summarize.Conditions{ sourcev1.BuildFailedCondition, sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -305,7 +308,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1 Err: fmt.Errorf("failed to get source: %w", err), Reason: "SourceUnavailable", } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "SourceUnavailable", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return Kubernetes client errors, but ignore others which can only be // solved by a change in generation @@ -395,7 +398,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Err: fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return error as the world as observed may change return sreconcile.ResultEmpty, e } @@ -407,7 +410,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Requeue as content of secret might change return sreconcile.ResultEmpty, e } @@ -419,7 +422,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Requeue as content of secret might change return sreconcile.ResultEmpty, e } @@ -436,14 +439,14 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Err: fmt.Errorf("invalid Helm repository URL: %w", err), Reason: sourcev1.URLInvalidReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e default: e := &serror.Stalling{ Err: fmt.Errorf("failed to construct Helm client: %w", err), Reason: meta.FailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } } @@ -486,9 +489,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create temporary working directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } defer os.RemoveAll(tmpDir) @@ -498,9 +501,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj if err := os.Mkdir(sourceDir, 0700); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create directory to untar source into: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -509,9 +512,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to open source artifact: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ReadOperationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } if _, err = untar.Untar(f, sourceDir); err != nil { @@ -535,7 +538,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj Err: fmt.Errorf("path calculation for chart '%s' failed: %w", obj.Spec.Chart, err), Reason: "IllegalPath", } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "IllegalPath", e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // We are unable to recover from this change without a change in generation return sreconcile.ResultEmpty, e } @@ -639,26 +642,32 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source // Ensure artifact directory exists and acquire lock if err := r.Storage.MkdirAll(artifact); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.AcquireLockFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } defer unlock() // Copy the packaged chart to the artifact path if err = r.Storage.CopyFromPath(&artifact, b.Path); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("unable to copy Helm chart to storage: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ArchiveOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Record it on the object @@ -674,12 +683,13 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source // Update symlink on a "best effort" basis symURL, err := r.Storage.Symlink(artifact, "latest.tar.gz") if err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason, "failed to update status URL symlink: %s", err) } if symURL != "" { obj.Status.URL = symURL } + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) return sreconcile.ResultSuccess, nil } diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index b4f68c0f5..82bae2ac5 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -59,6 +59,7 @@ var helmRepositoryReadyCondition = summarize.Conditions{ Owned: []string{ sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, @@ -66,12 +67,14 @@ var helmRepositoryReadyCondition = summarize.Conditions{ Summarize: []string{ sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.StorageOperationFailedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -289,7 +292,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -300,7 +303,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return err as the content of the secret may change. return sreconcile.ResultEmpty, e } @@ -312,7 +315,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Requeue as content of secret might change return sreconcile.ResultEmpty, e } @@ -327,14 +330,14 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou Err: fmt.Errorf("invalid Helm repository URL: %w", err), Reason: sourcev1.URLInvalidReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e default: e := &serror.Stalling{ Err: fmt.Errorf("failed to construct Helm client: %w", err), Reason: meta.FailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } } @@ -344,7 +347,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou Err: fmt.Errorf("failed to fetch Helm repository index: %w", err), Reason: meta.FailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e } @@ -354,9 +357,9 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou if err := chartRepo.LoadFromCache(); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to load Helm repository from cache: %w", err), - Reason: sourcev1.FetchFailedCondition, + Reason: sourcev1.IndexationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.IndexationFailedReason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } defer chartRepo.Unload() @@ -409,10 +412,12 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s // Create artifact dir if err := r.Storage.MkdirAll(*artifact); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.DirCreationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Acquire lock. @@ -427,19 +432,23 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s // Save artifact to storage. if err = r.Storage.CopyFromPath(artifact, chartRepo.CachePath); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("unable to save artifact to storage: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ArchiveOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Calculate the artifact size to be included in the NewArtifact event. fi, err := os.Stat(chartRepo.CachePath) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ + e := &serror.Event{ Err: fmt.Errorf("unable to read the artifact: %w", err), - Reason: sourcev1.StorageOperationFailedReason, + Reason: sourcev1.ReadOperationFailedReason, } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } size := units.HumanSize(float64(fi.Size())) @@ -454,13 +463,13 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s // Update index symlink. indexURL, err := r.Storage.Symlink(*artifact, "index.yaml") if err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason, "failed to update status URL symlink: %s", err) } if indexURL != "" { obj.Status.URL = indexURL } - + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) return sreconcile.ResultSuccess, nil } diff --git a/docs/spec/v1beta2/buckets.md b/docs/spec/v1beta2/buckets.md index 7d75d342e..7fc630989 100644 --- a/docs/spec/v1beta2/buckets.md +++ b/docs/spec/v1beta2/buckets.md @@ -903,17 +903,20 @@ without completing. This can occur due to some of the following factors: non-existing Secret. - The credentials in the referenced Secret are invalid. - The Bucket spec contains a generic misconfiguration. +- A storage related failure when storing the artifact. When this happens, the controller sets the `Ready` Condition status to `False`, and adds a Condition with the following attributes to the Bucket's `.status.conditions`: -- `type: FetchFailed` +- `type: FetchFailed` | `type: StorageOperationFailed` - `status: "True"` - `reason: AuthenticationFailed` | `reason: BucketOperationFailed` This condition has a ["negative polarity"][typical-status-properties], and is only present on the Bucket while the status value is `"True"`. +There may be more arbitrary values for the `reason` field to provide accurate +reason for a condition. While the Bucket has this Condition, the controller will continue to attempt to produce an Artifact for the resource with an exponential backoff, until diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 7e59c294a..fd30b6cfd 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -763,17 +763,20 @@ factors: - The verification of the Git commit signature failed. - The credentials in the referenced Secret are invalid. - The GitRepository spec contains a generic misconfiguration. +- A storage related failure when storing the artifact. When this happens, the controller sets the `Ready` Condition status to `False`, and adds a Condition with the following attributes to the GitRepository's `.status.conditions`: -- `type: FetchFailed` | `type: IncludeUnavailableCondition` +- `type: FetchFailed` | `type: IncludeUnavailable` | `type: StorageOperationFailed` - `status: "True"` -- `reason: AuthenticationFailed` | `reason: GitOperationFailed` | `reason: StorageOperationFailed` +- `reason: AuthenticationFailed` | `reason: GitOperationFailed` This condition has a ["negative polarity"][typical-status-properties], and is only present on the GitRepository while the status value is `"True"`. +There may be more arbitrary values for the `reason` field to provide accurate +reason for a condition. In addition to the above Condition types, when the [verification of a Git commit signature](#verification) fails. A condition with diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index ead65545e..b3f118ab6 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -532,17 +532,20 @@ factors: - The credentials in the [Source reference](#source-reference) Secret are invalid. - The HelmChart spec contains a generic misconfiguration. +- A storage related failure when storing the artifact. When this happens, the controller sets the `Ready` Condition status to `False`, and adds a Condition with the following attributes to the HelmChart's `.status.conditions`: -- `type: FetchFailed` +- `type: FetchFailed` | `type: StorageOperationFailed` - `status: "True"` - `reason: AuthenticationFailed` | `reason: StorageOperationFailed` | `reason: URLInvalid` | `reason: IllegalPath` | `reason: Failed` This condition has a ["negative polarity"][typical-status-properties], and is only present on the HelmChart while the status value is `"True"`. +There may be more arbitrary values for the `reason` field to provide accurate +reason for a condition. While the HelmChart has this Condition, the controller will continue to attempt to produce an Artifact for the resource with an exponential backoff, diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index e59fcb978..b3ef08f66 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -475,17 +475,20 @@ factors: non-existing Secret. - The credentials in the referenced Secret are invalid. - The HelmRepository spec contains a generic misconfiguration. +- A storage related failure when storing the artifact. When this happens, the controller sets the `Ready` Condition status to `False`, and adds a Condition with the following attributes to the HelmRepository's `.status.conditions`: -- `type: FetchFailed` +- `type: FetchFailed` | `type: StorageOperationFailed` - `status: "True"` - `reason: AuthenticationFailed` | `reason: IndexationFailed` | `reason: Failed` This condition has a ["negative polarity"][typical-status-properties], and is only present on the HelmRepository while the status value is `"True"`. +There may be more arbitrary values for the `reason` field to provide accurate +reason for a condition. While the HelmRepository has this Condition, the controller will continue to attempt to produce an Artifact for the resource with an exponential backoff,