From 1142a88b501b7d7dfcbf94fab0345991782d6d02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 11:19:03 +0100 Subject: [PATCH 1/9] feat: use SSA for resource status patching 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 | 16 ++++++++- .../event/ReconciliationDispatcher.java | 35 +++++++++++++------ .../event/ReconciliationDispatcherTest.java | 8 ++--- 4 files changed, 50 insertions(+), 16 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 53bfc75df9..881f2cbaac 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 @@ -391,4 +391,11 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() { return false; } + /** + * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus + */ + default boolean useSSAForResourceStatusPatch() { + 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 5879383464..6116af5c0f 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 @@ -40,6 +40,7 @@ public class ConfigurationServiceOverrider { private Set> defaultNonSSAResource; private Boolean previousAnnotationForDependentResources; private Boolean parseResourceVersions; + private Boolean useSSAForResourceStatusPatch; private DependentResourceFactory dependentResourceFactory; ConfigurationServiceOverrider(ConfigurationService original) { @@ -174,12 +175,17 @@ public ConfigurationServiceOverrider withPreviousAnnotationForDependentResources return this; } - public ConfigurationServiceOverrider wihtParseResourceVersions( + public ConfigurationServiceOverrider withParseResourceVersions( boolean value) { this.parseResourceVersions = value; return this; } + public ConfigurationServiceOverrider withUseSSAForResourceStatusPatch(boolean value) { + this.useSSAForResourceStatusPatch = value; + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, client) { @Override @@ -312,6 +318,14 @@ public boolean parseResourceVersionsForEventFilteringAndCaching() { ? parseResourceVersions : super.parseResourceVersionsForEventFilteringAndCaching(); } + + @Override + public boolean useSSAForResourceStatusPatch() { + return useSSAForResourceStatusPatch != null + ? useSSAForResourceStatusPatch + : super.useSSAForResourceStatusPatch(); + + } }; } 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 cd1d601487..d91c6084d6 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 @@ -12,6 +12,8 @@ import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; +import io.fabric8.kubernetes.client.dsl.base.PatchContext; +import io.fabric8.kubernetes.client.dsl.base.PatchType; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.ObservedGenerationAware; import io.javaoperatorsdk.operator.api.config.Cloner; @@ -147,24 +149,24 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); updatedCustomResource = updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus()); + updateControl.isPatchStatus(), context); } else if (updateControl.isUpdateStatus()) { updatedCustomResource = updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus()); + updateControl.isPatchStatus(), context); } else if (updateControl.isUpdateResource()) { updatedCustomResource = updateCustomResource(updateControl.getResource()); if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) { updatedCustomResource = updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus()); + updateControl.isPatchStatus(), context); } } else if (updateControl.isNoUpdate() && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) { updatedCustomResource = updateStatusGenerationAware(originalResource, originalResource, - updateControl.isPatchStatus()); + updateControl.isPatchStatus(), context); } return createPostExecutionControl(updatedCustomResource, updateControl); } @@ -195,7 +197,8 @@ public boolean isLastAttempt() { P updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade - .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource) + .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource, + context) : customResourceFacade .updateStatus(errorStatusUpdateControl.getResource().orElseThrow()); } @@ -223,10 +226,11 @@ private boolean isErrorStatusHandlerPresent() { return controller.getReconciler() instanceof ErrorStatusHandler; } - private P updateStatusGenerationAware(P resource, P originalResource, boolean patch) { + private P updateStatusGenerationAware(P resource, P originalResource, boolean patch, + Context

context) { updateStatusObservedGenerationIfRequired(resource); if (patch) { - return customResourceFacade.patchStatus(resource, originalResource); + return customResourceFacade.patchStatus(resource, originalResource, context); } else { return customResourceFacade.updateStatus(resource); } @@ -411,15 +415,24 @@ public R updateStatus(R resource) { .updateStatus(); } - public R patchStatus(R resource, R originalResource) { - log.trace("Updating status for resource: {}", resource); + public R patchStatus(R resource, R originalResource, Context context) { + var useSSA = context.getControllerConfiguration().getConfigurationService() + .useSSAForResourceStatusPatch(); + 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 { - return resource(originalResource) - .editStatus(r -> resource); + var res = resource(originalResource); + if (useSSA) { + return res.subresource("status").patch(new PatchContext.Builder() + .withFieldManager(context.getControllerConfiguration().fieldManager()) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + } else { + return res.editStatus(r -> resource); + } } finally { // restore initial resource version originalResource.getMetadata().setResourceVersion(resourceVersion); 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 c6aacb9071..2d7adbb79d 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 @@ -158,7 +158,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); verify(customResourceFacade, never()).updateResource(any()); } @@ -184,7 +184,7 @@ void patchesStatus() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); verify(customResourceFacade, never()).updateStatus(any()); verify(customResourceFacade, never()).updateResource(any()); } @@ -456,7 +456,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception { when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.patchStatus(observedGenResource)); - when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource); + when(facade.patchStatus(eq(observedGenResource), any(), any())).thenReturn(observedGenResource); PostExecutionControl control = dispatcher.handleExecution( executionScopeWithCREvent(observedGenResource)); @@ -609,7 +609,7 @@ void errorStatusHandlerCanPatchResource() { reconciliationDispatcher.handleExecution( new ExecutionScope(null).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } From 9eccbf4fdebad6738e6098cee5f9a6689c5d07ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 11:28:38 +0100 Subject: [PATCH 2/9] 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 | 5 ++++- .../operator/api/config/ConfigurationService.java | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index f69625afad..97f7882833 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -17,5 +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. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed +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) +4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed via the accordingly renamed `managedWorkflowAndDependentResourceContext` method. 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 881f2cbaac..a55502a691 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 @@ -392,7 +392,10 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() { } /** - * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus + * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus can either use + * simple update or SSA for status subresource patching. + * + * @return true by default */ default boolean useSSAForResourceStatusPatch() { return true; From 3f358c45aff80208635aa91a2bef0cdc86147101 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 12:52:25 +0100 Subject: [PATCH 3/9] managed fields fix 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 | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 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 d91c6084d6..1963667567 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,5 +1,6 @@ package io.javaoperatorsdk.operator.processing.event; +import java.util.Collections; import java.util.function.Function; import org.slf4j.Logger; @@ -424,13 +425,22 @@ public R patchStatus(R resource, R originalResource, Context context) { originalResource.getMetadata().setResourceVersion(null); resource.getMetadata().setResourceVersion(null); try { - var res = resource(originalResource); if (useSSA) { - return res.subresource("status").patch(new PatchContext.Builder() - .withFieldManager(context.getControllerConfiguration().fieldManager()) - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build()); + var managedFields = resource.getMetadata().getManagedFields(); + try { + + resource.getMetadata().setManagedFields(Collections.emptyList()); + var res = resource(resource); + return res.subresource("status").patch(new PatchContext.Builder() + .withFieldManager(context.getControllerConfiguration().fieldManager()) + .withForce(true) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + } finally { + resource.getMetadata().setManagedFields(managedFields); + } } else { + var res = resource(originalResource); return res.editStatus(r -> resource); } } finally { From 1b3626f63cb0c1957db20c6add693de80c3fdeb6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 11 Mar 2024 15:20:58 +0100 Subject: [PATCH 4/9] refactor: simplify Signed-off-by: Chris Laprun --- .../event/ReconciliationDispatcher.java | 71 ++++++++----------- .../event/ReconciliationDispatcherTest.java | 22 ++---- 2 files changed, 37 insertions(+), 56 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 1963667567..431aaee049 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 @@ -28,9 +28,7 @@ import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.processing.Controller; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; -import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; +import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.*; /** * Handles calls and results of a Reconciler and finalizer related logic @@ -48,8 +46,7 @@ class ReconciliationDispatcher

{ private final boolean retryConfigurationHasZeroAttempts; private final Cloner cloner; - ReconciliationDispatcher(Controller

controller, - CustomResourceFacade

customResourceFacade) { + ReconciliationDispatcher(Controller

controller, CustomResourceFacade

customResourceFacade) { this.controller = controller; this.customResourceFacade = customResourceFacade; this.cloner = controller.getConfiguration().getConfigurationService().getResourceCloner(); @@ -59,7 +56,8 @@ class ReconciliationDispatcher

{ } public ReconciliationDispatcher(Controller

controller) { - this(controller, new CustomResourceFacade<>(controller.getCRClient())); + this(controller, + new CustomResourceFacade<>(controller.getCRClient(), controller.getConfiguration())); } public PostExecutionControl

handleExecution(ExecutionScope

executionScope) { @@ -141,33 +139,24 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc UpdateControl

updateControl = controller.reconcile(resourceForExecution, context); P updatedCustomResource = null; + final var toUpdate = + updateControl.isNoUpdate() ? originalResource : updateControl.getResource(); if (updateControl.isUpdateResourceAndStatus()) { - updatedCustomResource = - updateCustomResource(updateControl.getResource()); - updateControl - .getResource() + updatedCustomResource = updateCustomResource(toUpdate); + toUpdate .getMetadata() .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); - updatedCustomResource = - updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus(), context); - } else if (updateControl.isUpdateStatus()) { - updatedCustomResource = - updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus(), context); } else if (updateControl.isUpdateResource()) { + updatedCustomResource = updateCustomResource(toUpdate); + } + + // check if status also needs to be updated + final var updateObservedGeneration = updateControl.isNoUpdate() ? + shouldUpdateObservedGenerationAutomatically(resourceForExecution) : + shouldUpdateObservedGenerationAutomatically(updatedCustomResource); + if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus() || updateObservedGeneration) { updatedCustomResource = - updateCustomResource(updateControl.getResource()); - if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) { - updatedCustomResource = - updateStatusGenerationAware(updateControl.getResource(), originalResource, - updateControl.isPatchStatus(), context); - } - } else if (updateControl.isNoUpdate() - && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) { - updatedCustomResource = - updateStatusGenerationAware(originalResource, originalResource, - updateControl.isPatchStatus(), context); + updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus()); } return createPostExecutionControl(updatedCustomResource, updateControl); } @@ -198,8 +187,7 @@ public boolean isLastAttempt() { P updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade - .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource, - context) + .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource) : customResourceFacade .updateStatus(errorStatusUpdateControl.getResource().orElseThrow()); } @@ -227,11 +215,10 @@ private boolean isErrorStatusHandlerPresent() { return controller.getReconciler() instanceof ErrorStatusHandler; } - private P updateStatusGenerationAware(P resource, P originalResource, boolean patch, - Context

context) { + private P updateStatusGenerationAware(P resource, P originalResource, boolean patch) { updateStatusObservedGenerationIfRequired(resource); if (patch) { - return customResourceFacade.patchStatus(resource, originalResource, context); + return customResourceFacade.patchStatus(resource, originalResource); } else { return customResourceFacade.updateStatus(resource); } @@ -384,10 +371,16 @@ public P conflictRetryingUpdate(P resource, Function modificationFun static class CustomResourceFacade { private final MixedOperation, Resource> resourceOperation; + private final boolean useSSAToUpdateStatus; + private final String fieldManager; public CustomResourceFacade( - MixedOperation, Resource> resourceOperation) { + MixedOperation, Resource> resourceOperation, + ControllerConfiguration configuration) { this.resourceOperation = resourceOperation; + this.useSSAToUpdateStatus = + configuration.getConfigurationService().useSSAForResourceStatusPatch(); + this.fieldManager = configuration.fieldManager(); } public R getResource(String namespace, String name) { @@ -416,23 +409,21 @@ public R updateStatus(R resource) { .updateStatus(); } - public R patchStatus(R resource, R originalResource, Context context) { - var useSSA = context.getControllerConfiguration().getConfigurationService() - .useSSAForResourceStatusPatch(); - log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA); + public R patchStatus(R resource, R originalResource) { + log.trace("Patching status for resource: {} with ssa: {}", resource, useSSAToUpdateStatus); String resourceVersion = resource.getMetadata().getResourceVersion(); // don't do optimistic locking on patch originalResource.getMetadata().setResourceVersion(null); resource.getMetadata().setResourceVersion(null); try { - if (useSSA) { + if (useSSAToUpdateStatus) { var managedFields = resource.getMetadata().getManagedFields(); try { resource.getMetadata().setManagedFields(Collections.emptyList()); var res = resource(resource); return res.subresource("status").patch(new PatchContext.Builder() - .withFieldManager(context.getControllerConfiguration().fieldManager()) + .withFieldManager(fieldManager) .withForce(true) .withPatchType(PatchType.SERVER_SIDE_APPLY) .build()); 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 2d7adbb79d..d2e4cd52dc 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 @@ -43,19 +43,9 @@ import static io.javaoperatorsdk.operator.TestUtils.markForDeletion; import static io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.MAX_UPDATE_RETRY; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @SuppressWarnings({"unchecked", "rawtypes"}) class ReconciliationDispatcherTest { @@ -158,7 +148,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(customResourceFacade, never()).updateResource(any()); } @@ -184,7 +174,7 @@ void patchesStatus() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(customResourceFacade, never()).updateStatus(any()); verify(customResourceFacade, never()).updateResource(any()); } @@ -456,7 +446,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception { when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.patchStatus(observedGenResource)); - when(facade.patchStatus(eq(observedGenResource), any(), any())).thenReturn(observedGenResource); + when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource); PostExecutionControl control = dispatcher.handleExecution( executionScopeWithCREvent(observedGenResource)); @@ -609,7 +599,7 @@ void errorStatusHandlerCanPatchResource() { reconciliationDispatcher.handleExecution( new ExecutionScope(null).setResource(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any(), any()); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } From 871f5b7c61bf041805faec5dcb2fd9b273e2523a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 15:29:35 +0100 Subject: [PATCH 5/9] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../processing/event/ReconciliationDispatcher.java | 9 +++++---- .../StatusPatchLockingCustomResource.java | 2 -- 2 files changed, 5 insertions(+), 6 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 431aaee049..32f55b9bc8 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,10 +151,11 @@ private PostExecutionControl

reconcileExecution(ExecutionScope

executionSc } // check if status also needs to be updated - final var updateObservedGeneration = updateControl.isNoUpdate() ? - shouldUpdateObservedGenerationAutomatically(resourceForExecution) : - shouldUpdateObservedGenerationAutomatically(updatedCustomResource); - if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus() || updateObservedGeneration) { + final var updateObservedGeneration = updateControl.isNoUpdate() + ? shouldUpdateObservedGenerationAutomatically(resourceForExecution) + : shouldUpdateObservedGenerationAutomatically(updatedCustomResource); + if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus() + || updateObservedGeneration) { updatedCustomResource = updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus()); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java index 4d77259999..643c9b0846 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java @@ -3,13 +3,11 @@ 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.Kind; import io.fabric8.kubernetes.model.annotation.ShortNames; import io.fabric8.kubernetes.model.annotation.Version; @Group("sample.javaoperatorsdk") @Version("v1") -@Kind("StatusUpdateLockingCustomResource") @ShortNames("sul") public class StatusPatchLockingCustomResource extends From b4ce0cd7b95aabb8c1f941c993dfac9934d428c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 16:38:40 +0100 Subject: [PATCH 6/9] fix IT 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 | 3 +-- .../javaoperatorsdk/operator/StatusPatchNotLockingIT.java | 6 ++++-- 2 files changed, 5 insertions(+), 4 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 32f55b9bc8..a240827d14 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,6 +1,5 @@ package io.javaoperatorsdk.operator.processing.event; -import java.util.Collections; import java.util.function.Function; import org.slf4j.Logger; @@ -421,7 +420,7 @@ public R patchStatus(R resource, R originalResource) { var managedFields = resource.getMetadata().getManagedFields(); try { - resource.getMetadata().setManagedFields(Collections.emptyList()); + resource.getMetadata().setManagedFields(null); var res = resource(resource); return res.subresource("status").patch(new PatchContext.Builder() .withFieldManager(fieldManager) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java index 1746e5737d..482d8b3d92 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java @@ -48,7 +48,7 @@ void noOptimisticLockingDoneOnStatusUpdate() throws InterruptedException { // see https://github.com/fabric8io/kubernetes-client/issues/4158 @Test - void valuesAreDeletedIfSetToNull() { + void valuesAreDeletedIfSetToNull() throws InterruptedException { var resource = operator.create(createResource()); await().untilAsserted(() -> { @@ -58,10 +58,12 @@ void valuesAreDeletedIfSetToNull() { assertThat(actual.getStatus().getMessage()).isEqualTo(MESSAGE); }); + // resource needs to be read again to we don't replace the with wrong managed fields + resource = operator.get(StatusPatchLockingCustomResource.class, TEST_RESOURCE_NAME); resource.getSpec().setMessageInStatus(false); operator.replace(resource); - await().untilAsserted(() -> { + await().timeout(Duration.ofMinutes(3)).untilAsserted(() -> { var actual = operator.get(StatusPatchLockingCustomResource.class, TEST_RESOURCE_NAME); assertThat(actual.getStatus()).isNotNull(); From 0f02f86993cb7cf10cc983aeca35599fd2620168 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 11 Mar 2024 19:04:16 +0100 Subject: [PATCH 7/9] fix on short name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../statuspatchnonlocking/StatusPatchLockingCustomResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java index 643c9b0846..93ec97884d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResource.java @@ -8,7 +8,7 @@ @Group("sample.javaoperatorsdk") @Version("v1") -@ShortNames("sul") +@ShortNames("spl") public class StatusPatchLockingCustomResource extends CustomResource From 6c99a78f90af3ff877ff01419dfa3a0c4c56601b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 12 Mar 2024 17:20:36 +0100 Subject: [PATCH 8/9] Integration test skeleton for SSA handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/StatusPatchNotLockingIT.java | 2 +- .../operator/StatusPatchSSAMigrationIT.java | 110 ++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java index 482d8b3d92..0a82bed51a 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java @@ -48,7 +48,7 @@ void noOptimisticLockingDoneOnStatusUpdate() throws InterruptedException { // see https://github.com/fabric8io/kubernetes-client/issues/4158 @Test - void valuesAreDeletedIfSetToNull() throws InterruptedException { + void valuesAreDeletedIfSetToNull() { var resource = operator.create(createResource()); await().untilAsserted(() -> { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java new file mode 100644 index 0000000000..e6c9dcb6dd --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java @@ -0,0 +1,110 @@ +package io.javaoperatorsdk.operator; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; + +import io.fabric8.kubernetes.api.model.Namespace; +import io.fabric8.kubernetes.api.model.NamespaceBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResource; +import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResourceSpec; +import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingReconciler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +public class StatusPatchSSAMigrationIT { + + public static final String TEST_RESOURCE_NAME = "test"; + + private final KubernetesClient client = new KubernetesClientBuilder().build(); + private String testNamespace; + + @BeforeEach + void beforeEach(TestInfo testInfo) { + LocallyRunOperatorExtension.applyCrd(StatusPatchLockingCustomResource.class, + client); + testInfo.getTestMethod() + .ifPresent(method -> testNamespace = KubernetesResourceUtil.sanitizeName(method.getName())); + client.namespaces().resource(testNamespace(testNamespace)).create(); + } + + @AfterEach + void afterEach() { + client.namespaces().withName(testNamespace).delete(); + await().untilAsserted(() -> { + var ns = client.namespaces().withName(testNamespace).get(); + assertThat(ns).isNull(); + }); + client.close(); + } + + @Test + void testMigratingFromNonSSAToSSA() { + var operator = startOperator(false); + var testResource = client.resource(testResource()).create(); + + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + assertThat(res.getStatus().getMessage()).isEqualTo(StatusPatchLockingReconciler.MESSAGE); + assertThat(res.getStatus().getValue()).isEqualTo(1); + }); + operator.stop(); + + // start operator with SSA + operator = startOperator(true); + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + assertThat(res.getStatus().getMessage()).isEqualTo(StatusPatchLockingReconciler.MESSAGE); + assertThat(res.getStatus().getValue()).isEqualTo(2); + }); + + var actualResource = client.resource(testResource()).get(); + actualResource.getSpec().setMessageInStatus(false); + client.resource(actualResource).update(); + + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + assertThat(res.getStatus().getMessage()).isNull(); + assertThat(res.getStatus().getValue()).isEqualTo(3); + }); + + client.resource(testResource()).delete(); + } + + + private Operator startOperator(boolean patchStatusWithSSA) { + var operator = new Operator(o -> o.withCloseClientOnStop(false) + .withUseSSAForResourceStatusPatch(patchStatusWithSSA)); + operator.register(new StatusPatchLockingReconciler(), + o -> o.settingNamespaces(testNamespace)); + + operator.start(); + return operator; + } + + StatusPatchLockingCustomResource testResource() { + StatusPatchLockingCustomResource res = new StatusPatchLockingCustomResource(); + res.setSpec(new StatusPatchLockingCustomResourceSpec()); + res.setMetadata(new ObjectMetaBuilder() + .withName(TEST_RESOURCE_NAME) + .withNamespace(testNamespace) + .build()); + return res; + } + + private Namespace testNamespace(String name) { + return new NamespaceBuilder().withMetadata(new ObjectMetaBuilder() + .withName(name) + .build()).build(); + } +} From 31fbd2c874c467554f962243eb6b104e3ba37faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 18 Mar 2024 10:32:44 +0100 Subject: [PATCH 9/9] Integration test 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 | 4 ++ .../operator/StatusPatchSSAMigrationIT.java | 47 ++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/docs/documentation/v5-0-migration.md b/docs/documentation/v5-0-migration.md index 97f7882833..b34a7a6b21 100644 --- a/docs/documentation/v5-0-migration.md +++ b/docs/documentation/v5-0-migration.md @@ -20,5 +20,9 @@ permalink: /docs/v5-0-migration 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) + !!! 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) 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. 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 e6c9dcb6dd..01702a5400 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java @@ -45,8 +45,9 @@ void afterEach() { client.close(); } + @Test - void testMigratingFromNonSSAToSSA() { + void testMigratingToSSA() { var operator = startOperator(false); var testResource = client.resource(testResource()).create(); @@ -71,6 +72,49 @@ void testMigratingFromNonSSAToSSA() { actualResource.getSpec().setMessageInStatus(false); client.resource(actualResource).update(); + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + // !!! This is wrong, the message should be null, + // see issue in Kubernetes: https://github.com/kubernetes/kubernetes/issues/99003 + assertThat(res.getStatus().getMessage()).isNotNull(); + assertThat(res.getStatus().getValue()).isEqualTo(3); + }); + + client.resource(testResource()).delete(); + operator.stop(); + } + + @Test + void workaroundMigratingFromToSSA() { + var operator = startOperator(false); + var testResource = client.resource(testResource()).create(); + + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + assertThat(res.getStatus().getMessage()).isEqualTo(StatusPatchLockingReconciler.MESSAGE); + assertThat(res.getStatus().getValue()).isEqualTo(1); + }); + operator.stop(); + + // start operator with SSA + operator = startOperator(true); + await().untilAsserted(() -> { + var res = client.resource(testResource).get(); + assertThat(res.getStatus()).isNotNull(); + assertThat(res.getStatus().getMessage()).isEqualTo(StatusPatchLockingReconciler.MESSAGE); + assertThat(res.getStatus().getValue()).isEqualTo(2); + }); + + var actualResource = client.resource(testResource()).get(); + actualResource.getSpec().setMessageInStatus(false); + // removing the managed field entry for former method works + actualResource.getMetadata().setManagedFields(actualResource.getMetadata().getManagedFields() + .stream().filter(r -> !r.getOperation().equals("Update") && r.getSubresource() != null) + .toList()); + client.resource(actualResource).update(); + await().untilAsserted(() -> { var res = client.resource(testResource).get(); assertThat(res.getStatus()).isNotNull(); @@ -79,6 +123,7 @@ void testMigratingFromNonSSAToSSA() { }); client.resource(testResource()).delete(); + operator.stop(); }