Skip to content

Commit

Permalink
fix: add disk lun check in AttachDisk to avoid race condition
Browse files Browse the repository at this point in the history
  • Loading branch information
@andyzhangx
andyzhangx committed Nov 19, 2022
1 parent 347015c commit d4df985
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 47 deletions.
13 changes: 9 additions & 4 deletions pkg/provider/azure_controller_standard.go
View file
Expand Up @@ -18,6 +18,7 @@ package provider

import (
"context"
"fmt"
"net/http"
"strings"

Expand Down Expand Up @@ -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
}

Expand Down
34 changes: 25 additions & 9 deletions pkg/provider/azure_controller_standard_test.go
View file
Expand Up @@ -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",
Expand All @@ -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,
},
}

Expand All @@ -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()
Expand All @@ -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,
}
Expand Down
13 changes: 9 additions & 4 deletions pkg/provider/azure_controller_vmss.go
View file
Expand Up @@ -18,6 +18,7 @@ package provider

import (
"context"
"fmt"
"net/http"
"strings"

Expand Down Expand Up @@ -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
}

Expand Down
74 changes: 44 additions & 30 deletions pkg/provider/azure_controller_vmss_test.go
View file
Expand Up @@ -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"),
},
}

Expand Down Expand Up @@ -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()
Expand All @@ -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)
}
}

Expand Down

0 comments on commit d4df985

Please sign in to comment.