Skip to content
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

Add 'ready' entrypoint for exec-based readiness checks #380

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

andrewstucki
Copy link
Contributor

@andrewstucki andrewstucki commented Jan 9, 2025

This is currently stacked on the PR wiring up the ready probe for the sidecar, but it introduces an entrypoint for also issuing a single exec-based readiness check using the same underlying prober (but with the more relaxed IsClusterBrokerReady rather than executing IsClusterBrokerHealthy). Adding this as after talking with @birdayz we discussed that for the current v1-based nodes in cloud the constrained resources would likely not work well with provisioning an additional sidecar that would sap resources from the main container.

Instead they can exec the probe directly in the main container and we move ever so closer to having more shared components between the v1/v2 implementation.

Once #379 lands I can adjust the base branches here.

@RafalKorepta
Copy link
Contributor

Adding this as after talking with @birdayz we discussed that for the current v1-based nodes in cloud the constrained resources would likely not work well with provisioning an additional sidecar that would sap resources from the main container.

I almost agree with this statement. The exec probe would eat resources too regardless of it's place. The issue of specifying another container that should have request and limits might be problematic in cloud environment.

@andrewstucki
Copy link
Contributor Author

@RafalKorepta sorry, yes, I should have worded that more carefully, the resource limits is the main concern.

Base automatically changed from wireup-prober-to-sidecar to main January 10, 2025 18:43
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.

2 participants