Proposal: Add reconciliation buffer to reduce load

[toonsevrin]

Some of my controllers receive a bunch of reconciliation requests. However there's no improvement to the controller quality if I handle them all individually compared to handling them once every now and then.

I propose that we can optionally add a following controller option to these options:

controller-runtime/pkg/controller/controller.go

Lines 33 to 44 in 2361b05

	type Options struct {
	// MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. Defaults to 1.
	MaxConcurrentReconciles int
	// Reconciler reconciles an object
	Reconciler reconcile.Reconciler
	// RateLimiter is used to limit how frequently requests may be queued.
	// Defaults to MaxOfRateLimiter which has both overall and per-item rate limiting.
	// The overall is a token bucket and the per-item is exponential.
	RateLimiter ratelimiter.RateLimiter
	}

I'm definitely not an expert, but I think the option could have the same interface as the rate limiting interface, but instead of working on retries, working on all requests?

[vincepri]

/priority awaiting-more-evidence

Can you clarify your use case a little bit more?

[vincepri]

/kind design

[toonsevrin]

/priority awaiting-more-evidence

Can you clarify your use case a little bit more?

This is generally useful when a CRD may be changed extremely often (eg multiple times per second) while you do not care about the reconciliation happening for each of these changes. Instead, you are fine with it happening once every now and then on the latest change.

[toonsevrin]

My personal use case is a controller which listens to all secret, namespace and serviceaccount changes. The quality of the controller would however not really change if the reconciliation was done only once every, say, 10 seconds. Even if 100 events were watched in this period.

[vincepri]

ah yes, that's what we initially understood when chatting at the community meeting. It seems an interesting use case indeed

cc @DirectXMan12

[toonsevrin]

I think so too! Another use case is where there is assymetry: eg. When changing one resource (for example a deployment) results in the controller changing hundreds of other resources. Adding a buffer protects these controllers from being dossed

It becomes even more useful when you create a circular reconciliation loop by accident: Let's say controller A watches secret changes and updates the relevant deployments whenever a secret is changed. Controller B updates relevant secrets whenever a deployment is updated. All though this is horrible in general, a buffer on B would at least prevent this from completely blowing up :)

[DirectXMan12]

aah, that makes sense. I think we'd have to heavily document this to avoid footguns, but this seems like a reasonable use case.

@toonsevrin can you write up a short design in a teensy bit more detail? Mainly interested in "how do we teach this to avoid confusion with the backoff rate limitter".

[DirectXMan12]

Here's our (short-and-sweet) template: https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/master/designs/template.md

[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

[vincepri]

/lifecycle frozen