Skip to content

Commit

Permalink
[release-1.31] Multi-slb related bug fixes (#7605)
Browse files Browse the repository at this point in the history 
* fix: Include all endpointslices for local services when using multi-slb

* fix: Reconcile correct backend pool when using multi-slb

---------

Co-authored-by: Qi Ni <pomelonicky@gmail.com>
  • Loading branch information
@k8s-infra-cherrypick-robot @nilo19
k8s-infra-cherrypick-robot and nilo19 authored Nov 19, 2024
1 parent 1e5ff41 commit 1b8f6c0
Show file tree
Hide file tree
Showing 9 changed files with 217 additions and 101 deletions.
2 changes: 1 addition & 1 deletion pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
az.localServiceNameToServiceInfoMap.Store(key, newServiceInfo(getServiceIPFamily(service), lbName))
// There are chances that the endpointslice changes after EnsureHostsInPool, so
// need to check endpointslice for a second time.
if err := az.checkAndApplyLocalServiceBackendPoolUpdates(ctx, *lb, service); err != nil {
if err := az.checkAndApplyLocalServiceBackendPoolUpdates(*lb, service); err != nil {
logger.Error(err, "Failed to checkAndApplyLocalServiceBackendPoolUpdates")
return nil, err
}
Expand Down
33 changes: 21 additions & 12 deletions pkg/provider/azure_loadbalancer_backendpool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (bc *backendPoolTypeNodeIPConfig) ReconcileBackendPools(
if bp.BackendAddressPoolPropertiesFormat != nil &&
bp.LoadBalancerBackendAddresses != nil &&
len(*bp.LoadBalancerBackendAddresses) > 0 {
if removeNodeIPAddressesFromBackendPool(bp, []string{}, true, false) {
if removeNodeIPAddressesFromBackendPool(bp, []string{}, true, false, false) {
isMigration = true
bp.VirtualNetwork = nil
if err := bc.CreateOrUpdateLBBackendPool(ctx, lbName, bp); err != nil {
Expand Down Expand Up @@ -410,7 +410,6 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
changed bool
numOfAdd, numOfDelete int
activeNodes *utilsets.IgnoreCaseSet
err error
)
if bi.useMultipleStandardLoadBalancers() {
if !isLocalService(service) {
Expand All @@ -425,10 +424,12 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
"current load balancer", si.lbName)
return nil
}
activeNodes, err = bi.getLocalServiceEndpointsNodeNames(ctx, service)
if err != nil {
return err
}
activeNodes = bi.getLocalServiceEndpointsNodeNames(service)
}

if isNICPool(backendPool) {
klog.V(4).InfoS("EnsureHostsInPool: skipping NIC-based backend pool", "backendPoolName", ptr.Deref(backendPool.Name, ""))
return nil
}
}

Expand Down Expand Up @@ -463,7 +464,7 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
}

if bi.useMultipleStandardLoadBalancers() {
if !activeNodes.Has(node.Name) {
if activeNodes != nil && !activeNodes.Has(node.Name) {
klog.V(4).Infof("bi.EnsureHostsInPool: node %s should not be in load balancer %q", node.Name, lbName)
continue
}
Expand Down Expand Up @@ -500,7 +501,7 @@ func (bi *backendPoolTypeNodeIP) EnsureHostsInPool(ctx context.Context, service
}
}
}
removeNodeIPAddressesFromBackendPool(backendPool, nodeIPsToBeDeleted, false, bi.useMultipleStandardLoadBalancers())
removeNodeIPAddressesFromBackendPool(backendPool, nodeIPsToBeDeleted, false, bi.useMultipleStandardLoadBalancers(), true)
}
if changed {
klog.V(2).Infof("bi.EnsureHostsInPool: updating backend pool %s of load balancer %s to add %d nodes and remove %d nodes", lbBackendPoolName, lbName, numOfAdd, numOfDelete)
Expand Down Expand Up @@ -633,7 +634,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(ctx context.Context, clus
if isMigration && bi.EnableMigrateToIPBasedBackendPoolAPI {
var backendPoolNames []string
for _, id := range lbBackendPoolIDsSlice {
name, err := getLBNameFromBackendPoolID(id)
name, err := getBackendPoolNameFromBackendPoolID(id)
if err != nil {
klog.Errorf("bi.ReconcileBackendPools for service (%s): failed to get LB name from backend pool ID: %s", serviceName, err.Error())
return false, false, nil, err
Expand Down Expand Up @@ -674,7 +675,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(ctx context.Context, clus
}
}
if len(nodeIPAddressesToBeDeleted) > 0 {
if removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted, false, false) {
if removeNodeIPAddressesFromBackendPool(bp, nodeIPAddressesToBeDeleted, false, false, true) {
updated = true
}
}
Expand Down Expand Up @@ -871,11 +872,13 @@ func hasIPAddressInBackendPool(backendPool *network.BackendAddressPool, ipAddres
func removeNodeIPAddressesFromBackendPool(
backendPool network.BackendAddressPool,
nodeIPAddresses []string,
removeAll, useMultipleStandardLoadBalancers bool,
removeAll, useMultipleStandardLoadBalancers, isNodeIP bool,
) bool {
changed := false
nodeIPsSet := utilsets.NewString(nodeIPAddresses...)

logger := klog.Background().WithName("removeNodeIPAddressFromBackendPool")

if backendPool.BackendAddressPoolPropertiesFormat == nil ||
backendPool.LoadBalancerBackendAddresses == nil {
return false
Expand All @@ -886,7 +889,13 @@ func removeNodeIPAddressesFromBackendPool(
if addresses[i].LoadBalancerBackendAddressPropertiesFormat != nil {
ipAddress := ptr.Deref((*backendPool.LoadBalancerBackendAddresses)[i].IPAddress, "")
if ipAddress == "" {
klog.V(4).Infof("removeNodeIPAddressFromBackendPool: LoadBalancerBackendAddress %s is not IP-based, skipping", ptr.Deref(addresses[i].Name, ""))
if isNodeIP {
logger.V(4).Info("LoadBalancerBackendAddress is not IP-based, removing", "LoadBalancerBackendAddress", ptr.Deref(addresses[i].Name, ""))
addresses = append(addresses[:i], addresses[i+1:]...)
changed = true
} else {
logger.V(4).Info("LoadBalancerBackendAddress is not IP-based, skipping", "LoadBalancerBackendAddress", ptr.Deref(addresses[i].Name, ""))
}
continue
}
if removeAll || nodeIPsSet.Has(ipAddress) {
Expand Down
70 changes: 44 additions & 26 deletions pkg/provider/azure_loadbalancer_backendpool_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,19 +201,14 @@ func TestEnsureHostsInPoolNodeIP(t *testing.T) {
},
},
{
desc: "should add correct nodes to the pool and remove unwanted ones when using multi-slb",
desc: "should skip NIC-based backend pool when using multi-slb",
backendPool: network.BackendAddressPool{
Name: ptr.To("kubernetes"),
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
{
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.1"),
},
},
{
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.3"),
IPAddress: ptr.To(""),
},
},
},
Expand All @@ -230,47 +225,53 @@ func TestEnsureHostsInPoolNodeIP(t *testing.T) {
expectedBackendPool: network.BackendAddressPool{
Name: ptr.To("kubernetes"),
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
VirtualNetwork: &network.SubResource{ID: ptr.To("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/vnet")},
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
{
Name: ptr.To("vmss-2"),
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.4"),
IPAddress: ptr.To(""),
},
},
},
},
},
skip: true,
},
{
desc: "should add ips to the local service dedicated backend pool",
local: true,
desc: "should add correct nodes to the pool and remove unwanted ones when using multi-slb",
backendPool: network.BackendAddressPool{
Name: ptr.To("default-svc-1"),
Name: ptr.To("kubernetes"),
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{},
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
{
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.1"),
},
},
{
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.3"),
},
},
},
},
},
multiSLBConfigs: []MultipleStandardLoadBalancerConfiguration{
{
Name: "kubernetes",
MultipleStandardLoadBalancerConfigurationStatus: MultipleStandardLoadBalancerConfigurationStatus{
ActiveNodes: utilsets.NewString("vmss-2"),
},
},
},
expectedBackendPool: network.BackendAddressPool{
Name: ptr.To("default-svc-1"),
Name: ptr.To("kubernetes"),
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
VirtualNetwork: &network.SubResource{ID: ptr.To("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/vnet")},
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
{
Name: ptr.To("vmss-0"),
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.2"),
},
},
{
Name: ptr.To("vmss-1"),
Name: ptr.To("vmss-2"),
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
IPAddress: ptr.To("10.0.0.1"),
IPAddress: ptr.To("10.0.0.4"),
},
},
},
Expand Down Expand Up @@ -934,20 +935,22 @@ func TestReconcileBackendPoolsNodeIPConfigToIPWithMigrationAPI(t *testing.T) {
mockVMSet.EXPECT().GetPrimaryVMSetName().Return("k8s-agentpool1-00000000").AnyTimes()

mockLBClient := mockloadbalancerclient.NewMockInterface(ctrl)
mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(retry.NewError(false, errors.New("error")))
mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), "testCluster", []string{"testCluster"}).Return(retry.NewError(false, errors.New("error")))

az := GetTestCloud(ctrl)
az.VMSet = mockVMSet
az.LoadBalancerClient = mockLBClient
az.EnableMigrateToIPBasedBackendPoolAPI = true
az.LoadBalancerSku = "standard"
az.MultipleStandardLoadBalancerConfigurations = []MultipleStandardLoadBalancerConfiguration{{Name: "kubernetes"}}

bi := newBackendPoolTypeNodeIP(az)
svc := getTestService("test", v1.ProtocolTCP, nil, false, 80)
_, _, _, err := bi.ReconcileBackendPools(context.TODO(), testClusterName, &svc, &lb)
assert.Error(t, err)
assert.Contains(t, err.Error(), "error")

mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
mockLBClient.EXPECT().MigrateToIPBasedBackendPool(gomock.Any(), gomock.Any(), "testCluster", []string{"testCluster"}).Return(nil)
bps := buildLBWithVMIPs(testClusterName, []string{"1.2.3.4", "2.3.4.5"}).BackendAddressPools
mockLBClient.EXPECT().GetLBBackendPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return((*bps)[0], nil)
mockLBClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(network.LoadBalancer{}, nil)
Expand Down Expand Up @@ -980,6 +983,7 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
description string
removeAll, useMultiSLB bool
unwantedIPs, existingIPs, expectedIPs []string
isNodeIP bool
}{
{
description: "removeNodeIPAddressFromBackendPool should remove the unwanted IP addresses from the backend pool",
Expand Down Expand Up @@ -1007,12 +1011,26 @@ func TestRemoveNodeIPAddressFromBackendPool(t *testing.T) {
existingIPs: []string{"1.2.3.4", "4.3.2.1", ""},
expectedIPs: []string{""},
},
{
description: "removeNodeIPAddressFromBackendPool should skip non-IP based backend addresses when isNodeIP is false",
unwantedIPs: []string{"1.2.3.4"},
existingIPs: []string{"1.2.3.4", ""},
expectedIPs: []string{""},
},
{
description: "removeNodeIPAddressFromBackendPool should remove non-IP based backend addresses when isNodeIP is true",
unwantedIPs: []string{"1.2.3.4"},
existingIPs: []string{"1.2.3.4", ""},
expectedIPs: []string{},
useMultiSLB: true,
isNodeIP: true,
},
} {
t.Run(tc.description, func(t *testing.T) {
backendPool := buildTestLoadBalancerBackendPoolWithIPs("kubernetes", tc.existingIPs)
expectedBackendPool := buildTestLoadBalancerBackendPoolWithIPs("kubernetes", tc.expectedIPs)

removeNodeIPAddressesFromBackendPool(backendPool, tc.unwantedIPs, tc.removeAll, tc.useMultiSLB)
removeNodeIPAddressesFromBackendPool(backendPool, tc.unwantedIPs, tc.removeAll, tc.useMultiSLB, tc.isNodeIP)
assert.Equal(t, expectedBackendPool, backendPool)
})
}
Expand Down
20 changes: 17 additions & 3 deletions pkg/provider/azure_loadbalancer_repo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ func (az *Cloud) MigrateToIPBasedBackendPoolAndWaitForCompletion(
}
return true, nil
})

if err != nil {
if errors.Is(err, wait.ErrWaitTimeout) {
klog.Warningf("MigrateToIPBasedBackendPoolAndWaitForCompletion: Timeout waiting for migration to IP based backend pool for lb %s, backend pool %s", lbName, strings.Join(backendPoolNames, ","))
Expand Down Expand Up @@ -351,15 +350,15 @@ func (az *Cloud) getAzureLoadBalancer(ctx context.Context, name string, crt azca
// If not same, the lbName for existingBackendPools would also be returned.
func isBackendPoolOnSameLB(newBackendPoolID string, existingBackendPools []string) (bool, string, error) {
matches := backendPoolIDRE.FindStringSubmatch(newBackendPoolID)
if len(matches) != 2 {
if len(matches) != 3 {
return false, "", fmt.Errorf("new backendPoolID %q is in wrong format", newBackendPoolID)
}

newLBName := matches[1]
newLBNameTrimmed := trimSuffixIgnoreCase(newLBName, consts.InternalLoadBalancerNameSuffix)
for _, backendPool := range existingBackendPools {
matches := backendPoolIDRE.FindStringSubmatch(backendPool)
if len(matches) != 2 {
if len(matches) != 3 {
return false, "", fmt.Errorf("existing backendPoolID %q is in wrong format", backendPool)
}

Expand All @@ -380,3 +379,18 @@ func (az *Cloud) serviceOwnsRule(service *v1.Service, rule string) bool {
prefix := az.getRulePrefix(service)
return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))
}

func isNICPool(bp network.BackendAddressPool) bool {
logger := klog.Background().WithName("isNICPool").WithValues("backendPoolName", ptr.Deref(bp.Name, ""))
if bp.BackendAddressPoolPropertiesFormat != nil &&
bp.LoadBalancerBackendAddresses != nil {
for _, addr := range *bp.LoadBalancerBackendAddresses {
if ptr.Deref(addr.IPAddress, "") == "" {
logger.V(4).Info("The load balancer backend address has empty ip address, assuming it is a NIC pool",
"loadBalancerBackendAddress", ptr.Deref(addr.Name, ""))
return true
}
}
}
return false
}
Loading

0 comments on commit 1b8f6c0

Please sign in to comment.