mostly removing the parse resource version term

and more of the previous annotation handling

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

Author: shawkins

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;
@@ -450,45 +448,15 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
   }
 
   /**
-   * 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.
-   *
-   * <p>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.
-   *
-   * <p>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.
-   *
-   * <p>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<Class<? extends HasMetadata>> withPreviousAnnotationForDependentResourcesBlocklist() {
-    return Set.of(Deployment.class, StatefulSet.class);
-  }
-
-  /**
-   * If the event logic should parse the resourceVersion to determine the ordering of dependent
-   * resource events.
+   * If the event logic can compare resourceVersions.
    *
    * <p>Enabled by default as Kubernetes does support this interpretation of resourceVersions.
-   * Disable only if your api server provides non comparable resource versions..
+   * Disable only if your api server provides non comparable resource versions.
    *
-   * @return if resource version should be parsed (as integer)
-   * @since 4.5.0
+   * @return if resource versions are comparable
+   * @since 5.3.0
    */
-  default boolean parseResourceVersionsForEventFilteringAndCaching() {
+  default boolean comparableResourceVersions() {
     return DEFAULT_COMPARABLE_RESOURCE_VERSIONS;
   }
 

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -51,11 +51,9 @@ public class ConfigurationServiceOverrider {
   private Duration reconciliationTerminationTimeout;
   private Boolean ssaBasedCreateUpdateMatchForDependentResources;
   private Set<Class<? extends HasMetadata>> defaultNonSSAResource;
-  private Boolean previousAnnotationForDependentResources;
-  private Boolean parseResourceVersions;
+  private Boolean comparableResourceVersions;
   private Boolean useSSAToPatchPrimaryResource;
   private Boolean cloneSecondaryResourcesWhenGettingFromCache;
-  private Set<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist;
 
   @SuppressWarnings("rawtypes")
   private DependentResourceFactory dependentResourceFactory;
@@ -168,28 +166,23 @@ 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;
+  public ConfigurationServiceOverrider withComparableResourceVersions(boolean value) {
+    this.comparableResourceVersions = value;
     return this;
   }
 
   /**
-   * @deprecated use withParseResourceVersions
+   * @deprecated use withComparableResourceVersions
    * @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;
+  public ConfigurationServiceOverrider withParseResourceVersions(boolean value) {
+    this.comparableResourceVersions = value;
     return this;
   }
 
@@ -204,12 +197,6 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC
     return this;
   }
 
-  public ConfigurationServiceOverrider withPreviousAnnotationForDependentResourcesBlocklist(
-      Set<Class<? extends HasMetadata>> blocklist) {
-    this.previousAnnotationUsageBlocklist = blocklist;
-    return this;
-  }
-
   public ConfigurationService build() {
     return new BaseConfigurationService(original.getVersion(), cloner, client) {
       @Override
@@ -331,13 +318,6 @@ public Set<Class<? extends HasMetadata>> defaultNonSSAResources() {
             defaultNonSSAResource, ConfigurationService::defaultNonSSAResources);
       }
 
-      @Override
-      public boolean parseResourceVersionsForEventFilteringAndCaching() {
-        return overriddenValueOrDefault(
-            parseResourceVersions,
-            ConfigurationService::parseResourceVersionsForEventFilteringAndCaching);
-      }
-
       @Override
       public boolean useSSAToPatchPrimaryResource() {
         return overriddenValueOrDefault(
@@ -352,11 +332,9 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() {
       }
 
       @Override
-      public Set<Class<? extends HasMetadata>>
-          withPreviousAnnotationForDependentResourcesBlocklist() {
+      public boolean comparableResourceVersions() {
         return overriddenValueOrDefault(
-            previousAnnotationUsageBlocklist,
-            ConfigurationService::withPreviousAnnotationForDependentResourcesBlocklist);
+            comparableResourceVersions, ConfigurationService::comparableResourceVersions);
       }
     };
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java

@@ -51,10 +51,7 @@ public ControllerEventSource(Controller<T> controller) {
         NAME,
         controller.getCRClient(),
         controller.getConfiguration(),
-        controller
-            .getConfiguration()
-            .getConfigurationService()
-            .parseResourceVersionsForEventFilteringAndCaching());
+        controller.getConfiguration().getConfigurationService().comparableResourceVersions());
     this.controller = controller;
 
     final var config = controller.getConfiguration();

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

@@ -97,15 +97,15 @@ public InformerEventSource(
   private InformerEventSource(
       InformerEventSourceConfiguration<R> configuration,
       KubernetesClient client,
-      boolean parseResourceVersions) {
+      boolean comparableResourceVersions) {
     super(
         configuration.name(),
         configuration
             .getGroupVersionKind()
             .map(gvk -> client.genericKubernetesResources(gvk.apiVersion(), gvk.getKind()))
             .orElseGet(() -> (MixedOperation) client.resources(configuration.getResourceClass())),
         configuration,
-        parseResourceVersions);
+        comparableResourceVersions);
     // If there is a primary to secondary mapper there is no need for primary to secondary index.
     primaryToSecondaryMapper = configuration.getPrimaryToSecondaryMapper();
     if (useSecondaryToPrimaryIndex()) {

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

@@ -56,17 +56,17 @@ public abstract class ManagedInformerEventSource<
 
   private static final Logger log = LoggerFactory.getLogger(ManagedInformerEventSource.class);
   private InformerManager<R, C> cache;
-  private final boolean parseResourceVersions;
+  private final boolean comparableResourceVersions;
   private ControllerConfiguration<R> controllerConfiguration;
   private final C configuration;
   private final Map<String, Function<R, List<String>>> indexers = new HashMap<>();
   protected TemporaryResourceCache<R> temporaryResourceCache;
   protected MixedOperation client;
 
   protected ManagedInformerEventSource(
-      String name, MixedOperation client, C configuration, boolean parseResourceVersions) {
+      String name, MixedOperation client, C configuration, boolean comparableResourceVersions) {
     super(configuration.getResourceClass(), name);
-    this.parseResourceVersions = parseResourceVersions;
+    this.comparableResourceVersions = comparableResourceVersions;
     this.client = client;
     this.configuration = configuration;
   }
@@ -103,7 +103,7 @@ public synchronized void start() {
     if (isRunning()) {
       return;
     }
-    temporaryResourceCache = new TemporaryResourceCache<>(parseResourceVersions);
+    temporaryResourceCache = new TemporaryResourceCache<>(comparableResourceVersions);
     this.cache = new InformerManager<>(client, configuration, this);
     cache.setControllerConfiguration(controllerConfiguration);
     cache.addIndexers(indexers);
@@ -135,7 +135,7 @@ public void handleRecentResourceCreate(ResourceID resourceID, R resource) {
   public Optional<R> get(ResourceID resourceID) {
     var res = cache.get(resourceID);
     Optional<R> resource = temporaryResourceCache.getResourceFromCache(resourceID);
-    if (parseResourceVersions
+    if (comparableResourceVersions
         && resource.isPresent()
         && res.filter(
                 r ->

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

@@ -33,15 +33,17 @@
  * a create or update is executed the subsequent getResource operation might not return the
  * up-to-date resource from informer cache, since it is not received yet.
  *
- * <p>Since an update (for create is simpler) was done successfully we can temporarily track 
- * that resource if its version is later than the events we've processed. We then know that we can skip
- * all events that have the same resource version or earlier than the tracked resource. Once we process 
- * an event that has the same resource version or later, then we know the tracked resource can be removed.
- * 
- * <p>In some cases it is possible for the informer to deliver events prior to the attempt to put the
- * resource in the temporal cache. The startModifying/doneModifying methods are used to pause event
- * delivery to ensure that temporal cache recognizes the put entry as an event that can be skipped.
- * 
+ * <p>Since an update (for create is simpler) was done successfully we can temporarily track that
+ * resource if its version is later than the events we've processed. We then know that we can skip
+ * all events that have the same resource version or earlier than the tracked resource. Once we
+ * process an event that has the same resource version or later, then we know the tracked resource
+ * can be removed.
+ *
+ * <p>In some cases it is possible for the informer to deliver events prior to the attempt to put
+ * the resource in the temporal cache. The startModifying/doneModifying methods are used to pause
+ * event delivery to ensure that temporal cache recognizes the put entry as an event that can be
+ * skipped.
+ *
  * <p>If comparable resource versions are disabled, then this cache is effectively disabled.
  *
  * @param <T> resource to cache.
@@ -51,16 +53,16 @@ public class TemporaryResourceCache<T extends HasMetadata> {
   private static final Logger log = LoggerFactory.getLogger(TemporaryResourceCache.class);
 
   private final Map<ResourceID, T> cache = new ConcurrentHashMap<>();
-  private final boolean parseResourceVersions;
+  private final boolean comparableResourceVersions;
   private final Map<ResourceID, ReentrantLock> activelyModifying = new ConcurrentHashMap<>();
   private String latestResourceVersion;
 
-  public TemporaryResourceCache(boolean parseResourceVersions) {
-    this.parseResourceVersions = parseResourceVersions;
+  public TemporaryResourceCache(boolean comparableResourceVersions) {
+    this.comparableResourceVersions = comparableResourceVersions;
   }
 
   public void startModifying(ResourceID id) {
-    if (!parseResourceVersions) {
+    if (!comparableResourceVersions) {
       return;
     }
     activelyModifying
@@ -77,7 +79,7 @@ public void startModifying(ResourceID id) {
   }
 
   public void doneModifying(ResourceID id) {
-    if (!parseResourceVersions) {
+    if (!comparableResourceVersions) {
       return;
     }
     activelyModifying.computeIfPresent(
@@ -134,7 +136,7 @@ private boolean onEvent(T resource, boolean unknownState) {
 
   /** put the item into the cache if it's for a later state than what has already been observed. */
   public synchronized void putResource(T newResource) {
-    if (!parseResourceVersions) {
+    if (!comparableResourceVersions) {
       return;
     }
 

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryPrimaryResourceCacheTest.java

@@ -18,6 +18,7 @@
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -108,7 +109,7 @@ void removesResourceFromCache() {
   }
 
   @Test
-  void resourceNoVersionParsing() {
+  void nonComparableResourceVersionsDisables() {
     this.temporaryResourceCache = new TemporaryResourceCache<>(false);
 
     this.temporaryResourceCache.putResource(testResource());
@@ -123,13 +124,16 @@ void lockedEventBeforePut() throws Exception {
 
     temporaryResourceCache.startModifying(ResourceID.fromResource(testResource));
 
-    try (ExecutorService ex = Executors.newSingleThreadExecutor()) {
+    ExecutorService ex = Executors.newSingleThreadExecutor();
+    try {
       var result = ex.submit(() -> temporaryResourceCache.onAddOrUpdateEvent(testResource));
 
       temporaryResourceCache.putResource(testResource);
       assertThat(result.isDone()).isFalse();
       temporaryResourceCache.doneModifying(ResourceID.fromResource(testResource));
-      assertThat(result.get()).isTrue();
+      assertThat(result.get(10, TimeUnit.SECONDS)).isTrue();
+    } finally {
+      ex.shutdownNow();
     }
   }
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/createupdateeventfilter/PreviousAnnotationDisabledIT.java renamed to operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/createupdateeventfilter/ComparableResourceVersionsDisabledIT.java

@@ -20,14 +20,13 @@
 
 import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
 
-class PreviousAnnotationDisabledIT {
+class ComparableResourceVersionsDisabledIT {
 
   @RegisterExtension
   LocallyRunOperatorExtension operator =
       LocallyRunOperatorExtension.builder()
           .withReconciler(new CreateUpdateEventFilterTestReconciler())
-          .withConfigurationService(
-              overrider -> overrider.withPreviousAnnotationForDependentResources(false))
+          .withConfigurationService(overrider -> overrider.withComparableResourceVersions(false))
           .build();
 
   @Test