Refactor MountManager for testability, really wish golang had extendable interfaces right about now

Author: davidz627

cmd/main.go

@@ -22,6 +22,7 @@ import (
 
 	gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider"
 	driver "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-pd-csi-driver"
+	mountmanager "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/mount-manager"
 )
 
 func init() {
@@ -55,7 +56,7 @@ func handle() {
 		glog.Fatalf("Failed to get cloud provider: %v", err)
 	}
 
-	mounter := driver.NewSafeFormatAndMounter()
+	mounter := mountmanager.NewMounter()
 
 	err = gceDriver.SetupGCEDriver(cloudProvider, mounter, *driverName, *nodeID, vendorVersion)
 	if err != nil {

deploy/setup-project.sh

@@ -31,12 +31,12 @@ sed -i "/serviceAccount:${IAM_NAME}/d" "${PKGDIR}/deploy/iam.json"
 gcloud projects set-iam-policy "${PROJECT}" "${PKGDIR}/deploy/iam.json"
 rm -f "${PKGDIR}/deploy/iam.json"
 # Delete Service Account
-gcloud iam service-accounts delete "$IAM_NAME" --quiet || true
+gcloud iam service-accounts delete "${IAM_NAME}" --project "${PROJECT}" --quiet || true
 
 # Create new Service Account and Keys
-gcloud iam service-accounts create "${GCEPD_SA_NAME}"
+gcloud iam service-accounts create "${GCEPD_SA_NAME}" --project "${PROJECT}"
 for role in ${BIND_ROLES}
 do
   gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role ${role}
 done
-gcloud iam service-accounts keys create "${SA_FILE}" --iam-account "${IAM_NAME}"
+gcloud iam service-accounts keys create "${SA_FILE}" --iam-account "${IAM_NAME}" --project "${PROJECT}"

pkg/gce-pd-csi-driver/gce-pd-driver.go

@@ -21,8 +21,8 @@ import (
 	"github.com/golang/glog"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
-	"k8s.io/kubernetes/pkg/util/mount"
 	gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider"
+	mountmanager "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/mount-manager"
 )
 
 type GCEDriver struct {
@@ -43,7 +43,7 @@ func GetGCEDriver() *GCEDriver {
 	return &GCEDriver{}
 }
 
-func (gceDriver *GCEDriver) SetupGCEDriver(cloudProvider gce.GCECompute, mounter mount.SafeFormatAndMount, name, nodeID, vendorVersion string) error {
+func (gceDriver *GCEDriver) SetupGCEDriver(cloudProvider gce.GCECompute, mounter mountmanager.MountManager, name, nodeID, vendorVersion string) error {
 	if name == "" {
 		return fmt.Errorf("Driver name missing")
 	}
@@ -129,7 +129,7 @@ func NewIdentityServer(gceDriver *GCEDriver) *GCEIdentityServer {
 	}
 }
 
-func NewNodeServer(gceDriver *GCEDriver, mounter mount.SafeFormatAndMount) *GCENodeServer {
+func NewNodeServer(gceDriver *GCEDriver, mounter mountmanager.MountManager) *GCENodeServer {
 	return &GCENodeServer{
 		Driver:  gceDriver,
 		Mounter: mounter,
@@ -143,45 +143,6 @@ func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute) *GC
 	}
 }
 
-func NewSafeFormatAndMounter() mount.SafeFormatAndMount {
-	realMounter := mount.New("")
-	realExec := mount.NewOsExec()
-	return mount.SafeFormatAndMount{
-		Interface: realMounter,
-		Exec:      realExec,
-	}
-}
-
-func NewFakeSafeFormatAndMounter(mountErrs []error) mount.SafeFormatAndMount {
-	execCallback := func(cmd string, args ...string) ([]byte, error) {
-		return nil, nil
-		// TODO: Fill out exec callback for errors
-		/*
-			if len(test.execScripts) <= execCallCount {
-				t.Errorf("Unexpected command: %s %v", cmd, args)
-				return nil, nil
-			}
-			script := test.execScripts[execCallCount]
-			execCallCount++
-			if script.command != cmd {
-				t.Errorf("Unexpected command %s. Expecting %s", cmd, script.command)
-			}
-			for j := range args {
-				if args[j] != script.args[j] {
-					t.Errorf("Unexpected args %v. Expecting %v", args, script.args)
-				}
-			}
-			return []byte(script.output), script.err
-		*/
-	}
-	fakeMounter := &mount.FakeMounter{MountPoints: []mount.MountPoint{}, Log: []mount.FakeAction{}}
-	fakeExec := mount.NewFakeExec(execCallback)
-	return mount.SafeFormatAndMount{
-		Interface: fakeMounter,
-		Exec:      fakeExec,
-	}
-}
-
 func (gceDriver *GCEDriver) Run(endpoint string) {
 	glog.Infof("Driver: %v", gceDriver.name)
 

pkg/gce-pd-csi-driver/node.go

@@ -17,48 +17,20 @@ package gceGCEDriver
 import (
 	"fmt"
 	"os"
-	"path"
-	"path/filepath"
-	"strings"
 	"sync"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi/v0"
 	"github.com/golang/glog"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
-	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/kubernetes/pkg/util/mount"
-	"k8s.io/utils/exec"
+	mountmanager "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/mount-manager"
 	utils "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/utils"
 )
 
-const (
-	diskByHostIdPath     = "/host/dev/disk/by-id/"
-	diskByIdPath         = "/dev/disk/by-id/"
-	diskGooglePrefix     = "google-"
-	diskScsiGooglePrefix = "scsi-0Google_PersistentDisk_"
-	diskPartitionSuffix  = "-part"
-	diskSDPath           = "/host/dev/sd"
-	diskSDPattern        = "/host/dev/sd*"
-	// How many times to retry for a consistent read of /proc/mounts.
-	maxListTries = 3
-	// Number of fields per line in /proc/mounts as per the fstab man page.
-	expectedNumFieldsPerLine = 6
-	// Location of the mount file to use
-	procMountsPath = "/proc/mounts"
-	// Location of the mountinfo file
-	procMountInfoPath = "/proc/self/mountinfo"
-	// 'fsck' found errors and corrected them
-	fsckErrorsCorrected = 1
-	// 'fsck' found errors but exited without correcting them
-	fsckErrorsUncorrected = 4
-	defaultMountCommand   = "mount"
-)
-
 type GCENodeServer struct {
 	Driver  *GCEDriver
-	Mounter mount.SafeFormatAndMount
+	Mounter mountmanager.MountManager
 	// TODO: Only lock mutually exclusive calls and make locking more fine grained
 	mux sync.Mutex
 }
@@ -89,7 +61,7 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		return nil, status.Error(codes.InvalidArgument, "NodePublishVolume Volume Capability must be provided")
 	}
 
-	notMnt, err := ns.Mounter.IsLikelyNotMountPoint(targetPath)
+	notMnt, err := ns.Mounter.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
 	if err != nil && !os.IsNotExist(err) {
 		glog.Errorf("cannot validate mount point: %s %v", targetPath, err)
 		return nil, err
@@ -99,7 +71,7 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		return nil, nil
 	}
 
-	if err := ns.Mounter.MakeDir(targetPath); err != nil {
+	if err := ns.Mounter.GetSafeMounter().Interface.MakeDir(targetPath); err != nil {
 		glog.Errorf("mkdir failed on disk %s (%v)", targetPath, err)
 		return nil, err
 	}
@@ -110,19 +82,19 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		options = append(options, "ro")
 	}
 
-	err = ns.Mounter.Mount(stagingTargetPath, targetPath, "ext4", options)
+	err = ns.Mounter.GetSafeMounter().Interface.Mount(stagingTargetPath, targetPath, "ext4", options)
 	if err != nil {
-		notMnt, mntErr := ns.Mounter.IsLikelyNotMountPoint(targetPath)
+		notMnt, mntErr := ns.Mounter.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
 		if mntErr != nil {
 			glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr)
 			return nil, status.Error(codes.Internal, fmt.Sprintf("TODO: %v", err))
 		}
 		if !notMnt {
-			if mntErr = ns.Mounter.Unmount(targetPath); mntErr != nil {
+			if mntErr = ns.Mounter.GetSafeMounter().Interface.Unmount(targetPath); mntErr != nil {
 				glog.Errorf("Failed to unmount: %v", mntErr)
 				return nil, status.Error(codes.Internal, fmt.Sprintf("TODO: %v", err))
 			}
-			notMnt, mntErr := ns.Mounter.IsLikelyNotMountPoint(targetPath)
+			notMnt, mntErr := ns.Mounter.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
 			if mntErr != nil {
 				glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr)
 				return nil, status.Error(codes.Internal, fmt.Sprintf("TODO: %v", err))
@@ -158,7 +130,7 @@ func (ns *GCENodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeU
 
 	// TODO: Check volume still exists
 
-	err := ns.Mounter.Unmount(targetPath)
+	err := ns.Mounter.GetSafeMounter().Interface.Unmount(targetPath)
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("Unmount failed: %v\nUnmounting arguments: %s\n", err, targetPath))
 	}
@@ -196,8 +168,8 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	// TODO: Get real partitions
 	partition := ""
 
-	devicePaths := getDiskByIdPaths(volumeName, partition)
-	devicePath, err := verifyDevicePath(devicePaths)
+	devicePaths := ns.Mounter.GetDiskByIdPaths(volumeName, partition)
+	devicePath, err := ns.Mounter.VerifyDevicePath(devicePaths)
 
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("Error verifying GCE PD (%q) is attached: %v", volumeName, err))
@@ -209,10 +181,10 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	glog.Infof("Successfully found attached GCE PD %q at device path %s.", volumeName, devicePath)
 
 	// Part 2: Check if mount already exists at targetpath
-	notMnt, err := ns.Mounter.IsLikelyNotMountPoint(stagingTargetPath)
+	notMnt, err := ns.Mounter.GetSafeMounter().Interface.IsLikelyNotMountPoint(stagingTargetPath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			if err := ns.Mounter.MakeDir(stagingTargetPath); err != nil {
+			if err := ns.Mounter.GetSafeMounter().Interface.MakeDir(stagingTargetPath); err != nil {
 				return nil, status.Error(codes.Internal, fmt.Sprintf("Failed to create directory (%q): %v", stagingTargetPath, err))
 			}
 			notMnt = true
@@ -246,7 +218,7 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 		return nil, status.Error(codes.Unimplemented, fmt.Sprintf("Block volume support is not yet implemented"))
 	}
 
-	err = ns.Mounter.FormatAndMount(devicePath, stagingTargetPath, fstype, options)
+	err = ns.Mounter.GetSafeMounter().FormatAndMount(devicePath, stagingTargetPath, fstype, options)
 	if err != nil {
 		return nil, status.Error(codes.Internal,
 			fmt.Sprintf("Failed to format and mount device from (%q) to (%q) with fstype (%q) and options (%q): %v",
@@ -270,7 +242,7 @@ func (ns *GCENodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUns
 		return nil, status.Error(codes.InvalidArgument, "NodeUnstageVolume Staging Target Path must be provided")
 	}
 
-	err := ns.Mounter.Unmount(stagingTargetPath)
+	err := ns.Mounter.GetSafeMounter().Interface.Unmount(stagingTargetPath)
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("NodeUnstageVolume failed to unmount at path %s: %v", stagingTargetPath, err))
 	}
@@ -305,107 +277,3 @@ func (ns *GCENodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRe
 	}
 	return resp, nil
 }
-
-// Returns list of all /dev/disk/by-id/* paths for given PD.
-func getDiskByIdPaths(pdName string, partition string) []string {
-	devicePaths := []string{
-		path.Join(diskByIdPath, diskGooglePrefix+pdName),
-		path.Join(diskByIdPath, diskScsiGooglePrefix+pdName),
-		path.Join(diskByHostIdPath, diskScsiGooglePrefix+pdName),
-		path.Join(diskByHostIdPath, diskScsiGooglePrefix+pdName),
-	}
-
-	if partition != "" {
-		for i, path := range devicePaths {
-			devicePaths[i] = path + diskPartitionSuffix + partition
-		}
-	}
-
-	return devicePaths
-}
-
-// Returns the first path that exists, or empty string if none exist.
-func verifyDevicePath(devicePaths []string) (string, error) {
-	sdBefore, err := filepath.Glob(diskSDPattern)
-	if err != nil {
-		// Seeing this error means that the diskSDPattern is malformed.
-		glog.Errorf("Error filepath.Glob(\"%s\"): %v\r\n", diskSDPattern, err)
-	}
-	sdBeforeSet := sets.NewString(sdBefore...)
-	// TODO: Remove this udevadm stuff. Not applicable because can't access /dev/sd* from container
-	if err := udevadmChangeToNewDrives(sdBeforeSet); err != nil {
-		// udevadm errors should not block disk detachment, log and continue
-		glog.Errorf("udevadmChangeToNewDrives failed with: %v", err)
-	}
-
-	for _, path := range devicePaths {
-		if pathExists, err := pathExists(path); err != nil {
-			return "", fmt.Errorf("Error checking if path exists: %v", err)
-		} else if pathExists {
-			return path, nil
-		}
-	}
-
-	return "", nil
-}
-
-// Triggers the application of udev rules by calling "udevadm trigger
-// --action=change" for newly created "/dev/sd*" drives (exist only in
-// after set). This is workaround for Issue #7972. Once the underlying
-// issue has been resolved, this may be removed.
-func udevadmChangeToNewDrives(sdBeforeSet sets.String) error {
-	sdAfter, err := filepath.Glob(diskSDPattern)
-	if err != nil {
-		return fmt.Errorf("Error filepath.Glob(\"%s\"): %v\r\n", diskSDPattern, err)
-	}
-
-	for _, sd := range sdAfter {
-		if !sdBeforeSet.Has(sd) {
-			return udevadmChangeToDrive(sd)
-		}
-	}
-
-	return nil
-}
-
-// Calls "udevadm trigger --action=change" on the specified drive.
-// drivePath must be the block device path to trigger on, in the format "/dev/sd*", or a symlink to it.
-// This is workaround for Issue #7972. Once the underlying issue has been resolved, this may be removed.
-func udevadmChangeToDrive(drivePath string) error {
-	glog.V(5).Infof("udevadmChangeToDrive: drive=%q", drivePath)
-
-	// Evaluate symlink, if any
-	drive, err := filepath.EvalSymlinks(drivePath)
-	if err != nil {
-		return fmt.Errorf("udevadmChangeToDrive: filepath.EvalSymlinks(%q) failed with %v.", drivePath, err)
-	}
-	glog.V(5).Infof("udevadmChangeToDrive: symlink path is %q", drive)
-
-	// Check to make sure input is "/dev/sd*"
-	if !strings.Contains(drive, diskSDPath) {
-		return fmt.Errorf("udevadmChangeToDrive: expected input in the form \"%s\" but drive is %q.", diskSDPattern, drive)
-	}
-
-	// Call "udevadm trigger --action=change --property-match=DEVNAME=/dev/sd..."
-	_, err = exec.New().Command(
-		"udevadm",
-		"trigger",
-		"--action=change",
-		fmt.Sprintf("--property-match=DEVNAME=%s", drive)).CombinedOutput()
-	if err != nil {
-		return fmt.Errorf("udevadmChangeToDrive: udevadm trigger failed for drive %q with %v.", drive, err)
-	}
-	return nil
-}
-
-// PathExists returns true if the specified path exists.
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	if err == nil {
-		return true, nil
-	} else if os.IsNotExist(err) {
-		return false, nil
-	} else {
-		return false, err
-	}
-}