diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 9f3cddf..a5b64cc 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -903,20 +903,20 @@ func (r *SyncReconciler[T]) Reconcile(ctx context.Context, resource T) (Result, if resource.GetDeletionTimestamp() == nil || r.SyncDuringFinalization { syncResult, err := r.sync(ctx, resource) + result = AggregateResults(result, syncResult) if err != nil { log.Error(err, "unable to sync") - return Result{}, err + return result, err } - result = AggregateResults(result, syncResult) } if resource.GetDeletionTimestamp() != nil { finalizeResult, err := r.finalize(ctx, resource) + result = AggregateResults(result, finalizeResult) if err != nil { log.Error(err, "unable to finalize") - return Result{}, err + return result, err } - result = AggregateResults(result, finalizeResult) } return result, nil @@ -1299,10 +1299,10 @@ func (r Sequence[T]) Reconcile(ctx context.Context, resource T) (Result, error) ctx = logr.NewContext(ctx, log) result, err := reconciler.Reconcile(ctx, resource) + aggregateResult = AggregateResults(result, aggregateResult) if err != nil { - return Result{}, err + return result, err } - aggregateResult = AggregateResults(result, aggregateResult) } return aggregateResult, nil @@ -1397,7 +1397,7 @@ func (r *CastResource[T, CT]) Reconcile(ctx context.Context, resource T) (Result castOriginal := castResource.DeepCopyObject().(client.Object) result, err := r.Reconciler.Reconcile(ctx, castResource) if err != nil { - return Result{}, err + return result, err } if !equality.Semantic.DeepEqual(castResource, castOriginal) { // patch the reconciled resource with the updated duck values diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index d417f42..47cd711 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -843,6 +843,47 @@ func TestResourceReconciler(t *testing.T) { }), }, }, + "sub reconciler halted with result": { + Request: testRequest, + StatusSubResourceTypes: []client.Object{ + &resources.TestResource{}, + }, + GivenObjects: []client.Object{ + resource, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { + return reconcilers.Sequence[*resources.TestResource]{ + &reconcilers.SyncReconciler[*resources.TestResource]{ + SyncWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + resource.Status.Fields = map[string]string{ + "want": "this to run", + } + return reconcilers.Result{ Requeue: true }, reconcilers.HaltSubReconcilers + }, + }, + &reconcilers.SyncReconciler[*resources.TestResource]{ + Sync: func(ctx context.Context, resource *resources.TestResource) error { + resource.Status.Fields = map[string]string{ + "don't want": "this to run", + } + return fmt.Errorf("reconciler error") + }, + }, + } + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "StatusUpdated", + `Updated status`), + }, + ExpectStatusUpdates: []client.Object{ + resource.StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("want", "this to run") + }), + }, + ExpectedResult: reconcilers.Result{ Requeue: true }, + }, "status update failed": { Request: testRequest, StatusSubResourceTypes: []client.Object{ @@ -1335,6 +1376,36 @@ func TestAggregateReconciler(t *testing.T) { AddData("foo", "bar"), }, }, + "reconcile halted with result": { + Request: request, + Metadata: map[string]interface{}{ + "Reconciler": func(t *testing.T, c reconcilers.Config) reconcile.Reconciler { + r := defaultAggregateReconciler(c) + r.Reconciler = reconcilers.Sequence[*corev1.ConfigMap]{ + &reconcilers.SyncReconciler[*corev1.ConfigMap]{ + SyncWithResult: func(ctx context.Context, resource *corev1.ConfigMap) (reconcilers.Result, error) { + return reconcilers.Result { Requeue: true }, reconcilers.HaltSubReconcilers + }, + }, + &reconcilers.SyncReconciler[*corev1.ConfigMap]{ + Sync: func(ctx context.Context, resource *corev1.ConfigMap) error { + return fmt.Errorf("test error") + }, + }, + } + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(configMapGiven, scheme, corev1.EventTypeNormal, "Created", + `Created ConfigMap %q`, testName), + }, + ExpectCreates: []client.Object{ + configMapCreate. + AddData("foo", "bar"), + }, + ExpectedResult: reconcilers.Result { Requeue: true }, + }, "context is stashable": { Request: request, GivenObjects: []client.Object{ @@ -1478,6 +1549,20 @@ func TestSyncReconciler(t *testing.T) { }, }, }, + "sync with result halted": { + Resource: resource.DieReleasePtr(), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { + return &reconcilers.SyncReconciler[*resources.TestResource]{ + SyncWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + return reconcilers.Result { Requeue: true }, reconcilers.HaltSubReconcilers + }, + } + }, + }, + ExpectedResult: reconcilers.Result { Requeue: true }, + ShouldErr: true, + }, "sync error": { Resource: resource.DieReleasePtr(), Metadata: map[string]interface{}{ @@ -1541,6 +1626,29 @@ func TestSyncReconciler(t *testing.T) { }, ExpectedResult: reconcile.Result{RequeueAfter: 3 * time.Hour}, }, + "finalize can halt subreconcilers": { + Resource: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + d.Finalizers(testFinalizer) + }). + DieReleasePtr(), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { + return &reconcilers.SyncReconciler[*resources.TestResource]{ + SyncWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + t.Errorf("reconciler should not call sync for deleted resources") + return reconcilers.Result{RequeueAfter: 2 * time.Hour}, nil + }, + FinalizeWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + return reconcilers.Result{RequeueAfter: 3 * time.Hour}, reconcilers.HaltSubReconcilers + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 3 * time.Hour}, + ShouldErr: true, + }, "should finalize and sync deleted resources when asked to": { Resource: resource. MetadataDie(func(d *diemetav1.ObjectMetaDie) { @@ -3640,6 +3748,26 @@ func TestSequence(t *testing.T) { }, ShouldErr: true, }, + "preserves result, sub reconciler halted": { + Resource: resource.DieReleasePtr(), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { + return reconcilers.Sequence[*resources.TestResource]{ + &reconcilers.SyncReconciler[*resources.TestResource]{ + SyncWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + return reconcilers.Result{RequeueAfter: 1 * time.Minute}, nil + }, + },&reconcilers.SyncReconciler[*resources.TestResource]{ + SyncWithResult: func(ctx context.Context, resource *resources.TestResource) (reconcilers.Result, error) { + return reconcilers.Result{RequeueAfter: 2 * time.Minute}, reconcilers.HaltSubReconcilers + }, + }, + } + }, + }, + ExpectedResult: reconcilers.Result{RequeueAfter: 1 * time.Minute}, + ShouldErr: true, + }, "preserves result, Requeue": { Resource: resource.DieReleasePtr(), Metadata: map[string]interface{}{ @@ -3941,19 +4069,20 @@ func TestCastResource(t *testing.T) { }, ExpectedResult: reconcilers.Result{Requeue: true}, }, - "return subreconciler err": { + "return subreconciler err, preserves result": { Resource: resource.DieReleasePtr(), Metadata: map[string]interface{}{ "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { return &reconcilers.CastResource[*resources.TestResource, *appsv1.Deployment]{ Reconciler: &reconcilers.SyncReconciler[*appsv1.Deployment]{ - Sync: func(ctx context.Context, resource *appsv1.Deployment) error { - return fmt.Errorf("subreconciler error") + SyncWithResult: func(ctx context.Context, resource *appsv1.Deployment) (reconcilers.Result, error) { + return reconcilers.Result{ Requeue: true }, fmt.Errorf("subreconciler error") }, }, } }, }, + ExpectedResult: reconcilers.Result{Requeue: true}, ShouldErr: true, }, "marshal error": {