Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) #2

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

phantasm66
Copy link

@phantasm66 phantasm66 commented Aug 10, 2022

There are some much needed features in the latest Velero chart 2.31.0 (we are currently running 2.17.2). The main one being the added ability to allow Velero to update existing resources in place, instead of saying they already exist and erroring. This is essential for our Zeebe PV restore DR strategy. The velero backed up PV and PVCs cannot be restored with older version of Velero unless the PVs and PVCs are deleted first.

In addition to the new existingResourcePolicy: update feature, I'm also adding the core kubernetes CRDs that are required for the Velero VolumeSnapshotClass CRD (needed for allowing Velero to backup CSI managed PV (EBS) volumes).

Broken down by commits:

  1. sync this forked repo with upstream velero helm chart's 2.31.0 release
  2. add velero's VolumeSnapshotClass resource for CSI EBS driver
  3. add the missing core Kubernetes CRDs that velero's VolumeSnapshotClass resource requires
  4. Include the original changes for the fork that were done here: Fork velero chart and make modifications to work with GitOps #1

I added commits 2 and 3 to this forked top level helm chart (instead of putting them in our sub chart in kubernetes-clusters) because the CRDs and VolumeSnapshotClass resource are 100% required for our kubernetes cluster setups (they all run Velero).

Tested in staging1 (live now)

All existing backup schedules, backups, PVs etc.. were all retained and the new Velero appVersion is correct 1.9.0 and the needed CRDs and volume snapshot class stuff was also all created. Tested backup and restore w/ Jon G.. works fine.

I'll have a PR up for the staging1 (and also production1) sub chart changes to use this new chart version shortly. I also already built and pushed this new parent (forked) chart version to AF (hence, I was able to test w/ our sub chart in staging1).

@phantasm66 phantasm66 requested a review from a team as a code owner August 10, 2022 21:10
@phantasm66 phantasm66 requested review from pinkertonpg, corymurphy and AlexMorreale and removed request for pinkertonpg August 10, 2022 21:10
@corymurphy
Copy link

What was the original purpose of forking this chart? Did those features get rolled into the official chart yet?

@phantasm66 phantasm66 changed the title Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) Aug 11, 2022
@phantasm66 phantasm66 marked this pull request as draft August 11, 2022 18:20
@phantasm66 phantasm66 changed the title Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) DRAFT: Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) Aug 11, 2022
@phantasm66 phantasm66 marked this pull request as ready for review August 17, 2022 20:14
@phantasm66 phantasm66 changed the title DRAFT: Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) Aug 17, 2022
@phantasm66
Copy link
Author

What was the original purpose of forking this chart? Did those features get rolled into the official chart yet?

There are a lot of references in newer PRs to those two PRs (now closed) that are in this fork's ezREADME, however for us (and anyone using newer ArgoCD, which uses Helm v3), the fix implemented in vmware-tanzu#171 does not address the underlying issue. See vmware-tanzu#171 (review) for details on why.

It's essentially a Helm lifecycle thing, by design.

@phantasm66 phantasm66 merged commit 29dcfa0 into main Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants