diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index c25b0f58ea..5f8d0ab36c 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/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) - if err != nil { + diskInfo, attached, err := c.checkIfAttachedToNode(diskName, nodeName) + + if diskInfo == nil { return "", err } - awsInstance, info, err := c.getFullInstance(nodeName) - if err != nil { - 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 && diskInfo.ec2Instance != nil { + glog.Warningf("DetachDisk %s called for node %s but volume is attached to node %s", diskName, nodeName, diskInfo.nodeName) + return "", nil } - mountDevice, alreadyAttached, err := c.getMountDevice(awsInstance, info, disk.awsID, false) + if !attached { + return "", nil + } + + 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) - if err != nil { - if err == cloudprovider.InstanceNotFound { - // If instance no longer exists, safe to assume volume is not attached. - glog.Warningf( - "Instance %q does not exist. DiskIsAttached will assume disk %q is not attached to it.", - nodeName, - diskName) - return false, nil - } + diskInfo, attached, err := c.checkIfAttachedToNode(diskName, nodeName) - return false, err + if diskInfo == nil { + return true, err } - 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 + return attached, nil } func (c *Cloud) DisksAreAttached(nodeDisks map[types.NodeName][]KubernetesVolumeID) (map[types.NodeName]map[KubernetesVolumeID]bool, error) { diff --git a/pkg/cloudprovider/providers/aws/volumes.go b/pkg/cloudprovider/providers/aws/volumes.go index c67f080fec..3a4aa6284e 100644 --- a/pkg/cloudprovider/providers/aws/volumes.go +++ b/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 { // * 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 +} diff --git a/pkg/volume/aws_ebs/attacher.go b/pkg/volume/aws_ebs/attacher.go index ac8ea48fe1..ce96b94ddd 100644 --- a/pkg/volume/aws_ebs/attacher.go +++ b/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 != 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 { + if _, err := detacher.awsVolumes.DetachDisk(volumeID, nodeName); err != nil { glog.Errorf("Error detaching volumeID %q: %v", volumeID, err) return err } diff --git a/pkg/volume/aws_ebs/attacher_test.go b/pkg/volume/aws_ebs/attacher_test.go index 813139e5b9..1076d06910 100644 --- a/pkg/volume/aws_ebs/attacher_test.go +++ b/pkg/volume/aws_ebs/attacher_test.go @@ -62,10 +62,9 @@ func TestGetVolumeName_PersistentVolume(t *testing.T) { type testcase struct { name aws.KubernetesVolumeID // For fake AWS: - attach attachCall - detach detachCall - diskIsAttached diskIsAttachedCall - t *testing.T + attach attachCall + detach detachCall + t *testing.T // Actual test to run test func(test *testcase) (string, error) @@ -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 { @@ -107,44 +105,18 @@ func TestAttachDetach(t *testing.T) { // Detach succeeds { - name: "Detach_Positive", - diskIsAttached: diskIsAttachedCall{diskName, nodeName, true, nil}, - detach: detachCall{diskName, nodeName, "/dev/sda", nil}, + name: "Detach_Positive", + 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) }, }, - - // 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}, + name: "Detach_Negative", + detach: detachCall{diskName, nodeName, "", detachError}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) mountPath := "/mnt/" + string(diskName) @@ -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 - - 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 + // DetachDisk no longer relies on DiskIsAttached api call + return false, nil } func (testcase *testcase) DisksAreAttached(nodeDisks map[types.NodeName][]aws.KubernetesVolumeID) (map[types.NodeName]map[aws.KubernetesVolumeID]bool, error) {