diff --git a/pkg/cloudprovider/providers/azure/azure_backoff.go b/pkg/cloudprovider/providers/azure/azure_backoff.go index 6988d3c4ed3..3947e912a39 100644 --- a/pkg/cloudprovider/providers/azure/azure_backoff.go +++ b/pkg/cloudprovider/providers/azure/azure_backoff.go @@ -26,10 +26,11 @@ import ( "k8s.io/apimachinery/pkg/types" ) -// getOrCreateRequestBackoff returns a new Backoff object steps = 1 +// requestBackoff if backoff is disabled in cloud provider it +// returns a new Backoff object steps = 1 // This is to make sure that the requested command executes // at least once -func (az *Cloud) getOrCreateRequestBackoff() (resourceRequestBackoff wait.Backoff) { +func (az *Cloud) requestBackoff() (resourceRequestBackoff wait.Backoff) { if az.CloudProviderBackoff { return az.resourceRequestBackoff } @@ -44,7 +45,7 @@ func (az *Cloud) getOrCreateRequestBackoff() (resourceRequestBackoff wait.Backof func (az *Cloud) GetVirtualMachineWithRetry(name types.NodeName) (compute.VirtualMachine, bool, error) { var machine compute.VirtualMachine var exists bool - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error machine, exists, retryErr = az.getVirtualMachine(name) if retryErr != nil { @@ -60,7 +61,7 @@ func (az *Cloud) GetVirtualMachineWithRetry(name types.NodeName) (compute.Virtua // VirtualMachineClientGetWithRetry invokes az.VirtualMachinesClient.Get with exponential backoff retry func (az *Cloud) VirtualMachineClientGetWithRetry(resourceGroup, vmName string, types compute.InstanceViewTypes) (compute.VirtualMachine, error) { var machine compute.VirtualMachine - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() machine, retryErr = az.VirtualMachinesClient.Get(resourceGroup, vmName, types) @@ -78,7 +79,7 @@ func (az *Cloud) VirtualMachineClientGetWithRetry(resourceGroup, vmName string, func (az *Cloud) VirtualMachineClientListWithRetry() ([]compute.VirtualMachine, error) { allNodes := []compute.VirtualMachine{} var result compute.VirtualMachineListResult - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("VirtualMachinesClient.List(%v): start", az.ResourceGroup) @@ -103,7 +104,7 @@ func (az *Cloud) VirtualMachineClientListWithRetry() ([]compute.VirtualMachine, appendResults = false // follow the next link to get all the vms for resource group if result.NextLink != nil { - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("VirtualMachinesClient.ListNextResults(%v): start", az.ResourceGroup) @@ -130,7 +131,7 @@ func (az *Cloud) VirtualMachineClientListWithRetry() ([]compute.VirtualMachine, // GetIPForMachineWithRetry invokes az.getIPForMachine with exponential backoff retry func (az *Cloud) GetIPForMachineWithRetry(name types.NodeName) (string, error) { var ip string - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error ip, retryErr = az.getIPForMachine(name) if retryErr != nil { @@ -145,7 +146,7 @@ func (az *Cloud) GetIPForMachineWithRetry(name types.NodeName) (string, error) { // CreateOrUpdateSGWithRetry invokes az.SecurityGroupsClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateSGWithRetry(sg network.SecurityGroup) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%s): start", *sg.Name) respChan, errChan := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *sg.Name, sg, nil) @@ -158,7 +159,7 @@ func (az *Cloud) CreateOrUpdateSGWithRetry(sg network.SecurityGroup) error { // CreateOrUpdateLBWithRetry invokes az.LoadBalancerClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateLBWithRetry(lb network.LoadBalancer) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("LoadBalancerClient.CreateOrUpdate(%s): start", *lb.Name) respChan, errChan := az.LoadBalancerClient.CreateOrUpdate(az.ResourceGroup, *lb.Name, lb, nil) @@ -174,7 +175,7 @@ func (az *Cloud) ListLBWithRetry() ([]network.LoadBalancer, error) { allLBs := []network.LoadBalancer{} var result network.LoadBalancerListResult - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("LoadBalancerClient.List(%v): start", az.ResourceGroup) @@ -200,7 +201,7 @@ func (az *Cloud) ListLBWithRetry() ([]network.LoadBalancer, error) { // follow the next link to get all the vms for resource group if result.NextLink != nil { - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("LoadBalancerClient.ListNextResults(%v): start", az.ResourceGroup) @@ -229,7 +230,7 @@ func (az *Cloud) ListLBWithRetry() ([]network.LoadBalancer, error) { func (az *Cloud) ListPIPWithRetry() ([]network.PublicIPAddress, error) { allPIPs := []network.PublicIPAddress{} var result network.PublicIPAddressListResult - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("PublicIPAddressesClient.List(%v): start", az.ResourceGroup) @@ -255,7 +256,7 @@ func (az *Cloud) ListPIPWithRetry() ([]network.PublicIPAddress, error) { // follow the next link to get all the vms for resource group if result.NextLink != nil { - err := wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + err := wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { var retryErr error az.operationPollRateLimiter.Accept() glog.V(10).Infof("PublicIPAddressesClient.ListNextResults(%v): start", az.ResourceGroup) @@ -282,7 +283,7 @@ func (az *Cloud) ListPIPWithRetry() ([]network.PublicIPAddress, error) { // CreateOrUpdatePIPWithRetry invokes az.PublicIPAddressesClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdatePIPWithRetry(pip network.PublicIPAddress) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("PublicIPAddressesClient.CreateOrUpdate(%s): start", *pip.Name) respChan, errChan := az.PublicIPAddressesClient.CreateOrUpdate(az.ResourceGroup, *pip.Name, pip, nil) @@ -295,7 +296,7 @@ func (az *Cloud) CreateOrUpdatePIPWithRetry(pip network.PublicIPAddress) error { // CreateOrUpdateInterfaceWithRetry invokes az.PublicIPAddressesClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateInterfaceWithRetry(nic network.Interface) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("InterfacesClient.CreateOrUpdate(%s): start", *nic.Name) respChan, errChan := az.InterfacesClient.CreateOrUpdate(az.ResourceGroup, *nic.Name, nic, nil) @@ -308,7 +309,7 @@ func (az *Cloud) CreateOrUpdateInterfaceWithRetry(nic network.Interface) error { // DeletePublicIPWithRetry invokes az.PublicIPAddressesClient.Delete with exponential backoff retry func (az *Cloud) DeletePublicIPWithRetry(pipName string) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("PublicIPAddressesClient.Delete(%s): start", pipName) respChan, errChan := az.PublicIPAddressesClient.Delete(az.ResourceGroup, pipName, nil) @@ -321,7 +322,7 @@ func (az *Cloud) DeletePublicIPWithRetry(pipName string) error { // DeleteLBWithRetry invokes az.LoadBalancerClient.Delete with exponential backoff retry func (az *Cloud) DeleteLBWithRetry(lbName string) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("LoadBalancerClient.Delete(%s): start", lbName) respChan, errChan := az.LoadBalancerClient.Delete(az.ResourceGroup, lbName, nil) @@ -334,7 +335,7 @@ func (az *Cloud) DeleteLBWithRetry(lbName string) error { // CreateOrUpdateRouteTableWithRetry invokes az.RouteTablesClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateRouteTableWithRetry(routeTable network.RouteTable) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("RouteTablesClient.CreateOrUpdate(%s): start", *routeTable.Name) respChan, errChan := az.RouteTablesClient.CreateOrUpdate(az.ResourceGroup, az.RouteTableName, routeTable, nil) @@ -347,7 +348,7 @@ func (az *Cloud) CreateOrUpdateRouteTableWithRetry(routeTable network.RouteTable // CreateOrUpdateRouteWithRetry invokes az.RoutesClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateRouteWithRetry(route network.Route) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("RoutesClient.CreateOrUpdate(%s): start", *route.Name) respChan, errChan := az.RoutesClient.CreateOrUpdate(az.ResourceGroup, az.RouteTableName, *route.Name, route, nil) @@ -360,7 +361,7 @@ func (az *Cloud) CreateOrUpdateRouteWithRetry(route network.Route) error { // DeleteRouteWithRetry invokes az.RoutesClient.Delete with exponential backoff retry func (az *Cloud) DeleteRouteWithRetry(routeName string) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("RoutesClient.Delete(%s): start", az.RouteTableName) respChan, errChan := az.RoutesClient.Delete(az.ResourceGroup, az.RouteTableName, routeName, nil) @@ -373,7 +374,7 @@ func (az *Cloud) DeleteRouteWithRetry(routeName string) error { // CreateOrUpdateVMWithRetry invokes az.VirtualMachinesClient.CreateOrUpdate with exponential backoff retry func (az *Cloud) CreateOrUpdateVMWithRetry(vmName string, newVM compute.VirtualMachine) error { - return wait.ExponentialBackoff(az.getOrCreateRequestBackoff(), func() (bool, error) { + return wait.ExponentialBackoff(az.requestBackoff(), func() (bool, error) { az.operationPollRateLimiter.Accept() glog.V(10).Infof("VirtualMachinesClient.CreateOrUpdate(%s): start", vmName) respChan, errChan := az.VirtualMachinesClient.CreateOrUpdate(az.ResourceGroup, vmName, newVM, nil) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 9e52f4a5de0..72ad6cfca63 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -18,11 +18,13 @@ package azure import ( "fmt" + "math" "strconv" "strings" "k8s.io/api/core/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" serviceapi "k8s.io/kubernetes/pkg/api/v1/service" "github.com/Azure/azure-sdk-for-go/arm/compute" @@ -41,6 +43,13 @@ const ServiceAnnotationLoadBalancerInternalSubnet = "service.beta.kubernetes.io/ // ServiceAnnotationLoadBalancerMode is the annotation used on the service to specify the // Azure load balancer selection based on availability sets +// There are currently three possible load balancer selection modes : +// 1. Default mode - service has no annotation ("service.beta.kubernetes.io/azure-load-balancer-mode") +// In this case the Loadbalancer of the primary Availability set is selected +// 2. "__auto__" mode - service is annotated with __auto__ value, this when loadbalancer from any availability set +// is selected which has the miinimum rules associated with it. +// 3. "as1,as2" mode - this is when the laod balancer from the specified availability sets is selected that has the +// miinimum rules associated with it. const ServiceAnnotationLoadBalancerMode = "service.beta.kubernetes.io/azure-load-balancer-mode" // ServiceAnnotationLoadBalancerAutoModeValue the annotation used on the service to specify the @@ -146,20 +155,21 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string, var defaultLB *network.LoadBalancer defaultLBName := az.getLoadBalancerName(clusterName, az.Config.PrimaryAvailabilitySetName, isInternal) - lbs, err := az.ListLBWithRetry() + existingLBs, err := az.ListLBWithRetry() if err != nil { return nil, nil, false, err } - if lbs != nil { - for lbx := range lbs { - lb := &(lbs[lbx]) - if strings.EqualFold(*lb.Name, defaultLBName) { - defaultLB = lb + + // check if the service already has a load balancer + if existingLBs != nil { + for _, existingLB := range existingLBs { + if strings.EqualFold(*existingLB.Name, defaultLBName) { + defaultLB = &existingLB } - if isInternalLoadBalancer(lb) != isInternal { + if isInternalLoadBalancer(&existingLB) != isInternal { continue } - status, err = az.getServiceLoadBalancerStatus(service, lb) + status, err = az.getServiceLoadBalancerStatus(service, &existingLB) if err != nil { return nil, nil, false, err } @@ -168,19 +178,22 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string, continue } - return lb, status, true, nil + return &existingLB, status, true, nil } } + // service does not have a load balancer, select one if wantLb { // select new load balancer for service - lb, exists, err = az.selectLoadBalancer(clusterName, service, &lbs, nodes) + selectedLB, exists, err := az.selectLoadBalancer(clusterName, service, &existingLBs, nodes) if err != nil { return nil, nil, false, err } - return lb, nil, exists, err + return selectedLB, nil, exists, err } + + // create a default LB with meta data if not present if defaultLB == nil { defaultLB = &network.LoadBalancer{ Name: &defaultLBName, @@ -192,6 +205,66 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string, return defaultLB, nil, false, nil } +// select load balancer for the service in the cluster +// the selection algorithm selectes the the load balancer with currently has +// the minimum lb rules, there there are multiple LB's with same number of rules +// it selects the first one (sorted based on name) +func (az *Cloud) selectLoadBalancer(clusterName string, service *v1.Service, existingLBs *[]network.LoadBalancer, nodes []*v1.Node) (selectedLB *network.LoadBalancer, existsLb bool, err error) { + isInternal := requiresInternalLoadBalancer(service) + serviceName := getServiceName(service) + glog.V(3).Infof("selectLoadBalancer(%s): isInternal(%s) - start", serviceName, isInternal) + availabilitySetNames, err := az.getLoadBalancerAvailabilitySetNames(service, nodes) + if err != nil { + glog.Errorf("az.selectLoadBalancer: cluster(%s) service(%s) isInternal(%t) - az.getLoadBalancerAvailabilitySetNames failed, err=(%v)", clusterName, serviceName, isInternal, err) + return nil, false, err + } + glog.Infof("selectLoadBalancer: cluster(%s) service(%s) isInternal(%t) - availabilitysetsnames %v", clusterName, serviceName, isInternal, *availabilitySetNames) + mapExistingLBs := map[string]network.LoadBalancer{} + for _, lb := range *existingLBs { + mapExistingLBs[*lb.Name] = lb + } + selectedLBRuleCount := math.MaxInt32 + for _, currASName := range *availabilitySetNames { + currLBName := az.getLoadBalancerName(clusterName, currASName, isInternal) + lb, exists := mapExistingLBs[currLBName] + if !exists { + // select this LB as this is a new LB and will have minimum rules + // create tmp lb struct to hold metadata for the new load-balancer + selectedLB = &network.LoadBalancer{ + Name: &currLBName, + Location: &az.Location, + LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{}, + } + + return selectedLB, false, nil + } + + lbRules := *lb.LoadBalancingRules + currLBRuleCount := 0 + if lbRules != nil { + currLBRuleCount = len(lbRules) + } + if currLBRuleCount < selectedLBRuleCount { + selectedLBRuleCount = currLBRuleCount + selectedLB = &lb + } + } + + if selectedLB == nil { + err = fmt.Errorf("selectLoadBalancer: cluster(%s) service(%s) isInternal(%t) - unable to find load balancer for selected availability sets %v", clusterName, serviceName, isInternal, *availabilitySetNames) + glog.Error(err) + return nil, false, err + } + // validate if the selected LB has not exceeded the MaximumLoadBalancerRuleCount + if az.Config.MaximumLoadBalancerRuleCount != 0 && selectedLBRuleCount >= az.Config.MaximumLoadBalancerRuleCount { + err = fmt.Errorf("selectLoadBalancer: cluster(%s) service(%s) isInternal(%t) - all available load balancers have exceeded maximum rule limit %d, availabilitysetnames (%v)", clusterName, serviceName, isInternal, selectedLBRuleCount, *availabilitySetNames) + glog.Error(err) + return selectedLB, existsLb, err + } + + return selectedLB, existsLb, nil +} + func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.LoadBalancer) (status *v1.LoadBalancerStatus, err error) { if lb == nil { glog.V(10).Infof("getServiceLoadBalancerStatus lb is nil") @@ -1043,15 +1116,26 @@ func subnet(service *v1.Service) *string { return nil } -func getServiceLoadBalancerMode(service *v1.Service) (hasMode bool, isAuto bool, asl []string) { +// getServiceLoadBalancerMode parses the mode value +// if the value is __auto__ it returns isAuto = TRUE +// if anything else it returns the unique availability set names after triming spaces +func getServiceLoadBalancerMode(service *v1.Service) (hasMode bool, isAuto bool, availabilitySetNames []string) { mode, hasMode := service.Annotations[ServiceAnnotationLoadBalancerMode] + mode = strings.TrimSpace(mode) isAuto = strings.EqualFold(mode, ServiceAnnotationLoadBalancerAutoModeValue) if !isAuto { - asTagList := strings.TrimSpace(mode) - // Break up list of "AS1,AS2" - asl = strings.Split(asTagList, ",") + availabilitySetParsedList := strings.Split(mode, ",") + + // Trim the availability set names and remove duplicates + // e.g. {"AS1"," AS2", "AS3", "AS3"} => {"AS1", "AS2", "AS3"} + availabilitySetNameSet := sets.NewString() + for _, v := range availabilitySetParsedList { + availabilitySetNameSet.Insert(strings.TrimSpace(v)) + } + + availabilitySetNames = availabilitySetNameSet.List() } - return hasMode, isAuto, asl + return hasMode, isAuto, availabilitySetNames } diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 8d6343d18f4..73af15642c2 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -19,6 +19,7 @@ package azure import ( "encoding/json" "fmt" + "math" "net/http" "net/http/httptest" "reflect" @@ -161,10 +162,8 @@ func testLoadBalancerServiceAutoModeSelection(t *testing.T, isInternal bool) { t.Errorf("Unexpected error: %s", svcName) } - expectedNumOfLB := index % availabilitySetCount - if index >= availabilitySetCount { - expectedNumOfLB = availabilitySetCount - } + // expected is MIN(index, availabilitySetCount) + expectedNumOfLB := int(math.Min(float64(index), float64(availabilitySetCount))) result, _ := az.LoadBalancerClient.List(az.Config.ResourceGroup) lbCount := len(*result.Value) if lbCount != expectedNumOfLB { @@ -192,6 +191,9 @@ func testLoadBalancerServiceAutoModeSelection(t *testing.T, isInternal bool) { // Validate availability set selection of services across load balancers // based on provided availability sets through service annotation +// The scenario is that there are 4 availability sets in the agent pool but the +// services will be assigned load balancers that are part of the provided availability sets +// specified in service annotation func testLoadBalancerServicesSpecifiedSelection(t *testing.T, isInternal bool) { az := getTestCloud() const vmCount = 8 @@ -201,8 +203,8 @@ func testLoadBalancerServicesSpecifiedSelection(t *testing.T, isInternal bool) { clusterResources := getClusterResources(az, vmCount, availabilitySetCount) getTestSecurityGroup(az) - selectedAvailabilitySetName1 := getASName(az, 1, availabilitySetCount) - selectedAvailabilitySetName2 := getASName(az, 2, availabilitySetCount) + selectedAvailabilitySetName1 := getAvailabilitySetName(az, 1, availabilitySetCount) + selectedAvailabilitySetName2 := getAvailabilitySetName(az, 2, availabilitySetCount) for index := 1; index <= serviceCount; index++ { svcName := fmt.Sprintf("service-%d", index) var svc v1.Service @@ -223,10 +225,8 @@ func testLoadBalancerServicesSpecifiedSelection(t *testing.T, isInternal bool) { t.Errorf("Unexpected error: %s", svcName) } - expectedNumOfLB := index % 2 - if index >= 2 { - expectedNumOfLB = 2 - } + // expected is MIN(index, 2) + expectedNumOfLB := int(math.Min(float64(index), float64(2))) result, _ := az.LoadBalancerClient.List(az.Config.ResourceGroup) lbCount := len(*result.Value) if lbCount != expectedNumOfLB { @@ -263,14 +263,12 @@ func testLoadBalancerMaxRulesServices(t *testing.T, isInternal bool) { t.Errorf("Unexpected error: %s", svcName) } - expectedNumOfLB := index % az.Config.MaximumLoadBalancerRuleCount - if index >= az.Config.MaximumLoadBalancerRuleCount { - expectedNumOfLB = az.Config.MaximumLoadBalancerRuleCount - } + // expected is MIN(index, az.Config.MaximumLoadBalancerRuleCount) + expectedNumOfLBRules := int(math.Min(float64(index), float64(az.Config.MaximumLoadBalancerRuleCount))) result, _ := az.LoadBalancerClient.List(az.Config.ResourceGroup) lbCount := len(*result.Value) - if lbCount != expectedNumOfLB { - t.Errorf("Unexpected number of LB's: Expected (%d) Found (%d)", expectedNumOfLB, lbCount) + if lbCount != expectedNumOfLBRules { + t.Errorf("Unexpected number of LB's: Expected (%d) Found (%d)", expectedNumOfLBRules, lbCount) } } @@ -286,11 +284,15 @@ func testLoadBalancerMaxRulesServices(t *testing.T, isInternal bool) { _, err := az.EnsureLoadBalancer(testClusterName, &svc, clusterResources.nodes) if err == nil { t.Errorf("Expect any new service to fail as max limit in lb has reached") + } else { + expectedErrMessageSubString := "all available load balancers have exceeded maximum rule limit" + if !strings.Contains(err.Error(), expectedErrMessageSubString) { + t.Errorf("Error message returned is not expected, expected sub string=%s, actual error message=%v", expectedErrMessageSubString, err) + } } } -// Validate even distribution of external services across load balances -// based on number of availability sets +// Validate service deletion in lb auto selection mode func testLoadBalancerServiceAutoModeDeleteSelection(t *testing.T, isInternal bool) { az := getTestCloud() const vmCount = 8 @@ -331,10 +333,8 @@ func testLoadBalancerServiceAutoModeDeleteSelection(t *testing.T, isInternal boo setLoadBalancerAutoModeAnnotation(&svc) - expectedNumOfLB := index % availabilitySetCount - if index >= availabilitySetCount { - expectedNumOfLB = availabilitySetCount - } + // expected is MIN(index, availabilitySetCount) + expectedNumOfLB := int(math.Min(float64(index), float64(availabilitySetCount))) result, _ := az.LoadBalancerClient.List(az.Config.ResourceGroup) lbCount := len(*result.Value) if lbCount != expectedNumOfLB { @@ -859,7 +859,7 @@ func getVMName(vmIndex int) string { return getTestResourceName(TestVMResourceBaseName, vmIndex) } -func getASName(az *Cloud, vmIndex int, numAS int) string { +func getAvailabilitySetName(az *Cloud, vmIndex int, numAS int) string { asIndex := vmIndex % numAS if asIndex == 0 { return az.Config.PrimaryAvailabilitySetName @@ -868,8 +868,10 @@ func getASName(az *Cloud, vmIndex int, numAS int) string { return getTestResourceName(TestASResourceBaseName, asIndex) } +// test supporting on 1 nic per vm +// we really dont care about the name of the nic +// just using the vm name for testing purposes func getNICName(vmIndex int) string { - // test supporting on 1 nic per vm return getVMName(vmIndex) } @@ -887,7 +889,7 @@ func getClusterResources(az *Cloud, vmCount int, availabilitySetCount int) (clus clusterResources.availabilitySetNames = []string{} for vmIndex := 0; vmIndex < vmCount; vmIndex++ { vmName := getVMName(vmIndex) - asName := getASName(az, vmIndex, availabilitySetCount) + asName := getAvailabilitySetName(az, vmIndex, availabilitySetCount) clusterResources.availabilitySetNames = append(clusterResources.availabilitySetNames, asName) nicName := getNICName(vmIndex) diff --git a/pkg/cloudprovider/providers/azure/azure_util.go b/pkg/cloudprovider/providers/azure/azure_util.go index cdacf7568d4..04ff821e76a 100644 --- a/pkg/cloudprovider/providers/azure/azure_util.go +++ b/pkg/cloudprovider/providers/azure/azure_util.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "hash/crc32" - "math" "regexp" "sort" "strconv" @@ -134,73 +133,12 @@ func (az *Cloud) getpublicIPAddressID(pipName string) string { pipName) } -// select load balancer for the service in the cluster -// the selection algorithm selectes the the load balancer with currently has -// the minimum lb rules, there there are multiple LB's with same number of rules -// it selects the first one (sorted based on name) -func (az *Cloud) selectLoadBalancer(clusterName string, service *v1.Service, existingLBs *[]network.LoadBalancer, nodes []*v1.Node) (selectedLB *network.LoadBalancer, existsLb bool, err error) { - isInternal := requiresInternalLoadBalancer(service) - serviceName := getServiceName(service) - glog.V(3).Infof("selectLoadBalancer(%s): isInternal(%s) - start", serviceName, isInternal) - availabilitySetNames, err := az.getLoadBalancerAvailabilitySetNames(service, nodes) - if err != nil { - glog.Errorf("az.selectLoadBalancer: cluster (%s) service(%s) - az.getLoadBalancerAvailabilitySetNames failed, err=(%v)", clusterName, serviceName, err) - return nil, false, err - } - glog.Infof("selectLoadBalancer(%s): isInternal(%s) - availabilitysetsname %v", serviceName, isInternal, *availabilitySetNames) - mapExistingLBs := map[string]*network.LoadBalancer{} - for lbx := range *existingLBs { - lb := (*existingLBs)[lbx] - mapExistingLBs[*lb.Name] = &lb - } - selectedLBRuleCount := math.MaxInt32 - for asx := range *availabilitySetNames { - currASName := (*availabilitySetNames)[asx] - currLBName := az.getLoadBalancerName(clusterName, currASName, isInternal) - lb, ok := mapExistingLBs[currLBName] - if !ok { - // select this LB as this is a new LB and will have minimum rules - // create tmp lb struct to hold metadata for the new load-balancer - selectedLB = &network.LoadBalancer{ - Name: &currLBName, - Location: &az.Location, - LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{}, - } - - return selectedLB, false, nil - } - - lbRules := *lb.LoadBalancingRules - currLBRuleCount := 0 - if lbRules != nil { - currLBRuleCount = len(lbRules) - } - if currLBRuleCount < selectedLBRuleCount { - selectedLBRuleCount = currLBRuleCount - selectedLB = lb - } - } - - if selectedLB == nil { - glog.Errorf("selectLoadBalancer service (%s) - unable to find load balancer for selected availability sets %v", serviceName, *availabilitySetNames) - return nil, false, fmt.Errorf("selectLoadBalancer (%s)- unable to find load balancer for selected availability sets %v", serviceName, *availabilitySetNames) - } - // validate if the selected LB has not exceeded the MaximumLoadBalancerRuleCount - if az.Config.MaximumLoadBalancerRuleCount != 0 && selectedLBRuleCount >= az.Config.MaximumLoadBalancerRuleCount { - err = fmt.Errorf("selectLoadBalancer service (%s) - all available load balancers have exceeded maximum rule limit %d", serviceName, selectedLBRuleCount) - glog.Error(err) - return selectedLB, existsLb, err - } - - return selectedLB, existsLb, nil -} - // getLoadBalancerAvailabilitySetNames selects all possible availability sets for // service load balancer, if the service has no loadbalancer mode annotaion returns the // primary availability set if service annotation for loadbalancer availability set // exists then return the eligible a availability set func (az *Cloud) getLoadBalancerAvailabilitySetNames(service *v1.Service, nodes []*v1.Node) (availabilitySetNames *[]string, err error) { - hasMode, isAuto, serviceASL := getServiceLoadBalancerMode(service) + hasMode, isAuto, serviceAvailabilitySetNames := getServiceLoadBalancerMode(service) if !hasMode { // no mode specified in service annotation default to PrimaryAvailabilitySetName availabilitySetNames = &[]string{az.Config.PrimaryAvailabilitySetName} @@ -218,25 +156,25 @@ func (az *Cloud) getLoadBalancerAvailabilitySetNames(service *v1.Service, nodes // sort the list to have deterministic selection sort.Strings(*availabilitySetNames) if !isAuto { - if serviceASL == nil || len(serviceASL) == 0 { + if serviceAvailabilitySetNames == nil || len(serviceAvailabilitySetNames) == 0 { return nil, fmt.Errorf("service annotation for LoadBalancerMode is empty, it should have __auto__ or availability sets value") } // validate availability set exists var found bool - for sasx := range serviceASL { + for sasx := range serviceAvailabilitySetNames { for asx := range *availabilitySetNames { - if strings.EqualFold((*availabilitySetNames)[asx], serviceASL[sasx]) { + if strings.EqualFold((*availabilitySetNames)[asx], serviceAvailabilitySetNames[sasx]) { found = true - serviceASL[sasx] = (*availabilitySetNames)[asx] + serviceAvailabilitySetNames[sasx] = (*availabilitySetNames)[asx] break } } if !found { - glog.Errorf("az.getLoadBalancerAvailabilitySetNames - Availability set (%s) in service annotation not found", serviceASL[sasx]) - return nil, fmt.Errorf("availability set (%s) - not found", serviceASL[sasx]) + glog.Errorf("az.getLoadBalancerAvailabilitySetNames - Availability set (%s) in service annotation not found", serviceAvailabilitySetNames[sasx]) + return nil, fmt.Errorf("availability set (%s) - not found", serviceAvailabilitySetNames[sasx]) } } - availabilitySetNames = &serviceASL + availabilitySetNames = &serviceAvailabilitySetNames } return availabilitySetNames, nil @@ -244,7 +182,7 @@ func (az *Cloud) getLoadBalancerAvailabilitySetNames(service *v1.Service, nodes // lists the virtual machines for for the resource group and then builds // a list of availability sets that match the nodes available to k8s -func (az *Cloud) getAgentPoolAvailabiliySets(nodes []*v1.Node) (agentPoolAs *[]string, err error) { +func (az *Cloud) getAgentPoolAvailabiliySets(nodes []*v1.Node) (agentPoolAvailabilitySets *[]string, err error) { vms, err := az.VirtualMachineClientListWithRetry() if err != nil { glog.Errorf("az.getNodeAvailabilitySet - VirtualMachineClientListWithRetry failed, err=%v", err) @@ -258,7 +196,7 @@ func (az *Cloud) getAgentPoolAvailabiliySets(nodes []*v1.Node) (agentPoolAs *[]s } } availabilitySetIDs := sets.NewString() - agentPoolAs = &[]string{} + agentPoolAvailabilitySets = &[]string{} for nx := range nodes { nodeName := (*nodes[nx]).Name if isMasterNode(nodes[nx]) { @@ -282,10 +220,10 @@ func (az *Cloud) getAgentPoolAvailabiliySets(nodes []*v1.Node) (agentPoolAs *[]s // We want to keep it lower case, before the ID get fixed asName = strings.ToLower(asName) - *agentPoolAs = append(*agentPoolAs, asName) + *agentPoolAvailabilitySets = append(*agentPoolAvailabilitySets, asName) } - return agentPoolAs, nil + return agentPoolAvailabilitySets, nil } func (az *Cloud) mapLoadBalancerNameToAvailabilitySet(lbName string, clusterName string) (availabilitySetName string) {