Issue #1609 - Handling Cluster AutoScaler Taint to determine the healthiness of node #1688
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@kishorj - Can you look at the PR and let me know if this looks fine? I needed to add IsNodeSuitableForTraffic function in main branch which was already present in master branch. |
pkg/k8s/node_utils.go
Outdated
| // 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 IsNodeSuitableForTraffic(node *corev1.Node) bool { |
There was a problem hiding this comment.
nit, the comment of function is "IsNodeSuitableAsTrafficProxy", maybe change this function to be "IsNodeSuitableAsTrafficProxy".
pkg/k8s/node_utils.go
Outdated
| @@ -6,12 +6,34 @@ import ( | |||
| "strings" | |||
| ) | |||
|
|
|||
| const ( | |||
| toBeDeletedTaint = "ToBeDeletedByClusterAutoscaler" | |||
There was a problem hiding this comment.
nit, maybe change to toBeDeletedByCATaint.
k8s-ci-robot
added
the
ok-to-test
|
Changes done as per comments. |
Codecov Report
@@ Coverage Diff @@
## main #1688 +/- ##
==========================================
+ Coverage 46.34% 46.76% +0.42%
==========================================
Files 110 110
Lines 5957 5989 +32
==========================================
+ Hits 2761 2801 +40
+ Misses 2930 2923 -7
+ Partials 266 265 -1
Continue to review full report at Codecov.
|
pkg/k8s/node_utils.go
Outdated
| // 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 { | ||
| if node.Spec.Unschedulable { |
There was a problem hiding this comment.
we shouldn't check for Unschedulable anymore.(it's removed from latest k8s of whether it should be a criteria for LB node pool)
Added a commit to this PR to remove it :D
k8s-ci-robot
added
the
lgtm
M00nF1sh
removed
the
lgtm
k8s-ci-robot
added
size/M
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
…checking if node is suitable to handle traffic.
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: atulaggarwal, M00nF1sh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…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 <yyyng@amazon.com>
Issue #1609 - Checking if the current node is tainted with ClusterAutoScaler and avoid marking that node as healthy so that it could be removed from load balancer target groups. This PR also adds node.Spec.UnSchedulable check as present in master branch too.