-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[release-0.22] 🐛 priority queue: properly sync the waiter manipulation #3371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release-0.22] 🐛 priority queue: properly sync the waiter manipulation #3371
Conversation
As described in kubernetes-sigs#3363, there are some circumstances under which `GetWithPriority` is not returning the correct/expected element. This can happen when a `GetWithPriority` is executed and the `Ascend` of the queue is not completed yet, causing not all the items of the BTree to evaluate the same w.waiters.Load() value. Adding a lock to manipulate the waiters will solve the issue. Since the lock is required, there is no need to use an atomic.Int64 anymore. Signed-off-by: fossedihelm <[email protected]>
|
@alvaroaleman Can we backport this up to 0.20? And maybe ask for a 20.z release? Thank you! |
This patch doesn't cleanly apply there and there are a number of fixes that are not in that version like for example #3338 or #3340. What keeps you from upgrading your k8s deps version? |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 08c76a1c0d5433a9544fbfabf27b5352f4405d0e
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer 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:
Approvers can indicate their approval by writing |
|
I'm also hesitant to backport it that far at this point. You might be able to copy the current PQ implementation, create a PQ instance yourself and then pass it into the manager. |
@alvaroaleman Actually is a mix or k8s dependencies / minimum go version and stable branches. |
As described in #3363,
there are some circumstances under which
GetWithPriorityis not returning the correct/expected element.
This can happen when a
GetWithPriorityis executedand the
Ascendof the queue is not completed yet,causing not all the items of the BTree to evaluate
the same w.waiters.Load() value.
Adding a lock to manipulate the waiters will solve the issue.
Since the lock is required, there is no need to use an
atomic.Int64 anymore.
Cherry-pick of #3368
This cherry-pick does not include the corresponding test, because it uses synctest which is only available in go 1.25 and we use go 1.24 here. Given that all changes have to pass CI and be merged into
mainfirst, I think this is acceptable./assign @sbueringer