Reconcile errors vs failure to make progress

[squaremo]

When using controller-runtime, if you return an error from a Reconcile method, the error stack trace will be logged, and the request will be requeued with a backoff.

source-controller will return an error from Reconcile in circumstances like failing to create credentials from a secret (https://github.com/fluxcd/source-controller/blob/master/controllers/helmrepository_controller.go#L199). This is an error, but it's an error on the part of the user, or a temporary situation, rather than an exceptional problem.

I think it is worth distinguishing between exceptional problems (e.g., the controller can't reach the API server), and a failure to make progress for the moment (e.g., the authentication secret doesn't have the right fields), for these reasons:

• it's the operator of the cluster that needs to know about exceptional problems, but the user of the cluster that needs to know about a failure to make progress. It's the user that will have to intervene to e.g., correct the authentication secret. The appropriate place to signal the latter kind of problem is in a custom resource status, not in controller logs
• it's not a failure of the controller if e.g., the user input isn't correct, or an external system can't be contacted; so while it might be noted in the logs and metrics regarding the resource in question, it's business as usual for the controller
• if the problem is user input, you probably don't want it to be unconditionally requeued -- you can just wait until the problematic resource is updated and is queued for that reason. (If the problem is in some dependency, like a secret, this can be a bit of trouble to arrange; in those cases, you can just return ctrl.Result{Requeue: true}, nil, which apparently will also do backoff for you)
• in terms of metrics, it seems useful to have distinct metrics for "reconcile crashed" and "unable to converge for this resource". Would you want to be woken by pagerduty because someone created a duff ImageUpdateAutomation object?

(The controller-runtime/kubebuilder community don't appear to have a settled idea of how this is supposed to work -- see the comments in kubernetes-sigs/controller-runtime#617 for example, especially kubernetes-sigs/controller-runtime#617 (comment))

[squaremo]

A similar conflict arises for the question "when has a controller successfully reconciled an object"? If returning nil is counted as success, then messages like this one ought to be revised: https://github.com/fluxcd/kustomize-controller/blob/master/controllers/kustomization_controller.go#L111

[seaneagan]

I think it is worth distinguishing between exceptional problems (e.g., the controller can't reach the API server), and a failure to make progress for the moment (e.g., the authentication secret doesn't have the right fields), for these reasons:

I think this is an important consideration (how likely are the requeues to succeed). In many cases though it is hard to know, for example k8s API failures could be due to e.g. rare intermittent etcd timeouts, OR could mean the entire cluster is down.

Another consideration is how expensive/risky is it to run the extra requeues, taking into account the exponential backoff which helps limit the amount of requeues.

In the case of the helm-controller, helm rapid install/upgrade/uninstall/rollback actions can be expensive and/or risky which is why we default to 0 retries, and require the user to opt in to additional ones. We could probably change it such that after retries are exhausted, exponential backoff no longer happens, since it will likely just result in "retries still exhausted" in this case.

I assume in all cases we'd want to honor the spec.interval requeue, which is useful in case the cause of the failure is something external to CR input, and it resolves itself, and this gives the user some control over the requeue aggressiveness.