From 4ee1394b71a0a41814a4c7613dcebda60a21d79a Mon Sep 17 00:00:00 2001
From: Kensei Nakada <handbomusic@gmail.com>
Date: Wed, 4 Sep 2024 15:33:22 +0900
Subject: [PATCH] feat: disable preCheck when QHint is enabled

---
 .../backend/queue/scheduling_queue.go         |  7 ++---
 pkg/scheduler/eventhandlers.go                |  7 +++++
 pkg/scheduler/eventhandlers_test.go           | 29 +++++++++++++++++--
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/pkg/scheduler/backend/queue/scheduling_queue.go b/pkg/scheduler/backend/queue/scheduling_queue.go
index e4da5b4a004..5386d918e4f 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 73024e3cb36..060e5b3994a 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.
diff --git a/pkg/scheduler/eventhandlers_test.go b/pkg/scheduler/eventhandlers_test.go
index a4e275bb7db..26ddc1e68a8 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 != "" {