Add metrics Service for prometheus-operator support #19
Conversation
sslavic
force-pushed
the
metrics-service
branch
from
466be42
to
b143841
Compare
|
Tested new nginx IC metrics service on giraffe, that its endpoints can be discovered (used as scraping targets) by prometheus-operator managed Prometheus. Used prometheus-operator-app which comes even with a ServiceMonitor definition for GS nginx IC https://github.com/giantswarm/prometheus-operator-app/blob/3351bf836a1087102233cd705d89e8dcb3720dd1/helm/prometheus-operator-app-chart/values.yaml#L1519-L1528 Thanks @pipo02mix for providing the prometheus-operator-app, it made testing this easy. |
| @@ -8,7 +8,7 @@ metadata: | |||
| giantswarm.io/service-type: "managed" | |||
| k8s-app: {{ .Values.controller.k8sAppLabel }} | |||
| annotations: | |||
| prometheus.io/port: '{{ .Values.controller.metricsPort }}' | |||
| prometheus.io/port: '{{ .Values.controller.metrics.port }}' | |||
| prometheus.io/scrape: "true" | ||
| prometheus.io/port: "10254" | ||
| prometheus.io/scrape: 'true' | ||
| prometheus.io/port: '{{ .Values.controller.metrics.port }}' |
There was a problem hiding this comment.
WHy the annotations are in this Service and not in the headless one?
There was a problem hiding this comment.
AFAIK these annotations are "classic" way of publishing metadata about prometheus exporter targets, while the new headless metrics Service is for prometheus-operator way of doing the same thing.
Classic metadata on NodePort (azure) and LoadBalancer (aws) types of Service doesn't harm prometheus-operator when new metrics Service is enabled.
prometheus-operator (ServiceMonitor) needs just metrics Service spec + Endpoints coming out of metrics Service. From my test it seems that absence of classic annotations didn't seem to do any harm, discovery works. Still, please check/verify if you see something strange in discovered Targets on Prometheus on giraffe - 5k5hg TC, it's still up from my tests, you can access it locally after running
$ kubectl port-forward -n monitoring prometheus-prometheus-operator-promet-prometheus-0 9090
There's also Grafana with nginx dashboard, you can access Grafana locally after running
$ kubectl port-forward -n monitoring prometheus-operator-grafana-845fd8b6cb-b9bh2 3000
admin password is prom-operator (default from chart)
@JosephSalisbury as our Prometheus expert, can you also please have a look too?
There was a problem hiding this comment.
From POV all is good. Thx for the explanation
| @@ -87,6 +86,13 @@ controller: | |||
| http: 30010 | |||
| https: 30011 | |||
|
|
|||
| metrics: | |||
| enabled: false | |||
There was a problem hiding this comment.
@rossf7 @pipo02mix I defined this flag here and not in configmap starting above on line 24 (comments on 22); was that correct? IIUC that configmap above is for tuning nginx itself with nginx specific config options.
There was a problem hiding this comment.
Yes this is correct. The configmap is for the ingress controller itself. It's templated based on the chart values.
There was a problem hiding this comment.
if this is false, why we need to specify ports anyway?
There was a problem hiding this comment.
@tomahawk28 to have defaults ready so it's easy to enable metrics service with simple custom values.yaml e.g. during app installation
controller:
metrics:
enabled: true
There was a problem hiding this comment.
@rossf7 what confused me was this comment for configmap
https://github.com/giantswarm/nginx-ingress-controller-app/blob/master/helm/nginx-ingress-controller-app/values.yaml#L22-L23
stating
configmap contains settings that can be overridden with a custom values configmap
which is true to some extent but it's not entirely clear to which configmap it refers to (our nginx-ingress-controller-user-values on CP or upstream documented ingress-nginx ConfigMap); any property in values.yaml like now metrics service enabling can be overriden with nginx-ingress-controller-user-values ConfigMap, a custom values configmap.
I believe that comment would be more accurate that that configmap is for configuring upstream documented nginx configmap directives (so ingress-nginx ConfigMap) https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/configmap.md#configuration-options
Another source of confusion are hpa settings placed under configmap there https://github.com/giantswarm/nginx-ingress-controller-app/blob/master/helm/nginx-ingress-controller-app/values.yaml#L48-L53 which are not controller's configmap directives. Those HPA settings should be placed outside of configmap property. In upstream chart they have HPA controlled via .Values.controller.autoscaling.* flags
Will remember to fix this in future iterations.
There was a problem hiding this comment.
@sslavic Sorry yes that comment is confusing and it has a very specific meaning.
For chartconfig CRs we were worried about users changing things. So we took a whitelist approach and only allowed values under .Values.configmap to be changed.
The approach was too restrictive and we removed it for app CRs. So any value can be overridden. When we migrated this chart to an app CR we kept the values structure and this comment.
I think the comment should be updated so its meaning is clear. Also over time we should improve the values structure since this restriction no longer applies.
|
Upstream chart already has this metrics service, so I used it for inspiration and tried to be aligned with it where it made sense, but it's not complete 1:1 copy of upstream solution https://github.com/helm/charts/tree/master/stable/nginx-ingress Upstream chart seems very powerful, has many configurable knobs, but that makes it also very hard to use. |
sslavic
force-pushed
the
metrics-service
branch
from
d066444
to
45ab2af
Compare
|
@JosephSalisbury let me know if you make some findings. For now I'll go with metrics service as defined in the PR. |
Towards https://github.com/giantswarm/giantswarm/issues/8357