Merge pull request #879 from jakobmoellerdev/btrfs-testing

fix: btrfs unmounting + e2e tests

cmd/topolvm-controller/app/root.go

@@ -25,6 +25,10 @@ const (
 	// Derived from the usual 4096K blocks, 1024 inode default and journaling overhead,
 	// Allows for more than 80% free space after formatting, anything lower significantly reduces this percentage.
 	DefaultMinimumAllocationSizeExt4 = "32Mi"
+	// DefaultMinimumAllocationSizeBtrfs is the default minimum size for a filesystem volume with btrfs formatting.
+	// Btrfs changes its minimum allocation size based on various underlying device block settings and the host OS,
+	// but 200Mi seemed to be safe after some experimentation.
+	DefaultMinimumAllocationSizeBtrfs = "200Mi"
 )
 
 var config struct {
@@ -92,8 +96,9 @@ func init() {
 		"Minimum Allocation Sizing for block storage. Logical Volumes will always be at least this big.")
 	config.controllerServerSettings.MinimumAllocationSettings.Filesystem = make(map[string]driver.Quantity)
 	for filesystem, minimum := range map[string]resource.Quantity{
-		"ext4": resource.MustParse(DefaultMinimumAllocationSizeExt4),
-		"xfs":  resource.MustParse(DefaultMinimumAllocationSizeXFS),
+		"ext4":  resource.MustParse(DefaultMinimumAllocationSizeExt4),
+		"xfs":   resource.MustParse(DefaultMinimumAllocationSizeXFS),
+		"btrfs": resource.MustParse(DefaultMinimumAllocationSizeBtrfs),
 	} {
 		config.controllerServerSettings.MinimumAllocationSettings.Filesystem[filesystem] = driver.NewQuantityFlagVar(fs,
 			fmt.Sprintf("minimum-allocation-%s", filesystem),

internal/driver/node.go

@@ -230,7 +230,7 @@ func (s *nodeServerNoLocked) nodePublishFilesystemVolume(req *csi.NodePublishVol
 		return status.Errorf(codes.Internal, "target device is already formatted with different filesystem: volume=%s, current=%s, new:%s", req.GetVolumeId(), fsType, mountOption.FsType)
 	}
 
-	mounted, err := filesystem.IsMounted(device, req.GetTargetPath())
+	mounted, err := s.mounter.IsMountPoint(req.GetTargetPath())
 	if err != nil {
 		return status.Errorf(codes.Internal, "mount check failed: target=%s, error=%v", req.GetTargetPath(), err)
 	}
@@ -363,22 +363,11 @@ func (s *nodeServerNoLocked) NodeUnpublishVolume(ctx context.Context, req *csi.N
 func (s *nodeServerNoLocked) nodeUnpublishFilesystemVolume(req *csi.NodeUnpublishVolumeRequest, device string) error {
 	targetPath := req.GetTargetPath()
 
-	mounted, err := filesystem.IsMounted(device, targetPath)
-	if err != nil {
-		return status.Errorf(codes.Internal, "mount check failed: target=%s, error=%v", targetPath, err)
-	}
-	if mounted {
-		if err := s.mounter.Unmount(targetPath); err != nil {
-			return status.Errorf(codes.Internal, "unmount failed for %s: error=%v", targetPath, err)
-		}
-	}
-
-	if err := os.RemoveAll(targetPath); err != nil {
-		return status.Errorf(codes.Internal, "remove dir failed for %s: error=%v", targetPath, err)
+	if err := mountutil.CleanupMountPoint(targetPath, s.mounter, true); err != nil {
+		return status.Errorf(codes.Internal, "unmount failed for %s: error=%v", targetPath, err)
 	}
 
-	err = os.Remove(device)
-	if err != nil && !os.IsNotExist(err) {
+	if err := os.Remove(device); err != nil && !os.IsNotExist(err) {
 		return status.Errorf(codes.Internal, "remove device failed for %s: error=%v", device, err)
 	}
 

internal/filesystem/util.go

@@ -1,25 +1,12 @@
-/*
- * kernelHasMountinfoBug() and the constants only used in this function are copied
- * from the following code:
- * https://github.com/kubernetes/mount-utils/blob/6f4aae5a6ab58574cac605cdd48bf5c0862c047f/mount_helper_unix.go#L211-L242
- *    LICENSE: http://www.apache.org/licenses/LICENSE-2.0
- *    Copyright The Kubernetes Authors.
- */
-
 package filesystem
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"os/exec"
-	"path/filepath"
-	"strconv"
 	"strings"
-	"sync"
 
 	"golang.org/x/sys/unix"
-	"k8s.io/utils/io"
 )
 
 const (
@@ -30,75 +17,6 @@ type temporaryer interface {
 	Temporary() bool
 }
 
-func isSameDevice(dev1, dev2 string) (bool, error) {
-	if dev1 == dev2 {
-		return true, nil
-	}
-
-	var st1, st2 unix.Stat_t
-	if err := Stat(dev1, &st1); err != nil {
-		// Some filesystems like tmpfs and nfs aren't backed by block device files.
-		// In such case, given device path does not exist,
-		// we regard it is not an error but is false.
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, fmt.Errorf("stat failed for %s: %v", dev1, err)
-	}
-	if err := Stat(dev2, &st2); err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, fmt.Errorf("stat failed for %s: %v", dev2, err)
-	}
-
-	return st1.Rdev == st2.Rdev, nil
-}
-
-// IsMounted returns true if device is mounted on target.
-// The implementation uses /proc/mounts because some filesystem uses a virtual device.
-func IsMounted(device, target string) (bool, error) {
-	abs, err := filepath.Abs(target)
-	if err != nil {
-		return false, err
-	}
-	target, err = filepath.EvalSymlinks(abs)
-	if err != nil {
-		return false, err
-	}
-
-	data, err := readMountInfo("/proc/mounts")
-	if err != nil {
-		return false, fmt.Errorf("could not read /proc/mounts: %v", err)
-	}
-
-	for _, line := range strings.Split(string(data), "\n") {
-		fields := strings.Fields(line)
-		if len(fields) < 2 {
-			continue
-		}
-
-		// If the filesystem is nfs and its connection is broken, EvalSymlinks will be stuck.
-		// So it should be in before calling EvalSymlinks.
-		ok, err := isSameDevice(device, fields[0])
-		if err != nil {
-			return false, err
-		}
-		if !ok {
-			continue
-		}
-		d, err := filepath.EvalSymlinks(fields[1])
-		if err != nil {
-			return false, err
-		}
-		if d == target {
-			return true, nil
-		}
-	}
-
-	return false, nil
-}
-
 // DetectFilesystem returns filesystem type if device has a filesystem.
 // This returns an empty string if no filesystem exists.
 func DetectFilesystem(device string) (string, error) {
@@ -174,56 +92,3 @@ func Statfs(path string, buf *unix.Statfs_t) (err error) {
 		return err
 	}
 }
-
-// These variables are used solely by kernelHasMountinfoBug.
-var (
-	hasMountinfoBug        bool
-	checkMountinfoBugOnce  sync.Once
-	maxConsistentReadTimes = 3
-)
-
-// kernelHasMountinfoBug checks if the kernel bug that can lead to incomplete
-// mountinfo being read is fixed. It does so by checking the kernel version.
-//
-// The bug was fixed by the kernel commit 9f6c61f96f2d97 (since Linux 5.8).
-// Alas, there is no better way to check if the bug is fixed other than to
-// rely on the kernel version returned by uname.
-//
-// Copied from
-// https://github.com/kubernetes/mount-utils/blob/6f4aae5a6ab58574cac605cdd48bf5c0862c047f/mount_helper_unix.go#L204C1-L250
-// *    LICENSE: http://www.apache.org/licenses/LICENSE-2.0
-// *    Copyright The Kubernetes Authors.
-func kernelHasMountinfoBug() bool {
-	checkMountinfoBugOnce.Do(func() {
-		// Assume old kernel.
-		hasMountinfoBug = true
-
-		uname := unix.Utsname{}
-		err := unix.Uname(&uname)
-		if err != nil {
-			return
-		}
-
-		end := bytes.IndexByte(uname.Release[:], 0)
-		v := bytes.SplitN(uname.Release[:end], []byte{'.'}, 3)
-		if len(v) != 3 {
-			return
-		}
-		major, _ := strconv.Atoi(string(v[0]))
-		minor, _ := strconv.Atoi(string(v[1]))
-
-		if major > 5 || (major == 5 && minor >= 8) {
-			hasMountinfoBug = false
-		}
-	})
-
-	return hasMountinfoBug
-}
-
-func readMountInfo(path string) ([]byte, error) {
-	if kernelHasMountinfoBug() {
-		return io.ConsistentRead(path, maxConsistentReadTimes)
-	}
-
-	return os.ReadFile(path)
-}