[stable/concourse]: Add ServiceMonitor #11289

ghost · 2019-02-08T20:18:43Z

What this PR does / why we need it:

This commit:

Adds documentation for Prometheus values.
Adds Prometheus Operator ServiceMonitor

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

This commit: - Adds documentation for Prometheus values. - Adds Prometheus Operator ServiceMonitor Signed-off-by: Kamil Zabielski <xul.sitatirev@gmail.com>

k8s-ci-robot · 2019-02-08T20:18:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xulsitatirev
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: william-tran

If they are not already assigned, you can assign the PR to them by writing /assign @william-tran in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

stable/concourse/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-02-08T20:18:57Z

Hi @xulsitatirev. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cirocosta

Thanks for the PR!

Do you think these changes could come together with #10881?

thx!

cirocosta · 2019-02-08T22:01:05Z

stable/concourse/README.md

@@ -81,6 +81,8 @@ The following table lists the configurable parameters of the Concourse chart and
 | `web.ingress.enabled` | Enable Concourse Web Ingress | `false` |
 | `web.ingress.hosts` | Concourse Web Ingress Hostnames | `[]` |
 | `web.ingress.tls` | Concourse Web Ingress TLS configuration | `[]` |
+| `web.prometheus.enabled` | Enable Prometheus metrics endpoint | `false` |


Hey, the prometheus.enabled thing under web is actually under concourse.web.prometheus.enabled, right? From the set of variables that are more directly mapped to concourse web commands (which we end up not documenting in the README.md)

@cirocosta I have seen these variables are not documented.
I think we might improve my pull-request and document these variables.

Yeah, not having those indeed break the pattern that is established regarding documenting all the values under values.yaml in README.md.

I'd say we can have a separate PR for that though - the change would involve a bunch of additions to the README.md. Wdyt?

Definitely. I think this should be a separate pull-request.
I will be happy to help with that.

👍 I just opened #11300 to keep track of it

cirocosta · 2019-02-08T22:02:13Z

stable/concourse/Chart.yaml

@@ -1,5 +1,5 @@
 name: concourse
-version: 3.7.2
+version: 3.7.3


Given that this ends up adding new functionality, would it make more sense to have it as a minor bump instead of a patch?

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

@cirocosta Regarding semantic versioning, I fully agree.
I decided to make a patch, because I didn't break the contract at any point and improved documentation, but of course, I have added one feature.
I will bump the version

cirocosta · 2019-02-08T22:04:28Z

/ok-to-test

ghost · 2019-02-08T22:41:40Z

Thanks for the PR!

Do you think these changes could come together with #10881?

thx!

I fully agree we should make:

Service separate.
Web endpoint separate.
Metrics endpoint separate.

What I would think about is to make somehow a refactor of variables convention.
Their placement wasn't super clear to me.
I would remove these hard-coded annotations for Prometheus.
What do you think, @cirocosta ?

cirocosta · 2019-02-08T22:51:39Z

Regarding refactoring the variables and removing the hard-coded annotations, I think that's great!

At the same time, it'd be nice to try to be as less backward incompatible as possible, although I think being able to support more workflows (not assuming stable#prometheus-operator or stable#prometheus - by not having too many opinions on the default variables) would end up requiring those changes anyway.

It'd also be nice to see if we can follow a pattern that might be already established out there in other charts around this 🤔

Maybe it's a matter of giving a try going with the refactor and collecting some thoughts from the other folks too.

Wdyt?

thx!

cc @william-tran

ghost · 2019-02-09T09:53:12Z

@cirocosta

Speaking about different charts. They mostly call it ServiceMonitor.
I do not like this name, but I am fully open to call it like that.

charts/stable/nginx-ingress/values.yaml

Line 279 in 61662f1

serviceMonitor:

charts/stable/redis/templates/metrics-prometheus.yaml

Line 1 in 301ac01

    
           {{- if and (.Values.metrics.enabled) (.Values.metrics.serviceMonitor.enabled) }}

charts/stable/elasticsearch-exporter/values.yaml

Line 82 in 117a0c2

serviceMonitor:

Regarding hard-coded Prometheus annotations.
Here comes magic. Some helm charts hard-code it as well, but as a service Annotation. Some add variables. Some hard-code.

charts/stable/kiam/templates/server-service.yaml

Line 19 in ab0ed34

charts/stable/helm-exporter/templates/service.yaml

Line 11 in f4b4ea2

prometheus.io/scrape: "true"

charts/stable/kuberhealthy/templates/service.yaml

Line 10 in 29e34f0

prometheus.io/scrape: "true"

I am fully in refactoring and I will be happy to help.

Signed-off-by: Kamil Zabielski <xul.sitatirev@gmail.com>

cirocosta · 2019-02-09T16:45:11Z

Thanks for putting the time into it!

From what you saw in the charts, does the following plan look like something that makes sense?

Prometheus Service segregated from web-svc (web-prometheus-svc.yaml ?)
ServiceMonitor as an object on its own, conditionally enabled by prometheus.serviceMonitor.enabled
- web-servicemonitor.yaml conditionally created
- selector matching the Prometheus service (created by web-prometheus.yaml or something similar)
Prometheus Service annotations with a default in values.yaml set to the standard prometheus.io/scrape

With those, here are some sample configurations that I think we'd enable:

Manually enabling helm/charts#stable/prometheus to scrape the pods directly.

concourse:
  web:
    prometheus:
      enabled: true
      bindPort: 1337

web:
  annotations: # overriding default annotations ({})
    prometheus.io/scrape: "true"
    prometheus.io/port: "1337"

This would generate just the regular Web service and put those annotations under the web deployment, thus, making the pods scrapable.

Enabling a Prometheus service without creating ServiceMonitor

concourse:
  web:
    prometheus:
      enabled: true
      bindPort: 1337

prometheus:
  service:
    enabled: true
    annotations:  # overriding default annotations ({prometheus.io ...})
      something.else.io/scrape: "true"
      something.else.io/port: "1337"
    serviceMonitor:
      enabled: false

This would generate an extra service from web-prometheus.yaml, but not a ServiceMonitor (so that someone wanting to do that by themselves would be able to - and not necessarily rely on prometheus-operator too).

Enabling Prometheus service + automatic serviceMonitor creation

concourse:
  web:
    prometheus:
      enabled: true

prometheus:
  service:
    enabled: true
    serviceMonitor:
      enabled: true

This would generate both the extra prometheus service, as well as the ServiceMonitor object.

Wdyt?

ps.: the rationale for making prometheus.service.serviceMonitor chained in that way is that it seems like having a Service but not a ServiceMonitor object makes sense, but the opposite doesn't.

Thx!

ghost · 2019-02-09T22:12:42Z

I would simplify it.

(a) If someone wants to add prometheus.io annotations, he can handle it on his own with additional annotations for the service. Amen. Nothing more.

(b) If someone wants to add a ServiceMonitor, in my opinion, we should deliver a template with a boolean on values. Selector on ServiceMonitor is automatically created.

(c) I would create a service for metrics every time (in the end, boolean values parameter to handle it).

(d) I am fully in separating service (endpoints) for service, per se, and monitoring. Useful for NetworkPolicy.

What do you think?
If you agree you can create a ticket, I will help you with pleasure or review your code.

ghost · 2019-02-14T16:15:10Z

@cirocosta Do you want me to resolve the conflict?

cirocosta · 2019-02-15T01:49:38Z

Hi @xulsitatirev , yeah, please!

The plan you outlined sounds good to me. How do you feel about going for it in this very same PR?

Thx!

ghost · 2019-02-15T10:14:50Z

@cirocosta I will handle it.

stale · 2019-03-17T10:47:04Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale · 2019-03-31T11:28:33Z

This issue is being automatically closed due to inactivity.

[stable/concourse]: Add ServiceMonitor

07b8756

This commit: - Adds documentation for Prometheus values. - Adds Prometheus Operator ServiceMonitor Signed-off-by: Kamil Zabielski <xul.sitatirev@gmail.com>

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Feb 8, 2019

k8s-ci-robot requested review from cirocosta and william-tran February 8, 2019 20:18

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 8, 2019

cirocosta reviewed Feb 8, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2019

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Feb 9, 2019

[stable/concourse] Fix Charts versioning

77ea940

Signed-off-by: Kamil Zabielski <xul.sitatirev@gmail.com>

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 9, 2019

cirocosta mentioned this pull request Feb 23, 2019

[stable/concourse] Segregate Prometheus service from Web service #10881

Closed

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 17, 2019

stale bot closed this Mar 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/concourse]: Add ServiceMonitor #11289

[stable/concourse]: Add ServiceMonitor #11289

ghost commented Feb 8, 2019 •

edited by ghost

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

cirocosta left a comment

cirocosta Feb 8, 2019

ghost Feb 8, 2019

cirocosta Feb 9, 2019

ghost Feb 9, 2019

cirocosta Feb 9, 2019

cirocosta Feb 8, 2019

ghost Feb 8, 2019

ghost Feb 9, 2019

cirocosta commented Feb 8, 2019

ghost commented Feb 8, 2019

cirocosta commented Feb 8, 2019

ghost commented Feb 9, 2019 •

edited by ghost

cirocosta commented Feb 9, 2019 •

edited

ghost commented Feb 9, 2019

ghost commented Feb 14, 2019

cirocosta commented Feb 15, 2019

ghost commented Feb 15, 2019

stale bot commented Mar 17, 2019

stale bot commented Mar 31, 2019

[stable/concourse]: Add ServiceMonitor #11289

[stable/concourse]: Add ServiceMonitor #11289

Conversation

ghost commented Feb 8, 2019 • edited by ghost

What this PR does / why we need it:

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

cirocosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirocosta commented Feb 8, 2019

ghost commented Feb 8, 2019

cirocosta commented Feb 8, 2019

ghost commented Feb 9, 2019 • edited by ghost

cirocosta commented Feb 9, 2019 • edited

ghost commented Feb 9, 2019

ghost commented Feb 14, 2019

cirocosta commented Feb 15, 2019

ghost commented Feb 15, 2019

stale bot commented Mar 17, 2019

stale bot commented Mar 31, 2019

ghost commented Feb 8, 2019 •

edited by ghost

ghost commented Feb 9, 2019 •

edited by ghost

cirocosta commented Feb 9, 2019 •

edited