Bug 2040715: UPSTREAM: 108284: fix: exclude non-ready nodes from azure load balancer

staging/src/k8s.io/legacy-cloud-providers/azure/azure.go

@@ -793,7 +793,7 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
 			}
 		}
 
-		//Remove from nodeZones cache if using depreciated LabelFailureDomainBetaZone
+		// Remove from nodeZones cache if using deprecated LabelFailureDomainBetaZone
 		prevZoneFailureDomain, ok := prevNode.ObjectMeta.Labels[v1.LabelFailureDomainBetaZone]
 		if ok && az.isAvailabilityZone(prevZoneFailureDomain) {
 			az.nodeZones[prevZone].Delete(prevNode.ObjectMeta.Name)
@@ -808,16 +808,17 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
 			delete(az.nodeResourceGroups, prevNode.ObjectMeta.Name)
 		}
 
-		// Remove from unmanagedNodes cache.
 		managed, ok := prevNode.ObjectMeta.Labels[managedByAzureLabel]
-		if ok && managed == "false" {
+		isNodeManagedByCloudProvider := !ok || managed != "false"
+
+		// Remove unmanagedNodes cache
+		if !isNodeManagedByCloudProvider {
 			az.unmanagedNodes.Delete(prevNode.ObjectMeta.Name)
-			az.excludeLoadBalancerNodes.Delete(prevNode.ObjectMeta.Name)
 		}
 
-		// Remove from excludeLoadBalancerNodes cache.
-		if _, hasExcludeBalancerLabel := prevNode.ObjectMeta.Labels[v1.LabelNodeExcludeBalancers]; hasExcludeBalancerLabel {
-			az.excludeLoadBalancerNodes.Delete(prevNode.ObjectMeta.Name)
+		if newNode == nil {
+			// the node is being deleted from the cluster, exclude it from load balancers
+			az.excludeLoadBalancerNodes.Insert(prevNode.ObjectMeta.Name)
 		}
 	}
 
@@ -840,16 +841,26 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
 			az.nodeResourceGroups[newNode.ObjectMeta.Name] = strings.ToLower(newRG)
 		}
 
-		// Add to unmanagedNodes cache.
+		_, hasExcludeBalancerLabel := newNode.ObjectMeta.Labels[v1.LabelNodeExcludeBalancers]
 		managed, ok := newNode.ObjectMeta.Labels[managedByAzureLabel]
-		if ok && managed == "false" {
+		isNodeManagedByCloudProvider := !ok || managed != "false"
+
+		// update unmanagedNodes cache
+		if !isNodeManagedByCloudProvider {
 			az.unmanagedNodes.Insert(newNode.ObjectMeta.Name)
-			az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
 		}
-
-		// Add to excludeLoadBalancerNodes cache.
-		if _, hasExcludeBalancerLabel := newNode.ObjectMeta.Labels[v1.LabelNodeExcludeBalancers]; hasExcludeBalancerLabel {
+		// update excludeLoadBalancerNodes cache
+		switch {
+		case !isNodeManagedByCloudProvider:
+			az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
+		case hasExcludeBalancerLabel:
 			az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
+		case !isNodeReady(newNode):
+			az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
+		default:
+			// Nodes not falling into the three cases above are valid backends and
+			// are removed from excludeLoadBalancerNodes cache.
+			az.excludeLoadBalancerNodes.Delete(newNode.ObjectMeta.Name)
 		}
 	}
 }
@@ -975,3 +986,14 @@ func (az *Cloud) ShouldNodeExcludedFromLoadBalancer(nodeName string) (bool, erro
 
 	return az.excludeLoadBalancerNodes.Has(nodeName), nil
 }
+
+// This, along with the few lines that call this function in updateNodeCaches, should be
+// replaced by https://github.com/kubernetes-sigs/cloud-provider-azure/pull/1195 once that merges.
+func isNodeReady(node *v1.Node) bool {
+	for _, cond := range node.Status.Conditions {
+		if cond.Type == v1.NodeReady && cond.Status == v1.ConditionTrue {
+			return true
+		}
+	}
+	return false
+}

staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go

@@ -3248,12 +3248,13 @@ func TestUpdateNodeCaches(t *testing.T) {
 			Name: "prevNode",
 		},
 	}
-
+	// node is deleted from the cluster
 	az.updateNodeCaches(&prevNode, nil)
 	assert.Equal(t, 0, len(az.nodeZones[zone]))
 	assert.Equal(t, 0, len(az.nodeResourceGroups))
 	assert.Equal(t, 0, len(az.unmanagedNodes))
-	assert.Equal(t, 0, len(az.excludeLoadBalancerNodes))
+	// deleted node should be excluded from load balancer
+	assert.Equal(t, 1, len(az.excludeLoadBalancerNodes))
 	assert.Equal(t, 0, len(az.nodeNames))
 
 	newNode := v1.Node{
@@ -3267,12 +3268,12 @@ func TestUpdateNodeCaches(t *testing.T) {
 			Name: "newNode",
 		},
 	}
-
+	// new node is added to the cluster
 	az.updateNodeCaches(nil, &newNode)
 	assert.Equal(t, 1, len(az.nodeZones[zone]))
 	assert.Equal(t, 1, len(az.nodeResourceGroups))
 	assert.Equal(t, 1, len(az.unmanagedNodes))
-	assert.Equal(t, 1, len(az.excludeLoadBalancerNodes))
+	assert.Equal(t, 2, len(az.excludeLoadBalancerNodes))
 	assert.Equal(t, 1, len(az.nodeNames))
 }
 
@@ -3310,6 +3311,48 @@ func TestUpdateNodeCacheExcludeLoadBalancer(t *testing.T) {
 	az.updateNodeCaches(&prevNode, &newNode)
 	assert.Equal(t, 1, len(az.excludeLoadBalancerNodes))
 
+	// non-ready node should be excluded
+	az.unmanagedNodes = sets.NewString()
+	az.excludeLoadBalancerNodes = sets.NewString()
+	az.nodeNames = sets.NewString()
+	nonReadyNode := v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels: map[string]string{
+				v1.LabelTopologyZone: zone,
+			},
+			Name: "aNode",
+		},
+		Status: v1.NodeStatus{
+			Conditions: []v1.NodeCondition{
+				{
+					Type:   v1.NodeReady,
+					Status: v1.ConditionFalse,
+				},
+			},
+		},
+	}
+	az.updateNodeCaches(nil, &nonReadyNode)
+	assert.Equal(t, 1, len(az.excludeLoadBalancerNodes))
+	// node becomes ready
+	readyNode := v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels: map[string]string{
+				v1.LabelTopologyZone: zone,
+			},
+			Name: "aNode",
+		},
+		Status: v1.NodeStatus{
+			Conditions: []v1.NodeCondition{
+				{
+					Type:   v1.NodeReady,
+					Status: v1.ConditionTrue,
+				},
+			},
+		},
+	}
+	az.updateNodeCaches(&nonReadyNode, &readyNode)
+	assert.Equal(t, 0, len(az.excludeLoadBalancerNodes))
+
 }
 
 func TestGetActiveZones(t *testing.T) {