-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
🐛 Revert: move health probes to runnable #2321
Conversation
/assign @vincepri @alvaroaleman /hold |
/hold cancel Ran the Cluster API test with the CR version from this PR locally and everything works as expected again. |
(cc just fyi @zqzten, we're reverting this part for now to ensure CR main works again, we can work on a new implementation independent of that) |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer, vincepri 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 |
I think we can introduce a new "basics" runnable group which includes the previous internal servers and start it before any other runnable groups, this seems to solve the dead lock issue here. wdyt @sbueringer |
Yup, something like this would work. |
This PR reverts part of: #2275
On main we have the following sequence in controllerManager.Start():
Usually this works except under the following circumstances (concrete example):
Now we have the following problem:
tl;dr cyclic dependency between start caches and start runnables.
This PR solves this by immediately starting the health probes again.