[deps][breaking] Upgrade controller-runtime to v0.20.1
#11
Conversation
shadialtarsha
changed the title
There was a problem hiding this comment.
Some minor comments + we should think about reworking the WithChannels builder option because channel sources' value is that the events could originate from anything, not just Kubernetes object events, so we should avoid constraining the channel to client.Object.
One possible approach is to generify WithChannels to WithSources and allow callers to pass in arbitrary sources. Illustrative code here
| // NewZeroDelayManagedRateLimiter returns a rate limiter that takes the | ||
| // maximum delay between the passed provider and a per-item exponential backoff | ||
| // limiter. The exponential backoff limiter has a base delay of 0s and a maximum of 60s. | ||
| func NewZeroDelayManagedRateLimiter(provider workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimiter[reconcile.Request] { |
There was a problem hiding this comment.
Out of curiosity, why was this introduced? Is 1s too long of a base delay for certain use cases?
There was a problem hiding this comment.
Check this out
The current in production workqueue.DefaultControllerRateLimiter() has no delays what so ever, so tests were passing. However, the new DefaultControllerRateLimiter has an initial delay causing the tests to fail
https://github.com/kubernetes/client-go/blob/97f3d2697cceec0c57cdac5744d7f6ce34c1991d/util/workqueue/default_rate_limiters.go#L50
There was a problem hiding this comment.
Does that make sense?
There was a problem hiding this comment.
On review, I'm still not sure I understand this change—where is NewZeroDelayManagedRateLimiter being used?
Prior to this upgrade, this repo uses client-go v0.29.3` whose rate limiting logic appears to be identical to the upgraded version, with the exception of the type constraint.
|
@harveyxia I opened a PR on our internal ingress controller and pointed to This would be a good way to test it in staging before merging this PR. Wanna report that the Ingress Controller e2e tests, unit tests and integration tests are all passed successfully |
harveyxia
changed the title
v0.19.4
shadialtarsha
changed the title
v0.19.4v0.20.1
Co-authored-by: Harvey Xia <[email protected]>
Closes #12
💸 TL;DR
controller-runtimeto versionv.0.20.1WatchesRawResource- breaking change.📜 Details
Due to the recent breaking PRs on
controller-runtimekubernetes-sigs/controller-runtime#2799 and kubernetes-sigs/controller-runtime#2783, I had to make some decisions and I would appreciate your inputs on it:WatchesRawSourcewith two new builder functions:WatchesChannelandWatchesKindbecause you cannot create a rawsource.Sourceanymore.WatchesRawResourceanymore because we cannot wrap the handler with our own observer.🧪 Testing Steps / Validation
I replaced the SDK on my Ingress Controller and things look like they are working. I will do more tests as well to make sure we are good.
✅ Checks