✨Make workqueue rate limiter configurable during controller creation

[rajathagasthya]

Exposes rate limiter as a controller option in case users want to use
a different rate limiter than the default one provided by
controller-runtime.

Fixes #631

[k8s-ci-robot]

Hi @rajathagasthya. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[rajathagasthya]

/assign @DirectXMan12

[DirectXMan12]

This seems reasonable. I'm iffy on the naming, but it currently has the nice property that the signatures match the client-go one, making the cast straightforward.

/ok-to-test

[shawn-hurley]

I am wondering why we would create our own RateLimiter interface.

if the client-go one changes we would have to decide:

1. To update our interface and still make it a copy. If this is the path then why would we have a copy?
2. Decide to create a shim, which will confirm to our interface and pass through correctly to the DefaultControllerRateLimiter.
3. Write our own rateLimiter as the default.

If we always choose 1, I think we should just use their interface and deal with the breaking API changes when they are made.

If we choose 2 or 3 don't we have a problem with users using other rate limiters from the work queue package in the option that would no longer compile? I am wondering if this is the more unexpected behavior then having to make a breaking change if this interface changes?

I think my vote is to remove the controller-runtime rate-limiter and use the client-go rate limiter even though we may be exposing an interface that we can not control, and therefore could break compatibility if we do not do a major version bump.

@DirectXMan12 thoughts?

[rajathagasthya]

Is just copying and maintaining those client-go rate limiters a bad idea? That way we are insulated from changes to those interfaces and their implementations. But maybe we would like those changes as well?

[DirectXMan12]

The idea is that we didn't want a change in client-go to force us to do a major revision.

[rajathagasthya]

@DirectXMan12 @shawn-hurley Any thoughts on how to proceed with this change? I'd love to get this merged soon and use it since we sorely need this option.

[DirectXMan12]

I think we're good to go forward with this the way it is now

[vincepri]

/lgtm

[vincepri]

/assign @DirectXMan12 @gerred

[jimmidyson]

Was just looking for this... would love to see this merged 🙏 @DirectXMan12. Thank you for adding this @rajathagasthya!

[gerred]

@vincepri @jimmidyson Looking now, I've been in meetings but will turn attention to this in the next hour.

[gerred]

this is great, thank you @rajathagasthya

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gerred, rajathagasthya

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 [gerred]

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

[mars1024]

@rajathagasthya This is a wonderful PR, could you help to backport this to older versions like 0.4.0 and 0.5.0?

[rajathagasthya]

@mars1024 I'm happy to backport. Thoughts @DirectXMan12 @vincepri?

[mars1024]

friendly ping @DirectXMan12 @vincepri , a configurable MakeQueue is strongly needed anywhere ~