Split the predicate function to solve the resource filtering problem encountered during preemption #2818
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
volcano-sh-bot
added
the
size/L
wangyang0616
changed the title
wangyang0616
force-pushed
the
feature_predicate_split
branch
from
bb8c128 to
71385c9
Compare
| task.Namespace, task.Name, node.Name) | ||
| return api.NewFitError(task, node, api.NodePodNumberExceeded) | ||
| } | ||
|
|
||
| if gpuDevice, ok := node.Others[api.GPUSharingDevice].(api.Devices); ok { |
There was a problem hiding this comment.
Whether a type assertion is required here？
There was a problem hiding this comment.
We have this issue to address it.
Hope it can be approved and merged as scheduler should not crash.
There was a problem hiding this comment.
Yes, assertion judgment is required here, the current PR code has already been processed, I should comment on the wrong PR.
wangyang0616
force-pushed
the
feature_predicate_split
branch
3 times, most recently
from
143d7a6 to
df4c199
Compare
|
/priority important-soon |
volcano-sh-bot
added
the
priority/important-soon
…cators, and predicateFn filters inherent properties of nodes Signed-off-by: wangyang <wangyang8126@gmail.com>
wangyang0616
force-pushed
the
feature_predicate_split
branch
from
df4c199 to
a8a888f
Compare
…ted from predicateFn to predicateResoureFn Signed-off-by: wangyang <wangyang8126@gmail.com>
wangyang0616
force-pushed
the
feature_predicate_split
branch
from
a8a888f to
2b25c6b
Compare
|
/lgtm |
|
@neujie: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -98,11 +98,16 @@ func (alloc *Action) Execute(ssn *framework.Session) { | |||
| allNodes := ssn.NodeList | |||
| predicateFn := func(task *api.TaskInfo, node *api.NodeInfo) error { | |||
| // Check for Resource Predicate | |||
| if ok, reason := task.InitResreq.LessEqualWithReason(node.FutureIdle(), api.Zero); !ok { | |||
| return api.NewFitError(task, node, reason) | |||
| if err := ssn.PredicateResourceFn(task, node); err != nil { | |||
There was a problem hiding this comment.
PredicateResource is just a resource comparation and do we have to make it as a session API.
It seems there is no need to implement the function with different ways in different plugins.
make it as a session API.
|
The problem has been dealt with in Predicate adapts allocate and preempt #2916, the current pr display is pending. /hold |
volcano-sh-bot
added
the
do-not-merge/hold
|
/close |
|
@wangyang0616: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Background:
allocate, preempt, and reclaim all call the predicate to filter nodes. If the predicate has a filter condition for setting whether idle resources are satisfied, then preempt and reclaim will not be able to perform the preemption action.
Solution:
Split the resource-related filter conditions in the predicate into predicateResource for independent judgment. The allocate phase calls predicate and predicateResource to filter nodes, and preempt and reclaim call predicate to filter nodes.
Notice:
Subsequent custom plug-ins also need to comply with the above principles, and the resource filter conditions are uniformly placed in the predicateResource node for processing, otherwise the preemption function may be affected
associate: #2739