Annotation removal using locking (#3015)

Signed-off-by: Steve Hawkins <[email protected]>

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;
@@ -46,6 +44,8 @@
 import io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowFactory;
 import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerEventSource;
 
+import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_COMPARABLE_RESOURCE_VERSIONS;
+
 /** An interface from which to retrieve configuration information. */
 public interface ConfigurationService {
 
@@ -448,61 +448,16 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
   }
 
   /**
-   * 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.
+   * If the event logic can compare resourceVersions.
    *
-   * <p>Disable this if you want to react to your own dependent resource updates
+   * <p>Enabled by default as Kubernetes does support this interpretation of resourceVersions.
+   * Disable only if your api server provides non comparable resource versions.
    *
-   * @return if special annotation should be used for dependent resource to filter events
-   * @since 4.5.0
+   * @return if resource versions are comparable
+   * @since 5.3.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.
-   *
-   * <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. This is typically not needed.
-   *
-   * <p>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;
+  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,20 +318,6 @@ public Set<Class<? extends HasMetadata>> 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(
@@ -359,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/api/config/informer/InformerEventSourceConfiguration.java

@@ -33,6 +33,7 @@
 import io.javaoperatorsdk.operator.processing.event.source.filter.OnUpdateFilter;
 import io.javaoperatorsdk.operator.processing.event.source.informer.Mappers;
 
+import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_COMPARABLE_RESOURCE_VERSIONS;
 import static io.javaoperatorsdk.operator.api.reconciler.Constants.SAME_AS_CONTROLLER_NAMESPACES_SET;
 import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_ALL_NAMESPACE_SET;
 import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_CURRENT_NAMESPACE_SET;
@@ -96,18 +97,21 @@ class DefaultInformerEventSourceConfiguration<R extends HasMetadata>
     private final GroupVersionKind groupVersionKind;
     private final InformerConfiguration<R> informerConfig;
     private final KubernetesClient kubernetesClient;
+    private final boolean comparableResourceVersions;
 
     protected DefaultInformerEventSourceConfiguration(
         GroupVersionKind groupVersionKind,
         PrimaryToSecondaryMapper<?> primaryToSecondaryMapper,
         SecondaryToPrimaryMapper<R> secondaryToPrimaryMapper,
         InformerConfiguration<R> informerConfig,
-        KubernetesClient kubernetesClient) {
+        KubernetesClient kubernetesClient,
+        boolean comparableResourceVersions) {
       this.informerConfig = Objects.requireNonNull(informerConfig);
       this.groupVersionKind = groupVersionKind;
       this.primaryToSecondaryMapper = primaryToSecondaryMapper;
       this.secondaryToPrimaryMapper = secondaryToPrimaryMapper;
       this.kubernetesClient = kubernetesClient;
+      this.comparableResourceVersions = comparableResourceVersions;
     }
 
     @Override
@@ -135,6 +139,11 @@ public Optional<GroupVersionKind> getGroupVersionKind() {
     public Optional<KubernetesClient> getKubernetesClient() {
       return Optional.ofNullable(kubernetesClient);
     }
+
+    @Override
+    public boolean comparableResourceVersions() {
+      return this.comparableResourceVersions;
+    }
   }
 
   @SuppressWarnings({"unused", "UnusedReturnValue"})
@@ -148,6 +157,7 @@ class Builder<R extends HasMetadata> {
     private PrimaryToSecondaryMapper<?> primaryToSecondaryMapper;
     private SecondaryToPrimaryMapper<R> secondaryToPrimaryMapper;
     private KubernetesClient kubernetesClient;
+    private boolean comparableResourceVersions = DEFAULT_COMPARABLE_RESOURCE_VERSIONS;
 
     private Builder(Class<R> resourceClass, Class<? extends HasMetadata> primaryResourceClass) {
       this(resourceClass, primaryResourceClass, null);
@@ -285,6 +295,11 @@ public Builder<R> withFieldSelector(FieldSelector fieldSelector) {
       return this;
     }
 
+    public Builder<R> withComparableResourceVersions(boolean comparableResourceVersions) {
+      this.comparableResourceVersions = comparableResourceVersions;
+      return this;
+    }
+
     public void updateFrom(InformerConfiguration<R> informerConfig) {
       if (informerConfig != null) {
         final var informerConfigName = informerConfig.getName();
@@ -324,7 +339,10 @@ public InformerEventSourceConfiguration<R> build() {
                   HasMetadata.getKind(primaryResourceClass),
                   false)),
           config.build(),
-          kubernetesClient);
+          kubernetesClient,
+          comparableResourceVersions);
     }
   }
+
+  boolean comparableResourceVersions();
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java

@@ -41,6 +41,7 @@ public final class Constants {
   public static final String RESOURCE_GVK_KEY = "josdk.resource.gvk";
   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_VERSIONS = true;
 
   private Constants() {}
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java

@@ -451,6 +451,11 @@ public static <P extends HasMetadata> P addFinalizerWithSSA(
     }
   }
 
+  public static int compareResourceVersions(HasMetadata h1, HasMetadata h2) {
+    return compareResourceVersions(
+        h1.getMetadata().getResourceVersion(), h2.getMetadata().getResourceVersion());
+  }
+
   public static int compareResourceVersions(String v1, String v2) {
     int v1Length = validateResourceVersion(v1);
     int v2Length = validateResourceVersion(v2);

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -55,7 +55,6 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   private final boolean garbageCollected = this instanceof GarbageCollected;
   private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
   private volatile Boolean useSSA;
-  private volatile Boolean usePreviousAnnotationForEventFiltering;
 
   public KubernetesDependentResource() {}
 
@@ -72,6 +71,27 @@ public void configureWith(KubernetesDependentResourceConfig<R> config) {
     this.kubernetesDependentResourceConfig = config;
   }
 
+  @Override
+  protected R handleCreate(R desired, P primary, Context<P> context) {
+    return eventSource()
+        .orElseThrow()
+        .updateAndCacheResource(
+            desired,
+            context,
+            toCreate -> KubernetesDependentResource.super.handleCreate(toCreate, primary, context));
+  }
+
+  @Override
+  protected R handleUpdate(R actual, R desired, P primary, Context<P> context) {
+    return eventSource()
+        .orElseThrow()
+        .updateAndCacheResource(
+            desired,
+            context,
+            toUpdate ->
+                KubernetesDependentResource.super.handleUpdate(actual, toUpdate, primary, context));
+  }
+
   @SuppressWarnings("unused")
   public R create(R desired, P primary, Context<P> context) {
     if (useSSA(context)) {
@@ -158,14 +178,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 +193,6 @@ protected boolean useSSA(Context<P> context) {
     return useSSA;
   }
 
-  private boolean usePreviousAnnotation(Context<P> 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<P> context) {
     if (secondary != null) {

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

@@ -47,7 +47,11 @@ public class ControllerEventSource<T extends HasMetadata>
 
   @SuppressWarnings({"unchecked", "rawtypes"})
   public ControllerEventSource(Controller<T> controller) {
-    super(NAME, controller.getCRClient(), controller.getConfiguration(), false);
+    super(
+        NAME,
+        controller.getCRClient(),
+        controller.getConfiguration(),
+        controller.getConfiguration().getConfigurationService().comparableResourceVersions());
     this.controller = controller;
 
     final var config = controller.getConfiguration();