Merge pull request #125644 from sanposhiho/automated-cherry-pick-of-#…

…125527-upstream-release-1.29 Automated cherry pick of #125527: fix: skip isPodWorthRequeuing only when SchedulingGates
kubernetes · Jul 11, 2024 · b866d22 · b866d22
2 parents fce6283 + 12f304c
commit b866d22
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 11 deletions.
diff --git a/pkg/scheduler/internal/queue/scheduling_queue.go b/pkg/scheduler/internal/queue/scheduling_queue.go
@@ -47,6 +47,7 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/scheduler/framework"
 	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/interpodaffinity"
+	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/names"
 	"k8s.io/kubernetes/pkg/scheduler/internal/heap"
 	"k8s.io/kubernetes/pkg/scheduler/metrics"
 	"k8s.io/kubernetes/pkg/scheduler/util"
@@ -1143,11 +1144,25 @@ func (p *PriorityQueue) requeuePodViaQueueingHint(logger klog.Logger, pInfo *fra
 func (p *PriorityQueue) movePodsToActiveOrBackoffQueue(logger klog.Logger, podInfoList []*framework.QueuedPodInfo, event framework.ClusterEvent, oldObj, newObj interface{}) {
 	activated := false
 	for _, pInfo := range podInfoList {
-		// Since there may be many gated pods and they will not move from the
-		// unschedulable pool, we skip calling the expensive isPodWorthRequeueing.
-		if pInfo.Gated {
+		// When handling events takes time, a scheduling throughput gets impacted negatively
+		// because of a shared lock within PriorityQueue, which Pop() also requires.
+		//
+		// Scheduling-gated Pods never get schedulable with any events,
+		// except the Pods themselves got updated, which isn't handled by movePodsToActiveOrBackoffQueue.
+		// So, we can skip them early here so that they don't go through isPodWorthRequeuing,
+		// which isn't fast enough to keep a sufficient scheduling throughput
+		// when the number of scheduling-gated Pods in unschedulablePods is large.
+		// https://github.com/kubernetes/kubernetes/issues/124384
+		// This is a hotfix for this issue, which might be changed
+		// once we have a better general solution for the shared lock issue.
+		//
+		// Note that we cannot skip all pInfo.Gated Pods here
+		// because PreEnqueue plugins apart from the scheduling gate plugin may change the gating status
+		// with these events.
+		if pInfo.Gated && pInfo.UnschedulablePlugins.Has(names.SchedulingGates) {
 			continue
 		}
+
 		schedulingHint := p.isPodWorthRequeuing(logger, pInfo, event, oldObj, newObj)
 		if schedulingHint == queueSkip {
 			// QueueingHintFn determined that this Pod isn't worth putting to activeQ or backoffQ by this event.

diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go
@@ -43,6 +43,7 @@ import (
 	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/scheduler/framework"
+	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/names"
 	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/queuesort"
 	"k8s.io/kubernetes/pkg/scheduler/metrics"
 	st "k8s.io/kubernetes/pkg/scheduler/testing"
@@ -1320,13 +1321,19 @@ func TestPriorityQueue_MoveAllToActiveOrBackoffQueueWithQueueingHint(t *testing.
 			expectedQ: unschedulablePods,
 		},
 		{
-			name:    "QueueHintFunction is not called when Pod is gated",
-			podInfo: setQueuedPodInfoGated(&framework.QueuedPodInfo{PodInfo: mustNewPodInfo(p), UnschedulablePlugins: sets.New("foo")}),
+			name:    "QueueHintFunction is not called when Pod is gated by SchedulingGates plugin",
+			podInfo: setQueuedPodInfoGated(&framework.QueuedPodInfo{PodInfo: mustNewPodInfo(p), UnschedulablePlugins: sets.New(names.SchedulingGates, "foo")}),
 			hint: func(logger klog.Logger, pod *v1.Pod, oldObj, newObj interface{}) (framework.QueueingHint, error) {
 				return framework.Queue, fmt.Errorf("QueueingHintFn should not be called as pod is gated")
 			},
 			expectedQ: unschedulablePods,
 		},
+		{
+			name:      "QueueHintFunction is called when Pod is gated by a plugin other than SchedulingGates",
+			podInfo:   setQueuedPodInfoGated(&framework.QueuedPodInfo{PodInfo: mustNewPodInfo(p), UnschedulablePlugins: sets.New("foo")}),
+			hint:      queueHintReturnQueue,
+			expectedQ: activeQ,
+		},
 	}
 
 	for _, test := range tests {
@@ -1339,14 +1346,13 @@ func TestPriorityQueue_MoveAllToActiveOrBackoffQueueWithQueueingHint(t *testing.
 					QueueingHintFn: test.hint,
 				},
 			}
-			test.podInfo.UnschedulablePlugins = sets.New("foo")
 			cl := testingclock.NewFakeClock(now)
 			q := NewTestQueue(ctx, newDefaultQueueSort(), WithQueueingHintMapPerProfile(m), WithClock(cl))
-			// add to unsched pod pool
 			q.activeQ.Add(q.newQueuedPodInfo(test.podInfo.Pod))
 			if p, err := q.Pop(logger); err != nil || p.Pod != test.podInfo.Pod {
 				t.Errorf("Expected: %v after Pop, but got: %v", test.podInfo.Pod.Name, p.Pod.Name)
 			}
+			// add to unsched pod pool
 			err := q.AddUnschedulableIfNotPresent(logger, test.podInfo, q.SchedulingCycle())
 			if err != nil {
 				t.Fatalf("unexpected error from AddUnschedulableIfNotPresent: %v", err)

diff --git a/test/integration/scheduler/plugins/plugins_test.go b/test/integration/scheduler/plugins/plugins_test.go
@@ -2657,8 +2657,8 @@ func (pl *SchedulingGatesPluginWOEvents) EventsToRegister() []framework.ClusterE
 	return nil
 }
 
-// This test helps to verify registering nil events for schedulingGates plugin works as expected.
-func TestSchedulingGatesPluginEventsToRegister(t *testing.T) {
+// This test helps to verify registering nil events for PreEnqueue plugin works as expected.
+func TestPreEnqueuePluginEventsToRegister(t *testing.T) {
 	defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.PodSchedulingReadiness, true)()
 
 	testContext := testutils.InitTestAPIServer(t, "preenqueue-plugin", nil)
@@ -2678,7 +2678,8 @@ func TestSchedulingGatesPluginEventsToRegister(t *testing.T) {
 	tests := []struct {
 		name          string
 		enqueuePlugin framework.PreEnqueuePlugin
-		count         int
+		// count is the expected number of calls to PreEnqueue().
+		count int
 	}{
 		{
 			name:          "preEnqueue plugin without event registered",
@@ -2688,7 +2689,7 @@ func TestSchedulingGatesPluginEventsToRegister(t *testing.T) {
 		{
 			name:          "preEnqueue plugin with event registered",
 			enqueuePlugin: &SchedulingGatesPluginWithEvents{SchedulingGates: schedulinggates.SchedulingGates{EnablePodSchedulingReadiness: true}},
-			count:         2,
+			count:         3,
 		},
 	}