diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java index 9743632404..5a87659246 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java @@ -253,6 +253,10 @@ default ResourceEventRecorder eventRecorder() { * reconciliation is already scheduled, which would in turn trigger another status update, thus * rendering the current one moot. * + *

This holds regardless of whether the current reconciliation succeeds or throws, so with + * {@link ControllerConfiguration#triggerReconcilerOnAllEvents()} it also covers an event that + * arrived after a delete event. + * * @return {@code true} is another reconciliation is already scheduled, {@code false} otherwise */ boolean isNextReconciliationImminent(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index 8931e49486..1b048123a0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -380,9 +380,7 @@ private void handleRetryOnException( ExecutionScope

executionScope, Exception exception, boolean errorHandledByReconciler) { final var state = getOrInitRetryExecution(executionScope); var resourceID = state.getId(); - boolean eventPresent = - state.eventPresent() - || (triggerOnAllEvents() && state.isAdditionalEventPresentAfterDeleteEvent()); + boolean eventPresent = isNextReconciliationImminent(state); state.markEventReceived(); retryAwareErrorLogging( state.getRetry(), eventPresent, errorHandledByReconciler, exception, executionScope); @@ -510,8 +508,20 @@ public synchronized void start() throws OperatorException { handleAlreadyMarkedEvents(); } - public boolean isNextReconciliationImminent(ResourceID resourceID) { - return resourceStateManager.getOrCreate(resourceID).eventPresent(); + public synchronized boolean isNextReconciliationImminent(ResourceID resourceID) { + return isNextReconciliationImminent(resourceStateManager.getOrCreate(resourceID)); + } + + /** + * An event that arrives after a delete event is tracked in a dedicated state, so {@link + * ResourceState#eventPresent()} alone does not cover it. Such an event triggers a new + * reconciliation right after the current one, both when it succeeds (see {@link + * #eventProcessingFinished}) and when it fails (see {@link #handleRetryOnException}), so it has + * to be reported as imminent too. + */ + private boolean isNextReconciliationImminent(ResourceState state) { + return state.eventPresent() + || (triggerOnAllEvents() && state.isAdditionalEventPresentAfterDeleteEvent()); } private void handleAlreadyMarkedEvents() { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventIT.java index 0f193d9440..a8b022b642 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventIT.java @@ -192,14 +192,16 @@ void additionalEventDuringRetryOnDeleteEvent() { await() .untilAsserted( () -> { - assertThat(reconciler.isWaiting()); + assertThat(reconciler.isWaiting()).isTrue(); }); // trigger reconciliation while waiting in reconciler res = getResource(); res.getMetadata().getAnnotations().put("my-annotation", "true"); extension.update(res); - // continue reconciliation + // continue reconciliation; the reconciler additionally waits until the framework actually + // registered the event above, otherwise the failure below would consume a retry attempt + // instead of being instantly re-triggered by the superseding event reconciler.setContinuerOnRetryWait(true); await() diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventReconciler.java index f8804bd25d..8849993f52 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/triggerallevent/eventing/TriggerReconcilerOnAllEventReconciler.java @@ -35,6 +35,10 @@ public class TriggerReconcilerOnAllEventReconciler public static final String ADDITIONAL_FINALIZER = "all.event.mode/finalizer2"; public static final String NO_MORE_EXCEPTION_ANNOTATION_KEY = "no.more.exception"; + // safety net so a missing event does not block the reconciler thread forever, the test assertions + // fail long before this elapses + private static final long MAX_WAIT_FOR_SUPERSEDING_EVENT_MILLIS = 30_000; + private static final Logger log = LoggerFactory.getLogger(TriggerReconcilerOnAllEventReconciler.class); @@ -47,6 +51,7 @@ public class TriggerReconcilerOnAllEventReconciler private volatile boolean waitAfterFirstRetry = false; private volatile boolean continuerOnRetryWait = false; private volatile boolean waiting = false; + private volatile boolean alreadyWaitedAfterFirstRetry = false; // control flag to throw an exception on first delete event private volatile boolean isFirstDeleteEvent = true; @@ -79,10 +84,24 @@ public UpdateControl reconcile( } if (waitAfterFirstRetry + && !alreadyWaitedAfterFirstRetry && context.getRetryInfo().isPresent() && context.getRetryInfo().orElseThrow().getAttemptCount() == 1) { + // The reconciliation triggered by the superseding event below reuses the same retry + // execution, so its attempt count is still 1. Wait only on the very first one, otherwise that + // follow-up reconciliation would block here too and never be released. + alreadyWaitedAfterFirstRetry = true; waiting = true; - while (!continuerOnRetryWait) { + // Releasing on continuerOnRetryWait alone is racy: the test sets that flag right after the + // update call returns, but the update event still has to travel back through the informer. + // If this reconciliation failed before the event was registered, the framework would treat + // the failure as a plain retry (consuming the last attempt) instead of instantly + // re-triggering because of a superseding event. isNextReconciliationImminent() reports the + // same condition the framework evaluates after this reconciliation fails, and it cannot be + // unset while this reconciliation is in progress. + var waitUntil = System.currentTimeMillis() + MAX_WAIT_FOR_SUPERSEDING_EVENT_MILLIS; + while ((!continuerOnRetryWait || !context.isNextReconciliationImminent()) + && System.currentTimeMillis() < waitUntil) { Thread.sleep(50); } waiting = false;