Allow a result that indicates the reconciliation is incomplete and does not trigger the exponential backoff logic

[akutz]

After discussing this with @detiber, we realized there's no good solution for the following case:

• A request is received
• The request cannot proceed because of a missing dependency on resource A
• The reconciler is watching events for resource A
• The reconciler returns a result that indicates the reconciliation is incomplete due to reason X, but the request should not be requeued

Instead the current logic is:

• A reconciler is using one or more watches to trigger requests
• A request cannot proceed due to a missing dependency
• If result.RequeueAfter > 0 then the request is added to the queue for processing after the value specified by result.RequeueAfter
• If result.Requeue is true then the request is added to the queue with the same exponential backoff logic used when an error is returned
• If an error is returned then the request is added to the queue with the exponential backoff logic

Today there is currently no way to indicate a reconciliation is incomplete without also having the request requeued by the manager either via an explicit amount of time or the exponential backoff logic (due to error or Requeue == true).

There should be a way to signal:

• The reconciliation is incomplete
• Do not requeue, the reconciler is watching the resources necessary to trigger its own events

Thanks!

[detiber]

To add a bit of additional context it is currently very difficult to test controller-runtime based reconciliation loops with the current behavior if there are cases where only partial reconciliation is expected due to external dependencies that trigger reconciliation based on watches.

Allowing for a case where Requeue is explicitly set to false and an error is returned would allow for easier testing of these cases.

[DirectXMan12]

To summarize: you want an error condition that says "don't requeue" (i.e. what we've referred to as ignorable errors in #377)?

[DirectXMan12]

/kind feature

If so, can we move the discussion over there?

[akutz]

Hi @DirectXMan12,

Yes, but I also wonder now if this is the right way to do this. After speaking more about this with @detiber and @vincepri, I created this example that relies on asserting expected, eventual state of the objects.

[DirectXMan12]

Yeah, we've thus far been recommending writing tests like that (using eventually and/or consistently), since it allows you to update your logic to occur over multiple reconciles, etc, w/o needing to update your tests.

P.S. Ginkgo tip: Expect(operation()).To(Succeed()) and Expect(err).NotTo(HaveOccurred()), but try not to Expect(operation()).NotTo(HaveOccurred()), since that's a bit confusing.

Also, Eventually(func() int { return operation() }).Should(BeNumerically(">", 10)) should yield better errors than just returning a bool, but that ones more of a matter of of taste.

[detiber]

Yes, the problem is very similar to #377.

The specific use case that is difficult to test for with the current model: We have some resources that we want to wait until they have an OwnerRef from a related resource prior to reconciling. Currently we have no way to test Reconcile in a way that tells us: 1) we haven't mutated the resource 2) We haven't completed a full reconciliation without requeueing.

Since the update to the ownerRef would trigger a new reconciliation, requeueing is pretty pointless here.

[akutz]

Hi @DirectXMan12,

Thank you again for your suggestions. I implemented most of them here!

[enxebre]

I came through this and #377 as well. @DirectXMan12 Some thoughts:

One approach:

type Result struct {
	// Requeue tells the Controller to requeue the reconcile key.  Defaults to false.
	Requeue bool
        // RequeueError tells the Controller to requeue the reconcile key when error is not nil.  If nil defaults to true.
	RequeueError *bool
	// RequeueAfter if greater than 0, tells the Controller to requeue the reconcile key after the Duration.
	// Implies that Requeue is true, there is no need to set Requeue to true at the same time as RequeueAfter.
	RequeueAfter time.Duration
}

if result, err := c.Do.Reconcile(req); err != nil {
	if requeueError(result.RequeueError) {
		c.Queue.AddRateLimited(req)
		log.Error(err, "Reconciler error", "controller", c.Name, "request", req)
		ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc()
		ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc()
		return false
	}
	c.Queue.Forget(obj)
	return false
}

func requeueError(requeueError *bool) bool {
	if reconcileError == nil {
		return true
	}
	return *requeueError
}

Another approach:

if result, err := c.Do.Reconcile(req); err != nil {
	if requeueError(err) {
		c.Queue.AddRateLimited(req)
		log.Error(err, "Reconciler error", "controller", c.Name, "request", req)
		ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc()
		ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc()
		return false
	}
	c.Queue.Forget(obj)
	return false
}

func requeueError(err error) bool {
	if apierrors.IsNotFound(err) {
		return false
	}
	if _, ok := err.(*client.IgnorableError); ok {
		return false
	}

	return true
}

// IgnorableError represents an error that should not be
// requeued for further processing.
type IgnorableError struct {}

// Error implements the error interface
func (e *IgnorableError) Error() string {
	return fmt.Sprintf("%s", e)
}

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[detiber]

/remove-lifecycle stale

[DirectXMan12]

I'm tempted to lean towards tying this into ignorable errors, because it seems to be another side of the same die, if you will. There's a few of options there:

1. make ignorable a property you can implement on errors
2. make ignorable a wrapper that we test with errors.Is or errors.As (not mutually exclusive with option 1)
3. make ignorability configurable via some sort of "error filter" that gets to mess with requeue logic. We default to requeuing in most cases except some sane set (like NotFound).