Skip to content

Commit

Permalink
Don't just set Requeue = true when the reconciliation should be retried
Browse files Browse the repository at this point in the history
because of error conditions and instead report the error so that
controller-runtime applies exponential back-off to delays between
the retries.
  • Loading branch information
metlos committed Jan 30, 2024
1 parent bdeb44d commit e346523
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
toolchainClusterKey := runtimeclient.ObjectKey{Name: spaceProvisionerConfig.Spec.ToolchainCluster, Namespace: spaceProvisionerConfig.Namespace}
toolchainPresent := corev1.ConditionTrue
toolchainPresenceReason := toolchainv1alpha1.SpaceProvisionerConfigValidReason
requeue := false
var reportedError error
toolchainPresenceMessage := ""
if err := r.Client.Get(ctx, toolchainClusterKey, toolchainCluster); err != nil {
if !errors.IsNotFound(err) {
Expand All @@ -85,8 +85,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
// that prevents us from reading the ToolchainCluster, clears. I.e. we need the requeue to keep the promise
// of eventual consistency.

requeue = true
toolchainPresenceMessage = "failed to get the referenced ToolchainCluster: " + err.Error()
reportedError = fmt.Errorf("failed to get the referenced ToolchainCluster: %w", err)
toolchainPresenceMessage = reportedError.Error()
}
toolchainPresenceReason = toolchainv1alpha1.SpaceProvisionerConfigToolchainClusterNotFoundReason
toolchainPresent = corev1.ConditionFalse
Expand All @@ -101,8 +101,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
})

if err := r.Client.Status().Update(ctx, spaceProvisionerConfig); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to update the SpaceProvisionerConfig status: %w", err)
if reportedError != nil {
logger.Info("failed to update the status (reported as failed reconciliation) with a previous unreported error during reconciliation", "unreportedError", reportedError)
}
reportedError = fmt.Errorf("failed to update the SpaceProvisionerConfig status: %w", err)
}

return ctrl.Result{Requeue: requeue}, nil
return ctrl.Result{}, reportedError
}
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,12 @@ func TestSpaceProvisionerConfigReEnqueing(t *testing.T) {
}

// when
res, reconcileErr := r.Reconcile(context.TODO(), req)
_, reconcileErr := r.Reconcile(context.TODO(), req)
spcInCluster := &toolchainv1alpha1.SpaceProvisionerConfig{}
require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(spc), spcInCluster))

// then
assert.NoError(t, reconcileErr)
assert.True(t, res.Requeue)
assert.Error(t, reconcileErr)
AssertThat(t, spcInCluster, Is(NotReadyWithReason(toolchainv1alpha1.SpaceProvisionerConfigToolchainClusterNotFoundReason)))
assert.Len(t, spcInCluster.Status.Conditions, 1)
assert.Equal(t, "failed to get the referenced ToolchainCluster: "+getErr.Error(), spcInCluster.Status.Conditions[0].Message)
Expand Down

0 comments on commit e346523

Please sign in to comment.