Avoid copying PriorityConfig and SchedulerExtender structs for every node while running priority functions #71722

bsalamat · 2018-12-04T19:41:12Z

What type of PR is this?
/kind cleanup

For lack of a better "kind", I should mark this as a clean-up, but this has caused performance regression in the scheduler.

What this PR does / why we need it:
There has been a performance degradation in the scheduler. We suspect it is caused after #70274. This PR fixes the issue and applies the same fix to several similar existing code with the hope that it fixes the issue.

Which issue(s) this PR fixes:

Fixes #

fixes (hopefully) #71659

Does this PR introduce a user-facing change?:

NONE

/sig scheduling

cc/ @wojtek-t @krzysied

k8s-ci-robot · 2018-12-04T19:41:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat

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:

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

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

wojtek-t · 2018-12-04T20:05:10Z

/lgtm

wojtek-t · 2018-12-04T21:53:47Z

/lgtm

bsalamat · 2018-12-04T23:27:37Z

While avoiding those copies seems to be important, I cannot verify that the PR improves performance.
I ran our scheduler benchmarks (integration tests) with and without this PR and I don't see any meaningful difference:

With this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkScheduling/5000Nodes/1000Pods-12         	   10000	  10530934 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	129.670s
+++ [1204 15:14:35] Cleaning up etcd
+++ [1204 15:14:35] Integration test cleanup complete

Without the PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkScheduling/5000Nodes/1000Pods-12         	   10000	  10317707 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	127.931s
+++ [1204 15:21:42] Cleaning up etcd
+++ [1204 15:21:42] Integration test cleanup complete

fejta-bot · 2018-12-05T01:32:33Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

…ones

bsalamat · 2018-12-05T02:03:58Z

I figured what caused the performance degradation after #70274 and #70892. It was not extra copying, it was the fact that our original code used to run old-style priority functions in parallel to the map-reduce style ones.
#70892 fixed a race condition, but it was running the old-style priority functions and when they finished, it started running the new style ones. The third commit of this PR, restores the original parallelism.

Benchmarks results with the latest version of this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkScheduling/5000Nodes/1000Pods-12         	   10000	   8932769 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	112.951s

krzysied · 2018-12-05T12:49:34Z

pkg/scheduler/core/generic_scheduler.go

-	for i, priorityConfig := range priorityConfigs {
-		if priorityConfig.Reduce == nil {
+	for i := range priorityConfigs {
+		if priorityConfigs[i].Reduce == nil {


Just out of curiosity. There will never be a situation when Function != nil and Reduce != nil, right?

That's right. The Map/Reduce functions are the new style for priority functions. The old-style was using "Function". No priority function is supposed to mix the two and none of our priority functions do that.

bsalamat · 2018-12-05T18:43:20Z

/retest

wojtek-t · 2018-12-05T19:37:42Z

/lgtm

great catch!

misterikkit

/lgtm

fejta-bot · 2018-12-05T21:49:47Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

…22-upstream-release-1.13 Automated cherry pick of #71722 upstream release 1.13

Avoid copying PriorityConfig struct while running priority functions

70f923e

bsalamat added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Dec 4, 2018

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 4, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2018

k8s-ci-robot requested review from k82cn and misterikkit December 4, 2018 19:42

bsalamat removed the request for review from k82cn December 4, 2018 19:43

k8s-ci-robot assigned wojtek-t Dec 4, 2018

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

Avoid copying Extender struct while running priority functions

76591db

bsalamat force-pushed the fix_priorityconfig branch from 04da3e1 to 76591db Compare December 4, 2018 21:51

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

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

Run old-style priority functions in parallel to the map-reduce style …

ece8f42

…ones

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2018

krzysied reviewed Dec 5, 2018

View reviewed changes

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

misterikkit reviewed Dec 5, 2018

View reviewed changes

k8s-ci-robot assigned misterikkit Dec 5, 2018

k8s-ci-robot merged commit 720c102 into kubernetes:master Dec 6, 2018

k8s-ci-robot added a commit that referenced this pull request Dec 7, 2018

Merge pull request #71824 from bsalamat/automated-cherry-pick-of-#717…

198e0f7

…22-upstream-release-1.13 Automated cherry pick of #71722 upstream release 1.13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid copying PriorityConfig and SchedulerExtender structs for every node while running priority functions #71722

Avoid copying PriorityConfig and SchedulerExtender structs for every node while running priority functions #71722

bsalamat commented Dec 4, 2018 •

edited by wojtek-t

k8s-ci-robot commented Dec 4, 2018

wojtek-t commented Dec 4, 2018

wojtek-t commented Dec 4, 2018

bsalamat commented Dec 4, 2018

fejta-bot commented Dec 5, 2018

bsalamat commented Dec 5, 2018

krzysied Dec 5, 2018

bsalamat Dec 5, 2018

bsalamat commented Dec 5, 2018

wojtek-t commented Dec 5, 2018

misterikkit left a comment

fejta-bot commented Dec 5, 2018

Avoid copying PriorityConfig and SchedulerExtender structs for every node while running priority functions #71722

Avoid copying PriorityConfig and SchedulerExtender structs for every node while running priority functions #71722

Conversation

bsalamat commented Dec 4, 2018 • edited by wojtek-t

k8s-ci-robot commented Dec 4, 2018

wojtek-t commented Dec 4, 2018

wojtek-t commented Dec 4, 2018

bsalamat commented Dec 4, 2018

fejta-bot commented Dec 5, 2018

bsalamat commented Dec 5, 2018

krzysied Dec 5, 2018

Choose a reason for hiding this comment

bsalamat Dec 5, 2018

Choose a reason for hiding this comment

bsalamat commented Dec 5, 2018

wojtek-t commented Dec 5, 2018

misterikkit left a comment

Choose a reason for hiding this comment

fejta-bot commented Dec 5, 2018

bsalamat commented Dec 4, 2018 •

edited by wojtek-t