feat(deploy): Provide Persistent Volume Claim

[mwangggg]

fixes: #5

[ebaron]

Hi @mwangggg, excellent work! I have a few small suggestions inline and some more general comments:

It would be nice to add some other PVC fields that the user may want to edit. Here are all the fields in the PVC spec the user might want to edit: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#persistentvolumeclaims

• We should default accessModes to just ReadWriteOnce. This is what we use in the operator.
• You can omit volumeMode, we don't want the user to be able to set this as a block device.
• selector would be good to add, but just keep the default as an empty object. (e.g. selector: {})
• storageClassName is important to add, but a bit tricky because Kubernetes distinguishes between the empty string and being unset. Let me think about this one. For now you can ignore this detail and just use the storageClassName (if any) as-is from values.yaml.
• Annotations would be good to provide as a config option as well. These could be done the same way as Ingress: ingress.yaml values.yaml

I remembered the operator sets some extra environment variables to configure H2Database in file mode when using a PVC instead of empty dir. We should set those environment variables in deployment.yaml as well if the PVC is enabled:
https://github.com/cryostatio/cryostat-operator/blob/2d386930dc96f0dcaf937987ec35874006c53b61/internal/controllers/common/resource_definitions/resource_definitions.go#L750-L770

[ebaron]

• storageClassName is important to add, but a bit tricky because Kubernetes distinguishes between the empty string and being unset. Let me think about this one. For now you can ignore this detail and just use the storageClassName (if any) as-is from values.yaml.

I found a good way to do this. We can leave the value commented-out in values.yaml, with just some metadata for the README generator:

  ## @param pvc.storageClassName [string,nullable] Storage class to use for the persistentVolumeClaim
  # storageClassName:

Then you can use the kindIs function to distinguish between an unset value and the empty string:

      {{- if kindIs "string" .Values.pvc.storageClassName }}
      storageClassName: {{ .Values.pvc.storageClassName | quote }}
      {{- end }}

Here's how it works with different values:
$ helm template --set pvc.enabled=true -s templates/persistent-volumelaim.yaml .

---
# Source: cryostat/templates/persistent-volumelaim.yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: release-name-persistent-volumeclaim
spec:
  resources:
    requests:
      storage: 500Mi

$ helm template --set pvc.enabled=true,pvc.storageClassName="" -s templates/persistent-volumelaim.yaml .

---
# Source: cryostat/templates/persistent-volumelaim.yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: release-name-persistent-volumeclaim
spec:
  resources:
    requests:
      storage: 500Mi
      storageClassName: ""

$ helm template --set pvc.enabled=true,pvc.storageClassName=foo -s templates/persistent-volumelaim.yaml .

---
# Source: cryostat/templates/persistent-volumelaim.yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: release-name-persistent-volumeclaim
spec:
  resources:
    requests:
      storage: 500Mi
      storageClassName: "foo"

[ebaron]

The testing seems to be failing with:
Error: UPGRADE FAILED: template: cryostat/templates/pvc.yaml:1:14: executing "cryostat/templates/pvc.yaml" at <.Values.pvc.enabled>: nil pointer evaluating interface {}.enabled

I think this is caused by helm/chart-testing#525, which keeps the old values.yaml without any pvc entries. I saw this workaround mentioned that is probably the simplest fix: https://stackoverflow.com/a/68807258. We should just need this specifically on pvc.enabled checks. Anything else referencing nested pvc values should be contained in those checks anyways.

[ebaron]

Hi @mwangggg, changes look great! Could I get you to regenerate the README and schema file?

We use this project: https://github.com/bitnami-labs/readme-generator-for-helm/

1. Clone that repo
2. cd /path/to/cryostat-helm/charts/cryostat
3. /path/to/readme-generator-for-helm/bin/index.js -v values.yaml -s values.schema.json -r README.md

[ebaron]

I had some trouble when testing this in OpenShift. I think I ran into trouble when executing the post-install steps:

NOTES:

1. Tell Cryostat how to serve external traffic:

export ROUTE_HOST=$(oc get route -n helm cryostat -o jsonpath="{.status.ingress[0].host}")
export GRAFANA_ROUTE_HOST=$(oc get route -n helm cryostat-grafana -o jsonpath="{.status.ingress[0].host}")
kubectl -n helm set env deploy --containers=cryostat cryostat CRYOSTAT_WEB_HOST=$ROUTE_HOST GRAFANA_DASHBOARD_URL=https://$GRAFANA_ROUTE_HOST

When running these steps, the configuration change triggers a redeployment. This can lead to these two bugs cryostatio/cryostat-operator#487, cryostatio/cryostat-operator#491. I think we should apply the same fix here that we did in the operator (cryostatio/cryostat-operator#499): let's set the deployment spec's replicas to 1 and strategy.type to Recreate.

[mwangggg]

I set the replicas and strategy.type specs to the deployment.yaml folder, but would it be preferable to add them values.yaml and set them there?

[ebaron]

I set the replicas and strategy.type specs to the deployment.yaml folder, but would it be preferable to add them values.yaml and set them there?

I think the way you did it is preferable. Allowing the user to change these fields could easily result in a non-working deployment.

[ebaron]

Seems to work well, thanks for the great work @mwangggg!