Skip to content

Commit 327dd9d

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#88444 from andyzhangx/azuredisk-remediator
fix: add remediation in azure disk attach/detach
2 parents 0225798 + 0e3b7a7 commit 327dd9d

File tree

4 files changed

+200
-19
lines changed

4 files changed

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

‎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)