Repository: kubernetes-sigs/cloud-provider-azure
Pull request #5559 (2024-02-28 14:21:26)

feat: add forceDetach parameter in DetachDisk function
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide/first-contribution.md#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

#### What type of PR is this?
/kind feature

#### What this PR does / why we need it:
feat: add forceDetach parameter in DetachDisk function

https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/update?view=rest-compute-2023-10-02&tabs=HTTP#diskdetachoptiontypes

DiskDetachOptionTypes | Specifies the detach behavior to be used while detaching a disk or which is already in the process of detachment from the virtual machine. Supported values: ForceDetach. detachOption: ForceDetach is applicable only for managed data disks. If a previous detachment attempt of the data disk did not complete due to an unexpected failure from the virtual machine and the disk is still not released then use force-detach as a last resort option to detach the disk forcibly from the VM. All writes might not have been flushed when using this detach behavior. This feature is still in preview mode and is not supported for VirtualMachineScaleSet. To force-detach a data disk update toBeDetached to 'true' along with setting detachOption: 'ForceDetach'.

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
feat: add forceDetach parameter in DetachDisk function
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs
feat: add forceDetach parameter in DetachDisk function
```



Patch:
diff --git a/pkg/provider/azure_controller_standard.go b/pkg/provider/azure_controller_standard.go
index e47b8296d..10f3b2f60 100644
--- a/pkg/provider/azure_controller_standard.go
+++ b/pkg/provider/azure_controller_standard.go
@@ -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
@@ -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
 			}
 		}
diff --git a/pkg/provider/azure_controller_vmss.go b/pkg/provider/azure_controller_vmss.go
index fada0393f..3a3cc2397 100644
--- a/pkg/provider/azure_controller_vmss.go
+++ b/pkg/provider/azure_controller_vmss.go
@@ -163,7 +163,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 {
@@ -193,6 +193,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
 			}
 		}
diff --git a/pkg/provider/azure_controller_vmssflex.go b/pkg/provider/azure_controller_vmssflex.go
index d34ea871e..383812041 100644
--- a/pkg/provider/azure_controller_vmssflex.go
+++ b/pkg/provider/azure_controller_vmssflex.go
@@ -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 {
@@ -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
 			}
 		}
diff --git a/pkg/provider/azure_mock_vmsets.go b/pkg/provider/azure_mock_vmsets.go
index 78de8bf84..c5b232bb6 100644
--- a/pkg/provider/azure_mock_vmsets.go
+++ b/pkg/provider/azure_mock_vmsets.go
@@ -92,7 +92,7 @@ func (mr *MockVMSetMockRecorder) DeleteCacheForNode(nodeName any) *gomock.Call {
 }
 
 // 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)
diff --git a/pkg/provider/azure_vmsets.go b/pkg/provider/azure_vmsets.go
index d680f5ced..8dfc95799 100644
--- a/pkg/provider/azure_vmsets.go
+++ b/pkg/provider/azure_vmsets.go
@@ -78,7 +78,7 @@ type VMSet interface {
 	// AttachDisk attaches a disk to vm
 	AttachDisk(ctx context.Context, nodeName types.NodeName, diskMap map[string]*AttachDiskOptions) 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
 
 	// GetDataDisks gets a list of data disks attached to the node.


Test patch:
diff --git a/pkg/provider/azure_controller_standard_test.go b/pkg/provider/azure_controller_standard_test.go
index c42647c8c..a46ad7ca3 100644
--- a/pkg/provider/azure_controller_standard_test.go
+++ b/pkg/provider/azure_controller_standard_test.go
@@ -149,6 +149,7 @@ func TestStandardDetachDisk(t *testing.T) {
 		nodeName      types.NodeName
 		disks         []string
 		isDetachFail  bool
+		forceDetach   bool
 		expectedError bool
 	}{
 		{
@@ -180,6 +181,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 {
@@ -203,7 +211,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)
diff --git a/pkg/provider/azure_controller_vmss_test.go b/pkg/provider/azure_controller_vmss_test.go
index 84186e2ef..dfc09c5f3 100644
--- a/pkg/provider/azure_controller_vmss_test.go
+++ b/pkg/provider/azure_controller_vmss_test.go
@@ -175,6 +175,7 @@ func TestDetachDiskWithVMSS(t *testing.T) {
 		vmssName       types.NodeName
 		vmssvmName     types.NodeName
 		disks          []string
+		forceDetach    bool
 		expectedErr    bool
 		expectedErrMsg error
 	}{
@@ -203,6 +204,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"},
@@ -284,7 +294,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)
diff --git a/pkg/provider/azure_controller_vmssflex_test.go b/pkg/provider/azure_controller_vmssflex_test.go
index 20dc9b98a..f66924fd3 100644
--- a/pkg/provider/azure_controller_vmssflex_test.go
+++ b/pkg/provider/azure_controller_vmssflex_test.go
@@ -144,6 +144,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
@@ -160,6 +161,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),
@@ -207,7 +220,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 {