volcano
volcano copied to clipboard
Introduce a new interface `AddPreBindFn` or `AddBindFn` in session
What would you like to be added:
Add a new interface AddPreBindFn or AddBindFn in session to support more flexible bind.
Why is this needed:
Currently, volcano has supported kube-scheduler default pulgin for a long time, and plugins of kube-scheduler have an extension PreBind, which is called before a bind request, while volcano has no related extension, which needs modify the framework when we import kube-scheduler's plugin, there are two plugins that volcano has imported or is importing called volumebinding and dynamicResourceAllocation.
When import thevolumebinding plugin of kube-scheduler,I found that it already initialized a VolumeBinder in schedulerCache, but actually it is only needed in plugin level,and the VolumeBinder should only exist in plugin, so there are some problems when supporting volumeBinding. This pr https://github.com/volcano-sh/volcano/pull/2506 added the volumeBinding plugin, but is reverted because there is memory leak issue, detail https://github.com/volcano-sh/volcano/pull/2555, it is precisely because the logic of volumebinding is scattered everywhere and multiple caches are initialized that causes memory leaks,so if we can support a new interface,the logic of volumebinding will be more cohesive and avoid some potential problems.
Also, volcano is also supporting dynamicRes ourceAllocation(DRA), pr is here:https://github.com/volcano-sh/volcano/pull/3577, and the dra plugin also has an extension preBind, if there is no support in framework of preBind, then the logic of dra will also be scattered everywhere and codes will becomes unmaintainable.
Based on the above two considerations, we should support prebind extension.
Sounds great to me. Will this be split into subtasks? Or will you take it? If possible, I would like to participate in the work on this issue
Task list:
- [ ] Add
PreBindFnandAddPreBindFnfunc in session_plugins.go and callPreBindFnbefore callAllocatein allocate and backfill, and unallocate when error occurs. - [ ] Add volumebinding plugin in predict and move volumeBind related logic from schedulerCache to volumebinding plugin itself.
- [ ] DRA plugin use above callback func to do preBind.
@googs1025 Hi task list has been added.
/assign @googs1025 @Monokaix
There is something I want to discuss. I saw the implementation of the k8s preBind interface as follows:
// PreBindPlugin is an interface that must be implemented by "PreBind" plugins.
// These plugins are called before a pod being scheduled.
type PreBindPlugin interface {
Plugin
// PreBind is called before binding a pod. All prebind plugins must return
// success or the pod will be rejected and won't be sent for binding.
PreBind(ctx context.Context, state *CycleState, p *v1.Pod, nodeName string) *Status
}
Maybe we need to define PreBindFn: like this?
type PreBindFn func(*TaskInfo) error
nodeName string
I think we can keep consistent with kube-scheduler and add nodeName string.
nodeName stringI think we can keep consistent with kube-scheduler and add
nodeName string.
maybe *api.NodeInfo is better?
When we execute Predicate, the output value is *api.NodeInfo
Predicate
You mean the input arg *api.NodeInfo?
The preBind is imported from k8s, so it's better to keep consistent with it: )
Predicate
You mean the input arg
*api.NodeInfo? The preBind is imported from k8s, so it's better to keep consistent with it: )
ok, got it
/unassign /assign @JesseStutler
more detail: https://github.com/volcano-sh/volcano/pull/3621#issuecomment-2411161399
@googs1025: GitHub didn't allow me to assign the following users: JesseStutler.
Note that only volcano-sh members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide
In response to this:
/unassign /assign @JesseStutler
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.
Hello 👋 Looks like there was no activity on this issue for last 90 days. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity for 60 days, this issue will be closed (we can always reopen an issue if we need!).
work in progress
/assign
new implements :https://github.com/volcano-sh/volcano/pull/4152
/close
@Monokaix: Closing this issue.
In response to this:
/close
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.