✨ Deduplicate events before sending them into the workqueue. by coderanger · Pull Request #1390 · kubernetes-sigs/controller-runtime

coderanger · 2021-02-21T23:23:46Z

This avoids race conditions where extra reconciles can happen rarely.

I still think kubernetes/kubernetes#99185 is a better solution but failing that, here is a "userspace" solution.

This avoids race conditions where extra reconciles can happen rarely.

coderanger · 2021-02-21T23:24:58Z

Replaces #1380 but covers all three handler types instead.

coderanger · 2021-02-23T01:01:41Z

If we got with this approach, should probably rewrite this to use k8s.io/apimachinery/pkg/util/sets.

Also make sure that enqueue_mapped preserves order.

coderanger · 2021-02-27T23:32:26Z

Will keep running them until they fail.
This was attempt #107
No, seriously... you can probably stop now.

Should be good on ordering. Still leaving owners as unordered since the underlying order is meaningless and can be shuffled.

alvaroaleman · 2021-03-01T23:54:13Z

 // Delete implements EventHandler
 func (e *EnqueueRequestForOwner) Delete(evt event.DeleteEvent, q workqueue.RateLimitingInterface) {
-	for _, req := range e.getOwnerReconcileRequest(evt.Object) {
+	reqs := map[reconcile.Request]empty{}


Is this needed here and in Generic? I would have assumed that kube doesn't allow multiple identical ownerRefs

Hmm, I don't actually know what happens if you try to give the same non-controller owner twice.

I bet you could have duplicates, +patchMergeKey=uid so it will theoretically dedup on UID but you could have invalid owner refs with the wrong UID and duplicate names? I would also be 100% fine with not handling that cleanly and risking spurious reconciles since that should be very hard to make happen.

Yeah I would prefer to omit that here and in Generic

If not in Delete nor Generic, why have it in Create?

Looking at the code again, I think we should probably just leave it. It would complicate the flow to only do it sometimes, would have to switch back to returning a slice and then process it again, the extra memory allocations and code complexity seems not worth it.

cbandy · 2021-03-04T06:21:36Z

🦆 I tried to wrap my head around if/why we should dedupe in the single-object events (Create, Delete, Generic). It seems to me that if we do it in the Update event, we should do it in all events, right?

🦆 Should a user expect EnqueueRequestsFromMapFunc to dedupe the one slice their MapFunc returns? If we do it in Update, would it be unexpected for the other events to not?

🦆 Perhaps MapFunc is what it is and there should be a new EventHandler implementation that deals in unordered sets of Request. We could pass in a mutable set and they could call set.Insert themselves. That might avoid one slice and an iteration. (e.g. the Owner implementation here.)

🦆 If workqueue starts accepting slices, we would want to stick with slices... 😖 In that case, the two EventHandler implementations could (would?) have the same semantics.

❓ If workqueue starts accepting slices, would we definitely append the two MapFunc slices in Update and pass them to the queue at once?

🤔 after all that...

I think we should dedupe as little or as much as we like. Any amount is an improvement and approaches the ideal, where it happens directly in workqueue.

Perhaps we should change the documentation on any/all of these Handlers now to allow deduplication to change over time.

alvaroaleman · 2021-03-04T14:34:37Z

I tried to wrap my head around if/why we should dedupe in the single-object events (Create, Delete, Generic). It seems to me that if we do it in the Update event, we should do it in all events, right?

Update events contain two objects, old obj and new obj which is why there is a good chance this might happen. Adding de-duplication for anything else is essentially assuming that users provide buggy mappers. I am apposed to that.

Should a user expect EnqueueRequestsFromMapFunc to dedupe the one slice their MapFunc returns? If we do it in Update, would it be unexpected for the other events to not?

No, see above.

Perhaps MapFunc is what it is and there should be a new EventHandler implementation that deals in unordered sets of Request. We could pass in a mutable set and they could call set.Insert themselves. That might avoid one slice and an iteration. (e.g. the Owner implementation here.)

I am opposed to that. What this change is doing in the first place is a performance optimization that for most controllers shouldn't matter. I am strictly opposed to complicating our API and dev user experience even the slightest for this. Adding a redundant way to do the same thing would fall into the "complicating API" category.

If workqueue starts accepting slices, we would want to stick with slices... In that case, the two EventHandler implementations could (would?) have the same semantics.

It won't, they rejected the upstream PR.

…en both objects.

coderanger · 2021-03-09T22:06:21Z

@alvaroaleman I think this should be all set now?

alvaroaleman

/label tide/merge-method-squash
/lgtm
/approve

Sorry, I didn't have time initially and then it fell off my radar 🙃

k8s-ci-robot · 2021-03-14T16:55:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

alvaroaleman · 2021-03-14T17:03:32Z

/retest

alvaroaleman · 2021-03-14T17:13:24Z

/retest

Update tests for controller-runtime changes from 0.8.2 to 0.9.2(+). Most of these changes react to kubernetes-sigs/controller-runtime#1399, which made the fake client's Delete behave more like the real thing: - If the object has finalizers, set `DeletionTimestamp` but don't actually delete. - If there are no finalizers, actually delete. - Updates that remove the last finalizer from an object with a `DeletionTimestamp` actually delete the object. Then there's kubernetes-sigs/controller-runtime#1390 which deduplicates objects in the reconcile queue for Update actions.

…tes-sigs#1390) * ✨ Deduplicate events before sending them into the workqueue. This avoids race conditions where extra reconciles can happen rarely. * ✨ Switch to map[string]struct{} to reduce memory usage slightly. Also make sure that enqueue_mapped preserves order. * 📝 Update function doc for getOwnerReconcileRequest. * 🎨 Fix up duplication tests and ensure Update for _mapped dedups between both objects.

✨ Deduplicate events before sending them into the workqueue.

cb3c054

This avoids race conditions where extra reconciles can happen rarely.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2021

k8s-ci-robot requested review from gerred and pwittrock February 21, 2021 23:23

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 21, 2021

mars1024 reviewed Feb 23, 2021

View reviewed changes

Comment thread pkg/handler/enqueue_mapped.go Outdated

✨ Switch to map[string]struct{} to reduce memory usage slightly.

341be59

Also make sure that enqueue_mapped preserves order.

alvaroaleman reviewed Mar 1, 2021

View reviewed changes

mars1024 mentioned this pull request Mar 4, 2021

✨ deduplicate repeated requests in a single update event #1380

Closed

mars1024 reviewed Mar 4, 2021

View reviewed changes

Comment thread pkg/handler/enqueue_mapped.go Outdated

Comment thread pkg/handler/enqueue_owner.go

coderanger added 2 commits March 7, 2021 15:55

📝 Update function doc for getOwnerReconcileRequest.

337285a

🎨 Fix up duplication tests and ensure Update for _mapped dedups betwe…

28f3a5d

…en both objects.

alvaroaleman approved these changes Mar 14, 2021

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 14, 2021

k8s-ci-robot assigned alvaroaleman Mar 14, 2021

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

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

k8s-ci-robot merged commit 253f275 into kubernetes-sigs:master Mar 14, 2021

varshaprasad96 mentioned this pull request Jun 9, 2021

Bump/k8s 1.21 operator-framework/operator-lib#64

Merged

varshaprasad96 mentioned this pull request Jul 13, 2021

Bump k8s dependencies to 1.21+ operator-framework/operator-sdk#5057

Merged

2 tasks

rfranzke mentioned this pull request Dec 19, 2024

Reconcile is triggered twice for mapped requests #490

Closed

Conversation

coderanger commented Feb 21, 2021

Uh oh!

coderanger commented Feb 21, 2021

Uh oh!

coderanger commented Feb 23, 2021

Uh oh!

Uh oh!

coderanger commented Feb 27, 2021

Uh oh!

alvaroaleman Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

coderanger Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

coderanger Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

cbandy Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

coderanger Mar 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbandy commented Mar 4, 2021

Uh oh!

alvaroaleman commented Mar 4, 2021

Uh oh!

coderanger commented Mar 9, 2021

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 14, 2021

Uh oh!

alvaroaleman commented Mar 14, 2021

Uh oh!

alvaroaleman commented Mar 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants