From 2a70c0d1b614371be380e2cda54b773649447d03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 20 Mar 2024 17:11:26 +0100 Subject: [PATCH 01/28] feat: using SSA for finalizer and primary patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 10 +++++++ .../event/ReconciliationDispatcher.java | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 7ce3ae627f..6932689294 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -385,4 +385,14 @@ default boolean useSSAForResourceStatusPatch() { return true; } + /** + * If SSA should be used to add finalizer and for patching the resource spec or metadata or other + * fields excluding the status subresource. + * + * @return true by default + */ + default boolean useSSAForPrimaryResourceNonStatusPath() { + return true; + } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index a240827d14..928bc1e4d5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -1,7 +1,9 @@ package io.javaoperatorsdk.operator.processing.event; +import java.lang.reflect.InvocationTargetException; import java.util.function.Function; +import io.fabric8.kubernetes.client.KubernetesClient; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -316,6 +318,19 @@ private PostExecutionControl

handleCleanup(P resource, return postExecutionControl; } + @SuppressWarnings("unchecked") + private P addFinalizerWithSSA(P originalResource) { + log.debug( + "Adding finalizer (using SSA) for resource: {} version: {}", + getUID(originalResource),getVersion(originalResource)); + try { + P resource = (P) originalResource.getClass().getConstructor().newInstance(); + return customResourceFacade.patchResourceWithSSA(resource); + } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { + throw new RuntimeException(e); + } + } + private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalResource) { log.debug( "Adding finalizer for resource: {} version: {}", getUID(originalResource), @@ -441,6 +456,19 @@ public R patchStatus(R resource, R originalResource) { } } + public R patchResourceWithSSA(R resource) { + var managedFields = resource.getMetadata().getManagedFields(); + try { + return resource(resource).patch(new PatchContext.Builder() + .withFieldManager(fieldManager) + .withForce(true) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + } finally { + resource.getMetadata().setManagedFields(managedFields); + } + } + private Resource resource(R resource) { return resource instanceof Namespaced ? resourceOperation .inNamespace(resource.getMetadata().getNamespace()) From 051aedca12ae1a1565365c48df3e7e5fed557328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 21 Mar 2024 13:10:59 +0100 Subject: [PATCH 02/28] test fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 10 ---------- .../event/ReconciliationDispatcher.java | 20 ++++++++++++++++--- .../event/ReconciliationDispatcherTest.java | 6 ++++-- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 6932689294..7ce3ae627f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -385,14 +385,4 @@ default boolean useSSAForResourceStatusPatch() { return true; } - /** - * If SSA should be used to add finalizer and for patching the resource spec or metadata or other - * fields excluding the status subresource. - * - * @return true by default - */ - default boolean useSSAForPrimaryResourceNonStatusPath() { - return true; - } - } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 928bc1e4d5..6f5b92df1d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -3,7 +3,7 @@ import java.lang.reflect.InvocationTargetException; import java.util.function.Function; -import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.api.model.ObjectMeta; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -46,6 +46,7 @@ class ReconciliationDispatcher

{ // Usually for testing purposes. private final boolean retryConfigurationHasZeroAttempts; private final Cloner cloner; + private final boolean useSSAForPrimaryResourceNonStatusPatch; ReconciliationDispatcher(Controller

controller, CustomResourceFacade

customResourceFacade) { this.controller = controller; @@ -54,6 +55,9 @@ class ReconciliationDispatcher

{ var retry = controller.getConfiguration().getRetry(); retryConfigurationHasZeroAttempts = retry == null || retry.initExecution().isLastAttempt(); + useSSAForPrimaryResourceNonStatusPatch = controller.getConfiguration() + // todo rename flag more generic + .getConfigurationService().useSSAForResourceStatusPatch(); } public ReconciliationDispatcher(Controller

controller) { @@ -114,8 +118,13 @@ private PostExecutionControl

handleReconcile( * finalizer add. This will make sure that the resources are not created before there is a * finalizer. */ - var updatedResource = - updateCustomResourceWithFinalizer(resourceForExecution, originalResource); + P updatedResource; + if (useSSAForPrimaryResourceNonStatusPatch) { + updatedResource = addFinalizerWithSSA(originalResource); + } else { + updatedResource = + updateCustomResourceWithFinalizer(resourceForExecution, originalResource); + } return PostExecutionControl.onlyFinalizerAdded(updatedResource); } else { try { @@ -325,6 +334,11 @@ private P addFinalizerWithSSA(P originalResource) { getUID(originalResource),getVersion(originalResource)); try { P resource = (P) originalResource.getClass().getConstructor().newInstance(); + ObjectMeta objectMeta = new ObjectMeta(); + objectMeta.setName(originalResource.getMetadata().getName()); + objectMeta.setNamespace(originalResource.getMetadata().getNamespace()); + resource.setMetadata(objectMeta); + objectMeta.getFinalizers().add(configuration().getFinalizerName()); return customResourceFacade.patchResourceWithSSA(resource); } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { throw new RuntimeException(e); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index d2e4cd52dc..362459fde6 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -77,7 +77,9 @@ static void classSetup() { public R clone(R object) { return object; } - })); + }) + .withUseSSAForResourceStatusPatch(false) + ); } @BeforeEach @@ -178,7 +180,7 @@ void patchesStatus() { verify(customResourceFacade, never()).updateStatus(any()); verify(customResourceFacade, never()).updateResource(any()); } - + @Test void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); From 19da907aa62528851dc5f317d28917b542bb0116 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 21 Mar 2024 13:12:54 +0100 Subject: [PATCH 03/28] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcher.java | 43 ++++++++++--------- .../event/ReconciliationDispatcherTest.java | 5 +-- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 6f5b92df1d..09bd45244b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -3,13 +3,13 @@ import java.lang.reflect.InvocationTargetException; import java.util.function.Function; -import io.fabric8.kubernetes.api.model.ObjectMeta; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; 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; @@ -56,8 +56,8 @@ class ReconciliationDispatcher

{ var retry = controller.getConfiguration().getRetry(); retryConfigurationHasZeroAttempts = retry == null || retry.initExecution().isLastAttempt(); useSSAForPrimaryResourceNonStatusPatch = controller.getConfiguration() - // todo rename flag more generic - .getConfigurationService().useSSAForResourceStatusPatch(); + // todo rename flag more generic + .getConfigurationService().useSSAForResourceStatusPatch(); } public ReconciliationDispatcher(Controller

controller) { @@ -123,7 +123,7 @@ private PostExecutionControl

handleReconcile( updatedResource = addFinalizerWithSSA(originalResource); } else { updatedResource = - updateCustomResourceWithFinalizer(resourceForExecution, originalResource); + updateCustomResourceWithFinalizer(resourceForExecution, originalResource); } return PostExecutionControl.onlyFinalizerAdded(updatedResource); } else { @@ -330,19 +330,20 @@ private PostExecutionControl

handleCleanup(P resource, @SuppressWarnings("unchecked") private P addFinalizerWithSSA(P originalResource) { log.debug( - "Adding finalizer (using SSA) for resource: {} version: {}", - getUID(originalResource),getVersion(originalResource)); - try { - P resource = (P) originalResource.getClass().getConstructor().newInstance(); - ObjectMeta objectMeta = new ObjectMeta(); - objectMeta.setName(originalResource.getMetadata().getName()); - objectMeta.setNamespace(originalResource.getMetadata().getNamespace()); - resource.setMetadata(objectMeta); - objectMeta.getFinalizers().add(configuration().getFinalizerName()); - return customResourceFacade.patchResourceWithSSA(resource); - } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { - throw new RuntimeException(e); - } + "Adding finalizer (using SSA) for resource: {} version: {}", + getUID(originalResource), getVersion(originalResource)); + try { + P resource = (P) originalResource.getClass().getConstructor().newInstance(); + ObjectMeta objectMeta = new ObjectMeta(); + objectMeta.setName(originalResource.getMetadata().getName()); + objectMeta.setNamespace(originalResource.getMetadata().getNamespace()); + resource.setMetadata(objectMeta); + objectMeta.getFinalizers().add(configuration().getFinalizerName()); + return customResourceFacade.patchResourceWithSSA(resource); + } catch (InstantiationException | IllegalAccessException | InvocationTargetException + | NoSuchMethodException e) { + throw new RuntimeException(e); + } } private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalResource) { @@ -474,10 +475,10 @@ public R patchResourceWithSSA(R resource) { var managedFields = resource.getMetadata().getManagedFields(); try { return resource(resource).patch(new PatchContext.Builder() - .withFieldManager(fieldManager) - .withForce(true) - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build()); + .withFieldManager(fieldManager) + .withForce(true) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); } finally { resource.getMetadata().setManagedFields(managedFields); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 362459fde6..9138e5d2a5 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -78,8 +78,7 @@ public R clone(R object) { return object; } }) - .withUseSSAForResourceStatusPatch(false) - ); + .withUseSSAForResourceStatusPatch(false)); } @BeforeEach @@ -180,7 +179,7 @@ void patchesStatus() { verify(customResourceFacade, never()).updateStatus(any()); verify(customResourceFacade, never()).updateResource(any()); } - + @Test void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); From 3c5646c39d701d7c3bdf2834d1f86fcb9d9e82a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 21 Mar 2024 14:20:16 +0100 Subject: [PATCH 04/28] wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../reconciler/ErrorStatusUpdateControl.java | 16 +-- .../api/reconciler/UpdateControl.java | 100 +++--------------- .../operator/processing/Controller.java | 2 +- .../processing/event/EventProcessor.java | 1 + .../event/PostExecutionControl.java | 2 +- .../event/ReconciliationDispatcher.java | 21 ++-- 6 files changed, 27 insertions(+), 115 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusUpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusUpdateControl.java index 48c0e32946..7236d5898b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusUpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ErrorStatusUpdateControl.java @@ -9,24 +9,18 @@ public class ErrorStatusUpdateControl

extends BaseControl> { private final P resource; - private final boolean patch; private boolean noRetry = false; public static ErrorStatusUpdateControl patchStatus(T resource) { - return new ErrorStatusUpdateControl<>(resource, true); - } - - public static ErrorStatusUpdateControl updateStatus(T resource) { - return new ErrorStatusUpdateControl<>(resource, false); + return new ErrorStatusUpdateControl<>(resource); } public static ErrorStatusUpdateControl noStatusUpdate() { - return new ErrorStatusUpdateControl<>(null, true); + return new ErrorStatusUpdateControl<>(null); } - private ErrorStatusUpdateControl(P resource, boolean patch) { + private ErrorStatusUpdateControl(P resource) { this.resource = resource; - this.patch = patch; } /** @@ -47,10 +41,6 @@ public boolean isNoRetry() { return noRetry; } - public boolean isPatch() { - return patch; - } - /** * If re-scheduled using this method, it is not considered as retry, it effectively cancels retry. * diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index 7eb7d2ae84..2f6afcfc92 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -6,37 +6,19 @@ public class UpdateControl

extends BaseControl> { private final P resource; - private final boolean updateStatus; - private final boolean updateResource; + private final boolean patchResource; private final boolean patchStatus; private UpdateControl( - P resource, boolean updateStatus, boolean updateResource, boolean patchStatus) { - if ((updateResource || updateStatus) && resource == null) { + P resource, boolean patchResource, boolean patchStatus) { + if ((patchResource || patchStatus) && resource == null) { throw new IllegalArgumentException("CustomResource cannot be null in case of update"); } this.resource = resource; - this.updateStatus = updateStatus; - this.updateResource = updateResource; + this.patchResource = patchResource; this.patchStatus = patchStatus; } - /** - * Creates an update control instance that instructs the framework to do an update on resource - * itself, not on the status. Note that usually as a results of a reconciliation should be a - * status update not an update to the resource itself. - * - * Using this update makes sure that the resource in the next reconciliation is the updated one - - * this is not guaranteed by default if you do an update on a resource by the Kubernetes client. - * - * @param custom resource type - * @param customResource customResource to use for update - * @return initialized update control - */ - public static UpdateControl updateResource(T customResource) { - return new UpdateControl<>(customResource, false, true, false); - } - /** * Preferred way to update the status. It does not do optimistic locking. Uses JSON Patch to patch * the resource. @@ -53,86 +35,32 @@ public static UpdateControl updateResource(T customRe * @return UpdateControl instance */ public static UpdateControl patchStatus(T customResource) { - return new UpdateControl<>(customResource, true, false, true); + return new UpdateControl<>(customResource, false, false); } - /** - * Note that usually "patchStatus" is advised to be used instead of this method. - *

- * 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. - *

- * - * @param resource type - * @param customResource the custom resource with target status - * @return UpdateControl instance - */ - public static UpdateControl updateStatus(T customResource) { - return new UpdateControl<>(customResource, true, false, false); + public static UpdateControl patchResource(T customResource) { + return new UpdateControl<>(customResource, true, 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. - * - * Using this update makes sure that the resource in the next reconciliation is the updated one - - * this is not guaranteed by default if you do an update on a resource by the Kubernetes client. - * - * @param resource type - * @param customResource - custom resource to use in both API calls - * @return UpdateControl instance - */ - public static UpdateControl updateResourceAndStatus( - T customResource) { - return new UpdateControl<>(customResource, true, true, false); - } - - /** - * Updates the resource - with optimistic locking - and patches the status without optimistic - * locking in place. - * - * Note that using this method, it is not guaranteed that the most recent updated resource will be - * in case for next reconciliation. - * * @param customResource to update * @return UpdateControl instance * @param resource type */ - public static UpdateControl updateResourceAndPatchStatus( - T customResource) { - return new UpdateControl<>(customResource, true, true, true); - } - - /** - * Marked for removal, because of confusing name. It does not patch the resource but rather - * updates it. - * - * @deprecated use {@link UpdateControl#updateResourceAndPatchStatus(HasMetadata)} - * - * @param customResource to update - * @return UpdateControl instance - * @param resource type - */ - @Deprecated(forRemoval = true) public static UpdateControl patchResourceAndStatus(T customResource) { - return updateResourceAndStatus(customResource); + return new UpdateControl<>(customResource, true, true); } public static UpdateControl noUpdate() { - return new UpdateControl<>(null, false, false, false); + return new UpdateControl<>(null, false, false); } public P getResource() { return resource; } - public boolean isUpdateStatus() { - return updateStatus; - } - - public boolean isUpdateResource() { - return updateResource; + public boolean isPatchResource() { + return patchResource; } public boolean isPatchStatus() { @@ -140,11 +68,11 @@ public boolean isPatchStatus() { } public boolean isNoUpdate() { - return !updateResource && !updateStatus; + return !patchResource && !patchStatus; } - public boolean isUpdateResourceAndStatus() { - return updateResource && updateStatus; + public boolean isPatchResourceAndStatus() { + return patchResource && patchStatus; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java index 7ab1eca457..975e37a712 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java @@ -125,7 +125,7 @@ public String successTypeName(UpdateControl

result) { if (result.isUpdateStatus()) { successType = STATUS; } - if (result.isUpdateResourceAndStatus()) { + if (result.isPatchResourceAndStatus()) { successType = BOTH; } return successType; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index b0bf48802a..53d15340e8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -249,6 +249,7 @@ synchronized void eventProcessingFinished( .getUpdatedCustomResource() .ifPresent( p -> { + // todo check if (!postExecutionControl.updateIsStatusPatch()) { eventSourceManager .getControllerResourceEventSource() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java index 6fddd5ad93..3343cff80a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java @@ -37,7 +37,7 @@ public static PostExecutionControl customResourceStat return new PostExecutionControl<>(false, updatedCustomResource, true, null); } - public static PostExecutionControl customResourceUpdated( + public static PostExecutionControl customResourcePatched( R updatedCustomResource) { return new PostExecutionControl<>(false, updatedCustomResource, false, null); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 09bd45244b..6368eeb973 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -151,12 +151,12 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc P updatedCustomResource = null; final var toUpdate = updateControl.isNoUpdate() ? originalResource : updateControl.getResource(); - if (updateControl.isUpdateResourceAndStatus()) { + if (updateControl.isPatchResourceAndStatus()) { updatedCustomResource = updateCustomResource(toUpdate); toUpdate .getMetadata() .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); - } else if (updateControl.isUpdateResource()) { + } else if (updateControl.isPatchResource()) { updatedCustomResource = updateCustomResource(toUpdate); } @@ -164,7 +164,7 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc final var updateObservedGeneration = updateControl.isNoUpdate() ? shouldUpdateObservedGenerationAutomatically(resourceForExecution) : shouldUpdateObservedGenerationAutomatically(updatedCustomResource); - if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus() + if (updateControl.isPatchResourceAndStatus() || updateControl.isPatchStatus() || updateObservedGeneration) { updatedCustomResource = updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus()); @@ -197,17 +197,14 @@ public boolean isLastAttempt() { P updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { - updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade - .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource) - : customResourceFacade - .updateStatus(errorStatusUpdateControl.getResource().orElseThrow()); + updatedResource = customResourceFacade + .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource); + } if (errorStatusUpdateControl.isNoRetry()) { PostExecutionControl

postExecutionControl; if (updatedResource != null) { - postExecutionControl = errorStatusUpdateControl.isPatch() - ? PostExecutionControl.customResourceStatusPatched(updatedResource) - : PostExecutionControl.customResourceUpdated(updatedResource); + postExecutionControl = PostExecutionControl.customResourceStatusPatched(updatedResource); } else { postExecutionControl = PostExecutionControl.defaultDispatch(); } @@ -267,12 +264,8 @@ private PostExecutionControl

createPostExecutionControl(P updatedCustomResour UpdateControl

updateControl) { PostExecutionControl

postExecutionControl; if (updatedCustomResource != null) { - if (updateControl.isUpdateStatus() && updateControl.isPatchStatus()) { postExecutionControl = PostExecutionControl.customResourceStatusPatched(updatedCustomResource); - } else { - postExecutionControl = PostExecutionControl.customResourceUpdated(updatedCustomResource); - } } else { postExecutionControl = PostExecutionControl.defaultDispatch(); } From 4b2bb22c42a761b11302a18523c1ff3285a346fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 21 Mar 2024 15:49:12 +0100 Subject: [PATCH 05/28] wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/processing/Controller.java | 2 +- .../event/ReconciliationDispatcher.java | 49 ++++++++++--------- .../processing/event/EventProcessorTest.java | 2 +- .../event/ReconciliationDispatcherTest.java | 44 ++++++++--------- .../sample/simple/TestCustomReconciler.java | 2 +- 5 files changed, 50 insertions(+), 49 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java index 975e37a712..540eb983dd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java @@ -122,7 +122,7 @@ public String controllerName() { @Override public String successTypeName(UpdateControl

result) { String successType = RESOURCE; - if (result.isUpdateStatus()) { + if (result.isPatchStatus()) { successType = STATUS; } if (result.isPatchResourceAndStatus()) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 6368eeb973..2f422c59de 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -46,7 +46,7 @@ class ReconciliationDispatcher

{ // Usually for testing purposes. private final boolean retryConfigurationHasZeroAttempts; private final Cloner cloner; - private final boolean useSSAForPrimaryResourceNonStatusPatch; + private final boolean useSSA; ReconciliationDispatcher(Controller

controller, CustomResourceFacade

customResourceFacade) { this.controller = controller; @@ -55,7 +55,7 @@ class ReconciliationDispatcher

{ var retry = controller.getConfiguration().getRetry(); retryConfigurationHasZeroAttempts = retry == null || retry.initExecution().isLastAttempt(); - useSSAForPrimaryResourceNonStatusPatch = controller.getConfiguration() + useSSA = controller.getConfiguration() // todo rename flag more generic .getConfigurationService().useSSAForResourceStatusPatch(); } @@ -119,7 +119,7 @@ private PostExecutionControl

handleReconcile( * finalizer. */ P updatedResource; - if (useSSAForPrimaryResourceNonStatusPatch) { + if (useSSA) { updatedResource = addFinalizerWithSSA(originalResource); } else { updatedResource = @@ -149,15 +149,17 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc UpdateControl

updateControl = controller.reconcile(resourceForExecution, context); P updatedCustomResource = null; + // todo this needs to be refactored, SSA will always use the resource from UpdateControl + // except when noUpdate but there is observed generation update supported final var toUpdate = updateControl.isNoUpdate() ? originalResource : updateControl.getResource(); if (updateControl.isPatchResourceAndStatus()) { - updatedCustomResource = updateCustomResource(toUpdate); + updatedCustomResource = patchResource(toUpdate); toUpdate .getMetadata() .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); } else if (updateControl.isPatchResource()) { - updatedCustomResource = updateCustomResource(toUpdate); + updatedCustomResource = patchResource(toUpdate); } // check if status also needs to be updated @@ -167,7 +169,7 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc if (updateControl.isPatchResourceAndStatus() || updateControl.isPatchStatus() || updateObservedGeneration) { updatedCustomResource = - updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus()); + patchStatusGenerationAware(toUpdate, originalResource); } return createPostExecutionControl(updatedCustomResource, updateControl); } @@ -223,13 +225,9 @@ private boolean isErrorStatusHandlerPresent() { return controller.getReconciler() instanceof ErrorStatusHandler; } - private P updateStatusGenerationAware(P resource, P originalResource, boolean patch) { + private P patchStatusGenerationAware(P resource, P originalResource) { updateStatusObservedGenerationIfRequired(resource); - if (patch) { - return customResourceFacade.patchStatus(resource, originalResource); - } else { - return customResourceFacade.updateStatus(resource); - } + return customResourceFacade.patchStatus(resource, originalResource); } @SuppressWarnings("rawtypes") @@ -347,12 +345,12 @@ private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalRe r -> r.addFinalizer(configuration().getFinalizerName())); } - private P updateCustomResource(P resource) { + private P patchResource(P resource) { log.debug("Updating resource: {} with version: {}", getUID(resource), getVersion(resource)); log.trace("Resource before update: {}", resource); - return customResourceFacade.updateResource(resource); + return customResourceFacade.patchResource(resource); } ControllerConfiguration

configuration() { @@ -370,7 +368,7 @@ public P conflictRetryingUpdate(P resource, Function modificationFun if (Boolean.FALSE.equals(modified)) { return resource; } - return customResourceFacade.updateResource(resource); + return customResourceFacade.patchResource(resource); } catch (KubernetesClientException e) { log.trace("Exception during patch for resource: {}", resource); retryIndex++; @@ -394,14 +392,14 @@ public P conflictRetryingUpdate(P resource, Function modificationFun static class CustomResourceFacade { private final MixedOperation, Resource> resourceOperation; - private final boolean useSSAToUpdateStatus; + private final boolean useSSA; private final String fieldManager; public CustomResourceFacade( MixedOperation, Resource> resourceOperation, ControllerConfiguration configuration) { this.resourceOperation = resourceOperation; - this.useSSAToUpdateStatus = + this.useSSA = configuration.getConfigurationService().useSSAForResourceStatusPatch(); this.fieldManager = configuration.fieldManager(); } @@ -425,21 +423,24 @@ public R updateResource(R resource) { .update(); } - public R updateStatus(R resource) { - log.trace("Updating status for resource: {}", resource); - return resource(resource) - .lockResourceVersion() - .updateStatus(); + public R patchResource(R resource) { + if (log.isDebugEnabled()) { + log.debug( + "Trying to replace resource {}, version: {}", + ResourceID.fromResource(resource), + resource.getMetadata().getResourceVersion()); + } + return resource(resource).patch(); } public R patchStatus(R resource, R originalResource) { - log.trace("Patching status for resource: {} with ssa: {}", resource, useSSAToUpdateStatus); + log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA); String resourceVersion = resource.getMetadata().getResourceVersion(); // don't do optimistic locking on patch originalResource.getMetadata().setResourceVersion(null); resource.getMetadata().setResourceVersion(null); try { - if (useSSAToUpdateStatus) { + if (useSSA) { var managedFields = resource.getMetadata().getManagedFields(); try { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java index c52a976b86..07f90764da 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java @@ -293,7 +293,7 @@ void updatesEventSourceHandlerIfResourceUpdated() { ExecutionScope executionScope = new ExecutionScope(null).setResource(customResource); PostExecutionControl postExecutionControl = - PostExecutionControl.customResourceUpdated(customResource); + PostExecutionControl.customResourcePatched(customResource); eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 9138e5d2a5..e60eadc13a 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -128,7 +128,7 @@ void addFinalizerOnNewResource() { verify(reconciler, never()) .reconcile(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) - .updateResource( + .patchResource( argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER))); assertThat(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)).isTrue(); } @@ -150,7 +150,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); - verify(customResourceFacade, never()).updateResource(any()); + verify(customResourceFacade, never()).patchResource(any()); } @Test @@ -158,12 +158,12 @@ void updatesBothResourceAndStatusIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.updateResourceAndStatus(testCustomResource); - when(customResourceFacade.updateResource(testCustomResource)) + when(customResourceFacade.patchResource(testCustomResource)) .thenReturn(testCustomResource); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).updateResource(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource); verify(customResourceFacade, times(1)).updateStatus(testCustomResource); } @@ -177,7 +177,7 @@ void patchesStatus() { verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(customResourceFacade, never()).updateStatus(any()); - verify(customResourceFacade, never()).updateResource(any()); + verify(customResourceFacade, never()).patchResource(any()); } @Test @@ -209,7 +209,7 @@ void removesDefaultFinalizerOnDeleteIfSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).updateResource(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource); } @Test @@ -218,7 +218,7 @@ void retriesFinalizerRemovalWithFreshResource() { markForDeletion(testCustomResource); var resourceWithFinalizer = TestUtils.testCustomResource(); resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); - when(customResourceFacade.updateResource(testCustomResource)) + when(customResourceFacade.patchResource(testCustomResource)) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())).thenReturn(resourceWithFinalizer); @@ -227,7 +227,7 @@ void retriesFinalizerRemovalWithFreshResource() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(2)).updateResource(any()); + verify(customResourceFacade, times(2)).patchResource(any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -237,7 +237,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { // of the finalizer removal testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.updateResource(any())) + when(customResourceFacade.patchResource(any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())).thenReturn(null); @@ -245,7 +245,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).updateResource(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -253,7 +253,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { void throwsExceptionIfFinalizerRemovalRetryExceeded() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.updateResource(any())) + when(customResourceFacade.patchResource(any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())) .thenAnswer((Answer) invocationOnMock -> createResourceWithFinalizer()); @@ -265,7 +265,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { assertThat(postExecControl.getRuntimeException()).isPresent(); assertThat(postExecControl.getRuntimeException().get()) .isInstanceOf(OperatorException.class); - verify(customResourceFacade, times(MAX_UPDATE_RETRY)).updateResource(any()); + verify(customResourceFacade, times(MAX_UPDATE_RETRY)).patchResource(any()); verify(customResourceFacade, times(MAX_UPDATE_RETRY - 1)).getResource(any(), any()); } @@ -274,7 +274,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.updateResource(any())) + when(customResourceFacade.patchResource(any())) .thenThrow(new KubernetesClientException(null, 400, null)); var res = @@ -282,7 +282,7 @@ void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { assertThat(res.getRuntimeException()).isPresent(); assertThat(res.getRuntimeException().get()).isInstanceOf(KubernetesClientException.class); - verify(customResourceFacade, times(1)).updateResource(any()); + verify(customResourceFacade, times(1)).patchResource(any()); verify(customResourceFacade, never()).getResource(any(), any()); } @@ -326,7 +326,7 @@ void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, never()).updateResource(any()); + verify(customResourceFacade, never()).patchResource(any()); } @Test @@ -336,7 +336,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, never()).updateResource(any()); + verify(customResourceFacade, never()).patchResource(any()); verify(customResourceFacade, never()).updateStatus(testCustomResource); } @@ -344,14 +344,14 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.updateResource(any())) + when(customResourceFacade.patchResource(any())) .thenReturn(testCustomResource); var postExecControl = reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, times(1)).updateResource(any()); + verify(customResourceFacade, times(1)).patchResource(any()); assertThat(postExecControl.updateIsStatusPatch()).isFalse(); assertThat(postExecControl.getUpdatedCustomResource()).isPresent(); } @@ -363,7 +363,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, never()).updateResource(any()); + verify(customResourceFacade, never()).patchResource(any()); verify(reconciler, never()).cleanup(eq(testCustomResource), any()); } @@ -486,7 +486,7 @@ void updateObservedGenerationOnCustomResourceUpdate() throws Exception { when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.updateResource(observedGenResource)); - when(facade.updateResource(any())).thenReturn(observedGenResource); + when(facade.patchResource(any())).thenReturn(observedGenResource); when(facade.updateStatus(observedGenResource)).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); @@ -650,7 +650,7 @@ void canSkipSchedulingMaxDelayIf() { void retriesAddingFinalizer() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.updateResource(any())) + when(customResourceFacade.patchResource(any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())) @@ -661,7 +661,7 @@ void retriesAddingFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(2)).updateResource(any()); + verify(customResourceFacade, times(2)).patchResource(any()); } @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomReconciler.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomReconciler.java index 748b76b72d..be2c80667e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomReconciler.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomReconciler.java @@ -104,7 +104,7 @@ public UpdateControl reconcile( } resource.getStatus().setConfigMapStatus("ConfigMap Ready"); } - return UpdateControl.updateResource(resource); + return UpdateControl.patchResource(resource); } private Map configMapData(TestCustomResource resource) { From da598ab803327587afc1c55f5ea66038c237828c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 25 Mar 2024 14:31:17 +0100 Subject: [PATCH 06/28] wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 7 +- .../config/ConfigurationServiceOverrider.java | 14 ++-- .../api/reconciler/UpdateControl.java | 6 +- .../event/ReconciliationDispatcher.java | 64 ++++++++++--------- .../event/ReconciliationDispatcherTest.java | 4 +- .../operator/StatusPatchSSAMigrationIT.java | 2 +- 6 files changed, 52 insertions(+), 45 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 7ce3ae627f..f5d3fe503a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -376,12 +376,13 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() { } /** - * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus can either use - * simple update or SSA for status subresource patching. + * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can either use + * simple patches or SSA. Setting this to true, controller will use SSA for adding finalizers, + * managing observed generation, patching resources and status. * * @return true by default */ - default boolean useSSAForResourceStatusPatch() { + default boolean useSSAToPatchPrimaryResource() { return true; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 27d3d7d080..4ccf85a8fd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -39,7 +39,7 @@ public class ConfigurationServiceOverrider { private Set> defaultNonSSAResource; private Boolean previousAnnotationForDependentResources; private Boolean parseResourceVersions; - private Boolean useSSAForResourceStatusPatch; + private Boolean useSSAToPatchPrimaryResource; private DependentResourceFactory dependentResourceFactory; ConfigurationServiceOverrider(ConfigurationService original) { @@ -175,8 +175,8 @@ public ConfigurationServiceOverrider withParseResourceVersions( return this; } - public ConfigurationServiceOverrider withUseSSAForResourceStatusPatch(boolean value) { - this.useSSAForResourceStatusPatch = value; + public ConfigurationServiceOverrider withUseSSAToPatchPrimaryResource(boolean value) { + this.useSSAToPatchPrimaryResource = value; return this; } @@ -309,10 +309,10 @@ public boolean parseResourceVersionsForEventFilteringAndCaching() { } @Override - public boolean useSSAForResourceStatusPatch() { - return useSSAForResourceStatusPatch != null - ? useSSAForResourceStatusPatch - : super.useSSAForResourceStatusPatch(); + public boolean useSSAToPatchPrimaryResource() { + return useSSAToPatchPrimaryResource != null + ? useSSAToPatchPrimaryResource + : super.useSSAToPatchPrimaryResource(); } }; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index 2f6afcfc92..ae8cbd2343 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -3,6 +3,8 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.CustomResource; +import java.util.Optional; + public class UpdateControl

extends BaseControl> { private final P resource; @@ -55,8 +57,8 @@ public static UpdateControl noUpdate() { return new UpdateControl<>(null, false, false); } - public P getResource() { - return resource; + public Optional

getResource() { + return Optional.ofNullable(resource); } public boolean isPatchResource() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 2f422c59de..b73e7e7d59 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -56,8 +56,7 @@ class ReconciliationDispatcher

{ var retry = controller.getConfiguration().getRetry(); retryConfigurationHasZeroAttempts = retry == null || retry.initExecution().isLastAttempt(); useSSA = controller.getConfiguration() - // todo rename flag more generic - .getConfigurationService().useSSAForResourceStatusPatch(); + .getConfigurationService().useSSAToPatchPrimaryResource(); } public ReconciliationDispatcher(Controller

controller) { @@ -148,26 +147,33 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc executionScope); UpdateControl

updateControl = controller.reconcile(resourceForExecution, context); + + final P toUpdate; P updatedCustomResource = null; - // todo this needs to be refactored, SSA will always use the resource from UpdateControl - // except when noUpdate but there is observed generation update supported - final var toUpdate = - updateControl.isNoUpdate() ? originalResource : updateControl.getResource(); - if (updateControl.isPatchResourceAndStatus()) { - updatedCustomResource = patchResource(toUpdate); - toUpdate - .getMetadata() - .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); - } else if (updateControl.isPatchResource()) { + if (useSSA) { + if (updateControl.isNoUpdate()) { + return createPostExecutionControl(null, updateControl); + } else { + toUpdate = updateControl.getResource().orElseThrow(); + } + } else { + toUpdate = updateControl.isNoUpdate() ? originalResource : updateControl.getResource().orElseThrow(); + } + + if (updateControl.isPatchResource()) { updatedCustomResource = patchResource(toUpdate); + if (!useSSA) { + toUpdate.getMetadata().setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); + } } // check if status also needs to be updated final var updateObservedGeneration = updateControl.isNoUpdate() ? shouldUpdateObservedGenerationAutomatically(resourceForExecution) : shouldUpdateObservedGenerationAutomatically(updatedCustomResource); - if (updateControl.isPatchResourceAndStatus() || updateControl.isPatchStatus() - || updateObservedGeneration) { + // if using SSA the observed generation is updated only if user instructs patching the status + if (updateControl.isPatchStatus() + || (updateObservedGeneration && !useSSA)) { updatedCustomResource = patchStatusGenerationAware(toUpdate, originalResource); } @@ -199,9 +205,8 @@ public boolean isLastAttempt() { P updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { - updatedResource = customResourceFacade - .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource); - + updatedResource = + patchStatusGenerationAware(errorStatusUpdateControl.getResource().orElseThrow(), originalResource); } if (errorStatusUpdateControl.isNoRetry()) { PostExecutionControl

postExecutionControl; @@ -350,6 +355,12 @@ private P patchResource(P resource) { getVersion(resource)); log.trace("Resource before update: {}", resource); + // todo unit test + if (useSSA && controller.useFinalizer() && + !resource.getMetadata().getFinalizers().contains(configuration().getFinalizerName())) { + resource.getMetadata().getFinalizers().add(configuration().getFinalizerName()); + } + return customResourceFacade.patchResource(resource); } @@ -400,7 +411,7 @@ public CustomResourceFacade( ControllerConfiguration configuration) { this.resourceOperation = resourceOperation; this.useSSA = - configuration.getConfigurationService().useSSAForResourceStatusPatch(); + configuration.getConfigurationService().useSSAToPatchPrimaryResource(); this.fieldManager = configuration.fieldManager(); } @@ -412,17 +423,6 @@ public R getResource(String namespace, String name) { } } - public R updateResource(R resource) { - if (log.isDebugEnabled()) { - log.debug( - "Trying to replace resource {}, version: {}", - ResourceID.fromResource(resource), - resource.getMetadata().getResourceVersion()); - } - return resource(resource).lockResourceVersion(resource.getMetadata().getResourceVersion()) - .update(); - } - public R patchResource(R resource) { if (log.isDebugEnabled()) { log.debug( @@ -430,7 +430,11 @@ public R patchResource(R resource) { ResourceID.fromResource(resource), resource.getMetadata().getResourceVersion()); } - return resource(resource).patch(); + if (useSSA) { + return patchResourceWithSSA(resource); + } else { + return resource(resource).patch(); + } } public R patchStatus(R resource, R originalResource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index e60eadc13a..40494eccd3 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -78,7 +78,7 @@ public R clone(R object) { return object; } }) - .withUseSSAForResourceStatusPatch(false)); + .withUseSSAToPatchPrimaryResource(false)); } @BeforeEach @@ -337,7 +337,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, never()).patchResource(any()); - verify(customResourceFacade, never()).updateStatus(testCustomResource); + verify(customResourceFacade, never()).patchStatus(testCustomResource); } @Test diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java index 01702a5400..fba39cc03f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java @@ -129,7 +129,7 @@ void workaroundMigratingFromToSSA() { private Operator startOperator(boolean patchStatusWithSSA) { var operator = new Operator(o -> o.withCloseClientOnStop(false) - .withUseSSAForResourceStatusPatch(patchStatusWithSSA)); + .withUseSSAToPatchPrimaryResource(patchStatusWithSSA)); operator.register(new StatusPatchLockingReconciler(), o -> o.settingNamespaces(testNamespace)); From e22129143e98ccb08ea09f070b7e4646651be1e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 25 Mar 2024 15:38:33 +0100 Subject: [PATCH 07/28] wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcher.java | 25 +++--- .../event/ReconciliationDispatcherTest.java | 77 +++++++++---------- 2 files changed, 50 insertions(+), 52 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index b73e7e7d59..1f362db2dd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -90,7 +90,7 @@ private PostExecutionControl

handleDispatch(ExecutionScope

executionScope) Context

context = new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource); if (markedForDeletion) { - return handleCleanup(resourceForExecution, context); + return handleCleanup(resourceForExecution, originalResource,context); } else { return handleReconcile(executionScope, resourceForExecution, originalResource, context); } @@ -161,7 +161,7 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc } if (updateControl.isPatchResource()) { - updatedCustomResource = patchResource(toUpdate); + updatedCustomResource = patchResource(toUpdate, originalResource); if (!useSSA) { toUpdate.getMetadata().setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); } @@ -283,7 +283,7 @@ private void updatePostExecutionControlWithReschedule( } private PostExecutionControl

handleCleanup(P resource, - Context

context) { + P originalResource, Context

context) { if (log.isDebugEnabled()) { log.debug( "Executing delete for resource: {} with version: {}", @@ -297,7 +297,7 @@ private PostExecutionControl

handleCleanup(P resource, // cleanup is finished, nothing left to done final var finalizerName = configuration().getFinalizerName(); if (deleteControl.isRemoveFinalizer() && resource.hasFinalizer(finalizerName)) { - P customResource = conflictRetryingUpdate(resource, r -> { + P customResource = conflictRetryingPatch(resource,originalResource, r -> { // the operator might not be allowed to retrieve the resource on a retry, e.g. when its // permissions are removed by deleting the namespace concurrently if (r == null) { @@ -334,7 +334,7 @@ private P addFinalizerWithSSA(P originalResource) { objectMeta.setName(originalResource.getMetadata().getName()); objectMeta.setNamespace(originalResource.getMetadata().getNamespace()); resource.setMetadata(objectMeta); - objectMeta.getFinalizers().add(configuration().getFinalizerName()); + resource.addFinalizer(configuration().getFinalizerName()); return customResourceFacade.patchResourceWithSSA(resource); } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { @@ -346,11 +346,11 @@ private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalRe log.debug( "Adding finalizer for resource: {} version: {}", getUID(originalResource), getVersion(originalResource)); - return conflictRetryingUpdate(resourceForExecution, + return conflictRetryingPatch(resourceForExecution,originalResource, r -> r.addFinalizer(configuration().getFinalizerName())); } - private P patchResource(P resource) { + private P patchResource(P resource, P originalResource) { log.debug("Updating resource: {} with version: {}", getUID(resource), getVersion(resource)); log.trace("Resource before update: {}", resource); @@ -361,14 +361,14 @@ private P patchResource(P resource) { resource.getMetadata().getFinalizers().add(configuration().getFinalizerName()); } - return customResourceFacade.patchResource(resource); + return customResourceFacade.patchResource(resource,originalResource); } ControllerConfiguration

configuration() { return controller.getConfiguration(); } - public P conflictRetryingUpdate(P resource, Function modificationFunction) { + public P conflictRetryingPatch(P resource, P originalResource, Function modificationFunction) { if (log.isDebugEnabled()) { log.debug("Conflict retrying update for: {}", ResourceID.fromResource(resource)); } @@ -379,7 +379,7 @@ public P conflictRetryingUpdate(P resource, Function modificationFun if (Boolean.FALSE.equals(modified)) { return resource; } - return customResourceFacade.patchResource(resource); + return customResourceFacade.patchResource(resource,originalResource); } catch (KubernetesClientException e) { log.trace("Exception during patch for resource: {}", resource); retryIndex++; @@ -423,7 +423,7 @@ public R getResource(String namespace, String name) { } } - public R patchResource(R resource) { + public R patchResource(R resource, R originalResource) { if (log.isDebugEnabled()) { log.debug( "Trying to replace resource {}, version: {}", @@ -433,7 +433,7 @@ public R patchResource(R resource) { if (useSSA) { return patchResourceWithSSA(resource); } else { - return resource(resource).patch(); + return resource(originalResource).edit(r->resource); } } @@ -447,7 +447,6 @@ public R patchStatus(R resource, R originalResource) { if (useSSA) { var managedFields = resource.getMetadata().getManagedFields(); try { - resource.getMetadata().setManagedFields(null); var res = resource(resource); return res.subresource("status").patch(new PatchContext.Builder() diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 40494eccd3..6b70fe6b9b 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -129,7 +129,7 @@ void addFinalizerOnNewResource() { .reconcile(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) .patchResource( - argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER))); + argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)),any()); assertThat(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)).isTrue(); } @@ -142,7 +142,7 @@ void callCreateOrUpdateOnNewResourceIfFinalizerSet() { } @Test - void updatesOnlyStatusSubResourceIfFinalizerSet() { + void patchesOnlyStatusSubResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.patchStatus(testCustomResource); @@ -150,21 +150,21 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); - verify(customResourceFacade, never()).patchResource(any()); + verify(customResourceFacade, never()).patchResource(any(),any()); } @Test - void updatesBothResourceAndStatusIfFinalizerSet() { + void patchesBothResourceAndStatusIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); - reconciler.reconcile = (r, c) -> UpdateControl.updateResourceAndStatus(testCustomResource); - when(customResourceFacade.patchResource(testCustomResource)) + reconciler.reconcile = (r, c) -> UpdateControl.patchResourceAndStatus(testCustomResource); + when(customResourceFacade.patchResource(testCustomResource, any())) .thenReturn(testCustomResource); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchResource(testCustomResource); - verify(customResourceFacade, times(1)).updateStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); + verify(customResourceFacade, times(1)).patchStatus(testCustomResource, any()); } @Test @@ -176,8 +176,7 @@ void patchesStatus() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); - verify(customResourceFacade, never()).updateStatus(any()); - verify(customResourceFacade, never()).patchResource(any()); + verify(customResourceFacade, never()).patchResource(any(), any()); } @Test @@ -209,7 +208,7 @@ void removesDefaultFinalizerOnDeleteIfSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); } @Test @@ -218,7 +217,7 @@ void retriesFinalizerRemovalWithFreshResource() { markForDeletion(testCustomResource); var resourceWithFinalizer = TestUtils.testCustomResource(); resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); - when(customResourceFacade.patchResource(testCustomResource)) + when(customResourceFacade.patchResource(testCustomResource, any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())).thenReturn(resourceWithFinalizer); @@ -227,7 +226,7 @@ void retriesFinalizerRemovalWithFreshResource() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(2)).patchResource(any()); + verify(customResourceFacade, times(2)).patchResource(any(), any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -237,7 +236,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { // of the finalizer removal testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any())) + when(customResourceFacade.patchResource(any(), any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())).thenReturn(null); @@ -245,7 +244,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(testCustomResource); + verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -253,7 +252,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { void throwsExceptionIfFinalizerRemovalRetryExceeded() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any())) + when(customResourceFacade.patchResource(any(), any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())) .thenAnswer((Answer) invocationOnMock -> createResourceWithFinalizer()); @@ -265,7 +264,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { assertThat(postExecControl.getRuntimeException()).isPresent(); assertThat(postExecControl.getRuntimeException().get()) .isInstanceOf(OperatorException.class); - verify(customResourceFacade, times(MAX_UPDATE_RETRY)).patchResource(any()); + verify(customResourceFacade, times(MAX_UPDATE_RETRY)).patchResource(any(), any()); verify(customResourceFacade, times(MAX_UPDATE_RETRY - 1)).getResource(any(), any()); } @@ -274,7 +273,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any())) + when(customResourceFacade.patchResource(any(), any())) .thenThrow(new KubernetesClientException(null, 400, null)); var res = @@ -282,7 +281,7 @@ void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { assertThat(res.getRuntimeException()).isPresent(); assertThat(res.getRuntimeException().get()).isInstanceOf(KubernetesClientException.class); - verify(customResourceFacade, times(1)).patchResource(any()); + verify(customResourceFacade, times(1)).patchResource(any(), any()); verify(customResourceFacade, never()).getResource(any(), any()); } @@ -326,7 +325,7 @@ void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, never()).patchResource(any()); + verify(customResourceFacade, never()).patchResource(any(), any()); } @Test @@ -336,22 +335,22 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, never()).patchResource(any()); - verify(customResourceFacade, never()).patchStatus(testCustomResource); + verify(customResourceFacade, never()).patchResource(any(), any()); + verify(customResourceFacade, never()).patchStatus(testCustomResource,any()); } @Test void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.patchResource(any())) + when(customResourceFacade.patchResource(any(),any())) .thenReturn(testCustomResource); var postExecControl = reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, times(1)).patchResource(any()); + verify(customResourceFacade, times(1)).patchResource(any(),any()); assertThat(postExecControl.updateIsStatusPatch()).isFalse(); assertThat(postExecControl.getUpdatedCustomResource()).isPresent(); } @@ -363,7 +362,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, never()).patchResource(any()); + verify(customResourceFacade, never()).patchResource(any(),any()); verify(reconciler, never()).cleanup(eq(testCustomResource), any()); } @@ -466,7 +465,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.noUpdate()); - when(facade.updateStatus(observedGenResource)).thenReturn(observedGenResource); + when(facade.patchResource(observedGenResource,any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -477,7 +476,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { } @Test - void updateObservedGenerationOnCustomResourceUpdate() throws Exception { + void pacchObservedGenerationOnCustomResourceUpdate() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); @@ -485,9 +484,9 @@ void updateObservedGenerationOnCustomResourceUpdate() throws Exception { CustomResourceFacade facade = mock(CustomResourceFacade.class); when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) - .thenReturn(UpdateControl.updateResource(observedGenResource)); - when(facade.patchResource(any())).thenReturn(observedGenResource); - when(facade.updateStatus(observedGenResource)).thenReturn(observedGenResource); + .thenReturn(UpdateControl.patchResource(observedGenResource)); + when(facade.patchResource(any(),any())).thenReturn(observedGenResource); + when(facade.patchStatus(observedGenResource,any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -506,7 +505,7 @@ void callErrorStatusHandlerIfImplemented() { }; reconciler.errorHandler = (r, ri, e) -> { testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE); - return ErrorStatusUpdateControl.updateStatus(testCustomResource); + return ErrorStatusUpdateControl.patchStatus(testCustomResource); }; reconciliationDispatcher.handleExecution( @@ -523,7 +522,7 @@ public boolean isLastAttempt() { } }).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).updateStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } @@ -537,12 +536,12 @@ void callErrorStatusHandlerEvenOnFirstError() { }; reconciler.errorHandler = (r, ri, e) -> { testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE); - return ErrorStatusUpdateControl.updateStatus(testCustomResource); + return ErrorStatusUpdateControl.patchStatus(testCustomResource); }; var postExecControl = reconciliationDispatcher.handleExecution( new ExecutionScope(null).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).updateStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); assertThat(postExecControl.exceptionDuringExecution()).isTrue(); @@ -556,7 +555,7 @@ void errorHandlerCanInstructNoRetryWithUpdate() { }; reconciler.errorHandler = (r, ri, e) -> { testCustomResource.getStatus().setConfigMapStatus(ERROR_MESSAGE); - return ErrorStatusUpdateControl.updateStatus(testCustomResource).withNoRetry(); + return ErrorStatusUpdateControl.patchStatus(testCustomResource).withNoRetry(); }; var postExecControl = reconciliationDispatcher.handleExecution( @@ -564,7 +563,7 @@ void errorHandlerCanInstructNoRetryWithUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(1)).updateStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } @@ -584,7 +583,7 @@ void errorHandlerCanInstructNoRetryNoUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(0)).updateStatus(testCustomResource); + verify(customResourceFacade, times(0)).patchStatus(testCustomResource,any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } @@ -650,7 +649,7 @@ void canSkipSchedulingMaxDelayIf() { void retriesAddingFinalizer() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.patchResource(any())) + when(customResourceFacade.patchResource(any(),any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())) @@ -661,7 +660,7 @@ void retriesAddingFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(2)).patchResource(any()); + verify(customResourceFacade, times(2)).patchResource(any(),any()); } @Test From d22e2d0d6575ff3ad9334ba2e6c10a5084a1a1fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 25 Mar 2024 16:23:16 +0100 Subject: [PATCH 08/28] builds now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/reconciler/UpdateControl.java | 2 +- .../event/ReconciliationDispatcherTest.java | 38 +++++++------------ .../operator/ReconcilerExecutorIT.java | 1 - .../ComplexDependentReconciler.java | 2 +- .../DoubleUpdateTestCustomReconciler.java | 2 +- .../ErrorStatusHandlerTestReconciler.java | 2 +- .../sample/simple/TestReconciler.java | 18 +-------- .../StatusUpdateLockingReconciler.java | 2 +- .../SubResourceTestCustomReconciler.java | 2 +- .../sample/LeaderElectionTestReconciler.java | 2 +- .../sample/MySQLSchemaReconciler.java | 2 +- .../operator/sample/Utils.java | 2 +- 12 files changed, 24 insertions(+), 51 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index ae8cbd2343..1395a34b74 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -37,7 +37,7 @@ private UpdateControl( * @return UpdateControl instance */ public static UpdateControl patchStatus(T customResource) { - return new UpdateControl<>(customResource, false, false); + return new UpdateControl<>(customResource, false, true); } public static UpdateControl patchResource(T customResource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 6b70fe6b9b..6c6d1669f1 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -141,30 +141,18 @@ void callCreateOrUpdateOnNewResourceIfFinalizerSet() { .reconcile(ArgumentMatchers.eq(testCustomResource), any()); } - @Test - void patchesOnlyStatusSubResourceIfFinalizerSet() { - testCustomResource.addFinalizer(DEFAULT_FINALIZER); - - reconciler.reconcile = (r, c) -> UpdateControl.patchStatus(testCustomResource); - - reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); - verify(customResourceFacade, never()).patchResource(any(),any()); - } - @Test void patchesBothResourceAndStatusIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.patchResourceAndStatus(testCustomResource); - when(customResourceFacade.patchResource(testCustomResource, any())) + when(customResourceFacade.patchResource(eq(testCustomResource), any())) .thenReturn(testCustomResource); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource, any()); + verify(customResourceFacade, times(1)).patchResource(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); } @Test @@ -208,7 +196,7 @@ void removesDefaultFinalizerOnDeleteIfSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); + verify(customResourceFacade, times(1)).patchResource(eq(testCustomResource), any()); } @Test @@ -217,7 +205,7 @@ void retriesFinalizerRemovalWithFreshResource() { markForDeletion(testCustomResource); var resourceWithFinalizer = TestUtils.testCustomResource(); resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); - when(customResourceFacade.patchResource(testCustomResource, any())) + when(customResourceFacade.patchResource(eq(testCustomResource), any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())).thenReturn(resourceWithFinalizer); @@ -244,7 +232,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(testCustomResource, any()); + verify(customResourceFacade, times(1)).patchResource(eq(testCustomResource), any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -336,7 +324,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, never()).patchResource(any(), any()); - verify(customResourceFacade, never()).patchStatus(testCustomResource,any()); + verify(customResourceFacade, never()).patchStatus(eq(testCustomResource),any()); } @Test @@ -465,7 +453,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.noUpdate()); - when(facade.patchResource(observedGenResource,any())).thenReturn(observedGenResource); + when(facade.patchResource(eq(observedGenResource),any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -486,7 +474,7 @@ void pacchObservedGenerationOnCustomResourceUpdate() throws Exception { when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.patchResource(observedGenResource)); when(facade.patchResource(any(),any())).thenReturn(observedGenResource); - when(facade.patchStatus(observedGenResource,any())).thenReturn(observedGenResource); + when(facade.patchStatus(eq(observedGenResource),any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -522,7 +510,7 @@ public boolean isLastAttempt() { } }).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } @@ -541,7 +529,7 @@ void callErrorStatusHandlerEvenOnFirstError() { var postExecControl = reconciliationDispatcher.handleExecution( new ExecutionScope(null).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); assertThat(postExecControl.exceptionDuringExecution()).isTrue(); @@ -563,7 +551,7 @@ void errorHandlerCanInstructNoRetryWithUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource,any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } @@ -583,7 +571,7 @@ void errorHandlerCanInstructNoRetryNoUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(0)).patchStatus(testCustomResource,any()); + verify(customResourceFacade, times(0)).patchStatus(eq(testCustomResource),any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ReconcilerExecutorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ReconcilerExecutorIT.java index 07a022adb1..476bd842a5 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ReconcilerExecutorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ReconcilerExecutorIT.java @@ -35,7 +35,6 @@ void configMapGetsCreatedForTestCustomResource() { @Test void patchesStatusForTestCustomResource() { - operator.getReconcilerOfType(TestReconciler.class).setPatchStatus(true); operator.getReconcilerOfType(TestReconciler.class).setUpdateStatus(true); TestCustomResource resource = TestUtils.testCustomResource(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/complexdependent/ComplexDependentReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/complexdependent/ComplexDependentReconciler.java index 81bcb7e153..2f1c272ce9 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/complexdependent/ComplexDependentReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/complexdependent/ComplexDependentReconciler.java @@ -47,7 +47,7 @@ public UpdateControl reconcile( status.setStatus(ready ? RECONCILE_STATUS.READY : RECONCILE_STATUS.NOT_READY); resource.setStatus(status); - return UpdateControl.updateStatus(resource); + return UpdateControl.patchStatus(resource); } @Override diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java index 11f0a54f3e..196fa4cb6d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java @@ -34,7 +34,7 @@ public UpdateControl reconcile( ensureStatusExists(resource); resource.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); - return UpdateControl.updateResourceAndStatus(resource); + return UpdateControl.patchResourceAndStatus(resource); } private void ensureStatusExists(DoubleUpdateTestCustomResource resource) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java index 412a784b7c..4abf982e0f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/errorstatushandler/ErrorStatusHandlerTestReconciler.java @@ -51,6 +51,6 @@ public ErrorStatusUpdateControl updateErro ensureStatusExists(resource); resource.getStatus().getMessages() .add(ERROR_STATUS_MESSAGE + context.getRetryInfo().orElseThrow().getAttemptCount()); - return ErrorStatusUpdateControl.updateStatus(resource); + return ErrorStatusUpdateControl.patchStatus(resource); } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java index fea06bba93..0bc523d51d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java @@ -27,20 +27,10 @@ public class TestReconciler private final AtomicInteger numberOfExecutions = new AtomicInteger(0); private final AtomicInteger numberOfCleanupExecutions = new AtomicInteger(0); private volatile boolean updateStatus; - private volatile boolean patchStatus; - public TestReconciler(boolean updateStatus) { - this(updateStatus, false); - } - public TestReconciler(boolean updateStatus, boolean patchStatus) { + public TestReconciler(boolean updateStatus) { this.updateStatus = updateStatus; - this.patchStatus = patchStatus; - } - - public TestReconciler setPatchStatus(boolean patchStatus) { - this.patchStatus = patchStatus; - return this; } public void setUpdateStatus(boolean updateStatus) { @@ -119,11 +109,7 @@ public UpdateControl reconcile( } resource.getStatus().setConfigMapStatus("ConfigMap Ready"); } - if (patchStatus) { - return UpdateControl.patchStatus(resource); - } else { - return UpdateControl.updateStatus(resource); - } + return UpdateControl.patchStatus(resource); } private Map configMapData(TestCustomResource resource) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java index e21897cf38..1fe376d8da 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java @@ -19,7 +19,7 @@ public UpdateControl reconcile( numberOfExecutions.addAndGet(1); Thread.sleep(WAIT_TIME); resource.getStatus().setValue(resource.getStatus().getValue() + 1); - return UpdateControl.updateStatus(resource); + return UpdateControl.patchStatus(resource); } public int getNumberOfExecutions() { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java index 1e76681f25..b50b9fc4b5 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java @@ -33,7 +33,7 @@ public UpdateControl reconcile( ensureStatusExists(resource); resource.getStatus().setState(SubResourceTestCustomResourceStatus.State.SUCCESS); waitXms(RECONCILER_MIN_EXEC_TIME); - return UpdateControl.updateStatus(resource); + return UpdateControl.patchStatus(resource); } private void ensureStatusExists(SubResourceTestCustomResource resource) { diff --git a/sample-operators/leader-election/src/main/java/io/javaoperatorsdk/operator/sample/LeaderElectionTestReconciler.java b/sample-operators/leader-election/src/main/java/io/javaoperatorsdk/operator/sample/LeaderElectionTestReconciler.java index 36fb3f2ef8..1e54ddd915 100644 --- a/sample-operators/leader-election/src/main/java/io/javaoperatorsdk/operator/sample/LeaderElectionTestReconciler.java +++ b/sample-operators/leader-election/src/main/java/io/javaoperatorsdk/operator/sample/LeaderElectionTestReconciler.java @@ -34,7 +34,7 @@ public UpdateControl reconcile( resource.getStatus().getReconciledBy().add(reconcilerName); // update status is with optimistic locking - return UpdateControl.updateStatus(resource).rescheduleAfter(Duration.ofSeconds(1)); + return UpdateControl.patchStatus(resource).rescheduleAfter(Duration.ofSeconds(1)); } } diff --git a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java index 1a4b704591..f75ac32ee7 100644 --- a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java +++ b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java @@ -50,7 +50,7 @@ public ErrorStatusUpdateControl updateErrorStatus(MySQLSchema schem status.setSecretName(null); status.setStatus("ERROR: " + e.getMessage()); schema.setStatus(status); - return ErrorStatusUpdateControl.updateStatus(schema); + return ErrorStatusUpdateControl.patchStatus(schema); } diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java index b37b98aa52..82cb3a86a4 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java @@ -33,7 +33,7 @@ public static String serviceName(WebPage webPage) { public static ErrorStatusUpdateControl handleError(WebPage resource, Exception e) { resource.getStatus().setErrorMessage("Error: " + e.getMessage()); - return ErrorStatusUpdateControl.updateStatus(resource); + return ErrorStatusUpdateControl.patchStatus(resource); } public static void simulateErrorIfRequested(WebPage webPage) throws ErrorSimulationException { From 7146d9b1748781fe0d75f0dfd88fe367aee3fac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 25 Mar 2024 17:33:22 +0100 Subject: [PATCH 09/28] unit tests pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../processing/event/ReconciliationDispatcherTest.java | 4 ++-- .../ControllerConfigurationAnnotationProcessorTest.java | 2 +- .../test/resources/compile-fixtures/MultilevelReconciler.java | 2 +- .../compile-fixtures/ReconcilerImplemented2Interfaces.java | 2 +- .../ReconcilerImplementedIntermediateAbstractClass.java | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 6c6d1669f1..1ceb4c6820 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -453,7 +453,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.noUpdate()); - when(facade.patchResource(eq(observedGenResource),any())).thenReturn(observedGenResource); + when(facade.patchStatus(any(),any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -464,7 +464,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { } @Test - void pacchObservedGenerationOnCustomResourceUpdate() throws Exception { + void patchObservedGenerationOnCustomResourceUpdate() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java index ce9637af9e..a7365c19b9 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java @@ -33,7 +33,7 @@ public void generateCorrectDoneableClassIfThereIsAbstractBaseController() { } @Test - public void generateDoneableClasswithMultilevelHierarchy() { + public void generateDoneableClassWithMultilevelHierarchy() { Compilation compilation = Compiler.javac() .withProcessors(new ControllerConfigurationAnnotationProcessor()) diff --git a/operator-framework/src/test/resources/compile-fixtures/MultilevelReconciler.java b/operator-framework/src/test/resources/compile-fixtures/MultilevelReconciler.java index acea0a0db2..254d211bd0 100644 --- a/operator-framework/src/test/resources/compile-fixtures/MultilevelReconciler.java +++ b/operator-framework/src/test/resources/compile-fixtures/MultilevelReconciler.java @@ -17,7 +17,7 @@ public static class MyCustomResource extends CustomResource { public UpdateControl reconcile( MultilevelReconciler.MyCustomResource customResource, Context context) { - return UpdateControl.updateResource(null); + return UpdateControl.patchResource(null); } public DeleteControl cleanup(MultilevelReconciler.MyCustomResource customResource, diff --git a/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplemented2Interfaces.java b/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplemented2Interfaces.java index 0adc9aee09..bd1ba773be 100644 --- a/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplemented2Interfaces.java +++ b/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplemented2Interfaces.java @@ -14,7 +14,7 @@ public static class MyCustomResource extends CustomResource { @Override public UpdateControl reconcile(MyCustomResource customResource, Context context) { - return UpdateControl.updateResource(null); + return UpdateControl.patchResource(null); } @Override diff --git a/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplementedIntermediateAbstractClass.java b/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplementedIntermediateAbstractClass.java index b95495a614..ee291cf9ce 100644 --- a/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplementedIntermediateAbstractClass.java +++ b/operator-framework/src/test/resources/compile-fixtures/ReconcilerImplementedIntermediateAbstractClass.java @@ -13,7 +13,7 @@ public class ReconcilerImplementedIntermediateAbstractClass extends public UpdateControl reconcile( AbstractReconciler.MyCustomResource customResource, Context context) { - return UpdateControl.updateResource(null); + return UpdateControl.patchResource(null); } public DeleteControl cleanup(AbstractReconciler.MyCustomResource customResource, From dec7c8fd2912102e13e6d59bcaad4c4ae6fce774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 25 Mar 2024 20:51:46 +0100 Subject: [PATCH 10/28] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 6 +-- .../api/reconciler/UpdateControl.java | 6 +-- .../event/ReconciliationDispatcher.java | 43 +++++++++++-------- .../event/ReconciliationDispatcherTest.java | 29 +++++++------ 4 files changed, 45 insertions(+), 39 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index f5d3fe503a..7b42b1a88f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -376,9 +376,9 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() { } /** - * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can either use - * simple patches or SSA. Setting this to true, controller will use SSA for adding finalizers, - * managing observed generation, patching resources and status. + * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can + * either use simple patches or SSA. Setting this to true, controller will use SSA for adding + * finalizers, managing observed generation, patching resources and status. * * @return true by default */ diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index 1395a34b74..2d49488c83 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -1,10 +1,10 @@ package io.javaoperatorsdk.operator.api.reconciler; +import java.util.Optional; + import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.CustomResource; -import java.util.Optional; - public class UpdateControl

extends BaseControl> { private final P resource; @@ -12,7 +12,7 @@ public class UpdateControl

extends BaseControl handleDispatch(ExecutionScope

executionScope) Context

context = new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource); if (markedForDeletion) { - return handleCleanup(resourceForExecution, originalResource,context); + return handleCleanup(resourceForExecution, originalResource, context); } else { return handleReconcile(executionScope, resourceForExecution, originalResource, context); } @@ -157,13 +157,15 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc toUpdate = updateControl.getResource().orElseThrow(); } } else { - toUpdate = updateControl.isNoUpdate() ? originalResource : updateControl.getResource().orElseThrow(); + toUpdate = + updateControl.isNoUpdate() ? originalResource : updateControl.getResource().orElseThrow(); } if (updateControl.isPatchResource()) { updatedCustomResource = patchResource(toUpdate, originalResource); if (!useSSA) { - toUpdate.getMetadata().setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); + toUpdate.getMetadata() + .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); } } @@ -206,12 +208,14 @@ public boolean isLastAttempt() { P updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { updatedResource = - patchStatusGenerationAware(errorStatusUpdateControl.getResource().orElseThrow(), originalResource); + patchStatusGenerationAware(errorStatusUpdateControl.getResource().orElseThrow(), + originalResource); } if (errorStatusUpdateControl.isNoRetry()) { PostExecutionControl

postExecutionControl; if (updatedResource != null) { - postExecutionControl = PostExecutionControl.customResourceStatusPatched(updatedResource); + postExecutionControl = + PostExecutionControl.customResourceStatusPatched(updatedResource); } else { postExecutionControl = PostExecutionControl.defaultDispatch(); } @@ -267,8 +271,8 @@ private PostExecutionControl

createPostExecutionControl(P updatedCustomResour UpdateControl

updateControl) { PostExecutionControl

postExecutionControl; if (updatedCustomResource != null) { - postExecutionControl = - PostExecutionControl.customResourceStatusPatched(updatedCustomResource); + postExecutionControl = + PostExecutionControl.customResourceStatusPatched(updatedCustomResource); } else { postExecutionControl = PostExecutionControl.defaultDispatch(); } @@ -283,7 +287,7 @@ private void updatePostExecutionControlWithReschedule( } private PostExecutionControl

handleCleanup(P resource, - P originalResource, Context

context) { + P originalResource, Context

context) { if (log.isDebugEnabled()) { log.debug( "Executing delete for resource: {} with version: {}", @@ -297,7 +301,7 @@ private PostExecutionControl

handleCleanup(P resource, // cleanup is finished, nothing left to done final var finalizerName = configuration().getFinalizerName(); if (deleteControl.isRemoveFinalizer() && resource.hasFinalizer(finalizerName)) { - P customResource = conflictRetryingPatch(resource,originalResource, r -> { + P customResource = conflictRetryingPatch(resource, originalResource, r -> { // the operator might not be allowed to retrieve the resource on a retry, e.g. when its // permissions are removed by deleting the namespace concurrently if (r == null) { @@ -346,7 +350,7 @@ private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalRe log.debug( "Adding finalizer for resource: {} version: {}", getUID(originalResource), getVersion(originalResource)); - return conflictRetryingPatch(resourceForExecution,originalResource, + return conflictRetryingPatch(resourceForExecution, originalResource, r -> r.addFinalizer(configuration().getFinalizerName())); } @@ -357,18 +361,19 @@ private P patchResource(P resource, P originalResource) { // todo unit test if (useSSA && controller.useFinalizer() && - !resource.getMetadata().getFinalizers().contains(configuration().getFinalizerName())) { - resource.getMetadata().getFinalizers().add(configuration().getFinalizerName()); + !resource.getMetadata().getFinalizers().contains(configuration().getFinalizerName())) { + resource.getMetadata().getFinalizers().add(configuration().getFinalizerName()); } - return customResourceFacade.patchResource(resource,originalResource); + return customResourceFacade.patchResource(resource, originalResource); } ControllerConfiguration

configuration() { return controller.getConfiguration(); } - public P conflictRetryingPatch(P resource, P originalResource, Function modificationFunction) { + public P conflictRetryingPatch(P resource, P originalResource, + Function modificationFunction) { if (log.isDebugEnabled()) { log.debug("Conflict retrying update for: {}", ResourceID.fromResource(resource)); } @@ -379,7 +384,7 @@ public P conflictRetryingPatch(P resource, P originalResource, Functionresource); + return resource(originalResource).edit(r -> resource); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index 1ceb4c6820..f63ec82552 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -129,7 +129,8 @@ void addFinalizerOnNewResource() { .reconcile(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) .patchResource( - argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)),any()); + argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)), + any()); assertThat(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)).isTrue(); } @@ -324,21 +325,21 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(customResourceFacade, never()).patchResource(any(), any()); - verify(customResourceFacade, never()).patchStatus(eq(testCustomResource),any()); + verify(customResourceFacade, never()).patchStatus(eq(testCustomResource), any()); } @Test void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.patchResource(any(),any())) + when(customResourceFacade.patchResource(any(), any())) .thenReturn(testCustomResource); var postExecControl = reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, times(1)).patchResource(any(),any()); + verify(customResourceFacade, times(1)).patchResource(any(), any()); assertThat(postExecControl.updateIsStatusPatch()).isFalse(); assertThat(postExecControl.getUpdatedCustomResource()).isPresent(); } @@ -350,7 +351,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, never()).patchResource(any(),any()); + verify(customResourceFacade, never()).patchResource(any(), any()); verify(reconciler, never()).cleanup(eq(testCustomResource), any()); } @@ -453,7 +454,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.noUpdate()); - when(facade.patchStatus(any(),any())).thenReturn(observedGenResource); + when(facade.patchStatus(any(), any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -473,8 +474,8 @@ void patchObservedGenerationOnCustomResourceUpdate() throws Exception { 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); + when(facade.patchResource(any(), any())).thenReturn(observedGenResource); + when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource); var dispatcher = init(observedGenResource, reconciler, config, facade, true); PostExecutionControl control = dispatcher.handleExecution( @@ -510,7 +511,7 @@ public boolean isLastAttempt() { } }).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } @@ -529,7 +530,7 @@ void callErrorStatusHandlerEvenOnFirstError() { var postExecControl = reconciliationDispatcher.handleExecution( new ExecutionScope(null).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); assertThat(postExecControl.exceptionDuringExecution()).isTrue(); @@ -551,7 +552,7 @@ void errorHandlerCanInstructNoRetryWithUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource),any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } @@ -571,7 +572,7 @@ void errorHandlerCanInstructNoRetryNoUpdate() { verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); - verify(customResourceFacade, times(0)).patchStatus(eq(testCustomResource),any()); + verify(customResourceFacade, times(0)).patchStatus(eq(testCustomResource), any()); assertThat(postExecControl.exceptionDuringExecution()).isFalse(); } @@ -637,7 +638,7 @@ void canSkipSchedulingMaxDelayIf() { void retriesAddingFinalizer() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.patchResource(any(),any())) + when(customResourceFacade.patchResource(any(), any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())) @@ -648,7 +649,7 @@ void retriesAddingFinalizer() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(2)).patchResource(any(),any()); + verify(customResourceFacade, times(2)).patchResource(any(), any()); } @Test From 5e28d7d6a32e263657948afd113cde406d25e115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 26 Mar 2024 15:10:15 +0100 Subject: [PATCH 11/28] finalizer removal with patch without SSA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcher.java | 17 +++++++++++++---- .../event/ReconciliationDispatcherTest.java | 18 +++++++++--------- .../sample/WebPageOperatorAbstractTest.java | 2 +- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 7d9f605b71..421787d4a1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -312,7 +312,7 @@ private PostExecutionControl

handleCleanup(P resource, return false; } return r.removeFinalizer(finalizerName); - }); + },true); return PostExecutionControl.customResourceFinalizerRemoved(customResource); } } @@ -351,7 +351,7 @@ private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalRe "Adding finalizer for resource: {} version: {}", getUID(originalResource), getVersion(originalResource)); return conflictRetryingPatch(resourceForExecution, originalResource, - r -> r.addFinalizer(configuration().getFinalizerName())); + r -> r.addFinalizer(configuration().getFinalizerName()), false); } private P patchResource(P resource, P originalResource) { @@ -373,7 +373,7 @@ ControllerConfiguration

configuration() { } public P conflictRetryingPatch(P resource, P originalResource, - Function modificationFunction) { + Function modificationFunction, boolean forceNotUseSSA) { if (log.isDebugEnabled()) { log.debug("Conflict retrying update for: {}", ResourceID.fromResource(resource)); } @@ -384,7 +384,11 @@ public P conflictRetryingPatch(P resource, P originalResource, if (Boolean.FALSE.equals(modified)) { return resource; } - return customResourceFacade.patchResource(resource, originalResource); + if (forceNotUseSSA) { + return customResourceFacade.patchResourceWithoutSSA(resource,originalResource); + } else { + return customResourceFacade.patchResource(resource, originalResource); + } } catch (KubernetesClientException e) { log.trace("Exception during patch for resource: {}", resource); retryIndex++; @@ -428,6 +432,10 @@ public R getResource(String namespace, String name) { } } + public R patchResourceWithoutSSA(R resource, R originalResource) { + return resource(originalResource).edit(r -> resource); + } + public R patchResource(R resource, R originalResource) { if (log.isDebugEnabled()) { log.debug( @@ -475,6 +483,7 @@ public R patchStatus(R resource, R originalResource) { public R patchResourceWithSSA(R resource) { var managedFields = resource.getMetadata().getManagedFields(); + resource.getMetadata().setManagedFields(null); try { return resource(resource).patch(new PatchContext.Builder() .withFieldManager(fieldManager) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index f63ec82552..fc62f85734 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -197,7 +197,7 @@ void removesDefaultFinalizerOnDeleteIfSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchResourceWithoutSSA(eq(testCustomResource), any()); } @Test @@ -206,7 +206,7 @@ void retriesFinalizerRemovalWithFreshResource() { markForDeletion(testCustomResource); var resourceWithFinalizer = TestUtils.testCustomResource(); resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); - when(customResourceFacade.patchResource(eq(testCustomResource), any())) + when(customResourceFacade.patchResourceWithoutSSA(eq(testCustomResource), any())) .thenThrow(new KubernetesClientException(null, 409, null)) .thenReturn(testCustomResource); when(customResourceFacade.getResource(any(), any())).thenReturn(resourceWithFinalizer); @@ -215,7 +215,7 @@ void retriesFinalizerRemovalWithFreshResource() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(2)).patchResource(any(), any()); + verify(customResourceFacade, times(2)).patchResourceWithoutSSA(any(), any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -225,7 +225,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { // of the finalizer removal testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any(), any())) + when(customResourceFacade.patchResourceWithoutSSA(any(), any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())).thenReturn(null); @@ -233,7 +233,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); assertThat(postExecControl.isFinalizerRemoved()).isTrue(); - verify(customResourceFacade, times(1)).patchResource(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchResourceWithoutSSA(eq(testCustomResource), any()); verify(customResourceFacade, times(1)).getResource(any(), any()); } @@ -241,7 +241,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { void throwsExceptionIfFinalizerRemovalRetryExceeded() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any(), any())) + when(customResourceFacade.patchResourceWithoutSSA(any(), any())) .thenThrow(new KubernetesClientException(null, 409, null)); when(customResourceFacade.getResource(any(), any())) .thenAnswer((Answer) invocationOnMock -> createResourceWithFinalizer()); @@ -253,7 +253,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { assertThat(postExecControl.getRuntimeException()).isPresent(); assertThat(postExecControl.getRuntimeException().get()) .isInstanceOf(OperatorException.class); - verify(customResourceFacade, times(MAX_UPDATE_RETRY)).patchResource(any(), any()); + verify(customResourceFacade, times(MAX_UPDATE_RETRY)).patchResourceWithoutSSA(any(), any()); verify(customResourceFacade, times(MAX_UPDATE_RETRY - 1)).getResource(any(), any()); } @@ -262,7 +262,7 @@ void throwsExceptionIfFinalizerRemovalRetryExceeded() { void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); markForDeletion(testCustomResource); - when(customResourceFacade.patchResource(any(), any())) + when(customResourceFacade.patchResourceWithoutSSA(any(), any())) .thenThrow(new KubernetesClientException(null, 400, null)); var res = @@ -270,7 +270,7 @@ void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { assertThat(res.getRuntimeException()).isPresent(); assertThat(res.getRuntimeException().get()).isInstanceOf(KubernetesClientException.class); - verify(customResourceFacade, times(1)).patchResource(any(), any()); + verify(customResourceFacade, times(1)).patchResourceWithoutSSA(any(), any()); verify(customResourceFacade, never()).getResource(any(), any()); } diff --git a/sample-operators/webpage/src/test/java/io/javaoperatorsdk/operator/sample/WebPageOperatorAbstractTest.java b/sample-operators/webpage/src/test/java/io/javaoperatorsdk/operator/sample/WebPageOperatorAbstractTest.java index 040b3b2f8f..6c9a5512bb 100644 --- a/sample-operators/webpage/src/test/java/io/javaoperatorsdk/operator/sample/WebPageOperatorAbstractTest.java +++ b/sample-operators/webpage/src/test/java/io/javaoperatorsdk/operator/sample/WebPageOperatorAbstractTest.java @@ -37,7 +37,7 @@ public abstract class WebPageOperatorAbstractTest { public static final String TEST_PAGE = "test-page"; public static final String TITLE1 = "Hello Operator World"; public static final String TITLE2 = "Hello Operator World Title 2"; - public static final int WAIT_SECONDS = 20; + public static final int WAIT_SECONDS = 360; public static final int LONG_WAIT_SECONDS = 120; public static final Duration POLL_INTERVAL = Duration.ofSeconds(1); From 6719270295eaeb33d62483306477b70ca7161615 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 26 Mar 2024 15:13:03 +0100 Subject: [PATCH 12/28] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/processing/event/ReconciliationDispatcher.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 421787d4a1..254021d2ee 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -312,7 +312,7 @@ private PostExecutionControl

handleCleanup(P resource, return false; } return r.removeFinalizer(finalizerName); - },true); + }, true); return PostExecutionControl.customResourceFinalizerRemoved(customResource); } } @@ -385,7 +385,7 @@ public P conflictRetryingPatch(P resource, P originalResource, return resource; } if (forceNotUseSSA) { - return customResourceFacade.patchResourceWithoutSSA(resource,originalResource); + return customResourceFacade.patchResourceWithoutSSA(resource, originalResource); } else { return customResourceFacade.patchResource(resource, originalResource); } From a201d1e71b43f79fa8dfdfc5926a04e64146393c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 27 Mar 2024 14:38:16 +0100 Subject: [PATCH 13/28] patch related changes + IT fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../processing/event/EventProcessor.java | 12 ---------- .../event/ReconciliationDispatcher.java | 1 - .../processing/event/EventProcessorTest.java | 17 +------------- .../operator/StatusUpdateLockingIT.java | 22 +++++++++++-------- .../sample/simple/TestReconciler.java | 12 ++++++---- .../StatusUpdateLockingCustomResource.java | 4 ---- .../StatusUpdateLockingReconciler.java | 1 + 7 files changed, 23 insertions(+), 46 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index 53d15340e8..32d80dc0f3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -245,18 +245,6 @@ synchronized void eventProcessingFinished( state.markProcessedMarkForDeletion(); metrics.cleanupDoneFor(resourceID, metricsMetadata); } else { - postExecutionControl - .getUpdatedCustomResource() - .ifPresent( - p -> { - // todo check - if (!postExecutionControl.updateIsStatusPatch()) { - eventSourceManager - .getControllerResourceEventSource() - .handleRecentResourceUpdate( - ResourceID.fromResource(p), p, executionScope.getResource()); - } - }); if (state.eventPresent()) { submitReconciliationExecution(state); } else { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 254021d2ee..a1e882c644 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -453,7 +453,6 @@ public R patchResource(R resource, R originalResource) { public R patchStatus(R resource, R originalResource) { log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA); String resourceVersion = resource.getMetadata().getResourceVersion(); - // don't do optimistic locking on patch originalResource.getMetadata().setResourceVersion(null); resource.getMetadata().setResourceVersion(null); try { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java index 07f90764da..4e37b09e50 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java @@ -286,22 +286,7 @@ void startProcessedMarkedEventReceivedBefore() { verify(reconciliationDispatcherMock, timeout(100).times(1)).handleExecution(any()); verify(metricsMock, times(1)).reconcileCustomResource(any(HasMetadata.class), isNull(), any()); } - - @Test - void updatesEventSourceHandlerIfResourceUpdated() { - TestCustomResource customResource = testCustomResource(); - ExecutionScope executionScope = - new ExecutionScope(null).setResource(customResource); - PostExecutionControl postExecutionControl = - PostExecutionControl.customResourcePatched(customResource); - - eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); - - - verify(controllerResourceEventSourceMock, times(1)).handleRecentResourceUpdate(any(), any(), - any()); - } - + @Test void notUpdatesEventSourceHandlerIfResourceUpdated() { TestCustomResource customResource = testCustomResource(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusUpdateLockingIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusUpdateLockingIT.java index 147c0403c3..e03883d8db 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusUpdateLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusUpdateLockingIT.java @@ -21,23 +21,27 @@ class StatusUpdateLockingIT { @RegisterExtension LocallyRunOperatorExtension operator = - LocallyRunOperatorExtension.builder().withReconciler(StatusUpdateLockingReconciler.class) + LocallyRunOperatorExtension.builder() + .withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false)) + .withReconciler(StatusUpdateLockingReconciler.class) .build(); @Test - void optimisticLockingDoneOnStatusUpdate() throws InterruptedException { + void noOptimisticLockingDoneOnStatusPatch() throws InterruptedException { var resource = operator.create(createResource()); Thread.sleep(WAIT_TIME / 2); resource.getMetadata().setAnnotations(Map.of("key", "value")); operator.replace(resource); - await().pollDelay(Duration.ofMillis(WAIT_TIME)).untilAsserted(() -> { - assertThat( - operator.getReconcilerOfType(StatusUpdateLockingReconciler.class).getNumberOfExecutions()) - .isEqualTo(2); - assertThat(operator.get(StatusUpdateLockingCustomResource.class, TEST_RESOURCE_NAME) - .getStatus().getValue()).isEqualTo(1); - }); + await().pollDelay(Duration.ofMillis(WAIT_TIME)).timeout(Duration.ofSeconds(460)) + .untilAsserted(() -> { + assertThat( + operator.getReconcilerOfType(StatusUpdateLockingReconciler.class) + .getNumberOfExecutions()) + .isEqualTo(1); + assertThat(operator.get(StatusUpdateLockingCustomResource.class, TEST_RESOURCE_NAME) + .getStatus().getValue()).isEqualTo(1); + }); } StatusUpdateLockingCustomResource createResource() { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java index 0bc523d51d..4ff4521d00 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java @@ -104,12 +104,16 @@ public UpdateControl reconcile( .createOrReplace(); } if (updateStatus) { - if (resource.getStatus() == null) { - resource.setStatus(new TestCustomResourceStatus()); - } + var statusUpdateResource = new TestCustomResource(); + statusUpdateResource.setMetadata(new ObjectMetaBuilder() + .withName(resource.getMetadata().getName()) + .withNamespace(resource.getMetadata().getNamespace()) + .build()); + resource.setStatus(new TestCustomResourceStatus()); resource.getStatus().setConfigMapStatus("ConfigMap Ready"); + return UpdateControl.patchStatus(resource); } - return UpdateControl.patchStatus(resource); + return UpdateControl.noUpdate(); } private Map configMapData(TestCustomResource resource) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingCustomResource.java index df832aaed0..0f95ccd824 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingCustomResource.java @@ -15,8 +15,4 @@ public class StatusUpdateLockingCustomResource extends CustomResource implements Namespaced { - @Override - protected StatusUpdateLockingCustomResourceStatus initStatus() { - return new StatusUpdateLockingCustomResourceStatus(); - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java index 1fe376d8da..fc007f5dfa 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statusupdatelocking/StatusUpdateLockingReconciler.java @@ -18,6 +18,7 @@ public UpdateControl reconcile( throws InterruptedException { numberOfExecutions.addAndGet(1); Thread.sleep(WAIT_TIME); + resource.setStatus(new StatusUpdateLockingCustomResourceStatus()); resource.getStatus().setValue(resource.getStatus().getValue() + 1); return UpdateControl.patchStatus(resource); } From d79eeb102f20e0d0725abbd72311e597d6f2c0ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 27 Mar 2024 14:40:11 +0100 Subject: [PATCH 14/28] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/processing/event/EventProcessorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java index 4e37b09e50..8c0e3f6410 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java @@ -286,7 +286,7 @@ void startProcessedMarkedEventReceivedBefore() { verify(reconciliationDispatcherMock, timeout(100).times(1)).handleExecution(any()); verify(metricsMock, times(1)).reconcileCustomResource(any(HasMetadata.class), isNull(), any()); } - + @Test void notUpdatesEventSourceHandlerIfResourceUpdated() { TestCustomResource customResource = testCustomResource(); From b0f144737cb0bf48b707c2fd00f69697b180b8ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 10:46:31 +0100 Subject: [PATCH 15/28] test wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- docs/documentation/v5-0-migration.md | 2 + .../event/ReconciliationDispatcherTest.java | 66 +++++++++++++++---- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index 92ddaaa8b8..f0c50e0be3 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -33,3 +33,5 @@ permalink: /docs/v5-0-migration 6. `ConfigurationService.getTerminationTimeoutSeconds` and associated overriding mechanism have been removed, use `Operator.stop(Duration)` instead. 7. `Operator.installShutdownHook()` has been removed, use `Operator.installShutdownHook(Duration)` instead +8. SSA for patch finalizer etc + TODO: explain usage, observed generation change handling etc - with SSA only for patch resources. \ No newline at end of file diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index fc62f85734..b59e140486 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -69,6 +69,28 @@ static void classSetup() { * equals will fail on the two equal but NOT identical TestCustomResources because equals is not * implemented on TestCustomResourceSpec or TestCustomResourceStatus */ + initConfigService(true); + // configurationService = + // ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(), + // overrider -> overrider.checkingCRDAndValidateLocalModel(false) + // .withResourceCloner(new Cloner() { + // @Override + // public R clone(R object) { + // return object; + // } + // }) + // .withUseSSAToPatchPrimaryResource(false)); + } + + @BeforeEach + void setup() { + testCustomResource = TestUtils.testCustomResource(); + reconciler = spy(new TestReconciler()); + reconciliationDispatcher = + init(testCustomResource, reconciler, null, customResourceFacade, true); + } + + static void initConfigService(boolean useSSA) { configurationService = ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(), overrider -> overrider.checkingCRDAndValidateLocalModel(false) @@ -78,15 +100,7 @@ public R clone(R object) { return object; } }) - .withUseSSAToPatchPrimaryResource(false)); - } - - @BeforeEach - void setup() { - testCustomResource = TestUtils.testCustomResource(); - reconciler = spy(new TestReconciler()); - reconciliationDispatcher = - init(testCustomResource, reconciler, null, customResourceFacade, true); + .withUseSSAToPatchPrimaryResource(useSSA)); } private ReconciliationDispatcher init(R customResource, @@ -445,7 +459,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception { } @Test - void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { + void doesNotUpdatesObservedGenerationIfStatusIsNotPatchedWhenUsingSSA() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); @@ -459,13 +473,11 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { PostExecutionControl control = dispatcher.handleExecution( executionScopeWithCREvent(observedGenResource)); - assertThat(control.getUpdatedCustomResource().orElseGet(() -> fail("Missing optional")) - .getStatus().getObservedGeneration()) - .isEqualTo(1L); + assertThat(control.getUpdatedCustomResource()).isEmpty(); } @Test - void patchObservedGenerationOnCustomResourceUpdate() throws Exception { + void patchObservedGenerationOnCustomResourcePatchIfNoSSA() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); @@ -476,6 +488,7 @@ void patchObservedGenerationOnCustomResourceUpdate() throws Exception { .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 control = dispatcher.handleExecution( @@ -485,6 +498,25 @@ void patchObservedGenerationOnCustomResourceUpdate() throws Exception { .isEqualTo(1L); } + @Test + void doesNotPatchObservedGenerationOnCustomResourcePatch() throws Exception { + var observedGenResource = createObservedGenCustomResource(); + + Reconciler reconciler = mock(Reconciler.class); + final var config = MockControllerConfiguration.forResource(ObservedGenCustomResource.class); + CustomResourceFacade 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); + var dispatcher = init(observedGenResource, reconciler, config, facade, true); + + PostExecutionControl control = dispatcher.handleExecution( + executionScopeWithCREvent(observedGenResource)); + assertThat(control.getUpdatedCustomResource()).isEmpty(); + } + @Test void callErrorStatusHandlerIfImplemented() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); @@ -670,6 +702,12 @@ void reSchedulesFromErrorHandler() { assertThat(res.getRuntimeException()).isEmpty(); } + @Test + void addsFinalizerToPatchWithSSA() { + + } + + private ObservedGenCustomResource createObservedGenCustomResource() { ObservedGenCustomResource observedGenCustomResource = new ObservedGenCustomResource(); observedGenCustomResource.setMetadata(new ObjectMeta()); From e970d46a22a750603b5d145ebc0bc06a92d862be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 14:16:20 +0100 Subject: [PATCH 16/28] unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcherTest.java | 66 ++++++++++--------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index b59e140486..bf4e5a8f27 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -6,7 +6,6 @@ import java.util.concurrent.TimeUnit; import java.util.function.BiFunction; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -60,30 +59,9 @@ class ReconciliationDispatcherTest { mock(ReconciliationDispatcher.CustomResourceFacade.class); private static ConfigurationService configurationService; - @BeforeAll - static void classSetup() { - /* - * We need this for mock reconcilers to properly generate the expected UpdateControl: without - * this, calls such as `when(reconciler.reconcile(eq(testCustomResource), - * any())).thenReturn(UpdateControl.updateStatus(testCustomResource))` will return null because - * equals will fail on the two equal but NOT identical TestCustomResources because equals is not - * implemented on TestCustomResourceSpec or TestCustomResourceStatus - */ - initConfigService(true); - // configurationService = - // ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(), - // overrider -> overrider.checkingCRDAndValidateLocalModel(false) - // .withResourceCloner(new Cloner() { - // @Override - // public R clone(R object) { - // return object; - // } - // }) - // .withUseSSAToPatchPrimaryResource(false)); - } - @BeforeEach void setup() { + initConfigService(true); testCustomResource = TestUtils.testCustomResource(); reconciler = spy(new TestReconciler()); reconciliationDispatcher = @@ -91,6 +69,13 @@ void setup() { } static void initConfigService(boolean useSSA) { + /* + * We need this for mock reconcilers to properly generate the expected UpdateControl: without + * this, calls such as `when(reconciler.reconcile(eq(testCustomResource), + * any())).thenReturn(UpdateControl.updateStatus(testCustomResource))` will return null because + * equals will fail on the two equal but NOT identical TestCustomResources because equals is not + * implemented on TestCustomResourceSpec or TestCustomResourceStatus + */ configurationService = ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(), overrider -> overrider.checkingCRDAndValidateLocalModel(false) @@ -139,6 +124,21 @@ public boolean useFinalizer() { void addFinalizerOnNewResource() { assertFalse(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); + verify(reconciler, never()) + .reconcile(ArgumentMatchers.eq(testCustomResource), any()); + verify(customResourceFacade, times(1)) + .patchResourceWithSSA( + argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER))); + } + + @Test + void addFinalizerOnNewResourceWithoutSSA() { + initConfigService(false); + final ReconciliationDispatcher dispatcher = + init(testCustomResource, reconciler, null, customResourceFacade, true); + + assertFalse(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)); + dispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(reconciler, never()) .reconcile(ArgumentMatchers.eq(testCustomResource), any()); verify(customResourceFacade, times(1)) @@ -346,14 +346,14 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); - when(customResourceFacade.patchResource(any(), any())) + when(customResourceFacade.patchResourceWithSSA(any())) .thenReturn(testCustomResource); var postExecControl = reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - assertEquals(1, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, times(1)).patchResource(any(), any()); + verify(customResourceFacade, times(1)) + .patchResourceWithSSA(argThat(a -> !a.getMetadata().getFinalizers().isEmpty())); assertThat(postExecControl.updateIsStatusPatch()).isFalse(); assertThat(postExecControl.getUpdatedCustomResource()).isPresent(); } @@ -509,12 +509,12 @@ void doesNotPatchObservedGenerationOnCustomResourcePatch() throws Exception { when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.patchResource(observedGenResource)); when(facade.patchResource(any(), any())).thenReturn(observedGenResource); - when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource); - var dispatcher = init(observedGenResource, reconciler, config, facade, true); + var dispatcher = init(observedGenResource, reconciler, config, facade, false); - PostExecutionControl control = dispatcher.handleExecution( + dispatcher.handleExecution( executionScopeWithCREvent(observedGenResource)); - assertThat(control.getUpdatedCustomResource()).isEmpty(); + + verify(facade, never()).patchStatus(any(), any()); } @Test @@ -667,7 +667,11 @@ void canSkipSchedulingMaxDelayIf() { } @Test - void retriesAddingFinalizer() { + void retriesAddingFinalizerWithoutSSA() { + initConfigService(false); + reconciliationDispatcher = + init(testCustomResource, reconciler, null, customResourceFacade, true); + removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); when(customResourceFacade.patchResource(any(), any())) From 04f1bad3bed82404df2e0114c891370c8787ba28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 15:08:29 +0100 Subject: [PATCH 17/28] Integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcher.java | 16 ++---- .../operator/PatchResourceWithSSAIT.java | 51 +++++++++++++++++++ .../PatchResourceWithSSACustomResource.java | 16 ++++++ .../PatchResourceWithSSAReconciler.java | 41 +++++++++++++++ .../PatchResourceWithSSASpec.java | 23 +++++++++ .../PatchResourceWithSSAStatus.java | 14 +++++ 6 files changed, 150 insertions(+), 11 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSACustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSASpec.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAStatus.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index a1e882c644..58cd78714b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -481,17 +481,11 @@ public R patchStatus(R resource, R originalResource) { } public R patchResourceWithSSA(R resource) { - var managedFields = resource.getMetadata().getManagedFields(); - resource.getMetadata().setManagedFields(null); - try { - return resource(resource).patch(new PatchContext.Builder() - .withFieldManager(fieldManager) - .withForce(true) - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build()); - } finally { - resource.getMetadata().setManagedFields(managedFields); - } + return resource(resource).patch(new PatchContext.Builder() + .withFieldManager(fieldManager) + .withForce(true) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); } private Resource resource(R resource) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java new file mode 100644 index 0000000000..d31f145fb3 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java @@ -0,0 +1,51 @@ +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.patchresourcewithssa.PatchResourceWithSSACustomResource; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSAReconciler; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSASpec; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +public class PatchResourceWithSSAIT { + + public static final String RESOURCE_NAME = "test1"; + public static final String INIT_VALUE = "init value"; + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(new PatchResourceWithSSAReconciler()) + .build(); + + @Test + void reconcilerPatchesResourceWithSSA() { + extension.create(testResource()); + + await().untilAsserted(() -> { + var actualResource = extension.get(PatchResourceWithSSACustomResource.class, RESOURCE_NAME); + + assertThat(actualResource.getSpec().getInitValue()).isEqualTo(INIT_VALUE); + assertThat(actualResource.getSpec().getControllerManagedValue()) + .isEqualTo(PatchResourceWithSSAReconciler.ADDED_VALUE); + // finalizer is added to the SSA patch in the background by the framework + assertThat(actualResource.getMetadata().getFinalizers()).isNotEmpty(); + assertThat(actualResource.getStatus().isSuccessfullyReconciled()).isTrue(); + }); + } + + PatchResourceWithSSACustomResource testResource() { + var res = new PatchResourceWithSSACustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(RESOURCE_NAME) + .build()); + res.setSpec(new PatchResourceWithSSASpec()); + res.getSpec().setInitValue(INIT_VALUE); + return res; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSACustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSACustomResource.java new file mode 100644 index 0000000000..602776f3cb --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSACustomResource.java @@ -0,0 +1,16 @@ +package io.javaoperatorsdk.operator.sample.patchresourcewithssa; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@ShortNames("prs") +public class PatchResourceWithSSACustomResource + extends CustomResource + implements Namespaced { + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java new file mode 100644 index 0000000000..5e3929a163 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java @@ -0,0 +1,41 @@ +package io.javaoperatorsdk.operator.sample.patchresourcewithssa; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.*; + +@ControllerConfiguration +public class PatchResourceWithSSAReconciler + implements Reconciler, + Cleaner { + + public static final String ADDED_VALUE = "Added Value"; + + @Override + public UpdateControl reconcile( + PatchResourceWithSSACustomResource resource, + Context context) { + + var res = new PatchResourceWithSSACustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(resource.getMetadata().getName()) + .withNamespace(resource.getMetadata().getNamespace()) + .build()); + + // first update the spec with missing value, then status in next reconciliation + if (resource.getSpec().getControllerManagedValue() == null) { + res.setSpec(new PatchResourceWithSSASpec()); + res.getSpec().setControllerManagedValue(ADDED_VALUE); + return UpdateControl.patchResource(res); + } else { + res.setStatus(new PatchResourceWithSSAStatus()); + res.getStatus().setSuccessfullyReconciled(true); + return UpdateControl.patchStatus(res); + } + } + + @Override + public DeleteControl cleanup(PatchResourceWithSSACustomResource resource, + Context context) { + return DeleteControl.defaultDelete(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSASpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSASpec.java new file mode 100644 index 0000000000..77d4fe0428 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSASpec.java @@ -0,0 +1,23 @@ +package io.javaoperatorsdk.operator.sample.patchresourcewithssa; + +public class PatchResourceWithSSASpec { + + private String initValue; + private String controllerManagedValue; + + public String getInitValue() { + return initValue; + } + + public void setInitValue(String initValue) { + this.initValue = initValue; + } + + public String getControllerManagedValue() { + return controllerManagedValue; + } + + public void setControllerManagedValue(String controllerManagedValue) { + this.controllerManagedValue = controllerManagedValue; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAStatus.java new file mode 100644 index 0000000000..982ef83129 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAStatus.java @@ -0,0 +1,14 @@ +package io.javaoperatorsdk.operator.sample.patchresourcewithssa; + +public class PatchResourceWithSSAStatus { + + private boolean successfullyReconciled; + + public boolean isSuccessfullyReconciled() { + return successfullyReconciled; + } + + public void setSuccessfullyReconciled(boolean successfullyReconciled) { + this.successfullyReconciled = successfullyReconciled; + } +} From 1253093f85b0195603a5c4c249b917fcd5da9da6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 15:35:21 +0100 Subject: [PATCH 18/28] ITs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../PatchResourceAndStatusWithSSAIT.java | 13 ++++ .../operator/PatchResourceWithSSAIT.java | 46 ++------------- .../operator/PatchWithSSAITBase.java | 59 +++++++++++++++++++ .../DoubleUpdateTestCustomReconciler.java | 20 ++++--- ...tchResourceAndStatusWithSSAReconciler.java | 37 ++++++++++++ 5 files changed, 127 insertions(+), 48 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusWithSSAIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchWithSSAITBase.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceAndStatusWithSSAReconciler.java diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusWithSSAIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusWithSSAIT.java new file mode 100644 index 0000000000..644316faf2 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusWithSSAIT.java @@ -0,0 +1,13 @@ +package io.javaoperatorsdk.operator; + +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceAndStatusWithSSAReconciler; + +public class PatchResourceAndStatusWithSSAIT extends PatchWithSSAITBase { + + @Override + protected Reconciler reconciler() { + return new PatchResourceAndStatusWithSSAReconciler(); + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java index d31f145fb3..80f81f78d1 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceWithSSAIT.java @@ -1,51 +1,15 @@ 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.patchresourcewithssa.PatchResourceWithSSACustomResource; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSAReconciler; -import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSASpec; -import static org.assertj.core.api.Assertions.assertThat; -import static org.awaitility.Awaitility.await; -public class PatchResourceWithSSAIT { +public class PatchResourceWithSSAIT extends PatchWithSSAITBase { - public static final String RESOURCE_NAME = "test1"; - public static final String INIT_VALUE = "init value"; - - @RegisterExtension - LocallyRunOperatorExtension extension = - LocallyRunOperatorExtension.builder() - .withReconciler(new PatchResourceWithSSAReconciler()) - .build(); - - @Test - void reconcilerPatchesResourceWithSSA() { - extension.create(testResource()); - - await().untilAsserted(() -> { - var actualResource = extension.get(PatchResourceWithSSACustomResource.class, RESOURCE_NAME); - - assertThat(actualResource.getSpec().getInitValue()).isEqualTo(INIT_VALUE); - assertThat(actualResource.getSpec().getControllerManagedValue()) - .isEqualTo(PatchResourceWithSSAReconciler.ADDED_VALUE); - // finalizer is added to the SSA patch in the background by the framework - assertThat(actualResource.getMetadata().getFinalizers()).isNotEmpty(); - assertThat(actualResource.getStatus().isSuccessfullyReconciled()).isTrue(); - }); + @Override + protected Reconciler reconciler() { + return new PatchResourceWithSSAReconciler(); } - PatchResourceWithSSACustomResource testResource() { - var res = new PatchResourceWithSSACustomResource(); - res.setMetadata(new ObjectMetaBuilder() - .withName(RESOURCE_NAME) - .build()); - res.setSpec(new PatchResourceWithSSASpec()); - res.getSpec().setInitValue(INIT_VALUE); - return res; - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchWithSSAITBase.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchWithSSAITBase.java new file mode 100644 index 0000000000..b3b6b4fc32 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchWithSSAITBase.java @@ -0,0 +1,59 @@ +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.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSACustomResource; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSAReconciler; +import io.javaoperatorsdk.operator.sample.patchresourcewithssa.PatchResourceWithSSASpec; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +public abstract class PatchWithSSAITBase { + + public static final String RESOURCE_NAME = "test1"; + public static final String INIT_VALUE = "init value"; + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(reconciler()) + .build(); + + @Test + void reconcilerPatchesResourceWithSSA() { + extension.create(testResource()); + + await().untilAsserted(() -> { + var actualResource = extension.get(PatchResourceWithSSACustomResource.class, RESOURCE_NAME); + + assertThat(actualResource.getSpec().getInitValue()).isEqualTo(INIT_VALUE); + assertThat(actualResource.getSpec().getControllerManagedValue()) + .isEqualTo(PatchResourceWithSSAReconciler.ADDED_VALUE); + // finalizer is added to the SSA patch in the background by the framework + assertThat(actualResource.getMetadata().getFinalizers()).isNotEmpty(); + assertThat(actualResource.getStatus().isSuccessfullyReconciled()).isTrue(); + // one for resource, one for subresource + assertThat(actualResource.getMetadata().getManagedFields().stream() + .filter(mf -> mf.getManager() + .equals(reconciler().getClass().getSimpleName().toLowerCase())) + .toList()).hasSize(2); + }); + } + + protected abstract Reconciler reconciler(); + + PatchResourceWithSSACustomResource testResource() { + var res = new PatchResourceWithSSACustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(RESOURCE_NAME) + .build()); + res.setSpec(new PatchResourceWithSSASpec()); + res.getSpec().setInitValue(INIT_VALUE); + return res; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java index 196fa4cb6d..9e90b236f8 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java @@ -6,6 +6,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -28,13 +29,18 @@ public UpdateControl reconcile( numberOfExecutions.addAndGet(1); log.info("Value: " + resource.getSpec().getValue()); - - resource.getMetadata().setAnnotations(new HashMap<>()); - resource.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE); - ensureStatusExists(resource); - resource.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); - - return UpdateControl.patchResourceAndStatus(resource); + DoubleUpdateTestCustomResource res = new DoubleUpdateTestCustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(resource.getMetadata().getName()) + .withNamespace(resource.getMetadata().getNamespace()) + .build()); + + res.getMetadata().setAnnotations(new HashMap<>()); + res.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE); + ensureStatusExists(res); + res.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); + + return UpdateControl.patchResourceAndStatus(res); } private void ensureStatusExists(DoubleUpdateTestCustomResource resource) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceAndStatusWithSSAReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceAndStatusWithSSAReconciler.java new file mode 100644 index 0000000000..0c9cbf0456 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceAndStatusWithSSAReconciler.java @@ -0,0 +1,37 @@ +package io.javaoperatorsdk.operator.sample.patchresourcewithssa; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.*; + +@ControllerConfiguration +public class PatchResourceAndStatusWithSSAReconciler + implements Reconciler, + Cleaner { + + public static final String ADDED_VALUE = "Added Value"; + + @Override + public UpdateControl reconcile( + PatchResourceWithSSACustomResource resource, + Context context) { + + var res = new PatchResourceWithSSACustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(resource.getMetadata().getName()) + .withNamespace(resource.getMetadata().getNamespace()) + .build()); + + res.setSpec(new PatchResourceWithSSASpec()); + res.getSpec().setControllerManagedValue(ADDED_VALUE); + res.setStatus(new PatchResourceWithSSAStatus()); + res.getStatus().setSuccessfullyReconciled(true); + + return UpdateControl.patchResourceAndStatus(res); + } + + @Override + public DeleteControl cleanup(PatchResourceWithSSACustomResource resource, + Context context) { + return DeleteControl.defaultDelete(); + } +} From 03f0010f0d89f55eee6d36360cc7f22775e84512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 15:37:16 +0100 Subject: [PATCH 19/28] IT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/UpdatingResAndSubResIT.java | 5 ++++- .../DoubleUpdateTestCustomReconciler.java | 20 +++++++------------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java index 3d7d27b4c3..5016eb1137 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java @@ -19,7 +19,10 @@ class UpdatingResAndSubResIT { @RegisterExtension LocallyRunOperatorExtension operator = - LocallyRunOperatorExtension.builder().withReconciler(DoubleUpdateTestCustomReconciler.class) + + LocallyRunOperatorExtension.builder() + .withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false)) + .withReconciler(DoubleUpdateTestCustomReconciler.class) .build(); @Test diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java index 9e90b236f8..196fa4cb6d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java @@ -6,7 +6,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -29,18 +28,13 @@ public UpdateControl reconcile( numberOfExecutions.addAndGet(1); log.info("Value: " + resource.getSpec().getValue()); - DoubleUpdateTestCustomResource res = new DoubleUpdateTestCustomResource(); - res.setMetadata(new ObjectMetaBuilder() - .withName(resource.getMetadata().getName()) - .withNamespace(resource.getMetadata().getNamespace()) - .build()); - - res.getMetadata().setAnnotations(new HashMap<>()); - res.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE); - ensureStatusExists(res); - res.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); - - return UpdateControl.patchResourceAndStatus(res); + + resource.getMetadata().setAnnotations(new HashMap<>()); + resource.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE); + ensureStatusExists(resource); + resource.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); + + return UpdateControl.patchResourceAndStatus(resource); } private void ensureStatusExists(DoubleUpdateTestCustomResource resource) { From 922925ff5f64e8e621c8190fe8ecf85b8e58e355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 15:42:25 +0100 Subject: [PATCH 20/28] web page sample update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../io/javaoperatorsdk/operator/sample/Utils.java | 11 +++++++++++ .../sample/WebPageDependentsWorkflowReconciler.java | 8 ++++---- .../sample/WebPageManagedDependentsReconciler.java | 7 ++----- .../operator/sample/WebPageReconciler.java | 5 +++-- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java index 82cb3a86a4..72d04b42ed 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/Utils.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.sample; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.fabric8.kubernetes.api.model.networking.v1.Ingress; import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusUpdateControl; import io.javaoperatorsdk.operator.sample.customresource.WebPage; @@ -11,6 +12,16 @@ public class Utils { private Utils() {} + public static WebPage createWebPageForStatusUpdate(WebPage webPage, String configMapName) { + WebPage res = new WebPage(); + res.setMetadata(new ObjectMetaBuilder() + .withName(webPage.getMetadata().getName()) + .withNamespace(webPage.getMetadata().getNamespace()) + .build()); + res.setStatus(createStatus(configMapName)); + return res; + } + public static WebPageStatus createStatus(String configMapName) { WebPageStatus status = new WebPageStatus(); status.setHtmlConfigMap(configMapName); diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java index 3d3b583659..06c4ae8721 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java @@ -61,10 +61,10 @@ public UpdateControl reconcile(WebPage webPage, Context contex workflow.reconcile(webPage, context); - webPage.setStatus( - createStatus( - context.getSecondaryResource(ConfigMap.class).orElseThrow().getMetadata().getName())); - return UpdateControl.patchStatus(webPage); + return UpdateControl + .patchStatus( + createWebPageForStatusUpdate(webPage, context.getSecondaryResource(ConfigMap.class) + .orElseThrow().getMetadata().getName())); } @Override diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageManagedDependentsReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageManagedDependentsReconciler.java index 44149aed4d..32811251d7 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageManagedDependentsReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageManagedDependentsReconciler.java @@ -6,9 +6,7 @@ import io.javaoperatorsdk.operator.sample.customresource.WebPage; import io.javaoperatorsdk.operator.sample.dependentresource.*; -import static io.javaoperatorsdk.operator.sample.Utils.createStatus; -import static io.javaoperatorsdk.operator.sample.Utils.handleError; -import static io.javaoperatorsdk.operator.sample.Utils.simulateErrorIfRequested; +import static io.javaoperatorsdk.operator.sample.Utils.*; /** * Shows how to implement a reconciler with managed dependent resources. @@ -39,8 +37,7 @@ public UpdateControl reconcile(WebPage webPage, Context contex final var name = context.getSecondaryResource(ConfigMap.class).orElseThrow() .getMetadata().getName(); - webPage.setStatus(createStatus(name)); - return UpdateControl.patchStatus(webPage); + return UpdateControl.patchStatus(createWebPageForStatusUpdate(webPage, name)); } @Override diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java index df6ba7ddb8..46bc3dd392 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java @@ -132,8 +132,9 @@ public UpdateControl reconcile(WebPage webPage, Context contex log.info("Restarting pods because HTML has changed in {}", ns); kubernetesClient.pods().inNamespace(ns).withLabel("app", deploymentName(webPage)).delete(); } - webPage.setStatus(createStatus(desiredHtmlConfigMap.getMetadata().getName())); - return UpdateControl.patchStatus(webPage); + + return UpdateControl.patchStatus( + createWebPageForStatusUpdate(webPage, desiredHtmlConfigMap.getMetadata().getName())); } private boolean match(Ingress desiredIngress, Ingress existingIngress) { From 500c18d80b6e62e06329c71ecce8c0186996f3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 15:53:27 +0100 Subject: [PATCH 21/28] refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- ...ava => PatchResourceAndStatusNoSSAIT.java} | 35 ++++++++++--------- .../DoubleUpdateTestCustomResourceSpec.java | 15 -------- .../DoubleUpdateTestCustomResourceStatus.java | 19 ---------- ...ResourceAndStatusNoSSACustomResource.java} | 6 ++-- ...atchResourceAndStatusNoSSAReconciler.java} | 21 +++++------ .../PatchResourceAndStatusNoSSASpec.java | 15 ++++++++ .../PatchResourceAndStatusNoSSAStatus.java | 19 ++++++++++ 7 files changed, 66 insertions(+), 64 deletions(-) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/{UpdatingResAndSubResIT.java => PatchResourceAndStatusNoSSAIT.java} (55%) delete mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceSpec.java delete mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceStatus.java rename operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/{doubleupdate/DoubleUpdateTestCustomResource.java => patchresourceandstatusnossa/PatchResourceAndStatusNoSSACustomResource.java} (66%) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/{doubleupdate/DoubleUpdateTestCustomReconciler.java => patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java} (61%) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSASpec.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAStatus.java diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusNoSSAIT.java similarity index 55% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusNoSSAIT.java index 5016eb1137..9583629a7c 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/UpdatingResAndSubResIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/PatchResourceAndStatusNoSSAIT.java @@ -7,47 +7,47 @@ import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; -import io.javaoperatorsdk.operator.sample.doubleupdate.DoubleUpdateTestCustomReconciler; -import io.javaoperatorsdk.operator.sample.doubleupdate.DoubleUpdateTestCustomResource; -import io.javaoperatorsdk.operator.sample.doubleupdate.DoubleUpdateTestCustomResourceSpec; -import io.javaoperatorsdk.operator.sample.doubleupdate.DoubleUpdateTestCustomResourceStatus; +import io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa.PatchResourceAndStatusNoSSACustomResource; +import io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa.PatchResourceAndStatusNoSSAReconciler; +import io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa.PatchResourceAndStatusNoSSASpec; +import io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa.PatchResourceAndStatusNoSSAStatus; import io.javaoperatorsdk.operator.support.TestUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; -class UpdatingResAndSubResIT { +class PatchResourceAndStatusNoSSAIT { @RegisterExtension LocallyRunOperatorExtension operator = LocallyRunOperatorExtension.builder() .withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false)) - .withReconciler(DoubleUpdateTestCustomReconciler.class) + .withReconciler(PatchResourceAndStatusNoSSAReconciler.class) .build(); @Test void updatesSubResourceStatus() { - DoubleUpdateTestCustomResource resource = createTestCustomResource("1"); + PatchResourceAndStatusNoSSACustomResource resource = createTestCustomResource("1"); operator.create(resource); awaitStatusUpdated(resource.getMetadata().getName()); // wait for sure, there are no more events TestUtils.waitXms(300); - DoubleUpdateTestCustomResource customResource = + PatchResourceAndStatusNoSSACustomResource customResource = operator - .get(DoubleUpdateTestCustomResource.class, + .get(PatchResourceAndStatusNoSSACustomResource.class, resource.getMetadata().getName()); assertThat(TestUtils.getNumberOfExecutions(operator)) .isEqualTo(1); assertThat(customResource.getStatus().getState()) - .isEqualTo(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); + .isEqualTo(PatchResourceAndStatusNoSSAStatus.State.SUCCESS); assertThat( customResource .getMetadata() .getAnnotations() - .get(DoubleUpdateTestCustomReconciler.TEST_ANNOTATION)) + .get(PatchResourceAndStatusNoSSAReconciler.TEST_ANNOTATION)) .isNotNull(); } @@ -56,21 +56,22 @@ void awaitStatusUpdated(String name) { .atMost(5, TimeUnit.SECONDS) .untilAsserted( () -> { - DoubleUpdateTestCustomResource cr = - operator.get(DoubleUpdateTestCustomResource.class, name); + PatchResourceAndStatusNoSSACustomResource cr = + operator.get(PatchResourceAndStatusNoSSACustomResource.class, name); assertThat(cr) .isNotNull(); assertThat(cr.getStatus()) .isNotNull(); assertThat(cr.getStatus().getState()) - .isEqualTo(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); + .isEqualTo(PatchResourceAndStatusNoSSAStatus.State.SUCCESS); }); } - public DoubleUpdateTestCustomResource createTestCustomResource(String id) { - DoubleUpdateTestCustomResource resource = new DoubleUpdateTestCustomResource(); + public PatchResourceAndStatusNoSSACustomResource createTestCustomResource(String id) { + PatchResourceAndStatusNoSSACustomResource resource = + new PatchResourceAndStatusNoSSACustomResource(); resource.setMetadata(new ObjectMetaBuilder().withName("doubleupdateresource-" + id).build()); - resource.setSpec(new DoubleUpdateTestCustomResourceSpec()); + resource.setSpec(new PatchResourceAndStatusNoSSASpec()); resource.getSpec().setValue(id); return resource; } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceSpec.java deleted file mode 100644 index 02212957a9..0000000000 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceSpec.java +++ /dev/null @@ -1,15 +0,0 @@ -package io.javaoperatorsdk.operator.sample.doubleupdate; - -public class DoubleUpdateTestCustomResourceSpec { - - private String value; - - public String getValue() { - return value; - } - - public DoubleUpdateTestCustomResourceSpec setValue(String value) { - this.value = value; - return this; - } -} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceStatus.java deleted file mode 100644 index 3c7b694853..0000000000 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResourceStatus.java +++ /dev/null @@ -1,19 +0,0 @@ -package io.javaoperatorsdk.operator.sample.doubleupdate; - -public class DoubleUpdateTestCustomResourceStatus { - - private State state; - - public State getState() { - return state; - } - - public DoubleUpdateTestCustomResourceStatus setState(State state) { - this.state = state; - return this; - } - - public enum State { - SUCCESS, ERROR - } -} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSACustomResource.java similarity index 66% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResource.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSACustomResource.java index 11c543e388..d5273d4e1d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSACustomResource.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.sample.doubleupdate; +package io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa; import io.fabric8.kubernetes.api.model.Namespaced; import io.fabric8.kubernetes.client.CustomResource; @@ -11,7 +11,7 @@ @Version("v1") @Kind("DoubleUpdateSample") @ShortNames("du") -public class DoubleUpdateTestCustomResource - extends CustomResource +public class PatchResourceAndStatusNoSSACustomResource + extends CustomResource implements Namespaced { } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java similarity index 61% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java index 196fa4cb6d..ecbceb8f65 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/doubleupdate/DoubleUpdateTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.sample.doubleupdate; +package io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa; import java.util.HashMap; import java.util.concurrent.atomic.AtomicInteger; @@ -13,18 +13,19 @@ import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; @ControllerConfiguration -public class DoubleUpdateTestCustomReconciler - implements Reconciler, TestExecutionInfoProvider { +public class PatchResourceAndStatusNoSSAReconciler + implements Reconciler, TestExecutionInfoProvider { private static final Logger log = - LoggerFactory.getLogger(DoubleUpdateTestCustomReconciler.class); + LoggerFactory.getLogger(PatchResourceAndStatusNoSSAReconciler.class); public static final String TEST_ANNOTATION = "TestAnnotation"; public static final String TEST_ANNOTATION_VALUE = "TestAnnotationValue"; private final AtomicInteger numberOfExecutions = new AtomicInteger(0); @Override - public UpdateControl reconcile( - DoubleUpdateTestCustomResource resource, Context context) { + public UpdateControl reconcile( + PatchResourceAndStatusNoSSACustomResource resource, + Context context) { numberOfExecutions.addAndGet(1); log.info("Value: " + resource.getSpec().getValue()); @@ -32,15 +33,15 @@ public UpdateControl reconcile( resource.getMetadata().setAnnotations(new HashMap<>()); resource.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE); ensureStatusExists(resource); - resource.getStatus().setState(DoubleUpdateTestCustomResourceStatus.State.SUCCESS); + resource.getStatus().setState(PatchResourceAndStatusNoSSAStatus.State.SUCCESS); return UpdateControl.patchResourceAndStatus(resource); } - private void ensureStatusExists(DoubleUpdateTestCustomResource resource) { - DoubleUpdateTestCustomResourceStatus status = resource.getStatus(); + private void ensureStatusExists(PatchResourceAndStatusNoSSACustomResource resource) { + PatchResourceAndStatusNoSSAStatus status = resource.getStatus(); if (status == null) { - status = new DoubleUpdateTestCustomResourceStatus(); + status = new PatchResourceAndStatusNoSSAStatus(); resource.setStatus(status); } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSASpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSASpec.java new file mode 100644 index 0000000000..ebc58bc862 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSASpec.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa; + +public class PatchResourceAndStatusNoSSASpec { + + private String value; + + public String getValue() { + return value; + } + + public PatchResourceAndStatusNoSSASpec setValue(String value) { + this.value = value; + return this; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAStatus.java new file mode 100644 index 0000000000..f31031cbcc --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAStatus.java @@ -0,0 +1,19 @@ +package io.javaoperatorsdk.operator.sample.patchresourceandstatusnossa; + +public class PatchResourceAndStatusNoSSAStatus { + + private State state; + + public State getState() { + return state; + } + + public PatchResourceAndStatusNoSSAStatus setState(State state) { + this.state = state; + return this; + } + + public enum State { + SUCCESS, ERROR + } +} From ff9c6994cb9df3f22a4a393f517439d741e85c7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 28 Mar 2024 16:12:14 +0100 Subject: [PATCH 22/28] docs updatestarted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- docs/documentation/v5-0-migration.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index f0c50e0be3..4eef460f5c 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -17,15 +17,17 @@ permalink: /docs/v5-0-migration [`EventSourceUtils`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceUtils.java#L11-L11) now contains all the utility methods used for event sources naming that were previously defined in the `EventSourceInitializer` interface. -3. Patching status through `UpdateControl` like the `patchStatus` method now by default - uses Server Side Apply instead of simple patch. To use the former approach, use the feature flag - in [`ConfigurationService`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L400-L400) +3. Updates through `UpdateControl` are by default now use SSA, this is true for adding finalizer and all + the patch operations in `UpdateControl`. The update operations were removed. Migrating to SSA can be tricky + in order to use non-SSA based patch set [`ConfigurationService.useSSAToPatchPrimaryResource()]`(https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L385-L385) to false. + !!! IMPORTANT !!! - Migration from a non-SSA based controller to SSA based controller can cause problems, due to known issues. - See the - following [integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L71-L82) + + See known issues with migration from non-SSA to SSA based status updates here: + [integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L71-L82) 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). + 4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed via the accordingly renamed `managedWorkflowAndDependentResourceContext` method. 5. `ResourceDiscriminator` was removed. In most of the cases you can just delete the discriminator, everything should @@ -33,5 +35,3 @@ permalink: /docs/v5-0-migration 6. `ConfigurationService.getTerminationTimeoutSeconds` and associated overriding mechanism have been removed, use `Operator.stop(Duration)` instead. 7. `Operator.installShutdownHook()` has been removed, use `Operator.installShutdownHook(Duration)` instead -8. SSA for patch finalizer etc - TODO: explain usage, observed generation change handling etc - with SSA only for patch resources. \ No newline at end of file From df454ebcd182e87bade870b6e20c0394e57b429e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 3 Apr 2024 17:21:14 +0200 Subject: [PATCH 23/28] docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- docs/documentation/v5-0-migration.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index 4eef460f5c..0a78132707 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -19,7 +19,8 @@ permalink: /docs/v5-0-migration the `EventSourceInitializer` interface. 3. Updates through `UpdateControl` are by default now use SSA, this is true for adding finalizer and all the patch operations in `UpdateControl`. The update operations were removed. Migrating to SSA can be tricky - in order to use non-SSA based patch set [`ConfigurationService.useSSAToPatchPrimaryResource()]`(https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L385-L385) to false. + in order to use non-SSA based patch you can use feature flag: + [`ConfigurationService.useSSAToPatchPrimaryResource()]`(https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L385-L385) to false. !!! IMPORTANT !!! @@ -28,6 +29,17 @@ 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). + SSA vs non-SSA resource handling required different approach for handling resource. + For SSA you send the "fully specified intent", what is a partial object that only includes the fields and values for which the user has an opinion. + That mean that usually resource are created from scratch. + See SSA sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java#L7-L7). + See non-SSA sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java#L16-L16). + + 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`. + 4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed via the accordingly renamed `managedWorkflowAndDependentResourceContext` method. 5. `ResourceDiscriminator` was removed. In most of the cases you can just delete the discriminator, everything should From d8ac96155176c454cd5deddc5ca020f2066ef2c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 4 Apr 2024 10:04:35 +0200 Subject: [PATCH 24/28] docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- docs/documentation/features.md | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index 3a3eb0c876..e673c34b34 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -92,16 +92,21 @@ the controller wants to update the resource itself (for example to add annotatio any updates, which is also supported. It is also possible to update both the status and the resource with the -`updateResourceAndStatus` method. In this case, the resource is updated first followed by the +`patchResourceAndStatus` method. In this case, the resource is updated first followed by the status, using two separate requests to the Kubernetes API. -You should always state your intent using `UpdateControl` and let the SDK deal with the actual -updates instead of performing these updates yourself using the actual Kubernetes client so that -the SDK can update its internal state accordingly. +From v5 `UpdateControl` only supports patching the resources, by default using [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/). +It is important to understand how SSA works in Kubernetes. Mainly that the resource sent with an SSA patch +should contain only the fields that we care about, thus usually using a resource created from scratch, see sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java#L18-L22). -Resource updates are protected using optimistic version control, to make sure that other updates -that might have occurred in the mean time on the server are not overwritten. This is ensured by -setting the `resourceVersion` field on the processed resources. +Non-SSA based patch is still supported. +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). +Related Integration test can be found [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java). + +It is completely fine to manage the resource using directly the client and not use `UpdateControl`, however it +is safer to start with `UpdateControl` also advised to use in general, since makes sure that operations are handled correctly. +Especially when a resource is patched (`UpdateControl.patchResource` - not the status) makes sure that the finalizer +is also send with SSA patch request thus not removing the finalizer unintentionally from the resource. [`DeleteControl`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DeleteControl.java) typically instructs the framework to remove the finalizer after the dependent @@ -170,6 +175,8 @@ You can specify the name of the finalizer to use for your `Reconciler` using the [`@ControllerConfiguration`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java) annotation. If you do not specify a finalizer name, one will be automatically generated for you. +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 @@ -187,11 +194,14 @@ In order to have this feature working: 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. Note that 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) -. +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 { From 4f1054bfc6d9fb563e44d62a5720874dc8eeaa4b Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Apr 2024 14:21:34 +0200 Subject: [PATCH 25/28] docs: improve Signed-off-by: Chris Laprun --- docs/documentation/features.md | 35 ++++++++++++------- docs/documentation/v5-0-migration.md | 12 ++----- .../api/config/ConfigurationService.java | 6 ++-- .../api/reconciler/UpdateControl.java | 4 +-- 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index e673c34b34..d4f52482b7 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -91,22 +91,31 @@ Those are the typical use cases of resource updates, however in some cases there the controller wants to update the resource itself (for example to add annotations) or not perform any updates, which is also supported. -It is also possible to update both the status and the resource with the -`patchResourceAndStatus` method. In this case, the resource is updated first followed by the -status, using two separate requests to the Kubernetes API. +It is also possible to update both the status and the resource with the `patchResourceAndStatus` method. In this case, +the resource is updated first followed by the status, using two separate requests to the Kubernetes API. -From v5 `UpdateControl` only supports patching the resources, by default using [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/). -It is important to understand how SSA works in Kubernetes. Mainly that the resource sent with an SSA patch -should contain only the fields that we care about, thus usually using a resource created from scratch, see sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java#L18-L22). +From v5 `UpdateControl` only supports patching the resources, by default +using [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/). +It is important to understand how SSA works in Kubernetes. Mainly, resources applied using SSA +should contain only the fields identifying the resource and those the user is interested in (a 'fully specified intent' +in Kubernetes parlance), thus usually using a resource created from scratch, see +[sample](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java#L18-L22). +To contrast, see the same sample, this time [without SSA](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java#L16-L16). Non-SSA based patch is still supported. -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). -Related Integration test can be found [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java). - -It is completely fine to manage the resource using directly the client and not use `UpdateControl`, however it -is safer to start with `UpdateControl` also advised to use in general, since makes sure that operations are handled correctly. -Especially when a resource is patched (`UpdateControl.patchResource` - not the status) makes sure that the finalizer -is also send with SSA patch request thus not removing the finalizer unintentionally from the resource. +You can control whether or not to use SSA +using [`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) +and the related `ConfigurationServiceOverrider.withUseSSAToPatchPrimaryResource` method. +Related integration test can be +found [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java). + +Handling resources directly using the client, instead of delegating these updates operations to JOSDK by returning +an `UpdateControl` at the end of your reconciliation, should work appropriately. However, we do recommend to +use `UpdateControl` instead since JOSDK makes sure that the operations are handled properly, since there are subtleties +to be aware of. For example, if you are using a finalizer, JOSDK makes sure to include it in your fully specified intent +so that it is not unintentionally removed from the resource (which would happen if you omit it, since your controller is +the designated manager for that field and Kubernetes interprets the finalizer being gone from the specified intent as a +request for removal). [`DeleteControl`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DeleteControl.java) typically instructs the framework to remove the finalizer after the dependent diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index 0a78132707..36748ea1dc 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -17,10 +17,8 @@ permalink: /docs/v5-0-migration [`EventSourceUtils`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceUtils.java#L11-L11) now contains all the utility methods used for event sources naming that were previously defined in the `EventSourceInitializer` interface. -3. Updates through `UpdateControl` are by default now use SSA, this is true for adding finalizer and all - the patch operations in `UpdateControl`. The update operations were removed. Migrating to SSA can be tricky - in order to use non-SSA based patch you can use feature flag: - [`ConfigurationService.useSSAToPatchPrimaryResource()]`(https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L385-L385) to false. +3. Updates through `UpdateControl` now use [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/) by default to add the finalizer and for all + the patch operations in `UpdateControl`. The update operations were removed. If you do not wish to use SSA, you can deactivate the feature using `ConfigurationService.useSSAToPatchPrimaryResource` and related `ConfigurationServiceOverrider.withUseSSAToPatchPrimaryResource`. !!! IMPORTANT !!! @@ -28,12 +26,6 @@ permalink: /docs/v5-0-migration [integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L71-L82) 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). - - SSA vs non-SSA resource handling required different approach for handling resource. - For SSA you send the "fully specified intent", what is a partial object that only includes the fields and values for which the user has an opinion. - That mean that usually resource are created from scratch. - See SSA sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourcewithssa/PatchResourceWithSSAReconciler.java#L7-L7). - See non-SSA sample [here](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java#L16-L16). Related automatic observed generation handling changes: Automated Observed Generation (see features in docs), is automatically handled for non-SSA, even if diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 7b42b1a88f..ad2bb46a32 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -377,10 +377,12 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() { /** * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can - * either use simple patches or SSA. Setting this to true, controller will use SSA for adding + * 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. * - * @return true by default + * @return {@code true} by default + * @since 5.0.0 + * @see ConfigurationServiceOverrider#withUseSSAToPatchPrimaryResource(boolean) */ default boolean useSSAToPatchPrimaryResource() { return true; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index 2d49488c83..54eafa3903 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -25,10 +25,10 @@ private UpdateControl( * Preferred way to update the status. It does not do optimistic locking. Uses JSON Patch to patch * the resource. *

- * Note that this does not work, if the {@link CustomResource#initStatus() initStatus} is + * Note that this does not work, if the {@link CustomResource#initStatus()} is * implemented, since it breaks the diffing process. Don't implement it if using this method. *

- * There is also an issue with setting value to null with older Kubernetes versions (1.19 and + * There is also an issue with setting value to {@code null} with older Kubernetes versions (1.19 and * below). See: https://github.com/fabric8io/kubernetes-client/issues/4158 * From 52f4da0521b77e2bb56ff138aa118ebed80944cd Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Apr 2024 14:24:18 +0200 Subject: [PATCH 26/28] refactor: simplify finalizer handling Signed-off-by: Chris Laprun --- .../event/ReconciliationDispatcher.java | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 58cd78714b..10761cb1d4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -51,12 +51,12 @@ class ReconciliationDispatcher

{ ReconciliationDispatcher(Controller

controller, CustomResourceFacade

customResourceFacade) { this.controller = controller; this.customResourceFacade = customResourceFacade; - this.cloner = controller.getConfiguration().getConfigurationService().getResourceCloner(); + final var configuration = controller.getConfiguration(); + this.cloner = configuration.getConfigurationService().getResourceCloner(); - var retry = controller.getConfiguration().getRetry(); + var retry = configuration.getRetry(); retryConfigurationHasZeroAttempts = retry == null || retry.initExecution().isLastAttempt(); - useSSA = controller.getConfiguration() - .getConfigurationService().useSSAToPatchPrimaryResource(); + useSSA = configuration.getConfigurationService().useSSAToPatchPrimaryResource(); } public ReconciliationDispatcher(Controller

controller) { @@ -174,10 +174,8 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc ? 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() || (updateObservedGeneration && !useSSA)) { + updatedCustomResource = patchStatusGenerationAware(toUpdate, originalResource); } return createPostExecutionControl(updatedCustomResource, updateControl); } @@ -360,11 +358,11 @@ private P patchResource(P resource, P originalResource) { log.trace("Resource before update: {}", resource); // todo unit test - if (useSSA && controller.useFinalizer() && - !resource.getMetadata().getFinalizers().contains(configuration().getFinalizerName())) { - resource.getMetadata().getFinalizers().add(configuration().getFinalizerName()); + final var finalizerName = configuration().getFinalizerName(); + if (useSSA && controller.useFinalizer()) { + // addFinalizer already prevents adding an already present finalizer so no need to check + resource.addFinalizer(finalizerName); } - return customResourceFacade.patchResource(resource, originalResource); } From 7b29dc054315d9f93a45b8b00678e131f4542578 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Apr 2024 14:55:50 +0200 Subject: [PATCH 27/28] fix: format Signed-off-by: Chris Laprun --- .../operator/api/config/ConfigurationService.java | 4 ++-- .../operator/api/reconciler/UpdateControl.java | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index ad2bb46a32..3fee02d044 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -377,8 +377,8 @@ 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. + * 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. * * @return {@code true} by default * @since 5.0.0 diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java index 54eafa3903..a8ae1331d7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java @@ -25,11 +25,11 @@ private UpdateControl( * Preferred way to update the status. It does not do optimistic locking. Uses JSON Patch to patch * the resource. *

- * Note that this does not work, if the {@link CustomResource#initStatus()} is - * implemented, since it breaks the diffing process. Don't implement it if using this method. + * Note that this does not work, if the {@link CustomResource#initStatus()} is implemented, since + * it breaks the diffing process. Don't implement it if using this method. *

- * There is also an issue with setting value to {@code null} with older Kubernetes versions (1.19 and - * below). See: https://github.com/fabric8io/kubernetes-client/issues/4158 * * @param resource type From 89f5e0e810d335ab7000b58d6791588d76a7280d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 8 Apr 2024 10:53:44 +0200 Subject: [PATCH 28/28] better expection message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/processing/event/ReconciliationDispatcher.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 10761cb1d4..f520c24438 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java @@ -340,7 +340,10 @@ private P addFinalizerWithSSA(P originalResource) { return customResourceFacade.patchResourceWithSSA(resource); } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { - throw new RuntimeException(e); + throw new RuntimeException("Issue with creating custom resource instance with reflection." + + " Custom Resources must provide a no-arg constructor. Class: " + + originalResource.getClass().getName(), + e); } }