Update nodes sorting function to respect available resources #1541
Conversation
k8s-ci-robot
added
cncf-cla: yes
ingvagabund
force-pushed
the
sortNodesByUsage-dont-hardcode-resource-names
branch
from
02b7f90
to
7eeb07d
Compare
ingvagabund
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
ingvagabund
deleted the
sortNodesByUsage-dont-hardcode-resource-names
branch
|
Conducting a self-review due to a limited availability of reviewers. However, contributors are still encouraged to perform a post-merge review to identify any additional improvements or to catch any potential regressions introduced by these changes. Your feedback, even after the merge, will help enhance the quality and robustness of the code. |
| } else { | ||
| tj += nodes[j].usage[resourceName].Value() | ||
| } | ||
| } | ||
|
|
||
| // extended resources | ||
| for name := range nodes[i].usage { |
There was a problem hiding this comment.
Shouldn't this block be now removed, so we do not count the other resources twice?
| @@ -368,8 +368,22 @@ func evictPods( | |||
| // sortNodesByUsage sorts nodes based on usage according to the given plugin. | |||
| func sortNodesByUsage(nodes []NodeInfo, ascending bool) { | |||
| sort.Slice(nodes, func(i, j int) bool { | |||
| ti := nodes[i].usage[v1.ResourceMemory].Value() + nodes[i].usage[v1.ResourceCPU].MilliValue() + nodes[i].usage[v1.ResourcePods].Value() | |||
| tj := nodes[j].usage[v1.ResourceMemory].Value() + nodes[j].usage[v1.ResourceCPU].MilliValue() + nodes[j].usage[v1.ResourcePods].Value() | |||
| ti := resource.NewQuantity(0, resource.DecimalSI).Value() | |||
There was a problem hiding this comment.
this seems like a quite elaborate way to declare int64 variable ;)
| tj := nodes[j].usage[v1.ResourceMemory].Value() + nodes[j].usage[v1.ResourceCPU].MilliValue() + nodes[j].usage[v1.ResourcePods].Value() | ||
| ti := resource.NewQuantity(0, resource.DecimalSI).Value() | ||
| tj := resource.NewQuantity(0, resource.DecimalSI).Value() | ||
| for resourceName := range nodes[i].usage { |
There was a problem hiding this comment.
Offtopic: intriguing to see that all resources have the same weight. Not sure if it makes sense, but this is decided by the user when configuring it, right?
There was a problem hiding this comment.
We have not explored this avenue. It's still open for debate.
So
sortNodesByUsagecan be invoked over any list of resources