Introduce node memory pressure condition to scheduler #25531

ingvagabund · 2016-05-12T12:07:37Z

Following the work done by @derekwaynecarr at #21274, introducing memory pressure predicate for scheduler.

Missing:

write down unit-test
test the implementation

At the moment this is a heads up for further discussion how the new node's memory pressure condition should be handled in the generic scheduler.

Additional info

Based on [1], only best effort pods are subject to filtering.
Based on [2], best effort pods are those pods "iff requests & limits are not specified for any resource across all containers".

[1] https://github.com/derekwaynecarr/kubernetes/blob/542668cc7998fe0acb315a43731e1f45ecdcc85b/docs/proposals/kubelet-eviction.md#scheduler
[2] #14943

derekwaynecarr · 2016-05-12T14:25:26Z

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

+func isPodBestEffort(pod *api.Pod) bool {
+	// A Pod is BestEffort iff requests & limits are not specified
+	// for any resource across all containers.
+	for _, container := range pod.Spec.Containers {


you can just use: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/qos/util/qos.go#L52

I was pretty sure it has to be already implemented ;-) Thanks for the link. Updated.

derekwaynecarr · 2016-05-12T14:27:12Z

/cc @vishh - we will add disk pressure as well once that condition is available.

@ingvagabund - I will notify you as I push updates to the referenced PRs, I am hoping after today it just goes down to 1 commit this PR would carry after I do a final round of cleanups (assuming merge queue merges stuff).

derekwaynecarr · 2016-05-14T15:37:55Z

@ingvagabund - the out of resource killing pr is down to 1 commit, so you should be able to rebase your pr relative to that so its easier for folks to follow. #21274

davidopp · 2016-05-16T00:22:59Z

plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go

@@ -111,6 +111,9 @@ func init() {
 			Weight: 1,
 		},
 	)
+
+	// Fit is determined by node memory pressure condition.
+	factory.RegisterFitPredicate("NodeMemoryPressure", predicates.NodeMemoryPressurePredicate)


You should either put this under defaultPredicates() or GeneralPredicates(). Putting it where you did, it is disabled by default.

Put in defaultPredicates() if you only want it to run in the scheduler; put it in GeneralPredicates() if you want it to run in the scheduler and kubelet admission controller. (Only put it in one or the other, not both.)

I was wondering what should be the best place to put it in and if it should be run by default. Thinking about it for the second time, it should be as the kubelet will evict all pods by default if its node is under memory pressure. @derekwaynecarr proof me wrong if we need the predicate in the kubelet admission controller. The predicate is mainly for excluding the node from scheduling best effort pods. So scheduler should be enough.

scheduler should be enough.

davidopp · 2016-05-16T00:26:36Z

Please add a test for the new scheduler predicate.

derekwaynecarr · 2016-05-16T14:11:58Z

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

+	return qosutil.GetPodQos(pod) == qosutil.BestEffort
+}
+
+// MemoryPressurePredicate checks if a pod can be scheduled on a node


s/MemoryPressurePredicate/CheckNodeMemoryPressurePredicate

ingvagabund · 2016-05-19T08:06:27Z

@davidopp PTAL

derekwaynecarr · 2016-05-19T14:40:48Z

the code lgtm, will let @davidopp take a pass. this needs to get in before code freeze since eviction is now enabled on node.

derekwaynecarr · 2016-05-20T14:31:56Z

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

@@ -2641,3 +2641,118 @@ func TestPodToleratesTaints(t *testing.T) {
 		}
 	}
 }
+
+func makeNonBestEffortResources(milliCPU int64, memory int64) api.ResourceList {


nit: just call this getResourceList ?

see an example: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/resourcequota/resource_quota_controller_test.go#L33

There is nothing here that actually guaranteed its not best effort since you can pass 0 for both values.

The predicate_test.go defines makeAllocatableResources function which I can use. I don't know why I did not use that before.

derekwaynecarr · 2016-05-20T14:34:25Z

@ingvagabund - just had two minor nits if you want to fix them up...

davidopp · 2016-05-21T22:33:56Z

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

+			pod:      bestEffortPod,
+			nodeInfo: makeEmptyNodeInfo(memoryPressureNode),
+			fits:     false,
+			name:     "best-effort pod schedulable on node with memory pressure condition on",


s/schedulable/not schedulable/

… pods for nodes that reports memory pressury. Introduce unit-test for CheckNodeMemoryPressurePredicate Following work done in kubernetes#14943

davidopp · 2016-05-22T00:26:37Z

LGTM

k8s-bot · 2016-05-22T02:11:52Z

GCE e2e build/test passed for commit b95b30b.

k8s-github-robot · 2016-05-22T02:37:15Z

Automatic merge from submit-queue

@derekwaynecarr

…-pressure-to-scheduler Automatic merge from submit-queue Introduce node memory pressure condition to scheduler Following the work done by @derekwaynecarr at kubernetes#21274, introducing memory pressure predicate for scheduler. Missing: * write down unit-test * test the implementation At the moment this is a heads up for further discussion how the new node's memory pressure condition should be handled in the generic scheduler. **Additional info** * Based on [1], only best effort pods are subject to filtering. * Based on [2], best effort pods are those pods "iff requests & limits are not specified for any resource across all containers". [1] https://github.com/derekwaynecarr/kubernetes/blob/542668cc7998fe0acb315a43731e1f45ecdcc85b/docs/proposals/kubelet-eviction.md#scheduler [2] kubernetes#14943

googlebot added the cla: yes label May 12, 2016

ingvagabund changed the title ~~WIP: Introduce memory pressure to scheduler~~ WIP: Introduce node memory pressure condition to scheduler May 12, 2016

k8s-github-robot assigned erictune May 12, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 12, 2016

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from 0a0591e to b8f551d Compare May 12, 2016 12:17

derekwaynecarr reviewed May 12, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2016

erictune assigned davidopp and unassigned erictune May 12, 2016

davidopp reviewed May 16, 2016
View reviewed changes

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from b8f551d to d24a3ef Compare May 16, 2016 10:17

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels May 16, 2016

derekwaynecarr reviewed May 16, 2016
View reviewed changes

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from 63c1b1d to 14a81c9 Compare May 18, 2016 11:48

ingvagabund changed the title ~~WIP: Introduce node memory pressure condition to scheduler~~ Introduce node memory pressure condition to scheduler May 18, 2016

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch 3 times, most recently from 45bbe0d to 3126caa Compare May 18, 2016 19:29

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from 3126caa to af1debb Compare May 18, 2016 20:36

k8s-github-robot added the needs-ok-to-merge label May 18, 2016

vishh added this to the v1.3 milestone May 18, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from af1debb to 2db76a2 Compare May 19, 2016 06:53

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2016

derekwaynecarr added ok-to-merge release-note-none Denotes a PR that doesn't merit a release note. and removed needs-ok-to-merge labels May 19, 2016

derekwaynecarr reviewed May 20, 2016
View reviewed changes

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from 2db76a2 to 55b92f1 Compare May 21, 2016 15:32

davidopp reviewed May 21, 2016
View reviewed changes

Scheduler: introduce CheckNodeMemoryPressurePredicate, don't schedule…

b95b30b

… pods for nodes that reports memory pressury. Introduce unit-test for CheckNodeMemoryPressurePredicate Following work done in kubernetes#14943

ingvagabund force-pushed the introduce-memory-pressure-to-scheduler branch from 55b92f1 to b95b30b Compare May 21, 2016 22:40

davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 22, 2016

k8s-github-robot merged commit c17465b into kubernetes:master May 22, 2016

ingvagabund deleted the introduce-memory-pressure-to-scheduler branch May 22, 2016 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce node memory pressure condition to scheduler #25531

Introduce node memory pressure condition to scheduler #25531

ingvagabund commented May 12, 2016 •

edited

Loading

derekwaynecarr May 12, 2016

vishh May 12, 2016

ingvagabund May 16, 2016

derekwaynecarr commented May 12, 2016

derekwaynecarr commented May 14, 2016

davidopp May 16, 2016 •

edited

Loading

ingvagabund May 16, 2016 •

edited

Loading

derekwaynecarr May 16, 2016

davidopp commented May 16, 2016

derekwaynecarr May 16, 2016

ingvagabund commented May 19, 2016

derekwaynecarr commented May 19, 2016

derekwaynecarr May 20, 2016

ingvagabund May 21, 2016

derekwaynecarr commented May 20, 2016

davidopp May 21, 2016

ingvagabund May 21, 2016

davidopp commented May 22, 2016

k8s-bot commented May 22, 2016

k8s-github-robot commented May 22, 2016

Introduce node memory pressure condition to scheduler #25531

Introduce node memory pressure condition to scheduler #25531

Conversation

ingvagabund commented May 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented May 12, 2016

derekwaynecarr commented May 14, 2016

davidopp May 16, 2016 • edited Loading

Choose a reason for hiding this comment

ingvagabund May 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp commented May 16, 2016

Choose a reason for hiding this comment

ingvagabund commented May 19, 2016

derekwaynecarr commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp commented May 22, 2016

k8s-bot commented May 22, 2016

k8s-github-robot commented May 22, 2016

ingvagabund commented May 12, 2016 •

edited

Loading

davidopp May 16, 2016 •

edited

Loading

ingvagabund May 16, 2016 •

edited

Loading