Skip to content

Commit 77ba4bc

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#3277 from marwanad/misc-delete-fixes
Decrement curSize by the number of instances to be deleted
2 parents ca6bbc9 + 786e5b5 commit 77ba4bc

File tree

5 files changed

+47
-35
lines changed

5 files changed

+47
-35
lines changed

‎cluster-autoscaler/cloudprovider/azure/azure_cloud_provider_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package azure
1818

1919
import (
20+
"fmt"
2021
"testing"
2122

2223
apiv1 "k8s.io/api/core/v1"
@@ -94,7 +95,7 @@ func TestNodeGroupForNode(t *testing.T) {
9495
ctrl := gomock.NewController(t)
9596
defer ctrl.Finish()
9697

97-
expectedVMSSVMs := newTestVMSSVMList()
98+
expectedVMSSVMs := newTestVMSSVMList(3)
9899
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
99100

100101
provider := newTestProvider(t)
@@ -112,7 +113,7 @@ func TestNodeGroupForNode(t *testing.T) {
112113

113114
node := &apiv1.Node{
114115
Spec: apiv1.NodeSpec{
115-
ProviderID: "azure://" + fakeVirtualMachineScaleSetVMID,
116+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0),
116117
},
117118
}
118119
// refresh cache

‎cluster-autoscaler/cloudprovider/azure/azure_fakes.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import (
2828
)
2929

3030
const (
31-
fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourceGroups/test-asg/providers/Microsoft.Compute/virtualMachineScaleSets/agents/virtualMachines/0"
31+
fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourceGroups/test-asg/providers/Microsoft.Compute/virtualMachineScaleSets/agents/virtualMachines/%d"
3232
)
3333

3434
// DeploymentsClientMock mocks for DeploymentsClient.

‎cluster-autoscaler/cloudprovider/azure/azure_manager_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -598,7 +598,7 @@ func TestFetchExplicitAsgs(t *testing.T) {
598598
}
599599

600600
manager := newTestAzureManager(t)
601-
expectedVMSSVMs := newTestVMSSVMList()
601+
expectedVMSSVMs := newTestVMSSVMList(3)
602602
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
603603

604604
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
@@ -889,7 +889,7 @@ func TestFetchAutoAsgsVmss(t *testing.T) {
889889
}
890890

891891
expectedScaleSets := []compute.VirtualMachineScaleSet{fakeVMSSWithTags(vmssName, map[string]*string{vmssTag: &vmssTagValue, "min": &minString, "max": &maxString})}
892-
expectedVMSSVMs := newTestVMSSVMList()
892+
expectedVMSSVMs := newTestVMSSVMList(1)
893893

894894
manager := newTestAzureManager(t)
895895
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)

‎cluster-autoscaler/cloudprovider/azure/azure_scale_set.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -414,12 +414,10 @@ func (scaleSet *ScaleSet) DeleteInstances(instances []*azureRef) error {
414414
return fmt.Errorf("cannot delete instance (%s) which don't belong to the same Scale Set (%q)", instance.Name, commonAsg)
415415
}
416416

417-
scaleSet.instanceMutex.Lock()
418417
if cpi, found := scaleSet.getInstanceByProviderID(instance.Name); found && cpi.Status != nil && cpi.Status.State == cloudprovider.InstanceDeleting {
419418
klog.V(3).Infof("Skipping deleting instance %s as its current state is deleting", instance.Name)
420419
continue
421420
}
422-
scaleSet.instanceMutex.Unlock()
423421

424422
instanceID, err := getLastSegment(instance.Name)
425423
if err != nil {
@@ -454,7 +452,7 @@ func (scaleSet *ScaleSet) DeleteInstances(instances []*azureRef) error {
454452
// Proactively decrement scale set size so that we don't
455453
// go below minimum node count if cache data is stale
456454
scaleSet.sizeMutex.Lock()
457-
scaleSet.curSize--
455+
scaleSet.curSize -= int64(len(instanceIDs))
458456
scaleSet.sizeMutex.Unlock()
459457

460458
go scaleSet.waitForDeleteInstances(future, requiredIds)
@@ -730,6 +728,8 @@ func buildInstanceCache(vms []compute.VirtualMachineScaleSetVM) []cloudprovider.
730728
}
731729

732730
func (scaleSet *ScaleSet) getInstanceByProviderID(providerID string) (cloudprovider.Instance, bool) {
731+
scaleSet.instanceMutex.Lock()
732+
defer scaleSet.instanceMutex.Unlock()
733733
for _, instance := range scaleSet.instanceCache {
734734
if instance.Id == providerID {
735735
return instance, true

‎cluster-autoscaler/cloudprovider/azure/azure_scale_set_test.go‎

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,19 @@ func newTestVMSSList(cap int64, name, loc string) []compute.VirtualMachineScaleS
5959
}
6060
}
6161

62-
func newTestVMSSVMList() []compute.VirtualMachineScaleSetVM {
63-
return []compute.VirtualMachineScaleSetVM{
64-
{
65-
ID: to.StringPtr(fakeVirtualMachineScaleSetVMID),
66-
InstanceID: to.StringPtr("0"),
62+
func newTestVMSSVMList(count int) []compute.VirtualMachineScaleSetVM {
63+
var vmssVMList []compute.VirtualMachineScaleSetVM
64+
for i := 0; i < count; i++ {
65+
vmssVM := compute.VirtualMachineScaleSetVM{
66+
ID: to.StringPtr(fmt.Sprintf(fakeVirtualMachineScaleSetVMID, i)),
67+
InstanceID: to.StringPtr(fmt.Sprintf("%d", i)),
6768
VirtualMachineScaleSetVMProperties: &compute.VirtualMachineScaleSetVMProperties{
68-
VMID: to.StringPtr("123E4567-E89B-12D3-A456-426655440000"),
69+
VMID: to.StringPtr(fmt.Sprintf("123E4567-E89B-12D3-A456-426655440000-%d", i)),
6970
},
70-
},
71+
}
72+
vmssVMList = append(vmssVMList, vmssVM)
7173
}
74+
return vmssVMList
7275
}
7376

7477
func TestMaxSize(t *testing.T) {
@@ -94,7 +97,7 @@ func TestTargetSize(t *testing.T) {
9497
defer ctrl.Finish()
9598

9699
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
97-
expectedVMSSVMs := newTestVMSSVMList()
100+
expectedVMSSVMs := newTestVMSSVMList(3)
98101

99102
provider := newTestProvider(t)
100103
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
@@ -119,7 +122,7 @@ func TestIncreaseSize(t *testing.T) {
119122
defer ctrl.Finish()
120123

121124
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
122-
expectedVMSSVMs := newTestVMSSVMList()
125+
expectedVMSSVMs := newTestVMSSVMList(3)
123126

124127
provider := newTestProvider(t)
125128
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
@@ -177,7 +180,7 @@ func TestIncreaseSizeOnVMSSUpdating(t *testing.T) {
177180
},
178181
},
179182
}
180-
expectedVMSSVMs := newTestVMSSVMList()
183+
expectedVMSSVMs := newTestVMSSVMList(3)
181184

182185
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
183186
mockVMSSClient.EXPECT().List(gomock.Any(), manager.config.ResourceGroup).Return(expectedScaleSets, nil)
@@ -206,7 +209,7 @@ func TestBelongs(t *testing.T) {
206209
defer ctrl.Finish()
207210

208211
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
209-
expectedVMSSVMs := newTestVMSSVMList()
212+
expectedVMSSVMs := newTestVMSSVMList(3)
210213

211214
provider := newTestProvider(t)
212215
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
@@ -236,7 +239,7 @@ func TestBelongs(t *testing.T) {
236239

237240
validNode := &apiv1.Node{
238241
Spec: apiv1.NodeSpec{
239-
ProviderID: "azure://" + fakeVirtualMachineScaleSetVMID,
242+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0),
240243
},
241244
}
242245
belongs, err := scaleSet.Belongs(validNode)
@@ -260,7 +263,7 @@ func TestDeleteNodes(t *testing.T) {
260263
},
261264
},
262265
}
263-
expectedVMSSVMs := newTestVMSSVMList()
266+
expectedVMSSVMs := newTestVMSSVMList(3)
264267

265268
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl)
266269
mockVMSSClient.EXPECT().List(gomock.Any(), manager.config.ResourceGroup).Return(expectedScaleSets, nil).AnyTimes()
@@ -288,11 +291,6 @@ func TestDeleteNodes(t *testing.T) {
288291
// logic is refactored out
289292
manager.regenerateCache()
290293

291-
node := &apiv1.Node{
292-
Spec: apiv1.NodeSpec{
293-
ProviderID: "azure://" + fakeVirtualMachineScaleSetVMID,
294-
},
295-
}
296294
scaleSet, ok := provider.NodeGroups()[0].(*ScaleSet)
297295
assert.True(t, ok)
298296

@@ -301,13 +299,25 @@ func TestDeleteNodes(t *testing.T) {
301299
assert.Equal(t, 3, targetSize)
302300

303301
// Perform the delete operation
304-
err = scaleSet.DeleteNodes([]*apiv1.Node{node})
302+
nodesToDelete := []*apiv1.Node{
303+
{
304+
Spec: apiv1.NodeSpec{
305+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0),
306+
},
307+
},
308+
{
309+
Spec: apiv1.NodeSpec{
310+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 2),
311+
},
312+
},
313+
}
314+
err = scaleSet.DeleteNodes(nodesToDelete)
305315
assert.NoError(t, err)
306316

307-
// Ensure the the cached size has been proactively decremented
317+
// Ensure the the cached size has been proactively decremented by 2
308318
targetSize, err = scaleSet.TargetSize()
309319
assert.NoError(t, err)
310-
assert.Equal(t, 2, targetSize)
320+
assert.Equal(t, 1, targetSize)
311321
}
312322

313323
func TestDeleteNoConflictRequest(t *testing.T) {
@@ -357,7 +367,7 @@ func TestDeleteNoConflictRequest(t *testing.T) {
357367

358368
node := &apiv1.Node{
359369
Spec: apiv1.NodeSpec{
360-
ProviderID: "azure://" + fakeVirtualMachineScaleSetVMID,
370+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0),
361371
},
362372
}
363373

@@ -390,7 +400,7 @@ func TestScaleSetNodes(t *testing.T) {
390400
ctrl := gomock.NewController(t)
391401
defer ctrl.Finish()
392402

393-
expectedVMSSVMs := newTestVMSSVMList()
403+
expectedVMSSVMs := newTestVMSSVMList(3)
394404
expectedScaleSets := newTestVMSSList(3, "test-asg", "eastus")
395405

396406
provider := newTestProvider(t)
@@ -409,10 +419,9 @@ func TestScaleSetNodes(t *testing.T) {
409419
assert.True(t, registered)
410420
assert.Equal(t, len(provider.NodeGroups()), 1)
411421

412-
fakeProviderID := "azure://" + fakeVirtualMachineScaleSetVMID
413422
node := &apiv1.Node{
414423
Spec: apiv1.NodeSpec{
415-
ProviderID: fakeProviderID,
424+
ProviderID: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0),
416425
},
417426
}
418427
group, err := provider.NodeGroupForNode(node)
@@ -427,8 +436,10 @@ func TestScaleSetNodes(t *testing.T) {
427436
assert.NotNil(t, ss)
428437
instances, err := group.Nodes()
429438
assert.NoError(t, err)
430-
assert.Equal(t, len(instances), 1)
431-
assert.Equal(t, instances[0], cloudprovider.Instance{Id: fakeProviderID})
439+
assert.Equal(t, len(instances), 3)
440+
assert.Equal(t, instances[0], cloudprovider.Instance{Id: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 0)})
441+
assert.Equal(t, instances[1], cloudprovider.Instance{Id: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 1)})
442+
assert.Equal(t, instances[2], cloudprovider.Instance{Id: "azure://" + fmt.Sprintf(fakeVirtualMachineScaleSetVMID, 2)})
432443
}
433444

434445
func TestTemplateNodeInfo(t *testing.T) {

0 commit comments

Comments
 (0)