Remove CSINode from scheduler cache. #84017

ahg-g · 2019-10-16T21:02:19Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Remove CSINodeInfo from scheduler cache. CSINodeInfo should be read directly from informer cache. Having in the scheduler cache consumes memory and adds no value.

Which issue(s) this PR fixes:
Part of #83922

NONE

/priority important-soon
/assign @liu-cong

k8s-ci-robot · 2019-10-16T21:03:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

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~~ [ahg-g]

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

Huang-Wei

LGTM as (1) CSINodeInfo is not included in the internal snapshot, (2) no particular caching/indexing/filtering logic is needed in the CSI predicate.

/lgtm
/hold
in case @davidz627 has concerns.

Huang-Wei · 2019-10-16T21:55:21Z

pkg/scheduler/eventhandlers.go

 				AddFunc:    sched.onCSINodeAdd,
 				UpdateFunc: sched.onCSINodeUpdate,


We can directly put:

AddFunc: sched.SchedulingQueue.MoveAllToActiveQueue(), UpdateFunc: sched.SchedulingQueue.MoveAllToActiveQueue(),

oh this is what i said elsewhere too
+1 to this

we can't the signature is different, so has to be wrapped somehow (inline or a separate func).

davidz627 · 2019-10-16T23:39:47Z

pkg/scheduler/algorithm/predicates/predicates.go

+var _ CSINodeInfo = &CachedCSINodeInfo{}
+
+// CachedCSINodeInfo implements CSINodeInfoInfo
+type CachedCSINodeInfo struct {


nit: the API object has been renamed to CSINode (the Info is omitted)

I wanted to be consistent with the other few interfaces defined in the same file (all suffixed with info). I agree we probably don't need any of that, but as I mentioned I would like to keep the code consistent while working on removing all of them completely.

davidz627 · 2019-10-16T23:41:32Z

pkg/scheduler/eventhandlers.go

-	if err := sched.SchedulerCache.AddCSINode(csiNode); err != nil {
-		klog.Errorf("scheduler cache AddCSINode failed: %v", err)
-	}
-
 	sched.SchedulingQueue.MoveAllToActiveQueue()
 }

 func (sched *Scheduler) onCSINodeUpdate(oldObj, newObj interface{}) {


why do we keep these stubs here now that they dont' do anything on update? If we want to just maintain sched.SchedulingQueue.MoveAllToActiveQueue() behavior all callers of these funcs should just call that function directly

we need to wrap them because MoveAllToActiveQueue has a different signature compared to the callbacks AddFunc and UpdateFunc.

davidz627 · 2019-10-16T23:42:01Z

pkg/scheduler/eventhandlers.go

 				AddFunc:    sched.onCSINodeAdd,
 				UpdateFunc: sched.onCSINodeUpdate,


oh this is what i said elsewhere too
+1 to this

davidz627 · 2019-10-16T23:43:04Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -172,6 +173,18 @@ type CSINodeInfo interface {
 	GetCSINodeInfo(nodeName string) (*storagev1beta1.CSINode, error)
 }

+var _ CSINodeInfo = &CachedCSINodeInfo{}


nit: this interface should be renamed to CSINodeGetter

replied in the other comment.

davidz627 · 2019-10-16T23:44:17Z

pkg/scheduler/algorithm/predicates/predicates.go

+
+// CachedCSINodeInfo implements CSINodeInfoInfo
+type CachedCSINodeInfo struct {
+	storagev1beta1listers.CSINodeLister


why even wrap this one interface inside this struct, why not just use the CSINodeLister interface directly

this comment superscedes others

I am planning a cleanup PR for all "listers/getters" the scheduler defines, so lets do that there. What I wanted to do in this PR is just cleanup the cache and maintain the existing interface that other parts of the code depend on (and hence I had to define this interface temporarily).

ahg-g · 2019-10-17T01:25:21Z

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

liu-cong

/lgtm with a nit.

liu-cong · 2019-10-17T14:53:54Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -37,6 +37,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	storagelisters "k8s.io/client-go/listers/storage/v1"
+	storagev1beta1listers "k8s.io/client-go/listers/storage/v1beta1"


nit: the alias naming is inconsistent in this file. Some are <name><version> such as storagev1, some are the reverse such as v1helper, and this one seems to be a combination. Not sure if there is a common standard in k8s but we should at least make it consistent here.

done. I followed the "<version><name>" pattern.

davidz627 · 2019-10-17T18:16:41Z

LGTM based on comment replies. Sounds like this is the first step to a larger refactor

ahg-g · 2019-10-17T18:28:18Z

LGTM based on comment replies. Sounds like this is the first step to a larger refactor

Indeed, we will likely remove most lister wrappers we have defined in the scheduler.

Huang-Wei · 2019-10-17T20:29:32Z

/lgtm
/hold cancel

ahg-g · 2019-10-18T00:14:06Z

/hold

ahg-g · 2019-10-18T02:56:49Z

/hold cancel

ahg-g · 2019-10-18T13:34:10Z

/hold

ahg-g · 2019-10-19T11:05:39Z

@Huang-Wei can you please take a quick look, had to rebase

ahg-g · 2019-10-19T11:05:52Z

/hold cancel

Huang-Wei · 2019-10-20T06:28:10Z

/lgtm

fejta-bot · 2019-10-20T09:46:06Z

/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 or /hold comment for consistent failures.

k8s-ci-robot assigned liu-cong Oct 16, 2019

k8s-ci-robot requested review from Huang-Wei and k82cn October 16, 2019 21:03

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 16, 2019

ahg-g force-pushed the ahg-csi branch from 2243900 to ebec412 Compare October 16, 2019 21:06

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

ahg-g force-pushed the ahg-csi branch from ebec412 to dc711ff Compare October 16, 2019 21:29

Huang-Wei reviewed Oct 16, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2019

k8s-ci-robot assigned Huang-Wei Oct 16, 2019

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

davidz627 reviewed Oct 16, 2019

View reviewed changes

ahg-g force-pushed the ahg-csi branch from dc711ff to d965c50 Compare October 17, 2019 00:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2019

liu-cong reviewed Oct 17, 2019

View reviewed changes

ahg-g force-pushed the ahg-csi branch from d965c50 to 880e88f Compare October 17, 2019 15:37

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 17, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2019

ahg-g force-pushed the ahg-csi branch from 880e88f to 54e5cdd Compare October 18, 2019 03:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2019

ahg-g force-pushed the ahg-csi branch 4 times, most recently from 4f0c272 to efaed0d Compare October 18, 2019 12:38

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2019

Remove CSINode from scheduler cache.

a772722

ahg-g force-pushed the ahg-csi branch from efaed0d to a772722 Compare October 19, 2019 03:52

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2019

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

k8s-ci-robot merged commit 019b662 into kubernetes:master Oct 20, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 20, 2019

ahg-g deleted the ahg-csi branch January 10, 2020 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove CSINode from scheduler cache. #84017

Remove CSINode from scheduler cache. #84017

ahg-g commented Oct 16, 2019

k8s-ci-robot commented Oct 16, 2019

Huang-Wei left a comment

Huang-Wei Oct 16, 2019

davidz627 Oct 16, 2019

ahg-g Oct 17, 2019 •

edited

davidz627 Oct 16, 2019

ahg-g Oct 17, 2019

davidz627 Oct 16, 2019

ahg-g Oct 17, 2019

davidz627 Oct 16, 2019

davidz627 Oct 16, 2019

ahg-g Oct 17, 2019

davidz627 Oct 16, 2019

ahg-g Oct 17, 2019

ahg-g commented Oct 17, 2019

liu-cong left a comment

liu-cong Oct 17, 2019

ahg-g Oct 17, 2019 •

edited

davidz627 commented Oct 17, 2019

ahg-g commented Oct 17, 2019

Huang-Wei commented Oct 17, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 19, 2019

ahg-g commented Oct 19, 2019

Huang-Wei commented Oct 20, 2019

fejta-bot commented Oct 20, 2019

		AddFunc: sched.onCSINodeAdd,
		UpdateFunc: sched.onCSINodeUpdate,

Remove CSINode from scheduler cache. #84017

Remove CSINode from scheduler cache. #84017

Conversation

ahg-g commented Oct 16, 2019

k8s-ci-robot commented Oct 16, 2019

Huang-Wei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Oct 17, 2019 • edited

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

ahg-g commented Oct 17, 2019

liu-cong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Oct 17, 2019 • edited

Choose a reason for hiding this comment

davidz627 commented Oct 17, 2019

ahg-g commented Oct 17, 2019

Huang-Wei commented Oct 17, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 18, 2019

ahg-g commented Oct 19, 2019

ahg-g commented Oct 19, 2019

Huang-Wei commented Oct 20, 2019

fejta-bot commented Oct 20, 2019

ahg-g Oct 17, 2019 •

edited

ahg-g Oct 17, 2019 •

edited