portainer / k8s

How to deploy Portainer inside a Kubernetes environment.
MIT License
87 stars 58 forks source link

PVC Template Uses Deprecated Volume Annotation For storageClass #133

Open pchang388 opened 1 year ago

pchang388 commented 1 year ago

The pvc template in the current helm chart uses the deprecated kubernetes volume annotation:

{{- if .Values.persistence.enabled -}}
{{- if not .Values.persistence.existingClaim -}}
---
kind: "PersistentVolumeClaim"
apiVersion: "v1"
metadata:
  name: {{ template "portainer.fullname" . }}
  namespace: {{ .Release.Namespace }}
  annotations:
  {{- if .Values.persistence.storageClass }}
    volume.beta.kubernetes.io/storage-class: {{ .Values.persistence.storageClass | quote }}
  {{- else }}
    volume.alpha.kubernetes.io/storage-class: "generic"
  {{- end }}
  {{- if .Values.persistence.annotations }}
  {{ toYaml .Values.persistence.annotations | indent 2 }}  
  {{ end }}
  labels:
    io.portainer.kubernetes.application.stack: portainer
    {{- include "portainer.labels" . | nindent 4 }}
spec:
  accessModes:
    - {{ default "ReadWriteOnce" .Values.persistence.accessMode | quote }}
  resources:
    requests:
      storage: {{ .Values.persistence.size | quote }}
  {{- if .Values.persistence.selector }}
  selector:
{{ toYaml .Values.persistence.selector | indent 4 }}
  {{ end }}
{{- end }}
{{- end }}

According to K8 documentation - https://kubernetes.io/docs/concepts/storage/persistent-volumes/

A PV can have a class, which is specified by setting the storageClassName attribute to the name of a [StorageClass](https://kubernetes.io/docs/concepts/storage/storage-classes/). A PV of a particular class can only be bound to PVCs requesting that class. A PV with no storageClassName has no class and can only be bound to PVCs that request no particular class.

In the past, the annotation volume.beta.kubernetes.io/storage-class was used instead of the storageClassName attribute. This annotation is still working; however, it will become fully deprecated in a future Kubernetes release

I noticed this when it caused an issue with some newer software, example - longhorn: https://github.com/longhorn/longhorn/issues/6264, that only looks for spec.storageClass instead of annotations.

We should push a change/enhancement to remove the deprecated annotation and instead use spec.storageClass

pchang388 commented 1 year ago

I noticed that once we removed this, you may face an error for immutable/unable to change pvc resource if you were using the annotation method previously. This is because those annotations like storageClasses are considered immutable and you cannot change or remove those once created.

You could do two things:

  1. Create a backup before using upgrading chart version ( do a helm rollback if it left you in an inconsistent state), remove the existing installation, and reinstall and import back up. This will ensure you are no longer using deprecated annotations that will go away and are using storageClasses moving froward.
  2. Could optionally modify helm chart to have a variable like, legacyStorageAnnotations, and if it is true (default false), then use annotation headers for PVC storage class instead. This is just kicking the can down the road so it is preferred to do option 1