diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure.go index 9c5a108cca88c..6790cfb82cde0 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure.go +++ b/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 +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go index 189f4ae046363..ce91ff658077c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go +++ b/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) {