Merge pull request #1217 from k8s-infra-cherrypick-robot/cherry-pick-1216-to-release-1.23

[release-1.23] fix: do not delete backend pool when reconciling lb backend pools

Author: k8s-ci-robot

pkg/provider/azure_loadbalancer_backendpool.go

@@ -155,18 +155,18 @@ func (bc *backendPoolTypeNodeIPConfig) ReconcileBackendPools(clusterName string,
 			// to nodeIPConfiguration, we need to decouple the VM NICs from the LB
 			// before attaching nodeIPs/podIPs to the LB backend pool.
 			if bp.BackendAddressPoolPropertiesFormat != nil &&
-				(bp.BackendIPConfigurations == nil || len(*bp.BackendIPConfigurations) == 0) {
-				// It is not allowed to change the type of the backend pool, so we delete it
-				// and create a new empty backend pool.
-				if err := bc.DeleteLBBackendPool(lbName, to.String(bp.Name)); err != nil {
-					klog.Errorf("bc.ReconcileBackendPools for service (%s): failed to DeleteLBBackendPool: %s", serviceName, err.Error())
-					return false, false, err
+				bp.LoadBalancerBackendAddresses != nil &&
+				len(*bp.LoadBalancerBackendAddresses) > 0 {
+				if removeNodeIPAddressesFromBackendPool(bp, []string{}, true) {
+					bp.Etag = nil
+					if err := bc.CreateOrUpdateLBBackendPool(lbName, bp); err != nil {
+						klog.Errorf("bc.ReconcileBackendPools for service (%s): failed to cleanup IP based backend pool %s: %s", serviceName, lbBackendPoolName, err.Error())
+						return false, false, fmt.Errorf("bc.ReconcileBackendPools for service (%s): failed to cleanup IP based backend pool %s: %w", serviceName, lbBackendPoolName, err)
+					}
+					newBackendPools[i] = bp
+					lb.BackendAddressPools = &newBackendPools
+					lb.Etag = nil
 				}
-				newBackendPools = append(newBackendPools[:i], newBackendPools[i+1:]...)
-				lb.BackendAddressPools = &newBackendPools
-				lb.Etag = nil
-				foundBackendPool = false
-				break
 			}
 
 			var backendIPConfigurationsToBeDeleted []network.InterfaceIPConfiguration
@@ -399,22 +399,15 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(clusterName string, servi
 			if bp.BackendAddressPoolPropertiesFormat != nil &&
 				bp.BackendIPConfigurations != nil &&
 				len(*bp.BackendIPConfigurations) > 0 {
-				// It is not allowed to change the type of the backend pool, so we delete it
-				// and create a new empty backend pool. Note that for IP configuration based
-				// backend pools, we must decouple it from the vmSets before deleting.
 				klog.V(2).Infof("bi.ReconcileBackendPools for service (%s): ensuring the LB is decoupled from the VMSet", serviceName)
 				if err := bi.VMSet.EnsureBackendPoolDeleted(service, lbBackendPoolID, vmSetName, lb.BackendAddressPools, true); err != nil {
 					klog.Errorf("bi.ReconcileBackendPools for service (%s): failed to EnsureBackendPoolDeleted: %s", serviceName, err.Error())
 					return false, false, err
 				}
-				if err := bi.DeleteLBBackendPool(lbName, to.String(bp.Name)); err != nil {
-					klog.Errorf("bi.ReconcileBackendPools for service (%s): failed to DeleteLBBackendPool: %s", serviceName, err.Error())
-					return false, false, err
-				}
-				newBackendPools = append(newBackendPools[:i], newBackendPools[i+1:]...)
-				lb.BackendAddressPools = &newBackendPools
+				newBackendPools[i].BackendAddressPoolPropertiesFormat.LoadBalancerBackendAddresses = &[]network.LoadBalancerBackendAddress{}
+				newBackendPools[i].BackendAddressPoolPropertiesFormat.BackendIPConfigurations = &[]network.InterfaceIPConfiguration{}
+				newBackendPools[i].Etag = nil
 				lb.Etag = nil
-				foundBackendPool = false
 				break
 			}
 
@@ -426,7 +419,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(clusterName string, servi
 				}
 			}
 			if len(nodeIPAddressesToBeDeleted) > 0 {
-				updated := removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted)
+				updated := removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted, false)
 				if updated {
 					(*lb.BackendAddressPools)[i] = bp
 					if err := bi.CreateOrUpdateLBBackendPool(lbName, bp); err != nil {
@@ -468,15 +461,19 @@ func newBackendPool(lb *network.LoadBalancer, isBackendPoolPreConfigured bool, p
 	return isBackendPoolPreConfigured
 }
 
-func removeNodeIPAddressesFromBackendPool(backendPool network.BackendAddressPool, nodeIPAddresses []string) bool {
+func removeNodeIPAddressesFromBackendPool(backendPool network.BackendAddressPool, nodeIPAddresses []string, removeAll bool) bool {
 	changed := false
 	nodeIPsSet := sets.NewString(nodeIPAddresses...)
 	if backendPool.BackendAddressPoolPropertiesFormat != nil &&
 		backendPool.LoadBalancerBackendAddresses != nil {
 		for i := len(*backendPool.LoadBalancerBackendAddresses) - 1; i >= 0; i-- {
 			if (*backendPool.LoadBalancerBackendAddresses)[i].LoadBalancerBackendAddressPropertiesFormat != nil {
 				ipAddress := to.String((*backendPool.LoadBalancerBackendAddresses)[i].IPAddress)
-				if nodeIPsSet.Has(ipAddress) {
+				if ipAddress == "" {
+					klog.V(4).Infof("removeNodeIPAddressFromBackendPool: LoadBalancerBackendAddress %s is not IP-based, skipping", to.String((*backendPool.LoadBalancerBackendAddresses)[i].Name))
+					continue
+				}
+				if removeAll || nodeIPsSet.Has(ipAddress) {
 					klog.V(4).Infof("removeNodeIPAddressFromBackendPool: removing %s from the backend pool %s", ipAddress, to.String(backendPool.Name))
 					*backendPool.LoadBalancerBackendAddresses = append((*backendPool.LoadBalancerBackendAddresses)[:i], (*backendPool.LoadBalancerBackendAddresses)[i+1:]...)
 					changed = true

pkg/provider/azure_loadbalancer_backendpool_test.go

@@ -322,17 +322,17 @@ func TestReconcileBackendPoolsNodeIPToIPConfig(t *testing.T) {
 
 	lb := buildLBWithVMIPs(testClusterName, []string{"10.0.0.1", "10.0.0.2"})
 	mockLBClient := mockloadbalancerclient.NewMockInterface(ctrl)
-	mockLBClient.EXPECT().DeleteLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(retry.NewError(false, fmt.Errorf("delete LB backend pool error")))
+	mockLBClient.EXPECT().CreateOrUpdateBackendPools(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(retry.NewError(false, fmt.Errorf("create or update LB backend pool error")))
 
 	az := GetTestCloud(ctrl)
 	az.LoadBalancerClient = mockLBClient
 	bc := newBackendPoolTypeNodeIPConfig(az)
 	svc := getTestService("test", v1.ProtocolTCP, nil, false, 80)
 	_, _, err := bc.ReconcileBackendPools(testClusterName, &svc, lb)
-	assert.Contains(t, err.Error(), "delete LB backend pool error")
+	assert.Contains(t, err.Error(), "create or update LB backend pool error")
 
 	lb = buildLBWithVMIPs(testClusterName, []string{"10.0.0.1", "10.0.0.2"})
-	mockLBClient.EXPECT().DeleteLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	mockLBClient.EXPECT().CreateOrUpdateBackendPools(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 	_, _, err = bc.ReconcileBackendPools(testClusterName, &svc, lb)
 	assert.NoError(t, err)
 	assert.Empty(t, (*lb.BackendAddressPools)[0].LoadBalancerBackendAddresses)
@@ -426,18 +426,6 @@ func TestReconcileBackendPoolsNodeIPConfigToIP(t *testing.T) {
 		"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool1-00000000-nic-1/ipConfigurations/ipconfig1",
 		"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool2-00000000-nic-1/ipConfigurations/ipconfig1",
 	})
-	mockLBClient := mockloadbalancerclient.NewMockInterface(ctrl)
-	mockLBClient.EXPECT().DeleteLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(retry.NewError(false, fmt.Errorf("error2")))
-	mockVMSet.EXPECT().EnsureBackendPoolDeleted(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-	az.LoadBalancerClient = mockLBClient
-	_, _, err = bi.ReconcileBackendPools(testClusterName, &svc, &lb)
-	assert.Contains(t, err.Error(), "error2")
-
-	lb = buildDefaultTestLB(testClusterName, []string{
-		"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool1-00000000-nic-1/ipConfigurations/ipconfig1",
-		"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool2-00000000-nic-1/ipConfigurations/ipconfig1",
-	})
-	mockLBClient.EXPECT().DeleteLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 	mockVMSet.EXPECT().EnsureBackendPoolDeleted(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 	_, _, err = bi.ReconcileBackendPools(testClusterName, &svc, &lb)
 	assert.NoError(t, err)
@@ -465,6 +453,9 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
 						IPAddress: to.StringPtr("4.3.2.1"),
 					},
 				},
+				{
+					LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{},
+				},
 			},
 		},
 	}
@@ -477,10 +468,13 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
 						IPAddress: to.StringPtr("5.6.7.8"),
 					},
 				},
+				{
+					LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{},
+				},
 			},
 		},
 	}
 
-	removeNodeIPAddressesFromBackendPool(backendPool, nodeIPAddresses)
+	removeNodeIPAddressesFromBackendPool(backendPool, nodeIPAddresses, false)
 	assert.Equal(t, expectedBackendPool, backendPool)
 }