adding predicate ordering design proposal #1152

yastij · 2017-10-05T17:12:07Z

bsalamat · 2017-10-10T00:08:00Z

contributors/design-proposals/predicates-ordering.md

+
+|Position                  | Predicate                        | comments (notes, justifications...)              |
+ ----------------- | ---------------------------- | ------------------
+| 1 | `CheckNodeConditionPredicate`  | we really don’t wanna check predicates against unschedulable nodes. |


s/wanna/want to/

bsalamat · 2017-10-10T00:17:02Z

contributors/design-proposals/predicates-ordering.md

+## End-user ordering
+
+using scheduling policy file, the cluster admin can override the default static ordering. This gives administrator the maximum flexibility regarding scheduler behaviour and enables scheduler to adapt to cluster usage. as an example the following is scheduler policy file using an end-user ordering:
+


Is order a positive integer?

Could you please also add that when the provided order is the same for two or more predicates, scheduler will determine the order and there is no guarantee that the order remains the same over time.

@bsalamat - indeed you're right

bsalamat · 2017-10-10T00:41:26Z

contributors/design-proposals/predicates-ordering.md

@@ -0,0 +1,90 @@
+# predicates ordering
+


This proposal should go in the scheduling directory.

@bsalamat - indeed

k82cn · 2017-10-10T02:33:58Z

contributors/design-proposals/predicates-ordering.md

+"kind" : "Policy",
+"apiVersion" : "v1",
+"predicates" : [
+	{"name" : "PodFitsHostPorts", "order": 2},


Can we just follow the order in configuration? so "order" is not necessary.

@k82cn - I'd prefer having a clear ordering here, as using an implicit one might confuse users.

I also like the explicit order, but we should be backward compatible. So, absence of "order" should be treated as order: 0. We should mention this in the design doc.

@bsalamat - also maybe add that if there's a non-logical ordering (e.g. order:1, order:2, order:16) default to static ordering with a warning ?

Also I agree that absence of order should be treated as order:0 (i.e. executed at the end & randomly right after the ranked predicates)

What is wrong with 1, 2, 16? It shows a clear ordering and should be treated a valid ordering.

Yes for the second part. Order:0 means executed at no particular order and after the ordered predicates. If none of the predicates has an order, then should run with the default order.

Yes, order must be a positive integer. In case of an invalid value (such as a negative number or a string) we should probably fail to start. Users may miss the warning and not notice the issue in their config. Failing to start is in-line with how scheduler reacts to other config issues.

k82cn · 2017-10-10T02:35:06Z

contributors/design-proposals/predicates-ordering.md

+
+status: proposal
+
+Author: Yassine TIJANI.


please also share your github handle :).

k82cn · 2017-10-10T02:35:19Z

contributors/design-proposals/predicates-ordering.md

+
+
+
+status: proposal


s/status/Status

k82cn · 2017-10-10T02:37:16Z

contributors/design-proposals/predicates-ordering.md

+
+## Abstract
+
+This document describes how and why reordering predicates helps to achieve performance for the kubernetes scheduler. We will expose the motivations behind this proposal, The two steps/solution we see to tackle this problem and the timeline decided to implement these.


There's general suggestion that: build paragraph multiple lines, so reviewers can comment on each line separately.

k82cn · 2017-10-10T03:00:14Z

contributors/design-proposals/predicates-ordering.md

+
+|Position                  | Predicate                        | comments (notes, justifications...)              |
+ ----------------- | ---------------------------- | ------------------
+| 1 | `CheckNodeConditionPredicate`  | we really don’t wanna check predicates against unschedulable nodes. |


@yastij , as we talked on Slack, prefer to also add something like O(n), n= # of node. The user can evaluate the complexity of predicates based on their environment; so the lowest complexity with most restrictive come first.

@k82cn - I thought that time complexity might be more relevant in the user docs, as most of end-user will refer to the documentation when trying to check time complexity, you think we should add it here instead ?

After thinking a bit more about time complexity, it seems that giving an estimation using the number of nodes might be not very useful, since the vast majority of predicates are computed against every node using the parallelized workqueue with a checkNode function.

@bsalamat @k82cn - to your knowledge is there any common input to predicates that can gives us insights on time complexity ?

I don't think there is any way to find runtime complexity of predicates other than examining their algorithms, but the autoscaler team recently ran some tests to find execution times of various predicates. You can find the results here: kubernetes/autoscaler#257. It gives you some idea about the runtime complexity of the predicates.

@bsalamat - thank you, indeed it should be enough

k82cn · 2017-10-11T06:24:29Z

contributors/design-proposals/predicates-ordering.md, line 40 at r1 (raw file):

Previously, yastij (Yassine TIJANI) wrote…

@k82cn - I thought that time complexity might be more relevant in the user docs, as most of end-user will refer to the documentation when trying to check time complexity, you think we should add it here instead ?

I'd suggest to add it; so when anyone draft the user doc, just copy it. And the time complexity will also help us to work out default order of predicates :).

Comments from Reviewable

k82cn · 2017-10-11T06:29:56Z

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions.

contributors/design-proposals/predicates-ordering.md, line 67 at r1 (raw file):

Previously, yastij (Yassine TIJANI) wrote…

SGTM

I'm ok with explicit order; please add enough validation for it, e.g. two predicates with same order.

Comments from Reviewable

gmarek · 2017-10-11T08:30:48Z

contributors/design-proposals/predicates-ordering.md

+
+## Timeline
+
+both types of ordering will be delivered as an alpha version, as for the timeline:


gmarek · 2017-10-11T08:30:57Z

contributors/design-proposals/predicates-ordering.md

+
+## End-user ordering
+
+using scheduling policy file, the cluster admin can override the default static ordering. This gives administrator the maximum flexibility regarding scheduler behaviour and enables scheduler to adapt to cluster usage. as an example the following is scheduler policy file using an end-user ordering:


gmarek · 2017-10-11T08:34:14Z

contributors/design-proposals/predicates-ordering.md

+
+both types of ordering will be delivered as an alpha version, as for the timeline:
+
+* static ordering: available in 1.9


Static ordering is fine to do straight away, as it doesn't change how users use clusters, but I think we probably should discuss putting dynamic ordering initially as beta. I'm not sure if it's worth it - we need to discuss it with SIG PM (whatever their handle is).

@gmarek - I thought that we'd wait for metrics from the community to start working on the dynamic ordering ? are you talking about end-user ordering instead ?

I think it is a good idea to add static ordering first and wait for feedback from users before adding dynamic ordering. If users asked for dynamic ordering, we will add it.

@gmarek @bsalamat - I'm a bit confused here, dynamic ordering isn't part of this design proposal, since we need data on predicates restrictiveness.

I'm fine with just having for static ordering and the end-user one for the moment.

I was referring to configurable ordering by end-users as "dynamic ordering". Do we already know customers who need the configurable ordering?

from my personal experience and what I heared yes. The only way to do it is to write a custom sched. and change the ordering there.

I think we should have it, for the dynamic one (i.e. ordering automatically predicates depending on cluster usage), we should wait that's why it's not part from this design proposal.

I still would like to hear who are the customers who needed it and had to write their own custom scheduler only to reorder predicates and why a smarter ordering (that we are proposing in static ordering of this proposal) is not enough for them.

If you are using very specific set of labels, executing checkNodeLabelPresence first is better.

@gmarek - I'll try to join the next SIG PM meeting or talk to someone from it.

Oh, sorry, if the 'static' means 'at startup' and dynamic means 'changes according to the cluster state' then you're right, I understood it differently. It's probably not worth joining the meeting, as they most likely have full agenda, email should work better.

@apsinha @timothysc - do you know if we should use normal alpha-beta-GA (most likely skipping alpha) lifecycle for things like this (allowing user to change the order of predicate computation in scheduler - setting good one improves scheduler's performance, setting terrible one pretty much kills it). It's small enough change from the user perspective, that maybe we can skip this?

bsalamat · 2017-10-13T18:03:31Z

We discussed the configurable ordering of predicates in the SIG scheduling meeting yesterday. We believe that the configurable ordering shouldn't be implemented at this point. We can go ahead and build the static ordering and leave the configurable ordering util we get more feedback from our customers that they need it.

yastij · 2017-10-16T19:47:21Z

@bsalamat - SGTM

bsalamat · 2017-10-16T20:43:08Z

contributors/design-proposals/predicates-ordering.md

+both types of ordering will be delivered as an alpha version, as for the timeline:
+
+* static ordering: available in 1.9
+* dynamic ordering: available in 1.10


So, could you please change this from 1.10 to "TBD based on customer feedback"?

yastij · 2017-10-19T20:00:37Z

@gmarek @bsalamat @k82cn - sorry guys i was off, I just came back, I updated the design proposal.

Also waiting for feedback from @timothysc and @apsinha on wether static ordering should be delivered as alpha or directly available

bsalamat · 2017-10-20T17:55:57Z

contributors/design-proposals/predicates-ordering.md

@@ -84,7 +88,7 @@ using scheduling policy file, the cluster admin can override the default static

 ## Timeline

-both types of ordering will be delivered as an alpha version, as for the timeline:
+Both types of ordering will be delivered as an alpha version, as for the timeline:


Static ordering of predicates shouldn't have noticeable user impact and we don't expect change of behavior. It is more of an optimization. So, I don't think we need to make it alpha. It can go to production directly.

bsalamat · 2017-10-20T18:42:01Z

LGTM
Let's wait for others to chime in.

yastij · 2017-10-24T12:49:07Z

@gmarek @k82cn - LGTY ?

gmarek · 2017-10-24T13:30:19Z

contributors/design-proposals/predicates-ordering.md

+
+
+
+##Abstract


Ech... it does md in comments as well. Add space between # and A.

gmarek · 2017-10-24T13:31:00Z

contributors/design-proposals/predicates-ordering.md

+We will expose the motivations behind this proposal, The two steps/solution we see to tackle this problem and the timeline decided to implement these.
+
+
+##Motivation


gmarek · 2017-10-24T13:31:06Z

contributors/design-proposals/predicates-ordering.md

+
+
+
+##Static ordering


gmarek · 2017-10-24T13:47:04Z

contributors/design-proposals/predicates-ordering.md

+## End-user ordering
+
+Using scheduling policy file, the cluster admin can override the default static ordering. This gives administrator the maximum flexibility regarding scheduler behaviour and enables scheduler to adapt to cluster usage. 
+Please note that the order must be a positive integer, also, when providing equal ordering for many predicates, scheduler will determine the order and won't guarantee that the order will remain the same between them.


I'm not sure if it's clear for everyone so we should mention that updating the Policy requires scheduler restart.

Ping this comment.

@gmarek @bsalamat - it's already the case when changing predicates on the policy file ?

I think it is worth noting that point again in this doc, just in case someone is not fully aware of that point.

k8s-ci-robot · 2017-10-24T13:49:10Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

yastij · 2017-10-24T13:53:08Z

@gmarek @bsalamat @k82cn - commits squashed and ready for approval

bsalamat · 2017-10-24T20:24:47Z

/lgtm

k8s-github-robot · 2017-10-24T20:27:33Z

Automatic merge from submit-queue.

bgrant0607 · 2017-10-27T23:00:41Z

@yastij @bsalamat Please move this doc to the design-proposals/scheduling subdir. We categorized all the design docs/proposals by SIG over a month ago.

yastij · 2017-10-27T23:30:31Z

@bgrant0607 - Indeed forgot to do it, I'll move it asap, thanks !

@gmarek

…oposal Automatic merge from submit-queue. adding predicate ordering design proposal /cc @gmarek @k82cn @bsalamat

k8s-ci-robot requested review from bsalamat, gmarek and k82cn October 5, 2017 17:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 5, 2017

k8s-github-robot assigned thockin and smarterclayton Oct 5, 2017

thockin assigned bsalamat and unassigned thockin Oct 5, 2017

yastij force-pushed the predicates-ordering-proposal branch from 7fccc1b to 1a4e1a0 Compare October 5, 2017 20:31

bsalamat reviewed Oct 10, 2017

View reviewed changes

k82cn reviewed Oct 10, 2017

View reviewed changes

contributors/design-proposals/predicates-ordering.md Outdated

status: proposal

Copy link

Member

k82cn Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/status/Status

yastij reacted with thumbs up emoji

k82cn reviewed Oct 10, 2017

View reviewed changes

gmarek reviewed Oct 11, 2017

View reviewed changes

yastij mentioned this pull request Oct 12, 2017

Implement predicate ordering for the kubernetes scheduler kubernetes/kubernetes#53812

Closed

bsalamat reviewed Oct 16, 2017

View reviewed changes

bsalamat reviewed Oct 20, 2017

View reviewed changes

gmarek reviewed Oct 24, 2017

View reviewed changes

contributors/design-proposals/predicates-ordering.md Outdated

##Static ordering

Copy link

Contributor

gmarek Oct 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space.

yastij force-pushed the predicates-ordering-proposal branch from 418ea9e to 08ceb2f Compare October 24, 2017 13:46

gmarek reviewed Oct 24, 2017

View reviewed changes

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 24, 2017

yastij force-pushed the predicates-ordering-proposal branch from 295ddea to 5a67cb5 Compare October 24, 2017 13:52

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 24, 2017

yastij force-pushed the predicates-ordering-proposal branch from 5a67cb5 to a370117 Compare October 24, 2017 20:20

adding predicate ordering design proposal

65e5d29

yastij force-pushed the predicates-ordering-proposal branch from a370117 to 65e5d29 Compare October 24, 2017 20:24

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2017

k8s-github-robot merged commit 762e5d4 into kubernetes:master Oct 24, 2017

yastij deleted the predicates-ordering-proposal branch October 24, 2017 21:36

cblecker mentioned this pull request Oct 30, 2017

moving predicates ordering design proposal under scheduling directory #1302

Merged

		## End-user ordering

		using scheduling policy file, the cluster admin can override the default static ordering. This gives administrator the maximum flexibility regarding scheduler behaviour and enables scheduler to adapt to cluster usage. as an example the following is scheduler policy file using an end-user ordering:


		## Abstract

		This document describes how and why reordering predicates helps to achieve performance for the kubernetes scheduler. We will expose the motivations behind this proposal, The two steps/solution we see to tackle this problem and the timeline decided to implement these.


		## Timeline

		both types of ordering will be delivered as an alpha version, as for the timeline:


		both types of ordering will be delivered as an alpha version, as for the timeline:

		* static ordering: available in 1.9

		We will expose the motivations behind this proposal, The two steps/solution we see to tackle this problem and the timeline decided to implement these.


		##Motivation




		##Abstract

adding predicate ordering design proposal #1152

adding predicate ordering design proposal #1152

Conversation

yastij commented Oct 5, 2017

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

bsalamat Oct 10, 2017 • edited Loading

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

k82cn Oct 10, 2017 • edited Loading

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 commented Oct 11, 2017

k82cn commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

yastij Oct 11, 2017 • edited Loading

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

yastij Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Oct 13, 2017

yastij commented Oct 16, 2017

Choose a reason for hiding this comment

yastij commented Oct 19, 2017

Choose a reason for hiding this comment

bsalamat commented Oct 20, 2017

yastij commented Oct 24, 2017

Choose a reason for hiding this comment

Abstract

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

k8s-ci-robot commented Oct 24, 2017

yastij commented Oct 24, 2017

bsalamat commented Oct 24, 2017

k8s-github-robot commented Oct 24, 2017

bgrant0607 commented Oct 27, 2017

yastij commented Oct 27, 2017

bsalamat Oct 10, 2017 •

edited

Loading

k82cn Oct 10, 2017 •

edited

Loading

gmarek Oct 11, 2017 •

edited

Loading

yastij Oct 11, 2017 •

edited

Loading

yastij Oct 11, 2017 •

edited

Loading

yastij Oct 11, 2017 •

edited

Loading