feat: allow returning additional information from conditions (#2426)

Fixes #2424. --------- Signed-off-by: Attila Mészáros <[email protected]> Signed-off-by: Chris Laprun <[email protected]> Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: Chris Laprun <[email protected]>
operator-framework · Jul 12, 2024 · e9bafc7 · e9bafc7
1 parent aaeb978
commit e9bafc7
Show file tree

Hide file tree

Showing 29 changed files with 797 additions and 283 deletions.
diff --git a/docs/content/en/docs/workflows/_index.md b/docs/content/en/docs/workflows/_index.md
@@ -49,11 +49,26 @@ reconciliation process.
   See related [integration test](https://github.com/operator-framework/java-operator-sdk/blob/ba5e33527bf9e3ea0bd33025ccb35e677f9d44b4/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDPresentActivationConditionIT.java).
 
   To have multiple resources of same type with an activation condition is a bit tricky, since you
-  don't want to have multiple `InformerEvetnSource` for the same type, you have to explicitly 
+  don't want to have multiple `InformerEventSource` for the same type, you have to explicitly 
   name the informer for the Dependent Resource (`@KubernetesDependent(informerConfig = @InformerConfig(name = "configMapInformer"))`)
   for all resource of same type with activation condition. This will make sure that only one is registered.
   See details at [low level api](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceRetriever.java#L20-L52).
 
+### Result conditions
+
+While simple conditions are usually enough, it might happen you want to convey extra information as a result of the
+evaluation of the conditions (e.g., to report error messages or because the result of the condition evaluation might be
+interesting for other purposes). In this situation, you should implement `DetailedCondition` instead of `Condition` and
+provide an implementation of the `detailedIsMet` method, which allows you to return a more detailed `Result` object via
+which you can provide extra information. The `DetailedCondition.Result` interface provides factory method for your
+convenience but you can also provide your own implementation if required.
+
+You can access the results for conditions from the `WorkflowResult` instance that is returned whenever a workflow is
+evaluated. You can access that result from the `ManagedWorkflowAndDependentResourceContext` accessible from the
+reconciliation `Context`. You can then access individual condition results using the `
+getDependentConditionResult` methods. You can see an example of this
+in [this integration test](https://github.com/operator-framework/java-operator-sdk/blob/fd0e92c0de55c47d5df50658cf4e147ee5e6102d/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java#L44-L49).
+
 ## Defining Workflows
 
 Similarly to dependent resources, there are two ways to define workflows, in managed and standalone

diff --git a/...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Workflow.java b/...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Workflow.java
@@ -8,6 +8,8 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent;
+import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowCleanupResult;
+import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowReconcileResult;
 
 @Inherited
 @Retention(RetentionPolicy.RUNTIME)
@@ -29,12 +31,11 @@
    * execution as would normally be the case. Instead, it will proceed to its
    * {@link Reconciler#reconcile(HasMetadata, Context)} method as if no error occurred. It is then
    * up to the developer to decide how to proceed by retrieving the errored dependents (and their
-   * associated exception) via
-   * {@link io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowResult#erroredDependents},
-   * the workflow result itself being accessed from
-   * {@link Context#managedWorkflowAndDependentResourceContext()}. If {@code false}, an exception
-   * will be automatically thrown at the end of the workflow execution, presenting an aggregated
-   * view of what happened.
+   * associated exception) via {@link WorkflowReconcileResult#getErroredDependents()} or
+   * {@link WorkflowCleanupResult#getErroredDependents()}, the workflow result itself being accessed
+   * from {@link Context#managedWorkflowAndDependentResourceContext()}. If {@code false}, an
+   * exception will be automatically thrown at the end of the workflow execution, presenting an
+   * aggregated view of what happened.
    */
   boolean handleExceptionsInReconciler() default false;
 

diff --git a/...a/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/...a/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java
@@ -1,12 +1,11 @@
 package io.javaoperatorsdk.operator.processing.dependent.workflow;
 
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Optional;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import org.slf4j.Logger;
@@ -20,25 +19,24 @@
 @SuppressWarnings("rawtypes")
 abstract class AbstractWorkflowExecutor<P extends HasMetadata> {
 
-  protected final Workflow<P> workflow;
+  protected final DefaultWorkflow<P> workflow;
   protected final P primary;
   protected final ResourceID primaryID;
   protected final Context<P> context;
+  protected final Map<DependentResourceNode<?, P>, WorkflowResult.DetailBuilder<?>> results;
   /**
    * Covers both deleted and reconciled
    */
-  private final Set<DependentResourceNode> alreadyVisited = ConcurrentHashMap.newKeySet();
   private final Map<DependentResourceNode, Future<?>> actualExecutions = new ConcurrentHashMap<>();
-  private final Map<DependentResourceNode, Exception> exceptionsDuringExecution =
-      new ConcurrentHashMap<>();
   private final ExecutorService executorService;
 
-  public AbstractWorkflowExecutor(Workflow<P> workflow, P primary, Context<P> context) {
+  protected AbstractWorkflowExecutor(DefaultWorkflow<P> workflow, P primary, Context<P> context) {
     this.workflow = workflow;
     this.primary = primary;
     this.context = context;
     this.primaryID = ResourceID.fromResource(primary);
     executorService = context.getWorkflowExecutorService();
+    results = new ConcurrentHashMap<>(workflow.getDependentResourcesByName().size());
   }
 
   protected abstract Logger logger();
@@ -75,11 +73,31 @@ protected boolean noMoreExecutionsScheduled() {
   }
 
   protected boolean alreadyVisited(DependentResourceNode<?, P> dependentResourceNode) {
-    return alreadyVisited.contains(dependentResourceNode);
+    return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::isVisited);
   }
 
-  protected void markAsVisited(DependentResourceNode<?, P> dependentResourceNode) {
-    alreadyVisited.add(dependentResourceNode);
+  protected boolean postDeleteConditionNotMet(DependentResourceNode<?, P> drn) {
+    return getResultFlagFor(drn, WorkflowResult.DetailBuilder::hasPostDeleteConditionNotMet);
+  }
+
+  protected boolean isMarkedForDelete(DependentResourceNode<?, P> drn) {
+    return getResultFlagFor(drn, WorkflowResult.DetailBuilder::isMarkedForDelete);
+  }
+
+  protected WorkflowResult.DetailBuilder createOrGetResultFor(
+      DependentResourceNode<?, P> dependentResourceNode) {
+    return results.computeIfAbsent(dependentResourceNode,
+        unused -> new WorkflowResult.DetailBuilder());
+  }
+
+  protected Optional<WorkflowResult.DetailBuilder<?>> getResultFor(
+      DependentResourceNode<?, P> dependentResourceNode) {
+    return Optional.ofNullable(results.get(dependentResourceNode));
+  }
+
+  protected boolean getResultFlagFor(DependentResourceNode<?, P> dependentResourceNode,
+      Function<WorkflowResult.DetailBuilder<?>, Boolean> flag) {
+    return getResultFor(dependentResourceNode).map(flag).orElse(false);
   }
 
   protected boolean isExecutingNow(DependentResourceNode<?, P> dependentResourceNode) {
@@ -94,17 +112,15 @@ protected void markAsExecuting(DependentResourceNode<?, P> dependentResourceNode
   protected synchronized void handleExceptionInExecutor(
       DependentResourceNode<?, P> dependentResourceNode,
       RuntimeException e) {
-    exceptionsDuringExecution.put(dependentResourceNode, e);
+    createOrGetResultFor(dependentResourceNode).withError(e);
   }
 
-  protected boolean isInError(DependentResourceNode<?, P> dependentResourceNode) {
-    return exceptionsDuringExecution.containsKey(dependentResourceNode);
+  protected boolean isNotReady(DependentResourceNode<?, P> dependentResourceNode) {
+    return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::isNotReady);
   }
 
-  protected Map<DependentResource, Exception> getErroredDependents() {
-    return exceptionsDuringExecution.entrySet().stream()
-        .collect(
-            Collectors.toMap(e -> e.getKey().getDependentResource(), Entry::getValue));
+  protected boolean isInError(DependentResourceNode<?, P> dependentResourceNode) {
+    return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::hasError);
   }
 
   protected synchronized void handleNodeExecutionFinish(
@@ -116,9 +132,17 @@ protected synchronized void handleNodeExecutionFinish(
     }
   }
 
-  protected <R> boolean isConditionMet(Optional<Condition<R, P>> condition,
-      DependentResource<R, P> dependentResource) {
-    return condition.map(c -> c.isMet(dependentResource, primary, context)).orElse(true);
+  @SuppressWarnings("unchecked")
+  protected <R> boolean isConditionMet(
+      Optional<ConditionWithType<R, P, ?>> condition,
+      DependentResourceNode<R, P> dependentResource) {
+    final var dr = dependentResource.getDependentResource();
+    return condition.map(c -> {
+      final DetailedCondition.Result<?> r = c.detailedIsMet(dr, primary, context);
+      results.computeIfAbsent(dependentResource, unused -> new WorkflowResult.DetailBuilder())
+          .withResultForCondition(c, r);
+      return r;
+    }).orElse(DetailedCondition.Result.metWithoutResult).isSuccess();
   }
 
   protected <R> void submit(DependentResourceNode<R, P> dependentResourceNode,
@@ -145,4 +169,10 @@ protected <R> void registerOrDeregisterEventSourceBasedOnActivation(
       }
     }
   }
+
+  protected Map<DependentResource, WorkflowResult.Detail<?>> asDetails() {
+    return results.entrySet().stream()
+        .collect(
+            Collectors.toMap(e -> e.getKey().getDependentResource(), e -> e.getValue().build()));
+  }
 }
diff --git a/...re/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java b/...re/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java
@@ -6,6 +6,10 @@
 
 public interface Condition<R, P extends HasMetadata> {
 
+  enum Type {
+    ACTIVATION, DELETE, READY, RECONCILE
+  }
+
   /**
    * Checks whether a condition holds true for a given
    * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} based on the

diff --git a/...ain/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java b/...ain/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java
@@ -0,0 +1,31 @@
+package io.javaoperatorsdk.operator.processing.dependent.workflow;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+
+class ConditionWithType<R, P extends HasMetadata, T> implements DetailedCondition<R, P, T> {
+  private final Condition<R, P> condition;
+  private final Type type;
+
+  ConditionWithType(Condition<R, P> condition, Type type) {
+    this.condition = condition;
+    this.type = type;
+  }
+
+  public Type type() {
+    return type;
+  }
+
+  @SuppressWarnings("unchecked")
+  @Override
+  public Result<T> detailedIsMet(DependentResource<R, P> dependentResource, P primary,
+      Context<P> context) {
+    if (condition instanceof DetailedCondition detailedCondition) {
+      return detailedCondition.detailedIsMet(dependentResource, primary, context);
+    } else {
+      return Result
+          .withoutResult(condition.isMet(dependentResource, primary, context));
+    }
+  }
+}
diff --git a/...rc/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java b/...rc/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java
@@ -0,0 +1,26 @@
+package io.javaoperatorsdk.operator.processing.dependent.workflow;
+
+public class DefaultResult<T> implements DetailedCondition.Result<T> {
+  private final T result;
+  private final boolean success;
+
+  public DefaultResult(boolean success, T result) {
+    this.result = result;
+    this.success = success;
+  }
+
+  @Override
+  public T getDetail() {
+    return result;
+  }
+
+  @Override
+  public boolean isSuccess() {
+    return success;
+  }
+
+  @Override
+  public String toString() {
+    return asString();
+  }
+}
diff --git a/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java b/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java
@@ -108,12 +108,10 @@ public WorkflowCleanupResult cleanup(P primary, Context<P> context) {
     return result;
   }
 
-  @Override
   public Set<DependentResourceNode> getTopLevelDependentResources() {
     return topLevelResources;
   }
 
-  @Override
   public Set<DependentResourceNode> getBottomLevelResource() {
     return bottomLevelResource;
   }
@@ -140,6 +138,11 @@ public boolean isEmpty() {
     return dependentResourceNodes.isEmpty();
   }
 
+  @Override
+  public int size() {
+    return dependentResourceNodes.size();
+  }
+
   @Override
   public Map<String, DependentResource> getDependentResourcesByName() {
     final var resources = new HashMap<String, DependentResource>(dependentResourceNodes.size());

diff --git a/...java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java b/...java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java
@@ -13,10 +13,10 @@ class DependentResourceNode<R, P extends HasMetadata> {
   private final List<DependentResourceNode> dependsOn = new LinkedList<>();
   private final List<DependentResourceNode> parents = new LinkedList<>();
 
-  private Condition<R, P> reconcilePrecondition;
-  private Condition<R, P> deletePostcondition;
-  private Condition<R, P> readyPostcondition;
-  private Condition<R, P> activationCondition;
+  private ConditionWithType<R, P, ?> reconcilePrecondition;
+  private ConditionWithType<R, P, ?> deletePostcondition;
+  private ConditionWithType<R, P, ?> readyPostcondition;
+  private ConditionWithType<R, P, ?> activationCondition;
   private final DependentResource<R, P> dependentResource;
 
   DependentResourceNode(DependentResource<R, P> dependentResource) {
@@ -26,10 +26,10 @@ class DependentResourceNode<R, P extends HasMetadata> {
   public DependentResourceNode(Condition<R, P> reconcilePrecondition,
       Condition<R, P> deletePostcondition, Condition<R, P> readyPostcondition,
       Condition<R, P> activationCondition, DependentResource<R, P> dependentResource) {
-    this.reconcilePrecondition = reconcilePrecondition;
-    this.deletePostcondition = deletePostcondition;
-    this.readyPostcondition = readyPostcondition;
-    this.activationCondition = activationCondition;
+    setReconcilePrecondition(reconcilePrecondition);
+    setDeletePostcondition(deletePostcondition);
+    setReadyPostcondition(readyPostcondition);
+    setActivationCondition(activationCondition);
     this.dependentResource = dependentResource;
   }
 
@@ -50,36 +50,40 @@ public List<DependentResourceNode> getParents() {
     return parents;
   }
 
-  public Optional<Condition<R, P>> getReconcilePrecondition() {
+  public Optional<ConditionWithType<R, P, ?>> getReconcilePrecondition() {
     return Optional.ofNullable(reconcilePrecondition);
   }
 
-  public Optional<Condition<R, P>> getDeletePostcondition() {
+  public Optional<ConditionWithType<R, P, ?>> getDeletePostcondition() {
     return Optional.ofNullable(deletePostcondition);
   }
 
-  public Optional<Condition<R, P>> getActivationCondition() {
+  public Optional<ConditionWithType<R, P, ?>> getActivationCondition() {
     return Optional.ofNullable(activationCondition);
   }
 
-  void setReconcilePrecondition(Condition<R, P> reconcilePrecondition) {
-    this.reconcilePrecondition = reconcilePrecondition;
+  public Optional<ConditionWithType<R, P, ?>> getReadyPostcondition() {
+    return Optional.ofNullable(readyPostcondition);
   }
 
-  void setDeletePostcondition(Condition<R, P> cleanupCondition) {
-    this.deletePostcondition = cleanupCondition;
+  void setReconcilePrecondition(Condition<R, P> reconcilePrecondition) {
+    this.reconcilePrecondition = reconcilePrecondition == null ? null
+        : new ConditionWithType<>(reconcilePrecondition, Condition.Type.RECONCILE);
   }
 
-  void setActivationCondition(Condition<R, P> activationCondition) {
-    this.activationCondition = activationCondition;
+  void setDeletePostcondition(Condition<R, P> deletePostcondition) {
+    this.deletePostcondition = deletePostcondition == null ? null
+        : new ConditionWithType<>(deletePostcondition, Condition.Type.DELETE);
   }
 
-  public Optional<Condition<R, P>> getReadyPostcondition() {
-    return Optional.ofNullable(readyPostcondition);
+  void setActivationCondition(Condition<R, P> activationCondition) {
+    this.activationCondition = activationCondition == null ? null
+        : new ConditionWithType<>(activationCondition, Condition.Type.ACTIVATION);
   }
 
   void setReadyPostcondition(Condition<R, P> readyPostcondition) {
-    this.readyPostcondition = readyPostcondition;
+    this.readyPostcondition = readyPostcondition == null ? null
+        : new ConditionWithType<>(readyPostcondition, Condition.Type.READY);
   }
 
   public DependentResource<R, P> getDependentResource() {