diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go index 5bb9558294..50b924f266 100644 --- a/pkg/consts/consts.go +++ b/pkg/consts/consts.go @@ -120,6 +120,8 @@ const ( TagsDelimiter = "," // TagKeyValueDelimiter is the delimiter between keys and values in tagas TagKeyValueDelimiter = "=" + // VMSetNamesSharingPrimarySLBDelimiter is the delimiter of vmSet names sharing the primary SLB + VMSetNamesSharingPrimarySLBDelimiter = "," ) // cache diff --git a/pkg/provider/azure.go b/pkg/provider/azure.go index 8f374098eb..58f3e20310 100644 --- a/pkg/provider/azure.go +++ b/pkg/provider/azure.go @@ -171,12 +171,16 @@ type Config struct { CloudProviderBackoff bool `json:"cloudProviderBackoff,omitempty" yaml:"cloudProviderBackoff,omitempty"` // Use instance metadata service where possible UseInstanceMetadata bool `json:"useInstanceMetadata,omitempty" yaml:"useInstanceMetadata,omitempty"` + // EnableMultipleStandardLoadBalancers determines the behavior of the standard load balancer. If set to true // there would be one standard load balancer per VMAS or VMSS, which is similar with the behavior of the basic // load balancer. Users could select the specific standard load balancer for their service by the service // annotation `service.beta.kubernetes.io/azure-load-balancer-mode`, If set to false, the same standard load balancer // would be shared by all services in the cluster. In this case, the mode selection annotation would be ignored. EnableMultipleStandardLoadBalancers bool `json:"enableMultipleStandardLoadBalancers,omitempty" yaml:"enableMultipleStandardLoadBalancers,omitempty"` + // NodePoolsWithoutDedicatedSLB stores the VMAS/VMSS names that share the primary standard load balancer instead + // of having a dedicated one. This is useful only when EnableMultipleStandardLoadBalancers is set to true. + NodePoolsWithoutDedicatedSLB string `json:"nodePoolsWithoutDedicatedSLB,omitempty" yaml:"nodePoolsWithoutDedicatedSLB,omitempty"` // Backoff exponent CloudProviderBackoffExponent float64 `json:"cloudProviderBackoffExponent,omitempty" yaml:"cloudProviderBackoffExponent,omitempty"` @@ -261,6 +265,8 @@ type Cloud struct { // ipv6DualStack allows overriding for unit testing. It's normally initialized from featuregates ipv6DualStackEnabled bool + // isSHaredLoadBalancerSynced indicates if the reconcileSharedLoadBalancer has been run + isSharedLoadBalancerSynced bool // Lock for access to node caches, includes nodeZones, nodeResourceGroups, and unmanagedNodes. nodeCachesLock sync.RWMutex // nodeNames holds current nodes for tracking added nodes in VM caches. diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go index 1527e79680..d184d3379b 100644 --- a/pkg/provider/azure_loadbalancer.go +++ b/pkg/provider/azure_loadbalancer.go @@ -62,7 +62,7 @@ func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, servic return existsPip }() - _, status, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false) + _, status, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false, []network.LoadBalancer{}) if err != nil { return nil, existsPip, err } @@ -203,22 +203,25 @@ func (az *Cloud) getLoadBalancerResourceGroup() string { return az.ResourceGroup } -// cleanBackendpoolForPrimarySLB decouples the unwanted nodes from the standard load balancer. -// This is needed because when migrating from single SLB to multiple SLBs, The existing +// cleanupVMSetFromBackendPoolByCondition removes nodes of the unwanted vmSet from the lb backend pool. +// This is needed in two scenarios: +// 1. When migrating from single SLB to multiple SLBs, the existing // SLB's backend pool contains nodes from different agent pools, while we only want the // nodes from the primary agent pool to join the backend pool. -func (az *Cloud) cleanBackendpoolForPrimarySLB(primarySLB *network.LoadBalancer, service *v1.Service, clusterName string) (*network.LoadBalancer, error) { +// 2. When migrating from dedicated SLB to shared SLB (or vice versa), we should move the vmSet from +// one SLB to another one. +func (az *Cloud) cleanupVMSetFromBackendPoolByCondition(slb *network.LoadBalancer, service *v1.Service, clusterName string, shouldRemoveVMSetFromSLB func(string) bool) (*network.LoadBalancer, error) { lbBackendPoolName := getBackendPoolName(clusterName, service) lbResourceGroup := az.getLoadBalancerResourceGroup() - lbBackendPoolID := az.getBackendPoolID(to.String(primarySLB.Name), lbResourceGroup, lbBackendPoolName) + lbBackendPoolID := az.getBackendPoolID(to.String(slb.Name), lbResourceGroup, lbBackendPoolName) newBackendPools := make([]network.BackendAddressPool, 0) - if primarySLB.LoadBalancerPropertiesFormat != nil && primarySLB.BackendAddressPools != nil { - newBackendPools = *primarySLB.BackendAddressPools + if slb.LoadBalancerPropertiesFormat != nil && slb.BackendAddressPools != nil { + newBackendPools = *slb.BackendAddressPools } vmSetNameToBackendIPConfigurationsToBeDeleted := make(map[string][]network.InterfaceIPConfiguration) for j, bp := range newBackendPools { if strings.EqualFold(to.String(bp.Name), lbBackendPoolName) { - klog.V(2).Infof("cleanBackendpoolForPrimarySLB: checking the backend pool %s from standard load balancer %s", to.String(bp.Name), to.String(primarySLB.Name)) + klog.V(2).Infof("cleanupVMSetFromBackendPoolByCondition: checking the backend pool %s from standard load balancer %s", to.String(bp.Name), to.String(slb.Name)) if bp.BackendAddressPoolPropertiesFormat != nil && bp.BackendIPConfigurations != nil { for i := len(*bp.BackendIPConfigurations) - 1; i >= 0; i-- { ipConf := (*bp.BackendIPConfigurations)[i] @@ -227,9 +230,9 @@ func (az *Cloud) cleanBackendpoolForPrimarySLB(primarySLB *network.LoadBalancer, if err != nil { return nil, err } - primaryVMSetName := az.VMSet.GetPrimaryVMSetName() - if !strings.EqualFold(primaryVMSetName, vmSetName) && vmSetName != "" { - klog.V(2).Infof("cleanBackendpoolForPrimarySLB: found unwanted vmSet %s, decouple it from the LB", vmSetName) + + if shouldRemoveVMSetFromSLB(vmSetName) { + klog.V(2).Infof("cleanupVMSetFromBackendPoolByCondition: found unwanted vmSet %s, decouple it from the LB", vmSetName) // construct a backendPool that only contains the IP config of the node to be deleted interfaceIPConfigToBeDeleted := network.InterfaceIPConfiguration{ ID: to.StringPtr(ipConfigID), @@ -258,9 +261,14 @@ func (az *Cloud) cleanBackendpoolForPrimarySLB(primarySLB *network.LoadBalancer, if err != nil { return nil, err } - primarySLB.BackendAddressPools = &newBackendPools + slb.BackendAddressPools = &newBackendPools + // Proactively disable the etag to prevent etag mismatch error when putting lb later. + // This could be happen because when we remove the hosts from the lb, the nrp + // would put the lb to remove the backend references as well. + slb.Etag = nil } - return primarySLB, nil + + return slb, nil } // shouldChangeLoadBalancer determines if the load balancer of the service should be switched to another one @@ -268,15 +276,18 @@ func (az *Cloud) cleanBackendpoolForPrimarySLB(primarySLB *network.LoadBalancer, // existing service is changed to another VMSS/VMAS. func (az *Cloud) shouldChangeLoadBalancer(service *v1.Service, currLBName, clusterName string) bool { hasMode, isAuto, vmSetName := az.getServiceLoadBalancerMode(service) + // if no mode is given or the mode is `__auto__`, the current LB should be kept if !hasMode || isAuto { return false } + // if using the single standard load balancer, the current LB should be kept useSingleSLB := az.useStandardLoadBalancer() && !az.EnableMultipleStandardLoadBalancers if useSingleSLB { return false } + // if the current LB is what we want, keep it lbName := strings.TrimSuffix(currLBName, consts.InternalLoadBalancerNameSuffix) if strings.EqualFold(lbName, vmSetName) { @@ -285,6 +296,16 @@ func (az *Cloud) shouldChangeLoadBalancer(service *v1.Service, currLBName, clust if strings.EqualFold(vmSetName, az.VMSet.GetPrimaryVMSetName()) && strings.EqualFold(clusterName, lbName) { return false } + + // if the vmSet selected by the annotation is sharing the primary slb, and the service + // has been associated to the primary slb, keep it + useMultipleSLBs := az.useStandardLoadBalancer() && az.EnableMultipleStandardLoadBalancers + if useMultipleSLBs && + az.getVMSetNamesSharingPrimarySLB().Has(strings.ToLower(vmSetName)) && + strings.EqualFold(lbName, clusterName) { + return false + } + // if the VMSS/VMAS of the current LB is different from the mode, change the LB // to another one klog.V(2).Infof("shouldChangeLoadBalancer(%s, %s, %s): change the LB to another one", service.Name, currLBName, clusterName) @@ -414,32 +435,177 @@ func (az *Cloud) cleanOrphanedLoadBalancer(lb *network.LoadBalancer, service *v1 return nil } +func (az *Cloud) reconcileSharedLoadBalancer(service *v1.Service, clusterName string, nodes []*v1.Node) ([]network.LoadBalancer, error) { + var ( + primarySLB network.LoadBalancer + changed bool + ipConfigIDsToBeAddedToPrimarySLB []string + existingLBs []network.LoadBalancer + err error + ) + + // only run once since the controller manager rebooted + if az.isSharedLoadBalancerSynced { + return nil, nil + } + defer func() { + if err == nil { + az.isSharedLoadBalancerSynced = true + } + }() + + // skip if the cluster doesn't enable the multiple slbs mode + useMultipleSLBs := az.useStandardLoadBalancer() && az.EnableMultipleStandardLoadBalancers + if !useMultipleSLBs { + return nil, nil + } + + existingLBs, err = az.ListLB(service) + if err != nil { + return nil, fmt.Errorf("reconcileSharedLoadBalancer: failed to list LB: %w", err) + } + + lbBackendPoolName := getBackendPoolName(clusterName, service) + + for i := len(existingLBs) - 1; i >= 0; i-- { + lb := existingLBs[i] + + // skip the primary load balancer + if strings.EqualFold(to.String(lb.Name), clusterName) { + primarySLB = lb + continue + } + + // For non-primary load balancer, the lb name is the name of the VMSet. + // If the VMSet name is in az.NodePoolsWithoutDedicatedSLB, we should + // decouple the VMSet from the lb and delete the lb. Then we should + // add the VMSet to the backend pool of the primary slb. + vmSetName := strings.ToLower(to.String(lb.Name)) + lbBackendPoolID := az.getBackendPoolID(to.String(lb.Name), az.getLoadBalancerResourceGroup(), getBackendPoolName(clusterName, service)) + if az.getVMSetNamesSharingPrimarySLB().Has(vmSetName) { + err = az.VMSet.EnsureBackendPoolDeleted(service, lbBackendPoolID, vmSetName, lb.BackendAddressPools) + if err != nil { + return nil, fmt.Errorf("reconcileSharedLoadBalancer: failed to EnsureBackendPoolDeleted: %w", err) + } + + klog.V(2).Infof("reconcileSharedLoadBalancer: deleting LB %s because the corresponding vmSet is sharing the primary SLB", to.String(lb.Name)) + err = az.DeleteLB(service, to.String(lb.Name)) + if err != nil { + return nil, fmt.Errorf("reconcileSharedLoadBalancer: failed to DeleteLB: %w", err) + } + _ = az.lbCache.Delete(to.String(lb.Name)) + + primaryLBBackendPoolID := az.getBackendPoolID(clusterName, az.getLoadBalancerResourceGroup(), getBackendPoolName(clusterName, service)) + err = az.VMSet.EnsureHostsInPool(service, nodes, primaryLBBackendPoolID, vmSetName, false) + if err != nil { + return nil, fmt.Errorf("reconcileSharedLoadBalancer: failed to EnsureHostsInPool: %w", err) + } + + if lb.LoadBalancerPropertiesFormat != nil && + lb.BackendAddressPools != nil { + for i := 0; i < len(*lb.BackendAddressPools); i++ { + backendPool := (*lb.BackendAddressPools)[i] + if strings.EqualFold(to.String(backendPool.Name), lbBackendPoolName) { + if backendPool.BackendAddressPoolPropertiesFormat != nil && + backendPool.BackendIPConfigurations != nil { + for _, ipConfiguration := range *backendPool.BackendIPConfigurations { + if ipConfiguration.ID != nil { + ipConfigIDsToBeAddedToPrimarySLB = append(ipConfigIDsToBeAddedToPrimarySLB, to.String(ipConfiguration.ID)) + } + } + } + } + } + } + + // remove the deleted lb from the list and construct a new primary + // lb, so that getServiceLoadBalancer doesn't have to call list api again + existingLBs = append(existingLBs[:i], existingLBs[i+1:]...) + changed = true + } + } + + if changed { + if primarySLB.LoadBalancerPropertiesFormat != nil && + primarySLB.BackendAddressPools != nil { + for i := 0; i < len(*primarySLB.BackendAddressPools); i++ { + if strings.EqualFold(to.String((*primarySLB.BackendAddressPools)[i].Name), lbBackendPoolName) { + backendPoolIPConfigs := (*primarySLB.BackendAddressPools)[i].BackendIPConfigurations + for _, id := range ipConfigIDsToBeAddedToPrimarySLB { + *backendPoolIPConfigs = append(*backendPoolIPConfigs, network.InterfaceIPConfiguration{ + ID: to.StringPtr(id), + }) + } + break + } + } + } + + for i, existingLB := range existingLBs { + if strings.EqualFold(to.String(existingLB.Name), clusterName) { + // Proactively disable the etag to prevent etag mismatch error when putting lb later. + // This could be happen because when we remove the hosts from the lb, the nrp + // would put the lb to remove the backend references as well. + primarySLB.Etag = nil + + existingLBs[i] = primarySLB + return existingLBs, nil + } + } + } + + return existingLBs, nil +} + // getServiceLoadBalancer gets the loadbalancer for the service if it already exists. // If wantLb is TRUE then -it selects a new load balancer. // In case the selected load balancer does not exist it returns network.LoadBalancer struct // with added metadata (such as name, location) and existsLB set to FALSE. // By default - cluster default LB is returned. -func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string, nodes []*v1.Node, wantLb bool) (lb *network.LoadBalancer, status *v1.LoadBalancerStatus, exists bool, err error) { +func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string, nodes []*v1.Node, wantLb bool, existingLBs []network.LoadBalancer) (lb *network.LoadBalancer, status *v1.LoadBalancerStatus, exists bool, err error) { isInternal := requiresInternalLoadBalancer(service) var defaultLB *network.LoadBalancer primaryVMSetName := az.VMSet.GetPrimaryVMSetName() defaultLBName := az.getAzureLoadBalancerName(clusterName, primaryVMSetName, isInternal) useMultipleSLBs := az.useStandardLoadBalancer() && az.EnableMultipleStandardLoadBalancers - existingLBs, err := az.ListLB(service) - if err != nil { - return nil, nil, false, err + // reuse the lb list from reconcileSharedLoadBalancer to reduce the api call + if len(existingLBs) == 0 { + existingLBs, err = az.ListLB(service) + if err != nil { + return nil, nil, false, err + } } // check if the service already has a load balancer for i := range existingLBs { existingLB := existingLBs[i] + + // for the primary standard load balancer, when enabled multiple slbs if strings.EqualFold(to.String(existingLB.Name), clusterName) && useMultipleSLBs { - cleanedLB, err := az.cleanBackendpoolForPrimarySLB(&existingLB, service, clusterName) + // there are two conditions we need to remove the vmSet from the + // backend pool of the primary SLB when enabling multiple SLBs: + shouldRemoveVMSetFromSLB := func(vmSetName string) bool { + // condition 1: not removing the vmSet from the primary SLB + // if it is supposed to share the primary SLB. + if az.getVMSetNamesSharingPrimarySLB().Has(strings.ToLower(vmSetName)) { + return false + } + + // condition 2: removing the vmSet from the primary SLB if + // it is not the primary vmSet. There are two situations: + // 1. when migrating from single SLB to multiple SLBs, we + // need to remove all non-primary vmSets from the primary SLB; + // 2. when migrating from shared mode to dedicated SLB, we + // need to remove the specific vmSet from the primary SLB. + return !strings.EqualFold(vmSetName, primaryVMSetName) && vmSetName != "" + } + cleanedLB, err := az.cleanupVMSetFromBackendPoolByCondition(&existingLB, service, clusterName, shouldRemoveVMSetFromSLB) if err != nil { return nil, nil, false, err } existingLB = *cleanedLB + existingLBs[i] = *cleanedLB } if strings.EqualFold(*existingLB.Name, defaultLBName) { defaultLB = &existingLB @@ -709,7 +875,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s return service.Status.LoadBalancer.Ingress[0].IP, nil } - _, lbStatus, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false) + _, lbStatus, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false, []network.LoadBalancer{}) if err != nil { return "", err } @@ -1204,7 +1370,14 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, isBackendPoolPreConfigured := az.isBackendPoolPreConfigured(service) serviceName := getServiceName(service) klog.V(2).Infof("reconcileLoadBalancer for service(%s) - wantLb(%t): started", serviceName, wantLb) - lb, _, _, err := az.getServiceLoadBalancer(service, clusterName, nodes, wantLb) + + existingLBs, err := az.reconcileSharedLoadBalancer(service, clusterName, nodes) + if err != nil { + klog.Errorf("reconcileLoadBalancer: failed to reconcile shared load balancer: %w", err) + return nil, err + } + + lb, _, _, err := az.getServiceLoadBalancer(service, clusterName, nodes, wantLb, existingLBs) if err != nil { klog.Errorf("reconcileLoadBalancer: failed to get load balancer for service %q, error: %v", serviceName, err) return nil, err @@ -2160,7 +2333,7 @@ func (az *Cloud) getExpectedSecurityRules(wantLb bool, ports []v1.ServicePort, s } func (az *Cloud) shouldUpdateLoadBalancer(clusterName string, service *v1.Service) bool { - _, _, existsLb, _ := az.getServiceLoadBalancer(service, clusterName, nil, false) + _, _, existsLb, _ := az.getServiceLoadBalancer(service, clusterName, nil, false, []network.LoadBalancer{}) return existsLb && service.ObjectMeta.DeletionTimestamp == nil } diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go index 67f51b143d..09c633d603 100644 --- a/pkg/provider/azure_loadbalancer_test.go +++ b/pkg/provider/azure_loadbalancer_test.go @@ -1363,7 +1363,7 @@ func TestGetServiceLoadBalancer(t *testing.T) { test.service.Annotations = test.annotations az.LoadBalancerSku = test.sku lb, status, exists, err := az.getServiceLoadBalancer(&test.service, testClusterName, - clusterResources.nodes, test.wantLB) + clusterResources.nodes, test.wantLB, []network.LoadBalancer{}) assert.Equal(t, test.expectedLB, lb, "TestCase[%d]: %s", i, test.desc) assert.Equal(t, test.expectedStatus, status, "TestCase[%d]: %s", i, test.desc) assert.Equal(t, test.expectedExists, exists, "TestCase[%d]: %s", i, test.desc) @@ -1395,7 +1395,7 @@ func TestGetServiceLoadBalancerWithExtendedLocation(t *testing.T) { az.LoadBalancerClient = mockLBsClient lb, status, exists, err := az.getServiceLoadBalancer(&service, testClusterName, - clusterResources.nodes, false) + clusterResources.nodes, false, []network.LoadBalancer{}) assert.Equal(t, expectedLB, lb, "GetServiceLoadBalancer shall return a default LB with expected location.") assert.Nil(t, status, "GetServiceLoadBalancer: Status should be nil for default LB.") assert.Equal(t, false, exists, "GetServiceLoadBalancer: Default LB should not exist.") @@ -1420,7 +1420,7 @@ func TestGetServiceLoadBalancerWithExtendedLocation(t *testing.T) { az.LoadBalancerClient = mockLBsClient lb, status, exists, err = az.getServiceLoadBalancer(&service, testClusterName, - clusterResources.nodes, true) + clusterResources.nodes, true, []network.LoadBalancer{}) assert.Equal(t, expectedLB, lb, "GetServiceLoadBalancer shall return a new LB with expected location.") assert.Nil(t, status, "GetServiceLoadBalancer: Status should be nil for new LB.") assert.Equal(t, false, exists, "GetServiceLoadBalancer: LB should not exist before hand.") @@ -3850,7 +3850,7 @@ func TestCheckLoadBalancerResourcesConflicted(t *testing.T) { } } -func TestCleanBackendpoolForPrimarySLB(t *testing.T) { +func TestCleanupVMSetFromBackendPoolByCondition(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() cloud := GetTestCloud(ctrl) @@ -3882,8 +3882,7 @@ func TestCleanBackendpoolForPrimarySLB(t *testing.T) { mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool2-00000000-nic-1", gomock.Any()).Return(existingNICForAS2, nil).Times(3) mockNICClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) cloud.InterfacesClient = mockNICClient - cleanedLB, err := cloud.cleanBackendpoolForPrimarySLB(&lb, &service, clusterName) - assert.NoError(t, err) + expectedLB := network.LoadBalancer{ Name: to.StringPtr("testCluster"), LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{ @@ -3901,6 +3900,12 @@ func TestCleanBackendpoolForPrimarySLB(t *testing.T) { }, }, } + + shouldRemoveVMSetFromSLB := func(vmSetName string) bool { + return !strings.EqualFold(vmSetName, cloud.VMSet.GetPrimaryVMSetName()) && vmSetName != "" + } + cleanedLB, err := cloud.cleanupVMSetFromBackendPoolByCondition(&lb, &service, clusterName, shouldRemoveVMSetFromSLB) + assert.NoError(t, err) assert.Equal(t, expectedLB, *cleanedLB) } @@ -4025,11 +4030,12 @@ func TestEnsureLoadBalancerTagged(t *testing.T) { } func TestShouldChangeLoadBalancer(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + cloud := GetTestCloud(ctrl) + cloud.LoadBalancerSku = consts.LoadBalancerSkuBasic + t.Run("shouldChangeLoadBalancer should return true if the mode is different from the current vm set", func(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - cloud := GetTestCloud(ctrl) - cloud.LoadBalancerSku = consts.LoadBalancerSkuBasic annotations := map[string]string{ consts.ServiceAnnotationLoadBalancerMode: "as2", } @@ -4037,6 +4043,19 @@ func TestShouldChangeLoadBalancer(t *testing.T) { res := cloud.shouldChangeLoadBalancer(&service, "as1", "testCluster") assert.True(t, res) }) + + t.Run("shouldChangeLoadBalancer should return false if the current lb is the primary slb and the vmSet selected by annotation is sharing the primary slb", func(t *testing.T) { + cloud.LoadBalancerSku = consts.LoadBalancerSkuStandard + cloud.EnableMultipleStandardLoadBalancers = true + cloud.NodePoolsWithoutDedicatedSLB = "vmss-1,vmss2" + + annotations := map[string]string{ + consts.ServiceAnnotationLoadBalancerMode: "vmss-1", + } + service := getTestService("service1", v1.ProtocolTCP, annotations, false, 80) + res := cloud.shouldChangeLoadBalancer(&service, "testCluster-internal", "testCluster") + assert.False(t, res) + }) } func TestRemoveFrontendIPConfigurationFromLoadBalancerDelete(t *testing.T) { @@ -4172,3 +4191,117 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) { }) } } + +func TestReconcileSharedLoadBalancer(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + for _, tc := range []struct { + description, vmSetsSharingPrimarySLB string + useMultipleSLBs bool + existingLBs []network.LoadBalancer + expectedListCount, expectedDeleteCount int + expectedLBs []network.LoadBalancer + expectedErr error + }{ + { + description: "reconcileSharedLoadBalancer should decouple the vmSet from its dedicated lb if the vmSet is sharing the primary slb", + useMultipleSLBs: true, + vmSetsSharingPrimarySLB: "vmss1,vmss2", + existingLBs: []network.LoadBalancer{ + { + Name: to.StringPtr("kubernetes"), + LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{ + BackendAddressPools: &[]network.BackendAddressPool{ + { + Name: to.StringPtr("kubernetes"), + BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{ + BackendIPConfigurations: &[]network.InterfaceIPConfiguration{ + { + ID: to.StringPtr("vmss2-nic-1"), + }, + }, + }, + }, + }, + }, + }, + { + Name: to.StringPtr("vmss1"), + LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{ + BackendAddressPools: &[]network.BackendAddressPool{ + { + Name: to.StringPtr("kubernetes"), + BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{ + BackendIPConfigurations: &[]network.InterfaceIPConfiguration{ + { + ID: to.StringPtr("vmss1-nic-1"), + }, + }, + }, + }, + }, + }, + }, + }, + expectedLBs: []network.LoadBalancer{ + { + Name: to.StringPtr("kubernetes"), + LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{ + BackendAddressPools: &[]network.BackendAddressPool{ + { + Name: to.StringPtr("kubernetes"), + BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{ + BackendIPConfigurations: &[]network.InterfaceIPConfiguration{ + { + ID: to.StringPtr("vmss2-nic-1"), + }, + { + ID: to.StringPtr("vmss1-nic-1"), + }, + }, + }, + }, + }, + }, + }, + }, + expectedListCount: 1, + expectedDeleteCount: 1, + }, + { + description: "reconcileSharedLoadBalancer should do nothing if the multiple slbs is not enabled", + }, + { + description: "reconcileSharedLoadBalancer should do nothing if the vmSet is not sharing the primary slb", + useMultipleSLBs: true, + vmSetsSharingPrimarySLB: "vmss2,vmss3", + expectedListCount: 1, + }, + } { + t.Run(tc.description, func(t *testing.T) { + cloud := GetTestCloud(ctrl) + + cloud.NodePoolsWithoutDedicatedSLB = tc.vmSetsSharingPrimarySLB + + if tc.useMultipleSLBs { + cloud.LoadBalancerSku = consts.VMTypeStandard + cloud.EnableMultipleStandardLoadBalancers = true + } + + mockLBClient := cloud.LoadBalancerClient.(*mockloadbalancerclient.MockInterface) + mockLBClient.EXPECT().List(gomock.Any(), cloud.ResourceGroup).Return(tc.existingLBs, nil).Times(tc.expectedListCount) + mockLBClient.EXPECT().Delete(gomock.Any(), cloud.ResourceGroup, "vmss1").Return(nil).Times(tc.expectedDeleteCount) + + mockVMSet := NewMockVMSet(ctrl) + mockVMSet.EXPECT().EnsureBackendPoolDeleted(gomock.Any(), gomock.Any(), "vmss1", gomock.Any()).Return(nil).Times(tc.expectedDeleteCount) + mockVMSet.EXPECT().EnsureHostsInPool(gomock.Any(), gomock.Any(), gomock.Any(), "vmss1", false).Return(nil).Times(tc.expectedDeleteCount) + cloud.VMSet = mockVMSet + + service := getTestService("test", v1.ProtocolTCP, nil, false, 80) + lbs, err := cloud.reconcileSharedLoadBalancer(&service, "kubernetes", []*v1.Node{}) + assert.Equal(t, tc.expectedErr, err) + assert.Equal(t, tc.expectedLBs, lbs) + }) + } +} diff --git a/pkg/provider/azure_standard.go b/pkg/provider/azure_standard.go index 74f242c80c..9992dccdd9 100644 --- a/pkg/provider/azure_standard.go +++ b/pkg/provider/azure_standard.go @@ -132,11 +132,16 @@ func (az *Cloud) getAzureLoadBalancerName(clusterName string, vmSetName string, lbNamePrefix := vmSetName // The LB name prefix is set to the name of the cluster when: // 1. the LB belongs to the primary agent pool. - // 2. using the single SLB; + // 2. using the single SLB. useSingleSLB := az.useStandardLoadBalancer() && !az.EnableMultipleStandardLoadBalancers if strings.EqualFold(vmSetName, az.VMSet.GetPrimaryVMSetName()) || useSingleSLB { lbNamePrefix = clusterName } + // 3. using multiple SLBs while the vmSet is sharing the primary SLB + useMultipleSLB := az.useStandardLoadBalancer() && az.EnableMultipleStandardLoadBalancers + if useMultipleSLB && az.getVMSetNamesSharingPrimarySLB().Has(strings.ToLower(vmSetName)) { + lbNamePrefix = clusterName + } if isInternal { return fmt.Sprintf("%s%s", lbNamePrefix, consts.InternalLoadBalancerNameSuffix) } diff --git a/pkg/provider/azure_standard_test.go b/pkg/provider/azure_standard_test.go index 114a8182d6..f7e344b2e2 100644 --- a/pkg/provider/azure_standard_test.go +++ b/pkg/provider/azure_standard_test.go @@ -216,13 +216,15 @@ func TestGetAzureLoadBalancerName(t *testing.T) { az.PrimaryAvailabilitySetName = primary cases := []struct { - description string - vmSet string - isInternal bool - useStandardLB bool - clusterName string - lbName string - expected string + description string + vmSet string + isInternal bool + useStandardLB bool + enableMultipleSLBs bool + vmSetsSharingPrimarySLB string + clusterName string + lbName string + expected string }{ { description: "prefix of loadBalancerName should be az.LoadBalancerName if az.LoadBalancerName is not nil", @@ -287,6 +289,23 @@ func TestGetAzureLoadBalancerName(t *testing.T) { clusterName: "azure", expected: "azure-internal", }, + { + description: "getAzureLoadBalancerName should return the vmSet name if multiple slbs are enabled", + vmSet: "as", + useStandardLB: true, + enableMultipleSLBs: true, + clusterName: "azure", + expected: "as", + }, + { + description: "getAzureLoadBalancerName should return the cluster name if multiple slbs are enabled and the vmSet is sharing the primary slb", + vmSet: "as", + useStandardLB: true, + enableMultipleSLBs: true, + vmSetsSharingPrimarySLB: "as , as-1", + clusterName: "azure", + expected: "azure", + }, } for _, c := range cases { @@ -295,6 +314,12 @@ func TestGetAzureLoadBalancerName(t *testing.T) { } else { az.Config.LoadBalancerSku = consts.LoadBalancerSkuBasic } + if c.enableMultipleSLBs { + az.EnableMultipleStandardLoadBalancers = true + } + if c.vmSetsSharingPrimarySLB != "" { + az.NodePoolsWithoutDedicatedSLB = c.vmSetsSharingPrimarySLB + } az.Config.LoadBalancerName = c.lbName loadbalancerName := az.getAzureLoadBalancerName(c.clusterName, c.vmSet, c.isInternal) assert.Equal(t, c.expected, loadbalancerName, c.description) diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go index 58ac42cbc0..489b1175e1 100644 --- a/pkg/provider/azure_utils.go +++ b/pkg/provider/azure_utils.go @@ -142,3 +142,15 @@ func (az *Cloud) reconcileTags(currentTagsOnResource, newTags map[string]*string return currentTagsOnResource, changed } + +func (az *Cloud) getVMSetNamesSharingPrimarySLB() sets.String { + vmSetNames := make([]string, 0) + if az.NodePoolsWithoutDedicatedSLB != "" { + vmSetNames = strings.Split(az.Config.NodePoolsWithoutDedicatedSLB, consts.VMSetNamesSharingPrimarySLBDelimiter) + for i := 0; i < len(vmSetNames); i++ { + vmSetNames[i] = strings.ToLower(strings.TrimSpace(vmSetNames[i])) + } + } + + return sets.NewString(vmSetNames...) +}