-
Notifications
You must be signed in to change notification settings - Fork 74
update ingress controller to 0.9.0-beta.2 and add prometheus scrape annotations to service #64
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
…nnotations to service
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.
Before we actually upgrade our productive ingress controllers I would suggest to test this with a single pod + nodeport. So in general I would hold back on this PR until we have confirmed that the stuff is actually working.
@@ -12,7 +12,7 @@ spec: | |||
spec: | |||
terminationGracePeriodSeconds: 60 | |||
containers: | |||
- image: gcr.io/google_containers/nginx-ingress-controller:0.8.3 | |||
- image: gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.1 |
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.
Why not 0.9.0-beta.2?
name: ingress-controller | ||
annotations: | ||
prometheus.io/scrape: "true" | ||
prometheus.io/port: "9101" |
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.
Why this port? Looking at the sources /metrics
should be on the same port as /healthz
which is 10254
.
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.
@MaikuMori: We are experimenting with a custom nginx.tpl
that contains instrumentation using https://github.com/knyar/nginx-lua-prometheus providing requests metrics.
This is not in a public repository yet so this is confusing. Apologies for that.
@auhlig Maybe we should switch the service annotation to the metrics port of the nginx-controller and add another annotation to the pod template for our custom metrics.
Beta.1 has serious bug where it fails to work with services which use port name instead of port as int. You also need to adopt other changes before merge such as |
@@ -41,7 +44,9 @@ spec: | |||
containerPort: 80 | |||
- name: https | |||
containerPort: 443 | |||
- name: metrics | |||
containerPort: 10254 |
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.
I think we are missing the port for the request metrics here:
@@ -14,6 +16,8 @@ spec: | |||
targetPort: http | |||
- name: https | |||
port: 443 | |||
targetPort: https | |||
targetPort: https | |||
- name: metrics |
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.
I think this is not needed here. prometheus scrapes the endpoints directly
Applied manually in staging. LGTM |
Does it make sense to try 0.9.0-beta.1 of the ingress controller as this version includes kubernetes/ingress-nginx#36,
@databus23?
Would test in staging after lunch if you agree.