-
Notifications
You must be signed in to change notification settings - Fork 2
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 metrics Service for prometheus-operator support #19
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{{- if .Values.controller.metrics.enabled }} | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
name: {{ .Values.controller.name }}-monitoring | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
app: {{ .Values.controller.name }} | ||
giantswarm.io/service-type: "managed" | ||
k8s-app: {{ .Values.controller.k8sAppLabel }} | ||
spec: | ||
clusterIP: None | ||
ports: | ||
- name: metrics | ||
port: {{ .Values.controller.metrics.service.servicePort }} | ||
targetPort: metrics | ||
selector: | ||
k8s-app: {{ .Values.controller.k8sAppLabel }} | ||
{{- end }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ kind: Service | |
metadata: | ||
annotations: | ||
prometheus.io/scheme: http | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WHy the annotations are in this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There's also Grafana with nginx dashboard, you can access Grafana locally after running
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From POV all is good. Thx for the explanation |
||
name: {{ .Values.controller.name }} | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,6 @@ configmap: | |
controller: | ||
name: nginx-ingress-controller | ||
k8sAppLabel: nginx-ingress-controller | ||
metricsPort: 10254 | ||
|
||
maxUnavailable: 1 | ||
|
||
|
@@ -87,6 +86,13 @@ controller: | |
http: 30010 | ||
https: 30011 | ||
|
||
metrics: | ||
enabled: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomahawk28 to have defaults ready so it's easy to enable metrics service with simple custom
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rossf7 what confused me was this comment for 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for heads up @rossf7! |
||
port: 10254 | ||
|
||
service: | ||
servicePort: 9913 | ||
|
||
resources: | ||
requests: | ||
cpu: 500m | ||
|
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.
<3