Skip to content

Reconcile utils new alg#3115

Merged
csviri merged 2 commits intooperator-framework:reconcile-utils-new-algfrom
shawkins:reconcile-utils-new-alg
Jan 14, 2026
Merged

Reconcile utils new alg#3115
csviri merged 2 commits intooperator-framework:reconcile-utils-new-algfrom
shawkins:reconcile-utils-new-alg

Conversation

@shawkins
Copy link
Collaborator

@shawkins shawkins commented Jan 8, 2026

My continued appologies, I'm not having a good day. Via the IDE I still ended up targeting upstream, instead of origin.

That should be cleanup up now. Here's even further refinements to make clearer why I want to differentate the behavior.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2026
@shawkins
Copy link
Collaborator Author

shawkins commented Jan 8, 2026

This is a foundamental question with the new design, that if there is no explicit event filtering, we should propagate the new event for our update or more recent (larger resource version). So this should be false based on that.

@csviri regardless of event filtering my expectation is that is something is added to the cache, it is something that we don't then care to get an event about.

I've updated things here to leave it more up to the caller to decide what to do in each of the cases.

Could you provide an example where someone adds a version to the temporary resource cache, but still wants that event?

@csviri
Copy link
Collaborator

csviri commented Jan 9, 2026 •
edited
Loading

This is a foundamental question with the new design, that if there is no explicit event filtering, we should propagate the new event for our update or more recent (larger resource version). So this should be false based on that.

@csviri regardless of event filtering my expectation is that is something is added to the cache, it is something that we don't then care to get an event about.

I've updated things here to leave it more up to the caller to decide what to do in each of the cases.

Hi @shawkins thx for the PR, tests are failing, can take a look a bit later more in depth into this PR.

Could you provide an example where someone adds a version to the temporary resource cache, but still wants that event?

Yeah, this is up to the discussion, my original idea was to provide a backwards compatible way for the case if some implemented a controller, that at the end of the reconciliation call UpdateControl.patchResource and the event resulted from that patch is triggered the reconiliation - as a I framework I think we cannot make an assumption in general that no one is expecting that it will trigger the reconiliation. So wanted to have a way to have still the guarantee that up to date resources is in cache always, but we want to trigger an event. Therefore I added this option:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/AbstractUpdateControl.java

Lines 20 to 31 in 793a56d

private boolean filterPatchEvent = true;
/**
* The event from resource primary updates are filtered by default, thus does not trigger the
* reconciliation. Setting this to false will turn the filtering off.
*
* @since 5.3.0
*/
public T filterPatchEvent(boolean filterPatchEvent) {
this.filterPatchEvent = filterPatchEvent;
return (T) this;
}

And we pass that flag also to in the reconciler for the utils, like here:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

Line 395 in 793a56d

return ReconcileUtils.serverSideApplyPrimary(context, resource, filterEvent);

However, now I'm thinking - and especially if this makes the implementation simlper - that we could rather just simply have a reSchedule() method without time delay here:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/BaseControl.java

Line 26 in 793a56d

public T rescheduleAfter(long delay) {

So we will produce that event explicitly in the event processor after reconciliation. I would acutally go rather with this option (will update the PR), and let you optimize filtering / caching part as in this PR, if that works for you.

thank you!

@csviri
Copy link
Collaborator

csviri commented Jan 9, 2026

see: 00f3245

@csviri csviri force-pushed the reconcile-utils-new-alg branch from 82b6075 to ec6af52 Compare January 9, 2026 12:43
@csviri
Copy link
Collaborator

csviri commented Jan 9, 2026

Could you provide an example where someone adds a version to the temporary resource cache, but still wants that event?

Ahh there is one more place, when we adding a finalizer we want to receive an event (and having an up-to-date resource) but we can also have there an explicit additional event.

@shawkins
Copy link
Collaborator Author

shawkins commented Jan 9, 2026

Hi @shawkins thx for the PR, tests are failing, can take a look a bit later more in depth into this PR.

Sure, I'll have a look.

I would acutally go rather with this option (will update the PR), and let you optimize filtering / caching part as in this PR, if that works for you.

An immediate re-reconciliation makes sense, and gives more control to the calling logic.

@csviri csviri mentioned this pull request Jan 9, 2026
Merged
@csviri csviri force-pushed the reconcile-utils-new-alg branch from f94b10b to 9c9d44a Compare January 9, 2026 17:21
@shawkins
simplifications and refinements to temporaryresource cache
1501a6e 
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins shawkins force-pushed the reconcile-utils-new-alg branch from ec6af52 to 1501a6e Compare January 10, 2026 14:55
csviri
csviri approved these changes Jan 13, 2026
View reviewed changes
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shawkins , this looks good to me, added just one minor comment, I plan to iterate to handle some edge cases nicer (see todos). Otherwise I think we can merge this if you agree

...operatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java Outdated
// the result is deferred
assertThat(result).isEqualTo(EventHandling.DEFER);
temporaryResourceCache.putResource(nextResource);
temporaryResourceCache.doneEventFilterModify(resourceId, "3");
Copy link
Collaborator

@csviri csviri Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to assert the result here

Copy link
Collaborator Author

@shawkins shawkins Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated that test and added one more.

@shawkins
Copy link
Collaborator Author

shawkins commented Jan 13, 2026

Otherwise I think we can merge this if you agree

Sounds good to me.

@shawkins shawkins force-pushed the reconcile-utils-new-alg branch from 2f99379 to 3c3de5c Compare January 13, 2026 20:56
@shawkins
updating tests
6208e43 
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins shawkins force-pushed the reconcile-utils-new-alg branch from 3c3de5c to 6208e43 Compare January 13, 2026 21:25
@csviri csviri marked this pull request as ready for review January 14, 2026 11:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2026
@openshift-ci openshift-ci bot requested review from csviri and xstefank January 14, 2026 11:27
@csviri csviri merged commit 2f9767d into operator-framework:reconcile-utils-new-alg Jan 14, 2026
21 checks passed
@csviri
Copy link
Collaborator

csviri commented Jan 14, 2026

Awesome, thank you @shawkins !!

@shawkins
Copy link
Collaborator Author

shawkins commented Jan 14, 2026

Thanks @csviri

@csviri csviri linked an issue Jan 15, 2026 that may be closed by this pull request
Utilities to update and cache resources also for secondary resources with comparable resource version #3011
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@csviri csviri Awaiting requested review from csviri

@xstefank xstefank Awaiting requested review from xstefank

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Utilities to update and cache resources also for secondary resources with comparable resource version

2 participants

@shawkins @csviri