From 898e2c234d3b960e5fb25309e608307104599e3b Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 30 Sep 2021 17:02:23 +0200 Subject: [PATCH 01/40] WIP --- .../event/internal/CustomResourceEventSource.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index c7a959061b..3affb3e946 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -4,6 +4,7 @@ import java.util.LinkedList; import java.util.List; +import io.fabric8.kubernetes.client.informers.SharedIndexInformer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -34,16 +35,15 @@ public class CustomResourceEventSource> extends A private static final Logger log = LoggerFactory.getLogger(CustomResourceEventSource.class); private final ConfiguredController controller; - private final List watches; - private final CustomResourceCache customResourceCache; +// private final List watches; + private final SharedIndexInformer sharedIndexInformer = null; +// private final CustomResourceCache customResourceCache; + // todo metric for custom resource caches + // todo namespaces for informers public CustomResourceEventSource(ConfiguredController controller) { this.controller = controller; - this.watches = new LinkedList<>(); - this.customResourceCache = new CustomResourceCache<>( - controller.getConfiguration().getConfigurationService().getObjectMapper(), - controller.getConfiguration().getConfigurationService().getMetrics()); } @Override From be4e5cf4ad919893a7b7f27e8d4aafd57e6baf66 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 1 Oct 2021 16:46:31 +0200 Subject: [PATCH 02/40] Addressing Custom Resource by Name and Namespace refactor + Informer Cache WIP --- .../operator/EventListUtils.java | 4 +- .../io/javaoperatorsdk/operator/Operator.java | 5 +- .../processing/CustomResourceCache.java | 2 + .../processing/DefaultEventHandler.java | 74 ++++++------ .../operator/processing/EventBuffer.java | 21 ++-- .../operator/processing/ExecutionScope.java | 5 +- .../operator/processing/ResourceCache.java | 13 ++ .../processing/event/AbstractEvent.java | 21 ---- .../processing/event/CustomResourceID.java | 50 ++++++++ .../processing/event/DefaultEvent.java | 62 ++-------- .../event/DefaultEventSourceManager.java | 56 ++++----- .../operator/processing/event/Event.java | 20 +--- .../processing/event/EventSource.java | 2 +- .../processing/event/EventSourceManager.java | 2 +- .../event/internal/CustomResourceEvent.java | 50 +++----- .../internal/CustomResourceEventSource.java | 113 ++++++++---------- .../event/internal/InformerEvent.java | 27 ++--- .../event/internal/InformerEventSource.java | 10 +- .../event/internal/ResourceAction.java | 5 + .../processing/event/internal/TimerEvent.java | 5 +- .../event/internal/TimerEventSource.java | 25 ++-- .../CustomResourceSelectorTest.java | 110 ----------------- .../processing/DefaultEventHandlerTest.java | 11 +- .../operator/processing/EventBufferTest.java | 20 ++-- .../processing/EventDispatcherTest.java | 37 +++--- .../event/DefaultEventSourceManagerTest.java | 6 +- .../CustomResourceEventSourceTest.java | 2 +- 27 files changed, 290 insertions(+), 468 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEvent.java create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/ResourceAction.java delete mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/CustomResourceSelectorTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/EventListUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/EventListUtils.java index 852b630af0..0fe18bdae0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/EventListUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/EventListUtils.java @@ -2,9 +2,9 @@ import java.util.List; -import io.fabric8.kubernetes.client.Watcher; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; +import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; public class EventListUtils { @@ -13,7 +13,7 @@ public static boolean containsCustomResourceDeletedEvent(List events) { .anyMatch( e -> { if (e instanceof CustomResourceEvent) { - return ((CustomResourceEvent) e).getAction() == Watcher.Action.DELETED; + return ((CustomResourceEvent) e).getAction() == ResourceAction.DELETED; } else { return false; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java index 834c516c68..e653324ea6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java @@ -20,6 +20,7 @@ import io.javaoperatorsdk.operator.processing.ConfiguredController; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; import io.javaoperatorsdk.operator.processing.DefaultEventHandler.EventMonitor; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; @SuppressWarnings("rawtypes") @@ -34,12 +35,12 @@ public Operator(KubernetesClient k8sClient, ConfigurationService configurationSe this.configurationService = configurationService; DefaultEventHandler.setEventMonitor(new EventMonitor() { @Override - public void processedEvent(String uid, Event event) { + public void processedEvent(CustomResourceID uid, Event event) { configurationService.getMetrics().incrementProcessedEventsNumber(); } @Override - public void failedEvent(String uid, Event event) { + public void failedEvent(CustomResourceID uid, Event event) { configurationService.getMetrics().incrementControllerRetriesNumber(); } }); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java index ec66ec210d..9ce8316c3c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java @@ -23,6 +23,8 @@ import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; @SuppressWarnings("rawtypes") +// todo remove +@Deprecated public class CustomResourceCache> { private static final Logger log = LoggerFactory.getLogger(CustomResourceCache.class); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 74d9e994de..e0e78acb10 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -8,7 +8,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Predicate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -17,6 +16,7 @@ import io.javaoperatorsdk.operator.api.RetryInfo; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.EventHandler; @@ -37,17 +37,17 @@ public class DefaultEventHandler> implements Even private static final Logger log = LoggerFactory.getLogger(DefaultEventHandler.class); private static EventMonitor monitor = new EventMonitor() { @Override - public void processedEvent(String uid, Event event) {} + public void processedEvent(CustomResourceID uid, Event event) {} @Override - public void failedEvent(String uid, Event event) {} + public void failedEvent(CustomResourceID uid, Event event) {} }; private final EventBuffer eventBuffer; - private final Set underProcessing = new HashSet<>(); + private final Set underProcessing = new HashSet<>(); private final EventDispatcher eventDispatcher; private final Retry retry; - private final Map retryState = new HashMap<>(); + private final Map retryState = new HashMap<>(); private final ExecutorService executor; private final String controllerName; private final ReentrantLock lock = new ReentrantLock(); @@ -87,9 +87,9 @@ public static void setEventMonitor(EventMonitor monitor) { } public interface EventMonitor { - void processedEvent(String uid, Event event); + void processedEvent(CustomResourceID uid, Event event); - void failedEvent(String uid, Event event); + void failedEvent(CustomResourceID uid, Event event); } @Override @@ -97,23 +97,19 @@ public void handleEvent(Event event) { try { lock.lock(); log.debug("Received event: {}", event); - - final Predicate selector = event.getCustomResourcesSelector(); - for (String uid : eventSourceManager.getLatestResourceUids(selector)) { - eventBuffer.addEvent(uid, event); - monitor.processedEvent(uid, event); - executeBufferedEvents(uid); - } + eventBuffer.addEvent(event.getRelatedCustomResourceID(), event); + monitor.processedEvent(event.getRelatedCustomResourceID(), event); + executeBufferedEvents(event.getRelatedCustomResourceID()); } finally { lock.unlock(); } } - private void executeBufferedEvents(String customResourceUid) { + private void executeBufferedEvents(CustomResourceID customResourceUid) { boolean newEventForResourceId = eventBuffer.containsEvents(customResourceUid); boolean controllerUnderExecution = isControllerUnderExecution(customResourceUid); Optional latestCustomResource = - eventSourceManager.getLatestResource(customResourceUid); + eventSourceManager.getCache().getCustomResource(customResourceUid); if (!controllerUnderExecution && newEventForResourceId && latestCustomResource.isPresent()) { setUnderExecutionProcessing(customResourceUid); @@ -135,7 +131,7 @@ private void executeBufferedEvents(String customResourceUid) { } } - private RetryInfo retryInfo(String customResourceUid) { + private RetryInfo retryInfo(CustomResourceID customResourceUid) { return retryState.get(customResourceUid); } @@ -147,12 +143,12 @@ void eventProcessingFinished( "Event processing finished. Scope: {}, PostExecutionControl: {}", executionScope, postExecutionControl); - unsetUnderExecution(executionScope.getCustomResourceUid()); + unsetUnderExecution(executionScope.getCustomResourceID()); if (retry != null && postExecutionControl.exceptionDuringExecution()) { handleRetryOnException(executionScope); executionScope.getEvents() - .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceUid(), e)); + .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceID(), e)); return; } @@ -160,11 +156,11 @@ void eventProcessingFinished( markSuccessfulExecutionRegardingRetry(executionScope); } if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { - cleanupAfterDeletedEvent(executionScope.getCustomResourceUid()); + cleanupAfterDeletedEvent(executionScope.getCustomResourceID()); } else { cacheUpdatedResourceIfChanged(executionScope, postExecutionControl); reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); - executeBufferedEvents(executionScope.getCustomResourceUid()); + executeBufferedEvents(executionScope.getCustomResourceID()); } } finally { lock.unlock(); @@ -185,12 +181,13 @@ private void reScheduleExecutionIfInstructed(PostExecutionControl postExecuti */ private void handleRetryOnException(ExecutionScope executionScope) { RetryExecution execution = getOrInitRetryExecution(executionScope); - boolean newEventsExists = eventBuffer.newEventsExists(executionScope.getCustomResourceUid()); - eventBuffer.putBackEvents(executionScope.getCustomResourceUid(), executionScope.getEvents()); + boolean newEventsExists = eventBuffer + .newEventsExists(CustomResourceID.fromResource(executionScope.getCustomResource())); + eventBuffer.putBackEvents(executionScope.getCustomResourceID(), executionScope.getEvents()); if (newEventsExists) { - log.debug("New events exists for for resource id: {}", executionScope.getCustomResourceUid()); - executeBufferedEvents(executionScope.getCustomResourceUid()); + log.debug("New events exists for for resource id: {}", executionScope.getCustomResourceID()); + executeBufferedEvents(executionScope.getCustomResourceID()); return; } Optional nextDelay = execution.nextDelay(); @@ -200,7 +197,7 @@ private void handleRetryOnException(ExecutionScope executionScope) { log.debug( "Scheduling timer event for retry with delay:{} for resource: {}", delay, - executionScope.getCustomResourceUid()); + executionScope.getCustomResourceID()); eventSourceManager .getRetryTimerEventSource() .scheduleOnce(executionScope.getCustomResource(), delay); @@ -212,17 +209,17 @@ private void markSuccessfulExecutionRegardingRetry(ExecutionScope executionSc log.debug( "Marking successful execution for resource: {}", getName(executionScope.getCustomResource())); - retryState.remove(executionScope.getCustomResourceUid()); + retryState.remove(executionScope.getCustomResourceID()); eventSourceManager .getRetryTimerEventSource() - .cancelOnceSchedule(executionScope.getCustomResourceUid()); + .cancelOnceSchedule(executionScope.getCustomResourceID()); } private RetryExecution getOrInitRetryExecution(ExecutionScope executionScope) { - RetryExecution retryExecution = retryState.get(executionScope.getCustomResourceUid()); + RetryExecution retryExecution = retryState.get(executionScope.getCustomResourceID()); if (retryExecution == null) { retryExecution = retry.initExecution(); - retryState.put(executionScope.getCustomResourceUid(), retryExecution); + retryState.put(executionScope.getCustomResourceID(), retryExecution); } return retryExecution; } @@ -255,27 +252,28 @@ private void cacheUpdatedResourceIfChanged( getName(originalCustomResource), getVersion(customResourceAfterExecution), getVersion(originalCustomResource)); - eventSourceManager.cacheResource( - customResourceAfterExecution, - customResource -> getVersion(customResource).equals(originalResourceVersion) - && !originalResourceVersion.equals(getVersion(customResourceAfterExecution))); + // todo how to handle this? + // eventSourceManager.cacheResource( + // customResourceAfterExecution, + // customResource -> getVersion(customResource).equals(originalResourceVersion) + // && !originalResourceVersion.equals(getVersion(customResourceAfterExecution))); } } - private void cleanupAfterDeletedEvent(String customResourceUid) { + private void cleanupAfterDeletedEvent(CustomResourceID customResourceUid) { eventSourceManager.cleanup(customResourceUid); eventBuffer.cleanup(customResourceUid); } - private boolean isControllerUnderExecution(String customResourceUid) { + private boolean isControllerUnderExecution(CustomResourceID customResourceUid) { return underProcessing.contains(customResourceUid); } - private void setUnderExecutionProcessing(String customResourceUid) { + private void setUnderExecutionProcessing(CustomResourceID customResourceUid) { underProcessing.add(customResourceUid); } - private void unsetUnderExecution(String customResourceUid) { + private void unsetUnderExecution(CustomResourceID customResourceUid) { underProcessing.remove(customResourceUid); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java index c9d248acf2..29ae58eca7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java @@ -2,45 +2,46 @@ import java.util.*; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; class EventBuffer { - private final Map> events = new HashMap<>(); + private final Map> events = new HashMap<>(); - /** @deprecated use {@link #addEvent(String, Event)} */ + /** @deprecated use {@link #addEvent(CustomResourceID, Event)} */ @Deprecated public void addEvent(Event event) { - addEvent(event.getRelatedCustomResourceUid(), event); + addEvent(event.getRelatedCustomResourceID(), event); } - public void addEvent(String uid, Event event) { + public void addEvent(CustomResourceID uid, Event event) { Objects.requireNonNull(uid, "uid"); Objects.requireNonNull(event, "event"); - List crEvents = events.computeIfAbsent(uid, (id) -> new LinkedList<>()); + List crEvents = events.computeIfAbsent(uid, (customResourceID) -> new LinkedList<>()); crEvents.add(event); } - public boolean newEventsExists(String resourceId) { + public boolean newEventsExists(CustomResourceID resourceId) { return events.get(resourceId) != null && !events.get(resourceId).isEmpty(); } - public void putBackEvents(String resourceUid, List oldEvents) { + public void putBackEvents(CustomResourceID resourceUid, List oldEvents) { List crEvents = events.computeIfAbsent(resourceUid, (id) -> new LinkedList<>()); crEvents.addAll(0, oldEvents); } - public boolean containsEvents(String customResourceId) { + public boolean containsEvents(CustomResourceID customResourceId) { return events.get(customResourceId) != null; } - public List getAndRemoveEventsForExecution(String resourceUid) { + public List getAndRemoveEventsForExecution(CustomResourceID resourceUid) { List crEvents = events.remove(resourceUid); return crEvents == null ? Collections.emptyList() : crEvents; } - public void cleanup(String resourceUid) { + public void cleanup(CustomResourceID resourceUid) { events.remove(resourceUid); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java index 53917cc094..4f5f0ca7f7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java @@ -4,6 +4,7 @@ import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.api.RetryInfo; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; public class ExecutionScope> { @@ -27,8 +28,8 @@ public R getCustomResource() { return customResource; } - public String getCustomResourceUid() { - return customResource.getMetadata().getUid(); + public CustomResourceID getCustomResourceID() { + return CustomResourceID.fromResource(customResource); } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java new file mode 100644 index 0000000000..0f513333a4 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java @@ -0,0 +1,13 @@ +package io.javaoperatorsdk.operator.processing; + +import java.util.Optional; + +import io.fabric8.kubernetes.client.CustomResource; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; + +// todo rename after deleted CustomResourceCache +public interface ResourceCache> { + + Optional getCustomResource(CustomResourceID resourceID); + +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEvent.java deleted file mode 100644 index 3429301fe1..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEvent.java +++ /dev/null @@ -1,21 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event; - -import java.util.function.Predicate; - -import io.fabric8.kubernetes.client.CustomResource; - -/** - * @deprecated use {@link DefaultEvent} instead - */ -@Deprecated -public class AbstractEvent extends DefaultEvent { - - public AbstractEvent(String relatedCustomResourceUid, EventSource eventSource) { - super(relatedCustomResourceUid, eventSource); - } - - public AbstractEvent( - Predicate customResourcesSelector, EventSource eventSource) { - super(customResourcesSelector, eventSource); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java new file mode 100644 index 0000000000..bf1958d9f1 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java @@ -0,0 +1,50 @@ +package io.javaoperatorsdk.operator.processing.event; + +import java.util.Objects; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; + +public class CustomResourceID { + + public static CustomResourceID fromResource(HasMetadata resource) { + return new CustomResourceID(resource.getMetadata().getName(), + resource.getMetadata().getNamespace()); + } + + private String name; + private String namespace; + + public CustomResourceID(String name, String namespace) { + this.name = name; + this.namespace = namespace; + } + + public CustomResourceID(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public Optional getNamespace() { + return Optional.ofNullable(namespace); + } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + CustomResourceID that = (CustomResourceID) o; + return Objects.equals(name, that.name) && Objects.equals(namespace, + that.namespace); + } + + @Override + public int hashCode() { + return Objects.hash(name, namespace); + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java index b966c06609..75d63c4174 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java @@ -1,70 +1,24 @@ package io.javaoperatorsdk.operator.processing.event; -import java.util.Objects; -import java.util.function.Predicate; - -import io.fabric8.kubernetes.client.CustomResource; - @SuppressWarnings("rawtypes") public class DefaultEvent implements Event { - private final Predicate customResourcesSelector; - private final EventSource eventSource; - - public DefaultEvent(String relatedCustomResourceUid, EventSource eventSource) { - this.customResourcesSelector = new UIDMatchingPredicate(relatedCustomResourceUid); - this.eventSource = eventSource; - } + private final CustomResourceID relatedCustomResource; - public DefaultEvent(Predicate customResourcesSelector, EventSource eventSource) { - this.customResourcesSelector = customResourcesSelector; - this.eventSource = eventSource; - } - @Override - public String getRelatedCustomResourceUid() { - if (customResourcesSelector instanceof UIDMatchingPredicate) { - UIDMatchingPredicate resourcesSelector = (UIDMatchingPredicate) customResourcesSelector; - return resourcesSelector.uid; - } else { - return null; - } + public DefaultEvent(CustomResourceID targetCustomResource) { + this.relatedCustomResource = targetCustomResource; } - public Predicate getCustomResourcesSelector() { - return customResourcesSelector; - } @Override - public EventSource getEventSource() { - return eventSource; + public CustomResourceID getRelatedCustomResourceID() { + return null; } @Override public String toString() { - return "{ class=" - + this.getClass().getName() - + ", customResourcesSelector=" - + customResourcesSelector - + ", eventSource=" - + eventSource - + " }"; - } - - public static class UIDMatchingPredicate implements Predicate { - private final String uid; - - public UIDMatchingPredicate(String uid) { - this.uid = uid; - } - - @Override - public boolean test(CustomResource customResource) { - return Objects.equals(uid, customResource.getMetadata().getUid()); - } - - @Override - public String toString() { - return "UIDMatchingPredicate{uid='" + uid + "'}"; - } + return "DefaultEvent{" + + "relatedCustomResource=" + relatedCustomResource + + '}'; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 3aad5131f8..8d062b3c00 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -2,14 +2,11 @@ import java.io.IOException; import java.util.Collections; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Predicate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -18,8 +15,8 @@ import io.javaoperatorsdk.operator.MissingCRDException; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.processing.ConfiguredController; -import io.javaoperatorsdk.operator.processing.CustomResourceCache; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; +import io.javaoperatorsdk.operator.processing.ResourceCache; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEventSource; import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource; @@ -115,7 +112,7 @@ public Optional deRegisterEventSource(String name) { @Override public Optional deRegisterCustomResourceFromEventSource( - String eventSourceName, String customResourceUid) { + String eventSourceName, CustomResourceID customResourceUid) { try { lock.lock(); EventSource eventSource = this.eventSources.get(eventSourceName); @@ -143,42 +140,37 @@ public Map getRegisteredEventSources() { return Collections.unmodifiableMap(eventSources); } - public void cleanup(String customResourceUid) { + public void cleanup(CustomResourceID customResourceUid) { getRegisteredEventSources() .keySet() .forEach(k -> deRegisterCustomResourceFromEventSource(k, customResourceUid)); - eventSources.remove(customResourceUid); - CustomResourceCache cache = getCache(); - if (cache != null) { - cache.cleanup(customResourceUid); - } } // todo: remove - public CustomResourceCache getCache() { + public ResourceCache getCache() { final var source = (CustomResourceEventSource) getRegisteredEventSources() .get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME); - return source.getCache(); - } - - // todo: remove - public Optional getLatestResource(String customResourceUid) { - return getCache().getLatestResource(customResourceUid); - } - - // todo: remove - public List getLatestResources(Predicate selector) { - return getCache().getLatestResources(selector); - } - - // todo: remove - public Set getLatestResourceUids(Predicate selector) { - return getCache().getLatestResourcesUids(selector); + return source; } - // todo: remove - public void cacheResource(CustomResource resource, Predicate predicate) { - getCache().cacheResource(resource, predicate); - } + // // todo: remove + // public Optional getLatestResource(String customResourceUid) { + // return getCache().getLatestResource(customResourceUid); + // } + // + // // todo: remove + // public List getLatestResources(Predicate selector) { + // return getCache().getLatestResources(selector); + // } + // + // // todo: remove + // public Set getLatestResourceUids(Predicate selector) { + // return getCache().getLatestResourcesUids(selector); + // } + // + // // todo: remove + // public void cacheResource(CustomResource resource, Predicate predicate) { + // getCache().cacheResource(resource, predicate); + // } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java index 095467fd39..b0d51a37c0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java @@ -1,25 +1,7 @@ package io.javaoperatorsdk.operator.processing.event; -import java.util.function.Predicate; - -import io.fabric8.kubernetes.client.CustomResource; - public interface Event { - /** - * @return the UID of the the {@link CustomResource} for which a reconcile loop should be - * triggered. - * @deprecated use {@link #getCustomResourcesSelector()} - */ - @Deprecated - String getRelatedCustomResourceUid(); - - /** - * The selector used to determine the {@link CustomResource} for which a reconcile loop should be - * triggered. - */ - Predicate getCustomResourcesSelector(); + CustomResourceID getRelatedCustomResourceID(); - /** @return the {@link EventSource} that has generated the event. */ - EventSource getEventSource(); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSource.java index 149e0acc1c..1cc05f632c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSource.java @@ -20,5 +20,5 @@ default void close() throws IOException {} void setEventHandler(EventHandler eventHandler); - default void eventSourceDeRegisteredForResource(String customResourceUid) {} + default void eventSourceDeRegisteredForResource(CustomResourceID customResourceUid) {} } 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 b59491042b..9f8b0f40fa 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 @@ -32,7 +32,7 @@ void registerEventSource(String name, EventSource eventSource) Optional deRegisterEventSource(String name); Optional deRegisterCustomResourceFromEventSource( - String name, String customResourceUid); + String name, CustomResourceID customResourceUid); Map getRegisteredEventSources(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java index 007ce49edc..15a67108db 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java @@ -1,54 +1,36 @@ package io.javaoperatorsdk.operator.processing.event.internal; import io.fabric8.kubernetes.client.CustomResource; -import io.fabric8.kubernetes.client.Watcher; -import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; - public class CustomResourceEvent extends DefaultEvent { - private final Watcher.Action action; + private final ResourceAction action; private final CustomResource customResource; - public CustomResourceEvent( - Watcher.Action action, - CustomResource resource, - CustomResourceEventSource customResourceEventSource) { - super(KubernetesResourceUtils.getUID(resource), customResourceEventSource); - this.action = action; - this.customResource = resource; - } - - public Watcher.Action getAction() { - return action; - } - public String resourceUid() { - return getCustomResource().getMetadata().getUid(); + public CustomResourceEvent(ResourceAction action, + CustomResource resource) { + super(CustomResourceID.fromResource(resource)); + this.customResource = resource; + this.action = action; } @Override public String toString() { - return "CustomResourceEvent{" - + "action=" - + action - + ", resource=[ name=" - + getName(getCustomResource()) - + ", kind=" - + getCustomResource().getKind() - + ", apiVersion=" - + getCustomResource().getApiVersion() - + " ,resourceVersion=" - + getCustomResource().getMetadata().getResourceVersion() - + ", markedForDeletion: " - + (getCustomResource().getMetadata().getDeletionTimestamp() != null - && !getCustomResource().getMetadata().getDeletionTimestamp().isEmpty()) - + " ]}"; + return "CustomResourceEvent{" + + "action=" + action + + ", customResource=" + customResource + + '}'; } public CustomResource getCustomResource() { return customResource; } + + public ResourceAction getAction() { + return action; + } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 3affb3e946..705d7b0cb8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -1,26 +1,25 @@ package io.javaoperatorsdk.operator.processing.event.internal; import java.io.IOException; -import java.util.LinkedList; -import java.util.List; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; -import io.fabric8.kubernetes.client.informers.SharedIndexInformer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.ListOptions; import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.client.KubernetesClientException; -import io.fabric8.kubernetes.client.Watch; -import io.fabric8.kubernetes.client.Watcher; -import io.fabric8.kubernetes.client.WatcherException; +import io.fabric8.kubernetes.client.informers.ResourceEventHandler; +import io.fabric8.kubernetes.client.informers.SharedIndexInformer; +import io.fabric8.kubernetes.client.informers.cache.Cache; import io.fabric8.kubernetes.client.utils.Utils; import io.javaoperatorsdk.operator.MissingCRDException; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.processing.ConfiguredController; -import io.javaoperatorsdk.operator.processing.CustomResourceCache; -import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; +import io.javaoperatorsdk.operator.processing.ResourceCache; import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; @@ -30,20 +29,19 @@ * This is a special case since is not bound to a single custom resource */ public class CustomResourceEventSource> extends AbstractEventSource - implements Watcher { + implements ResourceEventHandler, ResourceCache { + + private static final String ANY_NAMESPACE_MAP_KEY = "anyNamespace"; private static final Logger log = LoggerFactory.getLogger(CustomResourceEventSource.class); private final ConfiguredController controller; -// private final List watches; - private final SharedIndexInformer sharedIndexInformer = null; -// private final CustomResourceCache customResourceCache; + private final Map> sharedIndexInformers = + new ConcurrentHashMap<>(); // todo metric for custom resource caches - // todo namespaces for informers public CustomResourceEventSource(ConfiguredController controller) { this.controller = controller; - } @Override @@ -59,18 +57,20 @@ public void start() { try { if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { - var w = client.inAnyNamespace().watch(options, this); - watches.add(w); - log.debug("Registered {} -> {} for any namespace", controller, w); + var informer = client.inAnyNamespace().inform(this); + sharedIndexInformers.put(ANY_NAMESPACE_MAP_KEY, informer); + log.debug("Registered {} -> {} for any namespace", controller, informer); } else { targetNamespaces.forEach( ns -> { - var w = client.inNamespace(ns).watch(options, this); - watches.add(w); - log.debug("Registered {} -> {} for namespace: {}", controller, w, ns); + var informer = client.inNamespace(ns).inform(this); + sharedIndexInformers.put(ns, informer); + log.debug("Registered {} -> {} for namespace: {}", controller, informer, + ns); }); } } catch (Exception e) { + // todo double check this if still applies for informers if (e instanceof KubernetesClientException) { KubernetesClientException ke = (KubernetesClientException) e; if (404 == ke.getCode()) { @@ -88,35 +88,20 @@ public void start() { @Override public void close() throws IOException { eventHandler.close(); - for (Watch watch : this.watches) { + for (SharedIndexInformer informer : sharedIndexInformers.values()) { try { - log.info("Closing watch {} -> {}", controller, watch); - watch.close(); + log.info("Closing informer {} -> {}", controller, informer); + informer.close(); } catch (Exception e) { - log.warn("Error closing watcher {} -> {}", controller, watch, e); + log.warn("Error closing informer {} -> {}", controller, informer, e); } } } - @Override - public void eventReceived(Watcher.Action action, T customResource) { + // todo check if the resource version is the same? + public void eventReceived(ResourceAction action, T customResource, T oldResource) { log.debug( - "Event received for action: {}, resource: {}", action.name(), getName(customResource)); - - final String uuid = KubernetesResourceUtils.getUID(customResource); - final T oldResource = customResourceCache.getLatestResource(uuid).orElse(null); - - // cache the latest version of the CR - customResourceCache.cacheResource(customResource); - - if (action == Action.ERROR) { - log.debug( - "Skipping {} event for custom resource uid: {}, version: {}", - action, - getUID(customResource), - getVersion(customResource)); - return; - } + "Event received for resource: {}", getName(customResource)); final CustomResourceEventFilter filter = CustomResourceEventFilters.or( CustomResourceEventFilters.finalizerNeededAndApplied(), @@ -126,7 +111,7 @@ public void eventReceived(Watcher.Action action, T customResource) { CustomResourceEventFilters.generationAware())); if (filter.acceptChange(controller.getConfiguration(), oldResource, customResource)) { - eventHandler.handleEvent(new CustomResourceEvent(action, customResource, this)); + eventHandler.handleEvent(new CustomResourceEvent(action, customResource)); } else { log.debug( "Skipping event handling resource {} with version: {}", @@ -136,29 +121,27 @@ public void eventReceived(Watcher.Action action, T customResource) { } @Override - public void onClose(WatcherException e) { - if (e == null) { - return; - } - if (e.isHttpGone()) { - log.warn("Received error for watch, will try to reconnect.", e); - try { - close(); - start(); - } catch (Throwable ex) { - log.error("Unexpected error happened with watch reconnect. Will exit.", e); - System.exit(1); - } - } else { - // Note that this should not happen normally, since fabric8 client handles reconnect. - // In case it tries to reconnect this method is not called. - log.error("Unexpected error happened with watch. Will exit.", e); - System.exit(1); - } + public void onAdd(T resource) { + eventReceived(ResourceAction.ADDED, resource, null); + } + + @Override + public void onUpdate(T oldCustomResource, T newCustomResource) { + eventReceived(ResourceAction.UPDATED, newCustomResource, oldCustomResource); + } + + @Override + public void onDelete(T resource, boolean b) { + eventReceived(ResourceAction.DELETED, resource, null); } - // todo: remove - public CustomResourceCache getCache() { - return customResourceCache; + @Override + public Optional getCustomResource(CustomResourceID resourceID) { + var sharedIndexInformer = + sharedIndexInformers.get(resourceID.getNamespace().orElse(ANY_NAMESPACE_MAP_KEY)); + var resource = sharedIndexInformer.getStore() + .getByKey(Cache.namespaceKeyFunc(resourceID.getNamespace().orElse(null), + resourceID.getName())); + return Optional.ofNullable(resource); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java index ecfcece338..47e00b6900 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java @@ -1,30 +1,20 @@ package io.javaoperatorsdk.operator.processing.event.internal; import java.util.Optional; -import java.util.function.Predicate; -import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; -import io.javaoperatorsdk.operator.processing.event.EventSource; -public class InformerEvent extends DefaultEvent { +public class InformerEvent extends DefaultEvent { - private Action action; + private ResourceAction action; private T resource; private T oldResource; - public InformerEvent(String relatedCustomResourceUid, EventSource eventSource, Action action, - T resource, - T oldResource) { - this(new UIDMatchingPredicate(relatedCustomResourceUid), eventSource, action, resource, - oldResource); - - } - - public InformerEvent(Predicate customResourcesSelector, EventSource eventSource, - Action action, + public InformerEvent(ResourceAction action, T resource, T oldResource) { - super(customResourcesSelector, eventSource); + super(CustomResourceID.fromResource(resource)); this.action = action; this.resource = resource; this.oldResource = oldResource; @@ -38,11 +28,8 @@ public Optional getOldResource() { return Optional.ofNullable(oldResource); } - public Action getAction() { + public ResourceAction getAction() { return action; } - public enum Action { - ADD, UPDATE, DELETE - } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index 37e1add66e..f9d4799ef2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -54,7 +54,7 @@ public InformerEventSource(SharedInformer sharedInformer, sharedInformer.addEventHandler(new ResourceEventHandler<>() { @Override public void onAdd(T t) { - propagateEvent(InformerEvent.Action.ADD, t, null); + propagateEvent(ResourceAction.ADDED, t, null); } @Override @@ -64,23 +64,23 @@ public void onUpdate(T oldObject, T newObject) { .equals(newObject.getMetadata().getResourceVersion())) { return; } - propagateEvent(InformerEvent.Action.UPDATE, newObject, oldObject); + propagateEvent(ResourceAction.UPDATED, newObject, oldObject); } @Override public void onDelete(T t, boolean b) { - propagateEvent(InformerEvent.Action.DELETE, t, null); + propagateEvent(ResourceAction.DELETED, t, null); } }); } - private void propagateEvent(InformerEvent.Action action, T object, T oldObject) { + private void propagateEvent(ResourceAction action, T object, T oldObject) { var uids = resourceToUIDs.apply(object); if (uids.isEmpty()) { return; } uids.forEach(uid -> { - InformerEvent event = new InformerEvent(uid, this, action, object, oldObject); + InformerEvent event = new InformerEvent(action, object, oldObject); this.eventHandler.handleEvent(event); }); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/ResourceAction.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/ResourceAction.java new file mode 100644 index 0000000000..6fbc233133 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/ResourceAction.java @@ -0,0 +1,5 @@ +package io.javaoperatorsdk.operator.processing.event.internal; + +public enum ResourceAction { + ADDED, UPDATED, DELETED +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java index 6fa38674bf..9e9bfb5040 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java @@ -1,10 +1,11 @@ package io.javaoperatorsdk.operator.processing.event.internal; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; public class TimerEvent extends DefaultEvent { - public TimerEvent(String relatedCustomResourceUid, TimerEventSource eventSource) { - super(relatedCustomResourceUid, eventSource); + public TimerEvent(CustomResourceID relatedCustomResourceUid) { + super(relatedCustomResourceUid); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java index 9ad15e47f9..51f21fc4d4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java @@ -11,23 +11,23 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; public class TimerEventSource> extends AbstractEventSource { private static final Logger log = LoggerFactory.getLogger(TimerEventSource.class); private final Timer timer = new Timer(); private final AtomicBoolean running = new AtomicBoolean(); - private final Map onceTasks = new ConcurrentHashMap<>(); - private final Map timerTasks = new ConcurrentHashMap<>(); + private final Map onceTasks = new ConcurrentHashMap<>(); + private final Map timerTasks = new ConcurrentHashMap<>(); public void schedule(R customResource, long delay, long period) { if (!running.get()) { throw new IllegalStateException("The TimerEventSource is not running"); } - String resourceUid = KubernetesResourceUtils.getUID(customResource); + CustomResourceID resourceUid = CustomResourceID.fromResource(customResource); if (timerTasks.containsKey(resourceUid)) { return; } @@ -40,8 +40,7 @@ public void scheduleOnce(R customResource, long delay) { if (!running.get()) { throw new IllegalStateException("The TimerEventSource is not running"); } - - String resourceUid = KubernetesResourceUtils.getUID(customResource); + CustomResourceID resourceUid = CustomResourceID.fromResource(customResource); if (onceTasks.containsKey(resourceUid)) { cancelOnceSchedule(resourceUid); } @@ -51,19 +50,19 @@ public void scheduleOnce(R customResource, long delay) { } @Override - public void eventSourceDeRegisteredForResource(String customResourceUid) { + public void eventSourceDeRegisteredForResource(CustomResourceID customResourceUid) { cancelSchedule(customResourceUid); cancelOnceSchedule(customResourceUid); } - public void cancelSchedule(String customResourceUid) { - TimerTask timerTask = timerTasks.remove(customResourceUid); + public void cancelSchedule(CustomResourceID customResourceID) { + TimerTask timerTask = timerTasks.remove(customResourceID); if (timerTask != null) { timerTask.cancel(); } } - public void cancelOnceSchedule(String customResourceUid) { + public void cancelOnceSchedule(CustomResourceID customResourceUid) { TimerTask timerTask = onceTasks.remove(customResourceUid); if (timerTask != null) { timerTask.cancel(); @@ -85,9 +84,9 @@ public void close() throws IOException { public class EventProducerTimeTask extends TimerTask { - protected final String customResourceUid; + protected final CustomResourceID customResourceUid; - public EventProducerTimeTask(String customResourceUid) { + public EventProducerTimeTask(CustomResourceID customResourceUid) { this.customResourceUid = customResourceUid; } @@ -95,7 +94,7 @@ public EventProducerTimeTask(String customResourceUid) { public void run() { if (running.get()) { log.debug("Producing event for custom resource id: {}", customResourceUid); - eventHandler.handleEvent(new TimerEvent(customResourceUid, TimerEventSource.this)); + eventHandler.handleEvent(new TimerEvent(customResourceUid)); } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/CustomResourceSelectorTest.java deleted file mode 100644 index 33f8347f13..0000000000 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/CustomResourceSelectorTest.java +++ /dev/null @@ -1,110 +0,0 @@ -package io.javaoperatorsdk.operator.processing; - -import java.util.Objects; -import java.util.UUID; - -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.mockito.ArgumentCaptor; - -import io.javaoperatorsdk.operator.processing.event.DefaultEvent; -import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; -import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; - -import static io.javaoperatorsdk.operator.TestUtils.testCustomResource; -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doCallRealMethod; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.timeout; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -class CustomResourceSelectorTest { - - public static final int SEPARATE_EXECUTION_TIMEOUT = 450; - - private final EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); - private final CustomResourceCache customResourceCache = new CustomResourceCache(); - - private final DefaultEventSourceManager defaultEventSourceManagerMock = - mock(DefaultEventSourceManager.class); - - private final DefaultEventHandler defaultEventHandler = - new DefaultEventHandler(eventDispatcherMock, "Test", null); - - @BeforeEach - public void setup() { - defaultEventHandler.setEventSourceManager(defaultEventSourceManagerMock); - - // todo: remove - when(defaultEventSourceManagerMock.getCache()).thenReturn(customResourceCache); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResources(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResourceUids(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).cacheResource(any(), any()); - doAnswer( - invocation -> { - final var resourceId = (String) invocation.getArgument(0); - customResourceCache.cleanup(resourceId); - return null; - }) - .when(defaultEventSourceManagerMock) - .cleanup(any()); - } - - @Test - public void dispatchEventsWithPredicate() { - TestCustomResource cr1 = testCustomResource(UUID.randomUUID().toString()); - cr1.getSpec().setValue("1"); - TestCustomResource cr2 = testCustomResource(UUID.randomUUID().toString()); - cr2.getSpec().setValue("2"); - TestCustomResource cr3 = testCustomResource(UUID.randomUUID().toString()); - cr3.getSpec().setValue("3"); - - customResourceCache.cacheResource(cr1); - customResourceCache.cacheResource(cr2); - customResourceCache.cacheResource(cr3); - - defaultEventHandler.handleEvent( - new DefaultEvent( - c -> { - var tcr = ((TestCustomResource) c); - return Objects.equals("1", tcr.getSpec().getValue()) - || Objects.equals("3", tcr.getSpec().getValue()); - }, - null)); - - verify(eventDispatcherMock, timeout(SEPARATE_EXECUTION_TIMEOUT).times(2)) - .handleExecution(any()); - - waitMinimalTime(); - - ArgumentCaptor executionScopeArgumentCaptor = - ArgumentCaptor.forClass(ExecutionScope.class); - - verify(eventDispatcherMock, timeout(SEPARATE_EXECUTION_TIMEOUT).times(2)) - .handleExecution(executionScopeArgumentCaptor.capture()); - - assertThat(executionScopeArgumentCaptor.getAllValues()) - .hasSize(2) - .allSatisfy( - s -> { - assertThat(s.getEvents()).isNotEmpty().hasOnlyElementsOfType(DefaultEvent.class); - assertThat(s) - .satisfiesAnyOf( - e -> Objects.equals(cr1.getMetadata().getUid(), e.getCustomResourceUid()), - e -> Objects.equals(cr3.getMetadata().getUid(), e.getCustomResourceUid())); - }); - } - - private void waitMinimalTime() { - try { - Thread.sleep(1000); - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } - } - -} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 591c7fa1e0..546aaa828d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -12,6 +12,7 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.client.Watcher; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; @@ -232,7 +233,7 @@ private void waitMinimalTime() { } } - private String eventAlreadyUnderProcessing() { + private CustomResourceID eventAlreadyUnderProcessing() { when(eventDispatcherMock.handleExecution(any())) .then( (Answer) invocationOnMock -> { @@ -241,7 +242,7 @@ private String eventAlreadyUnderProcessing() { }); Event event = prepareCREvent(); defaultEventHandler.handleEvent(event); - return event.getRelatedCustomResourceUid(); + return event.getRelatedCustomResourceID(); } private CustomResourceEvent prepareCREvent() { @@ -251,11 +252,11 @@ private CustomResourceEvent prepareCREvent() { private CustomResourceEvent prepareCREvent(String uid) { TestCustomResource customResource = testCustomResource(uid); customResourceCache.cacheResource(customResource); - return new CustomResourceEvent(Watcher.Action.MODIFIED, customResource, null); + return new CustomResourceEvent(customResource); } - private Event nonCREvent(String relatedCustomResourceUid) { - TimerEvent timerEvent = new TimerEvent(relatedCustomResourceUid, null); + private Event nonCREvent(CustomResourceID relatedCustomResourceUid) { + TimerEvent timerEvent = new TimerEvent(relatedCustomResourceUid); return timerEvent; } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java index c9b414f52b..6dc16f8a69 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java @@ -5,6 +5,7 @@ import org.junit.jupiter.api.Test; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.TimerEvent; @@ -14,17 +15,18 @@ class EventBufferTest { private EventBuffer eventBuffer = new EventBuffer(); - String uid = UUID.randomUUID().toString(); - Event testEvent1 = new TimerEvent(uid, null); - Event testEvent2 = new TimerEvent(uid, null); + String name = UUID.randomUUID().toString(); + CustomResourceID customResourceID = new CustomResourceID(name); + Event testEvent1 = new TimerEvent(customResourceID); + Event testEvent2 = new TimerEvent(customResourceID); @Test public void storesEvents() { eventBuffer.addEvent(testEvent1); eventBuffer.addEvent(testEvent2); - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceUid())).isTrue(); - List events = eventBuffer.getAndRemoveEventsForExecution(uid); + assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isTrue(); + List events = eventBuffer.getAndRemoveEventsForExecution(customResourceID); assertThat(events).hasSize(2); } @@ -33,7 +35,7 @@ public void getsAndRemovesEvents() { eventBuffer.addEvent(testEvent1); eventBuffer.addEvent(testEvent2); - List events = eventBuffer.getAndRemoveEventsForExecution(uid); + List events = eventBuffer.getAndRemoveEventsForExecution(new CustomResourceID(name)); assertThat(events).hasSize(2); assertThat(events).contains(testEvent1, testEvent2); } @@ -43,7 +45,7 @@ public void checksIfThereAreStoredEvents() { eventBuffer.addEvent(testEvent1); eventBuffer.addEvent(testEvent2); - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceUid())).isTrue(); + assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isTrue(); } @Test @@ -51,8 +53,8 @@ public void canClearEvents() { eventBuffer.addEvent(testEvent1); eventBuffer.addEvent(testEvent2); - eventBuffer.cleanup(uid); + eventBuffer.cleanup(customResourceID); - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceUid())).isFalse(); + assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isFalse(); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index 2f9de4de82..352665d769 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -22,6 +22,7 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; +import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -72,7 +73,7 @@ void setup() { void addFinalizerOnNewResource() { assertFalse(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.ADDED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); verify(controller, never()) .createOrUpdateResource(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) @@ -85,7 +86,7 @@ void addFinalizerOnNewResource() { void callCreateOrUpdateOnNewResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.ADDED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); verify(controller, times(1)) .createOrUpdateResource(ArgumentMatchers.eq(testCustomResource), any()); } @@ -113,7 +114,7 @@ void updatesBothResourceAndStatusIfFinalizerSet() { when(customResourceFacade.replaceWithLock(testCustomResource)).thenReturn(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(customResourceFacade, times(1)).replaceWithLock(testCustomResource); verify(customResourceFacade, times(1)).updateStatus(testCustomResource); @@ -124,7 +125,7 @@ void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(controller, times(1)) .createOrUpdateResource(ArgumentMatchers.eq(testCustomResource), any()); } @@ -136,7 +137,7 @@ void callsDeleteIfObjectHasFinalizerAndMarkedForDelete() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(controller, times(1)).deleteResource(eq(testCustomResource), any()); } @@ -148,7 +149,7 @@ void callDeleteOnControllerIfMarkedForDeletionWhenNoFinalizerIsConfigured() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(controller).deleteResource(eq(testCustomResource), any()); } @@ -158,7 +159,7 @@ void doNotCallDeleteIfMarkedForDeletionWhenFinalizerHasAlreadyBeenRemoved() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(controller, never()).deleteResource(eq(testCustomResource), any()); } @@ -178,7 +179,7 @@ void doesNotAddFinalizerIfConfiguredNotTo() { configureToNotUseFinalizer(); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); assertEquals(0, testCustomResource.getMetadata().getFinalizers().size()); } @@ -189,7 +190,7 @@ void removesDefaultFinalizerOnDeleteIfSet() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); assertEquals(0, testCustomResource.getMetadata().getFinalizers().size()); verify(customResourceFacade, times(1)).replaceWithLock(any()); @@ -204,7 +205,7 @@ void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); verify(customResourceFacade, never()).replaceWithLock(any()); @@ -218,7 +219,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { .thenReturn(UpdateControl.noUpdate()); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(customResourceFacade, never()).replaceWithLock(any()); verify(customResourceFacade, never()).updateStatus(testCustomResource); } @@ -230,7 +231,7 @@ void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { .thenReturn(UpdateControl.noUpdate()); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); verify(customResourceFacade, times(1)).replaceWithLock(any()); @@ -242,7 +243,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { markForDeletion(testCustomResource); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(customResourceFacade, never()).replaceWithLock(any()); verify(controller, never()).deleteResource(eq(testCustomResource), any()); @@ -252,9 +253,9 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { void executeControllerRegardlessGenerationInNonGenerationAwareModeIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource)); verify(controller, times(2)).createOrUpdateResource(eq(testCustomResource), any()); } @@ -298,7 +299,7 @@ void setReScheduleToPostExecutionControlFromUpdateControl() { UpdateControl.updateStatusSubResource(testCustomResource).withReSchedule(1000l)); PostExecutionControl control = eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.ADDED, testCustomResource)); + executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); assertThat(control.getReScheduleDelay().get()).isEqualTo(1000l); } @@ -312,8 +313,8 @@ private void removeFinalizers(CustomResource customResource) { } public ExecutionScope executionScopeWithCREvent( - Watcher.Action action, CustomResource resource, Event... otherEvents) { - CustomResourceEvent event = new CustomResourceEvent(action, resource, null); + ResourceAction action, CustomResource resource, Event... otherEvents) { + CustomResourceEvent event = new CustomResourceEvent(action, resource); List eventList = new ArrayList<>(1 + otherEvents.length); eventList.add(event); eventList.addAll(Arrays.asList(otherEvents)); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java index 8862450d06..e594283c5c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java @@ -8,9 +8,7 @@ import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; -import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.Mockito.eq; @@ -73,9 +71,9 @@ public void deRegistersEventSources() { defaultEventSourceManager.registerEventSource(CUSTOM_EVENT_SOURCE_NAME, eventSource); defaultEventSourceManager.deRegisterCustomResourceFromEventSource( - CUSTOM_EVENT_SOURCE_NAME, getUID(customResource)); + CUSTOM_EVENT_SOURCE_NAME, CustomResourceID.fromResource(customResource)); verify(eventSource, times(1)) - .eventSourceDeRegisteredForResource(eq(KubernetesResourceUtils.getUID(customResource))); + .eventSourceDeRegisteredForResource(eq(CustomResourceID.fromResource(customResource))); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java index 727caf3be1..ecb21a2c8c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java @@ -44,7 +44,7 @@ public void skipsEventHandlingIfGenerationNotIncreased() { TestCustomResource customResource1 = TestUtils.testCustomResource(); customResource1.getMetadata().setFinalizers(List.of(FINALIZER)); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(customResource1, customResource1); verify(eventHandler, times(1)).handleEvent(any()); customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); From b2ab4b9ba935a33ac33bc3a11e89bfebbbc5fbfd Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 11:30:49 +0200 Subject: [PATCH 03/40] Build is fixed, (test failing) --- .../javaoperatorsdk/operator/TestUtils.java | 21 ++++++ .../processing/DefaultEventHandlerTest.java | 64 +++++++++++-------- .../processing/EventDispatcherTest.java | 10 +-- .../processing/event/EventListTest.java | 5 +- .../CustomResourceEventFilterTest.java | 13 ++-- .../CustomResourceEventSourceTest.java | 30 +++++---- .../event/internal/TimerEventSourceTest.java | 17 ++--- 7 files changed, 99 insertions(+), 61 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java index 93e6e57492..d0b4e31ea2 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java @@ -6,6 +6,7 @@ import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition; import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinitionBuilder; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceSpec; @@ -29,6 +30,7 @@ public static CustomResourceDefinition testCRD(String scope) { .build(); } + // todo remove? public static TestCustomResource testCustomResource(String uid) { TestCustomResource resource = new TestCustomResource(); resource.setMetadata( @@ -46,4 +48,23 @@ public static TestCustomResource testCustomResource(String uid) { resource.getSpec().setValue("test-value"); return resource; } + + public static TestCustomResource testCustomResource(CustomResourceID id) { + TestCustomResource resource = new TestCustomResource(); + resource.setMetadata( + new ObjectMetaBuilder() + .withName(id.getName()) + .withGeneration(1L) + .withNamespace(id.getNamespace().orElse(null)) + .build()); + resource.getMetadata().setAnnotations(new HashMap<>()); + resource.setKind("CustomService"); + resource.setSpec(new TestCustomResourceSpec()); + resource.getSpec().setConfigMapName("test-config-map"); + resource.getSpec().setKey("test-key"); + resource.getSpec().setValue("test-value"); + return resource; + } + + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 546aaa828d..5f0773fd56 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -11,17 +11,19 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.client.Watcher; +import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; +import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; import io.javaoperatorsdk.operator.processing.event.internal.TimerEvent; import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource; import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.TestUtils.testCustomResource; +import static io.javaoperatorsdk.operator.processing.event.internal.ResourceAction.DELETED; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; @@ -32,6 +34,7 @@ class DefaultEventHandlerTest { public static final int FAKE_CONTROLLER_EXECUTION_DURATION = 250; public static final int SEPARATE_EXECUTION_TIMEOUT = 450; + public static final String TEST_NAMESPACE = "default-event-handler-test"; private EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); private CustomResourceCache customResourceCache = new CustomResourceCache(); private DefaultEventSourceManager defaultEventSourceManagerMock = @@ -53,21 +56,21 @@ public void setup() { defaultEventHandler.setEventSourceManager(defaultEventSourceManagerMock); defaultEventHandlerWithRetry.setEventSourceManager(defaultEventSourceManagerMock); - // todo: remove - when(defaultEventSourceManagerMock.getCache()).thenReturn(customResourceCache); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResources(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResourceUids(any()); - doCallRealMethod().when(defaultEventSourceManagerMock).cacheResource(any(), any()); - doAnswer( - invocation -> { - final var resourceId = (String) invocation.getArgument(0); - customResourceCache.cleanup(resourceId); - return null; - }) - .when(defaultEventSourceManagerMock) - .cleanup(any()); + // // todo: remove + // when(defaultEventSourceManagerMock.getCache()).thenReturn(customResourceCache); + // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); + // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); + // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResources(any()); + // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResourceUids(any()); + // doCallRealMethod().when(defaultEventSourceManagerMock).cacheResource(any(), any()); + // doAnswer( + // invocation -> { + // final var resourceId = (String) invocation.getArgument(0); + // customResourceCache.cleanup(resourceId); + // return null; + // }) + // .when(defaultEventSourceManagerMock) + // .cleanup(any()); } @Test @@ -80,7 +83,7 @@ public void dispatchesEventsIfNoExecutionInProgress() { @Test public void skipProcessingIfLatestCustomResourceNotInCache() { Event event = prepareCREvent(); - customResourceCache.cleanup(event.getRelatedCustomResourceUid()); + // customResourceCache.cleanup(event.getRelatedCustomResourceID())); defaultEventHandler.handleEvent(event); @@ -89,7 +92,7 @@ public void skipProcessingIfLatestCustomResourceNotInCache() { @Test public void ifExecutionInProgressWaitsUntilItsFinished() throws InterruptedException { - String resourceUid = eventAlreadyUnderProcessing(); + CustomResourceID resourceUid = eventAlreadyUnderProcessing(); defaultEventHandler.handleEvent(nonCREvent(resourceUid)); @@ -99,7 +102,7 @@ public void ifExecutionInProgressWaitsUntilItsFinished() throws InterruptedExcep @Test public void buffersAllIncomingEventsWhileControllerInExecution() { - String resourceUid = eventAlreadyUnderProcessing(); + CustomResourceID resourceUid = eventAlreadyUnderProcessing(); defaultEventHandler.handleEvent(nonCREvent(resourceUid)); defaultEventHandler.handleEvent(prepareCREvent(resourceUid)); @@ -118,15 +121,14 @@ public void cleanUpAfterDeleteEvent() { TestCustomResource customResource = testCustomResource(); customResourceCache.cacheResource(customResource); CustomResourceEvent event = - new CustomResourceEvent(Watcher.Action.DELETED, customResource, null); - String uid = customResource.getMetadata().getUid(); + new CustomResourceEvent(DELETED, customResource); defaultEventHandler.handleEvent(event); waitMinimalTime(); - verify(defaultEventSourceManagerMock, times(1)).cleanup(uid); - assertThat(customResourceCache.getLatestResource(uid)).isNotPresent(); + verify(defaultEventSourceManagerMock, times(1)) + .cleanup(CustomResourceID.fromResource(customResource)); } @Test @@ -148,7 +150,7 @@ public void schedulesAnEventRetryOnException() { public void executesTheControllerInstantlyAfterErrorIfEventsBuffered() { Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); - customResource.getMetadata().setUid(event.getRelatedCustomResourceUid()); + overrideData(event.getRelatedCustomResourceID(), customResource); ExecutionScope executionScope = new ExecutionScope(Arrays.asList(event), customResource, null); PostExecutionControl postExecutionControl = PostExecutionControl.exceptionDuringExecution(new RuntimeException("test")); @@ -179,7 +181,7 @@ public void successfulExecutionResetsTheRetry() { Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); - customResource.getMetadata().setUid(event.getRelatedCustomResourceUid()); + overrideData(event.getRelatedCustomResourceID(), customResource); PostExecutionControl postExecutionControlWithException = PostExecutionControl.exceptionDuringExecution(new RuntimeException("test")); PostExecutionControl defaultDispatchControl = PostExecutionControl.defaultDispatch(); @@ -246,17 +248,23 @@ private CustomResourceID eventAlreadyUnderProcessing() { } private CustomResourceEvent prepareCREvent() { - return prepareCREvent(UUID.randomUUID().toString()); + return prepareCREvent(new CustomResourceID(UUID.randomUUID().toString(), TEST_NAMESPACE)); } - private CustomResourceEvent prepareCREvent(String uid) { + private CustomResourceEvent prepareCREvent(CustomResourceID uid) { TestCustomResource customResource = testCustomResource(uid); customResourceCache.cacheResource(customResource); - return new CustomResourceEvent(customResource); + return new CustomResourceEvent(ResourceAction.UPDATED, customResource); } private Event nonCREvent(CustomResourceID relatedCustomResourceUid) { TimerEvent timerEvent = new TimerEvent(relatedCustomResourceUid); return timerEvent; } + + private void overrideData(CustomResourceID id, CustomResource applyTo) { + applyTo.getMetadata().setName(id.getName()); + applyTo.getMetadata().setNamespace(id.getNamespace().orElse(null)); + } + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index 352665d769..b1e3d015eb 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -10,7 +10,6 @@ import org.mockito.ArgumentMatchers; import io.fabric8.kubernetes.client.CustomResource; -import io.fabric8.kubernetes.client.Watcher; import io.javaoperatorsdk.operator.Metrics; import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.api.Context; @@ -24,6 +23,7 @@ import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; +import static io.javaoperatorsdk.operator.processing.event.internal.ResourceAction.ADDED; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -73,7 +73,7 @@ void setup() { void addFinalizerOnNewResource() { assertFalse(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)); eventDispatcher.handleExecution( - executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); + executionScopeWithCREvent(ADDED, testCustomResource)); verify(controller, never()) .createOrUpdateResource(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) @@ -86,7 +86,7 @@ void addFinalizerOnNewResource() { void callCreateOrUpdateOnNewResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); eventDispatcher.handleExecution( - executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); + executionScopeWithCREvent(ADDED, testCustomResource)); verify(controller, times(1)) .createOrUpdateResource(ArgumentMatchers.eq(testCustomResource), any()); } @@ -99,7 +99,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { .thenReturn(UpdateControl.updateStatusSubResource(testCustomResource)); eventDispatcher.handleExecution( - executionScopeWithCREvent(Watcher.Action.ADDED, testCustomResource)); + executionScopeWithCREvent(ADDED, testCustomResource)); verify(customResourceFacade, times(1)).updateStatus(testCustomResource); verify(customResourceFacade, never()).replaceWithLock(any()); @@ -299,7 +299,7 @@ void setReScheduleToPostExecutionControlFromUpdateControl() { UpdateControl.updateStatusSubResource(testCustomResource).withReSchedule(1000l)); PostExecutionControl control = eventDispatcher.handleExecution( - executionScopeWithCREvent(ResourceAction.ADDED, testCustomResource)); + executionScopeWithCREvent(ADDED, testCustomResource)); assertThat(control.getReScheduleDelay().get()).isEqualTo(1000l); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java index 2f3dba65a5..dc65981cff 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java @@ -13,10 +13,11 @@ class EventListTest { @Test public void returnsLatestOfEventType() { - TimerEvent event2 = new TimerEvent("1", null); + TimerEvent event2 = new TimerEvent(new CustomResourceID("name1")); EventList eventList = new EventList( - Arrays.asList(mock(Event.class), new TimerEvent("2", null), event2, mock(Event.class))); + Arrays.asList(mock(Event.class), new TimerEvent(new CustomResourceID("name2")), event2, + mock(Event.class))); assertThat(eventList.getLatestOfType(TimerEvent.class).get()).isEqualTo(event2); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java index ab41b833ff..58956ee853 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java @@ -7,7 +7,6 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.KubernetesResourceList; -import io.fabric8.kubernetes.client.Watcher; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.javaoperatorsdk.operator.TestUtils; @@ -52,13 +51,13 @@ public void eventFilteredByCustomPredicate() { cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(1)).handleEvent(any()); cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(1)).handleEvent(any()); } @@ -80,13 +79,13 @@ public void eventFilteredByCustomPredicateAndGenerationAware() { cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(1)).handleEvent(any()); cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("2"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(1)).handleEvent(any()); } @@ -112,13 +111,13 @@ public void eventNotFilteredByCustomPredicateIfFinalizerIsRequired() { cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(1)).handleEvent(any()); cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(Watcher.Action.MODIFIED, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); verify(eventHandler, times(2)).handleEvent(any()); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java index ecb21a2c8c..81d1d235a3 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java @@ -7,7 +7,6 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.KubernetesResourceList; -import io.fabric8.kubernetes.client.Watcher; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.javaoperatorsdk.operator.Metrics; @@ -44,10 +43,11 @@ public void skipsEventHandlingIfGenerationNotIncreased() { TestCustomResource customResource1 = TestUtils.testCustomResource(); customResource1.getMetadata().setFinalizers(List.of(FINALIZER)); - customResourceEventSource.eventReceived(customResource1, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, null); verify(eventHandler, times(1)).handleEvent(any()); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(1)).handleEvent(any()); } @@ -55,12 +55,14 @@ public void skipsEventHandlingIfGenerationNotIncreased() { public void dontSkipEventHandlingIfMarkedForDeletion() { TestCustomResource customResource1 = TestUtils.testCustomResource(); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(1)).handleEvent(any()); // mark for deletion customResource1.getMetadata().setDeletionTimestamp(LocalDateTime.now().toString()); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(2)).handleEvent(any()); } @@ -68,11 +70,13 @@ public void dontSkipEventHandlingIfMarkedForDeletion() { public void normalExecutionIfGenerationChanges() { TestCustomResource customResource1 = TestUtils.testCustomResource(); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(1)).handleEvent(any()); customResource1.getMetadata().setGeneration(2L); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(2)).handleEvent(any()); } @@ -84,10 +88,12 @@ public void handlesAllEventIfNotGenerationAware() { TestCustomResource customResource1 = TestUtils.testCustomResource(); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(1)).handleEvent(any()); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(2)).handleEvent(any()); } @@ -95,10 +101,12 @@ public void handlesAllEventIfNotGenerationAware() { public void eventNotMarkedForLastGenerationIfNoFinalizer() { TestCustomResource customResource1 = TestUtils.testCustomResource(); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(1)).handleEvent(any()); - customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1); + customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, + customResource1); verify(eventHandler, times(2)).handleEvent(any()); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSourceTest.java index 892c422fc1..f7f2814255 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSourceTest.java @@ -12,12 +12,11 @@ import org.junit.jupiter.api.Test; import io.javaoperatorsdk.operator.TestUtils; -import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -48,9 +47,9 @@ public void producesEventsPeriodically() { assertThat(eventHandlerMock.events) .hasSizeGreaterThan(2); assertThat(eventHandlerMock.events) - .allMatch(e -> e.getRelatedCustomResourceUid().equals(getUID(customResource))); - assertThat(eventHandlerMock.events) - .allMatch(e -> e.getEventSource().equals(timerEventSource)); + .allMatch(e -> e.getRelatedCustomResourceID() + .equals(CustomResourceID.fromResource(customResource))); + }); } @@ -61,7 +60,8 @@ public void deRegistersPeriodicalEventSources() { timerEventSource.schedule(customResource, INITIAL_DELAY, PERIOD); untilAsserted(() -> assertThat(eventHandlerMock.events).hasSizeGreaterThan(1)); - timerEventSource.eventSourceDeRegisteredForResource(getUID(customResource)); + timerEventSource + .eventSourceDeRegisteredForResource(CustomResourceID.fromResource(customResource)); int size = eventHandlerMock.events.size(); untilAsserted(() -> assertThat(eventHandlerMock.events).hasSize(size)); @@ -82,7 +82,7 @@ public void canCancelOnce() { TestCustomResource customResource = TestUtils.testCustomResource(); timerEventSource.scheduleOnce(customResource, PERIOD); - timerEventSource.cancelOnceSchedule(KubernetesResourceUtils.getUID(customResource)); + timerEventSource.cancelOnceSchedule(CustomResourceID.fromResource(customResource)); untilAsserted(() -> assertThat(eventHandlerMock.events).isEmpty()); } @@ -102,7 +102,8 @@ public void deRegistersOnceEventSources() { TestCustomResource customResource = TestUtils.testCustomResource(); timerEventSource.scheduleOnce(customResource, PERIOD); - timerEventSource.eventSourceDeRegisteredForResource(getUID(customResource)); + timerEventSource + .eventSourceDeRegisteredForResource(CustomResourceID.fromResource(customResource)); untilAsserted(() -> assertThat(eventHandlerMock.events).isEmpty()); } From 9145b52d72ee2afb17eea16193e7233fe39a3785 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 14:48:35 +0200 Subject: [PATCH 04/40] Test fixes --- .../javaoperatorsdk/operator/processing/event/DefaultEvent.java | 2 +- .../operator/processing/DefaultEventHandlerTest.java | 1 - pom.xml | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java index 75d63c4174..c445f2bf27 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java @@ -12,7 +12,7 @@ public DefaultEvent(CustomResourceID targetCustomResource) { @Override public CustomResourceID getRelatedCustomResourceID() { - return null; + return relatedCustomResource; } @Override diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 5f0773fd56..e510703956 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -36,7 +36,6 @@ class DefaultEventHandlerTest { public static final int SEPARATE_EXECUTION_TIMEOUT = 450; public static final String TEST_NAMESPACE = "default-event-handler-test"; private EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); - private CustomResourceCache customResourceCache = new CustomResourceCache(); private DefaultEventSourceManager defaultEventSourceManagerMock = mock(DefaultEventSourceManager.class); diff --git a/pom.xml b/pom.xml index baa171c9fd..3047d7d17f 100644 --- a/pom.xml +++ b/pom.xml @@ -362,6 +362,7 @@ release + org.apache.maven.plugins maven-surefire-plugin From 34cc2a1a261f045309a613e85b26e541299c2979 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 14:59:11 +0200 Subject: [PATCH 05/40] minor update --- .../operator/processing/DefaultEventHandlerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index e510703956..099551c08a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -118,7 +118,8 @@ public void buffersAllIncomingEventsWhileControllerInExecution() { @Test public void cleanUpAfterDeleteEvent() { TestCustomResource customResource = testCustomResource(); - customResourceCache.cacheResource(customResource); +// todo +// customResourceCache.cacheResource(customResource); CustomResourceEvent event = new CustomResourceEvent(DELETED, customResource); From 4b15974b8000ce4ba87ee79f0cd296eb9a7c88b5 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 15:51:03 +0200 Subject: [PATCH 06/40] EventSourceManager small fix --- .../operator/processing/ConfiguredController.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java index dbac8b4d03..d34317fdb4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java @@ -27,7 +27,7 @@ public class ConfiguredController> implements Res private final ResourceController controller; private final ControllerConfiguration configuration; private final KubernetesClient k8sClient; - private EventSourceManager manager; + private EventSourceManager eventSourceManager; public ConfiguredController(ResourceController controller, ControllerConfiguration configuration, @@ -174,7 +174,7 @@ public void start() throws OperatorException { } try { - DefaultEventSourceManager eventSourceManager = new DefaultEventSourceManager<>(this); + eventSourceManager = new DefaultEventSourceManager<>(this); controller.init(eventSourceManager); } catch (MissingCRDException e) { throwMissingCRDException(crdName, specVersion, controllerName); @@ -217,11 +217,14 @@ private boolean failOnMissingCurrentNS() { return false; } + public EventSourceManager getEventSourceManager() { + return eventSourceManager; + } @Override public void close() throws IOException { - if (manager != null) { - manager.close(); + if (eventSourceManager != null) { + eventSourceManager.close(); } } } From e1b5926212cb2ccf80990f7cc3eb30f543a55880 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 16:58:43 +0200 Subject: [PATCH 07/40] Unit tests fixed --- .../processing/DefaultEventHandler.java | 20 +++++++++++++------ .../event/DefaultEventSourceManager.java | 17 ++++++++-------- .../processing/DefaultEventHandlerTest.java | 17 ++++++++-------- .../CustomResourceEventFilterTest.java | 4 ++-- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index e0e78acb10..398d0cce77 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -27,6 +27,7 @@ import static io.javaoperatorsdk.operator.EventListUtils.containsCustomResourceDeletedEvent; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; +import static io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager.CUSTOM_RESOURCE_EVENT_SOURCE_NAME; /** * Event handler that makes sure that events are processed in a "single threaded" way per resource @@ -51,21 +52,27 @@ public void failedEvent(CustomResourceID uid, Event event) {} private final ExecutorService executor; private final String controllerName; private final ReentrantLock lock = new ReentrantLock(); + private final ResourceCache resourceCache; private DefaultEventSourceManager eventSourceManager; public DefaultEventHandler(ConfiguredController controller) { - this(ExecutorServiceManager.instance().executorService(), + this( + (ResourceCache) controller.getEventSourceManager() + .getRegisteredEventSources().get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME), + ExecutorServiceManager.instance().executorService(), controller.getConfiguration().getName(), new EventDispatcher<>(controller), GenericRetry.fromConfiguration(controller.getConfiguration().getRetryConfiguration())); } - DefaultEventHandler(EventDispatcher eventDispatcher, String relatedControllerName, + DefaultEventHandler(EventDispatcher eventDispatcher, ResourceCache resourceCache, + String relatedControllerName, Retry retry) { - this(null, relatedControllerName, eventDispatcher, retry); + this(resourceCache, null, relatedControllerName, eventDispatcher, retry); } - private DefaultEventHandler(ExecutorService executor, String relatedControllerName, + private DefaultEventHandler(ResourceCache resourceCache, ExecutorService executor, + String relatedControllerName, EventDispatcher eventDispatcher, Retry retry) { this.executor = executor == null @@ -75,6 +82,7 @@ private DefaultEventHandler(ExecutorService executor, String relatedControllerNa this.controllerName = relatedControllerName; this.eventDispatcher = eventDispatcher; this.retry = retry; + this.resourceCache = resourceCache; eventBuffer = new EventBuffer(); } @@ -108,8 +116,8 @@ public void handleEvent(Event event) { private void executeBufferedEvents(CustomResourceID customResourceUid) { boolean newEventForResourceId = eventBuffer.containsEvents(customResourceUid); boolean controllerUnderExecution = isControllerUnderExecution(customResourceUid); - Optional latestCustomResource = - eventSourceManager.getCache().getCustomResource(customResourceUid); + Optional latestCustomResource = + resourceCache.getCustomResource(customResourceUid); if (!controllerUnderExecution && newEventForResourceId && latestCustomResource.isPresent()) { setUnderExecutionProcessing(customResourceUid); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 8d062b3c00..14b9fed8cf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -16,7 +16,6 @@ import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.processing.ConfiguredController; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; -import io.javaoperatorsdk.operator.processing.ResourceCache; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEventSource; import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource; @@ -24,7 +23,7 @@ public class DefaultEventSourceManager> implements EventSourceManager { public static final String RETRY_TIMER_EVENT_SOURCE_NAME = "retry-timer-event-source"; - private static final String CUSTOM_RESOURCE_EVENT_SOURCE_NAME = "custom-resource-event-source"; + public static final String CUSTOM_RESOURCE_EVENT_SOURCE_NAME = "custom-resource-event-source"; private static final Logger log = LoggerFactory.getLogger(DefaultEventSourceManager.class); private final ReentrantLock lock = new ReentrantLock(); @@ -146,13 +145,13 @@ public void cleanup(CustomResourceID customResourceUid) { .forEach(k -> deRegisterCustomResourceFromEventSource(k, customResourceUid)); } - // todo: remove - public ResourceCache getCache() { - final var source = - (CustomResourceEventSource) getRegisteredEventSources() - .get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME); - return source; - } + // // todo: remove + // public ResourceCache getCache() { + // final var source = + // (CustomResourceEventSource) getRegisteredEventSources() + // .get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME); + // return source; + // } // // todo: remove // public Optional getLatestResource(String customResourceUid) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 099551c08a..8cb6fd7633 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -2,6 +2,7 @@ import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.UUID; import org.junit.jupiter.api.BeforeEach; @@ -38,14 +39,15 @@ class DefaultEventHandlerTest { private EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); private DefaultEventSourceManager defaultEventSourceManagerMock = mock(DefaultEventSourceManager.class); + private ResourceCache resourceCache = mock(ResourceCache.class); private TimerEventSource retryTimerEventSourceMock = mock(TimerEventSource.class); private DefaultEventHandler defaultEventHandler = - new DefaultEventHandler(eventDispatcherMock, "Test", null); + new DefaultEventHandler(eventDispatcherMock, resourceCache, "Test", null); private DefaultEventHandler defaultEventHandlerWithRetry = - new DefaultEventHandler(eventDispatcherMock, "Test", + new DefaultEventHandler(eventDispatcherMock, resourceCache, "Test", GenericRetry.defaultLimitedExponentialRetry()); @BeforeEach @@ -82,7 +84,8 @@ public void dispatchesEventsIfNoExecutionInProgress() { @Test public void skipProcessingIfLatestCustomResourceNotInCache() { Event event = prepareCREvent(); - // customResourceCache.cleanup(event.getRelatedCustomResourceID())); + when(resourceCache.getCustomResource(event.getRelatedCustomResourceID())) + .thenReturn(Optional.empty()); defaultEventHandler.handleEvent(event); @@ -118,15 +121,14 @@ public void buffersAllIncomingEventsWhileControllerInExecution() { @Test public void cleanUpAfterDeleteEvent() { TestCustomResource customResource = testCustomResource(); -// todo -// customResourceCache.cacheResource(customResource); + when(resourceCache.getCustomResource(CustomResourceID.fromResource(customResource))) + .thenReturn(Optional.of(customResource)); CustomResourceEvent event = new CustomResourceEvent(DELETED, customResource); defaultEventHandler.handleEvent(event); waitMinimalTime(); - verify(defaultEventSourceManagerMock, times(1)) .cleanup(CustomResourceID.fromResource(customResource)); } @@ -151,7 +153,6 @@ public void executesTheControllerInstantlyAfterErrorIfEventsBuffered() { Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); overrideData(event.getRelatedCustomResourceID(), customResource); - ExecutionScope executionScope = new ExecutionScope(Arrays.asList(event), customResource, null); PostExecutionControl postExecutionControl = PostExecutionControl.exceptionDuringExecution(new RuntimeException("test")); @@ -253,7 +254,7 @@ private CustomResourceEvent prepareCREvent() { private CustomResourceEvent prepareCREvent(CustomResourceID uid) { TestCustomResource customResource = testCustomResource(uid); - customResourceCache.cacheResource(customResource); + when(resourceCache.getCustomResource(eq(uid))).thenReturn(Optional.of(customResource)); return new CustomResourceEvent(ResourceAction.UPDATED, customResource); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java index 58956ee853..bec0ff4c99 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java @@ -51,7 +51,7 @@ public void eventFilteredByCustomPredicate() { cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, null); verify(eventHandler, times(1)).handleEvent(any()); cr.getMetadata().setGeneration(1L); @@ -79,7 +79,7 @@ public void eventFilteredByCustomPredicateAndGenerationAware() { cr.getMetadata().setGeneration(1L); cr.getStatus().setConfigMapStatus("1"); - eventSource.eventReceived(ResourceAction.UPDATED, cr, cr); + eventSource.eventReceived(ResourceAction.UPDATED, cr, null); verify(eventHandler, times(1)).handleEvent(any()); cr.getMetadata().setGeneration(1L); From e7d1b99784fa1bd78e5220335377c601a887dbc8 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 17:37:21 +0200 Subject: [PATCH 08/40] fix: DefaultEventHandler init from EventSourceManager --- .../processing/DefaultEventHandler.java | 6 ++--- .../event/DefaultEventSourceManager.java | 27 +++++++++++-------- .../event/DefaultEventSourceManagerTest.java | 4 +-- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 398d0cce77..313e4a48de 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -27,7 +27,6 @@ import static io.javaoperatorsdk.operator.EventListUtils.containsCustomResourceDeletedEvent; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; -import static io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager.CUSTOM_RESOURCE_EVENT_SOURCE_NAME; /** * Event handler that makes sure that events are processed in a "single threaded" way per resource @@ -55,10 +54,9 @@ public void failedEvent(CustomResourceID uid, Event event) {} private final ResourceCache resourceCache; private DefaultEventSourceManager eventSourceManager; - public DefaultEventHandler(ConfiguredController controller) { + public DefaultEventHandler(ConfiguredController controller, ResourceCache resourceCache) { this( - (ResourceCache) controller.getEventSourceManager() - .getRegisteredEventSources().get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME), + resourceCache, ExecutorServiceManager.instance().executorService(), controller.getConfiguration().getName(), new EventDispatcher<>(controller), diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 14b9fed8cf..f623beac2a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -28,22 +28,27 @@ public class DefaultEventSourceManager> private final ReentrantLock lock = new ReentrantLock(); private final Map eventSources = new ConcurrentHashMap<>(); - private final DefaultEventHandler defaultEventHandler; + private DefaultEventHandler defaultEventHandler; private TimerEventSource retryTimerEventSource; - DefaultEventSourceManager(DefaultEventHandler defaultEventHandler, boolean supportRetry) { - this.defaultEventHandler = defaultEventHandler; - defaultEventHandler.setEventSourceManager(this); - if (supportRetry) { - this.retryTimerEventSource = new TimerEventSource<>(); - registerEventSource(RETRY_TIMER_EVENT_SOURCE_NAME, retryTimerEventSource); - } + DefaultEventSourceManager(DefaultEventHandler defaultEventHandler) { + init(defaultEventHandler); } public DefaultEventSourceManager(ConfiguredController controller) { - this(new DefaultEventHandler<>(controller), true); - registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, - new CustomResourceEventSource<>(controller)); + CustomResourceEventSource customResourceEventSource = + new CustomResourceEventSource<>(controller); + registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, customResourceEventSource); + init(new DefaultEventHandler<>(controller, customResourceEventSource)); + } + + private void init(DefaultEventHandler defaultEventHandler) { + this.defaultEventHandler = defaultEventHandler; + defaultEventHandler.setEventSourceManager(this); + + this.retryTimerEventSource = new TimerEventSource<>(); + registerEventSource(RETRY_TIMER_EVENT_SOURCE_NAME, retryTimerEventSource); + } @Override diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java index e594283c5c..275af74b10 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java @@ -22,7 +22,7 @@ class DefaultEventSourceManagerTest { private DefaultEventHandler defaultEventHandlerMock = mock(DefaultEventHandler.class); private DefaultEventSourceManager defaultEventSourceManager = - new DefaultEventSourceManager(defaultEventHandlerMock, false); + new DefaultEventSourceManager(defaultEventHandlerMock); @Test public void registersEventSource() { @@ -32,7 +32,7 @@ public void registersEventSource() { Map registeredSources = defaultEventSourceManager.getRegisteredEventSources(); - assertThat(registeredSources.entrySet()).hasSize(1); + assertThat(registeredSources.entrySet()).hasSize(2); assertThat(registeredSources.get(CUSTOM_EVENT_SOURCE_NAME)).isEqualTo(eventSource); verify(eventSource, times(1)).setEventHandler(eq(defaultEventHandlerMock)); verify(eventSource, times(1)).start(); From a1f92c65ba3aaba63ff43866c2fe0d9dcc4621c9 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 4 Oct 2021 18:46:00 +0200 Subject: [PATCH 09/40] fix: custom resource selector test improvement --- .../processing/event/DefaultEventSourceManager.java | 3 +-- .../event/internal/CustomResourceSelectorTest.java | 9 ++++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index f623beac2a..58f44851ad 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -38,8 +38,8 @@ public class DefaultEventSourceManager> public DefaultEventSourceManager(ConfiguredController controller) { CustomResourceEventSource customResourceEventSource = new CustomResourceEventSource<>(controller); - registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, customResourceEventSource); init(new DefaultEventHandler<>(controller, customResourceEventSource)); + registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, customResourceEventSource); } private void init(DefaultEventHandler defaultEventHandler) { @@ -48,7 +48,6 @@ private void init(DefaultEventHandler defaultEventHandler) { this.retryTimerEventSource = new TimerEventSource<>(); registerEventSource(RETRY_TIMER_EVENT_SOURCE_NAME, retryTimerEventSource); - } @Override diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index b6094a0ffe..fefe45602e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -42,6 +42,7 @@ public class CustomResourceSelectorTest { private static final Logger LOGGER = LoggerFactory.getLogger(CustomResourceSelectorTest.class); + public static final String NAMESPACE = "test"; KubernetesMockServer server; KubernetesClient client; @@ -112,8 +113,9 @@ void resourceWatchedByLabel() { new MyConfiguration(configurationService, "app=bar")); o2.start(); - client.resources(TestCustomResource.class).inNamespace("test").create(newMyResource("foo")); - client.resources(TestCustomResource.class).inNamespace("test").create(newMyResource("bar")); + client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("foo", + NAMESPACE)); + client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); await() .atMost(5, TimeUnit.SECONDS) @@ -133,9 +135,10 @@ void resourceWatchedByLabel() { } } - public TestCustomResource newMyResource(String app) { + public TestCustomResource newMyResource(String app, String namespace) { TestCustomResource resource = new TestCustomResource(); resource.setMetadata(new ObjectMetaBuilder().withName(app).addToLabels("app", app).build()); + resource.getMetadata().setNamespace(namespace); return resource; } From e928a3e6218d6c1d532958b7d2bf5d57e74bf716 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 5 Oct 2021 10:28:34 +0200 Subject: [PATCH 10/40] fix: wip test imrpovements --- .../internal/CustomResourceSelectorTest.java | 60 ++++++++++--------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index fefe45602e..26339923e9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -4,6 +4,7 @@ import java.time.LocalDateTime; import java.time.format.DateTimeFormatter; import java.util.Date; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; @@ -11,6 +12,7 @@ import org.awaitility.core.ConditionTimeoutException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.internal.util.collections.Sets; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -100,38 +102,38 @@ void resourceWatchedByLabel() { }), new MyConfiguration(configurationService, "app=foo")); o1.start(); - o2.register( - new MyController( - resource -> { - if ("bar".equals(resource.getMetadata().getName())) { - c2.incrementAndGet(); - } - if ("foo".equals(resource.getMetadata().getName())) { - c2err.incrementAndGet(); - } - }), - new MyConfiguration(configurationService, "app=bar")); - o2.start(); +// o2.register( +// new MyController( +// resource -> { +// if ("bar".equals(resource.getMetadata().getName())) { +// c2.incrementAndGet(); +// } +// if ("foo".equals(resource.getMetadata().getName())) { +// c2err.incrementAndGet(); +// } +// }), +// new MyConfiguration(configurationService, "app=bar")); +// o2.start(); client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("foo", NAMESPACE)); - client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); +// client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); await() - .atMost(5, TimeUnit.SECONDS) - .pollInterval(100, TimeUnit.MILLISECONDS) - .until(() -> c1.get() == 1 && c1err.get() == 0); - await() - .atMost(5, TimeUnit.SECONDS) + .atMost(55, TimeUnit.SECONDS) .pollInterval(100, TimeUnit.MILLISECONDS) - .until(() -> c2.get() == 1 && c2err.get() == 0); - - assertThrows( - ConditionTimeoutException.class, - () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c1err, is(greaterThan(0)))); - assertThrows( - ConditionTimeoutException.class, - () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c2err, is(greaterThan(0)))); + .until(() -> c1.get() > 0 ); +// await() +// .atMost(5, TimeUnit.SECONDS) +// .pollInterval(100, TimeUnit.MILLISECONDS) +// .until(() -> c2.get() == 1 && c2err.get() == 0); +// +// assertThrows( +// ConditionTimeoutException.class, +// () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c1err, is(greaterThan(0)))); +// assertThrows( +// ConditionTimeoutException.class, +// () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c2err, is(greaterThan(0)))); } } @@ -162,13 +164,17 @@ public String getAssociatedControllerClassName() { return MyController.class.getCanonicalName(); } + @Override public Set getNamespaces() { + return Sets.newSet(NAMESPACE); + } + @Override public ConfigurationService getConfigurationService() { return service; } } - @Controller + @Controller(namespaces = NAMESPACE) public static class MyController implements ResourceController { private final Consumer consumer; From f35a340990e24623b37cc08f1535c3ccf72f8961 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 5 Oct 2021 15:02:44 +0200 Subject: [PATCH 11/40] fix: test improvements --- .../internal/CustomResourceSelectorTest.java | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index 26339923e9..d8e95bae2c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -102,38 +102,38 @@ void resourceWatchedByLabel() { }), new MyConfiguration(configurationService, "app=foo")); o1.start(); -// o2.register( -// new MyController( -// resource -> { -// if ("bar".equals(resource.getMetadata().getName())) { -// c2.incrementAndGet(); -// } -// if ("foo".equals(resource.getMetadata().getName())) { -// c2err.incrementAndGet(); -// } -// }), -// new MyConfiguration(configurationService, "app=bar")); -// o2.start(); + o2.register( + new MyController( + resource -> { + if ("bar".equals(resource.getMetadata().getName())) { + c2.incrementAndGet(); + } + if ("foo".equals(resource.getMetadata().getName())) { + c2err.incrementAndGet(); + } + }), + new MyConfiguration(configurationService, "app=bar")); + o2.start(); client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("foo", NAMESPACE)); -// client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); + client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); await() .atMost(55, TimeUnit.SECONDS) .pollInterval(100, TimeUnit.MILLISECONDS) .until(() -> c1.get() > 0 ); -// await() -// .atMost(5, TimeUnit.SECONDS) -// .pollInterval(100, TimeUnit.MILLISECONDS) -// .until(() -> c2.get() == 1 && c2err.get() == 0); -// -// assertThrows( -// ConditionTimeoutException.class, -// () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c1err, is(greaterThan(0)))); -// assertThrows( -// ConditionTimeoutException.class, -// () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c2err, is(greaterThan(0)))); + await() + .atMost(5, TimeUnit.SECONDS) + .pollInterval(100, TimeUnit.MILLISECONDS) + .until(() -> c2.get() == 1 && c2err.get() == 0); + + assertThrows( + ConditionTimeoutException.class, + () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c1err, is(greaterThan(0)))); + assertThrows( + ConditionTimeoutException.class, + () -> await().atMost(2, TimeUnit.SECONDS).untilAtomic(c2err, is(greaterThan(0)))); } } @@ -164,7 +164,8 @@ public String getAssociatedControllerClassName() { return MyController.class.getCanonicalName(); } - @Override public Set getNamespaces() { + @Override + public Set getNamespaces() { return Sets.newSet(NAMESPACE); } From 5d5817e4c2f9cf737bf16d25c3188ed80c162292 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 5 Oct 2021 16:06:56 +0200 Subject: [PATCH 12/40] fix: further improvements --- .../internal/CustomResourceEventSource.java | 24 +++++++++++++++++-- .../processing/DefaultEventHandlerTest.java | 16 ------------- .../internal/CustomResourceSelectorTest.java | 10 +++++--- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 705d7b0cb8..a22cd1bc2c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -21,6 +21,9 @@ import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; + import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; @@ -38,10 +41,13 @@ public class CustomResourceEventSource> extends A private final ConfiguredController controller; private final Map> sharedIndexInformers = new ConcurrentHashMap<>(); + private ObjectMapper cloningObjectMapper; // todo metric for custom resource caches public CustomResourceEventSource(ConfiguredController controller) { this.controller = controller; + this.cloningObjectMapper = + controller.getConfiguration().getConfigurationService().getObjectMapper(); } @Override @@ -50,6 +56,7 @@ public void start() { final var targetNamespaces = configuration.getEffectiveNamespaces(); final var client = controller.getCRClient(); final var labelSelector = configuration.getLabelSelector(); + // todo informers not support label selectors currently var options = new ListOptions(); if (Utils.isNotNullOrEmpty(labelSelector)) { options.setLabelSelector(labelSelector); @@ -111,7 +118,7 @@ public void eventReceived(ResourceAction action, T customResource, T oldResource CustomResourceEventFilters.generationAware())); if (filter.acceptChange(controller.getConfiguration(), oldResource, customResource)) { - eventHandler.handleEvent(new CustomResourceEvent(action, customResource)); + eventHandler.handleEvent(new CustomResourceEvent(action, clone(customResource))); } else { log.debug( "Skipping event handling resource {} with version: {}", @@ -142,6 +149,19 @@ public Optional getCustomResource(CustomResourceID resourceID) { var resource = sharedIndexInformer.getStore() .getByKey(Cache.namespaceKeyFunc(resourceID.getNamespace().orElse(null), resourceID.getName())); - return Optional.ofNullable(resource); + if (resource == null) { + return Optional.empty(); + } else { + return Optional.of(clone(resource)); + } + } + + private T clone(T customResource) { + try { + return (T) cloningObjectMapper.readValue( + cloningObjectMapper.writeValueAsString(customResource), customResource.getClass()); + } catch (JsonProcessingException e) { + throw new IllegalStateException(e); + } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 8cb6fd7633..02164e810f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -56,22 +56,6 @@ public void setup() { .thenReturn(retryTimerEventSourceMock); defaultEventHandler.setEventSourceManager(defaultEventSourceManagerMock); defaultEventHandlerWithRetry.setEventSourceManager(defaultEventSourceManagerMock); - - // // todo: remove - // when(defaultEventSourceManagerMock.getCache()).thenReturn(customResourceCache); - // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); - // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResource(any()); - // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResources(any()); - // doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResourceUids(any()); - // doCallRealMethod().when(defaultEventSourceManagerMock).cacheResource(any(), any()); - // doAnswer( - // invocation -> { - // final var resourceId = (String) invocation.getArgument(0); - // customResourceCache.cleanup(resourceId); - // return null; - // }) - // .when(defaultEventSourceManagerMock) - // .cleanup(any()); } @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index d8e95bae2c..25dc21b21a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -11,6 +11,7 @@ import org.awaitility.core.ConditionTimeoutException; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.internal.util.collections.Sets; import org.slf4j.Logger; @@ -41,6 +42,8 @@ import static org.mockito.Mockito.when; @EnableKubernetesMockClient(crud = true, https = false) +// todo +@Disabled("Informers currently not support label selectors") public class CustomResourceSelectorTest { private static final Logger LOGGER = LoggerFactory.getLogger(CustomResourceSelectorTest.class); @@ -117,12 +120,13 @@ void resourceWatchedByLabel() { client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("foo", NAMESPACE)); - client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar",NAMESPACE)); + client.resources(TestCustomResource.class).inNamespace(NAMESPACE).create(newMyResource("bar", + NAMESPACE)); await() - .atMost(55, TimeUnit.SECONDS) + .atMost(325, TimeUnit.SECONDS) .pollInterval(100, TimeUnit.MILLISECONDS) - .until(() -> c1.get() > 0 ); + .until(() -> c1.get() == 1 && c1err.get() == 0); await() .atMost(5, TimeUnit.SECONDS) .pollInterval(100, TimeUnit.MILLISECONDS) From 75ad7d294311f41d1d8f7860f425241783a3bfe2 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 5 Oct 2021 16:27:31 +0200 Subject: [PATCH 13/40] fix: build --- .../operator/micrometer/MicrometerMetrics.java | 5 +++-- .../src/main/java/io/javaoperatorsdk/operator/Operator.java | 4 ---- .../operator/processing/DefaultEventHandlerTest.java | 2 -- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/micrometer/MicrometerMetrics.java index 7dd5515173..3c8074f8f8 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/micrometer/MicrometerMetrics.java @@ -5,6 +5,7 @@ import io.javaoperatorsdk.operator.Metrics; import io.javaoperatorsdk.operator.processing.DefaultEventHandler.EventMonitor; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Timer; @@ -15,12 +16,12 @@ public class MicrometerMetrics implements Metrics { private final MeterRegistry registry; private final EventMonitor monitor = new EventMonitor() { @Override - public void processedEvent(String uid, Event event) { + public void processedEvent(CustomResourceID uid, Event event) { incrementProcessedEventsNumber(); } @Override - public void failedEvent(String uid, Event event) { + public void failedEvent(CustomResourceID uid, Event event) { incrementControllerRetriesNumber(); } }; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java index 9b6510daec..c61bcb04b3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java @@ -18,10 +18,6 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; import io.javaoperatorsdk.operator.processing.ConfiguredController; -import io.javaoperatorsdk.operator.processing.DefaultEventHandler; -import io.javaoperatorsdk.operator.processing.DefaultEventHandler.EventMonitor; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.Event; @SuppressWarnings("rawtypes") public class Operator implements AutoCloseable { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 263f86dad5..d15ef69300 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -28,8 +28,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.timeout; From 7012fb344ed2e0741cfb46401772a4054853be04 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 5 Oct 2021 16:30:59 +0200 Subject: [PATCH 14/40] feature: add mvn jar to gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index d82a982579..1f3ddc0c32 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,5 @@ target/ # VSCode .factorypath + +.mvn/wrapper/maven-wrapper.jar From 3746122284a53fa94175947ca51d0ead6e2abc5b Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 6 Oct 2021 13:47:13 +0200 Subject: [PATCH 15/40] Exposing CustomResourceEventSource and informers --- .../processing/event/DefaultEventSourceManager.java | 5 +++++ .../operator/processing/event/EventSourceManager.java | 6 +++++- .../event/internal/CustomResourceEventSource.java | 11 ++++++++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 1240b15c8a..77935fed20 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -150,6 +150,11 @@ public Map getRegisteredEventSources() { return Collections.unmodifiableMap(eventSources); } + @Override + public CustomResourceEventSource getCustomResourceEventSource() { + return getCustomResourceEventSource(); + } + public void cleanup(CustomResourceID customResourceUid) { getRegisteredEventSources() .keySet() 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 9f8b0f40fa..86638c2786 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 @@ -5,9 +5,11 @@ import java.util.Map; import java.util.Optional; +import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.OperatorException; +import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEventSource; -public interface EventSourceManager extends Closeable { +public interface EventSourceManager> extends Closeable { /** * Add the {@link EventSource} identified by the given name to the event manager. @@ -36,6 +38,8 @@ Optional deRegisterCustomResourceFromEventSource( Map getRegisteredEventSources(); + CustomResourceEventSource getCustomResourceEventSource(); + @Override default void close() throws IOException {} } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index a22cd1bc2c..770b0d3e68 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -34,7 +34,7 @@ public class CustomResourceEventSource> extends AbstractEventSource implements ResourceEventHandler, ResourceCache { - private static final String ANY_NAMESPACE_MAP_KEY = "anyNamespace"; + public static final String ANY_NAMESPACE_MAP_KEY = "anyNamespace"; private static final Logger log = LoggerFactory.getLogger(CustomResourceEventSource.class); @@ -64,6 +64,7 @@ public void start() { try { if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { + var informer = client.inAnyNamespace().inform(this); sharedIndexInformers.put(ANY_NAMESPACE_MAP_KEY, informer); log.debug("Registered {} -> {} for any namespace", controller, informer); @@ -156,6 +157,14 @@ public Optional getCustomResource(CustomResourceID resourceID) { } } + /** + * @return shared informers by namespace. If custom resource is not namespace scoped use + * CustomResourceEventSource.ANY_NAMESPACE_MAP_KEY + */ + public Map> getInformers() { + return Collections.unmodifiableMap(sharedIndexInformers); + } + private T clone(T customResource) { try { return (T) cloningObjectMapper.readValue( From f0f2e91d7aebeff1622bb95142eeded14ae5d5c1 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 6 Oct 2021 14:10:49 +0200 Subject: [PATCH 16/40] fix: cleanup --- .../processing/CustomResourceCache.java | 118 ------------------ .../operator/processing/EventDispatcher.java | 1 - .../operator/processing/ResourceCache.java | 1 - .../event/DefaultEventSourceManager.java | 28 ----- .../internal/CustomResourceEventSource.java | 4 +- .../javaoperatorsdk/operator/TestUtils.java | 24 +--- pom.xml | 2 - 7 files changed, 2 insertions(+), 176 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java deleted file mode 100644 index 02c31b0991..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java +++ /dev/null @@ -1,118 +0,0 @@ -package io.javaoperatorsdk.operator.processing; - -import java.util.List; -import java.util.Optional; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Predicate; -import java.util.stream.Collectors; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.Metrics; - -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.ObjectMapper; - -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; - -@SuppressWarnings("rawtypes") -// todo remove -@Deprecated -public class CustomResourceCache> { - - private static final Logger log = LoggerFactory.getLogger(CustomResourceCache.class); - private static final Predicate passthrough = o -> true; - - private final ObjectMapper objectMapper; - private final ConcurrentMap resources; - private final Lock lock = new ReentrantLock(); - - public CustomResourceCache() { - this(new ObjectMapper(), Metrics.NOOP); - } - - public CustomResourceCache(ObjectMapper objectMapper, Metrics metrics) { - this.objectMapper = objectMapper; - if (metrics == null) { - metrics = Metrics.NOOP; - } - resources = metrics.monitorSizeOf(new ConcurrentHashMap<>(), "cache"); - } - - @SuppressWarnings("unchecked") - public void cacheResource(T resource) { - cacheResource(resource, passthrough); - } - - public void cacheResource(T resource, Predicate predicate) { - try { - lock.lock(); - final var uid = getUID(resource); - if (predicate.test(resources.get(uid))) { - if (passthrough != predicate) { - log.trace("Update cache after condition is true: {}", getName(resource)); - } - // defensive copy - resources.put(getUID(resource), clone(resource)); - } - } finally { - lock.unlock(); - } - } - - /** - * We clone the object so the one in the cache is not changed by the controller or dispatcher. - * Therefore the cached object always represents the object coming from the API server. - * - * @param uuid identifier of resource - * @return resource if found in cache - */ - public Optional getLatestResource(String uuid) { - return Optional.ofNullable(resources.get(uuid)).map(this::clone); - } - - public List getLatestResources(Predicate selector) { - try { - lock.lock(); - return resources.values().stream() - .filter(selector) - .map(this::clone) - .collect(Collectors.toList()); - } finally { - lock.unlock(); - } - } - - public Set getLatestResourcesUids(Predicate selector) { - try { - lock.lock(); - return resources.values().stream() - .filter(selector) - .map(r -> r.getMetadata().getUid()) - .collect(Collectors.toSet()); - } finally { - lock.unlock(); - } - } - - @SuppressWarnings("unchecked") - private T clone(CustomResource customResource) { - try { - return (T) objectMapper.readValue( - objectMapper.writeValueAsString(customResource), customResource.getClass()); - } catch (JsonProcessingException e) { - throw new IllegalStateException(e); - } - } - - public T cleanup(String customResourceUid) { - return resources.remove(customResourceUid); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java index 85fcbefb9d..92347d5460 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java @@ -168,7 +168,6 @@ private PostExecutionControl handleDelete(R resource, Context context) { if (deleteControl == DeleteControl.DEFAULT_DELETE && resource.hasFinalizer(configuration().getFinalizer())) { R customResource = removeFinalizer(resource); - // todo: should we patch the resource to remove the finalizer instead of updating it return PostExecutionControl.customResourceUpdated(customResource); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java index 0f513333a4..127926c724 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceCache.java @@ -5,7 +5,6 @@ import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -// todo rename after deleted CustomResourceCache public interface ResourceCache> { Optional getCustomResource(CustomResourceID resourceID); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 77935fed20..27391bb21a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -160,32 +160,4 @@ public void cleanup(CustomResourceID customResourceUid) { .keySet() .forEach(k -> deRegisterCustomResourceFromEventSource(k, customResourceUid)); } - - // // todo: remove - // public ResourceCache getCache() { - // final var source = - // (CustomResourceEventSource) getRegisteredEventSources() - // .get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME); - // return source; - // } - - // // todo: remove - // public Optional getLatestResource(String customResourceUid) { - // return getCache().getLatestResource(customResourceUid); - // } - // - // // todo: remove - // public List getLatestResources(Predicate selector) { - // return getCache().getLatestResources(selector); - // } - // - // // todo: remove - // public Set getLatestResourceUids(Predicate selector) { - // return getCache().getLatestResourcesUids(selector); - // } - // - // // todo: remove - // public void cacheResource(CustomResource resource, Predicate predicate) { - // getCache().cacheResource(resource, predicate); - // } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 770b0d3e68..21ea1527ab 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -43,7 +43,6 @@ public class CustomResourceEventSource> extends A new ConcurrentHashMap<>(); private ObjectMapper cloningObjectMapper; - // todo metric for custom resource caches public CustomResourceEventSource(ConfiguredController controller) { this.controller = controller; this.cloningObjectMapper = @@ -56,7 +55,7 @@ public void start() { final var targetNamespaces = configuration.getEffectiveNamespaces(); final var client = controller.getCRClient(); final var labelSelector = configuration.getLabelSelector(); - // todo informers not support label selectors currently + // todo label selectors currently var options = new ListOptions(); if (Utils.isNotNullOrEmpty(labelSelector)) { options.setLabelSelector(labelSelector); @@ -106,7 +105,6 @@ public void close() throws IOException { } } - // todo check if the resource version is the same? public void eventReceived(ResourceAction action, T customResource, T oldResource) { log.debug( "Event received for resource: {}", getName(customResource)); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java index d0b4e31ea2..c1e887de86 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java @@ -12,11 +12,8 @@ public class TestUtils { - public static final String TEST_CUSTOM_RESOURCE_NAME = "test-custom-resource"; - public static final String TEST_NAMESPACE = "java-operator-sdk-int-test"; - public static TestCustomResource testCustomResource() { - return testCustomResource(UUID.randomUUID().toString()); + return testCustomResource(new CustomResourceID(UUID.randomUUID().toString(), "test")); } public static CustomResourceDefinition testCRD(String scope) { @@ -30,25 +27,6 @@ public static CustomResourceDefinition testCRD(String scope) { .build(); } - // todo remove? - public static TestCustomResource testCustomResource(String uid) { - TestCustomResource resource = new TestCustomResource(); - resource.setMetadata( - new ObjectMetaBuilder() - .withName(TEST_CUSTOM_RESOURCE_NAME) - .withUid(uid) - .withGeneration(1L) - .withNamespace(TEST_NAMESPACE) - .build()); - resource.getMetadata().setAnnotations(new HashMap<>()); - resource.setKind("CustomService"); - resource.setSpec(new TestCustomResourceSpec()); - resource.getSpec().setConfigMapName("test-config-map"); - resource.getSpec().setKey("test-key"); - resource.getSpec().setValue("test-value"); - return resource; - } - public static TestCustomResource testCustomResource(CustomResourceID id) { TestCustomResource resource = new TestCustomResource(); resource.setMetadata( diff --git a/pom.xml b/pom.xml index 511a1f3f20..e1c859d3be 100644 --- a/pom.xml +++ b/pom.xml @@ -177,7 +177,6 @@ https://oss.sonatype.org/content/repositories/snapshots - @@ -432,5 +431,4 @@ - From 6638a4818c54336e0b096f48597ff7d92ef23197 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 6 Oct 2021 16:06:37 +0200 Subject: [PATCH 17/40] fix: remove caching optimization since it not possible anymore with informer --- .../processing/DefaultEventHandler.java | 40 ++----------------- .../internal/CustomResourceEventSource.java | 11 ++++- 2 files changed, 12 insertions(+), 39 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 883eef2254..28504eb596 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -174,6 +174,9 @@ private void executeBufferedEvents(CustomResourceID customResourceUid) { newEventForResourceId, controllerUnderExecution, latestCustomResource.isPresent()); + if (latestCustomResource.isEmpty()) { + log.warn("no custom resource found in cache for CustomResourceID: {}", customResourceUid); + } } } @@ -209,7 +212,6 @@ void eventProcessingFinished( if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { cleanupAfterDeletedEvent(executionScope.getCustomResourceID()); } else { - cacheUpdatedResourceIfChanged(executionScope, postExecutionControl); reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); executeBufferedEvents(executionScope.getCustomResourceID()); } @@ -275,42 +277,6 @@ private RetryExecution getOrInitRetryExecution(ExecutionScope executionScope) return retryExecution; } - /** - * Here we try to cache the latest resource after an update. The goal is to solve a concurrency - * issue we've seen: If an execution is finished, where we updated a custom resource, but there - * are other events already buffered for next execution, we might not get the newest custom - * resource from CustomResource event source in time. Thus we execute the next batch of events but - * with a non up to date CR. Here we cache the latest CustomResource from the update execution so - * we make sure its already used in the up-coming execution. - * - *

- * Note that this is an improvement, not a bug fix. This situation can happen naturally, we just - * make the execution more efficient, and avoid questions about conflicts. - * - *

- * Note that without the conditional locking in the cache, there is a very minor chance that we - * would override an additional change coming from a different client. - */ - private void cacheUpdatedResourceIfChanged( - ExecutionScope executionScope, PostExecutionControl postExecutionControl) { - if (postExecutionControl.customResourceUpdatedDuringExecution()) { - R originalCustomResource = executionScope.getCustomResource(); - R customResourceAfterExecution = postExecutionControl.getUpdatedCustomResource().get(); - String originalResourceVersion = getVersion(originalCustomResource); - - log.debug( - "Trying to update resource cache from update response for resource: {} new version: {} old version: {}", - getName(originalCustomResource), - getVersion(customResourceAfterExecution), - getVersion(originalCustomResource)); - // todo how to handle this? - // eventSourceManager.cacheResource( - // customResourceAfterExecution, - // customResource -> getVersion(customResource).equals(originalResourceVersion) - // && !originalResourceVersion.equals(getVersion(customResourceAfterExecution))); - } - } - private void cleanupAfterDeletedEvent(CustomResourceID customResourceUid) { eventSourceManager.cleanup(customResourceUid); eventBuffer.cleanup(customResourceUid); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 21ea1527ab..ff1469d2eb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -55,7 +55,7 @@ public void start() { final var targetNamespaces = configuration.getEffectiveNamespaces(); final var client = controller.getCRClient(); final var labelSelector = configuration.getLabelSelector(); - // todo label selectors currently + // todo label selectors var options = new ListOptions(); if (Utils.isNotNullOrEmpty(labelSelector)) { options.setLabelSelector(labelSelector); @@ -63,7 +63,6 @@ public void start() { try { if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { - var informer = client.inAnyNamespace().inform(this); sharedIndexInformers.put(ANY_NAMESPACE_MAP_KEY, informer); log.debug("Registered {} -> {} for any namespace", controller, informer); @@ -163,6 +162,14 @@ public Map> getInformers() { return Collections.unmodifiableMap(sharedIndexInformers); } + public SharedIndexInformer getInformer(String namespace) { + if (namespace == null) { + return getInformers().get(ANY_NAMESPACE_MAP_KEY); + } else { + return getInformers().get(namespace); + } + } + private T clone(T customResource) { try { return (T) cloningObjectMapper.readValue( From 1d786efca4f565a46a0daafc90407d086ca572a5 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 6 Oct 2021 16:21:50 +0200 Subject: [PATCH 18/40] fix: formatting --- .../javaoperatorsdk/operator/processing/DefaultEventHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 28504eb596..7d1e7f3602 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -27,7 +27,6 @@ import static io.javaoperatorsdk.operator.EventListUtils.containsCustomResourceDeletedEvent; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; /** * Event handler that makes sure that events are processed in a "single threaded" way per resource From a5343b7349934b9dcaa6d4c58e22121582da41ef Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 6 Oct 2021 16:49:05 +0200 Subject: [PATCH 19/40] refactor: make name/namespace final --- .../operator/processing/event/CustomResourceID.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java index bf1958d9f1..d405e48a5a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/CustomResourceID.java @@ -12,8 +12,8 @@ public static CustomResourceID fromResource(HasMetadata resource) { resource.getMetadata().getNamespace()); } - private String name; - private String namespace; + private final String name; + private final String namespace; public CustomResourceID(String name, String namespace) { this.name = name; @@ -21,7 +21,7 @@ public CustomResourceID(String name, String namespace) { } public CustomResourceID(String name) { - this.name = name; + this(name, null); } public String getName() { From 9e0430a019d6b3c85be9371998a47985235b4db8 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 11:17:34 +0200 Subject: [PATCH 20/40] feature: Simple label selector support --- .../internal/CustomResourceEventSource.java | 12 ++++---- .../event/internal/LabelSelectorParser.java | 23 ++++++++++++++ .../internal/CustomResourceSelectorTest.java | 2 -- .../internal/LabelSelectorParserTest.java | 30 +++++++++++++++++++ 4 files changed, 58 insertions(+), 9 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index ff1469d2eb..dbd6937923 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -27,6 +27,7 @@ import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; +import static io.javaoperatorsdk.operator.processing.event.internal.LabelSelectorParser.parseSimpleLabelSelector; /** * This is a special case since is not bound to a single custom resource @@ -55,21 +56,18 @@ public void start() { final var targetNamespaces = configuration.getEffectiveNamespaces(); final var client = controller.getCRClient(); final var labelSelector = configuration.getLabelSelector(); - // todo label selectors - var options = new ListOptions(); - if (Utils.isNotNullOrEmpty(labelSelector)) { - options.setLabelSelector(labelSelector); - } try { if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { - var informer = client.inAnyNamespace().inform(this); + var informer = client.inAnyNamespace() + .withLabels(parseSimpleLabelSelector(labelSelector)).inform(this); sharedIndexInformers.put(ANY_NAMESPACE_MAP_KEY, informer); log.debug("Registered {} -> {} for any namespace", controller, informer); } else { targetNamespaces.forEach( ns -> { - var informer = client.inNamespace(ns).inform(this); + var informer = client.inNamespace(ns) + .withLabels(parseSimpleLabelSelector(labelSelector)).inform(this); sharedIndexInformers.put(ns, informer); log.debug("Registered {} -> {} for namespace: {}", controller, informer, ns); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java new file mode 100644 index 0000000000..fe2f7d0905 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java @@ -0,0 +1,23 @@ +package io.javaoperatorsdk.operator.processing.event.internal; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +public class LabelSelectorParser { + + public static Map parseSimpleLabelSelector(String labelSelector) { + if (labelSelector == null || labelSelector.isBlank()) { + return Collections.EMPTY_MAP; + } + String[] selectors = labelSelector.split(","); + Map labels = new HashMap<>(selectors.length); + Arrays.stream(selectors).forEach(s -> { + var kv = s.split("="); + labels.put(kv[0],kv[1]); + }); + return labels; + } + +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index 25dc21b21a..b8ea4e1729 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -42,8 +42,6 @@ import static org.mockito.Mockito.when; @EnableKubernetesMockClient(crud = true, https = false) -// todo -@Disabled("Informers currently not support label selectors") public class CustomResourceSelectorTest { private static final Logger LOGGER = LoggerFactory.getLogger(CustomResourceSelectorTest.class); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java new file mode 100644 index 0000000000..fa0c86c67b --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java @@ -0,0 +1,30 @@ +package io.javaoperatorsdk.operator.processing.event.internal; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.*; + +class LabelSelectorParserTest { + + @Test + public void nullParamReturnsEmptyMap() { + var res = LabelSelectorParser.parseSimpleLabelSelector(null); + assertThat(res).hasSize(0); + } + + @Test + public void emptyLabelSelectorReturnsEmptyMap() { + var res = LabelSelectorParser.parseSimpleLabelSelector(" "); + assertThat(res).hasSize(0); + } + + @Test + public void parseSimpleLabelSelector() { + var res = LabelSelectorParser.parseSimpleLabelSelector("app=foo"); + assertThat(res).hasSize(1).containsEntry("app","foo"); + + res = LabelSelectorParser.parseSimpleLabelSelector("app=foo,owner=me"); + assertThat(res).hasSize(2).containsEntry("app","foo").containsEntry("owner","me"); + } +} From 0aa29a173dbe229001f02950da8fdd591ab415cc Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 11:21:16 +0200 Subject: [PATCH 21/40] fix: formatting --- .../internal/CustomResourceEventSource.java | 2 -- .../event/internal/LabelSelectorParser.java | 4 +-- .../internal/CustomResourceSelectorTest.java | 1 - .../internal/LabelSelectorParserTest.java | 34 +++++++++---------- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index dbd6937923..b2f5692834 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -7,13 +7,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.api.model.ListOptions; import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.informers.ResourceEventHandler; import io.fabric8.kubernetes.client.informers.SharedIndexInformer; import io.fabric8.kubernetes.client.informers.cache.Cache; -import io.fabric8.kubernetes.client.utils.Utils; import io.javaoperatorsdk.operator.MissingCRDException; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.processing.ConfiguredController; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java index fe2f7d0905..9c2061b642 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParser.java @@ -12,10 +12,10 @@ public static Map parseSimpleLabelSelector(String labelSelector) return Collections.EMPTY_MAP; } String[] selectors = labelSelector.split(","); - Map labels = new HashMap<>(selectors.length); + Map labels = new HashMap<>(selectors.length); Arrays.stream(selectors).forEach(s -> { var kv = s.split("="); - labels.put(kv[0],kv[1]); + labels.put(kv[0], kv[1]); }); return labels; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java index b8ea4e1729..7556d2412a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceSelectorTest.java @@ -11,7 +11,6 @@ import org.awaitility.core.ConditionTimeoutException; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.internal.util.collections.Sets; import org.slf4j.Logger; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java index fa0c86c67b..d7df4e87ba 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java @@ -7,24 +7,24 @@ class LabelSelectorParserTest { - @Test - public void nullParamReturnsEmptyMap() { - var res = LabelSelectorParser.parseSimpleLabelSelector(null); - assertThat(res).hasSize(0); - } + @Test + public void nullParamReturnsEmptyMap() { + var res = LabelSelectorParser.parseSimpleLabelSelector(null); + assertThat(res).hasSize(0); + } - @Test - public void emptyLabelSelectorReturnsEmptyMap() { - var res = LabelSelectorParser.parseSimpleLabelSelector(" "); - assertThat(res).hasSize(0); - } + @Test + public void emptyLabelSelectorReturnsEmptyMap() { + var res = LabelSelectorParser.parseSimpleLabelSelector(" "); + assertThat(res).hasSize(0); + } - @Test - public void parseSimpleLabelSelector() { - var res = LabelSelectorParser.parseSimpleLabelSelector("app=foo"); - assertThat(res).hasSize(1).containsEntry("app","foo"); + @Test + public void parseSimpleLabelSelector() { + var res = LabelSelectorParser.parseSimpleLabelSelector("app=foo"); + assertThat(res).hasSize(1).containsEntry("app", "foo"); - res = LabelSelectorParser.parseSimpleLabelSelector("app=foo,owner=me"); - assertThat(res).hasSize(2).containsEntry("app","foo").containsEntry("owner","me"); - } + res = LabelSelectorParser.parseSimpleLabelSelector("app=foo,owner=me"); + assertThat(res).hasSize(2).containsEntry("app", "foo").containsEntry("owner", "me"); + } } From 3ad2fc5a7fa2066ab47ff7089b818b98b4007dfd Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 13:31:56 +0200 Subject: [PATCH 22/40] fix: code inspection reports --- .../operator/ControllerUtils.java | 2 +- .../operator/api/Controller.java | 8 +++--- .../DefaultControllerConfiguration.java | 2 +- .../processing/DefaultEventHandler.java | 2 +- .../event/DefaultEventSourceManager.java | 3 ++- .../internal/CustomResourceEventFilters.java | 12 ++++----- .../internal/CustomResourceEventSource.java | 8 ++---- .../event/internal/InformerEvent.java | 6 ++--- .../processing/DefaultEventHandlerTest.java | 8 +++--- .../processing/EventDispatcherTest.java | 6 ++--- .../event/DefaultEventSourceManagerTest.java | 5 ++-- .../CustomResourceEventFilterTest.java | 8 +++--- .../internal/LabelSelectorParserTest.java | 1 - .../runtime/AccumulativeMappingWriter.java | 2 +- .../runtime/AnnotationConfiguration.java | 27 +++++++++---------- .../config/runtime/ClassMappingProvider.java | 2 +- .../operator/EventSourceIT.java | 6 ++--- 17 files changed, 48 insertions(+), 60 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java index 0097cb128a..023b333758 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java @@ -18,7 +18,7 @@ public static String getNameFor(Class controllerCl final var annotation = controllerClass.getAnnotation(Controller.class); if (annotation != null) { final var name = annotation.name(); - if (!Controller.NULL.equals(name)) { + if (!Controller.EMPTY_STRING.equals(name)) { return name; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Controller.java index ebf9c65d42..bf03b03309 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Controller.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Controller.java @@ -11,11 +11,11 @@ @Target({ElementType.TYPE}) public @interface Controller { - String NULL = ""; + String EMPTY_STRING = ""; String WATCH_CURRENT_NAMESPACE = "JOSDK_WATCH_CURRENT"; String NO_FINALIZER = "JOSDK_NO_FINALIZER"; - String name() default NULL; + String name() default EMPTY_STRING; /** * Optional finalizer name, if it is not provided, one will be automatically generated. If the @@ -24,7 +24,7 @@ * * @return the finalizer name */ - String finalizerName() default NULL; + String finalizerName() default EMPTY_STRING; /** * If true, will dispatch new event to the controller if generation increased since the last @@ -50,7 +50,7 @@ * * @return the label selector */ - String labelSelector() default NULL; + String labelSelector() default EMPTY_STRING; /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java index 9775ce2151..f98fa14007 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java @@ -19,7 +19,7 @@ public class DefaultControllerConfiguration> private final RetryConfiguration retryConfiguration; private final String labelSelector; private final CustomResourceEventFilter customResourceEventFilter; - private Class customResourceClass; + private final Class customResourceClass; private ConfigurationService service; public DefaultControllerConfiguration( diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 7d1e7f3602..bb7cc29ab8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -92,7 +92,7 @@ public void setEventSourceManager(DefaultEventSourceManager eventSourceManage /** * @deprecated the EventMonitor to be used should now be retrieved from * {@link Metrics#getEventMonitor()} - * @param monitor + * @param monitor to use */ @Deprecated public static void setEventMonitor(EventMonitor monitor) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 27391bb21a..8142544c01 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -152,7 +152,8 @@ public Map getRegisteredEventSources() { @Override public CustomResourceEventSource getCustomResourceEventSource() { - return getCustomResourceEventSource(); + return (CustomResourceEventSource) getRegisteredEventSources() + .get(CUSTOM_RESOURCE_EVENT_SOURCE_NAME); } public void cleanup(CustomResourceID customResourceUid) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilters.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilters.java index 306cde0888..2f3bc72327 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilters.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilters.java @@ -113,12 +113,12 @@ public static CustomResourceEventFilter markedForD } return (configuration, oldResource, newResource) -> { - for (int i = 0; i < items.length; i++) { - if (items[i] == null) { + for (CustomResourceEventFilter item : items) { + if (item == null) { continue; } - if (!items[i].acceptChange(configuration, oldResource, newResource)) { + if (!item.acceptChange(configuration, oldResource, newResource)) { return false; } } @@ -147,12 +147,12 @@ public static CustomResourceEventFilter markedForD } return (configuration, oldResource, newResource) -> { - for (int i = 0; i < items.length; i++) { - if (items[i] == null) { + for (CustomResourceEventFilter item : items) { + if (item == null) { continue; } - if (items[i].acceptChange(configuration, oldResource, newResource)) { + if (item.acceptChange(configuration, oldResource, newResource)) { return true; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index b2f5692834..9964e7b450 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -40,7 +40,7 @@ public class CustomResourceEventSource> extends A private final ConfiguredController controller; private final Map> sharedIndexInformers = new ConcurrentHashMap<>(); - private ObjectMapper cloningObjectMapper; + private final ObjectMapper cloningObjectMapper; public CustomResourceEventSource(ConfiguredController controller) { this.controller = controller; @@ -159,11 +159,7 @@ public Map> getInformers() { } public SharedIndexInformer getInformer(String namespace) { - if (namespace == null) { - return getInformers().get(ANY_NAMESPACE_MAP_KEY); - } else { - return getInformers().get(namespace); - } + return getInformers().get(Objects.requireNonNullElse(namespace, ANY_NAMESPACE_MAP_KEY)); } private T clone(T customResource) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java index 47e00b6900..81c78d31b4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java @@ -8,9 +8,9 @@ public class InformerEvent extends DefaultEvent { - private ResourceAction action; - private T resource; - private T oldResource; + private final ResourceAction action; + private final T resource; + private final T oldResource; public InformerEvent(ResourceAction action, T resource, T oldResource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index d15ef69300..9f5a7255f7 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.processing; -import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -128,7 +127,7 @@ public void schedulesAnEventRetryOnException() { Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); - ExecutionScope executionScope = new ExecutionScope(Arrays.asList(event), customResource, null); + ExecutionScope executionScope = new ExecutionScope(List.of(event), customResource, null); PostExecutionControl postExecutionControl = PostExecutionControl.exceptionDuringExecution(new RuntimeException("test")); @@ -208,7 +207,7 @@ public void successfulExecutionResetsTheRetry() { @Test public void scheduleTimedEventIfInstructedByPostExecutionControl() { - var testDelay = 10000l; + var testDelay = 10000L; when(eventDispatcherMock.handleExecution(any())) .thenReturn(PostExecutionControl.defaultDispatch().withReSchedule(testDelay)); @@ -257,8 +256,7 @@ private CustomResourceEvent prepareCREvent(CustomResourceID uid) { } private Event nonCREvent(CustomResourceID relatedCustomResourceUid) { - TimerEvent timerEvent = new TimerEvent(relatedCustomResourceUid); - return timerEvent; + return new TimerEvent(relatedCustomResourceUid); } private void overrideData(CustomResourceID id, CustomResource applyTo) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index b1e3d015eb..ce5b5b7367 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -266,7 +266,7 @@ void propagatesRetryInfoToContextIfFinalizerSet() { eventDispatcher.handleExecution( new ExecutionScope( - Arrays.asList(), + List.of(), testCustomResource, new RetryInfo() { @Override @@ -296,12 +296,12 @@ void setReScheduleToPostExecutionControlFromUpdateControl() { when(controller.createOrUpdateResource(eq(testCustomResource), any())) .thenReturn( - UpdateControl.updateStatusSubResource(testCustomResource).withReSchedule(1000l)); + UpdateControl.updateStatusSubResource(testCustomResource).withReSchedule(1000L)); PostExecutionControl control = eventDispatcher.handleExecution( executionScopeWithCREvent(ADDED, testCustomResource)); - assertThat(control.getReScheduleDelay().get()).isEqualTo(1000l); + assertThat(control.getReScheduleDelay().get()).isEqualTo(1000L); } private void markForDeletion(CustomResource customResource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java index 275af74b10..089215486e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java @@ -59,9 +59,8 @@ public void throwExceptionIfRegisteringEventSourceWithSameName() { defaultEventSourceManager.registerEventSource(CUSTOM_EVENT_SOURCE_NAME, eventSource); assertThatExceptionOfType(IllegalStateException.class) .isThrownBy( - () -> { - defaultEventSourceManager.registerEventSource(CUSTOM_EVENT_SOURCE_NAME, eventSource2); - }); + () -> defaultEventSourceManager.registerEventSource(CUSTOM_EVENT_SOURCE_NAME, + eventSource2)); } @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java index bec0ff4c99..d71f9004fa 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java @@ -94,11 +94,9 @@ public void eventNotFilteredByCustomPredicateIfFinalizerIsRequired() { var config = new TestControllerConfig( FINALIZER, false, - (configuration, oldResource, newResource) -> { - return !Objects.equals( - oldResource.getStatus().getConfigMapStatus(), - newResource.getStatus().getConfigMapStatus()); - }); + (configuration, oldResource, newResource) -> !Objects.equals( + oldResource.getStatus().getConfigMapStatus(), + newResource.getStatus().getConfigMapStatus())); when(config.getConfigurationService().getObjectMapper()) .thenReturn(ConfigurationService.OBJECT_MAPPER); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java index d7df4e87ba..3a619bbc19 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/LabelSelectorParserTest.java @@ -3,7 +3,6 @@ import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.*; class LabelSelectorParserTest { diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java index 279167fc22..67cc71ac5a 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java @@ -18,7 +18,7 @@ */ class AccumulativeMappingWriter { - private Map mappings = new ConcurrentHashMap<>(); + private final Map mappings = new ConcurrentHashMap<>(); private final String resourcePath; private final ProcessingEnvironment processingEnvironment; diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java index 04502d217f..ba40dccfca 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java @@ -1,8 +1,6 @@ package io.javaoperatorsdk.operator.config.runtime; -import java.util.Optional; import java.util.Set; -import java.util.function.Predicate; import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.ControllerUtils; @@ -17,12 +15,12 @@ public class AnnotationConfiguration implements ControllerConfiguration { private final ResourceController controller; - private final Optional annotation; + private final Controller annotation; private ConfigurationService service; public AnnotationConfiguration(ResourceController controller) { this.controller = controller; - this.annotation = Optional.ofNullable(controller.getClass().getAnnotation(Controller.class)); + this.annotation = controller.getClass().getAnnotation(Controller.class); } @Override @@ -32,15 +30,16 @@ public String getName() { @Override public String getFinalizer() { - return annotation - .map(Controller::finalizerName) - .filter(Predicate.not(String::isBlank)) - .orElse(ControllerUtils.getDefaultFinalizerName(getCRDName())); + if (annotation.finalizerName().isBlank()) { + return ControllerUtils.getDefaultFinalizerName(getCRDName()); + } else { + return annotation.finalizerName(); + } } @Override public boolean isGenerationAware() { - return annotation.map(Controller::generationAwareEventProcessing).orElse(true); + return annotation.generationAwareEventProcessing(); } @Override @@ -50,12 +49,12 @@ public Class getCustomResourceClass() { @Override public Set getNamespaces() { - return Set.of(annotation.map(Controller::namespaces).orElse(new String[] {})); + return Set.of(annotation.namespaces()); } @Override public String getLabelSelector() { - return annotation.map(Controller::labelSelector).orElse(""); + return annotation.labelSelector(); } @Override @@ -78,9 +77,9 @@ public String getAssociatedControllerClassName() { public CustomResourceEventFilter getEventFilter() { CustomResourceEventFilter answer = null; - var filterTypes = annotation.map(Controller::eventFilters); - if (filterTypes.isPresent()) { - for (var filterType : filterTypes.get()) { + var filterTypes = annotation.eventFilters(); + if (filterTypes.length > 0) { + for (var filterType : filterTypes) { try { CustomResourceEventFilter filter = filterType.getConstructor().newInstance(); diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java index 1258904394..d76db8fbcd 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java @@ -36,7 +36,7 @@ static Map provide(final String resourcePath, T key, V value) { throw new IllegalStateException( String.format( "%s is not valid Mapping metadata, defined in %s", - clazzPair, url.toString())); + clazzPair, url)); } result.put( diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/EventSourceIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/EventSourceIT.java index d2954b86ee..eb556535a1 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/EventSourceIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/EventSourceIT.java @@ -35,10 +35,8 @@ public void receivingPeriodicEvents() { .pollInterval( EventSourceTestCustomResourceController.TIMER_PERIOD / 2, TimeUnit.MILLISECONDS) .untilAsserted( - () -> { - assertThat(TestUtils.getNumberOfExecutions(operator)) - .isGreaterThanOrEqualTo(4); - }); + () -> assertThat(TestUtils.getNumberOfExecutions(operator)) + .isGreaterThanOrEqualTo(4)); } public EventSourceTestCustomResource createTestCustomResource(String id) { From 92d3ed33f45a1524a98d1ef648cd4eed1712b8b2 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 13:44:05 +0200 Subject: [PATCH 23/40] fix: merge from v2 --- .../javaoperatorsdk/operator/processing/DefaultEventHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index e06b54e5ec..631195ca8b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -9,7 +9,6 @@ import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.locks.ReentrantLock; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import org.slf4j.Logger; import org.slf4j.LoggerFactory; From d194d252cc583053e0adb2862a7df5f8a8bf3d07 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 13:47:38 +0200 Subject: [PATCH 24/40] fix: removed most deprecated apis --- .../AbstractControllerConfiguration.java | 35 ------------------- .../DefaultControllerConfiguration.java | 23 ------------ .../processing/DefaultEventHandler.java | 11 ------ .../operator/processing/EventBuffer.java | 2 -- 4 files changed, 71 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractControllerConfiguration.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractControllerConfiguration.java deleted file mode 100644 index 65b4a7ebc3..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractControllerConfiguration.java +++ /dev/null @@ -1,35 +0,0 @@ -package io.javaoperatorsdk.operator.api.config; - -import java.util.Set; - -import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEventFilter; - -/** - * @deprecated use {@link DefaultControllerConfiguration} instead - * @param - */ -@Deprecated -public class AbstractControllerConfiguration> - extends DefaultControllerConfiguration { - - @Deprecated - public AbstractControllerConfiguration(String associatedControllerClassName, String name, - String crdName, String finalizer, boolean generationAware, - Set namespaces, - RetryConfiguration retryConfiguration) { - super(associatedControllerClassName, name, crdName, finalizer, generationAware, namespaces, - retryConfiguration, null, null, null, null); - } - - public AbstractControllerConfiguration(String associatedControllerClassName, String name, - String crdName, String finalizer, boolean generationAware, - Set namespaces, - RetryConfiguration retryConfiguration, String labelSelector, - CustomResourceEventFilter customResourcePredicate, - Class customResourceClass, - ConfigurationService service) { - super(associatedControllerClassName, name, crdName, finalizer, generationAware, namespaces, - retryConfiguration, labelSelector, customResourcePredicate, customResourceClass, service); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java index f98fa14007..672704f3c4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java @@ -54,29 +54,6 @@ public DefaultControllerConfiguration( setConfigurationService(service); } - @Deprecated - public DefaultControllerConfiguration( - String associatedControllerClassName, - String name, - String crdName, - String finalizer, - boolean generationAware, - Set namespaces, - RetryConfiguration retryConfiguration) { - this( - associatedControllerClassName, - name, - crdName, - finalizer, - generationAware, - namespaces, - retryConfiguration, - null, - null, - null, - null); - } - @Override public String getName() { return name; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 631195ca8b..056074ec60 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -13,7 +13,6 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.Metrics; import io.javaoperatorsdk.operator.api.RetryInfo; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; @@ -89,16 +88,6 @@ public void setEventSourceManager(DefaultEventSourceManager eventSourceManage this.eventSourceManager = eventSourceManager; } - /** - * @deprecated the EventMonitor to be used should now be retrieved from - * {@link Metrics#getEventMonitor()} - * @param monitor to use - */ - @Deprecated - public static void setEventMonitor(EventMonitor monitor) { - DefaultEventHandler.monitor = monitor; - } - /* * TODO: promote this interface to top-level, probably create a `monitoring` package? */ diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java index 29ae58eca7..b9c565a001 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java @@ -9,8 +9,6 @@ class EventBuffer { private final Map> events = new HashMap<>(); - /** @deprecated use {@link #addEvent(CustomResourceID, Event)} */ - @Deprecated public void addEvent(Event event) { addEvent(event.getRelatedCustomResourceID(), event); } From 7ee0b7dbff1c2c53a8a8aa1bcf364643beb2fef1 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 7 Oct 2021 16:52:50 +0200 Subject: [PATCH 25/40] wip: started to remove events from variouse layers --- .../javaoperatorsdk/operator/api/Context.java | 4 +- .../operator/api/DefaultContext.java | 10 +-- .../processing/DefaultEventHandler.java | 81 +++++++++++-------- .../operator/processing/EventBuffer.java | 45 ----------- .../operator/processing/EventDispatcher.java | 15 +--- .../operator/processing/EventMarker.java | 50 ++++++++++++ .../operator/processing/ExecutionScope.java | 13 +-- .../event/DefaultEventSourceManager.java | 10 +-- .../operator/processing/EventBufferTest.java | 60 -------------- 9 files changed, 107 insertions(+), 181 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java delete mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Context.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Context.java index 963eb16f76..cc832a2e21 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Context.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/Context.java @@ -3,11 +3,9 @@ import java.util.Optional; import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.processing.event.EventList; public interface Context { - EventList getEvents(); - Optional getRetryInfo(); + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/DefaultContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/DefaultContext.java index 4614b562df..f32a784e11 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/DefaultContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/DefaultContext.java @@ -3,21 +3,13 @@ import java.util.Optional; import io.fabric8.kubernetes.client.CustomResource; -import io.javaoperatorsdk.operator.processing.event.EventList; public class DefaultContext implements Context { private final RetryInfo retryInfo; - private final EventList events; - public DefaultContext(EventList events, RetryInfo retryInfo) { + public DefaultContext(RetryInfo retryInfo) { this.retryInfo = retryInfo; - this.events = events; - } - - @Override - public EventList getEvents() { - return events; } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 056074ec60..d70faa0176 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -20,11 +20,12 @@ import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.EventHandler; +import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; +import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.processing.retry.Retry; import io.javaoperatorsdk.operator.processing.retry.RetryExecution; -import static io.javaoperatorsdk.operator.EventListUtils.containsCustomResourceDeletedEvent; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; /** @@ -38,7 +39,7 @@ public class DefaultEventHandler> implements Even @Deprecated private static EventMonitor monitor = EventMonitor.NOOP; - private final EventBuffer eventBuffer; + private final EventMarker eventMarker; private final Set underProcessing = new HashSet<>(); private final EventDispatcher eventDispatcher; private final Retry retry; @@ -79,9 +80,9 @@ private DefaultEventHandler(ResourceCache resourceCache, ExecutorService exec this.controllerName = relatedControllerName; this.eventDispatcher = eventDispatcher; this.retry = retry; - eventBuffer = new EventBuffer(); this.resourceCache = resourceCache; this.eventMonitor = monitor != null ? monitor : EventMonitor.NOOP; + this.eventMarker = new EventMarker(); } public void setEventSourceManager(DefaultEventSourceManager eventSourceManager) { @@ -113,34 +114,37 @@ private EventMonitor monitor() { @Override public void handleEvent(Event event) { + lock.lock(); try { - lock.lock(); log.debug("Received event: {}", event); if (!this.running) { log.debug("Skipping event: {} because the event handler is shutting down", event); return; } final var monitor = monitor(); - eventBuffer.addEvent(event.getRelatedCustomResourceID(), event); monitor.processedEvent(event.getRelatedCustomResourceID(), event); - executeBufferedEvents(event.getRelatedCustomResourceID()); + markEvent(event); + if (eventMarker.isEventReceived(event.getRelatedCustomResourceID())) { + submitReconciliationExecution(event.getRelatedCustomResourceID()); + } else { + cleanupForDeletedEvent(event.getRelatedCustomResourceID()); + } } finally { lock.unlock(); } } - @Override - public void close() { - try { - lock.lock(); - this.running = false; - } finally { - lock.unlock(); + private void markEvent(Event event) { + if (event instanceof CustomResourceEvent && + ((CustomResourceEvent) event).getAction() == ResourceAction.DELETED) { + eventMarker.markDeleteEventReceived(event.getRelatedCustomResourceID()); + } else if (!eventMarker.isDeleteEventReceived(event.getRelatedCustomResourceID())) { + eventMarker.markEventReceived(event.getRelatedCustomResourceID()); } } - private boolean executeBufferedEvents(CustomResourceID customResourceUid) { - boolean newEventForResourceId = eventBuffer.containsEvents(customResourceUid); + private boolean submitReconciliationExecution(CustomResourceID customResourceUid) { + boolean newEventForResourceId = eventMarker.isEventReceived(customResourceUid); boolean controllerUnderExecution = isControllerUnderExecution(customResourceUid); Optional latestCustomResource = resourceCache.getCustomResource(customResourceUid); @@ -149,9 +153,9 @@ private boolean executeBufferedEvents(CustomResourceID customResourceUid) { setUnderExecutionProcessing(customResourceUid); ExecutionScope executionScope = new ExecutionScope( - eventBuffer.getAndRemoveEventsForExecution(customResourceUid), latestCustomResource.get(), retryInfo(customResourceUid)); + eventMarker.unmarkEvent(customResourceUid); log.debug("Executing events for custom resource. Scope: {}", executionScope); executor.execute(new ControllerExecution(executionScope)); return true; @@ -176,8 +180,8 @@ private RetryInfo retryInfo(CustomResourceID customResourceUid) { void eventProcessingFinished( ExecutionScope executionScope, PostExecutionControl postExecutionControl) { + lock.lock(); try { - lock.lock(); if (!running) { return; } @@ -190,22 +194,23 @@ void eventProcessingFinished( if (retry != null && postExecutionControl.exceptionDuringExecution()) { handleRetryOnException(executionScope); - final var monitor = monitor(); - executionScope.getEvents() - .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceID(), e)); + // todo + // final var monitor = monitor(); + // executionScope.getEvents() + // .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceID(), e)); return; } if (retry != null) { markSuccessfulExecutionRegardingRetry(executionScope); } - if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { - cleanupAfterDeletedEvent(executionScope.getCustomResourceID()); - } else { - var executed = executeBufferedEvents(executionScope.getCustomResourceID()); - if (!executed) { - reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); - } + // if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { + // cleanupForDeletedEvent(executionScope.getCustomResourceID()); + // } else { + var executed = submitReconciliationExecution(executionScope.getCustomResourceID()); + if (!executed) { + reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); + // } } } finally { lock.unlock(); @@ -226,17 +231,17 @@ private void reScheduleExecutionIfInstructed(PostExecutionControl postExecuti */ private void handleRetryOnException(ExecutionScope executionScope) { RetryExecution execution = getOrInitRetryExecution(executionScope); - boolean newEventsExists = eventBuffer - .newEventsExists(CustomResourceID.fromResource(executionScope.getCustomResource())); - eventBuffer.putBackEvents(executionScope.getCustomResourceID(), executionScope.getEvents()); + boolean newEventsExists = eventMarker.isEventReceived(executionScope.getCustomResourceID()); + eventMarker.markEventReceived(executionScope.getCustomResourceID()); if (newEventsExists) { log.debug("New events exists for for resource id: {}", executionScope.getCustomResourceID()); - executeBufferedEvents(executionScope.getCustomResourceID()); + submitReconciliationExecution(executionScope.getCustomResourceID()); return; } Optional nextDelay = execution.nextDelay(); + // todo cover situation when a new event is received but the nextDelay.ifPresentOrElse( delay -> { log.debug( @@ -269,9 +274,10 @@ private RetryExecution getOrInitRetryExecution(ExecutionScope executionScope) return retryExecution; } - private void cleanupAfterDeletedEvent(CustomResourceID customResourceUid) { + // todo when to do cleanup, note retry might be relevant event if a delete event is received + private void cleanupForDeletedEvent(CustomResourceID customResourceUid) { eventSourceManager.cleanup(customResourceUid); - eventBuffer.cleanup(customResourceUid); + eventMarker.unmarkEvent(customResourceUid); } private boolean isControllerUnderExecution(CustomResourceID customResourceUid) { @@ -286,6 +292,15 @@ private void unsetUnderExecution(CustomResourceID customResourceUid) { underProcessing.remove(customResourceUid); } + @Override + public void close() { + lock.lock(); + try { + this.running = false; + } finally { + lock.unlock(); + } + } private class ControllerExecution implements Runnable { private final ExecutionScope executionScope; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java deleted file mode 100644 index b9c565a001..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java +++ /dev/null @@ -1,45 +0,0 @@ -package io.javaoperatorsdk.operator.processing; - -import java.util.*; - -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.Event; - -class EventBuffer { - - private final Map> events = new HashMap<>(); - - public void addEvent(Event event) { - addEvent(event.getRelatedCustomResourceID(), event); - } - - public void addEvent(CustomResourceID uid, Event event) { - Objects.requireNonNull(uid, "uid"); - Objects.requireNonNull(event, "event"); - - List crEvents = events.computeIfAbsent(uid, (customResourceID) -> new LinkedList<>()); - crEvents.add(event); - } - - public boolean newEventsExists(CustomResourceID resourceId) { - return events.get(resourceId) != null && !events.get(resourceId).isEmpty(); - } - - public void putBackEvents(CustomResourceID resourceUid, List oldEvents) { - List crEvents = events.computeIfAbsent(resourceUid, (id) -> new LinkedList<>()); - crEvents.addAll(0, oldEvents); - } - - public boolean containsEvents(CustomResourceID customResourceId) { - return events.get(customResourceId) != null; - } - - public List getAndRemoveEventsForExecution(CustomResourceID resourceUid) { - List crEvents = events.remove(resourceUid); - return crEvents == null ? Collections.emptyList() : crEvents; - } - - public void cleanup(CustomResourceID resourceUid) { - events.remove(resourceUid); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java index 92347d5460..6c2ea57cf8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java @@ -14,9 +14,7 @@ import io.javaoperatorsdk.operator.api.ResourceController; import io.javaoperatorsdk.operator.api.UpdateControl; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; -import io.javaoperatorsdk.operator.processing.event.EventList; -import static io.javaoperatorsdk.operator.EventListUtils.containsCustomResourceDeletedEvent; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; @@ -59,15 +57,7 @@ public PostExecutionControl handleExecution(ExecutionScope executionScope) private PostExecutionControl handleDispatch(ExecutionScope executionScope) { R resource = executionScope.getCustomResource(); - log.debug("Handling events: {} for resource {}", executionScope.getEvents(), getName(resource)); - - if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { - log.debug( - "Skipping dispatch processing because of a Delete event: {} with version: {}", - getName(resource), - getVersion(resource)); - return PostExecutionControl.defaultDispatch(); - } + log.debug("Handling dispatch for resource {}", getName(resource)); final var markedForDeletion = resource.isMarkedForDeletion(); if (markedForDeletion && shouldNotDispatchToDelete(resource)) { @@ -79,8 +69,7 @@ private PostExecutionControl handleDispatch(ExecutionScope executionScope) } Context context = - new DefaultContext<>( - new EventList(executionScope.getEvents()), executionScope.getRetryInfo()); + new DefaultContext<>(executionScope.getRetryInfo()); if (markedForDeletion) { return handleDelete(resource, context); } else { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java new file mode 100644 index 0000000000..d03d301b46 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java @@ -0,0 +1,50 @@ +package io.javaoperatorsdk.operator.processing; + +import java.util.HashSet; +import java.util.Set; + +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import io.javaoperatorsdk.operator.processing.event.Event; + +public class EventMarker { + + private final Set marker = new HashSet<>(); + private final Set deleteEventMarker = new HashSet<>(); + + public void markEventReceived(Event event) { + markEventReceived(event.getRelatedCustomResourceID()); + } + + public void markEventReceived(CustomResourceID customResourceID) { + marker.add(customResourceID); + } + + public boolean isEventReceived(Event event) { + return isEventReceived(event.getRelatedCustomResourceID()); + } + + public boolean isEventReceived(CustomResourceID customResourceID) { + return marker.contains(customResourceID); + } + + public boolean unmarkEvent(CustomResourceID customResourceID) { + return marker.remove(customResourceID); + } + + public void markDeleteEventReceived(CustomResourceID customResourceID) { + deleteEventMarker.add(customResourceID); + } + + public boolean isDeleteEventReceived(Event event) { + return isDeleteEventReceived(event.getRelatedCustomResourceID()); + } + + public boolean isDeleteEventReceived(CustomResourceID customResourceID) { + return deleteEventMarker.contains(customResourceID); + } + + public boolean unmarkDeleteReceived(CustomResourceID customResourceID) { + return deleteEventMarker.remove(customResourceID); + } + +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java index 4f5f0ca7f7..6cf05e9308 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java @@ -1,29 +1,20 @@ package io.javaoperatorsdk.operator.processing; -import java.util.List; - import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.api.RetryInfo; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.Event; public class ExecutionScope> { - private final List events; // the latest custom resource from cache private final R customResource; private final RetryInfo retryInfo; - public ExecutionScope(List list, R customResource, RetryInfo retryInfo) { - this.events = list; + public ExecutionScope(R customResource, RetryInfo retryInfo) { this.customResource = customResource; this.retryInfo = retryInfo; } - public List getEvents() { - return events; - } - public R getCustomResource() { return customResource; } @@ -35,8 +26,6 @@ public CustomResourceID getCustomResourceID() { @Override public String toString() { return "ExecutionScope{" - + "events=" - + events + ", customResource uid: " + customResource.getMetadata().getUid() + ", version: " diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 8142544c01..148b0092c4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -52,9 +52,8 @@ private void init(DefaultEventHandler defaultEventHandler) { @Override public void close() { + lock.lock(); try { - lock.lock(); - try { defaultEventHandler.close(); } catch (Exception e) { @@ -80,9 +79,8 @@ public void close() { public final void registerEventSource(String name, EventSource eventSource) throws OperatorException { Objects.requireNonNull(eventSource, "EventSource must not be null"); - + lock.lock(); try { - lock.lock(); if (eventSources.containsKey(name)) { throw new IllegalStateException( "Event source with name already registered. Event source name: " + name); @@ -103,8 +101,8 @@ public final void registerEventSource(String name, EventSource eventSource) @Override public Optional deRegisterEventSource(String name) { + lock.lock(); try { - lock.lock(); EventSource currentEventSource = eventSources.remove(name); if (currentEventSource != null) { try { @@ -123,8 +121,8 @@ public Optional deRegisterEventSource(String name) { @Override public Optional deRegisterCustomResourceFromEventSource( String eventSourceName, CustomResourceID customResourceUid) { + lock.lock(); try { - lock.lock(); EventSource eventSource = this.eventSources.get(eventSourceName); if (eventSource == null) { log.warn( diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java deleted file mode 100644 index 6dc16f8a69..0000000000 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java +++ /dev/null @@ -1,60 +0,0 @@ -package io.javaoperatorsdk.operator.processing; - -import java.util.List; -import java.util.UUID; - -import org.junit.jupiter.api.Test; - -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.Event; -import io.javaoperatorsdk.operator.processing.event.internal.TimerEvent; - -import static org.assertj.core.api.Assertions.assertThat; - -class EventBufferTest { - - private EventBuffer eventBuffer = new EventBuffer(); - - String name = UUID.randomUUID().toString(); - CustomResourceID customResourceID = new CustomResourceID(name); - Event testEvent1 = new TimerEvent(customResourceID); - Event testEvent2 = new TimerEvent(customResourceID); - - @Test - public void storesEvents() { - eventBuffer.addEvent(testEvent1); - eventBuffer.addEvent(testEvent2); - - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isTrue(); - List events = eventBuffer.getAndRemoveEventsForExecution(customResourceID); - assertThat(events).hasSize(2); - } - - @Test - public void getsAndRemovesEvents() { - eventBuffer.addEvent(testEvent1); - eventBuffer.addEvent(testEvent2); - - List events = eventBuffer.getAndRemoveEventsForExecution(new CustomResourceID(name)); - assertThat(events).hasSize(2); - assertThat(events).contains(testEvent1, testEvent2); - } - - @Test - public void checksIfThereAreStoredEvents() { - eventBuffer.addEvent(testEvent1); - eventBuffer.addEvent(testEvent2); - - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isTrue(); - } - - @Test - public void canClearEvents() { - eventBuffer.addEvent(testEvent1); - eventBuffer.addEvent(testEvent2); - - eventBuffer.cleanup(customResourceID); - - assertThat(eventBuffer.containsEvents(testEvent1.getRelatedCustomResourceID())).isFalse(); - } -} From 150e875fbfbbe3796af558320f48b92f472d00b5 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 8 Oct 2021 15:22:30 +0200 Subject: [PATCH 26/40] fix: progress with implementation and tests --- .../processing/DefaultEventHandler.java | 70 +++++++++------- .../operator/processing/EventMarker.java | 80 ++++++++++++++----- .../operator/processing/event/EventList.java | 27 ------- .../event/internal/InformerEvent.java | 35 -------- .../event/internal/InformerEventSource.java | 12 +-- .../processing/event/internal/TimerEvent.java | 11 --- .../event/internal/TimerEventSource.java | 3 +- .../processing/DefaultEventHandlerTest.java | 33 ++------ .../processing/EventDispatcherTest.java | 3 +- .../processing/event/EventListTest.java | 24 ------ ...entSourceTestCustomResourceController.java | 1 - 11 files changed, 116 insertions(+), 183 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventList.java delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java delete mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index d70faa0176..02e0f4983e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -123,8 +123,9 @@ public void handleEvent(Event event) { } final var monitor = monitor(); monitor.processedEvent(event.getRelatedCustomResourceID(), event); + markEvent(event); - if (eventMarker.isEventReceived(event.getRelatedCustomResourceID())) { + if (eventMarker.isEventPresent(event.getRelatedCustomResourceID())) { submitReconciliationExecution(event.getRelatedCustomResourceID()); } else { cleanupForDeletedEvent(event.getRelatedCustomResourceID()); @@ -134,28 +135,20 @@ public void handleEvent(Event event) { } } - private void markEvent(Event event) { - if (event instanceof CustomResourceEvent && - ((CustomResourceEvent) event).getAction() == ResourceAction.DELETED) { - eventMarker.markDeleteEventReceived(event.getRelatedCustomResourceID()); - } else if (!eventMarker.isDeleteEventReceived(event.getRelatedCustomResourceID())) { - eventMarker.markEventReceived(event.getRelatedCustomResourceID()); - } - } - private boolean submitReconciliationExecution(CustomResourceID customResourceUid) { - boolean newEventForResourceId = eventMarker.isEventReceived(customResourceUid); + boolean newEventForResourceId = eventMarker.isEventPresent(customResourceUid); boolean controllerUnderExecution = isControllerUnderExecution(customResourceUid); Optional latestCustomResource = resourceCache.getCustomResource(customResourceUid); - if (!controllerUnderExecution && newEventForResourceId && latestCustomResource.isPresent()) { + if (!controllerUnderExecution && newEventForResourceId + && latestCustomResource.isPresent()) { setUnderExecutionProcessing(customResourceUid); ExecutionScope executionScope = new ExecutionScope( latestCustomResource.get(), retryInfo(customResourceUid)); - eventMarker.unmarkEvent(customResourceUid); + eventMarker.unMarkEventReceived(customResourceUid); log.debug("Executing events for custom resource. Scope: {}", executionScope); executor.execute(new ControllerExecution(executionScope)); return true; @@ -168,12 +161,22 @@ private boolean submitReconciliationExecution(CustomResourceID customResourceUid controllerUnderExecution, latestCustomResource.isPresent()); if (latestCustomResource.isEmpty()) { - log.warn("no custom resource found in cache for CustomResourceID: {}", customResourceUid); + log.warn("no custom resource found in cache for CustomResourceID: {}", + customResourceUid); } return false; } } + private void markEvent(Event event) { + if (event instanceof CustomResourceEvent && + ((CustomResourceEvent) event).getAction() == ResourceAction.DELETED) { + eventMarker.markDeleteEventReceived(event); + } else if (!eventMarker.isDeleteEventPresent(event.getRelatedCustomResourceID())) { + eventMarker.markEventReceived(event); + } + } + private RetryInfo retryInfo(CustomResourceID customResourceUid) { return retryState.get(customResourceUid); } @@ -194,7 +197,7 @@ void eventProcessingFinished( if (retry != null && postExecutionControl.exceptionDuringExecution()) { handleRetryOnException(executionScope); - // todo + // todo revisit monitoring since events are not present anymore // final var monitor = monitor(); // executionScope.getEvents() // .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceID(), e)); @@ -202,21 +205,27 @@ void eventProcessingFinished( } if (retry != null) { - markSuccessfulExecutionRegardingRetry(executionScope); + handleSuccessfulExecutionRegardingRetry(executionScope); } - // if (containsCustomResourceDeletedEvent(executionScope.getEvents())) { - // cleanupForDeletedEvent(executionScope.getCustomResourceID()); - // } else { - var executed = submitReconciliationExecution(executionScope.getCustomResourceID()); - if (!executed) { - reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); - // } + if (readyForCleanup(executionScope.getCustomResourceID())) { + cleanupForDeletedEvent(executionScope.getCustomResourceID()); + } else { + var executed = submitReconciliationExecution(executionScope.getCustomResourceID()); + if (!executed) { + reScheduleExecutionIfInstructed(postExecutionControl, + executionScope.getCustomResource()); + } } } finally { lock.unlock(); } } + private boolean readyForCleanup(CustomResourceID customResourceID) { + return eventMarker + .getEventingState(customResourceID) == EventMarker.EventingState.ONLY_DELETE_EVENT_PRESENT; + } + private void reScheduleExecutionIfInstructed(PostExecutionControl postExecutionControl, R customResource) { postExecutionControl.getReScheduleDelay().ifPresent(delay -> eventSourceManager @@ -231,17 +240,21 @@ private void reScheduleExecutionIfInstructed(PostExecutionControl postExecuti */ private void handleRetryOnException(ExecutionScope executionScope) { RetryExecution execution = getOrInitRetryExecution(executionScope); - boolean newEventsExists = eventMarker.isEventReceived(executionScope.getCustomResourceID()); + EventMarker.EventingState eventingState = + eventMarker.getEventingState(executionScope.getCustomResourceID()); + boolean newEventsExists = + eventingState == EventMarker.EventingState.EVENT_PRESENT || + eventingState == EventMarker.EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT; eventMarker.markEventReceived(executionScope.getCustomResourceID()); if (newEventsExists) { - log.debug("New events exists for for resource id: {}", executionScope.getCustomResourceID()); + log.debug("New events exists for for resource id: {}", + executionScope.getCustomResourceID()); submitReconciliationExecution(executionScope.getCustomResourceID()); return; } Optional nextDelay = execution.nextDelay(); - // todo cover situation when a new event is received but the nextDelay.ifPresentOrElse( delay -> { log.debug( @@ -255,7 +268,7 @@ private void handleRetryOnException(ExecutionScope executionScope) { () -> log.error("Exhausted retries for {}", executionScope)); } - private void markSuccessfulExecutionRegardingRetry(ExecutionScope executionScope) { + private void handleSuccessfulExecutionRegardingRetry(ExecutionScope executionScope) { log.debug( "Marking successful execution for resource: {}", getName(executionScope.getCustomResource())); @@ -274,10 +287,9 @@ private RetryExecution getOrInitRetryExecution(ExecutionScope executionScope) return retryExecution; } - // todo when to do cleanup, note retry might be relevant event if a delete event is received private void cleanupForDeletedEvent(CustomResourceID customResourceUid) { eventSourceManager.cleanup(customResourceUid); - eventMarker.unmarkEvent(customResourceUid); + eventMarker.cleanup(customResourceUid); } private boolean isControllerUnderExecution(CustomResourceID customResourceUid) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java index d03d301b46..96a271739b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java @@ -1,50 +1,88 @@ package io.javaoperatorsdk.operator.processing; -import java.util.HashSet; -import java.util.Set; +import java.util.HashMap; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; public class EventMarker { - private final Set marker = new HashSet<>(); - private final Set deleteEventMarker = new HashSet<>(); + public enum EventingState { + EVENT_PRESENT, ONLY_DELETE_EVENT_PRESENT, DELETE_AND_NON_DELETE_EVENT_PRESENT, NO_EVENT_PRESENT + } + + private final HashMap eventingState = new HashMap<>(); + + public EventingState getEventingState(CustomResourceID customResourceID) { + EventingState actualState = eventingState.get(customResourceID); + return actualState == null ? EventingState.NO_EVENT_PRESENT : actualState; + } + + public void setEventingState(CustomResourceID customResourceID, EventingState state) { + eventingState.put(customResourceID, state); + } public void markEventReceived(Event event) { markEventReceived(event.getRelatedCustomResourceID()); } public void markEventReceived(CustomResourceID customResourceID) { - marker.add(customResourceID); - } - - public boolean isEventReceived(Event event) { - return isEventReceived(event.getRelatedCustomResourceID()); + var actualState = getEventingState(customResourceID); + switch (actualState) { + case ONLY_DELETE_EVENT_PRESENT: + setEventingState(customResourceID, + EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT); + break; + case NO_EVENT_PRESENT: + setEventingState(customResourceID, EventingState.EVENT_PRESENT); + break; + } } - public boolean isEventReceived(CustomResourceID customResourceID) { - return marker.contains(customResourceID); + public void unMarkEventReceived(CustomResourceID customResourceID) { + var actualState = getEventingState(customResourceID); + switch (actualState) { + case EVENT_PRESENT: + setEventingState(customResourceID, + EventingState.NO_EVENT_PRESENT); + break; + case DELETE_AND_NON_DELETE_EVENT_PRESENT: + setEventingState(customResourceID, + EventingState.ONLY_DELETE_EVENT_PRESENT); + break; + } } - public boolean unmarkEvent(CustomResourceID customResourceID) { - return marker.remove(customResourceID); + public void markDeleteEventReceived(Event event) { + markDeleteEventReceived(event.getRelatedCustomResourceID()); } public void markDeleteEventReceived(CustomResourceID customResourceID) { - deleteEventMarker.add(customResourceID); + var actualState = getEventingState(customResourceID); + switch (actualState) { + case NO_EVENT_PRESENT: + setEventingState(customResourceID, EventingState.ONLY_DELETE_EVENT_PRESENT); + break; + case EVENT_PRESENT: + setEventingState(customResourceID, + EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT); + break; + } } - public boolean isDeleteEventReceived(Event event) { - return isDeleteEventReceived(event.getRelatedCustomResourceID()); + public boolean isEventPresent(CustomResourceID customResourceID) { + var actualState = getEventingState(customResourceID); + return actualState == EventingState.EVENT_PRESENT || + actualState == EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT; } - public boolean isDeleteEventReceived(CustomResourceID customResourceID) { - return deleteEventMarker.contains(customResourceID); + public boolean isDeleteEventPresent(CustomResourceID customResourceID) { + var actualState = getEventingState(customResourceID); + return actualState == EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT || + actualState == EventingState.ONLY_DELETE_EVENT_PRESENT; } - public boolean unmarkDeleteReceived(CustomResourceID customResourceID) { - return deleteEventMarker.remove(customResourceID); + public void cleanup(CustomResourceID customResourceID) { + eventingState.remove(customResourceID); } - } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventList.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventList.java deleted file mode 100644 index d9560f6f1c..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventList.java +++ /dev/null @@ -1,27 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event; - -import java.util.List; -import java.util.Optional; - -public class EventList { - - private final List eventList; - - public EventList(List eventList) { - this.eventList = eventList; - } - - public List getList() { - return eventList; - } - - public Optional getLatestOfType(Class eventType) { - for (int i = eventList.size() - 1; i >= 0; i--) { - Event event = eventList.get(i); - if (event.getClass().isAssignableFrom(eventType)) { - return (Optional) Optional.of(event); - } - } - return Optional.empty(); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java deleted file mode 100644 index 81c78d31b4..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java +++ /dev/null @@ -1,35 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event.internal; - -import java.util.Optional; - -import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.DefaultEvent; - -public class InformerEvent extends DefaultEvent { - - private final ResourceAction action; - private final T resource; - private final T oldResource; - - public InformerEvent(ResourceAction action, - T resource, T oldResource) { - super(CustomResourceID.fromResource(resource)); - this.action = action; - this.resource = resource; - this.oldResource = oldResource; - } - - public T getResource() { - return resource; - } - - public Optional getOldResource() { - return Optional.ofNullable(oldResource); - } - - public ResourceAction getAction() { - return action; - } - -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index f9d4799ef2..85f8367239 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -12,6 +12,8 @@ import io.fabric8.kubernetes.client.informers.cache.Cache; import io.fabric8.kubernetes.client.informers.cache.Store; import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import io.javaoperatorsdk.operator.processing.event.DefaultEvent; public class InformerEventSource extends AbstractEventSource { @@ -54,7 +56,7 @@ public InformerEventSource(SharedInformer sharedInformer, sharedInformer.addEventHandler(new ResourceEventHandler<>() { @Override public void onAdd(T t) { - propagateEvent(ResourceAction.ADDED, t, null); + propagateEvent(t); } @Override @@ -64,23 +66,23 @@ public void onUpdate(T oldObject, T newObject) { .equals(newObject.getMetadata().getResourceVersion())) { return; } - propagateEvent(ResourceAction.UPDATED, newObject, oldObject); + propagateEvent(newObject); } @Override public void onDelete(T t, boolean b) { - propagateEvent(ResourceAction.DELETED, t, null); + propagateEvent(t); } }); } - private void propagateEvent(ResourceAction action, T object, T oldObject) { + private void propagateEvent(T object) { var uids = resourceToUIDs.apply(object); if (uids.isEmpty()) { return; } uids.forEach(uid -> { - InformerEvent event = new InformerEvent(action, object, oldObject); + DefaultEvent event = new DefaultEvent(CustomResourceID.fromResource(object)); this.eventHandler.handleEvent(event); }); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java deleted file mode 100644 index 9e9bfb5040..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java +++ /dev/null @@ -1,11 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event.internal; - -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import io.javaoperatorsdk.operator.processing.event.DefaultEvent; - -public class TimerEvent extends DefaultEvent { - - public TimerEvent(CustomResourceID relatedCustomResourceUid) { - super(relatedCustomResourceUid); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java index 51f21fc4d4..549a0827c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java @@ -13,6 +13,7 @@ import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import io.javaoperatorsdk.operator.processing.event.DefaultEvent; public class TimerEventSource> extends AbstractEventSource { private static final Logger log = LoggerFactory.getLogger(TimerEventSource.class); @@ -94,7 +95,7 @@ public EventProducerTimeTask(CustomResourceID customResourceUid) { public void run() { if (running.get()) { log.debug("Producing event for custom resource id: {}", customResourceUid); - eventHandler.handleEvent(new TimerEvent(customResourceUid)); + eventHandler.handleEvent(new DefaultEvent(customResourceUid)); } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 0bc3c1d016..ddc8e7f355 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -13,11 +13,11 @@ import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import io.javaoperatorsdk.operator.processing.event.DefaultEvent; import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; -import io.javaoperatorsdk.operator.processing.event.internal.TimerEvent; import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource; import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -26,14 +26,9 @@ import static io.javaoperatorsdk.operator.processing.event.internal.ResourceAction.DELETED; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.timeout; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +// todo review all the use cases class DefaultEventHandlerTest { private static final Logger log = LoggerFactory.getLogger(DefaultEventHandlerTest.class); @@ -91,22 +86,6 @@ public void ifExecutionInProgressWaitsUntilItsFinished() throws InterruptedExcep .handleExecution(any()); } - @Test - public void buffersAllIncomingEventsWhileControllerInExecution() { - CustomResourceID resourceUid = eventAlreadyUnderProcessing(); - - defaultEventHandler.handleEvent(nonCREvent(resourceUid)); - defaultEventHandler.handleEvent(prepareCREvent(resourceUid)); - - ArgumentCaptor captor = ArgumentCaptor.forClass(ExecutionScope.class); - verify(eventDispatcherMock, timeout(SEPARATE_EXECUTION_TIMEOUT).times(2)) - .handleExecution(captor.capture()); - List events = captor.getAllValues().get(1).getEvents(); - assertThat(events).hasSize(2); - assertThat(events.get(0)).isInstanceOf(TimerEvent.class); - assertThat(events.get(1)).isInstanceOf(CustomResourceEvent.class); - } - @Test public void cleanUpAfterDeleteEvent() { TestCustomResource customResource = testCustomResource(); @@ -127,7 +106,7 @@ public void schedulesAnEventRetryOnException() { Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); - ExecutionScope executionScope = new ExecutionScope(List.of(event), customResource, null); + ExecutionScope executionScope = new ExecutionScope(customResource, null); PostExecutionControl postExecutionControl = PostExecutionControl.exceptionDuringExecution(new RuntimeException("test")); @@ -160,7 +139,6 @@ public void executesTheControllerInstantlyAfterErrorIfEventsBuffered() { .handleExecution(executionScopeArgumentCaptor.capture()); List allValues = executionScopeArgumentCaptor.getAllValues(); assertThat(allValues).hasSize(2); - assertThat(allValues.get(1).getEvents()).hasSize(2); verify(retryTimerEventSourceMock, never()) .scheduleOnce(eq(customResource), eq(GenericRetry.DEFAULT_INITIAL_INTERVAL)); } @@ -217,6 +195,7 @@ public void scheduleTimedEventIfInstructedByPostExecutionControl() { .scheduleOnce(any(), eq(testDelay)); } + // todo "flakes" from console @Test public void reScheduleOnlyIfNotExecutedBufferedEvents() { var testDelay = 10000l; @@ -269,7 +248,7 @@ private CustomResourceEvent prepareCREvent(CustomResourceID uid) { } private Event nonCREvent(CustomResourceID relatedCustomResourceUid) { - return new TimerEvent(relatedCustomResourceUid); + return new DefaultEvent(relatedCustomResourceUid); } private void overrideData(CustomResourceID id, CustomResource applyTo) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index ce5b5b7367..934128ab5a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -266,7 +266,6 @@ void propagatesRetryInfoToContextIfFinalizerSet() { eventDispatcher.handleExecution( new ExecutionScope( - List.of(), testCustomResource, new RetryInfo() { @Override @@ -318,6 +317,6 @@ public ExecutionScope executionScopeWithCREvent( List eventList = new ArrayList<>(1 + otherEvents.length); eventList.add(event); eventList.addAll(Arrays.asList(otherEvents)); - return new ExecutionScope(eventList, resource, null); + return new ExecutionScope(resource, null); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java deleted file mode 100644 index dc65981cff..0000000000 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java +++ /dev/null @@ -1,24 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event; - -import java.util.Arrays; - -import org.junit.jupiter.api.Test; - -import io.javaoperatorsdk.operator.processing.event.internal.TimerEvent; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; - -class EventListTest { - - @Test - public void returnsLatestOfEventType() { - TimerEvent event2 = new TimerEvent(new CustomResourceID("name1")); - EventList eventList = - new EventList( - Arrays.asList(mock(Event.class), new TimerEvent(new CustomResourceID("name2")), event2, - mock(Event.class))); - - assertThat(eventList.getLatestOfType(TimerEvent.class).get()).isEqualTo(event2); - } -} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomResourceController.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomResourceController.java index f262440cae..d93c7cec70 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomResourceController.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomResourceController.java @@ -41,7 +41,6 @@ public UpdateControl createOrUpdateResource( timerEventSource.schedule(resource, TIMER_DELAY, TIMER_PERIOD); - log.info("Events:: " + context.getEvents()); numberOfExecutions.addAndGet(1); ensureStatusExists(resource); resource.getStatus().setState(EventSourceTestCustomResourceStatus.State.SUCCESS); From a03cfb91e5ccb99a05f271b4920545c7d644bdfe Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 8 Oct 2021 15:34:52 +0200 Subject: [PATCH 27/40] fix: Updated informer mapping to CustomResourceID --- .../event/internal/InformerEventSource.java | 11 ++++--- .../processing/event/internal/Mappers.java | 33 ++++++++++++++----- ...entSourceTestCustomResourceController.java | 2 +- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index f9d4799ef2..34e409fa17 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -12,33 +12,34 @@ import io.fabric8.kubernetes.client.informers.cache.Cache; import io.fabric8.kubernetes.client.informers.cache.Store; import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; public class InformerEventSource extends AbstractEventSource { private final SharedInformer sharedInformer; - private final Function> resourceToUIDs; + private final Function> resourceToUIDs; private final Function associatedWith; private final boolean skipUpdateEventPropagationIfNoChange; public InformerEventSource(SharedInformer sharedInformer, - Function> resourceToUIDs) { + Function> resourceToUIDs) { this(sharedInformer, resourceToUIDs, null, true); } public InformerEventSource(KubernetesClient client, Class type, - Function> resourceToUIDs) { + Function> resourceToUIDs) { this(client, type, resourceToUIDs, false); } InformerEventSource(KubernetesClient client, Class type, - Function> resourceToUIDs, + Function> resourceToUIDs, boolean skipUpdateEventPropagationIfNoChange) { this(client.informers().sharedIndexInformerFor(type, 0), resourceToUIDs, null, skipUpdateEventPropagationIfNoChange); } public InformerEventSource(SharedInformer sharedInformer, - Function> resourceToUIDs, + Function> resourceToUIDs, Function associatedWith, boolean skipUpdateEventPropagationIfNoChange) { this.sharedInformer = sharedInformer; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/Mappers.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/Mappers.java index dc0b4b4501..7351b80d4f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/Mappers.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/Mappers.java @@ -5,27 +5,42 @@ import java.util.function.Function; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; public class Mappers { - public static Function> fromAnnotation( - String annotationKey) { - return fromMetadata(annotationKey, false); + + public static Function> fromAnnotation( + String nameKey) { + return fromMetadata(nameKey, null, false); + } + + public static Function> fromAnnotation( + String nameKey, String namespaceKey) { + return fromMetadata(nameKey, namespaceKey, false); + } + + public static Function> fromLabel( + String nameKey) { + return fromMetadata(nameKey, null, true); } - public static Function> fromLabel( - String labelKey) { - return fromMetadata(labelKey, true); + public static Function> fromLabel( + String nameKey, String namespaceKey) { + return fromMetadata(nameKey, namespaceKey, true); } - private static Function> fromMetadata( - String key, boolean isLabel) { + private static Function> fromMetadata( + String nameKey, String namespaceKey, boolean isLabel) { return resource -> { final var metadata = resource.getMetadata(); if (metadata == null) { return Collections.emptySet(); } else { final var map = isLabel ? metadata.getLabels() : metadata.getAnnotations(); - return map != null ? Set.of(map.get(key)) : Collections.emptySet(); + var namespace = + namespaceKey == null ? resource.getMetadata().getNamespace() : map.get(namespaceKey); + return map != null ? Set.of(new CustomResourceID(map.get(nameKey), namespace)) + : Collections.emptySet(); } }; } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informereventsource/InformerEventSourceTestCustomResourceController.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informereventsource/InformerEventSourceTestCustomResourceController.java index e5c44198b4..e5c34e1610 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informereventsource/InformerEventSourceTestCustomResourceController.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informereventsource/InformerEventSourceTestCustomResourceController.java @@ -27,7 +27,7 @@ public class InformerEventSourceTestCustomResourceController implements private static final Logger LOGGER = LoggerFactory.getLogger(InformerEventSourceTestCustomResourceController.class); - public static final String RELATED_RESOURCE_UID = "relatedResourceUID"; + public static final String RELATED_RESOURCE_UID = "relatedResourceName"; public static final String TARGET_CONFIG_MAP_KEY = "targetStatus"; private KubernetesClient kubernetesClient; From f97ced55fbf556b7fd79863ec7faf21c10bdbda5 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 8 Oct 2021 15:53:21 +0200 Subject: [PATCH 28/40] fix: imports --- .../operator/processing/event/internal/InformerEventSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index b6b1314275..42b1c94084 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -14,7 +14,6 @@ import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; public class InformerEventSource extends AbstractEventSource { From 109b7bc2fdab982c41153d098be30b55e43493eb Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 11 Oct 2021 10:00:34 +0200 Subject: [PATCH 29/40] fix: decorational changes --- .../operator/processing/DefaultEventHandler.java | 5 ++--- .../operator/processing/event/DefaultEvent.java | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 02e0f4983e..0dc04285cb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -198,9 +198,8 @@ void eventProcessingFinished( if (retry != null && postExecutionControl.exceptionDuringExecution()) { handleRetryOnException(executionScope); // todo revisit monitoring since events are not present anymore - // final var monitor = monitor(); - // executionScope.getEvents() - // .forEach(e -> monitor.failedEvent(executionScope.getCustomResourceID(), e)); + // final var monitor = monitor(); executionScope.getEvents().forEach(e -> + // monitor.failedEvent(executionScope.getCustomResourceID(), e)); return; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java index c445f2bf27..7c55939da5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java @@ -2,14 +2,13 @@ @SuppressWarnings("rawtypes") public class DefaultEvent implements Event { - private final CustomResourceID relatedCustomResource; + private final CustomResourceID relatedCustomResource; public DefaultEvent(CustomResourceID targetCustomResource) { this.relatedCustomResource = targetCustomResource; } - @Override public CustomResourceID getRelatedCustomResourceID() { return relatedCustomResource; From 874c25ecab82c687581823b52d3c7e11da2b87fc Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 11 Oct 2021 10:21:09 +0200 Subject: [PATCH 30/40] fix: event marker unit test --- .../operator/processing/EventMarkerTest.java | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java new file mode 100644 index 0000000000..24bf2c088e --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java @@ -0,0 +1,69 @@ +package io.javaoperatorsdk.operator.processing; + +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; +import org.junit.jupiter.api.Test; + +import static io.javaoperatorsdk.operator.processing.EventMarker.EventingState.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.*; + +class EventMarkerTest { + + private final EventMarker eventMarker = new EventMarker(); + private CustomResourceID sampleCustomResourceID = new CustomResourceID("test-name"); + + @Test + public void returnsNoEventPresentIfNotMarkedYet() { + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + } + + @Test + public void marksEvent() { + eventMarker.markEventReceived(sampleCustomResourceID); + + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(EVENT_PRESENT); + assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); + } + + @Test + public void markEventAwareOfDeleteEvent() { + eventMarker.markDeleteEventReceived(sampleCustomResourceID); + + eventMarker.markEventReceived(sampleCustomResourceID); + + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); + } + + @Test + public void marksDeleteEvent() { + eventMarker.markDeleteEventReceived(sampleCustomResourceID); + + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(ONLY_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + } + + @Test + public void markDeleteEventAwareOfEvent() { + eventMarker.markEventReceived(sampleCustomResourceID); + + eventMarker.markDeleteEventReceived(sampleCustomResourceID); + + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + } + + @Test + public void cleansUp() { + eventMarker.markEventReceived(sampleCustomResourceID); + eventMarker.markDeleteEventReceived(sampleCustomResourceID); + + eventMarker.cleanup(sampleCustomResourceID); + + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + } + +} From ff2b32a48bbb7638e31e4d1899d0b13f18ffb6a3 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 11 Oct 2021 14:47:00 +0200 Subject: [PATCH 31/40] Default Event Handler Unit tests --- .../processing/DefaultEventHandler.java | 14 +-- .../event/internal/CustomResourceEvent.java | 13 +-- .../internal/CustomResourceEventSource.java | 3 +- .../processing/DefaultEventHandlerTest.java | 51 ++++++----- .../processing/EventDispatcherTest.java | 4 +- .../operator/processing/EventMarkerTest.java | 87 ++++++++++--------- 6 files changed, 89 insertions(+), 83 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 0dc04285cb..f65ceef64b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -39,7 +39,6 @@ public class DefaultEventHandler> implements Even @Deprecated private static EventMonitor monitor = EventMonitor.NOOP; - private final EventMarker eventMarker; private final Set underProcessing = new HashSet<>(); private final EventDispatcher eventDispatcher; private final Retry retry; @@ -51,6 +50,7 @@ public class DefaultEventHandler> implements Even private volatile boolean running; private final ResourceCache resourceCache; private DefaultEventSourceManager eventSourceManager; + private final EventMarker eventMarker; public DefaultEventHandler(ConfiguredController controller, ResourceCache resourceCache) { this( @@ -59,18 +59,20 @@ public DefaultEventHandler(ConfiguredController controller, ResourceCache controller.getConfiguration().getName(), new EventDispatcher<>(controller), GenericRetry.fromConfiguration(controller.getConfiguration().getRetryConfiguration()), - controller.getConfiguration().getConfigurationService().getMetrics().getEventMonitor()); + controller.getConfiguration().getConfigurationService().getMetrics().getEventMonitor(), + new EventMarker()); } DefaultEventHandler(EventDispatcher eventDispatcher, ResourceCache resourceCache, String relatedControllerName, - Retry retry) { - this(resourceCache, null, relatedControllerName, eventDispatcher, retry, null); + Retry retry, EventMarker eventMarker) { + this(resourceCache, null, relatedControllerName, eventDispatcher, retry, null, eventMarker); } private DefaultEventHandler(ResourceCache resourceCache, ExecutorService executor, String relatedControllerName, - EventDispatcher eventDispatcher, Retry retry, EventMonitor monitor) { + EventDispatcher eventDispatcher, Retry retry, EventMonitor monitor, + EventMarker eventMarker) { this.running = true; this.executor = executor == null @@ -82,7 +84,7 @@ private DefaultEventHandler(ResourceCache resourceCache, ExecutorService exec this.retry = retry; this.resourceCache = resourceCache; this.eventMonitor = monitor != null ? monitor : EventMonitor.NOOP; - this.eventMarker = new EventMarker(); + this.eventMarker = eventMarker; } public void setEventSourceManager(DefaultEventSourceManager eventSourceManager) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java index 15a67108db..0c20369e0a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java @@ -1,19 +1,15 @@ package io.javaoperatorsdk.operator.processing.event.internal; -import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; public class CustomResourceEvent extends DefaultEvent { private final ResourceAction action; - private final CustomResource customResource; - public CustomResourceEvent(ResourceAction action, - CustomResource resource) { - super(CustomResourceID.fromResource(resource)); - this.customResource = resource; + CustomResourceID customResourceID) { + super(customResourceID); this.action = action; } @@ -21,14 +17,9 @@ public CustomResourceEvent(ResourceAction action, public String toString() { return "CustomResourceEvent{" + "action=" + action + - ", customResource=" + customResource + '}'; } - public CustomResource getCustomResource() { - return customResource; - } - public ResourceAction getAction() { return action; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 9964e7b450..3b56e5b685 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -112,7 +112,8 @@ public void eventReceived(ResourceAction action, T customResource, T oldResource CustomResourceEventFilters.generationAware())); if (filter.acceptChange(controller.getConfiguration(), oldResource, customResource)) { - eventHandler.handleEvent(new CustomResourceEvent(action, clone(customResource))); + eventHandler.handleEvent( + new CustomResourceEvent(action, CustomResourceID.fromResource(customResource))); } else { log.debug( "Skipping event handling resource {} with version: {}", diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index ddc8e7f355..dc4b53c4f9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -28,7 +28,6 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; -// todo review all the use cases class DefaultEventHandlerTest { private static final Logger log = LoggerFactory.getLogger(DefaultEventHandlerTest.class); @@ -36,6 +35,7 @@ class DefaultEventHandlerTest { public static final int FAKE_CONTROLLER_EXECUTION_DURATION = 250; public static final int SEPARATE_EXECUTION_TIMEOUT = 450; public static final String TEST_NAMESPACE = "default-event-handler-test"; + private EventMarker eventMarker = new EventMarker(); private EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); private DefaultEventSourceManager defaultEventSourceManagerMock = mock(DefaultEventSourceManager.class); @@ -44,11 +44,11 @@ class DefaultEventHandlerTest { private TimerEventSource retryTimerEventSourceMock = mock(TimerEventSource.class); private DefaultEventHandler defaultEventHandler = - new DefaultEventHandler(eventDispatcherMock, resourceCache, "Test", null); + new DefaultEventHandler(eventDispatcherMock, resourceCache, "Test", null, eventMarker); private DefaultEventHandler defaultEventHandlerWithRetry = new DefaultEventHandler(eventDispatcherMock, resourceCache, "Test", - GenericRetry.defaultLimitedExponentialRetry()); + GenericRetry.defaultLimitedExponentialRetry(), eventMarker); @BeforeEach public void setup() { @@ -86,24 +86,8 @@ public void ifExecutionInProgressWaitsUntilItsFinished() throws InterruptedExcep .handleExecution(any()); } - @Test - public void cleanUpAfterDeleteEvent() { - TestCustomResource customResource = testCustomResource(); - when(resourceCache.getCustomResource(CustomResourceID.fromResource(customResource))) - .thenReturn(Optional.of(customResource)); - CustomResourceEvent event = - new CustomResourceEvent(DELETED, customResource); - - defaultEventHandler.handleEvent(event); - - waitMinimalTime(); - verify(defaultEventSourceManagerMock, times(1)) - .cleanup(CustomResourceID.fromResource(customResource)); - } - @Test public void schedulesAnEventRetryOnException() { - Event event = prepareCREvent(); TestCustomResource customResource = testCustomResource(); ExecutionScope executionScope = new ExecutionScope(customResource, null); @@ -195,7 +179,6 @@ public void scheduleTimedEventIfInstructedByPostExecutionControl() { .scheduleOnce(any(), eq(testDelay)); } - // todo "flakes" from console @Test public void reScheduleOnlyIfNotExecutedBufferedEvents() { var testDelay = 10000l; @@ -225,6 +208,31 @@ private void waitMinimalTime() { } } + @Test + public void cleansUpWhenDeleteEventReceivedAndNoEventPresent() { + Event deleteEvent = + new CustomResourceEvent(DELETED, prepareCREvent().getRelatedCustomResourceID()); + + defaultEventHandler.handleEvent(deleteEvent); + + verify(defaultEventSourceManagerMock, times(1)) + .cleanup(eq(deleteEvent.getRelatedCustomResourceID())); + } + + @Test + public void cleansUpAfterExecutionIfOnlyDeleteEventMarkLeft() { + var cr = testCustomResource(new CustomResourceID(UUID.randomUUID().toString())); + var crEvent = prepareCREvent(CustomResourceID.fromResource(cr)); + eventMarker.markDeleteEventReceived(crEvent.getRelatedCustomResourceID()); + var executionScope = new ExecutionScope(cr, null); + + defaultEventHandler.eventProcessingFinished(executionScope, + PostExecutionControl.defaultDispatch()); + + verify(defaultEventSourceManagerMock, times(1)) + .cleanup(eq(crEvent.getRelatedCustomResourceID())); + } + private CustomResourceID eventAlreadyUnderProcessing() { when(eventDispatcherMock.handleExecution(any())) .then( @@ -244,7 +252,8 @@ private CustomResourceEvent prepareCREvent() { private CustomResourceEvent prepareCREvent(CustomResourceID uid) { TestCustomResource customResource = testCustomResource(uid); when(resourceCache.getCustomResource(eq(uid))).thenReturn(Optional.of(customResource)); - return new CustomResourceEvent(ResourceAction.UPDATED, customResource); + return new CustomResourceEvent(ResourceAction.UPDATED, + CustomResourceID.fromResource(customResource)); } private Event nonCREvent(CustomResourceID relatedCustomResourceUid) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index 934128ab5a..1866fefeb9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -19,6 +19,7 @@ import io.javaoperatorsdk.operator.api.UpdateControl; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEvent; import io.javaoperatorsdk.operator.processing.event.internal.ResourceAction; @@ -313,7 +314,8 @@ private void removeFinalizers(CustomResource customResource) { public ExecutionScope executionScopeWithCREvent( ResourceAction action, CustomResource resource, Event... otherEvents) { - CustomResourceEvent event = new CustomResourceEvent(action, resource); + CustomResourceEvent event = + new CustomResourceEvent(action, CustomResourceID.fromResource(resource)); List eventList = new ArrayList<>(1 + otherEvents.length); eventList.add(event); eventList.addAll(Arrays.asList(otherEvents)); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java index 24bf2c088e..f7d3f5c6ad 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java @@ -1,69 +1,70 @@ package io.javaoperatorsdk.operator.processing; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import org.junit.jupiter.api.Test; +import io.javaoperatorsdk.operator.processing.event.CustomResourceID; + import static io.javaoperatorsdk.operator.processing.EventMarker.EventingState.*; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; class EventMarkerTest { - private final EventMarker eventMarker = new EventMarker(); - private CustomResourceID sampleCustomResourceID = new CustomResourceID("test-name"); + private final EventMarker eventMarker = new EventMarker(); + private CustomResourceID sampleCustomResourceID = new CustomResourceID("test-name"); - @Test - public void returnsNoEventPresentIfNotMarkedYet() { - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); - } + @Test + public void returnsNoEventPresentIfNotMarkedYet() { + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + } - @Test - public void marksEvent() { - eventMarker.markEventReceived(sampleCustomResourceID); + @Test + public void marksEvent() { + eventMarker.markEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(EVENT_PRESENT); - assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); - } + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(EVENT_PRESENT); + assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); + } - @Test - public void markEventAwareOfDeleteEvent() { - eventMarker.markDeleteEventReceived(sampleCustomResourceID); + @Test + public void markEventAwareOfDeleteEvent() { + eventMarker.markDeleteEventReceived(sampleCustomResourceID); - eventMarker.markEventReceived(sampleCustomResourceID); + eventMarker.markEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); - } + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); + } - @Test - public void marksDeleteEvent() { - eventMarker.markDeleteEventReceived(sampleCustomResourceID); + @Test + public void marksDeleteEvent() { + eventMarker.markDeleteEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(ONLY_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); - } + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(ONLY_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + } - @Test - public void markDeleteEventAwareOfEvent() { - eventMarker.markEventReceived(sampleCustomResourceID); + @Test + public void markDeleteEventAwareOfEvent() { + eventMarker.markEventReceived(sampleCustomResourceID); - eventMarker.markDeleteEventReceived(sampleCustomResourceID); + eventMarker.markDeleteEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); - } + assertThat(eventMarker.getEventingState(sampleCustomResourceID)) + .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); + assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + } - @Test - public void cleansUp() { - eventMarker.markEventReceived(sampleCustomResourceID); - eventMarker.markDeleteEventReceived(sampleCustomResourceID); + @Test + public void cleansUp() { + eventMarker.markEventReceived(sampleCustomResourceID); + eventMarker.markDeleteEventReceived(sampleCustomResourceID); - eventMarker.cleanup(sampleCustomResourceID); + eventMarker.cleanup(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); - } + assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + } } From b6c87f0c268a0dc0835a41e8946cf0c424dd914f Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 12 Oct 2021 17:19:10 +0200 Subject: [PATCH 32/40] fix: fixes after merge --- .../operator/processing/event/internal/TimerEventSource.java | 1 - .../javaoperatorsdk/operator/processing/EventDispatcherTest.java | 1 - 2 files changed, 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java index 97c39219dc..549a0827c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java @@ -14,7 +14,6 @@ import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.DefaultEvent; -import io.javaoperatorsdk.operator.processing.event.CustomResourceID; public class TimerEventSource> extends AbstractEventSource { private static final Logger log = LoggerFactory.getLogger(TimerEventSource.class); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java index 8890568c4f..e1e9722f96 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java @@ -268,7 +268,6 @@ void propagatesRetryInfoToContextIfFinalizerSet() { eventDispatcher.handleExecution( new ExecutionScope( - List.of(), testCustomResource, new RetryInfo() { @Override From af50089432e20be0353c97b3f672e3bf6d477a64 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 13:08:02 +0200 Subject: [PATCH 33/40] fix: changes from code review --- .../operator/api/ResourceController.java | 7 ++- .../processing/DefaultEventHandler.java | 36 +++++------ .../operator/processing/EventMarker.java | 60 +++++++++---------- .../operator/processing/EventMarkerTest.java | 45 +++++++------- 4 files changed, 69 insertions(+), 79 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java index 6d1adb84b7..7055f0d3bb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java @@ -6,12 +6,15 @@ public interface ResourceController { /** + * Note that this method is used in combination of finalizers. If automatic finalizer handling is + * turned off for the controller, this method is not called. + * * The implementation should delete the associated component(s). Note that this is method is * called when an object is marked for deletion. After it's executed the custom resource finalizer * is automatically removed by the framework; unless the return value is * {@link DeleteControl#NO_FINALIZER_REMOVAL}, which indicates that the controller has determined - * that the resource should not be deleted yet, in which case it is up to the controller to - * restore the resource's status so that it's not marked for deletion anymore. + * that the resource should not be deleted yet. This is usually a corner case, when a cleanup is + * tried again eventually. * *

* It's important that this method be idempotent, as it could be called several times, depending diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index f9c25db5e9..4508c04f98 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -127,7 +127,7 @@ public void handleEvent(Event event) { monitor.processedEvent(event.getRelatedCustomResourceID(), event); markEvent(event); - if (eventMarker.isEventPresent(event.getRelatedCustomResourceID())) { + if (!eventMarker.deleteEventPresent(event.getRelatedCustomResourceID())) { submitReconciliationExecution(event.getRelatedCustomResourceID()); } else { cleanupForDeletedEvent(event.getRelatedCustomResourceID()); @@ -138,12 +138,11 @@ public void handleEvent(Event event) { } private boolean submitReconciliationExecution(CustomResourceID customResourceUid) { - boolean newEventForResourceId = eventMarker.isEventPresent(customResourceUid); boolean controllerUnderExecution = isControllerUnderExecution(customResourceUid); Optional latestCustomResource = resourceCache.getCustomResource(customResourceUid); - if (!controllerUnderExecution && newEventForResourceId + if (!controllerUnderExecution && latestCustomResource.isPresent()) { setUnderExecutionProcessing(customResourceUid); ExecutionScope executionScope = @@ -156,10 +155,9 @@ private boolean submitReconciliationExecution(CustomResourceID customResourceUid return true; } else { log.debug( - "Skipping executing controller for resource id: {}. Events in queue: {}." + "Skipping executing controller for resource id: {}." + " Controller in execution: {}. Latest CustomResource present: {}", customResourceUid, - newEventForResourceId, controllerUnderExecution, latestCustomResource.isPresent()); if (latestCustomResource.isEmpty()) { @@ -174,7 +172,7 @@ private void markEvent(Event event) { if (event instanceof CustomResourceEvent && ((CustomResourceEvent) event).getAction() == ResourceAction.DELETED) { eventMarker.markDeleteEventReceived(event); - } else if (!eventMarker.isDeleteEventPresent(event.getRelatedCustomResourceID())) { + } else if (!eventMarker.deleteEventPresent(event.getRelatedCustomResourceID())) { eventMarker.markEventReceived(event); } } @@ -197,7 +195,11 @@ void eventProcessingFinished( postExecutionControl); unsetUnderExecution(executionScope.getCustomResourceID()); - if (retry != null && postExecutionControl.exceptionDuringExecution()) { + // If a delete event present it was received during reconciliation. + // So we either removed the finalizer during reconciliation or we don't use one + // for the cr. Neither way we want to retry. + if (retry != null && postExecutionControl.exceptionDuringExecution() && + !eventMarker.deleteEventPresent(executionScope.getCustomResourceID())) { handleRetryOnException(executionScope); // todo revisit monitoring since events are not present anymore // final var monitor = monitor(); executionScope.getEvents().forEach(e -> @@ -208,11 +210,12 @@ void eventProcessingFinished( if (retry != null) { handleSuccessfulExecutionRegardingRetry(executionScope); } - if (readyForCleanup(executionScope.getCustomResourceID())) { + if (eventMarker.deleteEventPresent(executionScope.getCustomResourceID())) { cleanupForDeletedEvent(executionScope.getCustomResourceID()); } else { - var executed = submitReconciliationExecution(executionScope.getCustomResourceID()); - if (!executed) { + if (eventMarker.eventPresent(executionScope.getCustomResourceID())) { + submitReconciliationExecution(executionScope.getCustomResourceID()); + } else { reScheduleExecutionIfInstructed(postExecutionControl, executionScope.getCustomResource()); } @@ -222,11 +225,6 @@ void eventProcessingFinished( } } - private boolean readyForCleanup(CustomResourceID customResourceID) { - return eventMarker - .getEventingState(customResourceID) == EventMarker.EventingState.ONLY_DELETE_EVENT_PRESENT; - } - private void reScheduleExecutionIfInstructed(PostExecutionControl postExecutionControl, R customResource) { postExecutionControl.getReScheduleDelay().ifPresent(delay -> eventSourceManager @@ -242,14 +240,10 @@ private void reScheduleExecutionIfInstructed(PostExecutionControl postExecuti private void handleRetryOnException(ExecutionScope executionScope) { RetryExecution execution = getOrInitRetryExecution(executionScope); var customResourceID = executionScope.getCustomResourceID(); - EventMarker.EventingState eventingState = - eventMarker.getEventingState(customResourceID); - boolean newEventsExists = - eventingState == EventMarker.EventingState.EVENT_PRESENT || - eventingState == EventMarker.EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT; + boolean eventPresent = eventMarker.eventPresent(customResourceID); eventMarker.markEventReceived(customResourceID); - if (newEventsExists) { + if (eventPresent) { log.debug("New events exists for for resource id: {}", customResourceID); submitReconciliationExecution(customResourceID); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java index 96a271739b..030e8bba81 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java @@ -5,20 +5,31 @@ import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; +/** + * Manages the state of received events. Basically there can be only three distinct states relevant + * for event processing. Either an even is received, so we eventually process or no event for + * processing at the moment. The third case is if a DELETE event is received, this is a special case + * meaning that the custom resource is deleted. We don't want to do any processing anymore is other + * events are irrelevant for us from this point. Note that the dependant resources are either + * cleaned up by K8S garbage collection or by the controller implementation for cleanup. + */ public class EventMarker { public enum EventingState { - EVENT_PRESENT, ONLY_DELETE_EVENT_PRESENT, DELETE_AND_NON_DELETE_EVENT_PRESENT, NO_EVENT_PRESENT + /** Event but NOT Delete event present */ + EVENT_PRESENT, NO_EVENT_PRESENT, + /** Delete event present, from this point other events are not relevant */ + DELETE_EVENT_PRESENT, } private final HashMap eventingState = new HashMap<>(); - public EventingState getEventingState(CustomResourceID customResourceID) { + private EventingState getEventingState(CustomResourceID customResourceID) { EventingState actualState = eventingState.get(customResourceID); return actualState == null ? EventingState.NO_EVENT_PRESENT : actualState; } - public void setEventingState(CustomResourceID customResourceID, EventingState state) { + private void setEventingState(CustomResourceID customResourceID, EventingState state) { eventingState.put(customResourceID, state); } @@ -27,16 +38,10 @@ public void markEventReceived(Event event) { } public void markEventReceived(CustomResourceID customResourceID) { - var actualState = getEventingState(customResourceID); - switch (actualState) { - case ONLY_DELETE_EVENT_PRESENT: - setEventingState(customResourceID, - EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT); - break; - case NO_EVENT_PRESENT: - setEventingState(customResourceID, EventingState.EVENT_PRESENT); - break; + if (deleteEventPresent(customResourceID)) { + throw new IllegalStateException("Cannot receive event after a delete event received"); } + setEventingState(customResourceID, EventingState.EVENT_PRESENT); } public void unMarkEventReceived(CustomResourceID customResourceID) { @@ -46,10 +51,8 @@ public void unMarkEventReceived(CustomResourceID customResourceID) { setEventingState(customResourceID, EventingState.NO_EVENT_PRESENT); break; - case DELETE_AND_NON_DELETE_EVENT_PRESENT: - setEventingState(customResourceID, - EventingState.ONLY_DELETE_EVENT_PRESENT); - break; + case DELETE_EVENT_PRESENT: + throw new IllegalStateException("Cannot unmark delete event."); } } @@ -58,28 +61,21 @@ public void markDeleteEventReceived(Event event) { } public void markDeleteEventReceived(CustomResourceID customResourceID) { - var actualState = getEventingState(customResourceID); - switch (actualState) { - case NO_EVENT_PRESENT: - setEventingState(customResourceID, EventingState.ONLY_DELETE_EVENT_PRESENT); - break; - case EVENT_PRESENT: - setEventingState(customResourceID, - EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT); - break; - } + setEventingState(customResourceID, EventingState.DELETE_EVENT_PRESENT); + } + + public boolean deleteEventPresent(CustomResourceID customResourceID) { + return getEventingState(customResourceID) == EventingState.DELETE_EVENT_PRESENT; } - public boolean isEventPresent(CustomResourceID customResourceID) { + public boolean eventPresent(CustomResourceID customResourceID) { var actualState = getEventingState(customResourceID); - return actualState == EventingState.EVENT_PRESENT || - actualState == EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT; + return actualState == EventingState.EVENT_PRESENT; } - public boolean isDeleteEventPresent(CustomResourceID customResourceID) { + public boolean noEventPresent(CustomResourceID customResourceID) { var actualState = getEventingState(customResourceID); - return actualState == EventingState.DELETE_AND_NON_DELETE_EVENT_PRESENT || - actualState == EventingState.ONLY_DELETE_EVENT_PRESENT; + return actualState == EventingState.NO_EVENT_PRESENT; } public void cleanup(CustomResourceID customResourceID) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java index f7d3f5c6ad..98b87f1713 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java @@ -1,12 +1,11 @@ package io.javaoperatorsdk.operator.processing; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; -import static io.javaoperatorsdk.operator.processing.EventMarker.EventingState.*; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.*; class EventMarkerTest { @@ -15,46 +14,35 @@ class EventMarkerTest { @Test public void returnsNoEventPresentIfNotMarkedYet() { - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + assertThat(eventMarker.noEventPresent(sampleCustomResourceID)).isTrue(); } @Test public void marksEvent() { eventMarker.markEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(EVENT_PRESENT); - assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); - } - - @Test - public void markEventAwareOfDeleteEvent() { - eventMarker.markDeleteEventReceived(sampleCustomResourceID); - - eventMarker.markEventReceived(sampleCustomResourceID); - - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isEventPresent(sampleCustomResourceID)).isTrue(); + assertThat(eventMarker.eventPresent(sampleCustomResourceID)).isTrue(); + assertThat(eventMarker.deleteEventPresent(sampleCustomResourceID)).isFalse(); } @Test public void marksDeleteEvent() { eventMarker.markDeleteEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(ONLY_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + assertThat(eventMarker.deleteEventPresent(sampleCustomResourceID)) + .isTrue(); + assertThat(eventMarker.eventPresent(sampleCustomResourceID)).isFalse(); } @Test - public void markDeleteEventAwareOfEvent() { + public void afterDeleteEventMarkEventIsNotRelevant() { eventMarker.markEventReceived(sampleCustomResourceID); eventMarker.markDeleteEventReceived(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)) - .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); - assertThat(eventMarker.isDeleteEventPresent(sampleCustomResourceID)).isTrue(); + assertThat(eventMarker.deleteEventPresent(sampleCustomResourceID)) + .isTrue(); + assertThat(eventMarker.eventPresent(sampleCustomResourceID)).isFalse(); } @Test @@ -64,7 +52,16 @@ public void cleansUp() { eventMarker.cleanup(sampleCustomResourceID); - assertThat(eventMarker.getEventingState(sampleCustomResourceID)).isEqualTo(NO_EVENT_PRESENT); + assertThat(eventMarker.deleteEventPresent(sampleCustomResourceID)).isFalse(); + assertThat(eventMarker.eventPresent(sampleCustomResourceID)).isFalse(); + } + + @Test + public void cannotMarkEventAfterDeleteEventReceived() { + Assertions.assertThrows(IllegalStateException.class, () -> { + eventMarker.markDeleteEventReceived(sampleCustomResourceID); + eventMarker.markEventReceived(sampleCustomResourceID); + }); } } From 58c3f5c52d973fdf699b5357b8964964b79d5fe5 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 13:12:09 +0200 Subject: [PATCH 34/40] fix: method naming --- .../operator/processing/DefaultEventHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index 4508c04f98..df6818b7af 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -126,7 +126,7 @@ public void handleEvent(Event event) { final var monitor = monitor(); monitor.processedEvent(event.getRelatedCustomResourceID(), event); - markEvent(event); + handleEventMarking(event); if (!eventMarker.deleteEventPresent(event.getRelatedCustomResourceID())) { submitReconciliationExecution(event.getRelatedCustomResourceID()); } else { @@ -168,7 +168,7 @@ private boolean submitReconciliationExecution(CustomResourceID customResourceUid } } - private void markEvent(Event event) { + private void handleEventMarking(Event event) { if (event instanceof CustomResourceEvent && ((CustomResourceEvent) event).getAction() == ResourceAction.DELETED) { eventMarker.markDeleteEventReceived(event); From 6635b5464495140322b66f03c3185e3f0d99ed13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 13 Oct 2021 15:07:04 +0200 Subject: [PATCH 35/40] Update operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java Co-authored-by: Chris Laprun --- .../io/javaoperatorsdk/operator/processing/EventMarker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java index 030e8bba81..59d2264a38 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java @@ -7,7 +7,7 @@ /** * Manages the state of received events. Basically there can be only three distinct states relevant - * for event processing. Either an even is received, so we eventually process or no event for + * for event processing. Either an event is received, so we eventually process or no event for * processing at the moment. The third case is if a DELETE event is received, this is a special case * meaning that the custom resource is deleted. We don't want to do any processing anymore is other * events are irrelevant for us from this point. Note that the dependant resources are either From f40ee4e7b616a14f1b00597c4fd5f98570bfc777 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 13 Oct 2021 15:07:12 +0200 Subject: [PATCH 36/40] Update operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java Co-authored-by: Chris Laprun --- .../io/javaoperatorsdk/operator/processing/EventMarker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java index 59d2264a38..9be023416b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java @@ -9,7 +9,7 @@ * Manages the state of received events. Basically there can be only three distinct states relevant * for event processing. Either an event is received, so we eventually process or no event for * processing at the moment. The third case is if a DELETE event is received, this is a special case - * meaning that the custom resource is deleted. We don't want to do any processing anymore is other + * meaning that the custom resource is deleted. We don't want to do any processing anymore so other * events are irrelevant for us from this point. Note that the dependant resources are either * cleaned up by K8S garbage collection or by the controller implementation for cleanup. */ From 73c32a8c63e020423d4abc580af93120b2b18d4e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 15:11:25 +0200 Subject: [PATCH 37/40] fix: comment --- .../operator/processing/DefaultEventHandler.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java index df6818b7af..cdd14f6b97 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java @@ -195,9 +195,9 @@ void eventProcessingFinished( postExecutionControl); unsetUnderExecution(executionScope.getCustomResourceID()); - // If a delete event present it was received during reconciliation. - // So we either removed the finalizer during reconciliation or we don't use one - // for the cr. Neither way we want to retry. + // If a delete event present at this phase, it was received during reconciliation. + // So we either removed the finalizer during reconciliation or we don't use finalizers. + // Either way we don't want to retry. if (retry != null && postExecutionControl.exceptionDuringExecution() && !eventMarker.deleteEventPresent(executionScope.getCustomResourceID())) { handleRetryOnException(executionScope); From e75cad4b27bdbdd7070d0b2b963938e23d6c1e1c Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 15:28:12 +0200 Subject: [PATCH 38/40] fix: fixes from merge --- .../operator/processing/DefaultEventHandlerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index a8cd0a299f..3e9b4ecc4c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -216,7 +216,7 @@ public void cleansUpWhenDeleteEventReceivedAndNoEventPresent() { defaultEventHandler.handleEvent(deleteEvent); verify(defaultEventSourceManagerMock, times(1)) - .cleanup(eq(deleteEvent.getRelatedCustomResourceID())); + .cleanupForCustomResource(eq(deleteEvent.getRelatedCustomResourceID())); } @Test @@ -230,7 +230,7 @@ public void cleansUpAfterExecutionIfOnlyDeleteEventMarkLeft() { PostExecutionControl.defaultDispatch()); verify(defaultEventSourceManagerMock, times(1)) - .cleanup(eq(crEvent.getRelatedCustomResourceID())); + .cleanupForCustomResource(eq(crEvent.getRelatedCustomResourceID())); } private CustomResourceID eventAlreadyUnderProcessing() { From b9fc1fc31ba3ebdd9e635e1f782e4b5c87071b8e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 15:30:01 +0200 Subject: [PATCH 39/40] fix: remove not used method --- .../operator/processing/DefaultEventHandlerTest.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 3e9b4ecc4c..588c8f58e5 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -199,15 +199,7 @@ public void doNotFireEventsIfClosing() { verify(eventDispatcherMock, timeout(50).times(0)).handleExecution(any()); } - - private void waitMinimalTime() { - try { - Thread.sleep(50); - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } - } - + @Test public void cleansUpWhenDeleteEventReceivedAndNoEventPresent() { Event deleteEvent = From 0d409ee86bd90bb0740268a66f3739fdeb4a219a Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 13 Oct 2021 15:31:32 +0200 Subject: [PATCH 40/40] fix: formatting --- .../operator/processing/DefaultEventHandlerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java index 588c8f58e5..1ecc551951 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java @@ -199,7 +199,7 @@ public void doNotFireEventsIfClosing() { verify(eventDispatcherMock, timeout(50).times(0)).handleExecution(any()); } - + @Test public void cleansUpWhenDeleteEventReceivedAndNoEventPresent() { Event deleteEvent =