Fix PD detach (fix the mount path/device name calculation).

pkg/kubelet/volume/gce_pd/gce_pd.go

@@ -128,9 +128,9 @@ func (plugin *gcePersistentDiskPlugin) newCleanerInternal(volName string, podUID
 // Abstract interface to PD operations.
 type pdManager interface {
 	// Attaches the disk to the kubelet's host machine.
-	AttachDisk(pd *gcePersistentDisk) error
+	AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath string) error
 	// Detaches the disk from the kubelet's host machine.
-	DetachDisk(pd *gcePersistentDisk, devicePath string) error
+	DetachDisk(pd *gcePersistentDisk) error
 }
 
 // gcePersistentDisk volumes are disk resources provided by Google Compute Engine
@@ -157,7 +157,7 @@ type gcePersistentDisk struct {
 }
 
 func detachDiskLogError(pd *gcePersistentDisk) {
-	err := pd.manager.DetachDisk(pd, "/dev/disk/by-id/google-"+pd.pdName)
+	err := pd.manager.DetachDisk(pd)
 	if err != nil {
 		glog.Warningf("Failed to detach disk: %v (%v)", pd, err)
 	}
@@ -179,7 +179,8 @@ func (pd *gcePersistentDisk) SetUp() error {
 		return nil
 	}
 
-	if err := pd.manager.AttachDisk(pd); err != nil {
+	globalPDPath := makeGlobalPDName(pd.plugin.host, pd.pdName)
+	if err := pd.manager.AttachAndMountDisk(pd, globalPDPath); err != nil {
 		return err
 	}
 
@@ -196,7 +197,6 @@ func (pd *gcePersistentDisk) SetUp() error {
 	}
 
 	// Perform a bind mount to the full path to allow duplicate mounts of the same PD.
-	globalPDPath := makeGlobalPDName(pd.plugin.host, pd.pdName, pd.readOnly)
 	err = pd.mounter.Mount(globalPDPath, pd.GetPath(), "", mount.FlagBind|flags, "")
 	if err != nil {
 		mountpoint, mntErr := isMountPoint(pd.GetPath())
@@ -229,7 +229,7 @@ func (pd *gcePersistentDisk) SetUp() error {
 	return nil
 }
 
-func makeGlobalPDName(host volume.Host, devName string, readOnly bool) string {
+func makeGlobalPDName(host volume.Host, devName string) string {
 	return path.Join(host.GetPluginDir(gcePersistentDiskPluginName), "mounts", devName)
 }
 
@@ -252,18 +252,20 @@ func (pd *gcePersistentDisk) TearDown() error {
 		return os.Remove(pd.GetPath())
 	}
 
-	devicePath, refCount, err := getMountRefCount(pd.mounter, pd.GetPath())
+	refs, err := getMountRefs(pd.mounter, pd.GetPath())
 	if err != nil {
 		return err
 	}
+	// Unmount the bind-mount inside this pod
 	if err := pd.mounter.Unmount(pd.GetPath(), 0); err != nil {
 		return err
 	}
-	refCount--
-	// If refCount is 1, then all bind mounts have been removed, and the
+	// If len(refs) is 1, then all bind mounts have been removed, and the
 	// remaining reference is the global mount. It is safe to detach.
-	if refCount == 1 {
-		if err := pd.manager.DetachDisk(pd, devicePath); err != nil {
+	if len(refs) == 1 {
+		// pd.pdName is not initially set for volume-cleaners, so set it here.
+		pd.pdName = path.Base(refs[0])
+		if err := pd.manager.DetachDisk(pd); err != nil {
 			return err
 		}
 	}

pkg/kubelet/volume/gce_pd/gce_pd_test.go

@@ -23,7 +23,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/volume"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/types"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/mount"
 )
 
 func TestCanSupport(t *testing.T) {
@@ -46,38 +45,24 @@ type fakePDManager struct{}
 
 // TODO(jonesdl) To fully test this, we could create a loopback device
 // and mount that instead.
-func (fake *fakePDManager) AttachDisk(pd *gcePersistentDisk) error {
-	globalPath := makeGlobalPDName(pd.plugin.host, pd.pdName, pd.readOnly)
+func (fake *fakePDManager) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath string) error {
+	globalPath := makeGlobalPDName(pd.plugin.host, pd.pdName)
 	err := os.MkdirAll(globalPath, 0750)
 	if err != nil {
 		return err
 	}
 	return nil
 }
 
-func (fake *fakePDManager) DetachDisk(pd *gcePersistentDisk, devicePath string) error {
-	globalPath := makeGlobalPDName(pd.plugin.host, pd.pdName, pd.readOnly)
+func (fake *fakePDManager) DetachDisk(pd *gcePersistentDisk) error {
+	globalPath := makeGlobalPDName(pd.plugin.host, pd.pdName)
 	err := os.RemoveAll(globalPath)
 	if err != nil {
 		return err
 	}
 	return nil
 }
 
-type fakeMounter struct{}
-
-func (fake *fakeMounter) Mount(source string, target string, fstype string, flags uintptr, data string) error {
-	return nil
-}
-
-func (fake *fakeMounter) Unmount(target string, flags int) error {
-	return nil
-}
-
-func (fake *fakeMounter) List() ([]mount.MountPoint, error) {
-	return []mount.MountPoint{}, nil
-}
-
 func TestPlugin(t *testing.T) {
 	plugMgr := volume.PluginMgr{}
 	plugMgr.InitPlugins(ProbeVolumePlugins(), &volume.FakeHost{"/tmp/fake", nil})

pkg/kubelet/volume/gce_pd/gce_util.go

@@ -21,9 +21,6 @@ import (
 	"fmt"
 	"os"
 	"path"
-	"path/filepath"
-	"regexp"
-	"strings"
 	"time"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider"
@@ -33,15 +30,11 @@ import (
 	"github.com/golang/glog"
 )
 
-const partitionRegex = "[a-z][a-z]*(?P<partition>[0-9][0-9]*)?"
-
-var regexMatcher = regexp.MustCompile(partitionRegex)
-
 type GCEDiskUtil struct{}
 
 // Attaches a disk specified by a volume.GCEPersistentDisk to the current kubelet.
 // Mounts the disk to it's global path.
-func (util *GCEDiskUtil) AttachDisk(pd *gcePersistentDisk) error {
+func (util *GCEDiskUtil) AttachAndMountDisk(pd *gcePersistentDisk, globalPDPath string) error {
 	gce, err := cloudprovider.GetCloudProvider("gce", nil)
 	if err != nil {
 		return err
@@ -73,7 +66,7 @@ func (util *GCEDiskUtil) AttachDisk(pd *gcePersistentDisk) error {
 		}
 		time.Sleep(time.Second)
 	}
-	globalPDPath := makeGlobalPDName(pd.plugin.host, pd.pdName, pd.readOnly)
+
 	// Only mount the PD globally once.
 	mountpoint, err := isMountPoint(globalPDPath)
 	if err != nil {
@@ -96,47 +89,22 @@ func (util *GCEDiskUtil) AttachDisk(pd *gcePersistentDisk) error {
 	return nil
 }
 
-func getDeviceName(devicePath, canonicalDevicePath string) (string, error) {
-	isMatch := regexMatcher.MatchString(path.Base(canonicalDevicePath))
-	if !isMatch {
-		return "", fmt.Errorf("unexpected device: %s", canonicalDevicePath)
-	}
-	if isMatch {
-		result := make(map[string]string)
-		substrings := regexMatcher.FindStringSubmatch(path.Base(canonicalDevicePath))
-		for i, label := range regexMatcher.SubexpNames() {
-			result[label] = substrings[i]
-		}
-		partition := result["partition"]
-		devicePath = strings.TrimSuffix(devicePath, "-part"+partition)
-	}
-	return strings.TrimPrefix(path.Base(devicePath), "google-"), nil
-}
-
 // Unmounts the device and detaches the disk from the kubelet's host machine.
-// Expects a GCE device path symlink. Ex: /dev/disk/by-id/google-mydisk-part1
-func (util *GCEDiskUtil) DetachDisk(pd *gcePersistentDisk, devicePath string) error {
-	// Follow the symlink to the actual device path.
-	canonicalDevicePath, err := filepath.EvalSymlinks(devicePath)
-	if err != nil {
-		return err
-	}
-	deviceName, err := getDeviceName(devicePath, canonicalDevicePath)
-	if err != nil {
-		return err
-	}
-	globalPDPath := makeGlobalPDName(pd.plugin.host, deviceName, pd.readOnly)
+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 {
 		return err
 	}
 	if err := os.Remove(globalPDPath); err != nil {
 		return err
 	}
+	// Detach the disk
 	gce, err := cloudprovider.GetCloudProvider("gce", nil)
 	if err != nil {
 		return err
 	}
-	if err := gce.(*gce_cloud.GCECloud).DetachDisk(deviceName); err != nil {
+	if err := gce.(*gce_cloud.GCECloud).DetachDisk(pd.pdName); err != nil {
 		return err
 	}
 	return nil
@@ -153,6 +121,10 @@ 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 {
+	// Don't attempt to format if mounting as readonly. Go straight to mounting.
+	if (flags & mount.FlagReadOnly) != 0 {
+		return mounter.Interface.Mount(source, target, fstype, flags, data)
+	}
 	args := []string{}
 	// ext4 is the default for safe_format_and_mount
 	if len(fstype) > 0 && fstype != "ext4" {

pkg/kubelet/volume/gce_pd/gce_util_test.go

@@ -23,40 +23,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/exec"
 )
 
-func TestGetDeviceName(t *testing.T) {
-	tests := []struct {
-		deviceName    string
-		canonicalName string
-		expectedName  string
-		expectError   bool
-	}{
-		{
-			deviceName:    "/dev/google-sd0-part0",
-			canonicalName: "/dev/google/sd0P1",
-			expectedName:  "sd0",
-		},
-		{
-			canonicalName: "0123456",
-			expectError:   true,
-		},
-	}
-	for _, test := range tests {
-		name, err := getDeviceName(test.deviceName, test.canonicalName)
-		if test.expectError {
-			if err == nil {
-				t.Error("unexpected non-error")
-			}
-			continue
-		}
-		if err != nil {
-			t.Errorf("unexpected error: %v", err)
-		}
-		if name != test.expectedName {
-			t.Errorf("expected: %s, got %s", test.expectedName, name)
-		}
-	}
-}
-
 func TestSafeFormatAndMount(t *testing.T) {
 	tests := []struct {
 		fstype       string

pkg/kubelet/volume/gce_pd/mount_util.go

@@ -20,17 +20,12 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/mount"
 )
 
-// Examines /proc/mounts to find the source device of the PD resource and the
-// number of references to that device. Returns both the full device path under
-// the /dev tree and the number of references.
-func getMountRefCount(mounter mount.Interface, mountPath string) (string, int, error) {
-	// TODO(jonesdl) This can be split up into two procedures, finding the device path
-	// and finding the number of references. The parsing could also be separated and another
-	// utility could determine if a path is an active mount point.
-
+// Examines /proc/mounts to find all other references to the device referenced
+// by mountPath.
+func getMountRefs(mounter mount.Interface, mountPath string) ([]string, error) {
 	mps, err := mounter.List()
 	if err != nil {
-		return "", -1, err
+		return nil, err
 	}
 
 	// Find the device name.
@@ -42,12 +37,12 @@ func getMountRefCount(mounter mount.Interface, mountPath string) (string, int, e
 		}
 	}
 
-	// Find the number of references to the device.
-	refCount := 0
+	// Find all references to the device.
+	var refs []string
 	for i := range mps {
-		if mps[i].Device == deviceName {
-			refCount++
+		if mps[i].Device == deviceName && mps[i].Path != mountPath {
+			refs = append(refs, mps[i].Path)
 		}
 	}
-	return deviceName, refCount, nil
+	return refs, nil
 }