🐛Implement priorityqueue as default on handlers if using priorityqueue interface #3111

troy0820 · 2025-02-10T19:35:35Z

Resolves #3105

Handlers:

TypedEnqueueRequestForObject
EnqueueRequestForOwner
EnqueueRequestFromMapFunc

This brings the handler TypedEnqueueRequestForObject , enqueueRequestForOwner and enqueuRequestFromMapFunc handlers to use priority queue if the workqueue.TypedRateLimitingInterface[reconcile.Request] is of type priorityqueue.PriorityQueue[reconcile.Request] without having to use the wrapper WithLowPriorityWhenUnchanged.

NOTE: Added tests implement the same logic to show that this maps to the functionality of the wrapped handler in the tests

/assign @alvaroaleman @sbueringer

troy0820 · 2025-02-10T19:53:35Z

/test pull-controller-runtime-test

troy0820 · 2025-02-12T16:20:37Z

I am not sure if you wanted to do this for the handler *enqueueRequestsFromMapFunc[object, request]) but I can add this here or make another PR with the change.

alvaroaleman · 2025-02-12T16:30:03Z

I am not sure if you wanted to do this for the handler *enqueueRequestsFromMapFunc[object, request]) but I can add this here or make another PR with the change.

Yeah we would want this for all handlers we offer if possible. This PR is on my list but it might take me a few more days to find the time, sorry

troy0820 · 2025-02-12T17:13:52Z

Yeah we would want this for all handlers we offer if possible. This PR is on my list but it might take me a few more days to find the time, sorry

No rush, just wanted to understand the scope/ask of the issue and what work may still need to be done.

pkg/handler/enqueue_mapped.go

alvaroaleman · 2025-02-17T15:36:51Z

pkg/handler/eventhandler.go

+
+// addToPriorityQueueCreate adds the reconcile.Request to the priorityqueue in the handler
+// for Create requests if and only if the workqueue being used is of type priorityqueue.PriorityQueue[reconcile.Request]
+func addToPriorityQueueCreate[T client.Object](q priorityqueue.PriorityQueue[reconcile.Request], evt event.TypedCreateEvent[T], item reconcile.Request) {


Same here, lets just wrap the handler in the constructor

These are functions to not repeat the logic in the TypedEnqueueRequestForObject[T] Create/Update as we do export this type.

Do you want this to have a constructor to wrap with WithLowPriorityWhenUnchanged because this will require adding more constraints to deal with the func signature with ^^ to equal TypedEventHandler[object,request]. Unless I am misunderstanding something.

Sorry I think I just commented on the wrong place here but I see you already updated it 👍

alvaroaleman · 2025-02-19T02:01:02Z

pkg/handler/eventhandler.go

+
+// addToPriorityQueueCreate adds the reconcile.Request to the priorityqueue in the handler
+// for Create requests if and only if the workqueue being used is of type priorityqueue.PriorityQueue[reconcile.Request]
+func addToPriorityQueueCreate[T client.Object](q priorityqueue.PriorityQueue[reconcile.Request], evt event.TypedCreateEvent[T], item reconcile.Request) {


Sorry I think I just commented on the wrong place here but I see you already updated it 👍

alvaroaleman · 2025-02-19T02:02:25Z

pkg/handler/enqueue.go

 switch {
 case !isNil(evt.ObjectNew):
-q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
+item := reconcile.Request{NamespacedName: types.NamespacedName{


Could we also do the same for Funcs? That is the only remaining handler

Not sure if this is how you wanted this done but any feedback is appreciated.

I think in Funcs we have to do the same dance as in TypedEnqueueRequestForObject because we export the type and not a constructor, otherwise the original goal of get this by default won't be achieved

Makes sense, I did what we did with TypedEnqueueRequestForObject because I can't add methods to the Funcs type. So I did the same thing we did with this.

alvaroaleman · 2025-02-19T02:02:41Z

/label tide-merge-method-squash

k8s-ci-robot · 2025-02-19T02:02:44Z

@alvaroaleman: The label(s) /label tide-merge-method-squash cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, ci-short, ci-extended, ci-full. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label tide-merge-method-squash

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 · 2025-02-20T10:21:05Z

pkg/handler/enqueue.go

+priorityQueue, isPriorityQueue := q.(priorityqueue.PriorityQueue[reconcile.Request])
+if !isPriorityQueue {
+q.Add(item)
+return
+}


Should we also move this into the util func? (and rename the func to addToQueueCreate)

Same for update

I can. I would have to move isObjectUnchanged as well. Not sure where to move it if we want that exposed or in some internal directory.

Not sure what you mean :)

I only mean that these 5 lines should be inside of the addToQueueCreate func. I wouldn't move them to another package or export

(the util func == addToQueueCreate)

Gotcha, I misunderstood. I can move them as well to do the check within the function. I did rename them but can move those lines inside.

Sounds good. Just a lot of code we can deduplicate :)

sbueringer · 2025-02-20T10:21:45Z

pkg/handler/eventhandler.go

 return e.Object.GetCreationTimestamp().Time.Before(time.Now().Add(-time.Minute))
 }
+
+// addToPriorityQueueCreate adds the reconcile.Request to the priorityqueue in the handler


Would be good if we can use these two funcs in WithLowPriorityWhenUnchanged

pkg/handler/eventhandler.go

alvaroaleman · 2025-02-20T22:31:33Z

pkg/handler/eventhandler.go

 }

-var _ EventHandler = Funcs{}
+var _ EventHandler = &Funcs{}


I think this can not be a pointer receiver otherwise this is a breaking change 😬 Can we also typecheck that TypedFuncs which is now a different implementation is a TypedEventHandler?

troy0820 · 2025-02-24T21:33:07Z

Test logic needs updating because now that this is wrapped in the WithLow..... tests are showing back with the wrong length for reconcile requests? 🤔 I can debug the test cases but also want to make sure I do not ruin any existing functionality as that would be a breaking change and change that 🌱 into a ⚠️

alvaroaleman · 2025-03-02T14:44:02Z

Test logic needs updating because now that this is wrapped in the WithLow..... tests are showing back with the wrong length for reconcile requests? 🤔 I can debug the test cases but also want to make sure I do not ruin any existing functionality as that would be a breaking change and change that 🌱 into a ⚠️

No, this should have no effect on existing functionality. I can see a bunch of panics in CI, so something seems to not be working correctly.

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

alvaroaleman · 2025-03-09T00:48:48Z

@troy0820 how about you leave out EnqueueRequestFromMapFunc from this change and I will have a look into that one, as it seems a bit more tricky?

troy0820 · 2025-03-09T00:55:19Z

@troy0820 how about you leave out EnqueueRequestFromMapFunc from this change and I will have a look into that one, as it seems a bit more tricky?

That’ll work. I’ll queue up the change in a second.

alvaroaleman · 2025-03-09T01:18:32Z

pkg/handler/eventhandler.go

+if ok {
+evt, ok := any(e).(event.TypedCreateEvent[client.Object])
+if ok {
+item := reconcile.Request{NamespacedName: types.NamespacedName{


This isn't quite correct, because we need to run CreateFunc to know what item looks like, here we basically hardcode the same behavior as EnqueueRequestForObjec - Lets maybe leave the TypedFuncs out as well of this PR?

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

alvaroaleman

Thanks!

k8s-ci-robot · 2025-03-10T02:13:43Z

LGTM label has been added.

Git tree hash: 3518285291eae78304b39908f55205b724196db9

k8s-ci-robot · 2025-03-10T02:13:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, troy0820

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:

~~OWNERS~~ [alvaroaleman]

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

sbueringer

@troy0820 Thx for working on this!

Just one small finding, would be nice if you can follow-up

sbueringer · 2025-03-13T04:14:49Z

pkg/handler/eventhandler.go

 //
 // Unless you are implementing your own TypedEventHandler, you can ignore the functions on the TypedEventHandler interface.
 // Most users shouldn't need to implement their own TypedEventHandler.
-//


This change breaks godoc rendering of TypedEventHandler. Now only l.69 shows up

…e interface (kubernetes-sigs#3111) * implement priority queue for handlers Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> * check object before placing in priorityqueue Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> * implement priority queue Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> --------- Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

k8s-ci-robot assigned alvaroaleman and sbueringer Feb 10, 2025

k8s-ci-robot requested review from alvaroaleman and inteon February 10, 2025 19:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 10, 2025

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 2 times, most recently from f7f9fbe to 753b14a Compare February 11, 2025 18:57

troy0820 changed the title ~~🐛Implement priority queue in EnqueueRequestForObject handler~~ 🐛Implement priorityqueue for EnqueueRequestForObject and EnqueueRequestForOwner handler Feb 11, 2025

troy0820 changed the title ~~🐛Implement priorityqueue for EnqueueRequestForObject and EnqueueRequestForOwner handler~~ 🐛Implement priorityqueue as default on handlers if using priorityqueue interface Feb 12, 2025

alvaroaleman reviewed Feb 17, 2025

View reviewed changes

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from 0f9055e to 8ef6629 Compare February 18, 2025 16:08

alvaroaleman reviewed Feb 19, 2025

View reviewed changes

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 19, 2025

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 3 times, most recently from aff6932 to eaf978d Compare February 19, 2025 20:52

sbueringer reviewed Feb 20, 2025

View reviewed changes

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 4 times, most recently from 10068c8 to 1d97d46 Compare February 20, 2025 22:08

alvaroaleman reviewed Feb 20, 2025

View reviewed changes

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from 1d97d46 to 8d804ba Compare February 20, 2025 22:39

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 2 times, most recently from 7aa17da to 5bbe24d Compare February 21, 2025 22:33

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 2 times, most recently from 58e6a86 to 90b1a3f Compare March 3, 2025 16:53

troy0820 added 2 commits March 3, 2025 12:16

implement priority queue for handlers

109d849

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

check object before placing in priorityqueue

3086f3d

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch 5 times, most recently from 489410a to a71f2cb Compare March 6, 2025 21:14

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from a71f2cb to 4e82bce Compare March 9, 2025 01:02

alvaroaleman reviewed Mar 9, 2025

View reviewed changes

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from 4e82bce to 1d0e50b Compare March 10, 2025 00:18

implement priority queue

16ff4af

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from 1d0e50b to 16ff4af Compare March 10, 2025 00:19

alvaroaleman approved these changes Mar 10, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2025

k8s-ci-robot merged commit f80bc5d into kubernetes-sigs:main Mar 10, 2025
14 checks passed

sbueringer reviewed Mar 13, 2025

View reviewed changes

sbueringer mentioned this pull request Mar 21, 2025

🌱 Fix godoc of TypedEventHandler #3169

Merged

sbueringer mentioned this pull request Oct 3, 2025

Feature: Priority Queue #2374

Open

22 tasks

🐛Implement priorityqueue as default on handlers if using priorityqueue interface #3111

🐛Implement priorityqueue as default on handlers if using priorityqueue interface #3111

Uh oh!

Conversation

troy0820 commented Feb 10, 2025 • edited by alvaroaleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

troy0820 commented Feb 10, 2025

troy0820 commented Feb 12, 2025

alvaroaleman commented Feb 12, 2025

troy0820 commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 19, 2025

k8s-ci-robot commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

troy0820 commented Feb 24, 2025

alvaroaleman commented Mar 2, 2025

alvaroaleman commented Mar 9, 2025

troy0820 commented Mar 9, 2025

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 10, 2025

k8s-ci-robot commented Mar 10, 2025

Uh oh!

sbueringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

troy0820 commented Feb 10, 2025 •

edited by alvaroaleman

Loading

sbueringer Feb 20, 2025 •

edited

Loading

alvaroaleman Feb 20, 2025 •

edited

Loading

sbueringer left a comment •

edited

Loading