Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Migrate prometheus operator from coreos #6765

Merged
merged 2 commits into from
Oct 23, 2018

Conversation

gianrubio
Copy link
Collaborator

@gianrubio gianrubio commented Jul 23, 2018

What this PR does / why we need it:

This a FR from coreos/prometheus-operator project. Moving the chart to helm upstream will make easier to run e2e tests and to accept/merge PR.

@richerve @pierreozoux @vsliouniaev please review it

prometheus-operator/prometheus-operator#1154
#3490

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 23, 2018
@gianrubio gianrubio force-pushed the prometheus-operator branch 2 times, most recently from afaa2b1 to 952e6e5 Compare July 23, 2018 14:12
# memory: 100Mi
# requests:
# cpu: 100m
# memory: 50Mi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prometheus-operator/prometheus-operator@dcca972#diff-316e3be0fdb46f4d221f84162f7d03f8

I think the values in the sample should reflect the updated memory limit - defaults are forever!

@@ -0,0 +1,2412 @@
{{- if .Values.createCustomResource -}}
Copy link
Collaborator

@vsliouniaev vsliouniaev Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createCustomResource should be documented and added to values.yaml ?

@@ -0,0 +1,16 @@
{{- if .Values.rbacEnable }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These appear as rbac.* configurations in the readme and values.yaml

@vsliouniaev
Copy link
Collaborator

Made updates here with these and other suggested changes.


## Introduction

This chart bootstraps a [prometheus-operator](https://github.com/coreos/prometheus-operator) deployment on a [Kubernetes](http://kubernetes.io) cluster using the [Helm](https://helm.sh) package manager.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have already https://github.com/helm/charts/tree/master/stable/prometheus. Can we quickly specify the differences to the users.

@gianrubio
Copy link
Collaborator Author

Made updates here with these and other suggested changes.

@vsliouniaev I applied your changes here.

TODO list

  • Review Readme.md
  • Load grafana dashboards

@stealthybox
Copy link
Collaborator

@gianrubio, since we're planning to use stable/grafana, it looks like we could enable the ConfigMap sidecar and create the datasource and dashboards directly from the prometheus-operator.

It should be easy to template and we could even provide it as a subchart so that it's usable with stable/prometheus
This same mechanism would be usable by users for their own dashboard charts.

I would say we should try to get #7060 merged if we go that route.

Using the grafana values to embed dashboards seems problematic for size reasons.

@richerve
Copy link
Contributor

@stealthybox I think the creation of dashboards and datasources is better handled by the kube-prometheus Chart, that at some point it will pull this new upstream Charts as dependencies, since dashboards and datasources are not directly related to the prometheus-operator.

stable/grafana has a way to read dashboards from ConfigMaps so the kube-prometheus Chart can create those and then reference them on the Grafana config.

@stealthybox
Copy link
Collaborator

@richerve we're on the same page.

We can configure stable/grafana as a sub-chart to use the sidecar that loads dashboards from ConfigMaps.

The incubator/kube-prometheus chart could then use incubator/kube-prometheus-dashboards as a subchart to create ConfigMaps that are properly labeled such that the grafana sidecar loads them.

incubator/kube-prometheus-dashboards would then be a usable, even for folks who manage their own prometheus or grafana.

@mattfarina mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@ey-bot ey-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 5, 2018
endpoints:
- port: http-metrics
interval: 15s
tlsConfig:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we drop/comment out tlsConfig here? kube-controller-manager metrics are only available via http kubernetes/kubernetes#58982

@mrueg
Copy link
Collaborator

mrueg commented Sep 19, 2018

@gianrubio anything I can do to help out here? I would love to see this PR getting merged.

@greggdonovan
Copy link

@gianrubio anything I can do to help out here? I would love to see this PR getting merged.

The pull-charts-e2e job appears to be failing on a stray tab character:

I0905 12:25:30.551] incubator/prometheus-operator/values.yaml
I0905 12:25:30.557]   29:15     error    syntax error: found character '\t' that cannot start any token

Can we remove that and try the e2e job again? We'd also love to see this move forward, as well.

@gianrubio
Copy link
Collaborator Author

I’m being lazy to finish this PR. I’ll get some time during this weekend to make it mergeable. Sorry for any inconvenience

@helm-bot helm-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 23, 2018
@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2018
Changed:
- Update Prometheus to 2.4.3, alertmanager to 0.15.2
- Change some job labels - should refer to a label appearing on the service. Tried to keep this consistent with what was used in the coreos version
- Add confguration for using CoreDNS
- Add insecureSkipVerify to kubeApiServer
- Add `prometheus.additionalScrapeConfigs`, `prometheus.additionalAlertManagerConfigs`

- Add ability to deploy an operated prometheus without installing anything else
  - Switch `prometheus.serviceAccountName` to be based on `prometheus.fullname` to prevent clashes
- Fix `serviceMonitorSelector` typo
- Add `serviceMonitorNamespaceSelector` to values.yaml
- Fix grafana typo (helm#6765 (comment))

Add CoreDNS dashboard

- Fixed dashboard use of updated name metric

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>
Signed-off-by: Vasily Sliouniaev <vsliouniaev@users.noreply.github.com>

Vas prometheus operator (#5)

* Fix lint

* Fix missing selector on kubelet

helm#6765 (comment)

* Fix braces in files

* Fix naming

* Move non-exporters out of exporters folder

* Use common value names, add to readme

* Change value names deploy -> enabled

helm#6765 (comment)
helm#6765 (comment)

* Update etcd rules

https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules.yml

* Fix etcd rules to match job name

* Review readme

* Change initial version to 0.1.0

Additional Fixes While Github is down (#6)

* Fix additional service monitor template

* Fix label indentation

helm#6765 (comment)

Fix cleanup, requirements.lock file (#7)
@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2018
@viglesiasce
Copy link
Contributor

Tested and worked great for me.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gianrubio, viglesiasce

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit 7d04502 into helm:master Oct 23, 2018
@vsliouniaev
Copy link
Collaborator

@trevex this was fixed
#6765 (comment)

@andig
Copy link
Contributor

andig commented Oct 23, 2018

The readme still points to incubator in various places like:

helm install --name my-release incubator/prometheus-operator

@andig
Copy link
Contributor

andig commented Oct 23, 2018

There also seems to be some confusion around the prometheus ingress config:

prometheus.ingress.enabled If true, **alertmanager** Ingress will be created

@gianrubio
Copy link
Collaborator Author

@andig prs with thiese fixes are very welcome 😋

@andig
Copy link
Contributor

andig commented Oct 23, 2018

Absolutely if these are as you expect them! Happy to have been watching this issue and looking forward to give it a ride tomorrow.

@andig
Copy link
Contributor

andig commented Oct 23, 2018

Another one: the readme gives an example for sendAnalytics, but its not in the values.yml?

Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
* Create promethues-operator chart

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>

* Fixes for stable/prometheus-operator

Changed:
- Update Prometheus to 2.4.3, alertmanager to 0.15.2
- Change some job labels - should refer to a label appearing on the service. Tried to keep this consistent with what was used in the coreos version
- Add confguration for using CoreDNS
- Add insecureSkipVerify to kubeApiServer
- Add `prometheus.additionalScrapeConfigs`, `prometheus.additionalAlertManagerConfigs`

- Add ability to deploy an operated prometheus without installing anything else
  - Switch `prometheus.serviceAccountName` to be based on `prometheus.fullname` to prevent clashes
- Fix `serviceMonitorSelector` typo
- Add `serviceMonitorNamespaceSelector` to values.yaml
- Fix grafana typo (helm#6765 (comment))

Add CoreDNS dashboard

- Fixed dashboard use of updated name metric

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>
Signed-off-by: Vasily Sliouniaev <vsliouniaev@users.noreply.github.com>

Vas prometheus operator (helm#5)

* Fix lint

* Fix missing selector on kubelet

helm#6765 (comment)

* Fix braces in files

* Fix naming

* Move non-exporters out of exporters folder

* Use common value names, add to readme

* Change value names deploy -> enabled

helm#6765 (comment)
helm#6765 (comment)

* Update etcd rules

https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules.yml

* Fix etcd rules to match job name

* Review readme

* Change initial version to 0.1.0

Additional Fixes While Github is down (helm#6)

* Fix additional service monitor template

* Fix label indentation

helm#6765 (comment)

Fix cleanup, requirements.lock file (helm#7)

Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
* Create promethues-operator chart

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>

* Fixes for stable/prometheus-operator

Changed:
- Update Prometheus to 2.4.3, alertmanager to 0.15.2
- Change some job labels - should refer to a label appearing on the service. Tried to keep this consistent with what was used in the coreos version
- Add confguration for using CoreDNS
- Add insecureSkipVerify to kubeApiServer
- Add `prometheus.additionalScrapeConfigs`, `prometheus.additionalAlertManagerConfigs`

- Add ability to deploy an operated prometheus without installing anything else
  - Switch `prometheus.serviceAccountName` to be based on `prometheus.fullname` to prevent clashes
- Fix `serviceMonitorSelector` typo
- Add `serviceMonitorNamespaceSelector` to values.yaml
- Fix grafana typo (helm#6765 (comment))

Add CoreDNS dashboard

- Fixed dashboard use of updated name metric

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>
Signed-off-by: Vasily Sliouniaev <vsliouniaev@users.noreply.github.com>

Vas prometheus operator (helm#5)

* Fix lint

* Fix missing selector on kubelet

helm#6765 (comment)

* Fix braces in files

* Fix naming

* Move non-exporters out of exporters folder

* Use common value names, add to readme

* Change value names deploy -> enabled

helm#6765 (comment)
helm#6765 (comment)

* Update etcd rules

https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules.yml

* Fix etcd rules to match job name

* Review readme

* Change initial version to 0.1.0

Additional Fixes While Github is down (helm#6)

* Fix additional service monitor template

* Fix label indentation

helm#6765 (comment)

Fix cleanup, requirements.lock file (helm#7)
scottrigby pushed a commit to prometheus-community/helm-charts that referenced this pull request Aug 8, 2020
* Create promethues-operator chart

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>

* Fixes for stable/prometheus-operator

Changed:
- Update Prometheus to 2.4.3, alertmanager to 0.15.2
- Change some job labels - should refer to a label appearing on the service. Tried to keep this consistent with what was used in the coreos version
- Add confguration for using CoreDNS
- Add insecureSkipVerify to kubeApiServer
- Add `prometheus.additionalScrapeConfigs`, `prometheus.additionalAlertManagerConfigs`

- Add ability to deploy an operated prometheus without installing anything else
  - Switch `prometheus.serviceAccountName` to be based on `prometheus.fullname` to prevent clashes
- Fix `serviceMonitorSelector` typo
- Add `serviceMonitorNamespaceSelector` to values.yaml
- Fix grafana typo (helm/charts#6765 (comment))

Add CoreDNS dashboard

- Fixed dashboard use of updated name metric

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>
Signed-off-by: Vasily Sliouniaev <vsliouniaev@users.noreply.github.com>

Vas prometheus operator (#5)

* Fix lint

* Fix missing selector on kubelet

helm/charts#6765 (comment)

* Fix braces in files

* Fix naming

* Move non-exporters out of exporters folder

* Use common value names, add to readme

* Change value names deploy -> enabled

helm/charts#6765 (comment)
helm/charts#6765 (comment)

* Update etcd rules

https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules.yml

* Fix etcd rules to match job name

* Review readme

* Change initial version to 0.1.0

Additional Fixes While Github is down (#6)

* Fix additional service monitor template

* Fix label indentation

helm/charts#6765 (comment)

Fix cleanup, requirements.lock file (#7)
endrec pushed a commit to Rungway/charts-we-use that referenced this pull request Aug 14, 2020
* Create promethues-operator chart

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>

* Fixes for stable/prometheus-operator

Changed:
- Update Prometheus to 2.4.3, alertmanager to 0.15.2
- Change some job labels - should refer to a label appearing on the service. Tried to keep this consistent with what was used in the coreos version
- Add confguration for using CoreDNS
- Add insecureSkipVerify to kubeApiServer
- Add `prometheus.additionalScrapeConfigs`, `prometheus.additionalAlertManagerConfigs`

- Add ability to deploy an operated prometheus without installing anything else
  - Switch `prometheus.serviceAccountName` to be based on `prometheus.fullname` to prevent clashes
- Fix `serviceMonitorSelector` typo
- Add `serviceMonitorNamespaceSelector` to values.yaml
- Fix grafana typo (helm/charts#6765 (comment))

Add CoreDNS dashboard

- Fixed dashboard use of updated name metric

Signed-off-by: Giancarlo Rubio <gianrubio@gmail.com>
Signed-off-by: Vasily Sliouniaev <vsliouniaev@users.noreply.github.com>

Vas prometheus operator (#5)

* Fix lint

* Fix missing selector on kubelet

helm/charts#6765 (comment)

* Fix braces in files

* Fix naming

* Move non-exporters out of exporters folder

* Use common value names, add to readme

* Change value names deploy -> enabled

helm/charts#6765 (comment)
helm/charts#6765 (comment)

* Update etcd rules

https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules.yml

* Fix etcd rules to match job name

* Review readme

* Change initial version to 0.1.0

Additional Fixes While Github is down (#6)

* Fix additional service monitor template

* Fix label indentation

helm/charts#6765 (comment)

Fix cleanup, requirements.lock file (#7)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet