Obsolete resource handling for read-cache-after-write

[csviri]

• Handles ghost resources
• Adds a new scheduled executor service to share between those (later we can use this also for TimerEventSource)

Signed-off-by: Attila Mészáros a_meszaros@apple.com

[shawkins]

edit: started my initial review with seeing this had been associated with #3208. Based upon that description this is for when a resource is added to the temporary cache, then deleted by another actor, and a relist happens before the either the new or the delete events are received.

Those stale entries won't actually get used because of the logic in ManagedInformerEventSource.get, but if this happens a sufficient number of times for resources that never again receive an event it can be a memory leak.

In addition to something like this, ManagedInformerEventSource.get could be refined.

in temporary	in cache	outcome
yes	yes	temporary used if later, but the stale entry is not proactively removed otherwise
yes	no	nothing returned - but could reason that this is a creation in temporary, not yet in the cache (using the caches last resource version)
no	yes	cache is used - but could reason over deletion in temporary
no	no	nothing returned

[csviri]

Those stale entries won't actually get used because of the logic in ManagedInformerEventSource.get, but if this happens a sufficient number of times for resources that never again receive an event it can be a memory leak.

I don't see why those would not be used:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java

Lines 191 to 200 in cfe42bb

	Optional<R> resource = temporaryResourceCache.getResourceFromCache(resourceID);
	var res = cache.get(resourceID);
	if (comparableResourceVersions
	&& resource.isPresent()
	&& res.filter(
	r -> ReconcilerUtilsInternal.compareResourceVersions(r, resource.orElseThrow()) > 0)
	.isEmpty()) {
	log.debug("Latest resource found in temporary cache for Resource ID: {}", resourceID);
	return resource;
	}

If there is a resource in TemporaryResourceCache (TRC), it will return that value.

This PR ensures that as you mentioned that we miss the delete event because of re-list, we eventually remove those resources.

The related PR in fabric8 client will allow us to ensure that we always get an up-to-date resource version (vi bookmark), even if there are no further resources changes related to this informer. So we can clear old these old resources from TRC.

temporary used if later, but the stale entry is not proactively removed otherwise.

Stale entries are removed as a result of an event. This PR ensures that also those for which we miss the DELETE event are removed too. Not sure why we would not proactively remote those?

[shawkins]

If there is a resource in TemporaryResourceCache (TRC), it will return that value.

edit: I was initially reading that effectively as isPresent, not isEmpty. So if the entry doesn't exist, we'll still use the temporary version. Yes, that logic should change to not even look for the cache entry, but to test the lastest resource version known to the informer.

Stale entries are removed as a result of an event. This PR ensures that also those for which we miss the DELETE event are removed too. Not sure why we would not proactively remote those?

That goes back to the original form of the comment, if you were trying to keep the temporary cache as small as possible - if so, then anytime we determine an entry is stale, there's no reason not to remove it.

[csviri]

Yes, that logic should change to not even look for the cache entry, but to test the lastest resource version known to the informer.

you mean the lastSyncVersion or latestResourceVersion from TRC ? hmm but I guess that does not matter that much in this case

[shawkins]

you mean the lastSyncVersion or latestResourceVersion from TRC ? hmm but I guess that does not matter that much in this case

I mean the cache lastSyncResourceVersion - that would allow any relist (regardless of whether list watches are used, and any other client changes) to make the TRC resource seem obsolete.

[csviri]

Yep, will change it also in this pr. Thank you.

We should still clear
the cache as in this PR imo.

Also if we can get the notification on the list from the client will cover that last corner case if there are no further events coming in. But frankly I think we can release without that after this change since the probability that all these circumstances happens is really close to zero.

[shawkins]

Also if we can get the notification on the list from the client will cover that last corner case if there are no further events coming in. But frankly I think we can release without that after this change since the probability that all these circumstances happens is really close to zero.

No it shouldn't hold up the release. You could even consider changing what you have here to be based upon a timer, and you'll be covered regardless.

[csviri]

@shawkins @xstefank @metacosm pls take a look if this makes sense for you this way. Should be fairly polished now.

[csviri]

Yes, maybe ghost resource is better name.

[Copilot]

Pull request overview

This PR enhances the framework’s read-cache-after-write behavior by introducing periodic cleanup of “obsolete” entries in the temporary resource cache, and wiring the cleanup interval into informer configuration.

Changes:

• Add scheduled obsolete-entry detection/removal to TemporaryResourceCache, driven by informer lastSyncResourceVersion().
• Extend informer configuration/annotation to support an obsoleteResourceCacheCheckInterval with a default.
• Update tests and sample logging configs; remove an unused sample dependency.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
operator-framework-core/src/main/java/.../TemporaryResourceCache.java	Adds scheduled obsolete-resource checking and uses informer sync RV for consistency decisions.
operator-framework-core/src/main/java/.../ManagedInformerEventSource.java	Constructs the updated temporary cache and uses sync RV in get(...) logic.
operator-framework-core/src/main/java/.../InformerManager.java	Exposes lastSyncResourceVersion(namespace) to support new cache logic.
operator-framework-core/src/main/java/.../ExecutorServiceManager.java	Adds a scheduled executor accessor/initialization for periodic tasks.
operator-framework-core/src/main/java/.../InformerConfiguration.java	Adds config field + defaults for obsolete check interval.
operator-framework-core/src/main/java/.../InformerEventSourceConfiguration.java	Moves comparable-RV configuration into InformerConfiguration and adds new interval builder method.
operator-framework-core/src/main/java/.../Informer.java	Adds annotation attribute for obsolete check interval.
operator-framework-core/src/test/java/.../TemporaryPrimaryResourceCacheTest.java	Adds tests for obsolete-resource removal behavior.
operator-framework-core/src/test/java/.../InformerEventSourceTest.java	Updates mocks to include new configuration.
operator-framework-core/src/test/java/.../ControllerEventSourceTest.java	Updates config setup for new interval plumbing.
operator-framework-core/src/main/java/.../InformerWrapper.java	Adds getter exposing the underlying informer (used for sync RV retrieval).
sample-operators/.../log4j2.xml	Adjusts logging levels/layouts in samples/tests.
sample-operators/leader-election/pom.xml	Removes org.takes:takes dependency.
test.sh	Adds a helper script to build/load a Docker image into kind.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java:431

• withTemporaryResourceCacheForGhostHandling() creates a real ScheduledExecutorService but it is never shut down, which can leak non-daemon threads and make the test suite hang or become flaky. Keep a reference and shut it down in an @AfterEach/@AfterAll, or inject a mock/test-controlled scheduler similar to the other test cases.

---

You can also share your feedback on Copilot code review. Take the survey.

[csviri]

@shawkins this should be the final version, pls take a look once more if you have the bandwidth

[shawkins]

Looks great @csviri