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

Merge csi plugin into velero repo #7408

Closed
reasonerjt opened this issue Feb 8, 2024 · 7 comments · Fixed by #7609
Closed

Merge csi plugin into velero repo #7408

reasonerjt opened this issue Feb 8, 2024 · 7 comments · Fixed by #7609
Assignees
Labels
Area/CSI Related to Container Storage Interface support Breaking change Impacts backwards compatibility has-e2e-tests kind/refactor kind/release-note
Milestone

Comments

@reasonerjt
Copy link
Contributor

With CSI-snapshot being more and more commonly used, we should consider merging the repo https://github.com/vmware-tanzu/velero-plugin-for-csi into https://github.com/vmware-tanzu/velero such that users will not need to install the csi plugin when installing velero.

This may save some release overhead and we may still turn on/off the feature by using the feature flag EnableCSI

@reasonerjt reasonerjt added Area/CSI Related to Container Storage Interface support kind/refactor kind/release-note 1.14-candidate labels Feb 8, 2024
@reasonerjt reasonerjt added the Breaking change Impacts backwards compatibility label Feb 9, 2024
@draghuram
Copy link
Contributor

It will be great to merge CSI plug-in. But I cm curious why EnableCSI flag is still needed. If users want to control snapshot type, it should be from Backup CR ideally.

@blackpiglet
Copy link
Contributor

After the CSI plugin merges into the Velero code base, then the feature option is unnecessary.
But the deprecation of existing feature flags may need a gap of two releases, so there is a period that the flag is still kept.

@reasonerjt
Copy link
Contributor Author

why EnableCSI flag is still needed...

  1. We don't wanna remove the flag right away for compatibility.
  2. There's use case of PV provisioned by public cloud provider that a user may wanna take snapshot via native snapshotter rather than CSI, this is currently is controlled by this flag

@sseago
Copy link
Collaborator

sseago commented Mar 5, 2024

@reasonerjt The second one is the longer-term sticking point. If we removed enableCSI we'd need to replace it with something like useCSI which would do the same thing.

@reasonerjt reasonerjt added this to the v1.14 milestone Mar 6, 2024
@reasonerjt
Copy link
Contributor Author

The second one is the longer-term sticking point. If we removed enableCSI we'd need to replace it with something like useCSI which would do the same thing.

I'm hoping at some point by default CSI will always have higher priority, unless the PV is provisioned by non-CSI driver, or explicitly annotated to use native volume snapshotter

@draghuram
Copy link
Contributor

why EnableCSI flag is still needed...

1. We don't wanna remove the flag right away for compatibility.

2. There's use case of PV provisioned by public cloud provider that a user may wanna take snapshot via native snapshotter rather than CSI, this is currently is controlled by this flag

"EnableCSI" flag controls non-CSI snapshotting? That doesn't sound right. Isn't it?

@blackpiglet
Copy link
Contributor

It sounds more like the future expected behavior. By then, there is already no EnableCSI feature flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/CSI Related to Container Storage Interface support Breaking change Impacts backwards compatibility has-e2e-tests kind/refactor kind/release-note
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants