Add support for applying jitter when requeuing resources after reconcile

[toastwaffle]

Description of your changes

Adds a WithPollJitter option to managed.Reconciler to support adding jitter to reconcile delay. Jitter defaults to 0, meaning this has no behavioural change unless the WithPollJitter option is set.

Fixes #312

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Ran tests in pkg/reconciler/managed/reconciler_test.go

[toastwaffle]

@turkenh @MisterMX could you take a look at this please?

[MisterMX]

To provide more flexibility to provider developers it might make sense to not hardcode the jitter algorithm and instead provide a hook via a delegate that can be passed via an option:

Suggested change

	reconcileAfter := r.pollInterval + time.Duration((rand.Float64()-0.5)*2*float64(r.pollJitter)) //#nosec G404 -- no need for secure randomness
	reconcileAfter := r.modifyRequeueAfter(managed, r.pollInterval)

In the future this might also be a way for providers to individually decide on the requeue time for individual resources.

[toastwaffle]

How do you feel about doing both, and having the modifyRequeueAfter hook accept both the configured poll interval and computed jitter (with the default implementation being to add them together)?

I think the library should strongly encourage provider developers to include support for jitter as a relatively well-establish best practice (whether they hardcode the amount or expose a flag), rather than it being something they each have to remember to do when they discover the hook exists?

[MisterMX]

I am not opposed to the Jitter itself or the formular it is calculated with. I also think it makes sense to provide a default Jitter with the NewReconciler. I just would prefer to have the Jitter algorithm (or any kind of "delay" mechanic) not directly implemented within the reconciler but provide it via some form of plugin mechanism.

[toastwaffle]

What do you think of this approach, with the WithJitter option implemented as a WithPollIntervalHook?

[MisterMX]

Looks good to me. However, is support for multiple hooks really necessary? I think it makes the hook mechanic more complex then it needs to be. And I spontaneously cannot think of a scenario where two hooks are dependent on each other (unlike overwriting).

[toastwaffle]

The main situation I'm thinking of is letting somebody do resource-based customisation, and have jitter applied on top, without them having to write(/copy-and-paste) their own jitter logic. Happy to change it to a single hook if you think that's insufficient justification.

[MisterMX]

If you would have multiple hooks they would be called for every resource anyway and since it is all inside a single controller I think one hook should be fine. If people want to apply resource-based jitter then I assume it would suffice if they only implement one hook that can decide on it.

[toastwaffle]

Okay, switched it to a single hook

[toastwaffle]

@MisterMX can you merge this, or do we need @turkenh ?

[toastwaffle]

Also, can this be released as a patch on v1.13, or will it wait for v1.14 (which looks like it's not going to be released until end of October based on https://github.com/crossplane/crossplane/milestone/17?)

[MisterMX]

I can't merge since I am not a maintainer - only a reviewer.

[turkenh]

Looking good! Thanks for your contribution @toastwaffle!