From 16c442ad78faeb24039b4c98e249489b4c1c54ed Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sat, 11 Jun 2022 14:55:29 +0000 Subject: [PATCH] feat: add subDir in storage class parameters fix subdir internal mount fix mount subdir in NodePublishVolume fix golint fix fix setKeyValueInMap --- docs/driver-parameters.md | 1 + pkg/nfs/controllerserver.go | 92 +++++++++++++++++---------- pkg/nfs/controllerserver_test.go | 76 ++++++++++++++++++---- pkg/nfs/nfs.go | 1 + pkg/nfs/nodeserver.go | 8 ++- pkg/nfs/utils.go | 15 +++++ pkg/nfs/utils_test.go | 54 ++++++++++++++++ test/e2e/dynamic_provisioning_test.go | 2 +- test/e2e/e2e_suite_test.go | 8 +++ 9 files changed, 209 insertions(+), 48 deletions(-) diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md index 6c54fdf1..bb702878 100644 --- a/docs/driver-parameters.md +++ b/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`
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) diff --git a/pkg/nfs/controllerserver.go b/pkg/nfs/controllerserver.go index 103e80d1..c1147dfe 100644 --- a/pkg/nfs/controllerserver.go +++ b/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 - parameters[paramShare] = cs.getVolumeSharePath(nfsVol) + setKeyValueInMap(parameters, paramSubDir, nfsVol.subDir) 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 { - volumeContext = make(map[string]string) - } - volumeContext[paramServer] = vol.server - volumeContext[paramShare] = sharePath + sharePath := filepath.Join(string(filepath.Separator) + vol.baseDir) + targetPath := getInternalMountPath(cs.Driver.workingMountDir, vol) - 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 -func (cs *ControllerServer) getInternalMountPath(vol *nfsVolume) string { - return filepath.Join(cs.Driver.workingMountDir, vol.subDir) +// getInternalMountPath: get working directory for CreateVolume and DeleteVolume +func getInternalMountPath(workingMountDir string, vol *nfsVolume) string { + 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 { - return filepath.Join(cs.getInternalMountPath(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) +func getInternalVolumePath(workingMountDir string, vol *nfsVolume) string { + return filepath.Join(getInternalMountPath(workingMountDir, vol), 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 } diff --git a/pkg/nfs/controllerserver_test.go b/pkg/nfs/controllerserver_test.go index 2444a4fa..4b0a52f4 100644 --- a/pkg/nfs/controllerserver_test.go +++ b/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" - testVolumeID = "test-server/test-base-dir/test-csi" - newTestVolumeID = "test-server#test-base-dir#test-csi" - testVolumeIDNested = "test-server/test/base/dir/test-csi" - newTestVolumeIDNested = "test-server#test/base/dir#test-csi" -) - -// for Windows support in the future -var ( - testShare = filepath.Join(string(filepath.Separator), testBaseDir, string(filepath.Separator), testCSIVolume) + testCSIVolume = "volume-name" + testVolumeID = "test-server/test-base-dir/volume-name" + newTestVolumeID = "test-server#test-base-dir#volume-name#" + testVolumeIDNested = "test-server/test/base/dir/volume-name" + newTestVolumeIDNested = "test-server#test/base/dir#volume-name#" + newTestVolumeIDUUID = "test-server#test-base-dir#volume-name#uuid" ) 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) + } +} diff --git a/pkg/nfs/nfs.go b/pkg/nfs/nfs.go index 9c977cb8..b45d707e 100644 --- a/pkg/nfs/nfs.go +++ b/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" ) diff --git a/pkg/nfs/nodeserver.go b/pkg/nfs/nodeserver.go index ea5e0de4..0688c78a 100644 --- a/pkg/nfs/nodeserver.go +++ b/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 { diff --git a/pkg/nfs/utils.go b/pkg/nfs/utils.go index c4fc0788..fc2b3ad6 100644 --- a/pkg/nfs/utils.go +++ b/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 +} diff --git a/pkg/nfs/utils_test.go b/pkg/nfs/utils_test.go index b9b4997e..d815cc00 100644 --- a/pkg/nfs/utils_test.go +++ b/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) + } + } +} diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go index 4680b172..e47d37df 100644 --- a/test/e2e/dynamic_provisioning_test.go +++ b/test/e2e/dynamic_provisioning_test.go @@ -242,7 +242,7 @@ var _ = ginkgo.Describe("Dynamic Provisioning", func() { test := testsuites.DynamicallyProvisionedPodWithMultiplePVsTest{ CSIDriver: testDriver, Pods: pods, - StorageClassParameters: defaultStorageClassParameters, + StorageClassParameters: subDirStorageClassParameters, } test.Run(cs, ns) }) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index ffc548e2..9c225f30 100644 --- a/test/e2e/e2e_suite_test.go +++ b/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 )