Skip to content

Commit 0453d72

Browse files
csviricsviri
committed
add finalizer first then execute controller
1 parent 4863a0a commit 0453d72

File tree

6 files changed

+60
-40
lines changed

6 files changed

+60
-40
lines changed

‎operator-framework/src/main/java/com/github/containersolutions/operator/ControllerUtils.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import java.util.Map;
1414

1515

16-
class ControllerUtils {
16+
public class ControllerUtils {
1717

1818
private final static double JAVA_VERSION = Double.parseDouble(System.getProperty("java.specification.version"));
1919

@@ -78,4 +78,11 @@ static String getCrdName(ResourceController controller) {
7878
private static Controller getAnnotation(ResourceController controller) {
7979
return controller.getClass().getAnnotation(Controller.class);
8080
}
81+
82+
public static boolean hasDefaultFinalizer(CustomResource resource, String finalizer) {
83+
if (resource.getMetadata().getFinalizers() != null) {
84+
return resource.getMetadata().getFinalizers().contains(finalizer);
85+
}
86+
return false;
87+
}
8188
}

‎operator-framework/src/main/java/com/github/containersolutions/operator/Operator.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,11 @@ private <R extends CustomResource> void registerController(ResourceController<R>
5656
Class<R> resClass = getCustomResourceClass(controller);
5757
CustomResourceDefinition crd = getCustomResourceDefinitionForController(controller);
5858
KubernetesDeserializer.registerCustomKind(getApiVersion(crd), getKind(crd), resClass);
59-
59+
String finalizer = getDefaultFinalizer(controller);
6060
MixedOperation client = k8sClient.customResources(crd, resClass, CustomResourceList.class, getCustomResourceDoneableClass(controller));
6161
EventDispatcher eventDispatcher = new EventDispatcher(controller,
62-
getDefaultFinalizer(controller), new EventDispatcher.CustomResourceFacade(client));
63-
EventScheduler eventScheduler = new EventScheduler(eventDispatcher, retry, ControllerUtils.getGenerationEventProcessing(controller));
62+
finalizer, new EventDispatcher.CustomResourceFacade(client));
63+
EventScheduler eventScheduler = new EventScheduler(eventDispatcher, retry, ControllerUtils.getGenerationEventProcessing(controller), finalizer);
6464
registerWatches(controller, client, resClass, watchAllNamespaces, targetNamespaces, eventScheduler);
6565
}
6666

‎operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventDispatcher.java

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.github.containersolutions.operator.processing;
22

3+
import com.github.containersolutions.operator.ControllerUtils;
34
import com.github.containersolutions.operator.api.*;
45
import io.fabric8.kubernetes.client.CustomResource;
56
import io.fabric8.kubernetes.client.Watcher;
@@ -39,29 +40,31 @@ public void handleEvent(CustomResourceEvent event) {
3940
}
4041
Context context = new DefaultContext(new RetryInfo(event.getRetryCount(), event.getRetryExecution().isLastExecution()));
4142
/* Its interesting problem if we should call delete if received event after object is marked for deletion
42-
but there is not our finalizer. Since it can happen that there are multiple finalizers, also other events after
43-
we called delete and remove finalizers already. But also it can happen that we did not manage to put
44-
finalizer into the resource before marked for delete. So for now we will call delete every time, since delete
45-
operation should be idempotent too, and this way we cover the corner case. */
46-
if (markedForDeletion(resource) || action == Watcher.Action.DELETED) {
43+
but finalizer is not on the object. Since it can happen that there are multiple finalizers, also other events after
44+
we called delete and remove finalizers already. Delete should be also idempotent, we call it now. */
45+
// todo should we check if it has also finalizer?
46+
if (markedForDeletion(resource)) {
4747
boolean removeFinalizer = controller.deleteResource(resource, context);
48-
if (removeFinalizer && hasDefaultFinalizer(resource)) {
48+
if (removeFinalizer && ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer)) {
4949
removeDefaultFinalizer(resource);
5050
}
5151
} else {
52-
UpdateControl<? extends CustomResource> updateControl = controller.createOrUpdateResource(resource, context);
53-
// note that we do the status sub-resource update first, since if there is an event from Custom resource
54-
// update as next step, the new status is already present.
55-
if (updateControl.isUpdateStatusSubResource()) {
56-
customResourceFacade.updateStatus(updateControl.getCustomResource());
57-
}
58-
if (updateControl.isUpdateCustomResource()) {
59-
updateCustomResource(updateControl.getCustomResource());
60-
} else if (!hasDefaultFinalizer(resource) && !markedForDeletion(resource)) {
61-
// We always add the default finalizer if missing and not marked for deletion.
62-
// Adding it for original resource since the update was not requested. (Although we are not cloning
63-
// passing it as argument)
52+
if (!ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer) && !markedForDeletion(resource)) {
53+
/* We always add the default finalizer if missing and not marked for deletion.
54+
We execute the controller processing only for processing the event sent as a results
55+
of the finalizer add. This will make sure that the resources are not created before
56+
there is a finalizer.
57+
*/
6458
updateCustomResourceWithFinalizer(resource);
59+
} else {
60+
UpdateControl<? extends CustomResource> updateControl = controller.createOrUpdateResource(resource, context);
61+
// note that we do the status sub-resource update first, since if there is an event from Custom resource
62+
// update as next step, the new status is already present.
63+
if (updateControl.isUpdateStatusSubResource()) {
64+
customResourceFacade.updateStatus(updateControl.getCustomResource());
65+
} else if (updateControl.isUpdateCustomResource()) {
66+
updateCustomResource(updateControl.getCustomResource());
67+
}
6568
}
6669
}
6770
}
@@ -83,13 +86,6 @@ private void updateCustomResource(CustomResource updatedResource) {
8386
}
8487

8588

86-
private boolean hasDefaultFinalizer(CustomResource resource) {
87-
if (resource.getMetadata().getFinalizers() != null) {
88-
return resource.getMetadata().getFinalizers().contains(resourceDefaultFinalizer);
89-
}
90-
return false;
91-
}
92-
9389
private void removeDefaultFinalizer(CustomResource resource) {
9490
log.debug("Removing finalizer on {}: {}", resource);
9591
resource.getMetadata().getFinalizers().remove(resourceDefaultFinalizer);
@@ -102,7 +98,7 @@ private void replace(CustomResource resource) {
10298
}
10399

104100
private void addFinalizerIfNotPresent(CustomResource resource) {
105-
if (!hasDefaultFinalizer(resource) && !markedForDeletion(resource)) {
101+
if (!ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer) && !markedForDeletion(resource)) {
106102
log.info("Adding default finalizer to {}", resource.getMetadata());
107103
if (resource.getMetadata().getFinalizers() == null) {
108104
resource.getMetadata().setFinalizers(new ArrayList<>(1));

‎operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventScheduler.java

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.github.containersolutions.operator.processing;
22

33

4+
import com.github.containersolutions.operator.ControllerUtils;
45
import com.github.containersolutions.operator.processing.retry.Retry;
56
import io.fabric8.kubernetes.client.CustomResource;
67
import io.fabric8.kubernetes.client.KubernetesClientException;
@@ -41,13 +42,15 @@ public class EventScheduler implements Watcher<CustomResource> {
4142
private final EventStore eventStore = new EventStore();
4243
private final Retry retry;
4344
private final boolean generationAware;
45+
private final String finalizer;
4446

4547
private ReentrantLock lock = new ReentrantLock();
4648

47-
public EventScheduler(EventDispatcher eventDispatcher, Retry retry, boolean generationAware) {
49+
public EventScheduler(EventDispatcher eventDispatcher, Retry retry, boolean generationAware, String finalizer) {
4850
this.eventDispatcher = eventDispatcher;
4951
this.retry = retry;
5052
this.generationAware = generationAware;
53+
this.finalizer = finalizer;
5154
executor = new ScheduledThreadPoolExecutor(1);
5255
executor.setRemoveOnCancelPolicy(true);
5356
}
@@ -64,15 +67,10 @@ void scheduleEventFromApi(CustomResourceEvent event) {
6467
try {
6568
lock.lock();
6669
log.debug("Scheduling event from Api: {}", event);
67-
if (event.getAction() == Action.DELETED && event.getResource().getMetadata().getDeletionTimestamp() != null) {
70+
if (event.getAction() == Action.DELETED) {
6871
// This removes data from memory for deleted resource (prevent memory leak).
69-
// There is am extreme corner case when there is no finalizer, we ignore this situation now.
7072
eventStore.cleanup(event.resourceUid());
71-
// Note that we always use finalizers, we want to process delete event just in corner case,
72-
// when we are not able to add finalizer (lets say because of optimistic locking error, and the resource was deleted instantly).
73-
// We want to skip in case of finalizer was there since we don't want to execute delete method always at least 2x,
74-
// which would be the result if we don't skip here. (there is no deletion timestamp if resource deleted without finalizer.)
75-
log.debug("Skipping delete event since deletion timestamp is present on resource, so finalizer was in place.");
73+
log.debug("Skipping delete event");
7674
return;
7775
}
7876
if (generationAware) {
@@ -89,11 +87,17 @@ void scheduleEventFromApi(CustomResourceEvent event) {
8987
eventStore.addOrReplaceEventAsNotScheduledAndUpdateLastGeneration(event);
9088
return;
9189
}
92-
if (generationAware && !eventStore.hasLargerGenerationThanLastStored(event)) {
90+
if (generationAware && !eventStore.hasLargerGenerationThanLastStored(event)
91+
&& eventStore.lastProcessingHadFinalizer(event)) {
9392
log.debug("Skipping event, has not larger generation than last stored, actual generation: {}, last stored: {} ",
9493
event.getResource().getMetadata().getGeneration(), eventStore.getLastStoredGeneration(event));
9594
return;
9695
}
96+
if (generationAware && !eventStore.lastProcessingHadFinalizer(event)
97+
&& ControllerUtils.hasDefaultFinalizer(event.getResource(), finalizer)) {
98+
eventStore.markProcessedWitFinalizer(event.resourceUid(), true);
99+
}
100+
97101
if (eventStore.containsEventUnderProcessing(event.resourceUid())) {
98102
log.debug("Scheduling event for later processing since there is an event under processing for same kind." +
99103
" New event: {}", event);

‎operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventStore.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ public class EventStore {
99
private final Map<String, CustomResourceEvent> eventsUnderProcessing = new HashMap<>();
1010
private final Map<String, Long> lastGeneration = new HashMap<>();
1111
private final Map<String, CustomResourceEvent> receivedLastEventForGenerationAwareRetry = new HashMap<>();
12+
private final Map<String, Boolean> lastProcessingWithFinalizer = new HashMap<>();
1213

1314
public boolean containsNotScheduledEvent(String uuid) {
1415
return eventsNotScheduled.containsKey(uuid);
@@ -61,8 +62,18 @@ public CustomResourceEvent getReceivedLastEventForGenerationAwareRetry(String uu
6162
return receivedLastEventForGenerationAwareRetry.get(uuid);
6263
}
6364

65+
public boolean lastProcessingHadFinalizer(CustomResourceEvent event) {
66+
Boolean res = lastProcessingWithFinalizer.get(event.resourceUid());
67+
return res == null ? false : res;
68+
}
69+
70+
public void markProcessedWitFinalizer(String uuid, boolean hadFinalizer) {
71+
lastProcessingWithFinalizer.put(uuid, hadFinalizer);
72+
}
73+
6474
public void cleanup(String uuid) {
6575
lastGeneration.remove(uuid);
6676
receivedLastEventForGenerationAwareRetry.remove(uuid);
77+
lastProcessingWithFinalizer.remove(uuid);
6778
}
6879
}

‎operator-framework/src/test/java/com/github/containersolutions/operator/EventSchedulerTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.github.containersolutions.operator;
22

3+
import com.github.containersolutions.operator.api.Controller;
34
import com.github.containersolutions.operator.processing.CustomResourceEvent;
45
import com.github.containersolutions.operator.processing.EventDispatcher;
56
import com.github.containersolutions.operator.processing.EventScheduler;
@@ -32,6 +33,7 @@ class EventSchedulerTest {
3233

3334
public static final int INVOCATION_DURATION = 80;
3435
public static final int MAX_RETRY_ATTEMPTS = 3;
36+
public static final String finalizer = Controller.DEFAULT_FINALIZER;
3537
@SuppressWarnings("unchecked")
3638
private EventDispatcher eventDispatcher = mock(EventDispatcher.class);
3739

@@ -231,7 +233,7 @@ private void generationUnAwareScheduler() {
231233

232234
private EventScheduler initScheduler(boolean generationAware) {
233235
return new EventScheduler(eventDispatcher,
234-
new GenericRetry().setMaxAttempts(MAX_RETRY_ATTEMPTS).withLinearRetry(), generationAware);
236+
new GenericRetry().setMaxAttempts(MAX_RETRY_ATTEMPTS).withLinearRetry(), generationAware, finalizer);
235237
}
236238

237239
private Object exceptionInExecution(InvocationOnMock invocation) {

0 commit comments

Comments
 (0)