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 f3fade4659..f1aeadd52a 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 @@ -15,15 +15,19 @@ */ package io.javaoperatorsdk.operator.api.reconciler; +import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedWorkflowAndDependentResourceContext; import io.javaoperatorsdk.operator.processing.Controller; @@ -41,6 +45,7 @@ public class DefaultContext

implements Context

{ defaultManagedDependentResourceContext; private final boolean primaryResourceDeleted; private final boolean primaryResourceFinalStateUnknown; + private final Map, Object> desiredStates = new ConcurrentHashMap<>(); public DefaultContext( RetryInfo retryInfo, @@ -157,4 +162,12 @@ public DefaultContext

setRetryInfo(RetryInfo retryInfo) { this.retryInfo = retryInfo; return this; } + + @SuppressWarnings("unchecked") + public R getOrComputeDesiredStateFor( + DependentResource dependentResource, Function desiredStateComputer) { + return (R) + desiredStates.computeIfAbsent( + dependentResource, ignored -> desiredStateComputer.apply(getPrimaryResource())); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index a7c5ce9e2d..8dc62b4ca7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -23,6 +23,7 @@ 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; @@ -85,7 +86,7 @@ protected ReconcileResult reconcile(P primary, R actualResource, Context

c if (creatable() || updatable()) { if (actualResource == null) { if (creatable) { - var desired = desired(primary, context); + var desired = getOrComputeDesired(context); throwIfNull(desired, primary, "Desired"); logForOperation("Creating", primary, desired); var createdResource = handleCreate(desired, primary, context); @@ -95,7 +96,8 @@ protected ReconcileResult reconcile(P primary, R actualResource, Context

c if (updatable()) { final Matcher.Result match = match(actualResource, primary, context); if (!match.matched()) { - final var desired = match.computedDesired().orElseGet(() -> desired(primary, context)); + final var desired = + match.computedDesired().orElseGet(() -> getOrComputeDesired(context)); throwIfNull(desired, primary, "Desired"); logForOperation("Updating", primary, desired); var updatedResource = handleUpdate(actualResource, desired, primary, context); @@ -127,7 +129,6 @@ protected ReconcileResult reconcile(P primary, R actualResource, Context

c @Override public Optional getSecondaryResource(P primary, Context

context) { - var secondaryResources = context.getSecondaryResources(resourceType()); if (secondaryResources.isEmpty()) { return Optional.empty(); @@ -212,6 +213,27 @@ protected R desired(P primary, Context

context) { + " updated"); } + /** + * Retrieves the desired state from the {@link Context} if it has already been computed or calls + * {@link #desired(HasMetadata, Context)} and stores its result in the context for further use. + * This ensures that {@code desired} is only called once per reconciliation to avoid unneeded + * processing and supports scenarios where idempotent computation of the desired state is not + * feasible. + * + *

Note that this method should normally only be called by the SDK itself and exclusively (i.e. + * {@link #desired(HasMetadata, Context)} should not be called directly by the SDK) whenever the + * desired state is needed to ensure it is properly cached for the current reconciliation. + * + * @param context the {@link Context} in scope for the current reconciliation + * @return the desired state associated with this dependent resource based on the currently + * in-scope primary resource as found in the context + */ + protected R getOrComputeDesired(Context

context) { + assert context instanceof DefaultContext

; + DefaultContext

defaultContext = (DefaultContext

) context; + return defaultContext.getOrComputeDesiredStateFor(this, p -> desired(p, defaultContext)); + } + public void delete(P primary, Context

context) { dependentResourceReconciler.delete(primary, context); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java index e601e937cf..7b83a377c1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java @@ -105,7 +105,7 @@ protected void handleExplicitStateCreation(P primary, R created, Context

cont @Override public Matcher.Result match(R resource, P primary, Context

context) { - var desired = desired(primary, context); + var desired = getOrComputeDesired(context); return Matcher.Result.computed(resource.equals(desired), 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 5b3617c26c..23135f81b1 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 @@ -27,7 +27,6 @@ import io.javaoperatorsdk.operator.api.reconciler.Ignore; import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; -import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result; class BulkDependentResourceReconciler implements DependentResourceReconciler { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java index 0ba48797af..5562c883e2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java @@ -138,7 +138,7 @@ public static Matcher.Result m Context

context, boolean labelsAndAnnotationsEquality, String... ignorePaths) { - final var desired = dependentResource.desired(primary, context); + final var desired = dependentResource.getOrComputeDesired(context); return match(desired, actualResource, labelsAndAnnotationsEquality, context, ignorePaths); } @@ -150,7 +150,7 @@ public static Matcher.Result m boolean specEquality, boolean labelsAndAnnotationsEquality, String... ignorePaths) { - final var desired = dependentResource.desired(primary, context); + final var desired = dependentResource.getOrComputeDesired(context); return match( desired, actualResource, labelsAndAnnotationsEquality, specEquality, context, ignorePaths); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index 562a6257b5..5d53b807cc 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -143,7 +143,7 @@ public R update(R actual, R desired, P primary, Context

context) { @Override public Result match(R actualResource, P primary, Context

context) { - final var desired = desired(primary, context); + final var desired = getOrComputeDesired(context); return match(actualResource, desired, primary, context); } @@ -297,7 +297,7 @@ protected Optional selectTargetSecondaryResource( * @return id of the target managed resource */ protected ResourceID targetSecondaryResourceID(P primary, Context

context) { - return ResourceID.fromResource(desired(primary, context)); + return ResourceID.fromResource(getOrComputeDesired(context)); } protected boolean addOwnerReference() { @@ -305,8 +305,8 @@ protected boolean addOwnerReference() { } @Override - protected R desired(P primary, Context

context) { - return super.desired(primary, context); + protected R getOrComputeDesired(Context

context) { + return super.getOrComputeDesired(context); } @Override diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java index bb9d6cf71e..1db69a1f9e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java @@ -21,8 +21,10 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static org.junit.jupiter.api.Assertions.*; @@ -31,6 +33,13 @@ class AbstractDependentResourceTest { + private static final TestCustomResource PRIMARY = new TestCustomResource(); + private static final DefaultContext CONTEXT = createContext(PRIMARY); + + private static DefaultContext createContext(TestCustomResource primary) { + return new DefaultContext<>(mock(), mock(), primary, false, false); + } + @Test void throwsExceptionIfDesiredIsNullOnCreate() { TestDependentResource testDependentResource = new TestDependentResource(); @@ -38,8 +47,7 @@ void throwsExceptionIfDesiredIsNullOnCreate() { testDependentResource.setDesired(null); assertThrows( - DependentResourceException.class, - () -> testDependentResource.reconcile(new TestCustomResource(), null)); + DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT)); } @Test @@ -49,8 +57,7 @@ void throwsExceptionIfDesiredIsNullOnUpdate() { testDependentResource.setDesired(null); assertThrows( - DependentResourceException.class, - () -> testDependentResource.reconcile(new TestCustomResource(), null)); + DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT)); } @Test @@ -60,8 +67,7 @@ void throwsExceptionIfCreateReturnsNull() { testDependentResource.setDesired(configMap()); assertThrows( - DependentResourceException.class, - () -> testDependentResource.reconcile(new TestCustomResource(), null)); + DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT)); } @Test @@ -71,8 +77,28 @@ void throwsExceptionIfUpdateReturnsNull() { testDependentResource.setDesired(configMap()); assertThrows( - DependentResourceException.class, - () -> testDependentResource.reconcile(new TestCustomResource(), null)); + DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT)); + } + + @Test + void checkThatDesiredIsOnlyCalledOnce() { + final var testDependentResource = new DesiredCallCountCheckingDR(); + final var primary = new TestCustomResource(); + final var spec = primary.getSpec(); + spec.setConfigMapName("foo"); + spec.setKey("key"); + spec.setValue("value"); + final var context = createContext(primary); + testDependentResource.reconcile(primary, context); + + spec.setValue("value2"); + testDependentResource.reconcile(primary, context); + + assertEquals(1, testDependentResource.desiredCallCount); + + context.getOrComputeDesiredStateFor( + testDependentResource, p -> testDependentResource.desired(p, context)); + assertEquals(1, testDependentResource.desiredCallCount); } private ConfigMap configMap() { @@ -130,22 +156,12 @@ protected ConfigMap desired(TestCustomResource primary, Context match( return result; } } + + private static class DesiredCallCountCheckingDR extends TestDependentResource { + private short desiredCallCount; + + @Override + public ConfigMap update( + ConfigMap actual, + ConfigMap desired, + TestCustomResource primary, + Context context) { + return desired; + } + + @Override + public ConfigMap create( + ConfigMap desired, TestCustomResource primary, Context context) { + return desired; + } + + @Override + protected ConfigMap desired(TestCustomResource primary, Context context) { + final var spec = primary.getSpec(); + desiredCallCount++; + return new ConfigMapBuilder() + .editOrNewMetadata() + .withName(spec.getConfigMapName()) + .endMetadata() + .addToData(spec.getKey(), spec.getValue()) + .build(); + } + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java index 495fe98416..8a920b28b9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java @@ -18,37 +18,48 @@ import java.util.Map; import java.util.Optional; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.*; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; import io.fabric8.kubernetes.api.model.apps.DeploymentStatusBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; @SuppressWarnings({"unchecked"}) class GenericKubernetesResourceMatcherTest { - private static final Context context = mock(Context.class); + private static final Context context = new TestContext(); + + private static class TestContext extends DefaultContext { + private final KubernetesClient client = MockKubernetesClient.client(HasMetadata.class); + + public TestContext() { + this(null); + } + + public TestContext(HasMetadata primary) { + super(mock(), mock(), primary, false, false); + } + + @Override + public KubernetesClient getClient() { + return client; + } + } Deployment actual = createDeployment(); Deployment desired = createDeployment(); TestDependentResource dependentResource = new TestDependentResource(desired); - @BeforeAll - static void setUp() { - final var client = MockKubernetesClient.client(HasMetadata.class); - when(context.getClient()).thenReturn(client); - } - @Test void matchesTrivialCases() { assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched()).isTrue(); @@ -77,9 +88,10 @@ void matchesWithStrongSpecEquality() { @Test void doesNotMatchRemovedValues() { actual = createDeployment(); + final var localContext = new TestContext(createPrimary("removed")); assertThat( GenericKubernetesResourceMatcher.match( - dependentResource.desired(createPrimary("removed"), null), actual, context) + dependentResource.getOrComputeDesired(localContext), actual, localContext) .matched()) .withFailMessage("Removing values in metadata should lead to a mismatch") .isFalse();