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

Add support for Multiple VolumeSnapshotClasses #178

Merged
merged 5 commits into from
Jul 19, 2023

Conversation

anshulahuja98
Copy link
Collaborator

@anshulahuja98 anshulahuja98 commented May 29, 2023

internal/util/util.go Outdated Show resolved Hide resolved
Copy link

@Shashank1306s Shashank1306s left a comment

Choose a reason for hiding this comment

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

Added comments in internal/util/util.go

@anshulahuja98 anshulahuja98 marked this pull request as ready for review June 5, 2023 04:55
CSIDeleteSnapshotSecretNamespace = "velero.io/csi-deletesnapshotsecret-namespace"
CSIVSCDeletionPolicy = "velero.io/csi-vsc-deletion-policy"
VolumeSnapshotClassSelectorLabel = "velero.io/csi-volumesnapshot-class"
VolumeSnapshotClassDriverBackupAnnotationPrefix = "velero.io/csi-volumesnapshot-class"
Copy link
Contributor

@blackpiglet blackpiglet Jun 5, 2023

Choose a reason for hiding this comment

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

Looks like VolumeSnapshotClassDriverBackupAnnotationPrefix and is duplicated with VolumeSnapshotClassDriverPVCAnnotation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually all 3 variables including VolumeSnapshotClassSelectorLabel have the same string
I kept that intentionally same to make user experience easier
While on the other hand - all are separate 3 variables such that it is easy to change in code than reusing the variables.

Do you have any suggestion for keeping the values different?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that these are logically different parameters. I think it's fine that they have the same value, since they're used in different contexts -- i.e. a PVC annotation shouldn't need to be called "csi-volumesnapshot-class-pvc" since it's already on the PVC.

@anshulahuja98
Copy link
Collaborator Author

CC @shubham-pampattiwar for review

Copy link
Collaborator

@sseago sseago left a comment

Choose a reason for hiding this comment

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

Added some comments. We can't merge this until we get the design PR approved and merged, though.

internal/backup/pvc_action.go Outdated Show resolved Hide resolved
CSIDeleteSnapshotSecretNamespace = "velero.io/csi-deletesnapshotsecret-namespace"
CSIVSCDeletionPolicy = "velero.io/csi-vsc-deletion-policy"
VolumeSnapshotClassSelectorLabel = "velero.io/csi-volumesnapshot-class"
VolumeSnapshotClassDriverBackupAnnotationPrefix = "velero.io/csi-volumesnapshot-class"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that these are logically different parameters. I think it's fine that they have the same value, since they're used in different contexts -- i.e. a PVC annotation shouldn't need to be called "csi-volumesnapshot-class-pvc" since it's already on the PVC.

internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
Anshul Ahuja and others added 2 commits June 28, 2023 10:10
@anshulahuja98
Copy link
Collaborator Author

Added some comments. We can't merge this until we get the design PR approved and merged, though.

Resolved all comments and merge conflicts
@sseago Can you please request someone from the maintainers team to review this PR/ approve design PR.
At this point I have requested too many times in the community meetings without receiving traction

Signed-off-by: Anshul Ahuja <[email protected]>
@shubham-pampattiwar
Copy link
Collaborator

Lets get the design merged in first.

@anshulahuja98
Copy link
Collaborator Author

The design PR has been approved by 2 maintainers, would be useful if someone can merge it and also meanwhile review/ approve this current PR.

@anshulahuja98 anshulahuja98 reopened this Jul 3, 2023
@github-actions github-actions bot requested a review from sseago July 3, 2023 08:25
@anshulahuja98
Copy link
Collaborator Author

I am not sure how this got closed, must have done by mistake
@blackpiglet / @sseago / @shubham-pampattiwar can you please approve this.

@anshulahuja98
Copy link
Collaborator Author

Design PR is merged

@anshulahuja98
Copy link
Collaborator Author

@blackpiglet / @shubham-pampattiwar can you please do final signoff and get this merged.

@anshulahuja98
Copy link
Collaborator Author

@reasonerjt can we get this merged as well?

@blackpiglet blackpiglet merged commit 77ee0b4 into vmware-tanzu:main Jul 19, 2023
5 checks passed
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.

CSI plugin should work with multiple VolumeSnapshotClasses
5 participants