Move informer event handlers to scheduler #69504

krmayankk · 2018-10-08T03:18:54Z

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68955

NONE

/kind cleanup

krmayankk · 2018-10-08T03:19:16Z

/assign @misterikkit

misterikkit

Overall looks good. I haven't done a careful review of the handlers themselves.

I think the only other change I'd hope to see in this PR would be to make sure that the functions in generic_scheduler.go are organized sensibly.

misterikkit · 2018-10-08T22:51:52Z

pkg/scheduler/core/generic_scheduler.go

@@ -125,12 +172,12 @@ func (g *genericScheduler) snapshot() error {
 	// 3. snapshot ecache
 	// 4. evaludate predicates
 	// 5. stale result will be written to ecache
-	if g.equivalenceCache != nil {
-		g.equivalenceCache.Snapshot()
+	if g.equivalencePodCache != nil {


This seems like an old variable name. What did you base this commit on?

i rebased on latest upstream master. I decided to rename it that way. Will fix if it doesnt make sense

Do you have a reason for renaming it that way? If not, then I'd prefer to leave it as is.

misterikkit · 2018-10-08T22:56:32Z

pkg/scheduler/core/generic_scheduler.go

+	// Enable equivalence class cache
+	enableEquivalenceClassCache bool
+
+	nodeInformer                  coreinformers.NodeInformer


It's not clear to me that all of these informers need to be in the struct. Which ones do we need after NewGenericScheduler() has finished?

will get back

I am trying to reuse the informers passed from cmd/kube-scheduler via NewConfigFactory. I think they are not used after the NewGenericScheduler finishes. Is it ok to not use the ones passed from cmd/kube-scheduler and just a local copy to add the event handlers ?

The expectation is that NewGenericScheduler() takes informer args for each kind that it cares about. If we add event handlers to those informers, then we might not need to keep a reference to the informer itself.

misterikkit · 2018-10-10T16:41:18Z

This is going to need a rebase, as a small change just went in. ( #69592 )

Also, can you provide an update on the progress of this PR? It's still marked as WIP and I haven't seen responses to my previous questions. If you're stuck, reach out here or on slack.

krmayankk · 2018-10-22T06:21:10Z

/retest

krmayankk · 2019-01-26T00:25:47Z

/test pull-kubernetes-e2e-gce-100-performance

misterikkit

Please address 1 new comment and squash commits, then we can get this merged. ^_^

misterikkit · 2019-01-30T00:20:39Z

test/integration/util/util.go

+		informerFactory.Storage().V1().StorageClasses(),
+	)
+
+	go func() {


I think this goroutine is redundant with the one created in factory.NewConfigFactory

wgliang · 2019-01-30T02:29:19Z

/assign @wgliang
Can you take a look at this PR and check how it will interact with your WIP PR to move everyone to scheduler.New?

This change should not impose any additional burden, and I am more worried about completing the code we need to modify a lot of integration tests. I'm trying... :)

krmayankk · 2019-01-30T03:26:25Z

/test pull-kubernetes-e2e-kops-aws

krmayankk · 2019-01-30T04:06:25Z

/test pull-kubernetes-e2e-kops-aws

misterikkit

/lgtm
🐳 🚢
/assign @k82cn @bsalamat
please review for approval

misterikkit · 2019-01-30T18:50:31Z

@wgliang Yeah, I've tried tweaking some of the integration test code in an experimental branch, and it's a lot of work to remove configFactory. Seems like you need to start with the TestContext type.

bsalamat

/approve

Thanks a lot, @krmayankk!

krmayankk · 2019-01-31T02:45:58Z

/assign @timothysc

krmayankk · 2019-01-31T05:14:43Z

/test pull-kubernetes-e2e-kops-aws

k82cn · 2019-01-31T07:38:03Z

/unassign @timothysc

@kubernetes/sig-testing-pr-reviews

krmayankk · 2019-02-01T01:52:09Z

/test pull-kubernetes-e2e-kops-aws

k8s-ci-robot · 2019-02-01T02:31:05Z

@krmayankk: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`e0a7d96`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

cblecker · 2019-02-01T02:31:12Z

/skip

deads2k · 2019-02-01T13:04:42Z

/approve

k8s-ci-robot · 2019-02-01T13:07:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, deads2k, krmayankk

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]
~~test/OWNERS~~ [deads2k]

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

krmayankk · 2019-02-01T17:35:57Z

thanks @misterikkit @bsalamat @deads2k

Clean empty package import by #69504

k8s-ci-robot requested review from k82cn and misterikkit October 8, 2018 03:19

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 8, 2018

k8s-ci-robot assigned misterikkit Oct 8, 2018

krmayankk changed the title ~~Move informer event handlers to genericScheduler~~ [WIP]Move informer event handlers to genericScheduler Oct 8, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2018

krmayankk force-pushed the refactor-sched1 branch from 8899c84 to cfa3195 Compare October 8, 2018 03:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2018

misterikkit reviewed Oct 8, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2018

krmayankk force-pushed the refactor-sched1 branch 2 times, most recently from ba830d0 to 383dac7 Compare October 14, 2018 09:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2018

krmayankk force-pushed the refactor-sched1 branch 2 times, most recently from 46af42b to 4df2248 Compare October 15, 2018 04:41

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2018

krmayankk force-pushed the refactor-sched1 branch from 4df2248 to 612e779 Compare October 19, 2018 00:10

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

krmayankk force-pushed the refactor-sched1 branch from 612e779 to af08644 Compare October 21, 2018 17:29

krmayankk force-pushed the refactor-sched1 branch from af08644 to 892ff48 Compare October 27, 2018 06:01

misterikkit reviewed Jan 30, 2019

View reviewed changes

Move informer event handlers to Scheduler

e0a7d96

krmayankk force-pushed the refactor-sched1 branch from 3495d3f to e0a7d96 Compare January 30, 2019 01:53

misterikkit approved these changes Jan 30, 2019

View reviewed changes

k8s-ci-robot assigned bsalamat and k82cn Jan 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2019

bsalamat reviewed Jan 30, 2019

View reviewed changes

k8s-ci-robot assigned timothysc Jan 31, 2019

k8s-ci-robot unassigned timothysc Jan 31, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019

k8s-ci-robot merged commit 2e01637 into kubernetes:master Feb 1, 2019

WanLinghao mentioned this pull request Mar 15, 2019

Clean empty package import by #69504 #75397

Merged

WanLinghao added a commit to WanLinghao/kubernetes that referenced this pull request Mar 15, 2019

Clean empty package import by kubernetes#69504

a0a8ea4

k8s-ci-robot added a commit that referenced this pull request Mar 20, 2019

Merge pull request #75397 from WanLinghao/equivalence_clean

79b0fcc

Clean empty package import by #69504

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move informer event handlers to scheduler #69504

Move informer event handlers to scheduler #69504

krmayankk commented Oct 8, 2018

krmayankk commented Oct 8, 2018

misterikkit left a comment

misterikkit Oct 8, 2018

krmayankk Oct 12, 2018

misterikkit Oct 15, 2018

misterikkit Oct 8, 2018

krmayankk Oct 12, 2018

krmayankk Oct 15, 2018

misterikkit Oct 15, 2018

misterikkit commented Oct 10, 2018

krmayankk commented Oct 22, 2018

krmayankk commented Jan 26, 2019

misterikkit left a comment

misterikkit Jan 30, 2019

krmayankk Jan 30, 2019

wgliang commented Jan 30, 2019

krmayankk commented Jan 30, 2019

krmayankk commented Jan 30, 2019

misterikkit left a comment

misterikkit commented Jan 30, 2019

bsalamat left a comment

krmayankk commented Jan 31, 2019

krmayankk commented Jan 31, 2019

k82cn commented Jan 31, 2019 •

edited

krmayankk commented Feb 1, 2019

k8s-ci-robot commented Feb 1, 2019

cblecker commented Feb 1, 2019

deads2k commented Feb 1, 2019

k8s-ci-robot commented Feb 1, 2019

krmayankk commented Feb 1, 2019

Move informer event handlers to scheduler #69504

Move informer event handlers to scheduler #69504

Conversation

krmayankk commented Oct 8, 2018

krmayankk commented Oct 8, 2018

misterikkit 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

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

misterikkit commented Oct 10, 2018

krmayankk commented Oct 22, 2018

krmayankk commented Jan 26, 2019

misterikkit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgliang commented Jan 30, 2019

krmayankk commented Jan 30, 2019

krmayankk commented Jan 30, 2019

misterikkit left a comment

Choose a reason for hiding this comment

misterikkit commented Jan 30, 2019

bsalamat left a comment

Choose a reason for hiding this comment

krmayankk commented Jan 31, 2019

krmayankk commented Jan 31, 2019

k82cn commented Jan 31, 2019 • edited

krmayankk commented Feb 1, 2019

k8s-ci-robot commented Feb 1, 2019

cblecker commented Feb 1, 2019

deads2k commented Feb 1, 2019

k8s-ci-robot commented Feb 1, 2019

krmayankk commented Feb 1, 2019

k82cn commented Jan 31, 2019 •

edited