From 4182c1001da81368481a65fd9899feb2fdda0938 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 15 Jun 2022 13:22:32 +0200 Subject: [PATCH 1/3] feat: retry and retry configuration decoupling --- .../api/config/ControllerConfiguration.java | 6 ++++-- .../ControllerConfigurationOverrider.java | 7 ++++--- .../config/DefaultControllerConfiguration.java | 17 +++++++++-------- .../processing/event/EventProcessor.java | 4 +--- .../event/ReconciliationDispatcher.java | 2 +- .../operator/processing/retry/GenericRetry.java | 6 +++--- .../operator/processing/retry/Retry.java | 5 ++--- .../processing/event/EventProcessorTest.java | 5 +++-- .../event/ReconciliationDispatcherTest.java | 4 ++-- .../retry/GenericRetryExecutionTest.java | 17 ++++++++++------- 10 files changed, 39 insertions(+), 34 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 33e5f0218f..219760fad5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -10,6 +10,8 @@ import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilters; +import io.javaoperatorsdk.operator.processing.retry.GenericRetry; +import io.javaoperatorsdk.operator.processing.retry.Retry; public interface ControllerConfiguration extends ResourceConfiguration { @@ -27,8 +29,8 @@ default boolean isGenerationAware() { String getAssociatedReconcilerClassName(); - default RetryConfiguration getRetryConfiguration() { - return RetryConfiguration.DEFAULT; + default Retry getRetry() { + return new GenericRetry(); } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index 88b511be31..6019a6425c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -9,6 +9,7 @@ import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter; +import io.javaoperatorsdk.operator.processing.retry.Retry; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET; import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_CURRENT_NAMESPACE_SET; @@ -19,7 +20,7 @@ public class ControllerConfigurationOverrider { private String finalizer; private boolean generationAware; private Set namespaces; - private RetryConfiguration retry; + private Retry retry; private String labelSelector; private ResourceEventFilter customResourcePredicate; private final ControllerConfiguration original; @@ -30,7 +31,7 @@ private ControllerConfigurationOverrider(ControllerConfiguration original) { finalizer = original.getFinalizerName(); generationAware = original.isGenerationAware(); namespaces = new HashSet<>(original.getNamespaces()); - retry = original.getRetryConfiguration(); + retry = original.getRetry(); labelSelector = original.getLabelSelector(); customResourcePredicate = original.getEventFilter(); reconciliationMaxInterval = original.reconciliationMaxInterval().orElse(null); @@ -90,7 +91,7 @@ public ControllerConfigurationOverrider watchingAllNamespaces() { return this; } - public ControllerConfigurationOverrider withRetry(RetryConfiguration retry) { + public ControllerConfigurationOverrider withRetry(Retry retry) { this.retry = retry; return this; } 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 38373f18f3..7046af3d85 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 @@ -9,6 +9,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter; +import io.javaoperatorsdk.operator.processing.retry.Retry; @SuppressWarnings("rawtypes") public class DefaultControllerConfiguration @@ -20,7 +21,7 @@ public class DefaultControllerConfiguration private final String crdName; private final String finalizer; private final boolean generationAware; - private final RetryConfiguration retryConfiguration; + private final Retry retry; private final ResourceEventFilter resourceEventFilter; private final List dependents; private final Duration reconciliationMaxInterval; @@ -33,7 +34,7 @@ public DefaultControllerConfiguration( String finalizer, boolean generationAware, Set namespaces, - RetryConfiguration retryConfiguration, + Retry retry, String labelSelector, ResourceEventFilter resourceEventFilter, Class resourceClass, @@ -46,10 +47,10 @@ public DefaultControllerConfiguration( this.finalizer = finalizer; this.generationAware = generationAware; this.reconciliationMaxInterval = reconciliationMaxInterval; - this.retryConfiguration = - retryConfiguration == null - ? ControllerConfiguration.super.getRetryConfiguration() - : retryConfiguration; + this.retry = + retry == null + ? ControllerConfiguration.super.getRetry() + : retry; this.resourceEventFilter = resourceEventFilter; this.dependents = dependents != null ? dependents : Collections.emptyList(); @@ -81,8 +82,8 @@ public String getAssociatedReconcilerClassName() { } @Override - public RetryConfiguration getRetryConfiguration() { - return retryConfiguration; + public Retry getRetry() { + return retry; } @Override 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 6597131990..849366f978 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 @@ -25,7 +25,6 @@ import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceAction; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEvent; import io.javaoperatorsdk.operator.processing.event.source.timer.TimerEventSource; -import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.processing.retry.Retry; import io.javaoperatorsdk.operator.processing.retry.RetryExecution; @@ -54,8 +53,7 @@ class EventProcessor implements EventHandler, LifecycleAw ExecutorServiceManager.instance().executorService(), eventSourceManager.getController().getConfiguration().getName(), new ReconciliationDispatcher<>(eventSourceManager.getController()), - GenericRetry.fromConfiguration( - eventSourceManager.getController().getConfiguration().getRetryConfiguration()), + eventSourceManager.getController().getConfiguration().getRetry(), ConfigurationServiceProvider.instance().getMetrics(), eventSourceManager); } 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 a76fcff8e4..778468f269 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 @@ -178,7 +178,7 @@ public int getAttemptCount() { @Override public boolean isLastAttempt() { - return controller.getConfiguration().getRetryConfiguration() == null; + return controller.getConfiguration().getRetry() == null; } }); ((DefaultContext) context).setRetryInfo(retryInfo); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java index bed8e43c9d..7804586fa3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java @@ -3,9 +3,9 @@ import io.javaoperatorsdk.operator.api.config.RetryConfiguration; public class GenericRetry implements Retry { - private int maxAttempts = DEFAULT_MAX_ATTEMPTS; - private long initialInterval = DEFAULT_INITIAL_INTERVAL; - private double intervalMultiplier = DEFAULT_MULTIPLIER; + private int maxAttempts = RetryConfiguration.DEFAULT_MAX_ATTEMPTS; + private long initialInterval = RetryConfiguration.DEFAULT_INITIAL_INTERVAL; + private double intervalMultiplier = RetryConfiguration.DEFAULT_MULTIPLIER; private long maxInterval = -1; public static GenericRetry defaultLimitedExponentialRetry() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/Retry.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/Retry.java index b7911d1a74..3500a196f8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/Retry.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/Retry.java @@ -1,8 +1,7 @@ package io.javaoperatorsdk.operator.processing.retry; -import io.javaoperatorsdk.operator.api.config.RetryConfiguration; - -public interface Retry extends RetryConfiguration { +public interface Retry { RetryExecution initExecution(); + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java index 4d03059c6b..dc1c32aeda 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java @@ -15,6 +15,7 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.config.RetryConfiguration; import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerResourceEventSource; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceAction; @@ -104,7 +105,7 @@ void schedulesAnEventRetryOnException() { eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); verify(retryTimerEventSourceMock, times(1)) - .scheduleOnce(eq(customResource), eq(GenericRetry.DEFAULT_INITIAL_INTERVAL)); + .scheduleOnce(eq(customResource), eq(RetryConfiguration.DEFAULT_INITIAL_INTERVAL)); } @Test @@ -135,7 +136,7 @@ void executesTheControllerInstantlyAfterErrorIfNewEventsReceived() { List allValues = executionScopeArgumentCaptor.getAllValues(); assertThat(allValues).hasSize(2); verify(retryTimerEventSourceMock, never()) - .scheduleOnce(eq(customResource), eq(GenericRetry.DEFAULT_INITIAL_INTERVAL)); + .scheduleOnce(eq(customResource), eq(RetryConfiguration.DEFAULT_INITIAL_INTERVAL)); } @Test 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 166075b6bb..f19ba918f3 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 @@ -25,7 +25,6 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; -import io.javaoperatorsdk.operator.api.config.RetryConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Cleaner; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.DeleteControl; @@ -36,6 +35,7 @@ import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.processing.Controller; import io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.CustomResourceFacade; +import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.sample.observedgeneration.ObservedGenCustomResource; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -110,7 +110,7 @@ private ReconciliationDispatcher init(R customResourc when(configuration.getFinalizerName()).thenReturn(DEFAULT_FINALIZER); when(configuration.getName()).thenReturn("EventDispatcherTestController"); when(configuration.getResourceClass()).thenReturn(resourceClass); - when(configuration.getRetryConfiguration()).thenReturn(RetryConfiguration.DEFAULT); + when(configuration.getRetry()).thenReturn(new GenericRetry()); when(configuration.reconciliationMaxInterval()) .thenReturn(Optional.of(Duration.ofHours(RECONCILIATION_MAX_INTERVAL))); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecutionTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecutionTest.java index 0f8e44d1b2..1dcd9df464 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecutionTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecutionTest.java @@ -4,14 +4,16 @@ import org.junit.jupiter.api.Test; -import static io.javaoperatorsdk.operator.processing.retry.GenericRetry.DEFAULT_INITIAL_INTERVAL; +import io.javaoperatorsdk.operator.api.config.RetryConfiguration; + import static org.assertj.core.api.Assertions.assertThat; public class GenericRetryExecutionTest { @Test public void forFirstBackOffAlwaysReturnsInitialInterval() { - assertThat(getDefaultRetryExecution().nextDelay().get()).isEqualTo(DEFAULT_INITIAL_INTERVAL); + assertThat(getDefaultRetryExecution().nextDelay().get()) + .isEqualTo(RetryConfiguration.DEFAULT_INITIAL_INTERVAL); } @Test @@ -19,18 +21,19 @@ public void delayIsMultipliedEveryNextDelayCall() { RetryExecution retryExecution = getDefaultRetryExecution(); Optional res = callNextDelayNTimes(retryExecution, 1); - assertThat(res.get()).isEqualTo(DEFAULT_INITIAL_INTERVAL); + assertThat(res.get()).isEqualTo(RetryConfiguration.DEFAULT_INITIAL_INTERVAL); res = retryExecution.nextDelay(); assertThat(res.get()) - .isEqualTo((long) (DEFAULT_INITIAL_INTERVAL * GenericRetry.DEFAULT_MULTIPLIER)); + .isEqualTo((long) (RetryConfiguration.DEFAULT_INITIAL_INTERVAL + * RetryConfiguration.DEFAULT_MULTIPLIER)); res = retryExecution.nextDelay(); assertThat(res.get()) .isEqualTo( - (long) (DEFAULT_INITIAL_INTERVAL - * GenericRetry.DEFAULT_MULTIPLIER - * GenericRetry.DEFAULT_MULTIPLIER)); + (long) (RetryConfiguration.DEFAULT_INITIAL_INTERVAL + * RetryConfiguration.DEFAULT_MULTIPLIER + * RetryConfiguration.DEFAULT_MULTIPLIER)); } @Test From 62045921c641fde8d7c7cf443e1bf9e55c661d4c Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 15 Jun 2022 14:18:22 +0200 Subject: [PATCH 2/3] backwards compatibility --- .../operator/api/config/ControllerConfiguration.java | 12 +++++++++++- .../api/config/ControllerConfigurationOverrider.java | 7 +++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 219760fad5..fc1892f3ab 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -30,7 +30,17 @@ default boolean isGenerationAware() { String getAssociatedReconcilerClassName(); default Retry getRetry() { - return new GenericRetry(); + return GenericRetry.fromConfiguration(getRetryConfiguration()); // NOSONAR + } + + /** + * getRetry instead. + * + * @return configuration for retry. + */ + @Deprecated + default RetryConfiguration getRetryConfiguration() { + return RetryConfiguration.DEFAULT; } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index 6019a6425c..7e987d4700 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -9,6 +9,7 @@ import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter; +import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.processing.retry.Retry; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET; @@ -96,6 +97,12 @@ public ControllerConfigurationOverrider withRetry(Retry retry) { return this; } + @Deprecated + public ControllerConfigurationOverrider withRetry(RetryConfiguration retry) { + this.retry = GenericRetry.fromConfiguration(retry); + return this; + } + public ControllerConfigurationOverrider withLabelSelector(String labelSelector) { this.labelSelector = labelSelector; return this; From acde2d1e6c8ec10a91ca3581c7311d53a39c06b8 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 15 Jun 2022 14:19:07 +0200 Subject: [PATCH 3/3] docs --- .../operator/api/config/ControllerConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index fc1892f3ab..60d47d2b47 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -34,7 +34,7 @@ default Retry getRetry() { } /** - * getRetry instead. + * Use getRetry instead. * * @return configuration for retry. */