Skip to content

Stop apiimporter, syncer, heartbeat from fighting over the Ready condition #864

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

Merged
merged 3 commits into from
Apr 8, 2022

Conversation

csams
Copy link
Contributor

@csams csams commented Apr 7, 2022

Give each controller its own condition and make heartbeat manager responsible for setting Ready by accounting for all conditions via conditions.SetSummary

Update the namespace scheduler to depend on total readiness.

Fixes #865

@csams csams force-pushed the fix-ready-slapfight branch 2 times, most recently from 6075c80 to 5bda7f7 Compare April 8, 2022 02:44
@csams csams requested review from ncdc and marun April 8, 2022 02:46
@csams csams marked this pull request as ready for review April 8, 2022 03:27
@csams csams force-pushed the fix-ready-slapfight branch from 5bda7f7 to 243f2b3 Compare April 8, 2022 13:24
Give each controller its own condition and make them responsible for
setting Ready by accounting for all conditions via conditions.SetSummary

Signed-off-by: Christopher Sams <[email protected]>
@csams csams force-pushed the fix-ready-slapfight branch from 243f2b3 to 9d381ac Compare April 8, 2022 13:25
@ncdc
Copy link
Member

ncdc commented Apr 8, 2022

@marun @imjasonh how does this look to you?

For background/additional context: the conditions library we're using (from Cluster API) went through extensive R&D, settling on the use of SetSummary to summarize a resource's Ready condition. It's a good practice to follow, and even if we only currently have a single condition that represents Ready, for future-proofing, we should choose a unique name for that condition, and have it flow into Ready via SetSummary.

Copy link
Contributor

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change, thanks! 👍

@ncdc ncdc merged commit 669da02 into kcp-dev:main Apr 8, 2022
kylape pushed a commit to kylape/kcp that referenced this pull request Apr 14, 2022
Stop apiimporter, syncer, heartbeat from fighting over the Ready condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apiimport_manager, push/pull managers, and heartbeat manager fight over workload cluster readiness
3 participants