From 302b169ea2607085cebaec4add78b93203305c59 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 17 May 2022 16:16:07 +0200 Subject: [PATCH 1/9] fix: null values deleted - using edit --- .../event/ReconciliationDispatcher.java | 67 ++++++++++--------- .../event/ReconciliationDispatcherTest.java | 8 +-- 2 files changed, 38 insertions(+), 37 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 3b9fa63156..ce60c21c62 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,9 @@ package io.javaoperatorsdk.operator.processing.event; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -9,6 +13,7 @@ import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperationsImpl; +import io.fabric8.kubernetes.client.utils.Serialization; import io.javaoperatorsdk.operator.api.ObservedGenerationAware; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; @@ -96,31 +101,19 @@ private PostExecutionControl handleReconcile( updateCustomResourceWithFinalizer(originalResource); return PostExecutionControl.onlyFinalizerAdded(); } else { + var resourceForExecution = + cloneResource(originalResource); try { - var resourceForExecution = - cloneResourceForErrorStatusHandlerIfNeeded(originalResource); return reconcileExecution(executionScope, resourceForExecution, originalResource, context); } catch (Exception e) { - return handleErrorStatusHandler(originalResource, context, e); + return handleErrorStatusHandler(resourceForExecution, originalResource, context, e); } } } - /** - * Resource make sense only to clone for the ErrorStatusHandler or if the observed generation in - * status is handled automatically. Otherwise, this operation can be skipped since it can be - * memory and time-consuming. However, it needs to be cloned since it's common that the custom - * resource is changed during an execution, and it's much cleaner to have to original resource in - * place for status update. - */ - private R cloneResourceForErrorStatusHandlerIfNeeded(R resource) { - if (isErrorStatusHandlerPresent() || - shouldUpdateObservedGenerationAutomatically(resource)) { - final var cloner = ConfigurationServiceProvider.instance().getResourceCloner(); - return cloner.clone(resource); - } else { - return resource; - } + private R cloneResource(R resource) { + final var cloner = ConfigurationServiceProvider.instance().getResourceCloner(); + return cloner.clone(resource); } private PostExecutionControl reconcileExecution(ExecutionScope executionScope, @@ -141,27 +134,31 @@ private PostExecutionControl reconcileExecution(ExecutionScope executionSc .getMetadata() .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion()); updatedCustomResource = - updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch()); + updateStatusGenerationAware(updateControl.getResource(), originalResource, + updateControl.isPatch()); } else if (updateControl.isUpdateStatus()) { updatedCustomResource = - updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch()); + updateStatusGenerationAware(updateControl.getResource(), originalResource, + updateControl.isPatch()); } else if (updateControl.isUpdateResource()) { updatedCustomResource = updateCustomResource(updateControl.getResource()); if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) { updatedCustomResource = - updateStatusGenerationAware(originalResource, updateControl.isPatch()); + updateStatusGenerationAware(updateControl.getResource(), originalResource, + updateControl.isPatch()); } } else if (updateControl.isNoUpdate() && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) { updatedCustomResource = - updateStatusGenerationAware(originalResource, updateControl.isPatch()); + updateStatusGenerationAware(originalResource, originalResource, updateControl.isPatch()); } return createPostExecutionControl(updatedCustomResource, updateControl); } @SuppressWarnings("unchecked") - private PostExecutionControl handleErrorStatusHandler(R resource, Context context, + private PostExecutionControl handleErrorStatusHandler(R resource, R originalResource, + Context context, Exception e) throws Exception { if (isErrorStatusHandlerPresent()) { try { @@ -183,7 +180,7 @@ public boolean isLastAttempt() { R updatedResource = null; if (errorStatusUpdateControl.getResource().isPresent()) { updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade - .patchStatus(errorStatusUpdateControl.getResource().orElseThrow()) + .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource) : customResourceFacade .updateStatus(errorStatusUpdateControl.getResource().orElseThrow()); } @@ -207,10 +204,10 @@ private boolean isErrorStatusHandlerPresent() { return controller.getReconciler() instanceof ErrorStatusHandler; } - private R updateStatusGenerationAware(R resource, boolean patch) { + private R updateStatusGenerationAware(R resource, R originalResource, boolean patch) { updateStatusObservedGenerationIfRequired(resource); if (patch) { - return customResourceFacade.patchStatus(resource); + return customResourceFacade.patchStatus(resource, originalResource); } else { return customResourceFacade.updateStatus(resource); } @@ -357,18 +354,22 @@ public R updateStatus(R resource) { return (R) hasMetadataOperation.replaceStatus(resource); } - public R patchStatus(R resource) { + public R patchStatus(R resource, R originalResource) { log.trace("Updating status for resource: {}", resource); String resourceVersion = resource.getMetadata().getResourceVersion(); - try { - // don't do optimistic locking on patch - resource.getMetadata().setResourceVersion(null); + // don't do optimistic locking on patch + originalResource.getMetadata().setResourceVersion(null); + resource.getMetadata().setResourceVersion(null); + try (var bis = new ByteArrayInputStream( + Serialization.asJson(originalResource).getBytes(StandardCharsets.UTF_8))) { return resourceOperation - .inNamespace(resource.getMetadata().getNamespace()) - .withName(getName(resource)) - .patchStatus(resource); + .load(bis) + .editStatus(r -> resource); + } catch (IOException e) { + throw new IllegalStateException(e); } finally { // restore initial resource version + originalResource.getMetadata().setResourceVersion(resourceVersion); resource.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 353759a05d..db2b52c295 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 @@ -142,7 +142,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(customResourceFacade, never()).replaceResourceWithLock(any()); } @@ -168,7 +168,7 @@ void patchesStatus() { reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(customResourceFacade, never()).updateStatus(any()); verify(customResourceFacade, never()).replaceResourceWithLock(any()); } @@ -362,7 +362,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception { when(reconciler.reconcile(any(), any())) .thenReturn(UpdateControl.patchStatus(observedGenResource)); - when(facade.patchStatus(observedGenResource)).thenReturn(observedGenResource); + when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource); PostExecutionControl control = dispatcher.handleExecution( executionScopeWithCREvent(observedGenResource)); @@ -521,7 +521,7 @@ void errorStatusHandlerCanPatchResource() { new ExecutionScope( testCustomResource, null)); - verify(customResourceFacade, times(1)).patchStatus(testCustomResource); + verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any()); verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource), any(), any()); } From ff1ad38d8d99f24b66ea3f98d11f249c69ace87a Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 17 May 2022 16:59:07 +0200 Subject: [PATCH 2/9] fix: namespace issue --- .../operator/processing/event/ReconciliationDispatcher.java | 1 + 1 file changed, 1 insertion(+) 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 ce60c21c62..861e212d81 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 @@ -363,6 +363,7 @@ public R patchStatus(R resource, R originalResource) { try (var bis = new ByteArrayInputStream( Serialization.asJson(originalResource).getBytes(StandardCharsets.UTF_8))) { return resourceOperation + .inNamespace(resource.getMetadata().getNamespace()) .load(bis) .editStatus(r -> resource); } catch (IOException e) { From a9178458006092ce83c699bc5d317cdc972c7837 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 17 May 2022 17:32:45 +0200 Subject: [PATCH 3/9] fix: tests --- .../changenamespace/ChangeNamespaceTestCustomResource.java | 4 ---- .../changenamespace/ChangeNamespaceTestReconciler.java | 4 +++- .../multiversioncrd/MultiVersionCRDTestCustomResource1.java | 4 ---- .../multiversioncrd/MultiVersionCRDTestCustomResource2.java | 5 ----- .../multiversioncrd/MultiVersionCRDTestReconciler1.java | 3 +++ .../multiversioncrd/MultiVersionCRDTestReconciler2.java | 3 +++ .../ObservedGenerationTestCustomResource.java | 5 ----- .../observedgeneration/ObservedGenerationTestReconciler.java | 3 +++ .../StatusPatchLockingCustomResource.java | 4 ---- .../statuspatchnonlocking/StatusPatchLockingReconciler.java | 3 +++ 10 files changed, 15 insertions(+), 23 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestCustomResource.java index 2b3b2295cd..a333dbe164 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestCustomResource.java @@ -11,8 +11,4 @@ public class ChangeNamespaceTestCustomResource extends CustomResource implements Namespaced { - @Override - protected ChangeNamespaceTestCustomResourceStatus initStatus() { - return new ChangeNamespaceTestCustomResourceStatus(); - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestReconciler.java index ed19eff39e..f984d30f33 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestReconciler.java @@ -45,7 +45,9 @@ public UpdateControl reconcile( } increaseNumberOfResourceExecutions(primary); - + if (primary.getStatus() == null) { + primary.setStatus(new ChangeNamespaceTestCustomResourceStatus()); + } var statusUpdates = primary.getStatus().getNumberOfStatusUpdates(); primary.getStatus().setNumberOfStatusUpdates(statusUpdates + 1); return UpdateControl.patchStatus(primary); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource1.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource1.java index 10236563c7..ea06b6a12e 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource1.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource1.java @@ -16,9 +16,5 @@ public class MultiVersionCRDTestCustomResource1 CustomResource implements Namespaced { - @Override - protected MultiVersionCRDTestCustomResourceStatus1 initStatus() { - return new MultiVersionCRDTestCustomResourceStatus1(); - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource2.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource2.java index de0a804049..ddd8ef48aa 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource2.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource2.java @@ -16,9 +16,4 @@ public class MultiVersionCRDTestCustomResource2 CustomResource implements Namespaced { - @Override - protected MultiVersionCRDTestCustomResourceStatus2 initStatus() { - return new MultiVersionCRDTestCustomResourceStatus2(); - } - } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler1.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler1.java index e0fed7985e..bd564e3d07 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler1.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler1.java @@ -20,6 +20,9 @@ public UpdateControl reconcile( Context context) { log.info("Reconcile MultiVersionCRDTestCustomResource1: {}", resource.getMetadata().getName()); + if (resource.getStatus() == null) { + resource.setStatus(new MultiVersionCRDTestCustomResourceStatus1()); + } resource.getStatus().setValue1(resource.getStatus().getValue1() + 1); if (!resource.getStatus().getReconciledBy().contains(getClass().getSimpleName())) { resource.getStatus().getReconciledBy().add(getClass().getSimpleName()); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler2.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler2.java index 6f78ff50cb..28f556afc0 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler2.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestReconciler2.java @@ -20,6 +20,9 @@ public UpdateControl reconcile( Context context) { log.info("Reconcile MultiVersionCRDTestCustomResource2: {}", resource.getMetadata().getName()); + if (resource.getStatus() == null) { + resource.setStatus(new MultiVersionCRDTestCustomResourceStatus2()); + } resource.getStatus().setValue1(resource.getStatus().getValue1() + 1); if (!resource.getStatus().getReconciledBy().contains(getClass().getSimpleName())) { resource.getStatus().getReconciledBy().add(getClass().getSimpleName()); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestCustomResource.java index 51e4a1113a..8a0ede3a5d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestCustomResource.java @@ -15,9 +15,4 @@ public class ObservedGenerationTestCustomResource extends CustomResource implements Namespaced { - @Override - protected ObservedGenerationTestCustomResourceStatus initStatus() { - return new ObservedGenerationTestCustomResourceStatus(); - } - } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestReconciler.java index fdc330ce28..7e2127ec7c 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/observedgeneration/ObservedGenerationTestReconciler.java @@ -20,6 +20,9 @@ public UpdateControl reconcile( Context context) { log.info("Reconcile ObservedGenerationTestCustomResource: {}", resource.getMetadata().getName()); + if (resource.getStatus() == null) { + resource.setStatus(new ObservedGenerationTestCustomResourceStatus()); + } return UpdateControl.patchStatus(resource); } } 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 146fb58bad..b443613f56 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 @@ -15,8 +15,4 @@ public class StatusPatchLockingCustomResource extends CustomResource implements Namespaced { - @Override - protected StatusPatchLockingCustomResourceStatus initStatus() { - return new StatusPatchLockingCustomResourceStatus(); - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java index 1f8ad7c635..7a0ebc5bd7 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java @@ -20,6 +20,9 @@ public UpdateControl reconcile( throws InterruptedException { numberOfExecutions.addAndGet(1); Thread.sleep(WAIT_TIME); + if (resource.getStatus() == null) { + resource.setStatus(new StatusPatchLockingCustomResourceStatus()); + } resource.getStatus().setValue(resource.getStatus().getValue() + 1); return UpdateControl.patchStatus(resource); } From 0c9819f95882c2456077a064b15b88916317c906 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 17 May 2022 17:47:50 +0200 Subject: [PATCH 4/9] test fix --- docs/documentation/features.md | 6 ++---- .../operator/api/reconciler/UpdateControl.java | 5 +++++ .../java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java | 6 ++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index 2ce952c45b..8cbf9ec841 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -152,10 +152,8 @@ In order to have this feature working: interface. See also the [`ObservedGenerationAwareStatus`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ObservedGenerationAwareStatus.java) which can also be extended. -- The other condition is that the `CustomResource.getStatus()` method should not return `null` - , but an instance of the class representing `status`. The best way to achieve this is to - override [`CustomResource.initStatus()`](https://github.com/fabric8io/kubernetes-client/blob/865e0ddf67b99f954aa55ab14e5806d53ae149ec/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java#L139) - . +- The other condition is that the `CustomResource.getStatus()` should not return `null`. So the status should be instantiated + when the object is returned using the `UpdateControl`. If these conditions are fulfilled and generation awareness not turned off, the observed generation is automatically set by the framework after the `reconcile` method is called. Note that the observed generation is updated also 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 c148cdb800..7c2b52ff2a 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,6 +1,7 @@ package io.javaoperatorsdk.operator.api.reconciler; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.CustomResource; public class UpdateControl extends BaseControl> { @@ -35,6 +36,10 @@ public static UpdateControl updateResource(T customRe /** * Preferred way to update the status. It does not do optimistic locking. + *

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

* * @param resource type * @param customResource the custom resource with target status diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java index f3d816961a..ed42ce3784 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java @@ -65,10 +65,8 @@ void invalidEventsDoesNotBreakEventHandling() { assertThat( operator .get(MultiVersionCRDTestCustomResource2.class, CR_V2_NAME) - .getStatus() - .getReconciledBy() - .size()) - .isZero(); + .getStatus()) + .isNull(); } From 80195c9e905954b277ed376f7385e667fa4c8a12 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 17 May 2022 17:52:12 +0200 Subject: [PATCH 5/9] docs --- .../javaoperatorsdk/operator/api/reconciler/UpdateControl.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 7c2b52ff2a..89d5e48314 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 @@ -35,7 +35,8 @@ public static UpdateControl updateResource(T customRe } /** - * Preferred way to update the status. It does not do optimistic locking. + * 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 * implemented, since it breaks the diffing process. Don't implement it if using this method. From 9a3f4e61f2e07b5be88d7911a170126d5c76d8d2 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 18 May 2022 09:23:36 +0200 Subject: [PATCH 6/9] added IT --- .../operator/StatusPatchNotLockingIT.java | 26 +++++++++++++++++++ .../StatusPatchLockingCustomResource.java | 3 ++- .../StatusPatchLockingCustomResourceSpec.java | 15 +++++++++++ ...tatusPatchLockingCustomResourceStatus.java | 11 ++++++++ .../StatusPatchLockingReconciler.java | 2 ++ 5 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceSpec.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 db736ee02d..fa17f49ee4 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java @@ -9,8 +9,10 @@ import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.javaoperatorsdk.operator.junit.LocalOperatorExtension; import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResource; +import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResourceSpec; import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingReconciler; +import static io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingReconciler.MESSAGE; import static io.javaoperatorsdk.operator.sample.statusupdatelocking.StatusUpdateLockingReconciler.WAIT_TIME; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; @@ -44,8 +46,32 @@ void noOptimisticLockingDoneOnStatusUpdate() throws InterruptedException { }); } + @Test + void valuesAreDeletedIfSetToNull() { + var resource = operator.create(StatusPatchLockingCustomResource.class, createResource()); + + await().untilAsserted(() -> { + var actual = operator.get(StatusPatchLockingCustomResource.class, + TEST_RESOURCE_NAME); + assertThat(actual.getStatus()).isNotNull(); + assertThat(actual.getStatus().getMessage()).isEqualTo(MESSAGE); + }); + + resource.getSpec().setMessageInStatus(false); + operator.replace(StatusPatchLockingCustomResource.class, resource); + + await().untilAsserted(() -> { + var actual = operator.get(StatusPatchLockingCustomResource.class, + TEST_RESOURCE_NAME); + assertThat(actual.getStatus()).isNotNull(); + assertThat(actual.getStatus().getMessage()).isNull(); + }); + } + + StatusPatchLockingCustomResource createResource() { StatusPatchLockingCustomResource res = new StatusPatchLockingCustomResource(); + res.setSpec(new StatusPatchLockingCustomResourceSpec()); res.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build()); return res; } 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 b443613f56..4d77259999 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 @@ -12,7 +12,8 @@ @Kind("StatusUpdateLockingCustomResource") @ShortNames("sul") public class StatusPatchLockingCustomResource - extends CustomResource + extends + CustomResource implements Namespaced { } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceSpec.java new file mode 100644 index 0000000000..3f0a59843e --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceSpec.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.statuspatchnonlocking; + +public class StatusPatchLockingCustomResourceSpec { + + private boolean messageInStatus = true; + + public boolean isMessageInStatus() { + return messageInStatus; + } + + public StatusPatchLockingCustomResourceSpec setMessageInStatus(boolean messageInStatus) { + this.messageInStatus = messageInStatus; + return this; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceStatus.java index c4b59bbbc8..1ab7abb4d0 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceStatus.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingCustomResourceStatus.java @@ -4,6 +4,17 @@ public class StatusPatchLockingCustomResourceStatus { private Integer value = 0; + private String message; + + public String getMessage() { + return message; + } + + public StatusPatchLockingCustomResourceStatus setMessage(String message) { + this.message = message; + return this; + } + public Integer getValue() { return value; } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java index 7a0ebc5bd7..21f83fc53f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/statuspatchnonlocking/StatusPatchLockingReconciler.java @@ -11,6 +11,7 @@ public class StatusPatchLockingReconciler implements Reconciler { + public static final String MESSAGE = "message"; public static final long WAIT_TIME = 500L; private final AtomicInteger numberOfExecutions = new AtomicInteger(0); @@ -23,6 +24,7 @@ public UpdateControl reconcile( if (resource.getStatus() == null) { resource.setStatus(new StatusPatchLockingCustomResourceStatus()); } + resource.getStatus().setMessage(resource.getSpec().isMessageInStatus() ? MESSAGE : null); resource.getStatus().setValue(resource.getStatus().getValue() + 1); return UpdateControl.patchStatus(resource); } From 3aff326d805774a2ec87387d84c4100bcddbb5a9 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 18 May 2022 10:17:19 +0200 Subject: [PATCH 7/9] fix: disabled test failing on old k8s instances --- .../io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java | 3 +++ 1 file changed, 3 insertions(+) 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 fa17f49ee4..6292da120c 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java @@ -3,6 +3,7 @@ import java.time.Duration; import java.util.Map; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -46,6 +47,8 @@ void noOptimisticLockingDoneOnStatusUpdate() throws InterruptedException { }); } + // see https://github.com/fabric8io/kubernetes-client/issues/4158 + @Disabled("Patch generated supported by K8S version below 1.20.x") @Test void valuesAreDeletedIfSetToNull() { var resource = operator.create(StatusPatchLockingCustomResource.class, createResource()); From 24244a71b9e4187bcd4719a0bf5a5cf6d11a63ff Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 18 May 2022 10:19:21 +0200 Subject: [PATCH 8/9] fixes --- .../javaoperatorsdk/operator/api/reconciler/UpdateControl.java | 3 +++ 1 file changed, 3 insertions(+) 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 89d5e48314..62f9bc0cd2 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 @@ -41,6 +41,9 @@ public static UpdateControl updateResource(T customRe * Note that this does not work, if the {@link CustomResource#initStatus() 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 + * below). See: https://github.com/fabric8io/kubernetes-client/issues/4158 * * @param resource type * @param customResource the custom resource with target status From 1c96e553b9cc66dafe1e174084b25a9b745b7493 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 18 May 2022 14:31:14 +0200 Subject: [PATCH 9/9] comment --- .../operator/processing/event/ReconciliationDispatcher.java | 1 + 1 file changed, 1 insertion(+) 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 861e212d81..600ebee221 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 @@ -364,6 +364,7 @@ public R patchStatus(R resource, R originalResource) { Serialization.asJson(originalResource).getBytes(StandardCharsets.UTF_8))) { return resourceOperation .inNamespace(resource.getMetadata().getNamespace()) + // will be simplified in fabric8 v6 .load(bis) .editStatus(r -> resource); } catch (IOException e) {