🐛 fix priority queue ordering when item priority changes

[zach593]

This PR fixes an ordering issue in the priority queue: items should be ordered by priority first, and by enqueue time (FIFO) within the same priority. When an item’s priority changes, the queue previously didn’t consistently preserve this rule. The PR corrects the behavior and adds corresponding test case.

Note: @alvaroaleman may want to review the semantic change to addedCounter.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zach593
Once this PR has been reviewed and has the lgtm label, please assign fillzpp for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @zach593. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[sbueringer]

/ok-to-test

[alvaroaleman]

You write items should be ordered by priority first, and by enqueue time (FIFO) within the same priority but you don't provide any justification for that. We generally take the "best" of all Adds we've over observed for a given key, where "best" means whichever gets the item to the front of the queue fastest: Highest priority, lowest after. Why should the queue behave differently when it comes to the addedCounter?

/hold

[zach593]

Good question. The most obvious benefit of the priority queue feature is to optimize controller startup by deferring low-priority events, thereby reducing externally perceived downtime. Based on this scenario, consider the following example.

Assume there is a single worker and 26 items, from A to Z. After the initial list completes, all items are enqueued into the priority queue in alphabetical order. While A is being processed, Z receives an update event, followed by Y, and then X. As a result, these items are moved into the high-priority queue, but the ordering becomes:

High priority: X, Y, Z
Low priority:  B, C, D, …, W

This behavior is unintuitive. Although the items trigger update events in the order Z, then Y, then X, the actual processing order is still dominated by the original alphabetical enqueue order rather than the update time.

From an external observer’s perspective, items in the low-priority queue are often ones that can reasonably be ignored, while the items that truly deserve attention are those that have triggered update events. However, for these important items, the controller does not behave in a FIFO manner. This is particularly surprising given that many users in the Kubernetes ecosystem are aware that controllers are internally implemented as queues and therefore naturally expect FIFO semantics.

Additionally, once the queue (including the low-priority queue) has been completely drained, and we later observe the same update sequence again, the ordering in queue becomes Z, Y, X, which is actually the expected FIFO order. However, this makes the controller’s behavior appear inconsistent to external observers, because the ordering differs significantly from the earlier case where low-priority items were still present in the queue.

[alvaroaleman]

Stefan and I had a longer discussion around this and arrived at the conclusion that the change makes sense, because it unbreaks the use-case of "While items from initial list are being processed, incoming update events are processed in fifo order" (which I think you tried to allude to, but I didn't understand initially). This is currently broken, since the items in the initial list are not ordered by anything useful.

For consistency, one could argue it would also make sense to do the same for requeueAfter, but that would break the current "Resync resets after" and any other code that wants to lower after but doesn't know the current priority so we leave it as-is for the moment but can revisit later if someone comes up with a good reason.

Could you please add a go doc about this change and the motivating use-case somewhere?

/hold cancel