Pod Scheduling Readiness #3658

ykcai-daniel · 2024-08-07T02:00:37Z

Updated PR of #3612, a gated task no longer blocks the job. Do not use new state.

pkg/scheduler/api/job_info.go

googs1025 · 2024-08-07T02:36:55Z

Does this change have the same purpose as this PR #3612 ?

Monokaix · 2024-08-08T03:41:51Z

Hi，I think there is no necessary to submit two PRs, just based the previous PR and update it OK，and please also update the design doc.

Monokaix · 2024-08-13T03:06:26Z

Please refer to kube-scheduler to add a featuregate to enable/disable this feature.

ykcai-daniel · 2024-08-15T02:45:53Z

@Monokaix PR updated. I will also update the design proposal #3581 to document the changes.

ykcai-daniel · 2024-08-15T02:59:38Z

PR summary: Pod Scheduling gates is a new feature in K8S, which uses the .spec.schedulingGates field of Pod to signal that a Pod is not ready for scheduling (Pod will be in pending state when gated). This feature is often used to implement customized resource managers and we want to support it for compatibility. Main Changes involve:

Scheduling Actions: Job with scheduling gated tasks mainly exist in the Inqueue state. In allocate action, if the pod of a task is scheduling gates, it will be skipped for allocation and not bound to a node. Consequently, if too many tasks are scheduling gates, the job will be gang-unschedulable
Plugins: Since scheduling gated pods are not ready to be allocated, we don't want scheduling gated tasks to consume inqueue resources, making other jobs uninqueuable. Therefore, proportion, capacity and overcommit plugins have to be changed to deduct the resources of scheduling gated tasks
Pod Conditions: When each session close, the Condition of scheduling gated pods will not be updated, showing the Schedulable False condition with reason ScheduleGated, which is created by api-server

Future Works that are not covered in this PR:

Now, we do not support removing scheduling gates by modifying the template of a vcjob because supporting this feature involve major changes to controller and Pod-level gates removal is sufficient for most usecase. In the future, if there is a need for task template support, we can add it.
Events and Correct Message: Now, if a Job is gang unschedulable due to scheduling gates, the reason in condition is "NoEnoughResources". We might need better reason and message for scheduling gates.

ykcai-daniel · 2024-08-30T01:52:08Z

Commit Author: /assign @ykcai-daniel

ykcai-daniel · 2024-08-30T01:53:18Z

/assign @Monokaix

ykcai-daniel · 2024-08-30T01:53:59Z

/assign @googs1025

Monokaix · 2024-08-30T03:14:17Z

/assign @lowang-bh @hwdef

lowang-bh · 2024-08-30T04:52:41Z

pkg/scheduler/api/job_info.go

@@ -228,6 +230,17 @@ func (ti *TaskInfo) ClearLastTxContext() {
 	ti.LastTransaction = nil
 }

+// Return if the pod of a task is scheduling gated by checking if length of sch gates


comments: if length of sch gates is zero?

Sorry for the typo. I will fix the commit

lowang-bh · 2024-08-30T04:57:06Z

pkg/scheduler/api/job_info.go

@@ -228,6 +230,17 @@ func (ti *TaskInfo) ClearLastTxContext() {
 	ti.LastTransaction = nil
 }

+// Return if the pod of a task is scheduling gated by checking if length of sch gates
+// When the Pod is not yet created or sch gates field not set, return false
+func (ti *TaskInfo) SchedulingGated() bool {


how about store this value in taskInfo, instead of a function calling?

I have added a field schGated into taskInfo

lowang-bh · 2024-08-30T05:00:32Z

pkg/scheduler/plugins/capacity/capacity.go

@@ -142,7 +142,7 @@ func (cp *capacityPlugin) OnSessionOpen(ssn *framework.Session) {
 		}

 		if job.PodGroup.Status.Phase == scheduling.PodGroupInqueue {
-			attr.inqueue.Add(job.GetMinResources())
+			attr.inqueue.Add(job.DeductSchGatedResources(job.GetMinResources()))


I think its better to do the calcalation in GetMinResources function. Let GetMinResources return the min resource to run the job.

Add this function into GetMinresources have some downsides. The motivation of DeductSchGatedResources is to deduct the resources of scheduling gated tasks in a job when calculating inqueued resources so that it will not block other jobs from being inqueued. Therefore, this function is will only be used in the queue-related plugins (proporation, capacity, overcommit).

On the other hand, GetMinResources is a function that returns the minimum resource requirement of a Job, which should be an immutable quantity determined at the creation of the PodGroup. This quantity should not change regardless of scheduling gated tasks. Unlike DeductSchGatedResources, apart from inqueue resource calculation, GetMinResources is also used in other places, such as GetElasticResources

If we include DeductSchGatedResources into GetMinresources and tightly couple these two functions, this will bring some runtime information about the state of Pods into a function that should return immutable information, making it bug-prone when future developers use it.

ykcai-daniel · 2024-09-02T02:29:20Z

@lowang-bh cc @Monokaix Pull request updated. Please review the changes. Thank you!

Monokaix · 2024-09-03T01:42:21Z

pkg/scheduler/api/job_info.go

+
+// ti.SchGated is initialized in NewTaskInfo
+func (ti *TaskInfo) SchedulingGated() bool {
+	return ti.SchGated


Should this filed be exported?

With this function, there is no need to export it. I will change it to schGated.

I would like to use this filed instead of a function calling.

It's an optional way, but seems there is no need to expose an extra filed of task.

I think it's reasonable.

Monokaix · 2024-09-03T01:45:45Z

pkg/scheduler/plugins/capacity/capacity.go

@@ -282,18 +282,18 @@ func (cp *capacityPlugin) OnSessionOpen(ssn *framework.Session) {
 		queue := ssn.Queues[queueID]
 		// If no capability is set, always enqueue the job.
 		if attr.realCapability == nil {
-			klog.V(4).Infof("Capability of queue <%s> was not set, allow job <%s/%s> to Inqueue.",
+			klog.V(3).Infof("Capability of queue <%s> was not set, allow job <%s/%s> to Inqueue.",


Why change these log level?

I changed this log level when debugging the capcacity plugin. Will change is back

Monokaix · 2024-09-03T02:02:33Z

pkg/scheduler/api/job_info.go

+// If resource is less than gated resources, return zero;
+// Gated resources might be more than minResource when total rep of each task > minAvailable
+func (ji *JobInfo) DeductSchGatedResources(res *Resource) *Resource {
+	result := res.Clone()


We can check whether res is zero to have a shortcut here, because in most case task is not gated.

ykcai-daniel · 2024-09-03T03:27:40Z

For more details about the design of the PR, see #3581 . Please also merge the design doc.

Monokaix · 2024-09-03T11:18:32Z

pkg/scheduler/api/job_info.go

+// If resource is less than gated resources, return zero;
+// Note: The purpose of this functionis to deduct the resources of scheduling gated tasks
+// in a job when calculating inqueued resources so that it will not block other jobs from being inqueued.
+func (ji *JobInfo) DeductSchGatedResources(res *Resource) *Resource {


The comment should start with DeductSchGatedResources to be consistent with func name.

Monokaix · 2024-09-03T11:24:55Z

pkg/scheduler/api/job_info.go

+// Note: The purpose of this functionis to deduct the resources of scheduling gated tasks
+// in a job when calculating inqueued resources so that it will not block other jobs from being inqueued.
+func (ji *JobInfo) DeductSchGatedResources(res *Resource) *Resource {
+	result := res.Clone()


Seems there is no need to clone res because no one will also change it, and also we can change to:

schGatedResource := ji.GetSchGatedPodResources() // Most jobs do not have any scheduling gated tasks, hence we add this short cut if schGatedResource.IsEmpty() { return res }

Now, we don't clone res if scheduling gated resource is zero.

pkg/scheduler/api/job_info.go

pkg/scheduler/api/resource_info_test.go

Monokaix · 2024-09-06T06:11:46Z

test/e2e/util/deployment.go

+	return CreateDeploymentGated(ctx, name, rep, img, req, []v1.PodSchedulingGate{})
+}
+
+// CreateDeployment creates a new deployment


CreateDeployment -》 CreateDeploymentGated

Signed-off-by: ykcai-daniel <[email protected]>

Monokaix · 2024-09-11T01:19:58Z

/lgtm
/approve

volcano-sh-bot · 2024-09-11T01:20:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Monokaix]

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

volcano-sh-bot requested review from hudson741, hwdef, kevin-wangzefeng and lowang-bh August 7, 2024 02:00

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2024

googs1025 reviewed Aug 7, 2024

View reviewed changes

pkg/scheduler/api/job_info.go Outdated Show resolved Hide resolved

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

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

ykcai-daniel requested a review from googs1025 August 15, 2024 02:45

ykcai-daniel mentioned this pull request Aug 15, 2024

Support Pod Scheduling Readiness #3555

Closed

ykcai-daniel mentioned this pull request Aug 16, 2024

Proposal for Support of Pod Scheduling Readiness #3581

Merged

volcano-sh-bot assigned Monokaix Aug 30, 2024

volcano-sh-bot assigned googs1025 Aug 30, 2024

volcano-sh-bot assigned hwdef and lowang-bh Aug 30, 2024

lowang-bh reviewed Aug 30, 2024

View reviewed changes

ykcai-daniel requested a review from lowang-bh September 2, 2024 02:25

Monokaix reviewed Sep 3, 2024

View reviewed changes

ykcai-daniel requested a review from Monokaix September 3, 2024 03:28

Monokaix reviewed Sep 3, 2024

View reviewed changes

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

Monokaix reviewed Sep 4, 2024

View reviewed changes

pkg/scheduler/api/job_info.go Outdated Show resolved Hide resolved

ykcai-daniel force-pushed the sch-gates-update branch from 17bbc42 to 5bf6f62 Compare September 6, 2024 01:21

volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 6, 2024

ykcai-daniel force-pushed the sch-gates-update branch 2 times, most recently from 9aee58a to 64b16e1 Compare September 6, 2024 01:36

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 6, 2024

Monokaix reviewed Sep 6, 2024

View reviewed changes

pkg/scheduler/api/resource_info_test.go Outdated Show resolved Hide resolved

ykcai-daniel requested a review from Monokaix September 6, 2024 02:11

ykcai-daniel mentioned this pull request Sep 6, 2024

Add new state SchGated volcano-sh/apis#131

Closed

Monokaix reviewed Sep 6, 2024

View reviewed changes

ykcai-daniel force-pushed the sch-gates-update branch 3 times, most recently from 82cef03 to 6ab5464 Compare September 10, 2024 04:24

support scheduling gates

757c682

Signed-off-by: ykcai-daniel <[email protected]>

ykcai-daniel force-pushed the sch-gates-update branch from 6ab5464 to 757c682 Compare September 10, 2024 13:47

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2024

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2024

volcano-sh-bot merged commit 5d3106d into volcano-sh:master Sep 11, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pod Scheduling Readiness #3658

Pod Scheduling Readiness #3658

ykcai-daniel commented Aug 7, 2024

googs1025 commented Aug 7, 2024 •

edited

Loading

Monokaix commented Aug 8, 2024

Monokaix commented Aug 13, 2024

ykcai-daniel commented Aug 15, 2024

ykcai-daniel commented Aug 15, 2024 •

edited

Loading

ykcai-daniel commented Aug 30, 2024 •

edited

Loading

ykcai-daniel commented Aug 30, 2024 •

edited

Loading

ykcai-daniel commented Aug 30, 2024

Monokaix commented Aug 30, 2024

lowang-bh Aug 30, 2024

ykcai-daniel Sep 2, 2024

lowang-bh Aug 30, 2024

ykcai-daniel Sep 2, 2024

lowang-bh Aug 30, 2024

ykcai-daniel Sep 2, 2024

ykcai-daniel commented Sep 2, 2024

Monokaix Sep 3, 2024

ykcai-daniel Sep 3, 2024

lowang-bh Sep 3, 2024

Monokaix Sep 3, 2024

Monokaix Sep 3, 2024

Monokaix Sep 3, 2024

ykcai-daniel Sep 3, 2024

Monokaix Sep 3, 2024 •

edited

Loading

ykcai-daniel commented Sep 3, 2024

Monokaix Sep 3, 2024

Monokaix Sep 3, 2024

ykcai-daniel Sep 6, 2024

Monokaix Sep 6, 2024

Monokaix commented Sep 11, 2024

volcano-sh-bot commented Sep 11, 2024

Pod Scheduling Readiness #3658

Pod Scheduling Readiness #3658

Conversation

ykcai-daniel commented Aug 7, 2024

googs1025 commented Aug 7, 2024 • edited Loading

Monokaix commented Aug 8, 2024

Monokaix commented Aug 13, 2024

ykcai-daniel commented Aug 15, 2024

ykcai-daniel commented Aug 15, 2024 • edited Loading

ykcai-daniel commented Aug 30, 2024 • edited Loading

ykcai-daniel commented Aug 30, 2024 • edited Loading

ykcai-daniel commented Aug 30, 2024

Monokaix commented Aug 30, 2024

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

ykcai-daniel commented Sep 2, 2024

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

Monokaix Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

ykcai-daniel commented Sep 3, 2024

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

Monokaix commented Sep 11, 2024

volcano-sh-bot commented Sep 11, 2024

googs1025 commented Aug 7, 2024 •

edited

Loading

ykcai-daniel commented Aug 15, 2024 •

edited

Loading

ykcai-daniel commented Aug 30, 2024 •

edited

Loading

ykcai-daniel commented Aug 30, 2024 •

edited

Loading

Monokaix Sep 3, 2024 •

edited

Loading