chore: when node disk attach limit is reached, the driver should return ResourceExhausted errors. #3237
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -646,32 +646,45 @@ func TestAttachDiskRequest(t *testing.T) { | |||||
| nodeName string | ||||||
| diskName string | ||||||
| diskNum int | ||||||
| numDisksAllowed int | ||||||
| duplicateDiskRequest bool | ||||||
| expectedErr bool | ||||||
| }{ | ||||||
| { | ||||||
| desc: "one disk request in queue", | ||||||
| diskURI: "diskURI", | ||||||
| nodeName: "nodeName", | ||||||
| diskName: "diskName", | ||||||
| diskNum: 1, | ||||||
| numDisksAllowed: 8, | ||||||
| expectedErr: false, | ||||||
| }, | ||||||
| { | ||||||
| desc: "multiple disk requests in queue", | ||||||
| diskURI: "diskURI", | ||||||
| nodeName: "nodeName", | ||||||
| diskName: "diskName", | ||||||
| diskNum: 10, | ||||||
| numDisksAllowed: 16, | ||||||
| expectedErr: false, | ||||||
| }, | ||||||
| { | ||||||
| desc: "multiple disk requests in queue but exceeds node limit", | ||||||
| diskURI: "diskURI", | ||||||
| nodeName: "nodeName", | ||||||
| diskName: "diskName", | ||||||
| diskNum: 10, | ||||||
| numDisksAllowed: 8, | ||||||
| expectedErr: false, | ||||||
| }, | ||||||
| { | ||||||
| desc: "zero disk request in queue", | ||||||
| diskURI: "diskURI", | ||||||
| nodeName: "nodeName", | ||||||
| diskName: "diskName", | ||||||
| diskNum: 0, | ||||||
| numDisksAllowed: 8, | ||||||
| expectedErr: false, | ||||||
| }, | ||||||
| { | ||||||
| desc: "multiple disk requests in queue", | ||||||
|
|
@@ -680,6 +693,7 @@ func TestAttachDiskRequest(t *testing.T) { | |||||
| diskName: "diskName", | ||||||
| duplicateDiskRequest: true, | ||||||
| diskNum: 10, | ||||||
| numDisksAllowed: 16, | ||||||
| expectedErr: false, | ||||||
| }, | ||||||
| } | ||||||
|
|
@@ -703,9 +717,13 @@ func TestAttachDiskRequest(t *testing.T) { | |||||
| } | ||||||
|
|
||||||
| diskURI := fmt.Sprintf("%s%d", test.diskURI, test.diskNum) | ||||||
| diskMap, err := common.retrieveBatchedAttachDiskRequests(test.nodeName, diskURI, test.numDisksAllowed) | ||||||
| assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s", i, test.desc) | ||||||
| if test.diskNum > test.numDisksAllowed { | ||||||
| assert.Equal(t, test.numDisksAllowed, len(diskMap), "TestCase[%d]: %s", i, test.desc) | ||||||
| } else { | ||||||
| assert.Equal(t, test.diskNum, len(diskMap), "TestCase[%d]: %s", i, test.desc) | ||||||
| } | ||||||
| for diskURI, opt := range diskMap { | ||||||
| assert.Equal(t, strings.Contains(diskURI, test.diskURI), true, "TestCase[%d]: %s", i, test.desc) | ||||||
| assert.Equal(t, strings.Contains(opt.DiskName, test.diskName), true, "TestCase[%d]: %s", i, test.desc) | ||||||
|
|
@@ -779,7 +797,7 @@ func TestDetachDiskRequest(t *testing.T) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| diskMap, err := common.retrieveBatchedDetachDiskRequests(test.nodeName, diskURI) | ||||||
| assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s", i, test.desc) | ||||||
| assert.Equal(t, test.diskNum, len(diskMap), "TestCase[%d]: %s", i, test.desc) | ||||||
| for diskURI, diskName := range diskMap { | ||||||
|
|
@@ -988,7 +1006,7 @@ func TestVerifyDetach(t *testing.T) { | |||||
| nodeName: "node3", | ||||||
| expectedErr: true, | ||||||
| expectedVM: map[string]string{"node3": "PowerState/Running"}, | ||||||
| expectedErrMsg: "disk is still attached to node(node3) on lun(2), vmState: Succeeded", | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -1067,8 +1085,8 @@ func TestConcurrentDetachDisk(t *testing.T) { | |||||
| func(_ context.Context, _ string, name string, params armcompute.VirtualMachine) (*armcompute.VirtualMachine, error) { | ||||||
| if atomic.AddInt32(&callCount, 1) == 1 { | ||||||
| klog.Info("First call to CreateOrUpdate succeeded", "VM Name:", name, "Params:", params) | ||||||
| time.Sleep(1000 * time.Millisecond) // Simulate some processing time to hold the node lock while the 3rd detach request is made | ||||||
|
There was a problem hiding this comment. The sleep duration was increased from 100ms to 1000ms (10x increase) without explanation. This significantly slows down the test. Consider using a more reasonable duration or explaining why such a long delay is necessary.
Suggested change
|
||||||
| return nil, nil // First call succeeds | ||||||
| } | ||||||
| return nil, errors.New("internal error") // Subsequent calls fail | ||||||
| }). | ||||||
|
|
@@ -1090,7 +1108,7 @@ func TestConcurrentDetachDisk(t *testing.T) { | |||||
| }() | ||||||
| } | ||||||
|
|
||||||
| time.Sleep(1100 * time.Millisecond) // Wait for the batching timeout | ||||||
| diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s", | ||||||
| testCloud.SubscriptionID, testCloud.ResourceGroup, "disk-not-batched") | ||||||
| klog.Info("Calling DetachDisk for non-batched disk detach", expectedVM) | ||||||
|
|
@@ -1110,6 +1128,38 @@ func TestConcurrentDetachDisk(t *testing.T) { | |||||
| assert.Error(t, err, "DetachDisk should return an error for the non-batched disk detach") | ||||||
| } | ||||||
|
|
||||||
| func TestClearAttachDiskRequests(t *testing.T) { | ||||||
| ctrl := gomock.NewController(t) | ||||||
| defer ctrl.Finish() | ||||||
|
|
||||||
| testCloud := provider.GetTestCloud(ctrl) | ||||||
| common := &controllerCommon{ | ||||||
| cloud: testCloud, | ||||||
| lockMap: newLockMap(), | ||||||
| } | ||||||
|
|
||||||
| nodeName := "testnode" | ||||||
| // Prepare attachDiskMap with some entries | ||||||
| diskMap := map[string]*provider.AttachDiskOptions{ | ||||||
| "diskURI1": {DiskName: "disk1"}, | ||||||
| "diskURI2": {DiskName: "disk2"}, | ||||||
| } | ||||||
| common.attachDiskMap.Store(nodeName, diskMap) | ||||||
|
|
||||||
| // Ensure attachDiskMap has entries before clearing | ||||||
| if v, ok := common.attachDiskMap.Load(nodeName); !ok || v == nil { | ||||||
| t.Fatalf("attachDiskMap should have entries before clear") | ||||||
| } | ||||||
|
|
||||||
| // Call clearAttachDiskRequests | ||||||
| common.clearAttachDiskRequests(nodeName) | ||||||
|
|
||||||
| // After clearing, attachDiskMap should have an empty map for the nodeName | ||||||
| v, ok := common.attachDiskMap.Load(nodeName) | ||||||
| assert.True(t, ok, "attachDiskMap should have an empty map for nodeName after clearAttachDiskRequests") | ||||||
| assert.Empty(t, v, "attachDiskMap should be empty for nodeName after clearAttachDiskRequests") | ||||||
| } | ||||||
|
|
||||||
| // setTestVirtualMachines sets test virtual machine with powerstate. | ||||||
| func setTestVirtualMachines(c *provider.Cloud, vmList map[string]string, isDataDisksFull bool) []armcompute.VirtualMachine { | ||||||
| expectedVMs := make([]armcompute.VirtualMachine, 0) | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition should use >= instead of > since reaching the maximum number of disks should also trigger the error. If currentNodeDisks equals maxNodeDisks, there's no room for additional disks.