From d4df985c6590253144426d8354c49a4b0262fa68 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sat, 19 Nov 2022 08:16:19 +0000 Subject: [PATCH] fix: add disk lun check in AttachDisk to avoid race condition --- pkg/provider/azure_controller_standard.go | 13 +++- .../azure_controller_standard_test.go | 34 ++++++--- pkg/provider/azure_controller_vmss.go | 13 +++- pkg/provider/azure_controller_vmss_test.go | 74 +++++++++++-------- 4 files changed, 87 insertions(+), 47 deletions(-) diff --git a/pkg/provider/azure_controller_standard.go b/pkg/provider/azure_controller_standard.go index 49f4a47151..e1d503bc2a 100644 --- a/pkg/provider/azure_controller_standard.go +++ b/pkg/provider/azure_controller_standard.go @@ -18,6 +18,7 @@ package provider import ( "context" + "fmt" "net/http" "strings" @@ -53,13 +54,17 @@ func (as *availabilitySet) AttachDisk(ctx context.Context, nodeName types.NodeNa opt := v attached := false for _, disk := range *vm.StorageProfile.DataDisks { - if disk.ManagedDisk != nil && strings.EqualFold(*disk.ManagedDisk.ID, diskURI) { - attached = true - break + if disk.ManagedDisk != nil && strings.EqualFold(*disk.ManagedDisk.ID, diskURI) && disk.Lun != nil { + if *disk.Lun == opt.lun { + attached = true + break + } else { + return nil, fmt.Errorf("disk(%s) already attached to node(%s) on LUN(%d), but target LUN is %d", diskURI, nodeName, *disk.Lun, opt.lun) + } } } if attached { - klog.V(2).Infof("azureDisk - disk(%s) already attached to node(%s)", diskURI, nodeName) + klog.V(2).Infof("azureDisk - disk(%s) already attached to node(%s) on LUN(%d)", diskURI, nodeName, opt.lun) continue } diff --git a/pkg/provider/azure_controller_standard_test.go b/pkg/provider/azure_controller_standard_test.go index 321b24f69e..3c800bb403 100644 --- a/pkg/provider/azure_controller_standard_test.go +++ b/pkg/provider/azure_controller_standard_test.go @@ -47,10 +47,11 @@ func TestStandardAttachDisk(t *testing.T) { defer cancel() testCases := []struct { - desc string - nodeName types.NodeName - isAttachFail bool - expectedErr bool + desc string + nodeName types.NodeName + inconsistentLUN bool + isAttachFail bool + expectedErr bool }{ { desc: "an error shall be returned if there's no corresponding vms", @@ -68,10 +69,15 @@ func TestStandardAttachDisk(t *testing.T) { expectedErr: false, }, { - desc: "an error shall be returned if update attach disk failed", - nodeName: "vm1", - isAttachFail: true, - expectedErr: true, + desc: "an error shall be returned if update attach disk failed", + nodeName: "vm1", + inconsistentLUN: true, + expectedErr: true, + }, + { + desc: "error should be returned when disk lun is inconsistent", + nodeName: "vm1", + expectedErr: false, }, } @@ -93,6 +99,13 @@ func TestStandardAttachDisk(t *testing.T) { }, DataDisks: &[]compute.DataDisk{}, } + if test.inconsistentLUN { + diskName := "disk-name" + diskURI := "uri" + vm.StorageProfile.DataDisks = &[]compute.DataDisk{ + {Lun: to.Int32Ptr(0), Name: &diskName, ManagedDisk: &compute.ManagedDiskParameters{ID: &diskURI}}, + } + } mockVMsClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, *vm.Name, gomock.Any()).Return(vm, nil).AnyTimes() } mockVMsClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, "vm2", gomock.Any()).Return(compute.VirtualMachine{}, &retry.Error{HTTPStatusCode: http.StatusNotFound, RawError: cloudprovider.InstanceNotFound}).AnyTimes() @@ -104,11 +117,14 @@ func TestStandardAttachDisk(t *testing.T) { options := AttachDiskOptions{ lun: 0, - diskName: "", + diskName: "disk-name", cachingMode: compute.CachingTypesReadOnly, diskEncryptionSetID: "", writeAcceleratorEnabled: false, } + if test.inconsistentLUN { + options.lun = 63 + } diskMap := map[string]*AttachDiskOptions{ "uri": &options, } diff --git a/pkg/provider/azure_controller_vmss.go b/pkg/provider/azure_controller_vmss.go index c10271a9a1..357432df48 100644 --- a/pkg/provider/azure_controller_vmss.go +++ b/pkg/provider/azure_controller_vmss.go @@ -18,6 +18,7 @@ package provider import ( "context" + "fmt" "net/http" "strings" @@ -60,13 +61,17 @@ func (ss *ScaleSet) AttachDisk(ctx context.Context, nodeName types.NodeName, dis opt := v attached := false for _, disk := range *storageProfile.DataDisks { - if disk.ManagedDisk != nil && strings.EqualFold(*disk.ManagedDisk.ID, diskURI) { - attached = true - break + if disk.ManagedDisk != nil && strings.EqualFold(*disk.ManagedDisk.ID, diskURI) && disk.Lun != nil { + if *disk.Lun == opt.lun { + attached = true + break + } else { + return nil, fmt.Errorf("disk(%s) already attached to node(%s) on LUN(%d), but target LUN is %d", diskURI, nodeName, *disk.Lun, opt.lun) + } } } if attached { - klog.V(2).Infof("azureDisk - disk(%s) already attached to node(%s)", diskURI, nodeName) + klog.V(2).Infof("azureDisk - disk(%s) already attached to node(%s) on LUN(%d)", diskURI, nodeName, opt.lun) continue } diff --git a/pkg/provider/azure_controller_vmss_test.go b/pkg/provider/azure_controller_vmss_test.go index 3fcb6e14d2..754242ce4f 100644 --- a/pkg/provider/azure_controller_vmss_test.go +++ b/pkg/provider/azure_controller_vmss_test.go @@ -45,38 +45,44 @@ func TestAttachDiskWithVMSS(t *testing.T) { fakeStatusNotFoundVMSSName := types.NodeName("FakeStatusNotFoundVMSSName") testCases := []struct { - desc string - vmssName types.NodeName - vmssvmName types.NodeName - disks []string - vmList map[string]string - vmssVMList []string - expectedErr bool - expectedErrMsg error + desc string + vmssName types.NodeName + vmssvmName types.NodeName + disks []string + vmList map[string]string + vmssVMList []string + inconsistentLUN bool + expectedErr error }{ { - desc: "no error shall be returned if everything is good with one managed disk", - vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, - vmssName: "vmss00", - vmssvmName: "vmss00-vm-000000", - disks: []string{"disk-name"}, - expectedErr: false, + desc: "no error shall be returned if everything is good with one managed disk", + vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, + vmssName: "vmss00", + vmssvmName: "vmss00-vm-000000", + disks: []string{"disk-name"}, }, { - desc: "no error shall be returned if everything is good with 2 managed disks", - vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, - vmssName: "vmss00", - vmssvmName: "vmss00-vm-000000", - disks: []string{"disk-name", "disk-name2"}, - expectedErr: false, + desc: "no error shall be returned if everything is good with 2 managed disks", + vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, + vmssName: "vmss00", + vmssvmName: "vmss00-vm-000000", + disks: []string{"disk-name", "disk-name2"}, }, { - desc: "no error shall be returned if everything is good with non-managed disk", - vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, - vmssName: "vmss00", - vmssvmName: "vmss00-vm-000000", - disks: []string{"disk-name"}, - expectedErr: false, + desc: "no error shall be returned if everything is good with non-managed disk", + vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, + vmssName: "vmss00", + vmssvmName: "vmss00-vm-000000", + disks: []string{"disk-name"}, + }, + { + desc: "error should be returned when disk lun is inconsistent", + vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, + vmssName: "vmss00", + vmssvmName: "vmss00-vm-000000", + disks: []string{"disk-name", "disk-name2"}, + inconsistentLUN: true, + expectedErr: fmt.Errorf("disk(/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/disks/disk-name) already attached to node(vmss00-vm-000000) on LUN(0), but target LUN is 63"), }, } @@ -108,6 +114,14 @@ func TestAttachDiskWithVMSS(t *testing.T) { }, DataDisks: &[]compute.DataDisk{}, } + if test.inconsistentLUN { + diskName := "disk-name" + diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s", + testCloud.SubscriptionID, testCloud.ResourceGroup, diskName) + vmssvm.StorageProfile.DataDisks = &[]compute.DataDisk{ + {Lun: to.Int32Ptr(0), Name: &diskName, ManagedDisk: &compute.ManagedDiskParameters{ID: &diskURI}}, + } + } } mockVMSSVMClient := testCloud.VirtualMachineScaleSetVMsClient.(*mockvmssvmclient.MockInterface) mockVMSSVMClient.EXPECT().List(gomock.Any(), testCloud.ResourceGroup, scaleSetName, gomock.Any()).Return(expectedVMSSVMs, nil).AnyTimes() @@ -126,16 +140,16 @@ func TestAttachDiskWithVMSS(t *testing.T) { diskEncryptionSetID: "", writeAcceleratorEnabled: true, } + if test.inconsistentLUN { + options.lun = 63 + } diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s", testCloud.SubscriptionID, testCloud.ResourceGroup, diskName) diskMap[diskURI] = &options } _, err = ss.AttachDisk(ctx, test.vmssvmName, diskMap) - assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, return error: %v", i, test.desc, err) - if test.expectedErr { - assert.EqualError(t, test.expectedErrMsg, err.Error(), "TestCase[%d]: %s, expected error: %v, return error: %v", i, test.desc, test.expectedErrMsg, err) - } + assert.Equal(t, test.expectedErr, err, "TestCase[%d]: %s, expected error: %v, return error: %v", i, test.desc, test.expectedErr, err) } }