[WIP] pkg/restmapper: use exponential backoff with DynamicRESTMapper calls

[estroz]

Description of the change: add exponential backoff to DynamicRESTMapper.reloadOnErr() calls.

Motivation for the change: see this discussion from #1329.

[theishshah]

I think it makes sense to have the simple back off as a fall-back before failing completely

[AlexNPavel]

I would say yes. Backoff duration hitting its maximum is, in the end, a wait timeout.

[joelanford]

If I'm understanding this correctly, we'll block the caller by sleeping for the backoff duration if we attempt a reload and get a NotKindMatchError. Is that correct?

I wonder if a better UX would be for us to be able to return a "you've been rate-limited" error immediately and let the caller have the chance to do other things in the meantime? On the other hand, that would then probably mean we'd need to return an error that the caller is able to recover the wait duration from.

I suppose if they need concurrency around this, they could do it themselves with their own goroutine.

Thoughts?

[estroz]

@joelanford that would be better UX, especially if this is being used in a reconcile loop. We could use the token bucket approach: if the limiter Allows an event to happen, reload; if not, return an ErrRateLimited. We can expose a method on DynamicRESTMapper.Delay() to get the time to wait if ErrRateLimited is returned, or expose a field on ErrRateLimited with that value.

[joelanford]

@estroz I was thinking the same think about the reconcile loop issue.

@DirectXMan12 This is definitely related to the conversation going in kubernetes-sigs/controller-runtime#321. Any thoughts on the rate-limiting approach?

[openshift-ci-robot]

@estroz: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-ansible	6f146c2	link	/test e2e-aws-ansible
ci/prow/e2e-aws-go	6f146c2	link	/test e2e-aws-go
ci/prow/images	6f146c2	link	/test images
ci/prow/e2e-aws-helm	6f146c2	link	/test e2e-aws-helm

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[estroz]

Instead of making two PR's with these changes (one for the SDK, one to upstream DynamicRESTMapper), one has been made to controller-runtime (kubernetes-sigs/controller-runtime#554) and a new PR will remove pkg/restmapper from the SDK and make necessary dependency changes once the former is merged and a release made.