feat: remove automatic observed generation handling (#2382)

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>

Author: csviri

README.md

@@ -40,7 +40,6 @@ It makes it easy to implement best practices and patterns for an Operator. Featu
 * Handling dependent resources, related events, and caching.
 * Automatic Retries
 * Smart event scheduling
-* Handling Observed Generations automatically
 * Easy to use Error Handling
 * ... and everything that a batteries included framework needs 
 

bootstrapper-maven-plugin/src/main/resources/templates/Status.java

@@ -1,7 +1,5 @@
 package {{groupId}};
 
-import io.javaoperatorsdk.operator.api.ObservedGenerationAwareStatus;
-
-public class {{artifactClassId}}Status extends ObservedGenerationAwareStatus {
+public class {{artifactClassId}}Status {
 
 }

caffeine-bounded-cache-support/src/test/java/io/javaoperatorsdk/operator/processing/event/source/cache/sample/namespacescope/BoundedCacheTestStatus.java

@@ -1,6 +1,4 @@
 package io.javaoperatorsdk.operator.processing.event.source.cache.sample.namespacescope;
 
-import io.javaoperatorsdk.operator.api.ObservedGenerationAwareStatus;
-
-public class BoundedCacheTestStatus extends ObservedGenerationAwareStatus {
+public class BoundedCacheTestStatus {
 }

docs/documentation/features.md

@@ -186,57 +186,6 @@ annotation. If you do not specify a finalizer name, one will be automatically ge
 
 From v5 by default finalizer is added using Served Side Apply. See also UpdateControl in docs.
 
-## Automatic Observed Generation Handling
-
-Having an `.observedGeneration` value on your resources' status is a best practice to
-indicate the last generation of the resource which was successfully reconciled by the controller.
-This helps users / administrators diagnose potential issues.
-
-In order to have this feature working:
-
-- the **status class** (not the resource itself) must implement the
-  [`ObservedGenerationAware`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ObservedGenerationAware.java)
-  interface. See also
-  the [`ObservedGenerationAwareStatus`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ObservedGenerationAwareStatus.java)
-  convenience implementation that you can extend in your own status class implementations.
-- The other condition is that the `CustomResource.getStatus()` method should not return `null`.
-  So the status should be instantiated when the object is returned using the `UpdateControl`.
-
-If these conditions are fulfilled and generation awareness is activated, the observed generation
-is automatically set by the framework after the `reconcile` method is called. 
-
-When using SSA based patches, the observed generation is only updated when `UpdateControl.patchStatus` or
-`UpdateControl.patchResourceAndStatus` is returned. In case the of non-SSA based patches
-the observed generation is also updated even when `UpdateControl.noUpdate()` is returned from the
-reconciler. 
-See this feature at work in the [WebPage example](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageStatus.java#L5).
-See turning off an on the SSA based patching at [`ConfigurationServcice.useSSAToPatchPrimaryResource()`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L385-L385).
-
-```java
-public class WebPageStatus extends ObservedGenerationAwareStatus {
- // omitted code
-}
-```
-
-Initializing status automatically on custom resource could be done by overriding the `initStatus` method
-of `CustomResource`. However, this is NOT advised, since breaks the status patching if you use:
-`UpdateControl.patchStatus`. See
-also [javadocs](https://github.com/java-operator-sdk/java-operator-sdk/blob/3994f5ffc1fb000af81aa198abf72a5f75fd3e97/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java#L41-L42)
-.
-
-```java 
-@Group("sample.javaoperatorsdk")
-@Version("v1")
-public class WebPage extends CustomResource<WebPageSpec, WebPageStatus>
-    implements Namespaced {
-
-  @Override
-  protected WebPageStatus initStatus() {
-    return new WebPageStatus();
-  }
-}
-```
-
 ## Generation Awareness and Event Filtering
 
 A best practice when an operator starts up is to reconcile all the associated resources because
@@ -254,9 +203,7 @@ To turn off this feature, set `generationAwareEventProcessing` to `false` for th
 ## Support for Well Known (non-custom) Kubernetes Resources
 
 A Controller can be registered for a non-custom resource, so well known Kubernetes resources like (
-`Ingress`, `Deployment`,...). Note that automatic observed generation handling is not supported
-for these resources, though, in this case, the handling of the observed generation is probably
-handled by the primary controller.
+`Ingress`, `Deployment`,...).
 
 See
 the [integration test](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/deployment/DeploymentReconciler.java)

docs/documentation/v5-0-migration.md

@@ -44,11 +44,6 @@ permalink: /docs/v5-0-migration
    where it is demonstrated. Also, the related part of
    a [workaround](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L110-L116).
 
-   Related automatic observed generation handling changes:
-   Automated Observed Generation (see features in docs), is automatically handled for non-SSA, even if
-   the status sub-resource is not instructed to be updated. This is not true for SSA, observed generation is updated
-   only when patch status is instructed by `UpdateControl`.
-
 6. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
    via the accordingly renamed `managedWorkflowAndDependentResourceContext` method.
 7. `ResourceDiscriminator` was removed. In most of the cases you can just delete the discriminator, everything should
@@ -57,3 +52,9 @@ permalink: /docs/v5-0-migration
 8. `ConfigurationService.getTerminationTimeoutSeconds` and associated overriding mechanism have been removed,
    use `Operator.stop(Duration)` instead.
 9. `Operator.installShutdownHook()` has been removed, use `Operator.installShutdownHook(Duration)` instead
+10. Automated observed generation handling feature was removed (`ObservedGenerationAware` interface
+    and `ObservedGenerationAwareStatus` class were deleted). Manually handling observed generation is fairly easy to do
+    in your reconciler, however, it cannot be done automatically when using SSA. We therefore removed the feature since
+    it would have been confusing to have a different behavior for SSA and non-SSA cases. For an example of how to do
+    observed generation handling manually in your reconciler, see
+    [this sample](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/manualobservedgeneration/ManualObservedGenerationReconciler.java).

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

@@ -395,7 +395,7 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
   /**
    * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can
    * either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for
-   * adding finalizers, managing observed generation, patching resources and status.
+   * adding finalizers, patching resources and status.
    *
    * @return {@code true} if Server-Side Apply (SSA) should be used when patching the primary
    *         resources, {@code false} otherwise

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

@@ -10,14 +10,12 @@
 import io.fabric8.kubernetes.api.model.KubernetesResourceList;
 import io.fabric8.kubernetes.api.model.Namespaced;
 import io.fabric8.kubernetes.api.model.ObjectMeta;
-import io.fabric8.kubernetes.client.CustomResource;
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.base.PatchContext;
 import io.fabric8.kubernetes.client.dsl.base.PatchType;
 import io.javaoperatorsdk.operator.OperatorException;
-import io.javaoperatorsdk.operator.api.ObservedGenerationAware;
 import io.javaoperatorsdk.operator.api.config.Cloner;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.BaseControl;
@@ -166,13 +164,8 @@ private PostExecutionControl<P> reconcileExecution(ExecutionScope<P> executionSc
       }
     }
 
-    // check if status also needs to be updated
-    final var updateObservedGeneration = updateControl.isNoUpdate()
-        ? shouldUpdateObservedGenerationAutomatically(resourceForExecution)
-        : shouldUpdateObservedGenerationAutomatically(updatedCustomResource);
-    // if using SSA the observed generation is updated only if user instructs patching the status
-    if (updateControl.isPatchStatus() || (updateObservedGeneration && !useSSA)) {
-      updatedCustomResource = patchStatusGenerationAware(toUpdate, originalResource);
+    if (updateControl.isPatchStatus()) {
+      customResourceFacade.patchStatus(toUpdate, originalResource);
     }
     return createPostExecutionControl(updatedCustomResource, updateControl);
   }
@@ -202,9 +195,8 @@ public boolean isLastAttempt() {
 
         P updatedResource = null;
         if (errorStatusUpdateControl.getResource().isPresent()) {
-          updatedResource =
-              patchStatusGenerationAware(errorStatusUpdateControl.getResource().orElseThrow(),
-                  originalResource);
+          updatedResource = customResourceFacade
+              .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource);
         }
         if (errorStatusUpdateControl.isNoRetry()) {
           PostExecutionControl<P> postExecutionControl;
@@ -230,38 +222,9 @@ private boolean isErrorStatusHandlerPresent() {
   }
 
   private P patchStatusGenerationAware(P resource, P originalResource) {
-    updateStatusObservedGenerationIfRequired(resource);
     return customResourceFacade.patchStatus(resource, originalResource);
   }
 
-  @SuppressWarnings("rawtypes")
-  private boolean shouldUpdateObservedGenerationAutomatically(P resource) {
-    if (configuration().isGenerationAware() && resource instanceof CustomResource<?, ?>) {
-      var customResource = (CustomResource) resource;
-      var status = customResource.getStatus();
-      // Note that if status is null we won't update the observed generation.
-      if (status instanceof ObservedGenerationAware) {
-        var observedGen = ((ObservedGenerationAware) status).getObservedGeneration();
-        var currentGen = resource.getMetadata().getGeneration();
-        return !currentGen.equals(observedGen);
-      }
-    }
-    return false;
-  }
-
-  @SuppressWarnings("rawtypes")
-  private void updateStatusObservedGenerationIfRequired(P resource) {
-    if (configuration().isGenerationAware() && resource instanceof CustomResource<?, ?>) {
-      var customResource = (CustomResource) resource;
-      var status = customResource.getStatus();
-      // Note that if status is null we won't update the observed generation.
-      if (status instanceof ObservedGenerationAware) {
-        ((ObservedGenerationAware) status)
-            .setObservedGeneration(resource.getMetadata().getGeneration());
-      }
-    }
-  }
-
   private PostExecutionControl<P> createPostExecutionControl(P updatedCustomResource,
       UpdateControl<P> updateControl) {
     PostExecutionControl<P> postExecutionControl;

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -435,29 +435,6 @@ void reScheduleOnDeleteWithoutFinalizerRemoval() {
         .isEqualTo(1000L);
   }
 
-  @Test
-  void setObservedGenerationForStatusIfNeeded() throws Exception {
-    var observedGenResource = createObservedGenCustomResource();
-
-    Reconciler<ObservedGenCustomResource> reconciler = mock(Reconciler.class);
-    ControllerConfiguration<ObservedGenCustomResource> config =
-        MockControllerConfiguration.forResource(ObservedGenCustomResource.class);
-    CustomResourceFacade<ObservedGenCustomResource> facade = mock(CustomResourceFacade.class);
-    var dispatcher = init(observedGenResource, reconciler, config, facade, true);
-
-    when(config.isGenerationAware()).thenReturn(true);
-
-    when(reconciler.reconcile(any(), any()))
-        .thenReturn(UpdateControl.patchStatus(observedGenResource));
-    when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource);
-
-    PostExecutionControl<ObservedGenCustomResource> control = dispatcher.handleExecution(
-        executionScopeWithCREvent(observedGenResource));
-    assertThat(control.getUpdatedCustomResource().orElseGet(() -> fail("Missing optional"))
-        .getStatus().getObservedGeneration())
-        .isEqualTo(1L);
-  }
-
   @Test
   void doesNotUpdatesObservedGenerationIfStatusIsNotPatchedWhenUsingSSA() throws Exception {
     var observedGenResource = createObservedGenCustomResource();
@@ -476,28 +453,6 @@ void doesNotUpdatesObservedGenerationIfStatusIsNotPatchedWhenUsingSSA() throws E
     assertThat(control.getUpdatedCustomResource()).isEmpty();
   }
 
-  @Test
-  void patchObservedGenerationOnCustomResourcePatchIfNoSSA() throws Exception {
-    var observedGenResource = createObservedGenCustomResource();
-
-    Reconciler<ObservedGenCustomResource> reconciler = mock(Reconciler.class);
-    final var config = MockControllerConfiguration.forResource(ObservedGenCustomResource.class);
-    CustomResourceFacade<ObservedGenCustomResource> facade = mock(CustomResourceFacade.class);
-    when(config.isGenerationAware()).thenReturn(true);
-    when(reconciler.reconcile(any(), any()))
-        .thenReturn(UpdateControl.patchResource(observedGenResource));
-    when(facade.patchResource(any(), any())).thenReturn(observedGenResource);
-    when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource);
-    initConfigService(false);
-    var dispatcher = init(observedGenResource, reconciler, config, facade, true);
-
-    PostExecutionControl<ObservedGenCustomResource> control = dispatcher.handleExecution(
-        executionScopeWithCREvent(observedGenResource));
-    assertThat(control.getUpdatedCustomResource().orElseGet(() -> fail("Missing optional"))
-        .getStatus().getObservedGeneration())
-        .isEqualTo(1L);
-  }
-
   @Test
   void doesNotPatchObservedGenerationOnCustomResourcePatch() throws Exception {
     var observedGenResource = createObservedGenCustomResource();

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

@@ -1,7 +1,5 @@
 package io.javaoperatorsdk.operator.sample.observedgeneration;
 
-import io.javaoperatorsdk.operator.api.ObservedGenerationAwareStatus;
-
-public class ObservedGenStatus extends ObservedGenerationAwareStatus {
+public class ObservedGenStatus {
 
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

@@ -0,0 +1,54 @@
+package io.javaoperatorsdk.operator;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+import io.javaoperatorsdk.operator.sample.manualobservedgeneration.ManualObservedGenerationCustomResource;
+import io.javaoperatorsdk.operator.sample.manualobservedgeneration.ManualObservedGenerationReconciler;
+import io.javaoperatorsdk.operator.sample.manualobservedgeneration.ManualObservedGenerationSpec;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+public class ManualObservedGenerationIT {
+
+  public static final String RESOURCE_NAME = "test1";
+  @RegisterExtension
+  LocallyRunOperatorExtension extension =
+      LocallyRunOperatorExtension.builder().withReconciler(new ManualObservedGenerationReconciler())
+          .build();
+
+  @Test
+  void observedGenerationUpdated() {
+    extension.create(testResource());
+
+    await().untilAsserted(() -> {
+      var r = extension.get(ManualObservedGenerationCustomResource.class, RESOURCE_NAME);
+      assertThat(r).isNotNull();
+      assertThat(r.getStatus().getObservedGeneration()).isEqualTo(1);
+      assertThat(r.getStatus().getObservedGeneration()).isEqualTo(r.getMetadata().getGeneration());
+    });
+
+    var changed = testResource();
+    changed.getSpec().setValue("changed value");
+    extension.replace(changed);
+
+    await().untilAsserted(() -> {
+      var r = extension.get(ManualObservedGenerationCustomResource.class, RESOURCE_NAME);
+      assertThat(r.getStatus().getObservedGeneration()).isEqualTo(2);
+      assertThat(r.getStatus().getObservedGeneration()).isEqualTo(r.getMetadata().getGeneration());
+    });
+  }
+
+  ManualObservedGenerationCustomResource testResource() {
+    var res = new ManualObservedGenerationCustomResource();
+    res.setMetadata(new ObjectMetaBuilder()
+        .withName(RESOURCE_NAME)
+        .build());
+    res.setSpec(new ManualObservedGenerationSpec());
+    res.getSpec().setValue("Initial Value");
+    return res;
+  }
+}