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 6215c20179..6ed9b7ff64 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 @@ -28,8 +28,6 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Secret; -import io.fabric8.kubernetes.api.model.apps.Deployment; -import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.ConfigBuilder; import io.fabric8.kubernetes.client.CustomResource; @@ -447,64 +445,6 @@ default Set> defaultNonSSAResource() { return defaultNonSSAResources(); } - /** - * If a javaoperatorsdk.io/previous annotation should be used so that the operator sdk can detect - * events from its own updates of dependent resources and then filter them. - * - *

Disable this if you want to react to your own dependent resource updates - * - * @return if special annotation should be used for dependent resource to filter events - * @since 4.5.0 - */ - default boolean previousAnnotationForDependentResourcesEventFiltering() { - return true; - } - - /** - * For dependent resources, the framework can add an annotation to filter out events resulting - * directly from the framework's operation. There are, however, some resources that do not follow - * the Kubernetes API conventions that changes in metadata should not increase the generation of - * the resource (as recorded in the {@code generation} field of the resource's {@code metadata}). - * For these resources, this convention is not respected and results in a new event for the - * framework to process. If that particular case is not handled correctly in the resource matcher, - * the framework will consider that the resource doesn't match the desired state and therefore - * triggers an update, which in turn, will re-add the annotation, thus starting the loop again, - * infinitely. - * - *

As a workaround, we automatically skip adding previous annotation for those well-known - * resources. Note that if you are sure that the matcher works for your use case, and it should in - * most instances, you can remove the resource type from the blocklist. - * - *

The consequence of adding a resource type to the set is that the framework will not use - * event filtering to prevent events, initiated by changes made by the framework itself as a - * result of its processing of dependent resources, to trigger the associated reconciler again. - * - *

Note that this method only takes effect if annotating dependent resources to prevent - * dependent resources events from triggering the associated reconciler again is activated as - * controlled by {@link #previousAnnotationForDependentResourcesEventFiltering()} - * - * @return a Set of resource classes where the previous version annotation won't be used. - */ - default Set> withPreviousAnnotationForDependentResourcesBlocklist() { - return Set.of(Deployment.class, StatefulSet.class); - } - - /** - * If the event logic should parse the resourceVersion to determine the ordering of dependent - * resource events. This is typically not needed. - * - *

Disabled by default as Kubernetes does not support, and discourages, this interpretation of - * resourceVersions. Enable only if your api server event processing seems to lag the operator - * logic, and you want to further minimize the amount of work done / updates issued by the - * operator. - * - * @return if resource version should be parsed (as integer) - * @since 4.5.0 - */ - default boolean parseResourceVersionsForEventFilteringAndCaching() { - return false; - } - /** * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can * either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for 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 3d29bb6589..cd9cdafb39 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 @@ -51,11 +51,8 @@ public class ConfigurationServiceOverrider { private Duration reconciliationTerminationTimeout; private Boolean ssaBasedCreateUpdateMatchForDependentResources; private Set> defaultNonSSAResource; - private Boolean previousAnnotationForDependentResources; - private Boolean parseResourceVersions; private Boolean useSSAToPatchPrimaryResource; private Boolean cloneSecondaryResourcesWhenGettingFromCache; - private Set> previousAnnotationUsageBlocklist; @SuppressWarnings("rawtypes") private DependentResourceFactory dependentResourceFactory; @@ -168,31 +165,6 @@ public ConfigurationServiceOverrider withDefaultNonSSAResource( return this; } - public ConfigurationServiceOverrider withPreviousAnnotationForDependentResources(boolean value) { - this.previousAnnotationForDependentResources = value; - return this; - } - - /** - * @param value true if internal algorithms can use metadata.resourceVersion as a numeric value. - * @return this - */ - public ConfigurationServiceOverrider withParseResourceVersions(boolean value) { - this.parseResourceVersions = value; - return this; - } - - /** - * @deprecated use withParseResourceVersions - * @param value true if internal algorithms can use metadata.resourceVersion as a numeric value. - * @return this - */ - @Deprecated(forRemoval = true) - public ConfigurationServiceOverrider wihtParseResourceVersions(boolean value) { - this.parseResourceVersions = value; - return this; - } - public ConfigurationServiceOverrider withUseSSAToPatchPrimaryResource(boolean value) { this.useSSAToPatchPrimaryResource = value; return this; @@ -204,12 +176,6 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC return this; } - public ConfigurationServiceOverrider withPreviousAnnotationForDependentResourcesBlocklist( - Set> blocklist) { - this.previousAnnotationUsageBlocklist = blocklist; - return this; - } - public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, client) { @Override @@ -331,20 +297,6 @@ public Set> defaultNonSSAResources() { defaultNonSSAResource, ConfigurationService::defaultNonSSAResources); } - @Override - public boolean previousAnnotationForDependentResourcesEventFiltering() { - return overriddenValueOrDefault( - previousAnnotationForDependentResources, - ConfigurationService::previousAnnotationForDependentResourcesEventFiltering); - } - - @Override - public boolean parseResourceVersionsForEventFilteringAndCaching() { - return overriddenValueOrDefault( - parseResourceVersions, - ConfigurationService::parseResourceVersionsForEventFilteringAndCaching); - } - @Override public boolean useSSAToPatchPrimaryResource() { return overriddenValueOrDefault( @@ -357,14 +309,6 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { cloneSecondaryResourcesWhenGettingFromCache, ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache); } - - @Override - public Set> - withPreviousAnnotationForDependentResourcesBlocklist() { - return overriddenValueOrDefault( - previousAnnotationUsageBlocklist, - ConfigurationService::withPreviousAnnotationForDependentResourcesBlocklist); - } }; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index efe25b9a43..d8890376bd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -55,7 +55,6 @@ public abstract class KubernetesDependentResource kubernetesDependentResourceConfig; private volatile Boolean useSSA; - private volatile Boolean usePreviousAnnotationForEventFiltering; public KubernetesDependentResource() {} @@ -158,14 +157,6 @@ protected void addMetadata( } else { annotations.remove(InformerEventSource.PREVIOUS_ANNOTATION_KEY); } - } else if (usePreviousAnnotation(context)) { // set a new one - eventSource() - .orElseThrow() - .addPreviousAnnotation( - Optional.ofNullable(actualResource) - .map(r -> r.getMetadata().getResourceVersion()) - .orElse(null), - target); } addReferenceHandlingMetadata(target, primary); } @@ -181,22 +172,6 @@ protected boolean useSSA(Context

context) { return useSSA; } - private boolean usePreviousAnnotation(Context

context) { - if (usePreviousAnnotationForEventFiltering == null) { - usePreviousAnnotationForEventFiltering = - context - .getControllerConfiguration() - .getConfigurationService() - .previousAnnotationForDependentResourcesEventFiltering() - && !context - .getControllerConfiguration() - .getConfigurationService() - .withPreviousAnnotationForDependentResourcesBlocklist() - .contains(this.resourceType()); - } - return usePreviousAnnotationForEventFiltering; - } - @Override protected void handleDelete(P primary, R secondary, Context

context) { if (secondary != null) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java index b7a6406e20..05d9a9018a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java @@ -47,7 +47,7 @@ public class ControllerEventSource @SuppressWarnings({"unchecked", "rawtypes"}) public ControllerEventSource(Controller controller) { - super(NAME, controller.getCRClient(), controller.getConfiguration(), false); + super(NAME, controller.getCRClient(), controller.getConfiguration()); this.controller = controller; final var config = controller.getConfiguration(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index ec11db25f4..f8205761cb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -82,32 +82,19 @@ public class InformerEventSource public InformerEventSource( InformerEventSourceConfiguration configuration, EventSourceContext

context) { - this( - configuration, - configuration.getKubernetesClient().orElse(context.getClient()), - context - .getControllerConfiguration() - .getConfigurationService() - .parseResourceVersionsForEventFilteringAndCaching()); - } - - InformerEventSource(InformerEventSourceConfiguration configuration, KubernetesClient client) { - this(configuration, client, false); + this(configuration, configuration.getKubernetesClient().orElse(context.getClient())); } + // visible for testing @SuppressWarnings({"unchecked", "rawtypes"}) - private InformerEventSource( - InformerEventSourceConfiguration configuration, - KubernetesClient client, - boolean parseResourceVersions) { + InformerEventSource(InformerEventSourceConfiguration configuration, KubernetesClient client) { super( configuration.name(), configuration .getGroupVersionKind() .map(gvk -> client.genericKubernetesResources(gvk.apiVersion(), gvk.getKind())) .orElseGet(() -> (MixedOperation) client.resources(configuration.getResourceClass())), - configuration, - parseResourceVersions); + configuration); // If there is a primary to secondary mapper there is no need for primary to secondary index. primaryToSecondaryMapper = configuration.getPrimaryToSecondaryMapper(); if (useSecondaryToPrimaryIndex()) { @@ -209,7 +196,7 @@ private synchronized void onAddOrUpdate( private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) { var res = temporaryResourceCache.getResourceFromCache(resourceID); if (res.isEmpty()) { - return isEventKnownFromAnnotation(newObject, oldObject); + return false; } boolean resVersionsEqual = newObject @@ -224,24 +211,6 @@ private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) { || temporaryResourceCache.isLaterResourceVersion(resourceID, res.get(), newObject); } - private boolean isEventKnownFromAnnotation(R newObject, R oldObject) { - String previous = newObject.getMetadata().getAnnotations().get(PREVIOUS_ANNOTATION_KEY); - boolean known = false; - if (previous != null) { - String[] parts = previous.split(","); - if (id.equals(parts[0])) { - if (oldObject == null && parts.length == 1) { - known = true; - } else if (oldObject != null - && parts.length == 2 - && oldObject.getMetadata().getResourceVersion().equals(parts[1])) { - known = true; - } - } - } - return known; - } - private void propagateEvent(R object) { var primaryResourceIdSet = configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object); @@ -333,22 +302,6 @@ private boolean acceptedByDeleteFilters(R resource, boolean b) { && (genericFilter == null || genericFilter.accept(resource)); } - /** - * Add an annotation to the resource so that the subsequent will be omitted - * - * @param resourceVersion null if there is no prior version - * @param target mutable resource that will be returned - */ - public R addPreviousAnnotation(String resourceVersion, R target) { - target - .getMetadata() - .getAnnotations() - .put( - PREVIOUS_ANNOTATION_KEY, - id + Optional.ofNullable(resourceVersion).map(rv -> "," + rv).orElse("")); - return target; - } - private enum Operation { ADD, UPDATE diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 2679918b60..f8acc07ab3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -55,17 +55,14 @@ public abstract class ManagedInformerEventSource< private static final Logger log = LoggerFactory.getLogger(ManagedInformerEventSource.class); private InformerManager cache; - private final boolean parseResourceVersions; private ControllerConfiguration controllerConfiguration; private final C configuration; private final Map>> indexers = new HashMap<>(); protected TemporaryResourceCache temporaryResourceCache; protected MixedOperation client; - protected ManagedInformerEventSource( - String name, MixedOperation client, C configuration, boolean parseResourceVersions) { + protected ManagedInformerEventSource(String name, MixedOperation client, C configuration) { super(configuration.getResourceClass(), name); - this.parseResourceVersions = parseResourceVersions; this.client = client; this.configuration = configuration; } @@ -102,7 +99,7 @@ public synchronized void start() { if (isRunning()) { return; } - temporaryResourceCache = new TemporaryResourceCache<>(this, parseResourceVersions); + temporaryResourceCache = new TemporaryResourceCache<>(this); this.cache = new InformerManager<>(client, configuration, this); cache.setControllerConfiguration(controllerConfiguration); cache.addIndexers(indexers); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java index 06226ae4ba..9cd8dd1a46 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java @@ -24,7 +24,6 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -94,13 +93,9 @@ void clean() { // keep up to the last million deletions for up to 10 minutes private final ExpirationCache tombstones = new ExpirationCache<>(1000000, 1200000); private final ManagedInformerEventSource managedInformerEventSource; - private final boolean parseResourceVersions; - public TemporaryResourceCache( - ManagedInformerEventSource managedInformerEventSource, - boolean parseResourceVersions) { + public TemporaryResourceCache(ManagedInformerEventSource managedInformerEventSource) { this.managedInformerEventSource = managedInformerEventSource; - this.parseResourceVersions = parseResourceVersions; } public synchronized void onDeleteEvent(T resource, boolean unknownState) { @@ -164,20 +159,12 @@ public synchronized void putResource(T newResource, String previousResourceVersi } } - /** - * @return true if {@link ConfigurationService#parseResourceVersionsForEventFilteringAndCaching()} - * is enabled and the resourceVersion of newResource is numerically greater than - * cachedResource, otherwise false - */ public boolean isLaterResourceVersion(ResourceID resourceId, T newResource, T cachedResource) { try { - if (parseResourceVersions - && Long.parseLong(newResource.getMetadata().getResourceVersion()) - > Long.parseLong(cachedResource.getMetadata().getResourceVersion())) { - return true; - } + return Long.parseLong(newResource.getMetadata().getResourceVersion()) + > Long.parseLong(cachedResource.getMetadata().getResourceVersion()); } catch (NumberFormatException e) { - log.debug( + log.warn( "Could not compare resourceVersions {} and {} for {}", newResource.getMetadata().getResourceVersion(), cachedResource.getMetadata().getResourceVersion(), diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 208d6aeaaa..6c7717a3b1 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -104,21 +104,6 @@ void skipsEventPropagationIfResourceWithSameVersionInResourceCache() { verify(eventHandlerMock, never()).handleEvent(any()); } - @Test - void skipsAddEventPropagationViaAnnotation() { - informerEventSource.onAdd(informerEventSource.addPreviousAnnotation(null, testDeployment())); - - verify(eventHandlerMock, never()).handleEvent(any()); - } - - @Test - void skipsUpdateEventPropagationViaAnnotation() { - informerEventSource.onUpdate( - testDeployment(), informerEventSource.addPreviousAnnotation("1", testDeployment())); - - verify(eventHandlerMock, never()).handleEvent(any()); - } - @Test void processEventPropagationWithoutAnnotation() { informerEventSource.onUpdate(testDeployment(), testDeployment()); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java index e3dc2c82e4..376b632348 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java @@ -46,7 +46,7 @@ class TemporaryPrimaryResourceCacheTest { @BeforeEach void setup() { informerEventSource = mock(InformerEventSource.class); - temporaryResourceCache = new TemporaryResourceCache<>(informerEventSource, false); + temporaryResourceCache = new TemporaryResourceCache<>(informerEventSource); } @Test @@ -109,7 +109,7 @@ void removesResourceFromCache() { @Test void resourceVersionParsing() { - this.temporaryResourceCache = new TemporaryResourceCache<>(informerEventSource, true); + this.temporaryResourceCache = new TemporaryResourceCache<>(informerEventSource); ConfigMap testResource = propagateTestResourceToCache(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/createupdateeventfilter/PreviousAnnotationDisabledIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/createupdateeventfilter/PreviousAnnotationDisabledIT.java deleted file mode 100644 index f80c75b84b..0000000000 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/createupdateeventfilter/PreviousAnnotationDisabledIT.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * 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.createupdateeventfilter; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; - -import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; - -class PreviousAnnotationDisabledIT { - - @RegisterExtension - LocallyRunOperatorExtension operator = - LocallyRunOperatorExtension.builder() - .withReconciler(new CreateUpdateEventFilterTestReconciler()) - .withConfigurationService( - overrider -> overrider.withPreviousAnnotationForDependentResources(false)) - .build(); - - @Test - void updateEventReceivedAfterCreateOrUpdate() { - CreateUpdateEventFilterTestCustomResource resource = - CreateUpdateInformerEventSourceEventFilterIT.prepareTestResource(); - var createdResource = operator.create(resource); - - CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, createdResource, 1, 2); - - CreateUpdateEventFilterTestCustomResource actualCreatedResource = - operator.get( - CreateUpdateEventFilterTestCustomResource.class, resource.getMetadata().getName()); - actualCreatedResource.getSpec().setValue("2"); - operator.replace(actualCreatedResource); - - CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, actualCreatedResource, 2, 4); - } -}