[grafana] Introduce toggle for volumeName lookup

[jkroepke]

Fixes #3125

#3041 was introduced without a real problem to mention.

However, with the lookup function, helm-diff shows always an diff, since the lookup function does not work together with helm template.

Since the change is merged, it's very to cut out. In conclusion, I add an toggle to turn off that feature. The toggle is enabled by default to avoid any breaking changes. Power users can decided to turn off that feature if they wish.

[kstevensonnv]

@jkroepke I'm trying this out but still seeing the change when running a diff, any ideas?

❯ helm list
NAME    NAMESPACE       REVISION        UPDATED                                 STATUS          CHART           APP VERSION
grafana dashboard       78              2024-06-11 12:41:43.064017 +0100 BST    deployed        grafana-7.3.12  10.4.3

persistence:
  enabled: true
  size: 1Gi
  storageClassName: gp2-sc
  lookupVolumeName: false

❯ git clone git@github.com:jkroepke/grafana-helm-charts.git

❯ git checkout lookup

❯ cd grafana-helm-charts/charts/grafana

❯ helm package .
Successfully packaged chart and saved it to: /Users/./Desktop/grafana-helm-charts/charts/grafana/grafana-8.0.2.tgz

❯ helm push grafana-8.0.2.tgz oci://1.dkr.ecr.eu-west-1.amazonaws.com/
Pushed: 1.dkr.ecr.eu-west-1.amazonaws.com/grafana:8.0.2
Digest: sha256:2859baa3121bd7d35c6e1a826b3542b9924d5fbc6b986151316ad9ca5d7f0d8e

❯ helm secrets diff upgrade grafana --namespace dashboard oci://1.dkr.ecr.eu-west-1.amazonaws.com/grafana --version 8.0.2 -f v
alues.yaml -f secrets.yaml -C 1

dashboard, grafana, PersistentVolumeClaim (v1) has changed:
...
    labels:
-     helm.sh/chart: grafana-7.3.12
+     helm.sh/chart: grafana-8.0.2
      app.kubernetes.io/name: grafana
...
        storage: "1Gi"
-   volumeName: pvc-33eb3487-c224-46d1-b08b-2b3f418320fb
    storageClassName: gp2-sc

❯ grep 'lookupVolumeName' templates/pvc.yaml 
  {{- if and (.Values.persistence.lookupVolumeName) (lookup "v1" "PersistentVolumeClaim" (include "grafana.namespace" .) (include "grafana.fullname" .)) }}

[jkroepke]

I'm trying this out but still seeing the change when running a diff, any ideas?

The branch needs to be apply once. Thats required to remove volumeName property from the helm state. (Background: by default, helm diff only do a diff against the helm state, not against real resources).

Once applied, the diff should normalized. With that knowledge. #3041 seems pointless. I also never ever saw that some major helm chart have that pattern.

It cloud be possible that Kubernetes may deny the upgrade here, since it's not allowed to modify volumeName property.

[kstevensonnv]

I have a temporary workaround for now and will try this when it's merged.

Thanks @jkroepke

[drew0ps]

Using ansible we also have problems with the change, therefore merging this PR would be very welcome.
As mentioned above, the change #3041 introduces diffs which breaks reconcile and also makes helm rollback (to revisions which do not have this field in the pvc templates) fail.

[jkroepke]

@zanhsieh @zalegrala @Xtigyro Could you please take a look here?

[ataut-pai]

I'm trying this out but still seeing the change when running a diff, any ideas?

The branch needs to be apply once. Thats required to remove volumeName property from the helm state. (Background: by default, helm diff only do a diff against the helm state, not against real resources).

Once applied, the diff should normalized. With that knowledge. #3041 seems pointless. I also never ever saw that some major helm chart have that pattern.

It cloud be possible that Kubernetes may deny the upgrade here, since it's not allowed to modify volumeName property.

@jkroepke about "The branch needs to be apply once. Thats required to remove volumeName property from the helm state." how this can be achieved?

unfortunately getting below and can't find a way of passing over it. I've seen recommendations to downgrade Grafana to 7.3.8, but we are using this Grafana chart as a dependency of kube-prometheus-stack which refers to grafana versions as 7.3.* as you can see here.

 Error: cannot patch "prometheus-operator-grafana" with kind PersistentVolumeClaim: PersistentVolumeClaim "prometheus-operator-grafana" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims
   core.PersistentVolumeClaimSpec{
   	AccessModes:      {"ReadWriteOnce"},
   	Selector:         nil,
   	Resources:        {Requests: {s"storage": {i: {...}, s: "10Gi", Format: "BinarySI"}}},
 - 	VolumeName:       "pvc-fcc8be5f-5d15-4fcf-ba5f-23746cf0c28e",
 + 	VolumeName:       "",
   	StorageClassName: &"kops-ssd-1-17",
   	VolumeMode:       &"Filesystem",
   	... // 3 identical fields
   }

Any thoughts?

[richardwhiuk]

Given we don't know what this was fixing, and it causes a bunch of issues, I'd be inclined to have this disabled by default, but thanks for the fix.

[jkroepke]

It is disabled by default. This PR add an toggle which does not change the behaivor by default.