From dc6c1ef86c47d702bd17c0fdd6e02ee1eb87a1ff Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Apr 2023 13:22:26 +0200 Subject: [PATCH 1/4] fix: delete dependent if garbage collected but precondition not holds --- .../dependent/workflow/WorkflowBuilder.java | 1 + .../dependent/workflow/WorkflowReconcileExecutor.java | 4 +++- .../workflow/AbstractWorkflowExecutorTest.java | 1 + .../workflow/WorkflowCleanupExecutorTest.java | 5 ++--- .../workflow/WorkflowReconcileExecutorTest.java | 11 +++++++++++ 5 files changed, 18 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index c9107a4266..88d9b37750 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -22,6 +22,7 @@ public class WorkflowBuilder

{ public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DependentResourceNode<>(dependentResource); + // todo is this wrong? isCleaner = dependentResource.isDeletable(); final var name = currentNode.getName(); dependentResourceNodes.put(name, currentNode); 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 773bb0332f..f2293b7dab 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 @@ -149,7 +149,9 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (dependentResource.isDeletable()) { + // GarbageCollected is irrelevant here, it is only called when a precondition not hold in that + // case a deleter should be deleted even if it is otherwise garbage collected + if (dependentResource instanceof Deleter) { ((Deleter

) dependentResource).delete(primary, context); } boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java index 467820940d..e8b26184c4 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java @@ -22,6 +22,7 @@ public class AbstractWorkflowExecutorTest { protected TestDeleterDependent drDeleter = new TestDeleterDependent("DR_DELETER"); protected TestErrorDependent drError = new TestErrorDependent("ERROR_1"); protected TestErrorDeleterDependent errorDD = new TestErrorDeleterDependent("ERROR_DELETER"); + protected GarbageCollectedDeleter gcDeleter = new GarbageCollectedDeleter("GC_DELETER"); @SuppressWarnings("rawtypes") protected final Condition noMetDeletePostCondition = (primary, secondary, context) -> false; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java index 9aa2fb07e9..a8e46fc258 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java @@ -117,15 +117,14 @@ void cleanupConditionDiamondWorkflow() { @Test void dontDeleteIfGarbageCollected() { - GarbageCollectedDeleter gcDel = new GarbageCollectedDeleter("GC_DELETER"); var workflow = new WorkflowBuilder() - .addDependentResource(gcDel) + .addDependentResource(gcDeleter) .build(); var res = workflow.cleanup(new TestCustomResource(), null); assertThat(executionHistory) - .notReconciled(gcDel); + .notReconciled(gcDeleter); Assertions.assertThat(res.getDeleteCalledOnDependents()).isEmpty(); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java index a445dc783f..a3cbb4bfb0 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java @@ -453,4 +453,15 @@ void diamondShareWithReadyCondition() { Assertions.assertThat(res.getNotReadyDependents()).containsExactlyInAnyOrder(dr2); } + @Test + void garbageCollectedResourceIsDeletedIfReconcilePreconditionNotHolds() { + var workflow = new WorkflowBuilder() + .addDependentResource(gcDeleter).withReconcilePrecondition(not_met_reconcile_condition) + .build(); + + var res = workflow.reconcile(new TestCustomResource(), mockContext); + + assertThat(executionHistory).deleted(gcDeleter); + } + } From 8632208d2dbef03e62f113c22f4dd4d7c880a435 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Apr 2023 13:27:52 +0200 Subject: [PATCH 2/4] wip --- .../operator/processing/dependent/workflow/WorkflowBuilder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index 88d9b37750..c9107a4266 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -22,7 +22,6 @@ public class WorkflowBuilder

{ public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DependentResourceNode<>(dependentResource); - // todo is this wrong? isCleaner = dependentResource.isDeletable(); final var name = currentNode.getName(); dependentResourceNodes.put(name, currentNode); From 0a3d4f3aa6c09317f538db4294e5ce80770d1c37 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Apr 2023 13:30:47 +0200 Subject: [PATCH 3/4] additional unit test --- .../workflow/WorkflowReconcileExecutorTest.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java index a3cbb4bfb0..f2a2fdacd8 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java @@ -461,6 +461,20 @@ void garbageCollectedResourceIsDeletedIfReconcilePreconditionNotHolds() { var res = workflow.reconcile(new TestCustomResource(), mockContext); + Assertions.assertThat(res.getErroredDependents()).isEmpty(); + assertThat(executionHistory).deleted(gcDeleter); + } + + @Test + void garbageCollectedDeepResourceIsDeletedIfReconcilePreconditionNotHolds() { + var workflow = new WorkflowBuilder() + .addDependentResource(dr1).withReconcilePrecondition(not_met_reconcile_condition) + .addDependentResource(gcDeleter).dependsOn(dr1) + .build(); + + var res = workflow.reconcile(new TestCustomResource(), mockContext); + + Assertions.assertThat(res.getErroredDependents()).isEmpty(); assertThat(executionHistory).deleted(gcDeleter); } From 77b4f5bf68eb22d5b896f3404ae2ff5083605aed Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 26 Apr 2023 10:52:43 +0200 Subject: [PATCH 4/4] fix: wording --- .../dependent/workflow/WorkflowReconcileExecutor.java | 5 +++-- .../dependent/workflow/WorkflowReconcileExecutorTest.java | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) 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 f2293b7dab..e437d58a27 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 @@ -149,8 +149,9 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - // GarbageCollected is irrelevant here, it is only called when a precondition not hold in that - // case a deleter should be deleted even if it is otherwise garbage collected + // 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 if (dependentResource instanceof Deleter) { ((Deleter

) dependentResource).delete(primary, context); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java index f2a2fdacd8..96de59ce5b 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java @@ -454,7 +454,7 @@ void diamondShareWithReadyCondition() { } @Test - void garbageCollectedResourceIsDeletedIfReconcilePreconditionNotHolds() { + void garbageCollectedResourceIsDeletedIfReconcilePreconditionDoesNotHold() { var workflow = new WorkflowBuilder() .addDependentResource(gcDeleter).withReconcilePrecondition(not_met_reconcile_condition) .build(); @@ -466,7 +466,7 @@ void garbageCollectedResourceIsDeletedIfReconcilePreconditionNotHolds() { } @Test - void garbageCollectedDeepResourceIsDeletedIfReconcilePreconditionNotHolds() { + void garbageCollectedDeepResourceIsDeletedIfReconcilePreconditionDoesNotHold() { var workflow = new WorkflowBuilder() .addDependentResource(dr1).withReconcilePrecondition(not_met_reconcile_condition) .addDependentResource(gcDeleter).dependsOn(dr1)