From a939c4bafeda365829d4b4a6c5bd09fd5afcafd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Fri, 4 Sep 2026 12:31:56 +0200 Subject: [PATCH 01/11] chore: bump version for 999-SNAPSHOT for next MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- bootstrapper-maven-plugin/pom.xml | 2 +- caffeine-bounded-cache-support/pom.xml | 2 +- micrometer-support/pom.xml | 2 +- migration/pom.xml | 2 +- operator-framework-bom/pom.xml | 2 +- operator-framework-core/pom.xml | 2 +- operator-framework-junit/pom.xml | 2 +- operator-framework/pom.xml | 2 +- pom.xml | 2 +- sample-operators/controller-namespace-deletion/pom.xml | 2 +- sample-operators/kotlin-operator/pom.xml | 2 +- sample-operators/leader-election/pom.xml | 2 +- sample-operators/mysql-schema/pom.xml | 2 +- sample-operators/operations/pom.xml | 2 +- sample-operators/pom.xml | 2 +- sample-operators/tomcat-operator/pom.xml | 2 +- sample-operators/webpage/pom.xml | 2 +- test-index-processor/pom.xml | 2 +- 18 files changed, 18 insertions(+), 18 deletions(-) diff --git a/bootstrapper-maven-plugin/pom.xml b/bootstrapper-maven-plugin/pom.xml index 75c1ebd50d..bcd6c2e743 100644 --- a/bootstrapper-maven-plugin/pom.xml +++ b/bootstrapper-maven-plugin/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT bootstrapper diff --git a/caffeine-bounded-cache-support/pom.xml b/caffeine-bounded-cache-support/pom.xml index 8ae3911352..be70ab9a2e 100644 --- a/caffeine-bounded-cache-support/pom.xml +++ b/caffeine-bounded-cache-support/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT caffeine-bounded-cache-support diff --git a/micrometer-support/pom.xml b/micrometer-support/pom.xml index be864bff8e..ae3c4d0be1 100644 --- a/micrometer-support/pom.xml +++ b/micrometer-support/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT micrometer-support diff --git a/migration/pom.xml b/migration/pom.xml index bdbc8fbc4f..cf5143c925 100644 --- a/migration/pom.xml +++ b/migration/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT migration diff --git a/operator-framework-bom/pom.xml b/operator-framework-bom/pom.xml index 5adbefd8d8..0f974400b1 100644 --- a/operator-framework-bom/pom.xml +++ b/operator-framework-bom/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk operator-framework-bom - 5.6.1-SNAPSHOT + 999-SNAPSHOT pom Operator SDK - Bill of Materials Java SDK for implementing Kubernetes operators diff --git a/operator-framework-core/pom.xml b/operator-framework-core/pom.xml index b0ef5cac6f..a7d06ebdc1 100644 --- a/operator-framework-core/pom.xml +++ b/operator-framework-core/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT ../pom.xml diff --git a/operator-framework-junit/pom.xml b/operator-framework-junit/pom.xml index 4ddbb31b3e..aa18d5c778 100644 --- a/operator-framework-junit/pom.xml +++ b/operator-framework-junit/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT operator-framework-junit diff --git a/operator-framework/pom.xml b/operator-framework/pom.xml index 4f57216ed4..6d314d4687 100644 --- a/operator-framework/pom.xml +++ b/operator-framework/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT operator-framework diff --git a/pom.xml b/pom.xml index 0aaad2e670..c88d2f4588 100644 --- a/pom.xml +++ b/pom.xml @@ -21,7 +21,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT pom Operator SDK for Java Java SDK for implementing Kubernetes operators diff --git a/sample-operators/controller-namespace-deletion/pom.xml b/sample-operators/controller-namespace-deletion/pom.xml index 33cf5ab823..af4be01972 100644 --- a/sample-operators/controller-namespace-deletion/pom.xml +++ b/sample-operators/controller-namespace-deletion/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-controller-namespace-deletion diff --git a/sample-operators/kotlin-operator/pom.xml b/sample-operators/kotlin-operator/pom.xml index 70e5e99d5d..a5ca180cd2 100644 --- a/sample-operators/kotlin-operator/pom.xml +++ b/sample-operators/kotlin-operator/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-kotlin-operator diff --git a/sample-operators/leader-election/pom.xml b/sample-operators/leader-election/pom.xml index 35cc48ca7f..4f896485d1 100644 --- a/sample-operators/leader-election/pom.xml +++ b/sample-operators/leader-election/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-leader-election diff --git a/sample-operators/mysql-schema/pom.xml b/sample-operators/mysql-schema/pom.xml index a7cb233772..d2872c921a 100644 --- a/sample-operators/mysql-schema/pom.xml +++ b/sample-operators/mysql-schema/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-mysql-schema-operator diff --git a/sample-operators/operations/pom.xml b/sample-operators/operations/pom.xml index 37f5a09fdc..239b8a4860 100644 --- a/sample-operators/operations/pom.xml +++ b/sample-operators/operations/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-operations diff --git a/sample-operators/pom.xml b/sample-operators/pom.xml index 0ba6238e45..704007c076 100644 --- a/sample-operators/pom.xml +++ b/sample-operators/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-operators diff --git a/sample-operators/tomcat-operator/pom.xml b/sample-operators/tomcat-operator/pom.xml index e7d8750a90..ea964a2b07 100644 --- a/sample-operators/tomcat-operator/pom.xml +++ b/sample-operators/tomcat-operator/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-tomcat-operator diff --git a/sample-operators/webpage/pom.xml b/sample-operators/webpage/pom.xml index 3ef0f2a11f..d50e5ef03c 100644 --- a/sample-operators/webpage/pom.xml +++ b/sample-operators/webpage/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk sample-operators - 5.6.1-SNAPSHOT + 999-SNAPSHOT sample-webpage-operator diff --git a/test-index-processor/pom.xml b/test-index-processor/pom.xml index b930d1de7d..2ae7c5f454 100644 --- a/test-index-processor/pom.xml +++ b/test-index-processor/pom.xml @@ -22,7 +22,7 @@ io.javaoperatorsdk java-operator-sdk - 5.6.1-SNAPSHOT + 999-SNAPSHOT test-index-processor From 110199c4760fb9f11b106d9f9b070ff85d4e66f3 Mon Sep 17 00:00:00 2001 From: Martin Stefanko Date: Mon, 7 Sep 2026 13:31:04 +0200 Subject: [PATCH 02/11] feat: add a desired state aspect (#3598) Signed-off-by: xstefank --- .../dependent-resources.md | 28 +++++ .../api/config/ConfigurationService.java | 16 +++ .../config/ConfigurationServiceOverrider.java | 42 +++++++ .../api/reconciler/DefaultContext.java | 22 +++- .../dependent/DesiredStateAspect.java | 55 ++++++++ .../BulkDependentResourceReconciler.java | 5 +- .../ConfigurationServiceOverriderTest.java | 34 +++++ .../AbstractDependentResourceTest.java | 47 ++++++- .../GenericKubernetesResourceMatcherTest.java | 18 ++- .../DesiredStateAspectCustomResource.java | 26 ++++ .../DesiredStateAspectIT.java | 117 ++++++++++++++++++ .../DesiredStateAspectReconciler.java | 59 +++++++++ 12 files changed, 465 insertions(+), 4 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DesiredStateAspect.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectReconciler.java diff --git a/docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md b/docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md index 538c52c00c..3ac1e88e71 100644 --- a/docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md +++ b/docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md @@ -531,6 +531,34 @@ samples [here](https://github.com/java-operator-sdk/java-operator-sdk/tree/main/ in [related integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/orderedmanageddependent/ConfigMapDependentResource2.java) . +### Adding Common Metadata to All Managed Resources (Desired State Aspects) + +Operators often need to mark every resource they manage in a uniform way, for example with a +`app.kubernetes.io/managed-by` label, so that these resources can easily be identified, selected or +garbage-collected later on. Instead of repeating that logic in every `desired()` implementation, a +`DesiredStateAspect` can be registered once, at the operator level, and is then applied to the +desired state of every Kubernetes dependent resource managed by the operator: + +```java +Operator operator = new Operator(overrider -> overrider + .withDesiredStateAspects(List.of( + (desired, dependentResource, context) -> desired.getMetadata().getLabels() + .put("app.kubernetes.io/managed-by", "my-operator")))); +``` + +Aspects are applied, in registration order, right after the desired state has been computed and +before the desired state is matched against the actual resource, created or updated. As a +consequence, the metadata added by an aspect is part of the desired state proper: if it is removed +from the actual resource, or if the aspect itself changes, the associated secondary resources are +updated accordingly on the next reconciliation. + +Since the desired state is computed at most once per reconciliation and cached in the `Context`, +aspects are called at most once per dependent resource and reconciliation. They are only called for +dependent resources whose desired state is a `HasMetadata`, meaning that external (non-Kubernetes) +dependent resources are left untouched. Implementations are expected to modify the provided desired +state in place and need to be thread-safe as they can be called concurrently for different primary +resources. + ## "Read-only" Dependent Resources vs. Event Source See Integration test for a read-only diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index db1b9a5fa5..35f46e5019 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -16,6 +16,7 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -41,6 +42,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Experimental; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; @@ -539,4 +541,18 @@ default InformerPool informerPool() { pool.setConfigurationService(this); return pool; } + + /** + * Retrieves the {@link DesiredStateAspect}s applied to the desired state of all the Kubernetes + * dependent resources managed by the operator. Aspects are applied in the order in which they are + * returned, right after the desired state has been computed, and are typically used to add common + * metadata (such as a label identifying the operator managing the resource) to all the resources + * the operator creates or updates. + * + * @return the list of aspects to apply to computed desired states, empty by default + * @since 5.6.0 + */ + default List desiredStateAspects() { + return List.of(); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 18a2e3fc38..2cf6540af0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -16,6 +16,8 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -31,6 +33,7 @@ import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.api.reconciler.Experimental; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect; import io.javaoperatorsdk.operator.processing.event.source.informer.pool.InformerPool; @SuppressWarnings({"unused", "UnusedReturnValue"}) @@ -59,6 +62,7 @@ public class ConfigurationServiceOverrider { private Boolean useSSAToPatchPrimaryResource; private Boolean cloneSecondaryResourcesWhenGettingFromCache; private InformerPool informerPool; + private List desiredStateAspects; @SuppressWarnings("rawtypes") private DependentResourceFactory dependentResourceFactory; @@ -229,6 +233,38 @@ public ConfigurationServiceOverrider withInformerPool(InformerPool informerPool) return this; } + /** + * Replaces the {@link DesiredStateAspect}s applied to the desired state of all the Kubernetes + * dependent resources managed by the operator by the specified ones. + * + * @param desiredStateAspects the aspects to apply, in the order in which they should be applied + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @since 5.6.0 + */ + public ConfigurationServiceOverrider withDesiredStateAspects( + List desiredStateAspects) { + this.desiredStateAspects = new ArrayList<>(desiredStateAspects); + return this; + } + + /** + * Appends the specified {@link DesiredStateAspect}s to the already configured ones, which are the + * ones configured on the overridden {@link ConfigurationService} unless {@link + * #withDesiredStateAspects(List)} was called on this overrider first. + * + * @param desiredStateAspects the aspects to append, in the order in which they should be applied + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @since 5.6.0 + */ + public ConfigurationServiceOverrider addDesiredStateAspects( + DesiredStateAspect... desiredStateAspects) { + if (this.desiredStateAspects == null) { + this.desiredStateAspects = new ArrayList<>(original.desiredStateAspects()); + } + this.desiredStateAspects.addAll(List.of(desiredStateAspects)); + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, client) { @Override @@ -383,6 +419,12 @@ public synchronized InformerPool informerPool() { informerPool.setConfigurationService(this); return informerPool; } + + @Override + public List desiredStateAspects() { + return overriddenValueOrDefault( + desiredStateAspects, ConfigurationService::desiredStateAspects); + } }; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java index e399f7fdfd..50e61526c4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java @@ -31,6 +31,7 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.event.ResourceEventRecorder; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedWorkflowAndDependentResourceContext; import io.javaoperatorsdk.operator.processing.Controller; @@ -258,6 +259,25 @@ public R getOrComputeDesiredStateFor( DependentResource dependentResource, Function desiredStateComputer) { return (R) desiredStates.computeIfAbsent( - dependentResource, ignored -> desiredStateComputer.apply(getPrimaryResource())); + dependentResource, + ignored -> { + final var desired = desiredStateComputer.apply(getPrimaryResource()); + applyDesiredStateAspects(desired, dependentResource); + return desired; + }); + } + + /** + * Applies the globally configured {@link DesiredStateAspect}s, in configuration order, to the + * freshly computed desired state. Aspects only apply to Kubernetes resources, external dependent + * resources are therefore left untouched. + */ + private void applyDesiredStateAspects(Object desired, DependentResource dependentResource) { + if (desired instanceof HasMetadata hasMetadata) { + controllerConfiguration + .getConfigurationService() + .desiredStateAspects() + .forEach(aspect -> aspect.apply(hasMetadata, dependentResource, this)); + } } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DesiredStateAspect.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DesiredStateAspect.java new file mode 100644 index 0000000000..826a15cb66 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DesiredStateAspect.java @@ -0,0 +1,55 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.api.reconciler.dependent; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.reconciler.Context; + +/** + * A cross-cutting hook applied to the desired state of every Kubernetes {@link DependentResource} + * managed by the operator, typically used to add common metadata (labels or annotations) marking + * the resources the operator manages. + * + *

Aspects are registered globally on the {@link ConfigurationService} and are applied, in + * registration order, right after the desired state has been computed and before it is matched + * against, created or updated. This means modifications performed by an aspect are taken into + * account when determining whether the actual resource matches its desired state, so that changing + * an aspect triggers an update of the associated secondary resources. + * + *

The desired state is computed at most once per reconciliation and cached in the {@link + * Context}, so aspects are also called at most once per dependent resource and reconciliation. + * Aspects are only applied to dependent resources whose desired state is a {@link HasMetadata}, + * i.e. they are not called for external (non-Kubernetes) dependent resources. + * + *

Implementations are expected to mutate the provided desired state in place and must be + * thread-safe as they can be called concurrently for different primary resources. + * + * @see ConfigurationService#desiredStateAspects() + */ +@FunctionalInterface +public interface DesiredStateAspect { + + /** + * Applies this aspect to the specified, freshly computed desired state. + * + * @param desired the desired state to modify in place + * @param dependentResource the {@link DependentResource} the desired state was computed for + * @param context the {@link Context} of the current reconciliation, from which the primary + * resource can be retrieved using {@link Context#getPrimaryResource()} + */ + void apply(HasMetadata desired, DependentResource dependentResource, Context context); +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java index 827961b77f..2428eeba96 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java @@ -117,7 +117,10 @@ public R update(R actual, R desired, P primary, Context

context) { @Override public Result match(R resource, P primary, Context

context) { - return bulkDependentResource.match(resource, desired, primary, context); + // retrieve the desired state via the context so that it is processed the same way as for + // non-bulk dependents, in particular so that configured DesiredStateAspects are applied + // before matching + return bulkDependentResource.match(resource, getOrComputeDesired(context), primary, context); } @Override diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java index 4356395618..aec8381135 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java @@ -16,6 +16,7 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.Executors; @@ -33,6 +34,7 @@ import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -187,4 +189,36 @@ void clusterScopedEventNamespaceDefaultsToTheDefaultNamespaceAndCanBeOverridden( .clusterScopedEventNamespace()) .isEqualTo("operator-ns"); } + + @Test + void desiredStateAspectsAreEmptyByDefaultAndCanBeOverridden() { + assertThat(config.desiredStateAspects()).isEmpty(); + + final DesiredStateAspect first = (desired, dependentResource, context) -> {}; + final DesiredStateAspect second = (desired, dependentResource, context) -> {}; + + assertThat( + new ConfigurationServiceOverrider(config) + .withDesiredStateAspects(List.of(first, second)) + .build() + .desiredStateAspects()) + .containsExactly(first, second); + } + + @Test + void desiredStateAspectsCanBeAppendedToAlreadyConfiguredOnes() { + final DesiredStateAspect first = (desired, dependentResource, context) -> {}; + final DesiredStateAspect second = (desired, dependentResource, context) -> {}; + final DesiredStateAspect third = (desired, dependentResource, context) -> {}; + + final var configWithAspect = + new ConfigurationServiceOverrider(config).withDesiredStateAspects(List.of(first)).build(); + + assertThat( + new ConfigurationServiceOverrider(configWithAspect) + .addDesiredStateAspects(second, third) + .build() + .desiredStateAspects()) + .containsExactly(first, second, third); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java index 1db69a1f9e..7c36aabe66 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java @@ -15,6 +15,7 @@ */ package io.javaoperatorsdk.operator.processing.dependent; +import java.util.List; import java.util.Optional; import java.util.Set; @@ -23,8 +24,12 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.ConfigMapBuilder; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect; +import io.javaoperatorsdk.operator.processing.Controller; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static org.junit.jupiter.api.Assertions.*; @@ -37,7 +42,18 @@ class AbstractDependentResourceTest { private static final DefaultContext CONTEXT = createContext(PRIMARY); private static DefaultContext createContext(TestCustomResource primary) { - return new DefaultContext<>(mock(), mock(), primary, false, false); + return createContext(primary, List.of()); + } + + private static DefaultContext createContext( + TestCustomResource primary, List aspects) { + final ConfigurationService configurationService = mock(); + when(configurationService.desiredStateAspects()).thenReturn(aspects); + final ControllerConfiguration controllerConfiguration = mock(); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + final Controller controller = mock(); + when(controller.getConfiguration()).thenReturn(controllerConfiguration); + return new DefaultContext<>(mock(), controller, primary, false, false); } @Test @@ -101,6 +117,35 @@ void checkThatDesiredIsOnlyCalledOnce() { assertEquals(1, testDependentResource.desiredCallCount); } + @Test + void appliesConfiguredDesiredStateAspectsInOrderAndOnlyOnce() { + final var testDependentResource = new DesiredCallCountCheckingDR(); + final var primary = new TestCustomResource(); + final var spec = primary.getSpec(); + spec.setConfigMapName("foo"); + spec.setKey("key"); + spec.setValue("value"); + final var context = + createContext( + primary, + List.of( + (desired, dependentResource, ctx) -> { + assertSame(testDependentResource, dependentResource); + assertSame(primary, ctx.getPrimaryResource()); + desired.getMetadata().getLabels().put("aspect", "first"); + }, + (desired, dependentResource, ctx) -> + desired.getMetadata().getLabels().put("aspect", "second"))); + + final var created = testDependentResource.reconcile(primary, context).getSingleResource(); + assertEquals("second", created.orElseThrow().getMetadata().getLabels().get("aspect")); + + // desired state is cached, aspects should therefore not be applied again + created.orElseThrow().getMetadata().getLabels().remove("aspect"); + testDependentResource.reconcile(primary, context); + assertNull(created.orElseThrow().getMetadata().getLabels().get("aspect")); + } + private ConfigMap configMap() { ConfigMap configMap = new ConfigMap(); configMap.setMetadata( diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java index 0c2583d594..51bba5c76d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java @@ -15,6 +15,7 @@ */ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.List; import java.util.Map; import java.util.Optional; @@ -27,12 +28,16 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.ReconcilerUtilsInternal; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; +import io.javaoperatorsdk.operator.processing.Controller; import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; @SuppressWarnings({"unchecked"}) class GenericKubernetesResourceMatcherTest { @@ -47,7 +52,18 @@ public TestContext() { } public TestContext(HasMetadata primary) { - super(mock(), mock(), primary, false, false); + super(mock(), mockController(), primary, false, false); + } + + @SuppressWarnings("rawtypes") + private static Controller mockController() { + final ConfigurationService configurationService = mock(); + when(configurationService.desiredStateAspects()).thenReturn(List.of()); + final ControllerConfiguration controllerConfiguration = mock(); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + final Controller controller = mock(); + when(controller.getConfiguration()).thenReturn(controllerConfiguration); + return controller; } @Override diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectCustomResource.java new file mode 100644 index 0000000000..e5be695160 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectCustomResource.java @@ -0,0 +1,26 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.dependent.desiredstateaspect; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +public class DesiredStateAspectCustomResource extends CustomResource + implements Namespaced {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java new file mode 100644 index 0000000000..ef8048b47d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java @@ -0,0 +1,117 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.dependent.desiredstateaspect; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.annotation.Sample; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +@Sample( + tldr = "Common metadata on all managed resources using desired state aspects", + description = + """ + Demonstrates how to register a global DesiredStateAspect on the ConfigurationService in \ + order to add common metadata, here labels identifying the operator and the dependent \ + resource the secondary resource originates from, to every Kubernetes resource managed by \ + the operator. Aspects are applied to the desired state right after it is computed, so the \ + added metadata is also taken into account when matching the actual resource against its \ + desired state. + """) +class DesiredStateAspectIT { + + public static final String TEST_RESOURCE_NAME = "test1"; + public static final String MANAGED_BY_LABEL_KEY = "app.kubernetes.io/managed-by"; + public static final String MANAGED_BY_LABEL_VALUE = "desired-state-aspect-operator"; + public static final String DEPENDENT_LABEL_KEY = "javaoperatorsdk.io/dependent"; + public static final String DEPENDENT_LABEL_VALUE = + DesiredStateAspectReconciler.ConfigMapDependentResource.class.getSimpleName().toLowerCase(); + + @RegisterExtension + LocallyRunOperatorExtension operator = + LocallyRunOperatorExtension.builder() + .withReconciler(DesiredStateAspectReconciler.class) + .withConfigurationService( + o -> + o.addDesiredStateAspects( + (desired, dependentResource, context) -> + desired + .getMetadata() + .getLabels() + .put(MANAGED_BY_LABEL_KEY, MANAGED_BY_LABEL_VALUE), + (desired, dependentResource, context) -> + desired + .getMetadata() + .getLabels() + .put( + DEPENDENT_LABEL_KEY, + dependentResource.getClass().getSimpleName().toLowerCase()))) + .build(); + + @Test + void aspectsAreAppliedToAllManagedResources() { + operator.create(testResource()); + + await() + .untilAsserted( + () -> { + var configMap = operator.get(ConfigMap.class, TEST_RESOURCE_NAME); + assertThat(configMap).isNotNull(); + assertThat(configMap.getMetadata().getLabels()) + .containsEntry(MANAGED_BY_LABEL_KEY, MANAGED_BY_LABEL_VALUE) + .containsEntry(DEPENDENT_LABEL_KEY, DEPENDENT_LABEL_VALUE); + }); + } + + @Test + void metadataAddedByAspectsIsRestoredIfRemoved() { + operator.create(testResource()); + + await() + .untilAsserted( + () -> + assertThat(operator.get(ConfigMap.class, TEST_RESOURCE_NAME)) + .isNotNull() + .extracting(cm -> cm.getMetadata().getLabels()) + .satisfies( + labels -> + assertThat(labels) + .containsEntry(MANAGED_BY_LABEL_KEY, MANAGED_BY_LABEL_VALUE))); + + var configMap = operator.get(ConfigMap.class, TEST_RESOURCE_NAME); + configMap.getMetadata().getLabels().remove(MANAGED_BY_LABEL_KEY); + operator.replace(configMap); + + await() + .untilAsserted( + () -> + assertThat( + operator.get(ConfigMap.class, TEST_RESOURCE_NAME).getMetadata().getLabels()) + .containsEntry(MANAGED_BY_LABEL_KEY, MANAGED_BY_LABEL_VALUE)); + } + + DesiredStateAspectCustomResource testResource() { + var res = new DesiredStateAspectCustomResource(); + res.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build()); + return res; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectReconciler.java new file mode 100644 index 0000000000..bdb34435e3 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectReconciler.java @@ -0,0 +1,59 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.dependent.desiredstateaspect; + +import java.util.Map; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.Workflow; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; + +@Workflow( + dependents = @Dependent(type = DesiredStateAspectReconciler.ConfigMapDependentResource.class)) +@ControllerConfiguration +public class DesiredStateAspectReconciler implements Reconciler { + + @Override + public UpdateControl reconcile( + DesiredStateAspectCustomResource resource, + Context context) { + return UpdateControl.noUpdate(); + } + + public static class ConfigMapDependentResource + extends CRUDKubernetesDependentResource { + + @Override + protected ConfigMap desired( + DesiredStateAspectCustomResource primary, + Context context) { + ConfigMap configMap = new ConfigMap(); + configMap.setMetadata( + new ObjectMetaBuilder() + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()); + configMap.setData(Map.of("data", primary.getMetadata().getName())); + return configMap; + } + } +} From 2339b7d6f9ff4b1efa85703c89d5159bc1def3bd Mon Sep 17 00:00:00 2001 From: Antonio <122279781+afalhambra-hivemq@users.noreply.github.com> Date: Wed, 9 Sep 2026 13:59:45 +0200 Subject: [PATCH 03/11] feat: configurable aggregation, owner references and naming for the event recorder (#3604) Signed-off-by: Antonio Fernandez Alhambra --- .../api/event/DefaultEventRecorder.java | 155 +++++++++- .../operator/api/event/EventKeyStrategy.java | 55 ++++ .../api/event/EventNamingStrategy.java | 51 ++++ .../operator/api/event/EventRecord.java | 41 +++ .../api/event/DefaultEventRecorderTest.java | 271 +++++++++++++++++- ...ConfiguredEventRecorderCustomResource.java | 30 ++ .../ConfiguredEventRecorderIT.java | 168 +++++++++++ .../ConfiguredEventRecorderReconciler.java | 49 ++++ 8 files changed, 808 insertions(+), 12 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventKeyStrategy.java create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventNamingStrategy.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderReconciler.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java index 2e7023623d..67764b7146 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java @@ -24,6 +24,7 @@ import java.time.temporal.ChronoUnit; import java.util.HexFormat; import java.util.Objects; +import java.util.regex.Pattern; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,10 +49,11 @@ * can be overridden, see {@link * io.javaoperatorsdk.operator.api.config.ConfigurationService#clusterScopedEventNamespace()}. * - *

Events are named deterministically, after the object they are about plus a hash of everything - * that identifies the event, so that recording the same event again resolves to the event already - * recorded for it. Repeat occurrences are then counted on that event rather than recorded as copies - * of it, see {@link DefaultEventSink}. + *

By default, events are named deterministically, after the object they are about plus a hash of + * everything that identifies the event, so that recording the same event again resolves to the + * event already recorded for it. Repeat occurrences are then counted on that event rather than + * recorded as copies of it, see {@link DefaultEventSink}. How events aggregate, how they are named + * and whether they carry an owner reference can be configured, see {@link #builder(EventSink)}. */ public class DefaultEventRecorder implements EventRecorder { @@ -73,10 +75,79 @@ public class DefaultEventRecorder implements EventRecorder { private static final int IDENTITY_HASH_LENGTH = 32; + /** What the API server accepts as an object name, see RFC 1123 on DNS subdomains. */ + private static final Pattern RFC_1123_SUBDOMAIN = + Pattern.compile("[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*"); + private final EventSink sink; + private final EventNamingStrategy namingStrategy; + private final EventKeyStrategy keyStrategy; + private final boolean ownerReference; public DefaultEventRecorder(EventSink sink) { + this(sink, EventNamingStrategy.none(), EventKeyStrategy.none(), false); + } + + private DefaultEventRecorder( + EventSink sink, + EventNamingStrategy namingStrategy, + EventKeyStrategy keyStrategy, + boolean ownerReference) { this.sink = sink; + this.namingStrategy = namingStrategy; + this.keyStrategy = keyStrategy; + this.ownerReference = ownerReference; + } + + public static Builder builder(EventSink sink) { + return new Builder(sink); + } + + /** Builder for {@link DefaultEventRecorder}. */ + public static final class Builder { + + private final EventSink sink; + private EventNamingStrategy namingStrategy = EventNamingStrategy.none(); + private EventKeyStrategy keyStrategy = EventKeyStrategy.none(); + private boolean ownerReference = false; + + private Builder(EventSink sink) { + this.sink = Objects.requireNonNull(sink, "sink must not be null"); + } + + /** The strategy naming recorded events, see {@link EventNamingStrategy}. */ + public Builder namingStrategy(EventNamingStrategy namingStrategy) { + this.namingStrategy = + Objects.requireNonNull(namingStrategy, "namingStrategy must not be null"); + return this; + } + + /** + * The strategy deriving the default aggregation key of records that do not set one, see {@link + * EventKeyStrategy}. The key is ignored for events whose name is set by the record or resolved + * by the naming strategy, see {@link EventNamingStrategy}. + */ + public Builder keyStrategy(EventKeyStrategy keyStrategy) { + this.keyStrategy = Objects.requireNonNull(keyStrategy, "keyStrategy must not be null"); + return this; + } + + /** + * When set, recorded events carry an {@code ownerReference} to the object they are about. The + * reference expresses ownership for tooling that reads it; note that the Kubernetes garbage + * collector ignores events, so it does not cause cascade deletion, events expire through the + * event TTL either way. Records can override this per event via {@link + * EventRecord.Builder#ownedByRegarding(boolean)}. The reference is only set when the object + * already has a uid. + */ + public Builder ownerReference(boolean ownerReference) { + this.ownerReference = ownerReference; + return this; + } + + public DefaultEventRecorder build() { + return new DefaultEventRecorder(sink, namingStrategy, keyStrategy, ownerReference); + } } /** @@ -111,14 +182,17 @@ private static String resolve() { public void record(EventRecord event, Context context) { Objects.requireNonNull(context, "the context of the reconciliation must not be null"); Objects.requireNonNull(event, "event must not be null"); + Event assembled = null; try { - sink.emit(toEvent(context, event), context); + assembled = toEvent(context, event); + sink.emit(assembled, context); } catch (Exception e) { // recording an event must never break the caller: a controller that fails to reconcile // because it could not write an event is strictly worse than one that records nothing log.warn( - "Could not record {} event with reason {} for resource {} in namespace {}", + "Could not record {} event named {} with reason {} for resource {} in namespace {}", event.type(), + assembled != null ? assembled.getMetadata().getName() : "unknown", event.reason(), context.getPrimaryResource().getMetadata().getName(), context.getPrimaryResource().getMetadata().getNamespace(), @@ -164,6 +238,23 @@ protected Event toEvent(Context context, EventRecord record) { .withNewSource() .withComponent(record.reportingComponent().orElse(controllerName)) .endSource(); + boolean ownedByRegarding = record.ownedByRegarding().orElse(ownerReference); + if (ownedByRegarding && regarding.getMetadata().getUid() == null) { + log.debug( + "Not setting the owner reference on the event about {}: the object has no uid yet", + regarding.getMetadata().getName()); + } + if (ownedByRegarding && regarding.getMetadata().getUid() != null) { + builder + .editMetadata() + .addNewOwnerReference() + .withApiVersion(regarding.getApiVersion()) + .withKind(regarding.getKind()) + .withName(regarding.getMetadata().getName()) + .withUid(regarding.getMetadata().getUid()) + .endOwnerReference() + .endMetadata(); + } record.action().ifPresent(builder::withAction); return builder.build(); } @@ -181,17 +272,56 @@ private String eventNamespace(HasMetadata regarding, Context context) { CLUSTER_SCOPED_EVENT_NAMESPACE); } + private String eventName(HasMetadata regarding, EventRecord record, String reportingController) { + return record + .name() + .filter(name -> !name.isBlank()) + .or(() -> namingStrategy.nameFor(regarding, record).filter(name -> !name.isBlank())) + .map(DefaultEventRecorder::truncateToMaxNameLength) + .filter(DefaultEventRecorder::isValidEventName) + .orElseGet(() -> identityHashName(regarding, record, reportingController)); + } + + private static boolean isValidEventName(String name) { + if (RFC_1123_SUBDOMAIN.matcher(name).matches()) { + return true; + } + log.warn( + "Falling back to the default event name: {} is not a valid RFC 1123 DNS subdomain", name); + return false; + } + + private static String truncateToMaxNameLength(String name) { + if (name.length() <= MAX_NAME_LENGTH) { + return name; + } + var truncated = name.substring(0, MAX_NAME_LENGTH); + while (truncated.endsWith("-") || truncated.endsWith(".")) { + truncated = truncated.substring(0, truncated.length() - 1); + } + log.warn( + "Truncated the name of event {} to {} to stay within the Kubernetes name limit", + name, + truncated); + return truncated; + } + /** * Names events {@code .}, following the convention of the Go client, hashing * everything that makes two events the same event: the object, the type, the reason, the - * reporting component and, unless the record sets a {@link EventRecord#key()}, the message. The - * name is therefore stable across occurrences, which is what lets the sink recognise a repeat, - * and stays so across operator restarts and between replicas, unlike a name remembered in memory. + * reporting component and, unless the record sets a {@link EventRecord#key()} or the recorder is + * built with a default {@link EventKeyStrategy}, the message. The name is therefore stable across + * occurrences, which is what lets the sink recognise a repeat, and stays so across operator + * restarts and between replicas, unlike a name remembered in memory. * *

The object is identified by its uid, with the kind as a fallback for objects that do not * have one yet, such as a dependent resource that has only been built so far. + * + *

This is the fallback when the record does not set a name and the naming strategy resolves to + * nothing, see {@link EventNamingStrategy}. */ - private String eventName(HasMetadata regarding, EventRecord record, String reportingController) { + private String identityHashName( + HasMetadata regarding, EventRecord record, String reportingController) { var metadata = regarding.getMetadata(); var identity = String.join( @@ -201,7 +331,10 @@ private String eventName(HasMetadata regarding, EventRecord record, String repor record.type().value(), record.reason(), record.reportingComponent().orElse(reportingController), - record.key().orElseGet(() -> requireNonNullElse(record.message(), ""))); + record + .key() + .or(() -> keyStrategy.keyFor(regarding, record)) + .orElseGet(() -> requireNonNullElse(record.message(), ""))); var suffix = "." + identityDigest(identity); var prefix = metadata.getName(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventKeyStrategy.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventKeyStrategy.java new file mode 100644 index 0000000000..632ff6ea6a --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventKeyStrategy.java @@ -0,0 +1,55 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.api.event; + +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Experimental; + +import static io.javaoperatorsdk.operator.api.reconciler.Experimental.API_MIGHT_CHANGE; + +/** + * Derives the default aggregation key of an event, used when the {@link EventRecord} does not set + * one explicitly. The key identifies an event among the events about the same object, so that + * repeated occurrences resolve to the same event rather than to one event each, see {@link + * EventRecord#key()}. + * + *

An empty result leaves the record without a default key, which keeps the message part of the + * event identity. + * + *

Implementations are called from concurrent reconciliations and must be thread safe. + */ +@Experimental(API_MIGHT_CHANGE) +@FunctionalInterface +public interface EventKeyStrategy { + + Optional keyFor(HasMetadata regarding, EventRecord record); + + /** No default key: the message stays part of the event identity. */ + static EventKeyStrategy none() { + return (regarding, record) -> Optional.empty(); + } + + /** + * Aggregates by event type and reason: all occurrences of a reason resolve to one event whose + * count grows and whose message is replaced with the latest one. The right choice for events that + * report a current state rather than individual occurrences. + */ + static EventKeyStrategy byReason() { + return (regarding, record) -> Optional.of(record.type().value() + "/" + record.reason()); + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventNamingStrategy.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventNamingStrategy.java new file mode 100644 index 0000000000..80f6f33e82 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventNamingStrategy.java @@ -0,0 +1,51 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.api.event; + +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Experimental; + +import static io.javaoperatorsdk.operator.api.reconciler.Experimental.API_MIGHT_CHANGE; + +/** + * Names the event recorded about an object. The name is what the sink looks a recorded event up by, + * so it is also the aggregation identity: two records resolving to the same name are counted as + * occurrences of one event. A name must therefore be unique among the events it should not + * aggregate with, and stable across operator restarts and replicas. + * + *

A name must be a valid RFC 1123 DNS subdomain: at most 253 lowercase alphanumeric characters, + * {@code -} or {@code .}, starting and ending with an alphanumeric character. Names longer than the + * limit are truncated. A name derived from the object and a fixed lowercase suffix (such as {@code + * -status-report}) satisfies all of this by construction. + * + *

An empty result or an invalid name falls back to the default {@code .} + * name, rather than the event being lost to the API server rejecting the name. + * + *

Implementations are called from concurrent reconciliations and must be thread safe. + */ +@Experimental(API_MIGHT_CHANGE) +@FunctionalInterface +public interface EventNamingStrategy { + + Optional nameFor(HasMetadata regarding, EventRecord record); + + /** No custom naming: every event gets the default {@code .} name. */ + static EventNamingStrategy none() { + return (regarding, record) -> Optional.empty(); + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventRecord.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventRecord.java index e7b736abc0..6d98b7f1fe 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventRecord.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/EventRecord.java @@ -32,7 +32,9 @@ public final class EventRecord { private final EventType type; private final String reason; private final String message; + private final String name; private final String key; + private final Boolean ownedByRegarding; private final String action; private final String reportingComponent; private final Map labels; @@ -42,11 +44,13 @@ private EventRecord(Builder builder) { this.type = builder.type; this.reason = builder.reason; this.message = builder.message; + this.name = builder.name; this.key = builder.key; this.action = builder.action; this.reportingComponent = builder.reportingComponent; this.labels = Map.copyOf(builder.labels); this.annotations = Map.copyOf(builder.annotations); + this.ownedByRegarding = builder.ownedByRegarding; } public static Builder builder() { @@ -75,14 +79,34 @@ public String message() { return message; } + /** + * The name of the recorded event, overriding the recorder's naming. The name is the aggregation + * identity and must be a valid RFC 1123 DNS subdomain, see {@link EventNamingStrategy}; an + * invalid name falls back to the default name. A blank name is treated as unset. + */ + public Optional name() { + return Optional.ofNullable(name); + } + /** * Identifies this event among the events about the same object, so that repeated occurrences * resolve to the same event rather than to one event each. + * + *

The key is ignored when the record sets a {@link #name()} or the recorder's naming strategy + * resolves one: the name is then the aggregation identity on its own. */ public Optional key() { return Optional.ofNullable(key); } + /** + * Whether the recorded event carries an {@code ownerReference} to the object it is about. When + * empty, the recorder's own setting applies. + */ + public Optional ownedByRegarding() { + return Optional.ofNullable(ownedByRegarding); + } + /** * The action taken or failed regarding the involved object, if any. Optional, and only meaningful * for consumers that read the {@code action} field of the event. @@ -119,7 +143,9 @@ public static final class Builder { private EventType type = EventType.NORMAL; private String reason; private String message; + private String name; private String key; + private Boolean ownedByRegarding; private String action; private String reportingComponent; private final Map labels = new HashMap<>(); @@ -142,12 +168,27 @@ public Builder message(String message) { return this; } + /** Sets the name of the recorded event, see {@link EventRecord#name()}. */ + public Builder name(String name) { + this.name = name; + return this; + } + /** Sets the key identifying this event, see {@link EventRecord#key()}. */ public Builder key(String key) { this.key = key; return this; } + /** + * Sets whether this event is owned by the object it is about, see {@link + * EventRecord#ownedByRegarding()}. + */ + public Builder ownedByRegarding(boolean ownedByRegarding) { + this.ownedByRegarding = ownedByRegarding; + return this; + } + public Builder action(String action) { this.action = action; return this; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java index eb713e5d74..388217b9be 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java @@ -226,6 +226,14 @@ void reasonIsRequired() { .isThrownBy(() -> EventRecord.builder().message("no reason given").build()); } + @Test + void alwaysDerivesTheSameDefaultNameForTheSameEvent() { + recorder.record(EventRecord.normal("Created", "created"), context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()) + .isEqualTo("test1.3c699548f37ff9cd6d2a786f64a27228"); + } + @Test void namesEventsWithADnsSafeHashSuffix() { recorder.record(EventRecord.normal("Created", "created"), context(configMap())); @@ -249,6 +257,267 @@ void givesEventsWhoseMessagesCollideUnderStringHashCodeDistinctNames() { .isNotEqualTo(emitted.get(1).getMetadata().getName()); } + @Test + void takesTheMessageOutOfTheEventIdentityWithADefaultKeyStrategy() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .keyStrategy(EventKeyStrategy.byReason()) + .build(); + var context = context(configMap()); + + recorder.record(EventRecord.warning("Failed", "first message"), context); + recorder.record(EventRecord.warning("Failed", "second message"), context); + + assertThat(emitted.get(0).getMetadata().getName()) + .isEqualTo(emitted.get(1).getMetadata().getName()); + } + + @Test + void prefersThePerRecordKeyOverTheDefaultKeyStrategy() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .keyStrategy(EventKeyStrategy.byReason()) + .build(); + var context = context(configMap()); + + recorder.record(EventRecord.warning("Failed", "message"), context); + recorder.record( + EventRecord.builder() + .type(EventType.WARNING) + .reason("Failed") + .message("message") + .key("another aggregate") + .build(), + context); + + assertThat(emitted.get(0).getMetadata().getName()) + .isNotEqualTo(emitted.get(1).getMetadata().getName()); + } + + @Test + void keepsTheMessageInTheEventIdentityWithoutADefaultKeyStrategy() { + var context = context(configMap()); + + recorder.record(EventRecord.warning("Failed", "first message"), context); + recorder.record(EventRecord.warning("Failed", "second message"), context); + + assertThat(emitted.get(0).getMetadata().getName()) + .isNotEqualTo(emitted.get(1).getMetadata().getName()); + } + + @Test + void keepsEventsWithTheSameReasonButDifferentTypesApartUnderByReason() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .keyStrategy(EventKeyStrategy.byReason()) + .build(); + var context = context(configMap()); + + recorder.record(EventRecord.normal("Flipped", "message"), context); + recorder.record(EventRecord.warning("Flipped", "message"), context); + + assertThat(emitted.get(0).getMetadata().getName()) + .isNotEqualTo(emitted.get(1).getMetadata().getName()); + } + + @Test + void setsTheOwnerReferenceToTheInvolvedObjectWhenOwningEventsByRegarding() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .ownerReference(true) + .build(); + + recorder.record(EventRecord.normal("Created", "created"), context(configMap())); + + assertThat(emitted.get(0).getMetadata().getOwnerReferences()) + .singleElement() + .satisfies( + owner -> { + assertThat(owner.getApiVersion()).isEqualTo("v1"); + assertThat(owner.getKind()).isEqualTo("ConfigMap"); + assertThat(owner.getName()).isEqualTo("test1"); + assertThat(owner.getUid()).isEqualTo("uid-1"); + }); + } + + @Test + void carriesNoOwnerReferenceByDefault() { + recorder.record(EventRecord.normal("Created", "created"), context(configMap())); + + assertThat(emitted.get(0).getMetadata().getOwnerReferences()).isEmpty(); + } + + @Test + void letsARecordOptOutOfTheRecorderLevelOwnerReference() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .ownerReference(true) + .build(); + + recorder.record( + EventRecord.builder().reason("Created").message("created").ownedByRegarding(false).build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getOwnerReferences()).isEmpty(); + } + + @Test + void letsARecordOptIntoTheOwnerReferenceOnItsOwn() { + recorder.record( + EventRecord.builder().reason("Created").message("created").ownedByRegarding(true).build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getOwnerReferences()).hasSize(1); + } + + @Test + void setsNoOwnerReferenceWhenTheRegardingObjectHasNoUidYet() { + var withoutUid = configMap(); + withoutUid.getMetadata().setUid(null); + + recorder.record( + EventRecord.builder().reason("Created").message("created").ownedByRegarding(true).build(), + context(withoutUid)); + + assertThat(emitted.get(0).getMetadata().getOwnerReferences()).isEmpty(); + } + + @Test + void namesTheEventAfterThePerRecordNameWhenOneIsSet() { + recorder.record( + EventRecord.builder().reason("Created").message("created").name("my-event-name").build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).isEqualTo("my-event-name"); + } + + @Test + void namesEventsThroughTheNamingStrategy() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy( + (regarding, record) -> + Optional.of(regarding.getMetadata().getName() + "-" + record.reason())) + .build(); + + recorder.record(EventRecord.warning("failed", "first"), context(configMap())); + recorder.record(EventRecord.warning("failed", "second"), context(configMap())); + + assertThat(emitted) + .allSatisfy(event -> assertThat(event.getMetadata().getName()).isEqualTo("test1-failed")); + } + + @Test + void fallsBackToTheIdentityHashNameWhenTheNamingStrategyResolvesNothing() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy((regarding, record) -> Optional.empty()) + .build(); + + recorder.record(EventRecord.warning("Failed", "message"), context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()) + .startsWith("test1.") + .hasSize("test1.".length() + 32); + } + + @Test + void truncatesSuppliedNamesToTheKubernetesNameLengthLimit() { + recorder.record( + EventRecord.builder().reason("Created").message("created").name("a".repeat(300)).build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).hasSize(253); + } + + @Test + void stripsTrailingSeparatorsFromTruncatedNames() { + recorder.record( + EventRecord.builder() + .reason("Created") + .message("created") + .name("a".repeat(252) + "." + "b".repeat(47)) + .build(), + context(configMap())); + recorder.record( + EventRecord.builder() + .reason("Created") + .message("created") + .name("b".repeat(252) + "-" + "c".repeat(47)) + .build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).isEqualTo("a".repeat(252)); + assertThat(emitted.get(1).getMetadata().getName()).isEqualTo("b".repeat(252)); + } + + @Test + void treatsABlankNameAsUnset() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy((regarding, record) -> Optional.of("strategy-name")) + .build(); + + recorder.record( + EventRecord.builder().reason("Created").message("created").name("").build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).isEqualTo("strategy-name"); + } + + @Test + void fallsBackToTheIdentityHashNameWhenEveryNameIsBlank() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy((regarding, record) -> Optional.of(" ")) + .build(); + + recorder.record( + EventRecord.builder().reason("Created").message("created").name("").build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).matches("test1\\.[0-9a-f]{32}"); + } + + @Test + void fallsBackToTheIdentityHashNameWhenTruncationLeavesNothing() { + recorder.record( + EventRecord.builder().reason("Created").message("created").name(".".repeat(300)).build(), + context(configMap())); + + assertThat(emitted.get(0).getMetadata().getName()).matches("test1\\.[0-9a-f]{32}"); + } + + @Test + void fallsBackToTheIdentityHashNameWhenTheSuppliedNameIsInvalid() { + recorder.record( + EventRecord.builder().reason("Created").message("created").name("Status").build(), + context(configMap())); + recorder.record( + EventRecord.builder().reason("Created").message("created").name("status_name").build(), + context(configMap())); + + assertThat(emitted) + .allSatisfy( + event -> assertThat(event.getMetadata().getName()).matches("test1\\.[0-9a-f]{32}")); + } + + @Test + void aFailingNamingStrategyNeverFailsTheCaller() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy( + (regarding, record) -> { + throw new RuntimeException("cannot derive a name"); + }) + .build(); + + assertThatCode( + () -> recorder.record(EventRecord.normal("Created", "created"), context(configMap()))) + .doesNotThrowAnyException(); + assertThat(emitted).isEmpty(); + } + Context context(HasMetadata primaryResource) { return context(primaryResource, DefaultEventRecorder.CLUSTER_SCOPED_EVENT_NAMESPACE); } @@ -259,7 +528,7 @@ Context context(HasMetadata primaryResource) { * the configuration service the reporting instance and the cluster scoped event namespace come * from. */ - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings("rawtypes") Context context(HasMetadata primaryResource, String clusterScopedEventNamespace) { var configurationService = mock(ConfigurationService.class); when(configurationService.getLeaderElectionConfiguration()) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderCustomResource.java new file mode 100644 index 0000000000..0e0e3cab5d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderCustomResource.java @@ -0,0 +1,30 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.eventrecorderconfigured; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.Kind; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@Kind("ConfiguredEventRecorderCustomResource") +@ShortNames("cerc") +public class ConfiguredEventRecorderCustomResource extends CustomResource + implements Namespaced {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderIT.java new file mode 100644 index 0000000000..537aa38f8d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderIT.java @@ -0,0 +1,168 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.eventrecorderconfigured; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.Event; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.OwnerReference; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.javaoperatorsdk.annotation.Sample; +import io.javaoperatorsdk.operator.api.event.DefaultEventRecorder; +import io.javaoperatorsdk.operator.api.event.DefaultEventSink; +import io.javaoperatorsdk.operator.api.event.EventKeyStrategy; +import io.javaoperatorsdk.operator.api.event.EventRecord; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; + +import static io.javaoperatorsdk.operator.baseapi.eventrecorderconfigured.ConfiguredEventRecorderReconciler.AGGREGATED_REASON; +import static io.javaoperatorsdk.operator.baseapi.eventrecorderconfigured.ConfiguredEventRecorderReconciler.NAMED_REASON; +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +@Sample( + tldr = "Configuring how the event recorder aggregates, names and owns events", + description = + """ + Demonstrates configuring the event recorder with a default aggregation key strategy, a \ + naming strategy and owner references. Aggregating by reason resolves repeated occurrences \ + with changing messages to one event whose count grows and whose message is replaced with \ + the latest one. A naming strategy gives events predictable names instead of the default \ + identity hash. Owner references relate the events to the object they are about. + """) +class ConfiguredEventRecorderIT { + + private static final String TEST_RESOURCE_NAME = "test1"; + + private final KubernetesClient sinkClient = new KubernetesClientBuilder().build(); + + private final ConfiguredEventRecorderReconciler reconciler = + new ConfiguredEventRecorderReconciler(); + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(reconciler) + .withConfigurationService(o -> o.withEventRecorder(configuredEventRecorder())) + .build(); + + @AfterEach + void closeSinkClient() { + sinkClient.close(); + } + + @Test + void aggregatesOccurrencesWithChangingMessagesOntoOneEvent() { + var resource = extension.create(testResource()); + await().untilAsserted(() -> assertThat(reconciler.getNumberOfExecutions()).isPositive()); + + // reconcile once more: aggregating by reason resolves the new messages to the same events + resource.getMetadata().setAnnotations(Map.of("reconcile", "again")); + extension.replace(resource); + + await() + .untilAsserted( + () -> { + assertThat(reconciler.getNumberOfExecutions()).isGreaterThanOrEqualTo(2); + + assertThat(eventsWithReason(AGGREGATED_REASON)) + .singleElement() + .satisfies( + event -> { + assertThat(event.getCount()).isGreaterThanOrEqualTo(2); + assertThat(event.getMessage()) + .isEqualTo("something changed in execution " + event.getCount()); + }); + }); + } + + @Test + void namesEventsThroughTheConfiguredNamingStrategy() { + extension.create(testResource()); + + await() + .untilAsserted( + () -> + assertThat(eventsWithReason(NAMED_REASON)) + .singleElement() + .extracting(event -> event.getMetadata().getName()) + .isEqualTo(TEST_RESOURCE_NAME + "-status-report")); + } + + @Test + void setsOwnerReferencesOnTheEventsItRecords() { + var resource = extension.create(testResource()); + + await() + .untilAsserted( + () -> { + var events = eventsForTestResource(); + assertThat(events).isNotEmpty(); + assertThat(events) + .allSatisfy( + event -> + assertThat(event.getMetadata().getOwnerReferences()) + .singleElement() + .returns( + "ConfiguredEventRecorderCustomResource", OwnerReference::getKind) + .returns(resource.getMetadata().getUid(), OwnerReference::getUid)); + }); + } + + private List eventsWithReason(String reason) { + return eventsForTestResource().stream().filter(e -> reason.equals(e.getReason())).toList(); + } + + @SuppressWarnings("resource") + private List eventsForTestResource() { + return extension + .getKubernetesClient() + .v1() + .events() + .inNamespace(extension.getNamespace()) + .withField("involvedObject.name", TEST_RESOURCE_NAME) + .list() + .getItems(); + } + + private ConfiguredEventRecorderCustomResource testResource() { + var resource = new ConfiguredEventRecorderCustomResource(); + resource.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build()); + return resource; + } + + private DefaultEventRecorder configuredEventRecorder() { + return DefaultEventRecorder.builder(new DefaultEventSink(sinkClient)) + .namingStrategy(ConfiguredEventRecorderIT::statusReportName) + .keyStrategy(EventKeyStrategy.byReason()) + .ownerReference(true) + .build(); + } + + private static Optional statusReportName(HasMetadata regarding, EventRecord record) { + return NAMED_REASON.equals(record.reason()) + ? Optional.of(regarding.getMetadata().getName() + "-status-report") + : Optional.empty(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderReconciler.java new file mode 100644 index 0000000000..af4d136a4d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/eventrecorderconfigured/ConfiguredEventRecorderReconciler.java @@ -0,0 +1,49 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.eventrecorderconfigured; + +import java.util.concurrent.atomic.AtomicInteger; + +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; + +@ControllerConfiguration(generationAwareEventProcessing = false) +public class ConfiguredEventRecorderReconciler + implements Reconciler { + + public static final String NAMED_REASON = "StatusReport"; + public static final String AGGREGATED_REASON = "SomethingChanged"; + + private final AtomicInteger numberOfExecutions = new AtomicInteger(); + + @Override + public UpdateControl reconcile( + ConfiguredEventRecorderCustomResource resource, + Context context) { + var execution = numberOfExecutions.incrementAndGet(); + context.eventRecorder().warn(NAMED_REASON, "status after execution " + execution); + context + .eventRecorder() + .normal(AGGREGATED_REASON, "something changed in execution " + execution); + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } +} From 5cfa07c7d03b2b8a88f9b2f923d0a2a0c9364de2 Mon Sep 17 00:00:00 2001 From: Antonio <122279781+afalhambra-hivemq@users.noreply.github.com> Date: Fri, 18 Sep 2026 08:04:27 +0200 Subject: [PATCH 04/11] fix: keep comparableResourceVersions when copying a dependent resource's informer configuration (#3625) Signed-off-by: Antonio Fernandez Alhambra --- .../InformerEventSourceConfiguration.java | 1 + .../InformerEventSourceConfigurationTest.java | 42 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerEventSourceConfigurationTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java index 9bd6f84d06..a4ca2f63d4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java @@ -316,6 +316,7 @@ public void updateFrom(InformerConfiguration informerConfig) { .withOnDeleteFilter(informerConfig.getOnDeleteFilter()) .withGenericFilter(informerConfig.getGenericFilter()) .withInformerListLimit(informerConfig.getInformerListLimit()) + .withComparableResourceVersions(informerConfig.isComparableResourceVersions()) .withFieldSelector(informerConfig.getFieldSelector()); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerEventSourceConfigurationTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerEventSourceConfigurationTest.java new file mode 100644 index 0000000000..9e43b288cc --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerEventSourceConfigurationTest.java @@ -0,0 +1,42 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.api.config; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.Secret; +import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; +import io.javaoperatorsdk.operator.api.config.informer.InformerEventSourceConfiguration; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +class InformerEventSourceConfigurationTest { + + @Test + void updateFromKeepsComparableResourceVersions() { + // the only caller is KubernetesDependentResource#createEventSource, so dropping it here means + // the setting silently has no effect on a dependent resource + var builder = InformerEventSourceConfiguration.from(ConfigMap.class, Secret.class); + + builder.updateFrom( + InformerConfiguration.builder(ConfigMap.class) + .withComparableResourceVersions(false) + .build()); + + assertFalse(builder.build().getInformerConfig().isComparableResourceVersions()); + } +} From 36b2a929057e56c621146e8736fe045c2ce24110 Mon Sep 17 00:00:00 2001 From: Antonio <122279781+afalhambra-hivemq@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:04:37 +0200 Subject: [PATCH 05/11] fix: rename version.properties to a unique name to avoid resource collisions (#3628) Signed-off-by: Antonio Fernandez Alhambra --- operator-framework-core/pom.xml | 2 +- .../io/javaoperatorsdk/operator/api/config/Utils.java | 10 ++++++++-- .../operator/api/config/VersionTest.java | 11 +++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/pom.xml b/operator-framework-core/pom.xml index a7d06ebdc1..e0c6031670 100644 --- a/operator-framework-core/pom.xml +++ b/operator-framework-core/pom.xml @@ -128,7 +128,7 @@ ${git-commit-id-maven-plugin.version} true - ${project.build.outputDirectory}/version.properties + ${project.build.outputDirectory}/operator-sdk-version.properties ^git.build.time$ ^git.commit.id.(abbrev|full)$ diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java index 6ad4928c86..4613d9645d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java @@ -42,6 +42,8 @@ public class Utils { public static final String GENERIC_PARAMETER_TYPE_ERROR_PREFIX = "Couldn't retrieve generic parameter type from "; + public static final String VERSION_PROPERTIES_FILE_NAME = "operator-sdk-version.properties"; + public static final Version VERSION = loadFromProperties(); /** @@ -52,7 +54,9 @@ public class Utils { */ private static Version loadFromProperties() { final var is = - Thread.currentThread().getContextClassLoader().getResourceAsStream("version.properties"); + Thread.currentThread() + .getContextClassLoader() + .getResourceAsStream(VERSION_PROPERTIES_FILE_NAME); final var properties = new Properties(); if (is != null) { @@ -62,7 +66,9 @@ private static Version loadFromProperties() { log.warn("Couldn't load version information: {}", e.getMessage()); } } else { - log.warn("Couldn't find version.properties file. Default version information will be used."); + log.warn( + "Couldn't find {} file. Default version information will be used.", + VERSION_PROPERTIES_FILE_NAME); } Date builtTime; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/VersionTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/VersionTest.java index a8202dd0b3..7fff961ff7 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/VersionTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/VersionTest.java @@ -18,6 +18,8 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; class VersionTest { @@ -28,4 +30,13 @@ void versionShouldReturnTheSameResultFromMavenAndProperties() { assertEquals(versionFromProperties, versionFromMaven); } + + @Test + void versionShouldBeLoadedFromTheGeneratedPropertiesFile() { + assertNotNull( + Thread.currentThread() + .getContextClassLoader() + .getResource(Utils.VERSION_PROPERTIES_FILE_NAME)); + assertNotEquals(Version.UNKNOWN.getCommit(), Utils.VERSION.getCommit()); + } } From 5100af735a0dd8adf66aef57452b198bbc6f4a8a Mon Sep 17 00:00:00 2001 From: Antonio <122279781+afalhambra-hivemq@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:30:18 +0200 Subject: [PATCH 06/11] feat: allow removing the default namespace index of an informer (#3623) Signed-off-by: Antonio Fernandez Alhambra --- .../content/en/docs/documentation/eventing.md | 29 ++++- .../documentation/operations/configuration.md | 1 + .../ControllerConfigurationOverrider.java | 12 ++ .../api/config/informer/Informer.java | 18 +++ .../informer/InformerConfiguration.java | 30 +++++ .../InformerEventSourceConfiguration.java | 6 + .../operator/api/reconciler/Constants.java | 1 + .../source/informer/InformerManager.java | 3 +- .../informer/pool/AbstractInformerPool.java | 7 +- .../informer/pool/DefaultInformerPool.java | 8 ++ .../informer/pool/InformerClassifier.java | 31 ++++- .../api/config/InformerConfigurationTest.java | 22 ++++ .../InformerManagerConcurrentReleaseTest.java | 10 +- .../source/informer/InformerWrapperTest.java | 2 +- .../pool/AbstractInformerPoolTest.java | 76 ++++++++++++ .../pool/DefaultInformerPoolTest.java | 6 +- .../informer/pool/InformerClassifierTest.java | 115 +++++++++++++++--- .../pool/NonSharingInformerPoolTest.java | 2 +- .../operator/config/loader/ConfigLoader.java | 8 +- .../WithoutNamespaceIndexIT.java | 76 ++++++++++++ .../WithoutNamespaceIndexTestReconciler.java | 86 +++++++++++++ .../config/loader/ConfigLoaderTest.java | 17 ++- 22 files changed, 529 insertions(+), 37 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPoolTest.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexTestReconciler.java diff --git a/docs/content/en/docs/documentation/eventing.md b/docs/content/en/docs/documentation/eventing.md index e7aea6b065..4b890407e4 100644 --- a/docs/content/en/docs/documentation/eventing.md +++ b/docs/content/en/docs/documentation/eventing.md @@ -348,6 +348,30 @@ See also [CaffeineBoundedItemStores](https://github.com/operator-framework/java-operator-sdk/blob/main/caffeine-bounded-cache-support/src/main/java/io/javaoperatorsdk/operator/processing/event/source/cache/CaffeineBoundedItemStores.java) for more details. +### Removing the Namespace Index + +Informers keep an index from namespace to the resources cached for it. JOSDK never reads that index, +only the ones registered explicitly through `IndexerResourceCache.addIndexers(..)`, so it can be +removed to save an entry per cached resource: + +```java +@ControllerConfiguration(informer = @Informer(withoutNamespaceIndex = true)) +public class MyReconciler implements Reconciler { } +``` + +The same option is available on `InformerEventSourceConfiguration.Builder` for event sources, and on +`InformerConfiguration.Builder`. + +This matters most for informers that cache a large number of resources, and in particular together +with a custom [item store](#bounded-caches-for-informers) that keeps only a reduced form of each +resource: the index is keyed independently of what the store does with the resource itself, so +shrinking what is cached does not shrink the index. + +Note that this takes part in the informer pool identity described below: an event source that +removes the index does not share an informer with one that keeps it. If two event sources watch the +same resource type and only one of them sets the option, they end up with two informers, and two +caches of that resource type, which can cost far more memory than the index ever did. + ### Sharing Informers Between Controllers (Informer Pool) {{% alert title="Experimental" color="warning" %}} @@ -374,7 +398,10 @@ Two event sources share an informer when their effective informer configuration - the resource type (or the group/version/kind for generic resources), - the watched namespace, - the label, field and shard selectors, -- the configured [item store](#bounded-caches-for-informers). +- the configured [item store](#bounded-caches-for-informers), +- whether the [namespace index is removed](#removing-the-namespace-index): it cannot be present for + one event source and absent for another on one shared informer, so event sources that disagree on + it are backed by separate informers. The `informerListLimit` is intentionally *not* part of this identity: if two otherwise-equivalent event sources request a different list limit, the existing informer is reused (a warning is logged diff --git a/docs/content/en/docs/documentation/operations/configuration.md b/docs/content/en/docs/documentation/operations/configuration.md index cdfb1b7fdb..9e2576cbfc 100644 --- a/docs/content/en/docs/documentation/operations/configuration.md +++ b/docs/content/en/docs/documentation/operations/configuration.md @@ -356,6 +356,7 @@ All controller-level keys are prefixed with `josdk.controller.. | `josdk.controller..informer.label-selector` | `String` | Label selector for the primary resource informer (alias for `label-selector`) | | `josdk.controller..informer.shard-selector` | `String` | Shard selector for the primary resource informer (alias for `shard-selector`) | | `josdk.controller..informer.list-limit` | `Long` | Page size for paginated informer list requests; omit for no pagination | +| `josdk.controller..informer.without-namespace-index` | `Boolean` | Removes the namespace index the informer maintains; defaults to `false`. See [Removing the Namespace Index](../eventing#removing-the-namespace-index) | #### Retry diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index 1c1e03c870..0a32bbdcaf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -210,6 +210,18 @@ public ControllerConfigurationOverrider withInformerListLimit(Long informerLi return this; } + /** + * Whether to remove the namespace index the underlying informer maintains by default. Note that + * event sources that disagree on this setting do not share an informer. + * + * @param withoutNamespaceIndex true to remove the namespace index, false (the default) to keep it + * @see io.javaoperatorsdk.operator.api.config.informer.Informer#withoutNamespaceIndex() + */ + public ControllerConfigurationOverrider withoutNamespaceIndex(boolean withoutNamespaceIndex) { + config.withoutNamespaceIndex(withoutNamespaceIndex); + return this; + } + public ControllerConfigurationOverrider replacingNamedDependentResourceConfig( String name, Object dependentResourceConfig) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/Informer.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/Informer.java index 04f97902d3..69999ef04f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/Informer.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/Informer.java @@ -31,6 +31,7 @@ import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_COMPARABLE_RESOURCE_VERSION; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_FOLLOW_CONTROLLER_NAMESPACE_CHANGES; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_GHOST_RESOURCE_CHECK_INTERVAL_MILLIS; +import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_WITHOUT_NAMESPACE_INDEX; import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_LONG_VALUE_SET; import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_VALUE_SET; @@ -153,6 +154,23 @@ */ boolean comparableResourceVersions() default DEFAULT_COMPARABLE_RESOURCE_VERSION; + /** + * Whether to remove the namespace index that the underlying informer maintains by default. + * + *

The framework never reads that index, it only reads the indexes registered through {@link + * io.javaoperatorsdk.operator.processing.event.source.IndexerResourceCache#addIndexers}, so + * dropping it saves an entry per cached resource. It is worth setting when the informer caches a + * large number of resources and nothing looks them up by namespace, in particular together with a + * custom {@link #itemStore()} that keeps only a reduced form of each resource. + * + *

Note that this makes the informer a distinct one for pooling purposes: event sources that + * disagree on this setting do not share an informer, because the index cannot be present for one + * of them and absent for the other. + * + * @since 5.7.0 + */ + boolean withoutNamespaceIndex() default DEFAULT_WITHOUT_NAMESPACE_INDEX; + /** * @deprecated Ghost resource checking is now triggered by the informer's onList callback. This * setting is no longer used. diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerConfiguration.java index 9fe25c999d..106f5898c5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerConfiguration.java @@ -58,6 +58,7 @@ public class InformerConfiguration { private Long informerListLimit; private FieldSelector fieldSelector; private Boolean comparableResourceVersions; + private Boolean withoutNamespaceIndex; protected InformerConfiguration( Class resourceClass, @@ -75,6 +76,7 @@ protected InformerConfiguration( Long informerListLimit, FieldSelector fieldSelector, Boolean comparableResourceVersions, + Boolean withoutNamespaceIndex, // TODO for removal in major release Duration ghostResourceCacheCheckInterval) { this(resourceClass, resourceGroupVersionKind); @@ -91,6 +93,7 @@ protected InformerConfiguration( this.informerListLimit = informerListLimit; this.fieldSelector = fieldSelector; this.comparableResourceVersions = comparableResourceVersions; + this.withoutNamespaceIndex = withoutNamespaceIndex; } private InformerConfiguration(Class resourceClass, GroupVersionKind resourceGroupVersionKind) { @@ -140,6 +143,7 @@ public static InformerConfiguration.Builder builder( original.informerListLimit, original.fieldSelector, original.comparableResourceVersions, + original.withoutNamespaceIndex, null) .builder; } @@ -332,6 +336,15 @@ public boolean isComparableResourceVersions() { return comparableResourceVersions; } + /** + * Whether the namespace index the underlying informer maintains by default is removed. + * + * @see Informer#withoutNamespaceIndex() + */ + public boolean isWithoutNamespaceIndex() { + return withoutNamespaceIndex; + } + @SuppressWarnings("UnusedReturnValue") public class Builder { @@ -349,6 +362,9 @@ public InformerConfiguration buildForController() { if (comparableResourceVersions == null) { comparableResourceVersions = DEFAULT_COMPARABLE_RESOURCE_VERSION; } + if (withoutNamespaceIndex == null) { + withoutNamespaceIndex = DEFAULT_WITHOUT_NAMESPACE_INDEX; + } return InformerConfiguration.this; } @@ -364,6 +380,9 @@ public InformerConfiguration build() { if (comparableResourceVersions == null) { comparableResourceVersions = DEFAULT_COMPARABLE_RESOURCE_VERSION; } + if (withoutNamespaceIndex == null) { + withoutNamespaceIndex = DEFAULT_WITHOUT_NAMESPACE_INDEX; + } return InformerConfiguration.this; } @@ -417,6 +436,7 @@ public InformerConfiguration.Builder initFromAnnotation( .map(f -> new FieldSelector.Field(f.path(), f.value(), f.negated())) .toList())); withComparableResourceVersions(informerConfig.comparableResourceVersions()); + withoutNamespaceIndex(informerConfig.withoutNamespaceIndex()); } return this; } @@ -533,6 +553,16 @@ private static boolean isEmpty(FieldSelector fieldSelector) { || fieldSelector.getFields().isEmpty(); } + /** + * Whether to remove the namespace index the underlying informer maintains by default. + * + * @see Informer#withoutNamespaceIndex() + */ + public Builder withoutNamespaceIndex(boolean withoutNamespaceIndex) { + InformerConfiguration.this.withoutNamespaceIndex = withoutNamespaceIndex; + return this; + } + public Builder withComparableResourceVersions(boolean comparableResourceVersions) { InformerConfiguration.this.comparableResourceVersions = comparableResourceVersions; return this; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java index a4ca2f63d4..d40469de4c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java @@ -292,6 +292,11 @@ public Builder withComparableResourceVersion(boolean comparableResourceVersio return this; } + public Builder withoutNamespaceIndex(boolean withoutNamespaceIndex) { + config.withoutNamespaceIndex(withoutNamespaceIndex); + return this; + } + @Deprecated(forRemoval = true) public Builder withGhostResourceCacheCheckInterval( Duration ghostResourceCacheCheckInterval) { @@ -317,6 +322,7 @@ public void updateFrom(InformerConfiguration informerConfig) { .withGenericFilter(informerConfig.getGenericFilter()) .withInformerListLimit(informerConfig.getInformerListLimit()) .withComparableResourceVersions(informerConfig.isComparableResourceVersions()) + .withoutNamespaceIndex(informerConfig.isWithoutNamespaceIndex()) .withFieldSelector(informerConfig.getFieldSelector()); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java index fad19d2021..2ae2de8b4f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java @@ -43,6 +43,7 @@ public final class Constants { public static final String CONTROLLER_NAME = "controller.name"; public static final boolean DEFAULT_FOLLOW_CONTROLLER_NAMESPACE_CHANGES = true; public static final boolean DEFAULT_COMPARABLE_RESOURCE_VERSION = true; + public static final boolean DEFAULT_WITHOUT_NAMESPACE_INDEX = false; @Deprecated(forRemoval = true) public static final long DEFAULT_GHOST_RESOURCE_CHECK_INTERVAL_MILLIS = 3 * 60 * 1000; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java index 6caf39ccd9..ae6694cfe4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java @@ -169,7 +169,8 @@ private InformerClassifier getClassifier(String namespaceIdentifier) { configuration.getInformerConfig().getResourceGroupVersionKind(), configuration.getInformerConfig().getFieldSelector(), configuration.getInformerConfig().getInformerListLimit(), - configuration.getInformerConfig().getItemStore()); + configuration.getInformerConfig().getItemStore(), + configuration.getInformerConfig().isWithoutNamespaceIndex()); } private KubernetesClient getTargetClient() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java index 4dc1920955..21464b378b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java @@ -71,7 +71,7 @@ public void setConfigurationService(ConfigurationService configurationService) { */ public abstract long numberOfInformersForResource(Class resourceClass); - @SuppressWarnings({"rawtypes", "unchecked"}) + @SuppressWarnings({"rawtypes", "unchecked", "resource"}) protected SharedIndexInformer createInformer(InformerClassifier classifier) { var client = classifier.client(); @@ -116,6 +116,11 @@ protected SharedIndexInformer createInformer(InformerClassifier classifier) { Optional.ofNullable(classifier.itemStore()).ifPresent(informer::itemStore); + if (classifier.withoutNamespaceIndex()) { + // the framework only reads the indexes registered through the resource cache, never this one + informer.removeNamespaceIndex(); + } + configurationService .getInformerStoppedHandler() .ifPresent( diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPool.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPool.java index b85b7d9f0b..24dcdba4c8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPool.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPool.java @@ -43,6 +43,14 @@ public SharedIndexInformer getInformer( synchronized (this) { var pooled = informers.get(classifier); if (pooled == null) { + if (informers.keySet().stream() + .anyMatch(existing -> existing.differsOnlyByNamespaceIndex(classifier))) { + log.warn( + "Creating a second informer for classifier {} that differs from an existing one only" + + " by withoutNamespaceIndex, so the resource type is cached twice. Set the" + + " option the same way on both to share one informer.", + classifier); + } informer = createInformer(classifier); informers.put(classifier, new PooledInformer(informer, new AtomicInteger(1))); log.debug( diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifier.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifier.java index e4023a93e9..5f4cfae87f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifier.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifier.java @@ -36,6 +36,9 @@ * limit still share an informer; the limit of whichever classifier created the informer is * kept (a pool is expected to warn about this, see {@link * #differsOnlyByInformerListLimit(InformerClassifier)}). + *

  • {@link #withoutNamespaceIndex()} on the other hand is part of the + * identity: the namespace index cannot be present for one event source and absent for another + * that shares the same informer, so event sources disagreeing on it get separate informers. *
  • Indexers are not part of the classifier at all: they are registered on the informer under a * name qualified with the event source that added them, so those of different event sources * can live side by side on a shared informer without colliding. @@ -61,7 +64,8 @@ public record InformerClassifier( GroupVersionKind groupVersionKind, FieldSelector fieldSelector, Long informerListLimit, - ItemStore itemStore) { + ItemStore itemStore, + boolean withoutNamespaceIndex) { @Override public boolean equals(Object o) { @@ -71,6 +75,16 @@ public boolean equals(Object o) { if (!(o instanceof InformerClassifier that)) { return false; } + return equalsIgnoringNamespaceIndex(that) + && withoutNamespaceIndex == that.withoutNamespaceIndex; + } + + /** + * Equality of everything the identity is made of except {@link #withoutNamespaceIndex()}, so that + * {@link #equals(Object)} and {@link #differsOnlyByNamespaceIndex(InformerClassifier)} cannot + * drift apart when a component is added. + */ + private boolean equalsIgnoringNamespaceIndex(InformerClassifier that) { return client == that.client && Objects.equals(labelSelector, that.labelSelector) && Objects.equals(shardSelector, that.shardSelector) @@ -91,7 +105,8 @@ public int hashCode() { resourceClass, groupVersionKind, fieldSelector, - itemStore); + itemStore, + withoutNamespaceIndex); } /** @@ -123,6 +138,8 @@ public String toString() { + informerListLimit + ", itemStore=" + itemStore + + ", withoutNamespaceIndex=" + + withoutNamespaceIndex + "]"; } @@ -140,4 +157,14 @@ private String masterUrl() { public boolean differsOnlyByInformerListLimit(InformerClassifier other) { return equals(other) && !Objects.equals(informerListLimit, other.informerListLimit); } + + /** + * Checks whether this classifier and the other are equal in every attribute except for {@link + * #withoutNamespaceIndex()}, which differs between them. Unlike the list limit, that setting is + * part of the identity, so such a pair is served by two informers rather than one. + */ + public boolean differsOnlyByNamespaceIndex(InformerClassifier other) { + return equalsIgnoringNamespaceIndex(other) + && withoutNamespaceIndex != other.withoutNamespaceIndex; + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerConfigurationTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerConfigurationTest.java index 16e5ab578b..ef1bfafd24 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerConfigurationTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/InformerConfigurationTest.java @@ -75,6 +75,28 @@ void nullLabelSelectorByDefault() { assertNull(informerConfig.getLabelSelector()); } + @Test + void keepsTheNamespaceIndexByDefault() { + final var informerConfig = InformerConfiguration.builder(ConfigMap.class).build(); + assertFalse(informerConfig.isWithoutNamespaceIndex()); + } + + @Test + void keepsTheNamespaceIndexByDefaultForController() { + final var informerConfig = InformerConfiguration.builder(ConfigMap.class).buildForController(); + assertFalse(informerConfig.isWithoutNamespaceIndex()); + } + + @Test + void withoutNamespaceIndexIsCarriedOverWhenCopyingTheConfiguration() { + final var original = + InformerConfiguration.builder(ConfigMap.class).withoutNamespaceIndex(true).build(); + + final var copy = InformerConfiguration.builder(original).build(); + + assertTrue(copy.isWithoutNamespaceIndex()); + } + @Test void nullShardSelectorByDefault() { final var informerConfig = InformerConfiguration.builder(ConfigMap.class).build(); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManagerConcurrentReleaseTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManagerConcurrentReleaseTest.java index e8801781db..6fe3b3734e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManagerConcurrentReleaseTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManagerConcurrentReleaseTest.java @@ -52,7 +52,7 @@ * InformerManager#changeNamespaces(Set)} can run concurrently, so removing the source from the * manager has to be what claims the right to release it. */ -@SuppressWarnings({"rawtypes", "unchecked"}) +@SuppressWarnings("unchecked") class InformerManagerConcurrentReleaseTest { private static final String NAMESPACE = "ns1"; @@ -94,6 +94,9 @@ void concurrentStopAndNamespaceChangeReleaseTheInformerOnlyOnce() throws Excepti pool.proceed.countDown(); namespaceChange.join(TimeUnit.SECONDS.toMillis(5)); + assertThat(pool.proceededInTime) + .as("the blocked release must be let through, otherwise the paths never interleaved") + .isTrue(); assertThat(pool.releaseCount.get()) .as("the same namespace must not be released twice") .isEqualTo(1); @@ -114,7 +117,7 @@ private ControllerConfiguration controllerConfiguration() { /** Has to match what the manager builds for {@link #NAMESPACE} so it hits the same pool entry. */ private InformerClassifier classifier() { return new InformerClassifier<>( - clientMock, null, null, NAMESPACE, Deployment.class, null, null, null, null); + clientMock, null, null, NAMESPACE, Deployment.class, null, null, null, null, false); } /** Blocks inside the first release so the two teardown paths can be interleaved on purpose. */ @@ -124,6 +127,7 @@ private static class LatchingInformerPool extends DefaultInformerPool { private final CountDownLatch proceed = new CountDownLatch(1); private final AtomicInteger releaseCount = new AtomicInteger(); private final AtomicBoolean blockNextRelease = new AtomicBoolean(true); + private final AtomicBoolean proceededInTime = new AtomicBoolean(); @Override public Optional> releaseInformer( @@ -134,7 +138,7 @@ public Optional> releaseInformer( if (blockNextRelease.compareAndSet(true, false)) { enteredRelease.countDown(); try { - proceed.await(5, TimeUnit.SECONDS); + proceededInTime.set(proceed.await(5, TimeUnit.SECONDS)); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapperTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapperTest.java index e175074125..0bdcee35df 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapperTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapperTest.java @@ -112,7 +112,7 @@ private InformerWrapper wrapper(String controller, String ev informer, "default", new InformerClassifier<>( - null, null, null, "default", TestCustomResource.class, null, null, null, null), + null, null, null, "default", TestCustomResource.class, null, null, null, null, false), controller, eventSource); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPoolTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPoolTest.java new file mode 100644 index 0000000000..e5a1208f06 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPoolTest.java @@ -0,0 +1,76 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.processing.event.source.informer.pool; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.client.KubernetesClient; +import io.javaoperatorsdk.operator.MockKubernetesClient; +import io.javaoperatorsdk.operator.api.config.BaseConfigurationService; +import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; + +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +/** + * Unit tests for the informer creation that {@link AbstractInformerPool} performs for every pool, + * as opposed to the sharing strategy a concrete pool adds on top (covered by {@link + * DefaultInformerPoolTest} and {@link NonSharingInformerPoolTest}). {@link NonSharingInformerPool} + * is used merely as the simplest concrete subclass to reach that inherited behavior through. + */ +class AbstractInformerPoolTest { + + private static final String CONTROLLER = "controller"; + private static final String ES_NAME = "event-source"; + private static final String NAMESPACE = "default"; + + private final KubernetesClient client = MockKubernetesClient.client(TestCustomResource.class); + private final NonSharingInformerPool pool = new NonSharingInformerPool(); + + @BeforeEach + void setUp() { + pool.setConfigurationService(new BaseConfigurationService()); + } + + @Test + void keepsTheNamespaceIndexWhenTheClassifierDoesNotAskForIt() { + var informer = pool.getInformer(CONTROLLER, ES_NAME, classifier(false)); + + verify(informer, never()).removeNamespaceIndex(); + } + + @Test + void removesTheNamespaceIndexWhenTheClassifierAsksForIt() { + var informer = pool.getInformer(CONTROLLER, ES_NAME, classifier(true)); + + verify(informer).removeNamespaceIndex(); + } + + private InformerClassifier classifier(boolean withoutNamespaceIndex) { + return new InformerClassifier<>( + client, + null, + null, + NAMESPACE, + TestCustomResource.class, + null, + null, + null, + null, + withoutNamespaceIndex); + } +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPoolTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPoolTest.java index 4ca4db22bd..744dbdccee 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPoolTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/DefaultInformerPoolTest.java @@ -89,10 +89,10 @@ void createsSeparateInformersForDifferentClientsWithTheSameApiServerUrl() { void sharesInformerWhenClassifiersDifferOnlyByListLimit() { var withLimit100 = new InformerClassifier<>( - client, null, null, "default", TestCustomResource.class, null, null, 100L, null); + client, null, null, "default", TestCustomResource.class, null, null, 100L, null, false); var withLimit200 = new InformerClassifier<>( - client, null, null, "default", TestCustomResource.class, null, null, 200L, null); + client, null, null, "default", TestCustomResource.class, null, null, 200L, null, false); var first = pool.getInformer(CONTROLLER, ES_NAME, withLimit100); var second = pool.getInformer("other-controller", "other-es", withLimit200); @@ -149,6 +149,6 @@ private InformerClassifier classifier(String namespace) { private InformerClassifier classifier( KubernetesClient forClient, String namespace) { return new InformerClassifier<>( - forClient, null, null, namespace, TestCustomResource.class, null, null, null, null); + forClient, null, null, namespace, TestCustomResource.class, null, null, null, null, false); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifierTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifierTest.java index 47f359a3f3..9025158918 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifierTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/InformerClassifierTest.java @@ -45,7 +45,7 @@ class InformerClassifierTest { private static final FieldSelector FIELD_SELECTOR = new FieldSelector(new FieldSelector.Field("status.phase", "Running")); private static final Long LIMIT = 100L; - private static final ItemStore ITEM_STORE = mock(ItemStore.class); + private static final ItemStore ITEM_STORE = mock(); private static InformerClassifier base() { return new InformerClassifier<>( @@ -57,7 +57,8 @@ private static InformerClassifier base() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE); + ITEM_STORE, + false); } @Test @@ -78,7 +79,8 @@ void informerListLimitIsExcludedFromEqualityAndHashCode() { GVK, FIELD_SELECTOR, 999L, - ITEM_STORE); + ITEM_STORE, + false); assertThat(base()).isEqualTo(withOtherLimit); assertThat(base()).hasSameHashCodeAs(withOtherLimit); @@ -103,7 +105,8 @@ void toStringContainsTheApiServerUrlDerivedFromTheClient() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE); + ITEM_STORE, + false); assertThat(classifier.toString()) .contains("https://localhost:8443/") @@ -124,7 +127,8 @@ void toStringDoesNotFailWithoutAClient() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE); + ITEM_STORE, + false); assertThat(classifier.toString()).contains(NAMESPACE); } @@ -144,7 +148,8 @@ void differsWhenClientDiffers() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test @@ -160,7 +165,8 @@ void differsWhenLabelSelectorDiffers() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test @@ -176,7 +182,8 @@ void differsWhenShardSelectorDiffers() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test @@ -192,14 +199,16 @@ void differsWhenNamespaceDiffers() { GVK, FIELD_SELECTOR, LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test void differsWhenResourceClassDiffers() { - // item stores are null here because their generic type is tied to the resource class, which is - // exactly the field under test; this keeps the resource class the only difference. - var forTestResource = + // the resource class is the field under test, so everything tied to it follows: the item stores + // are null to keep it the only difference, and both are declared as wildcards since their type + // argument differs with it + InformerClassifier forTestResource = new InformerClassifier<>( CLIENT, LABEL, @@ -209,8 +218,9 @@ void differsWhenResourceClassDiffers() { GVK, FIELD_SELECTOR, LIMIT, - null); - var forOtherResource = + null, + false); + InformerClassifier forOtherResource = new InformerClassifier<>( CLIENT, LABEL, @@ -220,7 +230,8 @@ void differsWhenResourceClassDiffers() { GVK, FIELD_SELECTOR, LIMIT, - null); + null, + false); assertThat(forTestResource).isNotEqualTo(forOtherResource); } @@ -238,7 +249,8 @@ void differsWhenGroupVersionKindDiffers() { new GroupVersionKind("sample.io/v1", "Bar"), FIELD_SELECTOR, LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test @@ -254,7 +266,8 @@ void differsWhenFieldSelectorDiffers() { GVK, new FieldSelector(new FieldSelector.Field("status.phase", "Pending")), LIMIT, - ITEM_STORE)); + ITEM_STORE, + false)); } @Test @@ -270,7 +283,25 @@ void differsWhenItemStoreDiffers() { GVK, FIELD_SELECTOR, LIMIT, - mock(ItemStore.class))); + mock(), + false)); + } + + @Test + void differsWhenWithoutNamespaceIndexDiffers() { + assertThat(base()) + .isNotEqualTo( + new InformerClassifier<>( + CLIENT, + LABEL, + SHARD, + NAMESPACE, + TestCustomResource.class, + GVK, + FIELD_SELECTOR, + LIMIT, + ITEM_STORE, + true)); } @Test @@ -285,7 +316,8 @@ void differsOnlyByInformerListLimitIsTrueWhenOnlyLimitDiffers() { GVK, FIELD_SELECTOR, 999L, - ITEM_STORE); + ITEM_STORE, + false); assertThat(base().differsOnlyByInformerListLimit(withOtherLimit)).isTrue(); } @@ -308,8 +340,51 @@ void differsOnlyByInformerListLimitIsFalseWhenAnotherFieldDiffers() { GVK, FIELD_SELECTOR, 999L, - ITEM_STORE); + ITEM_STORE, + false); assertThat(base().differsOnlyByInformerListLimit(differentNamespaceAndLimit)).isFalse(); } + + @Test + void differsOnlyByNamespaceIndexIsTrueWhenOnlyIndexDiffers() { + var withoutIndex = + new InformerClassifier<>( + CLIENT, + LABEL, + SHARD, + NAMESPACE, + TestCustomResource.class, + GVK, + FIELD_SELECTOR, + LIMIT, + ITEM_STORE, + true); + + assertThat(base().differsOnlyByNamespaceIndex(withoutIndex)).isTrue(); + } + + @Test + void differsOnlyByNamespaceIndexIsFalseWhenFullyEqual() { + assertThat(base().differsOnlyByNamespaceIndex(base())).isFalse(); + } + + @Test + void differsOnlyByNamespaceIndexIsFalseWhenAnotherFieldDiffers() { + // different namespace AND different index setting: not "only by namespace index" + var differentNamespaceAndIndex = + new InformerClassifier<>( + CLIENT, + LABEL, + SHARD, + "other-ns", + TestCustomResource.class, + GVK, + FIELD_SELECTOR, + LIMIT, + ITEM_STORE, + true); + + assertThat(base().differsOnlyByNamespaceIndex(differentNamespaceAndIndex)).isFalse(); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/NonSharingInformerPoolTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/NonSharingInformerPoolTest.java index 364103849e..939905f065 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/NonSharingInformerPoolTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/NonSharingInformerPoolTest.java @@ -138,6 +138,6 @@ void releaseOfUnknownInformerReturnsEmptyAndDoesNotThrow() { private InformerClassifier classifier(String namespace) { return new InformerClassifier<>( - client, null, null, namespace, TestCustomResource.class, null, null, null, null); + client, null, null, namespace, TestCustomResource.class, null, null, null, null, false); } } diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java index c8daf89724..bc9a4ecbbb 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java @@ -170,7 +170,11 @@ public static ConfigLoader getDefault() { new ConfigBinding<>( "informer.list-limit", Long.class, - ControllerConfigurationOverrider::withInformerListLimit)); + ControllerConfigurationOverrider::withInformerListLimit), + new ConfigBinding<>( + "informer.without-namespace-index", + Boolean.class, + ControllerConfigurationOverrider::withoutNamespaceIndex)); private final ConfigProvider configProvider; @@ -228,7 +232,7 @@ Consumer> applyControllerConfigs(String cont Consumer> retryStep = buildRetryConsumer(prefix); if (retryStep != null) { - consumer = consumer == null ? retryStep : consumer.andThen(retryStep); + consumer = consumer.andThen(retryStep); } Consumer> rateLimiterStep = buildRateLimiterConsumer(prefix); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexIT.java new file mode 100644 index 0000000000..bd68813608 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexIT.java @@ -0,0 +1,76 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.withoutnamespaceindex; + +import java.time.Duration; +import java.util.Map; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.SecretBuilder; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +class WithoutNamespaceIndexIT { + + private static final String TEST_RESOURCE_NAME = "test1"; + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(new WithoutNamespaceIndexTestReconciler()) + .build(); + + @Test + void reconcilesAndResolvesSecondariesWithoutTheNamespaceIndex() { + var configMap = + extension.create( + new ConfigMapBuilder() + .withMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build()) + .withData(Map.of("key", "value")) + .build()); + extension.create( + new SecretBuilder() + .withMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build()) + .build()); + + var reconciler = extension.getReconcilerOfType(WithoutNamespaceIndexTestReconciler.class); + await() + .pollDelay(Duration.ofMillis(150)) + .untilAsserted( + () -> { + assertThat(reconciler.getNumberOfExecutions()).isPositive(); + assertThat(reconciler.getSecondariesFound()).contains(TEST_RESOURCE_NAME); + }); + + // an update has to reach the reconciler too: the informer keeps serving events and cache reads + // with the index gone, it is not just the initial sync that works + var executionsBeforeUpdate = reconciler.getNumberOfExecutions(); + configMap.setData(Map.of("key", "updated")); + extension.update(configMap); + + await() + .untilAsserted( + () -> + assertThat(reconciler.getNumberOfExecutions()) + .isGreaterThan(executionsBeforeUpdate)); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexTestReconciler.java new file mode 100644 index 0000000000..930e41480a --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/withoutnamespaceindex/WithoutNamespaceIndexTestReconciler.java @@ -0,0 +1,86 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.withoutnamespaceindex; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.Secret; +import io.javaoperatorsdk.operator.api.config.informer.Informer; +import io.javaoperatorsdk.operator.api.config.informer.InformerEventSourceConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.javaoperatorsdk.operator.processing.event.source.EventSource; +import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; +import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; + +/** + * Both informers drop the namespace index: the primary one through the annotation, the secondary + * one through the builder. Reconciling and resolving the secondary resource have to keep working, + * since neither reads that index. + */ +@ControllerConfiguration(informer = @Informer(withoutNamespaceIndex = true)) +public class WithoutNamespaceIndexTestReconciler + implements Reconciler, TestExecutionInfoProvider { + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + private final Set secondariesFound = Collections.synchronizedSet(new HashSet<>()); + + @Override + public UpdateControl reconcile(Secret resource, Context context) { + numberOfExecutions.addAndGet(1); + // reads through the secondary informer's cache, the part that would break if the framework + // relied on the index it just removed + context + .getSecondaryResource(ConfigMap.class) + .ifPresent(configMap -> secondariesFound.add(configMap.getMetadata().getName())); + return UpdateControl.noUpdate(); + } + + @Override + public List> prepareEventSources(EventSourceContext context) { + return List.of( + new InformerEventSource<>( + InformerEventSourceConfiguration.from(ConfigMap.class, Secret.class) + .withNamespacesInheritedFromController() + .withoutNamespaceIndex(true) + // the ConfigMap of a Secret is the one sharing its name + .withSecondaryToPrimaryMapper( + configMap -> + Set.of( + new ResourceID( + configMap.getMetadata().getName(), + configMap.getMetadata().getNamespace()))) + .build())); + } + + @Override + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } + + public Set getSecondariesFound() { + return secondariesFound; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java index 44fac32b7d..3ac9cea0dc 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.ConfigMap; import io.javaoperatorsdk.operator.api.config.BaseConfigurationService; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceOverrider; @@ -218,10 +219,23 @@ public Optional getValue(String key, Class type) { "josdk.controller.ctrl.informer.label-selector", "josdk.controller.ctrl.informer.shard-selector", "josdk.controller.ctrl.informer.list-limit", + "josdk.controller.ctrl.informer.without-namespace-index", "josdk.controller.ctrl.rate-limiter.refresh-period", "josdk.controller.ctrl.rate-limiter.limit-for-period"); } + @Test + void applyControllerConfigsAppliesInformerWithoutNamespaceIndex() { + var loader = + new ConfigLoader( + mapProvider(Map.of("josdk.controller.ctrl.informer.without-namespace-index", true))); + var overrider = ControllerConfigurationOverrider.override(baseControllerConfig()); + + loader.applyControllerConfigs("ctrl").accept(overrider); + + assertThat(overrider.build().getInformerConfig().isWithoutNamespaceIndex()).isTrue(); + } + @Test void operatorKeyPrefixIsJosdkDot() { assertThat(ConfigLoader.DEFAULT_OPERATOR_KEY_PREFIX).isEqualTo("josdk."); @@ -567,7 +581,6 @@ private static boolean isTypeCompatible(Class methodParam, Class bindingTy if (methodParam == long.class && bindingType == Long.class) return true; if (methodParam == Long.class && bindingType == long.class) return true; if (methodParam == double.class && bindingType == Double.class) return true; - if (methodParam == Double.class && bindingType == double.class) return true; - return false; + return methodParam == Double.class && bindingType == double.class; } } From 393ad2152d6a8bbd8eecd446da0235007abd33d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 17 Sep 2026 09:39:50 +0200 Subject: [PATCH 07/11] feat: schedule polling and retries on managed executors instead of Timers PollingEventSource and TimerEventSource each created a java.util.Timer, and PerResourcePollingEventSource a ScheduledThreadPoolExecutor of its own, so every polling event source and every controller cost a thread that the operator neither sized nor shut down. They now schedule on executors managed by ExecutorServiceManager, which builds them from ConfigurationService: - getScheduledExecutorService() backs the polling event sources - getRetryAndRescheduleExecutorService() backs the retry and reschedule timer of every controller, kept separate so that a slow poll cannot delay a retry Both are sized by concurrentScheduledTaskThreads() (4 by default) and concurrentRetryAndRescheduleThreads() (2 by default), overridable programmatically or through the josdk.scheduled-tasks.concurrent-threads and josdk.retry-and-reschedule.concurrent-threads keys. Both default to daemon threads, discard the tasks scheduled for later on shutdown so they don't hold up the termination of the operator, and drop cancelled tasks eagerly. The previous Executors.newScheduledThreadPool(0) was effectively single threaded and created non daemon threads. An event source resolves its executor on every start rather than at creation time, since the manager replaces its pools when the operator is restarted, and only shuts one down if it created it itself. That also fixes PerResourcePollingEventSource shutting down a user supplied executor and not registering its tasks again after a restart. PollingEventSource gains a constructor taking an EventSourceContext, the one without it is deprecated, and both polling configurations accept an executor to run a single event source on a pool of its own. --- .../content/en/docs/documentation/eventing.md | 22 +++ .../documentation/operations/configuration.md | 7 + .../api/config/ConfigurationService.java | 80 +++++++++++ .../config/ConfigurationServiceOverrider.java | 104 ++++++++++++++ .../api/config/ExecutorServiceManager.java | 41 ++++-- .../operator/api/config/Utils.java | 48 +++++++ .../external/PollingDependentResource.java | 1 + .../processing/event/EventSourceManager.java | 4 +- .../processing/event/EventSources.java | 13 +- .../PerResourcePollingConfiguration.java | 12 +- .../PerResourcePollingEventSource.java | 57 +++++++- .../source/polling/PollingConfiguration.java | 18 ++- .../polling/PollingConfigurationBuilder.java | 20 ++- .../source/polling/PollingEventSource.java | 129 ++++++++++++++---- .../event/source/timer/TimerEventSource.java | 91 +++++++++--- .../ConfigurationServiceOverriderTest.java | 59 ++++++++ .../config/ExecutorServiceManagerTest.java | 6 + .../polling/PollingEventSourceTest.java | 77 ++++++++++- .../source/timer/TimerEventSourceTest.java | 57 ++++++++ .../operator/config/loader/ConfigLoader.java | 8 ++ .../config/loader/ConfigLoaderTest.java | 24 ++++ ...edExternalDependentResourceReconciler.java | 1 + 22 files changed, 804 insertions(+), 75 deletions(-) diff --git a/docs/content/en/docs/documentation/eventing.md b/docs/content/en/docs/documentation/eventing.md index 4b890407e4..e954f8a31f 100644 --- a/docs/content/en/docs/documentation/eventing.md +++ b/docs/content/en/docs/documentation/eventing.md @@ -222,6 +222,28 @@ is similar to `PerResourcePollingEventSource` except that, contrary to that even doesn't poll a specific API separately per resource, but periodically and independently of actually observed primary resources. +#### Threading of the polling event sources + +Both polling event sources schedule their polls on an executor the operator shares between all of +its polling event sources. A poll therefore only starts once one of that executor's threads is +free, so if your operator registers many polling event sources, or if fetching your external +resources is slow, size the pool accordingly with +`ConfigurationServiceOverrider.withConcurrentScheduledTaskThreads` (4 threads by default): + +```java +Operator operator = new Operator(overrider -> overrider.withConcurrentScheduledTaskThreads(20)); +``` + +Retried and rescheduled reconciliations are triggered on a separate executor, so a slow poll can +never delay them. It is sized with `withConcurrentRetryAndRescheduleThreads` (2 threads by +default); few threads are needed there since triggering a reconciliation only enqueues an event +for one of the reconciliation threads to pick up. + +Use `ConfigurationServiceOverrider.withScheduledExecutorService` to replace the polling executor +altogether, or the `withExecutorService` method of the event source's own configuration builder to +poll a single event source on an executor of its own. An executor provided that way is not managed +by the operator: it is your responsibility to shut it down. + #### Inbound event sources [SimpleInboundEventSource](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/SimpleInboundEventSource.java) diff --git a/docs/content/en/docs/documentation/operations/configuration.md b/docs/content/en/docs/documentation/operations/configuration.md index 9e2576cbfc..eef800d66b 100644 --- a/docs/content/en/docs/documentation/operations/configuration.md +++ b/docs/content/en/docs/documentation/operations/configuration.md @@ -281,6 +281,13 @@ All operator-level keys are prefixed with `josdk.`. |---|---|---| | `josdk.workflow.executor-threads` | `Integer` | Thread pool size for workflow execution | +#### Scheduled Tasks + +| Key | Type | Description | +|---|---|---| +| `josdk.scheduled-tasks.concurrent-threads` | `Integer` | Thread pool size for the operator's scheduled tasks, i.e. the polling event sources | +| `josdk.retry-and-reschedule.concurrent-threads` | `Integer` | Thread pool size for triggering retried and rescheduled reconciliations | + #### Informer | Key | Type | Description | diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 35f46e5019..2dec5aabf2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -21,6 +21,7 @@ import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.function.Consumer; import org.slf4j.Logger; @@ -64,6 +65,20 @@ public interface ConfigurationService { /** The default number of threads used to process dependent workflows */ int DEFAULT_WORKFLOW_EXECUTOR_THREAD_NUMBER = DEFAULT_RECONCILIATION_THREADS_NUMBER; + /** + * The default number of threads used to run the operator's scheduled tasks, i.e. the periodic + * polls of {@link io.javaoperatorsdk.operator.processing.event.source.polling.PollingEventSource} + * and {@link + * io.javaoperatorsdk.operator.processing.event.source.polling.PerResourcePollingEventSource} + */ + int DEFAULT_SCHEDULED_TASK_THREADS_NUMBER = 4; + + /** + * The default number of threads used to trigger the operator's retried and rescheduled + * reconciliations + */ + int DEFAULT_RETRY_AND_RESCHEDULE_THREADS_NUMBER = 2; + /** * Creates a new {@link ConfigurationService} instance used to configure an {@link * io.javaoperatorsdk.operator.Operator} instance, starting from the specified base configuration @@ -219,6 +234,33 @@ default int concurrentWorkflowExecutorThreads() { return DEFAULT_WORKFLOW_EXECUTOR_THREAD_NUMBER; } + /** + * Number of threads the operator can spin out to run its scheduled (i.e. periodic or delayed) + * tasks with the default executor. These threads are shared by all the polling event sources of + * the operator, so this number should be raised when many, or slow, polling event sources are + * registered: a poll only starts once a thread is available, and a slow poll therefore delays the + * polls of the other event sources. + * + * @return the maximum number of concurrent scheduled task threads + * @since 5.6.0 + */ + default int concurrentScheduledTaskThreads() { + return DEFAULT_SCHEDULED_TASK_THREADS_NUMBER; + } + + /** + * Number of threads the operator can spin out to trigger its retried and rescheduled + * reconciliations with the default executor. These threads are shared by all the controllers of + * the operator, but the tasks they run only enqueue an event for the reconciliation to happen on + * a reconciliation thread, so few of them are needed. + * + * @return the maximum number of concurrent retry and reschedule threads + * @since 5.6.0 + */ + default int concurrentRetryAndRescheduleThreads() { + return DEFAULT_RETRY_AND_RESCHEDULE_THREADS_NUMBER; + } + /** * Override to provide a custom {@link Metrics} implementation * @@ -249,6 +291,44 @@ default ExecutorService getWorkflowExecutorService() { return Executors.newFixedThreadPool(concurrentWorkflowExecutorThreads()); } + /** + * Override to provide a custom {@link ScheduledExecutorService} implementation to change how the + * operator's scheduled (i.e. periodic or delayed) tasks are run. This executor is shared by all + * the polling event sources of the operator. The retried and rescheduled reconciliations run on + * an executor of their own, see {@link #getRetryAndRescheduleExecutorService()}, so that a slow + * poll can't delay them. + * + *

    Note that the default implementation lets the executor discard the tasks that were scheduled + * for later when it is shut down, so that they don't delay the termination of the operator, and + * that it creates daemon threads so that a never stopped operator doesn't keep the JVM alive. + * Custom implementations are advised to do the same. + * + * @return the {@link ScheduledExecutorService} implementation to use to run scheduled tasks + * @since 5.6.0 + */ + default ScheduledExecutorService getScheduledExecutorService() { + return Utils.daemonScheduledThreadPool( + concurrentScheduledTaskThreads(), "josdk-scheduled-task"); + } + + /** + * Override to provide a custom {@link ScheduledExecutorService} implementation to change how the + * operator's retried and rescheduled reconciliations are triggered. This executor is kept + * separate from the one the polling event sources use, see {@link + * #getScheduledExecutorService()}, so that a slow poll can't delay a retry. + * + *

    The same notes as for {@link #getScheduledExecutorService()} apply to custom + * implementations. + * + * @return the {@link ScheduledExecutorService} implementation to use to trigger retried and + * rescheduled reconciliations + * @since 5.6.0 + */ + default ScheduledExecutorService getRetryAndRescheduleExecutorService() { + return Utils.daemonScheduledThreadPool( + concurrentRetryAndRescheduleThreads(), "josdk-retry-reschedule"); + } + /** * Determines whether the associated Kubernetes client should be closed when the associated {@link * io.javaoperatorsdk.operator.Operator} is stopped. diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 2cf6540af0..9ac8668b20 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -21,6 +21,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; +import java.util.concurrent.ScheduledExecutorService; import java.util.function.Function; import org.slf4j.Logger; @@ -45,11 +46,15 @@ public class ConfigurationServiceOverrider { private Boolean checkCR; private Integer concurrentReconciliationThreads; private Integer concurrentWorkflowExecutorThreads; + private Integer concurrentScheduledTaskThreads; + private Integer concurrentRetryAndRescheduleThreads; private Cloner cloner; private Boolean closeClientOnStop; private KubernetesClient client; private ExecutorService executorService; private ExecutorService workflowExecutorService; + private ScheduledExecutorService scheduledExecutorService; + private ScheduledExecutorService retryAndRescheduleExecutorService; private LeaderElectionConfiguration leaderElectionConfiguration; private String clusterScopedEventNamespace; private EventRecorder eventRecorder; @@ -86,6 +91,34 @@ public ConfigurationServiceOverrider withConcurrentWorkflowExecutorThreads(int t return this; } + /** + * Sets the number of threads used to run the operator's scheduled (i.e. periodic or delayed) + * tasks, which are shared by all its polling event sources. + * + * @param threadNumber the maximum number of concurrent scheduled task threads + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @see ConfigurationService#concurrentScheduledTaskThreads() + * @since 5.6.0 + */ + public ConfigurationServiceOverrider withConcurrentScheduledTaskThreads(int threadNumber) { + this.concurrentScheduledTaskThreads = threadNumber; + return this; + } + + /** + * Sets the number of threads used to trigger the operator's retried and rescheduled + * reconciliations, which are shared by all its controllers. + * + * @param threadNumber the maximum number of concurrent retry and reschedule threads + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @see ConfigurationService#concurrentRetryAndRescheduleThreads() + * @since 5.6.0 + */ + public ConfigurationServiceOverrider withConcurrentRetryAndRescheduleThreads(int threadNumber) { + this.concurrentRetryAndRescheduleThreads = threadNumber; + return this; + } + @SuppressWarnings("rawtypes") public ConfigurationServiceOverrider withDependentResourceFactory( DependentResourceFactory dependentResourceFactory) { @@ -119,6 +152,37 @@ public ConfigurationServiceOverrider withWorkflowExecutorService( return this; } + /** + * Replaces the executor used to run the operator's scheduled (i.e. periodic or delayed) tasks, + * which are shared by all its polling event sources. + * + * @param scheduledExecutorService the executor to run scheduled tasks on + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @see ConfigurationService#getScheduledExecutorService() + * @since 5.6.0 + */ + public ConfigurationServiceOverrider withScheduledExecutorService( + ScheduledExecutorService scheduledExecutorService) { + this.scheduledExecutorService = scheduledExecutorService; + return this; + } + + /** + * Replaces the executor used to trigger the operator's retried and rescheduled reconciliations, + * which is shared by all its controllers. + * + * @param retryAndRescheduleExecutorService the executor to trigger retried and rescheduled + * reconciliations on + * @return this {@link ConfigurationServiceOverrider} for chained customization + * @see ConfigurationService#getRetryAndRescheduleExecutorService() + * @since 5.6.0 + */ + public ConfigurationServiceOverrider withRetryAndRescheduleExecutorService( + ScheduledExecutorService retryAndRescheduleExecutorService) { + this.retryAndRescheduleExecutorService = retryAndRescheduleExecutorService; + return this; + } + /** * Replaces the default {@link KubernetesClient} instance by the specified one. This is the * preferred mechanism to configure which client will be used to access the cluster. @@ -312,6 +376,28 @@ public int concurrentWorkflowExecutorThreads() { original.concurrentWorkflowExecutorThreads()); } + @Override + public int concurrentScheduledTaskThreads() { + return Utils.ensureValid( + overriddenValueOrDefault( + concurrentScheduledTaskThreads, + ConfigurationService::concurrentScheduledTaskThreads), + "maximum scheduled task threads", + 1, + original.concurrentScheduledTaskThreads()); + } + + @Override + public int concurrentRetryAndRescheduleThreads() { + return Utils.ensureValid( + overriddenValueOrDefault( + concurrentRetryAndRescheduleThreads, + ConfigurationService::concurrentRetryAndRescheduleThreads), + "maximum retry and reschedule threads", + 1, + original.concurrentRetryAndRescheduleThreads()); + } + @Override public Metrics getMetrics() { return overriddenValueOrDefault(metrics, ConfigurationService::getMetrics); @@ -340,6 +426,24 @@ public ExecutorService getWorkflowExecutorService() { } } + @Override + public ScheduledExecutorService getScheduledExecutorService() { + if (scheduledExecutorService != null) { + return scheduledExecutorService; + } else { + return super.getScheduledExecutorService(); + } + } + + @Override + public ScheduledExecutorService getRetryAndRescheduleExecutorService() { + if (retryAndRescheduleExecutorService != null) { + return retryAndRescheduleExecutorService; + } else { + return super.getRetryAndRescheduleExecutorService(); + } + } + @Override public Optional getLeaderElectionConfiguration() { return leaderElectionConfiguration != null diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java index cdcafcaa46..437bbfeae9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java @@ -42,6 +42,7 @@ public class ExecutorServiceManager { private ExecutorService workflowExecutor; private ExecutorService cachingExecutorService; private ScheduledExecutorService scheduledExecutorService; + private ScheduledExecutorService retryAndRescheduleExecutorService; private boolean started; private ConfigurationService configurationService; @@ -128,30 +129,54 @@ public ExecutorService cachingExecutorService() { return cachingExecutorService; } + /** + * The executor the operator runs its scheduled (i.e. periodic or delayed) tasks on, shared by its + * polling event sources. Note that it is only valid while the manager is started: it is shut down + * by {@link #stop(Duration)} and replaced by a fresh one on the next {@link + * #start(ConfigurationService)}, so callers should retrieve it when they start rather than hold + * on to it. + * + * @return the executor to run scheduled tasks on + */ public ScheduledExecutorService scheduledExecutorService() { return scheduledExecutorService; } + /** + * The executor the operator triggers its retried and rescheduled reconciliations on, kept + * separate from {@link #scheduledExecutorService()} so that a slow poll can't delay a retry. The + * same lifecycle caveat as for {@link #scheduledExecutorService()} applies. + * + * @return the executor to trigger retried and rescheduled reconciliations on + */ + public ScheduledExecutorService retryAndRescheduleExecutorService() { + return retryAndRescheduleExecutorService; + } + public synchronized void start(ConfigurationService configurationService) { if (!started) { this.configurationService = configurationService; // used to lazy init workflow executor this.cachingExecutorService = Executors.newCachedThreadPool(); - this.scheduledExecutorService = Executors.newScheduledThreadPool(0); + this.scheduledExecutorService = configurationService.getScheduledExecutorService(); + this.retryAndRescheduleExecutorService = + configurationService.getRetryAndRescheduleExecutorService(); this.executor = new InstrumentedExecutorService(configurationService.getExecutorService()); started = true; } } public synchronized void stop(Duration gracefulShutdownTimeout) { - var parallelExec = Executors.newFixedThreadPool(4); + var shutdowns = + List.of( + shutdown(executor, gracefulShutdownTimeout), + shutdown(workflowExecutor, gracefulShutdownTimeout), + shutdown(cachingExecutorService, gracefulShutdownTimeout), + shutdown(scheduledExecutorService, gracefulShutdownTimeout), + shutdown(retryAndRescheduleExecutorService, gracefulShutdownTimeout)); + var parallelExec = Executors.newFixedThreadPool(shutdowns.size()); try { log.debug("Closing executor"); - parallelExec.invokeAll( - List.of( - shutdown(executor, gracefulShutdownTimeout), - shutdown(workflowExecutor, gracefulShutdownTimeout), - shutdown(cachingExecutorService, gracefulShutdownTimeout), - shutdown(scheduledExecutorService, gracefulShutdownTimeout))); + parallelExec.invokeAll(shutdowns); } catch (InterruptedException e) { log.debug("Exception closing executor: {}", e.getLocalizedMessage()); Thread.currentThread().interrupt(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java index 4613d9645d..95e4fba6e1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java @@ -26,6 +26,11 @@ import java.util.Date; import java.util.Optional; import java.util.Properties; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicLong; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -108,6 +113,49 @@ public static int ensureValid(int value, String description, int minValue, int d return value; } + /** + * Creates a {@link ThreadFactory} producing daemon threads named after the specified prefix. + * Daemon threads don't keep the JVM alive if the {@link io.javaoperatorsdk.operator.Operator} is + * never stopped, and naming them makes the pool they belong to identifiable in thread dumps. + * + * @param namePrefix the prefix the created threads are named after + * @return a {@link ThreadFactory} creating named daemon threads + * @since 5.6.0 + */ + public static ThreadFactory daemonThreadFactory(String namePrefix) { + final var defaultThreadFactory = Executors.defaultThreadFactory(); + final var counter = new AtomicLong(); + return runnable -> { + final var thread = defaultThreadFactory.newThread(runnable); + thread.setName(namePrefix + "-" + counter.incrementAndGet()); + thread.setDaemon(true); + return thread; + }; + } + + /** + * Creates the kind of {@link ScheduledExecutorService} the operator runs its scheduled (i.e. + * periodic or delayed) tasks on: one whose threads are daemon threads named after the specified + * prefix, and which doesn't let the tasks that were scheduled for later delay its shutdown. + * + * @param corePoolSize the number of threads to keep in the pool + * @param threadNamePrefix the prefix the threads of the pool are named after + * @return a {@link ScheduledExecutorService} to run scheduled tasks on + * @since 5.6.0 + */ + public static ScheduledExecutorService daemonScheduledThreadPool( + int corePoolSize, String threadNamePrefix) { + final var executor = + new ScheduledThreadPoolExecutor(corePoolSize, daemonThreadFactory(threadNamePrefix)); + // tasks that are scheduled far out (a reconciliation rescheduled in an hour, say) would + // otherwise keep the pool from terminating until the graceful shutdown timeout expires + executor.setExecuteExistingDelayedTasksAfterShutdownPolicy(false); + // cancelled tasks are frequent (every retry that is superseded by a new event cancels one) and + // would otherwise be retained until their delay elapses + executor.setRemoveOnCancelPolicy(true); + return executor; + } + @SuppressWarnings("unused") // this is used in the Quarkus extension public static boolean isValidateCustomResourcesEnvVarSet() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/PollingDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/PollingDependentResource.java index 894e359d57..a524fc2ec1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/PollingDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/PollingDependentResource.java @@ -55,6 +55,7 @@ protected ExternalResourceCachingEventSource createEventSource( EventSourceContext

    context) { return new PollingEventSource<>( resourceType(), + context, new PollingConfiguration<>(name(), this, getPollingPeriod(), resourceIDMapper)); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java index d553d14cf9..bd5222fbbb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java @@ -55,7 +55,9 @@ public class EventSourceManager

    public EventSourceManager(Controller

    controller) { this( controller, - new EventSources<>(controller.getConfiguration().triggerReconcilerOnAllEvents())); + new EventSources<>( + controller.getConfiguration().triggerReconcilerOnAllEvents(), + () -> controller.getExecutorServiceManager().retryAndRescheduleExecutorService())); } EventSourceManager(Controller

    controller, EventSources

    eventSources) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java index b482d26dca..2e8a523de0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java @@ -23,6 +23,8 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.ScheduledExecutorService; +import java.util.function.Supplier; import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -41,8 +43,17 @@ class EventSources

    { private ControllerEventSource

    controllerEventSource; public EventSources(boolean triggerReconcilerOnAllEvents) { + this(triggerReconcilerOnAllEvents, null); + } + + public EventSources( + boolean triggerReconcilerOnAllEvents, + Supplier scheduledExecutorServiceSupplier) { retryAndRescheduleTimerEventSource = - new TimerEventSource<>("RetryAndRescheduleTimerEventSource", triggerReconcilerOnAllEvents); + new TimerEventSource<>( + "RetryAndRescheduleTimerEventSource", + triggerReconcilerOnAllEvents, + scheduledExecutorServiceSupplier); } EventSources() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java index 599647ff29..5c84a800a7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java @@ -18,12 +18,15 @@ import java.time.Duration; import java.util.Objects; import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.function.Predicate; import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.processing.ResourceIDMapper; +/** + * @param executorService the executor to run the polls on, {@code null} (the default) to run them + * on the executor the operator shares between all its scheduled tasks + */ public record PerResourcePollingConfiguration( String name, ScheduledExecutorService executorService, @@ -32,8 +35,6 @@ public record PerResourcePollingConfiguration( Predicate

    registerPredicate, Duration defaultPollingPeriod) { - public static final int DEFAULT_EXECUTOR_THREAD_NUMBER = 1; - public PerResourcePollingConfiguration( String name, ScheduledExecutorService executorService, @@ -42,10 +43,7 @@ public PerResourcePollingConfiguration( Predicate

    registerPredicate, Duration defaultPollingPeriod) { this.name = name; - this.executorService = - executorService == null - ? new ScheduledThreadPoolExecutor(DEFAULT_EXECUTOR_THREAD_NUMBER) - : executorService; + this.executorService = executorService; this.resourceIDMapper = resourceIDMapper == null ? ResourceIDMapper.resourceIdProviderMapper() : resourceIDMapper; this.resourceFetcher = Objects.requireNonNull(resourceFetcher); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java index 1ab750d8f0..6303912c5a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java @@ -21,6 +21,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -31,6 +32,8 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.OperatorException; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.Utils; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.Cache; @@ -56,11 +59,15 @@ public class PerResourcePollingEventSource private final Cache

    primaryResourceCache; private final Set fetchedForPrimaries = ConcurrentHashMap.newKeySet(); - private final ScheduledExecutorService executorService; + private final ScheduledExecutorService configuredExecutorService; + private final ConfigurationService configurationService; + private final boolean ownsExecutorService; private final ResourceFetcher resourceFetcher; private final Predicate

    registerPredicate; private final Duration period; + private volatile ScheduledExecutorService executorService; + public PerResourcePollingEventSource( Class resourceClass, EventSourceContext

    context, @@ -69,10 +76,48 @@ public PerResourcePollingEventSource( this.primaryResourceCache = context.getPrimaryCache(); this.resourceFetcher = config.resourceFetcher(); this.registerPredicate = config.registerPredicate(); - this.executorService = config.executorService(); + this.configuredExecutorService = config.executorService(); + this.configurationService = configurationServiceOf(context); + // when neither the configuration nor the operator provides one, the event source has to create + // an executor of its own, and is then the one responsible for shutting it down + this.ownsExecutorService = configuredExecutorService == null && configurationService == null; this.period = config.defaultPollingPeriod(); } + /** + * The configuration of the operator the event source belongs to, or {@code null} if it doesn't + * belong to one, which only happens when the event source is used standalone, outside an + * operator. + */ + private static ConfigurationService configurationServiceOf(EventSourceContext context) { + final var controllerConfiguration = context.getControllerConfiguration(); + return controllerConfiguration == null + ? null + : controllerConfiguration.getConfigurationService(); + } + + @Override + public void start() throws OperatorException { + executorService = resolveExecutorService(); + super.start(); + } + + /** + * Resolves the executor to poll on. Note that this happens on every start, and not once at + * creation time, since the operator shuts its executors down when it is stopped and creates new + * ones if it is started again. + */ + private ScheduledExecutorService resolveExecutorService() { + if (configuredExecutorService != null) { + return configuredExecutorService; + } + if (configurationService != null) { + return configurationService.getExecutorServiceManager().scheduledExecutorService(); + } + return Executors.newSingleThreadScheduledExecutor( + Utils.daemonThreadFactory("josdk-polling-" + name())); + } + private Set getAndCacheResource(P primary, boolean fromGetter) { var values = resourceFetcher.fetchResources(primary); var primaryID = ResourceID.fromResource(primary); @@ -202,6 +247,12 @@ default Optional fetchDelay(Set lastFetchedResource, P primary) { @Override public void stop() throws OperatorException { super.stop(); - executorService.shutdownNow(); + // the tasks have to be cancelled explicitly now that the executor can be shared with the rest + // of the operator, and the map cleared so that they are registered again on a restart + scheduledFutures.values().forEach(future -> future.cancel(true)); + scheduledFutures.clear(); + if (ownsExecutorService && executorService != null) { + executorService.shutdownNow(); + } } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfiguration.java index 9ac1b8cc96..0836058cf4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfiguration.java @@ -17,24 +17,40 @@ import java.time.Duration; import java.util.Objects; +import java.util.concurrent.ScheduledExecutorService; import io.javaoperatorsdk.operator.processing.ResourceIDMapper; +/** + * @param executorService the executor to run the polls on, {@code null} (the default) to run them + * on the executor the operator shares between all its scheduled tasks + */ public record PollingConfiguration( String name, PollingEventSource.GenericResourceFetcher genericResourceFetcher, Duration period, - ResourceIDMapper resourceIDMapper) { + ResourceIDMapper resourceIDMapper, + ScheduledExecutorService executorService) { public PollingConfiguration( String name, PollingEventSource.GenericResourceFetcher genericResourceFetcher, Duration period, ResourceIDMapper resourceIDMapper) { + this(name, genericResourceFetcher, period, resourceIDMapper, null); + } + + public PollingConfiguration( + String name, + PollingEventSource.GenericResourceFetcher genericResourceFetcher, + Duration period, + ResourceIDMapper resourceIDMapper, + ScheduledExecutorService executorService) { this.name = name; this.genericResourceFetcher = Objects.requireNonNull(genericResourceFetcher); this.period = period; this.resourceIDMapper = resourceIDMapper == null ? ResourceIDMapper.resourceIdProviderMapper() : resourceIDMapper; + this.executorService = executorService; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfigurationBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfigurationBuilder.java index 0e68876b60..5a12fc4d50 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfigurationBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfigurationBuilder.java @@ -16,6 +16,7 @@ package io.javaoperatorsdk.operator.processing.event.source.polling; import java.time.Duration; +import java.util.concurrent.ScheduledExecutorService; import io.javaoperatorsdk.operator.processing.ResourceIDMapper; @@ -24,6 +25,7 @@ public final class PollingConfigurationBuilder { private final PollingEventSource.GenericResourceFetcher genericResourceFetcher; private ResourceIDMapper resourceIDMapper; private String name; + private ScheduledExecutorService executorService; public PollingConfigurationBuilder( PollingEventSource.GenericResourceFetcher fetcher, Duration period) { @@ -42,7 +44,23 @@ public PollingConfigurationBuilder withName(String name) { return this; } + /** + * Runs the polls on the specified executor instead of the one the operator shares between all its + * scheduled tasks. Note that an explicitly provided executor is not managed by the operator: it + * is the caller's responsibility to shut it down. + * + * @param executorService the executor to run the polls on + * @return this builder for chained customization + * @since 5.6.0 + */ + public PollingConfigurationBuilder withExecutorService( + ScheduledExecutorService executorService) { + this.executorService = executorService; + return this; + } + public PollingConfiguration build() { - return new PollingConfiguration<>(name, genericResourceFetcher, period, resourceIDMapper); + return new PollingConfiguration<>( + name, genericResourceFetcher, period, resourceIDMapper, executorService); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java index 5c85c24f90..8e615a6059 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java @@ -18,8 +18,10 @@ import java.time.Duration; import java.util.Map; import java.util.Set; -import java.util.Timer; -import java.util.TimerTask; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import org.slf4j.Logger; @@ -27,6 +29,9 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.OperatorException; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.Utils; +import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.health.Status; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.ExternalResourceCachingEventSource; @@ -62,45 +67,109 @@ public class PollingEventSource private static final Logger log = LoggerFactory.getLogger(PollingEventSource.class); - private Timer timer; private final GenericResourceFetcher genericResourceFetcher; private final Duration period; private final AtomicBoolean healthy = new AtomicBoolean(true); + private final ScheduledExecutorService configuredExecutorService; + private final ConfigurationService configurationService; + private final boolean ownsExecutorService; + private volatile ScheduledExecutorService executorService; + private volatile ScheduledFuture pollingTask; + + /** + * Creates an event source polling on the executor the operator shares between all its scheduled + * tasks, unless the configuration provides one of its own. + * + * @param resourceClass the type of the polled resource + * @param context the context this event source is created for + * @param config the configuration of the polling + * @since 5.6.0 + */ + public PollingEventSource( + Class resourceClass, EventSourceContext

    context, PollingConfiguration config) { + this(resourceClass, config, configurationServiceOf(context)); + } + + /** + * @deprecated use {@link #PollingEventSource(Class, EventSourceContext, PollingConfiguration)} + * instead: without a context, and unless the configuration provides an executor, this event + * source has to create a thread of its own to poll on instead of using the one the operator + * shares between all its scheduled tasks. + */ + @Deprecated(since = "5.6.0") public PollingEventSource(Class resourceClass, PollingConfiguration config) { + this(resourceClass, config, null); + } + + private PollingEventSource( + Class resourceClass, + PollingConfiguration config, + ConfigurationService configurationService) { super(config.name(), resourceClass, config.resourceIDMapper()); this.genericResourceFetcher = config.genericResourceFetcher(); this.period = config.period(); + this.configuredExecutorService = config.executorService(); + this.configurationService = configurationService; + // when neither the configuration nor the operator provides one, the event source has to create + // an executor of its own, and is then the one responsible for shutting it down + this.ownsExecutorService = configuredExecutorService == null && configurationService == null; + } + + /** + * The configuration of the operator the event source belongs to, or {@code null} if it doesn't + * belong to one, which only happens when the event source is used standalone, outside an + * operator. + */ + private static ConfigurationService configurationServiceOf(EventSourceContext context) { + final var controllerConfiguration = context.getControllerConfiguration(); + return controllerConfiguration == null + ? null + : controllerConfiguration.getConfigurationService(); } @Override public void start() throws OperatorException { - if (timer != null) { + if (pollingTask != null) { return; } super.start(); + executorService = resolveExecutorService(); getStateAndFillCache(); - timer = new Timer(true); - timer.schedule( - new TimerTask() { - @Override - public void run() { - try { - if (!isRunning()) { - log.debug("Event source not yet started. Will not run."); - return; - } - getStateAndFillCache(); - healthy.set(true); - } catch (Exception e) { - // Exception is required because of Kotlin - healthy.set(false); - log.error("Error during polling.", e); - } - } - }, - period.toMillis(), - period.toMillis()); + pollingTask = + executorService.scheduleWithFixedDelay( + this::poll, period.toMillis(), period.toMillis(), TimeUnit.MILLISECONDS); + } + + private void poll() { + try { + if (!isRunning()) { + log.debug("Event source not yet started. Will not run."); + return; + } + getStateAndFillCache(); + healthy.set(true); + } catch (Exception e) { + // Exception is required because of Kotlin + healthy.set(false); + log.error("Error during polling.", e); + } + } + + /** + * Resolves the executor to poll on. Note that this happens on every start, and not once at + * creation time, since the operator shuts its executors down when it is stopped and creates new + * ones if it is started again. + */ + private ScheduledExecutorService resolveExecutorService() { + if (configuredExecutorService != null) { + return configuredExecutorService; + } + if (configurationService != null) { + return configurationService.getExecutorServiceManager().scheduledExecutorService(); + } + return Executors.newSingleThreadScheduledExecutor( + Utils.daemonThreadFactory("josdk-polling-" + name())); } protected synchronized void getStateAndFillCache() { @@ -115,9 +184,13 @@ public interface GenericResourceFetcher { @Override public void stop() throws OperatorException { super.stop(); - if (timer != null) { - timer.cancel(); - timer = null; + if (pollingTask != null) { + // as with the java.util.Timer this replaces, an ongoing poll is left to finish + pollingTask.cancel(false); + pollingTask = null; + } + if (ownsExecutorService && executorService != null) { + executorService.shutdownNow(); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java index eae9663fe6..b4d7aa36a3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java @@ -17,14 +17,18 @@ import java.util.Map; import java.util.Set; -import java.util.Timer; -import java.util.TimerTask; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.config.Utils; import io.javaoperatorsdk.operator.api.reconciler.BaseControl; import io.javaoperatorsdk.operator.health.Status; import io.javaoperatorsdk.operator.processing.event.Event; @@ -36,17 +40,47 @@ public class TimerEventSource extends AbstractEventSource implements ResourceEventAware { private static final Logger log = LoggerFactory.getLogger(TimerEventSource.class); - private Timer timer; - private final Map onceTasks = new ConcurrentHashMap<>(); + private final Map> onceTasks = new ConcurrentHashMap<>(); + private final Supplier executorServiceSupplier; + private final boolean ownsExecutorService; private boolean triggerReconcilerOnAllEvents; + private volatile ScheduledExecutorService executorService; public TimerEventSource() { - super(Void.class); + this((Supplier) null); } public TimerEventSource(String name, boolean triggerReconcilerOnAllEvents) { + this(name, triggerReconcilerOnAllEvents, null); + } + + /** + * Creates an event source scheduling on the executor provided by the specified supplier. The + * supplier is called on every start, and not once at creation time, since the operator shuts its + * executors down when it is stopped and creates new ones if it is started again. + * + * @param executorServiceSupplier supplies the executor to schedule on, {@code null} to have the + * event source create, and shut down, an executor of its own + * @since 5.6.0 + */ + public TimerEventSource(Supplier executorServiceSupplier) { + super(Void.class); + this.executorServiceSupplier = executorServiceSupplier; + this.ownsExecutorService = executorServiceSupplier == null; + } + + /** + * @see #TimerEventSource(Supplier) + * @since 5.6.0 + */ + public TimerEventSource( + String name, + boolean triggerReconcilerOnAllEvents, + Supplier executorServiceSupplier) { super(Void.class, name); this.triggerReconcilerOnAllEvents = triggerReconcilerOnAllEvents; + this.executorServiceSupplier = executorServiceSupplier; + this.ownsExecutorService = executorServiceSupplier == null; } @SuppressWarnings("unused") @@ -55,20 +89,25 @@ public void scheduleOnce(R resource, long delay) { } public void scheduleOnce(ResourceID resourceID, long delay) { - if (!isRunning()) { + final var executor = executorService; + if (!isRunning() || executor == null) { throw new IllegalStateException("The TimerEventSource is not running"); } - if (onceTasks.containsKey(resourceID)) { - cancelOnceSchedule(resourceID); - } - EventProducerTimeTask task = new EventProducerTimeTask(resourceID); if (delay == BaseControl.INSTANT_RESCHEDULE) { - task.run(); - } else { - onceTasks.put(resourceID, task); - timer.schedule(task, delay); + cancelOnceSchedule(resourceID); + new EventProducerTimeTask(resourceID).run(); + return; } + + onceTasks.compute( + resourceID, + (id, alreadyScheduled) -> { + if (alreadyScheduled != null) { + alreadyScheduled.cancel(false); + } + return executor.schedule(new EventProducerTimeTask(id), delay, TimeUnit.MILLISECONDS); + }); } @Override @@ -81,9 +120,11 @@ public void onResourceDeleted(R resource) { } public void cancelOnceSchedule(ResourceID customResourceUid) { - TimerTask timerTask = onceTasks.remove(customResourceUid); - if (timerTask != null) { - timerTask.cancel(); + var scheduled = onceTasks.remove(customResourceUid); + if (scheduled != null) { + // as with the java.util.TimerTask this replaces, a task that is already running is left to + // finish + scheduled.cancel(false); } } @@ -91,16 +132,26 @@ public void cancelOnceSchedule(ResourceID customResourceUid) { public void start() { if (!isRunning()) { super.start(); - timer = new Timer(true); + executorService = resolveExecutorService(); } } + private ScheduledExecutorService resolveExecutorService() { + if (executorServiceSupplier != null) { + return executorServiceSupplier.get(); + } + return Executors.newSingleThreadScheduledExecutor( + Utils.daemonThreadFactory("josdk-timer-" + name())); + } + @Override public void stop() { if (isRunning()) { onceTasks.keySet().forEach(this::cancelOnceSchedule); - timer.cancel(); super.stop(); + if (ownsExecutorService && executorService != null) { + executorService.shutdownNow(); + } } } @@ -114,7 +165,7 @@ public Set getSecondaryResources(HasMetadata primary) { return Set.of(); } - public class EventProducerTimeTask extends TimerTask { + public class EventProducerTimeTask implements Runnable { protected final ResourceID customResourceUid; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java index aec8381135..aebd7bc9b0 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java @@ -20,6 +20,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ThreadPoolExecutor; import org.junit.jupiter.api.Test; @@ -117,6 +118,64 @@ public R clone(R object) { config.reconciliationTerminationTimeout(), overridden.reconciliationTerminationTimeout()); } + @Test + void scheduledExecutorCanBeOverridden() { + final var scheduledExecutorService = Executors.newScheduledThreadPool(1); + final var retryExecutorService = Executors.newScheduledThreadPool(1); + try { + final var overridden = + new ConfigurationServiceOverrider(config) + .withConcurrentScheduledTaskThreads(7) + .withScheduledExecutorService(scheduledExecutorService) + .withConcurrentRetryAndRescheduleThreads(5) + .withRetryAndRescheduleExecutorService(retryExecutorService) + .build(); + + assertThat(overridden.concurrentScheduledTaskThreads()).isEqualTo(7); + assertThat(overridden.getScheduledExecutorService()).isSameAs(scheduledExecutorService); + assertThat(overridden.concurrentRetryAndRescheduleThreads()).isEqualTo(5); + assertThat(overridden.getRetryAndRescheduleExecutorService()).isSameAs(retryExecutorService); + } finally { + scheduledExecutorService.shutdownNow(); + retryExecutorService.shutdownNow(); + } + } + + @Test + void scheduledExecutorDefaultsToADaemonPoolOfTheConfiguredSize() { + final var overridden = + (ScheduledThreadPoolExecutor) + new ConfigurationServiceOverrider(config) + .withConcurrentScheduledTaskThreads(3) + .build() + .getScheduledExecutorService(); + + try { + assertThat(overridden.getCorePoolSize()).isEqualTo(3); + // scheduled tasks must not delay the termination of the operator, nor keep the JVM alive + assertThat(overridden.getExecuteExistingDelayedTasksAfterShutdownPolicy()).isFalse(); + assertThat(overridden.getThreadFactory().newThread(() -> {}).isDaemon()).isTrue(); + } finally { + overridden.shutdownNow(); + } + } + + @Test + void retryAndRescheduleExecutorIsSeparateFromTheScheduledTaskOne() { + final var scheduled = config.getScheduledExecutorService(); + final var retryAndReschedule = config.getRetryAndRescheduleExecutorService(); + + try { + // a slow poll on the scheduled task executor must not be able to delay a retry + assertThat(retryAndReschedule).isNotSameAs(scheduled); + assertThat(((ScheduledThreadPoolExecutor) retryAndReschedule).getCorePoolSize()) + .isEqualTo(ConfigurationService.DEFAULT_RETRY_AND_RESCHEDULE_THREADS_NUMBER); + } finally { + scheduled.shutdownNow(); + retryAndReschedule.shutdownNow(); + } + } + @Test void eventRecorderIsNotConfiguredByDefaultAndCanBeOverridden() { final var eventRecorder = diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManagerTest.java index 40ffded241..995a6e374f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManagerTest.java @@ -30,14 +30,19 @@ void stopShutsDownTheScheduledExecutorService() { ConfigurationService configurationService = new BaseConfigurationService(); var manager = configurationService.getExecutorServiceManager(); var scheduled = manager.scheduledExecutorService(); + var retryAndReschedule = manager.retryAndRescheduleExecutorService(); try { assertThat(scheduled.isShutdown()).isFalse(); + assertThat(retryAndReschedule.isShutdown()).isFalse(); + // retries are triggered on an executor of their own so that a slow poll can't delay them + assertThat(retryAndReschedule).isNotSameAs(scheduled); } finally { manager.stop(SHUTDOWN_TIMEOUT); } assertThat(scheduled.isShutdown()).isTrue(); + assertThat(retryAndReschedule.isShutdown()).isTrue(); } @Test @@ -53,6 +58,7 @@ void canBeRestartedAfterStop() { assertThat(manager.reconcileExecutorService().isShutdown()).isFalse(); assertThat(manager.cachingExecutorService().isShutdown()).isFalse(); assertThat(manager.scheduledExecutorService().isShutdown()).isFalse(); + assertThat(manager.retryAndRescheduleExecutorService().isShutdown()).isFalse(); manager.stop(SHUTDOWN_TIMEOUT); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java index 92400df4de..43b20d368f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java @@ -19,11 +19,16 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.config.BaseConfigurationService; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.health.Status; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -46,6 +51,7 @@ class PollingEventSourceTest private final PollingEventSource.GenericResourceFetcher resourceFetcher = mock(PollingEventSource.GenericResourceFetcher.class); + @SuppressWarnings("deprecation") private final PollingEventSource pollingEventSource = new PollingEventSource<>( SampleExternalResource.class, @@ -67,7 +73,7 @@ void canBeRestartedAfterStop() throws InterruptedException { Thread.sleep(DEFAULT_WAIT_PERIOD); pollingEventSource.stop(); - // a cancelled java.util.Timer cannot be reused, a new one has to be created on start + // the polling task is cancelled on stop, a new one has to be scheduled on start pollingEventSource.start(); Thread.sleep(DEFAULT_WAIT_PERIOD); @@ -75,7 +81,20 @@ void canBeRestartedAfterStop() throws InterruptedException { } @Test - void timerThreadIsADaemonSoItDoesNotKeepTheJvmAlive() throws InterruptedException { + void stopCancelsThePollingTask() throws InterruptedException { + when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues()); + pollingEventSource.start(); + Thread.sleep(DEFAULT_WAIT_PERIOD); + pollingEventSource.stop(); + clearInvocations(resourceFetcher); + + Thread.sleep(DEFAULT_WAIT_PERIOD); + + verify(resourceFetcher, never()).fetchResources(); + } + + @Test + void pollingThreadIsADaemonSoItDoesNotKeepTheJvmAlive() throws InterruptedException { when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues()); var threadsBeforeStart = Thread.getAllStackTraces().keySet(); @@ -83,13 +102,13 @@ void timerThreadIsADaemonSoItDoesNotKeepTheJvmAlive() throws InterruptedExceptio pollingEventSource.start(); Thread.sleep(DEFAULT_WAIT_PERIOD); - var newTimerThreads = + var newPollingThreads = Thread.getAllStackTraces().keySet().stream() - .filter(t -> t.getName().startsWith("Timer-")) + .filter(t -> t.getName().startsWith("josdk-polling-")) .filter(t -> !threadsBeforeStart.contains(t)) .toList(); - assertThat(newTimerThreads).isNotEmpty().allMatch(Thread::isDaemon); + assertThat(newPollingThreads).isNotEmpty().allMatch(Thread::isDaemon); } @Test @@ -154,6 +173,54 @@ void updatesHealthIndicatorBasedOnExceptionsInFetcher() { .untilAsserted(() -> assertThat(pollingEventSource.getStatus()).isEqualTo(Status.HEALTHY)); } + @Test + void pollsOnTheOperatorsSharedSchedulerWhenCreatedWithAContext() { + var pollingThreadNames = new CopyOnWriteArrayList(); + when(resourceFetcher.fetchResources()) + .thenAnswer( + invocation -> { + pollingThreadNames.add(Thread.currentThread().getName()); + return testResponseWithOneValue(); + }); + + var configurationService = new BaseConfigurationService(); + var executorServiceManager = configurationService.getExecutorServiceManager(); + var eventSource = + new PollingEventSource( + SampleExternalResource.class, + contextFor(configurationService), + new PollingConfiguration<>(null, resourceFetcher, POLL_PERIOD, null)); + eventSource.setEventHandler(mock(EventHandler.class)); + + try { + eventSource.start(); + + // the initial fetch happens on the calling thread, the scheduled ones on the shared pool + await() + .untilAsserted( + () -> + assertThat(pollingThreadNames) + .anyMatch(name -> name.startsWith("josdk-scheduled-task-"))); + + eventSource.stop(); + + // the shared executor belongs to the operator, stopping an event source must not shut it down + assertThat(executorServiceManager.scheduledExecutorService().isShutdown()).isFalse(); + } finally { + executorServiceManager.stop(Duration.ofMillis(100)); + } + } + + @SuppressWarnings("unchecked") + private static EventSourceContext contextFor( + ConfigurationService configurationService) { + var controllerConfiguration = mock(ControllerConfiguration.class); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + var context = mock(EventSourceContext.class); + when(context.getControllerConfiguration()).thenReturn(controllerConfiguration); + return context; + } + private Map> testResponseWithTwoValueForSameId() { Map> res = new HashMap<>(); res.put(primaryID1(), Set.of(testResource1(), testResource2())); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSourceTest.java index 3a4e1cb80d..e8d36c0034 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSourceTest.java @@ -17,6 +17,7 @@ import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import org.awaitility.Awaitility; @@ -26,6 +27,7 @@ import org.junit.jupiter.api.Test; import io.javaoperatorsdk.operator.TestUtils; +import io.javaoperatorsdk.operator.api.config.Utils; import io.javaoperatorsdk.operator.api.reconciler.BaseControl; import io.javaoperatorsdk.operator.health.Status; import io.javaoperatorsdk.operator.processing.event.Event; @@ -125,6 +127,59 @@ public void handlesInstanceReschedule() { assertThat(eventHandler.events).hasSize(1); } + @Test + public void schedulesOnTheProvidedExecutorAndLeavesItRunningOnStop() { + var providedExecutor = + Executors.newSingleThreadScheduledExecutor( + Utils.daemonThreadFactory("provided-timer-executor")); + var handler = new CapturingEventHandler(); + var eventSource = new TimerEventSource(() -> providedExecutor); + eventSource.setEventHandler(handler); + + try { + eventSource.start(); + eventSource.scheduleOnce(ResourceID.fromResource(TestUtils.testCustomResource()), PERIOD); + + untilAsserted( + () -> + assertThat(handler.eventProducingThreadNames) + .containsExactly("provided-timer-executor-1")); + + eventSource.stop(); + + // the executor is not the event source's to shut down + assertThat(providedExecutor.isShutdown()).isFalse(); + } finally { + providedExecutor.shutdownNow(); + } + } + + @Test + public void shutsDownTheExecutorItCreatedItselfAndCreatesANewOneOnRestart() { + var eventSource = new TimerEventSource(); + var handler = new CapturingEventHandler(); + eventSource.setEventHandler(handler); + + eventSource.start(); + eventSource.scheduleOnce(ResourceID.fromResource(TestUtils.testCustomResource()), PERIOD); + untilAsserted(() -> assertThat(handler.events).hasSize(1)); + var firstThreadName = handler.eventProducingThreadNames.get(0); + assertThat(firstThreadName).startsWith("josdk-timer-"); + + eventSource.stop(); + Awaitility.await() + .untilAsserted( + () -> + assertThat(Thread.getAllStackTraces().keySet()) + .noneMatch(t -> t.getName().equals(firstThreadName))); + + eventSource.start(); + eventSource.scheduleOnce(ResourceID.fromResource(TestUtils.testCustomResource()), PERIOD); + untilAsserted(() -> assertThat(handler.events).hasSize(2)); + + eventSource.stop(); + } + private void untilAsserted(ThrowingRunnable assertion) { untilAsserted(INITIAL_DELAY, PERIOD, assertion); } @@ -150,10 +205,12 @@ private void untilAsserted(long initialDelay, long interval, ThrowingRunnable as public static class CapturingEventHandler implements EventHandler { private final List events = new CopyOnWriteArrayList<>(); + private final List eventProducingThreadNames = new CopyOnWriteArrayList<>(); @Override public void handleEvent(Event event) { events.add(event); + eventProducingThreadNames.add(Thread.currentThread().getName()); } } } diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java index bc9a4ecbbb..a8d7162f84 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java @@ -78,6 +78,14 @@ public static ConfigLoader getDefault() { "workflow.executor-threads", Integer.class, ConfigurationServiceOverrider::withConcurrentWorkflowExecutorThreads), + new ConfigBinding<>( + "scheduled-tasks.concurrent-threads", + Integer.class, + ConfigurationServiceOverrider::withConcurrentScheduledTaskThreads), + new ConfigBinding<>( + "retry-and-reschedule.concurrent-threads", + Integer.class, + ConfigurationServiceOverrider::withConcurrentRetryAndRescheduleThreads), new ConfigBinding<>( "close-client-on-stop", Boolean.class, diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java index 3ac9cea0dc..fb92591cf4 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java @@ -85,6 +85,30 @@ void applyConfigsAppliesConcurrentWorkflowExecutorThreads() { assertThat(result.concurrentWorkflowExecutorThreads()).isEqualTo(3); } + @Test + void applyConfigsAppliesConcurrentScheduledTaskThreads() { + var loader = + new ConfigLoader(mapProvider(Map.of("josdk.scheduled-tasks.concurrent-threads", 8))); + + var base = new BaseConfigurationService(null); + var result = + ConfigurationService.newOverriddenConfigurationService(base, loader.applyConfigs()); + + assertThat(result.concurrentScheduledTaskThreads()).isEqualTo(8); + } + + @Test + void applyConfigsAppliesConcurrentRetryAndRescheduleThreads() { + var loader = + new ConfigLoader(mapProvider(Map.of("josdk.retry-and-reschedule.concurrent-threads", 6))); + + var base = new BaseConfigurationService(null); + var result = + ConfigurationService.newOverriddenConfigurationService(base, loader.applyConfigs()); + + assertThat(result.concurrentRetryAndRescheduleThreads()).isEqualTo(6); + } + @Test void applyConfigsAppliesBooleanFlags() { var values = new HashMap(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/multiplemanagedexternaldependenttype/MultipleManagedExternalDependentResourceReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/multiplemanagedexternaldependenttype/MultipleManagedExternalDependentResourceReconciler.java index dec9422486..33b7f965f3 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/multiplemanagedexternaldependenttype/MultipleManagedExternalDependentResourceReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/multiplemanagedexternaldependenttype/MultipleManagedExternalDependentResourceReconciler.java @@ -96,6 +96,7 @@ public int getNumberOfExecutions() { pollingEventSource = new PollingEventSource<>( ExternalResource.class, + context, new PollingConfigurationBuilder( fetcher, Duration.ofMillis(1000L)) .withName(EVENT_SOURCE_NAME) From 7b4f3f2af8690db04f7151fa520cdaba5cf1aa16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Fri, 25 Sep 2026 14:28:03 +0200 Subject: [PATCH 08/11] fix: keep DEFAULT_EXECUTOR_THREAD_NUMBER and withdraw polls scheduled after stop --- .../source/polling/PerResourcePollingConfiguration.java | 7 +++++++ .../source/polling/PerResourcePollingEventSource.java | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java index 5c84a800a7..49bc8dd7ba 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java @@ -35,6 +35,13 @@ public record PerResourcePollingConfiguration( Predicate

    registerPredicate, Duration defaultPollingPeriod) { + /** + * @deprecated not used anymore, polls now run on the executor the operator shares between all its + * scheduled tasks unless an executor is explicitly configured + */ + @Deprecated(forRemoval = true) + public static final int DEFAULT_EXECUTOR_THREAD_NUMBER = 1; + public PerResourcePollingConfiguration( String name, ScheduledExecutorService executorService, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java index 6303912c5a..0e02482d9e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java @@ -137,6 +137,12 @@ private void scheduleNextExecution(P primary, Set actualResources) { executorService.schedule( new FetchingExecutor(primaryID), fetchDuration.toMillis(), TimeUnit.MILLISECONDS); scheduledFutures.put(primaryID, scheduledFuture); + // a fetch that was already running when the event source got stopped can get here after stop + // cleared the map; since the shared executor stays alive, the task has to be withdrawn, + // otherwise the stale entry would prevent the resource from being registered on a restart + if (!isRunning() && scheduledFutures.remove(primaryID, scheduledFuture)) { + scheduledFuture.cancel(true); + } } @Override From b1f0d910d0dbf2c515e6ccb62c588d04e5b772cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Sun, 25 Oct 2026 18:59:01 +0100 Subject: [PATCH 09/11] test: cover restarting polling event sources, including after the operator replaces its executors --- .../PerResourcePollingEventSourceTest.java | 54 +++++++++++++++++++ .../polling/PollingEventSourceTest.java | 35 +++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSourceTest.java index 907ed19fe5..2f406f0489 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSourceTest.java @@ -24,6 +24,8 @@ import org.junit.jupiter.api.Test; import io.javaoperatorsdk.operator.TestUtils; +import io.javaoperatorsdk.operator.api.config.BaseConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.processing.ResourceIDMapper; import io.javaoperatorsdk.operator.processing.event.EventHandler; @@ -215,6 +217,58 @@ void supportsDynamicPollingDelay() { .untilAsserted(() -> verify(supplier, times(2)).fetchResources(any())); } + @Test + void pollsAgainAfterRestart() { + source.onResourceCreated(testCustomResource); + await().untilAsserted(() -> verify(supplier, atLeast(2)).fetchResources(any())); + source.stop(); + + source.start(); + clearInvocations(supplier); + source.onResourceCreated(testCustomResource); + + await().untilAsserted(() -> verify(supplier, atLeast(2)).fetchResources(any())); + } + + @Test + void pollsOnTheNewSharedSchedulerWhenTheOperatorIsRestarted() { + var configurationService = new BaseConfigurationService(); + var executorServiceManager = configurationService.getExecutorServiceManager(); + var controllerConfiguration = mock(ControllerConfiguration.class); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + when(context.getControllerConfiguration()).thenReturn(controllerConfiguration); + setUpSource( + new PerResourcePollingEventSource<>( + SampleExternalResource.class, + context, + new PerResourcePollingConfigurationBuilder< + SampleExternalResource, TestCustomResource, String>( + supplier, Duration.ofMillis(PERIOD)) + .withResourceIDMapper(r -> r.getName() + "#" + r.getValue()) + .build())); + + try { + source.onResourceCreated(testCustomResource); + await().untilAsserted(() -> verify(supplier, atLeast(2)).fetchResources(any())); + var firstExecutor = executorServiceManager.scheduledExecutorService(); + source.stop(); + + executorServiceManager.stop(Duration.ofMillis(100)); + executorServiceManager.start(configurationService); + assertThat(firstExecutor.isShutdown()).isTrue(); + assertThat(executorServiceManager.scheduledExecutorService()).isNotSameAs(firstExecutor); + + source.start(); + clearInvocations(supplier); + source.onResourceCreated(testCustomResource); + + await().untilAsserted(() -> verify(supplier, atLeast(2)).fetchResources(any())); + } finally { + source.stop(); + executorServiceManager.stop(Duration.ofMillis(100)); + } + } + @Test void deleteEventCancelsTheScheduling() { when(supplier.fetchResources(any())).thenReturn(Set.of(SampleExternalResource.testResource1())); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java index 43b20d368f..c9be8caf58 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java @@ -75,11 +75,44 @@ void canBeRestartedAfterStop() throws InterruptedException { // the polling task is cancelled on stop, a new one has to be scheduled on start pollingEventSource.start(); - Thread.sleep(DEFAULT_WAIT_PERIOD); + clearInvocations(resourceFetcher); + await().untilAsserted(() -> verify(resourceFetcher, atLeast(2)).fetchResources()); assertThat(pollingEventSource.getStatus()).isEqualTo(Status.HEALTHY); } + @Test + void pollsOnTheNewSharedSchedulerWhenTheOperatorIsRestarted() { + when(resourceFetcher.fetchResources()).thenReturn(testResponseWithOneValue()); + var configurationService = new BaseConfigurationService(); + var executorServiceManager = configurationService.getExecutorServiceManager(); + var eventSource = + new PollingEventSource( + SampleExternalResource.class, + contextFor(configurationService), + new PollingConfiguration<>(null, resourceFetcher, POLL_PERIOD, null)); + eventSource.setEventHandler(mock(EventHandler.class)); + + try { + eventSource.start(); + var firstExecutor = executorServiceManager.scheduledExecutorService(); + eventSource.stop(); + + executorServiceManager.stop(Duration.ofMillis(100)); + executorServiceManager.start(configurationService); + assertThat(firstExecutor.isShutdown()).isTrue(); + assertThat(executorServiceManager.scheduledExecutorService()).isNotSameAs(firstExecutor); + + eventSource.start(); + clearInvocations(resourceFetcher); + + await().untilAsserted(() -> verify(resourceFetcher, atLeast(2)).fetchResources()); + } finally { + eventSource.stop(); + executorServiceManager.stop(Duration.ofMillis(100)); + } + } + @Test void stopCancelsThePollingTask() throws InterruptedException { when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues()); From 849d2ae45aac481a7677a3bb1ed79d968fdcdc46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Sun, 25 Oct 2026 19:31:36 +0100 Subject: [PATCH 10/11] fix: close timer scheduling race on stop, guard repeated start of per resource polling, clarify executor ownership in docs --- .../content/en/docs/documentation/eventing.md | 9 +++-- .../PerResourcePollingEventSource.java | 3 ++ .../event/source/timer/TimerEventSource.java | 36 ++++++++++++++----- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/docs/content/en/docs/documentation/eventing.md b/docs/content/en/docs/documentation/eventing.md index e954f8a31f..aec261bbf0 100644 --- a/docs/content/en/docs/documentation/eventing.md +++ b/docs/content/en/docs/documentation/eventing.md @@ -240,9 +240,12 @@ default); few threads are needed there since triggering a reconciliation only en for one of the reconciliation threads to pick up. Use `ConfigurationServiceOverrider.withScheduledExecutorService` to replace the polling executor -altogether, or the `withExecutorService` method of the event source's own configuration builder to -poll a single event source on an executor of its own. An executor provided that way is not managed -by the operator: it is your responsibility to shut it down. +altogether. The operator still manages the lifecycle of that executor: it shuts it down when the +operator is stopped, so don't shut it down yourself. + +Use the `withExecutorService` method of the event source's own configuration builder to poll a +single event source on an executor of its own. An executor provided that way is not managed by the +operator: it is your responsibility to shut it down. #### Inbound event sources diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java index 0e02482d9e..7b9f1b5fba 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java @@ -98,6 +98,9 @@ private static ConfigurationService configurationServiceOf(EventSourceContext @Override public void start() throws OperatorException { + if (isRunning()) { + return; + } executorService = resolveExecutorService(); super.start(); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java index b4d7aa36a3..f9207c6e1b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java @@ -19,9 +19,11 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import org.slf4j.Logger; @@ -100,14 +102,30 @@ public void scheduleOnce(ResourceID resourceID, long delay) { return; } - onceTasks.compute( - resourceID, - (id, alreadyScheduled) -> { - if (alreadyScheduled != null) { - alreadyScheduled.cancel(false); - } - return executor.schedule(new EventProducerTimeTask(id), delay, TimeUnit.MILLISECONDS); - }); + var scheduled = new AtomicReference>(); + try { + onceTasks.compute( + resourceID, + (id, alreadyScheduled) -> { + if (alreadyScheduled != null) { + alreadyScheduled.cancel(false); + } + var future = + executor.schedule(new EventProducerTimeTask(id), delay, TimeUnit.MILLISECONDS); + scheduled.set(future); + return future; + }); + } catch (RejectedExecutionException e) { + if (isRunning()) { + throw e; + } + throw new IllegalStateException("The TimerEventSource is not running", e); + } + // stop() clears the running flag before cancelling the tasks, so a task that got registered + // concurrently with it is either cancelled by stop, or withdrawn here + if (!isRunning() && onceTasks.remove(resourceID, scheduled.get())) { + scheduled.get().cancel(false); + } } @Override @@ -147,8 +165,8 @@ private ScheduledExecutorService resolveExecutorService() { @Override public void stop() { if (isRunning()) { - onceTasks.keySet().forEach(this::cancelOnceSchedule); super.stop(); + onceTasks.keySet().forEach(this::cancelOnceSchedule); if (ownsExecutorService && executorService != null) { executorService.shutdownNow(); } From a2539502fd80c6bca8d67663c5e0b66859949b20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Sun, 25 Oct 2026 19:43:55 +0100 Subject: [PATCH 11/11] fix: keep EventProducerTimeTask a TimerTask and cancel a poll scheduled after stop during start --- .../event/source/polling/PollingEventSource.java | 9 ++++++++- .../processing/event/source/timer/TimerEventSource.java | 3 ++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java index 8e615a6059..d25caee74c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java @@ -136,9 +136,16 @@ public void start() throws OperatorException { super.start(); executorService = resolveExecutorService(); getStateAndFillCache(); - pollingTask = + var task = executorService.scheduleWithFixedDelay( this::poll, period.toMillis(), period.toMillis(), TimeUnit.MILLISECONDS); + pollingTask = task; + // stop() clears the running flag before it looks for the task, so a stop that happened during + // the initial fetch is either seen by stop (which cancels the task) or caught here + if (!isRunning()) { + task.cancel(false); + pollingTask = null; + } } private void poll() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java index f9207c6e1b..e8a0e351d0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java @@ -17,6 +17,7 @@ import java.util.Map; import java.util.Set; +import java.util.TimerTask; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; import java.util.concurrent.RejectedExecutionException; @@ -183,7 +184,7 @@ public Set getSecondaryResources(HasMetadata primary) { return Set.of(); } - public class EventProducerTimeTask implements Runnable { + public class EventProducerTimeTask extends TimerTask { protected final ResourceID customResourceUid;