fix: using tombstones to account for rapid deletion

closes: #2314

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

Author: shawkins

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

@@ -51,17 +51,17 @@ protected ManagedInformerEventSource(
 
   @Override
   public void onAdd(R resource) {
-    temporaryResourceCache.onEvent(resource, false);
+    temporaryResourceCache.onAddOrUpdateEvent(resource);
   }
 
   @Override
   public void onUpdate(R oldObj, R newObj) {
-    temporaryResourceCache.onEvent(newObj, false);
+    temporaryResourceCache.onAddOrUpdateEvent(newObj);
   }
 
   @Override
   public void onDelete(R obj, boolean deletedFinalStateUnknown) {
-    temporaryResourceCache.onEvent(obj, deletedFinalStateUnknown);
+    temporaryResourceCache.onDeleteEvent(obj, deletedFinalStateUnknown);
   }
 
   protected InformerManager<R, C> manager() {

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

@@ -38,8 +38,11 @@ public class TemporaryResourceCache<T extends HasMetadata> {
 
   private static final Logger log = LoggerFactory.getLogger(TemporaryResourceCache.class);
   private static final int MAX_RESOURCE_VERSIONS = 256;
+  private static final int TOMBSTONE_RETENTION_TIME = 120000;
 
   private final Map<ResourceID, T> cache = new ConcurrentHashMap<>();
+
+  private final Map<String, Long> tombstones = new LinkedHashMap<String, Long>();
   private final ManagedInformerEventSource<T, ?, ?> managedInformerEventSource;
   private final boolean parseResourceVersions;
   private final Set<String> knownResourceVersions;
@@ -51,7 +54,7 @@ public TemporaryResourceCache(ManagedInformerEventSource<T, ?, ?> managedInforme
     if (parseResourceVersions) {
       knownResourceVersions = Collections.newSetFromMap(new LinkedHashMap<String, Boolean>() {
         @Override
-        protected boolean removeEldestEntry(java.util.Map.Entry<String, Boolean> eldest) {
+        protected boolean removeEldestEntry(Map.Entry<String, Boolean> eldest) {
           return size() >= MAX_RESOURCE_VERSIONS;
         }
       });
@@ -60,7 +63,23 @@ protected boolean removeEldestEntry(java.util.Map.Entry<String, Boolean> eldest)
     }
   }
 
-  public synchronized void onEvent(T resource, boolean unknownState) {
+  public synchronized void onDeleteEvent(T resource, boolean unknownState) {
+    tombstones.put(resource.getMetadata().getUid(), System.currentTimeMillis());
+    onEvent(resource, unknownState);
+  }
+
+  public synchronized void onAddOrUpdateEvent(T resource) {
+    onEvent(resource, false);
+  }
+
+  synchronized void onEvent(T resource, boolean unknownState) {
+    if (!tombstones.isEmpty()) {
+      var iter = tombstones.entrySet().iterator();
+      var entry = iter.next();
+      if (System.currentTimeMillis() - entry.getValue() > TOMBSTONE_RETENTION_TIME) {
+        iter.remove();
+      }
+    }
     cache.computeIfPresent(ResourceID.fromResource(resource),
         (id, cached) -> (unknownState || !isLaterResourceVersion(id, cached, resource)) ? null
             : cached);
@@ -84,14 +103,25 @@ public synchronized void putResource(T newResource, String previousResourceVersi
     var cachedResource = getResourceFromCache(resourceId)
         .orElse(managedInformerEventSource.get(resourceId).orElse(null));
 
-    if ((previousResourceVersion == null && cachedResource == null)
+    boolean moveAhead = false;
+    if (previousResourceVersion == null && cachedResource == null) {
+      if (tombstones.get(newResource.getMetadata().getUid()) != null) {
+        log.debug(
+            "Won't resurrect uid {} for resource id: {}",
+            newResource.getMetadata().getUid(), resourceId);
+        return;
+      }
+      moveAhead = true;
+    }
+
+    if (moveAhead
         || (cachedResource != null
             && (cachedResource.getMetadata().getResourceVersion().equals(previousResourceVersion))
             || isLaterResourceVersion(resourceId, newResource, cachedResource))) {
       log.debug(
           "Temporarily moving ahead to target version {} for resource id: {}",
           newResource.getMetadata().getResourceVersion(), resourceId);
-      putToCache(newResource, resourceId);
+      cache.put(resourceId, newResource);
     } else if (cache.remove(resourceId) != null) {
       log.debug("Removed an obsolete resource from cache for id: {}", resourceId);
     }
@@ -123,10 +153,6 @@ private boolean isLaterResourceVersion(ResourceID resourceId, T newResource, T c
     return false;
   }
 
-  private void putToCache(T resource, ResourceID resourceID) {
-    cache.put(resourceID == null ? ResourceID.fromResource(resource) : resourceID, resource);
-  }
-
   public synchronized Optional<T> getResourceFromCache(ResourceID resourceID) {
     return Optional.ofNullable(cache.get(resourceID));
   }

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

@@ -120,7 +120,7 @@ void propagateEventAndRemoveResourceFromTempCacheIfResourceVersionMismatch() {
     informerEventSource.onUpdate(cachedDeployment, testDeployment());
 
     verify(eventHandlerMock, times(1)).handleEvent(any());
-    verify(temporaryResourceCacheMock, times(1)).onEvent(testDeployment(), false);
+    verify(temporaryResourceCacheMock, times(1)).onAddOrUpdateEvent(testDeployment());
   }
 
   @Test

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

@@ -81,7 +81,7 @@ void addOperationNotAddsTheResourceIfInformerCacheNotEmpty() {
   void removesResourceFromCache() {
     ConfigMap testResource = propagateTestResourceToCache();
 
-    temporaryResourceCache.onEvent(testResource(), false);
+    temporaryResourceCache.onAddOrUpdateEvent(testResource());
 
     assertThat(temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)))
         .isNotPresent();
@@ -96,20 +96,33 @@ void resourceVersionParsing() {
     ConfigMap testResource = propagateTestResourceToCache();
 
     // an event with a newer version will not remove
-    temporaryResourceCache.onEvent(new ConfigMapBuilder(testResource).editMetadata()
-        .withResourceVersion("1").endMetadata().build(), false);
+    temporaryResourceCache.onAddOrUpdateEvent(new ConfigMapBuilder(testResource).editMetadata()
+        .withResourceVersion("1").endMetadata().build());
 
     assertThat(temporaryResourceCache.isKnownResourceVersion(testResource)).isTrue();
     assertThat(temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)))
         .isPresent();
 
     // anything else will remove
-    temporaryResourceCache.onEvent(testResource(), false);
+    temporaryResourceCache.onAddOrUpdateEvent(testResource());
 
     assertThat(temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)))
         .isNotPresent();
   }
 
+  @Test
+  void rapidDeletion() {
+    var testResource = testResource();
+
+    temporaryResourceCache.onAddOrUpdateEvent(testResource);
+    temporaryResourceCache.onDeleteEvent(new ConfigMapBuilder(testResource).editMetadata()
+        .withResourceVersion("3").endMetadata().build(), false);
+    temporaryResourceCache.putAddedResource(testResource);
+
+    assertThat(temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)))
+        .isEmpty();
+  }
+
   private ConfigMap propagateTestResourceToCache() {
     var testResource = testResource();
     when(informerEventSource.get(any())).thenReturn(Optional.empty());
@@ -127,6 +140,7 @@ ConfigMap testResource() {
     configMap.getMetadata().setName("test");
     configMap.getMetadata().setNamespace("default");
     configMap.getMetadata().setResourceVersion(RESOURCE_VERSION);
+    configMap.getMetadata().setUid("test-uid");
     return configMap;
   }