charts: Fail when using existing claim for Artifactory

@jainishshah17

Hi,

I haven’t tried it, but i just had a quick look at it.

In the Helm install command you use --set postgresql.persistence.existingClaim=artifactory-database-pvc and have a PVC with the name artifactory-data-pvc. They dont match.

But. The helm chart doesnt try to find a PVC by name, but with a label app: artifactory. Thats hard coded into the chart.

That means, that no matter what i write in existingClaim, it will use that label selection.

I, as a user, have no control over which PVC it chooses if i have multible PVC’ with that one label.

And, as i stated in my initial post, if you choose to use existingClaim, it forgets to add resources and storageclass. And then the deployment breaks.

So, the name existingClaim is misleading and it doesnt work yet. And there are better ways to do it, as i have pointed out. Like using labels defined by the user, and not call it existingClaim.

getting same issue here. I think the common way to dealing with existing claim is like this

      volumes:
      {{- if and .Values.persistence.enabled .Values.persistence.existingClaim }}
      - name: {{ .Values.persistence.existingClaim }}
        persistentVolumeClaim:
          claimName: {{ .Values.persistence.existingClaim }}
      {{- end }}

{{- if and .Values.persistence.enabled (not .Values.persistence.existingClaim) }}
  volumeClaimTemplates:
    - metadata:
        name: artifactory-volume
  ...
{{- end }}  

Thanks for the reply @hoeghh.

What you get, is a PVC with a match on label “app: artifactory”. It doesnt even use the existingClaim the user specifies. It uses the name of the helm app.

      selector:
        matchLabels:
          app: {{ template "artifactory.name" . }}

This could be changed to

      selector:
        matchLabels:
          app: {{ .Values.artifactory.persistence.existingClaim }}

I did notice this as well, after I played with it and looked through the template. I did make a PVC with the label “app: artifactory”, and despite that it still didn’t use the existing one. I’m still curious if making a volumeClaimTemplate actually allows you to specify an old PVC using a selector or if it will allows create something. My workaround was similar to yours listed above, but I went at it from a slightly different angle. This is what I came up with:

In the volumes definition of spec, I added:

volumes:
      {{- if .Values.artifactory.persistence.existingClaim }}
      - name: artifactory-volume
        persistentVolumeClaim:
          claimName: {{ .Values.artifactory.persistence.existingClaim }}
      {{- end }}

And then my volumeClaimTemplates looked like this:

{{- if .Values.artifactory.persistence.existingClaim }} # Couldn't figure out how to do a !defined
{{- else }}  # just did an empty if and then put the important part in else instead
  {{- if .Values.artifactory.persistence.enabled }}
volumeClaimTemplates:
- metadata:
    name: artifactory-volume
  spec:
    {{- if .Values.artifactory.persistence.storageClass }}
    {{- if (eq "-" .Values.artifactory.persistence.storageClass) }}
    storageClassName: ""
    {{- else }}
    storageClassName: "{{ .Values.artifactory.persistence.storageClass }}"
    {{- end }}
    {{- end }}
    accessModes: [ "{{ .Values.artifactory.persistence.accessMode }}" ]
    resources:
      requests:
        storage: {{ .Values.artifactory.persistence.size }}
    {{- else }}
    - name: artifactory-volume
      emptyDir: {}
  {{- end }}
{{- end }}

So existingClaim gets passed in as the name of the claim like you said above, but it attaches it in the volume block instead of dealing with the volumeClaimTemplate. When existingClaim doesn’t exist, it will make a new volume with the other values.

@hoeghh I tried out your template change from your original comment, and while it no longer fails the deployment, it seems like the statefulset tries to make its own volume claim instead (ignoring my existing claim). Are you seeing that behavior?

Hi @jainishshah17 ,

sorry for the delay.

So, with other charts when they have an “existingClaim”, what is expected from the user, is to provide the name of an already existing volume claim. Like Jenkins here :

persistentVolumeClaim:
          claimName: {{ .Values.Persistence.ExistingClaim | default (include "jenkins.fullname" .) }}

But this claim will take the value of existingClaim and use it to match a label with a key “app” as seen here :

    {{- if .Values.artifactory.persistence.existingClaim }}
      selector:
        matchLabels:
          app: {{ template "artifactory.name" . }}

I think its fine that you as a user can define labels. But i would then wish it was done like here :

  {{- if .Values.Persistence.VolumeClaimTemplates.Selector.Enabled }}
      selector:
        matchLabels:
          {{- range $key,$value := .Values.Persistence.VolumeClaimTemplates.Selector.MatchLabels }}
          {{ $key }} : {{ $value }}
          {{- end }}
  {{- end }}

So the user can give both the key and value, and as many as they need like here :

     Selector:
       Enabled: true
       MatchLabels:
         app: artifactory
         system: production
         type: homefolder

But, for now, it would be great if the chart would work at all, when specifying an “existingClaim”.

The problem is that when you specify an “existingClaim”, it will not add accessModes or resources elements, and it will fail.

If you use the helm code in my first post, it will add these for both an existingclaim AND if you dont.

Hope it makes sense.