diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 71360dd7..e2f9343e 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -47,6 +47,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/sourceignore" @@ -119,6 +120,8 @@ type BucketReconciler struct { Storage *Storage ControllerName string + + patchOptions []patch.Option } type BucketReconcilerOptions struct { @@ -151,7 +154,7 @@ type BucketProvider interface { // bucketReconcileFunc is the function type for all the v1beta2.Bucket // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. -type bucketReconcileFunc func(ctx context.Context, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) +type bucketReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) // etagIndex is an index of storage object keys and their Etag values. type etagIndex struct { @@ -234,6 +237,8 @@ func (r *BucketReconciler) SetupWithManager(mgr ctrl.Manager) error { } func (r *BucketReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts BucketReconcilerOptions) error { + r.patchOptions = getPatchOptions(bucketReadyCondition.Owned, r.ControllerName) + return ctrl.NewControllerManagedBy(mgr). For(&sourcev1.Bucket{}). WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{})). @@ -259,10 +264,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -270,7 +272,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res // Always attempt to patch the object and status after each reconciliation // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(bucketReadyCondition), summarize.WithReconcileResult(recResult), @@ -316,19 +318,35 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the bucketReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *BucketReconciler) reconcile(ctx context.Context, obj *sourcev1.Bucket, reconcilers []bucketReconcileFunc) (sreconcile.Result, error) { +func (r *BucketReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, reconcilers []bucketReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var recAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + recAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case recAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Create temp working dir @@ -356,7 +374,7 @@ func (r *BucketReconciler) reconcile(ctx context.Context, obj *sourcev1.Bucket, ) for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, index, tmpDir) + recResult, err := rec(ctx, sp, obj, index, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -421,22 +439,31 @@ func (r *BucketReconciler) notify(ctx context.Context, oldObj, newObj *sourcev1. // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *BucketReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.Bucket, _ *etagIndex, _ string) (sreconcile.Result, error) { +func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, _ *etagIndex, _ string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -453,7 +480,7 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.B // When a SecretRef is defined, it attempts to fetch the Secret before calling // the provider. If this fails, it records v1beta2.FetchFailedCondition=True on // the object and returns early. -func (r *BucketReconciler) reconcileSource(ctx context.Context, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) { +func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) { secret, err := r.getBucketSecret(ctx, obj) if err != nil { e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason} @@ -528,8 +555,14 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, obj *sourcev1.Bu if !obj.GetArtifact().HasRevision(revision) { message := fmt.Sprintf("new upstream revision '%s'", revision) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return + } } }() @@ -554,7 +587,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, obj *sourcev1.Bu // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) { +func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.Bucket, index *etagIndex, dir string) (sreconcile.Result, error) { // Calculate revision revision, err := index.Revision() if err != nil { @@ -572,7 +605,7 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. if obj.GetArtifact().HasRevision(artifact.Revision) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, - "stored artifact for revision '%s'", artifact.Revision) + "stored artifact: revision '%s'", artifact.Revision) } }() diff --git a/controllers/bucket_controller_test.go b/controllers/bucket_controller_test.go index 883f0864..0593c608 100644 --- a/controllers/bucket_controller_test.go +++ b/controllers/bucket_controller_test.go @@ -185,6 +185,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -201,6 +202,17 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -217,7 +229,8 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -235,6 +248,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -248,6 +262,9 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } for _, tt := range tests { @@ -259,22 +276,31 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { }() r := &BucketReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(bucketReadyCondition.Owned, "sc"), } obj := &sourcev1.Bucket{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } - index := newEtagIndex() + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() - got, err := r.reconcileStorage(context.TODO(), obj, index, "") + index := newEtagIndex() + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileStorage(context.TODO(), sp, obj, index, "") g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -292,6 +318,10 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -327,8 +357,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), }, }, // TODO(hidde): middleware for mock server @@ -343,11 +373,15 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { obj.Spec.SecretRef = &meta.LocalObjectReference{ Name: "dummy", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -362,11 +396,15 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { obj.Spec.SecretRef = &meta.LocalObjectReference{ Name: "dummy", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid 'dummy' secret data: required fields 'accesskey' and 'secretkey'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -374,11 +412,15 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { bucketName: "dummy", beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.BucketName = "invalid" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, "bucket 'invalid' not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -386,11 +428,15 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.Endpoint = "transient.example.com" obj.Spec.BucketName = "unavailable" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, "failed to confirm existence of 'unavailable' bucket"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -423,8 +469,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), }, }, { @@ -462,8 +508,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), }, }, { @@ -473,6 +519,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{ Revision: "b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, bucketObjects: []*s3mock.Object{ { @@ -488,7 +536,10 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { "test.txt": "098f6bcd4621d373cade4e832627b4f6", }, }, - assertConditions: []metav1.Condition{}, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), + }, }, { name: "Removes FetchFailedCondition after reconciling source", @@ -510,9 +561,38 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { "test.txt": "098f6bcd4621d373cade4e832627b4f6", }, }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + }, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + bucketName: "dummy", + bucketObjects: []*s3mock.Object{ + { + Key: "test.txt", + Content: []byte("test"), + ContentType: "text/plain", + LastModified: time.Now(), + }, + }, + beforeFunc: func(obj *sourcev1.Bucket) { + obj.Status.Artifact = &sourcev1.Artifact{ + Path: "some-path", + Revision: "some-rev", + } + }, + want: sreconcile.ResultSuccess, + assertIndex: &etagIndex{ + index: map[string]string{ + "test.txt": "098f6bcd4621d373cade4e832627b4f6", + }, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), }, }, } @@ -528,6 +608,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), Storage: testStorage, + patchOptions: getPatchOptions(bucketReadyCondition.Owned, "sc"), } tmpDir := t.TempDir() @@ -536,7 +617,8 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { Kind: sourcev1.BucketKind, }, ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", + Name: "test-bucket", + Generation: 1, }, Spec: sourcev1.BucketSpec{ Timeout: &metav1.Duration{Duration: timeout}, @@ -563,14 +645,24 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) { tt.beforeFunc(obj) } - index := newEtagIndex() + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() - got, err := r.reconcileSource(context.TODO(), obj, index, tmpDir) + index := newEtagIndex() + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(context.TODO(), sp, obj, index, tmpDir) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(index.Index()).To(Equal(tt.assertIndex.Index())) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -620,8 +712,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), }, }, { @@ -631,12 +723,16 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { obj.Spec.SecretRef = &meta.LocalObjectReference{ Name: "dummy", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/dummy': secrets \"dummy\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -651,12 +747,16 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { obj.Spec.SecretRef = &meta.LocalObjectReference{ Name: "dummy", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "invalid 'dummy' secret data: required fields"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -664,12 +764,16 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { bucketName: "dummy", beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.BucketName = "invalid" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, "bucket 'invalid' not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -677,12 +781,16 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { beforeFunc: func(obj *sourcev1.Bucket) { obj.Spec.Endpoint = "transient.example.com" obj.Spec.BucketName = "unavailable" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertIndex: newEtagIndex(), assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, "failed to confirm existence of 'unavailable' bucket"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -715,8 +823,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision '9fc2ddfc4a6f44e6c3efee40af36578b9e76d4d930eaf384b8435a0aa0bf7a0f'"), }, }, { @@ -754,8 +862,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision '117f586dc64cfc559329e21d286edcbb94cb6b1581517eaddc0ab5292b470cd5'"), }, }, { @@ -765,6 +873,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{ Revision: "b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, bucketObjects: []*gcsmock.Object{ { @@ -780,7 +890,10 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { "test.txt": "098f6bcd4621d373cade4e832627b4f6", }, }, - assertConditions: []metav1.Condition{}, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), + }, }, { name: "Removes FetchFailedCondition after reconciling source", @@ -802,9 +915,38 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { "test.txt": "098f6bcd4621d373cade4e832627b4f6", }, }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + }, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + bucketName: "dummy", + bucketObjects: []*gcsmock.Object{ + { + Key: "test.txt", + ContentType: "text/plain", + Content: []byte("test"), + Generation: 3, + }, + }, + beforeFunc: func(obj *sourcev1.Bucket) { + obj.Status.Artifact = &sourcev1.Artifact{ + Path: "some-path", + Revision: "some-rev", + } + }, + want: sreconcile.ResultSuccess, + assertIndex: &etagIndex{ + index: map[string]string{ + "test.txt": "098f6bcd4621d373cade4e832627b4f6", + }, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'b4c2a60ce44b67f5b659a95ce4e4cc9e2a86baf13afb72bd397c5384cbc0e479'"), }, }, // TODO: Middleware for mock server to test authentication using secret. @@ -821,6 +963,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), Storage: testStorage, + patchOptions: getPatchOptions(bucketReadyCondition.Owned, "sc"), } tmpDir := t.TempDir() @@ -830,7 +973,8 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { Kind: sourcev1.BucketKind, }, ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", + Name: "test-bucket", + Generation: 1, }, Spec: sourcev1.BucketSpec{ BucketName: tt.bucketName, @@ -860,15 +1004,25 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) { g.Expect(os.Unsetenv(EnvGcpStorageHost)).ToNot(HaveOccurred()) }() - index := newEtagIndex() + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() - got, err := r.reconcileSource(context.TODO(), obj, index, tmpDir) + index := newEtagIndex() + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(context.TODO(), sp, obj, index, tmpDir) t.Log(err) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(index.Index()).To(Equal(tt.assertIndex.Index())) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -886,10 +1040,14 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { name: "Archiving artifact to storage makes ArtifactInStorage=True", beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) { obj.Spec.Interval = metav1.Duration{Duration: interval} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -899,6 +1057,8 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { obj.Spec.Interval = metav1.Duration{Duration: interval} // Incomplete artifact obj.Status.Artifact = &sourcev1.Artifact{Revision: revision} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) { // Still incomplete @@ -906,7 +1066,9 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -914,16 +1076,22 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) { obj.Spec.Interval = metav1.Duration{Duration: interval} conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { name: "Creates latest symlink to the created artifact", beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) { obj.Spec.Interval = metav1.Duration{Duration: interval} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) { localPath := testStorage.LocalPath(*obj.GetArtifact()) @@ -934,18 +1102,24 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { name: "Dir path deleted", beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) { t.Expect(os.RemoveAll(dir)).ToNot(HaveOccurred()) + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.StatOperationFailedReason, "failed to stat source path"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, { @@ -957,6 +1131,8 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { f, err := os.Create(dir) defer f.Close() t.Expect(err).ToNot(HaveOccurred()) + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) { t.Expect(os.RemoveAll(dir)).ToNot(HaveOccurred()) @@ -965,6 +1141,8 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "is not a directory"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, }, } @@ -974,8 +1152,10 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { g := NewWithT(t) r := &BucketReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(bucketReadyCondition.Owned, "sc"), } tmpDir := t.TempDir() @@ -1000,7 +1180,14 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { tt.beforeFunc(g, obj, index, tmpDir) } - got, err := r.reconcileArtifact(context.TODO(), obj, index, tmpDir) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileArtifact(context.TODO(), sp, obj, index, tmpDir) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -1011,6 +1198,10 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { if tt.afterFunc != nil { tt.afterFunc(g, obj, tmpDir) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -1128,8 +1319,7 @@ func TestBucketReconciler_statusConditions(t *testing.T) { clientBuilder.WithObjects(obj) c := clientBuilder.Build() - patchHelper, err := patch.NewHelper(obj, c) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, c) if tt.beforeFunc != nil { tt.beforeFunc(obj) @@ -1139,7 +1329,7 @@ func TestBucketReconciler_statusConditions(t *testing.T) { recResult := sreconcile.ResultSuccess var retErr error - summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(bucketReadyCondition), summarize.WithReconcileResult(recResult), @@ -1247,6 +1437,7 @@ func TestBucketReconciler_notify(t *testing.T) { reconciler := &BucketReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(bucketReadyCondition.Owned, "sc"), } index := &etagIndex{ index: map[string]string{ diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 997f4022..40894ad1 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -50,6 +50,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/pkg/sourceignore" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" @@ -102,6 +103,15 @@ var gitRepositoryFailConditions = []string{ sourcev1.StorageOperationFailedCondition, } +// getPatchOptions composes patch options based on the given parameters. +// It is used as the options used when patching an object. +func getPatchOptions(ownedConditions []string, controllerName string) []patch.Option { + return []patch.Option{ + patch.WithOwnedConditions{Conditions: ownedConditions}, + patch.WithFieldOwner(controllerName), + } +} + // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories/status,verbs=get;update;patch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories/finalizers,verbs=get;create;update;patch;delete @@ -118,6 +128,8 @@ type GitRepositoryReconciler struct { requeueDependency time.Duration features map[string]bool + + patchOptions []patch.Option } type GitRepositoryReconcilerOptions struct { @@ -128,13 +140,15 @@ type GitRepositoryReconcilerOptions struct { // gitRepositoryReconcileFunc is the function type for all the // v1beta2.GitRepository (sub)reconcile functions. -type gitRepositoryReconcileFunc func(ctx context.Context, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) +type gitRepositoryReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) func (r *GitRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, GitRepositoryReconcilerOptions{}) } func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts GitRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(gitRepositoryReadyCondition.Owned, r.ControllerName) + r.requeueDependency = opts.DependencyRequeueInterval if r.features == nil { @@ -167,10 +181,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -178,7 +189,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Always attempt to patch the object and status after each reconciliation // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(gitRepositoryReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -227,19 +238,36 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.reconcileInclude, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the gitRepositoryReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.GitRepository, reconcilers []gitRepositoryReconcileFunc) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.GitRepository, reconcilers []gitRepositoryReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var recAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + recAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case recAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Create temp dir for Git clone @@ -268,7 +296,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G resErr error ) for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &commit, &includes, tmpDir) + recResult, err := rec(ctx, sp, obj, &commit, &includes, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -359,23 +387,32 @@ func (r *GitRepositoryReconciler) shouldNotify(oldObj, newObj *sourcev1.GitRepos // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, _ *git.Commit, _ *artifactSet, _ string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -417,7 +454,7 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, // and the local artifact are on the same revision, and no other source content // related configurations have changed since last reconciliation. If there's a // change, it short-circuits the whole reconciliation with an early return. -func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Remove previously failed source verification status conditions. The // failing verification should be recalculated. But an existing successful @@ -477,9 +514,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Observe if the artifacts still match the previous included ones if artifacts.Diff(obj.Status.IncludedArtifacts) { message := fmt.Sprintf("included artifacts differ from last observed includes") - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) - conditions.MarkReconciling(obj, "IncludeChange", message) + if obj.Status.IncludedArtifacts != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } + conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) // Persist the ArtifactSet. *includes = *artifacts @@ -540,8 +583,13 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Mark observations about the revision on the object if !obj.GetArtifact().HasRevision(commit.String()) { message := fmt.Sprintf("new upstream revision '%s'", commit.String()) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } return sreconcile.ResultSuccess, nil } @@ -558,7 +606,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // On a successful archive, the Artifact, Includes, observed ignore, recurse // submodules and observed include in the Status of the object are set, and the // symlink in the Storage is updated to its path. -func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Create potential new artifact with current available metadata @@ -672,7 +720,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // v1beta2.IncludeUnavailableCondition from the object. // When the composed artifactSet differs from the current set in the Status of // the object, it marks the object with v1beta2.ArtifactOutdatedCondition=True. -func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, _ *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { for i, incl := range obj.Spec.Include { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c46a1a5e..7ecab241 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -38,7 +38,6 @@ import ( sshtestdata "golang.org/x/crypto/ssh/testdata" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" @@ -241,16 +240,16 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "HTTP without secretRef makes ArtifactOutdated=True", + name: "HTTP without secretRef makes Reconciling=True", protocol: "http", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { - name: "HTTP with Basic Auth secret makes ArtifactOutdated=True", + name: "HTTP with Basic Auth secret makes Reconciling=True", protocol: "http", server: options{ username: "git", @@ -270,12 +269,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { - name: "HTTPS with CAFile secret makes ArtifactOutdated=True", + name: "HTTPS with CAFile secret makes Reconciling=True", protocol: "https", server: options{ publicKey: tlsPublicKey, @@ -295,8 +294,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { @@ -317,6 +316,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, wantErr: true, assertConditions: []metav1.Condition{ @@ -326,10 +327,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { // // Trimming the expected error message for consistent results. *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "x509: "), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), }, }, { - name: "SSH with private key secret makes ArtifactOutdated=True", + name: "SSH with private key secret makes Reconciling=True", protocol: "ssh", server: options{ username: "git", @@ -348,12 +351,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/"), }, }, { - name: "SSH with password protected private key secret makes ArtifactOutdated=True", + name: "SSH with password protected private key secret makes Reconciling=True", protocol: "ssh", server: options{ username: "git", @@ -373,8 +376,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { @@ -385,10 +388,46 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/non-existing': secrets \"non-existing\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), + }, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + protocol: "http", + server: options{ + username: "git", + password: "1234", + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "basic-auth", + }, + Data: map[string][]byte{ + "username": []byte("git"), + "password": []byte("1234"), + }, + }, + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/some-revision", + Path: randStringRunes(10), + }, + } + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, } @@ -400,6 +439,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.GitRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -468,6 +508,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { features: map[string]bool{ features.OptimizedGitClones: true, }, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } tmpDir := t.TempDir() @@ -479,14 +520,24 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", obj.Spec.URL) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var commit git.Commit var includes artifactSet + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileSource(context.TODO(), obj, &commit, &includes, tmpDir) + got, err := r.reconcileSource(context.TODO(), sp, obj, &commit, &includes, tmpDir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(commit).ToNot(BeNil()) + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -505,30 +556,31 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) wantErr bool wantRevision string wantArtifactOutdated bool + wantReconciling bool }{ { - name: "Nil reference (default branch)", - want: sreconcile.ResultSuccess, - wantRevision: "master/", - wantArtifactOutdated: true, + name: "Nil reference (default branch)", + want: sreconcile.ResultSuccess, + wantRevision: "master/", + wantReconciling: true, }, { name: "Branch", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: true, }, { name: "Tag", reference: &sourcev1.GitRepositoryRef{ Tag: "v0.1.0", }, - want: sreconcile.ResultSuccess, - wantRevision: "v0.1.0/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "v0.1.0/", + wantReconciling: true, }, { name: "Branch commit", @@ -536,36 +588,56 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: true, }, { name: "SemVer", reference: &sourcev1.GitRepositoryRef{ SemVer: "*", }, - want: sreconcile.ResultSuccess, - wantRevision: "v2.0.0/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "v2.0.0/", + wantReconciling: true, }, { name: "SemVer range", reference: &sourcev1.GitRepositoryRef{ SemVer: "", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "0.2.0/", + wantReconciling: true, }, { name: "SemVer prerelease", reference: &sourcev1.GitRepositoryRef{ SemVer: ">=1.0.0-0 <1.1.0-0", }, - wantRevision: "v1.0.0-alpha/", + wantRevision: "v1.0.0-alpha/", + want: sreconcile.ResultSuccess, + wantReconciling: true, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + reference: &sourcev1.GitRepositoryRef{ + Branch: "staging", + }, + beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/some-revision", + Path: randStringRunes(10), + }, + } + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") + }, want: sreconcile.ResultSuccess, + wantRevision: "staging/", wantArtifactOutdated: true, + wantReconciling: true, }, { name: "Optimized clone", @@ -581,11 +653,12 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) }, } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + wantReconciling: false, }, { name: "Optimized clone different ignore", @@ -603,10 +676,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) }, } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: false, }, } @@ -632,12 +706,13 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } r := &GitRepositoryReconciler{ - Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: map[string]bool{ features.OptimizedGitClones: true, }, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -645,6 +720,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "checkout-strategy-", + Generation: 1, }, Spec: sourcev1.GitRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -664,9 +740,15 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) tt.beforeFunc(obj, headRef.Hash().String()) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var commit git.Commit var includes artifactSet - got, err := r.reconcileSource(ctx, obj, &commit, &includes, tmpDir) + sp := patch.NewSerialPatcher(obj, r.Client) + got, err := r.reconcileSource(ctx, sp, obj, &commit, &includes, tmpDir) if err != nil { println(err.Error()) } @@ -676,7 +758,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) revision := strings.ReplaceAll(tt.wantRevision, "", headRef.Hash().String()) g.Expect(commit.String()).To(Equal(revision)) g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(Equal(tt.wantArtifactOutdated)) + g.Expect(conditions.IsTrue(obj, meta.ReconcilingCondition)).To(Equal(tt.wantReconciling)) } + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -855,6 +941,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -873,8 +960,9 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { Hash: []byte("revision"), Reference: "refs/heads/main", } + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileArtifact(ctx, obj, &commit, &tt.includes, tt.dir) + got, err := r.reconcileArtifact(ctx, sp, obj, &commit, &tt.includes, tt.dir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -1000,6 +1088,7 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { Storage: storage, requeueDependency: dependencyInterval, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1036,7 +1125,9 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) includes = *artifactSet - got, err := r.reconcileInclude(ctx, obj, &commit, &includes, tmpDir) + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileInclude(ctx, sp, obj, &commit, &includes, tmpDir) g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) if err == nil { @@ -1093,6 +1184,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -1109,6 +1201,17 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -1125,7 +1228,8 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -1143,6 +1247,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -1156,6 +1261,9 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } for _, tt := range tests { @@ -1167,23 +1275,32 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { }() r := &GitRepositoryReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var c *git.Commit var as artifactSet - got, err := r.reconcileStorage(context.TODO(), obj, c, &as, "") + sp := patch.NewSerialPatcher(obj, r.Client) + got, err := r.reconcileStorage(context.TODO(), sp, obj, c, &as, "") g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -1201,6 +1318,10 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -1212,6 +1333,7 @@ func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1350,6 +1472,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1492,6 +1615,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } key := client.ObjectKeyFromObject(obj) @@ -1710,8 +1834,7 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) { clientBuilder.WithObjects(obj) c := clientBuilder.Build() - patchHelper, err := patch.NewHelper(obj, c) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, c) if tt.beforeFunc != nil { tt.beforeFunc(obj) @@ -1721,9 +1844,10 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) { recResult := sreconcile.ResultSuccess var retErr error - summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(gitRepositoryReadyCondition), + summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), summarize.WithReconcileResult(recResult), summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), @@ -1857,6 +1981,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { reconciler := &GitRepositoryReconciler{ EventRecorder: recorder, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } reconciler.notify(ctx, oldObj, newObj, tt.commit, tt.res, tt.resErr) @@ -1996,6 +2121,7 @@ func TestGitRepositoryReconciler_fetchIncludes(t *testing.T) { r := &GitRepositoryReconciler{ Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index c6a57692..f5cf3f93 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -30,6 +30,8 @@ import ( eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" soci "github.com/fluxcd/source-controller/internal/oci" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/v1/remote" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" @@ -56,9 +58,8 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/pkg/untar" - "github.com/google/go-containerregistry/pkg/authn" - "github.com/google/go-containerregistry/pkg/v1/remote" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" @@ -133,6 +134,8 @@ type HelmChartReconciler struct { Cache *cache.Cache TTL time.Duration *cache.CacheRecorder + + patchOptions []patch.Option } func (r *HelmChartReconciler) SetupWithManager(mgr ctrl.Manager) error { @@ -147,9 +150,11 @@ type HelmChartReconcilerOptions struct { // helmChartReconcileFunc is the function type for all the v1beta2.HelmChart // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. -type helmChartReconcileFunc func(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) +type helmChartReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmChartReconcilerOptions) error { + r.patchOptions = getPatchOptions(helmChartReadyCondition.Owned, r.ControllerName) + if err := mgr.GetCache().IndexField(context.TODO(), &sourcev1.HelmRepository{}, sourcev1.HelmRepositoryURLIndexKey, r.indexHelmRepositoryByURL); err != nil { return fmt.Errorf("failed setting index fields: %w", err) @@ -200,10 +205,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -211,7 +213,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Always attempt to patch the object after each reconciliation. // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(helmChartReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -259,19 +261,35 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the helmChartReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmChart, reconcilers []helmChartReconcileFunc) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, reconcilers []helmChartReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Run the sub-reconcilers and build the result of reconciliation. @@ -281,7 +299,7 @@ func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmC resErr error ) for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &build) + recResult, err := rec(ctx, sp, obj, &build) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -344,22 +362,31 @@ func (r *HelmChartReconciler) notify(ctx context.Context, oldObj, newObj *source // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -371,7 +398,7 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev return sreconcile.ResultSuccess, nil } -func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { +func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { // Remove any failed verification condition. // The reason is that a failing verification should be recalculated. if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) { @@ -418,7 +445,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1 // Defer observation of build result defer func() { // Record both success and error observations on the object - observeChartBuild(obj, build, retErr) + observeChartBuild(ctx, sp, r.patchOptions, obj, build, retErr) // If we actually build a chart, take a historical note of any dependencies we resolved. // The reason this is a done conditionally, is because if we have a cached one in storage, @@ -810,7 +837,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { // Without a complete chart build, there is little to reconcile if !b.Complete() { return sreconcile.ResultRequeue, nil @@ -1265,10 +1292,16 @@ func (r *HelmChartReconciler) eventLogf(ctx context.Context, obj runtime.Object, } // observeChartBuild records the observation on the given given build and error on the object. -func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { +func observeChartBuild(ctx context.Context, sp *patch.SerialPatcher, pOpts []patch.Option, obj *sourcev1.HelmChart, build *chart.Build, err error) { if build.HasMetadata() { if build.Name != obj.Status.ObservedChartName || !obj.GetArtifact().HasRevision(build.Version) { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewChart", build.Summary()) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewChart", build.Summary()) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", build.Summary()) + if err := sp.Patch(ctx, obj, pOpts...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + } } } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index cd71e1e8..15b2424f 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -309,6 +309,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -325,6 +326,17 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -341,7 +353,8 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -359,6 +372,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -372,6 +386,9 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } for _, tt := range tests { @@ -383,20 +400,30 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { }() r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } - got, err := r.reconcileStorage(context.TODO(), obj, nil) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileStorage(context.TODO(), sp, obj, nil) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -414,6 +441,10 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -468,13 +499,51 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), })) }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) }, }, + { + name: "Existing artifact makes AritfactOutdated=True", + source: &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gitrepository", + Namespace: "default", + }, + Status: sourcev1.GitRepositoryStatus{ + Artifact: gitArtifact, + }, + }, + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = "testdata/charts/helmchart-0.1.0.tgz" + obj.Spec.SourceRef = sourcev1.LocalHelmChartSourceReference{ + Name: "gitrepository", + Kind: sourcev1.GitRepositoryKind, + } + obj.Status.Artifact = &sourcev1.Artifact{ + Path: "some-path", + Revision: "some-rev", + } + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, build chart.Build, obj sourcev1.HelmChart) { + g.Expect(build.Complete()).To(BeTrue()) + g.Expect(build.Name).To(Equal("helmchart")) + g.Expect(build.Version).To(Equal("0.1.0")) + g.Expect(build.Path).To(BeARegularFile()) + + g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + })) + }, + }, { name: "Error on unavailable source", beforeFunc: func(obj *sourcev1.HelmChart) { @@ -482,6 +551,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Name: "unavailable", Kind: sourcev1.GitRepositoryKind, } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: &serror.Event{Err: errors.New("gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found")}, @@ -490,6 +561,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "SourceUnavailable", "failed to get source: gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), })) }, }, @@ -500,6 +573,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Name: "unavailable", Kind: "Unsupported", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultEmpty, wantErr: &serror.Stalling{Err: errors.New("unsupported source kind 'Unsupported'")}, @@ -508,6 +583,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "SourceUnavailable", "failed to get source: unsupported source kind"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -529,6 +606,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Kind: sourcev1.GitRepositoryKind, } obj.Spec.ValuesFiles = []string{"invalid.yaml"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultEmpty, wantErr: &serror.Stalling{Err: errors.New("values files merge error: no values file found at path")}, @@ -537,6 +616,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ValuesFilesError", "values files merge error: no values file found at path"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -556,6 +637,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Kind: sourcev1.GitRepositoryKind, } obj.Status.ObservedSourceArtifactRevision = "foo" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultRequeue, assertFunc: func(g *WithT, build chart.Build, obj sourcev1.HelmChart) { @@ -564,6 +647,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal("foo")) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "NoSourceArtifact", "no artifact available"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -572,7 +657,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - clientBuilder := fake.NewClientBuilder() + clientBuilder := fake.NewClientBuilder().WithScheme(testEnv.GetScheme()) if tt.source != nil { clientBuilder.WithRuntimeObjects(tt.source) } @@ -581,12 +666,14 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Client: clientBuilder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: storage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ - Name: "chart", - Namespace: "default", + Name: "chart", + Namespace: "default", + Generation: 1, }, Spec: sourcev1.HelmChartSpec{}, } @@ -599,7 +686,14 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { defer tt.cleanFunc(g, &b) } - got, err := r.reconcileSource(context.TODO(), &obj, &b) + g.Expect(r.Client.Create(context.TODO(), &obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), &obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(&obj, r.Client) + + got, err := r.reconcileSource(context.TODO(), sp, &obj, &b) g.Expect(err != nil).To(Equal(tt.wantErr != nil)) if tt.wantErr != nil { @@ -611,6 +705,10 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { if tt.assertFunc != nil { tt.assertFunc(g, b, obj) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, &obj) }) } } @@ -840,6 +938,7 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, Storage: storage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } repository := &sourcev1.HelmRepository{ @@ -1006,8 +1105,6 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) { g.Expect(build.Name).To(Equal(metadata.Name)) g.Expect(build.Version).To(Equal(metadata.Version)) - fmt.Println("buildpath", build.Path) - fmt.Println("storage Path", storage.LocalPath(*cachedArtifact.DeepCopy())) g.Expect(build.Path).ToNot(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) g.Expect(build.Path).To(BeARegularFile()) }, @@ -1071,6 +1168,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { Getters: testGetters, Storage: storage, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } repository := &sourcev1.HelmRepository{ @@ -1276,6 +1374,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { Storage: storage, Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1433,8 +1532,10 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { g := NewWithT(t) r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1448,7 +1549,14 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { tt.beforeFunc(obj) } - got, err := r.reconcileArtifact(ctx, obj, tt.build) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileArtifact(ctx, sp, obj, tt.build) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) @@ -1477,7 +1585,8 @@ func TestHelmChartReconciler_getHelmRepositorySecret(t *testing.T) { clientBuilder.WithObjects(mock) r := &HelmChartReconciler{ - Client: clientBuilder.Build(), + Client: clientBuilder.Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } tests := []struct { @@ -1572,7 +1681,8 @@ func TestHelmChartReconciler_getSource(t *testing.T) { clientBuilder.WithObjects(mocks...) r := &HelmChartReconciler{ - Client: clientBuilder.Build(), + Client: clientBuilder.Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } tests := []struct { @@ -1678,6 +1788,7 @@ func TestHelmChartReconciler_reconcileDelete(t *testing.T) { r := &HelmChartReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1704,7 +1815,7 @@ func TestHelmChartReconciler_reconcileDelete(t *testing.T) { func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { // Helper to build simple helmChartReconcileFunc with result and error. buildReconcileFuncs := func(r sreconcile.Result, e error) helmChartReconcileFunc { - return func(_ context.Context, _ *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + return func(_ context.Context, _ *patch.SerialPatcher, _ *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { return r, e } } @@ -1725,6 +1836,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultSuccess, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "successful reconciliation with generation difference", @@ -1736,7 +1851,8 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { wantResult: sreconcile.ResultSuccess, wantErr: false, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "reconciling new object generation (3)"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), }, }, { @@ -1746,15 +1862,19 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "multiple object status conditions mutations", reconcileFuncs: []helmChartReconcileFunc{ - func(_ context.Context, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + func(_ context.Context, _ *patch.SerialPatcher, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") return sreconcile.ResultSuccess, nil }, - func(_ context.Context, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + func(_ context.Context, _ *patch.SerialPatcher, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { conditions.MarkTrue(obj, meta.ReconcilingCondition, "Progressing", "creating artifact") return sreconcile.ResultSuccess, nil }, @@ -1763,7 +1883,8 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { wantErr: false, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "Progressing", "creating artifact"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "creating artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), }, }, { @@ -1775,6 +1896,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultRequeue, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "subrecs with error before result=Requeue", @@ -1785,6 +1910,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, } @@ -1792,7 +1921,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - r := &HelmChartReconciler{} + r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), + } obj := &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -1803,7 +1935,14 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, } - got, err := r.reconcile(context.TODO(), obj, tt.reconcileFuncs) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcile(context.TODO(), sp, obj, tt.reconcileFuncs) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.wantResult)) @@ -1898,8 +2037,7 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) { clientBuilder.WithObjects(obj) c := clientBuilder.Build() - patchHelper, err := patch.NewHelper(obj, c) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, c) if tt.beforeFunc != nil { tt.beforeFunc(obj) @@ -1909,9 +2047,10 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) { recResult := sreconcile.ResultSuccess var retErr error - summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(helmChartReadyCondition), + summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), summarize.WithReconcileResult(recResult), summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), @@ -2012,6 +2151,7 @@ func TestHelmChartReconciler_notify(t *testing.T) { reconciler := &HelmChartReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } build := &chart.Build{ Name: "foo", @@ -2061,7 +2201,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { name: "HTTP without basic auth", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -2075,7 +2216,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { password: testRegistryPassword, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -2114,7 +2256,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { }, provider: "azure", assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, } @@ -2200,6 +2343,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } var b chart.Build @@ -2216,7 +2360,14 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", metadata.Version) } - got, err := r.reconcileSource(ctx, obj, &b) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(ctx, sp, obj, &b) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) @@ -2323,8 +2474,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version "), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -2342,6 +2494,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -2383,6 +2537,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Getters: testGetters, Storage: storage, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -2434,7 +2589,14 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T defer tt.cleanFunc(g, &b) } - got, err := r.reconcileSource(ctx, obj, &b) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(ctx, sp, obj, &b) if tt.wantErr { tt.wantErrMsg = strings.ReplaceAll(tt.wantErrMsg, "", chartUrl) g.Expect(err).ToNot(BeNil()) diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 4a1e89ff..595efd9c 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -43,6 +43,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" @@ -109,6 +110,8 @@ type HelmRepositoryReconciler struct { Cache *cache.Cache TTL time.Duration *cache.CacheRecorder + + patchOptions []patch.Option } type HelmRepositoryReconcilerOptions struct { @@ -120,13 +123,15 @@ type HelmRepositoryReconcilerOptions struct { // v1beta2.HelmRepository (sub)reconcile functions. The type implementations // are grouped and executed serially to perform the complete reconcile of the // object. -type helmRepositoryReconcileFunc func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) +type helmRepositoryReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) func (r *HelmRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{}) } func (r *HelmRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(helmRepositoryReadyCondition.Owned, r.ControllerName) + return ctrl.NewControllerManagedBy(mgr). For(&sourcev1.HelmRepository{}). WithEventFilter( @@ -160,10 +165,7 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -171,7 +173,7 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Always attempt to patch the object after each reconciliation. // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(helmRepositoryReadyCondition), summarize.WithReconcileResult(recResult), @@ -219,19 +221,36 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the helmRepositoryReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository, reconcilers []helmRepositoryReconcileFunc) (sreconcile.Result, error) { +func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.HelmRepository, reconcilers []helmRepositoryReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } var chartRepo repository.ChartRepository @@ -241,7 +260,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1. var res sreconcile.Result var resErr error for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &artifact, &chartRepo) + recResult, err := rec(ctx, sp, obj, &artifact, &chartRepo) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -311,22 +330,32 @@ func (r *HelmRepositoryReconciler) notify(ctx context.Context, oldObj, newObj *s // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository, _ *sourcev1.Artifact, _ *repository.ChartRepository) (sreconcile.Result, error) { +func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.HelmRepository, _ *sourcev1.Artifact, _ *repository.ChartRepository) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -346,7 +375,8 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so // If successful and the index is valid, any previous // v1beta2.FetchFailedCondition is removed, and the repository.ChartRepository // pointer is set to the newly fetched index. -func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { +func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { var tlsConfig *tls.Config // Configure Helm client to access repository @@ -455,8 +485,14 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou // Mark observations about the revision on the object. if !obj.GetArtifact().HasRevision(chartRepo.Checksum) { message := fmt.Sprintf("new index revision '%s'", checksum) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return sreconcile.ResultEmpty, err + } } // Create potential new artifact. @@ -482,13 +518,13 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { +func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { // Set the ArtifactInStorageCondition if there's no drift. defer func() { if obj.GetArtifact().HasRevision(artifact.Revision) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, - "stored artifact for revision '%s'", artifact.Revision) + "stored artifact: revision '%s'", artifact.Revision) } chartRepo.Unload() diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 7e383e0c..d311e224 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -24,6 +24,7 @@ import ( "os" "time" + "github.com/google/go-containerregistry/pkg/authn" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" @@ -45,7 +46,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" - "github.com/google/go-containerregistry/pkg/authn" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/source-controller/api/v1beta2" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" @@ -79,6 +80,8 @@ type HelmRepositoryOCIReconciler struct { Getters helmgetter.Providers ControllerName string RegistryClientGenerator RegistryClientGeneratorFunc + + patchOptions []patch.Option } // RegistryClientGeneratorFunc is a function that returns a registry client @@ -92,6 +95,8 @@ func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error { } func (r *HelmRepositoryOCIReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(helmRepositoryOCIOwnedConditions, r.ControllerName) + return ctrl.NewControllerManagedBy(mgr). For(&sourcev1.HelmRepository{}). WithEventFilter( @@ -122,34 +127,26 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // Always attempt to patch the object after each reconciliation. defer func() { - // Patch the object, prioritizing the conditions owned by the controller in - // case of any conflicts. - patchOpts := []patch.Option{ - patch.WithOwnedConditions{ - Conditions: helmRepositoryOCIOwnedConditions, - }, - } - patchOpts = append(patchOpts, patch.WithFieldOwner(r.ControllerName)) // If a reconcile annotation value is found, set it in the object status // as status.lastHandledReconcileAt. if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { object.SetStatusLastHandledReconcileAt(obj, v) } + patchOpts := []patch.Option{} + patchOpts = append(patchOpts, r.patchOptions...) + // Set status observed generation option if the object is stalled, or // if the object is ready. if conditions.IsStalled(obj) || conditions.IsReady(obj) { patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) } - if err = patchHelper.Patch(ctx, obj, patchOpts...); err != nil { + if err := serialPatcher.Patch(ctx, obj, patchOpts...); err != nil { // Ignore patch error "not found" when the object is being deleted. if !obj.GetDeletionTimestamp().IsZero() { err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) @@ -188,7 +185,7 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } - result, retErr = r.reconcile(ctx, obj) + result, retErr = r.reconcile(ctx, serialPatcher, obj) return } @@ -198,7 +195,7 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re // status conditions and the returned results are evaluated in the deferred // block at the very end to summarize the conditions to be in a consistent // state. -func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta2.HelmRepository) (result ctrl.Result, retErr error) { +func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *v1beta2.HelmRepository) (result ctrl.Result, retErr error) { ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() @@ -224,6 +221,15 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta } } + // Presence of reconciling means that the reconciliation didn't succeed. + // Set the Reconciling reason to ProgressingWithRetry to indicate a + // failure retry. + if conditions.IsReconciling(obj) { + reconciling := conditions.Get(obj, meta.ReconcilingCondition) + reconciling.Reason = meta.ProgressingWithRetryReason + conditions.Set(obj, reconciling) + } + // If it's still a successful reconciliation and it's not reconciling or // stalled, mark Ready=True. if !conditions.IsReconciling(obj) && !conditions.IsStalled(obj) && @@ -244,8 +250,27 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta }() // Set reconciling condition. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + result, retErr = ctrl.Result{}, err + return + } + case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + result, retErr = ctrl.Result{}, err + return + } } // Ensure that it's an OCI URL before continuing. diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go index de0d51af..f4bbe790 100644 --- a/controllers/helmrepository_controller_oci_test.go +++ b/controllers/helmrepository_controller_oci_test.go @@ -208,6 +208,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { password: "wrong-pass", }, assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to login to registry"), }, }, @@ -217,6 +218,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { provider: "aws", providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test", assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get credential from"), }, }, @@ -249,6 +251,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { obj := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.HelmRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -293,12 +296,27 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmRepositoryOCIOwnedConditions, "sc"), } - got, err := r.reconcile(ctx, obj) + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcile(ctx, sp, obj) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + // NOTE: Check the object directly as reconcile() doesn't apply the + // final patch, the object has unapplied changes. + checker.DisableFetch = true + checker.CheckErr(ctx, obj) }) } } diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 31d1beb6..40b10650 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -159,6 +159,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -175,6 +176,17 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -191,7 +203,8 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -209,6 +222,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -222,6 +236,9 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } for _, tt := range tests { @@ -229,23 +246,32 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { g := NewWithT(t) r := &HelmRepositoryReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var chartRepo repository.ChartRepository var artifact sourcev1.Artifact + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileStorage(context.TODO(), obj, &artifact, &chartRepo) + got, err := r.reconcileStorage(context.TODO(), sp, obj, &artifact, &chartRepo) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -263,6 +289,10 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -302,8 +332,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, }, { @@ -311,8 +341,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol: "http", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) @@ -342,8 +372,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) @@ -373,8 +403,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) @@ -401,10 +431,14 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create TLS client config with secret data: cannot append certificate into certificate pool: invalid caFile"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // No repo index due to fetch fail. @@ -419,11 +453,15 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol: "http", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "") + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, "first path segment in URL cannot contain colon"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // No repo index due to fetch fail. @@ -438,11 +476,15 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol: "http", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://") + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "scheme \"ftp\" not supported"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // No repo index due to fetch fail. @@ -457,10 +499,14 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { protocol: "http", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "secrets \"non-existing\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // No repo index due to fetch fail. @@ -483,10 +529,14 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "malformed-basic-auth"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "required fields 'username' and 'password"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // No repo index due to fetch fail. @@ -504,6 +554,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Revision: checksum, Checksum: checksum, } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { // chartRepo.Checksum isn't populated, artifact.Checksum is @@ -523,6 +579,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Revision: checksum, Checksum: "foo", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) @@ -532,12 +594,29 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, want: sreconcile.ResultSuccess, }, + { + name: "Existing artifact makes ArtifactOutdated=True", + protocol: "http", + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + obj.Status.Artifact = &sourcev1.Artifact{ + Path: "some-path", + Revision: "some-rev", + } + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), + }, + }, } for _, tt := range tests { obj := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.HelmRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -642,11 +721,19 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Client: builder.Build(), Storage: testStorage, Getters: testGetters, + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var chartRepo repository.ChartRepository var artifact sourcev1.Artifact - got, err := r.reconcileSource(context.TODO(), obj, &artifact, &chartRepo) + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(context.TODO(), sp, obj, &artifact, &chartRepo) defer os.Remove(chartRepo.CachePath) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) @@ -656,6 +743,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if tt.afterFunc != nil { tt.afterFunc(g, obj, artifact, chartRepo) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -676,7 +767,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, }, { @@ -690,7 +781,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, }, { @@ -701,7 +792,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, }, { @@ -718,7 +809,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, }, } @@ -728,8 +819,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { g := NewWithT(t) r := &HelmRepositoryReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmRepository{ @@ -766,8 +859,9 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { if tt.beforeFunc != nil { tt.beforeFunc(g, obj, artifact, chartRepo) } + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileArtifact(context.TODO(), obj, &artifact, chartRepo) + got, err := r.reconcileArtifact(context.TODO(), sp, obj, &artifact, chartRepo) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -788,7 +882,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { // Helper to build simple helmRepositoryReconcileFunc with result and error. buildReconcileFuncs := func(r sreconcile.Result, e error) helmRepositoryReconcileFunc { - return func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + return func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { return r, e } } @@ -809,6 +903,10 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultSuccess, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "successful reconciliation with generation difference", @@ -820,7 +918,8 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { wantResult: sreconcile.ResultSuccess, wantErr: false, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "reconciling new object generation (3)"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), }, }, { @@ -830,16 +929,20 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "multiple object status conditions mutations", reconcileFuncs: []helmRepositoryReconcileFunc{ - func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") return sreconcile.ResultSuccess, nil }, - func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { - conditions.MarkTrue(obj, meta.ReconcilingCondition, "Progressing", "creating artifact") + func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + conditions.MarkTrue(obj, meta.ReconcilingCondition, meta.ProgressingReason, "creating artifact") return sreconcile.ResultSuccess, nil }, }, @@ -847,7 +950,8 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { wantErr: false, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "Progressing", "creating artifact"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "creating artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), }, }, { @@ -859,6 +963,10 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultRequeue, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "subrecs with error before result=Requeue", @@ -869,6 +977,10 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, } @@ -876,7 +988,10 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - r := &HelmRepositoryReconciler{} + r := &HelmRepositoryReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), + } obj := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -887,9 +1002,15 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }, } - ctx := context.TODO() + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() - gotRes, gotErr := r.reconcile(ctx, obj, tt.reconcileFuncs) + ctx := context.TODO() + sp := patch.NewSerialPatcher(obj, r.Client) + + gotRes, gotErr := r.reconcile(ctx, sp, obj, tt.reconcileFuncs) g.Expect(gotErr != nil).To(Equal(tt.wantErr)) g.Expect(gotRes).To(Equal(tt.wantResult)) @@ -960,8 +1081,7 @@ func TestHelmRepositoryReconciler_statusConditions(t *testing.T) { clientBuilder.WithObjects(obj) c := clientBuilder.Build() - patchHelper, err := patch.NewHelper(obj, c) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, c) if tt.beforeFunc != nil { tt.beforeFunc(obj) @@ -971,7 +1091,7 @@ func TestHelmRepositoryReconciler_statusConditions(t *testing.T) { recResult := sreconcile.ResultSuccess var retErr error - summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(helmRepositoryReadyCondition), summarize.WithReconcileResult(recResult), @@ -1084,6 +1204,7 @@ func TestHelmRepositoryReconciler_notify(t *testing.T) { reconciler := &HelmRepositoryReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } chartRepo := repository.ChartRepository{ URL: "some-address", diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index c0160a31..79c091ba 100644 --- a/controllers/ocirepository_controller.go +++ b/controllers/ocirepository_controller.go @@ -31,7 +31,6 @@ import ( "time" "github.com/Masterminds/semver/v3" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" soci "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/authn/k8schain" @@ -54,6 +53,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/ratelimiter" + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/oci" "github.com/fluxcd/pkg/oci/auth/login" @@ -61,9 +61,11 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/pkg/sourceignore" "github.com/fluxcd/pkg/untar" "github.com/fluxcd/pkg/version" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" serror "github.com/fluxcd/source-controller/internal/error" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" @@ -120,7 +122,7 @@ func (e invalidOCIURLError) Error() string { // ociRepositoryReconcileFunc is the function type for all the v1beta2.OCIRepository // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. -type ociRepositoryReconcileFunc func(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) +type ociRepositoryReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) // OCIRepositoryReconciler reconciles a v1beta2.OCIRepository object type OCIRepositoryReconciler struct { @@ -131,6 +133,8 @@ type OCIRepositoryReconciler struct { Storage *Storage ControllerName string requeueDependency time.Duration + + patchOptions []patch.Option } type OCIRepositoryReconcilerOptions struct { @@ -145,6 +149,8 @@ func (r *OCIRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { } func (r *OCIRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts OCIRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(ociRepositoryReadyCondition.Owned, r.ControllerName) + r.requeueDependency = opts.DependencyRequeueInterval return ctrl.NewControllerManagedBy(mgr). @@ -178,10 +184,7 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -189,7 +192,7 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Always attempt to patch the object and status after each reconciliation // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(ociRepositoryReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -236,19 +239,36 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the ociRepositoryReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.OCIRepository, reconcilers []ociRepositoryReconcileFunc) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.OCIRepository, reconcilers []ociRepositoryReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = v + } + + // Persist reconciling status if generation differs or reconciliation is + // requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Create temp working dir @@ -276,7 +296,7 @@ func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.O // Run the sub-reconcilers and build the result of reconciliation. for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &metadata, tmpDir) + recResult, err := rec(ctx, sp, obj, &metadata, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -299,7 +319,8 @@ func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.O // reconcileSource fetches the upstream OCI artifact metadata and content. // If this fails, it records v1beta2.FetchFailedCondition=True on the object and returns early. -func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { var auth authn.Authenticator ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) @@ -385,8 +406,14 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour defer func() { if !obj.GetArtifact().HasRevision(revision) { message := fmt.Sprintf("new revision '%s' for '%s'", revision, url) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return + } } }() @@ -876,22 +903,32 @@ func oidcAuth(ctx context.Context, url, provider string) (authn.Authenticator, e // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.OCIRepository, _ *sourcev1.Artifact, _ string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, _ *sourcev1.Artifact, _ string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -911,7 +948,8 @@ func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sou // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { revision := metadata.Revision // Create artifact diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 061978a5..0abd1648 100644 --- a/controllers/ocirepository_controller_test.go +++ b/controllers/ocirepository_controller_test.go @@ -335,7 +335,7 @@ func TestOCIRepository_Reconcile_MediaType(t *testing.T) { return false } readyCondition := conditions.Get(obj, meta.ReadyCondition) - return readyCondition != nil + return readyCondition != nil && !conditions.IsUnknown(obj, meta.ReadyCondition) }, timeout).Should(BeTrue()) g.Expect(conditions.IsReady(obj)).To(BeIdenticalTo(!tt.wantErr)) @@ -383,8 +383,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { name: "HTTP without basic auth", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -404,8 +404,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { includeSecret: true, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -425,8 +425,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { includeSA: true, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -508,8 +508,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -580,8 +580,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, provider: "azure", assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, } @@ -595,6 +595,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -667,6 +668,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } opts := craneOptions(ctx, true) @@ -680,8 +682,15 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", repoURL) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, &sourcev1.Artifact{}, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, &sourcev1.Artifact{}, tmpDir) if tt.wantErr { g.Expect(err).ToNot(BeNil()) } else { @@ -778,6 +787,7 @@ func TestOCIRepository_CertSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "ocirepository-test-resource", Namespace: ns.Name, + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: tt.url, @@ -818,7 +828,7 @@ func TestOCIRepository_CertSecret(t *testing.T) { return false } readyCondition := conditions.Get(&resultobj, meta.ReadyCondition) - if readyCondition == nil { + if readyCondition == nil || conditions.IsUnknown(&resultobj, meta.ReadyCondition) { return false } return obj.Generation == readyCondition.ObservedGeneration && @@ -866,8 +876,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("latest/%s", img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -878,8 +888,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -890,8 +900,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -902,8 +912,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { wantRevision: img6.digest.Hex, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -948,8 +958,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -962,8 +972,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: img5.digest.Hex, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, } @@ -974,6 +984,7 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -981,6 +992,7 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "checkout-strategy-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -993,9 +1005,16 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { obj.Spec.Reference = tt.reference } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1043,8 +1062,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { shouldSign: true, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of revision "), }, }, @@ -1058,8 +1077,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { wantErrMsg: "failed to verify the signature using provider 'cosign': no matching signatures were found for ''", want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider '': no matching signatures were found for ''"), }, }, @@ -1073,8 +1092,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { want: sreconcile.ResultEmpty, keyless: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider ' keyless': no matching signatures"), }, }, @@ -1132,8 +1151,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { wantErr: true, want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "cosign does not support insecure registries"), }, }, @@ -1145,6 +1164,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } pf := func(b bool) ([]byte, error) { @@ -1175,6 +1195,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "verify-oci-source-signature-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -1236,8 +1257,15 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { tt.beforeFunc(obj) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) if tt.wantErr { tt.wantErrMsg = strings.ReplaceAll(tt.wantErrMsg, "", artifactURL) g.Expect(err).ToNot(BeNil()) @@ -1373,6 +1401,7 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1382,6 +1411,7 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "noop-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -1395,9 +1425,16 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { tt.beforeFunc(obj) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(Equal(sreconcile.ResultSuccess)) @@ -1593,6 +1630,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1602,6 +1640,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "reconcile-artifact-", + Generation: 1, }, } if tt.beforeFunc != nil { @@ -1612,7 +1651,15 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { if tt.artifact != nil { artifact = tt.artifact } - got, err := r.reconcileArtifact(ctx, obj, artifact, tt.targetPath) + + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileArtifact(ctx, sp, obj, artifact, tt.targetPath) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1698,6 +1745,7 @@ func TestOCIRepository_getArtifactURL(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1763,7 +1811,7 @@ func TestOCIRepository_stalled(t *testing.T) { return false } return obj.Generation == readyCondition.ObservedGeneration && - !conditions.IsReady(&resultobj) + !conditions.IsUnknown(&resultobj, meta.ReadyCondition) }, timeout).Should(BeTrue()) // Verify that stalled condition is present in status @@ -1809,6 +1857,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -1825,6 +1874,17 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { "!/oci-reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -1841,7 +1901,8 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { "!/oci-reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -1859,6 +1920,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -1872,6 +1934,9 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/oci-reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } @@ -1880,6 +1945,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1888,11 +1954,22 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } - g.Expect(tt.beforeFunc(obj)).To(Succeed()) - got, err := r.reconcileStorage(ctx, obj, &sourcev1.Artifact{}, "") + if tt.beforeFunc != nil { + g.Expect(tt.beforeFunc(obj)).To(Succeed()) + } + + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileStorage(ctx, sp, obj, &sourcev1.Artifact{}, "") if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1916,6 +1993,10 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { g.Expect(absoluteP).ToNot(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -1926,6 +2007,7 @@ func TestOCIRepository_ReconcileDelete(t *testing.T) { r := &OCIRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.OCIRepository{ @@ -2058,6 +2140,7 @@ func TestOCIRepositoryReconciler_notify(t *testing.T) { reconciler := &OCIRepositoryReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } reconciler.notify(ctx, oldObj, newObj, tt.res, tt.resErr) diff --git a/docs/spec/v1beta2/buckets.md b/docs/spec/v1beta2/buckets.md index 23c036fd..6d6a6271 100644 --- a/docs/spec/v1beta2/buckets.md +++ b/docs/spec/v1beta2/buckets.md @@ -866,9 +866,9 @@ Status: ... Conditions: Last Transition Time: 2022-02-02T13:26:55Z - Message: reconciling new object generation (2) + Message: processing object: new generation 1 -> 2 Observed Generation: 2 - Reason: NewGeneration + Reason: ProgressingWithRetry Status: True Type: Reconciling Last Transition Time: 2022-02-02T13:26:55Z @@ -978,13 +978,13 @@ is true: - The generation of the Bucket is newer than the [Observed Generation](#observed-generation). - The newly calculated Artifact revision differs from the current Artifact. -When the Bucket is "reconciling", the `Ready` Condition status becomes `False`, -and the controller adds a Condition with the following attributes to the -Bucket's `.status.conditions`: +When the Bucket is "reconciling", the `Ready` Condition status becomes +`Unknown` when the controller detects drift, and the controller adds a Condition +with the following attributes to the Bucket's `.status.conditions`: - `type: Reconciling` - `status: "True"` -- `reason: NewGeneration` | `reason: NoArtifact` | `reason: NewRevision` +- `reason: Progressing` | `reason: ProgressingWithRetry` If the reconciling state is due to a new revision, an additional Condition is added with the following attributes: @@ -1062,7 +1062,9 @@ it succeeds and the Bucket is marked as [ready](#ready-bucket). Note that a Bucket can be [reconciling](#reconciling-bucket) while failing at the same time, for example due to a newly introduced configuration issue in the -Bucket spec. +Bucket spec. When a reconciliation fails, the `Reconciling` Condition reason +would be `ProgressingWithRetry`. When the reconciliation is performed again +after the failure, the reason is updated to `Progressing`. ### Observed Ignore diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index f1c046da..55c84f6b 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -610,9 +610,9 @@ Status: ... Conditions: Last Transition Time: 2022-02-14T09:40:27Z - Message: reconciling new object generation (2) + Message: processing object: new generation 1 -> 2 Observed Generation: 2 - Reason: NewGeneration + Reason: ProgressingWithRetry Status: True Type: Reconciling Last Transition Time: 2022-02-14T09:40:27Z @@ -724,12 +724,13 @@ following is true: - The newly resolved Artifact revision differs from the current Artifact. When the GitRepository is "reconciling", the `Ready` Condition status becomes -`False`, and the controller adds a Condition with the following attributes to -the GitRepository's `.status.conditions`: +`Unknown` when the controller detects drift, and the controller adds a Condition +with the following attributes to the GitRepository's +`.status.conditions`: - `type: Reconciling` - `status: "True"` -- `reason: NewGeneration` | `reason: NoArtifact` | `reason: NewRevision` +- `reason: Progressing` | `reason: ProgressingWithRetry` If the reconciling state is due to a new revision, an additional Condition is added with the following attributes: @@ -819,7 +820,10 @@ exponential backoff, until it succeeds and the GitRepository is marked as Note that a GitRepository can be [reconciling](#reconciling-gitrepository) while failing at the same time, for example due to a newly introduced -configuration issue in the GitRepository spec. +configuration issue in the GitRepository spec. When a reconciliation fails, the +`Reconciling` Condition reason would be `ProgressingWithRetry`. When the +reconciliation is performed again after the failure, the reason is updated to +`Progressing`. ### Content Configuration Checksum diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index 990ff869..0924876f 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -633,12 +633,12 @@ following is true: - The newly fetched Artifact revision differs from the current Artifact. When the HelmChart is "reconciling", the `Ready` Condition status becomes -`False`, and the controller adds a Condition with the following attributes to -the HelmChart's `.status.conditions`: +`Unknown` when the controller detects drift, and the controller adds a Condition +with the following attributes to the HelmChart's `.status.conditions`: - `type: Reconciling` - `status: "True"` -- `reason: NewGeneration` | `reason: NoArtifact` +- `reason: Progressing` | `reason: ProgressingWithRetry` If the reconciling state is due to a new version, it adds an additional Condition with the following attributes: @@ -715,7 +715,10 @@ until it succeeds and the HelmChart is marked as [ready](#ready-helmchart). Note that a HelmChart can be [reconciling](#reconciling-helmchart) while failing at the same time, for example due to a newly introduced -configuration issue in the HelmChart spec. +configuration issue in the HelmChart spec. When a reconciliation fails, the +`Reconciling` Condition reason would be `ProgressingWithRetry`. When the +reconciliation is performed again after the failure, the reason is updated to +`Progressing`. #### Stalled HelmChart diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index 15db55b3..570abb49 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -676,12 +676,12 @@ is true: - The newly fetched Artifact revision differs from the current Artifact. When the HelmRepository is "reconciling", the `Ready` Condition status becomes -`False`, and the controller adds a Condition with the following attributes to -the HelmRepository's `.status.conditions`: +`Unknown` when the controller detects drift, and the controller adds a Condition +with the following attributes to the HelmRepository's `.status.conditions`: - `type: Reconciling` - `status: "True"` -- `reason: NewGeneration` | `reason: NoArtifact` | `reason: NewRevision` +- `reason: Progressing` | `reason: ProgressingWithRetry` If the reconciling state is due to a new revision, it adds an additional Condition with the following attributes: @@ -760,7 +760,10 @@ until it succeeds and the HelmRepository is marked as [ready](#ready-helmreposit Note that a HelmRepository can be [reconciling](#reconciling-helmrepository) while failing at the same time, for example due to a newly introduced -configuration issue in the HelmRepository spec. +configuration issue in the HelmRepository spec. When a reconciliation fails, the +`Reconciling` Condition reason would be `ProgressingWithRetry`. When the +reconciliation is performed again after the failure, the reason is updated to +`Progressing`. #### Stalled HelmRepository diff --git a/docs/spec/v1beta2/ocirepositories.md b/docs/spec/v1beta2/ocirepositories.md index 39d1decf..17c4e481 100644 --- a/docs/spec/v1beta2/ocirepositories.md +++ b/docs/spec/v1beta2/ocirepositories.md @@ -642,9 +642,9 @@ Status: ... Conditions: Last Transition Time: 2022-02-14T09:40:27Z - Message: reconciling new object generation (2) + Message: processing object: new generation 1 -> 2 Observed Generation: 2 - Reason: NewGeneration + Reason: ProgressingWithRetry Status: True Type: Reconciling Last Transition Time: 2022-02-14T09:40:27Z @@ -769,12 +769,12 @@ following is true: - The newly resolved Artifact digest differs from the current Artifact. When the OCIRepository is "reconciling", the `Ready` Condition status becomes -`False`, and the controller adds a Condition with the following attributes to -the OCIRepository's `.status.conditions`: +`Unknown` when the controller detects drift, and the controller adds a Condition +with the following attributes to the OCIRepository's `.status.conditions`: - `type: Reconciling` - `status: "True"` -- `reason: NewGeneration` | `reason: NoArtifact` | `reason: NewRevision` +- `reason: Progressing` | `reason: ProgressingWithRetry` If the reconciling state is due to a new revision, an additional Condition is added with the following attributes: @@ -862,7 +862,10 @@ exponential backoff, until it succeeds and the OCIRepository is marked as Note that a OCIRepository can be [reconciling](#reconciling-ocirepository) while failing at the same time, for example due to a newly introduced -configuration issue in the OCIRepository spec. +configuration issue in the OCIRepository spec. When a reconciliation fails, the +`Reconciling` Condition reason would be `ProgressingWithRetry`. When the +reconciliation is performed again after the failure, the reason is updated to +`Progressing`. ### Content Configuration Checksum diff --git a/go.mod b/go.mod index 0f7a7b4c..a05b1027 100644 --- a/go.mod +++ b/go.mod @@ -31,7 +31,7 @@ require ( github.com/fluxcd/pkg/lockedfile v0.1.0 github.com/fluxcd/pkg/masktoken v0.2.0 github.com/fluxcd/pkg/oci v0.17.0 - github.com/fluxcd/pkg/runtime v0.24.0 + github.com/fluxcd/pkg/runtime v0.25.0 github.com/fluxcd/pkg/sourceignore v0.3.0 github.com/fluxcd/pkg/ssh v0.7.0 github.com/fluxcd/pkg/testserver v0.4.0 diff --git a/go.sum b/go.sum index 67db7168..acc5ee68 100644 --- a/go.sum +++ b/go.sum @@ -549,8 +549,8 @@ github.com/fluxcd/pkg/masktoken v0.2.0 h1:HoSPTk4l1fz5Fevs2vVRvZGru33blfMwWSZKsH github.com/fluxcd/pkg/masktoken v0.2.0/go.mod h1:EA7GleAHL33kN6kTW06m5R3/Q26IyuGO7Ef/0CtpDI0= github.com/fluxcd/pkg/oci v0.17.0 h1:DYoT0HG3DogEmeXRif6ZzTYwAZe+iqYWP4QpsP37ZBE= github.com/fluxcd/pkg/oci v0.17.0/go.mod h1:UjxCQcdcKtog/ad9Vr2yPYjz9keNSoLdTOOiUNqCRiY= -github.com/fluxcd/pkg/runtime v0.24.0 h1:rQmm5Xq8K7f8xcPj1oNOInM1x4YwmgTucZJOP51Xmr4= -github.com/fluxcd/pkg/runtime v0.24.0/go.mod h1:I2T+HWVNzX0cxm9TgH+SVNHTwqlmEDiSke43JXsq9iY= +github.com/fluxcd/pkg/runtime v0.25.0 h1:Lk5WrKDJKsayymLnnSCY/Zn77/mrlIf+skYz64suoww= +github.com/fluxcd/pkg/runtime v0.25.0/go.mod h1:I2T+HWVNzX0cxm9TgH+SVNHTwqlmEDiSke43JXsq9iY= github.com/fluxcd/pkg/sourceignore v0.3.0 h1:pFO3hKV9ub+2SrNZPZE7xfiRhxsycRrd7JK7qB26nVw= github.com/fluxcd/pkg/sourceignore v0.3.0/go.mod h1:ak3Tve/KwVzytZ5V2yBlGGpTJ/2oQ9kcP3iuwBOAHGo= github.com/fluxcd/pkg/ssh v0.7.0 h1:FX5ky8SU9dYwbM6zEIDR3TSveLF01iyS95CtB5Ykpno= diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 3c25474d..58a160b8 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -124,6 +124,15 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb conditions.Delete(obj, meta.ReconcilingCondition) } + // Presence of reconciling means that the reconciliation didn't succeed. + // Set the Reconciling reason to ProgressingWithRetry to indicate a failure + // retry. + if conditions.IsReconciling(obj) { + reconciling := conditions.Get(obj, meta.ReconcilingCondition) + reconciling.Reason = meta.ProgressingWithRetryReason + conditions.Set(obj, reconciling) + } + // Analyze the reconcile error. switch t := recErr.(type) { case *serror.Stalling: diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index b9b2ccfe..15a60b0d 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -206,6 +206,21 @@ func TestComputeReconcileResult(t *testing.T) { t.Expect(conditions.IsUnknown(obj, meta.StalledCondition)).To(BeTrue()) }, }, + { + name: "failed with Reconciling=True adds ProgressingWithRetry reason", + beforeFunc: func(obj conditions.Setter) { + conditions.MarkReconciling(obj, meta.ProgressingReason, "some msg") + }, + result: ResultEmpty, + recErr: fmt.Errorf("some error"), + wantResult: ctrl.Result{}, + wantErr: true, + afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "some msg"), + }, + }, } for _, tt := range tests { diff --git a/internal/reconcile/summarize/summary.go b/internal/reconcile/summarize/summary.go index 6a556a18..3977ccdf 100644 --- a/internal/reconcile/summarize/summary.go +++ b/internal/reconcile/summarize/summary.go @@ -50,15 +50,15 @@ type Conditions struct { // Helper is SummarizeAndPatch helper. type Helper struct { - recorder kuberecorder.EventRecorder - patchHelper *patch.Helper + recorder kuberecorder.EventRecorder + serialPatcher *patch.SerialPatcher } // NewHelper returns an initialized Helper. -func NewHelper(recorder kuberecorder.EventRecorder, patchHelper *patch.Helper) *Helper { +func NewHelper(recorder kuberecorder.EventRecorder, serialPatcher *patch.SerialPatcher) *Helper { return &Helper{ - recorder: recorder, - patchHelper: patchHelper, + recorder: recorder, + serialPatcher: serialPatcher, } } @@ -250,7 +250,7 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o } // Finally, patch the resource. - if err := h.patchHelper.Patch(ctx, obj, patchOpts...); err != nil { + if err := h.serialPatcher.Patch(ctx, obj, patchOpts...); err != nil { // Ignore patch error "not found" when the object is being deleted. if opts.IgnoreNotFound && !obj.GetDeletionTimestamp().IsZero() { err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) diff --git a/internal/reconcile/summarize/summary_test.go b/internal/reconcile/summarize/summary_test.go index 18de95f4..48ee5648 100644 --- a/internal/reconcile/summarize/summary_test.go +++ b/internal/reconcile/summarize/summary_test.go @@ -128,7 +128,7 @@ func TestSummarizeAndPatch(t *testing.T) { name: "Success, removes reconciling for successful result", generation: 2, beforeFunc: func(obj conditions.Setter) { - conditions.MarkReconciling(obj, "NewRevision", "new index version") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new index version") conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "stored artifact") }, conditions: []Conditions{testReadyConditions}, @@ -167,7 +167,7 @@ func TestSummarizeAndPatch(t *testing.T) { generation: 7, beforeFunc: func(obj conditions.Setter) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") - conditions.MarkReconciling(obj, "NewRevision", "new index revision") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new index revision") }, conditions: []Conditions{testReadyConditions}, reconcileErr: fmt.Errorf("failed to create dir"), @@ -175,7 +175,7 @@ func TestSummarizeAndPatch(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"), *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "new index revision"), }, afterFunc: func(t *WithT, obj client.Object) { t.Expect(obj).ToNot(HaveStatusObservedGeneration(7)) @@ -264,7 +264,7 @@ func TestSummarizeAndPatch(t *testing.T) { name: "Fail, reconciling with bipolar condition False, Ready gets bipolar failure value", generation: 2, beforeFunc: func(obj conditions.Setter) { - conditions.MarkReconciling(obj, "NewRevision", "new index revision") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new index revision") conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed") }, result: reconcile.ResultEmpty, @@ -275,14 +275,14 @@ func TestSummarizeAndPatch(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.FalseCondition(meta.ReadyCondition, "VerifyFailed", "verify failed"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "new index revision"), }, }, { name: "Fail, bipolar condition True, negative polarity True, Ready gets negative polarity value", generation: 2, beforeFunc: func(obj conditions.Setter) { - conditions.MarkReconciling(obj, "NewGeneration", "new obj gen") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new obj gen") conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest") conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified") }, @@ -294,7 +294,7 @@ func TestSummarizeAndPatch(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new digest"), *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "new obj gen"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "new obj gen"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"), }, }, @@ -345,10 +345,9 @@ func TestSummarizeAndPatch(t *testing.T) { ctx := context.TODO() g.Expect(client.Create(ctx, obj)).To(Succeed()) - patchHelper, err := patch.NewHelper(obj, client) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, client) - summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper) + summaryHelper := NewHelper(record.NewFakeRecorder(32), serialPatcher) summaryOpts := []Option{ WithReconcileResult(tt.result), WithReconcileError(tt.reconcileErr), @@ -471,15 +470,14 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { ctx := context.TODO() g.Expect(kclient.Create(ctx, obj)).To(Succeed()) - patchHelper, err := patch.NewHelper(obj, kclient) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, kclient) - summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper) + summaryHelper := NewHelper(record.NewFakeRecorder(32), serialPatcher) summaryOpts := []Option{ WithConditions(tt.conditions...), WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}), } - _, err = summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...) + _, err := summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...) g.Expect(err).ToNot(HaveOccurred()) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))