From 3f9ca0a3209a2ac6ce225cc3adf2df4147c61b64 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Tue, 9 Jun 2020 14:24:21 +0000 Subject: [PATCH 1/2] fix: use force detach for azure disk revert attach disk change revert some change fix: should not filter out disk in attach azure disk --- .../azure/azure_controller_standard.go | 8 +++++--- .../legacy-cloud-providers/azure/azure_controller_vmss.go | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go index ab154c14704..54ca4f880ff 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go @@ -44,7 +44,8 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri return err } - disks := filterDetachingDisks(*vm.StorageProfile.DataDisks) + disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) + copy(disks, *vm.StorageProfile.DataDisks) if isManagedDisk { managedDisk := &compute.ManagedDiskParameters{ID: &diskURI} @@ -131,7 +132,8 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N return err } - disks := filterDetachingDisks(*vm.StorageProfile.DataDisks) + disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) + copy(disks, *vm.StorageProfile.DataDisks) bFoundDisk := false for i, disk := range disks { @@ -140,7 +142,7 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N (disk.ManagedDisk != nil && diskURI != "" && strings.EqualFold(*disk.ManagedDisk.ID, diskURI)) { // found the disk klog.V(2).Infof("azureDisk - detach disk: name %q uri %q", diskName, diskURI) - disks = append(disks[:i], disks[i+1:]...) + disks[i].ToBeDetached = to.BoolPtr(true) bFoundDisk = true break } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go index 75443dfb959..344e4f03d4a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go @@ -46,7 +46,8 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod disks := []compute.DataDisk{} if vm.StorageProfile != nil && vm.StorageProfile.DataDisks != nil { - disks = filterDetachingDisks(*vm.StorageProfile.DataDisks) + disks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) + copy(disks, *vm.StorageProfile.DataDisks) } if isManagedDisk { managedDisk := &compute.ManagedDiskParameters{ID: &diskURI} @@ -136,7 +137,8 @@ func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName disks := []compute.DataDisk{} if vm.StorageProfile != nil && vm.StorageProfile.DataDisks != nil { - disks = filterDetachingDisks(*vm.StorageProfile.DataDisks) + disks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) + copy(disks, *vm.StorageProfile.DataDisks) } bFoundDisk := false for i, disk := range disks { @@ -145,7 +147,7 @@ func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName (disk.ManagedDisk != nil && diskURI != "" && strings.EqualFold(*disk.ManagedDisk.ID, diskURI)) { // found the disk klog.V(2).Infof("azureDisk - detach disk: name %q uri %q", diskName, diskURI) - disks = append(disks[:i], disks[i+1:]...) + disks[i].ToBeDetached = to.BoolPtr(true) bFoundDisk = true break } From 5f710324cb6df428e2e34d5274949f65e93081b1 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Thu, 18 Jun 2020 08:02:02 +0000 Subject: [PATCH 2/2] test: add unit test --- .../azure/azure_controller_standard_test.go | 4 ++++ .../azure/azure_controller_vmss_test.go | 19 +++++++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard_test.go index daae34a46bc..0303da77ba8 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard_test.go @@ -163,6 +163,10 @@ func TestStandardDetachDisk(t *testing.T) { err := vmSet.DetachDisk(test.diskName, "", test.nodeName) assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc) + if !test.expectedError && test.diskName != "" { + dataDisks, err := vmSet.GetDataDisks(test.nodeName, azcache.CacheReadTypeDefault) + assert.Equal(t, true, len(dataDisks) == 1, "TestCase[%d]: %s, err: %v", i, test.desc, err) + } } } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss_test.go index ebd82a1c020..18afa6046f2 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss_test.go @@ -141,6 +141,7 @@ func TestDetachDiskWithVMSS(t *testing.T) { defer ctrl.Finish() fakeStatusNotFoundVMSSName := types.NodeName("FakeStatusNotFoundVMSSName") + diskName := "disk-name" testCases := []struct { desc string vmList map[string]string @@ -156,7 +157,7 @@ func TestDetachDiskWithVMSS(t *testing.T) { vmssVMList: []string{"vmss-vm-000001"}, vmssName: "vm1", vmssvmName: "vm1", - existedDisk: compute.Disk{Name: to.StringPtr("disk-name")}, + existedDisk: compute.Disk{Name: to.StringPtr(diskName)}, expectedErr: true, expectedErrMsg: fmt.Errorf("not a vmss instance"), }, @@ -165,7 +166,7 @@ func TestDetachDiskWithVMSS(t *testing.T) { vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, vmssName: "vmss00", vmssvmName: "vmss00-vm-000000", - existedDisk: compute.Disk{Name: to.StringPtr("disk-name")}, + existedDisk: compute.Disk{Name: to.StringPtr(diskName)}, expectedErr: false, }, { @@ -173,7 +174,7 @@ func TestDetachDiskWithVMSS(t *testing.T) { vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"}, vmssName: fakeStatusNotFoundVMSSName, vmssvmName: "vmss00-vm-000000", - existedDisk: compute.Disk{Name: to.StringPtr("disk-name")}, + existedDisk: compute.Disk{Name: to.StringPtr(diskName)}, expectedErr: true, expectedErrMsg: fmt.Errorf("Retriable: false, RetryAfter: 0s, HTTPStatusCode: 404, RawError: instance not found"), }, @@ -213,7 +214,7 @@ func TestDetachDiskWithVMSS(t *testing.T) { }, DataDisks: &[]compute.DataDisk{{ Lun: to.Int32Ptr(0), - Name: to.StringPtr("disk-name"), + Name: to.StringPtr(diskName), }}, } } @@ -225,12 +226,14 @@ func TestDetachDiskWithVMSS(t *testing.T) { mockVMSSVMClient.EXPECT().Update(gomock.Any(), testCloud.ResourceGroup, scaleSetName, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() } - diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s", - testCloud.SubscriptionID, testCloud.ResourceGroup, *test.existedDisk.Name) - - err = ss.DetachDisk(*test.existedDisk.Name, diskURI, test.vmssvmName) + err = ss.DetachDisk(*test.existedDisk.Name, diskName, test.vmssvmName) assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, err: %v", i, test.desc, err) assert.Equal(t, test.expectedErrMsg, err, "TestCase[%d]: %s, expected error: %v, return error: %v", i, test.desc, test.expectedErrMsg, err) + + if !test.expectedErr { + dataDisks, err := ss.GetDataDisks(test.vmssvmName, azcache.CacheReadTypeDefault) + assert.Equal(t, true, len(dataDisks) == 1, "TestCase[%d]: %s, actual data disk num: %d, err: %v", i, test.desc, len(dataDisks), err) + } } }