From 6e83fd83a56251417802fec7dda4a54adefcb471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Fri, 26 Jan 2024 14:15:08 +0100 Subject: [PATCH 1/3] fix: possible issue with concurrency for activation dynamic event source registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- ...AbstractEventSourceHolderDependentResource.java | 14 ++++++++++++-- .../workflow/WorkflowReconcileExecutor.java | 1 - 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java index 04aa5631cf..fea33954d5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java @@ -34,13 +34,22 @@ protected AbstractEventSourceHolderDependentResource(Class resourceType) { this.resourceType = resourceType; } - public Optional eventSource(EventSourceContext

context) { + /** + * Method is synchronized since when used in case of dynamic registration (thus for activation + * conditions) can be called concurrently to create the target event source. In that case only one + * instance should be created, since this also sets the event source, and dynamic registration + * will just start one with the same name. So if this would not be synchronized it could happen + * that multiple event sources would be created and only one started and registered. Note that + * this method does not start the event source, so no blocking IO is involved. + */ + public synchronized Optional eventSource(EventSourceContext

context) { // some sub-classes (e.g. KubernetesDependentResource) can have their event source created // before this method is called in the managed case, so only create the event source if it // hasn't already been set. // The filters are applied automatically only if event source is created automatically. So if an // event source // is shared between dependent resources this does not override the existing filters. + if (eventSource == null && eventSourceNameToUse == null) { setEventSource(createEventSource(context)); applyFilters(); @@ -83,9 +92,10 @@ public Class resourceType() { protected abstract T createEventSource(EventSourceContext

context); - protected void setEventSource(T eventSource) { + public void setEventSource(T eventSource) { isCacheFillerEventSource = eventSource instanceof RecentOperationCacheFiller; this.eventSource = eventSource; + applyFilters(); } protected void applyFilters() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 33aad99811..b8f470a32f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -88,7 +88,6 @@ private void registerOrDeregisterEventSourceBasedOnActivation(boolean activa var es = eventSource.orElseThrow(); context.eventSourceRetriever() .dynamicallyRegisterEventSource(dependentResourceNode.getName(), es); - } else { context.eventSourceRetriever() .dynamicallyDeRegisterEventSource(dependentResourceNode.getName()); From de438d6486b313279ca4cd1f58a799f714b5bdfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 29 Jan 2024 12:23:32 +0100 Subject: [PATCH 2/3] reverts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../dependent/AbstractEventSourceHolderDependentResource.java | 3 +-- .../dependent/workflow/WorkflowReconcileExecutor.java | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java index fea33954d5..6562afd09f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java @@ -92,10 +92,9 @@ public Class resourceType() { protected abstract T createEventSource(EventSourceContext

context); - public void setEventSource(T eventSource) { + protected void setEventSource(T eventSource) { isCacheFillerEventSource = eventSource instanceof RecentOperationCacheFiller; this.eventSource = eventSource; - applyFilters(); } protected void applyFilters() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index b8f470a32f..05c06413a0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -88,6 +88,7 @@ private void registerOrDeregisterEventSourceBasedOnActivation(boolean activa var es = eventSource.orElseThrow(); context.eventSourceRetriever() .dynamicallyRegisterEventSource(dependentResourceNode.getName(), es); + } else { context.eventSourceRetriever() .dynamicallyDeRegisterEventSource(dependentResourceNode.getName()); From b9a2238b9718210c3b3ca12af3c15872e1d2ecd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 29 Jan 2024 14:35:16 +0100 Subject: [PATCH 3/3] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../dependent/workflow/WorkflowReconcileExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 05c06413a0..33aad99811 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -88,7 +88,7 @@ private void registerOrDeregisterEventSourceBasedOnActivation(boolean activa var es = eventSource.orElseThrow(); context.eventSourceRetriever() .dynamicallyRegisterEventSource(dependentResourceNode.getName(), es); - + } else { context.eventSourceRetriever() .dynamicallyDeRegisterEventSource(dependentResourceNode.getName());