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..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 @@ -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( @@ -254,17 +259,58 @@ public DefaultContext

setRetryInfo(RetryInfo retryInfo) { return this; } - @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; - }); + 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 + 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, aspectTarget); + } finally { + desiredStatesInProgress.remove(dependentResource); + } + desiredStates.put(dependentResource, desired); + return desired; + } finally { + lock.unlock(); + } } /** @@ -273,11 +319,13 @@ 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() .forEach(aspect -> aspect.apply(hasMetadata, dependentResource, this)); } } + + private record DesiredStateInProgress(Thread thread, Object desired) {} } 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; + } + } +} 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); + } + } +}