From 63367a5945e9010c5841c37e7a7f7b887bad00d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 28 Oct 2026 10:30:36 +0100 Subject: [PATCH 1/2] fix: compute desired states outside of ConcurrentHashMap.computeIfAbsent Applying the desired state aspects inside computeIfAbsent failed with "Recursive update" when an aspect looked up the actual resource of the dependent (same key) or of another dependent whose key landed in the same bin. Compute with a lock per dependent instead, and hand the thread already computing a desired state the state as it is so far. Co-Authored-By: Claude Sonnet 5.5 --- .../api/reconciler/DefaultContext.java | 52 ++++++- .../DesiredStateAspectReentrancyTest.java | 145 ++++++++++++++++++ 2 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectReentrancyTest.java 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, Object> desiredStates = new ConcurrentHashMap<>(); + private final Map, DesiredStateInProgress> desiredStatesInProgress = + new ConcurrentHashMap<>(); + private final Map, Lock> desiredStateLocks = new ConcurrentHashMap<>(); private final ResourceOperations

resourceOperations; public DefaultContext( @@ -257,14 +262,43 @@ public DefaultContext

setRetryInfo(RetryInfo retryInfo) { @SuppressWarnings("unchecked") public R getOrComputeDesiredStateFor( DependentResource dependentResource, Function 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 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 contextWith(List aspects) { + final ConfigurationService configurationService = mock(); + when(configurationService.desiredStateAspects()).thenReturn(aspects); + final ControllerConfiguration controllerConfiguration = mock(); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + final Controller controller = mock(); + when(controller.getConfiguration()).thenReturn(controllerConfiguration); + final EventSourceManager esm = mock(); + final EventSource es = mock(); + when(es.getSecondaryResources(any())).thenReturn(Set.of(configMap("actual"))); + when(esm.getEventSourcesFor(ConfigMap.class)).thenReturn(List.of(es)); + when(controller.getEventSourceManager()).thenReturn(esm); + return new DefaultContext<>(null, controller, PRIMARY, false, false); + } + + private static ConfigMap configMap(String value) { + return new ConfigMapBuilder() + .withNewMetadata() + .withName("child") + .withNamespace("ns") + .endMetadata() + .withData(Map.of("key", value)) + .build(); + } + + private static class ChildConfigMapDependent + extends KubernetesDependentResource { + ChildConfigMapDependent() { + super(ConfigMap.class); + } + + @Override + public int hashCode() { + return 1; + } + + @Override + protected ConfigMap desired(ConfigMap primary, Context context) { + return configMap("desired"); + } + + @Override + public ConfigMap getOrComputeDesired(Context context) { + return super.getOrComputeDesired(context); + } + } +} From decb81ffac208e64d19bf9725267c74bc90fdc04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 29 Oct 2026 11:29:11 +0100 Subject: [PATCH 2/2] fix: desired state aspects for bulk dependents and a missing controller configuration Hand the user's bulk dependent, rather than the stand-in the framework creates per resource, to the desired state aspects, and skip the aspects if the context has no controller configuration. --- .../api/reconciler/DefaultContext.java | 18 +- .../BulkDependentResourceReconciler.java | 10 + .../DesiredStateAspectAuditTest.java | 222 ++++++++++++++++++ 3 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectAuditTest.java 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 328ef651c9..d48d97986b 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 @@ -259,9 +259,21 @@ public DefaultContext

setRetryInfo(RetryInfo retryInfo) { return this; } - @SuppressWarnings("unchecked") public R getOrComputeDesiredStateFor( DependentResource dependentResource, Function desiredStateComputer) { + return getOrComputeDesiredStateFor(dependentResource, dependentResource, desiredStateComputer); + } + + /** + * Same as {@link #getOrComputeDesiredStateFor(DependentResource, Function)} but hands {@code + * aspectTarget} rather than {@code dependentResource} to the {@link DesiredStateAspect}s, for + * dependents that are only a stand-in for the one the user configured. + */ + @SuppressWarnings("unchecked") + public R getOrComputeDesiredStateFor( + DependentResource dependentResource, + DependentResource aspectTarget, + Function desiredStateComputer) { // 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 @@ -290,7 +302,7 @@ public R getOrComputeDesiredStateFor( desiredStatesInProgress.put( dependentResource, new DesiredStateInProgress(Thread.currentThread(), desired)); try { - applyDesiredStateAspects(desired, dependentResource); + applyDesiredStateAspects(desired, aspectTarget); } finally { desiredStatesInProgress.remove(dependentResource); } @@ -307,7 +319,7 @@ public R getOrComputeDesiredStateFor( * resources are therefore left untouched. */ private void applyDesiredStateAspects(Object desired, DependentResource dependentResource) { - if (desired instanceof HasMetadata hasMetadata) { + if (desired instanceof HasMetadata hasMetadata && controllerConfiguration != null) { controllerConfiguration .getConfigurationService() .desiredStateAspects() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java index 2428eeba96..bfe46332c9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java @@ -24,8 +24,10 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; import io.javaoperatorsdk.operator.api.reconciler.Ignore; import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; class BulkDependentResourceReconciler @@ -115,6 +117,14 @@ public R update(R actual, R desired, P primary, Context

context) { return ((Updater) bulkDependentResource).update(actual, desired, primary, context); } + @Override + @SuppressWarnings("unchecked") + protected R getOrComputeDesired(Context

context) { + return ((DefaultContext

) context) + .getOrComputeDesiredStateFor( + this, (DependentResource) bulkDependentResource, p -> desired); + } + @Override public Result match(R resource, P primary, Context

context) { // retrieve the desired state via the context so that it is processed the same way as for diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectAuditTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectAuditTest.java new file mode 100644 index 0000000000..595327ec50 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/DesiredStateAspectAuditTest.java @@ -0,0 +1,222 @@ +/* + * 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.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +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.Matcher.Result; +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; + +/** Audit repro tests for the DesiredStateAspect feature (commit 110199c47). */ +@SuppressWarnings({"unchecked", "rawtypes"}) +class DesiredStateAspectAuditTest { + + private static final ConfigMap PRIMARY = + new ConfigMapBuilder() + .withNewMetadata() + .withName("primary") + .withNamespace("ns") + .endMetadata() + .build(); + + private static Controller controllerWith(List aspects) { + final ConfigurationService configurationService = mock(); + when(configurationService.desiredStateAspects()).thenReturn(aspects); + final ControllerConfiguration controllerConfiguration = mock(); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + final Controller controller = mock(); + when(controller.getConfiguration()).thenReturn(controllerConfiguration); + return controller; + } + + /** + * The aspect contract says it receives "the DependentResource the desired state was computed + * for". For bulk dependents, it instead receives the private per-item wrapper + * BulkDependentResourceReconciler.BulkDependentResourceInstance, so aspects keyed on the + * dependent (type checks, name(), getClass().getSimpleName() as in DesiredStateAspectIT) + * misbehave. + */ + @Test + void bulkDependentAspectReceivesTheUserBulkDependentResource() { + final List> seen = new ArrayList<>(); + final DesiredStateAspect aspect = + (desired, dependentResource, ctx) -> seen.add(dependentResource); + final var context = + new DefaultContext<>(null, controllerWith(List.of(aspect)), PRIMARY, false, false); + + final var bulk = new CreatingBulk(); + bulk.reconcile(PRIMARY, context); + + assertThat(seen).hasSize(1); + assertThat(seen.get(0).name()).isEqualTo(bulk.name()); + assertThat(seen.get(0)).isSameAs(bulk); + } + + /** + * Aspects run inside ConcurrentHashMap.computeIfAbsent. An aspect that looks up the actual + * resource of the dependent it is given (e.g. to preserve an existing label) re-enters + * getOrComputeDesired for the same key through KubernetesDependentResource + * .selectTargetSecondaryResource -> targetSecondaryResourceID, and CHM throws "Recursive update". + */ + @Test + void aspectCanLookUpActualResourceOfTheDependent() { + final DesiredStateAspect aspect = + (desired, dependentResource, ctx) -> + ((DependentResource) dependentResource) + .getSecondaryResource(ctx.getPrimaryResource(), ctx); + final var controller = controllerWith(List.of(aspect)); + final EventSourceManager esm = mock(); + final EventSource es = mock(); + when(es.getSecondaryResources(any())).thenReturn(Set.of(configMap("child", "actual"))); + when(esm.getEventSourcesFor(ConfigMap.class)).thenReturn(List.of(es)); + when(controller.getEventSourceManager()).thenReturn(esm); + final var context = new DefaultContext<>(null, controller, PRIMARY, false, false); + + final var dependent = new ChildConfigMapDependent(); + + assertThatCode(() -> dependent.getSecondaryResource(PRIMARY, context)) + .doesNotThrowAnyException(); + } + + /** + * Before 110199c47, a DefaultContext built on a Controller whose getConfiguration() is not + * stubbed (the pattern the existing tests used: new DefaultContext<>(mock(), mock(), ...)) could + * compute desired states. Now applyDesiredStateAspects dereferences the null + * ControllerConfiguration for every HasMetadata desired state. + */ + @Test + void desiredComputationDoesNotRequireAspectConfiguration() { + final var context = new DefaultContext<>(null, mock(Controller.class), PRIMARY, false, false); + final var dependent = new ChildConfigMapDependent(); + + assertThatCode(() -> dependent.getOrComputeDesired(context)).doesNotThrowAnyException(); + } + + private static ConfigMap configMap(String name, String value) { + return new ConfigMapBuilder() + .withNewMetadata() + .withName(name) + .withNamespace("ns") + .endMetadata() + .withData(Map.of("key", value)) + .build(); + } + + private static class ChildConfigMapDependent + extends KubernetesDependentResource { + ChildConfigMapDependent() { + super(ConfigMap.class); + } + + @Override + protected ConfigMap desired(ConfigMap primary, Context context) { + return configMap("child", "desired"); + } + + @Override + public ConfigMap getOrComputeDesired(Context context) { + return super.getOrComputeDesired(context); + } + } + + private static class CreatingBulk extends AbstractDependentResource + implements BulkDependentResource, + Creator { + + CreatingBulk() { + super("my-bulk"); + } + + @Override + public Map desiredResources(ConfigMap primary, Context context) { + var desired = new LinkedHashMap(); + desired.put("a", configMap("a", "desired")); + return desired; + } + + @Override + public Map getSecondaryResources( + ConfigMap primary, Context context) { + return Map.of(); + } + + @Override + public ConfigMap create(ConfigMap desired, ConfigMap primary, Context context) { + return desired; + } + + @Override + public void deleteTargetResource( + ConfigMap primary, ConfigMap resource, String key, Context context) {} + + @Override + public Result match( + ConfigMap actualResource, + ConfigMap desired, + ConfigMap primary, + Context context) { + return Result.computed(false, desired); + } + + @Override + public Result match( + ConfigMap resource, ConfigMap primary, Context context) { + return Result.computed(false, resource); + } + + @Override + protected Optional selectTargetSecondaryResource( + Set secondaryResources, ConfigMap primary, Context context) { + return Optional.empty(); + } + + @Override + protected void onCreated(ConfigMap primary, ConfigMap created, Context context) {} + + @Override + protected void onUpdated( + ConfigMap primary, ConfigMap updated, ConfigMap actual, Context context) {} + + @Override + public Class resourceType() { + return ConfigMap.class; + } + } +}