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

[RFC] Add support for using PVC name as the Share name and ability to skip the share prefix #96

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

Conversation

geoah
Copy link

@geoah geoah commented Dec 26, 2024

Hey all, wanted to get people's thoughts on a couple of changes I'd like to propose/ask-for.

The aim of these are to mainly to allow users to specify the name of the synology share name to be created or used.
I basically wanted to a) define the names of the shared, and b) use existing shares without having to rename them or use a separate CSI Samba plugin.

This PR/proposal is adding a couple of (off-by-default) options:

  1. skip_lun_prefix and skip_share_prefix options in the client-info config.

Setting this to true will skip the prefix being added to the share name.

skip_lun_prefix: false    # New parameter
skip_share_prefix: true   # New parameter
clients:
  - host: "10.0.0.10"
    username: "foo"
    password: "bar"
    https: false
    port: 5000
  1. use_pvc_name option in the StorageClass parameters.

Setting this to true will use the CSI provided parameter csi.storage.k8s.io/pvc/name as the share name.

Ideally I would have liked to be able to provide a name for the volume directly, but from a quick Google and lurk in the CSI interface issues, it doesn't seem to be a way to pass PVC parameters to CSI plugins by design.

apiVersion: storage.k8s.io/v1
kind: StorageClass
parameters:
  csi.storage.k8s.io/node-stage-secret-name: synology-csi-credentials
  csi.storage.k8s.io/node-stage-secret-namespace: system-synology-csi
  dsm: 10.0.0.10
  location: /volume1
  protocol: smb
  use_pvc_name: "true" # New parameter
provisioner: csi.san.synology.com
reclaimPolicy: Retain
volumeBindingMode: Immediate
allowVolumeExpansion: true

In combination, (1) and (2) allow you to use an existing share (as long as the share quota matches the requrested capacity).


I've only been using this plugin for a day or so, so I'm not sure if there is a reason why this shouldn't be supported or if there are additional things I might have missed in the code that require changing, as the lack of tests make it a bit hard to ensure I didn't break things.

I've deployed this in a local cluster and it seems to be working as expected with Samba shared both at creating new ones with the expected names, as well as reusing existing ones.


Now to the questions at hand:

a) Is this a feature you'd be interested in?
b) Would there be an alternative way to do this that you'd preffer to see for either (1) or (2)?
c) The code in this repo isn't really formatted, and in order to keep the PR more readable I've opted to not commit the fmt changes that go fmt is really insisting on.

Let me know I'd be happy to make any changes need to accomodate as I'd really not want to have to maintain a fork just for this. :)

Thank you for taking the time to build this plugin, it's really appreciated. 🙏
Happy winter holidays and such! 🎄

Kind regards, @geoah

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.

1 participant