[stable/grafana] Utilize 5.x datasource and dashboard import tooling and general refactor #4713

rtluckie · 2018-04-04T22:39:23Z

utilize new features of grafana 5.x to configure datasources and dashboards
- removed all jobs
update ingress manifest to align with helm create
remove opinionated affinity config
consolidate configmaps
cleanup and refactor values.yaml
removed unnecessary nesting from values.yaml
add initContainer to download dashboards
update labels and selectors to align with helm best practices
general cleanup to align with helm best practices/patterns observed in helm create
update values, NOTES, README and _helpers accordingly
cleanup service manifest to allow more flexibility
add rtluckie to maintainers
bump chart version

NOTE: This change is not backwards compatible with grafana < 5.0.0

Tested on:
k8s 1.8.10, 1.9.6 and 1.10.0

Limitations:

Imported dashboards only support a single datasource

- rename manifest to align with helm create - remove unnecessary manifests

rtluckie · 2018-04-05T01:48:30Z

/assign @unguiculus

rtluckie · 2018-04-05T01:48:34Z

/assign @foxish

- utilize new features of grafana 5.x to configure datasources and dashboards - remove all jobs - update ingress manifest to align with helm create - consolidate configmaps - cleanup and refactor values.yaml - remove unnecessary nesting from values.yaml - add initContainer to download dashboards - update labels and selectors to align with helm best practices - general cleanup to align with helm best practices/patterns observed in `helm create` - update values, NOTES, README and _helpers accordingly - cleanup service manifest to allow more flexibility - add rtluckie to maintainers - bump chart version Tested on: k8s 1.8.10, 1.9.6 and 1.10.0 Limitations: - Imported dashboards only support a single datasource

DaytonG · 2018-04-05T14:49:41Z

stable/grafana/templates/deployment.yaml

-          image: busybox
-          command: ['sh', '-c', 'cp /tmp/config-volume-configmap/* /tmp/config-volume 2>/dev/null || true; cp /tmp/dashboard-volume-configmap/* /tmp/dashboard-volume 2>/dev/null || true']
+        - name: download-dashboards
+          image: appropriate/curl


Could this also be a setting in values.yaml? Use case there: using a private mirror for deployment

Another little thing: this init container is gonna launch even if we have 0 dashboards to download (perhaps you only provide them via config or retrieve them from grafana via ID). Would be nice if there was some simple way to disable this container or have it only launch when it's necessary.

@DaytonG Sure. I can make the download-dashboard image a setting in values.yaml

Re: the init container: It was my intention to have it only run when there were dashboards to download, but I haven't been able to figure out how. I will restructure .Values.dashboards to hopefully make it a bit easier.
Any help would be appreciated!

Hm. The simplest change could be to just move the downloaded dashboards to its own variable and process it independently of the dashboards provided locally. Could potentially increase configuration complexity a little, but it may also simplify some of the template code.

I thought about that, but I'd like to avoid the configuration complexity. I'll ruminate on it for a while. Even if the initContainer runs unnecessarily, the image is light (~7mb) and with nothing to download the script is benign.

DaytonG · 2018-04-05T14:54:58Z

stable/grafana/values.yaml

+#      disableDeletion: false
+#      editable: true
+#      options:
+#        path: /var/lib/grafana/dashboards


Perhaps it's just me, but I find that I always need to provision dashboards. It would be ideal if provisioning was enabled by default, so I just needed to fill in dashboards

@DaytonG I think its a general best practice to have minimal sane defaults in values.yaml

Yea, I can see that. My only concern is if someone tried to set dashboards but didn't know they also needed to configure providioners it may be difficult to figure out what to do. Perhaps a comment would do for dashboards saying "if you need this, you must uncomment the providers configuration"?

Added comment to values.yaml

DaytonG · 2018-04-05T14:55:53Z

stable/grafana/values.yaml

+    level: info
+  dashboards.json:
+    enabled: true
+    path: /var/lib/grafana/dashboards


In G5, dashboards.json is deprecated. Since this change is not backwards compatible with G4, could we just nuke the section, for clarity?

@DaytonG Good catch. Fixed

DaytonG · 2018-04-05T15:00:32Z

stable/grafana/templates/configmap.yaml

+  {{ $key }}: |
+{{ toYaml $value | indent 4 }}
+  {{- end -}}
+{{- end -}}


The datasources configuration can contain passwords and other sensitive information. Seems like it should go into a Secret instead of a ConfigMap.

Good call. grafana.ini should probably also be a secret. Will fix in follow-up PR.

DaytonG · 2018-04-05T15:02:27Z

stable/grafana/Chart.yaml

@@ -1,5 +1,5 @@
 name: grafana
-version: 0.8.5
+version: 0.9.0


Nitpicky: I would argue 1.0.0 is more appropriate here, since this is a backwards incompatible change

DaytonG · 2018-04-05T15:04:46Z

(Sorry, I think I may have posted review comments a little wrong. Not used to making them much here on github).

Overall, I think your PR is much better than mine. We should approve yours.

timstoop · 2018-04-05T15:58:12Z

@DaytonG in that case, I recommend you close the other one in favour of this one?

- remove deprecated grafana.ini option - rework .Values.dashboards to make triggering download initContainer easier - make initContainer image configurable - add note to values.yaml regarding dashboards' dependency on dashboardProviders - bump chart version to 1.0.0

timstoop · 2018-04-06T06:51:22Z

/ok-to-test

DaytonG · 2018-04-06T11:39:44Z

/lgtm

k8s-ci-robot · 2018-04-06T11:39:51Z

@DaytonG: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

/lgtm

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.

timstoop · 2018-04-06T11:42:39Z

stable/grafana/templates/NOTES.txt

-     export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ template "grafana.fullname" . }},component={{ .Values.server.name }}" -o jsonpath="{.items[0].metadata.name}")
+        You can watch the status of by running 'kubectl get svc --namespace {{ .Release.Namespace }} -w {{ template "grafana.fullname" . }}'
+     export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ template "grafana.fullname" . }} -o jsonpath='{.status.loadBalancer.ingress[0].ip}')
+     echo http://$SERVICE_IP:{{ .Values.httpPort -}}


Is the "echo" in this line needed?

@timstoop good catch! fixed.

- remove unnecessary echo - fix service.port references

timstoop · 2018-04-06T17:00:50Z

/lgtm

unguiculus · 2018-04-07T11:56:04Z

stable/grafana/Chart.yaml

@@ -9,4 +9,6 @@ sources:
 maintainers:
  - name: Ming Hsieh
    email: [email protected]
+  - name: Ryan Luckie


Use Github usernames

unguiculus · 2018-04-07T11:57:13Z

stable/grafana/templates/configmap.yaml

-  {{- if .Values.server.installPlugins }}
-  grafana-install-plugins: {{ .Values.server.installPlugins | quote }}
+{{- if .Values.plugins }}
+  plugins: {{ default "" .Values.plugins | quote }}


Don't use the default function. Default values should come from values.yaml. Apply everywhere.

unguiculus · 2018-04-07T12:00:33Z

stable/grafana/templates/service.yaml

+{{ toYaml . | indent 4 }}
+{{- end }}
+spec:
+{{- if (or (eq .Values.service.type "ClusterIP" "") (empty .Values.service.type)) }}


Why would this make sense? eq .Values.service.type "ClusterIP" ""

It really doesn't-- it was a relic from refactor. Fixed.

- use default when using templated value for pv name when existingClaim is not defined

unguiculus · 2018-04-10T17:22:12Z

/lgtm

k8s-ci-robot · 2018-04-10T17:22:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaytonG, rtluckie, timstoop, unguiculus

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:

~~OWNERS~~ [unguiculus]

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

amnk · 2018-04-10T22:48:21Z

@rtluckie I always get Error: YAML parse error on grafana/templates/dashboards-json-configmap.yaml: error converting YAML to JSON: yaml: line 14: did not find expected key when trying to use this, but I can't figure out what is wrong. My dashboards were working with previous Grafana chart - did we change something with this 1.0 release (except required json key)?

EDIT: I figured the workaround. Basically, $RAW_JSON should be one liner. Otherwise I always get the above output.

rtluckie · 2018-04-10T23:05:24Z

@amnk can you post your values.yaml and I will try to help.
Also, please review changes to values.yaml. Specifically, https://github.com/kubernetes/charts/blob/master/stable/grafana/values.yaml#L111

amnk · 2018-04-10T23:24:57Z

@rtluckie Thanks for reply :) yeah, I have that all done. And I've figured out the problem - all dashboards definitions should be one-liners in $RAW_JSON, otherwise Helm parser fails. Not sure if it is intended or not, but not vary handy :(

Jokero · 2018-04-11T09:16:05Z

@rtluckie Also faced with the same error.
values.yaml https://gist.github.com/Jokero/abd444598e819c961fc773ca7bccd69d

Jokero · 2018-04-11T09:17:18Z

If one-liner solution works, it's really not very convenient

DaytonG · 2018-04-11T16:53:45Z

Think I got a fix. In the json-configmap:

{{- if .Values.dashboards }}
  {{- range $key, $value := .Values.dashboards }}
    {{- if hasKey $value "json" }}
  {{ $key }}.json: |
{{ $value.json | indent 4 }}  <-- this line is changed
    {{- end }}
  {{- end }}
{{- end -}}

Basically, the json needed to be indented properly so the YAML could be parsed.

Will make a PR shortly.

amnk · 2018-04-11T17:42:12Z

@DaytonG thanks!

Jokero · 2018-04-14T15:35:38Z

Is there any working example of values.yaml? Grafana even does not load dashboards from grafana.net

monotek · 2018-04-14T15:57:21Z

https://github.com/kubernetes/charts/blob/master/stable/grafana/values.yaml

Jokero · 2018-04-14T15:59:03Z

@monotek It does not work. See my example https://gist.github.com/Jokero/abd444598e819c961fc773ca7bccd69d
I tested without custom json dashboard

monotek · 2018-04-14T16:00:22Z

I use it for a week like this.

Edit: Seems you're using Minikube.
Please make sure you updated your local helm repo via: "helm repo update"

Jokero · 2018-04-14T16:26:54Z

Ok. Figured out that I have to use exactly dashboardProviders.dashboardproviders.yaml but I changed it to be consistent to dashboardProviders.dashboardProviders.yaml (camelCase for both).

…and general refactor (helm#4713) * Refactor grafana - rename manifest to align with helm create - remove unnecessary manifests * Grafana refactor - utilize new features of grafana 5.x to configure datasources and dashboards - remove all jobs - update ingress manifest to align with helm create - consolidate configmaps - cleanup and refactor values.yaml - remove unnecessary nesting from values.yaml - add initContainer to download dashboards - update labels and selectors to align with helm best practices - general cleanup to align with helm best practices/patterns observed in `helm create` - update values, NOTES, README and _helpers accordingly - cleanup service manifest to allow more flexibility - add rtluckie to maintainers - bump chart version Tested on: k8s 1.8.10, 1.9.6 and 1.10.0 Limitations: - Imported dashboards only support a single datasource * Changes from comments - remove deprecated grafana.ini option - rework .Values.dashboards to make triggering download initContainer easier - make initContainer image configurable - add note to values.yaml regarding dashboards' dependency on dashboardProviders - bump chart version to 1.0.0 * NOTES fixes - remove unnecessary echo - fix service.port references * More changes from comments - update name to gh username in Charts.yaml - cleanup plugins install - cleanup dashboards install -- don't rely on `defaults` - cleanup reusing pv claim -- don't reply on `defaults` - cleanup secrets -- don't rely on `defaults` - cleanup service manifest -- don't rely `defaults` - use storageClassName instead of storageClass (don't rely on `defaults`) - remove redundant defaults from grafana.ini configmap * Fix small regression - use default when using templated value for pv name when existingClaim is not defined

…and general refactor (helm#4713) * Refactor grafana - rename manifest to align with helm create - remove unnecessary manifests * Grafana refactor - utilize new features of grafana 5.x to configure datasources and dashboards - remove all jobs - update ingress manifest to align with helm create - consolidate configmaps - cleanup and refactor values.yaml - remove unnecessary nesting from values.yaml - add initContainer to download dashboards - update labels and selectors to align with helm best practices - general cleanup to align with helm best practices/patterns observed in `helm create` - update values, NOTES, README and _helpers accordingly - cleanup service manifest to allow more flexibility - add rtluckie to maintainers - bump chart version Tested on: k8s 1.8.10, 1.9.6 and 1.10.0 Limitations: - Imported dashboards only support a single datasource * Changes from comments - remove deprecated grafana.ini option - rework .Values.dashboards to make triggering download initContainer easier - make initContainer image configurable - add note to values.yaml regarding dashboards' dependency on dashboardProviders - bump chart version to 1.0.0 * NOTES fixes - remove unnecessary echo - fix service.port references * More changes from comments - update name to gh username in Charts.yaml - cleanup plugins install - cleanup dashboards install -- don't rely on `defaults` - cleanup reusing pv claim -- don't reply on `defaults` - cleanup secrets -- don't rely on `defaults` - cleanup service manifest -- don't rely `defaults` - use storageClassName instead of storageClass (don't rely on `defaults`) - remove redundant defaults from grafana.ini configmap * Fix small regression - use default when using templated value for pv name when existingClaim is not defined Signed-off-by: voron <[email protected]>

Refactor grafana

5c79afa

- rename manifest to align with helm create - remove unnecessary manifests

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 4, 2018

k8s-ci-robot assigned unguiculus Apr 5, 2018

k8s-ci-robot assigned foxish Apr 5, 2018

This was referenced Apr 5, 2018

[stable/grafana] Support Provisioner Configuration #3514

Closed

use grafana 5 datasource and dashboard provisioning #4541

Closed

DaytonG reviewed Apr 5, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 6, 2018

timstoop reviewed Apr 6, 2018

View reviewed changes

NOTES fixes

374d306

- remove unnecessary echo - fix service.port references

k8s-ci-robot assigned timstoop Apr 6, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018

unguiculus suggested changes Apr 7, 2018

View reviewed changes

unguiculus removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2018

Fix small regression

b1a5b90

- use default when using templated value for pv name when existingClaim is not defined

rtluckie mentioned this pull request Apr 10, 2018

automated deploy of grafana chart takes a long time #4868

Closed

unguiculus approved these changes Apr 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2018

k8s-ci-robot merged commit 3672ec1 into helm:master Apr 10, 2018

rtluckie deleted the feature/grafana_rewrite branch April 10, 2018 18:09

DaytonG mentioned this pull request Apr 11, 2018

[stable/grafana] fix dashboards-json-configmap, add registry+pullSecrets #4925

Merged

unguiculus mentioned this pull request Apr 12, 2018

[stable/grafana] - Fix for config-map read-only mountpoint issue (#4708) #4828

Closed

richerve mentioned this pull request Apr 17, 2018

Helm: Add the ability to specify custom configuration files for Grafana prometheus-operator/prometheus-operator#1147

Closed

ebabani mentioned this pull request Apr 26, 2018

Grafana could not set secret values in a safe way #3739

Closed

mattfarina unassigned timstoop Jul 19, 2018

[stable/grafana] Utilize 5.x datasource and dashboard import tooling and general refactor #4713

[stable/grafana] Utilize 5.x datasource and dashboard import tooling and general refactor #4713

Conversation

rtluckie commented Apr 4, 2018 • edited Loading

rtluckie commented Apr 5, 2018

rtluckie commented Apr 5, 2018

DaytonG Apr 5, 2018 • edited Loading

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

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

DaytonG commented Apr 5, 2018

timstoop commented Apr 5, 2018

timstoop commented Apr 6, 2018

DaytonG commented Apr 6, 2018

k8s-ci-robot commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstoop commented Apr 6, 2018

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

unguiculus commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

amnk commented Apr 10, 2018 • edited Loading

rtluckie commented Apr 10, 2018 • edited Loading

amnk commented Apr 10, 2018

Jokero commented Apr 11, 2018

Jokero commented Apr 11, 2018

DaytonG commented Apr 11, 2018 • edited Loading

amnk commented Apr 11, 2018

Jokero commented Apr 14, 2018

monotek commented Apr 14, 2018

Jokero commented Apr 14, 2018 • edited Loading

monotek commented Apr 14, 2018 • edited Loading

Jokero commented Apr 14, 2018

rtluckie commented Apr 4, 2018 •

edited

Loading

DaytonG Apr 5, 2018 •

edited

Loading

amnk commented Apr 10, 2018 •

edited

Loading

rtluckie commented Apr 10, 2018 •

edited

Loading

DaytonG commented Apr 11, 2018 •

edited

Loading

Jokero commented Apr 14, 2018 •

edited

Loading

monotek commented Apr 14, 2018 •

edited

Loading