Skip to content

Commit 882a85b

Browse files
darkowlzzdarkowlzz
committed
Add new condition StorageOperationFailedCondition
Introduce new condition StorageOperationFailedCondition for all the failures related to the storage. It is a negative polarity condition and is considered in computing summary of reconciliation. Also, introduce more granular event reasons related to StorageOperationFailedCondition for precise reasoning behind failures. These replace the vague StorageOperationFailedReason. Signed-off-by: Sunny <darkowlzz@protonmail.com>
1 parent 02c8fba commit 882a85b

File tree

7 files changed

+163
-77
lines changed

7 files changed

+163
-77
lines changed

‎api/v1beta2/condition_types.go‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,45 @@ const (
4343
// of a Source's Artifact.
4444
// If True, the Source can be in an ArtifactOutdatedCondition.
4545
BuildFailedCondition string = "BuildFailed"
46+
47+
// StorageOperationFailedCondition indicates a transient or persistent
48+
// failure related to storage. If True, the reconciliation failed while
49+
// performing some filesystem operation.
50+
StorageOperationFailedCondition string = "StorageOperationFailed"
4651
)
4752

4853
const (
4954
// URLInvalidReason signals that a given Source has an invalid URL.
5055
URLInvalidReason string = "URLInvalid"
5156

52-
// StorageOperationFailedReason signals a failure caused by a storage
53-
// operation.
54-
StorageOperationFailedReason string = "StorageOperationFailed"
55-
5657
// AuthenticationFailedReason signals that a Secret does not have the
5758
// required fields, or the provided credentials do not match.
5859
AuthenticationFailedReason string = "AuthenticationFailed"
60+
61+
// FileCreationFailedReason signals a failure caused by a file creation
62+
// operation.
63+
FileCreationFailedReason string = "FileCreationFailed"
64+
65+
// DirCreationFailedReason signals a failure caused by a directory creation
66+
// operation.
67+
DirCreationFailedReason string = "DirectoryCreationFailed"
68+
69+
// StatOperationFailedReason signals a failure caused by a stat operation on
70+
// a path.
71+
StatOperationFailedReason string = "StatOperationFailed"
72+
73+
// ReadOperationFailedReason signals a failure caused by a read operation.
74+
ReadOperationFailedReason string = "ReadOperationFailed"
75+
76+
// AcquireLockFailedReason signals a failure in acquiring lock.
77+
AcquireLockFailedReason string = "AcquireLockFailed"
78+
79+
// InvalidPathReason signals a failure caused by an invalid path.
80+
InvalidPathReason string = "InvalidPath"
81+
82+
// ArchiveOperationFailedReason signals a failure in archive operation.
83+
ArchiveOperationFailedReason string = "ArchiveOperationFailed"
84+
85+
// SymlinkUpdateFailedReason signals a failure in updating a symlink.
86+
SymlinkUpdateFailedReason string = "SymlinkUpdateFailed"
5987
)

‎controllers/bucket_controller.go‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -74,21 +74,24 @@ const maxConcurrentBucketFetches = 100
7474
var bucketReadyCondition = summarize.Conditions{
7575
Target: meta.ReadyCondition,
7676
Owned: []string{
77-
sourcev1.ArtifactOutdatedCondition,
7877
sourcev1.FetchFailedCondition,
78+
sourcev1.ArtifactOutdatedCondition,
79+
sourcev1.StorageOperationFailedCondition,
7980
meta.ReadyCondition,
8081
meta.ReconcilingCondition,
8182
meta.StalledCondition,
8283
},
8384
Summarize: []string{
84-
sourcev1.ArtifactOutdatedCondition,
8585
sourcev1.FetchFailedCondition,
86+
sourcev1.ArtifactOutdatedCondition,
87+
sourcev1.StorageOperationFailedCondition,
8688
meta.StalledCondition,
8789
meta.ReconcilingCondition,
8890
},
8991
NegativePolarity: []string{
90-
sourcev1.ArtifactOutdatedCondition,
9192
sourcev1.FetchFailedCondition,
93+
sourcev1.ArtifactOutdatedCondition,
94+
sourcev1.StorageOperationFailedCondition,
9295
meta.StalledCondition,
9396
meta.ReconcilingCondition,
9497
},
@@ -313,16 +316,19 @@ func (r *BucketReconciler) reconcile(ctx context.Context, obj *sourcev1.Bucket,
313316
// Create temp working dir
314317
tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-%s-", obj.Kind, obj.Namespace, obj.Name))
315318
if err != nil {
316-
return sreconcile.ResultEmpty, &serror.Event{
317-
Err: fmt.Errorf("failed to create temporary directory: %w", err),
318-
Reason: sourcev1.StorageOperationFailedReason,
319+
e := &serror.Event{
320+
Err: fmt.Errorf("failed to create temporary working directory: %w", err),
321+
Reason: sourcev1.DirCreationFailedReason,
319322
}
323+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
324+
return sreconcile.ResultEmpty, e
320325
}
321326
defer func() {
322327
if err = os.RemoveAll(tmpDir); err != nil {
323328
ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary working directory")
324329
}
325330
}()
331+
conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
326332

327333
// Run the sub-reconcilers and build the result of reconciliation.
328334
var (
@@ -521,23 +527,29 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
521527

522528
// Ensure target path exists and is a directory
523529
if f, err := os.Stat(dir); err != nil {
524-
return sreconcile.ResultEmpty, &serror.Event{
530+
e := &serror.Event{
525531
Err: fmt.Errorf("failed to stat source path: %w", err),
526-
Reason: sourcev1.StorageOperationFailedReason,
532+
Reason: sourcev1.StatOperationFailedReason,
527533
}
534+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
535+
return sreconcile.ResultEmpty, e
528536
} else if !f.IsDir() {
529-
return sreconcile.ResultEmpty, &serror.Event{
537+
e := &serror.Event{
530538
Err: fmt.Errorf("source path '%s' is not a directory", dir),
531-
Reason: sourcev1.StorageOperationFailedReason,
539+
Reason: sourcev1.InvalidPathReason,
532540
}
541+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
542+
return sreconcile.ResultEmpty, e
533543
}
534544

535545
// Ensure artifact directory exists and acquire lock
536546
if err := r.Storage.MkdirAll(artifact); err != nil {
537-
return sreconcile.ResultEmpty, &serror.Event{
547+
e := &serror.Event{
538548
Err: fmt.Errorf("failed to create artifact directory: %w", err),
539-
Reason: sourcev1.StorageOperationFailedReason,
549+
Reason: sourcev1.DirCreationFailedReason,
540550
}
551+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
552+
return sreconcile.ResultEmpty, e
541553
}
542554
unlock, err := r.Storage.Lock(artifact)
543555
if err != nil {
@@ -550,10 +562,12 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
550562

551563
// Archive directory to storage
552564
if err := r.Storage.Archive(&artifact, dir, nil); err != nil {
553-
return sreconcile.ResultEmpty, &serror.Event{
565+
e := &serror.Event{
554566
Err: fmt.Errorf("unable to archive artifact to storage: %s", err),
555-
Reason: sourcev1.StorageOperationFailedReason,
567+
Reason: sourcev1.ArchiveOperationFailedReason,
556568
}
569+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
570+
return sreconcile.ResultEmpty, e
557571
}
558572
r.annotatedEventLogf(ctx, obj, map[string]string{
559573
"revision": artifact.Revision,
@@ -566,12 +580,13 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
566580
// Update symlink on a "best effort" basis
567581
url, err := r.Storage.Symlink(artifact, "latest.tar.gz")
568582
if err != nil {
569-
r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
583+
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason,
570584
"failed to update status URL symlink: %s", err)
571585
}
572586
if url != "" {
573587
obj.Status.URL = url
574588
}
589+
conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
575590
return sreconcile.ResultSuccess, nil
576591
}
577592

‎controllers/bucket_controller_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -947,6 +947,9 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) {
947947
},
948948
want: sreconcile.ResultEmpty,
949949
wantErr: true,
950+
assertConditions: []metav1.Condition{
951+
*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.StatOperationFailedReason, "failed to stat source path"),
952+
},
950953
},
951954
{
952955
name: "Dir path is not a directory",
@@ -963,6 +966,9 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) {
963966
},
964967
want: sreconcile.ResultEmpty,
965968
wantErr: true,
969+
assertConditions: []metav1.Condition{
970+
*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "is not a directory"),
971+
},
966972
},
967973
}
968974

‎controllers/gitrepository_controller.go‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ var gitRepositoryReadyCondition = summarize.Conditions{
6363
sourcev1.FetchFailedCondition,
6464
sourcev1.IncludeUnavailableCondition,
6565
sourcev1.ArtifactOutdatedCondition,
66+
sourcev1.StorageOperationFailedCondition,
6667
meta.ReadyCondition,
6768
meta.ReconcilingCondition,
6869
meta.StalledCondition,
@@ -72,13 +73,15 @@ var gitRepositoryReadyCondition = summarize.Conditions{
7273
sourcev1.SourceVerifiedCondition,
7374
sourcev1.FetchFailedCondition,
7475
sourcev1.ArtifactOutdatedCondition,
76+
sourcev1.StorageOperationFailedCondition,
7577
meta.StalledCondition,
7678
meta.ReconcilingCondition,
7779
},
7880
NegativePolarity: []string{
7981
sourcev1.FetchFailedCondition,
8082
sourcev1.IncludeUnavailableCondition,
8183
sourcev1.ArtifactOutdatedCondition,
84+
sourcev1.StorageOperationFailedCondition,
8285
meta.StalledCondition,
8386
meta.ReconcilingCondition,
8487
},
@@ -213,16 +216,19 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G
213216
// Create temp dir for Git clone
214217
tmpDir, err := util.TempDirForObj("", obj)
215218
if err != nil {
216-
return sreconcile.ResultEmpty, &serror.Event{
217-
Err: fmt.Errorf("failed to create temporary directory: %w", err),
218-
Reason: sourcev1.StorageOperationFailedReason,
219+
e := &serror.Event{
220+
Err: fmt.Errorf("failed to create temporary working directory: %w", err),
221+
Reason: sourcev1.DirCreationFailedReason,
219222
}
223+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
224+
return sreconcile.ResultEmpty, e
220225
}
221226
defer func() {
222227
if err = os.RemoveAll(tmpDir); err != nil {
223228
ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary working directory")
224229
}
225230
}()
231+
conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
226232

227233
// Run the sub-reconcilers and build the result of reconciliation.
228234
var (
@@ -322,7 +328,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
322328
Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err),
323329
Reason: sourcev1.AuthenticationFailedReason,
324330
}
325-
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
331+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
326332
// Return error as the world as observed may change
327333
return sreconcile.ResultEmpty, e
328334
}
@@ -338,7 +344,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
338344
Err: fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
339345
Reason: sourcev1.AuthenticationFailedReason,
340346
}
341-
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
347+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
342348
// Return error as the contents of the secret may change
343349
return sreconcile.ResultEmpty, e
344350
}
@@ -358,7 +364,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
358364
Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
359365
Reason: sourcev1.GitOperationFailedReason,
360366
}
361-
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, e.Err.Error())
367+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
362368
// Do not return err as recovery without changes is impossible
363369
return sreconcile.ResultEmpty, e
364370
}
@@ -372,7 +378,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
372378
Err: fmt.Errorf("failed to checkout and determine revision: %w", err),
373379
Reason: sourcev1.GitOperationFailedReason,
374380
}
375-
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, e.Err.Error())
381+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
376382
// Coin flip on transient or persistent error, return error and hope for the best
377383
return sreconcile.ResultEmpty, e
378384
}
@@ -429,24 +435,27 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context,
429435
// Ensure target path exists and is a directory
430436
if f, err := os.Stat(dir); err != nil {
431437
e := &serror.Event{
432-
Err: fmt.Errorf("failed to stat target path: %w", err),
433-
Reason: sourcev1.StorageOperationFailedReason,
438+
Err: fmt.Errorf("failed to stat target artifact path: %w", err),
439+
Reason: sourcev1.StatOperationFailedReason,
434440
}
441+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
435442
return sreconcile.ResultEmpty, e
436443
} else if !f.IsDir() {
437444
e := &serror.Event{
438445
Err: fmt.Errorf("invalid target path: '%s' is not a directory", dir),
439-
Reason: sourcev1.StorageOperationFailedReason,
446+
Reason: sourcev1.InvalidPathReason,
440447
}
448+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
441449
return sreconcile.ResultEmpty, e
442450
}
443451

444452
// Ensure artifact directory exists and acquire lock
445453
if err := r.Storage.MkdirAll(artifact); err != nil {
446454
e := &serror.Event{
447455
Err: fmt.Errorf("failed to create artifact directory: %w", err),
448-
Reason: sourcev1.StorageOperationFailedReason,
456+
Reason: sourcev1.DirCreationFailedReason,
449457
}
458+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
450459
return sreconcile.ResultEmpty, e
451460
}
452461
unlock, err := r.Storage.Lock(artifact)
@@ -472,10 +481,12 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context,
472481

473482
// Archive directory to storage
474483
if err := r.Storage.Archive(&artifact, dir, SourceIgnoreFilter(ps, nil)); err != nil {
475-
return sreconcile.ResultEmpty, &serror.Event{
484+
e := &serror.Event{
476485
Err: fmt.Errorf("unable to archive artifact to storage: %w", err),
477-
Reason: sourcev1.StorageOperationFailedReason,
486+
Reason: sourcev1.ArchiveOperationFailedReason,
478487
}
488+
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
489+
return sreconcile.ResultEmpty, e
479490
}
480491
r.AnnotatedEventf(obj, map[string]string{
481492
"revision": artifact.Revision,
@@ -489,12 +500,13 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context,
489500
// Update symlink on a "best effort" basis
490501
url, err := r.Storage.Symlink(artifact, "latest.tar.gz")
491502
if err != nil {
492-
r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
503+
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.SymlinkUpdateFailedReason,
493504
"failed to update status URL symlink: %s", err)
494505
}
495506
if url != "" {
496507
obj.Status.URL = url
497508
}
509+
conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
498510
return sreconcile.ResultSuccess, nil
499511
}
500512

@@ -520,7 +532,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
520532
Err: fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err),
521533
Reason: "IllegalPath",
522534
}
523-
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "IllegalPath", e.Err.Error())
535+
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
524536
return sreconcile.ResultEmpty, e
525537
}
526538

@@ -531,7 +543,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
531543
Err: fmt.Errorf("could not get resource for include '%s': %w", incl.GitRepositoryRef.Name, err),
532544
Reason: "NotFound",
533545
}
534-
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NotFound", e.Err.Error())
546+
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
535547
return sreconcile.ResultEmpty, err
536548
}
537549

@@ -541,7 +553,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
541553
Err: fmt.Errorf("no artifact available for include '%s'", incl.GitRepositoryRef.Name),
542554
Reason: "NoArtifact",
543555
}
544-
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NoArtifact", e.Err.Error())
556+
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
545557
return sreconcile.ResultEmpty, e
546558
}
547559

@@ -551,7 +563,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
551563
Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
552564
Reason: "CopyFailure",
553565
}
554-
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "CopyFailure", e.Err.Error())
566+
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
555567
return sreconcile.ResultEmpty, e
556568
}
557569
artifacts[i] = dep.GetArtifact().DeepCopy()
@@ -598,7 +610,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
598610
Err: fmt.Errorf("PGP public keys secret error: %w", err),
599611
Reason: "VerificationError",
600612
}
601-
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, meta.FailedReason, e.Err.Error())
613+
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
602614
return sreconcile.ResultEmpty, e
603615
}
604616

@@ -612,7 +624,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
612624
Err: fmt.Errorf("signature verification of commit '%s' failed: %w", commit.Hash.String(), err),
613625
Reason: "InvalidCommitSignature",
614626
}
615-
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, meta.FailedReason, e.Err.Error())
627+
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
616628
// Return error in the hope the secret changes
617629
return sreconcile.ResultEmpty, e
618630
}

0 commit comments

Comments
 (0)