-
Notifications
You must be signed in to change notification settings - Fork 468
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
Adds timeouts for reconciler contexts #7147
Conversation
Skipping CI for Draft Pull Request. |
3cffc9a
to
a8cd66a
Compare
/assign |
pkg/controllermanager/controller/seed/backupbucketscheck/reconciler.go
Outdated
Show resolved
Hide resolved
@timebertt thanks for the review! There was something else that I thought would be good to add - timeout contexts for some of our mapper and predicate functions that also inject the parent context (stop channel) from the manager, as they could potentially block as well, if there are calls to the api server. |
a8cd66a
to
9b294c5
Compare
@timebertt @rfranzke, this should be ready for review now. I've still used just a single context for the gardener/pkg/controllermanager/controller/seed/backupbucketscheck/reconciler.go Lines 47 to 48 in 9b294c5
and the extensionscheck reconciler: gardener/pkg/controllermanager/controller/seed/extensionscheck/reconciler.go Lines 54 to 55 in 9b294c5
because those checks are actually pretty fast and small, so I think that only one context is enough there. However, if you feel that's incorrect I will apply the initial suggestion to separate them in two contexts: #7147 (comment) Edit: oops, forgot about also adapting the tests that use mocks. Fixing them now ... |
0502c77
to
4a8d485
Compare
pkg/resourcemanager/controller/health/progressing/reconciler.go
Outdated
Show resolved
Hide resolved
… controllers in controllermanager - Make `ShootQuotaControllerConfiguration` a pointer - Make `SyncPeriod` pointer. - Default `SyncPeriod` to 60 minutes.
Thanks for addressing my comments. Leaving it to @timebertt if he wants to take one more look. |
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.
One remaining comment, otherwise lgtm
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: 68635569718b7c4f98a87cf0e02fa42b179092d2
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: timebertt 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:
Approvers can indicate their approval by writing |
* Add timeout for reconciler contexts * Adapts unit tests * Fixes integration tests * Use separate context for garden client and seed client * Address PR review feedback * Fix failing test * Address PR review feedback * Introduce utility function to have consistent contextTimeouts in all controllers * [PR feedback] Revert back the changes. * Address PR review feedback * Streamline resourcemanager controller to use context with timeouts * Streamline ShootQuotaControllerConfiguration struct inline with other controllers in controllermanager - Make `ShootQuotaControllerConfiguration` a pointer - Make `SyncPeriod` pointer. - Default `SyncPeriod` to 60 minutes. * Remove timeout from context in case of seed and shoot reconcilers for now. * Address PR review feedback --------- Co-authored-by: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
How to categorize this PR?
/area scalability
/kind enhancement
What this PR does / why we need it:
This PR introduces a timeout for contexts used by reconciler functions.
Which issue(s) this PR fixes:
Part of #4251
Special notes for your reviewer:
As the issue also mentions adding waits for cache syncs - this comes out of the box when using controller runtime with a default timeout of 2 minutes.
The shoot and seed care controllers were already using contexts that timeout for the health check functions:
gardener/pkg/gardenlet/controller/shoot/shoot_care_control.go
Lines 233 to 234 in a60a42f
and
gardener/pkg/gardenlet/controller/seed/care/reconciler.go
Lines 61 to 62 in a60a42f
I was not sure whether we should keep those as they are, or unify them so that only one time context is used.
Release note: