feature: add prebind for scheduler #3621
Conversation
volcano-sh-bot
added
do-not-merge/work-in-progress
volcano-sh-bot
added
the
size/M
googs1025
force-pushed
the
prebind_feature
branch
4 times, most recently
from
dfa4ced
to
31fbc9c
Compare
|
It's not just simply add a prebind，we should also call unallocate to do the reverse in framework, you can refer to the kube-scheduler logic: ) |
googs1025
force-pushed
the
prebind_feature
branch
from
31fbc9c
to
b8c183d
Compare
As far as I understand, can we split it into smaller PRs to complete different features? Because adding the prebind method does not seem to be related to the unallocate method. |
I am not sure if I need to implement a method similar to unreserve, because there is no process similar to reserve in the volcano scheduler process. Maybe pipeline is a process similar to reserve? |
googs1025
force-pushed
the
prebind_feature
branch
2 times, most recently
from
5e4c5db
to
722c283
Compare
pkg/scheduler/framework/session.go
Outdated
| @@ -437,6 +438,41 @@ func (ssn *Session) Allocate(task *api.TaskInfo, nodeInfo *api.NodeInfo) (err er | |||
| return nil | |||
| } | |||
|
|
|||
| // Unallocate is called when the prebind function fails | |||
| func (ssn *Session) Unallocate(task *api.TaskInfo) error { | |||
There was a problem hiding this comment.
The logic of the Unallocate method is to modify the cache in ssn
- Change the task status to pending
- Delete this task (Pod) in the node
- send Deallocate event (I'm not sure if this step is necessary)
- set task.Name = = ""
added |
googs1025
changed the title
volcano-sh-bot
removed
the
do-not-merge/work-in-progress
|
/assign @Monokaix |
pkg/scheduler/framework/session.go
Outdated
| @@ -437,6 +438,41 @@ func (ssn *Session) Allocate(task *api.TaskInfo, nodeInfo *api.NodeInfo) (err er | |||
| return nil | |||
| } | |||
|
|
|||
| // Unallocate is called when the prebind function fails | |||
| func (ssn *Session) Unallocate(task *api.TaskInfo) error { | |||
| @@ -91,6 +91,17 @@ func (backfill *Action) Execute(ssn *framework.Session) { | |||
| } | |||
| } | |||
|
|
|||
| // before allocating to a node, run prebind fn | |||
There was a problem hiding this comment.
What will preBind be used for? And why don't put it in ssn.Allocate?
There was a problem hiding this comment.
This needs @Monokaix to help explain.
I think it is a method to expand to adapt to k8s plugins.
There was a problem hiding this comment.
Because k8s is always changing, I'm not sure if this incremental adaptation is a good design approach.
There was a problem hiding this comment.
As described in issue，without preBind plugin's logic is scattered in framework，for volumeBinidng plugin, both framewok and plugin hold cache and lead to inconsistent cache and memory leak, it's also unacceptable.
If you guys have better thoughts, any suggestions are welcome: )
Signed-off-by: googs1025 <[email protected]>
googs1025
force-pushed
the
prebind_feature
branch
from
722c283
to
cab9e4f
Compare
|
[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 |
|
I don't have much time to complete this PR. I have discussed this with @JesseStutler offline, and this part has been transferred to him. But I will continue to follow this part cc @Monokaix |
|
/close |
|
@googs1025: 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. |
/kind feature
fix: #3618
part of issue: #3618