Filter gated pods before calling isPodWorthRequeueing #124618
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/test
cncf-cla: yes
|
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. |
k8s-ci-robot
added
the
needs-priority
|
Welcome @gabesaba! |
|
Hi @gabesaba. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -2677,7 +2677,7 @@ func TestSchedulingGatesPluginEventsToRegister(t *testing.T) { | |||
| { | |||
| name: "preEnqueue plugin with event registered", | |||
| enqueuePlugin: &SchedulingGatesPluginWithEvents{SchedulingGates: schedulinggates.SchedulingGates{}}, | |||
| count: 3, | |||
| count: 2, | |||
There was a problem hiding this comment.
@kerthcet you added this test. Do you think it still makes sense to keep?
Or should we be testing it a different way?
There was a problem hiding this comment.
Sorry, I was on vacation last week.
I think we can replace the schedulingGate plugin with other plugin simply, we can leave this as a follow up since we're considering cherrypick. If no one has context, I can do this later.
There was a problem hiding this comment.
sounds good, let's leave it for a follow up.
|
@sanposhiho do you want to give it a pass? |
I asked for a change of this nature. Since we are changing how we are processing gated pods, I wanted to make sure they are still scheduled after a delete event for a different pod. |
|
TestGatedPodSchedulableAfterEvent is ok then, but Test_queuedPodInfo_gatedSetUponCreationAndUnsetUponUpdate doesn't go thru any changed part at all. |
| // unschedulable pool, we skip calling the expensive isPodWorthRequeueing. | ||
| if pInfo.Gated { | ||
| continue | ||
| } | ||
| schedulingHint := p.isPodWorthRequeuing(logger, pInfo, event, oldObj, newObj) |
There was a problem hiding this comment.
How was the gated status checked before? Can we remove that check to avoid confusion with the new check?
There was a problem hiding this comment.
This was done in isPodWorthRequeueing. There is no specific logic for gated, but rather general logic for unschedulable/pending plugins
There was a problem hiding this comment.
was that by calling the PreEnqueue plugin?
@Huang-Wei, any concern with just using the .Gated field directly?
There was a problem hiding this comment.
Yes, the PreEnqueue plugin. That's why I added this #124669 unit test alongside this change, to ensure that the plugin and the field stay in sync as we will now rely on derived field.
There was a problem hiding this comment.
@Huang-Wei, any concern with just using the .Gated field directly?
No concern on my side. .gated is a field to store the state of calling all PreEnqueue plugins on the pod - mostly prior to adding the pod to activeQ.
|
Right, it can be added in a separate PR, so that it's not part of the cherry-pick. |
k8s-ci-robot
added
size/M
Minor changes to make integ tests compatable with 1.29: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - verify pods scheduled, rather than call ScheduleOne - remove PodsInActiveQ check, as not public interface
Minor changes to make integ tests compatable with 1.29: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - remove PodsInActiveQ check, as not public interface
Changes to make integ tests compatable with 1.29: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated
Changes to make integ tests compatable with 1.28: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated - use wait.Poll rather than wait.PollUntilContextTimeout
Changes to make integ tests compatable with 1.27: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated - use wait.Poll rather than wait.PollUntilContextTimeout
Changes to make integ tests compatable with 1.28: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated - use wait.Poll rather than wait.PollUntilContextTimeout - update scheduling_queue_test type
Changes to make integ tests compatable with 1.29: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated
Changes to make integ tests compatable with 1.28: - omit rename of "want" to "schedule" to reduce diff - call Scheduler.Run(), as ScheduleOne is not public interface - only verify pods scheduled; don't call ScheduleOne - Replace PodsInActiveQ check with PodSchedulingGated - update scheduling_queue_test type
cherry pick #124618 to 1.28
cherry pick #124618 to 1.29
cherry pick #124618 to 1.27
cherry pick #124618 to 1.30
…ing" The main part of PR kubernetes#124618 was adding this if check: pkg/scheduler/internal/queue/scheduling_queue.go: movePodsToActiveOrBackoffQueue if pInfo.Gated { continue } This was supposed to shortcut expensive work. But if a pod is gated because a plugin's PreEnqueue return false, then the event that caused movePodsToActiveOrBackoffQueue to be called must not be ignored for the pod. PreEnqueue has to be called for the pod again to check whether it is now scheduleable. This affects DRA when using claim templates. This is independent from using classic DRA or structured parameters, in both cases a pod gets created, then the claim, and pod scheduling can only start once the claim exists. Depending on timing, the scheduler sees the pod update (because the claim name is recorded in status) or the claim add first. If it first sees the pod update, the pod gets stuck because the claim is still unknown. Then when the claim add event is processed, the pod gets skipped because of the check above and remains stuck.
What type of PR is this?
/kind bug
What this PR does / why we need it:
When there are many gated pods and many events which trigger requeueing from the unschedulable pool (e.g. deletePodFromCache) , we observe increased scheduling latency. We mitigate this issue by exiting before hitting the hotspots identified in #124384.
Which issue(s) this PR fixes:
Fixes #124384
/sig scheduling
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: