Skip to content

Commit

Permalink
feat: add forceDetach parameter in DetachDisk function
Browse files Browse the repository at this point in the history
  • Loading branch information
andyzhangx authored and k8s-infra-cherrypick-robot committed Mar 5, 2024
1 parent 931dfa6 commit 54386c1
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 8 deletions.
5 changes: 4 additions & 1 deletion pkg/provider/azure_controller_standard.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func (as *availabilitySet) WaitForUpdateResult(ctx context.Context, future *azur
}

// DetachDisk detaches a disk from VM
func (as *availabilitySet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string) error {
func (as *availabilitySet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string, forceDetach bool) error {
vm, err := as.getVirtualMachine(nodeName, azcache.CacheReadTypeDefault)
if err != nil {
// if host doesn't exist, no need to detach
Expand All @@ -179,6 +179,9 @@ func (as *availabilitySet) DetachDisk(ctx context.Context, nodeName types.NodeNa
// found the disk
klog.V(2).Infof("azureDisk - detach disk: name %s uri %s", diskName, diskURI)
disks[i].ToBeDetached = pointer.Bool(true)
if forceDetach {
disks[i].DetachOption = compute.ForceDetach
}
bFoundDisk = true
}
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/provider/azure_controller_standard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ func TestStandardDetachDisk(t *testing.T) {
nodeName types.NodeName
disks []string
isDetachFail bool
forceDetach bool
expectedError bool
}{
{
Expand Down Expand Up @@ -178,6 +179,13 @@ func TestStandardDetachDisk(t *testing.T) {
isDetachFail: true,
expectedError: true,
},
{
desc: "no error shall be returned if there's a corresponding disk with forceDetach",
nodeName: "vm1",
disks: []string{"disk1"},
forceDetach: true,
expectedError: false,
},
}

for i, test := range testCases {
Expand All @@ -201,7 +209,7 @@ func TestStandardDetachDisk(t *testing.T) {
testCloud.SubscriptionID, testCloud.ResourceGroup, diskName)
diskMap[diskURI] = diskName
}
err := vmSet.DetachDisk(ctx, test.nodeName, diskMap)
err := vmSet.DetachDisk(ctx, test.nodeName, diskMap, test.forceDetach)
assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
if !test.expectedError && len(test.disks) > 0 {
dataDisks, _, err := vmSet.GetDataDisks(test.nodeName, azcache.CacheReadTypeDefault)
Expand Down
5 changes: 4 additions & 1 deletion pkg/provider/azure_controller_vmss.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (ss *ScaleSet) WaitForUpdateResult(ctx context.Context, future *azure.Futur
}

// DetachDisk detaches a disk from VM
func (ss *ScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string) error {
func (ss *ScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string, forceDetach bool) error {
vmName := mapNodeNameToVMName(nodeName)
vm, err := ss.getVmssVM(vmName, azcache.CacheReadTypeDefault)
if err != nil {
Expand Down Expand Up @@ -192,6 +192,9 @@ func (ss *ScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, dis
// found the disk
klog.V(2).Infof("azureDisk - detach disk: name %s uri %s", diskName, diskURI)
disks[i].ToBeDetached = pointer.Bool(true)
if forceDetach {
disks[i].DetachOption = compute.ForceDetach
}
bFoundDisk = true
}
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/provider/azure_controller_vmss_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ func TestDetachDiskWithVMSS(t *testing.T) {
vmssName types.NodeName
vmssvmName types.NodeName
disks []string
forceDetach bool
expectedErr bool
expectedErrMsg error
}{
Expand Down Expand Up @@ -201,6 +202,15 @@ func TestDetachDiskWithVMSS(t *testing.T) {
disks: []string{diskName, "disk2"},
expectedErr: false,
},
{
desc: "no error shall be returned with force detach",
vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"},
vmssName: "vmss00",
vmssvmName: "vmss00-vm-000000",
disks: []string{diskName, "disk2"},
forceDetach: true,
expectedErr: false,
},
{
desc: "an error shall be returned if response StatusNotFound",
vmssVMList: []string{"vmss00-vm-000000", "vmss00-vm-000001", "vmss00-vm-000002"},
Expand Down Expand Up @@ -282,7 +292,7 @@ func TestDetachDiskWithVMSS(t *testing.T) {
testCloud.SubscriptionID, testCloud.ResourceGroup, diskName)
diskMap[diskURI] = diskName
}
err = ss.DetachDisk(ctx, test.vmssvmName, diskMap)
err = ss.DetachDisk(ctx, test.vmssvmName, diskMap, test.forceDetach)
assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, err: %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)
Expand Down
5 changes: 4 additions & 1 deletion pkg/provider/azure_controller_vmssflex.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (fs *FlexScaleSet) AttachDisk(ctx context.Context, nodeName types.NodeName,
}

// DetachDisk detaches a disk from VM
func (fs *FlexScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string) error {
func (fs *FlexScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string, forceDetach bool) error {
vmName := mapNodeNameToVMName(nodeName)
vm, err := fs.getVmssFlexVM(vmName, azcache.CacheReadTypeDefault)
if err != nil {
Expand All @@ -148,6 +148,9 @@ func (fs *FlexScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName,
// found the disk
klog.V(2).Infof("azureDisk - detach disk: name %s uri %s", diskName, diskURI)
disks[i].ToBeDetached = pointer.Bool(true)
if forceDetach {
disks[i].DetachOption = compute.ForceDetach
}
bFoundDisk = true
}
}
Expand Down
15 changes: 14 additions & 1 deletion pkg/provider/azure_controller_vmssflex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ func TestDettachDiskWithVmssFlex(t *testing.T) {
vmName string
testVMListWithoutInstanceView []compute.VirtualMachine
testVMListWithOnlyInstanceView []compute.VirtualMachine
forceDetach bool
vmListErr error
vmssFlexVMUpdateError *retry.Error
diskMap map[string]string
Expand All @@ -159,6 +160,18 @@ func TestDettachDiskWithVmssFlex(t *testing.T) {
diskMap: map[string]string{"diskUri1": "dataDisktestvm1"},
expectedErr: nil,
},
{
description: "DetachDisk should work as expected with managed disk with forceDetach",
nodeName: types.NodeName(testVM1Spec.ComputerName),
vmName: testVM1Spec.VMName,
testVMListWithoutInstanceView: testVMListWithoutInstanceView,
testVMListWithOnlyInstanceView: testVMListWithOnlyInstanceView,
forceDetach: true,
vmListErr: nil,
vmssFlexVMUpdateError: nil,
diskMap: map[string]string{"diskUri1": "dataDisktestvm1"},
expectedErr: nil,
},
{
description: "AttachDisk should should do nothing if the VM cannot be found",
nodeName: types.NodeName(nonExistingNodeName),
Expand Down Expand Up @@ -206,7 +219,7 @@ func TestDettachDiskWithVmssFlex(t *testing.T) {

mockVMClient.EXPECT().Update(gomock.Any(), gomock.Any(), tc.vmName, gomock.Any(), "detach_disk").Return(nil, tc.vmssFlexVMUpdateError).AnyTimes()

err = fs.DetachDisk(ctx, tc.nodeName, tc.diskMap)
err = fs.DetachDisk(ctx, tc.nodeName, tc.diskMap, tc.forceDetach)
if tc.expectedErr == nil {
assert.NoError(t, err)
} else {
Expand Down
2 changes: 1 addition & 1 deletion pkg/provider/azure_mock_vmsets.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (mr *MockVMSetMockRecorder) DeleteCacheForNode(nodeName interface{}) *gomoc
}

// DetachDisk mocks base method.
func (m *MockVMSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string) error {
func (m *MockVMSet) DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string, forceDetach bool) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "DetachDisk", ctx, nodeName, diskMap)
ret0, _ := ret[0].(error)
Expand Down
2 changes: 1 addition & 1 deletion pkg/provider/azure_vmsets.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ type VMSet interface {
// AttachDisk attaches a disk to vm
AttachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]*AttachDiskOptions) (*azure.Future, error)
// DetachDisk detaches a disk from vm
DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string) error
DetachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]string, forceDetach bool) error
// WaitForUpdateResult waits for the response of the update request
WaitForUpdateResult(ctx context.Context, future *azure.Future, nodeName types.NodeName, source string) error

Expand Down

0 comments on commit 54386c1

Please sign in to comment.