Merge pull request #349 from andyzhangx/subDir

feat: add subDir in storage class parameters

docs/driver-parameters.md

@@ -8,6 +8,7 @@ Name | Meaning | Example Value | Mandatory | Default value
 --- | --- | --- | --- | ---
 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 |
+subDir | sub directory under nfs share |  | No | if sub directory does not exist, this driver would create a new one
 mountPermissions | mounted folder permissions. The default is `0777`, if set as `0`, driver will not perform `chmod` after mount |  | No |
 
 ### PV/PVC usage (static provisioning)

pkg/nfs/controllerserver.go

@@ -52,6 +52,8 @@ type nfsVolume struct {
 	subDir string
 	// size of volume
 	size int64
+	// pv name when subDir is not empty
+	uuid string
 }
 
 // Ordering of elements in the CSI volume id.
@@ -64,6 +66,7 @@ const (
 	idServer = iota
 	idBaseDir
 	idSubDir
+	idUUID
 	totalIDElements // Always last
 )
 
@@ -93,6 +96,8 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 			// no op
 		case paramShare:
 			// no op
+		case paramSubDir:
+			// no op
 		case mountPermissionsField:
 			if v != "" {
 				var err error
@@ -126,7 +131,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	fileMode := os.FileMode(mountPermissions)
 	// Create subdirectory under base-dir
-	internalVolumePath := cs.getInternalVolumePath(nfsVol)
+	internalVolumePath := getInternalVolumePath(cs.Driver.workingMountDir, nfsVol)
 	if err = os.Mkdir(internalVolumePath, fileMode); err != nil && !os.IsExist(err) {
 		return nil, status.Errorf(codes.Internal, "failed to make subdirectory: %v", err.Error())
 	}
@@ -135,8 +140,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		klog.Warningf("failed to chmod subdirectory: %v", err.Error())
 	}
 
-	parameters[paramServer] = nfsVol.server
+	setKeyValueInMap(parameters, paramSubDir, nfsVol.subDir)
-	parameters[paramShare] = cs.getVolumeSharePath(nfsVol)
 	return &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
 			VolumeId:      nfsVol.id,
@@ -183,7 +187,7 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 	}()
 
 	// delete subdirectory under base-dir
-	internalVolumePath := cs.getInternalVolumePath(nfsVol)
+	internalVolumePath := getInternalVolumePath(cs.Driver.workingMountDir, nfsVol)
 
 	klog.V(2).Infof("Removing subdirectory at %v", internalVolumePath)
 	if err = os.RemoveAll(internalVolumePath); err != nil {
@@ -255,9 +259,6 @@ func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi
 
 // Mount nfs server at base-dir
 func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, volumeContext map[string]string, volCap *csi.VolumeCapability) error {
-	sharePath := filepath.Join(string(filepath.Separator) + vol.baseDir)
-	targetPath := cs.getInternalMountPath(vol)
-
 	if volCap == nil {
 		volCap = &csi.VolumeCapability{
 			AccessType: &csi.VolumeCapability_Mount{
@@ -266,16 +267,24 @@ func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, v
 		}
 	}
 
-	if volumeContext == nil {
+	sharePath := filepath.Join(string(filepath.Separator) + vol.baseDir)
-		volumeContext = make(map[string]string)
+	targetPath := getInternalMountPath(cs.Driver.workingMountDir, vol)
-	}
-	volumeContext[paramServer] = vol.server
-	volumeContext[paramShare] = sharePath
 
-	klog.V(2).Infof("internally mounting %v:%v at %v", vol.server, sharePath, targetPath)
+	volContext := map[string]string{
+		paramServer: vol.server,
+		paramShare:  sharePath,
+	}
+	for k, v := range volumeContext {
+		// don't set subDir field since only nfs-server:/share should be mounted in CreateVolume/DeleteVolume
+		if strings.ToLower(k) != paramSubDir {
+			volContext[k] = v
+		}
+	}
+
+	klog.V(2).Infof("internally mounting %s:%s at %s", vol.server, sharePath, targetPath)
 	_, err := cs.Driver.ns.NodePublishVolume(ctx, &csi.NodePublishVolumeRequest{
 		TargetPath:       targetPath,
-		VolumeContext:    volumeContext,
+		VolumeContext:    volContext,
 		VolumeCapability: volCap,
 		VolumeId:         vol.id,
 	})
@@ -284,20 +293,20 @@ func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, v
 
 // Unmount nfs server at base-dir
 func (cs *ControllerServer) internalUnmount(ctx context.Context, vol *nfsVolume) error {
-	targetPath := cs.getInternalMountPath(vol)
+	targetPath := getInternalMountPath(cs.Driver.workingMountDir, vol)
 
 	// Unmount nfs server at base-dir
 	klog.V(4).Infof("internally unmounting %v", targetPath)
 	_, err := cs.Driver.ns.NodeUnpublishVolume(ctx, &csi.NodeUnpublishVolumeRequest{
 		VolumeId:   vol.id,
-		TargetPath: cs.getInternalMountPath(vol),
+		TargetPath: targetPath,
 	})
 	return err
 }
 
 // Convert VolumeCreate parameters to an nfsVolume
 func (cs *ControllerServer) newNFSVolume(name string, size int64, params map[string]string) (*nfsVolume, error) {
-	var server, baseDir string
+	var server, baseDir, subDir string
 
 	// validate parameters (case-insensitive)
 	for k, v := range params {
@@ -306,6 +315,8 @@ func (cs *ControllerServer) newNFSVolume(name string, size int64, params map[str
 			server = v
 		case paramShare:
 			baseDir = v
+		case paramSubDir:
+			subDir = v
 		}
 	}
 
@@ -319,17 +330,30 @@ func (cs *ControllerServer) newNFSVolume(name string, size int64, params map[str
 	vol := &nfsVolume{
 		server:  server,
 		baseDir: baseDir,
-		subDir:  name,
 		size:    size,
 	}
+	if subDir == "" {
+		// use pv name by default if not specified
+		vol.subDir = name
+	} else {
+		vol.subDir = subDir
+		// make volume id unique if subDir is provided
+		vol.uuid = name
+	}
 	vol.id = cs.getVolumeIDFromNfsVol(vol)
-
 	return vol, nil
 }
 
-// Get working directory for CreateVolume and DeleteVolume
+// getInternalMountPath: get working directory for CreateVolume and DeleteVolume
-func (cs *ControllerServer) getInternalMountPath(vol *nfsVolume) string {
+func getInternalMountPath(workingMountDir string, vol *nfsVolume) string {
-	return filepath.Join(cs.Driver.workingMountDir, vol.subDir)
+	if vol == nil {
+		return ""
+	}
+	mountDir := vol.uuid
+	if vol.uuid == "" {
+		mountDir = vol.subDir
+	}
+	return filepath.Join(workingMountDir, mountDir)
 }
 
 // Get internal path where the volume is created
@@ -339,13 +363,8 @@ func (cs *ControllerServer) getInternalMountPath(vol *nfsVolume) string {
 //     CreateVolume calls in parallel and they may use the same underlying share.
 //     Instead of refcounting how many CreateVolume calls are using the same
 //     share, it's simpler to just do a mount per request.
-func (cs *ControllerServer) getInternalVolumePath(vol *nfsVolume) string {
+func getInternalVolumePath(workingMountDir string, vol *nfsVolume) string {
-	return filepath.Join(cs.getInternalMountPath(vol), vol.subDir)
+	return filepath.Join(getInternalMountPath(workingMountDir, vol), vol.subDir)
-}
-
-// Get user-visible share path for the volume
-func (cs *ControllerServer) getVolumeSharePath(vol *nfsVolume) string {
-	return filepath.Join(string(filepath.Separator), vol.baseDir, vol.subDir)
 }
 
 // Given a nfsVolume, return a CSI volume id
@@ -354,22 +373,25 @@ func (cs *ControllerServer) getVolumeIDFromNfsVol(vol *nfsVolume) string {
 	idElements[idServer] = strings.Trim(vol.server, "/")
 	idElements[idBaseDir] = strings.Trim(vol.baseDir, "/")
 	idElements[idSubDir] = strings.Trim(vol.subDir, "/")
+	idElements[idUUID] = vol.uuid
 	return strings.Join(idElements, separator)
 }
 
 // Given a CSI volume id, return a nfsVolume
 // sample volume Id:
-//   new volumeID: nfs-server.default.svc.cluster.local#share#pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
+//   new volumeID:
+//	    nfs-server.default.svc.cluster.local#share#pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
+//	    nfs-server.default.svc.cluster.local#share#subdir#pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
 //   old volumeID: nfs-server.default.svc.cluster.local/share/pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
 func getNfsVolFromID(id string) (*nfsVolume, error) {
-	var server, baseDir, subDir string
+	var server, baseDir, subDir, uuid string
 	segments := strings.Split(id, separator)
 	if len(segments) < 3 {
 		klog.V(2).Infof("could not split %s into server, baseDir and subDir with separator(%s)", id, separator)
-		// try with separator "/""
+		// try with separator "/"
 		volRegex := regexp.MustCompile("^([^/]+)/(.*)/([^/]+)$")
 		tokens := volRegex.FindStringSubmatch(id)
-		if tokens == nil {
+		if tokens == nil || len(tokens) < 4 {
 			return nil, fmt.Errorf("could not split %s into server, baseDir and subDir with separator(%s)", id, "/")
 		}
 		server = tokens[1]
@@ -379,6 +401,9 @@ func getNfsVolFromID(id string) (*nfsVolume, error) {
 		server = segments[0]
 		baseDir = segments[1]
 		subDir = segments[2]
+		if len(segments) >= 4 {
+			uuid = segments[3]
+		}
 	}
 
 	return &nfsVolume{
@@ -386,6 +411,7 @@ func getNfsVolFromID(id string) (*nfsVolume, error) {
 		server:  server,
 		baseDir: baseDir,
 		subDir:  subDir,
+		uuid:    uuid,
 	}, nil
 }
 

pkg/nfs/controllerserver_test.go

@@ -27,6 +27,7 @@ import (
 	"fmt"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/stretchr/testify/assert"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -37,16 +38,12 @@ const (
 	testServer            = "test-server"
 	testBaseDir           = "test-base-dir"
 	testBaseDirNested     = "test/base/dir"
-	testCSIVolume         = "test-csi"
+	testCSIVolume         = "volume-name"
-	testVolumeID          = "test-server/test-base-dir/test-csi"
+	testVolumeID          = "test-server/test-base-dir/volume-name"
-	newTestVolumeID       = "test-server#test-base-dir#test-csi"
+	newTestVolumeID       = "test-server#test-base-dir#volume-name#"
-	testVolumeIDNested    = "test-server/test/base/dir/test-csi"
+	testVolumeIDNested    = "test-server/test/base/dir/volume-name"
-	newTestVolumeIDNested = "test-server#test/base/dir#test-csi"
+	newTestVolumeIDNested = "test-server#test/base/dir#volume-name#"
-)
+	newTestVolumeIDUUID   = "test-server#test-base-dir#volume-name#uuid"
-
-// for Windows support in the future
-var (
-	testShare = filepath.Join(string(filepath.Separator), testBaseDir, string(filepath.Separator), testCSIVolume)
 )
 
 func initTestController(t *testing.T) *ControllerServer {
@@ -110,7 +107,8 @@ func TestCreateVolume(t *testing.T) {
 					VolumeId: newTestVolumeID,
 					VolumeContext: map[string]string{
 						paramServer:           testServer,
-						paramShare:            testShare,
+						paramShare:            testBaseDir,
+						paramSubDir:           testCSIVolume,
 						mountPermissionsField: "0750",
 					},
 				},
@@ -133,14 +131,16 @@ func TestCreateVolume(t *testing.T) {
 				Parameters: map[string]string{
 					paramServer: testServer,
 					paramShare:  testBaseDir,
+					paramSubDir: testCSIVolume,
 				},
 			},
 			resp: &csi.CreateVolumeResponse{
 				Volume: &csi.Volume{
-					VolumeId: newTestVolumeID,
+					VolumeId: newTestVolumeID + testCSIVolume,
 					VolumeContext: map[string]string{
 						paramServer: testServer,
-						paramShare:  testShare,
+						paramShare:  testBaseDir,
+						paramSubDir: testCSIVolume,
 					},
 				},
 			},
@@ -417,6 +417,18 @@ func TestNfsVolFromId(t *testing.T) {
 			},
 			expectErr: false,
 		},
+		{
+			name:     "valid request nested baseDir with newTestVolumeIDNested",
+			volumeID: newTestVolumeIDUUID,
+			resp: &nfsVolume{
+				id:      newTestVolumeIDUUID,
+				server:  testServer,
+				baseDir: testBaseDir,
+				subDir:  testCSIVolume,
+				uuid:    "uuid",
+			},
+			expectErr: false,
+		},
 	}
 
 	for _, test := range cases {
@@ -479,3 +491,41 @@ func TestIsValidVolumeCapabilities(t *testing.T) {
 		}
 	}
 }
+
+func TestGetInternalMountPath(t *testing.T) {
+	cases := []struct {
+		desc            string
+		workingMountDir string
+		vol             *nfsVolume
+		result          string
+	}{
+		{
+			desc:            "nil volume",
+			workingMountDir: "/tmp",
+			result:          "",
+		},
+		{
+			desc:            "uuid not empty",
+			workingMountDir: "/tmp",
+			vol: &nfsVolume{
+				subDir: "subdir",
+				uuid:   "uuid",
+			},
+			result: filepath.Join("/tmp", "uuid"),
+		},
+		{
+			desc:            "uuid empty",
+			workingMountDir: "/tmp",
+			vol: &nfsVolume{
+				subDir: "subdir",
+				uuid:   "",
+			},
+			result: filepath.Join("/tmp", "subdir"),
+		},
+	}
+
+	for _, test := range cases {
+		path := getInternalMountPath(test.workingMountDir, test.vol)
+		assert.Equal(t, path, test.result)
+	}
+}

pkg/nfs/nfs.go

@@ -55,6 +55,7 @@ const (
 	// The root directory is omitted from the string, for example:
 	//     "base" instead of "/base"
 	paramShare            = "share"
+	paramSubDir           = "subdir"
 	mountOptionsField     = "mountoptions"
 	mountPermissionsField = "mountpermissions"
 )

pkg/nfs/nodeserver.go

@@ -56,7 +56,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 		mountOptions = append(mountOptions, "ro")
 	}
 
-	var server, baseDir string
+	var server, baseDir, subDir string
 	mountPermissions := ns.Driver.mountPermissions
 	performChmodOp := (mountPermissions > 0)
 	for k, v := range req.GetVolumeContext() {
@@ -65,6 +65,8 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 			server = v
 		case paramShare:
 			baseDir = v
+		case paramSubDir:
+			subDir = v
 		case mountOptionsField:
 			if v != "" {
 				mountOptions = append(mountOptions, v)
@@ -93,6 +95,10 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	}
 	server = getServerFromSource(server)
 	source := fmt.Sprintf("%s:%s", server, baseDir)
+	if subDir != "" {
+		source = strings.TrimRight(source, "/")
+		source = fmt.Sprintf("%s/%s", source, subDir)
+	}
 
 	notMnt, err := ns.mounter.IsLikelyNotMountPoint(targetPath)
 	if err != nil {

pkg/nfs/utils.go

@@ -160,3 +160,18 @@ func getServerFromSource(server string) string {
 	}
 	return server
 }
+
+// setKeyValueInMap set key/value pair in map
+// key in the map is case insensitive, if key already exists, overwrite existing value
+func setKeyValueInMap(m map[string]string, key, value string) {
+	if m == nil {
+		return
+	}
+	for k := range m {
+		if strings.EqualFold(k, key) {
+			m[k] = value
+			return
+		}
+	}
+	m[key] = value
+}

pkg/nfs/utils_test.go

@@ -247,3 +247,57 @@ func TestGetServerFromSource(t *testing.T) {
 		}
 	}
 }
+
+func TestSetKeyValueInMap(t *testing.T) {
+	tests := []struct {
+		desc     string
+		m        map[string]string
+		key      string
+		value    string
+		expected map[string]string
+	}{
+		{
+			desc:  "nil map",
+			key:   "key",
+			value: "value",
+		},
+		{
+			desc:     "empty map",
+			m:        map[string]string{},
+			key:      "key",
+			value:    "value",
+			expected: map[string]string{"key": "value"},
+		},
+		{
+			desc:  "non-empty map",
+			m:     map[string]string{"k": "v"},
+			key:   "key",
+			value: "value",
+			expected: map[string]string{
+				"k":   "v",
+				"key": "value",
+			},
+		},
+		{
+			desc:     "same key already exists",
+			m:        map[string]string{"subDir": "value2"},
+			key:      "subDir",
+			value:    "value",
+			expected: map[string]string{"subDir": "value"},
+		},
+		{
+			desc:     "case insentive key already exists",
+			m:        map[string]string{"subDir": "value2"},
+			key:      "subdir",
+			value:    "value",
+			expected: map[string]string{"subDir": "value"},
+		},
+	}
+
+	for _, test := range tests {
+		setKeyValueInMap(test.m, test.key, test.value)
+		if !reflect.DeepEqual(test.m, test.expected) {
+			t.Errorf("test[%s]: unexpected output: %v, expected result: %v", test.desc, test.m, test.expected)
+		}
+	}
+}

test/e2e/dynamic_provisioning_test.go

@@ -186,6 +186,35 @@ var _ = ginkgo.Describe("Dynamic Provisioning", func() {
 		test.Run(cs, ns)
 	})
 
+	ginkgo.It("[subDir]should create a deployment object, write and read to it, delete the pod and write and read to it again [nfs.csi.k8s.io]", func() {
+		pod := testsuites.PodDetails{
+			Cmd: "echo 'hello world' >> /mnt/test-1/data && while true; do sleep 100; done",
+			Volumes: []testsuites.VolumeDetails{
+				{
+					ClaimSize: "10Gi",
+					VolumeMount: testsuites.VolumeMountDetails{
+						NameGenerate:      "test-volume-",
+						MountPathGenerate: "/mnt/test-",
+					},
+				},
+			},
+		}
+
+		podCheckCmd := []string{"cat", "/mnt/test-1/data"}
+		expectedString := "hello world\n"
+
+		test := testsuites.DynamicallyProvisionedDeletePodTest{
+			CSIDriver: testDriver,
+			Pod:       pod,
+			PodCheck: &testsuites.PodExecCheck{
+				Cmd:            podCheckCmd,
+				ExpectedString: expectedString, // pod will be restarted so expect to see 2 instances of string
+			},
+			StorageClassParameters: subDirStorageClassParameters,
+		}
+		test.Run(cs, ns)
+	})
+
 	ginkgo.It(fmt.Sprintf("should delete PV with reclaimPolicy %q [nfs.csi.k8s.io]", v1.PersistentVolumeReclaimDelete), func() {
 		reclaimPolicy := v1.PersistentVolumeReclaimDelete
 		volumes := []testsuites.VolumeDetails{
@@ -242,7 +271,7 @@ var _ = ginkgo.Describe("Dynamic Provisioning", func() {
 		test := testsuites.DynamicallyProvisionedPodWithMultiplePVsTest{
 			CSIDriver:              testDriver,
 			Pods:                   pods,
-			StorageClassParameters: defaultStorageClassParameters,
+			StorageClassParameters: subDirStorageClassParameters,
 		}
 		test.Run(cs, ns)
 	})

test/e2e/e2e_suite_test.go

@@ -59,6 +59,14 @@ var (
 		"csi.storage.k8s.io/provisioner-secret-namespace": "default",
 		"mountPermissions": "0",
 	}
+	subDirStorageClassParameters = map[string]string{
+		"server": nfsServerAddress,
+		"share":  nfsShare,
+		"subDir": "subDirectory",
+		"csi.storage.k8s.io/provisioner-secret-name":      "mount-options",
+		"csi.storage.k8s.io/provisioner-secret-namespace": "default",
+		"mountPermissions": "0755",
+	}
 	controllerServer *nfs.ControllerServer
 )