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

[stable/grafana] Support Provisioner Configuration #3514

Closed
wants to merge 1 commit into from

Conversation

BugRoger
Copy link

@BugRoger BugRoger commented Feb 1, 2018

This PR extends the stable/grafana chart to allow for outside configuration of provisioners.

With Grafana 5 it will be possible to manage datasources and dashboards using provisioners. Adding a provisioner is done by adding one or more yaml configuration files in the provisioning/datasources and provisioning/dashboards directory. See also: http://docs.grafana.org/administration/provisioning/

For datasources the file can contain a list of datasources that will be added or updated during start-up. If the datasource already exists, Grafana will update it to match the configuration file.

For dashboards the files contain the configuration for provisioning from external resources. It currently only support reading dashboards from file but more provisioners are in the works.

This functionality partly clashes with the jobs provided by this Helm chart. In order to support both Grafana 4/5 in the same chart, I think, it is a sensible approach to allow for both provisioning mechanisms. In the absence of more sophisticated provisioners (that allow the decoupling from Helm and dashboards) it is anyway required to load the dashboard from the filesystem and inject it using the existing mechanism.

@k8s-ci-robot 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 1, 2018
@BugRoger
Copy link
Author

BugRoger commented Feb 1, 2018

/assign @linki

@BugRoger
Copy link
Author

BugRoger commented Feb 4, 2018

/assign @prydonius

@BugRoger
Copy link
Author

BugRoger commented Feb 4, 2018

/unassign @linki

@NicolasT
Copy link
Contributor

Looking forward to this to land. Too bad though that creating a default datasource for e.g. Prometheus deployed by the stable Chart (in a Chart having both Grafana and Prometheus as requirements) still won't be possible using this approach, because its service name/DNS entry depends on the Helm release name 😞

@monotek
Copy link
Collaborator

monotek commented Feb 19, 2018

@BugRoger
Looking forward to see this merged. Would you mind to fix the the merge conflict?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @prydonius 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:

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

@BugRoger
Copy link
Author

Fixed. It was just some other change increasing the version

@BugRoger
Copy link
Author

/assign @prydonius

@pesimon
Copy link

pesimon commented Mar 1, 2018

Can confirm that it works like a charm and grafana 5.0.0 was just released
https://github.com/grafana/grafana/releases

@@ -83,6 +83,14 @@ spec:
resources:
{{ toYaml .Values.server.resources | indent 12 }}
volumeMounts:
{{- if .Values.provisioningDatasourcesFiles }}
- name: provisioning-volume
mountPath: {{ default "/var/lib/grafana/provisioning" .Values.server.provisioningLocalPath }}/datasources
Copy link
Member

Choose a reason for hiding this comment

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

Don't use the default function. Defaults come from values.yaml anyways which is better. Apply to all your changes.

Copy link
Member

Choose a reason for hiding this comment

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

I figured this is common practice in the chart. So, let's leave it for now. Would be cool to get that fixed in a separate PR.

@jdumars
Copy link

jdumars commented Mar 2, 2018

/ok-to-test

@k8s-ci-robot 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 Mar 2, 2018
@monotek
Copy link
Collaborator

monotek commented Mar 2, 2018

/retest

@jpds
Copy link
Collaborator

jpds commented Mar 5, 2018

This merge proposal doesn't actually change the default version to 5.0.0 (relevant issue: #3937).

@sstarcher
Copy link
Collaborator

This should bump the version to 5.0.1 and add the AppVersion to the charts.yaml

@@ -83,6 +83,14 @@ spec:
resources:
{{ toYaml .Values.server.resources | indent 12 }}
volumeMounts:
{{- if .Values.provisioningDatasourcesFiles }}
- name: provisioning-volume
mountPath: {{ default "/var/lib/grafana/provisioning" .Values.server.provisioningLocalPath }}/datasources
Copy link
Member

Choose a reason for hiding this comment

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

I figured this is common practice in the chart. So, let's leave it for now. Would be cool to get that fixed in a separate PR.

@sstarcher
Copy link
Collaborator

@BugRoger could you fix the linting issues from the circleci build

@richerve
Copy link
Contributor

@BugRoger are you still working on this, if not I can help

@richerve
Copy link
Contributor

richerve commented Mar 23, 2018

In my opinion is better to bump the minor version and remove at least the support to configure datasources using the Job, it is a hack that is difficult to maintain and not needed anymore. In general the dashboards Job will also not be needed but I think is handy to have the functionality to import remote dashboards.

About bumping the minor version (or major) of the Chart, Grafana is not a project that has Long Term Support releases so keeping a Chart that is compatible with version 4.6.x and another compatible with >= 5 is not a big issue I think, eventually the Chart versions compatible with Grafana <= 4 will not be updated anymore and this will happen sooner than later.

I want to use this Chart on the kube_prometheus "meta Chart" in prometheus-operator/prometheus-operator#1127, they are maintaining what is basically a copy of the stable Chart but adding grafana-watcher that essentially serves the same purpose as the Jobs here, both cases are hacks that with Grafana >= 5 are not required.

@BugRoger
Copy link
Author

Fixed the linter nitpicks. Bumped the version. Rebased on master.

@richerve
Copy link
Contributor

Any way to move this forward?

@monotek
Copy link
Collaborator

monotek commented Mar 28, 2018

@BugRoger

Please update to the newest Grafana 5.0.4 Docker image as the older images don't work with Kubernetes 1.7.14, 1.8.9, or 1.9.4. This would probably also fix the build.

See: grafana/grafana-docker#140

@BugRoger BugRoger force-pushed the master branch 4 times, most recently from 9f99687 to fc91d68 Compare March 28, 2018 18:49
@BugRoger
Copy link
Author

Rebased. Updated Image to 5.0.4. Added appVersion to make Linter happy. Tests are green.

@richerve
Copy link
Contributor

@BugRoger, sadly again conflicting files. I think this should be coordinated with someone that can merge, do the fix and immediately apply it. I'm waiting for this PR to be merged to work on the integration in https://github.com/coreos/kube-prometheus

@knopkodav
Copy link

The same issue on 1.9.6-gke.0

@matthope
Copy link
Contributor

matthope commented Apr 4, 2018

How does this compare to #4541 ?

@richerve
Copy link
Contributor

richerve commented Apr 4, 2018

@matthope In concept both are the same in approach are different, this PR include changes to support the new Grafana >= 5 config but also maintains backward compatibility. The PR on #4541 removes all backwards compatibility.

@BugRoger friendly ping about updating the branch.

@rtluckie
Copy link
Contributor

rtluckie commented Apr 5, 2018

please check out #4713 before merging.

@BugRoger
Copy link
Author

BugRoger commented Apr 9, 2018

It seems i can't fix this. There's a deadlock between the different checks.

The linter requires me to set the version to version in chart.yaml to 0.8.5. This causes a conflict against master. That fails the e2e tests.

I can either resolve the conflict or fix the version.

@sstarcher
Copy link
Collaborator

sstarcher commented Apr 9, 2018

master is already at 0.8.5. You need to rebase and bump it to 0.8.6 or 0.9.0

@BugRoger
Copy link
Author

BugRoger commented Apr 9, 2018

Ah, rebased and bumped the version again....

@unguiculus
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2018
@unguiculus
Copy link
Member

I'm inclined to close this in favor of #4713.

@sstarcher
Copy link
Collaborator

@unguiculus if #4713 is merged it would make this PR no longer needed since #4713 is such a large overhaul and simplification.

I have not tested #4713, but the changes themselves are heading in a good direction.

@unguiculus
Copy link
Member

Closing in favor of #4713. Thanks @BugRoger.

@unguiculus unguiculus closed this Apr 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.