Issue kubernetes-sigs#1609 - Handling Cluster AutoScaler Taint to det…

…ermine the healthiness of node (kubernetes-sigs#1688) * Issue kubernetes-sigs#1609. Checking for taint ToBeDeletedByClusterAutoscaler while checking if node is suitable to handle traffic. * Changes as per the PR comments. * remove unschedulable from LB node pool critera Co-authored-by: M00nF1sh <[email protected]>
adammw · Nov 9, 2023 · ceaa524 · ceaa524
1 parent ffb3a74
commit ceaa524
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 3 deletions.
diff --git a/controllers/elbv2/eventhandlers/node.go b/controllers/elbv2/eventhandlers/node.go
@@ -61,11 +61,11 @@ func (h *enqueueRequestsForNodeEvent) enqueueImpactedTargetGroupBindings(queue w
 	nodeNewIsReady := false
 	if nodeOld != nil {
 		nodeKey = k8s.NamespacedName(nodeOld)
-		nodeOldIsReady = k8s.IsNodeReady(nodeOld)
+		nodeOldIsReady = k8s.IsNodeSuitableAsTrafficProxy(nodeOld)
 	}
 	if nodeNew != nil {
 		nodeKey = k8s.NamespacedName(nodeNew)
-		nodeNewIsReady = k8s.IsNodeReady(nodeNew)
+		nodeNewIsReady = k8s.IsNodeSuitableAsTrafficProxy(nodeNew)
 	}
 
 	tgbList := &elbv2api.TargetGroupBindingList{}

diff --git a/pkg/backend/endpoint_resolver.go b/pkg/backend/endpoint_resolver.go
@@ -133,7 +133,7 @@ func (r *defaultEndpointResolver) ResolveNodePortEndpoints(ctx context.Context,
 	var endpoints []NodePortEndpoint
 	for i := range nodeList.Items {
 		node := &nodeList.Items[i]
-		if !k8s.IsNodeReady(node) {
+		if !k8s.IsNodeSuitableAsTrafficProxy(node) {
 			continue
 		}
 		instanceID, err := k8s.ExtractNodeInstanceID(node)

diff --git a/pkg/k8s/node_utils.go b/pkg/k8s/node_utils.go
@@ -6,12 +6,30 @@ import (
 	"strings"
 )
 
+const (
+	toBeDeletedByCATaint = "ToBeDeletedByClusterAutoscaler"
+)
+
 // IsNodeReady returns whether node is ready.
 func IsNodeReady(node *corev1.Node) bool {
 	nodeReadyCond := GetNodeCondition(node, corev1.NodeReady)
 	return nodeReadyCond != nil && nodeReadyCond.Status == corev1.ConditionTrue
 }
 
+// IsNodeSuitableAsTrafficProxy check whether node is suitable as a traffic proxy.
+// mimic the logic of serviceController: https://github.com/kubernetes/kubernetes/blob/b6b494b4484b51df8dc6b692fab234573da30ab4/pkg/controller/service/controller.go#L605
+func IsNodeSuitableAsTrafficProxy(node *corev1.Node) bool {
+	// ToBeDeletedByClusterAutoscaler taint is added by cluster autoscaler before removing node from cluster
+	// Marking the node as unsuitable for traffic once the taint is observed on the node
+	for _, taint := range node.Spec.Taints {
+		if taint.Key == toBeDeletedByCATaint {
+			return false
+		}
+	}
+
+	return IsNodeReady(node)
+}
+
 // GetNodeCondition will get pointer to Node's existing condition.
 // returns nil if no matching condition found.
 func GetNodeCondition(node *corev1.Node, conditionType corev1.NodeConditionType) *corev1.NodeCondition {

diff --git a/pkg/k8s/node_utils_test.go b/pkg/k8s/node_utils_test.go
@@ -69,6 +69,68 @@ func TestIsNodeReady(t *testing.T) {
 	}
 }
 
+func TestIsNodeSuitableForTraffic(t *testing.T) {
+	type args struct {
+		node *corev1.Node
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "node is ready and suitable for traffic",
+			args: args{
+				node: &corev1.Node{
+					Status: corev1.NodeStatus{
+						Conditions: []corev1.NodeCondition{
+							{
+								Type:   corev1.NodeReady,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+					Spec: corev1.NodeSpec{
+						Unschedulable: false,
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "node is ready but tainted with ToBeDeletedByClusterAutoscaler",
+			args: args{
+				node: &corev1.Node{
+					Status: corev1.NodeStatus{
+						Conditions: []corev1.NodeCondition{
+							{
+								Type:   corev1.NodeReady,
+								Status: corev1.ConditionTrue,
+							},
+						},
+					},
+					Spec: corev1.NodeSpec{
+						Unschedulable: false,
+						Taints: []corev1.Taint{
+							{
+								Key:   toBeDeletedByCATaint,
+								Value: "True",
+							},
+						},
+					},
+				},
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := IsNodeSuitableAsTrafficProxy(tt.args.node)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestGetNodeCondition(t *testing.T) {
 	type args struct {
 		node          *corev1.Node