improve: remove ErrorStatusHandler interface (#2438)

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

docsy/content/en/docs/migration/v5-0-migration.md

@@ -60,4 +60,5 @@ description: Migrating from v4.7 to v5.0
     This also means, that `BulkDependentResource` now does not automatically implement `Creator` and `Deleter` as before.
     Make sure to implement those interfaces in your bulk dependent resources. You can use also the new helper interface, the
     [`CRUDBulkDependentResource`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/CRUDBulkDependentResource.java)
-    what also implement `BulkUpdater` interface.
+    what also implement `BulkUpdater` interface.
+12. `ErrorStatusHandler` is deleted. Just delete the interface from your impl.

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

@@ -10,6 +10,7 @@ public class ErrorStatusUpdateControl<P extends HasMetadata>
 
   private final P resource;
   private boolean noRetry = false;
+  private final boolean defaultErrorProcessing;
 
   public static <T extends HasMetadata> ErrorStatusUpdateControl<T> patchStatus(T resource) {
     return new ErrorStatusUpdateControl<>(resource);
@@ -19,8 +20,21 @@ public static <T extends HasMetadata> ErrorStatusUpdateControl<T> noStatusUpdate
     return new ErrorStatusUpdateControl<>(null);
   }
 
+  /**
+   * No special processing of the error, the error will be thrown and default error handling will
+   * apply
+   */
+  public static <T extends HasMetadata> ErrorStatusUpdateControl<T> defaultErrorProcessing() {
+    return new ErrorStatusUpdateControl<>(null, true);
+  }
+
   private ErrorStatusUpdateControl(P resource) {
+    this(resource, false);
+  }
+
+  private ErrorStatusUpdateControl(P resource, boolean defaultErrorProcessing) {
     this.resource = resource;
+    this.defaultErrorProcessing = defaultErrorProcessing;
   }
 
   /**
@@ -29,6 +43,9 @@ private ErrorStatusUpdateControl(P resource) {
    * @return ErrorStatusUpdateControl
    */
   public ErrorStatusUpdateControl<P> withNoRetry() {
+    if (defaultErrorProcessing) {
+      throw new IllegalStateException("Cannot set no-retry for default error processing");
+    }
     this.noRetry = true;
     return this;
   }
@@ -41,6 +58,10 @@ public boolean isNoRetry() {
     return noRetry;
   }
 
+  public boolean isDefaultErrorProcessing() {
+    return defaultErrorProcessing;
+  }
+
   /**
    * If re-scheduled using this method, it is not considered as retry, it effectively cancels retry.
    *

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

@@ -31,4 +31,28 @@ default List<EventSource> prepareEventSources(EventSourceContext<P> context) {
     return Collections.emptyList();
   }
 
+  /**
+   * <p>
+   * Reconciler can override this method in order to update the status sub-resource in the case an
+   * exception in thrown. In that case {@link #updateErrorStatus(HasMetadata, Context, Exception)}
+   * is called automatically.
+   * <p>
+   * The result of the method call is used to make a status update on the custom resource. This is
+   * always a sub-resource update request, so no update on custom resource itself (like spec of
+   * metadata) happens. Note that this update request will also produce an event, and will result in
+   * a reconciliation if the controller is not generation aware.
+   * <p>
+   * Note that the scope of this feature is only the reconcile method of the reconciler, since there
+   * should not be updates on custom resource after it is marked for deletion.
+   *
+   * @param resource to update the status on
+   * @param context the current context
+   * @param e exception thrown from the reconciler
+   * @return the updated resource
+   */
+  default ErrorStatusUpdateControl<P> updateErrorStatus(P resource, Context<P> context,
+      Exception e) {
+    return ErrorStatusUpdateControl.defaultErrorProcessing();
+  }
+
 }

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

@@ -22,7 +22,6 @@
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
 import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
-import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusHandler;
 import io.javaoperatorsdk.operator.api.reconciler.RetryInfo;
 import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
 import io.javaoperatorsdk.operator.processing.Controller;
@@ -174,55 +173,46 @@ private PostExecutionControl<P> reconcileExecution(ExecutionScope<P> executionSc
   private PostExecutionControl<P> handleErrorStatusHandler(P resource, P originalResource,
       Context<P> context,
       Exception e) throws Exception {
-    if (isErrorStatusHandlerPresent()) {
-      try {
-        RetryInfo retryInfo = context.getRetryInfo().orElseGet(() -> new RetryInfo() {
-          @Override
-          public int getAttemptCount() {
-            return 0;
-          }
 
-          @Override
-          public boolean isLastAttempt() {
-            // check also if the retry is limited to 0
-            return retryConfigurationHasZeroAttempts ||
-                controller.getConfiguration().getRetry() == null;
-          }
-        });
-        ((DefaultContext<P>) context).setRetryInfo(retryInfo);
-        var errorStatusUpdateControl = ((ErrorStatusHandler<P>) controller.getReconciler())
-            .updateErrorStatus(resource, context, e);
-
-        P updatedResource = null;
-        if (errorStatusUpdateControl.getResource().isPresent()) {
-          updatedResource = customResourceFacade
-              .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource);
-        }
-        if (errorStatusUpdateControl.isNoRetry()) {
-          PostExecutionControl<P> postExecutionControl;
-          if (updatedResource != null) {
-            postExecutionControl =
-                PostExecutionControl.customResourceStatusPatched(updatedResource);
-          } else {
-            postExecutionControl = PostExecutionControl.defaultDispatch();
-          }
-          errorStatusUpdateControl.getScheduleDelay()
-              .ifPresent(postExecutionControl::withReSchedule);
-          return postExecutionControl;
-        }
-      } catch (RuntimeException ex) {
-        log.error("Error during error status handling.", ex);
+    RetryInfo retryInfo = context.getRetryInfo().orElseGet(() -> new RetryInfo() {
+      @Override
+      public int getAttemptCount() {
+        return 0;
       }
-    }
-    throw e;
-  }
 
-  private boolean isErrorStatusHandlerPresent() {
-    return controller.getReconciler() instanceof ErrorStatusHandler;
-  }
+      @Override
+      public boolean isLastAttempt() {
+        // check also if the retry is limited to 0
+        return retryConfigurationHasZeroAttempts ||
+            controller.getConfiguration().getRetry() == null;
+      }
+    });
+    ((DefaultContext<P>) context).setRetryInfo(retryInfo);
+    var errorStatusUpdateControl = controller.getReconciler()
+        .updateErrorStatus(resource, context, e);
 
-  private P patchStatusGenerationAware(P resource, P originalResource) {
-    return customResourceFacade.patchStatus(resource, originalResource);
+    if (errorStatusUpdateControl.isDefaultErrorProcessing()) {
+      throw e;
+    }
+
+    P updatedResource = null;
+    if (errorStatusUpdateControl.getResource().isPresent()) {
+      updatedResource = customResourceFacade
+          .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource);
+    }
+    if (errorStatusUpdateControl.isNoRetry()) {
+      PostExecutionControl<P> postExecutionControl;
+      if (updatedResource != null) {
+        postExecutionControl =
+            PostExecutionControl.customResourceStatusPatched(updatedResource);
+      } else {
+        postExecutionControl = PostExecutionControl.defaultDispatch();
+      }
+      errorStatusUpdateControl.getScheduleDelay()
+          .ifPresent(postExecutionControl::withReSchedule);
+      return postExecutionControl;
+    }
+    throw e;
   }
 
   private PostExecutionControl<P> createPostExecutionControl(P updatedCustomResource,

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

@@ -5,11 +5,11 @@
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiFunction;
+import java.util.function.Supplier;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
-import org.mockito.ArgumentMatcher;
 import org.mockito.ArgumentMatchers;
 import org.mockito.stubbing.Answer;
 
@@ -28,7 +28,6 @@
 import io.javaoperatorsdk.operator.api.reconciler.Cleaner;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
-import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusHandler;
 import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusUpdateControl;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.api.reconciler.RetryInfo;
@@ -479,7 +478,7 @@ void callErrorStatusHandlerIfImplemented() {
     reconciler.reconcile = (r, c) -> {
       throw new IllegalStateException("Error Status Test");
     };
-    reconciler.errorHandler = (r, ri, e) -> {
+    reconciler.errorHandler = () -> {
       testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE);
       return ErrorStatusUpdateControl.patchStatus(testCustomResource);
     };
@@ -499,7 +498,7 @@ public boolean isLastAttempt() {
             }).setResource(testCustomResource));
 
     verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
-    verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
+    verify(reconciler, times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
   }
 
@@ -510,15 +509,15 @@ void callErrorStatusHandlerEvenOnFirstError() {
     reconciler.reconcile = (r, c) -> {
       throw new IllegalStateException("Error Status Test");
     };
-    reconciler.errorHandler = (r, ri, e) -> {
+    reconciler.errorHandler = () -> {
       testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE);
       return ErrorStatusUpdateControl.patchStatus(testCustomResource);
     };
 
     var postExecControl = reconciliationDispatcher.handleExecution(
         new ExecutionScope(null).setResource(testCustomResource));
     verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
-    verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
+    verify(reconciler, times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
     assertThat(postExecControl.exceptionDuringExecution()).isTrue();
   }
@@ -529,15 +528,15 @@ void errorHandlerCanInstructNoRetryWithUpdate() {
     reconciler.reconcile = (r, c) -> {
       throw new IllegalStateException("Error Status Test");
     };
-    reconciler.errorHandler = (r, ri, e) -> {
+    reconciler.errorHandler = () -> {
       testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE);
       return ErrorStatusUpdateControl.patchStatus(testCustomResource).withNoRetry();
     };
 
     var postExecControl = reconciliationDispatcher.handleExecution(
         new ExecutionScope(null).setResource(testCustomResource));
 
-    verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
+    verify(reconciler, times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
     verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
     assertThat(postExecControl.exceptionDuringExecution()).isFalse();
@@ -549,15 +548,15 @@ void errorHandlerCanInstructNoRetryNoUpdate() {
     reconciler.reconcile = (r, c) -> {
       throw new IllegalStateException("Error Status Test");
     };
-    reconciler.errorHandler = (r, ri, e) -> {
+    reconciler.errorHandler = () -> {
       testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE);
       return ErrorStatusUpdateControl.<TestCustomResource>noStatusUpdate().withNoRetry();
     };
 
     var postExecControl = reconciliationDispatcher.handleExecution(
         new ExecutionScope(null).setResource(testCustomResource));
 
-    verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
+    verify(reconciler, times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
     verify(customResourceFacade, times(0)).patchStatus(eq(testCustomResource), any());
     assertThat(postExecControl.exceptionDuringExecution()).isFalse();
@@ -570,13 +569,13 @@ void errorStatusHandlerCanPatchResource() {
       throw new IllegalStateException("Error Status Test");
     };
     reconciler.errorHandler =
-        (r, ri, e) -> ErrorStatusUpdateControl.patchStatus(testCustomResource);
+        () -> ErrorStatusUpdateControl.patchStatus(testCustomResource);
 
     reconciliationDispatcher.handleExecution(
         new ExecutionScope(null).setResource(testCustomResource));
 
     verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
-    verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
+    verify(reconciler, times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
   }
 
@@ -592,16 +591,14 @@ void ifRetryLimitedToZeroMaxAttemptsErrorHandlerGetsCorrectLastAttempt() {
     reconciler.reconcile = (r, c) -> {
       throw new IllegalStateException("Error Status Test");
     };
-    var mockErrorHandler = mock(ErrorStatusHandler.class);
-    when(mockErrorHandler.updateErrorStatus(any(), any(), any()))
-        .thenReturn(ErrorStatusUpdateControl.noStatusUpdate());
-    reconciler.errorHandler = mockErrorHandler;
+
+    reconciler.errorHandler = () -> ErrorStatusUpdateControl.noStatusUpdate();
 
     reconciliationDispatcher.handleExecution(
         new ExecutionScope(null).setResource(testCustomResource));
 
-    verify(mockErrorHandler, times(1)).updateErrorStatus(any(),
-        ArgumentMatchers.argThat((ArgumentMatcher<Context<TestCustomResource>>) context -> {
+    verify(reconciler, times(1)).updateErrorStatus(any(),
+        ArgumentMatchers.argThat(context -> {
           var retryInfo = context.getRetryInfo().orElseThrow();
           return retryInfo.isLastAttempt();
         }), any());
@@ -651,7 +648,7 @@ void reSchedulesFromErrorHandler() {
       throw new IllegalStateException("Error Status Test");
     };
     reconciler.errorHandler =
-        (r, ri, e) -> ErrorStatusUpdateControl.<TestCustomResource>noStatusUpdate()
+        () -> ErrorStatusUpdateControl.<TestCustomResource>noStatusUpdate()
             .rescheduleAfter(delay);
 
     var res = reconciliationDispatcher.handleExecution(
@@ -691,12 +688,11 @@ public <T extends HasMetadata> ExecutionScope<T> executionScopeWithCREvent(T res
   }
 
   private class TestReconciler
-      implements Reconciler<TestCustomResource>, Cleaner<TestCustomResource>,
-      ErrorStatusHandler<TestCustomResource> {
+      implements Reconciler<TestCustomResource>, Cleaner<TestCustomResource> {
 
     private BiFunction<TestCustomResource, Context, UpdateControl<TestCustomResource>> reconcile;
     private BiFunction<TestCustomResource, Context, DeleteControl> cleanup;
-    private ErrorStatusHandler<TestCustomResource> errorHandler;
+    private Supplier<ErrorStatusUpdateControl> errorHandler;
 
     @Override
     public UpdateControl<TestCustomResource> reconcile(TestCustomResource resource,
@@ -719,7 +715,7 @@ public DeleteControl cleanup(TestCustomResource resource, Context context) {
     public ErrorStatusUpdateControl<TestCustomResource> updateErrorStatus(
         TestCustomResource resource,
         Context<TestCustomResource> context, Exception e) {
-      return errorHandler != null ? errorHandler.updateErrorStatus(resource, context, e)
+      return errorHandler != null ? errorHandler.get()
           : ErrorStatusUpdateControl.noStatusUpdate();
     }
   }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

@@ -21,8 +21,7 @@
         dependsOn = SECRET_NAME)})
 @ControllerConfiguration
 public class DependentResourceCrossRefReconciler
-    implements Reconciler<DependentResourceCrossRefResource>,
-    ErrorStatusHandler<DependentResourceCrossRefResource> {
+    implements Reconciler<DependentResourceCrossRefResource> {
 
   public static final String SECRET_NAME = "secret";
   private final AtomicInteger numberOfExecutions = new AtomicInteger(0);