docs

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

docs/content/en/docs/documentation/reconciler.md

@@ -175,20 +175,23 @@ From v5, by default, the finalizer is added using Server Side Apply. See also `U
 It is typical to want to update the status subresource with the information that is available during the reconciliation.
 This is sometimes referred to as the last observed state. When the primary resource is updated, though, the framework
 does not cache the resource directly, relying instead on the propagation of the update to the underlying informer's
-cache. It can, therefore, happen that, if other events trigger other reconciliations before the informer cache gets
+cache. It can, therefore, happen that, if other events trigger other reconciliations, before the informer cache gets
 updated, your reconciler does not see the latest version of the primary resource. While this might not typically be a
 problem in most cases, as caches eventually become consistent, depending on your reconciliation logic, you might still
-require the latest status version possible, for example if the status subresource is used as a communication mechanism,
-see [Representing Allocated Values](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#representing-allocated-values)
+require the latest status version possible, for example, if the status subresource is used to store allocated values.
+See [Representing Allocated Values](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#representing-allocated-values)
 from the Kubernetes docs for more details.
 
 The framework provides utilities to help with these use cases with 
 [`PrimaryUpdateAndCacheUtils`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java).
-These utility methods come in two flavors:
+These utility methods come in multiple flavors:
 
 #### Using internal cache
 
-In almost all cases for this purpose, you can use internal caches:
+In almost all cases for this purpose, you can use internal caches in combination with update methods that use 
+optimistic locking (end with *WithLock(...)). If the update method fails on optimistic locking, it will retry 
+using a fresh resource from the server as base for modification. Again, this is the default option and will probably
+work for you.
 
 ```java
  @Override
@@ -201,27 +204,32 @@ public UpdateControl<StatusPatchCacheCustomResource> reconcile(
     var freshCopy = createFreshCopy(primary);
     freshCopy.getStatus().setValue(statusWithState());
 
-    var updatedResource = PrimaryUpdateAndCacheUtils.ssaPatchStatusAndCacheResource(resource, freshCopy, context);
+    var updatedResource = PrimaryUpdateAndCacheUtils.ssaPatchStatusAndCacheResourceWithLock(resource, freshCopy, context);
 
     return UpdateControl.noUpdate();
   }
 ```
 
-In the background `PrimaryUpdateAndCacheUtils.ssaPatchAndCacheStatus` puts the result of the update into an internal
-cache and will make sure that the next reconciliation will contain the most recent version of the resource. Note that it
-is not necessarily the version of the resource you got as response from the update, it can be newer since other parties
+After the update `PrimaryUpdateAndCacheUtils.ssaPatchStatusAndCacheResourceWithLock` puts the result of the update into an internal
+cache and will the framework will make sure that the next reconciliation will contain the most recent version of the resource.
+Note that it is not necessarily the version of the resource you got as response from the update, it can be newer since other parties
 can do additional updates meanwhile, but if not explicitly modified, it will contain the up-to-date status.
 
-See related integration test [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/internal).
+See related integration test [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/internalwithlock).
 
 This approach works with the default configuration of the framework and should be good to go in most of the cases.
-Without going further into the details, this won't work if `ConfigurationService.parseResourceVersionsForEventFilteringAndCaching`
-is set to `false` (more precisely there are some edge cases when it won't work). For that case framework provides the following solution:
+
+Without going further into the details, a bit more experimental way we provide overloaded methods without optimistic locking,
+to use those you have to set `ConfigurationService.parseResourceVersionsForEventFilteringAndCaching`
+to `true`. This in practice would mean that request won't fail on optimistic locking, but requires bending a bit 
+the rules regarding Kubernetes API contract. This might be needed only if you have multiple resources frequently
+writing the resource.
 
 #### Fallback approach: using `PrimaryResourceCache` cache
 
-As an alternative, for very rare cases when `ConfigurationService.parseResourceVersionsForEventFilteringAndCaching` 
-needs to be set to `false` you can use an explicit caching approach:
+For the sake of completeness, we also provide a more explicit approach to manage the cache yourself. 
+This approach has the advantage that you don't have to do neither optimistic locking nor 
+setting the `parseResourceVersionsForEventFilteringAndCaching` to `true`:
 
 ```java
 
@@ -277,9 +285,7 @@ their associated primary resource from the underlying informer event source cach
 
 #### Additional remarks
 
-As shown in the integration tests, there is no optimistic locking used when updating the
+As shown in the last two cases, there is no optimistic locking used when updating the
 [resource](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/internal/StatusPatchCacheReconciler.java#L41)
-(in other words `metadata.resourceVersion` is set to `null`). This is desired since you don't want the patch to fail on
-update.
-
-In addition, you can configure the [Fabric8 client retry](https://github.com/fabric8io/kubernetes-client?tab=readme-ov-file#configuring-the-client).
+(in other words `metadata.resourceVersion` is set to `null`). This has nice property the request will be successful. 
+However, it might be desirable to configure retry on [Fabric8 client](https://github.com/fabric8io/kubernetes-client?tab=readme-ov-file#configuring-the-client).

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

@@ -22,12 +22,13 @@
  * various options, where all of them have pros and cons.
  *
  * <ul>
- *   <li>Retryable updates with optimistic locking (*withLock) - you can use this approach out of
- *       the box, it updates the resource using optimistic locking and caches the resource. If the
- *       update fails it reads the primary resource and applies the modifications again and retries
- *       the update. After successful update it caches the resource for next reconciliation. The
- *       disadvantage of this method is that theoretically it could fail the max attempt retry. Note
- *       that optimistic locking is essential to have the caching work in general.
+ *   <li>(Preferred) Retryable updates with optimistic locking (*withLock) - you can use this
+ *       approach out of the box, it updates the resource using optimistic locking and caches the
+ *       resource. If the update fails it reads the primary resource and applies the modifications
+ *       again and retries the update. After successful update it caches the resource for next
+ *       reconciliation. The disadvantage of this method is that theoretically it could fail the max
+ *       attempt retry. Note that optimistic locking is essential to have the caching work in
+ *       general.
  *   <li>Caching without optimistic locking but with parsing the resource version - to use this you
  *       have to set {@link ConfigurationService#parseResourceVersionsForEventFilteringAndCaching()}
  *       to true. The update won't fail on optimistic locking so there is much higher chance to
@@ -48,6 +49,65 @@ private PrimaryUpdateAndCacheUtils() {}
 
   private static final Logger log = LoggerFactory.getLogger(PrimaryUpdateAndCacheUtils.class);
 
+  /**
+   * Updates the status with optimistic locking and caches the result for next reconciliation. For
+   * details see {@link #updateAndCacheResourceWithLock}.
+   */
+  public static <P extends HasMetadata> P updateStatusAndCacheResourceWithLock(
+      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
+    return updateAndCacheResourceWithLock(
+        primary,
+        context,
+        modificationFunction,
+        r -> context.getClient().resource(r).updateStatus());
+  }
+
+  /**
+   * Patches the status using JSON Merge Patch with optimistic locking and caches the result for
+   * next reconciliation. For details see {@link #updateAndCacheResourceWithLock}.
+   */
+  public static <P extends HasMetadata> P patchStatusAndCacheResourceWithLock(
+      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
+    return updateAndCacheResourceWithLock(
+        primary, context, modificationFunction, r -> context.getClient().resource(r).patchStatus());
+  }
+
+  /**
+   * Patches the status using JSON Patch with optimistic locking and caches the result for next
+   * reconciliation. For details see {@link #updateAndCacheResourceWithLock}.
+   */
+  public static <P extends HasMetadata> P editStatusAndCacheResourceWithLock(
+      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
+    return updateAndCacheResourceWithLock(
+        primary,
+        context,
+        UnaryOperator.identity(),
+        r -> context.getClient().resource(r).editStatus(modificationFunction));
+  }
+
+  /**
+   * Patches the status using Server Side Apply with optimistic locking and caches the result for
+   * next reconciliation. For details see {@link #updateAndCacheResourceWithLock}.
+   */
+  public static <P extends HasMetadata> P ssaPatchStatusAndCacheResourceWithLock(
+      P primary, P freshResourceWithStatus, Context<P> context) {
+    return updateAndCacheResourceWithLock(
+        primary,
+        context,
+        r -> freshResourceWithStatus,
+        r ->
+            context
+                .getClient()
+                .resource(r)
+                .subresource("status")
+                .patch(
+                    new PatchContext.Builder()
+                        .withForce(true)
+                        .withFieldManager(context.getControllerConfiguration().fieldManager())
+                        .withPatchType(PatchType.SERVER_SIDE_APPLY)
+                        .build()));
+  }
+
   /**
    * Updates status and makes sure that the up-to-date primary resource will be present during the
    * next reconciliation. Using update (PUT) method.
@@ -64,15 +124,6 @@ public static <P extends HasMetadata> P updateStatusAndCacheResource(
         primary, context, () -> context.getClient().resource(primary).updateStatus());
   }
 
-  public static <P extends HasMetadata> P updateStatusAndCacheResourceWithLock(
-      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
-    return updateAndCacheResourceWithLock(
-        primary,
-        context,
-        modificationFunction,
-        r -> context.getClient().resource(r).updateStatus());
-  }
-
   /**
    * Patches status with and makes sure that the up-to-date primary resource will be present during
    * the next reconciliation. Using JSON Merge patch.
@@ -89,12 +140,6 @@ public static <P extends HasMetadata> P patchStatusAndCacheResource(
         primary, context, () -> context.getClient().resource(primary).patchStatus());
   }
 
-  public static <P extends HasMetadata> P patchStatusAndCacheResourceWithLock(
-      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
-    return updateAndCacheResourceWithLock(
-        primary, context, modificationFunction, r -> context.getClient().resource(r).patchStatus());
-  }
-
   /**
    * Patches status and makes sure that the up-to-date primary resource will be present during the
    * next reconciliation. Using JSON Patch.
@@ -116,15 +161,6 @@ public static <P extends HasMetadata> P editStatusAndCacheResource(
         primary, context, () -> context.getClient().resource(primary).editStatus(operation));
   }
 
-  public static <P extends HasMetadata> P editStatusAndCacheResourceWithLock(
-      P primary, Context<P> context, UnaryOperator<P> modificationFunction) {
-    return updateAndCacheResourceWithLock(
-        primary,
-        context,
-        UnaryOperator.identity(),
-        r -> context.getClient().resource(r).editStatus(modificationFunction));
-  }
-
   /**
    * Patches the resource with supplied method and makes sure that the up-to-date primary resource
    * will be present during the next reconciliation.
@@ -174,25 +210,6 @@ public static <P extends HasMetadata> P ssaPatchStatusAndCacheResource(
                         .build()));
   }
 
-  public static <P extends HasMetadata> P ssaPatchStatusAndCacheResourceWithLock(
-      P primary, P freshResourceWithStatus, Context<P> context) {
-    return updateAndCacheResourceWithLock(
-        primary,
-        context,
-        r -> freshResourceWithStatus,
-        r ->
-            context
-                .getClient()
-                .resource(r)
-                .subresource("status")
-                .patch(
-                    new PatchContext.Builder()
-                        .withForce(true)
-                        .withFieldManager(context.getControllerConfiguration().fieldManager())
-                        .withPatchType(PatchType.SERVER_SIDE_APPLY)
-                        .build()));
-  }
-
   /**
    * Patches the resource status and caches the response in provided {@link PrimaryResourceCache}.
    * Uses Server Side Apply.
@@ -309,6 +326,23 @@ private static <P extends HasMetadata> void checkResourceVersionNotPresentAndPar
     }
   }
 
+  /**
+   * Modifies the primary using modificationFunction, then uses the modified resource for the
+   * request to update with provided update method. But before the update operation sets the
+   * resourceVersion to the modified resource from the primary resource, so there is always
+   * optimistic locking happening. If the request fails on optimistic update, we read the resource
+   * again from the K8S API server and retry the whole process. In short, we make sure we always
+   * update the resource with optimistic locking, after we cache the resource in internal cache.
+   * Without further going into the details, the optimistic locking is needed so we can reliably
+   * handle the caching.
+   *
+   * @param primary original resource to update
+   * @param context of reconciliation
+   * @param modificationFunction modifications to make on primary
+   * @param updateMethod the update method implementation
+   * @return updated resource
+   * @param <P> primary type
+   */
   public static <P extends HasMetadata> P updateAndCacheResourceWithLock(
       P primary,
       Context<P> context,
@@ -318,6 +352,24 @@ public static <P extends HasMetadata> P updateAndCacheResourceWithLock(
         primary, context, modificationFunction, updateMethod, DEFAULT_MAX_RETRY);
   }
 
+  /**
+   * Modifies the primary using modificationFunction, then uses the modified resource for the
+   * request to update with provided update method. But before the update operation sets the
+   * resourceVersion to the modified resource from the primary resource, so there is always
+   * optimistic locking happening. If the request fails on optimistic update, we read the resource
+   * again from the K8S API server and retry the whole process. In short, we make sure we always
+   * update the resource with optimistic locking, after we cache the resource in internal cache.
+   * Without further going into the details, the optimistic locking is needed so we can reliably
+   * handle the caching.
+   *
+   * @param primary original resource to update
+   * @param context of reconciliation
+   * @param modificationFunction modifications to make on primary
+   * @param updateMethod the update method implementation
+   * @param maxRetry - maximum number of retries of conflicts
+   * @return updated resource
+   * @param <P> primary type
+   */
   @SuppressWarnings("unchecked")
   public static <P extends HasMetadata> P updateAndCacheResourceWithLock(
       P primary,

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/internal/StatusPatchCacheReconciler.java

@@ -31,6 +31,10 @@ public UpdateControl<StatusPatchCacheCustomResource> reconcile(
               + resource.getStatus().getValue());
     }
 
+    // test also resource update happening meanwhile reconciliation
+    resource.getSpec().setCounter(resource.getSpec().getCounter() + 1);
+    context.getClient().resource(resource).update();
+
     var freshCopy = createFreshCopy(resource);
 
     freshCopy

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/internalwithlock/StatusPatchCacheWithLockReconciler.java

@@ -33,6 +33,10 @@ public UpdateControl<StatusPatchCacheWithLockCustomResource> reconcile(
               + resource.getStatus().getValue());
     }
 
+    // test also resource update happening meanwhile reconciliation
+    resource.getSpec().setCounter(resource.getSpec().getCounter() + 1);
+    context.getClient().resource(resource).update();
+
     var freshCopy = createFreshCopy(resource);
 
     freshCopy

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/primarycache/StatusPatchPrimaryCacheReconciler.java

@@ -46,6 +46,10 @@ public UpdateControl<StatusPatchPrimaryCacheCustomResource> reconcile(
               + primary.getStatus().getValue());
     }
 
+    // test also resource update happening meanwhile reconciliation
+    primary.getSpec().setCounter(primary.getSpec().getCounter() + 1);
+    context.getClient().resource(primary).update();
+
     var freshCopy = createFreshCopy(primary);
     freshCopy
         .getStatus()

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuscache/primarycache/StatusPatchPrimaryCacheSpec.java

@@ -2,14 +2,13 @@
 
 public class StatusPatchPrimaryCacheSpec {
 
-  private boolean messageInStatus = true;
+  private int counter = 0;
 
-  public boolean isMessageInStatus() {
-    return messageInStatus;
+  public int getCounter() {
+    return counter;
   }
 
-  public StatusPatchPrimaryCacheSpec setMessageInStatus(boolean messageInStatus) {
-    this.messageInStatus = messageInStatus;
-    return this;
+  public void setCounter(int counter) {
+    this.counter = counter;
   }
 }