Repository: kubernetes/kubernetes (language: go)

Issue:
scheduler: move all preCheck to QueueingHint
### What happened?

In our custom plugin, we want to move pods that is unscheduled by the plugin to activeQ/backoffQ when a new Pod is created.
So, we define `EventsToRegister` like this:

```go
func (p *Plugin) EventsToRegister() []framework.ClusterEvent {
	return []framework.ClusterEvent{{
		Resource:   framework.Node,
		ActionType: framework.All,
	}, {
		Resource:   framework.Pod,
		ActionType: framework.All,
	}}
}
```

But, even when a new Pod is created, the Pod that was unscheduled by our custom plugin isn't moved to backoffQ/activeQ.

### What did you expect to happen?

when a new Pod is created, the Pod unscheduled by our custom plugin is moved to backoffQ/activeQ.

### How can we reproduce it (as minimally and precisely as possible)?

1. create a custom plugin that has pod events in EventsToRegister.
2. create a new Pod that the custom plugin will reject.
3. create another new Pod that makes the Pod created in (1) schedulable.
4. you can see the Pod created in (1) isn't scheduled immediately (it should've been scheduled immediately after (3) because (3) should move the Pod created in (1) to activeQ/backoffQ.)

### Anything else we need to know?

If I understand correctly and if it's not expected behavior, this bug has a non-small impact, especially in v1.24 because we change `PodMaxUnschedulableQDuration` to 5 min.
https://github.com/kubernetes/kubernetes/pull/108761

### Kubernetes version

v1.23.4 but I think latest master has the same issue.

### Cloud provider

<details>

</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
/assign
/sig scheduling
Maybe it's only on a newly created unscheduled Pod.
We've moved correctly for events related to assigned Pod.
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/internal/queue/scheduling_queue.go#L597
And deleting Node events has the same issue...? I guess we need to run `sched.SchedulingQueue.MoveAllToActiveOrBackoffQueue` when some Nodes are deleted.
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/eventhandlers.go#L92-L112

In in-tree plugins, only pod topology spread uses the deleting Node event and I guess this bug affects its performance. (Even when some Nodes are deleted, the Pods, that were unscheduled by Pod topology spread, aren't moved to activeQ/backoffQ by that deleting events.) 
https://github.com/kubernetes/kubernetes/blob/e0dbea2443eb9015684dfc3361e9176e6d0ce312/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go#L146
Hm, it may be a problem that events related to Node and Pod are not always called when an event occurs. (Even if it's intended behavior, at least we need to write this on the comment of `EventsToRegister`.)

/retitle scheduler: events in `EventsToRegister` related to Pod and Node don't always work 
To summarize: The problem is "not all events related to Pod or Node don't always make move from unschedulableQ to active/backoffQ and it may not be intended for custom-plugin developers because this behavior isn't even documented."

I think we should move all unschedulable Pods with all Pod and Node events. If concerned about performance degradation, we can extend `EventsToRegister` or `EnqueueExtensions` , for example, to have the function like  [`PreEnqueueCheck`](https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/internal/queue/scheduling_queue.go#L76)?

This problem has a non-small impact, especially in v1.24+ because we change PodMaxUnschedulableQDuration to 5 min.
https://github.com/kubernetes/kubernetes/pull/108761

Let's see the details... ↓

### Assigned Pod's add/update events

We move the unscheduled Pods (rejected by the plugin that has Pod add/update events in `EventsToRegister`) in `AssignedPodAdded` or `AssignedPodUpdated`
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/internal/queue/scheduling_queue.go#L593-L607

But, the target Pods against this moving are only unschedulable pods which have any affinity term that matches "pod".
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/internal/queue/scheduling_queue.go#L660-L678

So,,,, users cannot move from unschedulableQ to activeQ/backoffQ with all assignedPod events? 
For example, for Pod Topology Spread (it uses Pod All event in `EventsToRegister`), the new assigned Pod creation may make some Pods, that are rejected by Pod Topology Spread, schedulable. Regardless of whether that unschedulable Pods have any affinity term that matches a newly created assigned pod or not.
Of course this is not just a problem for Pod Topology Spread, it could be a problem for user's custom-plugins.

I guess we should pass all unscheduled Pods to `movePodsToActiveOrBackoffQueue`. Or at least we should document this behavior.

### Assigned Pod's delete events

No issue on this.

https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/eventhandlers.go#L234

### Non-scheduled Pod's add/update/delete events

We don't move the unschedulable Pods with those events at all. That's a problem...  Even when a plugin uses Pod add/update/delete event in `EventsToRegister` and wants to move the rejected Pods with non-scheduled Pod's add/update/delete events, scheduling queue doesn't move the unschedulable Pods with those events.

(In our company, we have the custom plugin for `Gang Scheduling` and the plugin wants the scheduler queye to move the Pods rejected by the plugin from unschedulableQ to active/backoffQ when a new non-scheduled Pod is created.)

### Node's add/update events

We move only unscheduled Pods that pass `preCheckForNode`.
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/eventhandlers.go#L70
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/eventhandlers.go#L88
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/eventhandlers.go#L474-L483

`preCheckForNode` checks the Node can run that unscheduled Pod from the points of view of some in-tree filter plugins like noderesources/nodeport/nodeAffinity/nodename.

The problem that arises here is similar to "Assigned Pod's add/update events". 
For example, for Pod Topology Spread (it uses Node Add event in `EventsToRegister`), the new Node creation may make some Pods, that are rejected by Pod Topology Spread, schedulable. Regardless of the results of `preCheckForNode` against that Node. (In https://github.com/kubernetes/kubernetes/pull/107009, we change the skew calculation of Pod Topology Spread to exclude Nodes that don't match the node affinity. But, after [NodeInclusionPolicy](https://github.com/kubernetes/enhancements/issues/3094) is implemented, the calculation can be configured to include those Nodes.)

### Node's delete events

We don't move the unschedulable Pods with Node delete events... That's a problem. Even when the plugin use Node delete event in `EventsToRegister`, scheduling queue doesn't move the unschedulable Pods with any Node delete events.)
/cc @Huang-Wei @alculquicondor @ahg-g 

Could you please take a look
These are all intended optimizations. These basic checks prevent the scheduler from retrying unschedulable pods over and and over.

So far we haven't had the need to make these checks configurable as they check well established scheduling features.

+1 on documenting the behavior, but I'm not sure about changing it or making it configurable (that adds complexity that might not be justified).

What is it that you are trying to do?
> +1 on documenting the behavior, but I'm not sure about changing it or making it configurable (that adds complexity that might not be justified).
What is it that you are trying to do?

We are trying to remove flushing in queue in https://github.com/kubernetes/kubernetes/issues/87850, right?

Without improving like I suggest, some Pods in unschedulableQ cannot move back to activeQ forever in some cases if we remove flushing. 
(Actually, our company's custom plugin needs to wait flush because non-assigned Pod's events don't let scheduling queue move unschedulable Pods rejected by that plugin.)
So, I think we should simply inform all events to queue and let queue move unschedulable Pods with that events. 

This can happen in some custom plugins like our company's one and also even in in-tree plugins.
For example, Pod Topology Spread (after [NodeInclusionPolicy](https://github.com/kubernetes/enhancements/issues/3094) is implemented). When a user uses `NodeAffinityPolicy: Ignore` and `WhenUnsatisfiable: DoNotSchedule`, the creation of Node that doesn't match nodeAffinity/nodeSelector may change the scheduling result.
cc @kerthcet, who implemented NodeInclusionPolicy.

> For example, Pod Topology Spread (after https://github.com/kubernetes/enhancements/issues/3094 is implemented). When a user uses NodeAffinityPolicy: Ignore and WhenUnsatisfiable: DoNotSchedule, the creation of Node that doesn't match nodeAffinity/nodeSelector may change the scheduling result.

Can you provide a concrete example? The policy just refers to the topology spread calculations. It doesn't mean that the node doesn't have to match the nodeaffinity to be able to accept the pod.

> What is it that you are trying to do?

I'm asking what is your custom plugin trying to do. Depending on whether it makes sense, we need to evaluate if it's worth the complexity.
> So,,,, users cannot move from unschedulableQ to activeQ/backoffQ with all assignedPod events?
For example, for Pod Topology Spread (it uses Pod All event in EventsToRegister), the new assigned Pod creation may make some Pods, that are rejected by Pod Topology Spread, schedulable

This is actually a problem here, 5 mins would be too long for background flush. But it doesn't mean that we should simply shorten the interval time.

> This can happen in some custom plugins like our company's one and also even in in-tree plugins.
For example, Pod Topology Spread (after https://github.com/kubernetes/enhancements/issues/3094 is implemented). When a user uses NodeAffinityPolicy: Ignore and WhenUnsatisfiable: DoNotSchedule, the creation of Node that doesn't match nodeAffinity/nodeSelector may change the scheduling result.

The skew of a new created node is always 0, so it's still unfit I think.
@alculquicondor 

> I'm asking what is your custom plugin trying to do. Depending on whether it makes sense, we need to evaluate if it's worth the complexity.

Our company's custom plugin is like [coscheduling](https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/pkg/coscheduling) plugin.

And like coscheduling's prefilter, our plugin rejects Pods in PreFilter when the number of Pods is less than the defined number.
https://github.com/kubernetes-sigs/scheduler-plugins/blob/57499a6332c124314020a368c862b8b97470cc11/pkg/coscheduling/coscheduling.go#L128

Yes, so, maybe coscheduling plugin has the same issue as our plugin. The coscheduling plugin also uses Pod Add event and I guess it's for non-scheduled Pod's creation. (I don't know much about the coscheduling plugin though.)
https://github.com/kubernetes-sigs/scheduler-plugins/blob/57499a6332c124314020a368c862b8b97470cc11/pkg/coscheduling/coscheduling.go#L98

@kerthcet 

> The skew of a new created node is always 0, so it's still unfit I think.

Oh, that's true 😓 Please forget about that example.
But, how about like this scenario:
1. a user uses `NodeTaintsPolicy: Respect` and `WhenUnsatisfiable: DoNotSchedule`
2. a Pod is rejected by Pod Topology Spread because of the skew. 
e.g. a user uses `maxSkew: 1` and TopologyKey is hostname, NodeA/NodeB has 10 Pods each, and NodeC has 8 Pods.
3. user adds un-tolerated taint to NodeC
4. The skew calculation result should be changed and the Pod rejected in (2) will be allowed by Pod Topology Spread. But the Pod isn't moved from unschedulableQ without flushing because of this issue.


Maybe we can find some pointcuts by the `UnschedulablePlugins` in `QueuedPodInfo`, e.g. if pod was rejected by PodTopologySpread, we'll `movePodsToActiveOrBackoffQueue` when Assigned Pod's add/update events arrived. 

For customize plugins, we may have to do some refactoring to register the filter func together with the event.
So now NodeC is excluded from calculations. Hence the pod fits in either NodeA or NodeB, which were unchanged.

Tricky... I'll have to think about it. We really need those quick checks to pass for performance in most scenarios.

/triage accepted
Thanks @sanposhiho for bringing this up. Some comments below:

> Re: Assigned Pod's add/update events

You're right. Pods failed by PodTopologySpread may not get re-queued immediately.
However, an assigned Pod's add/update event would only impact unschedulable Pods with cross-node
scheduling directives. As of today, it's InterPodAffinity and PodTopologySpread.

So instead of "pass all unscheduled Pods to movePodsToActiveOrBackoffQueue", I'd like
to come up with a solution like:

```go
p.movePodsToActiveOrBackoffQueue(
    p.getUnschedulablePodsWithMatchingAffinityTerm(pod),
    p.getUnschedulablePodsWithPodTopologySpreadConstraints(pod), /* Not Implemented Yet */
    AssignedPodAdd,
) 
```

> Re: Non-scheduled Pod's add/update/delete events

It's pretty rare to move pods depending on **Unschedulable** Pods' events.

In your particular case, maybe a proactive [Activate](https://github.com/kubernetes-sigs/scheduler-plugins/pull/299)
call would behave more efficiently.

> Re: Node's add/update events

It doesn't sound correlated, is it? As the `preCheckForNode` check will just pre-verify the basic
checks which are orthogonal with PodTopologySpread. In other words, it's impossible that a Pod failed
`preCheckForNode` can become schedulable eventually.

**Updated:** The example https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1141952386 is a valid point...

> Re: Node's delete events

Can you share some use-cases to move Pods upon node's deletion? In the before, I don't think we have any,
until the last release the introduction of `minDomains` feature (as in that case deleting a node would impact
the number of domains).
> So instead of "pass all unscheduled Pods to movePodsToActiveOrBackoffQueue", I'd like
to come up with a solution like:

If we do so, also, we need to provide the way to pass some functions to decide which Pods should pass to `movePodsToActiveOrBackoffQueue` like `getUnschedulablePodsWithPodTopologySpreadConstraints` for out-of-tree plugins?

> In your particular case, maybe a proactive https://github.com/kubernetes-sigs/scheduler-plugins/pull/299
call would behave more efficiently.

Ah~, that's a good idea :)

> Can you share some use-cases to move Pods upon node's deletion? 

Maybe like this (similar to https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1141952386)

1. a user uses WhenUnsatisfiable: DoNotSchedule.
2. a Pod is rejected by Pod Topology Spread because of the skew.
e.g. a user uses maxSkew: 1 and TopologyKey is hostname, NodeA/NodeB has 10 Pods each, and NodeC has 8 Pods.
3. user deletes NodeC
4. The skew calculation result should be changed and the Pod rejected in (2) will be allowed by Pod Topology Spread. But the Pod isn't moved from unschedulableQ without flushing because of this issue.

---

> I think we should move all unschedulable Pods with all Pod and Node events. If concerned about performance degradation, we can extend EventsToRegister or EnqueueExtensions , for example, to have the function like [PreEnqueueCheck](https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/internal/queue/scheduling_queue.go#L76)?

So,,, ignoring some events for performance in scheduler side (without providing a way to change this) could be inconvenient for some plugins. What I thought in the above previous comment (https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1140397251) is to define "which kind of resource's events should be ignored" in plugin side. 
(similar to @kerthcet 's idea? https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1142202416)

We can add pre filter functions in `ClusterEvent` like:

```go
// obj: the created, updated, or deleted object
// rejectedPod: one of the unschedulable Pod in unschedulableQ
// return value: whether this unschedulable Pod should be moved to activeQ/backoffQ or not.
type PreFilterFn func(obj interface{}, rejectedPod *v1.Pod) bool

type ClusterEvent struct { 
	Resource   GVK
	ActionType ActionType
	Label      string
	// PreFilterFn is the function to check which Pod should be moved from unschedulableQ to activeQ/backoffQ by this event happens.
	PreFilterFn    PreFilterFn
}
```
https://github.com/kubernetes/kubernetes/blob/da7f184344d841807c2da88a92ee96e1de32d97b/pkg/scheduler/framework/types.go#L80-L84

Then we can set any pre-filter functions for each plugin.
And, we can define some default behavior for PreFilterFn and use them when `ClusterEvent.PreFilterFn` is empty.
So, for example, if most of the plugins are considered to prefer to ignore the event for Node that doesn't match nodeAffinity/nodeSelector, we can define this behavior as the default behavior.

What do you think about this idea?
> If we do so, also, we need to provide the way to pass some functions to decide which Pods should pass to movePodsToActiveOrBackoffQueue like getUnschedulablePodsWithPodTopologySpreadConstraints for out-of-tree plugins?
> 

Nope, that is something the scheduler core needs to implement.

> Maybe like this (similar to #110175 (comment))
> 
> 1. a user uses WhenUnsatisfiable: DoNotSchedule.
> 2. a Pod is rejected by Pod Topology Spread because of the skew.
> e.g. a user uses maxSkew: 1 and TopologyKey is hostname, NodeA/NodeB has 10 Pods each, and NodeC has 8 Pods.
> 3. user deletes NodeC
> 4. The skew calculation result should be changed and the Pod rejected in (2) will be allowed by Pod Topology Spread. But the Pod isn't moved from unschedulableQ without flushing because of this issue.

Thanks. It makes sense to me.

> What I thought in the above previous comment (https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1140397251) is to define "which kind of resource's events should be ignored" in plugin side.
> 

A customizable PreCheck approach for sure can satisfy more dynamic cases, but it's inappropriate to associate it with ClusterEvent as (1) ClusterEvent is basically an immutable constant, and (2) we use it to [map](https://github.com/kubernetes/kubernetes/blob/9f82d81e55cafdedab619ea25cabf5d42736dacf/pkg/scheduler/framework/runtime/framework.go#L545-L553) to plugins. If we go this way, we need to wrap ClusterEvent into another data structure which is kind of complicated.

---

To summarize, the following cases doesn't properly re-queue relevant pods:

1. Assigned Pod's add/update events
2. Non-scheduled Pod's add/update/delete events
3. Node's add/update/delete events

For 1 & 2, I've commented above with solutions. (1 needs core changes similar like podAffinity, 2 may leverage the Activate hook)

For 3, with @sanposhiho 's examples, I'm convinced this is some area we need to improve. One simple and stratightforward solution is to plumb a hook to let the plugin claim "alright, **upon Node events**, move the pod failed by me anyways". Then once this knob is registered, we move pods failed by that plugin back unconditionally; instead of proceeding with  `preCheckForNode()`. This hook should be available to in-tree and out-of-tree plugins. In terms of in-tree plugins, it's just PodTopologySpread (we discussed this in today's sig meeting, even InterPodAffinity doesn't fall into this case)

So, as a first step, let's ensure we're on the same page and agree on the ^^ summary. Next, we can better evaluate trade-offs and come up with an optimal solution.
Thanks for discussing this topic in the sig meeting. 🙏 
But, I am still believing that all logic related to a plugin should be controllable from the plugin side and we should not create something that can be achieved in in-tree plugins but not in out-of-tree plugins. 

So... the benefit of the "customizable PreCheck approach" (https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1142836465) isn't only to solve this issue. We can use this feature for other optimization: like improving the whole performance of scheduler by moving only Pods that are more likely to be able to be scheduled by defining more detailed events on `EventToRegister` with customizable PreCheck. 
Many plugins have a particular scenario that makes rejected Pods schedulable and the current `EventsToRegister` doesn't have an abundant enough ability of expression for this. (So, today many plugins need to say "register this hoge event on foo resource. This event ***may*** make rejected Pods schedulable" on comments in `EventsToRegister`. )

>  If we go this way, we need to wrap ClusterEvent into another data structure which is kind of complicated.

Yes, I watched the sig-meeting recording as well and the opinion that the "customizable PreCheck approach" increases some implementation complexity surely makes sense. But,,, isn't this complexity increasing acceptable even if the above is taken into account? 

---

EDIT: It is a bad habit of mine to always write long sentences even though I am not very good at English. 😓 
Summarizing the above:
- I think it's not a good idea to create something that can be achieved in in-tree plugins but not in out-of-tree plugins. 
- If we need a solid advantage to extend like this, then it has the advantage of optimizing the queue per plugin like described above.
I didn't jump in the thread since it's actively on the discussion.
But, the issue https://kubernetes.slack.com/archives/C09TP78DV/p1659383737933799 is the similar one.
(This time, we can solve that issue by changing scheduling queue logic since It's the k/k feature though.)


> This time, we can solve that issue by changing scheduling queue logic since It's the k/k feature though.

@sanposhiho I'm not sure I fully understand your intention. The slack thread was discussing the AssignedPod update case, so it fallls into case 1 in https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1145351228, which we already agreed to improve in upstream.
Hi @sanposhiho Are you working on this? I think this is quite important and if you're not working on this, I can follow.
Sorry, leave here for a while. I wanted to wait for [KEP-3521: Pod Scheduling Readiness](https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/3521-pod-scheduling-readiness) (and then forgot to come back here. 😓 )
That proposal is quite similar to what I want to see in the conclusion of this issue; every plugin can judge if each Pod in unschedQ can be moved to schedQ/backoffQ. 

@Huang-Wei Do you think it makes sense to add "By which event the pod will be moved back to schedQ/backoffQ" on `PreEnqueue`'s argument?

```go
type EnqueuePlugin interface {
    Plugin
    // event: By which event the pod will be moved back to schedQ/backoffQ.
    // If the Pod will be backed to schedQ/backoffQ by flushing, it will be nil. (Flushing plans to be removed in https://github.com/kubernetes/kubernetes/issues/87850 though.)
    // involvedObj: the object involved in that event.
    // For example, the given event is "Node deleted", the involvedObj will be that deleted Node.
    PreEnqueue(ctx context.Context, state *CycleState, p *v1.Pod, event framework.ClusterEvent, involvedObj runtime.Object) *Status
}
```

I know `event` and `involvedObj` isn't needed in DefaultEnqueue plugin for `schedulingGates` API, but that's the last missing puzzle piece 🧩 : If `PreEnqueue` has `event`, enqueue plugin can judge if the event makes it possible to schedule the Pod.

For example, I believe we can move ↓ this logic to Pod Affinity plugin by implementing `PreEnqueue` in Pod Affinity plugin.
https://github.com/kubernetes/kubernetes/blob/31a10245d67544b6718f569a1442b1de97a91e7d/pkg/scheduler/internal/queue/scheduling_queue.go#L660-L678

Pod Affinity plugin will remember Pods rejected by itself in `Filter`. And in `PreEnqueue`, if the given Pod was the one rejected by Pod Affinity, `event` is AssignedPod/Created|Updated and `involvedObj` is Pod affinity with the rejected Pod, then move the Pod back to activeQ/backoffQ. Otherwise reject the Pod moving.

We can resolve (3) by implementing `PreEnqueue` as well. (by exactly the way you said.↓)
> "alright, upon Node events, move the pod failed by me anyways"
https://github.com/kubernetes/kubernetes/issues/110175#issuecomment-1145351228

My original concern in this issue is that having logic for specific plugins in the scheduler core may prevent out-of-tree plugins' extendability. The scheduler itself should be pure, and shouldn't do anything special for any plugins implementation ideally. If we can move all such logic to each plugin, that'd be perfect. 

^ is more like a new feature than just a solution to this problem. I created another issue and describe my proposal in detail. Let's have a discussion there.
https://github.com/kubernetes/kubernetes/issues/114297
It can be resolved by moving all preCheck into each plugin's QueueingHint. Let me retitle this issue so that the current status is easy to understand.

/retitle scheduler: move all preCheck to QueueingHint
This issue has not been updated in over 1 year, and should be re-triaged.

You can:
- Confirm that this issue is still relevant with `/triage accepted` (org members only)
- Close this issue with `/close`

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted
/triage accepted

Fix patch:
diff --git a/pkg/scheduler/backend/queue/scheduling_queue.go b/pkg/scheduler/backend/queue/scheduling_queue.go
index e4da5b4a0046e..5386d918e4fb6 100644
--- a/pkg/scheduler/backend/queue/scheduling_queue.go
+++ b/pkg/scheduler/backend/queue/scheduling_queue.go
@@ -111,10 +111,9 @@ type SchedulingQueue interface {
 	Done(types.UID)
 	Update(logger klog.Logger, oldPod, newPod *v1.Pod)
 	Delete(pod *v1.Pod)
-	// TODO(sanposhiho): move all PreEnqueueCheck to Requeue and delete it from this parameter eventually.
-	// Some PreEnqueueCheck include event filtering logic based on some in-tree plugins
-	// and it affect badly to other plugins.
-	// See https://github.com/kubernetes/kubernetes/issues/110175
+	// Important Note: preCheck shouldn't include anything that depends on the in-tree plugins' logic.
+	// (e.g., filter Pods based on added/updated Node's capacity, etc.)
+	// We know currently some do, but we'll eventually remove them in favor of the scheduling queue hint.
 	MoveAllToActiveOrBackoffQueue(logger klog.Logger, event framework.ClusterEvent, oldObj, newObj interface{}, preCheck PreEnqueueCheck)
 	AssignedPodAdded(logger klog.Logger, pod *v1.Pod)
 	AssignedPodUpdated(logger klog.Logger, oldPod, newPod *v1.Pod, event framework.ClusterEvent)
diff --git a/pkg/scheduler/eventhandlers.go b/pkg/scheduler/eventhandlers.go
index 73024e3cb3617..060e5b3994a50 100644
--- a/pkg/scheduler/eventhandlers.go
+++ b/pkg/scheduler/eventhandlers.go
@@ -605,6 +605,13 @@ func addAllEventHandlers(
 }
 
 func preCheckForNode(nodeInfo *framework.NodeInfo) queue.PreEnqueueCheck {
+	if utilfeature.DefaultFeatureGate.Enabled(features.SchedulerQueueingHints) {
+		// QHint is initially created from the motivation of replacing this preCheck.
+		// It assumes that the scheduler only has in-tree plugins, which is problematic for our extensibility.
+		// Here, we skip preCheck if QHint is enabled, and we eventually remove it after QHint is graduated.
+		return nil
+	}
+
 	// Note: the following checks doesn't take preemption into considerations, in very rare
 	// cases (e.g., node resizing), "pod" may still fail a check but preemption helps. We deliberately
 	// chose to ignore those cases as unschedulable pods will be re-queued eventually.


Test patch:
diff --git a/pkg/scheduler/eventhandlers_test.go b/pkg/scheduler/eventhandlers_test.go
index a4e275bb7db6a..26ddc1e68a8fc 100644
--- a/pkg/scheduler/eventhandlers_test.go
+++ b/pkg/scheduler/eventhandlers_test.go
@@ -115,6 +115,7 @@ func TestPreCheckForNode(t *testing.T) {
 		nodeFn             func() *v1.Node
 		existingPods, pods []*v1.Pod
 		want               []bool
+		qHintEnabled       bool
 	}{
 		{
 			name: "regular node, pods with a single constraint",
@@ -137,6 +138,28 @@ func TestPreCheckForNode(t *testing.T) {
 			},
 			want: []bool{true, false, false, true, false, true, false, true, false},
 		},
+		{
+			name: "no filtering when QHint is enabled",
+			nodeFn: func() *v1.Node {
+				return st.MakeNode().Name("fake-node").Label("hostname", "fake-node").Capacity(cpu8).Obj()
+			},
+			existingPods: []*v1.Pod{
+				st.MakePod().Name("p").HostPort(80).Obj(),
+			},
+			pods: []*v1.Pod{
+				st.MakePod().Name("p1").Req(cpu4).Obj(),
+				st.MakePod().Name("p2").Req(cpu16).Obj(),
+				st.MakePod().Name("p3").Req(cpu4).Req(cpu8).Obj(),
+				st.MakePod().Name("p4").NodeAffinityIn("hostname", []string{"fake-node"}).Obj(),
+				st.MakePod().Name("p5").NodeAffinityNotIn("hostname", []string{"fake-node"}).Obj(),
+				st.MakePod().Name("p6").Obj(),
+				st.MakePod().Name("p7").Node("invalid-node").Obj(),
+				st.MakePod().Name("p8").HostPort(8080).Obj(),
+				st.MakePod().Name("p9").HostPort(80).Obj(),
+			},
+			qHintEnabled: true,
+			want:         []bool{true, true, true, true, true, true, true, true, true},
+		},
 		{
 			name: "tainted node, pods with a single constraint",
 			nodeFn: func() *v1.Node {
@@ -194,13 +217,15 @@ func TestPreCheckForNode(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SchedulerQueueingHints, tt.qHintEnabled)
+
 			nodeInfo := framework.NewNodeInfo(tt.existingPods...)
 			nodeInfo.SetNode(tt.nodeFn())
 			preCheckFn := preCheckForNode(nodeInfo)
 
-			var got []bool
+			got := make([]bool, 0, len(tt.pods))
 			for _, pod := range tt.pods {
-				got = append(got, preCheckFn(pod))
+				got = append(got, preCheckFn == nil || preCheckFn(pod))
 			}
 
 			if diff := cmp.Diff(tt.want, got); diff != "" {