Add webhook mutate and controller actions and test case for minAvailable #2896

renwenlong-github · 2023-06-05T13:33:46Z

lowang-bh · 2023-06-06T05:20:56Z

how about put the podtemplate/pod genaration in a function so that there are fewer code lines?

renwenlong-github · 2023-06-06T07:39:49Z

how about put the podtemplate/pod genaration in a function so that there are fewer code lines?

I have updated it

hwdef · 2023-06-06T08:44:57Z

Sorry, I don't understand the purpose of this pr and the description in the issue. Can you describe in more detail, is this PR to fix some bugs, or to enhance some functions?

renwenlong-github · 2023-06-07T08:51:38Z

Sorry, I don't understand the purpose of this pr and the description in the issue. Can you describe in more detail, is this PR to fix some bugs, or to enhance some functions?

Sorry, I may not have made it clear. This is an optimization; I want to enhance the minAvailable function. As you know, there is a minavailable attribute in the task, and the job.spec also has the minavailable attribute.

I think users only need to write task.minavailable , Volcano's webhook calculates job.spec.minavailable;

on the other hand, I think there are bugs in the here of code that generate the pod group, especially the dependson task.

func (cc *jobcontroller) calcPGMinResources(job *batch.Job) *v1.ResourceList {
	// sort task by priorityClasses
	var tasksPriority TasksPriority
	for _, task := range job.Spec.Tasks {
		tp := TaskPriority{0, task}
		pc := task.Template.Spec.PriorityClassName

		if pc != "" {
			priorityClass, err := cc.pcLister.Get(pc)
			if err != nil || priorityClass == nil {
				klog.Warningf("Ignore task %s priority class %s: %v", task.Name, pc, err)
			} else {
				tp.priority = priorityClass.Value
			}
		}
		tasksPriority = append(tasksPriority, tp)
	}

	sort.Sort(tasksPriority)     // sort tasks

	minReq := v1.ResourceList{}
	podCnt := int32(0)
	for _, task := range tasksPriority {
		for i := int32(0); i < task.Replicas; i++ {
			if podCnt >= job.Spec.MinAvailable {
				break
			}

			podCnt++
			pod := &v1.Pod{
				Spec: task.Template.Spec,
			}
			minReq = quotav1.Add(minReq, *util.GetPodQuotaUsage(pod))
		}
	}

	return &minReq
}

It first calculates the resources of job.Spec.MinAvailable jobs with high priority. I think this is wrong in many cases, such as dependsOn task, or multi-task jobs with equal Priority levels. If job.Spec.MinAvailable < sum(tasks .MinAvailable), he will randomly calculate the minimum resource.

I think I'm going to write a detailed design document。

hwdef · 2023-06-07T09:41:59Z

I probably see what you mean, thanks for the suggestion, and you are right.
I think it's a more complex change and it has a big impact, if you will write a detailed document, I'll be happy to review it.

renwenlong-github · 2023-06-08T09:13:23Z

I probably see what you mean, thanks for the suggestion, and you are right. I think it's a more complex change and it has a big impact, if you will write a detailed document, I'll be happy to review it.

I know this is a very complicated issue, I may give a suggestion, the modification still requires the overall evaluation of the volcano team, and I will provide a design document. give me some time.

renwenlong-github · 2023-06-26T08:55:54Z

This problem is difficult. I wrote a design document, which is currently in Chinese and can be changed to English later.

https://github.com/renwenlong-github/volcano/blob/pref-minAvailable/docs/design/task-minAvailable-dependson-design.md

If you have time, we can discuss in detail

renwenlong-github · 2023-06-26T08:57:56Z

issue #2921

hwdef · 2023-06-26T09:56:24Z

Thanks for your design docs, I will read it later.

hwdef · 2023-06-26T10:33:22Z

先用中文沟通了，我看了下文档，大致有下面几个问题，如果能更新下设计文档或者做出一些说明就更好了。

哪些设计是已有的，哪些是新加的，这个最好写清楚

如果有依赖则在job annotation中标注dependent-job=true

这个的目的是什么

把该运行的bind，不该运行的资源预留下来

如何预留资源？例如master先运行，worker后运行，如何保证master运行后，worker的资源不被抢走。如何兼容多调度器场景？例如在volcano做了资源预留，但是k8s原生调度器把剩余资源抢走了

注意：仅设置job.minAvailable不设置task.minAvailable，volcano会计算job.minAvailable=sum(tasks.replaces)，用户设置的job.minAvailable值会被覆盖。

这里不应该覆盖，即，仅设置job.minAvailable不设置task.minAvailable时，则应该按照job.minAvailable的设置为主，若job.minAvailable和task.minAvailable都未设置时，则job.minAvailable=sum(tasks.replaces)

renwenlong-github · 2023-06-29T07:59:53Z

issue #2944

lowang-bh · 2023-06-29T13:24:47Z

I think it should consider issue #2921 together. We need to discard this feature deeply. Nowadays there is nobady knows the design perpose which is introduced by @shinytang6 in PR #1459. I read the code and find some problems that the code logic is not self-consistent.

我觉得我们需要深度讨论一下这个功能，issue #2921 记录了需要考虑到的情况，包括目前已有的逻辑和还缺少的，自从PR#1459引入任务级别的minAvaiable之后，代码逻辑不再完备。

Signed-off-by: Ren <[email protected]>

renwenlong-github · 2023-06-30T04:04:24Z

I think it should consider issue #2921 together. We need to discard this feature deeply. Nowadays there is nobady knows the design perpose which is introduced by @shinytang6 in PR #1459. I read the code and find some problems that the code logic is not self-consistent.

我觉得我们需要深度讨论一下这个功能，issue #2921 记录了需要考虑到的情况，包括目前已有的逻辑和还缺少的，自从PR#1459引入任务级别的minAvaiable之后，代码逻辑不再完备。

Yeah, at first I thought it was a small bug, but I found some logic issues with both the controller and the scheduler, and like you said, we need to discuss this feature.
是的，刚开始的时候我以为是一个小bug，但是我发现无论是控制器创建podgroup还是调度器的job ready验证都有一些逻辑问题，就像你说的，我们需要讨论这个功能。

renwenlong-github · 2023-06-30T04:06:15Z

先用中文沟通了，我看了下文档，大致有下面几个问题，如果能更新下设计文档或者做出一些说明就更好了。

哪些设计是已有的，哪些是新加的，这个最好写清楚

如果有依赖则在job annotation中标注dependent-job=true

这个的目的是什么

把该运行的bind，不该运行的资源预留下来

如何预留资源？例如master先运行，worker后运行，如何保证master运行后，worker的资源不被抢走。如何兼容多调度器场景？例如在volcano做了资源预留，但是k8s原生调度器把剩余资源抢走了

注意：仅设置job.minAvailable不设置task.minAvailable，volcano会计算job.minAvailable=sum(tasks.replaces)，用户设置的job.minAvailable值会被覆盖。

这里不应该覆盖，即，仅设置job.minAvailable不设置task.minAvailable时，则应该按照job.minAvailable的设置为主，若job.minAvailable和task.minAvailable都未设置时，则job.minAvailable=sum(tasks.replaces)

I modified the design document, help me review it, thank you

Signed-off-by: Ren <[email protected]>

hwdef · 2023-07-05T11:44:40Z

pkg/controllers/job/job_controller_util.go

@@ -35,12 +35,15 @@ import (

 var detectionPeriodOfDependsOntask time.Duration

+// TaskMinAvailable todo(renwenlong) move to volcano.sh api
+const TaskMinAvailable = "volcano.sh/task-min-available"


You can do this now.

ok，I will move it

hwdef · 2023-07-05T11:46:20Z

pkg/scheduler/actions/allocate/allocate.go

@@ -235,7 +235,10 @@ func (alloc *Action) Execute(ssn *framework.Session) {
 			}

 			if ssn.JobReady(job) && !tasks.Empty() {
-				jobs.Push(job)
+				// skip dependent job push, because dependent job has other pods will be creat


Suggested change

// skip dependent job push, because dependent job has other pods will be creat

// skip dependent job push, because dependent job has other pods will be create

hwdef · 2023-07-05T11:48:15Z

pkg/scheduler/api/job_info.go

+	TaskPriorityAnnotation = "volcano.sh/task-priority"
+	TaskMinAvailable       = "volcano.sh/task-min-available"
+)


I think these need to be normalized, such as put them in the api

ok，I will move it

hwdef · 2023-07-06T07:22:54Z

pkg/webhooks/admission/jobs/validate/admit_job.go

+		if len(job.Spec.Tasks) > 1 && job.Spec.MinAvailable != totalReplicas {
+			msg += " Multi-tasks job 'minAvailable' needs to set task 'minAvailable', " +
+				"otherwise job 'minAvailable' must be equal to the sum of task 'replicas';"
+		}


I don't think this is needed

If
job.Spec.MinAvailable == 5 , totalReplicas =6 ,I think this is OK

I did not explain my idea. If multi-task does not set task.minAvailable, it cannot explain that those tasks are equal to job.minAvailable.

apiVersion: batch.volcano.sh/v1alpha1 kind: Job metadata: name: minavailable-job spec: schedulerName: volcano minAvailable: 2 tasks: - replicas: 3 name: "master" template: metadata: name: master spec: containers: - image: nginx name: nginx resources: requests: cpu: "1" memory: "1Gi" restartPolicy: OnFailure - replicas: 3 name: "work" template: metadata: name: web spec: containers: - image: nginx name: nginx resources: requests: cpu: "1" memory: "1Gi" restartPolicy: OnFailure

master Running 2 ? work Running 2 ? also master 1 and work 1 is ok?

hwdef · 2023-07-06T07:23:31Z

pkg/webhooks/admission/jobs/validate/admit_job.go

+		if len(new.Spec.Tasks) > 1 && new.Spec.MinAvailable != totalReplicas {
+			return fmt.Errorf(" Multi-tasks job 'minAvailable' needs to set task 'minAvailable', " +
+				"otherwise job 'minAvailable' must be equal to total task 'replicas'")
+		}


I don't think this is needed

If
job.Spec.MinAvailable == 5 , totalReplicas =6 ,I think this is OK.

hwdef · 2023-07-06T07:31:27Z

docs/design/task-minAvailable-dependson-design.md

+# user-guide
+
+When the user uses the vcjob minAvailable feature, only should set each `task.minAvailable` value, if not set `task.minAvailable`,
+the default `task.minAvailable=task.replaces`.


task.minavailable is not equal to task.replaces if job.minavailable is set

There is only one task in job, job.minavailable or task.minavailable set any one is ok, but Multitasking is ambiguous.

hwdef · 2023-07-06T07:41:42Z

docs/design/task-minAvailable-dependson-design.md

+
+<img src="images/minAvailable-dependsOn-3.png" alt="dependsOn1" width="700"><br>
+
+### 2、task gang


Duplicated with line 218?

I will fix it

hwdef · 2023-07-06T07:50:06Z

docs/design/task-minAvailable-dependson-design.md

+```
+
+# TODO
+How to optimize dependsOn job to meet gang scheduling?


I think it is most important that we deal with resource reservation, including resource reservation in the case of multiple schedulers, please add this information

It's a complicated issue, I can't fix it in this pr, which means that dependency tasks don't support gang like before, but this pr solves the problem of scheduling deadlock

hwdef · 2023-07-06T07:54:04Z

@lowang-bh
If you can, please also review this PR and let's improve the related features together.

lowang-bh · 2023-07-14T14:28:36Z

@lowang-bh If you can, please also review this PR and let's improve the related features together.

I just find the others can not see my comments in code review... >_<

hwdef · 2023-07-14T17:42:15Z

@lowang-bh If you can, please also review this PR and let's improve the related features together.

I just find the others can not see my comments in code review... >_<

you should click submit click

lowang-bh · 2023-06-10T05:12:46Z

pkg/controllers/job/job_controller_actions.go

+			break
+		}
+		if task.MinAvailable == nil {
+			task.MinAvailable = &task.Replicas


Is it reasonable to modify task's attribute here?

And, what's more, it dosn't cover all the cases.

lowang-bh · 2023-06-10T05:15:29Z

pkg/controllers/job/job_controller_actions.go

+			if task.DependsOn == nil {
+				tmpMinReq := v1.ResourceList{}
+				// only calc task minAvailable
+				for i := int32(0); i < *task.MinAvailable; i++ {


can we refact those duplicate codes? eg, define in a function.

lowang-bh · 2023-07-02T12:25:31Z

pkg/scheduler/plugins/priority/priority.go

+					Pods: work-0、work-1、work-2
+
+				the right pods order should be:
+					master-0、master-1、master-2、work-0、work-1、master-3、master-4、work-2、work-3


I think it should put in gang plugin.

lowang-bh · 2023-07-02T12:27:28Z

pkg/webhooks/admission/jobs/mutate/mutate_job.go

@@ -188,6 +213,11 @@ func mutateSpec(tasks []v1alpha1.TaskSpec, basePath string, job *v1alpha1.Job) *
 	// 	mpi.AddDependsOn(job)
 	// }
 	patched := false
+	if len(tasks) == 1 && tasks[0].MinAvailable == nil {


this is special code, I don't think it is a good change.

I agren with you .

That means i was wrong，I'm going to rethink this question🤔️。

lowang-bh · 2023-07-15T01:13:40Z

I just find the others can not see my comments in code review... >_<

you should click submit click

Thanks.

stale · 2023-10-15T09:14:41Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

hwdef · 2023-12-15T08:53:06Z

/reopen

volcano-sh-bot · 2023-12-15T08:53:10Z

@hwdef: Reopened this PR.

In response to this:

/reopen

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 · 2023-12-15T08:53:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign william-wang
You can assign the PR to them by writing /assign @william-wang in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stale · 2024-03-17T09:30:40Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

volcano-sh-bot · 2024-04-09T02:03:20Z

@renwenlong-github: PR needs rebase.

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 requested review from hzxuzhonghu and k82cn June 5, 2023 13:33

volcano-sh-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 5, 2023

volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 6, 2023

renwenlong-github force-pushed the pref-minAvailable branch from e53bc7e to 8cc60c0 Compare June 6, 2023 07:52

volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 6, 2023

renwenlong-github force-pushed the pref-minAvailable branch from 8cc60c0 to 6fdadb3 Compare June 26, 2023 08:47

volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 26, 2023

optimize minAvailable and dependson

b955463

Signed-off-by: Ren <[email protected]>

renwenlong-github force-pushed the pref-minAvailable branch from 6fdadb3 to b955463 Compare June 30, 2023 03:24

add webhook admit test

b5e98bb

Signed-off-by: Ren <[email protected]>

fix e2e test for minAvailable

ab0c01a

Signed-off-by: Ren <[email protected]>

renwenlong-github force-pushed the pref-minAvailable branch from b09ae2e to ab0c01a Compare July 3, 2023 03:36

renwenlong-github force-pushed the pref-minAvailable branch from 200327b to ab0c01a Compare July 3, 2023 03:48

renwenlong-github and others added 2 commits July 3, 2023 11:49

Merge branch 'master' into pref-minAvailable

90a8e8e

fix e2e test minAvailable

cfbb7e2

Signed-off-by: Ren <[email protected]>

hwdef reviewed Jul 6, 2023

View reviewed changes

Update dependson design

cc278df

lowang-bh reviewed Jul 15, 2023

View reviewed changes

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 15, 2023

stale bot closed this Dec 15, 2023

volcano-sh-bot reopened this Dec 15, 2023

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 15, 2023

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 17, 2024

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2024

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024

	// skip dependent job push, because dependent job has other pods will be creat
	// skip dependent job push, because dependent job has other pods will be create


		<img src="images/minAvailable-dependsOn-3.png" alt="dependsOn1" width="700"><br>

		### 2、task gang

Add webhook mutate and controller actions and test case for minAvailable #2896

Are you sure you want to change the base?

Add webhook mutate and controller actions and test case for minAvailable #2896

Conversation

renwenlong-github commented Jun 5, 2023

lowang-bh commented Jun 6, 2023

renwenlong-github commented Jun 6, 2023

hwdef commented Jun 6, 2023

renwenlong-github commented Jun 7, 2023

hwdef commented Jun 7, 2023

renwenlong-github commented Jun 8, 2023

renwenlong-github commented Jun 26, 2023

renwenlong-github commented Jun 26, 2023 • edited Loading

hwdef commented Jun 26, 2023

hwdef commented Jun 26, 2023

renwenlong-github commented Jun 29, 2023

lowang-bh commented Jun 29, 2023 • edited Loading

renwenlong-github commented Jun 30, 2023

renwenlong-github commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwdef commented Jul 6, 2023

lowang-bh commented Jul 14, 2023

hwdef commented Jul 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowang-bh commented Jul 15, 2023

stale bot commented Oct 15, 2023

hwdef commented Dec 15, 2023

volcano-sh-bot commented Dec 15, 2023

volcano-sh-bot commented Dec 15, 2023

stale bot commented Mar 17, 2024

volcano-sh-bot commented Apr 9, 2024

renwenlong-github commented Jun 26, 2023 •

edited

Loading

lowang-bh commented Jun 29, 2023 •

edited

Loading