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

feat: add config-reloader readiness #1075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheSpiritXIII
Copy link
Member

@TheSpiritXIII TheSpiritXIII commented Jul 16, 2024

This change adds a readiness endpoint to the config-reloader. This helps Kubernetes be more accurate in accessing that workloads are up.

The primary reason I added it is for #1076 -- I wanted the config-reloader to start running before Prometheus is running. However, the config-reloader must hit a reload URL. As a "hack", I made it hit an endpoint of itself that does nothing. Since this endpoint is functionally the same as a readiness endpoint, I figured we might as well add a few more missing pieces to turn this into a readiness endpoint that Kubernetes can hit. This also helps make the PRs smaller and hopefully easier to review.

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/config-ready branch 2 times, most recently from 6c4038c to 42d972c Compare July 17, 2024 14:56
@pintohutch
Copy link
Collaborator

Can you fill out the description and describe what this change is doing and the motivation for it?

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/config-ready branch from 42d972c to 28e6cf4 Compare August 2, 2024 14:30
@TheSpiritXIII
Copy link
Member Author

@pintohutch I updated the description. Feel free to push back on this change!

@pintohutch
Copy link
Collaborator

@TheSpiritXIII - IIUC our discussion around supporting "empty" mandatory flag/config will make this change unnecessary. Is that your understanding as well?

@TheSpiritXIII
Copy link
Member Author

@pintohutch yes this change becomes non-critical. This PR may still provide a slightly better signal over when a workload is "ready" than we have today, but again, let's not prioritize it unless we need to.

@bwplotka
Copy link
Collaborator

LGTM, but we need to resolve the conflicts. Thanks!

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.

3 participants