Skip to content
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

[grafana] feat: adds multi-org support to sidecar dashboards #3240

Closed
wants to merge 6 commits into from

Conversation

VeseliD
Copy link

@VeseliD VeseliD commented Jul 24, 2024

Converts sidecar.dashboards.provider to a list at
sidecar.dashboards.providers allowing the definition of multiple
providers that can be mappped to orgs as required.

Still supports the singular variant to retain backwards compatibility
and will prepend it to any configured providers

Results in multi-org support defining providers like the example below which

sidecar:
    folderAnnotation: grafana_folder
    dashboards:
      enabled: true
      providers:
        - name: prod
          orgid: 1
        - name: nonprod
          orgid: 2
        - name: ops
          orgid: 3

This then allows dashboard configmaps to be define the annotation grafana_folder: /tmp/dashboards/ops instructing the sidecar to put the dashboard in the folder that's been specific in our configuration above.

apiVersion: v1
kind: ConfigMap
metadata:
  name: my-dashboard
  namespace: monitoring
  labels:
    grafana_dashboard: '1'
  annotations:
    grafana_folder: /tmp/dashboards/ops/
data:
  my-dashboard.json: |-
   {}

Please, have in mind that you can define your own folderAnnotation:
We used value grafana_folder just as example.

We reference to this PR: #2939

@CLAassistant
Copy link

CLAassistant commented Jul 24, 2024

CLA assistant check
All committers have signed the CLA.

@zanhsieh zanhsieh changed the title feat: adds multi-org support to sidecar dashboards [grafana] feat: adds multi-org support to sidecar dashboards Aug 3, 2024
@VeseliD
Copy link
Author

VeseliD commented Aug 14, 2024

Hello team, any update on this PR?
Thanks!

@zanhsieh
Copy link
Collaborator

@jkroepke @Xtigyro Can you review this PR please?

Copy link
Collaborator

@jkroepke jkroepke left a comment

Choose a reason for hiding this comment

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

I have that feeling, that the current implementation is too complex for normal end-users.

While the provisioning part looks good, using an annotation like grafana_folder and require with value like /tmp/dashboards/ops/ into it where ops stands for orgID 3, it feels not great.

What happens, if /tmp/dashboards/ops2/ is used? An error will be not reported. to end-users.

Too much glue is used here to archive something.

However, I will be not the part, who is blocking this.

@TheGeniesis
Copy link

Any chance to merge it?

I have scenario when I want to put different types of metrics stored as configmaps in different directories.
e.g.

logs/
  log_dashboard_1
  log_dashboard_1
cluster_metrics/
  metric_dashboard_1
app_metrics/
  metric_dashboard_1
  metric_dashboard_2
...

Currently, it's not possible to do this separation, since I can have only one provider declared.

@jkroepke
Copy link
Collaborator

jkroepke commented Oct 11, 2024

Any chance to merge it?

No. At least a richer documentation which explains mechanic is needed. And I'm currently have the feeling that the current approach is well designed .

@TheGeniesis
Copy link

I'm currently have the feeling that the current approach is well designed .

Regarding to the charts/grafana/templates/_config.tpl it's possible to declare multiple providers inside provider.yaml part. This chart doesn't allow that (only one provider allowed). This means that chart limits available options for Grafana.

I understand that the design suggested by VeseliD is not ideal from your perspective.

Can I suggest to implement simpler solution just to allow to declare a list of dashboard providers instead of one? If I understand correctly the "dashboardProviders" section allows that, but was designed to create dashboards from files placed in volume. I would prefer avoid creating stretches to use dashboardProviders with configmaps.

@jkroepke
Copy link
Collaborator

jkroepke commented Oct 11, 2024

Like this, right?

{{- define "grafana.configDashboardProviderData" -}}
provider.yaml: |-
  apiVersion: 1
  providers:
    - name: '{{ .Values.sidecar.dashboards.provider.name }}'
      orgId: {{ .Values.sidecar.dashboards.provider.orgid }}
      {{- if not .Values.sidecar.dashboards.provider.foldersFromFilesStructure }}
      folder: '{{ .Values.sidecar.dashboards.provider.folder }}'
      folderUid: '{{ .Values.sidecar.dashboards.provider.folderUid }}'
      {{- end }}
      type: {{ .Values.sidecar.dashboards.provider.type }}
      disableDeletion: {{ .Values.sidecar.dashboards.provider.disableDelete }}
      allowUiUpdates: {{ .Values.sidecar.dashboards.provider.allowUiUpdates }}
      updateIntervalSeconds: {{ .Values.sidecar.dashboards.provider.updateIntervalSeconds | default 30 }}
      options:
        foldersFromFilesStructure: {{ .Values.sidecar.dashboards.provider.foldersFromFilesStructure }}
        path: {{ .Values.sidecar.dashboards.folder }}{{- with .Values.sidecar.dashboards.defaultFolderName }}/{{ . }}{{- end }}
+    {{- with .Values.sidecar.dashboards.additionalProviders }}
+    {{- tpl (toYaml .) $ | nindent 4 }}
+    {{- end -}}
{{- end -}}

@TheGeniesis
Copy link

I think it would be more consistent to go with .Values.sidecar.dashboards.provider pattern (not tested, but you should get the idea):

  providers:
    - name: '{{ .Values.sidecar.dashboards.provider.name }}'
      orgId: {{ .Values.sidecar.dashboards.provider.orgid }}
      {{- if not .Values.sidecar.dashboards.provider.foldersFromFilesStructure }}
      folder: '{{ .Values.sidecar.dashboards.provider.folder }}'
      folderUid: '{{ .Values.sidecar.dashboards.provider.folderUid }}'
      {{- end }}
      type: {{ .Values.sidecar.dashboards.provider.type }}
      disableDeletion: {{ .Values.sidecar.dashboards.provider.disableDelete }}
      allowUiUpdates: {{ .Values.sidecar.dashboards.provider.allowUiUpdates }}
      updateIntervalSeconds: {{ .Values.sidecar.dashboards.provider.updateIntervalSeconds | default 30 }}
      options:
        foldersFromFilesStructure: {{ .Values.sidecar.dashboards.provider.foldersFromFilesStructure }}
        path: {{ .Values.sidecar.dashboards.folder }}{{- with .Values.sidecar.dashboards.defaultFolderName }}/{{ . }}{{- end }}
{{- range $sdcProvider := .Values.sidecar.dashboards.additionalProviders }}
    - name: '{{ $sdcProvider.name }}'
      orgId: {{ $sdcProvider.orgid }}
      {{- if not $sdcProvider.foldersFromFilesStructure }}
      folder: '{{ $sdcProvider.folder }}'
      folderUid: '{{ $sdcProvider.folderUid }}'
      {{- end }}
      type: {{ $sdcProvider.type }}
      disableDeletion: {{ $sdcProvider.disableDelete }}
      allowUiUpdates: {{ $sdcProvider.allowUiUpdates }}
      updateIntervalSeconds: {{ $sdcProvider.updateIntervalSeconds | default 30 }}
      options:
        foldersFromFilesStructure: {{ $sdcProvider.foldersFromFilesStructure }}
        path: {{ .Values.sidecar.dashboards.folder }}{{- with .Values.sidecar.dashboards.defaultFolderName }}/{{ . }}{{- end }}
    {{- end -}}

Ofc, if you don't like it then your proposition will work too :)

@jkroepke
Copy link
Collaborator

Thanks fine, but please also enrich one of the existing ci/*values.yaml files to cover that.

@TheGeniesis
Copy link

@jkroepke I spent some time on this today and I have two conclusions:

  1. There was a flaw in my thinking - provider part allows to set specific directory (ok), but part which should help me to say "hey Grafana, put dashboard with X label to Y directory" is not there. I can only say "put all dashboards in directory Y". The "label" parameter is a part of "dashboard" section, which is the problem here. I found it's later used as env variable LABEL. I don't see an easy way to translate it to be a part of provider section to make it work as I expect (I don't know Grafana code).

  2. At the current moment if I update the code to allow multiple providers it will be the same as this MR (with a change to name additionalProviders param instead of providers), and then I will need to repeat steps as MR describes to mount configmaps as files in volume (use folderAnnotation and have dashboards in different directories instead of label).

You said earlier that the current approach is well designed. I think I'm missing something - from my point of view it's not possible to achieve my goal in a simple way. Did I miss something in the documentation?

@jkroepke
Copy link
Collaborator

jkroepke commented Oct 14, 2024

@TheGeniesis know you may understand my point that this is farly complex to reach the expected results and I'm not gonna approve/merge this PR.

For point 1:

You can solve that by enabled foldersFromFilesStructure, then grafana should take of the folder hierarchy and should replicate the dashboard to grafana folders. We are using that kind of feature with combination of grafana_folder annotation at ConfigMaps.


For point 2: Yeah, each provider needs a distinct volumeMount, which reference one shared volume.


I'm still happy to assist and figure out what a good solution here. But I guess we are know on a point, where should move the discussion to a distinct issue/new PR, since I would like to close this PR.

I'm happy to assistance in a form of contribution at the k8s-sidecar project what ever is needed to archive that.

The cleanest approach would be one dedicated sidecar for each provider.

@jkroepke
Copy link
Collaborator

Closed in favor of #3359 (feature needs a redesign)

@jkroepke jkroepke closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants