Ignore unschedulable nodes for eviction decisions

[concaf]

This commit makes descheduler ignore the unschedulable nodes from
being considered for taking eviction decisions

[aveshagarwal]

Can one of the admins verify this patch?

[ravisantoshgudimetla]

@containscafeine You need to fix the test case and also please look at the new flag node-selector #33, through which you can ignore certain nodes based on labels.

Also, there is a plan in upstream community to replace UnSchedulable field with unschedulable taint and in descheduler we plan to have taint based descheduling.

[concaf]

@ravisantoshgudimetla ack, fixed the failing test.

Does it make sense that till the time spec.unschedulable is removed, we use it in descheduler?

[ravisantoshgudimetla]

@containscafeine

Does it make sense that till the time spec.unschedulable is removed, we use it in descheduler?

A readynode in descheduler's context means we have a node on which descheduler could run. Even if the node is unschedulable, it could have some pods running(unless it has been drained) which can cause it to be over highUtilization threshold. In this case, descheduler should run against it.

IOW, descheduler is just a pod evictor. For eviction, we don't need to check if the node is unschedulable.

If you don't want node to be considered by descheduler, you should label the node and ask it to be excluded by descheduler.

[aveshagarwal]

A readynode in descheduler's context means we have a node on which descheduler could run. Even if the node is unschedulable, it could have some pods running(unless it has been drained) which can cause it to be over highUtilization threshold. In this case, descheduler should run against it.

Yes that was the intention to consider unschedulable but ready nodes. However, for LowNodeUtilization strategy, if unschedulable nodes are under utilized, we must ignore them as pods wont be scheduled on them. IOW unschedulable nodes must only be considered for eviction if they are above target utilization but we must not consider unschedulable nodes if they are under low utilization threshold. If it is not the case, we should fix the LowNodeUtilization strategy.

[ravisantoshgudimetla]

Ok after reading #42, I understood @containscafeine wants to do here but I believe the change in code should be in utilization strategy file. I will send a PR soon.

[concaf]

Yay, thanks @ravisantoshgudimetla. Closing in favor of #45