fix: bug with the ready

Author: csviri

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

@@ -22,7 +22,7 @@ public class WorkflowReconcileExecutor<P extends HasMetadata> {
 
   private final Set<DependentResourceNode<?, ?>> alreadyReconciled = ConcurrentHashMap.newKeySet();
   private final Set<DependentResourceNode<?, ?>> errored = ConcurrentHashMap.newKeySet();
-  private final Set<DependentResourceNode<?, ?>> reconciledButNotReady =
+  private final Set<DependentResourceNode<?, ?>> notReady =
       ConcurrentHashMap.newKeySet();
   private final Set<DependentResourceNode<?, ?>> reconcileConditionOrParentsConditionNotMet =
       ConcurrentHashMap.newKeySet();
@@ -73,8 +73,8 @@ private synchronized void handleReconcile(
 
     if (alreadyReconciled(dependentResourceNode)
         || isReconcilingNow(dependentResourceNode)
-        || !allDependsReconciledAndReady(dependentResourceNode)
-        || hasErroredDependOn(dependentResourceNode)) {
+        || !allParentsReconciledAndReady(dependentResourceNode)
+        || hasErroredParent(dependentResourceNode)) {
       log.debug("Skipping submit of: {}, ", dependentResourceNode);
       return;
     }
@@ -103,16 +103,26 @@ private synchronized void handleExceptionInExecutor(DependentResourceNode depend
   }
 
   private synchronized void handleNodeExecutionFinish(DependentResourceNode dependentResourceNode) {
+    log.debug("Finished execution for: {}", dependentResourceNode);
     actualExecutions.remove(dependentResourceNode);
     if (actualExecutions.isEmpty()) {
       this.notifyAll();
     }
   }
 
+  // needs to be synced
   private synchronized void updateStatusForNotReady(ReadyCondition<?, P> readyCondition) {
     readyCondition.addNotReadyStatusInfo(primary);
   }
 
+  // needs to be in one step
+  private synchronized void setAlreadyReconciledButNotReady(
+      DependentResourceNode<?, P> dependentResourceNode) {
+    log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode);
+    alreadyReconciled.add(dependentResourceNode);
+    notReady.add(dependentResourceNode);
+  }
+
   private boolean ownOrParentsReconcileConditionNotMet(
       DependentResourceNode<?, ?> dependentResourceNode) {
     return reconcileConditionOrParentsConditionNotMet.contains(dependentResourceNode) ||
@@ -136,7 +146,7 @@ private NodeExecutor(DependentResourceNode<?, P> dependentResourceNode,
     public void run() {
       try {
         DependentResource dependentResource = dependentResourceNode.getDependentResource();
-        boolean handleDependents = true;
+        boolean ready = true;
         if (onlyReconcileForPossibleDelete) {
           if (dependentResource instanceof Deleter) {
             ((Deleter<P>) dependentResource).delete(primary, context);
@@ -146,15 +156,17 @@ public void run() {
           if (dependentResourceNode.getReadyCondition().isPresent()
               && !dependentResourceNode.getReadyCondition().get()
                   .isMet(dependentResource, primary, context)) {
-            handleDependents = false;
-            reconciledButNotReady.add(dependentResourceNode);
-            // needs to be synced
+            ready = false;
             updateStatusForNotReady(dependentResourceNode.getReadyCondition().get());
           }
         }
-        alreadyReconciled.add(dependentResourceNode);
-        if (handleDependents) {
+
+        if (ready) {
+          log.debug("Setting already reconciled for: {}", dependentResourceNode);
+          alreadyReconciled.add(dependentResourceNode);
           handleDependentsReconcile(dependentResourceNode, onlyReconcileForPossibleDelete);
+        } else {
+          setAlreadyReconciledButNotReady(dependentResourceNode);
         }
       } catch (RuntimeException e) {
         handleExceptionInExecutor(dependentResourceNode, e);
@@ -172,7 +184,11 @@ private synchronized void handleDependentsReconcile(
       DependentResourceNode<?, P> dependentResourceNode, boolean onlyReconcileForPossibleDelete) {
     var dependents = workflow.getDependents().get(dependentResourceNode);
     if (dependents != null) {
-      dependents.forEach(d -> handleReconcile(d, onlyReconcileForPossibleDelete));
+
+      dependents.forEach(d -> {
+        log.debug("Handle reconcile for dependent: {} of parent:{}", d, dependentResourceNode);
+        handleReconcile(d, onlyReconcileForPossibleDelete);
+      });
     }
   }
 
@@ -199,15 +215,14 @@ private void handleReconcileCondition(DependentResourceNode<?, ?> dependentResou
     }
   }
 
-  private boolean allDependsReconciledAndReady(
+  private boolean allParentsReconciledAndReady(
       DependentResourceNode<?, ?> dependentResourceNode) {
     return dependentResourceNode.getDependsOn().isEmpty()
         || dependentResourceNode.getDependsOn().stream()
-            .allMatch(d -> alreadyReconciled(d)
-                && !reconciledButNotReady.contains(dependentResourceNode));
+            .allMatch(d -> alreadyReconciled(d) && !notReady.contains(d));
   }
 
-  private boolean hasErroredDependOn(
+  private boolean hasErroredParent(
       DependentResourceNode<?, ?> dependentResourceNode) {
     return !dependentResourceNode.getDependsOn().isEmpty()
         && dependentResourceNode.getDependsOn().stream()

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

@@ -162,7 +162,7 @@ void onlyOneDependsOnErroredResourceNotReconciled() {
     var workflow = new WorkflowBuilder<TestCustomResource>()
         .addDependent(dr1).build()
         .addDependent(drError).build()
-        .addDependent(dr2).dependsOn(drError).dependsOn(dr1).build()
+        .addDependent(dr2).dependsOn(drError, dr1).build()
         .build();
     assertThrows(AggregatedOperatorException.class,
         () -> workflow.reconcile(new TestCustomResource(), null));
@@ -313,17 +313,17 @@ void diamondShareWithReadyCondition() {
     TestDependent dr4 = new TestDependent("DR_4");
 
     var workflow = new WorkflowBuilder<TestCustomResource>()
-            .addDependent(dr1).build()
-            .addDependent(dr2).dependsOn(dr1).withReadyCondition(notMetReadyCondition).build()
-            .addDependent(dr3).dependsOn(dr1).build()
-            .addDependent(dr4).dependsOn(dr2,dr3).build()
-            .build();
+        .addDependent(dr1).build()
+        .addDependent(dr2).dependsOn(dr1).withReadyCondition(notMetReadyCondition).build()
+        .addDependent(dr3).dependsOn(dr1).build()
+        .addDependent(dr4).dependsOn(dr2, dr3).build()
+        .build();
 
     workflow.reconcile(new TestCustomResource(), null);
 
     assertThat(executionHistory).reconciledInOrder(dr1, dr2)
-            .reconciledInOrder(dr1, dr3)
-            .notReconciled(dr4);
+        .reconciledInOrder(dr1, dr3)
+        .notReconciled(dr4);
   }
 
   private class TestDependent implements DependentResource<String, TestCustomResource> {