RemoveDuplicates: take node taints, node affinity and node selector into account when computing a number of feasible nodes for the average occurence of pods per node #563
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
ingvagabund
force-pushed
the
removeduplicates-take-taints-into-account-for-node-count
branch
from
e91aa16
to
f29cc0b
Compare
k8s-ci-robot
added
size/XXL
ingvagabund
force-pushed
the
removeduplicates-take-taints-into-account-for-node-count
branch
from
f29cc0b
to
e0e5b73
Compare
ingvagabund
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| if utils.TolerationsEqual(pod.Spec.Tolerations, dp.Spec.Tolerations) && | ||
| utils.NodeSelectorsEqual(getNodeAffinityNodeSelector(pod), getNodeAffinityNodeSelector(dp)) && | ||
| reflect.DeepEqual(pod.Spec.NodeSelector, dp.Spec.NodeSelector) { | ||
| duplicated = true | ||
| continue |
There was a problem hiding this comment.
will this count pods as duplicate if they don't have tolerations/selectors/affinity?
There was a problem hiding this comment.
also could you explain a bit more about what this check is for? I am a little confused about the check for duplicate node selector rules here (not that it's wrong, it's likely just me 🙂 )
There was a problem hiding this comment.
The idea is to assign each pod a (tolerations, nodeselectors, node affinity) triple (including nil instances). Also, there's some logic that reduces duplicated tolerations/nodeselectors/nodeaffinities terms and sorts the terms so it's easy to identify the duplicated terms. So when two triples are equal, evaluation of both corresponding pods wrt. feasible nodes will end up the same. Thus, reducing the number of (pod, node) evaluations with the same result. Thus, minimizing the set of pods where we check how many nodes are feasible for them once evicted. getTargetNodes is computing how many nodes are feasible for all the pods there were marked for eviction. So the average number of pods per node can be adjusted based on what getTargetNodes returns.
The checks in getTargetNodes will be most probably all duplicates of checks in IsEvictable.
There was a problem hiding this comment.
Ah, I see so this check is meant for efficiency in the later checks if I understand correctly?
There was a problem hiding this comment.
Right, efficiency purposes only.
There was a problem hiding this comment.
I think that's a great idea. Would you mind adding that to the comment for this block, to explain that this is for efficiency? It wasn't clear to me why we were looking for distinct pods at this point
There was a problem hiding this comment.
Done. Adding
// In order to reduce the number of pods processed, identify pods which have
// equal (tolerations, nodeselectors, node affinity) terms and considered them
// as identical. Identical pods wrt. (tolerations, nodeselectors, node affinity) terms
// will produce the same result when checking if a pod is feasible for a node.
// Thus, improving efficiency of processing pods marked for eviction.
…nto account when computing a number of feasible nodes for the average occurence of pods per node Nodes with taints which are not tolerated by evicted pods will never run the pods. The same holds for node affinity and node selector. So increase the number of pods per feasible nodes to decrease the number of evicted pods.
ingvagabund
force-pushed
the
removeduplicates-take-taints-into-account-for-node-count
branch
from
e0e5b73
to
5396282
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
ingvagabund
deleted the
removeduplicates-take-taints-into-account-for-node-count
branch
…tes-take-taints-into-account-for-node-count RemoveDuplicates: take node taints, node affinity and node selector into account when computing a number of feasible nodes for the average occurence of pods per node
Nodes with taints which are not tolerated by evicted pods will never run the pods. The same holds for node affinity and node selector. So increase the number of pods per feasible nodes to decrease the number of evicted pods.
Fixes: #551
TODO: