-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[deps][breaking] Upgrade controller-runtime to v0.20.1
#11
[deps][breaking] Upgrade controller-runtime to v0.20.1
#11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this out
assert.Equal(t, queue.Len(), len(tc.expectedLogs)) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
v0.19.4
v0.19.4
v0.20.1
Co-authored-by: Harvey Xia <[email protected]>
Closes #12
💸 TL;DR
controller-runtime
to versionv.0.20.1
WatchesRawResource
- breaking change.📜 Details
Due to the recent breaking PRs on
controller-runtime
kubernetes-sigs/controller-runtime#2799 and kubernetes-sigs/controller-runtime#2783, I had to make some decisions and I would appreciate your inputs on it:WatchesRawSource
with two new builder functions:WatchesChannel
andWatchesKind
because you cannot create a rawsource.Source
anymore.WatchesRawResource
anymore 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