Skip to content

Commit 6877b07

Browse files
nilo19nilo19
authored
fix: Empty node selector should work after non-empty node selector (#8327)
1 parent e253678 commit 6877b07

File tree

4 files changed

+122
-11
lines changed

4 files changed

+122
-11
lines changed

‎pkg/provider/azure_loadbalancer.go‎

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2065,6 +2065,7 @@ func (az *Cloud) removeNodeFromLBConfig(nodeNameToLBConfigIDXMap map[string]int,
20652065
// removeDeletedNodesFromLoadBalancerConfigurations removes the deleted nodes
20662066
// that do not exist in nodes list from the load balancer configurations
20672067
func (az *Cloud) removeDeletedNodesFromLoadBalancerConfigurations(nodes []*v1.Node) map[string]int {
2068+
logger := klog.Background().WithName("removeDeletedNodesFromLoadBalancerConfigurations")
20682069
nodeNamesSet := utilsets.NewString()
20692070
for _, node := range nodes {
20702071
nodeNamesSet.Insert(node.Name)
@@ -2076,12 +2077,14 @@ func (az *Cloud) removeDeletedNodesFromLoadBalancerConfigurations(nodes []*v1.No
20762077
// Remove the nodes from the load balancer configurations if they are not in the node list.
20772078
nodeNameToLBConfigIDXMap := make(map[string]int)
20782079
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
2080+
logger.V(4).Info("checking load balancer configuration", "lb", multiSLBConfig.Name)
20792081
if multiSLBConfig.ActiveNodes != nil {
20802082
for _, nodeName := range multiSLBConfig.ActiveNodes.UnsortedList() {
20812083
if nodeNamesSet.Has(nodeName) {
2084+
logger.V(4).Info("found node in load balancer configuration", "node", nodeName, "lb", multiSLBConfig.Name)
20822085
nodeNameToLBConfigIDXMap[nodeName] = i
20832086
} else {
2084-
klog.V(4).Infof("reconcileMultipleStandardLoadBalancerBackendNodes: node(%s) is gone, remove it from lb(%s)", nodeName, multiSLBConfig.Name)
2087+
logger.V(4).Info("removing node which is not found in input node list from load balancer configuration", "node", nodeName, "lb", multiSLBConfig.Name)
20852088
az.MultipleStandardLoadBalancerConfigurations[i].ActiveNodes.Delete(nodeName)
20862089
}
20872090
}
@@ -2139,35 +2142,39 @@ func (az *Cloud) accommodateNodesByPrimaryVMSet(
21392142

21402143
// accommodateNodesByNodeSelector decides which load balancer configuration the node should be added to by node selector
21412144
func (az *Cloud) accommodateNodesByNodeSelector(
2145+
ctx context.Context,
21422146
lbName string,
21432147
lbs *[]network.LoadBalancer,
21442148
service *v1.Service,
21452149
nodes []*v1.Node,
21462150
nodeNameToLBConfigIDXMap map[string]int,
21472151
) error {
2152+
logger := klog.FromContext(ctx).WithName("accommodateNodesByNodeSelector")
2153+
21482154
for _, node := range nodes {
21492155
// Skip nodes that have been matched with a load balancer
21502156
// by primary vmSet.
21512157
if _, ok := az.nodesWithCorrectLoadBalancerByPrimaryVMSet.Load(strings.ToLower(node.Name)); ok {
21522158
continue
21532159
}
21542160

2161+
logger.V(4).Info("checking node", "node", node.Name)
2162+
21552163
// If the vmSet of the node does not match any load balancer,
21562164
// pick all load balancers whose node selector matches the node.
21572165
var eligibleLBsIDX []int
21582166
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
2159-
if multiSLBConfig.NodeSelector != nil &&
2160-
(len(multiSLBConfig.NodeSelector.MatchLabels) > 0 || len(multiSLBConfig.NodeSelector.MatchExpressions) > 0) {
2167+
if !isEmptyLabelSelector(multiSLBConfig.NodeSelector) {
21612168
nodeSelector, err := metav1.LabelSelectorAsSelector(multiSLBConfig.NodeSelector)
21622169
if err != nil {
2163-
klog.Errorf("accommodateNodesByNodeSelector: failed to parse nodeSelector for lb(%s): %s", multiSLBConfig.Name, err.Error())
2170+
logger.Error(err, "failed to parse nodeSelector", "lb", multiSLBConfig.Name)
21642171
return err
21652172
}
21662173
if nodeSelector.Matches(labels.Set(node.Labels)) {
2167-
klog.V(4).Infof("accommodateNodesByNodeSelector: lb(%s) matches node(%s) labels", multiSLBConfig.Name, node.Name)
2174+
logger.V(4).Info("node matches nodeSelector", "node", node.Name, "lb", multiSLBConfig.Name)
21682175
found := isLBInList(lbs, multiSLBConfig.Name)
21692176
if !found && !strings.EqualFold(trimSuffixIgnoreCase(lbName, consts.InternalLoadBalancerNameSuffix), multiSLBConfig.Name) {
2170-
klog.V(4).Infof("accommodateNodesByNodeSelector: but the lb is not found and will not be created this time, will ignore this load balancer")
2177+
logger.V(4).Info("but the lb is not found and will not be created this time, will ignore this load balancer", "lb", multiSLBConfig.Name)
21712178
continue
21722179
}
21732180
eligibleLBsIDX = append(eligibleLBsIDX, i)
@@ -2177,7 +2184,8 @@ func (az *Cloud) accommodateNodesByNodeSelector(
21772184
// If no load balancer is matched, all load balancers without node selector are eligible.
21782185
if len(eligibleLBsIDX) == 0 {
21792186
for i, multiSLBConfig := range az.MultipleStandardLoadBalancerConfigurations {
2180-
if multiSLBConfig.NodeSelector == nil {
2187+
logger.V(4).Info("checking the node selector of the lb", "lb", multiSLBConfig.Name, "nodeSelector", multiSLBConfig.NodeSelector)
2188+
if isEmptyLabelSelector(multiSLBConfig.NodeSelector) {
21812189
eligibleLBsIDX = append(eligibleLBsIDX, i)
21822190
}
21832191
}
@@ -2188,28 +2196,35 @@ func (az *Cloud) accommodateNodesByNodeSelector(
21882196
multiSLBConfig := az.MultipleStandardLoadBalancerConfigurations[eligibleLBsIDX[i]]
21892197
found := isLBInList(lbs, multiSLBConfig.Name)
21902198
if !found && !strings.EqualFold(trimSuffixIgnoreCase(lbName, consts.InternalLoadBalancerNameSuffix), multiSLBConfig.Name) {
2191-
klog.V(4).Infof("accommodateNodesByNodeSelector: the load balancer %s is a valid placement target for node %s, but the lb is not found and will not be created this time, ignore this load balancer", multiSLBConfig.Name, node.Name)
2199+
logger.V(4).Info(
2200+
"the load balancer is a valid placement target for node, but the lb is not found and will not be created this time, ignore this load balancer",
2201+
"lb", multiSLBConfig.Name, "node", node.Name,
2202+
)
21922203
eligibleLBsIDX = append(eligibleLBsIDX[:i], eligibleLBsIDX[i+1:]...)
21932204
}
21942205
}
21952206
if idx, ok := nodeNameToLBConfigIDXMap[node.Name]; ok {
21962207
if IntInSlice(idx, eligibleLBsIDX) {
2197-
klog.V(4).Infof("accommodateNodesByNodeSelector: node(%s) is already on the eligible lb(%s)", node.Name, az.MultipleStandardLoadBalancerConfigurations[idx].Name)
2208+
logger.V(4).Info("node is already on the eligible lb", "node", node.Name, "lb", az.MultipleStandardLoadBalancerConfigurations[idx].Name)
21982209
continue
21992210
}
22002211
}
22012212

2213+
logger.V(4).Info("showing eligible load balancer indices for the node", "node", node.Name, "lbs", eligibleLBsIDX)
2214+
22022215
// Pick one with the fewest nodes among all eligible load balancers.
22032216
minNodesIDX := -1
22042217
minNodes := math.MaxInt32
22052218
az.multipleStandardLoadBalancersActiveNodesLock.Lock()
22062219
for _, idx := range eligibleLBsIDX {
22072220
multiSLBConfig := az.MultipleStandardLoadBalancerConfigurations[idx]
22082221
if multiSLBConfig.ActiveNodes.Len() < minNodes {
2222+
logger.V(4).Info("found an lb with fewer nodes", "lb", multiSLBConfig.Name, "nodes", multiSLBConfig.ActiveNodes.Len())
22092223
minNodes = multiSLBConfig.ActiveNodes.Len()
22102224
minNodesIDX = idx
22112225
}
22122226
}
2227+
logger.V(4).Info("showing the lb with the fewest nodes", "lb index", minNodesIDX, "node count", minNodes)
22132228
az.multipleStandardLoadBalancersActiveNodesLock.Unlock()
22142229

22152230
if idx, ok := nodeNameToLBConfigIDXMap[node.Name]; ok && idx != minNodesIDX {
@@ -2269,7 +2284,8 @@ func (az *Cloud) reconcileMultipleStandardLoadBalancerBackendNodes(
22692284
nodes []*v1.Node,
22702285
init bool,
22712286
) error {
2272-
logger := klog.Background().WithName("reconcileMultipleStandardLoadBalancerBackendNodes").
2287+
logger := klog.FromContext(ctx).
2288+
WithName("reconcileMultipleStandardLoadBalancerBackendNodes").
22732289
WithValues(
22742290
"clusterName", clusterName,
22752291
"lbName", lbName,
@@ -2291,7 +2307,7 @@ func (az *Cloud) reconcileMultipleStandardLoadBalancerBackendNodes(
22912307
return err
22922308
}
22932309

2294-
err = az.accommodateNodesByNodeSelector(lbName, lbs, service, nodes, nodeNameToLBConfigIDXMap)
2310+
err = az.accommodateNodesByNodeSelector(ctx, lbName, lbs, service, nodes, nodeNameToLBConfigIDXMap)
22952311
if err != nil {
22962312
return err
22972313
}

‎pkg/provider/azure_loadbalancer_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8435,6 +8435,87 @@ func TestReconcileMultipleStandardLoadBalancerNodes(t *testing.T) {
84358435
"lb4": utilsets.NewString("node2", "node6"),
84368436
},
84378437
},
8438+
{
8439+
description: "should handle empty node selector",
8440+
existingLBConfigs: []config.MultipleStandardLoadBalancerConfiguration{
8441+
{
8442+
Name: "lb1",
8443+
MultipleStandardLoadBalancerConfigurationSpec: config.MultipleStandardLoadBalancerConfigurationSpec{
8444+
PrimaryVMSet: "vmss-1",
8445+
NodeSelector: &metav1.LabelSelector{},
8446+
},
8447+
MultipleStandardLoadBalancerConfigurationStatus: config.MultipleStandardLoadBalancerConfigurationStatus{
8448+
ActiveNodes: utilsets.NewString("node1"),
8449+
},
8450+
},
8451+
{
8452+
Name: "lb2",
8453+
MultipleStandardLoadBalancerConfigurationSpec: config.MultipleStandardLoadBalancerConfigurationSpec{
8454+
PrimaryVMSet: "vmss-2",
8455+
},
8456+
MultipleStandardLoadBalancerConfigurationStatus: config.MultipleStandardLoadBalancerConfigurationStatus{
8457+
ActiveNodes: utilsets.NewString("node2", "node3"),
8458+
},
8459+
},
8460+
},
8461+
existingNodes: []*v1.Node{
8462+
getTestNodeWithMetadata("node1", "vmss-1", nil, "10.1.0.1"),
8463+
getTestNodeWithMetadata("node2", "vmss-2", nil, "10.1.0.2"),
8464+
getTestNodeWithMetadata("node3", "vmss-2", nil, "10.1.0.3"),
8465+
},
8466+
existingLBs: []network.LoadBalancer{
8467+
{
8468+
Name: ptr.To("lb1"),
8469+
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
8470+
BackendAddressPools: &[]network.BackendAddressPool{
8471+
{
8472+
Name: ptr.To("kubernetes"),
8473+
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
8474+
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
8475+
{
8476+
Name: ptr.To("node1"),
8477+
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
8478+
IPAddress: ptr.To("10.1.0.1"),
8479+
},
8480+
},
8481+
},
8482+
},
8483+
},
8484+
},
8485+
},
8486+
},
8487+
{
8488+
Name: ptr.To("lb2"),
8489+
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
8490+
BackendAddressPools: &[]network.BackendAddressPool{
8491+
{
8492+
Name: ptr.To("kubernetes"),
8493+
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
8494+
LoadBalancerBackendAddresses: &[]network.LoadBalancerBackendAddress{
8495+
{
8496+
Name: ptr.To("node2"),
8497+
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
8498+
IPAddress: ptr.To("10.1.0.2"),
8499+
},
8500+
},
8501+
{
8502+
Name: ptr.To("node3"),
8503+
LoadBalancerBackendAddressPropertiesFormat: &network.LoadBalancerBackendAddressPropertiesFormat{
8504+
IPAddress: ptr.To("10.1.0.3"),
8505+
},
8506+
},
8507+
},
8508+
},
8509+
},
8510+
},
8511+
},
8512+
},
8513+
},
8514+
expectedLBToNodesMap: map[string]*utilsets.IgnoreCaseSet{
8515+
"lb1": utilsets.NewString("node1"),
8516+
"lb2": utilsets.NewString("node2", "node3"),
8517+
},
8518+
},
84388519
{
84398520
description: "should remove the node on the lb if it is no longer eligible",
84408521
existingLBConfigs: []config.MultipleStandardLoadBalancerConfiguration{

‎pkg/provider/azure_utils.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2022-07-01/network"
3131

3232
v1 "k8s.io/api/core/v1"
33+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3334
"k8s.io/klog/v2"
3435
utilnet "k8s.io/utils/net"
3536
"k8s.io/utils/ptr"
@@ -568,3 +569,10 @@ func ToArmcomputeDisk(disks []compute.DataDisk) ([]*armcompute.DataDisk, error)
568569

569570
return result, nil
570571
}
572+
573+
func isEmptyLabelSelector(selector *metav1.LabelSelector) bool {
574+
if selector == nil {
575+
return true
576+
}
577+
return len(selector.MatchLabels) == 0 && len(selector.MatchExpressions) == 0
578+
}

‎pkg/provider/config/multi_slb.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,20 @@ type MultipleStandardLoadBalancerConfigurationSpec struct {
5151

5252
// Services that must match this selector can be placed on this load balancer. If not supplied,
5353
// services with any labels can be created on the load balancer.
54+
// A ServiceLabelSelector with empty matchLabels and matchExpressions will match all services, but
55+
// only works if no non-empty ServiceLabelSelector has matched the service.
5456
ServiceLabelSelector *metav1.LabelSelector `json:"serviceLabelSelector" yaml:"serviceLabelSelector"`
5557

5658
// Services created in namespaces with the supplied label will be allowed to select that load balancer.
5759
// If not supplied, services created in any namespaces can be created on that load balancer.
60+
// A ServiceNamespaceSelector with empty matchLabels and matchExpressions will match all nodes, but
61+
// only works if no non-empty ServiceNamespaceSelector has matched the service.
5862
ServiceNamespaceSelector *metav1.LabelSelector `json:"serviceNamespaceSelector" yaml:"serviceNamespaceSelector"`
5963

6064
// Nodes matching this selector will be preferentially added to the load balancers that
6165
// they match selectors for. NodeSelector does not override primaryAgentPool for node allocation.
66+
// A NodeSelector with empty matchLabels and matchExpressions will match all nodes, but
67+
// only works if no non-empty NodeSelector has matched the node.
6268
NodeSelector *metav1.LabelSelector `json:"nodeSelector" yaml:"nodeSelector"`
6369
}
6470

0 commit comments

Comments
 (0)