🐛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

k8s-ci-robot · 2025-02-11T14:38:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

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

 }

 // Update implements EventHandler.
 func (e *TypedEnqueueRequestForObject[T]) Update(ctx context.Context, evt event.TypedUpdateEvent[T], q workqueue.TypedRateLimitingInterface[reconcile.Request]) {
 	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

@@ -199,3 +205,23 @@ func (w workqueueWithCustomAddFunc[request]) Add(item request) {
 func isObjectUnchanged[object client.Object](e event.TypedCreateEvent[object]) bool {
 	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

@@ -82,10 +83,72 @@ type TypedEventHandler[object any, request comparable] interface {
 	Generic(context.Context, event.TypedGenericEvent[object], workqueue.TypedRateLimitingInterface[request])
 }

-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?

Signed-off-by: Troy Connor <[email protected]>

k8s-ci-robot · 2025-02-20T22:53:31Z

@troy0820: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	`29780d9`	link	false	`/test pull-controller-runtime-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alvaroaleman · 2025-02-20T23:02:15Z

pkg/handler/eventhandler.go

+type Funcs = funcs[client.Object, reconcile.Request]
+
+// funcs implements eventhandler
+type funcs[object client.Object, request reconcile.Request] struct{}


Actually this is not correct, right now this is just a re-implementation of EnqueueRequestForOwner 😅 We need to have public properties for CreateFunc/UpdateFunc/DeleteFunc/GenericFunc and delegate to them just like we do in TypedFuncs

Does Funcs need to be it's own type? I can't add methods to Funcs if it's a TypedFuncs[client.Object, reconcile.Request] and declaring CreateFunc methods causes an overflow of calls if I try to inject it in the h.CreateFunc(ctx, e, q) call. 🤔

Do you want the default behavior of the priorityqueue to be based on if the CreateFunc is not defined?

Do you want the default behavior of the priorityqueue to be based on if the CreateFunc is not defined?

I think if the CreateFunc is not defined we should just not do anything just like we do today.

Maybe a better approach is to use Reflect in TypedFuncs to figure out if object is assignable to client.Object and request is a reconcile.Request and if so, use the helpers you added for EnqueueRequestForObject? I did some of that in the Builder because For there can only be used if request is a reconcile.Request

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

implement priority queue for handlers

29780d9

Signed-off-by: Troy Connor <[email protected]>

troy0820 force-pushed the troy0820/priority-queue-on-enqueue-handler branch from 8d804ba to 29780d9 Compare February 20, 2025 22:47

alvaroaleman reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

troy0820 commented Feb 10, 2025 •

edited

Loading

troy0820 commented Feb 10, 2025

k8s-ci-robot commented Feb 11, 2025

troy0820 commented Feb 12, 2025

alvaroaleman commented Feb 12, 2025

troy0820 commented Feb 12, 2025

alvaroaleman Feb 17, 2025

troy0820 Feb 18, 2025

alvaroaleman Feb 19, 2025

alvaroaleman Feb 19, 2025

alvaroaleman Feb 19, 2025

troy0820 Feb 19, 2025

alvaroaleman Feb 19, 2025

troy0820 Feb 20, 2025

alvaroaleman commented Feb 19, 2025

k8s-ci-robot commented Feb 19, 2025

sbueringer Feb 20, 2025

troy0820 Feb 20, 2025

sbueringer Feb 20, 2025 •

edited

Loading

troy0820 Feb 20, 2025

sbueringer Feb 20, 2025

sbueringer Feb 20, 2025

alvaroaleman Feb 20, 2025 •

edited

Loading

k8s-ci-robot commented Feb 20, 2025

alvaroaleman Feb 20, 2025

troy0820 Feb 20, 2025

alvaroaleman Feb 20, 2025

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

Are you sure you want to change the base?

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

Conversation

troy0820 commented Feb 10, 2025 • edited Loading

troy0820 commented Feb 10, 2025

k8s-ci-robot commented Feb 11, 2025

troy0820 commented Feb 12, 2025

alvaroaleman commented Feb 12, 2025

troy0820 commented Feb 12, 2025

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

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 Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troy0820 commented Feb 10, 2025 •

edited

Loading

sbueringer Feb 20, 2025 •

edited

Loading

alvaroaleman Feb 20, 2025 •

edited

Loading