Skip to content

Commit a4d6bbc

Browse files
hiddecohiddeco
authored
Merge pull request #612 from fluxcd/granular-reasons
2 parents 02c8fba + 6830e4e commit a4d6bbc

11 files changed

+176
-82
lines changed

‎api/v1beta2/condition_types.go

+28-4
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,41 @@ 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+
// DirCreationFailedReason signals a failure caused by a directory creation
62+
// operation.
63+
DirCreationFailedReason string = "DirectoryCreationFailed"
64+
65+
// StatOperationFailedReason signals a failure caused by a stat operation on
66+
// a path.
67+
StatOperationFailedReason string = "StatOperationFailed"
68+
69+
// ReadOperationFailedReason signals a failure caused by a read operation.
70+
ReadOperationFailedReason string = "ReadOperationFailed"
71+
72+
// AcquireLockFailedReason signals a failure in acquiring lock.
73+
AcquireLockFailedReason string = "AcquireLockFailed"
74+
75+
// InvalidPathReason signals a failure caused by an invalid path.
76+
InvalidPathReason string = "InvalidPath"
77+
78+
// ArchiveOperationFailedReason signals a failure in archive operation.
79+
ArchiveOperationFailedReason string = "ArchiveOperationFailed"
80+
81+
// SymlinkUpdateFailedReason signals a failure in updating a symlink.
82+
SymlinkUpdateFailedReason string = "SymlinkUpdateFailed"
5983
)

‎controllers/bucket_controller.go

+30-15
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

+6
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

+32-20
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)