Merge pull request #81266 from andyzhangx/fix-detach-azuredisk-issue

fix: detach azure disk issue using dangling error

Author: k8s-ci-robot

pkg/volume/azure_dd/attacher.go

@@ -87,7 +87,7 @@ func (a *azureDiskAttacher) Attach(spec *volume.Spec, nodeName types.NodeName) (
 			klog.V(2).Infof("Attach operation successful: volume %q attached to node %q.", volumeSource.DataDiskURI, nodeName)
 		} else {
 			klog.V(2).Infof("Attach volume %q to instance %q failed with %v", volumeSource.DataDiskURI, nodeName, err)
-			return "", fmt.Errorf("Attach volume %q to instance %q failed with %v", volumeSource.DiskName, nodeName, err)
+			return "", err
 		}
 	}
 

staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go

@@ -19,13 +19,15 @@ package azure
 import (
 	"context"
 	"fmt"
+	"path"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-03-01/compute"
 
 	"k8s.io/apimachinery/pkg/types"
 	kwait "k8s.io/apimachinery/pkg/util/wait"
 	cloudprovider "k8s.io/cloud-provider"
+	volerr "k8s.io/cloud-provider/volume/errors"
 	"k8s.io/klog"
 	"k8s.io/utils/keymutex"
 )
@@ -93,6 +95,32 @@ func (c *controllerCommon) getNodeVMSet(nodeName types.NodeName) (VMSet, error)
 // AttachDisk attaches a vhd to vm. The vhd must exist, can be identified by diskName, diskURI.
 // return (lun, error)
 func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI string, nodeName types.NodeName, cachingMode compute.CachingTypes) (int32, error) {
+	if isManagedDisk {
+		diskName := path.Base(diskURI)
+		resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
+		if err != nil {
+			return -1, err
+		}
+
+		ctx, cancel := getContextWithCancel()
+		defer cancel()
+
+		disk, err := c.cloud.DisksClient.Get(ctx, resourceGroup, diskName)
+		if err != nil {
+			return -1, err
+		}
+
+		if disk.ManagedBy != nil {
+			attachErr := fmt.Sprintf(
+				"disk(%s) already attached to node(%s), could not be attached to node(%s)",
+				diskURI, *disk.ManagedBy, nodeName)
+			attachedNode := path.Base(*disk.ManagedBy)
+			klog.V(2).Infof("found dangling volume %s attached to node %s", diskURI, attachedNode)
+			danglingErr := volerr.NewDanglingError(attachErr, types.NodeName(attachedNode), "")
+			return -1, danglingErr
+		}
+	}
+
 	vmset, err := c.getNodeVMSet(nodeName)
 	if err != nil {
 		return -1, err

staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go

@@ -53,8 +53,8 @@ func TestCommonAttachDisk(t *testing.T) {
 			desc:        "correct LUN and no error shall be returned if everything is good",
 			vmList:      map[string]string{"vm1": "PowerState/Running"},
 			nodeName:    "vm1",
-			expectedLun: 1,
-			expectedErr: false,
+			expectedLun: -1,
+			expectedErr: true,
 		},
 	}
 
@@ -73,7 +73,7 @@ func TestCommonAttachDisk(t *testing.T) {
 
 		lun, err := common.AttachDisk(true, "", diskURI, test.nodeName, compute.CachingTypesReadOnly)
 		assert.Equal(t, test.expectedLun, lun, "TestCase[%d]: %s", i, test.desc)
-		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s", i, test.desc)
+		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, return error: %v", i, test.desc, err)
 	}
 }
 

staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go

@@ -118,9 +118,9 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N
 
 	bFoundDisk := false
 	for i, disk := range disks {
-		if disk.Lun != nil && (disk.Name != nil && diskName != "" && *disk.Name == diskName) ||
-			(disk.Vhd != nil && disk.Vhd.URI != nil && diskURI != "" && *disk.Vhd.URI == diskURI) ||
-			(disk.ManagedDisk != nil && diskURI != "" && *disk.ManagedDisk.ID == diskURI) {
+		if disk.Lun != nil && (disk.Name != nil && diskName != "" && strings.EqualFold(*disk.Name, diskName)) ||
+			(disk.Vhd != nil && disk.Vhd.URI != nil && diskURI != "" && strings.EqualFold(*disk.Vhd.URI, diskURI)) ||
+			(disk.ManagedDisk != nil && diskURI != "" && strings.EqualFold(*disk.ManagedDisk.ID, diskURI)) {
 			// found the disk
 			klog.V(2).Infof("azureDisk - detach disk: name %q uri %q", diskName, diskURI)
 			disks = append(disks[:i], disks[i+1:]...)

staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go

@@ -123,9 +123,9 @@ func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName
 	}
 	bFoundDisk := false
 	for i, disk := range disks {
-		if disk.Lun != nil && (disk.Name != nil && diskName != "" && *disk.Name == diskName) ||
-			(disk.Vhd != nil && disk.Vhd.URI != nil && diskURI != "" && *disk.Vhd.URI == diskURI) ||
-			(disk.ManagedDisk != nil && diskURI != "" && *disk.ManagedDisk.ID == diskURI) {
+		if disk.Lun != nil && (disk.Name != nil && diskName != "" && strings.EqualFold(*disk.Name, diskName)) ||
+			(disk.Vhd != nil && disk.Vhd.URI != nil && diskURI != "" && strings.EqualFold(*disk.Vhd.URI, diskURI)) ||
+			(disk.ManagedDisk != nil && diskURI != "" && strings.EqualFold(*disk.ManagedDisk.ID, diskURI)) {
 			// found the disk
 			klog.V(2).Infof("azureDisk - detach disk: name %q uri %q", diskName, diskURI)
 			disks = append(disks[:i], disks[i+1:]...)