From 1335f6470c4ae023ad385ad9fb765eea35751933 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sun, 11 Aug 2019 13:19:40 +0000 Subject: [PATCH 1/2] fix: detach azure disk issue using dangling error fix: unit test failure --- pkg/volume/azure_dd/attacher.go | 2 +- .../azure/azure_controller_common.go | 28 +++++++++++++++++++ .../azure/azure_controller_common_test.go | 6 ++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/pkg/volume/azure_dd/attacher.go b/pkg/volume/azure_dd/attacher.go index 4eec28f23b4..77deb1ae773 100644 --- a/pkg/volume/azure_dd/attacher.go +++ b/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 } } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go index c10061da24c..049051fc58c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go @@ -19,6 +19,7 @@ package azure import ( "context" "fmt" + "path" "time" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-03-01/compute" @@ -26,6 +27,7 @@ import ( "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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go index 34b1037094c..32b2c76cefe 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go +++ b/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) } } From 1fe12be56e427720f71fc33e71efb4d563ac78e0 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sun, 18 Aug 2019 03:34:25 +0000 Subject: [PATCH 2/2] fix: disk not found issue in detaching azure disk --- .../azure/azure_controller_standard.go | 6 +++--- .../legacy-cloud-providers/azure/azure_controller_vmss.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go index 19366f817b0..cd6aa48cd6e 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go +++ b/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:]...) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go index 67955b3a502..4a74f1bb915 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go +++ b/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:]...)