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: Storage class support #61

Merged
merged 3 commits into from
Jul 11, 2020

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Jul 11, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
feat: Storage class support

We could provide storage class support even it's static provisioning, and then this driver would pass storage parameters to NodeStageVolume

refer to https://kubernetes-csi.github.io/docs/secrets-and-credentials-storage-class.html

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: smb
provisioner: smb.csi.k8s.io
parameters:
  source: "//smb-server.default.svc.cluster.local/share"
  csi.storage.k8s.io/node-stage-secret-name: "smbcreds"
  csi.storage.k8s.io/node-stage-secret-namespace: "default"
reclaimPolicy: Retain # can only support retain
volumeBindingMode: Immediate

Which issue(s) this PR fixes:

Fixes #60

Requirements:

Special notes for your reviewer:

Release note:

feat: Storage class support

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 11, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from jingxu97 and msau42 July 11, 2020 02:01
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2020
@coveralls
Copy link

coveralls commented Jul 11, 2020

Pull Request Test Coverage Report for Build 165261697

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 89.25%

Totals Coverage Status
Change from base Build 164317715: 0.09%
Covered Lines: 631
Relevant Lines: 707

💛 - Coveralls

@andyzhangx andyzhangx merged commit 733634b into kubernetes-csi:master Jul 11, 2020
@msau42
Copy link

msau42 commented Jul 11, 2020

Do you really need to provide storageclass support for this? Couldn't you put these fields directly in the PV object?

@andyzhangx
Copy link
Member Author

Do you really need to provide storageclass support for this? Couldn't you put these fields directly in the PV object?

@msau42
provide storage class support is another option, it's quite useful in helm charts, e.g.

helm install --set persistence.storageClass="smb" --set persistence.size=100Gi --generate-name bitnami/wordpress

@andyzhangx
Copy link
Member Author

@msau42 also, I think for nfs driver, it has same requirement, e.g. kubernetes-csi/csi-driver-nfs#30
I will try to provide "dynamic" provisioning for this smb driver, e.g. user brings own samba server, and this driver could provision a new folder under samba server in "dynamic" provisioning stage.

@msau42
Copy link

msau42 commented Jul 14, 2020

IIUC, the use case for implementing a no-op CreateVolume is to support Helm charts that only know how to create PVCs, instead of manually creating the PV?

@andyzhangx
Copy link
Member Author

IIUC, the use case for implementing a no-op CreateVolume is to support Helm charts that only know how to create PVCs, instead of manually creating the PV?

yes, by providing storage class support, we can use this driver in helm chart or other deployments

@msau42
Copy link

msau42 commented Jul 14, 2020

Ok, I would clarify that in the docs because that's not the usual expected behavior.

@andyzhangx
Copy link
Member Author

Ok, I would clarify that in the docs because that's not the usual expected behavior.

@msau42 thanks, will do.
Do you think I could port same functionality to nfs csi driver? kubernetes-csi/csi-driver-nfs#30 (comment)

@msau42
Copy link

msau42 commented Jul 14, 2020

I was thinking of actually having the CreateVolume mount the share to create the subdir, and then unmount it when done, but this would require the provisioner to run as privileged.

I'm not a big fan of not being able to support DeleteVolume, but if there are folks asking for this, we can consider it as a stop-gap solution.

andyzhangx pushed a commit to andyzhangx/csi-driver-smb that referenced this pull request May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide storage class support
4 participants