Use Allocatable to replace Capacity #19083

resouer · 2015-12-24T13:02:08Z

Scheduler should use Allocatable instead of Capacity to make decision. But the general behavior does not need to change.

We should also make sure older kubelet can also work well with this code (use Capacity instead in this case).

k8s-github-robot · 2015-12-24T13:08:41Z

Labelling this PR as size/M

k8s-bot · 2015-12-24T13:35:23Z

GCE e2e test build/test passed for commit 5a04faecbe941564e751071a6e25788c569133d3.

timstclair · 2016-01-04T20:47:23Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

-		glog.V(10).Infof("Cannot schedule Pod %+v, because Node %+v is full, running %v out of %v Pods.", podName(pod), node, len(existingPods), info.Status.Capacity.Pods().Value())
+	var allocatable api.ResourceList
+
+	// In case old version kubelet does not set Allocatable


This shouldn't be necessary. This is already performed when converting to the internal API format, here.

k8s-bot · 2016-01-05T07:30:51Z

GCE e2e test build/test passed for commit 67ca07b3daaeb00dcf45dcada911e8d5abeb70c4.

k8s-bot · 2016-01-05T07:49:34Z

GCE e2e test build/test passed for commit 2d8eac2.

timstclair · 2016-01-07T18:43:49Z

LGTM, but @davidopp should have the final word.

davidopp · 2016-01-09T20:48:26Z

Capacity is also used in
https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/scheduler/algorithm/priorities/priorities.go

Shouldn't the logic in this PR be "if kuebelet reports allocatable, use that, otherwise use capacity" so that it doesn't break with old kubelets?

resouer · 2016-01-11T06:08:11Z

Yeah, it is originally implemented like that, but @timstclair mentioned that default has already deal with that logic. WDYT?

k8s-bot · 2016-01-11T07:32:49Z

GCE e2e test build/test passed for commit e1bda61.

resouer · 2016-01-11T07:53:12Z

@k8s-bot unit test this

timothysc · 2016-01-11T20:05:26Z

@resouer @davidopp Is there an e2e that verifies system behavior?

/cc @ncdc please track on 1.2-candidate.

resouer · 2016-01-12T01:54:01Z

@timothysc Seems not, I will add one

resouer · 2016-01-12T10:13:15Z

@timothysc It just came to me that we can not control NodeStatus in e2e test. So I propose to write a integration test to cover this logic. Agreed?

cc @davidopp

timothysc · 2016-01-12T15:32:11Z

@resouer sgtm

k8s-github-robot · 2016-01-13T10:08:48Z

Labelling this PR as size/L

k8s-github-robot · 2016-01-13T10:25:10Z

Labelling this PR as size/M

k8s-bot · 2016-01-13T10:40:34Z

GCE e2e build/test failed for commit 1a6be7f99554ca9526b88c5cd3b3fe09d3a4d334.

k8s-bot · 2016-01-18T08:24:00Z

GCE e2e test build/test passed for commit 4640d12ca95bd6895c497538902f8eafac5d20c5.

resouer · 2016-01-18T08:41:48Z

ping @davidopp @timothysc updated and rebased, looks better?

timothysc · 2016-01-18T15:09:42Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

-	if int64(len(existingPods))+1 > info.Status.Capacity.Pods().Value() {
-		glog.V(10).Infof("Cannot schedule Pod %+v, because Node %+v is full, running %v out of %v Pods.", podName(pod), node, len(existingPods), info.Status.Capacity.Pods().Value())
+	allocatable := info.Status.Allocatable
+


nix the extra line

timothysc · 2016-01-18T15:29:15Z

minor comments.

davidopp · 2016-01-18T22:14:12Z

test/integration/scheduler_test.go

+		t.Fatalf("Failed to create node: %v", err)
+	}
+
+	// 3. create resource pod which requires more than Allocatable but less than Capacity


"more than Allocatable" doesn't make sense because you are not reporting Allocatable.

Add integration test to verify allocatable can works

k8s-bot · 2016-01-20T08:15:21Z

GCE e2e test build/test passed for commit 0202a20.

resouer · 2016-01-20T08:42:15Z

Fixed based on previous comments

timothysc · 2016-01-20T15:57:49Z

thx for the fixes.
lgtm from my side, @davidopp for final say.

davidopp · 2016-01-20T20:39:12Z

@dchen1107

davidopp · 2016-01-20T20:39:42Z

LGTM

dchen1107 · 2016-01-20T20:59:17Z

LGTM too. Thanks for picking up this.

dchen1107 · 2016-01-20T21:00:32Z

#13984

k8s-github-robot · 2016-01-21T15:06:06Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-01-21T15:42:25Z

GCE e2e test build/test passed for commit 0202a20.

alex-mohr · 2016-01-21T22:56:24Z

@k8s-bot unit test this

Use Allocatable to replace Capacity

dchen1107 · 2016-01-22T00:39:47Z

Super!

dchen1107 · 2016-01-22T00:40:16Z

cc/ @derekwaynecarr too

derekwaynecarr · 2016-01-22T17:04:05Z

Awesome!

On Thursday, January 21, 2016, Dawn Chen notifications@github.com wrote:

cc/ @derekwaynecarr https://github.com/derekwaynecarr too

—
Reply to this email directly or view it on GitHub
#19083 (comment)
.

googlebot added the cla: yes label Dec 24, 2015

k8s-github-robot assigned davidopp Dec 24, 2015

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 24, 2015

timstclair reviewed Jan 4, 2016
View reviewed changes

resouer force-pushed the allocatable branch 2 times, most recently from 67ca07b to 2d8eac2 Compare January 5, 2016 07:21

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 11, 2016

ncdc added this to the v1.2-candidate milestone Jan 12, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2016

resouer force-pushed the allocatable branch from 1a6be7f to e1bda61 Compare January 13, 2016 10:12

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2016

resouer force-pushed the allocatable branch from 1400c09 to 4640d12 Compare January 18, 2016 07:39

timothysc reviewed Jan 18, 2016
View reviewed changes

davidopp reviewed Jan 18, 2016
View reviewed changes

Integration test to verify alloc works

0202a20

Add integration test to verify allocatable can works

resouer force-pushed the allocatable branch from 4640d12 to 0202a20 Compare January 20, 2016 07:36

davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Jan 20, 2016

davidopp modified the milestones: v1.2, v1.2-candidate Jan 20, 2016

alex-mohr added a commit that referenced this pull request Jan 22, 2016

Merge pull request #19083 from resouer/allocatable

76f02d5

Use Allocatable to replace Capacity

alex-mohr merged commit 76f02d5 into kubernetes:master Jan 22, 2016

resouer deleted the allocatable branch January 24, 2016 08:23

Use Allocatable to replace Capacity #19083

Use Allocatable to replace Capacity #19083

Conversation

resouer commented Dec 24, 2015

k8s-github-robot commented Dec 24, 2015

k8s-bot commented Dec 24, 2015

timstclair Jan 4, 2016

Choose a reason for hiding this comment

k8s-bot commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

timstclair commented Jan 7, 2016

davidopp commented Jan 9, 2016

resouer commented Jan 11, 2016

k8s-bot commented Jan 11, 2016

resouer commented Jan 11, 2016

timothysc commented Jan 11, 2016

resouer commented Jan 12, 2016

resouer commented Jan 12, 2016

timothysc commented Jan 12, 2016

k8s-github-robot commented Jan 13, 2016

k8s-github-robot commented Jan 13, 2016

k8s-bot commented Jan 13, 2016

k8s-bot commented Jan 18, 2016

resouer commented Jan 18, 2016

timothysc Jan 18, 2016

Choose a reason for hiding this comment

timothysc commented Jan 18, 2016

davidopp Jan 18, 2016

Choose a reason for hiding this comment

k8s-bot commented Jan 20, 2016

resouer commented Jan 20, 2016

timothysc commented Jan 20, 2016

davidopp commented Jan 20, 2016

davidopp commented Jan 20, 2016

dchen1107 commented Jan 20, 2016

dchen1107 commented Jan 20, 2016

k8s-github-robot commented Jan 21, 2016

k8s-bot commented Jan 21, 2016

alex-mohr commented Jan 21, 2016

dchen1107 commented Jan 22, 2016

dchen1107 commented Jan 22, 2016

derekwaynecarr commented Jan 22, 2016