[velero] Add support for multiple backupstoragelocations and volumesnapshotlocations

[bsteinm2]

Special notes for your reviewer:

Fixes #254

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the values.yaml or README.md
• Title of the PR starts with chart name (e.g. [velero])

[bsteinm2]

I would also like to add a couple things:

1. These changes have been tested to the best of my knowledge
2. A side note, at this page under header "Velero is not publishing prometheus metrics", shouldn't the 'metrics' port be 'http-monitoring' ? The prometheus metrics port opened by the deployment template is called http-monitoring, not metrics. I would try to open a separate PR for this, but I don't see where I can do that. Thanks in advance!

[bsteinm2]

Hi, these lint tests are failing due to empty values in values.yaml. If it would helpful, I could confirm that this can lint correctly by temporarily pushing some populated values under .velero.configuration.backupstoragelocation and .velero.configuration.volumesnapshotlocation? I have seen these errors before while linting this chart.

[jenting]

Hi, these lint tests are failing due to empty values in values.yaml. If it would helpful, I could confirm that this can lint correctly by temporarily pushing some populated values under .velero.configuration.backupstoragelocation and .velero.configuration.volumesnapshotlocation? I have seen these errors before while linting this chart.

Thanks for your contribution @bsteinm2
The problem is that the user might configure the backupstoragelocation and volumesnapshotlocation manually after the helm install/upgrade. Furthermore, the volumesnapshotlocation is an optional field only required when we use restic to backup/restore pod volume.

[bsteinm2]

Thanks for taking a look @jenting

The problem is that the user might configure the backupstoragelocation and volumesnapshotlocation manually after the helm install/upgrade.

If I understand correctly, you're talking about the user manually editing the backupstoragelocation/volumesnapshotlocation k8s resource? Are you saying that my additions to the backupstoragelocation and volumesnapshotlocation templates would impede editing those resources?

Furthermore, the volumesnapshotlocation is an optional field only required when we use restic to backup/restore pod volume.

I am aware of this.

It sounds to me like I should undo my changes to the values file and some logic needs to be added to the backupstoragelocation and volumesnapshotlocation, but I'm not sure what. Do you have a proposed fix?

Also, I'm just going to use BSL and VSL to abbreviate backupstoragelocation and volumesnapshotlocation now, those are too long to type out every time :)

[jenting]

Not yet. The problem is how can we make a backward compatible. I have no idea for now.

[bsteinm2]

Hi @jenting, I believe these latest commits address the issue. I've tested with the old (current) values format and also with multiple BSL and VSL values configuration. Let me know what you think!

[jenting]

Fantastic @bsteinm2
I left some comments. It's a cool improvement.

[jenting]

@bsteinm2 Do you have time to address the comments?

[bsteinm2]

Hopefully your comments have all been adequately addressed. Apologies that it took a while!

[jenting]

I did a test, the BSL credential is mandatory but should be optional.

helm install velero \
  --namespace=velero \
  --create-namespace \
  --set-file credentials.secretContents.cloud=credentials-velero \
  --set configuration.provider=aws \
  --set configuration.backupStorageLocation[0].name=default \
  --set configuration.backupStorageLocation[0].bucket=velero \
  --set configuration.backupStorageLocation[0].config.region=minio-default \
  --set configuration.backupStorageLocation[0].config.s3ForcePathStyle=true \
  --set configuration.backupStorageLocation[0].config.s3Url=http://minio-default.velero.svc.cluster.local:9000 \
  --set configuration.backupStorageLocation[0].config.publicUrl=http://localhost:9000 \
  --set snapshotsEnabled=true \
  --set deployNodeAgent=true \
  --set configuration.volumeSnapshotLocation[0].name=default \
  --set configuration.volumeSnapshotLocation[0].config.region=minio-default \
  --set initContainers[0].name=velero-plugin-for-aws \
  --set initContainers[0].image=velero/velero-plugin-for-aws:v1.5.3 \
  --set initContainers[0].volumeMounts[0].mountPath=/target \
  --set initContainers[0].volumeMounts[0].name=plugins \
  --dry-run \
  ./charts/velero/

[jenting]

@bsteinm2 I have fixed all the issues I found.

[benedikt-bartscher]

Hey there, thanks for your work on the helm chart.
I just updated my helm deployment and realized that the velero deployment only gets created if .Values.configuration.provider exists, which got removed from the default values with this PR. I than readded configuration.provider to my values.yaml to fix this (redeploy the velero controller) but the helm charts checks do not allow this, the installs fails with this message from NOTES.txt:
REMOVED: .configuration.provider has been removed, instead each backupStorageLocation and volumeSnapshotLocation has a provider configured

[wilmardo]

Is there a reason a slice was chosen over a map? schedules already is a map based on names and allows for multiple schedules to be created. This now is a slice and deviates from that, not really intuitive from a user perspective.

Using maps is also one of the best-practices outlined in the Helm docs:

For this reason, it's often better to structure your values file using maps.

source: https://helm.sh/docs/chart_best_practices/values/#consider-how-users-will-use-your-values

Also lists are annoying since they are non mergeable. You need to redeclare the whole list to add something.
We for example had a velero umbrella chart providing some defaults which were getting merged with the user provided values, this now isn't possible with the backupStorageLocation anymore.

For example:

# defaults in the umbrella
      configuration:
          provider: aws
          backupStorageLocation:
            bucket: dummy
            config:
              region: eu-central-1

# user-provided
          backupStorageLocation:
            bucket: name

Keeps things nice and DRY :)

This now isn't possible and the user needs to redeclare the whole list.

An example approach with a map would have kept this possibility:

# defaults in the umbrella
      configuration:
          backupStorageLocation:
            default:
              bucket: dummy
              provider: aws              
              config:
                region: eu-central-1
 # user-provided
          backupStorageLocation:
            default:
               bucket: name
           customerLocationUserSpecific:
              bucket: somethingvalid
              provider: aws              
              config:
                region: us-west-1

Let me know if this is something that would be considered for a PR. To bad I see this PR so late, feels a little to late now :)