volcano-sh / volcano

A Cloud Native Batch System (Project under CNCF)
https://volcano.sh
Apache License 2.0
4.1k stars 948 forks source link

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

Open Monokaix opened 2 months ago

Monokaix commented 2 months ago

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.

googs1025 commented 2 months ago

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

Monokaix commented 2 months ago

Task list:

Monokaix commented 2 months ago

@googs1025 Hi task list has been added.

Monokaix commented 2 months ago

/assign @googs1025 @Monokaix

googs1025 commented 2 months ago

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
Monokaix commented 2 months ago
nodeName string

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

googs1025 commented 2 months ago
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

Monokaix commented 2 months ago

Predicate

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

googs1025 commented 2 months ago

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