Feature request: Override ZFS params at PVC level

[jobo-prod]

Describe the problem/challenge you have
Localpv-zfs is very flexible in the sense that it reflects a lot of volume/dataset parameters in the StorageClass. For specific workloads (e.g. a Postgres database), one might desire some special recordsize/compression/etc options on the block level.

However, it is challenging for admins and confusing for users to have a separate storageclass for each possible set of parameters.

Describe the solution you'd like
I am not a ZFS expert but afaik, parameters may differ between datasets/zvols, even in the same pool.

Thus, it would be very handy to have the option to override those on a per-PVC level where necessary. This could be achieved with a set of annotations, such as

kind: PersistentVolumeClaim
metadata:
  name: my-zfs-pvc
  annotations:
    zfs.csi.openebs.io/recordsize: 16k
    zfs.csi.openebs.io/compression: zstd
spec:
  storageClassName: local-zfs

Anything else you would like to add:
An option to enable/disable this feature might be desirable for those who want to strictly enforce parameters set in the storageclass. Ideally, admins would decide which parameters are changeable and which ones aren't.

[Abhinandan-Purkait]

By K8s standards the storage class is supposed to be the point where properties of volume are to be defined. Having this seems to be redundant as same can be achieved via storageclass, now I do understand it might get tricky to handle lot of storageclasses but that's how it should be done ideally.

Wdyt @avishnu @tiagolobocastro?

Either way @jobo-prod we still would be happy to get more of your thoughts on this, I do like the idea of enable/disable, so let us know if you are willing to take this up for a contribution. Thanks!

[tiagolobocastro]

One downside is that the storage admin has less control since the user can now override storage parameters?

Though this could be something which is disabled by default for example and enabled on install or even the StorageClass?

[jobo-prod]

That sounds nice. So the next possible steps could be:

1. Identify useful parameters for overriding at PVC level (where supported by ZFS on the dataset/zvol level) – maybe all?
2. For each of those, add an additional storageclass parameter for enabling the override feature, such as recordsize-override-enable (default false)
3. Implement 🙃

Tbh, I have zero experience in reading/writing Golang code, so I'm probably gonna have a hard time with that contribution. Do you think that this might be a good first issue for a Golang noob? I'm assuming there are some different controllers involved here (storageclass, pvc, CSI provisioner/node) and some data being passed around in between, so it's probably not that trivial 😅

[Abhinandan-Purkait]

@jobo-prod I think rather than an override storageclass param the param should go as a global cli arg to the csi controller and then the controller on getting create requests will process the annotations if that cli arg is set, something like that.

[jobo-prod]

Hm but that would mean you could only globally define which parameters may be overridden by users. In that case you cannot have one storageclass where it is ok to override the compression parameter and another SC where the compression is enforced (for example)

[Abhinandan-Purkait]

Hm but that would mean you could only globally define which parameters may be overridden by users. In that case you cannot have one storageclass where it is ok to override the compression parameter and another SC where the compression is enforced (for example)

The problem with that might be we end up having too many sc parameters for each override and too many combinations, which might be fine but was wondering if there's a cleaner way, maybe a configmap which contains a list of overridable params, and then reference that configmap in the storageclass for that volume. That way the storageclass stays clean and most of the details move to configmap. Wdyt @tiagolobocastro ?

The main reason for avoiding too many parameters here is because each of that param need to be translated to a field in the ZFSVolume CR as that's how things are propagated, that would add quite a few fields on the CRs.

[tiagolobocastro]

The problem with that might be we end up having too many sc parameters for each override and too many combinations, which might be fine but was wondering if there's a cleaner way, maybe a configmap which contains a list of overridable params, and then reference that configmap in the storageclass for that volume. That way the storageclass stays clean and most of the details move to configmap. Wdyt @tiagolobocastro ?

We'd just be moving the params from a to b. I don't think that would tackle the main issue here.

The main reason for avoiding too many parameters here is because each of that param need to be translated to a field in the ZFSVolume CR as that's how things are propagated, that would add quite a few fields on the CRs.

Presumably we'd use the same parameter names, and something like this?

for name := range params {
   value := getPvcParams(name)
   if value == "" {
        value := getScParams(name)
   }
   addCrParam(param, value)
}

[jobo-prod]

Another thing that just came to my mind is a special syntax for declaring a parameter as default value (which may be overridden for a PVC), something like:

kind: StorageClass
metadata:
  name: my-storageclass
provisioner: zfs.csi.openebs.io
parameters:
  # the ? denotes default values that can be overridden
  compression: lz4?
  recordsize: 128k?
  # other values are fixed/enforced
  dedup: 'off'
  fstype: zfs
  poolname: pool-a
  shared: 'yes'
  thinprovision: 'yes'

This might ease implementation and would not introduce any new fields. However it introduces some specialized syntax and might reduce readability.

[tiagolobocastro]

Maybe another way may be in the name of the parameter?

kind: StorageClass
metadata:
  name: my-storageclass
provisioner: zfs.csi.openebs.io
parameters:
  # the ? denotes default values that can be overridden
  ?compression: lz4
  # or at the end?
  recordsize?: 128k
  # other values are fixed/enforced
  dedup: 'off'
  fstype: zfs

[jobo-prod]

That should also be possible but I thought the point was avoiding the introduction of additional parameters?

[tiagolobocastro]

The ? will have to be filtered out from either side, so probably doesn't change much, other than having the parsing on the key or value
Anyhow, from birds eye, either way seems fine