-
Notifications
You must be signed in to change notification settings - Fork 5.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
[kube-prometheus-stack] add serviceName option to prometheus CR #5273
base: main
Are you sure you want to change the base?
Conversation
a3d4842
to
9793ce2
Compare
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.
In general, I like this change. Currently, the service is just prometheus-operated or alertmanager-operated and the are different from all other names.
However, this is a breaking changes. A lot of people may depend against the operated service names (including the helm chart itself + private deployments)
In order to approve this PR, my requirements are:
-
fix all references to the hardcoded service names within the helm chart, e.g.
helm-charts/charts/kube-prometheus-stack/templates/grafana/configmaps-datasources.yaml
Line 54 in f4f38ef
url: http://prometheus-{{ template "kube-prometheus-stack.prometheus.crname" $ }}-{{ . }}.prometheus-operated:9090/{{ trimPrefix "/" $.Values.prometheus.prometheusSpec.routePrefix }} -
Ensure, that the service generated by helm differs from the service name generated by the operator (https://github.com/prometheus-community/helm-charts/blob/f4f38ef92e6cb01433ac0e2546bed0774e3b9d7a/charts/kube-prometheus-stack/templates/prometheus/service.yaml)
-
Add a toggle to disable the service generated by helm
-
Bump the major version
-
Add a note into the README and add a snippet how to restore the orginal behavior
And what about Alertmanager?
agree! I will work on those changes, thanks for the feedback
Currently alertmanager spec does not support serviceName option, but there is open issue for it and ThanosRuler. |
good catch, but it seems implemented. prometheus-operator/prometheus-operator#5632 I will contribute that feature to AM resource. Once implemented, they in this PR, all three components can be adjusted. |
this should be just passing the name of the operated service which is created by default right now definitely a place to think about when actually implementing the serviceName option |
Maybe keep this on hold until upstream changes are done. I would like to avoid 2 breaking changes. |
- AM - Prometheus - ThanosRuler Signed-off-by: trouaux <[email protected]> Signed-off-by: Thomas Rouaux <[email protected]>
3c85a35
to
b4cf5d2
Compare
What this PR does / why we need it
Adding serviceName option to Prometheus CR. This allows prometheus-operator reconciler to associate templated Service to Prometheus StatefulSet instead of creating an operated service.
see https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.PrometheusSpec
Which issue this PR fixes
Special notes for your reviewer
Checklist
[prometheus-couchdb-exporter]
)