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

feat(csi): allow setting annotation on StorageClass to determine VolumeSnapshotClass #8646

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cfi2017
Copy link

@cfi2017 cfi2017 commented Jan 23, 2025

Fixes #8807

This PR adds a way to specify the desired VolumeSnapshotClass by annotation in the StorageClass.

An example use case here is Nutanix: The Nutanix CSI driver uses a single provisioner but allows for StorageClasses on either NFS (NutanixFiles) or Volumes (NutanixVolumes). These two StorageClasses require differently configured VolumeSnapshotClasses even though they use the same provisioner.

In Velero, currently, this is only supported by setting an annotation on every PVC you want to back up.

A more scalable approach is to allow setting a default for the StorageClass.

Please indicate you've done the following:

Signed-off-by: Carlo Field <[email protected]>
Signed-off-by: Carlo Field <[email protected]>
@cfi2017
Copy link
Author

cfi2017 commented Jan 23, 2025

For reference, this is the same PR I first opened in the old CSI plugin repo, before realising that repo is outdated.

vmware-tanzu/velero-plugin-for-csi#237

@cfi2017 cfi2017 force-pushed the feat/storageclass-volumesnapshotclass-annotation branch from 2a931b5 to e8c8283 Compare January 28, 2025 08:56
@reasonerjt reasonerjt added the Needs triage We need discussion to understand problem and decide the priority label Feb 5, 2025
Signed-off-by: Carlo Field <[email protected]>
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 70.83333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.39%. Comparing base (a9031eb) to head (218f588).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
pkg/util/csi/volume_snapshot.go 69.56% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8646   +/-   ##
=======================================
  Coverage   59.39%   59.39%           
=======================================
  Files         370      370           
  Lines       39952    40009   +57     
=======================================
+ Hits        23730    23765   +35     
- Misses      14731    14750   +19     
- Partials     1491     1494    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cfi2017
Copy link
Author

cfi2017 commented Feb 12, 2025

@kaovilai is code coverage for error logs something that's needed for approval?

@kaovilai
Copy link
Member

Not necessarily

@cfi2017
Copy link
Author

cfi2017 commented Feb 13, 2025

I'll wait for your feedback then :)

@cfi2017 cfi2017 requested a review from kaovilai February 24, 2025 07:47
@cfi2017
Copy link
Author

cfi2017 commented Mar 20, 2025

Is there anything I can do to help push this forward?

We can't use csi snapshots without this feature :)

@kaovilai
Copy link
Member

kaovilai commented Mar 20, 2025

Is there an existing Github issue on this? If not, create one then set Fixes #issuenumber in PR description.

Additionally this is a functionality change/add and not a bug fix, I think we are waiting for GA of velero 1.16 per plan and won't be merging non bug fixes code changes till after that.

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests indicates this PR does not break existing cases, this lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation has-changelog has-unit-tests Needs triage We need discussion to understand problem and decide the priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support per-storageclass VSC annotation
3 participants