From 4fe4bc575bc59b5bf6a9ed76c8b1869a915d999d Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 27 Oct 2021 15:51:11 +0200 Subject: [PATCH 1/6] fix: properly start event handler when starting the event source Fixes #630 --- .../operator/processing/DefaultEventHandler.java | 9 +++++++++ .../operator/processing/event/EventHandler.java | 2 ++ .../event/internal/CustomResourceEventSource.java | 2 ++ 3 files changed, 13 insertions(+) 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 27843a669a..de5e819285 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 @@ -142,6 +142,15 @@ public void handleEvent(Event event) { } } + public void start() { + try { + lock.lock(); + this.running = true; + } finally { + lock.unlock(); + } + } + @Override public void close() { try { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventHandler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventHandler.java index e0a657e1d1..3bab14c2e0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventHandler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventHandler.java @@ -9,4 +9,6 @@ public interface EventHandler extends Closeable { @Override default void close() throws IOException {} + + default void start() {} } 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..360f57bdca 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 @@ -48,6 +48,8 @@ public CustomResourceEventSource(ConfiguredController controller) { @Override public void start() { + eventHandler.start(); + final var configuration = controller.getConfiguration(); final var targetNamespaces = configuration.getEffectiveNamespaces(); final var client = controller.getCRClient(); From 62f29933371b56c576d8b8b6177056755049cac2 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 27 Oct 2021 17:35:41 +0200 Subject: [PATCH 2/6] fix: wrong method name in exception --- .../operator/api/config/ExecutorServiceManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java index 682b004c3d..f593b3c755 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java @@ -46,7 +46,7 @@ public static void close() { public static ExecutorServiceManager instance() { if (instance == null) { throw new IllegalStateException( - "ExecutorServiceManager hasn't been started. Call start method before using!"); + "ExecutorServiceManager hasn't been started. Call init method before using!"); } return instance; } From b17d92fbe3be1b02faa0d84d40282981022bfa2c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 27 Oct 2021 17:37:59 +0200 Subject: [PATCH 3/6] fix: duplicated method call --- .../operator/processing/DefaultEventHandlerTest.java | 1 - 1 file changed, 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 a03f4bd8a1..36269ddd57 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 @@ -63,7 +63,6 @@ public void setup() { // 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()); From adf39fbae83a90579ecee87051c61015d9f469ae Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 27 Oct 2021 18:12:14 +0200 Subject: [PATCH 4/6] chore: add tests (sync commit) Currently not working: test should fail but not at this stage :thinking: --- .../processing/DefaultEventHandler.java | 6 +- .../internal/CustomResourceEventSource.java | 3 +- .../CustomResourceEventSourceTest.java | 55 +++++++++++++++++++ 3 files changed, 62 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 de5e819285..bcaf395136 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 @@ -60,11 +60,15 @@ public DefaultEventHandler(ConfiguredController controller) { controller.getConfiguration().getConfigurationService().getMetrics().getEventMonitor()); } - DefaultEventHandler(EventDispatcher eventDispatcher, String relatedControllerName, + public DefaultEventHandler(EventDispatcher eventDispatcher, String relatedControllerName, Retry retry) { this(null, relatedControllerName, eventDispatcher, retry, null); } + public boolean isRunning() { + return running; + } + private DefaultEventHandler(ExecutorService executor, String relatedControllerName, EventDispatcher eventDispatcher, Retry retry, EventMonitor monitor) { this.running = 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 360f57bdca..31677a2ca5 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 @@ -48,7 +48,8 @@ public CustomResourceEventSource(ConfiguredController controller) { @Override public void start() { - eventHandler.start(); + // todo: this will be enabled once the tests are sorted out + // eventHandler.start(); final var configuration = controller.getConfiguration(); final var targetNamespaces = configuration.getEffectiveNamespaces(); 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..d57c16eeee 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 @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.processing.event.internal; +import java.io.IOException; import java.time.LocalDateTime; import java.util.List; @@ -15,11 +16,19 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.DefaultControllerConfiguration; import io.javaoperatorsdk.operator.processing.ConfiguredController; +import io.javaoperatorsdk.operator.processing.CustomResourceCache; +import io.javaoperatorsdk.operator.processing.DefaultEventHandler; +import io.javaoperatorsdk.operator.processing.EventDispatcher; +import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -102,6 +111,52 @@ public void eventNotMarkedForLastGenerationIfNoFinalizer() { verify(eventHandler, times(2)).handleEvent(any()); } + @Test + public void restartingShouldResumeEventHandling() throws IOException { + final var cr = TestUtils.testCustomResource(); + + CustomResourceCache customResourceCache = new CustomResourceCache(); + customResourceCache.cacheResource(cr); + DefaultEventSourceManager defaultEventSourceManagerMock = + mock(DefaultEventSourceManager.class); + EventDispatcher eventDispatcherMock = mock(EventDispatcher.class); + DefaultEventHandler local = new DefaultEventHandler(eventDispatcherMock, "Test", + null); + local.setEventSourceManager(defaultEventSourceManagerMock); + 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()); + + customResourceEventSource.setEventHandler(local); + + customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); + verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); + waitMinimalTime(); + + + customResourceEventSource.close(); + assertFalse(local.isRunning()); + waitMinimalTime(); + customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); + verify(eventDispatcherMock, timeout(50).times(0)).handleExecution(any()); + + customResourceEventSource.start(); + assertTrue(local.isRunning()); + waitMinimalTime(); + customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); + verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); + } + + private void waitMinimalTime() { + try { + Thread.sleep(200); + } catch (InterruptedException e) { + throw new IllegalStateException(e); + } + } + private static class TestConfiguredController extends ConfiguredController { public TestConfiguredController(boolean generationAware) { From 8e530f8d87212089ce1b69b7be3faaf1579af7c5 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 28 Oct 2021 10:29:41 +0200 Subject: [PATCH 5/6] fix: intermediate error in test --- .../event/internal/CustomResourceEventSourceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d57c16eeee..5e1a8e652f 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 @@ -140,7 +140,7 @@ public void restartingShouldResumeEventHandling() throws IOException { assertFalse(local.isRunning()); waitMinimalTime(); customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); - verify(eventDispatcherMock, timeout(50).times(0)).handleExecution(any()); + verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); customResourceEventSource.start(); assertTrue(local.isRunning()); From 6e129062e51f4a6694ec6abb5fd2b6d4954ce5a8 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 28 Oct 2021 12:51:40 +0200 Subject: [PATCH 6/6] fix: activate fix and fix CustomResourceEventSourceTest --- .../internal/CustomResourceEventSource.java | 3 +-- .../CustomResourceEventSourceTest.java | 23 ++++++++----------- 2 files changed, 11 insertions(+), 15 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 31677a2ca5..360f57bdca 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 @@ -48,8 +48,7 @@ public CustomResourceEventSource(ConfiguredController controller) { @Override public void start() { - // todo: this will be enabled once the tests are sorted out - // eventHandler.start(); + eventHandler.start(); final var configuration = controller.getConfiguration(); final var targetNamespaces = configuration.getEffectiveNamespaces(); 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 5e1a8e652f..cefa720833 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 @@ -8,7 +8,10 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.KubernetesResourceList; +import io.fabric8.kubernetes.api.model.ListOptions; +import io.fabric8.kubernetes.client.Watch; import io.fabric8.kubernetes.client.Watcher; +import io.fabric8.kubernetes.client.dsl.FilterWatchListMultiDeletable; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.javaoperatorsdk.operator.Metrics; @@ -129,32 +132,26 @@ public void restartingShouldResumeEventHandling() throws IOException { doCallRealMethod().when(defaultEventSourceManagerMock).getLatestResourceUids(any()); doCallRealMethod().when(defaultEventSourceManagerMock).cacheResource(any(), any()); + final var mock = mock(FilterWatchListMultiDeletable.class); + when(mock.watch((ListOptions) any(), any())).thenReturn(mock(Watch.class)); + when(client.inAnyNamespace()).thenReturn(mock); + customResourceEventSource.setEventHandler(local); customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); - waitMinimalTime(); - customResourceEventSource.close(); assertFalse(local.isRunning()); - waitMinimalTime(); customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); + // mockito times method is not reset and keeps increasing so here we stay at 1 call verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); customResourceEventSource.start(); assertTrue(local.isRunning()); - waitMinimalTime(); customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, cr); - verify(eventDispatcherMock, timeout(50).times(1)).handleExecution(any()); - } - - private void waitMinimalTime() { - try { - Thread.sleep(200); - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } + // we're expecting another call to the dispatcher, so total number of calls should now be 2 + verify(eventDispatcherMock, timeout(50).times(2)).handleExecution(any()); } private static class TestConfiguredController extends ConfiguredController {