Repository: kubernetes-csi/csi-driver-nfs (go)

Issue:
NFS mount options ignored by CSI in Volume Snapshot request
Hi all, 
I have the following problem using the csi-driver-nfs driver in both v4.9.0 and v4.10.0 recently updated.

Both versions were installed with helm according to the instructions without specifying the externalSnapshotter option as it was giving an error:

   Error: INSTALLATION FAILED: Unable to continue with install: CustomResourceDefinition "volumesnapshots.snapshot.storage.k8s.io" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-name" must equal "csi-driver-nfs": current value is "rke2-snapshot-controller-crd"

I configured two different storage classes with the same driver, one that points to an nfs share exposed by a storage system without the need to specify mount options and a second instead towards a Qumulo storage system that requires the following mount options set (nolock, proto=tcp, vers=3).

I have no problem with both in their use and I correctly create PVCs on both and the PVs are correctly in boud and by accessing the share I find the content.

The real problem I have on the second Qumulo share with the mount options is in the use of the corresponding VolumeSnapshotClasses that allows me to backup the PVs via Rubrik, because it seems not to pass the mount options in particular the nolock.

Unfortunately on the Qumulo storage system it is not currently possible to disable it for general or single share configuration.

Here are some details:

  1) As as confirmed the csi-nfs-controller.log details:

   I0204 05:54:11.509884 1 utils.go:110] GRPC call: /csi.v1.Controller/CreateSnapshot
   I0204 05:54:11.510015 1 utils.go:111] GRPC request: {"name":"snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da","source_volume_id":"qumulo.opbg.dom#RancherPrimeTestSistemisti#pvc-2461ab54-6003-4a53-87fd-b83a6233f576##"}
   I0204 05:54:11.510110 1 controllerserver.go:496] internally mounting qumulo.opbg.dom:/RancherPrimeTestSistemisti at /tmp/snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da
   I0204 05:54:11.510191 1 nodeserver.go:132] NodePublishVolume: volumeID(qumulo.opbg.dom#RancherPrimeTestSistemisti#snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da#snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da#pvc-2461ab54-6003-4a53-87fd-b83a6233f576) source(qumulo.opbg.dom:/RancherPrimeTestSistemisti) targetPath(/tmp/snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da) mountflags([])
   I0204 05:54:11.510242 1 mount_linux.go:218] Mounting cmd (mount) with arguments (-t nfs qumulo.opbg.dom:/RancherPrimeTestSistemisti /tmp/snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da)
   E0204 05:54:17.885288 1 mount_linux.go:230] Mount failed: exit status 32
   Mounting command: mount
   Mounting arguments: -t nfs qumulo.opbg.dom:/RancherPrimeTestSistemisti /tmp/snapshot-4e132ac9-4201-4990-be4c-204b266f4bfb
   Output: mount.nfs: rpc.statd is not running but is required for remote locking.
   mount.nfs: Either use '-o nolock' to keep locks local, or start statd.
   mount.nfs: Protocol not supported

  2) Notably, line #4,#5 shows empty mount flags:

   I0204 05:54:11.510191 1 nodeserver.go:132] NodePublishVolume: volumeID(qumulo.opbg.dom#RancherPrimeTestSistemisti#snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da#snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da#pvc-2461ab54-6003-4a53-87fd-b83a6233f576) source(qumulo.opbg.dom:/RancherPrimeTestSistemisti) targetPath(/tmp/snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da) mountflags([])
   I0204 05:54:11.510242 1 mount_linux.go:218] Mounting cmd (mount) with arguments (-t nfs qumulo.opbg.dom:/RancherPrimeTestSistemisti /tmp/snapshot-70fc316a-e7e3-4cdd-9096-43188ba4b0da)

  3) The mount options are being ignored by the CSI. So seems like a defect with this CSI. 

I kindly ask for support.

Thanks for your collaboration, Gabriele.

Pull request:
feat: support mountOptions parameter in VolumeSnapshotClass
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind feature

**What this PR does / why we need it**:
feat: support mountOptions parameter in VolumeSnapshotClass

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #861

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
feat: support mountOptions parameter in VolumeSnapshotClass
```


Patch:
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
index abb1f705..3c5f8c4c 100644
--- a/docs/driver-parameters.md
+++ b/docs/driver-parameters.md
@@ -28,6 +28,14 @@ volumeAttributes.server | NFS Server address | domain name `nfs-server.default.s
 volumeAttributes.share | NFS share path | `/` |  Yes  |
 volumeAttributes.mountPermissions | mounted folder permissions. The default is `0`, if set as non-zero, driver will perform `chmod` after mount |  | No |
 
+### `VolumeSnapshotClass`
+
+Name | Meaning | Available Value | Mandatory | Default value
+--- | --- | --- | --- | ---
+mountOptions | mount options separated by comma, e.g. `"nfsvers=4.1,sec=sys"` |  | No | ""
+server | NFS Server address | domain name `nfs-server.default.svc.cluster.local` <br>or IP address `127.0.0.1` | Yes |
+share | NFS share path | `/` | Yes |
+
 ### Tips
 #### `subDir` parameter supports following pv/pvc metadata conversion
 > if `subDir` value contains following strings, it would be converted into corresponding pv/pvc name or namespace
diff --git a/pkg/nfs/controllerserver.go b/pkg/nfs/controllerserver.go
index c572154b..ff1a9167 100644
--- a/pkg/nfs/controllerserver.go
+++ b/pkg/nfs/controllerserver.go
@@ -376,7 +376,7 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 		return nil, status.Errorf(codes.NotFound, "failed to create nfsSnapshot: %v", err)
 	}
 	snapVol := volumeFromSnapshot(snapshot)
-	if err = cs.internalMount(ctx, snapVol, nil, nil); err != nil {
+	if err = cs.internalMount(ctx, snapVol, req.GetParameters(), nil); err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to mount snapshot nfs server: %v", err)
 	}
 	defer func() {
@@ -392,7 +392,7 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 		return nil, err
 	}
 
-	if err = cs.internalMount(ctx, srcVol, nil, nil); err != nil {
+	if err = cs.internalMount(ctx, srcVol, req.GetParameters(), nil); err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to mount src nfs server: %v", err)
 	}
 	defer func() {
@@ -652,6 +652,8 @@ func newNFSSnapshot(name string, params map[string]string, vol *nfsVolume) (*nfs
 			server = v
 		case paramShare:
 			baseDir = v
+		case mountOptionsField:
+			// no op
 		default:
 			return nil, status.Errorf(codes.InvalidArgument, "invalid parameter %q in snapshot storage class", k)
 		}


Test patch:
diff --git a/pkg/nfs/controllerserver_test.go b/pkg/nfs/controllerserver_test.go
index 196a5aa5..540292ee 100644
--- a/pkg/nfs/controllerserver_test.go
+++ b/pkg/nfs/controllerserver_test.go
@@ -933,6 +933,7 @@ func TestCreateSnapshot(t *testing.T) {
 			req: &csi.CreateSnapshotRequest{
 				SourceVolumeId: "nfs-server.default.svc.cluster.local#share#subdir#src-pv-name",
 				Name:           "snapshot-name",
+				Parameters:     map[string]string{"mountOptions": "nfsvers=4.1,sec=sys"},
 			},
 			expResp: &csi.CreateSnapshotResponse{
 				Snapshot: &csi.Snapshot{
@@ -954,6 +955,15 @@ func TestCreateSnapshot(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			desc: "create snapshot with non supported parameters",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "nfs-server.default.svc.cluster.local#share#subdir#src-pv-name",
+				Name:           "snapshot-name",
+				Parameters:     map[string]string{"unknown": "value"},
+			},
+			expectErr: true,
+		},
 	}
 	for _, test := range cases {
 		t.Run(test.desc, func(t *testing.T) {