feat: patch does not do optimistic locking (#1210)

Author: csviri

docs/documentation/architecture-and-internals.md

@@ -47,7 +47,7 @@ A typical workflows looks like following:
 4. Executor calls ReconcilerDispatcher which decides which method to execute of the reconciler. Let's say in this case it
    was `reconcile(...)`
 5. After reconciler execution the Dispatcher calls Kubernetes API server, since the `reconcile` method returned
-   with `UpdateControl.updateStatus(...)` result.
+   with `UpdateControl.patchStatus(...)` result.
 6. Now the dispatcher finishes the execution and calls back `EventProcessor` to finalize the execution.
 7. EventProcessor checks if there is no `reschedule` or `retry` required and if there are no subsequent events received
    for the custom resource

docs/documentation/dependent-resources.md

@@ -174,7 +174,7 @@ public class WebPageManagedDependentsReconciler
         final var name = context.getSecondaryResource(ConfigMap.class).orElseThrow()
                 .getMetadata().getName();
         webPage.setStatus(createStatus(name));
-        return UpdateControl.updateStatus(webPage);
+        return UpdateControl.patchStatus(webPage);
     }
 
 }
@@ -227,7 +227,7 @@ public class WebPageStandaloneDependentsReconciler
 
         // 3.
         if (!isValidHtml(webPage.getHtml())) {
-            return UpdateControl.updateStatus(setInvalidHtmlErrorMessage(webPage)); 
+            return UpdateControl.patchStatus(setInvalidHtmlErrorMessage(webPage)); 
         }
 
         // 4.  
@@ -245,7 +245,7 @@ public class WebPageStandaloneDependentsReconciler
         // 6.
         webPage.setStatus(
                 createStatus(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName()));
-        return UpdateControl.updateStatus(webPage);
+        return UpdateControl.patchStatus(webPage);
     }
 
    private void createDependentResources(KubernetesClient client) {

docs/documentation/faq.md

@@ -22,7 +22,7 @@ Yes, this can be done using [`UpdateControl`](https://github.com/java-operator-s
   public UpdateControl<MyCustomResource> reconcile(
      EventSourceTestCustomResource resource, Context context) {
     ...
-    return UpdateControl.updateStatus(resource).rescheduleAfter(10, TimeUnit.SECONDS);
+    return UpdateControl.patchStatus(resource).rescheduleAfter(10, TimeUnit.SECONDS);
   }
 ```
 

docs/documentation/features.md

@@ -93,7 +93,7 @@ reconciliation with a desired time delay.
   public UpdateControl<MyCustomResource> reconcile(
      EventSourceTestCustomResource resource, Context context) {
     ...
-    return UpdateControl.updateStatus(resource).rescheduleAfter(10, TimeUnit.SECONDS);
+    return UpdateControl.patchStatus(resource).rescheduleAfter(10, TimeUnit.SECONDS);
   }
 ```
 
@@ -299,7 +299,7 @@ resource after it is marked for deletion.
 Retry can be skipped for the cases of unrecoverable errors:
 
 ```java
- ErrorStatusUpdateControl.updateStatus(customResource).withNoRetry();
+ ErrorStatusUpdateControl.patchStatus(customResource).withNoRetry();
 ```
 
 ### Correctness and Automatic Retries

docs/documentation/use-samples.md

@@ -72,7 +72,7 @@ public class WebPageReconciler implements Reconciler<WebPage> {
     public UpdateControl<CustomService> reconcile(CustomService resource,
                                                                Context context) {
         // ... your logic ...
-        return UpdateControl.updateStatus(resource);
+        return UpdateControl.patchStatus(resource);
     }
 }
 ```

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusUpdateControl.java

@@ -7,18 +7,25 @@
 public class ErrorStatusUpdateControl<P extends HasMetadata> {
 
   private final P resource;
+  private final boolean patch;
   private boolean noRetry = false;
 
+
+  public static <T extends HasMetadata> ErrorStatusUpdateControl<T> patchStatus(T resource) {
+    return new ErrorStatusUpdateControl<>(resource, true);
+  }
+
   public static <T extends HasMetadata> ErrorStatusUpdateControl<T> updateStatus(T resource) {
-    return new ErrorStatusUpdateControl<>(resource);
+    return new ErrorStatusUpdateControl<>(resource, false);
   }
 
   public static <T extends HasMetadata> ErrorStatusUpdateControl<T> noStatusUpdate() {
-    return new ErrorStatusUpdateControl<>(null);
+    return new ErrorStatusUpdateControl<>(null, true);
   }
 
-  private ErrorStatusUpdateControl(P resource) {
+  private ErrorStatusUpdateControl(P resource, boolean patch) {
     this.resource = resource;
+    this.patch = patch;
   }
 
   /**
@@ -38,4 +45,8 @@ public Optional<P> getResource() {
   public boolean isNoRetry() {
     return noRetry;
   }
+
+  public boolean isPatch() {
+    return patch;
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java

@@ -33,18 +33,33 @@ public static <T extends HasMetadata> UpdateControl<T> updateResource(T customRe
     return new UpdateControl<>(customResource, false, true, false);
   }
 
-  public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customResource) {
-    return new UpdateControl<>(customResource, true, false, false);
-  }
-
-  public static <T extends HasMetadata> UpdateControl<T> patchResource(T customResource) {
-    return new UpdateControl<>(customResource, false, true, true);
-  }
-
+  /**
+   * Preferred way to update the status. It does not do optimistic locking.
+   *
+   * @param <T> resource type
+   * @param customResource the custom resource with target status
+   * @return UpdateControl instance
+   */
   public static <T extends HasMetadata> UpdateControl<T> patchStatus(T customResource) {
     return new UpdateControl<>(customResource, true, false, true);
   }
 
+  /**
+   * Note that usually "patchStatus" is advised to be used instead of this method.
+   * <p>
+   * Updates the status with optimistic locking regarding current resource version reconciled. Note
+   * that this also ensures that on next reconciliation is the most up-to-date custom resource is
+   * used.
+   * </p>
+   *
+   * @param <T> resource type
+   * @param customResource the custom resource with target status
+   * @return UpdateControl instance
+   */
+  public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customResource) {
+    return new UpdateControl<>(customResource, true, false, false);
+  }
+
   /**
    * As a results of this there will be two call to K8S API. First the custom resource will be
    * updates then the status sub-resource.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java

@@ -200,10 +200,17 @@ void eventProcessingFinished(
       if (eventMarker.deleteEventPresent(resourceID)) {
         cleanupForDeletedEvent(executionScope.getResourceID());
       } else {
-        postExecutionControl.getUpdatedCustomResource().ifPresent(
-            r -> eventSourceManager.getControllerResourceEventSource().handleRecentResourceUpdate(
-                ResourceID.fromResource(r), r,
-                executionScope.getResource()));
+        postExecutionControl
+            .getUpdatedCustomResource()
+            .ifPresent(
+                r -> {
+                  if (!postExecutionControl.updateIsStatusPatch()) {
+                    eventSourceManager
+                        .getControllerResourceEventSource()
+                        .handleRecentResourceUpdate(
+                            ResourceID.fromResource(r), r, executionScope.getResource());
+                  }
+                });
         if (eventMarker.eventPresent(resourceID)) {
           submitReconciliationExecution(resourceID);
         } else {

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java

@@ -8,35 +8,42 @@ final class PostExecutionControl<R extends HasMetadata> {
 
   private final boolean onlyFinalizerHandled;
   private final R updatedCustomResource;
+  private final boolean updateIsStatusPatch;
   private final Exception runtimeException;
 
   private Long reScheduleDelay = null;
 
   private PostExecutionControl(
       boolean onlyFinalizerHandled,
       R updatedCustomResource,
-      Exception runtimeException) {
+      boolean updateIsStatusPatch, Exception runtimeException) {
     this.onlyFinalizerHandled = onlyFinalizerHandled;
     this.updatedCustomResource = updatedCustomResource;
+    this.updateIsStatusPatch = updateIsStatusPatch;
     this.runtimeException = runtimeException;
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> onlyFinalizerAdded() {
-    return new PostExecutionControl<>(true, null, null);
+    return new PostExecutionControl<>(true, null, false, null);
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> defaultDispatch() {
-    return new PostExecutionControl<>(false, null, null);
+    return new PostExecutionControl<>(false, null, false, null);
+  }
+
+  public static <R extends HasMetadata> PostExecutionControl<R> customResourceStatusPatched(
+      R updatedCustomResource) {
+    return new PostExecutionControl<>(false, updatedCustomResource, true, null);
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> customResourceUpdated(
       R updatedCustomResource) {
-    return new PostExecutionControl<>(false, updatedCustomResource, null);
+    return new PostExecutionControl<>(false, updatedCustomResource, false, null);
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> exceptionDuringExecution(
       Exception exception) {
-    return new PostExecutionControl<>(false, null, exception);
+    return new PostExecutionControl<>(false, null, false, exception);
   }
 
   public boolean isOnlyFinalizerHandled() {
@@ -68,6 +75,10 @@ public Optional<Long> getReScheduleDelay() {
     return Optional.ofNullable(reScheduleDelay);
   }
 
+  public boolean updateIsStatusPatch() {
+    return updateIsStatusPatch;
+  }
+
   @Override
   public String toString() {
     return "PostExecutionControl{"

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

@@ -8,6 +8,7 @@
 import io.fabric8.kubernetes.client.CustomResource;
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;
+import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperationsImpl;
 import io.javaoperatorsdk.operator.api.ObservedGenerationAware;
 import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
@@ -134,7 +135,7 @@ private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionSc
     R updatedCustomResource = null;
     if (updateControl.isUpdateResourceAndStatus()) {
       updatedCustomResource =
-          updateCustomResource(updateControl.getResource(), updateControl.isPatch());
+          updateCustomResource(updateControl.getResource());
       updateControl
           .getResource()
           .getMetadata()
@@ -146,7 +147,7 @@ private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionSc
           updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch());
     } else if (updateControl.isUpdateResource()) {
       updatedCustomResource =
-          updateCustomResource(updateControl.getResource(), updateControl.isPatch());
+          updateCustomResource(updateControl.getResource());
       if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
         updatedCustomResource =
             updateStatusGenerationAware(originalResource, updateControl.isPatch());
@@ -181,13 +182,16 @@ public boolean isLastAttempt() {
 
         R updatedResource = null;
         if (errorStatusUpdateControl.getResource().isPresent()) {
-          updatedResource =
-              customResourceFacade
+          updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade
+              .patchStatus(errorStatusUpdateControl.getResource().orElseThrow())
+              : customResourceFacade
                   .updateStatus(errorStatusUpdateControl.getResource().orElseThrow());
         }
         if (errorStatusUpdateControl.isNoRetry()) {
           if (updatedResource != null) {
-            return PostExecutionControl.customResourceUpdated(updatedResource);
+            return errorStatusUpdateControl.isPatch()
+                ? PostExecutionControl.customResourceStatusPatched(updatedResource)
+                : PostExecutionControl.customResourceUpdated(updatedResource);
           } else {
             return PostExecutionControl.defaultDispatch();
           }
@@ -212,6 +216,7 @@ private R updateStatusGenerationAware(R resource, boolean patch) {
     }
   }
 
+  @SuppressWarnings("rawtypes")
   private boolean shouldUpdateObservedGenerationAutomatically(R resource) {
     if (configuration().isGenerationAware() && resource instanceof CustomResource<?, ?>) {
       var customResource = (CustomResource) resource;
@@ -226,6 +231,7 @@ private boolean shouldUpdateObservedGenerationAutomatically(R resource) {
     return false;
   }
 
+  @SuppressWarnings("rawtypes")
   private void updateStatusObservedGenerationIfRequired(R resource) {
     if (configuration().isGenerationAware() && resource instanceof CustomResource<?, ?>) {
       var customResource = (CustomResource) resource;
@@ -242,7 +248,12 @@ private PostExecutionControl<R> createPostExecutionControl(R updatedCustomResour
       UpdateControl<R> updateControl) {
     PostExecutionControl<R> postExecutionControl;
     if (updatedCustomResource != null) {
-      postExecutionControl = PostExecutionControl.customResourceUpdated(updatedCustomResource);
+      if (updateControl.isUpdateStatus() && updateControl.isPatch()) {
+        postExecutionControl =
+            PostExecutionControl.customResourceStatusPatched(updatedCustomResource);
+      } else {
+        postExecutionControl = PostExecutionControl.customResourceUpdated(updatedCustomResource);
+      }
     } else {
       postExecutionControl = PostExecutionControl.defaultDispatch();
     }
@@ -294,14 +305,10 @@ private void updateCustomResourceWithFinalizer(R resource) {
     customResourceFacade.replaceResourceWithLock(resource);
   }
 
-  private R updateCustomResource(R resource, boolean patch) {
+  private R updateCustomResource(R resource) {
     log.debug("Updating resource: {} with version: {}", getUID(resource), getVersion(resource));
     log.trace("Resource before update: {}", resource);
-    if (patch) {
-      return customResourceFacade.patchResource(resource);
-    } else {
-      return customResourceFacade.replaceResourceWithLock(resource);
-    }
+    return customResourceFacade.replaceResourceWithLock(resource);
   }
 
   private R removeFinalizer(R resource) {
@@ -340,27 +347,30 @@ public R replaceResourceWithLock(R resource) {
           .replace(resource);
     }
 
-    public R patchResource(R resource) {
-      return resourceOperation
-          .inNamespace(resource.getMetadata().getNamespace())
-          .withName(getName(resource))
-          .patch(resource);
-    }
-
+    @SuppressWarnings({"rawtypes", "unchecked"})
     public R updateStatus(R resource) {
       log.trace("Updating status for resource: {}", resource);
-      return resourceOperation
+      HasMetadataOperationsImpl hasMetadataOperation = (HasMetadataOperationsImpl) resourceOperation
           .inNamespace(resource.getMetadata().getNamespace())
           .withName(getName(resource))
-          .replaceStatus(resource);
+          .lockResourceVersion(resource.getMetadata().getResourceVersion());
+      return (R) hasMetadataOperation.replaceStatus(resource);
     }
 
     public R patchStatus(R resource) {
       log.trace("Updating status for resource: {}", resource);
-      return resourceOperation
-          .inNamespace(resource.getMetadata().getNamespace())
-          .withName(getName(resource))
-          .patchStatus(resource);
+      String resourceVersion = resource.getMetadata().getResourceVersion();
+      try {
+        // don't do optimistic locking on patch
+        resource.getMetadata().setResourceVersion(null);
+        return resourceOperation
+            .inNamespace(resource.getMetadata().getNamespace())
+            .withName(getName(resource))
+            .patchStatus(resource);
+      } finally {
+        // restore initial resource version
+        resource.getMetadata().setResourceVersion(resourceVersion);
+      }
     }
   }
 }