narrow down the scope of EnqueueExtensions to subscribe less cluster events #121571
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
/cc @alculquicondor /hold |
9b9a82e
to
c7842d9
Compare
| framework.CSINode: framework.All - framework.Delete, | ||
| framework.CSIDriver: framework.All - framework.Delete, | ||
| framework.CSIStorageCapacity: framework.All - framework.Delete, | ||
| framework.PersistentVolume: framework.All - framework.Delete, | ||
| framework.PersistentVolumeClaim: framework.All - framework.Delete, | ||
| framework.StorageClass: framework.All - framework.Delete, |
There was a problem hiding this comment.
I think this justifies a cherry-pick to 1.28
There was a problem hiding this comment.
Please don't. It's more an optimization instead of essential bug fixes.
|
cc @Huang-Wei in case this could affect out-of-tree plugins |
LGTM. Some out-of-tree Score plugins are intentionally to leave EventsToRegister un-implemented. |
|
/retest |
|
LGTM label has been added. Git tree hash: e0ea4d15e782a3c5f2c1ee855880264b0676ff1a
|
|
/remove-kind cleanup |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, sanposhiho 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 |
|
@alculquicondor it's not a bug fix actually. /remove-kind bug |
| framework.StorageClass: framework.All, | ||
| framework.PodSchedulingContext: framework.All, | ||
| }, | ||
| want: map[framework.GVK]framework.ActionType{}, | ||
| }, | ||
| { | ||
| name: "plugins with default profile", | ||
| plugins: schedulerapi.PluginSet{Enabled: defaults.PluginsV1.MultiPoint.Enabled}, | ||
| want: map[framework.GVK]framework.ActionType{ | ||
| framework.Pod: framework.All, | ||
| framework.Node: framework.All, |
There was a problem hiding this comment.
which plugin cares about Node Delete?
There was a problem hiding this comment.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Currently, we register default EventsToRegister for all plugins that don't have EnqueueExtension.
But, this PR changes it to register the default one only for PreEnqueue, PreFilter, Filter, Reserve, and Permit plugins. For other plugins, the scheduling framework doesn't register the default one.
EventsToRegister results in which events the scheduler watches, and when registered events come, the scheduling queue requeue Pods according to them. The queue checks the event can make Pod schedulable or not via QueueingHint from pInfo.UnschedulablePlugins/PendingPlugins.
Given this fact, plugins that never be registered in pInfo.UnschedulablePlugins don't need to have EventsToRegister.
And, currently, only PreEnqueue, PreFilter, Filter, Reserve, and Permit affect pInfo.UnschedulablePlugins/PendingPlugins.
Meaning all other plugins' EventsToRegister actually don't affect anything, and registering default EventsToRegister to other plugins might result in watching events in vain, which all of PreEnqueue, PreFilter, Filter, Reserve, and Permit plugins aren't interested in.
Which issue(s) this PR fixes:
Fixes #119783
Also help #120622 a bit by reducing the number of events the scheduler subscribes.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: