Open hwdef opened 1 month ago
/cc
/good-first-issue
@Monokaix: This request has been marked as suitable for new contributors.
Please ensure the request meets the requirements listed here.
If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue
command.
/assign
I will find time to research and try to implement
/assign
@Sianao Hello, I am currently working on this issue. Can you find other issues? Or do you have any special requirements for this issue?
@Sianao Hello, I am currently working on this issue. Can you find other issues? Or do you have any special requirements for this issue?
I unassign it already
@Sianao Do you want to try this issue? I'm currently busy with other projects and don't have time to do this. :) If you want, feel free to take it. I will review and help in the PR /unassign
/assign @Sianao
/unassign
Sir , I m busy , too .
I'm glad to work on this.
/assign
@hwdef Hi, Do we only need to support preemptionPolicy at the pod level? Not to add more fields in job_info. So that we only need to add some logic in preempting, like this: If the preemptionPolicy is 'Never', not allowing the pod to preempt others, directly skip this pod.
We should also consider this case: https://github.com/volcano-sh/volcano/issues/3512 @hwdef @JesseStutler
I think we can keep the property of priority as a number, and add extra info about preemptionPolicy
in taskInfo or JobInfor when parse it.
@lowang-bh I think get the preeemptionPolicy
from pod spec in taskInfo is enough? K8s default scheduler only use the preemptionPolicy
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go#L240-L242 here to judge whether the preemptionPolicy
is Never. So there is no need to add extra field I think if we only consider implementing preemptionPolicy
at the pod level.
There is the scenario we need to make sure that is whether we need to support preemptionPolicy
at the vcjob level(Put field into JobInfo), because vcjob also has priorityClass
field. But when vc-controller creates the pod, the PriorityClass in the pod spec was not set to the same value as vcjob, just use the priorityClass
specified in the template, we need to discuss how to implement this, or we don't need to implement preemptionPolicy
at the vcjob level. @hwdef
You are right. If you only implement preemptionpolicy for pods, you do not need to add other fields. But I think it's better implement preemptionPolicy
for vcjob.
You are right. If you only implement preemptionpolicy for pods, you do not need to add other fields. But I think it's better implement
preemptionPolicy
for vcjob.
What I'm concern is that do we really have a use case for supporting preemptionPolicy
for vcjob, becuase the field is just derived from kube-scheduler and is just for pod before: )
If we support preemptionPolicy
for vcjob, becasuse preemptionPolicy
field comes from priorityclass, I think all pods in vcjob should set the same PriorityClassName
with vcjob. In kube-apiserver, there is a plugin called Priority will set the pod's preemptionPolicy
field:
https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/priority/admission.go#L193-L200
So we only need to modify vc-controller to set the pod template's PriorityClassName
when creating pod, scheduler only need to add logic in preempt and reclaim to skip if the PreemtionPolicy
is Never
, like this:
if task.Pod.Spec.PreemptionPolicy != nil && *task.Pod.Spec.PreemptionPolicy == v1.PreemptNever {
klog.V(3).Infof("Task %s/%s is not eligible to preempt other tasks due to preemptionPolicy is Never", task.Namespace, task.Name)
...
}
You are right. If you only implement preemptionpolicy for pods, you do not need to add other fields. But I think it's better implement
preemptionPolicy
for vcjob.What I'm concern is that do we really have a use case for supporting
preemptionPolicy
for vcjob, becuase the field is just derived from kube-scheduler and is just for pod before: )
I think the answer is yes, we currently have such a scenario. I implemented it through annotation.
What would you like to be added:
priorityclass has a field named preemptionPolicy. https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#non-preempting-priority-class
volcano didn't implement its logic.
solution
In volcano's cache, priorityclas is just a number, so we need to modify the priorityclass in the cache and then implement preemptionpolicy-related logic.
https://github.com/volcano-sh/volcano/blob/c91e42e07b55fbc0686095aedb0f9436b1991163/pkg/scheduler/api/job_info.go#L121