From c6e7e34c76b4099bef6124db35887bf5ab63978f Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 23 Sep 2022 14:17:16 +0200 Subject: [PATCH 1/3] fix: concurrency issue with workflow cleanup executor --- .../processing/dependent/workflow/WorkflowCleanupExecutor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index f32192e839..7468e05482 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -104,16 +104,17 @@ public void run() { ((Deleter

) dependentResourceNode.getDependentResource()).delete(primary, context); deleteCalled.add(dependentResourceNode); } - alreadyVisited.add(dependentResourceNode); boolean deletePostConditionMet = deletePostCondition.map(c -> c.isMet(primary, dependentResourceNode.getDependentResource().getSecondaryResource(primary) .orElse(null), context)).orElse(true); if (deletePostConditionMet) { + alreadyVisited.add(dependentResourceNode); handleDependentCleaned(dependentResourceNode); } else { postDeleteConditionNotMet.add(dependentResourceNode); + alreadyVisited.add(dependentResourceNode); } } catch (RuntimeException e) { handleExceptionInExecutor(dependentResourceNode, e); From 7b712532dbfcbdde455f7d9f1f0f20f4ed5c219f Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 26 Sep 2022 14:51:08 +0200 Subject: [PATCH 2/3] comments on impl --- .../dependent/workflow/WorkflowCleanupExecutor.java | 4 ++++ .../dependent/workflow/WorkflowReconcileExecutor.java | 3 +++ 2 files changed, 7 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 7468e05482..41a6950f35 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -109,10 +109,14 @@ public void run() { dependentResourceNode.getDependentResource().getSecondaryResource(primary) .orElse(null), context)).orElse(true); + if (deletePostConditionMet) { alreadyVisited.add(dependentResourceNode); handleDependentCleaned(dependentResourceNode); } else { + // updating alreadyVisited needs to be the last operation otherwise could lead to a race + // condition + // in handleCleanup condition checks postDeleteConditionNotMet.add(dependentResourceNode); alreadyVisited.add(dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index cf209c3cea..727e8d2366 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -217,6 +217,9 @@ public void run() { alreadyVisited.add(dependentResourceNode); handleDependentDeleted(dependentResourceNode); } else { + // updating alreadyVisited needs to be the last operation otherwise could lead to a race + // condition + // in handleDelete condition checks deletePostConditionNotMet.add(dependentResourceNode); alreadyVisited.add(dependentResourceNode); } From 6f8ab6b688f7d2d6a1515046d94d4367e97b6377 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 26 Sep 2022 14:53:16 +0200 Subject: [PATCH 3/3] format --- .../processing/dependent/workflow/WorkflowCleanupExecutor.java | 3 +-- .../dependent/workflow/WorkflowReconcileExecutor.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 41a6950f35..98c6789869 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -115,8 +115,7 @@ public void run() { handleDependentCleaned(dependentResourceNode); } else { // updating alreadyVisited needs to be the last operation otherwise could lead to a race - // condition - // in handleCleanup condition checks + // condition in handleCleanup condition checks postDeleteConditionNotMet.add(dependentResourceNode); alreadyVisited.add(dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 727e8d2366..14028cb980 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -218,8 +218,7 @@ public void run() { handleDependentDeleted(dependentResourceNode); } else { // updating alreadyVisited needs to be the last operation otherwise could lead to a race - // condition - // in handleDelete condition checks + // condition in handleDelete condition checks deletePostConditionNotMet.add(dependentResourceNode); alreadyVisited.add(dependentResourceNode); }