Re-architecture of scheduler perf tests to make them more extendable #44770

ravisantoshgudimetla · 2017-04-21T16:09:17Z

What this PR does / why we need it:

Special notes for your reviewer:
This is for re-architecture of scheduler, so that we can enable or disable certain predicates and priorities and see their impact.

Release note:

Scheduler perf modular extensions.

k8s-reviewable · 2017-04-21T16:09:23Z

This change is

k8s-ci-robot · 2017-04-21T16:09:24Z

Hi @ravisantoshgudimetla. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

ravisantoshgudimetla · 2017-04-21T18:01:31Z

/cc @jayunit100

jayunit100

@gmarek this mostly looks good to me I have some nits . This implements the changes discussed with the exception of a couple of minor details (like taint predicates and force limitation to 1 logical operation) .

Notwithstanding a formal review I think it's basically exactly what we want. Ravi is already working on the next iteration but we thought we should get this out first since it's backwards compatible and not adding any new features .

gmarek · 2017-04-24T08:04:08Z

test/integration/scheduler_perf/scheduler_perf_types.go

+// High Level Configuration for all predicates and priorities.
+type schedulerPerfConfig struct {
+	NodeAffinity nodeAffinity
+	InterpodAffinity interpodAffinity


InterPodAffinity, or just PodAffinity

agree InterPodAffinity

gmarek · 2017-04-24T08:07:02Z

test/integration/scheduler_perf/scheduler_test.go

 type testConfig struct {
-	numPods                   int
-	numNodes                  int
+	// Note: We don't need numPods, numNodes anymore in this struct but keeping them for backward compatibility


Where numNodes got replaced, i.e. how you set number of Nodes created? From what I can tell it's still used by the mutate function.

So, the idea is to push these values into schedulerPerfConfig struct, so that there will be a single entry point into code base. Once we have them in that struct, we can remove them from the testConfig struct.(This will have fields that are specific to internal data representation like nodeStrategies etc.)

I don't think it's a good idea, but I don't care about having the comment for now;)

gmarek · 2017-04-24T08:07:56Z

test/integration/scheduler_perf/test-performance.sh

@@ -48,5 +48,6 @@ if ${RUN_BENCHMARK:-false}; then
 fi
 # Running density tests. It might take a long time.
 kube::log::status "performance test (density) start"
-go test -test.run=. -test.timeout=60m -test.short=false
+go test -test.timeout=60m -test.short=false -test.run=.


Is there a reason for this change?

Nothing. I was testing with few other changes and this got pushed. No need to touch this file as such. I will remove it.

gmarek · 2017-04-24T08:09:41Z

test/integration/scheduler_perf/scheduler_perf_types.go

+
+// nodeAffinity priority configuration details.
+type nodeAffinity struct {
+	Enabled         bool   //If not enabled, node affinity is disabled.


I'd just use a pointer in higher level struct instead.

For that matter interpret nil as "false"? That way no coupling of high struct to predicate/prio names

Yup. We do it in number of places in the codebase.

makes sense going with nil. I will make that change.

gmarek · 2017-04-24T08:10:09Z

test/integration/scheduler_perf/scheduler_perf_types.go

+// nodeAffinity priority configuration details.
+type nodeAffinity struct {
+	Enabled         bool   //If not enabled, node affinity is disabled.
+	numGroups       int    // the % of nodes and pods that should match. Higher # -> smaller performance deficit at scale.


I have no idea how this comment matches this variable.

These comments were originally my idea based on internal discussions, I agree they have less useful context here though... Ravi any ideas ? .md file or struct header comment?

A README.md would be a better place, I guess.

gmarek · 2017-04-24T08:10:17Z

test/integration/scheduler_perf/scheduler_perf_types.go

+type nodeAffinity struct {
+	Enabled         bool   //If not enabled, node affinity is disabled.
+	numGroups       int    // the % of nodes and pods that should match. Higher # -> smaller performance deficit at scale.
+	nodeAffinityKey string // the number of labels needed to match.  Higher # -> larger performance deficit at scale.


Same as above.

gmarek · 2017-04-24T08:12:57Z

test/integration/scheduler_perf/scheduler_perf_types.go

+)
+
+// High Level Configuration for all predicates and priorities.
+type schedulerPerfConfig struct {


This is pretty non-extensible design - you allow only one NodeAffinity, and one PodAffinity. It may be fine, but it may bite us in the future as well.

agree we could have multiple affinities. Are you suggesting a slice ? That's fine by me and not a major change to implement

I think we could, via composition, compose multiple schedulerPerfConfigs to support scenarios where there are multiple competing node affinities in the future. Either way i think leaving as-is for now is good for the first iteration: We can expand it further in the future, and this is a step forward from what is already there... Is that ok with you @gmarek ?

Composing multiple schedulerPerfConfigs doesn't sound too natural to me. It's probably better to have a slice here if you consider having multiple affinities in future (but this is actually hard problem, I don't really know how to solve correctly).

Generally it'd be great if you could discuss this config with @jeremyeder and @sjug.

Yeah composition is awkward once we have a 1->N relationship between multiple pod+node mutations, but i think the slice is also awkward since we arent sure how we want to distribute label 'intersections' yet.. sebastian isn't currently experimenting deeply with scheduler predicates per internal conv.

so, imo, There isnt much value in adding more cardinality to the nodeAffinity when we're only supporting one filter for now. i guess we will move forward and make handling the more complex scenarios our next priority for the improvements.

gmarek · 2017-04-24T08:13:33Z

test/integration/scheduler_perf/scheduler_perf_types.go

+// interpodAffinity priority configuration details.
+type interpodAffinity struct {
+	Enabled     bool
+	Operator    metav1.LabelSelectorOperator


I have no idea what the Operator is in the context of Affinity. Is this together with affinityKey just a way to implement NodeSelector? Why not include it explicitly? Plus I believe it makes no sense in PodAffinity, as it's used only in the NodeAffinity if I'm not mistaken.

Even for pod affinity, we need a label selector. So I guess, we need operator. I was looking at https://raw.githubusercontent.com/kubernetes/kubernetes.github.io/master/docs/concepts/configuration/pod-with-pod-affinity.yaml. Please let me know, if it is not needed.

Then you just need LabelSelector. No need to inlining it's definition here.

gmarek · 2017-04-24T08:21:45Z

test/integration/scheduler_perf/scheduler_test.go

+}
+
+// readInConfiguration reads the input parameters for every predicate and priority that are needed for test run.
+// TODO: As of now, returning configs hardcoded, need to read from yaml or some other file.


No. First figure out how this should work when it's done, then we can discuss what part of this is needed for MVP. I want to see an interface first. We certainly don't want to loose ability to run tests without affinity, etc.

I don't think readInConfig to be necessary ; these tests can be extended easily by an engineer by simply writing a new test with a struct in it .

To be clear ; I originally figured we should have default behavior for readInConfig ... but am now thinking it really should be moved out to be done later (or maybe never ;)) ; good point that the MVP shouldn't hint at a user interface .

gmarek · 2017-04-24T08:28:59Z

test/integration/scheduler_perf/scheduler_test.go

@@ -44,8 +39,10 @@ func TestSchedule100Node3KPods(t *testing.T) {
 	if testing.Short() {
 		t.Skip("Skipping because we want to run short tests")
 	}
-
-	config := defaultSchedulerBenchmarkConfig(100, 3000)
+	config := baseConfig()


Instead of removing default... function maybe just re-implement it with those 3 lines?

gmarek · 2017-04-24T08:35:37Z

@jayunit100 @ravisantoshgudimetla - I have mostly comments to the API, rest looks more or less OK-ish. Thing I'd like to see would be actually handling inputs somehow + fixing the API. Those are somewhat coupled, as we need to understand what knobs there'll be to know how to expose them.

jayunit100 · 2017-04-24T09:14:03Z

test/integration/scheduler_perf/scheduler_test.go

 	nodePreparer              testutils.TestNodePreparer
 	podCreator                *testutils.TestPodCreator
 	schedulerSupportFunctions scheduler.Configurator
 	destroyFunc               func()
 }

+//  baseConfig returns a minimal testConfig to be customized be different tests.


jayunit100 · 2017-04-24T09:17:41Z

test/integration/scheduler_perf/scheduler_test.go

+
+
+
+// parseInput reads a configuration and then applies it to a test configuration.


I think this function isn't necessaey

jayunit100 · 2017-04-24T09:24:14Z

@gmarek what kind of an interface did you want to see... one describing the mutation operations? or an interface for configuration that could be sent to a run() testing stub?

I see potential value in both but just curious what your looking for .

gmarek · 2017-04-24T11:10:59Z

By interface I meant API:). Is this somehow connected with cluster-loader? It's trying to accomplish similar thing (i.e. specify how to load the cluster). @sjug @jeremyeder

sjug · 2017-04-24T18:14:47Z

@gmarek @jayunit100 and I need to discuss further to see how it would be best to integrate scheduler-perf, I do like the concept.

xiang90 · 2017-04-24T18:23:27Z

test/integration/scheduler_perf/scheduler_test.go

+	}
+	return
+}
+


remove extra empty lines here?

Done. Thanx.

ravisantoshgudimetla · 2017-04-25T01:19:13Z

@gmarek @jayunit100 - This is ready for another round of review. I have addressed most of your comments.

gmarek · 2017-04-25T09:08:33Z

One comment, but except that it looks fine-ish, as long as you can come up with some consistent API with cluster loader @sjug @jeremyeder.

jayunit100 · 2017-04-25T12:44:21Z

@gmarek maybe cluster-loader could be able to leverage the predicate definitions structure as a mechansim for specifying and building its own pods ? Is that what your suggesting? That is a clean and decoupled way to share APIs I guess.

gmarek · 2017-04-25T13:09:16Z

No, it's not about decoupling. It's that we're creating to separate APIs to define how to put some load on the cluster. Of course cluster Loader is more complex, as it needs to handle way more stuff, but on a high level it does the same thing. I believe there can be some common ground here, but if both you and CL people decide that there's no point in working together then I'm fine with what's there now (modulo one comment I had).

jayunit100 · 2017-04-25T13:25:25Z

@gmarek gotcha; how do you feel about possibly creating a separate utility for load-generation that generates pods, nodes.... really dependent on either scheduler_perf or clusterloader ? Then we could both pull the same dependency............. created kubernetes/perf-tests#41 to discuss there.

ravisantoshgudimetla · 2017-04-25T13:44:02Z

@gmarek @jayunit100 I am kind of inclined to think that the data structure that we are creating should be part of clusterloader or other utility(which can be imported as library) by both clusterloader and scheduler_test.go. This includes the mutatePod and mutateNode functions(or probably generators). LMK, what you think.

gmarek · 2017-04-27T08:01:38Z

@ravisantoshgudimetla I'm fine with whatever you decide together with @sjug and @jeremyeder. You work in the same company, so it might be easier for you to just talk internally:)

jayunit100 · 2017-05-05T01:30:58Z

@k8s-bot bazel test this

jayunit100 · 2017-05-05T01:32:04Z

test/integration/scheduler_perf/scheduler_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2015 The Kubernetes Authors.
+Copyright 2017 The Kubernetes Authors.


? this should still be 2015

jayunit100 · 2017-05-05T01:32:37Z

test/integration/scheduler_perf/scheduler_test.go

@@ -209,7 +195,7 @@ func schedulePods(config *testConfig) int32 {

 	// Bake in time for the first pod scheduling event.
 	for {
-		time.Sleep(50 * time.Millisecond)
+		//time.Sleep(50 * time.Millisecond)


Why are we commenting this out? dont recall this from the last one.

So, I tried multiple iterations and it worked even with this. So, commented that.(instead of deleting for future reference).

jayunit100 · 2017-05-05T01:33:17Z

test/integration/scheduler_perf/scheduler_test.go

@@ -228,6 +214,7 @@ func schedulePods(config *testConfig) int32 {
 		// This can potentially affect performance of scheduler, since List() is done under mutex.
 		// Listing 10000 pods is an expensive operation, so running it frequently may impact scheduler.
 		// TODO: Setup watch on apiserver and wait until all pods scheduled.
+		time.Sleep(10)


see above... ?

This is an unnecessary one. Will remove it. Thnx for catching it.

jayunit100 · 2017-05-05T01:33:36Z

test/integration/scheduler_perf/scheduler_test.go

@@ -242,7 +229,7 @@ func schedulePods(config *testConfig) int32 {
 			return minQps
 		}

-		// There's no point in printing it for the last iteration, as the value is random
+		// There's no point in printing it for the last iteration, as the\ value is random


what is this slash here for?

Typo, I will remove it. Thnx.

gmarek · 2017-05-05T07:51:10Z

I have few minor comments, but we can fix them later, so it LGTM now.

gmarek · 2017-05-05T07:51:48Z

/approve

jayunit100 · 2017-05-05T13:00:28Z

/lgtm

k8s-github-robot · 2017-05-05T13:00:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmarek, jayunit100, ravisantoshgudimetla

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

~~test/OWNERS~~ [gmarek]

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-05-05T15:19:41Z

Automatic merge from submit-queue (batch tested with PRs 45322, 44770, 45411)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 21, 2017

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

k8s-github-robot assigned 0xmichalis and saad-ali Apr 21, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 21, 2017

jayunit100 reviewed Apr 21, 2017

View reviewed changes

0xmichalis assigned gmarek and unassigned 0xmichalis Apr 23, 2017

gmarek suggested changes Apr 24, 2017

View reviewed changes

jayunit100 reviewed Apr 24, 2017

View reviewed changes

xiang90 reviewed Apr 24, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch 3 times, most recently from 05ba315 to 3e7b239 Compare April 25, 2017 01:17

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

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch from d1146ad to fee3a13 Compare May 4, 2017 22:07

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

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch from 816a554 to a223d5b Compare May 4, 2017 23:56

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

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch from a223d5b to 509c2d3 Compare May 5, 2017 00:20

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

jayunit100 reviewed May 5, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch 2 times, most recently from 2f75a8d to 764a11a Compare May 5, 2017 01:50

ravisantoshgudimetla added 2 commits May 4, 2017 21:51

Changes suggested by reviewers for scheduler extensibility

cdee055

Build files generated

d3df9f5

ravisantoshgudimetla force-pushed the scheduler_perf_tests_makeover branch from 764a11a to d3df9f5 Compare May 5, 2017 01:51

gmarek approved these changes May 5, 2017

View reviewed changes

gmarek assigned jayunit100 and unassigned saad-ali May 5, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2017

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

k8s-github-robot merged commit a8522b0 into kubernetes:master May 5, 2017

ravisantoshgudimetla deleted the scheduler_perf_tests_makeover branch May 7, 2017 16:02

ravisantoshgudimetla mentioned this pull request May 17, 2017

Architectural changes for scheduler perf #45973

Closed




		// parseInput reads a configuration and then applies it to a test configuration.

Re-architecture of scheduler perf tests to make them more extendable #44770

Re-architecture of scheduler perf tests to make them more extendable #44770

Conversation

ravisantoshgudimetla commented Apr 21, 2017 • edited Loading

k8s-reviewable commented Apr 21, 2017

k8s-ci-robot commented Apr 21, 2017

ravisantoshgudimetla commented Apr 21, 2017

jayunit100 left a 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

ravisantoshgudimetla Apr 24, 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

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

jayunit100 Apr 27, 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

jayunit100 Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayunit100 commented Apr 24, 2017

gmarek commented Apr 24, 2017

sjug commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Apr 25, 2017

gmarek commented Apr 25, 2017

jayunit100 commented Apr 25, 2017

gmarek commented Apr 25, 2017

jayunit100 commented Apr 25, 2017 • edited Loading

ravisantoshgudimetla commented Apr 25, 2017

gmarek commented Apr 27, 2017

jayunit100 commented May 5, 2017

Choose a reason for hiding this comment

jayunit100 May 5, 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

gmarek commented May 5, 2017

gmarek commented May 5, 2017

jayunit100 commented May 5, 2017

k8s-github-robot commented May 5, 2017

k8s-github-robot commented May 5, 2017

ravisantoshgudimetla commented Apr 21, 2017 •

edited

Loading

ravisantoshgudimetla Apr 24, 2017 •

edited

Loading

jayunit100 Apr 27, 2017 •

edited

Loading

jayunit100 Apr 24, 2017 •

edited

Loading

jayunit100 commented Apr 25, 2017 •

edited

Loading

jayunit100 May 5, 2017 •

edited

Loading