gissilabs / charts

Apache License 2.0
38 stars 22 forks source link

allow custom volumes #15

Closed PrivatePuffin closed 3 years ago

PrivatePuffin commented 4 years ago

This commits adds the option to setup custom volumes without PVC This might be prefered in some situations where one wants a hostpath without pvc.

Update to docs is included.

Signed-off-by: Kjeld Schouten-Lebbing kjeld@schouten-lebbing.nl

sgissi commented 4 years ago

Thanks for the PR! Looking at the Kubernetes Volumes, you can create your own PVC pointing to your volume of choice (hostpath, etc.) and use the existingClaim setting that is already in the chart. Can you give an example where you need to specify new volumes directly in the Pod deployment?

PrivatePuffin commented 4 years ago

Thanks for the PR! Looking at the Kubernetes Volumes, you can create your own PVC pointing to your volume of choice (hostpath, etc.) and use the existingClaim setting that is already in the chart. Can you give an example where you need to specify new volumes directly in the Pod deployment?

It has been thoroughly discussed during development of TrueNAS SCALE: There are some issues with roleback/snapshots of PVC's, mostly because PVC settings are immutable and hostpads are not. So a PVC with hostpath is not directly the same as an hostpath...

sgissi commented 4 years ago

Thanks for the reply. First because I was living under a rock and didn't hear about TrueNAS SCALE! Also, I didn't know most (all?) PVC fields were immutable. Thinking more about the issue, I propose we default customVolume to "emptyDir:"{} and get rid of the extra logic.

Values.yaml would look like this:

# Volume settings if not using persistence
customVolume:
  config:
    emptyDir: {}
    # any volume type can be used here
    #hostPath:
    #  path: "/examplefolder/bitwardenrs"

Deployment.yaml would look like this:

      volumes:
      - name: {{ include "bitwardenrs.fullname" . }}
        {{- if .Values.persistence.enabled }}
        persistentVolumeClaim:
          claimName: {{ if .Values.persistence.existingClaim }}{{ .Values.persistence.existingClaim | quote }}{{- else }}{{ include "bitwardenrs.fullname" . }}{{- end }}
        {{- else }}
          {{- toYaml .Values.customVolume.config | nindent 8 }}
        {{- end }}
sgissi commented 4 years ago

Well, we can leave “config” off too :) Just customVolume as a map.

sgissi commented 3 years ago

I sent a proposed patch. I didn't combine with a default "emptyDir" because otherwise the user will need to manually set customVolume.emptyDir=null. Also, since the default customVolume is empty, we can check if both options are enabled and abort. My local tests went well, let me know if you agree.

Minor comment, I have reverted the version bump, I have more changes to include in the next release later today.

PrivatePuffin commented 3 years ago

squashed.

sgissi commented 3 years ago

@Ornias1993 Sorry I missed that one :( I'm finishing the updates to 1.18.0 and will include this PR. Thanks!