VolumeSnapshot path

[saif-88]

What happened:

When i create a VolumeSnapshot the path in the nfs server contain the full path of the PersistentVolume
PersistentVolume path : /mnt/nfs/pvc-xxxxxx
VolumeSnapshot path /mnt/nfs/mnt/nfs/snapcontent-xxxxxx

What you expected to happen:

VolumeSnapshot path /mnt/nfs/snapcontent-xxxxxx

How to reproduce it:

---
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotClass
metadata:
  name: snapshot-csi
  annotations:
    snapshot.storage.kubernetes.io/is-default-class: "true"
driver: nfs.csi.k8s.io
deletionPolicy: Delete
parameters:
  server: xxxxxx
  share: "/mnt/nfs"
---
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  name: snapshot-test
spec:
  source:
    persistentVolumeClaimName: pvc-test

Anything else we need to know?:

Environment:

• CSI Driver version: 4.3.0
• Kubernetes version (use kubectl version): 1.26.5 (v1.26.5+rke2r1)
• OS (e.g. from /etc/os-release): Debian 11.5
• Kernel (e.g. uname -a): 5.10.0-19-amd64
• Install tools:
• Others:

[andyzhangx]

@saif-88 is there sth. broken with this snapshot feature? btw, following parameters are not supported in snapshot class:

parameters:
  server: xxxxxx
  share: "/mnt/nfs"

[saif-88]

@andyzhangx Thank you for response
the path in the nfs server should be
/mnt/nfs/snapcontent-xxxxxx
and not
/mnt/nfs/mnt/nfs/snapcontent-xxxxxx

For the parameters part, i saw that in the code of the controller and CreateSnapshotRequest spec and the snapshots are placed in the directory set in the share parameter when i change it

[wozniakjan]

I suppose you have a StorageClass with share: "/mnt/nfs" as well, it looks like I messed up some part of the #430 and somehow it maybe concatenates StorageClass share with VolumeSnapshotClass share? I will take a look and test this more thoroughly.

btw, following parameters are not supported in snapshot class:

also thanks for this, perhaps it's accidentally poorly supported by my mistake

[saif-88]

@wozniakjan Thank you for response
Yes exact i have a StorageClass with share: "/mnt/nfs"

[wozniakjan]

/assign @wozniakjan

[wozniakjan]

I0605 09:10:34.899900       1 controllerserver.go:353] archiving /tmp/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609 -> /tmp/snapshot-d69caa26-c6fb-44fc-b356-5bda4fa4733b/mnt/nfs/snapshot-d69caa26-c6fb-44fc-b356-5bda4fa4733b/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609.tar.gz
I0605 09:10:34.904830       1 controllerserver.go:358] archived /tmp/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609 -> /tmp/snapshot-d69caa26-c6fb-44fc-b356-5bda4fa4733b/mnt/nfs/snapshot-d69caa26-c6fb-44fc-b356-5bda4fa4733b/pvc-07bee7cb-c9b4-4de4-929e-c580c1662609.tar.gz

Thanks @saif-88 for discovering this, it reproduces quite nicely and there is a bug in the paths for snapshots. It's enough to just add share: "/mnt/nfs" in the StorageClass, the share parameter for VolumeSnapshotClass remains ignored as desired.

I will see how quickly I can fix this.

[saif-88]

@wozniakjan why not to keep server and share parameters for VolumeSnapshotClass ? if we want to put snapshots in different server or directory like /mnt/snapshots by keeping those parameters, we can do it

[wozniakjan]

that's a valid point, I will need to refresh my understanding of this. But first I will try to fix the borked /mnt/nfs/mnt/nfs path.

[wozniakjan]

why not to keep server and share parameters for VolumeSnapshotClass?

good news @saif-88, it actually is going to work as you'd expect (and kind of does already, except for the duplicate path). If you specify the share parameter on StorageClass, the snapshots are going to respect that. You can also override the share parameter on VolumeSnapshotClass to separate where you store snapshots and where you store volumes.

#461 contains a quick fix for the bug you mentioned, it however comes with a caveat. I couldn't figure out any nice and clean way to handle both path types

/mnt/nfs/mnt/nfs/snapshot-id/src-pvc-id.tar.gz  # old & broken
/mnt/nfs/snapshot-id/src-pvc-id.tar.gz          # new & fixed

Given it's already been released, I would like to allow for some discussion there, perhaps @andyzhangx or someone else (or myself tomorrow) will have a good idea how to do this without breaking the old snapshots.

[saif-88]

@wozniakjan Thank you for the fix.

To handle both path types, it's not very clean but you can check the driver version to choose which one to use and if someone wants to upgrade, he has to migrate (mv /old/path/snapcontent-xxxx /new/path) snapshots on the nfs server, it should be enough, no?

[wozniakjan]

yes, that is a good idea, it would be ideal to automate this fully so users don't have to worry about it and run any semi-manual migrations.

But I think #461 should fix the issue and the automated migration can be implemented separately, wdyt @andyzhangx?

[saif-88]

for automated migration you can use annotations on VolumeSnapshotContent, for the old ones migrate and add annotation, for the new one annotation is added on creation so you don't re-run migration again no ?