volcano icon indicating copy to clipboard operation
volcano copied to clipboard

Introduce a new interface `AddPreBindFn` or `AddBindFn` in session

Open Monokaix opened this issue 1 year ago • 11 comments

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.

Monokaix avatar Jul 23 '24 12:07 Monokaix

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

googs1025 avatar Jul 24 '24 00:07 googs1025

Task list:

  • [ ] Add PreBindFn and AddPreBindFn func in session_plugins.go and call PreBindFn before call Allocate in 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.

Monokaix avatar Jul 24 '24 02:07 Monokaix

@googs1025 Hi task list has been added.

Monokaix avatar Jul 24 '24 02:07 Monokaix

/assign @googs1025 @Monokaix

Monokaix avatar Jul 24 '24 02:07 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

googs1025 avatar Jul 25 '24 01:07 googs1025

nodeName string

I think we can keep consistent with kube-scheduler and add nodeName string.

Monokaix avatar Jul 25 '24 02:07 Monokaix

nodeName string

I 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

googs1025 avatar Jul 25 '24 03:07 googs1025

Predicate

You mean the input arg *api.NodeInfo? The preBind is imported from k8s, so it's better to keep consistent with it: )

Monokaix avatar Jul 25 '24 07:07 Monokaix

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

googs1025 avatar Jul 25 '24 07:07 googs1025

/unassign /assign @JesseStutler

more detail: https://github.com/volcano-sh/volcano/pull/3621#issuecomment-2411161399

googs1025 avatar Oct 14 '24 12:10 googs1025

@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.

volcano-sh-bot avatar Oct 14 '24 12:10 volcano-sh-bot

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!).

stale[bot] avatar Feb 01 '25 01:02 stale[bot]

work in progress

hwdef avatar Feb 06 '25 07:02 hwdef

/assign

JesseStutler avatar Apr 30 '25 03:04 JesseStutler

new implements :https://github.com/volcano-sh/volcano/pull/4152

Monokaix avatar Jun 03 '25 07:06 Monokaix

/close

Monokaix avatar Jun 03 '25 07:06 Monokaix

@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.

volcano-sh-bot avatar Jun 03 '25 07:06 volcano-sh-bot