⚠️ Deprecate reconcile.Result.Requeue

[alvaroaleman]

There is no good reason to use this setting, either an error or RequeueAfter should be used instead. Deprecate it to avoid confusion. From the godoc:

// This setting is deprecated as it causes confusion and there is
// no good reason to use it. When waiting for an external event to
// happen, either the duration until it is supposed to happen or an
// appropriate poll interval should be used, rather than an
// interval emitted by a ratelimiter whose purpose it is to control
// retry on error.

[fabriziopandini]

I just want to bring up some cases when requeue helped us in Cluster API

When a reconcile loop is depending on external objects that you cannot actively watch, having in controller runtime an option to requeue with an exponential backoff (without raising an error) helped CAPI to ensure a good reactivity of the system without overload it.

When we tried to use RequeueAfter for similar use cases instead we faced a few issues:

• short RequeueAfter periods makes the system more reactive, but if you keep requeing frequently for long you can get into other problems, like e.g.
  • rate limiting on external resources
  • scalability problems in your controllers if too many objects are requeing frequently at the same time
• long RequeueAfter periods makes the system less reactive

Note. In our experience the current implementation of requeue works when things in the external system happens quickly, less when the exponential backoff was getting too high.
Note. Whenever possible, we are replacing requeue/requreAfter with watches, but in some cases this is not feasible

[alvaroaleman]

Note. In our experience the current implementation of requeue works when things in the external system happens quickly, less when the exponential backoff was getting too high.

Right, but wouldn't it be a lot better to set up your ratelimiter for this and use what interval it emits in RequeueAfter rather than re-using one that is not meant for that and where you already noticed that the ceiling is too high?

[fabriziopandini]

Right, but wouldn't it be a lot better to set up your ratelimiter for this and use what interval it emits in RequeueAfter rather than re-using one that is not meant for that and where you already noticed that the ceiling is too high?

What exists today in controller runtime worked for use in most cases, and this was enough for us in Cluster API to avoid creating our own custom rate limiter (having to implement one will be a step back).

Looking at this from another angle, if CR runtime is going to provide as a building block a rated limiter to use in controller with requreAfter, then the impact of deprecation/removal of requeue will be less relevant.

[alvaroaleman]

Looking at this from another angle, if CR runtime is going to provide as a building block a rated limiter to use in controller with requreAfter, then the impact of deprecation/removal of requeue will be less relevant.

Not controller-runtime itself, but there are ratelimiters in the upstream workqueue package: https://pkg.go.dev/k8s.io/client-go/util/workqueue

[fabriziopandini]

@alvaroaleman thanks for sharing your PoV, appreciated
As far as we have options to address more advanced use cases, I'm ok.

[sbueringer]

/lgtm

/hold
/assign @vincepri

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 03aec75ade1ae9ba8d53fba906b118608bc4e2b6

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

/hold cancel

[jonathan-innis]

What exists today in controller runtime worked for use in most cases, and this was enough for us in Cluster API to avoid creating our own custom rate limiter

FWIW, we also thought the use of Requeue was reasonable in kubernetes-sigs/karpenter and were a bit surprised to see this deprecated in v0.21