Skip to content

Commit e5bb6af

Browse files
andyzhangxandyzhangx
committed
fix: add remediation in azure disk attach/detach
add one comment
1 parent ac25069 commit e5bb6af

File tree

4 files changed

+194
-19
lines changed

4 files changed

+194
-19
lines changed

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ package azure
2121
import (
2222
"context"
2323
"fmt"
24+
"net/http"
2425
"path"
2526
"regexp"
2627
"strings"
@@ -345,6 +346,48 @@ func filterDetachingDisks(unfilteredDisks []compute.DataDisk) []compute.DataDisk
345346
return filteredDisks
346347
}
347348

349+
func (c *controllerCommon) filterNonExistingDisks(ctx context.Context, unfilteredDisks []compute.DataDisk) []compute.DataDisk {
350+
filteredDisks := []compute.DataDisk{}
351+
for _, disk := range unfilteredDisks {
352+
filter := false
353+
if disk.ManagedDisk != nil && disk.ManagedDisk.ID != nil {
354+
diskURI := *disk.ManagedDisk.ID
355+
exist, err := c.cloud.checkDiskExists(ctx, diskURI)
356+
if err != nil {
357+
klog.Errorf("checkDiskExists(%s) failed with error: %v", diskURI, err)
358+
} else {
359+
// only filter disk when checkDiskExists returns <false, nil>
360+
filter = !exist
361+
if filter {
362+
klog.Errorf("disk(%s) does not exist, removed from data disk list", diskURI)
363+
}
364+
}
365+
}
366+
367+
if !filter {
368+
filteredDisks = append(filteredDisks, disk)
369+
}
370+
}
371+
return filteredDisks
372+
}
373+
374+
func (c *controllerCommon) checkDiskExists(ctx context.Context, diskURI string) (bool, error) {
375+
diskName := path.Base(diskURI)
376+
resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
377+
if err != nil {
378+
return false, err
379+
}
380+
381+
if _, rerr := c.cloud.DisksClient.Get(ctx, resourceGroup, diskName); rerr != nil {
382+
if rerr.HTTPStatusCode == http.StatusNotFound {
383+
return false, nil
384+
}
385+
return false, rerr.Error()
386+
}
387+
388+
return true, nil
389+
}
390+
348391
func getValidCreationData(subscriptionID, resourceGroup, sourceResourceID, sourceType string) (compute.CreationData, error) {
349392
if sourceResourceID == "" {
350393
return compute.CreationData{

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

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,3 +427,113 @@ func TestGetValidCreationData(t *testing.T) {
427427
}
428428
}
429429
}
430+
431+
func TestCheckDiskExists(t *testing.T) {
432+
ctx, cancel := getContextWithCancel()
433+
defer cancel()
434+
435+
testCloud := GetTestCloud()
436+
common := &controllerCommon{
437+
location: testCloud.Location,
438+
storageEndpointSuffix: testCloud.Environment.StorageEndpointSuffix,
439+
resourceGroup: testCloud.ResourceGroup,
440+
subscriptionID: testCloud.SubscriptionID,
441+
cloud: testCloud,
442+
vmLockMap: newLockMap(),
443+
}
444+
// create a new disk before running test
445+
newDiskName := "newdisk"
446+
newDiskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
447+
testCloud.SubscriptionID, testCloud.ResourceGroup, newDiskName)
448+
fDC := newFakeDisksClient()
449+
rerr := fDC.CreateOrUpdate(ctx, testCloud.ResourceGroup, newDiskName, compute.Disk{})
450+
assert.Equal(t, rerr == nil, true, "return error: %v", rerr)
451+
testCloud.DisksClient = fDC
452+
453+
testCases := []struct {
454+
diskURI string
455+
expectedResult bool
456+
expectedErr bool
457+
}{
458+
{
459+
diskURI: "incorrect disk URI format",
460+
expectedResult: false,
461+
expectedErr: true,
462+
},
463+
{
464+
diskURI: "/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Compute/disks/non-existing-disk",
465+
expectedResult: false,
466+
expectedErr: false,
467+
},
468+
{
469+
diskURI: newDiskURI,
470+
expectedResult: true,
471+
expectedErr: false,
472+
},
473+
}
474+
475+
for i, test := range testCases {
476+
exist, err := common.checkDiskExists(ctx, test.diskURI)
477+
assert.Equal(t, test.expectedResult, exist, "TestCase[%d]", i, exist)
478+
assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d], return error: %v", i, err)
479+
}
480+
}
481+
482+
func TestFilterNonExistingDisks(t *testing.T) {
483+
ctx, cancel := getContextWithCancel()
484+
defer cancel()
485+
486+
testCloud := GetTestCloud()
487+
common := &controllerCommon{
488+
location: testCloud.Location,
489+
storageEndpointSuffix: testCloud.Environment.StorageEndpointSuffix,
490+
resourceGroup: testCloud.ResourceGroup,
491+
subscriptionID: testCloud.SubscriptionID,
492+
cloud: testCloud,
493+
vmLockMap: newLockMap(),
494+
}
495+
// create a new disk before running test
496+
diskURIPrefix := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/",
497+
testCloud.SubscriptionID, testCloud.ResourceGroup)
498+
newDiskName := "newdisk"
499+
newDiskURI := diskURIPrefix + newDiskName
500+
fDC := newFakeDisksClient()
501+
rerr := fDC.CreateOrUpdate(ctx, testCloud.ResourceGroup, newDiskName, compute.Disk{})
502+
assert.Equal(t, rerr == nil, true, "return error: %v", rerr)
503+
testCloud.DisksClient = fDC
504+
505+
disks := []compute.DataDisk{
506+
{
507+
Name: &newDiskName,
508+
ManagedDisk: &compute.ManagedDiskParameters{
509+
ID: &newDiskURI,
510+
},
511+
},
512+
{
513+
Name: pointer.StringPtr("DiskName2"),
514+
ManagedDisk: &compute.ManagedDiskParameters{
515+
ID: pointer.StringPtr(diskURIPrefix + "DiskName2"),
516+
},
517+
},
518+
{
519+
Name: pointer.StringPtr("DiskName3"),
520+
ManagedDisk: &compute.ManagedDiskParameters{
521+
ID: pointer.StringPtr(diskURIPrefix + "DiskName3"),
522+
},
523+
},
524+
{
525+
Name: pointer.StringPtr("DiskName4"),
526+
ManagedDisk: &compute.ManagedDiskParameters{
527+
ID: pointer.StringPtr(diskURIPrefix + "DiskName4"),
528+
},
529+
},
530+
}
531+
532+
filteredDisks := common.filterNonExistingDisks(ctx, disks)
533+
assert.Equal(t, 1, len(filteredDisks))
534+
assert.Equal(t, newDiskName, *filteredDisks[0].Name)
535+
536+
disks = []compute.DataDisk{}
537+
filteredDisks = filterDetachingDisks(disks)
538+
assert.Equal(t, 0, len(filteredDisks))
539+
}

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

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ limitations under the License.
1919
package azure
2020

2121
import (
22+
"net/http"
2223
"strings"
2324

2425
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-07-01/compute"
@@ -98,18 +99,18 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri
9899

99100
rerr := as.VirtualMachinesClient.Update(ctx, nodeResourceGroup, vmName, newVM, "attach_disk")
100101
if rerr != nil {
101-
klog.Errorf("azureDisk - attach disk(%s, %s) failed, err: %v", diskName, diskURI, rerr)
102-
detail := rerr.Error().Error()
103-
if strings.Contains(detail, errLeaseFailed) || strings.Contains(detail, errDiskBlobNotFound) {
104-
// if lease cannot be acquired or disk not found, immediately detach the disk and return the original error
105-
klog.V(2).Infof("azureDisk - err %v, try detach disk(%s, %s)", rerr, diskName, diskURI)
106-
as.DetachDisk(diskName, diskURI, nodeName)
102+
klog.Errorf("azureDisk - attach disk(%s, %s) on rg(%s) vm(%s) failed, err: %v", diskName, diskURI, nodeResourceGroup, vmName, rerr)
103+
if rerr.HTTPStatusCode == http.StatusNotFound {
104+
klog.Errorf("azureDisk - begin to filterNonExistingDisks(%s, %s) on rg(%s) vm(%s)", diskName, diskURI, nodeResourceGroup, vmName)
105+
disks := as.filterNonExistingDisks(ctx, *newVM.VirtualMachineProperties.StorageProfile.DataDisks)
106+
newVM.VirtualMachineProperties.StorageProfile.DataDisks = &disks
107+
if rerr = as.VirtualMachinesClient.Update(ctx, nodeResourceGroup, vmName, newVM, "attach_disk"); rerr != nil {
108+
return rerr.Error()
109+
}
107110
}
108-
109-
return rerr.Error()
110111
}
111112

112-
klog.V(2).Infof("azureDisk - attach disk(%s, %s) succeeded", diskName, diskURI)
113+
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - attach disk(%s, %s) succeeded", nodeResourceGroup, vmName, diskName, diskURI)
113114
return nil
114115
}
115116

@@ -166,9 +167,18 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N
166167

167168
rerr := as.VirtualMachinesClient.Update(ctx, nodeResourceGroup, vmName, newVM, "detach_disk")
168169
if rerr != nil {
169-
return rerr.Error()
170+
klog.Errorf("azureDisk - detach disk(%s, %s) on rg(%s) vm(%s) failed, err: %v", diskName, diskURI, nodeResourceGroup, vmName, rerr)
171+
if rerr.HTTPStatusCode == http.StatusNotFound {
172+
klog.Errorf("azureDisk - begin to filterNonExistingDisks(%s, %s) on rg(%s) vm(%s)", diskName, diskURI, nodeResourceGroup, vmName)
173+
disks := as.filterNonExistingDisks(ctx, *vm.StorageProfile.DataDisks)
174+
newVM.VirtualMachineProperties.StorageProfile.DataDisks = &disks
175+
if rerr = as.VirtualMachinesClient.Update(ctx, nodeResourceGroup, vmName, newVM, "detach_disk"); rerr != nil {
176+
return rerr.Error()
177+
}
178+
}
170179
}
171180

181+
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - detach disk(%s, %s) succeeded", nodeResourceGroup, vmName, diskName, diskURI)
172182
return nil
173183
}
174184

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

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ limitations under the License.
1919
package azure
2020

2121
import (
22+
"net/http"
2223
"strings"
2324

2425
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-07-01/compute"
@@ -103,17 +104,18 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod
103104
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - attach disk(%s, %s) with DiskEncryptionSetID(%s)", nodeResourceGroup, nodeName, diskName, diskURI, diskEncryptionSetID)
104105
rerr := ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM, "attach_disk")
105106
if rerr != nil {
106-
detail := rerr.Error().Error()
107-
if strings.Contains(detail, errLeaseFailed) || strings.Contains(detail, errDiskBlobNotFound) {
108-
// if lease cannot be acquired or disk not found, immediately detach the disk and return the original error
109-
klog.Infof("azureDisk - err %s, try detach disk(%s, %s)", detail, diskName, diskURI)
110-
ss.DetachDisk(diskName, diskURI, nodeName)
107+
klog.Errorf("azureDisk - attach disk(%s, %s) on rg(%s) vm(%s) failed, err: %v", diskName, diskURI, nodeResourceGroup, nodeName, rerr)
108+
if rerr.HTTPStatusCode == http.StatusNotFound {
109+
klog.Errorf("azureDisk - begin to filterNonExistingDisks(%s, %s) on rg(%s) vm(%s)", diskName, diskURI, nodeResourceGroup, nodeName)
110+
disks := ss.filterNonExistingDisks(ctx, *newVM.VirtualMachineScaleSetVMProperties.StorageProfile.DataDisks)
111+
newVM.VirtualMachineScaleSetVMProperties.StorageProfile.DataDisks = &disks
112+
if rerr = ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM, "attach_disk"); rerr != nil {
113+
return rerr.Error()
114+
}
111115
}
112-
113-
return rerr.Error()
114116
}
115117

116-
klog.V(2).Infof("azureDisk - attach disk(%s, %s) succeeded", diskName, diskURI)
118+
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - attach disk(%s, %s) succeeded", nodeResourceGroup, nodeName, diskName, diskURI)
117119
return nil
118120
}
119121

@@ -174,9 +176,19 @@ func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName
174176
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - detach disk(%s, %s)", nodeResourceGroup, nodeName, diskName, diskURI)
175177
rerr := ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM, "detach_disk")
176178
if rerr != nil {
177-
return rerr.Error()
179+
klog.Errorf("azureDisk - detach disk(%s, %s) on rg(%s) vm(%s) failed, err: %v", diskName, diskURI, nodeResourceGroup, nodeName, rerr)
180+
if rerr.HTTPStatusCode == http.StatusNotFound {
181+
klog.Errorf("azureDisk - begin to filterNonExistingDisks(%s, %s) on rg(%s) vm(%s)", diskName, diskURI, nodeResourceGroup, nodeName)
182+
disks := ss.filterNonExistingDisks(ctx, *newVM.VirtualMachineScaleSetVMProperties.StorageProfile.DataDisks)
183+
newVM.VirtualMachineScaleSetVMProperties.StorageProfile.DataDisks = &disks
184+
if rerr = ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM, "detach_disk"); rerr != nil {
185+
return rerr.Error()
186+
}
187+
}
178188
}
179189

190+
klog.V(2).Infof("azureDisk - update(%s): vm(%s) - detach disk(%s, %s) succeeded", nodeResourceGroup, nodeName, diskName, diskURI)
191+
180192
return nil
181193
}
182194

0 commit comments

Comments
 (0)