From deaff919a987547d585f9ab8a7dfdc2763429ba3 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 14:05:22 +0100 Subject: [PATCH 1/6] fix: cover informer automatic start case --- .../processing/event/internal/InformerEventSource.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index d9252bb4c6..041834cf54 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -82,7 +82,13 @@ private void propagateEvent(T object) { } uids.forEach(uid -> { Event event = new Event(CustomResourceID.fromResource(object)); - this.eventHandler.handleEvent(event); + /* In fabric8 client for certain cases informers can be created on in a way that + they are automatically started, what would cause a NullPointerException here, + since an event might be received between creation and registration. + */ + if (eventHandler != null) { + this.eventHandler.handleEvent(event); + } }); } From e05349774900da105eebdaa779b523254ae6ac99 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 14:06:59 +0100 Subject: [PATCH 2/6] fix: formatting --- .../processing/event/internal/InformerEventSource.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index 041834cf54..77534abda7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -82,10 +82,11 @@ private void propagateEvent(T object) { } uids.forEach(uid -> { Event event = new Event(CustomResourceID.fromResource(object)); - /* In fabric8 client for certain cases informers can be created on in a way that - they are automatically started, what would cause a NullPointerException here, - since an event might be received between creation and registration. - */ + /* + * In fabric8 client for certain cases informers can be created on in a way that they are + * automatically started, what would cause a NullPointerException here, since an event might + * be received between creation and registration. + */ if (eventHandler != null) { this.eventHandler.handleEvent(event); } From 43e3d8c609bb5c2c6491ead9f051a486df609818 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 14:31:42 +0100 Subject: [PATCH 3/6] fix: event handler volatile --- .../operator/processing/event/AbstractEventSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEventSource.java index eba4e7ac0a..8d9984db14 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/AbstractEventSource.java @@ -2,7 +2,7 @@ public abstract class AbstractEventSource implements EventSource { - protected EventHandler eventHandler; + protected volatile EventHandler eventHandler; @Override public void setEventHandler(EventHandler eventHandler) { From 2a08236a0849cc3b2b1af0450b3597b889684daa Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 15:04:06 +0100 Subject: [PATCH 4/6] refactor: rename private field --- .../event/internal/InformerEventSource.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index 77534abda7..9bc61005dd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -17,33 +17,33 @@ public class InformerEventSource extends AbstractEventSource { private final SharedInformer sharedInformer; - private final Function> resourceToUIDs; + private final Function> resourceToCustomResourceIDSet; private final Function associatedWith; private final boolean skipUpdateEventPropagationIfNoChange; public InformerEventSource(SharedInformer sharedInformer, - Function> resourceToUIDs) { - this(sharedInformer, resourceToUIDs, null, true); + Function> resourceToCustomResourceIDSet) { + this(sharedInformer, resourceToCustomResourceIDSet, null, true); } public InformerEventSource(KubernetesClient client, Class type, - Function> resourceToUIDs) { - this(client, type, resourceToUIDs, false); + Function> resourceToCustomResourceIDSet) { + this(client, type, resourceToCustomResourceIDSet, false); } InformerEventSource(KubernetesClient client, Class type, - Function> resourceToUIDs, + Function> resourceToCustomResourceIDSet, boolean skipUpdateEventPropagationIfNoChange) { - this(client.informers().sharedIndexInformerFor(type, 0), resourceToUIDs, null, + this(client.informers().sharedIndexInformerFor(type, 0), resourceToCustomResourceIDSet, null, skipUpdateEventPropagationIfNoChange); } public InformerEventSource(SharedInformer sharedInformer, - Function> resourceToUIDs, + Function> resourceToCustomResourceIDSet, Function associatedWith, boolean skipUpdateEventPropagationIfNoChange) { this.sharedInformer = sharedInformer; - this.resourceToUIDs = resourceToUIDs; + this.resourceToCustomResourceIDSet = resourceToCustomResourceIDSet; this.skipUpdateEventPropagationIfNoChange = skipUpdateEventPropagationIfNoChange; this.associatedWith = Objects.requireNonNullElseGet(associatedWith, () -> cr -> { @@ -76,7 +76,7 @@ public void onDelete(T t, boolean b) { } private void propagateEvent(T object) { - var uids = resourceToUIDs.apply(object); + var uids = resourceToCustomResourceIDSet.apply(object); if (uids.isEmpty()) { return; } From b8ed31e959c3f9655604b8c06c6f9f95d260d6be Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 15:53:04 +0100 Subject: [PATCH 5/6] feature: log warning if passing an already runnning informer --- .../processing/event/internal/InformerEventSource.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index 9bc61005dd..e93467107b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -13,9 +13,13 @@ import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class InformerEventSource extends AbstractEventSource { + private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class); + private final SharedInformer sharedInformer; private final Function> resourceToCustomResourceIDSet; private final Function associatedWith; @@ -45,6 +49,11 @@ public InformerEventSource(SharedInformer sharedInformer, this.sharedInformer = sharedInformer; this.resourceToCustomResourceIDSet = resourceToCustomResourceIDSet; this.skipUpdateEventPropagationIfNoChange = skipUpdateEventPropagationIfNoChange; + if (sharedInformer.isRunning()) { + log.warn( + "Informer is already running on event source creation, this is not desirable and may " + + "lead to non deterministic behavior."); + } this.associatedWith = Objects.requireNonNullElseGet(associatedWith, () -> cr -> { final var metadata = cr.getMetadata(); From b2183ed750803d218fab587e1ad2800385094604 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 8 Nov 2021 15:55:34 +0100 Subject: [PATCH 6/6] fix: formatting --- .../processing/event/internal/InformerEventSource.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java index e93467107b..64b4a8d753 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java @@ -4,6 +4,9 @@ import java.util.Set; import java.util.function.Function; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.informers.ResourceEventHandler; @@ -13,8 +16,6 @@ import io.javaoperatorsdk.operator.processing.event.AbstractEventSource; import io.javaoperatorsdk.operator.processing.event.CustomResourceID; import io.javaoperatorsdk.operator.processing.event.Event; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class InformerEventSource extends AbstractEventSource { @@ -51,8 +52,8 @@ public InformerEventSource(SharedInformer sharedInformer, this.skipUpdateEventPropagationIfNoChange = skipUpdateEventPropagationIfNoChange; if (sharedInformer.isRunning()) { log.warn( - "Informer is already running on event source creation, this is not desirable and may " + - "lead to non deterministic behavior."); + "Informer is already running on event source creation, this is not desirable and may " + + "lead to non deterministic behavior."); } this.associatedWith = Objects.requireNonNullElseGet(associatedWith, () -> cr -> {