Skip to content

Commit aeba560

Browse files
csviricsviri
committed
cleanup workflow process
1 parent 6abb88a commit aeba560

File tree

8 files changed

+135
-56
lines changed

8 files changed

+135
-56
lines changed

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

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import io.fabric8.kubernetes.api.model.HasMetadata;
1313
import io.javaoperatorsdk.operator.api.reconciler.Context;
1414
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
15+
import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected;
1516

1617
public class WorkflowCleanupExecutor<P extends HasMetadata> {
1718

@@ -22,7 +23,7 @@ public class WorkflowCleanupExecutor<P extends HasMetadata> {
2223
private final Map<DependentResourceNode<?, ?>, Exception> exceptionsDuringExecution =
2324
new HashMap<>();
2425
private final Set<DependentResourceNode<?, ?>> alreadyVisited = new HashSet<>();
25-
private final Set<DependentResourceNode<?, ?>> notReady = new HashSet<>();
26+
private final Set<DependentResourceNode<?, ?>> cleanupConditionNotMet = new HashSet<>();
2627

2728
private final Workflow<P> workflow;
2829
private final P primary;
@@ -35,7 +36,6 @@ public WorkflowCleanupExecutor(Workflow<P> workflow, P primary, Context<P> conte
3536
}
3637

3738
// todo cleanup condition
38-
// todo error handling
3939

4040
public synchronized WorkflowCleanupResult cleanup() {
4141
for (DependentResourceNode<?, P> dependentResourceNode : workflow
@@ -67,8 +67,8 @@ private synchronized void handleCleanup(DependentResourceNode<?, P> dependentRes
6767

6868
if (alreadyVisited(dependentResourceNode)
6969
|| isCleaningNow(dependentResourceNode)
70-
|| !allParentsCleaned(dependentResourceNode)
71-
|| hasErroredParent(dependentResourceNode)) {
70+
|| !allDependentsCleaned(dependentResourceNode)
71+
|| hasErroredDependent(dependentResourceNode)) {
7272
log.debug("Skipping submit of: {}, ", dependentResourceNode);
7373
return;
7474
}
@@ -92,12 +92,22 @@ private NodeExecutor(DependentResourceNode<?, P> dependentResourceNode) {
9292
@SuppressWarnings("unchecked")
9393
public void run() {
9494
try {
95-
if (dependentResourceNode.getDependentResource() instanceof Deleter) {
96-
// todo check if not garbage collected
95+
var dependentResource = dependentResourceNode.getDependentResource();
96+
97+
var cleanupCondition = dependentResourceNode.getCleanupCondition();
98+
99+
if (dependentResource instanceof Deleter
100+
&& !(dependentResource instanceof GarbageCollected)) {
97101
((Deleter<P>) dependentResourceNode.getDependentResource()).delete(primary, context);
98102
}
99103
alreadyVisited.add(dependentResourceNode);
100-
handleDependentCleaned(dependentResourceNode);
104+
boolean cleanupConditionMet =
105+
cleanupCondition.map(c -> c.isMet(dependentResource, primary, context)).orElse(true);
106+
if (cleanupConditionMet) {
107+
handleDependentCleaned(dependentResourceNode);
108+
} else {
109+
cleanupConditionNotMet.add(dependentResourceNode);
110+
}
101111
} catch (RuntimeException e) {
102112
handleExceptionInExecutor(dependentResourceNode, e);
103113
} finally {
@@ -142,15 +152,15 @@ private boolean alreadyVisited(
142152
return alreadyVisited.contains(dependentResourceNode);
143153
}
144154

145-
private boolean allParentsCleaned(
155+
private boolean allDependentsCleaned(
146156
DependentResourceNode<?, P> dependentResourceNode) {
147157
var parents = workflow.getDependents(dependentResourceNode);
148158
return parents.isEmpty()
149159
|| parents.stream()
150-
.allMatch(d -> alreadyVisited(d) && !notReady.contains(d));
160+
.allMatch(d -> alreadyVisited(d) && !cleanupConditionNotMet.contains(d));
151161
}
152162

153-
private boolean hasErroredParent(
163+
private boolean hasErroredDependent(
154164
DependentResourceNode<?, P> dependentResourceNode) {
155165
var parents = workflow.getDependents(dependentResourceNode);
156166
return !parents.isEmpty()

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

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import io.javaoperatorsdk.operator.api.reconciler.Context;
1212
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
1313
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
14-
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReadyCondition;
1514
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReconcileCondition;
1615

1716
public class WorkflowReconcileExecutor<P extends HasMetadata> {
@@ -106,11 +105,6 @@ private synchronized void handleNodeExecutionFinish(DependentResourceNode depend
106105
}
107106
}
108107

109-
// needs to be synced
110-
private synchronized void updateStatusForNotReady(ReadyCondition<?, P> readyCondition) {
111-
readyCondition.addNotReadyStatusInfo(primary);
112-
}
113-
114108
// needs to be in one step
115109
private synchronized void setAlreadyReconciledButNotReady(
116110
DependentResourceNode<?, P> dependentResourceNode) {
@@ -148,12 +142,11 @@ public void run() {
148142
((Deleter<P>) dependentResource).delete(primary, context);
149143
}
150144
} else {
151-
dependentResource.reconcile(primary, context);
145+
var reconcileResult = dependentResource.reconcile(primary, context);
152146
if (dependentResourceNode.getReadyCondition().isPresent()
153147
&& !dependentResourceNode.getReadyCondition().get()
154148
.isMet(dependentResource, primary, context)) {
155149
ready = false;
156-
updateStatusForNotReady(dependentResourceNode.getReadyCondition().get());
157150
}
158151
}
159152

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/DependentBuilder.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.fabric8.kubernetes.api.model.HasMetadata;
44
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
55
import io.javaoperatorsdk.operator.processing.dependent.workflow.DependentResourceNode;
6+
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.CleanupCondition;
67
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReadyCondition;
78
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReconcileCondition;
89

@@ -34,6 +35,11 @@ public DependentBuilder<P> withReadyCondition(ReadyCondition readyCondition) {
3435
return this;
3536
}
3637

38+
public DependentBuilder<P> withCleanupCondition(CleanupCondition readyCondition) {
39+
node.setCleanupCondition(readyCondition);
40+
return this;
41+
}
42+
3743
public WorkflowBuilder<P> build() {
3844
return workflowBuilder;
3945
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/condition/CleanupCondition.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
package io.javaoperatorsdk.operator.processing.dependent.workflow.condition;
22

3-
public class CleanupCondition {
3+
import io.fabric8.kubernetes.api.model.HasMetadata;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
6+
7+
public interface CleanupCondition<R, P extends HasMetadata> {
8+
9+
boolean isMet(DependentResource<R, P> dependentResource, P primary, Context<P> context);
410
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/condition/ReadyCondition.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,4 @@ public interface ReadyCondition<R, P extends HasMetadata> {
88

99
boolean isMet(DependentResource<R, P> dependentResource, P primary, Context<P> context);
1010

11-
/**
12-
* If condition not met, the primary resource status might be updated by overriding this method.
13-
* In case there are multiple conditions in a workflow it is updated multiple times.
14-
*
15-
* @param primary
16-
*/
17-
default void addNotReadyStatusInfo(P primary) {}
1811
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.javaoperatorsdk.operator.api.reconciler.Context;
99
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
1010
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
11+
import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected;
1112
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
1213
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
1314

@@ -65,6 +66,28 @@ public void delete(TestCustomResource primary, Context<TestCustomResource> conte
6566
}
6667
}
6768

69+
public class GarbageCollectedDeleter extends TestDeleterDependent
70+
implements GarbageCollected<TestCustomResource> {
71+
72+
public GarbageCollectedDeleter(String name) {
73+
super(name);
74+
}
75+
}
76+
77+
public class TestErrorDeleterDependent extends TestDependent
78+
implements Deleter<TestCustomResource> {
79+
80+
public TestErrorDeleterDependent(String name) {
81+
super(name);
82+
}
83+
84+
@Override
85+
public void delete(TestCustomResource primary, Context<TestCustomResource> context) {
86+
executionHistory.add(new ReconcileRecord(this, true));
87+
throw new IllegalStateException("Test exception");
88+
}
89+
}
90+
6891
public class TestErrorDependent implements DependentResource<String, TestCustomResource> {
6992
private String name;
7093

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

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,24 @@
33
import org.junit.jupiter.api.Test;
44

55
import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder;
6+
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.CleanupCondition;
67
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
78

89
import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat;
9-
import static org.junit.jupiter.api.Assertions.*;
1010

1111
class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest {
1212

1313
protected TestDeleterDependent dd1 = new TestDeleterDependent("DR_DELETER_1");
1414
protected TestDeleterDependent dd2 = new TestDeleterDependent("DR_DELETER_2");
1515
protected TestDeleterDependent dd3 = new TestDeleterDependent("DR_DELETER_3");
1616

17+
protected TestErrorDeleterDependent errorDD = new TestErrorDeleterDependent("ERROR_DELETER");
18+
19+
private final CleanupCondition noMetCleanupCondition =
20+
(dependentResource, primary, context) -> false;
21+
private final CleanupCondition metCleanupCondition =
22+
(dependentResource, primary, context) -> true;
23+
1724
@Test
1825
void cleanUpDiamondWorkflow() {
1926
var workflow = new WorkflowBuilder<TestCustomResource>()
@@ -28,6 +35,75 @@ void cleanUpDiamondWorkflow() {
2835
assertThat(executionHistory).reconciledInOrder(dd3, dd2, dd1).notReconciled(dr1);
2936
}
3037

38+
@Test
39+
void dontDeleteIfDependentErrored() {
40+
var workflow = new WorkflowBuilder<TestCustomResource>()
41+
.addDependent(dd1).build()
42+
.addDependent(dd2).dependsOn(dd1).build()
43+
.addDependent(dd3).dependsOn(dd2).build()
44+
.addDependent(errorDD).dependsOn(dd2).build()
45+
.build();
46+
47+
workflow.cleanup(new TestCustomResource(), null);
48+
49+
assertThat(executionHistory).deleted(dd3, errorDD).notReconciled(dd1, dd2);
50+
}
51+
52+
53+
@Test
54+
void cleanupConditionTrivialCase() {
55+
var workflow = new WorkflowBuilder<TestCustomResource>()
56+
.addDependent(dd1).build()
57+
.addDependent(dd2).dependsOn(dd1).withCleanupCondition(noMetCleanupCondition).build()
58+
.build();
59+
60+
workflow.cleanup(new TestCustomResource(), null);
61+
62+
assertThat(executionHistory).deleted(dd2).notReconciled(dd1);
63+
}
64+
65+
@Test
66+
void cleanupConditionMet() {
67+
var workflow = new WorkflowBuilder<TestCustomResource>()
68+
.addDependent(dd1).build()
69+
.addDependent(dd2).dependsOn(dd1).withCleanupCondition(metCleanupCondition).build()
70+
.build();
71+
72+
workflow.cleanup(new TestCustomResource(), null);
73+
74+
assertThat(executionHistory).deleted(dd2, dd1);
75+
}
76+
77+
@Test
78+
void cleanupConditionDiamondWorkflow() {
79+
TestDeleterDependent dd4 = new TestDeleterDependent("DR_DELETER_4");
80+
81+
var workflow = new WorkflowBuilder<TestCustomResource>()
82+
.addDependent(dd1).build()
83+
.addDependent(dd2).dependsOn(dd1).build()
84+
.addDependent(dd3).dependsOn(dd1).withCleanupCondition(noMetCleanupCondition).build()
85+
.addDependent(dd4).dependsOn(dd2, dd3).build()
86+
.build();
87+
88+
workflow.cleanup(new TestCustomResource(), null);
89+
90+
assertThat(executionHistory)
91+
.reconciledInOrder(dd4, dd2)
92+
.reconciledInOrder(dd4, dd3)
93+
.notReconciled(dr1);
94+
}
95+
96+
@Test
97+
void dontDeleteIfGarbageCollected() {
98+
GarbageCollectedDeleter gcDel = new GarbageCollectedDeleter("GC_DELETER");
99+
var workflow = new WorkflowBuilder<TestCustomResource>()
100+
.addDependent(gcDel).build()
101+
.build();
31102

103+
workflow.cleanup(new TestCustomResource(), null);
104+
105+
assertThat(executionHistory)
106+
.notReconciled(gcDel);
107+
}
32108

33109
}

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

Lines changed: 1 addition & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@
44
import org.junit.jupiter.api.Test;
55

66
import io.javaoperatorsdk.operator.AggregatedOperatorException;
7-
import io.javaoperatorsdk.operator.api.reconciler.Context;
8-
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
97
import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder;
108
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReadyCondition;
119
import io.javaoperatorsdk.operator.processing.dependent.workflow.condition.ReconcileCondition;
@@ -28,18 +26,7 @@ class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest {
2826
(dependentResource, primary, context) -> false;
2927

3028
private ReadyCondition<String, TestCustomResource> notMetReadyConditionWithStatusUpdate =
31-
new ReadyCondition<>() {
32-
@Override
33-
public boolean isMet(DependentResource<String, TestCustomResource> dependentResource,
34-
TestCustomResource primary, Context<TestCustomResource> context) {
35-
return false;
36-
}
37-
38-
@Override
39-
public void addNotReadyStatusInfo(TestCustomResource primary) {
40-
primary.getStatus().setConfigMapStatus(NOT_READY_YET);
41-
}
42-
};
29+
(dependentResource, primary, context) -> false;
4330

4431
@Test
4532
void reconcileTopLevelResources() {
@@ -274,21 +261,6 @@ void readyConditionNotMetTrivialCase() {
274261
assertThat(executionHistory).reconciled(dr1).notReconciled(dr2);
275262
}
276263

277-
@Test
278-
void readyConditionNotMetStatusUpdates() {
279-
var workflow = new WorkflowBuilder<TestCustomResource>()
280-
.addDependent(dr1).withReadyCondition(notMetReadyConditionWithStatusUpdate).build()
281-
.addDependent(dr2).dependsOn(dr1).build()
282-
.build();
283-
284-
var cr = new TestCustomResource();
285-
var res = workflow.reconcile(cr, null);
286-
287-
Assertions.assertThat(res.getErroredDependents()).isEmpty();
288-
assertThat(executionHistory).reconciled(dr1).notReconciled(dr2);
289-
Assertions.assertThat(cr.getStatus().getConfigMapStatus()).isEqualTo(NOT_READY_YET);
290-
}
291-
292264
@Test
293265
void readyConditionNotMetInOneParent() {
294266
TestDependent dr3 = new TestDependent("DR_3");

0 commit comments

Comments
 (0)