Merge pull request #3294 from landreasyan/force-detach-on-timeout

fix: detach call should not use the full context deadline and should leave an active context for force detach on timeout

Author: andyzhangx

pkg/azuredisk/azure_controller_common.go

@@ -61,6 +61,9 @@ const (
 	// default initial delay in milliseconds for batch disk attach/detach
 	defaultAttachDetachInitialDelayInMs = 1000
 
+	// default detach operation timeout
+	defaultDetachOperationMinTimeoutInSeconds = 240
+
 	// WriteAcceleratorEnabled support for Azure Write Accelerator on Azure Disks
 	// https://docs.microsoft.com/azure/virtual-machines/windows/how-to-enable-write-accelerator
 	WriteAcceleratorEnabled = "writeacceleratorenabled"
@@ -90,6 +93,8 @@ type controllerCommon struct {
 	detachDiskMap sync.Map
 	// DisableDiskLunCheck whether disable disk lun check after disk attach/detach
 	DisableDiskLunCheck bool
+	// DetachOperationMinTimeoutInSeconds determines timeout for waiting on detach operation before a force detach
+	DetachOperationMinTimeoutInSeconds int
 	// AttachDetachInitialDelayInMs determines initial delay in milliseconds for batch disk attach/detach
 	AttachDetachInitialDelayInMs int
 	ForceDetachBackoff           bool
@@ -394,14 +399,29 @@ func (c *controllerCommon) DetachDisk(ctx context.Context, diskName, diskURI str
 
 	if len(diskMap) == 0 {
 		// disk was already processed in the batch, return the result
+		// returns success when the disk has toBeDetached flag set to true
+		// we could consider issuing a force detach if the disk is stuck in ToBeDetached state for too long
 		return c.verifyDetach(ctx, diskName, diskURI, nodeName)
 	}
 
 	klog.V(2).Infof("Trying to detach volume %s from node %s, diskMap len:%d, %s", diskURI, nodeName, len(diskMap), diskMap)
 	c.diskStateMap.Store(formattedDiskURI, "detaching")
 	defer c.diskStateMap.Delete(formattedDiskURI)
 
-	if err = vmset.DetachDisk(ctx, nodeName, diskMap, false); err != nil {
+	// Calculate timeout for regular detach operation, if operation fails or times out,
+	// we need to have an active context to issue a force detach
+	detachContextTimeout := time.Duration(c.DetachOperationMinTimeoutInSeconds) * time.Second
+	if deadline, ok := ctx.Deadline(); ok {
+		remaining := time.Until(deadline)
+		halfRemaining := remaining / 2
+		if halfRemaining > detachContextTimeout {
+			detachContextTimeout = halfRemaining
+		}
+	}
+	detachCtx, cancel := context.WithTimeout(ctx, detachContextTimeout)
+	defer cancel()
+
+	if err = vmset.DetachDisk(detachCtx, nodeName, diskMap, false); err != nil {
 		if isInstanceNotFoundError(err) {
 			// if host doesn't exist, no need to detach
 			klog.Warningf("azureDisk - got InstanceNotFoundError(%v), DetachDisk(%s) will assume disk is already detached",

pkg/azuredisk/azure_controller_common_test.go

@@ -208,6 +208,205 @@ func TestCommonAttachDisk(t *testing.T) {
 	}
 }
 
+func TestForceDetach(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	testCases := []struct {
+		desc                   string
+		vmList                 map[string]string
+		nodeName               types.NodeName
+		diskName               string
+		forceDetachBackoff     bool
+		detachOperationTimeout int
+		contextTimeout         time.Duration
+		firstDetachError       error
+		forceDetachError       error
+		expectedErr            bool
+		expectForceDetach      bool
+	}{
+		{
+			desc:                   "force detach should be called when regular detach times out",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 1,               // 1s timeout
+			contextTimeout:         2 * time.Second, // not more than double the detach timeout
+			firstDetachError:       context.DeadlineExceeded,
+			forceDetachError:       nil,
+			expectedErr:            false,
+			expectForceDetach:      true,
+		},
+		{
+			desc:                   "detach operation timeout of more than half the context timeout should be respected",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 2,               // 2s timeout
+			contextTimeout:         3 * time.Second, // not more than double the detach timeout
+			firstDetachError:       context.DeadlineExceeded,
+			forceDetachError:       nil,
+			expectedErr:            false,
+			expectForceDetach:      true,
+		},
+		{
+			desc:                   "force detach should be called with half context timeout when min detach timeout is less than half context timeout",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 1,               // 1s timeout
+			contextTimeout:         3 * time.Second, // more than double the detach timeout
+			firstDetachError:       context.DeadlineExceeded,
+			forceDetachError:       nil,
+			expectedErr:            false,
+			expectForceDetach:      true,
+		},
+		{
+			desc:                   "force detach should be called when regular detach fails",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 1,
+			contextTimeout:         2 * time.Second,
+			firstDetachError:       errors.New("detach failed"),
+			forceDetachError:       nil,
+			expectedErr:            false,
+			expectForceDetach:      true,
+		},
+		{
+			desc:                   "should return error when force detach also fails",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 1,
+			contextTimeout:         2 * time.Second,
+			firstDetachError:       context.DeadlineExceeded,
+			forceDetachError:       errors.New("force detach failed"),
+			expectedErr:            true,
+			expectForceDetach:      true,
+		},
+		{
+			desc:                   "force detach should not be called when forceDetachBackoff is false",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     false,
+			detachOperationTimeout: 1,
+			contextTimeout:         2 * time.Second,
+			firstDetachError:       errors.New("detach failed"),
+			forceDetachError:       nil,
+			expectedErr:            true,
+			expectForceDetach:      false,
+		},
+		{
+			desc:                   "successful regular detach should not trigger force detach",
+			vmList:                 map[string]string{"vm1": "PowerState/Running"},
+			nodeName:               "vm1",
+			diskName:               "disk1",
+			forceDetachBackoff:     true,
+			detachOperationTimeout: 1,
+			contextTimeout:         2 * time.Second,
+			firstDetachError:       nil,
+			forceDetachError:       nil,
+			expectedErr:            false,
+			expectForceDetach:      false,
+		},
+	}
+
+	for i, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), test.contextTimeout)
+			defer cancel()
+			testCloud := provider.GetTestCloud(ctrl)
+			common := &controllerCommon{
+				cloud:                              testCloud,
+				lockMap:                            newLockMap(),
+				ForceDetachBackoff:                 test.forceDetachBackoff,
+				DetachOperationMinTimeoutInSeconds: test.detachOperationTimeout,
+				DisableDiskLunCheck:                true, // Disable lun check to simplify test
+			}
+
+			diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
+				testCloud.SubscriptionID, testCloud.ResourceGroup, test.diskName)
+
+			expectedVMs := setTestVirtualMachines(testCloud, test.vmList, false)
+			mockVMClient := testCloud.ComputeClientFactory.GetVirtualMachineClient().(*mockvmclient.MockInterface)
+
+			for _, vm := range expectedVMs {
+				mockVMClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, *vm.Name, gomock.Any()).Return(&vm, nil).AnyTimes()
+			}
+
+			// Set up expectations for CreateOrUpdate calls
+			callCount := 0
+			if test.expectForceDetach {
+				// Expect two calls: regular detach and force detach
+				mockVMClient.EXPECT().CreateOrUpdate(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any()).DoAndReturn(
+					func(ctx context.Context, _ string, _ string, vm armcompute.VirtualMachine) (*armcompute.VirtualMachine, error) {
+						callCount++
+						if callCount == 1 {
+							// First call is regular detach
+							// Verify that context timeout is at least the min detach timeout
+							contextDeadline, ok := ctx.Deadline()
+							assert.True(t, ok, "Context should have a deadline")
+							assert.True(t, contextDeadline.After(time.Now()), "Context deadline should be in the future")
+							assert.True(t, time.Until(contextDeadline) >= time.Duration(test.detachOperationTimeout)*time.Millisecond-100*time.Millisecond, "Context deadline should exceed min detach timeout.")
+							assert.True(t, time.Until(contextDeadline) >= test.contextTimeout/2-100*time.Millisecond, "Context deadline should be at least half of context timeout.")
+							// Simulate timeout by sleeping longer than context deadline
+							if test.firstDetachError == context.DeadlineExceeded {
+								time.Sleep(time.Until(contextDeadline.Add(50 * time.Millisecond)))
+							}
+							return nil, test.firstDetachError
+						} else if callCount == 2 {
+							// Second call is force detach
+							// Verify force detach parameter is set
+							if vm.Properties != nil && vm.Properties.StorageProfile != nil {
+								for _, disk := range vm.Properties.StorageProfile.DataDisks {
+									if disk.Name != nil && *disk.Name == test.diskName {
+										assert.NotNil(t, disk.DetachOption, "DetachOption should be set for force detach")
+										if disk.DetachOption != nil {
+											assert.Equal(t, armcompute.DiskDetachOptionTypesForceDetach, *disk.DetachOption, "DetachOption should be ForceDetach")
+										}
+									}
+								}
+							}
+							return nil, test.forceDetachError
+						}
+						return nil, errors.New("unexpected call")
+					}).Times(2)
+			} else {
+				// Expect only one call for regular detach
+				mockVMClient.EXPECT().CreateOrUpdate(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any()).Return(nil, test.firstDetachError).Times(1)
+			}
+
+			// Create context with custom timeout if specified
+			testCtx := ctx
+			if test.contextTimeout > 0 {
+				testCtx, cancel = context.WithTimeout(ctx, test.contextTimeout)
+				defer cancel()
+			}
+
+			err := common.DetachDisk(testCtx, test.diskName, diskURI, test.nodeName)
+
+			if test.expectedErr {
+				assert.Error(t, err, "TestCase[%d]: %s", i, test.desc)
+			} else {
+				assert.NoError(t, err, "TestCase[%d]: %s", i, test.desc)
+			}
+
+			if test.expectForceDetach {
+				assert.Equal(t, 2, callCount, "TestCase[%d]: %s - Expected force detach to be called", i, test.desc)
+			} else {
+				assert.LessOrEqual(t, callCount, 1, "TestCase[%d]: %s - Force detach should not be called", i, test.desc)
+			}
+		})
+	}
+}
+
 func TestCommonDetachDisk(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()

pkg/azuredisk/azure_managedDiskController.go

@@ -49,10 +49,11 @@ type ManagedDiskController struct {
 
 func NewManagedDiskController(provider *provider.Cloud) *ManagedDiskController {
 	common := &controllerCommon{
-		cloud:                        provider,
-		lockMap:                      newLockMap(),
-		AttachDetachInitialDelayInMs: defaultAttachDetachInitialDelayInMs,
-		clientFactory:                provider.ComputeClientFactory,
+		cloud:                              provider,
+		lockMap:                            newLockMap(),
+		AttachDetachInitialDelayInMs:       defaultAttachDetachInitialDelayInMs,
+		DetachOperationMinTimeoutInSeconds: defaultDetachOperationMinTimeoutInSeconds,
+		clientFactory:                      provider.ComputeClientFactory,
 	}
 	getter := func(_ context.Context, _ string) (interface{}, error) { return nil, nil }
 	common.hitMaxDataDiskCountCache, _ = azcache.NewTimedCache(5*time.Minute, getter, false)

pkg/azuredisk/azure_managedDiskController_test.go

@@ -265,10 +265,11 @@ func TestCreateManagedDisk(t *testing.T) {
 		testCloud := provider.GetTestCloud(ctrl)
 
 		common := &controllerCommon{
-			cloud:                        testCloud,
-			lockMap:                      newLockMap(),
-			AttachDetachInitialDelayInMs: defaultAttachDetachInitialDelayInMs,
-			clientFactory:                testCloud.ComputeClientFactory,
+			cloud:                              testCloud,
+			lockMap:                            newLockMap(),
+			AttachDetachInitialDelayInMs:       defaultAttachDetachInitialDelayInMs,
+			DetachOperationMinTimeoutInSeconds: defaultDetachOperationMinTimeoutInSeconds,
+			clientFactory:                      testCloud.ComputeClientFactory,
 		}
 
 		managedDiskController := &ManagedDiskController{common}
@@ -330,10 +331,11 @@ func TestCreateManagedDiskWithExtendedLocation(t *testing.T) {
 	}
 
 	common := &controllerCommon{
-		cloud:                        testCloud,
-		lockMap:                      newLockMap(),
-		AttachDetachInitialDelayInMs: defaultAttachDetachInitialDelayInMs,
-		clientFactory:                testCloud.ComputeClientFactory,
+		cloud:                              testCloud,
+		lockMap:                            newLockMap(),
+		AttachDetachInitialDelayInMs:       defaultAttachDetachInitialDelayInMs,
+		DetachOperationMinTimeoutInSeconds: defaultDetachOperationMinTimeoutInSeconds,
+		clientFactory:                      testCloud.ComputeClientFactory,
 	}
 
 	managedDiskController := &ManagedDiskController{common}
@@ -397,10 +399,11 @@ func TestDeleteManagedDisk(t *testing.T) {
 		testCloud := provider.GetTestCloud(ctrl)
 
 		common := &controllerCommon{
-			cloud:                        testCloud,
-			lockMap:                      newLockMap(),
-			AttachDetachInitialDelayInMs: defaultAttachDetachInitialDelayInMs,
-			clientFactory:                testCloud.ComputeClientFactory,
+			cloud:                              testCloud,
+			lockMap:                            newLockMap(),
+			AttachDetachInitialDelayInMs:       defaultAttachDetachInitialDelayInMs,
+			DetachOperationMinTimeoutInSeconds: defaultDetachOperationMinTimeoutInSeconds,
+			clientFactory:                      testCloud.ComputeClientFactory,
 		}
 
 		managedDiskController := &ManagedDiskController{common}