🐛 Priorityqueue: Yet another queue_depth metric fix #3085
Conversation
k8s-ci-robot added the cncf-cla: yes 
| [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman 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 |
k8s-ci-robot added approved 
| /cherrypick release-0.20 |
| @alvaroaleman: once the present PR merges, I will cherry-pick it on top of In response to this:
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. |
| if item.ReadyAt != nil && (readyAt == nil || readyAt.Before(*item.ReadyAt)) { | ||
| if readyAt == nil { | ||
| if readyAt == nil && !w.becameReady.Has(key) { | ||
| w.metrics.add(key) |
There was a problem hiding this comment.
Just to be sure.
Do we also have to add it to becameReady here?
w.becameReady.Insert(item.Key)I'm not sure if we otherwise still have another edge case where we count twice
Should we maybe simply always add it to becameReady when we call metrics.add?
If I see correctly we always remove it from there when we get the item out of the queue, so it should be fine to always add it?
(might also make sense to rename becameReady to something that expresses that an item is currently counted in the queue depth or something)
There was a problem hiding this comment.
The reason that isn't needed is that in spin we only call add if the item has a non-nil readyAt wheras here we only call it here if we are going to set ReadyAt to nil which is how mutual exclusivity this way round is ensured.
I've extended the two last tests that validate that we do call metrics.add here to do a queue.get at the end which ensures the codepath that potentially calls add again in spin gets execercised so that we validate the case you described.
I do agree the current name isn't great but I couldn't come up with a better one - calledAdd would be confusing because this is only relevant for items that have a RequeueAfter and because we remove from it when we do the metrics.get. If you have ideas for a better name here, I am all ears :)
There was a problem hiding this comment.
Also no better ideas for the name :)
Inside the priorityqueues `spin` we call the metrics `add` if an item becomes ready so that the `queue_depth` metric gets incremented. To avoid doing this multiple times for the same item, we track the key in a map and remove it there when we hand the item out. If an item gets added without `RequeueAfter` that is already on the queue but with a `RequeueAfter` we also call the metrics `add` - But if we already did that in `spin` we will count the item twice.
k8s-ci-robot added size/M 
| Thank you! /lgtm |
k8s-ci-robot added the lgtm | LGTM label has been added. Git tree hash: 81504124b34419388ffa46f2d7ac946a97c79085 |
| @alvaroaleman: #3085 failed to apply on top of branch "release-0.20": In response to this:
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. |
| Let's try again after the other cherry-pick is merged |
| /cherrypick release-0.20 |
| @sbueringer: new pull request created: #3089 In response to this:
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. |
4 participants
Inside the priorityqueues
spinwe call the metricsaddif an item becomes ready so that thequeue_depthmetric gets incremented. To avoid doing this multiple times for the same item, we track the key in a map and remove it there when we hand the item out.If an item gets added without
RequeueAfterthat is already on the queue but with aRequeueAfterwe also call the metricsadd- But if we already did that inspinwe will count the item twice.