Merge pull request #6400 from ddysher/nfs-volume-mounter

Implement Mount interface using mount(8) and umount(8)

pkg/util/mount/fake.go

@@ -38,13 +38,13 @@ func (f *FakeMounter) ResetLog() {
 	f.Log = []FakeAction{}
 }
 
-func (f *FakeMounter) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+func (f *FakeMounter) Mount(source string, target string, fstype string, options []string) error {
 	f.MountPoints = append(f.MountPoints, MountPoint{Device: source, Path: target, Type: fstype})
 	f.Log = append(f.Log, FakeAction{Action: FakeActionMount, Target: target, Source: source, FSType: fstype})
 	return nil
 }
 
-func (f *FakeMounter) Unmount(target string, flags int) error {
+func (f *FakeMounter) Unmount(target string) error {
 	newMountpoints := []MountPoint{}
 	for _, mp := range f.MountPoints {
 		if mp.Path != target {

pkg/util/mount/mount.go

@@ -18,14 +18,11 @@ limitations under the License.
 // an alternate platform, we will need to abstract further.
 package mount
 
-// Each supported platform must define the following flags:
-//   - FlagBind: specifies a bind mount
-//   - FlagReadOnly: the mount will be read-only
 type Interface interface {
-	// Mount wraps syscall.Mount().
+	// Mount mounts source to target as fstype with given options.
-	Mount(source string, target string, fstype string, flags uintptr, data string) error
+	Mount(source string, target string, fstype string, options []string) error
-	// Umount wraps syscall.Mount().
+	// Unmount unmounts given target.
-	Unmount(target string, flags int) error
+	Unmount(target string) error
 	// List returns a list of all mounted filesystems.  This can be large.
 	// On some platforms, reading mounts is not guaranteed consistent (i.e.
 	// it could change between chunked reads). This is guaranteed to be

pkg/util/mount/mount_linux.go

@@ -24,6 +24,7 @@ import (
 	"hash/adler32"
 	"io"
 	"os"
+	"os/exec"
 	"strconv"
 	"strings"
 	"syscall"
@@ -31,28 +32,91 @@ import (
 	"github.com/golang/glog"
 )
 
-const FlagBind = syscall.MS_BIND
+const (
-const FlagReadOnly = syscall.MS_RDONLY
+	// 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
+)
 
 // Mounter implements mount.Interface for linux platform.
 type Mounter struct{}
 
-// Mount wraps syscall.Mount()
+// Mount mounts source to target as fstype with given options. 'source' and 'fstype' must
-func (mounter *Mounter) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+// be an emtpy string in case it's not required, e.g. for remount, or for auto filesystem
-	glog.V(5).Infof("Mounting %s %s %s %d %s", source, target, fstype, flags, data)
+// type, where kernel handles fs type for you. The mount 'options' is a list of options,
-	return syscall.Mount(source, target, fstype, flags, data)
+// currently come from mount(8), e.g. "ro", "remount", "bind", etc. If no more option is
+// required, call Mount with an empty string list or nil.
+func (mounter *Mounter) Mount(source string, target string, fstype string, options []string) error {
+	// The remount options to use in case of bind mount, due to the fact that bind mount doesn't
+	// respect mount options.  The list equals:
+	//   options - 'bind' + 'remount' (no duplicate)
+	bindRemountOpts := []string{"remount"}
+	bind := false
+
+	if len(options) != 0 {
+		for _, option := range options {
+			switch option {
+			case "bind":
+				bind = true
+				break
+			case "remount":
+				break
+			default:
+				bindRemountOpts = append(bindRemountOpts, option)
+			}
+		}
 	}
 
-// Unmount wraps syscall.Unmount()
+	if bind {
-func (mounter *Mounter) Unmount(target string, flags int) error {
+		err := doMount(source, target, fstype, []string{"bind"})
-	return syscall.Unmount(target, flags)
+		if err != nil {
+			return err
+		}
+		return doMount(source, target, fstype, bindRemountOpts)
+	} else {
+		return doMount(source, target, fstype, options)
+	}
 }
 
-// How many times to retry for a consistent read of /proc/mounts.
+func doMount(source string, target string, fstype string, options []string) error {
-const maxListTries = 3
+	glog.V(5).Infof("Mounting %s %s %s %v", source, target, fstype, options)
+	// Build mount command as follows:
+	//   mount [-t $fstype] [-o $options] [$source] $target
+	mountArgs := []string{}
+	if len(fstype) > 0 {
+		mountArgs = append(mountArgs, "-t", fstype)
+	}
+	if len(options) > 0 {
+		mountArgs = append(mountArgs, "-o", strings.Join(options, ","))
+	}
+	if len(source) > 0 {
+		mountArgs = append(mountArgs, source)
+	}
+	mountArgs = append(mountArgs, target)
+	command := exec.Command("mount", mountArgs...)
+	output, err := command.CombinedOutput()
+	if err != nil {
+		glog.Errorf("Mount failed: %v\nMounting arguments: %s %s %s %v\nOutput: %s\n",
+			err, source, target, fstype, options, string(output))
+	}
+	return err
+}
+
+// Unmount unmounts target with given options.
+func (mounter *Mounter) Unmount(target string) error {
+	glog.V(5).Infof("Unmounting %s %v")
+	command := exec.Command("umount", target)
+	output, err := command.CombinedOutput()
+	if err != nil {
+		glog.Errorf("Unmount failed: %v\nUnmounting arguments: %s\nOutput: %s\n", err, target, string(output))
+		return err
+	}
+	return nil
+}
 
 // List returns a list of all mounted filesystems.
-func (*Mounter) List() ([]MountPoint, error) {
+func (mounter *Mounter) List() ([]MountPoint, error) {
 	hash1, err := readProcMounts(nil)
 	if err != nil {
 		return nil, err
@@ -89,9 +153,6 @@ func (mounter *Mounter) IsMountPoint(file string) (bool, error) {
 	return stat.Sys().(*syscall.Stat_t).Dev != rootStat.Sys().(*syscall.Stat_t).Dev, nil
 }
 
-// As per the fstab man page.
-const expectedNumFieldsPerLine = 6
-
 // readProcMounts reads /proc/mounts and produces a hash of the contents.  If the out
 // argument is not nil, this fills it with MountPoint structs.
 func readProcMounts(out *[]MountPoint) (uint32, error) {

pkg/util/mount/mount_unsupported.go

@@ -18,16 +18,13 @@ limitations under the License.
 
 package mount
 
-const FlagBind = 0
-const FlagReadOnly = 0
-
 type Mounter struct{}
 
-func (mounter *Mounter) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+func (mounter *Mounter) Mount(source string, target string, fstype string, options []string) error {
 	return nil
 }
 
-func (mounter *Mounter) Unmount(target string, flags int) error {
+func (mounter *Mounter) Unmount(target string) error {
 	return nil
 }
 

pkg/volume/aws_ebs/aws_ebs.go

@@ -192,11 +192,6 @@ func (pd *awsElasticBlockStore) SetUpAt(dir string) error {
 		return err
 	}
 
-	flags := uintptr(0)
-	if pd.readOnly {
-		flags = mount.FlagReadOnly
-	}
-
 	if err := os.MkdirAll(dir, 0750); err != nil {
 		// TODO: we should really eject the attach/detach out into its own control loop.
 		detachDiskLogError(pd)
@@ -204,7 +199,11 @@ func (pd *awsElasticBlockStore) SetUpAt(dir string) error {
 	}
 
 	// Perform a bind mount to the full path to allow duplicate mounts of the same PD.
-	err = pd.mounter.Mount(globalPDPath, dir, "", mount.FlagBind|flags, "")
+	options := []string{"bind"}
+	if pd.readOnly {
+		options = append(options, "ro")
+	}
+	err = pd.mounter.Mount(globalPDPath, dir, "", options)
 	if err != nil {
 		mountpoint, mntErr := pd.mounter.IsMountPoint(dir)
 		if mntErr != nil {
@@ -212,7 +211,7 @@ func (pd *awsElasticBlockStore) SetUpAt(dir string) error {
 			return err
 		}
 		if mountpoint {
-			if mntErr = pd.mounter.Unmount(dir, 0); mntErr != nil {
+			if mntErr = pd.mounter.Unmount(dir); mntErr != nil {
 				glog.Errorf("Failed to unmount: %v", mntErr)
 				return err
 			}
@@ -291,7 +290,7 @@ func (pd *awsElasticBlockStore) TearDownAt(dir string) error {
 		return err
 	}
 	// Unmount the bind-mount inside this pod
-	if err := pd.mounter.Unmount(dir, 0); err != nil {
+	if err := pd.mounter.Unmount(dir); err != nil {
 		glog.V(2).Info("Error unmounting dir ", dir, ": ", err)
 		return err
 	}

pkg/volume/aws_ebs/aws_util.go

@@ -36,10 +36,6 @@ func (util *AWSDiskUtil) AttachAndMountDisk(pd *awsElasticBlockStore, globalPDPa
 	if err != nil {
 		return err
 	}
-	flags := uintptr(0)
-	if pd.readOnly {
-		flags = mount.FlagReadOnly
-	}
 	devicePath, err := volumes.AttachDisk("", pd.volumeID, pd.readOnly)
 	if err != nil {
 		return err
@@ -76,8 +72,12 @@ func (util *AWSDiskUtil) AttachAndMountDisk(pd *awsElasticBlockStore, globalPDPa
 			return err
 		}
 	}
+	options := []string{}
+	if pd.readOnly {
+		options = append(options, "ro")
+	}
 	if !mountpoint {
-		err = pd.diskMounter.Mount(devicePath, globalPDPath, pd.fsType, flags, "")
+		err = pd.diskMounter.Mount(devicePath, globalPDPath, pd.fsType, options)
 		if err != nil {
 			os.Remove(globalPDPath)
 			return err
@@ -90,7 +90,7 @@ func (util *AWSDiskUtil) AttachAndMountDisk(pd *awsElasticBlockStore, globalPDPa
 func (util *AWSDiskUtil) DetachDisk(pd *awsElasticBlockStore) error {
 	// Unmount the global PD mount, which should be the only one.
 	globalPDPath := makeGlobalPDPath(pd.plugin.host, pd.volumeID)
-	if err := pd.mounter.Unmount(globalPDPath, 0); err != nil {
+	if err := pd.mounter.Unmount(globalPDPath); err != nil {
 		glog.V(2).Info("Error unmount dir ", globalPDPath, ": ", err)
 		return err
 	}
@@ -121,18 +121,21 @@ type awsSafeFormatAndMount struct {
 }
 
 // uses /usr/share/google/safe_format_and_mount to optionally mount, and format a disk
-func (mounter *awsSafeFormatAndMount) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+func (mounter *awsSafeFormatAndMount) Mount(source string, target string, fstype string, options []string) error {
 	// Don't attempt to format if mounting as readonly. Go straight to mounting.
-	if (flags & mount.FlagReadOnly) != 0 {
+	// Don't attempt to format if mounting as readonly. Go straight to mounting.
-		return mounter.Interface.Mount(source, target, fstype, flags, data)
+	for _, option := range options {
+		if option == "ro" {
+			return mounter.Interface.Mount(source, target, fstype, options)
+		}
 	}
 	args := []string{}
 	// ext4 is the default for safe_format_and_mount
 	if len(fstype) > 0 && fstype != "ext4" {
 		args = append(args, "-m", fmt.Sprintf("mkfs.%s", fstype))
 	}
+	args = append(args, options...)
 	args = append(args, source, target)
-	// TODO: Accept other options here?
 	glog.V(5).Infof("exec-ing: /usr/share/google/safe_format_and_mount %v", args)
 	cmd := mounter.runner.Command("/usr/share/google/safe_format_and_mount", args...)
 	dataOut, err := cmd.CombinedOutput()

pkg/volume/aws_ebs/aws_util_test.go

@@ -64,7 +64,7 @@ func TestSafeFormatAndMount(t *testing.T) {
 			runner: &fake,
 		}
 
-		err := mounter.Mount("/dev/foo", "/mnt/bar", test.fstype, 0, "")
+		err := mounter.Mount("/dev/foo", "/mnt/bar", test.fstype, nil)
 		if test.err == nil && err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}

pkg/volume/empty_dir/empty_dir.go

@@ -197,6 +197,7 @@ func (ed *emptyDir) setupTmpfs(dir string) error {
 	if isMnt && medium == mediumMemory {
 		return nil
 	}
+
 	// By default a tmpfs mount will receive a different SELinux context
 	// from that of the Kubelet root directory which is not readable from
 	// the SELinux context of a docker container.
@@ -206,15 +207,15 @@ func (ed *emptyDir) setupTmpfs(dir string) error {
 	// the container.
 	opts := ed.getTmpfsMountOptions()
 	glog.V(3).Infof("pod %v: mounting tmpfs for volume %v with opts %v", ed.podUID, ed.volName, opts)
-	return ed.mounter.Mount("tmpfs", dir, "tmpfs", 0, opts)
+	return ed.mounter.Mount("tmpfs", dir, "tmpfs", opts)
 }
 
-func (ed *emptyDir) getTmpfsMountOptions() string {
+func (ed *emptyDir) getTmpfsMountOptions() []string {
 	if ed.rootContext == "" {
-		return ""
+		return []string{""}
 	}
 
-	return fmt.Sprintf("rootcontext=\"%v\"", ed.rootContext)
+	return []string{fmt.Sprintf("rootcontext=\"%v\"", ed.rootContext)}
 }
 
 func (ed *emptyDir) GetPath() string {
@@ -261,7 +262,7 @@ func (ed *emptyDir) teardownTmpfs(dir string) error {
 	if ed.mounter == nil {
 		return fmt.Errorf("memory storage requested, but mounter is nil")
 	}
-	if err := ed.mounter.Unmount(dir, 0); err != nil {
+	if err := ed.mounter.Unmount(dir); err != nil {
 		return err
 	}
 	if err := os.RemoveAll(dir); err != nil {

pkg/volume/gce_pd/gce_pd.go

@@ -201,11 +201,6 @@ func (pd *gcePersistentDisk) SetUpAt(dir string) error {
 		return err
 	}
 
-	flags := uintptr(0)
-	if pd.readOnly {
-		flags = mount.FlagReadOnly
-	}
-
 	if err := os.MkdirAll(dir, 0750); err != nil {
 		// TODO: we should really eject the attach/detach out into its own control loop.
 		detachDiskLogError(pd)
@@ -213,7 +208,11 @@ func (pd *gcePersistentDisk) SetUpAt(dir string) error {
 	}
 
 	// Perform a bind mount to the full path to allow duplicate mounts of the same PD.
-	err = pd.mounter.Mount(globalPDPath, dir, "", mount.FlagBind|flags, "")
+	options := []string{"bind"}
+	if pd.readOnly {
+		options = append(options, "ro")
+	}
+	err = pd.mounter.Mount(globalPDPath, dir, "", options)
 	if err != nil {
 		mountpoint, mntErr := pd.mounter.IsMountPoint(dir)
 		if mntErr != nil {
@@ -221,7 +220,7 @@ func (pd *gcePersistentDisk) SetUpAt(dir string) error {
 			return err
 		}
 		if mountpoint {
-			if mntErr = pd.mounter.Unmount(dir, 0); mntErr != nil {
+			if mntErr = pd.mounter.Unmount(dir); mntErr != nil {
 				glog.Errorf("Failed to unmount: %v", mntErr)
 				return err
 			}
@@ -279,7 +278,7 @@ func (pd *gcePersistentDisk) TearDownAt(dir string) error {
 		return err
 	}
 	// Unmount the bind-mount inside this pod
-	if err := pd.mounter.Unmount(dir, 0); err != nil {
+	if err := pd.mounter.Unmount(dir); err != nil {
 		return err
 	}
 	// If len(refs) is 1, then all bind mounts have been removed, and the

pkg/volume/gce_pd/gce_pd_test.go

@@ -80,7 +80,7 @@ func (fake *fakePDManager) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPat
 	fake.attachCalled = true
 	// Simulate the global mount so that the fakeMounter returns the
 	// expected number of mounts for the attached disk.
-	pd.mounter.Mount(globalPath, globalPath, pd.fsType, 0, "")
+	pd.mounter.Mount(globalPath, globalPath, pd.fsType, nil)
 	return nil
 }
 

pkg/volume/gce_pd/gce_util.go

@@ -39,10 +39,6 @@ func (util *GCEDiskUtil) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath
 	if err != nil {
 		return err
 	}
-	flags := uintptr(0)
-	if pd.readOnly {
-		flags = mount.FlagReadOnly
-	}
 	if err := gce.(*gce_cloud.GCECloud).AttachDisk(pd.pdName, pd.readOnly); err != nil {
 		return err
 	}
@@ -79,8 +75,12 @@ func (util *GCEDiskUtil) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath
 			return err
 		}
 	}
+	options := []string{}
+	if pd.readOnly {
+		options = append(options, "ro")
+	}
 	if !mountpoint {
-		err = pd.diskMounter.Mount(devicePath, globalPDPath, pd.fsType, flags, "")
+		err = pd.diskMounter.Mount(devicePath, globalPDPath, pd.fsType, options)
 		if err != nil {
 			os.Remove(globalPDPath)
 			return err
@@ -93,7 +93,7 @@ func (util *GCEDiskUtil) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath
 func (util *GCEDiskUtil) DetachDisk(pd *gcePersistentDisk) error {
 	// Unmount the global PD mount, which should be the only one.
 	globalPDPath := makeGlobalPDName(pd.plugin.host, pd.pdName)
-	if err := pd.mounter.Unmount(globalPDPath, 0); err != nil {
+	if err := pd.mounter.Unmount(globalPDPath); err != nil {
 		return err
 	}
 	if err := os.Remove(globalPDPath); err != nil {
@@ -120,18 +120,20 @@ type gceSafeFormatAndMount struct {
 }
 
 // uses /usr/share/google/safe_format_and_mount to optionally mount, and format a disk
-func (mounter *gceSafeFormatAndMount) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+func (mounter *gceSafeFormatAndMount) Mount(source string, target string, fstype string, options []string) error {
 	// Don't attempt to format if mounting as readonly. Go straight to mounting.
-	if (flags & mount.FlagReadOnly) != 0 {
+	for _, option := range options {
-		return mounter.Interface.Mount(source, target, fstype, flags, data)
+		if option == "ro" {
+			return mounter.Interface.Mount(source, target, fstype, options)
+		}
 	}
 	args := []string{}
 	// ext4 is the default for safe_format_and_mount
 	if len(fstype) > 0 && fstype != "ext4" {
 		args = append(args, "-m", fmt.Sprintf("mkfs.%s", fstype))
 	}
+	args = append(args, options...)
 	args = append(args, source, target)
-	// TODO: Accept other options here?
 	glog.V(5).Infof("exec-ing: /usr/share/google/safe_format_and_mount %v", args)
 	cmd := mounter.runner.Command("/usr/share/google/safe_format_and_mount", args...)
 	dataOut, err := cmd.CombinedOutput()

pkg/volume/gce_pd/gce_util_test.go

@@ -64,7 +64,7 @@ func TestSafeFormatAndMount(t *testing.T) {
 			runner: &fake,
 		}
 
-		err := mounter.Mount("/dev/foo", "/mnt/bar", test.fstype, 0, "")
+		err := mounter.Mount("/dev/foo", "/mnt/bar", test.fstype, nil)
 		if test.err == nil && err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}

pkg/volume/glusterfs/glusterfs.go

@@ -19,7 +19,6 @@ package glusterfs
 import (
 	"math/rand"
 	"os"
-	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/types"
@@ -129,16 +128,15 @@ func (glusterfsVolume *glusterfs) SetUpAt(dir string) error {
 	if mountpoint {
 		return nil
 	}
-	path := glusterfsVolume.path
+
 	os.MkdirAll(dir, 0750)
-	err = glusterfsVolume.execMount(glusterfsVolume.hosts, path, dir, glusterfsVolume.readonly)
+	err = glusterfsVolume.setUpAtInternal(dir)
 	if err == nil {
 		return nil
 	}
 
-	// cleanup upon failure
+	// Cleanup upon failure.
 	glusterfsVolume.cleanup(dir)
-	// return error
 	return err
 }
 
@@ -165,7 +163,7 @@ func (glusterfsVolume *glusterfs) cleanup(dir string) error {
 		return os.RemoveAll(dir)
 	}
 
-	if err := glusterfsVolume.mounter.Unmount(dir, 0); err != nil {
+	if err := glusterfsVolume.mounter.Unmount(dir); err != nil {
 		glog.Errorf("Glusterfs: Unmounting failed: %v", err)
 		return err
 	}
@@ -183,30 +181,21 @@ func (glusterfsVolume *glusterfs) cleanup(dir string) error {
 	return nil
 }
 
-func (glusterfsVolume *glusterfs) execMount(hosts *api.Endpoints, path string, mountpoint string, readonly bool) error {
+func (glusterfsVolume *glusterfs) setUpAtInternal(dir string) error {
 	var errs error
-	var command exec.Cmd
-	var mountArgs []string
-	var opt []string
 
-	// build option array
+	options := []string{}
-	if readonly == true {
+	if glusterfsVolume.readonly {
-		opt = []string{"-o", "ro"}
+		options = append(options, "ro")
-	} else {
-		opt = []string{"-o", "rw"}
 	}
 
-	l := len(hosts.Subsets)
+	l := len(glusterfsVolume.hosts.Subsets)
-	// avoid mount storm, pick a host randomly
+	// Avoid mount storm, pick a host randomly.
 	start := rand.Int() % l
-	// iterate all hosts until mount succeeds.
+	// Iterate all hosts until mount succeeds.
 	for i := start; i < start+l; i++ {
-		arg := []string{"-t", "glusterfs", hosts.Subsets[i%l].Addresses[0].IP + ":" + path, mountpoint}
+		hostIP := glusterfsVolume.hosts.Subsets[i%l].Addresses[0].IP
-		mountArgs = append(arg, opt...)
+		errs = glusterfsVolume.mounter.Mount(hostIP+":"+glusterfsVolume.path, dir, "glusterfs", options)
-		glog.V(1).Infof("Glusterfs: mount cmd: mount %v", strings.Join(mountArgs, " "))
-		command = glusterfsVolume.exe.Command("mount", mountArgs...)
-
-		_, errs = command.CombinedOutput()
 		if errs == nil {
 			return nil
 		}

pkg/volume/iscsi/disk_manager.go

@@ -55,11 +55,11 @@ func diskSetUp(manager diskManager, disk iscsiDisk, volPath string, mounter moun
 		return err
 	}
 	// Perform a bind mount to the full path to allow duplicate mounts of the same disk.
-	flags := uintptr(0)
+	options := []string{"bind"}
 	if disk.readOnly {
-		flags = mount.FlagReadOnly
+		options = append(options, "ro")
 	}
-	err = mounter.Mount(globalPDPath, volPath, "", mount.FlagBind|flags, "")
+	err = mounter.Mount(globalPDPath, volPath, "", options)
 	if err != nil {
 		glog.Errorf("failed to bind mount:%s", globalPDPath)
 		return err
@@ -83,8 +83,8 @@ func diskTearDown(manager diskManager, disk iscsiDisk, volPath string, mounter m
 		glog.Errorf("failed to get reference count %s", volPath)
 		return err
 	}
-	if err := mounter.Unmount(volPath, 0); err != nil {
+	if err := mounter.Unmount(volPath); err != nil {
-		glog.Errorf("failed to umount %s", volPath)
+		glog.Errorf("failed to unmount %s", volPath)
 		return err
 	}
 	// If len(refs) is 1, then all bind mounts have been removed, and the

pkg/volume/iscsi/iscsi.go

@@ -110,6 +110,11 @@ func (plugin *ISCSIPlugin) newCleanerInternal(volName string, podUID types.UID,
 	}, nil
 }
 
+func (plugin *ISCSIPlugin) execCommand(command string, args []string) ([]byte, error) {
+	cmd := plugin.exe.Command(command, args...)
+	return cmd.CombinedOutput()
+}
+
 type iscsiDisk struct {
 	volName  string
 	podUID   types.UID
@@ -142,15 +147,13 @@ func (iscsi *iscsiDisk) SetUpAt(dir string) error {
 		return err
 	}
 	globalPDPath := iscsi.manager.MakeGlobalPDName(*iscsi)
-	// make mountpoint rw/ro work as expected
+	var options []string
-	//FIXME revisit pkg/util/mount and ensure rw/ro is implemented as expected
-	mode := "rw"
 	if iscsi.readOnly {
-		mode = "ro"
+		options = []string{"remount", "ro"}
+	} else {
+		options = []string{"remount", "rw"}
 	}
-	iscsi.plugin.execCommand("mount", []string{"-o", "remount," + mode, globalPDPath, dir})
+	return iscsi.mounter.Mount(globalPDPath, dir, "", options)
-
-	return nil
 }
 
 // Unmounts the bind mount, and detaches the disk only if the disk
@@ -162,8 +165,3 @@ func (iscsi *iscsiDisk) TearDown() error {
 func (iscsi *iscsiDisk) TearDownAt(dir string) error {
 	return diskTearDown(iscsi.manager, *iscsi, dir, iscsi.mounter)
 }
-
-func (plugin *ISCSIPlugin) execCommand(command string, args []string) ([]byte, error) {
-	cmd := plugin.exe.Command(command, args...)
-	return cmd.CombinedOutput()
-}

pkg/volume/iscsi/iscsi_test.go

@@ -55,7 +55,7 @@ func (fake *fakeDiskManager) AttachDisk(disk iscsiDisk) error {
 	}
 	// Simulate the global mount so that the fakeMounter returns the
 	// expected number of mounts for the attached disk.
-	disk.mounter.Mount(globalPath, globalPath, disk.fsType, 0, "")
+	disk.mounter.Mount(globalPath, globalPath, disk.fsType, nil)
 
 	fake.attachCalled = true
 	return nil

pkg/volume/iscsi/iscsi_util.go

@@ -112,7 +112,7 @@ func (util *ISCSIUtil) AttachDisk(iscsi iscsiDisk) error {
 		return err
 	}
 
-	err = iscsi.mounter.Mount(devicePath, globalPDPath, iscsi.fsType, uintptr(0), "")
+	err = iscsi.mounter.Mount(devicePath, globalPDPath, iscsi.fsType, nil)
 	if err != nil {
 		glog.Errorf("iscsi: failed to mount iscsi volume %s [%s] to %s, error %v", devicePath, iscsi.fsType, globalPDPath, err)
 	}
@@ -126,8 +126,8 @@ func (util *ISCSIUtil) DetachDisk(iscsi iscsiDisk, mntPath string) error {
 		glog.Errorf("iscsi detach disk: failed to get device from mnt: %s\nError: %v", mntPath, err)
 		return err
 	}
-	if err = iscsi.mounter.Unmount(mntPath, 0); err != nil {
+	if err = iscsi.mounter.Unmount(mntPath); err != nil {
-		glog.Errorf("iscsi detach disk: failed to umount: %s\nError: %v", mntPath, err)
+		glog.Errorf("iscsi detach disk: failed to unmount: %s\nError: %v", mntPath, err)
 		return err
 	}
 	cnt--

pkg/volume/nfs/nfs.go

@@ -17,23 +17,25 @@ limitations under the License.
 package nfs
 
 import (
+	"fmt"
 	"os"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/types"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/mount"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/volume"
 	"github.com/golang/glog"
 )
 
 // This is the primary entrypoint for volume plugins.
 func ProbeVolumePlugins() []volume.VolumePlugin {
-	return []volume.VolumePlugin{&nfsPlugin{nil, newNFSMounter()}}
+	return []volume.VolumePlugin{&nfsPlugin{nil, mount.New()}}
 }
 
 type nfsPlugin struct {
 	host    volume.VolumeHost
-	mounter nfsMountInterface
+	mounter mount.Interface
 }
 
 var _ volume.VolumePlugin = &nfsPlugin{}
@@ -66,7 +68,7 @@ func (plugin *nfsPlugin) NewBuilder(spec *volume.Spec, podRef *api.ObjectReferen
 	return plugin.newBuilderInternal(spec, podRef, plugin.mounter)
 }
 
-func (plugin *nfsPlugin) newBuilderInternal(spec *volume.Spec, podRef *api.ObjectReference, mounter nfsMountInterface) (volume.Builder, error) {
+func (plugin *nfsPlugin) newBuilderInternal(spec *volume.Spec, podRef *api.ObjectReference, mounter mount.Interface) (volume.Builder, error) {
 	return &nfs{
 		volName:    spec.Name,
 		server:     spec.VolumeSource.NFS.Server,
@@ -82,7 +84,7 @@ func (plugin *nfsPlugin) NewCleaner(volName string, podUID types.UID) (volume.Cl
 	return plugin.newCleanerInternal(volName, podUID, plugin.mounter)
 }
 
-func (plugin *nfsPlugin) newCleanerInternal(volName string, podUID types.UID, mounter nfsMountInterface) (volume.Cleaner, error) {
+func (plugin *nfsPlugin) newCleanerInternal(volName string, podUID types.UID, mounter mount.Interface) (volume.Cleaner, error) {
 	return &nfs{
 		volName:    volName,
 		server:     "",
@@ -101,7 +103,7 @@ type nfs struct {
 	server     string
 	exportPath string
 	readOnly   bool
-	mounter    nfsMountInterface
+	mounter    mount.Interface
 	plugin     *nfsPlugin
 }
 
@@ -119,9 +121,13 @@ func (nfsVolume *nfs) SetUpAt(dir string) error {
 	if mountpoint {
 		return nil
 	}
-	exportDir := nfsVolume.exportPath
 	os.MkdirAll(dir, 0750)
-	err = nfsVolume.mounter.Mount(nfsVolume.server, exportDir, dir, nfsVolume.readOnly)
+	source := fmt.Sprintf("%s:%s", nfsVolume.server, nfsVolume.exportPath)
+	options := []string{}
+	if nfsVolume.readOnly {
+		options = append(options, "ro")
+	}
+	err = nfsVolume.mounter.Mount(source, dir, "nfs", options)
 	if err != nil {
 		mountpoint, mntErr := nfsVolume.mounter.IsMountPoint(dir)
 		if mntErr != nil {

pkg/volume/nfs/nfs_mount.go

@@ -1,72 +0,0 @@
-/*
-Copyright 2014 Google Inc. All rights reserved.
-
-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 nfs
-
-import (
-	"os/exec"
-
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/mount"
-	"github.com/golang/glog"
-)
-
-type nfsMountInterface interface {
-	// Mount takes an NFS host ip or hostname, a source directory (the exported directory), a target directory where the source directory will be mounted, and a boolean readOnly
-	Mount(server string, source string, target string, readOnly bool) error
-
-	// Umount wraps syscall.Mount().
-	Unmount(target string) error
-
-	List() ([]mount.MountPoint, error)
-
-	IsMountPoint(dir string) (bool, error)
-}
-
-// newNFSMounter returns an nfsMountInterface for the current system.
-func newNFSMounter() nfsMountInterface {
-	return &nfsMounter{}
-}
-
-type nfsMounter struct{}
-
-func (mounter *nfsMounter) Mount(server string, exportDir string, mountDir string, readOnly bool) error {
-	mountOptions := "rw"
-	if readOnly {
-		mountOptions = "ro"
-	}
-	mountArgs := []string{"-t", "nfs", server + ":" + exportDir, mountDir, "-o", mountOptions}
-	command := exec.Command("mount", mountArgs...)
-	output, errs := command.CombinedOutput()
-	if errs != nil {
-		glog.Errorf("NFS mounting failed: %v\n\tMount args are: %v\n\texportDir is: %v\n\tmountDir is: %v\n\tserver is: %v\n\tmount output is: %v", errs, mountArgs, exportDir, mountDir, server, string(output))
-		return errs
-	}
-	return nil
-}
-
-func (mounter *nfsMounter) Unmount(target string) error {
-	unmounter := mount.New()
-	return unmounter.Unmount(target, 0)
-}
-
-func (mounter *nfsMounter) List() ([]mount.MountPoint, error) {
-	return nil, nil
-}
-
-func (mounter *nfsMounter) IsMountPoint(dir string) (bool, error) {
-	isMounter := mount.New()
-	return isMounter.IsMountPoint(dir)
-}

pkg/volume/nfs/nfs_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package nfs
 
 import (
-	"fmt"
 	"os"
 	"testing"
 
@@ -67,35 +66,6 @@ func contains(modes []api.AccessModeType, mode api.AccessModeType) bool {
 	return false
 }
 
-type fakeNFSMounter struct {
-	FakeMounter mount.FakeMounter
-}
-
-func (fake *fakeNFSMounter) Mount(server string, source string, target string, readOnly bool) error {
-	flags := 0
-	if readOnly {
-		flags |= mount.FlagReadOnly
-	}
-	fake.FakeMounter.MountPoints = append(fake.FakeMounter.MountPoints, mount.MountPoint{Device: server, Path: target, Type: "nfs", Opts: nil, Freq: 0, Pass: 0})
-	return fake.FakeMounter.Mount(fmt.Sprintf("%s:%s", server, source), target, "nfs", 0, "")
-}
-
-func (fake *fakeNFSMounter) Unmount(target string) error {
-	fake.FakeMounter.MountPoints = []mount.MountPoint{}
-	return fake.FakeMounter.Unmount(target, 0)
-}
-
-func (fake *fakeNFSMounter) List() ([]mount.MountPoint, error) {
-	list, _ := fake.FakeMounter.List()
-	return list, nil
-}
-
-func (fake *fakeNFSMounter) IsMountPoint(dir string) (bool, error) {
-	list, _ := fake.FakeMounter.List()
-	isMount := len(list) > 0
-	return isMount, nil
-}
-
 func TestPlugin(t *testing.T) {
 	plugMgr := volume.VolumePluginMgr{}
 	plugMgr.InitPlugins(ProbeVolumePlugins(), volume.NewFakeVolumeHost("/tmp/fake", nil, nil))
@@ -107,7 +77,7 @@ func TestPlugin(t *testing.T) {
 		Name:         "vol1",
 		VolumeSource: api.VolumeSource{NFS: &api.NFSVolumeSource{"localhost", "/tmp", false}},
 	}
-	fake := &fakeNFSMounter{}
+	fake := &mount.FakeMounter{}
 	builder, err := plug.(*nfsPlugin).newBuilderInternal(volume.NewSpecFromVolume(spec), &api.ObjectReference{UID: types.UID("poduid")}, fake)
 	volumePath := builder.GetPath()
 	if err != nil {
@@ -133,14 +103,14 @@ func TestPlugin(t *testing.T) {
 	if builder.(*nfs).readOnly {
 		t.Errorf("The volume source should not be read-only and it is.")
 	}
-	if len(fake.FakeMounter.Log) != 1 {
+	if len(fake.Log) != 1 {
-		t.Errorf("Mount was not called exactly one time. It was called %d times.", len(fake.FakeMounter.Log))
+		t.Errorf("Mount was not called exactly one time. It was called %d times.", len(fake.Log))
 	} else {
-		if fake.FakeMounter.Log[0].Action != mount.FakeActionMount {
+		if fake.Log[0].Action != mount.FakeActionMount {
-			t.Errorf("Unexpected mounter action: %#v", fake.FakeMounter.Log[0])
+			t.Errorf("Unexpected mounter action: %#v", fake.Log[0])
 		}
 	}
-	fake.FakeMounter.ResetLog()
+	fake.ResetLog()
 
 	cleaner, err := plug.(*nfsPlugin).newCleanerInternal("vol1", types.UID("poduid"), fake)
 	if err != nil {
@@ -157,13 +127,13 @@ func TestPlugin(t *testing.T) {
 	} else if !os.IsNotExist(err) {
 		t.Errorf("SetUp() failed: %v", err)
 	}
-	if len(fake.FakeMounter.Log) != 1 {
+	if len(fake.Log) != 1 {
-		t.Errorf("Unmount was not called exactly one time. It was called %d times.", len(fake.FakeMounter.Log))
+		t.Errorf("Unmount was not called exactly one time. It was called %d times.", len(fake.Log))
 	} else {
-		if fake.FakeMounter.Log[0].Action != mount.FakeActionUnmount {
+		if fake.Log[0].Action != mount.FakeActionUnmount {
-			t.Errorf("Unexpected mounter action: %#v", fake.FakeMounter.Log[0])
+			t.Errorf("Unexpected mounter action: %#v", fake.Log[0])
 		}
 	}
 
-	fake.FakeMounter.ResetLog()
+	fake.ResetLog()
 }