fix: handling delete non-active dependent when parent reconcile condition false (#3273)

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>

Author: csviri

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java

@@ -157,6 +157,11 @@ protected <R> boolean isConditionMet(
     return condition
         .map(
             c -> {
+              Optional<DetailedCondition.Result> existingResult =
+                  createOrGetResultFor(dependentResource).getConditionResult(c.type());
+              if (existingResult.isPresent()) {
+                return existingResult.get();
+              }
               final DetailedCondition.Result<?> r = c.detailedIsMet(dr, primary, context);
               synchronized (this) {
                 results

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/BaseWorkflowResult.java

@@ -172,6 +172,15 @@ public boolean hasPostDeleteConditionNotMet() {
       return deletePostconditionResult != null && !deletePostconditionResult.isSuccess();
     }
 
+    public Optional<DetailedCondition.Result> getConditionResult(Condition.Type conditionType) {
+      return switch (conditionType) {
+        case ACTIVATION -> Optional.ofNullable(activationConditionResult);
+        case DELETE -> Optional.ofNullable(deletePostconditionResult);
+        case READY -> Optional.ofNullable(readyPostconditionResult);
+        case RECONCILE -> Optional.ofNullable(reconcilePostconditionResult);
+      };
+    }
+
     public boolean isReady() {
       return readyPostconditionResult == null || readyPostconditionResult.isSuccess();
     }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java

@@ -100,7 +100,7 @@ private synchronized <R> void handleReconcile(DependentResourceNode<R, P> depend
           isConditionMet(dependentResourceNode.getReconcilePrecondition(), dependentResourceNode);
     }
     if (!reconcileConditionMet || !activationConditionMet) {
-      handleReconcileOrActivationConditionNotMet(dependentResourceNode, activationConditionMet);
+      handleReconcileOrActivationConditionNotMet(dependentResourceNode);
     } else {
       submit(dependentResourceNode, new NodeReconcileExecutor<>(dependentResourceNode), RECONCILE);
     }
@@ -183,7 +183,10 @@ private NodeDeleteExecutor(DependentResourceNode<R, P> dependentResourceNode) {
     @SuppressWarnings("unchecked")
     protected void doRun(DependentResourceNode<R, P> dependentResourceNode) {
       boolean deletePostConditionMet = true;
-      if (isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode)) {
+      var active =
+          isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode);
+      registerOrDeregisterEventSourceBasedOnActivation(active, dependentResourceNode);
+      if (active) {
         // GarbageCollected status is irrelevant here, as this method is only called when a
         // precondition does not hold,
         // a deleter should be deleted even if it is otherwise garbage collected
@@ -194,7 +197,6 @@ protected void doRun(DependentResourceNode<R, P> dependentResourceNode) {
         deletePostConditionMet =
             isConditionMet(dependentResourceNode.getDeletePostcondition(), dependentResourceNode);
       }
-
       createOrGetResultFor(dependentResourceNode).markAsVisited();
       if (deletePostConditionMet) {
         handleDependentDeleted(dependentResourceNode);
@@ -232,38 +234,20 @@ private synchronized void handleDependentsReconcile(
   }
 
   private void handleReconcileOrActivationConditionNotMet(
-      DependentResourceNode<?, P> dependentResourceNode, boolean activationConditionMet) {
+      DependentResourceNode<?, P> dependentResourceNode) {
     Set<DependentResourceNode> bottomNodes = new HashSet<>();
-    markDependentsForDelete(dependentResourceNode, bottomNodes, activationConditionMet);
+    markDependentsForDelete(dependentResourceNode, bottomNodes);
     bottomNodes.forEach(this::handleDelete);
   }
 
   private void markDependentsForDelete(
-      DependentResourceNode<?, P> dependentResourceNode,
-      Set<DependentResourceNode> bottomNodes,
-      boolean activationConditionMet) {
-    // this is a check so the activation condition is not evaluated twice,
-    // so if the activation condition was false, this node is not meant to be deleted.
+      DependentResourceNode<?, P> dependentResourceNode, Set<DependentResourceNode> bottomNodes) {
     var dependents = dependentResourceNode.getParents();
-    if (activationConditionMet) {
-      // make sure we register the dependent's event source if it hasn't been added already
-      // this might be needed in corner cases such as
-      // https://github.com/operator-framework/java-operator-sdk/issues/3249
-      registerOrDeregisterEventSourceBasedOnActivation(true, dependentResourceNode);
-      createOrGetResultFor(dependentResourceNode).markForDelete();
-      if (dependents.isEmpty()) {
-        bottomNodes.add(dependentResourceNode);
-      } else {
-        dependents.forEach(d -> markDependentsForDelete(d, bottomNodes, true));
-      }
+    createOrGetResultFor(dependentResourceNode).markForDelete();
+    if (dependents.isEmpty()) {
+      bottomNodes.add(dependentResourceNode);
     } else {
-      // this is for an edge case when there is only one resource but that is not active
-      createOrGetResultFor(dependentResourceNode).markAsVisited();
-      if (dependents.isEmpty()) {
-        handleNodeExecutionFinish(dependentResourceNode);
-      } else {
-        dependents.forEach(d -> markDependentsForDelete(d, bottomNodes, true));
-      }
+      dependents.forEach(d -> markDependentsForDelete(d, bottomNodes));
     }
   }
 

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java

@@ -45,6 +45,7 @@ class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest {
   Context<TestCustomResource> mockContext = spy(Context.class);
 
   ExecutorService executorService = Executors.newCachedThreadPool();
+  EventSourceRetriever eventSourceRetriever = mock(EventSourceRetriever.class);
 
   TestDependent dr3 = new TestDependent("DR_3");
   TestDependent dr4 = new TestDependent("DR_4");
@@ -56,7 +57,7 @@ void setup(TestInfo testInfo) {
     when(mockContext.managedWorkflowAndDependentResourceContext())
         .thenReturn(mock(ManagedWorkflowAndDependentResourceContext.class));
     when(mockContext.getWorkflowExecutorService()).thenReturn(executorService);
-    when(mockContext.eventSourceRetriever()).thenReturn(mock(EventSourceRetriever.class));
+    when(mockContext.eventSourceRetriever()).thenReturn(eventSourceRetriever);
   }
 
   @Test
@@ -693,6 +694,52 @@ void activationConditionOnlyCalledOnceOnDeleteDependents() {
     verify(condition, times(1)).isMet(any(), any(), any());
   }
 
+  @Test
+  @SuppressWarnings("unchecked")
+  void activationConditionEventSourceRegistrationWithParentWithFalsePrecondition() {
+    var workflow =
+        new WorkflowBuilder<TestCustomResource>()
+            .addDependentResourceAndConfigure(dr1)
+            .withReconcilePrecondition(notMetCondition)
+            .addDependentResourceAndConfigure(drDeleter)
+            .withActivationCondition(notMetCondition)
+            .dependsOn(dr1)
+            .build();
+
+    workflow.reconcile(new TestCustomResource(), mockContext);
+    assertThat(executionHistory).notReconciled(dr1, drDeleter);
+    verify(eventSourceRetriever, never()).dynamicallyRegisterEventSource(any());
+    verify(eventSourceRetriever, times(1)).dynamicallyDeRegisterEventSource(any());
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  void activationConditionEventSourceRegistration() {
+    var workflow =
+        new WorkflowBuilder<TestCustomResource>()
+            .addDependentResourceAndConfigure(dr1)
+            .addDependentResourceAndConfigure(drDeleter)
+            .withActivationCondition(notMetCondition)
+            .dependsOn(dr1)
+            .build();
+
+    workflow.reconcile(new TestCustomResource(), mockContext);
+    assertThat(executionHistory).reconciled(dr1).notReconciled(drDeleter);
+    verify(eventSourceRetriever, never()).dynamicallyRegisterEventSource(any());
+    verify(eventSourceRetriever, atLeast(1)).dynamicallyDeRegisterEventSource(any());
+  }
+
+  @Test
+  void oneDependentWithActivationCondition() {
+    var workflow =
+        new WorkflowBuilder<TestCustomResource>()
+            .addDependentResourceAndConfigure(dr1)
+            .withActivationCondition(notMetCondition)
+            .build();
+    workflow.reconcile(new TestCustomResource(), mockContext);
+    assertThat(executionHistory).notReconciled(dr1);
+  }
+
   @Test
   void resultFromReadyConditionShouldBeAvailableIfExisting() {
     final var result = Integer.valueOf(42);

operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/onedependentactivationcondition/ConfigMapDependentResource.java

@@ -0,0 +1,39 @@
+/*
+ * 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.workflow.onedependentactivationcondition;
+
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource;
+
+public class ConfigMapDependentResource
+    extends CRUDKubernetesDependentResource<
+        ConfigMap, OneDependentActivationConditionCustomResource> {
+
+  @Override
+  protected ConfigMap desired(
+      OneDependentActivationConditionCustomResource primary,
+      Context<OneDependentActivationConditionCustomResource> context) {
+    ConfigMap configMap = new ConfigMap();
+    configMap.setMetadata(
+        new ObjectMetaBuilder()
+            .withName(primary.getMetadata().getName())
+            .withNamespace(primary.getMetadata().getNamespace())
+            .build());
+    return configMap;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/onedependentactivationcondition/FalseActivationCondition.java

@@ -0,0 +1,32 @@
+/*
+ * 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.workflow.onedependentactivationcondition;
+
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;
+
+public class FalseActivationCondition
+    implements Condition<ConfigMap, OneDependentActivationConditionCustomResource> {
+  @Override
+  public boolean isMet(
+      DependentResource<ConfigMap, OneDependentActivationConditionCustomResource> dependentResource,
+      OneDependentActivationConditionCustomResource primary,
+      Context<OneDependentActivationConditionCustomResource> context) {
+    return false;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/onedependentactivationcondition/OneDependentActivationConditionCustomResource.java

@@ -0,0 +1,28 @@
+/*
+ * 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.workflow.onedependentactivationcondition;
+
+import io.fabric8.kubernetes.api.model.Namespaced;
+import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.model.annotation.Group;
+import io.fabric8.kubernetes.model.annotation.ShortNames;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+@Group("sample.javaoperatorsdk")
+@Version("v1")
+@ShortNames("odac")
+public class OneDependentActivationConditionCustomResource extends CustomResource<Void, Void>
+    implements Namespaced {}

operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/onedependentactivationcondition/OneDependentActivationConditionIT.java

@@ -0,0 +1,62 @@
+/*
+ * 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.workflow.onedependentactivationcondition;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class OneDependentActivationConditionIT {
+
+  public static final String TEST_RESOURCE_NAME = "test1";
+
+  @RegisterExtension
+  LocallyRunOperatorExtension extension =
+      LocallyRunOperatorExtension.builder()
+          .withReconciler(OneDependentActivationConditionReconciler.class)
+          .build();
+
+  @Test
+  void handlesOnlyNonActiveDependent() {
+    extension.create(testResource());
+
+    await()
+        .untilAsserted(
+            () -> {
+              assertThat(
+                      extension
+                          .getReconcilerOfType(OneDependentActivationConditionReconciler.class)
+                          .getNumberOfReconciliationExecution())
+                  .isPositive();
+            });
+
+    // ConfigMap should not be created since the activation condition is false
+    var cm = extension.get(ConfigMap.class, TEST_RESOURCE_NAME);
+    assertThat(cm).isNull();
+  }
+
+  private OneDependentActivationConditionCustomResource testResource() {
+    var res = new OneDependentActivationConditionCustomResource();
+    res.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build());
+    return res;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/workflow/onedependentactivationcondition/OneDependentActivationConditionReconciler.java

@@ -0,0 +1,50 @@
+/*
+ * 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.workflow.onedependentactivationcondition;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
+import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
+import io.javaoperatorsdk.operator.api.reconciler.Workflow;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent;
+
+@Workflow(
+    dependents = {
+      @Dependent(
+          type = ConfigMapDependentResource.class,
+          activationCondition = FalseActivationCondition.class)
+    })
+@ControllerConfiguration
+public class OneDependentActivationConditionReconciler
+    implements Reconciler<OneDependentActivationConditionCustomResource> {
+
+  private final AtomicInteger numberOfReconciliationExecution = new AtomicInteger(0);
+
+  @Override
+  public UpdateControl<OneDependentActivationConditionCustomResource> reconcile(
+      OneDependentActivationConditionCustomResource resource,
+      Context<OneDependentActivationConditionCustomResource> context) {
+    numberOfReconciliationExecution.incrementAndGet();
+    return UpdateControl.noUpdate();
+  }
+
+  public int getNumberOfReconciliationExecution() {
+    return numberOfReconciliationExecution.get();
+  }
+}