diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java index 50e61526c4..328ef651c9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java @@ -21,6 +21,8 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -50,6 +52,9 @@ public class DefaultContext
implements Context
{
private final boolean primaryResourceDeleted;
private final boolean primaryResourceFinalStateUnknown;
private final Map resourceOperations;
public DefaultContext(
@@ -257,14 +262,43 @@ public DefaultContext setRetryInfo(RetryInfo retryInfo) {
@SuppressWarnings("unchecked")
public desiredStateComputer) {
- return (R)
- desiredStates.computeIfAbsent(
- dependentResource,
- ignored -> {
- final var desired = desiredStateComputer.apply(getPrimaryResource());
- applyDesiredStateAspects(desired, dependentResource);
- return desired;
- });
+ // an aspect can look up the actual resource of its dependent, which asks for the desired state
+ // being computed: the thread computing it is handed the state as it is so far, instead of
+ // re-entering the computation
+ final var computing = desiredStatesInProgress.get(dependentResource);
+ if (computing != null && computing.thread() == Thread.currentThread()) {
+ return (R) computing.desired();
+ }
+ final var existing = desiredStates.get(dependentResource);
+ if (existing != null) {
+ return (R) existing;
+ }
+ // not computed within ConcurrentHashMap.computeIfAbsent, since an aspect can look up the actual
+ // resource of another dependent, which re-enters this method for a different key
+ final var lock =
+ desiredStateLocks.computeIfAbsent(dependentResource, ignored -> new ReentrantLock());
+ lock.lock();
+ try {
+ final var computedMeanwhile = desiredStates.get(dependentResource);
+ if (computedMeanwhile != null) {
+ return (R) computedMeanwhile;
+ }
+ final var desired = desiredStateComputer.apply(getPrimaryResource());
+ if (desired == null) {
+ return null;
+ }
+ desiredStatesInProgress.put(
+ dependentResource, new DesiredStateInProgress(Thread.currentThread(), desired));
+ try {
+ applyDesiredStateAspects(desired, dependentResource);
+ } finally {
+ desiredStatesInProgress.remove(dependentResource);
+ }
+ desiredStates.put(dependentResource, desired);
+ return desired;
+ } finally {
+ lock.unlock();
+ }
}
/**
@@ -280,4 +314,6 @@ private void applyDesiredStateAspects(Object desired, DependentResource, P> de
.forEach(aspect -> aspect.apply(hasMetadata, dependentResource, this));
}
}
+
+ private record DesiredStateInProgress(Thread thread, Object desired) {}
}
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectReentrancyTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectReentrancyTest.java
new file mode 100644
index 0000000000..a7c85d928a
--- /dev/null
+++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectReentrancyTest.java
@@ -0,0 +1,145 @@
+/*
+ * Copyright Java Operator SDK Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.javaoperatorsdk.operator.processing.dependent;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.jupiter.api.Test;
+
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
+import io.javaoperatorsdk.operator.api.config.ConfigurationService;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DesiredStateAspect;
+import io.javaoperatorsdk.operator.processing.Controller;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
+import io.javaoperatorsdk.operator.processing.event.EventSourceManager;
+import io.javaoperatorsdk.operator.processing.event.source.EventSource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+@SuppressWarnings({"unchecked", "rawtypes"})
+class DesiredStateAspectReentrancyTest {
+
+ private static final ConfigMap PRIMARY =
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName("primary")
+ .withNamespace("ns")
+ .endMetadata()
+ .build();
+
+ @Test
+ void aspectCanLookUpActualResourceOfTheDependent() {
+ final DesiredStateAspect aspect =
+ (desired, dependentResource, ctx) ->
+ ((DependentResource) dependentResource)
+ .getSecondaryResource(ctx.getPrimaryResource(), ctx);
+ final var context = contextWith(List.of(aspect));
+
+ final var dependent = new ChildConfigMapDependent();
+
+ assertThatCode(() -> dependent.getSecondaryResource(PRIMARY, context))
+ .doesNotThrowAnyException();
+ }
+
+ /**
+ * The desired state of the other dependent is asked for while the one of the first is still being
+ * computed. Dependents that land in the same bin of the map holding the desired states used to
+ * fail with "Recursive update".
+ */
+ @Test
+ void aspectCanLookUpActualResourceOfAnotherDependent() {
+ final var other = new ChildConfigMapDependent();
+ final var dependent = new ChildConfigMapDependent();
+ final DesiredStateAspect aspect =
+ (desired, dependentResource, ctx) -> {
+ if (dependentResource == dependent) {
+ ((DependentResource) other).getSecondaryResource(ctx.getPrimaryResource(), ctx);
+ }
+ };
+ final var context = contextWith(List.of(aspect));
+
+ assertThatCode(() -> dependent.getSecondaryResource(PRIMARY, context))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void desiredStateIsComputedOnce() {
+ final var context = contextWith(List.of());
+ final var dependent = new ChildConfigMapDependent();
+
+ final var first = dependent.getOrComputeDesired(context);
+
+ assertThat(dependent.getOrComputeDesired(context)).isSameAs(first);
+ }
+
+ private static DefaultContext