Informer pools - #3325
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces initial scaffolding for “event source pooling” (starting with informers) under processing.event.source.pool, likely to enable sharing/reuse of informers across components/controllers.
Changes:
- Added
EventSourcePoolinterface andAbstractEventSourcePoolbase type. - Added
InformerClassifierrecord to key pooled informers by selector/namespace/resource type. - Added initial (currently incomplete)
InformerPoolimplementation and a minor whitespace cleanup inInformerManager.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/pool/InformerPool.java |
Adds a new pool for SharedIndexInformer<?> instances (currently stubbed/incomplete). |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/pool/InformerClassifier.java |
Adds a classifier record intended as the cache key for pooled informers. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/pool/EventSourcePool.java |
Introduces a generic pool interface for event sources. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/pool/AbstractEventSourcePool.java |
Adds a base class placeholder for pool implementations. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java |
Removes an extraneous whitespace line in createEventSource. |
d5157bd to
0297680
Compare
e45bf4c to
1e91a47
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java:109
- informerInfo() currently returns "InformerWrapper [informerInfo]", which looks like a leftover debug label and makes toString()/logs confusing when diagnosing informer issues. It should format consistently without the stray "informerInfo" token.
private String informerInfo() {
return "InformerWrapper [informerInfo" + informer.getApiTypeClass().getSimpleName() + "]";
}
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AlwaysNewInformerPool.java:47
- getInformer() uses informers.containsKey(key) followed by informers.put(key, informer). With a ConcurrentHashMap this check-then-act sequence is not atomic: two concurrent callers can both observe the key as absent, create two informers, and one put will overwrite the other (leaking an informer) without throwing the intended OperatorException. Use putIfAbsent (or compute) to make the registration atomic and stop the newly-created informer on collision.
var key = new ClassifierWithName(controllerName, name, classifier);
if (informers.containsKey(key)) {
throw new OperatorException(
"Informer already registered for controller: "
+ controllerName
operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/standalonedependent/StandaloneDependentResourceIT.java:136
- This test-only ConfigurationService implementation returns null from informerPool(). Even if currently “never accessed”, it’s brittle and can turn into an unexpected NPE if the cloner implementation (or any other default method used here) starts consulting informerPool(). Since the test already has a running LocallyRunOperatorExtension, it can avoid the ad-hoc ConfigurationService entirely and reuse the operator’s real ConfigurationService/resource cloner instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AlwaysNewInformerPool.java:47
getInformerusescontainsKeyfollowed byput, which is not atomic even withConcurrentHashMap. Under concurrent calls for the same (controllerName, event source name, classifier) key, this can still create (and potentially leak) multiple informers and bypass the intended "already registered" exception. UseputIfAbsent(orcomputeIfAbsent) so the check+insert is atomic, and stop the extra informer if another thread won the race.
var key = new ClassifierWithName(controllerName, name, classifier);
if (informers.containsKey(key)) {
throw new OperatorException(
"Informer already registered for controller: "
+ controllerName
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java:109
informerInfo()currently returns"InformerWrapper [informerInfo" + ..., which looks like an accidental leftover token and makestoString()output confusing.
private String informerInfo() {
return "InformerWrapper [informerInfo" + informer.getApiTypeClass().getSimpleName() + "]";
}
|
@SamBarker @k-wall I saw that you added explicit support in kroxy operator to share informers, in case you are interested and have the bandwidth PTAL if this makes sense for you; this should solve that problem out of the box. thank you! |
Thank you for the heads up. We'll take a look and feed back. |
xstefank
left a comment
There was a problem hiding this comment.
approving, I have only opinioned enhancements. Feel free to comment on each and we can either do it in this PR or as followups.
| var classifier = classifier("default"); | ||
| var informer = pool.getInformer(CONTROLLER, ES_NAME, classifier, client); | ||
|
|
||
| var released = pool.releaseInformer(CONTROLLER, ES_NAME, classifier); |
There was a problem hiding this comment.
maybe we could also have pool.releaseInformer(informer)
There was a problem hiding this comment.
I was thinking about this a lot that time, I would rather keep the contract simpler with just the current method; I marked it @experimental ( well @PublicEvolving might be a better annotation) so we can enhance this in the future.
…ources
Introduce an informer pool so that InformerEventSources watching the same
resource type with an equivalent configuration share a single underlying
fabric8 SharedIndexInformer instead of creating one per event source. This
reduces memory usage and the number of watch connections opened against the API
server when many controllers watch the same (secondary) resource type.
- Add an InformerPool abstraction with two strategies:
- DefaultInformerPool (default): reference-counted sharing of informers keyed
by an InformerClassifier (API server URL, resource type / GVK, namespace,
label/field/shard selectors, item store). The informer is created on first
use and stopped only when the last user releases it. informerListLimit is
intentionally not part of the identity; indexers are added independently.
- AlwaysNewInformerPool: never shares (one informer per event source), to opt
out of pooling and keep the previous behavior.
- Resolve the pool via ConfigurationService.informerPool() (an abstract method,
cached as a per-ConfigurationService singleton in AbstractConfigurationService)
and expose ConfigurationServiceOverrider.withInformerPool(...) to select the
strategy.
- Route InformerManager / ManagedInformerEventSource through the pool. Dynamic
event source registration reuses an already-running informer and relies on the
informer replaying its cache to the newly added handler for initial state.
- Add group/version/kind and field-selector support to the classifier.
- Tests: unit tests for InformerClassifier equality and pool reference counting;
integration tests for basic sharing, dynamic registration and de-registration,
each run against both pool strategies.
- Add documentation for informer pooling.
The feature is marked @experimental: the runtime behavior is production-ready,
but the configuration API may still change in a non-backwards-compatible way.
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
…ormerPool Replace DefaultInformerPool's parallel informers/counters maps with a single map to a PooledInformer(informer, referenceCount) record, so the two can no longer get out of sync. Rename AlwaysNewInformerPool to NonSharingInformerPool, which states the behavior directly as the counterpart to the sharing DefaultInformerPool.
Move numberOfInformersForResource and setConfigurationService from the InformerPool interface to AbstractInformerPool: neither is part of what the event sources need, they exist for the framework (config service injection) and for observability. The ConfigurationService plumbing is typed to AbstractInformerPool so nothing has to cast, while InformerManager keeps consuming the narrow interface. Also document what was implicit: the releaseInformer contract (last release stops the informer, the informer is returned even while still running so the caller can remove its own handler) and the InformerClassifier equality that deliberately excludes informerListLimit. Plus small consistency fixes: FieldSelector.toString so field-selector informers are identifiable in classifier log messages, package-private size() on both pools since it is only a test hook, and the restored lock-striping rationale in EventSourceManager.
…ract Follow-up to narrowing InformerPool: since the configuration API only accepts AbstractInformerPool, that is where custom strategies plug in, so say so in the docs and on both types instead of pointing at the interface. Document the getInformer contract, the remaining undocumented one and the sharpest: the returned informer is not started, it may already be running with a populated cache when joining a shared one, and each call has to be paired with exactly one releaseInformer. Mark the two configuration entry points @experimental as well; the annotation only sat on InformerPool, which users no longer touch.
The classifier identified the target cluster by the client's master URL, so two event sources watching the same URL through two different clients shared one informer, the one built from whichever client got there first. The second event source then silently watched through credentials, impersonation and TLS material that are not its own: too narrow and it never syncs, too broad and it sees resources its own client could not read. Compare the client by identity instead. Normally every event source resolves the operator's own client, so all intended sharing is unaffected; two distinct instances are simply never assumed to be interchangeable. Nothing from the client's configuration is included, since classifiers are logged and a credential held here would leak into log and exception messages. With the client in the classifier the client parameter of getInformer became redundant, so createInformer now takes it from the classifier. That also removes the parameter that used to be silently ignored when joining a shared informer. The classifier's identity now depends on getKubernetesClient() returning a stable instance, which AbstractConfigurationService does but the interface default does not, so InformerManager resolves the target client once.
The URL was derived from the classifier's client, so it could never tell two classifiers apart that the client comparison had not already told apart. Worse, being mutable on the client's config, it could only ever split informers that should be shared. Nothing read the accessor either. Its one real use was making a classifier readable in the log and exception messages the pools emit, since a KubernetesClient prints as a bare instance identity. Keep that by hand writing toString and deriving the URL from the client there, leaving the identity with a single source of truth.
…ed informer A pooled informer has a single indexer, but index names were passed through to it unchanged, so its indexer became a namespace shared by every event source using that informer. Two controllers registering the same index name on identically configured event sources hit "Indexer conflict" from the client, failing startup with a message that names neither sharing nor the other controller, and querying a name you never registered could silently return another event source's index. Since the documentation actively teaches addIndexer, a shared constant across two controllers was enough to trigger it. InformerWrapper now qualifies index names with the controller and event source that registered them. Callers keep using their own names: the wrapper is the only place that talks to the informer's indexer, so registration, lookup and removal apply the same qualification. The qualified name keeps both names in it, so client side errors stay diagnosable. Indexers are also removed when an event source releases its informer. A shared informer outlives its users, so it used to keep the index and the capturing index function of every event source that had ever used it, and re-registering an event source against a still shared informer was rejected as a name conflict.
Summary
Introduces an informer pool so that informers backing
InformerEventSources (and the internal controller event source) can be shared across controllers instead of each controller/event source always creating its ownSharedIndexInformer. This reduces the number of watch connections opened against the API server and memory overhead in operators where multiple controllers (or multiple dynamically-registered event sources) watch the same secondary resource type (e.g.ConfigMap,Secret).This is marked
@Experimental: the pooling behavior itself is production ready, but the configuration API may still evolve in a non-backwards-compatible way.What's new
InformerPoolabstraction (processing/event/source/informer/pool)InformerPool— interface for getting/releasing informers, keyed by anInformerClassifier, plus reference counting vianumberOfInformersForResource.InformerClassifier— identifies "equivalent" informer requests: API server URL, label/field/shard selectors, namespace, resource type (or GVK for generic resources), and item store.informerListLimitand indexers are intentionally excluded from identity (a warning is logged if an existing informer is reused with a different list limit; indexers can be added independently to a shared informer as long as names don't collide).AbstractInformerPool— shared logic for building the underlyingSharedIndexInformerfrom a classifier (selectors, field selectors, item store, list limit, exception/stopped handlers) and starting it with the configuredcacheSyncTimeout.DefaultInformerPool(new default) — reference-counted, sharing pool: creates an informer on first request for a given classifier and reuses it for subsequent requests; only stops it once the last user releases it.AlwaysNewInformerPool— opt-out pool that creates a dedicated informer per event source, preserving pre-pooling behavior.Configuration
ConfigurationService.informerPool()— new (non-default) method; implementations must return the same instance on every call so controllers using the sameConfigurationServiceshare the pool.AbstractConfigurationServicecaches aDefaultInformerPoollazily (synchronized to avoid creating more than one instance under concurrent first access).ConfigurationServiceOverrider.withInformerPool(InformerPool)— lets users opt intoAlwaysNewInformerPool(or a custom implementation) to disable/customize sharing.Integration
InformerManagerandInformerWrapperwere reworked to go through the pool instead of creating/starting/stopping theSharedIndexInformerdirectly:InformerManagernow builds anInformerClassifierper namespace and callsinformerPool.getInformer(...)/releaseInformer(...)instead of constructing the informer from the Fabric8 client itself.getInformeronly registers/reference-counts,InformerPool#startblocks (idempotently) until cache sync, andreleaseInformerdecrements the reference count, stopping the informer only when it drops to zero. The event handler is always removed from the returned informer, even when it stays running for other users.InformerWrapperno longer owns lifecycle (start/stopremoved); it now just exposes the informer/classifier and cache-facing behavior.ControllerEventSourceandManagedInformerEventSourceupdated to go through the same pooled path.Docs
Added a new "Sharing Informers Between Controllers (Informer Pool)" section to
docs/content/en/docs/documentation/eventing.mddescribing sharing semantics, what counts toward informer identity, and how to select/override the pooling strategy.Tests
AlwaysNewInformerPool,DefaultInformerPool, andInformerClassifier(equality/identity semantics).informerpool:basic— two reconcilers sharing an informer for the same secondary resource type, parameterized to also run againstAlwaysNewInformerPoolfor regression coverage.deregister— verifies informer lifecycle when an event source is dynamically deregistered while shared.dynamic— verifies dynamically-registered/static shared informers, including replay of existing cache state to a newly attached handler.ControllerTest,EventSourceManagerTest,ReconciliationDispatcherTest,ControllerEventSourceTest,InformerEventSourceTest,MockKubernetesClient,StandaloneDependentResourceIT,WorkflowMultipleActivationIT) updated for the new construction/wiring paths.