Skip to content

Commit

Permalink
chore: add locks and comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@nilo19
nilo19 committed Jul 5, 2023
1 parent 6509c7e commit 88a7901
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 35 deletions.
13 changes: 11 additions & 2 deletions pkg/provider/azure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,11 @@ type MultipleStandardLoadBalancerConfigurationSpec struct {

// MultipleStandardLoadBalancerConfigurationStatus stores the properties regarding multiple standard load balancers.
type MultipleStandardLoadBalancerConfigurationStatus struct {
// ActiveServices stores the services that are supposed to use the load balancer.
ActiveServices sets.Set[string] `json:"activeServices" yaml:"activeServices"`

// ActiveNodes stores the nodes that are supposed to be in the load balancer.
// It will be used in EnsureHostsInPool to make sure the given ones are in the backend pool.
ActiveNodes sets.Set[string] `json:"activeNodes" yaml:"activeNodes"`
}

Expand Down Expand Up @@ -428,7 +431,10 @@ type Cloud struct {
// runs only once every time the cloud provide restarts.
multipleStandardLoadBalancerConfigurationsSynced bool
// nodesWithCorrectLoadBalancerByPrimaryVMSet marks nodes that are matched with load balancers by primary vmSet.
nodesWithCorrectLoadBalancerByPrimaryVMSet sets.Set[string]
nodesWithCorrectLoadBalancerByPrimaryVMSet sync.Map

multipleStandardLoadBalancersActiveServicesLock sync.Mutex
multipleStandardLoadBalancersActiveNodesLock sync.Mutex
}

// NewCloud returns a Cloud with initialized clients
Expand Down Expand Up @@ -1215,7 +1221,7 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
// if the node is being deleted from the cluster, exclude it from load balancers
if newNode == nil {
az.excludeLoadBalancerNodes.Insert(prevNode.ObjectMeta.Name)
az.nodesWithCorrectLoadBalancerByPrimaryVMSet, _ = safeRemoveKeyFromStringsSet(az.nodesWithCorrectLoadBalancerByPrimaryVMSet, prevNode.ObjectMeta.Name)
az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Delete(strings.ToLower(prevNode.ObjectMeta.Name))
}

// Remove from nodePrivateIPs cache.
Expand Down Expand Up @@ -1424,6 +1430,9 @@ func isNodeReady(node *v1.Node) bool {
}

func (az *Cloud) getActiveNodesByLoadBalancerName(lbName string) sets.Set[string] {
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
defer az.multipleStandardLoadBalancersActiveNodesLock.Unlock()

for _, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
if strings.EqualFold(strings.TrimSuffix(lbName, consts.InternalLoadBalancerNameSuffix), multiSLBConfig.Name) {
return multiSLBConfig.ActiveNodes
Expand Down
81 changes: 50 additions & 31 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ func (az *Cloud) shouldChangeLoadBalancer(service *v1.Service, currLBName, clust
return true
}

// removeFrontendIPConfigurationFromLoadBalancer removes the given ip configs from the load balancer
// and delete the load balancer if there is no ip config on it. It returns the name of the deleted load balancer
// and it will be used in reconcileLoadBalancer to remove the load balancer from the list.
func (az *Cloud) removeFrontendIPConfigurationFromLoadBalancer(lb *network.LoadBalancer, existingLBs []network.LoadBalancer, fips []*network.FrontendIPConfiguration, clusterName string, service *v1.Service) (string, error) {
if lb == nil || lb.LoadBalancerPropertiesFormat == nil || lb.FrontendIPConfigurations == nil {
return "", nil
Expand Down Expand Up @@ -540,14 +543,14 @@ func (az *Cloud) safeDeleteLoadBalancer(lb network.LoadBalancer, clusterName, vm
_ = az.lbCache.Delete(pointer.StringDeref(lb.Name, ""))

// Remove corresponding nodes in ActiveNodes and nodesWithCorrectLoadBalancerByPrimaryVMSet.
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
for i := range az.MultipleStandardLoadBalancerConfigurations {
if strings.EqualFold(
strings.TrimSuffix(pointer.StringDeref(lb.Name, ""), consts.InternalLoadBalancerNameSuffix),
multiSLBConfig.Name,
az.MultipleStandardLoadBalancerConfigurations[i].Name,
) {
if multiSLBConfig.ActiveNodes != nil {
for nodeName := range multiSLBConfig.ActiveNodes {
safeRemoveKeyFromStringsSet(az.nodesWithCorrectLoadBalancerByPrimaryVMSet, nodeName)
if az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes != nil {
for nodeName := range az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes {
az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Delete(strings.ToLower(nodeName))
}
}
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes = sets.New[string]()
Expand Down Expand Up @@ -1535,11 +1538,13 @@ func (az *Cloud) reconcileMultipleStandardLoadBalancerConfigurations(
)
for i := range az.MultipleStandardLoadBalancerConfigurations {
if strings.EqualFold(strings.TrimSuffix(lbName, consts.InternalLoadBalancerNameSuffix), az.MultipleStandardLoadBalancerConfigurations[i].Name) {
az.multipleStandardLoadBalancersActiveServicesLock.Lock()
if az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices == nil {
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices = sets.New[string]()
}
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Insert(strings.ToLower(svcName))
az.multipleStandardLoadBalancersActiveServicesLock.Unlock()
klog.V(2).Infof("reconcileMultipleStandardLoadBalancerConfigurations: service(%s) is active on lb(%s)", svcName, lbName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Insert(svcName)
}
}
}
Expand Down Expand Up @@ -1787,6 +1792,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
return lb, nil
}

// removeLBFromList removes the given lb from the list
func removeLBFromList(lbs *[]network.LoadBalancer, lbName string) {
if lbs != nil {
for i := len(*lbs) - 1; i >= 0; i-- {
Expand All @@ -1798,29 +1804,28 @@ func removeLBFromList(lbs *[]network.LoadBalancer, lbName string) {
}
}

func (az *Cloud) removeNodeFromLBConfig(nodeNameToLBConfigIDXMap map[string]int, nodeName string) string {
var (
lbConfigName string
changed bool
)
// removeNodeFromLBConfig searches for the occurrence of the given node in the lb configs and removes it
func (az *Cloud) removeNodeFromLBConfig(nodeNameToLBConfigIDXMap map[string]int, nodeName string) {
if idx, ok := nodeNameToLBConfigIDXMap[nodeName]; ok {
currentLBConfigName := az.MultipleStandardLoadBalancerConfigurations[idx].Name
klog.V(4).Infof("reconcileMultipleStandardLoadBalancerBackendNodes: remove node(%s) on lb(%s)", nodeName, currentLBConfigName)
az.MultipleStandardLoadBalancerConfigurations[idx].ActiveNodes, changed = safeRemoveKeyFromStringsSet(az.MultipleStandardLoadBalancerConfigurations[idx].ActiveNodes, nodeName)
if changed {
lbConfigName = currentLBConfigName
}
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
az.MultipleStandardLoadBalancerConfigurations[idx].ActiveNodes.Delete(strings.ToLower(nodeName))
az.multipleStandardLoadBalancersActiveNodesLock.Unlock()
}

return lbConfigName
}

// removeDeletedNodesFromLoadBalancerConfigurations removes the deleted nodes
// that do not exist in nodes list from the load balancer configurations
func (az *Cloud) removeDeletedNodesFromLoadBalancerConfigurations(nodes []*v1.Node) map[string]int {
nodeNamesSet := sets.New[string]()
for _, node := range nodes {
nodeNamesSet.Insert(node.Name)
nodeNamesSet.Insert(strings.ToLower(node.Name))
}

az.multipleStandardLoadBalancersActiveNodesLock.Lock()
defer az.multipleStandardLoadBalancersActiveNodesLock.Unlock()

// Remove the nodes from the load balancer configurations if they are not in the node list.
nodeNameToLBConfigIDXMap := make(map[string]int)
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
Expand All @@ -1830,7 +1835,7 @@ func (az *Cloud) removeDeletedNodesFromLoadBalancerConfigurations(nodes []*v1.No
nodeNameToLBConfigIDXMap[nodeName] = i
} else {
klog.V(4).Infof("reconcileMultipleStandardLoadBalancerBackendNodes: node(%s) is gone, remove it from lb(%s)", nodeName, multiSLBConfig.Name)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes, _ = safeRemoveKeyFromStringsSet(az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes, nodeName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes.Delete(strings.ToLower(nodeName))
}
}
}
Expand All @@ -1839,17 +1844,19 @@ func (az *Cloud) removeDeletedNodesFromLoadBalancerConfigurations(nodes []*v1.No
return nodeNameToLBConfigIDXMap
}

// accommodateNodesByPrimaryVMSet decides which load balancer configuration the node should be added to by primary vmSet
func (az *Cloud) accommodateNodesByPrimaryVMSet(
lbName string,
lbs *[]network.LoadBalancer,
nodes []*v1.Node,
nodeNameToLBConfigIDXMap map[string]int,
) error {
for _, node := range nodes {
if az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Has(node.Name) {
if _, ok := az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Load(strings.ToLower(node.Name)); ok {
continue
}

// TODO(niqi): reduce the API calls for VMAS and standalone VMs
vmSetName, err := az.VMSet.GetNodeVMSetName(node)
if err != nil {
klog.Errorf("accommodateNodesByPrimaryVMSet: failed to get vmSetName for node(%s): %s", node.Name, err.Error())
Expand All @@ -1872,13 +1879,15 @@ func (az *Cloud) accommodateNodesByPrimaryVMSet(
continue
}

az.nodesWithCorrectLoadBalancerByPrimaryVMSet = safeAddKeyToStringsSet(az.nodesWithCorrectLoadBalancerByPrimaryVMSet, node.Name)
az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Store(strings.ToLower(node.Name), sets.Empty{})
if !multiSLBConfig.ActiveNodes.Has(node.Name) {
klog.V(4).Infof("accommodateNodesByPrimaryVMSet: node(%s) should be on lb(%s) because of primary vmSet (%s)", node.Name, multiSLBConfig.Name, vmSetName)

az.removeNodeFromLBConfig(nodeNameToLBConfigIDXMap, node.Name)

az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes = safeAddKeyToStringsSet(az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes, node.Name)
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes = safeAddKeyToStringsSet(az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes, strings.ToLower(node.Name))
az.multipleStandardLoadBalancersActiveNodesLock.Unlock()
}
break
}
Expand All @@ -1888,6 +1897,7 @@ func (az *Cloud) accommodateNodesByPrimaryVMSet(
return nil
}

// accommodateNodesByNodeSelector decides which load balancer configuration the node should be added to by node selector
func (az *Cloud) accommodateNodesByNodeSelector(
lbName string,
lbs *[]network.LoadBalancer,
Expand All @@ -1898,16 +1908,16 @@ func (az *Cloud) accommodateNodesByNodeSelector(
for _, node := range nodes {
// Skip nodes that have been matched with a load balancer
// by primary vmSet.
if az.nodesWithCorrectLoadBalancerByPrimaryVMSet != nil &&
az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Has(node.Name) {
if _, ok := az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Load(strings.ToLower(node.Name)); ok {
continue
}

// If the vmSet of the node does not match any load balancer,
// pick all load balancers whose node selector matches the node.
var eligibleLBsIDX []int
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
if multiSLBConfig.NodeSelector != nil {
if multiSLBConfig.NodeSelector != nil &&
(len(multiSLBConfig.NodeSelector.MatchLabels) > 0 || len(multiSLBConfig.NodeSelector.MatchExpressions) > 0) {
nodeSelector, err := metav1.LabelSelectorAsSelector(multiSLBConfig.NodeSelector)
if err != nil {
klog.Errorf("accommodateNodesByNodeSelector: failed to parse nodeSelector for lb(%s): %s", multiSLBConfig.Name, err.Error())
Expand Down Expand Up @@ -1950,13 +1960,15 @@ func (az *Cloud) accommodateNodesByNodeSelector(
// Pick one with the fewest nodes among all eligible load balancers.
minNodesIDX := -1
minNodes := math.MaxInt32
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
for _, idx := range eligibleLBsIDX {
multiSLBConfig := az.MultipleStandardLoadBalancerConfigurations[idx]
if multiSLBConfig.ActiveNodes.Len() < minNodes {
minNodes = multiSLBConfig.ActiveNodes.Len()
minNodesIDX = idx
}
}
az.multipleStandardLoadBalancersActiveNodesLock.Unlock()

if idx, ok := nodeNameToLBConfigIDXMap[node.Name]; ok && idx != minNodesIDX {
az.removeNodeFromLBConfig(nodeNameToLBConfigIDXMap, node.Name)
Expand All @@ -1970,7 +1982,9 @@ func (az *Cloud) accommodateNodesByNodeSelector(
}

klog.V(4).Infof("accommodateNodesByNodeSelector: node(%s) should be on lb(%s) it is the eligible LB with fewest number of nodes", node.Name, az.MultipleStandardLoadBalancerConfigurations[minNodesIDX].Name)
az.MultipleStandardLoadBalancerConfigurations[minNodesIDX].ActiveNodes = safeAddKeyToStringsSet(az.MultipleStandardLoadBalancerConfigurations[minNodesIDX].ActiveNodes, node.Name)
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
az.MultipleStandardLoadBalancerConfigurations[minNodesIDX].ActiveNodes = safeAddKeyToStringsSet(az.MultipleStandardLoadBalancerConfigurations[minNodesIDX].ActiveNodes, strings.ToLower(node.Name))
az.multipleStandardLoadBalancersActiveNodesLock.Unlock()
}

return nil
Expand Down Expand Up @@ -2018,17 +2032,19 @@ func (az *Cloud) reconcileMultipleStandardLoadBalancerConfigurationStatus(wantLb
lbName = strings.TrimSuffix(lbName, consts.InternalLoadBalancerNameSuffix)
for i := range az.MultipleStandardLoadBalancerConfigurations {
if strings.EqualFold(lbName, az.MultipleStandardLoadBalancerConfigurations[i].Name) {
az.multipleStandardLoadBalancersActiveServicesLock.Lock()
if az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices == nil {
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices = sets.New[string]()
}

if wantLb {
klog.V(4).Infof("reconcileMultipleStandardLoadBalancerConfigurationStatus: service(%s) is active on lb(%s)", svcName, lbName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Insert(svcName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Insert(strings.ToLower(svcName))
} else {
klog.V(4).Infof("reconcileMultipleStandardLoadBalancerConfigurationStatus: service(%s) is not active on lb(%s) any more", svcName, lbName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Delete(svcName)
az.MultipleStandardLoadBalancerConfigurations[i].ActiveServices.Delete(strings.ToLower(svcName))
}
az.multipleStandardLoadBalancersActiveServicesLock.Unlock()
break
}
}
Expand Down Expand Up @@ -4284,7 +4300,7 @@ func getMostEligibleLBForService(

func (az *Cloud) getServiceCurrentLoadBalancerName(service *v1.Service) string {
for _, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
if isLoadBalancerInUseByService(service, multiSLBConfig) {
if az.isLoadBalancerInUseByService(service, multiSLBConfig) {
return multiSLBConfig.Name
}
}
Expand Down Expand Up @@ -4339,7 +4355,7 @@ func (az *Cloud) getEligibleLoadBalancersForService(service *v1.Service) ([]stri
// 2. If the LB does not allow service placement, it is not eligible,
// unless the service is already using the LB.
if !pointer.BoolDeref(eligibleLB.AllowServicePlacement, true) {
if isLoadBalancerInUseByService(service, eligibleLB) {
if az.isLoadBalancerInUseByService(service, eligibleLB) {
klog.V(4).Infof("getEligibleLoadBalancersForService: although load balancer %q has AllowServicePlacement=false, service %q is allowed to be placed on load balancer %q because it is using the load balancer", eligibleLB.Name, service.Name, eligibleLB.Name)
} else {
klog.V(4).Infof("getEligibleLoadBalancersForService: service %q is not allowed to be placed on load balancer %q", service.Name, eligibleLB.Name)
Expand Down Expand Up @@ -4410,7 +4426,10 @@ func (az *Cloud) getEligibleLoadBalancersForService(service *v1.Service) ([]stri
return eligibleLBNames, nil
}

func isLoadBalancerInUseByService(service *v1.Service, lbConfig MultipleStandardLoadBalancerConfiguration) bool {
func (az *Cloud) isLoadBalancerInUseByService(service *v1.Service, lbConfig MultipleStandardLoadBalancerConfiguration) bool {
az.multipleStandardLoadBalancersActiveServicesLock.Lock()
defer az.multipleStandardLoadBalancersActiveServicesLock.Unlock()

serviceName := getServiceName(service)
if lbConfig.ActiveServices != nil {
return lbConfig.ActiveServices.Has(serviceName)
Expand Down
11 changes: 9 additions & 2 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7287,7 +7287,9 @@ func TestSafeDeleteLoadBalancer(t *testing.T) {
cloud.LoadBalancerClient = mockLBClient
if len(tc.multiSLBConfigs) > 0 {
cloud.MultipleStandardLoadBalancerConfigurations = tc.multiSLBConfigs
cloud.nodesWithCorrectLoadBalancerByPrimaryVMSet = tc.nodesWithCorrectVMSet
for nodeName := range tc.nodesWithCorrectVMSet {
cloud.nodesWithCorrectLoadBalancerByPrimaryVMSet.Store(nodeName, sets.Empty{})
}
}
svc := getTestService("svc", v1.ProtocolTCP, nil, false, 80)
lb := network.LoadBalancer{
Expand All @@ -7300,7 +7302,12 @@ func TestSafeDeleteLoadBalancer(t *testing.T) {
assert.Equal(t, tc.expectedErr, err)
if len(tc.multiSLBConfigs) > 0 {
assert.Equal(t, tc.expectedMultiSLBConfigs, cloud.MultipleStandardLoadBalancerConfigurations)
assert.Equal(t, tc.expectedNodesWithCorrectVMSet, cloud.nodesWithCorrectLoadBalancerByPrimaryVMSet)
actualNodesWithCorrectLoadBalancerByPrimaryVMSet := sets.New[string]()
cloud.nodesWithCorrectLoadBalancerByPrimaryVMSet.Range(func(key, value interface{}) bool {
actualNodesWithCorrectLoadBalancerByPrimaryVMSet.Insert(key.(string))
return true
})
assert.Equal(t, tc.expectedNodesWithCorrectVMSet, actualNodesWithCorrectLoadBalancerByPrimaryVMSet)
}
})
}
Expand Down

0 comments on commit 88a7901

Please sign in to comment.