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

[ON HOLD]feature annotation for snapshotclass provisioner #102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

big-appled
Copy link

@big-appled big-appled commented Apr 13, 2022

Signed-off-by: Ning Ding [email protected]

Feature

Add a new annotation in snapshotclass to let user input the provisioner name, as some CSI providers have different provisioner name and driver name.

Example

In one cloud provider, the csi snapclass driver name is disk.csi.everest.io, but the storageclass provisioner name is everest-csi-provisioner.

user@machine:~$ kubectl get volumesnapshotclass csi-disk-snapclass -o yaml |grep driver
driver: disk.csi.everest.io

user@machine:~$ kubectl get sc csi-disk
NAME       PROVISIONER               RECLAIMPOLICY   VOLUMEBINDINGMODE   ALLOWVOLUMEEXPANSION   AGE
csi-disk   everest-csi-provisioner   Delete          Immediate           true                   2d6h

So with this PR feature, user can add annotation in snapclass to let velero find the correct snapclass.

velero.io/csi-volumesnapshot-class-provisioner=everest-csi-provisioner

@reasonerjt
Copy link
Contributor

reasonerjt commented Apr 13, 2022

In one cloud provider, the csi snapclass driver name is disk.csi.everest.io, but the storageclass provisioner name is everest-csi-provisioner.

Does it follow the CSI spec? If it's not following the spec, I don't think we should patch velero to support it.

@big-appled
Copy link
Author

In one cloud provider, the csi snapclass driver name is disk.csi.everest.io, but the storageclass provisioner name is everest-csi-provisioner.

Does it follow the CSI spec? If it's not following the spec, I don't think we should patch velero to support it.

K8s CSI spec does NOT require the two names (provisioner and driver) be the same. So I believe it is a valid case.

@big-appled big-appled force-pushed the feature-annotation-snapshotclass branch 2 times, most recently from 3839eba to c1d61ca Compare April 13, 2022 03:35
@big-appled big-appled force-pushed the feature-annotation-snapshotclass branch from c1d61ca to c1723de Compare April 13, 2022 03:50
@jerry-jibu
Copy link

In one cloud provider, the csi snapclass driver name is disk.csi.everest.io, but the storageclass provisioner name is everest-csi-provisioner.

Does it follow the CSI spec? If it's not following the spec, I don't think we should patch velero to support it.

CSI Spec doesn't define this part that snapshot driver name must be same to corresponding storage class provisioner name for same storage. I think it's a kind of common convention that most vendors follow it with above corner case from huawei cloud.

@reasonerjt
Copy link
Contributor

@jerry-jibu sorry for the late response, as we synced offline with jibu folks, this is a corner case due to impl of Huawei's CSI driver, I suggest we hold this PR until there are more of such use cases.

@reasonerjt reasonerjt changed the title feature annotation for snapshotclass provisioner [ON HOLD]feature annotation for snapshotclass provisioner Jun 10, 2022
Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

Blocking the merge of this PR, until we reach consensus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants