AWS: Implement fix for detaching volume from stopped instances

Clean up detach disk functions and remove duplication

pkg/cloudprovider/providers/aws/aws.go

@@ -2021,26 +2021,24 @@ func (c *Cloud) AttachDisk(diskName KubernetesVolumeID, nodeName types.NodeName,
 
 // DetachDisk implements Volumes.DetachDisk
 func (c *Cloud) DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName) (string, error) {
-	disk, err := newAWSDisk(c, diskName)
+	diskInfo, attached, err := c.checkIfAttachedToNode(diskName, nodeName)
-	if err != nil {
+
+	if diskInfo == nil {
 		return "", err
 	}
 
-	awsInstance, info, err := c.getFullInstance(nodeName)
+	if !attached && diskInfo.ec2Instance != nil {
-	if err != nil {
+		glog.Warningf("DetachDisk %s called for node %s but volume is attached to node %s", diskName, nodeName, diskInfo.nodeName)
-		if err == cloudprovider.InstanceNotFound {
-			// If instance no longer exists, safe to assume volume is not attached.
-			glog.Warningf(
-				"Instance %q does not exist. DetachDisk will assume disk %q is not attached to it.",
-				nodeName,
-				diskName)
 		return "", nil
 	}
 
-		return "", err
+	if !attached {
+		return "", nil
 	}
 
-	mountDevice, alreadyAttached, err := c.getMountDevice(awsInstance, info, disk.awsID, false)
+	awsInstance := newAWSInstance(c.ec2, diskInfo.ec2Instance)
+
+	mountDevice, alreadyAttached, err := c.getMountDevice(awsInstance, diskInfo.ec2Instance, diskInfo.disk.awsID, false)
 	if err != nil {
 		return "", err
 	}
@@ -2052,18 +2050,19 @@ func (c *Cloud) DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName)
 
 	request := ec2.DetachVolumeInput{
 		InstanceId: &awsInstance.awsID,
-		VolumeId:   disk.awsID.awsString(),
+		VolumeId:   diskInfo.disk.awsID.awsString(),
 	}
 
 	response, err := c.ec2.DetachVolume(&request)
 	if err != nil {
-		return "", fmt.Errorf("error detaching EBS volume %q from %q: %q", disk.awsID, awsInstance.awsID, err)
+		return "", fmt.Errorf("error detaching EBS volume %q from %q: %q", diskInfo.disk.awsID, awsInstance.awsID, err)
 	}
+
 	if response == nil {
 		return "", errors.New("no response from DetachVolume")
 	}
 
-	attachment, err := disk.waitForAttachmentStatus("detached")
+	attachment, err := diskInfo.disk.waitForAttachmentStatus("detached")
 	if err != nil {
 		return "", err
 	}
@@ -2076,7 +2075,7 @@ func (c *Cloud) DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName)
 	}
 
 	if mountDevice != "" {
-		c.endAttaching(awsInstance, disk.awsID, mountDevice)
+		c.endAttaching(awsInstance, diskInfo.disk.awsID, mountDevice)
 		// We don't check the return value - we don't really expect the attachment to have been
 		// in progress, though it might have been
 	}
@@ -2320,32 +2319,13 @@ func (c *Cloud) GetDiskPath(volumeName KubernetesVolumeID) (string, error) {
 
 // DiskIsAttached implements Volumes.DiskIsAttached
 func (c *Cloud) DiskIsAttached(diskName KubernetesVolumeID, nodeName types.NodeName) (bool, error) {
-	_, instance, err := c.getFullInstance(nodeName)
+	diskInfo, attached, err := c.checkIfAttachedToNode(diskName, nodeName)
-	if err != nil {
+
-		if err == cloudprovider.InstanceNotFound {
+	if diskInfo == nil {
-			// If instance no longer exists, safe to assume volume is not attached.
+		return true, err
-			glog.Warningf(
-				"Instance %q does not exist. DiskIsAttached will assume disk %q is not attached to it.",
-				nodeName,
-				diskName)
-			return false, nil
 	}
 
-		return false, err
+	return attached, nil
-	}
-
-	diskID, err := diskName.mapToAWSVolumeID()
-	if err != nil {
-		return false, fmt.Errorf("error mapping volume spec %q to aws id: %v", diskName, err)
-	}
-
-	for _, blockDevice := range instance.BlockDeviceMappings {
-		id := awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId))
-		if id == diskID {
-			return true, nil
-		}
-	}
-	return false, nil
 }
 
 func (c *Cloud) DisksAreAttached(nodeDisks map[types.NodeName][]KubernetesVolumeID) (map[types.NodeName]map[KubernetesVolumeID]bool, error) {

pkg/cloudprovider/providers/aws/volumes.go

@@ -23,6 +23,9 @@ import (
 	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/golang/glog"
+	"k8s.io/apimachinery/pkg/types"
 )
 
 // awsVolumeRegMatch represents Regex Match for AWS volume.
@@ -46,6 +49,16 @@ func (i awsVolumeID) awsString() *string {
 //  * <awsVolumeId>
 type KubernetesVolumeID string
 
+// DiskInfo returns aws disk information in easy to use manner
+type diskInfo struct {
+	ec2Instance     *ec2.Instance
+	nodeName        types.NodeName
+	volumeState     string
+	attachmentState string
+	hasAttachment   bool
+	disk            *awsDisk
+}
+
 // mapToAWSVolumeID extracts the awsVolumeID from the KubernetesVolumeID
 func (name KubernetesVolumeID) mapToAWSVolumeID() (awsVolumeID, error) {
 	// name looks like aws://availability-zone/awsVolumeId
@@ -85,3 +98,55 @@ func (name KubernetesVolumeID) mapToAWSVolumeID() (awsVolumeID, error) {
 
 	return awsVolumeID(awsID), nil
 }
+
+func GetAWSVolumeID(kubeVolumeID string) (string, error) {
+	kid := KubernetesVolumeID(kubeVolumeID)
+	awsID, err := kid.mapToAWSVolumeID()
+	return string(awsID), err
+}
+
+func (c *Cloud) checkIfAttachedToNode(diskName KubernetesVolumeID, nodeName types.NodeName) (*diskInfo, bool, error) {
+	disk, err := newAWSDisk(c, diskName)
+
+	if err != nil {
+		return nil, true, err
+	}
+
+	awsDiskInfo := &diskInfo{
+		disk: disk,
+	}
+
+	info, err := disk.describeVolume()
+
+	if err != nil {
+		describeError := fmt.Errorf("Error describing volume %s with %v", diskName, err)
+		glog.Warning(describeError)
+		awsDiskInfo.volumeState = "unknown"
+		return awsDiskInfo, false, describeError
+	}
+
+	awsDiskInfo.volumeState = aws.StringValue(info.State)
+
+	if len(info.Attachments) > 0 {
+		attachment := info.Attachments[0]
+		awsDiskInfo.attachmentState = aws.StringValue(attachment.State)
+		instanceID := aws.StringValue(attachment.InstanceId)
+		instanceInfo, err := c.getInstanceByID(instanceID)
+
+		// This should never happen but if it does it could mean there was a race and instance
+		// has been deleted
+		if err != nil {
+			fetchErr := fmt.Errorf("Error fetching instance %s for volume %s", instanceID, diskName)
+			glog.Warning(fetchErr)
+			return awsDiskInfo, false, fetchErr
+		}
+
+		awsDiskInfo.ec2Instance = instanceInfo
+		awsDiskInfo.nodeName = mapInstanceToNodeName(instanceInfo)
+		awsDiskInfo.hasAttachment = true
+		if awsDiskInfo.nodeName == nodeName {
+			return awsDiskInfo, true, nil
+		}
+	}
+	return awsDiskInfo, false, nil
+}

pkg/volume/aws_ebs/attacher.go

@@ -256,21 +256,7 @@ func (plugin *awsElasticBlockStorePlugin) NewDetacher() (volume.Detacher, error)
 func (detacher *awsElasticBlockStoreDetacher) Detach(volumeName string, nodeName types.NodeName) error {
 	volumeID := aws.KubernetesVolumeID(path.Base(volumeName))
 
-	attached, err := detacher.awsVolumes.DiskIsAttached(volumeID, nodeName)
+	if _, err := detacher.awsVolumes.DetachDisk(volumeID, nodeName); err != nil {
-	if err != nil {
-		// Log error and continue with detach
-		glog.Errorf(
-			"Error checking if volume (%q) is already attached to current node (%q). Will continue and try detach anyway. err=%v",
-			volumeID, nodeName, err)
-	}
-
-	if err == nil && !attached {
-		// Volume is already detached from node.
-		glog.Infof("detach operation was successful. volume %q is already detached from node %q.", volumeID, nodeName)
-		return nil
-	}
-
-	if _, err = detacher.awsVolumes.DetachDisk(volumeID, nodeName); err != nil {
 		glog.Errorf("Error detaching volumeID %q: %v", volumeID, err)
 		return err
 	}

pkg/volume/aws_ebs/attacher_test.go

@@ -64,7 +64,6 @@ type testcase struct {
 	// For fake AWS:
 	attach attachCall
 	detach detachCall
-	diskIsAttached diskIsAttachedCall
 	t      *testing.T
 
 	// Actual test to run
@@ -81,7 +80,6 @@ func TestAttachDetach(t *testing.T) {
 	spec := createVolSpec(diskName, readOnly)
 	attachError := errors.New("Fake attach error")
 	detachError := errors.New("Fake detach error")
-	diskCheckError := errors.New("Fake DiskIsAttached error")
 	tests := []testcase{
 		// Successful Attach call
 		{
@@ -108,7 +106,6 @@ func TestAttachDetach(t *testing.T) {
 		// Detach succeeds
 		{
 			name:   "Detach_Positive",
-			diskIsAttached: diskIsAttachedCall{diskName, nodeName, true, nil},
 			detach: detachCall{diskName, nodeName, "/dev/sda", nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
@@ -116,34 +113,9 @@ func TestAttachDetach(t *testing.T) {
 				return "", detacher.Detach(mountPath, nodeName)
 			},
 		},
-
-		// Disk is already detached
-		{
-			name:           "Detach_Positive_AlreadyDetached",
-			diskIsAttached: diskIsAttachedCall{diskName, nodeName, false, nil},
-			test: func(testcase *testcase) (string, error) {
-				detacher := newDetacher(testcase)
-				mountPath := "/mnt/" + string(diskName)
-				return "", detacher.Detach(mountPath, nodeName)
-			},
-		},
-
-		// Detach succeeds when DiskIsAttached fails
-		{
-			name:           "Detach_Positive_CheckFails",
-			diskIsAttached: diskIsAttachedCall{diskName, nodeName, false, diskCheckError},
-			detach:         detachCall{diskName, nodeName, "/dev/sda", nil},
-			test: func(testcase *testcase) (string, error) {
-				detacher := newDetacher(testcase)
-				mountPath := "/mnt/" + string(diskName)
-				return "", detacher.Detach(mountPath, nodeName)
-			},
-		},
-
 		// Detach fails
 		{
 			name:   "Detach_Negative",
-			diskIsAttached: diskIsAttachedCall{diskName, nodeName, false, diskCheckError},
 			detach: detachCall{diskName, nodeName, "", detachError},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
@@ -298,28 +270,8 @@ func (testcase *testcase) DetachDisk(diskName aws.KubernetesVolumeID, nodeName t
 }
 
 func (testcase *testcase) DiskIsAttached(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) {
-	expected := &testcase.diskIsAttached
+	// DetachDisk no longer relies on DiskIsAttached api call
-
+	return false, nil
-	if expected.diskName == "" && expected.nodeName == "" {
-		// testcase.diskIsAttached looks uninitialized, test did not expect to
-		// call DiskIsAttached
-		testcase.t.Errorf("Unexpected DiskIsAttached call!")
-		return false, errors.New("Unexpected DiskIsAttached call!")
-	}
-
-	if expected.diskName != diskName {
-		testcase.t.Errorf("Unexpected DiskIsAttached call: expected diskName %s, got %s", expected.diskName, diskName)
-		return false, errors.New("Unexpected DiskIsAttached call: wrong diskName")
-	}
-
-	if expected.nodeName != nodeName {
-		testcase.t.Errorf("Unexpected DiskIsAttached call: expected nodeName %s, got %s", expected.nodeName, nodeName)
-		return false, errors.New("Unexpected DiskIsAttached call: wrong nodeName")
-	}
-
-	glog.V(4).Infof("DiskIsAttached call: %s, %s, returning %v, %v", diskName, nodeName, expected.isAttached, expected.ret)
-
-	return expected.isAttached, expected.ret
 }
 
 func (testcase *testcase) DisksAreAttached(nodeDisks map[types.NodeName][]aws.KubernetesVolumeID) (map[types.NodeName]map[aws.KubernetesVolumeID]bool, error) {