Refactor out mountmanager middle-man object. Cleaned up interfaces to split mounting and device management

Author: davidz627

cmd/main.go

@@ -56,9 +56,10 @@ func handle() {
 		glog.Fatalf("Failed to get cloud provider: %v", err)
 	}
 
-	mounter := mountmanager.NewMounter()
+	mounter := mountmanager.NewSafeMounter()
+	deviceUtils := mountmanager.NewDeviceUtils()
 
-	err = gceDriver.SetupGCEDriver(cloudProvider, mounter, *driverName, *nodeID, vendorVersion)
+	err = gceDriver.SetupGCEDriver(cloudProvider, mounter, deviceUtils, *driverName, *nodeID, vendorVersion)
 	if err != nil {
 		glog.Fatalf("Failed to initialize GCE CSI Driver: %v", err)
 	}

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

@@ -147,7 +147,7 @@ func TestCreateVolumeArguments(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Failed to create fake cloud provider: %v", err)
 		}
-		err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, driver, node, "vendor-version")
+		err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, nil, driver, node, "vendor-version")
 		if err != nil {
 			t.Fatalf("Failed to setup GCE Driver: %v", err)
 		}

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

@@ -21,6 +21,7 @@ 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"
 )
@@ -43,7 +44,8 @@ func GetGCEDriver() *GCEDriver {
 	return &GCEDriver{}
 }
 
-func (gceDriver *GCEDriver) SetupGCEDriver(cloudProvider gce.GCECompute, mounter mountmanager.MountManager, name, nodeID, vendorVersion string) error {
+func (gceDriver *GCEDriver) SetupGCEDriver(cloudProvider gce.GCECompute, mounter *mount.SafeFormatAndMount,
+	deviceUtils mountmanager.DeviceUtils, name, nodeID, vendorVersion string) error {
 	if name == "" {
 		return fmt.Errorf("Driver name missing")
 	}
@@ -73,7 +75,7 @@ func (gceDriver *GCEDriver) SetupGCEDriver(cloudProvider gce.GCECompute, mounter
 
 	// Set up RPC Servers
 	gceDriver.ids = NewIdentityServer(gceDriver)
-	gceDriver.ns = NewNodeServer(gceDriver, mounter)
+	gceDriver.ns = NewNodeServer(gceDriver, mounter, deviceUtils)
 	gceDriver.cs = NewControllerServer(gceDriver, cloudProvider)
 
 	return nil
@@ -129,10 +131,11 @@ func NewIdentityServer(gceDriver *GCEDriver) *GCEIdentityServer {
 	}
 }
 
-func NewNodeServer(gceDriver *GCEDriver, mounter mountmanager.MountManager) *GCENodeServer {
+func NewNodeServer(gceDriver *GCEDriver, mounter *mount.SafeFormatAndMount, deviceUtils mountmanager.DeviceUtils) *GCENodeServer {
 	return &GCENodeServer{
-		Driver:  gceDriver,
-		Mounter: mounter,
+		Driver:      gceDriver,
+		Mounter:     mounter,
+		DeviceUtils: deviceUtils,
 	}
 }
 

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

@@ -27,7 +27,7 @@ func initGCEDriver(t *testing.T) *GCEDriver {
 	if err != nil {
 		t.Fatalf("Failed to create fake cloud provider: %v", err)
 	}
-	err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, driver, node, vendorVersion)
+	err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, nil, driver, node, vendorVersion)
 	if err != nil {
 		t.Fatalf("Failed to setup GCE Driver: %v", err)
 	}

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

@@ -24,7 +24,7 @@ import (
 func TestGetPluginInfo(t *testing.T) {
 	vendorVersion := "test-vendor"
 	gceDriver := GetGCEDriver()
-	err := gceDriver.SetupGCEDriver(nil, nil, driver, node, vendorVersion)
+	err := gceDriver.SetupGCEDriver(nil, nil, nil, driver, node, vendorVersion)
 	if err != nil {
 		t.Fatalf("Failed to setup GCE Driver: %v", err)
 	}
@@ -46,7 +46,7 @@ func TestGetPluginInfo(t *testing.T) {
 
 func TestGetPluginCapabilities(t *testing.T) {
 	gceDriver := GetGCEDriver()
-	err := gceDriver.SetupGCEDriver(nil, nil, driver, node, "test-vendor")
+	err := gceDriver.SetupGCEDriver(nil, nil, nil, driver, node, "test-vendor")
 	if err != nil {
 		t.Fatalf("Failed to setup GCE Driver: %v", err)
 	}
@@ -67,7 +67,7 @@ func TestGetPluginCapabilities(t *testing.T) {
 
 func TestProbe(t *testing.T) {
 	gceDriver := GetGCEDriver()
-	err := gceDriver.SetupGCEDriver(nil, nil, driver, node, "test-vendor")
+	err := gceDriver.SetupGCEDriver(nil, nil, nil, driver, node, "test-vendor")
 	if err != nil {
 		t.Fatalf("Failed to setup GCE Driver: %v", err)
 	}

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

@@ -24,13 +24,15 @@ import (
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"k8s.io/kubernetes/pkg/util/mount"
 	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"
 )
 
 type GCENodeServer struct {
-	Driver  *GCEDriver
-	Mounter mountmanager.MountManager
+	Driver      *GCEDriver
+	Mounter     *mount.SafeFormatAndMount
+	DeviceUtils mountmanager.DeviceUtils
 	// TODO: Only lock mutually exclusive calls and make locking more fine grained
 	mux sync.Mutex
 }
@@ -61,7 +63,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.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
+	notMnt, err := ns.Mounter.Interface.IsLikelyNotMountPoint(targetPath)
 	if err != nil && !os.IsNotExist(err) {
 		glog.Errorf("cannot validate mount point: %s %v", targetPath, err)
 		return nil, err
@@ -71,7 +73,7 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		return nil, nil
 	}
 
-	if err := ns.Mounter.GetSafeMounter().Interface.MakeDir(targetPath); err != nil {
+	if err := ns.Mounter.Interface.MakeDir(targetPath); err != nil {
 		glog.Errorf("mkdir failed on disk %s (%v)", targetPath, err)
 		return nil, err
 	}
@@ -82,19 +84,19 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		options = append(options, "ro")
 	}
 
-	err = ns.Mounter.GetSafeMounter().Interface.Mount(stagingTargetPath, targetPath, "ext4", options)
+	err = ns.Mounter.Interface.Mount(stagingTargetPath, targetPath, "ext4", options)
 	if err != nil {
-		notMnt, mntErr := ns.Mounter.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
+		notMnt, mntErr := ns.Mounter.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.GetSafeMounter().Interface.Unmount(targetPath); mntErr != nil {
+			if mntErr = ns.Mounter.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.GetSafeMounter().Interface.IsLikelyNotMountPoint(targetPath)
+			notMnt, mntErr := ns.Mounter.Interface.IsLikelyNotMountPoint(targetPath)
 			if mntErr != nil {
 				glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr)
 				return nil, status.Error(codes.Internal, fmt.Sprintf("TODO: %v", err))
@@ -130,7 +132,7 @@ func (ns *GCENodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeU
 
 	// TODO: Check volume still exists
 
-	err := ns.Mounter.GetSafeMounter().Interface.Unmount(targetPath)
+	err := ns.Mounter.Interface.Unmount(targetPath)
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("Unmount failed: %v\nUnmounting arguments: %s\n", err, targetPath))
 	}
@@ -168,8 +170,8 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	// TODO: Get real partitions
 	partition := ""
 
-	devicePaths := ns.Mounter.GetDiskByIdPaths(volumeName, partition)
-	devicePath, err := ns.Mounter.VerifyDevicePath(devicePaths)
+	devicePaths := ns.DeviceUtils.GetDiskByIdPaths(volumeName, partition)
+	devicePath, err := ns.DeviceUtils.VerifyDevicePath(devicePaths)
 
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("Error verifying GCE PD (%q) is attached: %v", volumeName, err))
@@ -181,10 +183,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.GetSafeMounter().Interface.IsLikelyNotMountPoint(stagingTargetPath)
+	notMnt, err := ns.Mounter.Interface.IsLikelyNotMountPoint(stagingTargetPath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			if err := ns.Mounter.GetSafeMounter().Interface.MakeDir(stagingTargetPath); err != nil {
+			if err := ns.Mounter.Interface.MakeDir(stagingTargetPath); err != nil {
 				return nil, status.Error(codes.Internal, fmt.Sprintf("Failed to create directory (%q): %v", stagingTargetPath, err))
 			}
 			notMnt = true
@@ -218,7 +220,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.GetSafeMounter().FormatAndMount(devicePath, stagingTargetPath, fstype, options)
+	err = ns.Mounter.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",
@@ -242,7 +244,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.GetSafeMounter().Interface.Unmount(stagingTargetPath)
+	err := ns.Mounter.Interface.Unmount(stagingTargetPath)
 	if err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("NodeUnstageVolume failed to unmount at path %s: %v", stagingTargetPath, err))
 	}

pkg/mount-manager/mount-manager.go renamed to pkg/mount-manager/device-utils.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/golang/glog"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/utils/exec"
 )
 
@@ -50,41 +49,24 @@ const (
 	defaultMountCommand   = "mount"
 )
 
-// TODO: Info
-type MountManager interface {
-	// TODO: Info
-	GetSafeMounter() *mount.SafeFormatAndMount
-	// TODO: Info
+type DeviceUtils interface {
 	GetDiskByIdPaths(pdName string, partition string) []string
 	// TODO: Info
 	VerifyDevicePath(devicePaths []string) (string, error)
 }
 
 // TODO: Info
-type GCEMounter struct {
-	// TODO: Info
-	SafeMounter *mount.SafeFormatAndMount
+type deviceUtils struct {
 }
 
-var _ MountManager = &GCEMounter{}
-
-func NewMounter() *GCEMounter {
-	realMounter := mount.New("")
-	realExec := mount.NewOsExec()
-	return &GCEMounter{
-		SafeMounter: &mount.SafeFormatAndMount{
-			Interface: realMounter,
-			Exec:      realExec,
-		},
-	}
-}
+var _ DeviceUtils = &deviceUtils{}
 
-func (m *GCEMounter) GetSafeMounter() *mount.SafeFormatAndMount {
-	return m.SafeMounter
+func NewDeviceUtils() *deviceUtils {
+	return &deviceUtils{}
 }
 
 // Returns list of all /dev/disk/by-id/* paths for given PD.
-func (m *GCEMounter) GetDiskByIdPaths(pdName string, partition string) []string {
+func (m *deviceUtils) GetDiskByIdPaths(pdName string, partition string) []string {
 	devicePaths := []string{
 		path.Join(diskByIdPath, diskGooglePrefix+pdName),
 		path.Join(diskByIdPath, diskScsiGooglePrefix+pdName),
@@ -102,7 +84,7 @@ func (m *GCEMounter) GetDiskByIdPaths(pdName string, partition string) []string
 }
 
 // Returns the first path that exists, or empty string if none exist.
-func (m *GCEMounter) VerifyDevicePath(devicePaths []string) (string, error) {
+func (m *deviceUtils) VerifyDevicePath(devicePaths []string) (string, error) {
 	sdBefore, err := filepath.Glob(diskSDPattern)
 	if err != nil {
 		// Seeing this error means that the diskSDPattern is malformed.

pkg/mount-manager/fake-device-utils.go

@@ -0,0 +1,36 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package mountmanager
+
+type fakeDeviceUtils struct {
+}
+
+var _ DeviceUtils = &fakeDeviceUtils{}
+
+func NewFakeDeviceUtils() *fakeDeviceUtils {
+	return &fakeDeviceUtils{}
+}
+
+// Returns list of all /dev/disk/by-id/* paths for given PD.
+func (m *fakeDeviceUtils) GetDiskByIdPaths(pdName string, partition string) []string {
+	// Don't need to implement this in the fake because we have no actual device paths
+	return nil
+}
+
+// Returns the first path that exists, or empty string if none exist.
+func (m *fakeDeviceUtils) VerifyDevicePath(devicePaths []string) (string, error) {
+	// Return any random device path to use as mount source
+	return "/dev/disk/fake-path", nil
+}

pkg/mount-manager/fake.go renamed to pkg/mount-manager/fake-safe-mounter.go

@@ -16,14 +16,7 @@ package mountmanager
 
 import "k8s.io/kubernetes/pkg/util/mount"
 
-type FakeGCEMounter struct {
-	// TODO: Info
-	SafeMounter *mount.SafeFormatAndMount
-}
-
-var _ MountManager = &FakeGCEMounter{}
-
-func FakeMounter(mountErrs []error) *FakeGCEMounter {
+func NewFakeSafeMounter() *mount.SafeFormatAndMount {
 	execCallback := func(cmd string, args ...string) ([]byte, error) {
 		return nil, nil
 		// TODO: Fill out exec callback for errors
@@ -47,26 +40,8 @@ func FakeMounter(mountErrs []error) *FakeGCEMounter {
 	}
 	fakeMounter := &mount.FakeMounter{MountPoints: []mount.MountPoint{}, Log: []mount.FakeAction{}}
 	fakeExec := mount.NewFakeExec(execCallback)
-	return &FakeGCEMounter{
-		SafeMounter: &mount.SafeFormatAndMount{
-			Interface: fakeMounter,
-			Exec:      fakeExec,
-		},
+	return &mount.SafeFormatAndMount{
+		Interface: fakeMounter,
+		Exec:      fakeExec,
 	}
 }
-
-func (m *FakeGCEMounter) GetSafeMounter() *mount.SafeFormatAndMount {
-	return m.SafeMounter
-}
-
-// Returns list of all /dev/disk/by-id/* paths for given PD.
-func (m *FakeGCEMounter) GetDiskByIdPaths(pdName string, partition string) []string {
-	// Don't need to implement this in the fake because we have no actual device paths
-	return nil
-}
-
-// Returns the first path that exists, or empty string if none exist.
-func (m *FakeGCEMounter) VerifyDevicePath(devicePaths []string) (string, error) {
-	// Return any random device path to use as mount source
-	return "/dev/disk/fake-path", nil
-}

pkg/mount-manager/safe-mounter.go

@@ -0,0 +1,27 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package mountmanager
+
+import "k8s.io/kubernetes/pkg/util/mount"
+
+func NewSafeMounter() *mount.SafeFormatAndMount {
+	realMounter := mount.New("")
+	realExec := mount.NewOsExec()
+	return &mount.SafeFormatAndMount{
+		Interface: realMounter,
+		Exec:      realExec,
+	}
+
+}