Skip to content

Commit

Permalink
Remove event error
Browse files Browse the repository at this point in the history 
Remove deprecated Event error. Event error was used for scenarios where
an error should result in an event/notification. It was introduced as a
contextual error along with Stalling and Waiting errors but was later
replaced with Generic error which doesn't have any contextual meaning.
The Generic error provided error configuration which allowed defining
how the error should be handled. This replaced the contextual error
handling with error action handlers which behaved on the error
configuration of the errors.

The Generic error was first introduced to be used in GitRepository
reconciler and was used by new reconcilers like the OCIRepository
reconcilers. The old reconcilers bucket, helmrepository and helmchart
reconcilers were still using the deprecated Event error. This change
replaces the Event errors in these reconcilers with a Generic error.

It also fixes a bug in the Generic error constructor which configured
the error to be logged by default. This resulted in an error to be
logged by the result processor and the runtime, double logging. This
behavior has been changed to not log explicitly and allow the runtime to
log the error. Since the Generic error is based on defining the error
handling behavior in the error configuration, a generic error that needs
to be ignored (not returned to the runtime), but logged can enable the
logging behavior explicitly on the Generic error instance. This is done
in GitRepository reconciler for no-op reconciliations where an ignore
error is returned.

Signed-off-by: Sunny <[email protected]>
  • Loading branch information
@darkowlzz
darkowlzz committed Sep 12, 2023
1 parent 900411f commit 5caa076
Show file tree
Hide file tree
Showing 8 changed files with 190 additions and 231 deletions.
84 changes: 42 additions & 42 deletions internal/controller/bucket_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
summarize.WithReconcileError(retErr),
summarize.WithIgnoreNotFound(),
summarize.WithProcessors(
summarize.RecordContextualError,
summarize.ErrorActionHandler,
summarize.RecordReconcileReq,
),
summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{
Expand Down Expand Up @@ -279,10 +279,10 @@ func (r *BucketReconciler) reconcile(ctx context.Context, sp *patch.SerialPatche
// Create temp working dir
tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-%s-", obj.Kind, obj.Namespace, obj.Name))
if err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to create temporary working directory: %w", err),
Reason: sourcev1.DirCreationFailedReason,
}
e := serror.NewGeneric(
fmt.Errorf("failed to create temporary working directory: %w", err),
sourcev1.DirCreationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
Expand Down Expand Up @@ -423,7 +423,7 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria
func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *bucketv1.Bucket, index *index.Digester, dir string) (sreconcile.Result, error) {
secret, err := r.getBucketSecret(ctx, obj)
if err != nil {
e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason}
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
Expand All @@ -434,42 +434,42 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
switch obj.Spec.Provider {
case bucketv1.GoogleBucketProvider:
if err = gcp.ValidateSecret(secret); err != nil {
e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason}
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
if provider, err = gcp.NewClient(ctx, secret); err != nil {
e := &serror.Event{Err: err, Reason: "ClientError"}
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
case bucketv1.AzureBucketProvider:
if err = azure.ValidateSecret(secret); err != nil {
e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason}
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
if provider, err = azure.NewClient(obj, secret); err != nil {
e := &serror.Event{Err: err, Reason: "ClientError"}
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
default:
if err = minio.ValidateSecret(secret); err != nil {
e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason}
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
if provider, err = minio.NewClient(obj, secret); err != nil {
e := &serror.Event{Err: err, Reason: "ClientError"}
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
}

// Fetch etag index
if err = fetchEtagIndex(ctx, provider, obj, index, dir); err != nil {
e := &serror.Event{Err: err, Reason: bucketv1.BucketOperationFailedReason}
e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
Expand Down Expand Up @@ -501,7 +501,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
}()

if err = fetchIndexFiles(ctx, provider, obj, index, dir); err != nil {
e := &serror.Event{Err: err, Reason: bucketv1.BucketOperationFailedReason}
e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
return sreconcile.ResultEmpty, e
}
Expand Down Expand Up @@ -550,45 +550,45 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri

// 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 source path: %w", err),
Reason: sourcev1.StatOperationFailedReason,
}
e := serror.NewGeneric(
fmt.Errorf("failed to stat source path: %w", err),
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("source path '%s' is not a directory", dir),
Reason: sourcev1.InvalidPathReason,
}
e := serror.NewGeneric(
fmt.Errorf("source path '%s' is not a directory", dir),
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 {
e := &serror.Event{
Err: fmt.Errorf("failed to create artifact directory: %w", err),
Reason: sourcev1.DirCreationFailedReason,
}
e := serror.NewGeneric(
fmt.Errorf("failed to create artifact directory: %w", err),
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{
Err: fmt.Errorf("failed to acquire lock for artifact: %w", err),
Reason: meta.FailedReason,
}
return sreconcile.ResultEmpty, serror.NewGeneric(
fmt.Errorf("failed to acquire lock for artifact: %w", err),
meta.FailedReason,
)
}
defer unlock()

// Archive directory to storage
if err := r.Storage.Archive(&artifact, dir, nil); err != nil {
e := &serror.Event{
Err: fmt.Errorf("unable to archive artifact to storage: %s", err),
Reason: sourcev1.ArchiveOperationFailedReason,
}
e := serror.NewGeneric(
fmt.Errorf("unable to archive artifact to storage: %s", err),
sourcev1.ArchiveOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
Expand Down Expand Up @@ -635,10 +635,10 @@ func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *bucketv1.Bu
func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *bucketv1.Bucket) error {
if !obj.DeletionTimestamp.IsZero() {
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
return &serror.Event{
Err: fmt.Errorf("garbage collection for deleted resource failed: %s", err),
Reason: "GarbageCollectionFailed",
}
return serror.NewGeneric(
fmt.Errorf("garbage collection for deleted resource failed: %s", err),
"GarbageCollectionFailed",
)
} else if deleted != "" {
r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected artifacts for deleted resource")
Expand All @@ -649,10 +649,10 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *bucketv1.Buc
if obj.GetArtifact() != nil {
delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
if err != nil {
return &serror.Event{
Err: fmt.Errorf("garbage collection of artifacts failed: %w", err),
Reason: "GarbageCollectionFailed",
}
return serror.NewGeneric(
fmt.Errorf("garbage collection of artifacts failed: %w", err),
"GarbageCollectionFailed",
)
}
if len(delFiles) > 0 {
r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded",
Expand Down
10 changes: 6 additions & 4 deletions internal/controller/gitrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
)
ge.Notification = false
ge.Ignore = true
// Log it as this will not be passed to the runtime.
ge.Log = true
ge.Event = corev1.EventTypeNormal
// Remove any stale fetch failed condition.
conditions.Delete(obj, sourcev1.FetchFailedCondition)
Expand Down Expand Up @@ -815,10 +817,10 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patc

// Copy artifact (sub)contents to configured directory.
if err := r.Storage.CopyToPath(artifact, incl.GetFromPath(), toPath); err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
Reason: "CopyFailure",
}
e := serror.NewGeneric(
fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
"CopyFailure",
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
Expand Down
Loading

0 comments on commit 5caa076

Please sign in to comment.