Implementing predicates ordering #57168

yastij · 2017-12-14T01:29:06Z

What this PR does / why we need it: implements predicates ordering for the scheduler

Which issue(s) this PR fixes : Fixes #53812

Special notes for your reviewer:

@bsalamat @gmarek @resouer as discussed on slack, to implement ordering we have to choices:

use a layered approach with a list that indexes the order of the predicates map
change the underlying data structure used to represent a collection of predicates (a map in our case) into a list of predicates objects.
Going with this solution might be "cleaner" but it will require a lot of changes and will increase the cost for accessing predicates from O(1) to O(n) (n being the number of predicates used by the scheduler).

we might go with this solution for now. If the number of predicates start growing, we might switch to the second option.

Release note:

adding predicates ordering for the kubernetes scheduler.

yastij · 2017-12-14T01:29:31Z

/assign @bsalamat @gmarek @resouer

bsalamat

Thanks, @yastij

bsalamat · 2017-12-14T08:40:14Z

plugin/pkg/scheduler/core/generic_scheduler.go

-				}
+		for _, predicateKey := range predicates.getPredicatesOrdering() {
+			//TODO (yastij) : compute average predicate restrictiveness to export it as promethus metric
+			if predicate := predicateFuncs[predicateKey]; predicate != nil {


You should also check if the key exists in the map.

yup i'll check the returned bool instead.

bsalamat · 2017-12-14T08:41:01Z

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

@@ -79,6 +79,16 @@ const (
 // For example:
 // https://github.com/kubernetes/kubernetes/blob/36a218e/plugin/pkg/scheduler/factory/factory.go#L422

+var (
+	predicatesOrdering = []string{"CheckNodeCondition",


It would be great to use constants instead of string literals here.

Yes that is the plan.

bsalamat · 2017-12-14T08:43:04Z

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

@@ -79,6 +79,16 @@ const (
 // For example:
 // https://github.com/kubernetes/kubernetes/blob/36a218e/plugin/pkg/scheduler/factory/factory.go#L422

+var (
+	predicatesOrdering = []string{"CheckNodeCondition",


We should also add a comment that this list must be updated when new predicates are added.

yastij · 2017-12-14T11:32:03Z

@bsalamat thanks for the review, updated this PR.

dims · 2017-12-14T13:44:54Z

/ok-to-test

@yastij i have triggered the tests ... you probably should squash the commits

yastij · 2017-12-14T14:14:22Z

@dims - can you add it to the 1.10 milestone ?

dims · 2017-12-14T14:38:37Z

@yastij i don't have enough karma to do that :) but you should be good. the milestones kick on only late in the cycle. so stuff that is not marked but that have /lgtm and /approve without any milestones will still merge in master

yastij · 2017-12-14T14:56:16Z

@bsalamat - some tests might fail, maybe we should add a way to append a predicate name for test purpose ?

bsalamat · 2017-12-14T21:55:26Z

@yastij It is fine to add a way for tests to add their predicates.

yastij · 2017-12-14T22:05:20Z

SGTM

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

resouer · 2017-12-15T19:19:34Z

@yastij I think you can discuss this during sig-scheduling meeting to see if it's a proper candidate for 1.10 release

bsalamat · 2017-12-15T23:07:36Z

/sig scheduling

yastij · 2017-12-16T00:44:20Z

@resouer - I'll be there to discuss this + some other items

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

plugin/pkg/scheduler/scheduler_test.go

bsalamat · 2017-12-19T19:44:25Z

/lgtm

Thanks, @yastij!

yastij · 2017-12-19T19:46:14Z

/retest

fejta-bot · 2017-12-19T22:18:35Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

resouer · 2017-12-19T22:53:14Z

The test failure should be related, I will help to look at it later today

yastij · 2017-12-19T23:00:16Z

@resouer - can you undo the lgtm to avoid spam by the bot ?

resouer · 2017-12-19T23:14:10Z

/lgtm cancel

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

yastij · 2017-12-20T01:01:01Z

@resouer - the only way I can think of is to expose ordering through policy file, otherwise we have to hardcode ordering in our code. But as said @bsalamat we might want to wait and get feedback before doing it

bsalamat · 2017-12-20T01:15:06Z

@resouer - the only way I can think of is to expose ordering through policy file, otherwise we have to hardcode ordering in our code. But as said @bsalamat we might want to wait and get feedback before doing it

Making the ordering configurable is certainly possible, but as we had discussed in the past, it may have limited usage. We should wait and see if there is any request to add the feature.

resouer · 2017-12-20T01:42:44Z

Slack talked with @yastij , the CI failure has been captured and will be fixed soon, really appreciate his effort!

yastij · 2017-12-20T14:26:33Z

@resouer @bsalamat - CI all green, ready to ship. Thanks everyone !

dims · 2017-12-20T14:57:45Z

reapplying lgtm

/lgtm

k8s-ci-robot · 2017-12-20T14:57:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, dims, yastij

Associated issue: #53812

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

Needs approval from an approver in each of these OWNERS Files:

~~plugin/pkg/scheduler/OWNERS~~ [bsalamat]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-12-20T15:48:33Z

Automatic merge from submit-queue (batch tested with PRs 57252, 57168). If you want to cherry-pick this change to another branch, please follow the instructions here.

@bsalamat

Automatic merge from submit-queue (batch tested with PRs 61963, 64279, 64130, 64125, 64049). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Fix TestSchedulerWithVolumeBinding to avoid setting predicate ordering. It is causing data race condition as predicate ordering is changing global variable `predicatesOrdering`. Infact this test does not require any special predicate order and should work on default predicate ordering as far as VolumeScheduling feature is enabled. See these logs: ``` ================== ================== WARNING: DATA RACE Read at 0x00c420894180 by goroutine 156: k8s.io/kubernetes/pkg/scheduler/core.podFitsOnNode() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/core/generic_scheduler.go:503 +0xbb k8s.io/kubernetes/pkg/scheduler/core.(*genericScheduler).findNodesThatFit.func1() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/core/generic_scheduler.go:353 +0x2f0 k8s.io/kubernetes/vendor/k8s.io/client-go/util/workqueue.Parallelize.func1() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/vendor/k8s.io/client-go/util/workqueue/parallelizer.go:47 +0xa3 Previous write at 0x00c420894180 by goroutine 186: k8s.io/kubernetes/pkg/scheduler.TestSchedulerWithVolumeBinding() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/scheduler_test.go:663 +0x71 testing.tRunner() /usr/lib/golang/src/testing/testing.go:777 +0x16d Goroutine 156 (running) created at: k8s.io/kubernetes/vendor/k8s.io/client-go/util/workqueue.Parallelize() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/vendor/k8s.io/client-go/util/workqueue/parallelizer.go:43 +0x139 k8s.io/kubernetes/pkg/scheduler/core.(*genericScheduler).findNodesThatFit() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/core/generic_scheduler.go:378 +0xe8a k8s.io/kubernetes/pkg/scheduler/core.(*genericScheduler).Schedule() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/core/generic_scheduler.go:131 +0x385 k8s.io/kubernetes/pkg/scheduler.(*Scheduler).schedule() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/scheduler.go:192 +0xcd k8s.io/kubernetes/pkg/scheduler.(*Scheduler).scheduleOne() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/scheduler.go:447 +0x598 k8s.io/kubernetes/pkg/scheduler.(*Scheduler).(k8s.io/kubernetes/pkg/scheduler.scheduleOne)-fm() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/pkg/scheduler/scheduler.go:182 +0x41 k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133 +0x61 k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134 +0xcd k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.Until() /home/avagarwa/upstream-code/gocode/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88 +0x5a Goroutine 186 (running) created at: testing.(*T).Run() /usr/lib/golang/src/testing/testing.go:824 +0x564 testing.runTests.func1() /usr/lib/golang/src/testing/testing.go:1063 +0xa4 testing.tRunner() /usr/lib/golang/src/testing/testing.go:777 +0x16d testing.runTests() /usr/lib/golang/src/testing/testing.go:1061 +0x4e1 testing.(*M).Run() /usr/lib/golang/src/testing/testing.go:978 +0x2cd main.main() _testmain.go:52 +0x22a ================== --- FAIL: TestSchedulerWithVolumeBinding (18.04s) testing.go:730: race detected during execution of test FAIL ``` It is pretty easy to reproduce this race by following these steps: ``` cd pkg/scheduler go test -c -race stress -p 100 ./scheduler.test ``` Predicate ordering to this unit test was added here: #57168 Since the whole scheduler instance uses just one ordering at time, not sure what is the advantage. @kubernetes/sig-scheduling-bugs @bsalamat @k82cn @frobware @smarterclayton @sjenning ```release-note None ```

k8s-ci-robot assigned bsalamat, gmarek and resouer Dec 14, 2017

bsalamat reviewed Dec 14, 2017

View reviewed changes

yastij changed the title ~~WIP: Implementing predicates ordering~~ Implementing predicates ordering Dec 14, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 14, 2017

yastij force-pushed the predicates-ordering branch from 5d43295 to 11d02e1 Compare December 14, 2017 14:11

resouer reviewed Dec 14, 2017

View reviewed changes

plugin/pkg/scheduler/algorithm/predicates/predicates.go Outdated Show resolved Hide resolved

resouer reviewed Dec 14, 2017

View reviewed changes

plugin/pkg/scheduler/algorithm/predicates/predicates.go Outdated Show resolved Hide resolved

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Dec 15, 2017

yastij force-pushed the predicates-ordering branch from 11d02e1 to 6cd80be Compare December 16, 2017 14:37

k8s-ci-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 Dec 16, 2017

yastij force-pushed the predicates-ordering branch from 3e62663 to 96b64f8 Compare December 19, 2017 16:40

bsalamat reviewed Dec 19, 2017

View reviewed changes

plugin/pkg/scheduler/algorithm/predicates/predicates.go Outdated Show resolved Hide resolved

plugin/pkg/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

yastij force-pushed the predicates-ordering branch from 96b64f8 to 2daff08 Compare December 19, 2017 19:08

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 19, 2017

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 19, 2017

resouer reviewed Dec 20, 2017

View reviewed changes

plugin/pkg/scheduler/algorithm/predicates/predicates.go Show resolved Hide resolved

using consts to refer to predicate names

e62952d

yastij force-pushed the predicates-ordering branch from 2daff08 to e62952d Compare December 20, 2017 13:21

k8s-ci-robot assigned dims Dec 20, 2017

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

k8s-github-robot merged commit 51fbd6e into kubernetes:master Dec 20, 2017

resouer mentioned this pull request Jan 11, 2018

Fitler out nodes in batch when scheduling pods #50419

Closed

yastij deleted the predicates-ordering branch January 11, 2018 18:24

aveshagarwal mentioned this pull request May 18, 2018

Fix TestSchedulerWithVolumeBinding to avoid setting predicate ordering. #64049

Merged

hex108 mentioned this pull request Oct 21, 2019

Keep predicates ordering in scheduler filter plugins? #84155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing predicates ordering #57168

Implementing predicates ordering #57168

yastij commented Dec 14, 2017

yastij commented Dec 14, 2017

bsalamat left a comment

bsalamat Dec 14, 2017

yastij Dec 14, 2017

bsalamat Dec 14, 2017

yastij Dec 14, 2017

bsalamat Dec 14, 2017

yastij Dec 14, 2017

yastij commented Dec 14, 2017

dims commented Dec 14, 2017

yastij commented Dec 14, 2017

dims commented Dec 14, 2017

yastij commented Dec 14, 2017

bsalamat commented Dec 14, 2017

yastij commented Dec 14, 2017

resouer commented Dec 15, 2017

bsalamat commented Dec 15, 2017

yastij commented Dec 16, 2017

bsalamat commented Dec 19, 2017

yastij commented Dec 19, 2017

fejta-bot commented Dec 19, 2017

resouer commented Dec 19, 2017

yastij commented Dec 19, 2017

resouer commented Dec 19, 2017

yastij commented Dec 20, 2017

bsalamat commented Dec 20, 2017

resouer commented Dec 20, 2017 •

edited

yastij commented Dec 20, 2017

dims commented Dec 20, 2017

k8s-ci-robot commented Dec 20, 2017

k8s-github-robot commented Dec 20, 2017

Implementing predicates ordering #57168

Implementing predicates ordering #57168

Conversation

yastij commented Dec 14, 2017

yastij commented Dec 14, 2017

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat Dec 14, 2017

Choose a reason for hiding this comment

yastij Dec 14, 2017

Choose a reason for hiding this comment

bsalamat Dec 14, 2017

Choose a reason for hiding this comment

yastij Dec 14, 2017

Choose a reason for hiding this comment

bsalamat Dec 14, 2017

Choose a reason for hiding this comment

yastij Dec 14, 2017

Choose a reason for hiding this comment

yastij commented Dec 14, 2017

dims commented Dec 14, 2017

yastij commented Dec 14, 2017

dims commented Dec 14, 2017

yastij commented Dec 14, 2017

bsalamat commented Dec 14, 2017

yastij commented Dec 14, 2017

resouer commented Dec 15, 2017

bsalamat commented Dec 15, 2017

yastij commented Dec 16, 2017

bsalamat commented Dec 19, 2017

yastij commented Dec 19, 2017

fejta-bot commented Dec 19, 2017

resouer commented Dec 19, 2017

yastij commented Dec 19, 2017

resouer commented Dec 19, 2017

yastij commented Dec 20, 2017

bsalamat commented Dec 20, 2017

resouer commented Dec 20, 2017 • edited

yastij commented Dec 20, 2017

dims commented Dec 20, 2017

k8s-ci-robot commented Dec 20, 2017

k8s-github-robot commented Dec 20, 2017

resouer commented Dec 20, 2017 •

edited