Don't rely on device name provided by Cinder

pkg/cloudprovider/providers/openstack/openstack.go

@@ -683,26 +683,6 @@ func (os *OpenStack) DeleteVolume(volumeName string) error {
 	return err
 }
 
-// Get device path of attached volume to the compute running kubelet
-func (os *OpenStack) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) {
-	disk, err := os.getVolume(diskName)
-	if err != nil {
-		return "", err
-	}
-	if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil {
-		if instanceID == disk.Attachments[0]["server_id"] {
-			// Attachment[0]["device"] points to the device path
-			// see http://developer.openstack.org/api-ref-blockstorage-v1.html
-			return disk.Attachments[0]["device"].(string), nil
-		} else {
-			errMsg := fmt.Sprintf("Disk %q is attached to a different compute: %q, should be detached before proceeding", diskName, disk.Attachments[0]["server_id"])
-			glog.Errorf(errMsg)
-			return "", errors.New(errMsg)
-		}
-	}
-	return "", fmt.Errorf("volume %s is not attached to %s", diskName, instanceID)
-}
-
 // query if a volume is attached to a compute instance
 func (os *OpenStack) DiskIsAttached(diskName, instanceID string) (bool, error) {
 	disk, err := os.getVolume(diskName)

pkg/cloudprovider/providers/rackspace/rackspace.go

@@ -613,26 +613,6 @@ func (rs *Rackspace) DetachDisk(instanceID string, partialDiskId string) error {
 	return nil
 }
 
-// Get device path of attached volume to the compute running kubelet
-func (rs *Rackspace) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) {
-	disk, err := rs.getVolume(diskName)
-	if err != nil {
-		return "", err
-	}
-	if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil {
-		if instanceID == disk.Attachments[0]["server_id"] {
-			// Attachment[0]["device"] points to the device path
-			// see http://developer.openstack.org/api-ref-blockstorage-v1.html
-			return disk.Attachments[0]["device"].(string), nil
-		} else {
-			errMsg := fmt.Sprintf("Disk %q is attached to a different compute: %q, should be detached before proceeding", diskName, disk.Attachments[0]["server_id"])
-			glog.Errorf(errMsg)
-			return "", errors.New(errMsg)
-		}
-	}
-	return "", fmt.Errorf("volume %s is not attached to %s", diskName, instanceID)
-}
-
 // query if a volume is attached to a compute instance
 func (rs *Rackspace) DiskIsAttached(diskName, instanceID string) (bool, error) {
 	disk, err := rs.getVolume(diskName)

pkg/volume/cinder/attacher.go

@@ -99,27 +99,12 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) (
 		}
 	}
 
-	devicePath, err := attacher.cinderProvider.GetAttachmentDiskPath(instanceid, volumeID)
-	if err != nil {
-		glog.Infof("Attach volume %q to instance %q failed with %v", volumeID, instanceid, err)
-		return "", err
-	}
-
-	return devicePath, err
+	// Fake the "devicePath" and actually just return the Cinder ID for the volume, allowing the kubelet to find
+	// the real device path (See Issue #33128)
+	return volumeID, err
 }
 
-func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string, timeout time.Duration) (string, error) {
-	volumeSource, _, err := getVolumeSource(spec)
-	if err != nil {
-		return "", err
-	}
-
-	volumeID := volumeSource.VolumeID
-
-	if devicePath == "" {
-		return "", fmt.Errorf("WaitForAttach failed for Cinder disk %q: devicePath is empty.", volumeID)
-	}
-
+func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, volumeID string, timeout time.Duration) (string, error) {
 	ticker := time.NewTicker(checkSleepDuration)
 	defer ticker.Stop()
 	timer := time.NewTimer(timeout)
@@ -131,13 +116,16 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath
 		case <-ticker.C:
 			glog.V(5).Infof("Checking Cinder disk %q is attached.", volumeID)
 			probeAttachedVolume()
-			exists, err := volumeutil.PathExists(devicePath)
-			if exists && err == nil {
+
+			// Using the Cinder volume ID, find the real device path (See Issue #33128)
+			devicePath := attacher.cinderProvider.GetDevicePath(volumeID)
+
+			if devicePath != "" {
 				glog.Infof("Successfully found attached Cinder disk %q.", volumeID)
 				return devicePath, nil
 			} else {
-				//Log error, if any, and continue checking periodically
-				glog.Errorf("Error Stat Cinder disk (%q) is attached: %v", volumeID, err)
+				// Log an error, and continue checking periodically
+				glog.Errorf("Error could not find attached Cinder disk %q", volumeID)
 			}
 		case <-timer.C:
 			return "", fmt.Errorf("Could not find attached Cinder disk %q. Timeout waiting for mount paths to be created.", volumeID)
@@ -156,7 +144,14 @@ func (attacher *cinderDiskAttacher) GetDeviceMountPath(
 }
 
 // FIXME: this method can be further pruned.
-func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string) error {
+func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, volumeID string, deviceMountPath string) error {
+	// Using the Cinder volume ID, find the real device path (See Issue #33128)
+	devicePath := attacher.cinderProvider.GetDevicePath(volumeID)
+
+	if devicePath == "" {
+		return fmt.Errorf("MountDevice failed for Cinder disk %q: devicePath is empty.", volumeID)
+	}
+
 	mounter := attacher.host.GetMounter()
 	notMnt, err := mounter.IsLikelyNotMountPoint(deviceMountPath)
 	if err != nil {
@@ -241,12 +236,20 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, hostName stri
 	return nil
 }
 
-func (detacher *cinderDiskDetacher) WaitForDetach(devicePath string, timeout time.Duration) error {
+func (detacher *cinderDiskDetacher) WaitForDetach(volumeID string, timeout time.Duration) error {
 	ticker := time.NewTicker(checkSleepDuration)
 	defer ticker.Stop()
 	timer := time.NewTimer(timeout)
 	defer timer.Stop()
 
+	// Using the Cinder volume ID, find the real device path (See Issue #33128)
+	devicePath := detacher.cinderProvider.GetDevicePath(volumeID)
+
+	if devicePath == "" {
+		// Volume has already been unmounted
+		return nil
+	}
+
 	for {
 		select {
 		case <-ticker.C:

pkg/volume/cinder/attacher_test.go

@@ -82,7 +82,6 @@ func TestAttachDetach(t *testing.T) {
 	attachError := errors.New("Fake attach error")
 	detachError := errors.New("Fake detach error")
 	diskCheckError := errors.New("Fake DiskIsAttached error")
-	diskPathError := errors.New("Fake GetAttachmentDiskPath error")
 	tests := []testcase{
 		// Successful Attach call
 		{
@@ -95,7 +94,7 @@ func TestAttachDetach(t *testing.T) {
 				attacher := newAttacher(testcase)
 				return attacher.Attach(spec, instanceID)
 			},
-			expectedDevice: "/dev/sda",
+			expectedDevice: diskName,
 		},
 
 		// Disk is already attached
@@ -108,7 +107,7 @@ func TestAttachDetach(t *testing.T) {
 				attacher := newAttacher(testcase)
 				return attacher.Attach(spec, instanceID)
 			},
-			expectedDevice: "/dev/sda",
+			expectedDevice: diskName,
 		},
 
 		// DiskIsAttached fails and Attach succeeds
@@ -122,7 +121,7 @@ func TestAttachDetach(t *testing.T) {
 				attacher := newAttacher(testcase)
 				return attacher.Attach(spec, instanceID)
 			},
-			expectedDevice: "/dev/sda",
+			expectedDevice: diskName,
 		},
 
 		// Attach call fails
@@ -138,20 +137,6 @@ func TestAttachDetach(t *testing.T) {
 			expectedError: attachError,
 		},
 
-		// GetAttachmentDiskPath call fails
-		{
-			name:           "Attach_Negative_DiskPatchFails",
-			instanceID:     instanceID,
-			diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError},
-			attach:         attachCall{diskName, instanceID, "", nil},
-			diskPath:       diskPathCall{diskName, instanceID, "", diskPathError},
-			test: func(testcase *testcase) (string, error) {
-				attacher := newAttacher(testcase)
-				return attacher.Attach(spec, instanceID)
-			},
-			expectedError: diskPathError,
-		},
-
 		// Detach succeeds
 		{
 			name:           "Detach_Positive",
@@ -367,30 +352,6 @@ func (testcase *testcase) DiskIsAttached(diskName, instanceID string) (bool, err
 	return expected.isAttached, expected.ret
 }
 
-func (testcase *testcase) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) {
-	expected := &testcase.diskPath
-	if expected.diskName == "" && expected.instanceID == "" {
-		// testcase.diskPath looks uninitialized, test did not expect to
-		// call GetAttachmentDiskPath
-		testcase.t.Errorf("Unexpected GetAttachmentDiskPath call!")
-		return "", errors.New("Unexpected GetAttachmentDiskPath call!")
-	}
-
-	if expected.diskName != diskName {
-		testcase.t.Errorf("Unexpected GetAttachmentDiskPath call: expected diskName %s, got %s", expected.diskName, diskName)
-		return "", errors.New("Unexpected GetAttachmentDiskPath call: wrong diskName")
-	}
-
-	if expected.instanceID != instanceID {
-		testcase.t.Errorf("Unexpected GetAttachmentDiskPath call: expected instanceID %s, got %s", expected.instanceID, instanceID)
-		return "", errors.New("Unexpected GetAttachmentDiskPath call: wrong instanceID")
-	}
-
-	glog.V(4).Infof("GetAttachmentDiskPath call: %s, %s, returning %v, %v", diskName, instanceID, expected.retPath, expected.ret)
-
-	return expected.retPath, expected.ret
-}
-
 func (testcase *testcase) CreateVolume(name string, size int, tags *map[string]string) (volumeName string, err error) {
 	return "", errors.New("Not implemented")
 }

pkg/volume/cinder/cinder.go

@@ -48,7 +48,6 @@ type CinderProvider interface {
 	CreateVolume(name string, size int, tags *map[string]string) (volumeName string, err error)
 	GetDevicePath(diskId string) string
 	InstanceID() (string, error)
-	GetAttachmentDiskPath(instanceID string, diskName string) (string, error)
 	DiskIsAttached(diskName, instanceID string) (bool, error)
 	Instances() (cloudprovider.Instances, bool)
 }