Take init containers into account when getting pod resource request #638

hex108 · 2019-03-13T03:25:46Z

What this PR does / why we need it:
Take init containers into account when getting pod resource request. The code refers k8s.io/kubernetes/pkg/scheduler/algorithm/predicates/predicates.go#GetResourceRequest.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Take init containers into account when getting pod resource request

k82cn · 2019-03-14T09:21:38Z

/approve

LGTM overall, let's get it merged when CI is happy :)

k8s-ci-robot · 2019-03-14T09:21:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hex108, k82cn

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~~ [k82cn]

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

hex108 · 2019-03-15T01:09:30Z

@k82cn I'll look into it.

I checked the code of k8s scheduler again, and found it only consider init containers' resource request for the predicate PodFitsResource, it will not calculate them when adding it to node's total requested resource. Need we be consistent with this behavior?

Another question: is there any reason to use float64 instead of int64 to record pod resource?

k82cn · 2019-03-15T01:15:19Z

Need we be consistent with this behavior?

Yes; we allocate resources in allocate action so PodFitsResource is not necessary; but initContainer is still necessary :)

Another question: is there any reason to use float64 instead of int64 to record pod resource?

We used to use CPU instead of MillCPU, so float64 is there.

hex108 · 2019-03-15T02:41:49Z

Need we be consistent with this behavior?

Yes; we allocate resources in allocate action so PodFitsResource is not necessary; but initContainer is still necessary :)

Addressed. PTAL.

Another question: is there any reason to use float64 instead of int64 to record pod resource?

We used to use CPU instead of MillCPU, so float64 is there.

Get it. Thanks.

k82cn · 2019-03-15T03:46:39Z

pkg/scheduler/actions/allocate/allocate.go

@@ -141,9 +141,10 @@ func (alloc *allocateAction) Execute(ssn *framework.Session) {
 				}
 			}
 			selectedNodes := util.SelectBestNode(nodeScores)
+			req := api.GetPodResourceRequest(task.Pod)


task.Resreq already includes initContainer, why do we need to re-calculate it again?

task.Resreq is calculated by GetPodResourceWithoutInitContainers, it does not contain initContainer. We only need consider initContainer when Allocate, do I understand it correctly?

We also need Resreq in reclaim, preemption and backfill. And predicates&priorities will use Pods instead of task info :)

Addressed. Thanks!

hm.... can we update task.Resreq directly, so we do not need to update different actions :)

I updated it at first time. However a lot of functions(e.g. jobinfo.AddTaskInfo, NodeInfo.AddTaskInfo) will use task.Resq that does not contain init container. Maybe we could add another variable ResqWithInitContainers in TaskInfo?

~~If so, prefer to update related code to consider initContainer :)~~

hm.... maybe we need to distinguish them. In nodeInfo, it's not necessary to consider initContainer if it's in running state.

k82cn · 2019-03-15T08:57:23Z

pkg/scheduler/api/job_info.go

+	// To be consistent with kubernetes default scheduler, ResreqWithInitContainers is only used for predicates
+	// of actions (e.g.allocate, backfill, preempt, reclaim), please use Resreq for other cases.
+	Resreq                   *Resource
+	ResreqWithInitContainers *Resource


How about InitResreq? The Resreq is the resource that used when task running; and the InitResreq is the resource that used to launch a task.

OK, it is shorter :) Addressed.

k82cn · 2019-03-15T09:05:51Z

/lgtm

k82cn · 2019-03-15T09:29:00Z

pkg/scheduler/api/job_info.go

+		Priority:   1,
+		Pod:        pod,
+		Resreq:     req,
+		InitResreq: initResreq,


we also need to update Clone() :)

Sorry for it, forget to run test. Updated.

hex108 · 2019-03-15T10:12:54Z

/retest

hex108 · 2019-03-15T10:16:04Z

Test errors are because namespace test is not deleted.

k82cn · 2019-03-16T03:22:41Z

/lgtm

…#642-#638-#645-#647-#651-#652-#655-#658-#649-#660-#666-#671-#673-upstream-release-0.4 Automated cherry pick of #643: Return err in Allocate if any error occurs #642: Add event when task is scheduled #638: Take init containers into account when getting pod resource #645: Order task by CreationTimestamp first, then by UID #647: In allocate, skip adding Job if its queue is not found #651: Return err in functions of session.go if any error occurs #652: Change run option SchedulePeriod's type to make it clear #655: Do graceful eviction using default policy #658: Address helm install error in tutorial.md #649: Preempt lowest priority task first #660: Fix sub exception in reclaim and preempt #666: Fix wrong caculation for deserved in proportion plugin #671: Change base image to alphine to reduce image size #673: Do not create PodGroup and Job for task whose scheduler is

Take init containers into account when getting pod resource request

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2019

k8s-ci-robot requested review from k82cn and suleisl2000 March 13, 2019 03:25

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2019

hex108 force-pushed the init_container branch from 12403d7 to 71cdd7b Compare March 13, 2019 03:28

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019

hex108 force-pushed the init_container branch 3 times, most recently from 691f73f to 6de00e0 Compare March 15, 2019 02:40

k82cn reviewed Mar 15, 2019

View reviewed changes

hex108 force-pushed the init_container branch 2 times, most recently from ee19552 to 5bd2487 Compare March 15, 2019 08:44

k82cn reviewed Mar 15, 2019

View reviewed changes

hex108 force-pushed the init_container branch from 5bd2487 to de4eaf8 Compare March 15, 2019 09:02

k8s-ci-robot assigned k82cn Mar 15, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019

k82cn reviewed Mar 15, 2019

View reviewed changes

Take init containers into account when getting pod resource request

73e763e

hex108 force-pushed the init_container branch from de4eaf8 to 73e763e Compare March 15, 2019 09:41

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2019

k8s-ci-robot merged commit 5132fca into kubernetes-retired:master Mar 16, 2019

hex108 deleted the init_container branch March 17, 2019 07:27

kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019

Merge pull request kubernetes-retired#638 from hex108/init_container

4786384

Take init containers into account when getting pod resource request

kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019

Merge pull request kubernetes-retired#638 from hex108/init_container

586cd03

Take init containers into account when getting pod resource request

kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019

Merge pull request kubernetes-retired#638 from hex108/init_container

7c08464

Take init containers into account when getting pod resource request

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take init containers into account when getting pod resource request #638

Take init containers into account when getting pod resource request #638

hex108 commented Mar 13, 2019

k82cn commented Mar 14, 2019

k8s-ci-robot commented Mar 14, 2019

hex108 commented Mar 15, 2019

k82cn commented Mar 15, 2019

hex108 commented Mar 15, 2019

k82cn Mar 15, 2019

hex108 Mar 15, 2019

k82cn Mar 15, 2019

hex108 Mar 15, 2019

k82cn Mar 15, 2019

hex108 Mar 15, 2019

k82cn Mar 15, 2019 •

edited

Loading

k82cn Mar 15, 2019

hex108 Mar 15, 2019

k82cn commented Mar 15, 2019

k82cn Mar 15, 2019

hex108 Mar 15, 2019

hex108 commented Mar 15, 2019

hex108 commented Mar 15, 2019

k82cn commented Mar 16, 2019

Take init containers into account when getting pod resource request #638

Take init containers into account when getting pod resource request #638

Conversation

hex108 commented Mar 13, 2019

k82cn commented Mar 14, 2019

k8s-ci-robot commented Mar 14, 2019

hex108 commented Mar 15, 2019

k82cn commented Mar 15, 2019

hex108 commented Mar 15, 2019

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

k82cn Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hex108 commented Mar 15, 2019

hex108 commented Mar 15, 2019

k82cn commented Mar 16, 2019

k82cn Mar 15, 2019 •

edited

Loading