KEP-5142: Pop pod from backoffQ when activeQ is empty

[macsko]

• One-line PR description: Add KEP-5142

• Issue link: Pop pod from backoffQ when activeQ is empty #5142

• Other comments:

[macsko]

/cc @dom4ha @sanposhiho @Huang-Wei @alculquicondor

I haven't finished all the points yet, but the general idea is there.

[sanposhiho]

Though there're some sections with TODO, looking good overall, aligned with the discussion we had in the issue.

[sanposhiho]

/cc @wojtek-t

[wojtek-t]

ack - waiting for sig buy-in (the PRR is looking good enough, I'm just not sure I understand the change itself - see my comment below).

[sanposhiho]

Looking mostly good for me, left only nits.

/cc @alculquicondor @macsko @Huang-Wei

[sanposhiho]

can we start it from beta per the discussion at the issue? i.e., enable from the day1, but we have a feature gate for a safe guird. wdyt

[macsko]

I think we could do this.

[k8s-ci-robot]

@sanposhiho: GitHub didn't allow me to request PR reviews from the following users: macsko.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Looking mostly good for me, left only nits.

/cc @alculquicondor @macsko @Huang-Wei

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-sigs/prow repository.

[sanposhiho]

I meant @dom4ha

[dom4ha]

I think we have to distinguish unschedulable from error cases in the first version, otherwise we risk the system overload due to scheduler sending retries storm.

[macsko]

Right, especially if we decide to start from beta directly, it will be implemented in the first version.

[dom4ha]

I assumed we can leave the logic of incrementing scheduling attempts only after the backoff time expires to avoid rapid increase of the backoff time for pods triggered by large number of events.

It's probably debatable, because the side effect is increasing the chance of pod starvation as described here (pod would get back with it's former backoff timeout, unless it expired).

The approach proposed here would make the backoff more natural, but would cause reporting way higher (in the order of magnitude) values for scheduling attempts.

[dom4ha]

increasing attempts number.

It's debatable. It can cause rapid increase in the backoff time for some pods (when many event trigger retry), so I was thinking about computing the backoff based on the original approach to not change the logic too much. In other words, not increase scheduling attempts if all of them happened within the last backoff time window. This way this mechanism would be just a best effort improvement rather than a new approach.

[sanposhiho]

It can cause rapid increase in the backoff time for some pods (when many event trigger retry)

I believe we should increment the attempt number.
It is exactly an intentional behavior to move the pods to backoffQ immediately and make a lot of scheduling retries (regardless of whether with or without this feature), if many events are coming and each triggers the pod's requeueing.
As long as QHints determine the event might make the pod schedulable and then move this pod to backoffQ, this Pod should be able to get retried at any timing, regardless of with or without backoff time.
In case this tons of retries don't make the pod schedulable after all, and make the pod's backoff time much longer, that's the fault of QHint accuracy, not this feature.

[sanposhiho]

The only concern is the backoff time for pods getting error (not unschedulable status) because their scheduling failure isn't solved by events, but solved by some other factors (e.g., the networking issue etc).
In this case, it could happen: pod gets error status -> enqueued directly to backoffQ -> the scheduler pops this pod from backoffQ directly immediately (this feature) -> ... -> the attempt counter is incremented every time and the backoff time gets too long.
But, as the KEP mentioned, we'll address this point along with kubernetes/kubernetes#128748

[dom4ha]

We had a followup discussion with @macsko, so this is what we came up with:

We can count errors and unschedulable attempts separately, but we need two queues - for errors and for unschedulable pods, since the feature applies only to unschedulable pods (scheduling errors need the original backoff logic for rate limiting).

We agreed that it may be fine to increment attempts on every pop (we need to be prepared to see much higher values for attempts counter than before), although there are a few important things:

1. the interesting is the worst case (when active queue is permanently non-empty), so we think the existing mechanism should still apply and move expired items to the active queue - to sort them by priority.

2. since the original mechanism was triggered every 1s, the pods within this second got sorted in the active queue by priority. If we pop just based on expiration time, we would loose this, so we could have sorting by priority within every full second to reflect the current behavior.

Unfortunately I saw a case where max backoff of 10s was constantly moving big number of high priority pods to active, which were processed longer than these 10s, causing starvation of lower priority pods. So this feature does not help in this scenario directly, but it does indirectly, because we should be able to safely increase the default value to something like 120s without risking any pod would need to wait that long in practice.

[sanposhiho]

since the original mechanism was triggered every 1s, the pods within this second got sorted in the active queue by priority.

Yeah, this is the point I missed. So,

so we think the existing mechanism should still apply and move expired items to the active queue - to sort them by priority.

do you mean, instead of just directly popping pods out of backoffQ when activeQ is empty, we'd once move pods from backoffQ to activeQ, let activeQ sort them, and restart popping from activeQ? I.e., when activeQ is empty, trigger flushBackoffQCompleted forcibly?

[macsko]

do you mean, instead of just directly popping pods out of backoffQ when activeQ is empty, we'd once move pods from backoffQ to activeQ, let activeQ sort them, and restart popping from activeQ? I.e., when activeQ is empty, trigger flushBackoffQCompleted forcibly?

He just emphasized that we'll still keep the old flushing from backoffQ to activeQ apart from new popping.

Sorting by priority for popping when activeQ is empty can be done by changing sorting key of backoffQ's heap - we could ignore the milliseconds in backoff time expiration and add priority to comparison. It would probably also require to align those windows with flushing windows, so to trigger the flush every full second.

[sanposhiho]

He just emphasized that we'll still keep the old flushing from backoffQ to activeQ apart from new popping.

Yes agree, we definitely should keep that.

by changing sorting key of backoffQ's heap - we could ignore the milliseconds in backoff time expiration and add priority to comparison.

Aha, that looks awesome. So, like sorting by the expiration time first (but ignore miliseconds), and sorting by the priority next, right?

[macsko]

So, like sorting by the expiration time first (but ignore miliseconds), and sorting by the priority next, right?

Exactly, I added it to the kep, so please take a look.

[haosdent]

metric with an activeQ queue

Should it be backoffQ?

[macsko]

This metric shows the pod movement between queues, where the queue means the destination. As pod is already in backoffQ, its destination have to be activeQ (we just skip putting the pod there, but treat it as it would be in activeQ).

[haosdent]

Regarding point 2, it means waiting for the pods in ActiveQ, right?

[macsko]

It will be waiting for pods both from activeQ and backoffQ, whatever will be first.

[wojtek-t]

Forgive me my ignorance, but if pods were unschedulable, wouldn't it be put into the unschedulableQ instead of backoffQ?
Also - if it was unschedulable, the we need some cluster state change to even wish it could become schedulable.

[sanposhiho]

You're right for pods that were rejected by Unschedulable status.
But, for example, when a network error happened and the pod is rejected, in this case, the pod would get Error status, and directly put in backoffQ, skipping unschedQ. This is because we cannot see when to requeue this pod (i.e., when the network failure is solved) by the cluster event. So, we just put such pod to backoffQ directly to keep it in the scheduling queue for a while, and retry it afterwards with the hope that the network failure is gone now.

What here is saying is that it's better not to skip backoff of such pods that got Error status. Because if we retry those pods immediately, the network instability is likely not yet solved. Rather if, for another example, an external component that is used during the scheduling is overwhelmed and pods are getting Error at the scheduling or binding cycles, to keep retrying those pods with Error status might result in a worse situation. (that would put more load on the component unnecessarily.)
We want to skip backoff time of pods that were in the queue due to Unschedulable status.

[wojtek-t]

What here is saying is that it's better not to skip backoff of such pods that got Error status. Because the sooner we retry those pods, the more likely the network instability is not yet solved.

Agree with that.

We want to skip backoff time of pods that were in the queue due to Unschedulable status.

This is what I don't understand. Aren't those pods supposed to live in unschedulableQ instead of backoffQ? If they are unschedulable, I thought they are added to unschedulableQ instead.

[sanposhiho]

Those pods are put in unschedQ first, and then events trigger requeueing to backoffQ.
e.g., If a pod is rejected by the resource fit plugin, it is, first, put in unschedQ. Then, let's say new node is created, and the scheduler notices it. The queue would move this pod from unschedQ to backoffQ.
So, backoffQ could have those two types of pods; pods that got rejected by Error status, and pods that got rejected by Unschedulable status and moved from unschedQ by some event.

[wojtek-t]

Then, let's say new node is created, and the scheduler notices it. The queue would move this pod from unschedQ to backoffQ.

Ok - that was the bit I was missing. This makes sense now - thanks!

[dom4ha]

The reason why there is the transition from the unschedulableQ -> backoffQ (instead of going to activeQ directly) is that the mechanism of triggering this transition not super precise for performance reasons. So it triggers whenever there is a chance the received cluster event may make an unschedulable pod schedulable, but scheduler may very often assess the pod unschedulable again, so we have so called a wasted cycle.

So the backoffQ serve the purpose of a buffer so that scheduling reattempts won't consume all scheduler cycles (and starve out other schedulable pods). However, it introduces scheduling delay if an event indeed made a pod schedulable. This is why we propose to pop from the backoff queue whenever activeQ is empty (the scheduler is idle anyway)

[wojtek-t]

That makes sense - thanks!

[wojtek-t]

ack - waiting for sig buy-in (the PRR is looking good enough, I'm just not sure I understand the change itself - see my comment below).

[sanposhiho]

/lgtm

[dom4ha]

/lgtm

[macsko]

/assign @alculquicondor
for approval

[dom4ha]

/lgtm

[alculquicondor]

/approve

[wojtek-t]

/lgtm
/approve PRR

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, macsko, sanposhiho, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-scheduling/OWNERS [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dom4ha]

Nice work! Thanks Maciek!