-
Notifications
You must be signed in to change notification settings - Fork 80
chore: add probes on all containers #234
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
Conversation
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.
Hi! Thanks for your contribution!
The /metrics
endpoint generate metrics on the fly, so relying on it for probes isn't very cost-efficient. Could you add and use a /healthz
endpoint instead? The liveness and readiness probes can both rely on it. You don't have to implement any logic, in the futur we could think about implementing some real checks, but for now simply responding would be enough.
Sure, that makes much more sense I'll work on this soon and will update the branch |
@paullaffitte I added a super basic handler to the ServeMux (/healthz as suggested) |
1f45928
to
aa331fb
Compare
Hi, sorry for the delay. I was busy on other open-source projects. To not make you wait more I rebased it to solve merge conflicts so I will be able to merge it today. EDIT: I'm now evaluating the fact that when kube-rbac-proxy is enabled, you move the probes on this container instead. I don't know if it is the way to go. I didn't implement this part myself and I don't know much about this tool. I will try to ask my colleagues about it before merging. |
🎉 This PR is included in version 3.19.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.19.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Hello,
I'm deploying this exporter on my cluster and we have a Kyverno policy that requires all pods to have liveness/readiness probes
Since those are exporters containers, I felt it was not a bad thing to make sure that the exporter is indeed listening on the exposed port
It's best practice to not have liveness === readiness but in this case I think this is acceptable, unless you want to take the time to create proper HTTP endpoints in the code to add a /live and a /ready (I can do it but I'm not sure this is worth it)