Fix vmssflex ensureBackendPoolDeletedFromNode

* Fix loop pointer issue * Use lock on nicUpdated var * Fix log format Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
kubernetes-sigs · Jun 8, 2023 · 5a09b6b · 5a09b6b
1 parent 1682c6b
commit 5a09b6b
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 24 deletions.
diff --git a/pkg/provider/azure_standard.go b/pkg/provider/azure_standard.go
@@ -1176,6 +1176,7 @@ func (as *availabilitySet) EnsureBackendPoolDeleted(service *v1.Service, backend
 			ipconfigPrefixToNicMap[ipConfigIDPrefix] = nic
 		}
 	}
+	nicUpdaterLock := "nicUpdaterLock"
 	for k := range ipconfigPrefixToNicMap {
 		nic := ipconfigPrefixToNicMap[k]
 		newIPConfigs := *nic.IPConfigurations
@@ -1208,7 +1209,9 @@ func (as *availabilitySet) EnsureBackendPoolDeleted(service *v1.Service, backend
 				klog.Errorf("EnsureBackendPoolDeleted CreateOrUpdate for NIC(%s, %s) failed with error %v", as.ResourceGroup, pointer.StringDeref(nic.Name, ""), rerr.Error())
 				return rerr.Error()
 			}
+			as.lockMap.LockEntry(nicUpdaterLock)
 			nicUpdated = true
+			as.lockMap.UnlockEntry(nicUpdaterLock)
 			return nil
 		})
 	}

diff --git a/pkg/provider/azure_vmss.go b/pkg/provider/azure_vmss.go
@@ -1878,6 +1878,7 @@ func (ss *ScaleSet) ensureBackendPoolDeleted(service *v1.Service, backendPoolIDs
 
 	// Update VMs with best effort that have already been added to nodeUpdates.
 	var updatedVM bool
+	updatedVMLock := "updatedVMLock"
 	for meta, update := range nodeUpdates {
 		// create new instance of meta and update for passing to anonymous function
 		meta := meta
@@ -1906,7 +1907,9 @@ func (ss *ScaleSet) ensureBackendPoolDeleted(service *v1.Service, backendPoolIDs
 				return rerr.Error()
 			}
 
+			ss.lockMap.LockEntry(updatedVMLock)
 			updatedVM = true
+			ss.lockMap.UnlockEntry(updatedVMLock)
 			return nil
 		})
 	}

diff --git a/pkg/provider/azure_vmssflex.go b/pkg/provider/azure_vmssflex.go
@@ -945,8 +945,8 @@ func (fs *FlexScaleSet) EnsureBackendPoolDeleted(service *v1.Service, backendPoo
 		}
 	}
 
-	klog.V(2).Infof("Ensure backendPoolIDs deleted from the VMSS VMs.", backendPoolIDs)
-	klog.V(2).Infof("go into fs.ensureBackendPoolDeletedFromNode, vmssFlexVMNameMap: %s, size: %s", vmssFlexVMNameMap, len(vmssFlexVMNameMap))
+	klog.V(2).Infof("Ensure backendPoolIDs %q deleted from the VMSS VMs.", backendPoolIDs)
+	klog.V(2).Infof("go into fs.ensureBackendPoolDeletedFromNode, vmssFlexVMNameMap: %s, size: %d", vmssFlexVMNameMap, len(vmssFlexVMNameMap))
 	nicUpdated, err := fs.ensureBackendPoolDeletedFromNode(vmssFlexVMNameMap, backendPoolIDs)
 	klog.V(2).Infof("exit from fs.ensureBackendPoolDeletedFromNode")
 	if err != nil {
@@ -989,7 +989,9 @@ func (fs *FlexScaleSet) ensureBackendPoolDeletedFromNode(vmssFlexVMNameMap map[s
 			nics[nicName] = nic
 		}
 	}
+	nicUpdaterLock := "nicUpdaterLock"
 	for _, nic := range nics {
+		nic := nic
 		newIPConfigs := *nic.IPConfigurations
 		for j, ipConf := range newIPConfigs {
 			if !pointer.BoolDeref(ipConf.Primary, false) {
@@ -1020,12 +1022,14 @@ func (fs *FlexScaleSet) ensureBackendPoolDeletedFromNode(vmssFlexVMNameMap map[s
 				klog.Errorf("EnsureBackendPoolDeleted CreateOrUpdate for NIC(%s, %s) failed with error %v", fs.ResourceGroup, pointer.StringDeref(nic.Name, ""), rerr.Error())
 				return rerr.Error()
 			}
+			fs.lockMap.LockEntry(nicUpdaterLock)
 			nicUpdated = true
+			fs.lockMap.UnlockEntry(nicUpdaterLock)
 			klog.V(2).Infof("EnsureBackendPoolDeleted done")
 			return nil
 		})
 	}
-	klog.V(2).Infof("nicUpdaters size: %s", len(nicUpdaters))
+	klog.V(2).Infof("nicUpdaters size: %d", len(nicUpdaters))
 	errs := utilerrors.AggregateGoroutines(nicUpdaters...)
 	if errs != nil {
 		allErrs = append(allErrs, utilerrors.Flatten(errs))

diff --git a/pkg/provider/azure_vmssflex_test.go b/pkg/provider/azure_vmssflex_test.go
@@ -1538,19 +1538,23 @@ func TestEnsureBackendPoolDeletedFromNodeVmssFlex(t *testing.T) {
 		description         string
 		vmssFlexVMNameMap   map[string]string
 		backendPoolID       string
-		nic                 network.Interface
+		nics                []network.Interface
 		expectedPutNICTimes int
 		nicGetErr           *retry.Error
 		nicPutErr           *retry.Error
 		expectedErr         error
 	}{
 		{
-			description: "EnsureBackendPoolDeletedFromNode should remove a backend pool from the vmss flex vm",
+			description: "EnsureBackendPoolDeletedFromNode should remove backend pools from the vmss flex vm",
 			vmssFlexVMNameMap: map[string]string{
 				"vmssflex1000001": "testvm1-nic",
+				"vmssflex1000002": "testvm2-nic",
+			},
+			backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/backendAddressPools/backendpool-0",
+			nics: []network.Interface{
+				generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1"),
+				generateTestNic("testvm2-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm2"),
 			},
-			backendPoolID:       "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/backendAddressPools/backendpool-0",
-			nic:                 generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1"),
 			expectedPutNICTimes: 1,
 			nicGetErr:           nil,
 			expectedErr:         nil,
@@ -1561,7 +1565,7 @@ func TestEnsureBackendPoolDeletedFromNodeVmssFlex(t *testing.T) {
 				"vmssflex1000001": "testvm1-nic",
 			},
 			backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/backendAddressPools/backendpool-0",
-			nic:           generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1"),
+			nics:          []network.Interface{generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1")},
 			nicGetErr:     &retry.Error{RawError: fmt.Errorf("failed to get nic")},
 			expectedErr:   fmt.Errorf("ensureBackendPoolDeletedFromNode: failed to get interface of name testvm1-nic: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: failed to get nic"),
 		},
@@ -1571,7 +1575,7 @@ func TestEnsureBackendPoolDeletedFromNodeVmssFlex(t *testing.T) {
 				"vmssflex1000001": "testvm1-nic",
 			},
 			backendPoolID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/backendAddressPools/backendpool-0",
-			nic:           generateTestNic("testvm1-nic", false, network.ProvisioningStateFailed, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1"),
+			nics:          []network.Interface{generateTestNic("testvm1-nic", false, network.ProvisioningStateFailed, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1")},
 			nicGetErr:     nil,
 			expectedErr:   nil,
 		},
@@ -1581,7 +1585,7 @@ func TestEnsureBackendPoolDeletedFromNodeVmssFlex(t *testing.T) {
 				"vmssflex1000001": "testvm1-nic",
 			},
 			backendPoolID:       "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/backendAddressPools/backendpool-0",
-			nic:                 generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1"),
+			nics:                []network.Interface{generateTestNic("testvm1-nic", false, network.ProvisioningStateSucceeded, "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/testvm1")},
 			expectedPutNICTimes: 1,
 			nicGetErr:           nil,
 			nicPutErr:           &retry.Error{RawError: fmt.Errorf("failed to update nic")},
@@ -1590,25 +1594,29 @@ func TestEnsureBackendPoolDeletedFromNodeVmssFlex(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		fs, err := NewTestFlexScaleSet(ctrl)
-		assert.NoError(t, err, "unexpected error when creating test FlexScaleSet")
+		t.Run(tc.description, func(t *testing.T) {
+			fs, err := NewTestFlexScaleSet(ctrl)
+			assert.NoError(t, err, "unexpected error when creating test FlexScaleSet")
 
-		mockInterfacesClient := fs.InterfacesClient.(*mockinterfaceclient.MockInterface)
-		mockInterfacesClient.EXPECT().Get(gomock.Any(), gomock.Any(), "testvm1-nic", gomock.Any()).Return(tc.nic, tc.nicGetErr).AnyTimes()
-		mockInterfacesClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.nicPutErr).Times(tc.expectedPutNICTimes)
+			mockInterfacesClient := fs.InterfacesClient.(*mockinterfaceclient.MockInterface)
+			for i := range tc.nics {
+				nic := tc.nics[i]
+				mockInterfacesClient.EXPECT().Get(gomock.Any(), gomock.Any(), *nic.Name, gomock.Any()).Return(nic, tc.nicGetErr).AnyTimes()
+				mockInterfacesClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), *nic.Name, gomock.Any()).Return(tc.nicPutErr).Times(tc.expectedPutNICTimes)
+			}
 
-		updated, err := fs.ensureBackendPoolDeletedFromNode(tc.vmssFlexVMNameMap, []string{tc.backendPoolID})
+			updated, err := fs.ensureBackendPoolDeletedFromNode(tc.vmssFlexVMNameMap, []string{tc.backendPoolID})
 
-		if tc.expectedErr != nil {
-			assert.EqualError(t, err, tc.expectedErr.Error(), tc.description)
-		} else {
-			assert.NoError(t, err, tc.description)
-			if tc.expectedPutNICTimes > 0 {
-				assert.True(t, updated, tc.description)
+			if tc.expectedErr != nil {
+				assert.EqualError(t, err, tc.expectedErr.Error())
+			} else {
+				assert.NoError(t, err)
+				if tc.expectedPutNICTimes > 0 {
+					assert.True(t, updated)
+				}
 			}
-		}
+		})
 	}
-
 }
 
 func TestEnsureBackendPoolDeletedVmssFlex(t *testing.T) {