diff --git a/docs/documentation/features.md b/docs/documentation/features.md index 194effb36e..b710eeb3e3 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -166,19 +166,21 @@ A successful execution resets the retry. ### Setting Error Status After Last Retry Attempt -In order to facilitate error reporting in case a last retry attempt fails, Reconciler can implement the following +In order to facilitate error reporting Reconciler can implement the following [interface](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusHandler.java): ```java -public interface ErrorStatusHandler> { +public interface ErrorStatusHandler { - T updateErrorStatus(T resource, RuntimeException e); + Optional updateErrorStatus(T resource, RetryInfo retryInfo, RuntimeException e); } ``` -The `updateErrorStatus` resource is called when it's the last retry attempt according the retry configuration and the -reconciler execution still resulted in a runtime exception. +The `updateErrorStatus` method is called in case an exception is thrown from the reconciler. It is also called when +there is no retry configured, just after the reconciler execution. +In the first call the `RetryInfo.getAttemptCount()` is always zero, since it is not a result of a retry +(regardless if retry is configured or not). 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 diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusHandler.java index 2d5592053b..60e9e671b2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusHandler.java @@ -1,5 +1,7 @@ package io.javaoperatorsdk.operator.api.reconciler; +import java.util.Optional; + import io.fabric8.kubernetes.api.model.HasMetadata; public interface ErrorStatusHandler { @@ -19,9 +21,10 @@ public interface ErrorStatusHandler { * should not be updates on custom resource after it is marked for deletion. * * @param resource to update the status on + * @param retryInfo * @param e exception thrown from the reconciler * @return the updated resource */ - T updateErrorStatus(T resource, RuntimeException e); + Optional updateErrorStatus(T resource, RetryInfo retryInfo, RuntimeException e); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 01f6de14ac..f29865d338 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -11,13 +11,7 @@ import io.fabric8.kubernetes.client.dsl.Resource; import io.javaoperatorsdk.operator.api.ObservedGenerationAware; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; -import io.javaoperatorsdk.operator.api.reconciler.BaseControl; -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.Reconciler; -import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.*; import io.javaoperatorsdk.operator.processing.Controller; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; @@ -114,7 +108,7 @@ private PostExecutionControl handleReconcile( cloneResourceForErrorStatusHandlerIfNeeded(originalResource, context); return reconcileExecution(executionScope, resourceForExecution, originalResource, context); } catch (RuntimeException e) { - handleLastAttemptErrorStatusHandler(originalResource, context, e); + handleErrorStatusHandler(originalResource, context, e); throw e; } } @@ -128,7 +122,7 @@ private PostExecutionControl handleReconcile( * place for status update. */ private R cloneResourceForErrorStatusHandlerIfNeeded(R resource, Context context) { - if (isLastAttemptOfRetryAndErrorStatusHandlerPresent(context) || + if (isErrorStatusHandlerPresent() || shouldUpdateObservedGenerationAutomatically(resource)) { return configuration().getConfigurationService().getResourceCloner().clone(resource); } else { @@ -164,26 +158,32 @@ && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) { return createPostExecutionControl(updatedCustomResource, updateControl); } - private void handleLastAttemptErrorStatusHandler(R resource, Context context, + private void handleErrorStatusHandler(R resource, Context context, RuntimeException e) { - if (isLastAttemptOfRetryAndErrorStatusHandlerPresent(context)) { + if (isErrorStatusHandlerPresent()) { try { + var retryInfo = context.getRetryInfo().orElse(new RetryInfo() { + @Override + public int getAttemptCount() { + return 0; + } + + @Override + public boolean isLastAttempt() { + return controller.getConfiguration().getRetryConfiguration() == null; + } + }); var updatedResource = ((ErrorStatusHandler) controller.getReconciler()) - .updateErrorStatus(resource, e); - customResourceFacade.updateStatus(updatedResource); + .updateErrorStatus(resource, retryInfo, e); + updatedResource.ifPresent(customResourceFacade::updateStatus); } catch (RuntimeException ex) { log.error("Error during error status handling.", ex); } } } - private boolean isLastAttemptOfRetryAndErrorStatusHandlerPresent(Context context) { - if (context.getRetryInfo().isPresent()) { - return context.getRetryInfo().get().isLastAttempt() - && controller.getReconciler() instanceof ErrorStatusHandler; - } else { - return false; - } + private boolean isErrorStatusHandlerPresent() { + return controller.getReconciler() instanceof ErrorStatusHandler; } private R updateStatusGenerationAware(R resource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index db4736697a..dd9d5e6fda 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -1,6 +1,7 @@ package io.javaoperatorsdk.operator.processing.event; import java.util.ArrayList; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -11,6 +12,7 @@ import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.TestUtils; +import io.javaoperatorsdk.operator.api.config.Cloner; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.monitoring.Metrics; @@ -56,9 +58,12 @@ private ReconciliationDispatcher init(R customResourc when(configuration.getName()).thenReturn("EventDispatcherTestController"); when(configService.getMetrics()).thenReturn(Metrics.NOOP); when(configuration.getConfigurationService()).thenReturn(configService); - when(configService.getResourceCloner()).thenReturn(ConfigurationService.DEFAULT_CLONER); - when(reconciler.reconcile(eq(customResource), any())) - .thenReturn(UpdateControl.updateResource(customResource)); + when(configService.getResourceCloner()).thenReturn(new Cloner() { + @Override + public R clone(R object) { + return object; + } + }); when(reconciler.cleanup(eq(customResource), any())) .thenReturn(DeleteControl.defaultDelete()); when(customResourceFacade.replaceWithLock(any())).thenReturn(null); @@ -351,9 +356,9 @@ void callErrorStatusHandlerIfImplemented() { when(reconciler.reconcile(any(), any())) .thenThrow(new IllegalStateException("Error Status Test")); - when(((ErrorStatusHandler) reconciler).updateErrorStatus(any(), any())).then(a -> { + when(((ErrorStatusHandler) reconciler).updateErrorStatus(any(), any(), any())).then(a -> { testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE); - return testCustomResource; + return Optional.of(testCustomResource); }); reconciliationDispatcher.handleExecution( @@ -373,7 +378,25 @@ public boolean isLastAttempt() { verify(customResourceFacade, times(1)).updateStatus(testCustomResource); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), - any()); + any(), any()); + } + + @Test + void callErrorStatusHandlerEvenOnFirstError() { + testCustomResource.addFinalizer(DEFAULT_FINALIZER); + + when(reconciler.reconcile(any(), any())) + .thenThrow(new IllegalStateException("Error Status Test")); + when(((ErrorStatusHandler) reconciler).updateErrorStatus(any(), any(), any())).then(a -> { + testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE); + return Optional.of(testCustomResource); + }); + reconciliationDispatcher.handleExecution( + new ExecutionScope( + testCustomResource, null)); + verify(customResourceFacade, times(1)).updateStatus(testCustomResource); + verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), + any(), any()); } private ObservedGenCustomResource createObservedGenCustomResource() { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerIT.java index 01908757bc..4a1b3293e7 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerIT.java @@ -17,13 +17,15 @@ public class ErrorStatusHandlerIT { + public static final int MAX_RETRY_ATTEMPTS = 3; ErrorStatusHandlerTestReconciler reconciler = new ErrorStatusHandlerTestReconciler(); @RegisterExtension OperatorExtension operator = OperatorExtension.builder() .withConfigurationService(DefaultConfigurationService.instance()) - .withReconciler(reconciler, new GenericRetry().setMaxAttempts(3).withLinearRetry()) + .withReconciler(reconciler, + new GenericRetry().setMaxAttempts(MAX_RETRY_ATTEMPTS).withLinearRetry()) .build(); @Test @@ -32,16 +34,18 @@ public void testErrorMessageSetEventually() { operator.create(ErrorStatusHandlerTestCustomResource.class, createCustomResource()); await() - .atMost(15, TimeUnit.SECONDS) - .pollInterval(1L, TimeUnit.SECONDS) + .atMost(10, TimeUnit.SECONDS) + .pollInterval(250, TimeUnit.MICROSECONDS) .untilAsserted( () -> { ErrorStatusHandlerTestCustomResource res = operator.get(ErrorStatusHandlerTestCustomResource.class, resource.getMetadata().getName()); assertThat(res.getStatus()).isNotNull(); - assertThat(res.getStatus().getMessage()) - .isEqualTo(ErrorStatusHandlerTestReconciler.ERROR_STATUS_MESSAGE); + for (int i = 0; i < MAX_RETRY_ATTEMPTS + 1; i++) { + assertThat(res.getStatus().getMessages()) + .contains(ErrorStatusHandlerTestReconciler.ERROR_STATUS_MESSAGE + i); + } }); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryIT.java index cb7109f8cf..4ab63c7513 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryIT.java @@ -20,15 +20,18 @@ public class RetryIT { public static final int RETRY_INTERVAL = 150; + public static final int MAX_RETRY_ATTEMPTS = 5; + + public static final int NUMBER_FAILED_EXECUTIONS = 3; @RegisterExtension OperatorExtension operator = OperatorExtension.builder() .withConfigurationService(DefaultConfigurationService.instance()) .withReconciler( - new RetryTestCustomReconciler(), + new RetryTestCustomReconciler(NUMBER_FAILED_EXECUTIONS), new GenericRetry().setInitialInterval(RETRY_INTERVAL).withLinearRetry() - .setMaxAttempts(5)) + .setMaxAttempts(MAX_RETRY_ATTEMPTS)) .build(); @@ -40,7 +43,7 @@ public void retryFailedExecution() { await("cr status updated") .pollDelay( - RETRY_INTERVAL * (RetryTestCustomReconciler.NUMBER_FAILED_EXECUTIONS + 2), + RETRY_INTERVAL * (NUMBER_FAILED_EXECUTIONS + 2), TimeUnit.MILLISECONDS) .pollInterval( RETRY_INTERVAL, @@ -49,7 +52,7 @@ public void retryFailedExecution() { .untilAsserted(() -> { assertThat( TestUtils.getNumberOfExecutions(operator)) - .isEqualTo(RetryTestCustomReconciler.NUMBER_FAILED_EXECUTIONS + 1); + .isEqualTo(NUMBER_FAILED_EXECUTIONS + 1); RetryTestCustomResource finalResource = operator.get(RetryTestCustomResource.class, @@ -59,7 +62,7 @@ public void retryFailedExecution() { }); } - public RetryTestCustomResource createTestCustomResource(String id) { + public static RetryTestCustomResource createTestCustomResource(String id) { RetryTestCustomResource resource = new RetryTestCustomResource(); resource.setMetadata( new ObjectMetaBuilder() diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryMaxAttemptIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryMaxAttemptIT.java new file mode 100644 index 0000000000..e855b016fd --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RetryMaxAttemptIT.java @@ -0,0 +1,42 @@ +package io.javaoperatorsdk.operator; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.javaoperatorsdk.operator.config.runtime.DefaultConfigurationService; +import io.javaoperatorsdk.operator.junit.OperatorExtension; +import io.javaoperatorsdk.operator.processing.retry.GenericRetry; +import io.javaoperatorsdk.operator.sample.retry.RetryTestCustomReconciler; +import io.javaoperatorsdk.operator.sample.retry.RetryTestCustomResource; + +import static io.javaoperatorsdk.operator.RetryIT.createTestCustomResource; +import static org.assertj.core.api.Assertions.assertThat; + +public class RetryMaxAttemptIT { + + public static final int MAX_RETRY_ATTEMPTS = 3; + public static final int RETRY_INTERVAL = 100; + public static final int ALL_EXECUTION_TO_FAIL = 99; + + RetryTestCustomReconciler reconciler = new RetryTestCustomReconciler(ALL_EXECUTION_TO_FAIL); + + @RegisterExtension + OperatorExtension operator = + OperatorExtension.builder() + .withConfigurationService(DefaultConfigurationService.instance()) + .withReconciler(reconciler, + new GenericRetry().setInitialInterval(RETRY_INTERVAL).withLinearRetry() + .setMaxAttempts(MAX_RETRY_ATTEMPTS)) + .build(); + + + @Test + public void retryFailedExecution() throws InterruptedException { + RetryTestCustomResource resource = createTestCustomResource("max-retry"); + + operator.create(RetryTestCustomResource.class, resource); + + Thread.sleep((MAX_RETRY_ATTEMPTS + 2) * RETRY_INTERVAL); + assertThat(reconciler.getNumberOfExecutions()).isEqualTo(4); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestCustomResourceStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestCustomResourceStatus.java index 03fc517ecd..4e54c877a5 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestCustomResourceStatus.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestCustomResourceStatus.java @@ -1,15 +1,21 @@ package io.javaoperatorsdk.operator.sample.errorstatushandler; +import java.util.ArrayList; +import java.util.List; + public class ErrorStatusHandlerTestCustomResourceStatus { - private String message; + private List messages; - public String getMessage() { - return message; + public List getMessages() { + if (messages == null) { + messages = new ArrayList<>(); + } + return messages; } - public ErrorStatusHandlerTestCustomResourceStatus setMessage(String message) { - this.message = message; + public ErrorStatusHandlerTestCustomResourceStatus setMessages(List messages) { + this.messages = messages; return this; } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java index 3a922ce092..8f20c44353 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.sample.errorstatushandler; +import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; import org.slf4j.Logger; @@ -45,11 +46,11 @@ public int getNumberOfExecutions() { } @Override - public ErrorStatusHandlerTestCustomResource updateErrorStatus( - ErrorStatusHandlerTestCustomResource resource, RuntimeException e) { + public Optional updateErrorStatus( + ErrorStatusHandlerTestCustomResource resource, RetryInfo retryInfo, RuntimeException e) { log.info("Setting status."); ensureStatusExists(resource); - resource.getStatus().setMessage(ERROR_STATUS_MESSAGE); - return resource; + resource.getStatus().getMessages().add(ERROR_STATUS_MESSAGE + retryInfo.getAttemptCount()); + return Optional.of(resource); } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java index b2a7ca0e17..21e44631fb 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java @@ -17,8 +17,6 @@ public class RetryTestCustomReconciler implements Reconciler, TestExecutionInfoProvider { - public static final int NUMBER_FAILED_EXECUTIONS = 2; - public static final String FINALIZER_NAME = ControllerUtils.getDefaultFinalizerName( CustomResource.getCRDName(RetryTestCustomResource.class)); @@ -26,6 +24,12 @@ public class RetryTestCustomReconciler LoggerFactory.getLogger(RetryTestCustomReconciler.class); private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + private int numberOfExecutionFails; + + public RetryTestCustomReconciler(int numberOfExecutionFails) { + this.numberOfExecutionFails = numberOfExecutionFails; + } + @Override public UpdateControl reconcile( RetryTestCustomResource resource, Context context) { @@ -36,7 +40,7 @@ public UpdateControl reconcile( } log.info("Value: " + resource.getSpec().getValue()); - if (numberOfExecutions.get() < NUMBER_FAILED_EXECUTIONS + 1) { + if (numberOfExecutions.get() < numberOfExecutionFails + 1) { throw new RuntimeException("Testing Retry"); } if (context.getRetryInfo().isEmpty() || context.getRetryInfo().get().isLastAttempt()) { diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java index 37caf1503c..ddb353ac05 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java @@ -4,6 +4,7 @@ import java.io.InputStream; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; @@ -173,8 +174,9 @@ private T loadYaml(Class clazz, String yaml) { } @Override - public WebPage updateErrorStatus(WebPage resource, RuntimeException e) { + public Optional updateErrorStatus(WebPage resource, RetryInfo retryInfo, + RuntimeException e) { resource.getStatus().setErrorMessage("Error: " + e.getMessage()); - return resource; + return Optional.of(resource); } }