Merge pull request #878 from andyzhangx/fix-remove-root-dir-issue

fix: only cleanup empty parent directories during DeleteVolume

Author: andyzhangx

pkg/nfs/controllerserver.go

@@ -292,17 +292,16 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 			}
 			klog.V(2).Infof("archived subdirectory %s --> %s", internalVolumePath, archivedInternalVolumePath)
 		} else {
-			rootDir := getRootDir(nfsVol.subDir)
-			if rootDir != "" {
-				rootDir = filepath.Join(getInternalMountPath(cs.Driver.workingMountDir, nfsVol), rootDir)
-			} else {
-				rootDir = internalVolumePath
-			}
-			// delete subdirectory under base-dir
-			klog.V(2).Infof("removing subdirectory at %v on internalVolumePath %s", rootDir, internalVolumePath)
-			if err = os.RemoveAll(rootDir); err != nil {
+			klog.V(2).Infof("removing subdirectory at %v", internalVolumePath)
+			if err = os.RemoveAll(internalVolumePath); err != nil {
 				return nil, status.Errorf(codes.Internal, "delete subdirectory(%s) failed with %v", internalVolumePath, err)
 			}
+
+			parentDir := filepath.Dir(internalVolumePath)
+			klog.V(2).Infof("DeleteVolume: removing empty directories in %s", parentDir)
+			if err = removeEmptyDirs(getInternalMountPath(cs.Driver.workingMountDir, nfsVol), parentDir); err != nil {
+				return nil, status.Errorf(codes.Internal, "failed to remove empty directories: %v", err)
+			}
 		}
 	} else {
 		klog.V(2).Infof("DeleteVolume: volume(%s) is set to retain, not deleting/archiving subdirectory", volumeID)

pkg/nfs/utils.go

@@ -19,6 +19,7 @@ package nfs
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"time"
@@ -216,10 +217,50 @@ func waitForPathNotExistWithTimeout(path string, timeout time.Duration) error {
 	}
 }
 
-// getRootDir returns the root directory of the given directory
-func getRootDir(path string) string {
-	parts := strings.Split(path, "/")
-	return parts[0]
+// removeEmptyDirs removes empty directories in the given directory dir until the parent directory parentDir
+// It will remove all empty directories in the path from the given directory to the parent directory
+// It will not remove the parent directory parentDir
+func removeEmptyDirs(parentDir, dir string) error {
+	if parentDir == "" || dir == "" {
+		return nil
+	}
+
+	absParentDir, err := filepath.Abs(parentDir)
+	if err != nil {
+		return err
+	}
+
+	absDir, err := filepath.Abs(dir)
+	if err != nil {
+		return err
+	}
+
+	if !strings.HasPrefix(absDir, absParentDir) {
+		return fmt.Errorf("dir %s is not a subdirectory of parentDir %s", dir, parentDir)
+	}
+
+	var depth int
+	for absDir != absParentDir {
+		entries, err := os.ReadDir(absDir)
+		if err != nil {
+			return err
+		}
+		if len(entries) == 0 {
+			klog.V(2).Infof("Removing empty directory %s", absDir)
+			if err := os.Remove(absDir); err != nil {
+				return err
+			}
+		} else {
+			klog.V(2).Infof("Directory %s is not empty", absDir)
+			break
+		}
+		if depth++; depth > 10 {
+			return fmt.Errorf("depth of directory %s is too deep", dir)
+		}
+		absDir = filepath.Dir(absDir)
+	}
+
+	return nil
 }
 
 // ExecFunc returns a exec function's output and error

pkg/nfs/utils_test.go

@@ -388,43 +388,63 @@ func TestWaitForPathNotExistWithTimeout(t *testing.T) {
 	}
 }
 
-func TestGetRootPath(t *testing.T) {
+func TestRemoveEmptyDirs(t *testing.T) {
+	parentDir, _ := os.Getwd()
+	emptyDirOneLevel, _ := getWorkDirPath("emptyDir1")
+	emptyDirTwoLevels, _ := getWorkDirPath("emptyDir2/emptyDir2")
+	emptyDirThreeLevels, _ := getWorkDirPath("emptyDir3/emptyDir2/emptyDir3")
+
 	tests := []struct {
-		desc     string
-		dir      string
-		expected string
+		desc      string
+		parentDir string
+		dir       string
+		expected  error
 	}{
 		{
-			desc:     "empty path",
-			dir:      "",
-			expected: "",
+			desc:      "empty path",
+			parentDir: parentDir,
+			expected:  nil,
 		},
 		{
-			desc:     "root path",
-			dir:      "/",
-			expected: "",
+			desc:      "empty dir with one level",
+			parentDir: parentDir,
+			dir:       emptyDirOneLevel,
+			expected:  nil,
 		},
 		{
-			desc:     "subdir path",
-			dir:      "/subdir",
-			expected: "",
+			desc:      "dir is not a subdirectory of parentDir",
+			parentDir: "/dir1",
+			dir:       "/dir2",
+			expected:  fmt.Errorf("dir /dir2 is not a subdirectory of parentDir /dir1"),
 		},
 		{
-			desc:     "subdir path without leading slash",
-			dir:      "subdir",
-			expected: "subdir",
+			desc:      "empty dir with two levels",
+			parentDir: parentDir,
+			dir:       emptyDirTwoLevels,
+			expected:  nil,
 		},
 		{
-			desc:     "multiple subdir path without leading slash",
-			dir:      "subdir/subdir2",
-			expected: "subdir",
+			desc:      "empty dir with three levels",
+			parentDir: parentDir,
+			dir:       emptyDirThreeLevels,
+			expected:  nil,
 		},
 	}
 
 	for _, test := range tests {
-		result := getRootDir(test.dir)
-		if result != test.expected {
-			t.Errorf("Unexpected result: %s, expected: %s", result, test.expected)
+		if strings.Contains(test.dir, "emptyDir") {
+			_ = makeDir(test.dir)
+			defer os.RemoveAll(test.dir)
+		}
+		err := removeEmptyDirs(test.parentDir, test.dir)
+		if !reflect.DeepEqual(err, test.expected) {
+			t.Errorf("test[%s]: unexpected output: %v, expected result: %v", test.desc, err, test.expected)
+		}
+		if strings.Contains(test.dir, "emptyDir") {
+			// directory should be removed
+			if _, err := os.Stat(emptyDirOneLevel); !os.IsNotExist(err) {
+				t.Errorf("test[%s]: directory %s should be removed", test.desc, emptyDirOneLevel)
+			}
 		}
 	}
 }