From 5376b645e306f493ff3b5243be17628c6800277b Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sat, 13 Apr 2019 12:45:32 +0000 Subject: [PATCH] refactor detach azure disk retry operation --- .../azure/azure_controller_common.go | 24 +++++++++++++-- .../azure/azure_controller_standard.go | 30 +++++-------------- .../providers/azure/azure_controller_vmss.go | 30 +++++-------------- .../providers/azure/azure_fakes.go | 4 +-- .../providers/azure/azure_vmsets.go | 6 ++-- pkg/volume/azure_dd/attacher.go | 2 +- pkg/volume/azure_dd/azure_dd.go | 2 +- 7 files changed, 45 insertions(+), 53 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_controller_common.go b/pkg/cloudprovider/providers/azure/azure_controller_common.go index 7109ea73bd..1b5148d3d5 100644 --- a/pkg/cloudprovider/providers/azure/azure_controller_common.go +++ b/pkg/cloudprovider/providers/azure/azure_controller_common.go @@ -95,14 +95,32 @@ func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI stri return vmset.AttachDisk(isManagedDisk, diskName, diskURI, nodeName, lun, cachingMode) } -// DetachDiskByName detaches a vhd from host. The vhd can be identified by diskName or diskURI. -func (c *controllerCommon) DetachDiskByName(diskName, diskURI string, nodeName types.NodeName) error { +// DetachDisk detaches a disk from host. The vhd can be identified by diskName or diskURI. +func (c *controllerCommon) DetachDisk(diskName, diskURI string, nodeName types.NodeName) error { vmset, err := c.getNodeVMSet(nodeName) if err != nil { return err } - return vmset.DetachDiskByName(diskName, diskURI, nodeName) + resp, err := vmset.DetachDisk(diskName, diskURI, nodeName) + if c.cloud.CloudProviderBackoff && shouldRetryHTTPRequest(resp, err) { + klog.V(2).Infof("azureDisk - update backing off: detach disk(%s, %s), err: %v", diskName, diskURI, err) + retryErr := kwait.ExponentialBackoff(c.cloud.requestBackoff(), func() (bool, error) { + resp, err := vmset.DetachDisk(diskName, diskURI, nodeName) + return c.cloud.processHTTPRetryResponse(nil, "", resp, err) + }) + if retryErr != nil { + err = retryErr + klog.V(2).Infof("azureDisk - update abort backoff: detach disk(%s, %s), err: %v", diskName, diskURI, err) + } + } + if err != nil { + klog.Errorf("azureDisk - detach disk(%s, %s) failed, err: %v", diskName, diskURI, err) + } else { + klog.V(2).Infof("azureDisk - detach disk(%s, %s) succeeded", diskName, diskURI) + } + + return err } // getNodeDataDisks invokes vmSet interfaces to get data disks for the node. diff --git a/pkg/cloudprovider/providers/azure/azure_controller_standard.go b/pkg/cloudprovider/providers/azure/azure_controller_standard.go index b380996f4e..e19bccc31e 100644 --- a/pkg/cloudprovider/providers/azure/azure_controller_standard.go +++ b/pkg/cloudprovider/providers/azure/azure_controller_standard.go @@ -18,6 +18,7 @@ package azure import ( "fmt" + "net/http" "strings" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" @@ -88,7 +89,7 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri if strings.Contains(detail, errLeaseFailed) || strings.Contains(detail, errDiskBlobNotFound) { // if lease cannot be acquired or disk not found, immediately detach the disk and return the original error klog.V(2).Infof("azureDisk - err %v, try detach disk(%s, %s)", err, diskName, diskURI) - as.DetachDiskByName(diskName, diskURI, nodeName) + as.DetachDisk(diskName, diskURI, nodeName) } } else { klog.V(2).Infof("azureDisk - attach disk(%s, %s) succeeded", diskName, diskURI) @@ -96,20 +97,20 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri return err } -// DetachDiskByName detaches a vhd from host +// DetachDisk detaches a disk from host // the vhd can be identified by diskName or diskURI -func (as *availabilitySet) DetachDiskByName(diskName, diskURI string, nodeName types.NodeName) error { +func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.NodeName) (*http.Response, error) { vm, err := as.getVirtualMachine(nodeName) if err != nil { // if host doesn't exist, no need to detach klog.Warningf("azureDisk - cannot find node %s, skip detaching disk(%s, %s)", nodeName, diskName, diskURI) - return nil + return nil, nil } vmName := mapNodeNameToVMName(nodeName) nodeResourceGroup, err := as.GetNodeResourceGroup(vmName) if err != nil { - return err + return nil, err } disks := *vm.StorageProfile.DataDisks @@ -127,7 +128,7 @@ func (as *availabilitySet) DetachDiskByName(diskName, diskURI string, nodeName t } if !bFoundDisk { - return fmt.Errorf("detach azure disk failure, disk %s not found, diskURI: %s", diskName, diskURI) + return nil, fmt.Errorf("detach azure disk failure, disk %s not found, diskURI: %s", diskName, diskURI) } newVM := compute.VirtualMachine{ @@ -146,22 +147,7 @@ func (as *availabilitySet) DetachDiskByName(diskName, diskURI string, nodeName t // Invalidate the cache right after updating defer as.cloud.vmCache.Delete(vmName) - resp, err := as.VirtualMachinesClient.CreateOrUpdate(ctx, nodeResourceGroup, vmName, newVM) - if as.CloudProviderBackoff && shouldRetryHTTPRequest(resp, err) { - klog.V(2).Infof("azureDisk - update(%s) backing off: vm(%s) detach disk(%s, %s), err: %v", nodeResourceGroup, vmName, diskName, diskURI, err) - retryErr := as.CreateOrUpdateVMWithRetry(nodeResourceGroup, vmName, newVM) - if retryErr != nil { - err = retryErr - klog.V(2).Infof("azureDisk - update(%s) abort backoff: vm(%s) detach disk(%s, %s), err: %v", nodeResourceGroup, vmName, diskName, diskURI, err) - } - } - if err != nil { - klog.Errorf("azureDisk - detach disk(%s, %s) failed, err: %v", diskName, diskURI, err) - } else { - klog.V(2).Infof("azureDisk - detach disk(%s, %s) succeeded", diskName, diskURI) - } - - return err + return as.VirtualMachinesClient.CreateOrUpdate(ctx, nodeResourceGroup, vmName, newVM) } // GetDataDisks gets a list of data disks attached to the node. diff --git a/pkg/cloudprovider/providers/azure/azure_controller_vmss.go b/pkg/cloudprovider/providers/azure/azure_controller_vmss.go index 442dd8fedc..a76c2d575f 100644 --- a/pkg/cloudprovider/providers/azure/azure_controller_vmss.go +++ b/pkg/cloudprovider/providers/azure/azure_controller_vmss.go @@ -18,6 +18,7 @@ package azure import ( "fmt" + "net/http" "strings" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" @@ -93,7 +94,7 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod if strings.Contains(detail, errLeaseFailed) || strings.Contains(detail, errDiskBlobNotFound) { // if lease cannot be acquired or disk not found, immediately detach the disk and return the original error klog.Infof("azureDisk - err %s, try detach disk(%s, %s)", detail, diskName, diskURI) - ss.DetachDiskByName(diskName, diskURI, nodeName) + ss.DetachDisk(diskName, diskURI, nodeName) } } else { klog.V(2).Infof("azureDisk - attach disk(%s, %s) succeeded", diskName, diskURI) @@ -101,18 +102,18 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod return err } -// DetachDiskByName detaches a vhd from host +// DetachDisk detaches a disk from host // the vhd can be identified by diskName or diskURI -func (ss *scaleSet) DetachDiskByName(diskName, diskURI string, nodeName types.NodeName) error { +func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName) (*http.Response, error) { vmName := mapNodeNameToVMName(nodeName) ssName, instanceID, vm, err := ss.getVmssVM(vmName) if err != nil { - return err + return nil, err } nodeResourceGroup, err := ss.GetNodeResourceGroup(vmName) if err != nil { - return err + return nil, err } disks := []compute.DataDisk{} @@ -133,7 +134,7 @@ func (ss *scaleSet) DetachDiskByName(diskName, diskURI string, nodeName types.No } if !bFoundDisk { - return fmt.Errorf("detach azure disk failure, disk %s not found, diskURI: %s", diskName, diskURI) + return nil, fmt.Errorf("detach azure disk failure, disk %s not found, diskURI: %s", diskName, diskURI) } newVM := compute.VirtualMachineScaleSetVM{ @@ -156,22 +157,7 @@ func (ss *scaleSet) DetachDiskByName(diskName, diskURI string, nodeName types.No defer ss.vmssVMCache.Delete(key) klog.V(2).Infof("azureDisk - update(%s): vm(%s) - detach disk(%s, %s)", nodeResourceGroup, nodeName, diskName, diskURI) - resp, err := ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM) - if ss.CloudProviderBackoff && shouldRetryHTTPRequest(resp, err) { - klog.V(2).Infof("azureDisk - update(%s) backing off: vm(%s) detach disk(%s, %s), err: %v", nodeResourceGroup, nodeName, diskName, diskURI, err) - retryErr := ss.UpdateVmssVMWithRetry(nodeResourceGroup, ssName, instanceID, newVM) - if retryErr != nil { - err = retryErr - klog.V(2).Infof("azureDisk - update(%s) abort backoff: vm(%s) detach disk(%s, %s), err: %v", nodeResourceGroup, nodeName, diskName, diskURI, err) - } - } - if err != nil { - klog.Errorf("azureDisk - detach disk(%s, %s) from %s failed, err: %v", diskName, diskURI, nodeName, err) - } else { - klog.V(2).Infof("azureDisk - detach disk(%s, %s) succeeded", diskName, diskURI) - } - - return err + return ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM) } // GetDataDisks gets a list of data disks attached to the node. diff --git a/pkg/cloudprovider/providers/azure/azure_fakes.go b/pkg/cloudprovider/providers/azure/azure_fakes.go index 77cc02cb64..af8e47abbb 100644 --- a/pkg/cloudprovider/providers/azure/azure_fakes.go +++ b/pkg/cloudprovider/providers/azure/azure_fakes.go @@ -910,8 +910,8 @@ func (f *fakeVMSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod return fmt.Errorf("unimplemented") } -func (f *fakeVMSet) DetachDiskByName(diskName, diskURI string, nodeName types.NodeName) error { - return fmt.Errorf("unimplemented") +func (f *fakeVMSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName) (*http.Response, error) { + return nil, fmt.Errorf("unimplemented") } func (f *fakeVMSet) GetDataDisks(nodeName types.NodeName) ([]compute.DataDisk, error) { diff --git a/pkg/cloudprovider/providers/azure/azure_vmsets.go b/pkg/cloudprovider/providers/azure/azure_vmsets.go index 0d37a91c07..6dbdc1dd65 100644 --- a/pkg/cloudprovider/providers/azure/azure_vmsets.go +++ b/pkg/cloudprovider/providers/azure/azure_vmsets.go @@ -17,6 +17,8 @@ limitations under the License. package azure import ( + "net/http" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2017-09-01/network" @@ -60,8 +62,8 @@ type VMSet interface { // AttachDisk attaches a vhd to vm. The vhd must exist, can be identified by diskName, diskURI, and lun. AttachDisk(isManagedDisk bool, diskName, diskURI string, nodeName types.NodeName, lun int32, cachingMode compute.CachingTypes) error - // DetachDiskByName detaches a vhd from host. The vhd can be identified by diskName or diskURI. - DetachDiskByName(diskName, diskURI string, nodeName types.NodeName) error + // DetachDisk detaches a vhd from host. The vhd can be identified by diskName or diskURI. + DetachDisk(diskName, diskURI string, nodeName types.NodeName) (*http.Response, error) // GetDataDisks gets a list of data disks attached to the node. GetDataDisks(nodeName types.NodeName) ([]compute.DataDisk, error) diff --git a/pkg/volume/azure_dd/attacher.go b/pkg/volume/azure_dd/attacher.go index 90a0bf414d..b9453f7015 100644 --- a/pkg/volume/azure_dd/attacher.go +++ b/pkg/volume/azure_dd/attacher.go @@ -301,7 +301,7 @@ func (d *azureDiskDetacher) Detach(diskURI string, nodeName types.NodeName) erro getLunMutex.LockKey(instanceid) defer getLunMutex.UnlockKey(instanceid) - err = diskController.DetachDiskByName("", diskURI, nodeName) + err = diskController.DetachDisk("", diskURI, nodeName) if err != nil { klog.Errorf("failed to detach azure disk %q, err %v", diskURI, err) } diff --git a/pkg/volume/azure_dd/azure_dd.go b/pkg/volume/azure_dd/azure_dd.go index b5b040b4ab..29bde18dc9 100644 --- a/pkg/volume/azure_dd/azure_dd.go +++ b/pkg/volume/azure_dd/azure_dd.go @@ -46,7 +46,7 @@ type DiskController interface { // Attaches the disk to the host machine. AttachDisk(isManagedDisk bool, diskName, diskUri string, nodeName types.NodeName, lun int32, cachingMode compute.CachingTypes) error // Detaches the disk, identified by disk name or uri, from the host machine. - DetachDiskByName(diskName, diskUri string, nodeName types.NodeName) error + DetachDisk(diskName, diskUri string, nodeName types.NodeName) error // Check if a list of volumes are attached to the node with the specified NodeName DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error)