Pass InformerFactory/SharedLister instead of individual informers/listers in scheduler config logic #85150
Conversation
k8s-ci-robot
added
release-note-none
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
ahg-g
changed the title
pkg/scheduler/factory.go
Outdated
| c.nodeLister, | ||
| c.podLister, | ||
| c.informerFactory.Core().V1().Nodes().Lister(), | ||
| c.informerFactory.Core().V1().Pods().Lister(), |
There was a problem hiding this comment.
This seems to be the only place were we use pod informer (other than tests). Maybe we can just create it here, instead of cmd/kube-scheduler/app/options/options.go
There was a problem hiding this comment.
Good catch.
It is used when registering the handlers in scheduler.go, this is what feeds the scheduler's cache.
pkg/scheduler/factory.go
Outdated
| @@ -651,3 +579,19 @@ func (b *binder) Bind(binding *v1.Binding) error { | |||
| klog.V(3).Infof("Attempting to bind %v to %v", binding.Name, binding.Target.Name) | |||
| return b.Client.CoreV1().Pods(binding.Namespace).Bind(binding) | |||
| } | |||
|
|
|||
| // GetPodDisruptionBudgetLister returns pdb lister from the given informer factory. Returns nil if PodDisruptionBudget feature is disabled. | |||
| func GetPodDisruptionBudgetLister(informerFactory informers.SharedInformerFactory) (pdbLister policylisters.PodDisruptionBudgetLister) { | |||
There was a problem hiding this comment.
prefer non-named return values.
And does it need to be exported?
There was a problem hiding this comment.
this one does not, the other one does, so thought I would be consistent.
pkg/scheduler/factory.go
Outdated
| } | ||
|
|
||
| // GetCSINodeLister returns CSINode lister from the given informer factory. Returns nil if CSINodeInfo feature is disabled. | ||
| func GetCSINodeLister(informerFactory informers.SharedInformerFactory) (csiNodeLister storagelistersv1beta1.CSINodeLister) { |
ahg-g
force-pushed
the
ahg-informer
branch
3 times, most recently
from
64be6b3
to
3f8ecb6
Compare
| PluginConfigProducerRegistry: pluginConfigProducerRegistry, | ||
| }) | ||
| snapshot := nodeinfosnapshot.NewEmptySnapshot() | ||
| return &Configurator{ |
There was a problem hiding this comment.
This is missing enableNonPreempting, but it's hard to see if there's anything else missing. Can you think of a way of avoiding this?
There was a problem hiding this comment.
Unit tests is supposed to capture this if this is a parameter that is actually tested. It seems that we don't quite care about the actual parameters in this test file.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-integration |
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/milestone v1.17 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We pass around a lot of parameters representing the various informers the scheduler uses, which makes the scheduler config logic quite messy. This PR replaces that with a two parameters: SharedInformerFactory and the scheduler's SharedLister.
The PR also removes the redundant ConfigFactoryArgs struct.
Which issue(s) this PR fixes:
Part of #72547
Does this PR introduce a user-facing change?: