Repository navigation
Priority queue feature request: priority preservation #3295
Description
Your priority queue implementation is great, and seemingly the only one out there!
We use it in KubeVirt, but found out it could use a small but powerful feature: preserving priority when re-enqueuing an existing item.
For example, Add() could look for the item in the queue and re-use its priority if found (whereas AddWithOpts() would override it).
Another simple solution that wouldn't disturb existing code would be to write a new AddWithExistingOpts() function.
Even simpler, if there was a GetItemPriority() or GetItemOpts() function, we could implement the priority preservation logic in our code.
In practice, that would allow our informer callbacks to re-enqueue items without overriding their priority.
If you'd be willing to accept any of the above, I'd be more than happy to open a PR!
Activity
sbueringer commentedon Aug 21, 2025
Just to double check. I think we have roughly that behavior in the controller, but that is not what you are looking for, right?
It was implemented here: #3167
jean-edouard commentedon Aug 21, 2025
Right, we do that too in our main loop when re-enqueuing items that need further processing.
However, when an informer callback enqueues an object by its key, it doesn't have that context.
alvaroaleman commentedon Aug 21, 2025
Thinking about this a bit, while technically possible I don't think its a good idea. Imagine you have a key already in the workqueue with low priority and then another source records an event relevant to the key but doesn't have PriorityQueue integration so just calls
Add()- At that point, we currently use the default priority if zero, if we changed things to retain the priority, that item would retain its old, low priority.I thought for a moment if maybe adding an
RetainExistingPriorityknob on theAddOptionswould be a good idea, but the problem is that one could supply that and also supply a priority and we do not return an error inAddWithOptsso don't really have a good way to deal with that there.But what is the point of that? If you add an item with the exact same opts, nothing happens as items get de-duplicated.
That would not break anything so that is good. But would this be the only use-case?
If you just need something for yourself, you could probably achieve what you want with a tracking wrapper around the PQ btw.
alvaroaleman commentedon Aug 21, 2025
I think in principle this is the best option and ideally, thhe
Prioritywould be defined as a field that takes an enum that is either a number or aRetainPriorityOrDefault. But the only way to do that in golang to my knowledge is to use an unexported interface and implement it with two structs that represent the options, which works but is really ugly IMHO.Maybe we could say that if
RetainExistingPriorityis set, thePriorityis a default that will only be used if the item doesn't already exist? It might be a bit confusing I guess, as it re-defines the meaning of thePriorityfield toDefaultPriorityalvaroaleman commentedon Aug 21, 2025
The PQ btw as of today will use the max of existing, new priority if an item gets added again. Would you be able to briefly describe the usecase that makes you want to use the existing one rather than trying to figure out in your source what the appripriate priority for this event is?
sbueringer commentedon Aug 22, 2025
@alvaroaleman Slightly orthogonal. Do you think it is sustainable long-term that this func does not return an error?
I know we have to call it in Add / AddAfter / AddRateLimited and in these cases we cannot return an error, still wondering if our "own" AddWithOpts should maybe return an error
jean-edouard commentedon Aug 22, 2025
That is actually what I want in my use-case.
Here's more info about what we do. KubeVirt manages VMs and lets you live-migrate them. For that, we have migration objects, and a migration controller that tracks them.
Since migrations are resource-heavy, we only allow a handful to run concurrently. However, a user may request thousands of migrations at once, we'll just process them a handful at a time.
Before we implemented a priority queue, active migrations would get staved of controller cycles, because we kept going through all the pending ones.
But now we have a priority queues, active migrations are high-priority, pending ones are low and it works great!
However, another thing adds to the queue, which is the informer, to make sure we also act on potential VM/migration object changes. And now our problem is that the informer has no way to know if a given key is already in the queue, and at which priority. So it just calls
Addand overrides priorities... If that happens enough times, we're back to square one and active migrations starve.I like that idea a lot! Supplying both
RetainExistingPriorityandPrioritycould just mean "retain priority if the item already exists, use supplied priority if not".Yes, I would like that :)
I thought about that, but is it doable without forking your code? The wrapper would need access to the underlying queue... We'd love to avoid forking!
Thanks a lot for discussing this!!
alvaroaleman commentedon Aug 22, 2025
No,
Adddoes not override the priority. It will end up callingAddWithOptswhich then defaults the priority to zero and the queue internally will keep the max of existing priority, new priority which it sounds like will be the existing in your case, as its higher. In other words it seems like what you are asking for should already work?jean-edouard commentedon Aug 25, 2025
Ah! Missed that part! Why zero though?
Maybe the priority should be a uint, because zero is a pretty high priority in my implementation... But I guess I can bump my priorities to all be higher than zero and it should just work!Thank you!
Edit: actually sub-zero priorities still make sense, ignore my uint comment. I would still like to be able to set an if-not-exist priority, but this should solve my immediate problem, thanks!
alvaroaleman commentedon Aug 25, 2025
In the very beginning it was an uint until I realized it would be good to have a default that is somewhere in the middle, so that people can easily specify priorities that are higher or lower than the default. Zero is the default simply because that is the zero value of an int in golang.
jean-edouard commentedon Aug 25, 2025
Makes sense
alvaroaleman commentedon Aug 25, 2025
@jean-edouard can this be closed?
jean-edouard commentedon Aug 25, 2025
I still think there would be value in an option to provide a fallback priority in case none is already set (i.e. allow defaulting to non-zero), but yes my immediate problem should be solved by just bumping the numbers I use, so feel free to close.
Thanks again for all the help!
alvaroaleman commentedon Aug 26, 2025
Please open a dedicated issue for that then and describe the use-case for which you need a different default