diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java index b9ef475509..205d389a95 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java @@ -31,6 +31,7 @@ import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; import io.javaoperatorsdk.operator.processing.event.source.informer.ManagedInformerEventSource; +import static io.javaoperatorsdk.operator.api.reconciler.Experimental.API_MIGHT_CHANGE; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion; @@ -68,6 +69,11 @@ public ResourceOperations(Context

context) { * @param resource type */ public R serverSideApply(R resource) { + return serverSideApply(resource, (Options) null); + } + + @Experimental(API_MIGHT_CHANGE) + public R serverSideApply(R resource, Options options) { return resourcePatch( resource, r -> @@ -79,7 +85,13 @@ public R serverSideApply(R resource) { .withForce(true) .withFieldManager(context.getControllerConfiguration().fieldManager()) .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build())); + .build()), + options); + } + + public R serverSideApply( + R resource, InformerEventSource informerEventSource) { + return serverSideApply(resource, informerEventSource, null); } /** @@ -96,8 +108,9 @@ public R serverSideApply(R resource) { * @param informerEventSource InformerEventSource to use for resource caching and filtering * @param resource type */ + @Experimental(API_MIGHT_CHANGE) public R serverSideApply( - R resource, InformerEventSource informerEventSource) { + R resource, InformerEventSource informerEventSource, Options options) { if (informerEventSource == null) { return serverSideApply(resource); } @@ -113,7 +126,8 @@ public R serverSideApply( .withFieldManager(context.getControllerConfiguration().fieldManager()) .withPatchType(PatchType.SERVER_SIDE_APPLY) .build()), - informerEventSource); + informerEventSource, + options); } /** @@ -131,7 +145,7 @@ public R serverSideApply( * @return updated resource * @param resource type */ - public R serverSideApplyStatus(R resource) { + public R serverSideApplyStatus(R resource, Options options) { return resourcePatch( resource, r -> @@ -144,7 +158,12 @@ public R serverSideApplyStatus(R resource) { .withForce(true) .withFieldManager(context.getControllerConfiguration().fieldManager()) .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build())); + .build()), + options); + } + + public R serverSideApplyStatus(R resource) { + return serverSideApplyStatus(resource, null); } /** @@ -208,6 +227,10 @@ public P serverSideApplyPrimaryStatus(P resource) { context.eventSourceRetriever().getControllerEventSource()); } + public R update(R resource) { + return update(resource, (Options) null); + } + /** * Updates the resource and caches the response if needed, thus making sure that next * reconciliation will see to updated resource - or more recent one if additional update happened @@ -221,8 +244,14 @@ public P serverSideApplyPrimaryStatus(P resource) { * @return updated resource * @param resource type */ - public R update(R resource) { - return resourcePatch(resource, r -> context.getClient().resource(r).update()); + @Experimental(API_MIGHT_CHANGE) + public R update(R resource, Options options) { + return resourcePatch(resource, r -> context.getClient().resource(r).update(), options); + } + + public R update( + R resource, InformerEventSource informerEventSource) { + return update(resource, informerEventSource, new Options(true)); } /** @@ -239,13 +268,14 @@ public R update(R resource) { * @param informerEventSource InformerEventSource to use for resource caching and filtering * @param resource type */ + @Experimental(API_MIGHT_CHANGE) public R update( - R resource, InformerEventSource informerEventSource) { + R resource, InformerEventSource informerEventSource, Options options) { if (informerEventSource == null) { return update(resource); } return resourcePatch( - resource, r -> context.getClient().resource(r).update(), informerEventSource); + resource, r -> context.getClient().resource(r).update(), informerEventSource, options); } /** @@ -262,7 +292,8 @@ public R update( * @param resource type */ public R create(R resource) { - return resourcePatch(resource, r -> context.getClient().resource(r).create()); + return resourcePatch( + resource, r -> context.getClient().resource(r).create(), new Options(true)); } /** @@ -284,8 +315,12 @@ public R create( if (informerEventSource == null) { return create(resource); } + // it is safe to do event filtering for create since check if the resource already exists. return resourcePatch( - resource, r -> context.getClient().resource(r).create(), informerEventSource); + resource, + r -> context.getClient().resource(r).create(), + informerEventSource, + new Options(true)); } /** @@ -304,7 +339,8 @@ public R create( * @param resource type */ public R updateStatus(R resource) { - return resourcePatch(resource, r -> context.getClient().resource(r).updateStatus()); + return resourcePatch( + resource, r -> context.getClient().resource(r).updateStatus(), new Options(true)); } /** @@ -367,7 +403,13 @@ public P updatePrimaryStatus(P resource) { * @param resource type */ public R jsonPatch(R resource, UnaryOperator unaryOperator) { - return resourcePatch(resource, r -> context.getClient().resource(r).edit(unaryOperator)); + return jsonPatch(resource, unaryOperator, null); + } + + public R jsonPatch( + R resource, UnaryOperator unaryOperator, Options options) { + return resourcePatch( + resource, r -> context.getClient().resource(r).edit(unaryOperator), options); } /** @@ -387,8 +429,14 @@ public R jsonPatch(R resource, UnaryOperator unaryOpe * @return updated resource * @param resource type */ + public R jsonPatchStatus( + R resource, UnaryOperator unaryOperator, Options options) { + return resourcePatch( + resource, r -> context.getClient().resource(r).editStatus(unaryOperator), options); + } + public R jsonPatchStatus(R resource, UnaryOperator unaryOperator) { - return resourcePatch(resource, r -> context.getClient().resource(r).editStatus(unaryOperator)); + return jsonPatchStatus(resource, unaryOperator, null); } /** @@ -452,7 +500,11 @@ public P jsonPatchPrimaryStatus(P resource, UnaryOperator

unaryOperator) { * @param resource type */ public R jsonMergePatch(R resource) { - return resourcePatch(resource, r -> context.getClient().resource(r).patch()); + return jsonMergePatch(resource, null); + } + + public R jsonMergePatch(R resource, Options options) { + return resourcePatch(resource, r -> context.getClient().resource(r).patch(), options); } /** @@ -471,7 +523,11 @@ public R jsonMergePatch(R resource) { * @param resource type */ public R jsonMergePatchStatus(R resource) { - return resourcePatch(resource, r -> context.getClient().resource(r).patchStatus()); + return jsonMergePatchStatus(resource, null); + } + + public R jsonMergePatchStatus(R resource, Options options) { + return resourcePatch(resource, r -> context.getClient().resource(r).patchStatus(), options); } /** @@ -518,6 +574,10 @@ public P jsonMergePatchPrimaryStatus(P resource) { context.eventSourceRetriever().getControllerEventSource()); } + public R resourcePatch(R resource, UnaryOperator updateOperation) { + return resourcePatch(resource, updateOperation, (Options) null); + } + /** * Utility method to patch a resource and cache the result. Automatically discovers the event * source for the resource type and delegates to {@link #resourcePatch(HasMetadata, UnaryOperator, @@ -529,8 +589,10 @@ public P jsonMergePatchPrimaryStatus(P resource) { * @param resource type * @throws IllegalStateException if no event source or multiple event sources are found */ + @Experimental(API_MIGHT_CHANGE) @SuppressWarnings({"rawtypes", "unchecked"}) - public R resourcePatch(R resource, UnaryOperator updateOperation) { + public R resourcePatch( + R resource, UnaryOperator updateOperation, Options options) { var esList = context.eventSourceRetriever().getEventSourcesFor(resource.getClass()); if (esList.isEmpty()) { @@ -544,7 +606,8 @@ public R resourcePatch(R resource, UnaryOperator upda es.name()); } if (es instanceof ManagedInformerEventSource mes) { - return resourcePatch(resource, updateOperation, (ManagedInformerEventSource) mes); + return resourcePatch( + resource, updateOperation, (ManagedInformerEventSource) mes, options); } else { throw new IllegalStateException( "Target event source must be a subclass off " @@ -565,7 +628,16 @@ public R resourcePatch(R resource, UnaryOperator upda */ public R resourcePatch( R resource, UnaryOperator updateOperation, ManagedInformerEventSource ies) { - return ies.eventFilteringUpdateAndCacheResource(resource, updateOperation); + return resourcePatch(resource, updateOperation, ies, (Options) null); + } + + public R resourcePatch( + R resource, + UnaryOperator updateOperation, + ManagedInformerEventSource ies, + Options options) { + return ies.eventFilteringUpdateAndCacheResource( + resource, updateOperation, options != null && options.forceEventFiltering()); } /** @@ -754,4 +826,12 @@ public P addFinalizerWithSSA(String finalizerName) { e); } } + + /** + * Force filtering only if it is made sure that the update not results on a no-op change. See + * details here: PR + * 3484 + */ + @Experimental("This API might change") + public record Options(boolean forceEventFiltering) {} } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index f8d7c07b01..b69cf933a4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -30,6 +30,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.api.reconciler.Ignore; +import io.javaoperatorsdk.operator.api.reconciler.ResourceOperations; import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ConfiguredDependentResource; import io.javaoperatorsdk.operator.processing.GroupVersionKind; @@ -90,7 +91,6 @@ public R create(R desired, P primary, Context

context) { desired.getClass(), ResourceID.fromResource(desired), ssa); - return ssa ? context.resourceOperations().serverSideApply(desired, eventSource().orElse(null)) : context.resourceOperations().create(desired, eventSource().orElse(null)); @@ -114,7 +114,10 @@ public R update(R actual, R desired, P primary, Context

context) { ssa); if (ssa) { updatedResource = - context.resourceOperations().serverSideApply(desired, eventSource().orElse(null)); + context + .resourceOperations() + .serverSideApply( + desired, eventSource().orElse(null), new ResourceOperations.Options(true)); } else { var updatedActual = GenericResourceUpdater.updateResource(actual, desired, context); updatedResource = 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 6e7ace0447..18eb986efe 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 @@ -138,8 +138,7 @@ private PostExecutionControl

handleReconcile( } else { updatedResource = context.resourceOperations().addFinalizer(); } - return PostExecutionControl.onlyFinalizerAdded(updatedResource) - .withReSchedule(BaseControl.INSTANT_RESCHEDULE); + return PostExecutionControl.onlyFinalizerAdded(updatedResource); } else { try { return reconcileExecution(executionScope, resourceForExecution, originalResource, context); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 5a239a7377..9739a6d163 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -39,6 +39,7 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.config.Informable; import io.javaoperatorsdk.operator.api.config.NamespaceChangeable; +import io.javaoperatorsdk.operator.api.reconciler.Experimental; import io.javaoperatorsdk.operator.api.reconciler.dependent.RecentOperationCacheFiller; import io.javaoperatorsdk.operator.health.InformerHealthIndicator; import io.javaoperatorsdk.operator.health.InformerWrappingEventSourceHealthIndicator; @@ -88,13 +89,27 @@ public void changeNamespaces(Set namespaces) { } } + @Experimental( + "Internal API. Use ResourceOperation not this directly, this API might change in the future") + public R eventFilteringUpdateAndCacheResource(R resourceToUpdate, UnaryOperator updateMethod) { + return eventFilteringUpdateAndCacheResource(resourceToUpdate, updateMethod, false); + } + /** * Updates the resource and makes sure that the response is available for the next reconciliation. * Also makes sure that the even produced by this update is filtered, thus does not trigger the * reconciliation. */ + @Experimental( + "Internal API. Use ResourceOperation not this directly, this API might change in the future") @SuppressWarnings("unchecked") - public R eventFilteringUpdateAndCacheResource(R resourceToUpdate, UnaryOperator updateMethod) { + public R eventFilteringUpdateAndCacheResource( + R resourceToUpdate, UnaryOperator updateMethod, boolean forceUpdateFilter) { + if (resourceToUpdate.getMetadata().getResourceVersion() == null && !forceUpdateFilter) { + log.debug("No resourceVersion set. Skipping event filtering."); + return updateAndCacheResource(resourceToUpdate, updateMethod); + } + ResourceID id = ResourceID.fromResource(resourceToUpdate); log.debug("Starting event filtering and caching update for id={}", id); R updatedResource = null; @@ -134,6 +149,12 @@ public R eventFilteringUpdateAndCacheResource(R resourceToUpdate, UnaryOperator< } } + private R updateAndCacheResource(R resourceToUpdate, UnaryOperator updateOperation) { + var result = updateOperation.apply(resourceToUpdate); + handleRecentResourceUpdate(ResourceID.fromResource(resourceToUpdate), result, resourceToUpdate); + return result; + } + protected abstract void handleEvent( ResourceAction action, R resource, diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java index 8d0176cd4a..abc4326dd1 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java @@ -84,7 +84,7 @@ void addsFinalizer() { // Mock successful finalizer addition when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenAnswer( invocation -> { var res = TestUtils.testCustomResource1(); @@ -99,7 +99,7 @@ void addsFinalizer() { assertThat(result.hasFinalizer(FINALIZER_NAME)).isTrue(); assertThat(result.getMetadata().getResourceVersion()).isEqualTo("2"); verify(controllerEventSource, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); } @Test @@ -111,7 +111,7 @@ void addsFinalizerWithSSA() { // Mock successful SSA finalizer addition when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenAnswer( invocation -> { var res = TestUtils.testCustomResource1(); @@ -126,7 +126,7 @@ void addsFinalizerWithSSA() { assertThat(result.hasFinalizer(FINALIZER_NAME)).isTrue(); assertThat(result.getMetadata().getResourceVersion()).isEqualTo("2"); verify(controllerEventSource, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); } @Test @@ -139,7 +139,7 @@ void removesFinalizer() { // Mock successful finalizer removal when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenAnswer( invocation -> { var res = TestUtils.testCustomResource1(); @@ -154,7 +154,7 @@ void removesFinalizer() { assertThat(result.hasFinalizer(FINALIZER_NAME)).isFalse(); assertThat(result.getMetadata().getResourceVersion()).isEqualTo("2"); verify(controllerEventSource, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); } @Test @@ -166,7 +166,7 @@ void retriesAddingFinalizerWithoutSSA() { // First call throws conflict, second succeeds when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenThrow(new KubernetesClientException("Conflict", 409, null)) .thenAnswer( invocation -> { @@ -184,7 +184,7 @@ void retriesAddingFinalizerWithoutSSA() { assertThat(result).isNotNull(); assertThat(result.hasFinalizer(FINALIZER_NAME)).isTrue(); verify(controllerEventSource, times(2)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); verify(resourceOp, times(1)).get(); } @@ -198,7 +198,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { // First call throws conflict when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenThrow(new KubernetesClientException("Conflict", 409, null)); // Return null on retry (resource was deleted) @@ -207,7 +207,7 @@ void nullResourceIsGracefullyHandledOnFinalizerRemovalRetry() { resourceOperations.removeFinalizer(FINALIZER_NAME); verify(controllerEventSource, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); verify(resourceOp, times(1)).get(); } @@ -221,7 +221,7 @@ void retriesFinalizerRemovalWithFreshResource() { // First call throws unprocessable (422), second succeeds when(controllerEventSource.eventFilteringUpdateAndCacheResource( - any(), any(UnaryOperator.class))) + any(), any(UnaryOperator.class), anyBoolean())) .thenThrow(new KubernetesClientException("Unprocessable", 422, null)) .thenAnswer( invocation -> { @@ -243,7 +243,7 @@ void retriesFinalizerRemovalWithFreshResource() { assertThat(result.getMetadata().getResourceVersion()).isEqualTo("3"); assertThat(result.hasFinalizer(FINALIZER_NAME)).isFalse(); verify(controllerEventSource, times(2)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); verify(resourceOp, times(1)).get(); } @@ -261,7 +261,8 @@ void resourcePatchWithSingleEventSource() { when(context.eventSourceRetriever()).thenReturn(eventSourceRetriever); when(eventSourceRetriever.getEventSourcesFor(TestCustomResource.class)) .thenReturn(List.of(managedEventSource)); - when(managedEventSource.eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class))) + when(managedEventSource.eventFilteringUpdateAndCacheResource( + any(), any(UnaryOperator.class), anyBoolean())) .thenReturn(updatedResource); var result = resourceOperations.resourcePatch(resource, UnaryOperator.identity()); @@ -269,7 +270,7 @@ void resourcePatchWithSingleEventSource() { assertThat(result).isNotNull(); assertThat(result.getMetadata().getResourceVersion()).isEqualTo("2"); verify(managedEventSource, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); } @Test @@ -303,7 +304,7 @@ void resourcePatchUsesFirstEventSourceIfMultipleEventSourcesPresent() { resourceOperations.resourcePatch(resource, UnaryOperator.identity()); verify(eventSource1, times(1)) - .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class)); + .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class), anyBoolean()); } @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index c62a1d1a3a..31cbc26d11 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -780,6 +780,47 @@ void filteringUpdateFallsBackToMapperWhenNoPrimaryToSecondaryIndex() { verify(eventHandlerMock, times(1)).handleEvent(any()); } + @Test + void skipsEventFilteringWhenResourceVersionIsNull() { + // Without a resourceVersion there is nothing to correlate own-write echoes against, so + // event filtering is bypassed: the update is applied and cached directly, no filter window + // is opened and no event is propagated through handleEvent. + var resourceToUpdate = testDeployment(); + resourceToUpdate.getMetadata().setResourceVersion(null); + var updated = deploymentWithResourceVersion(3); + + var result = + informerEventSource.eventFilteringUpdateAndCacheResource(resourceToUpdate, r -> updated); + + assertThat(result).isSameAs(updated); + verify(temporaryResourceCache, times(1)).putResource(updated); + verify(temporaryResourceCache, never()).startEventFilteringModify(any()); + verify(temporaryResourceCache, never()).doneEventFilterModify(any()); + verify(eventHandlerMock, never()).handleEvent(any()); + } + + @Test + void forceUpdateFilterOpensFilterWindowEvenWhenResourceVersionIsNull() { + // A write without a resourceVersion (e.g. an SSA finalizer add, which uses no optimistic + // locking) would normally skip event filtering. forceUpdateFilter=true forces filtering + // anyway so the resulting own event is correlated and does not trigger a spurious + // reconciliation. + var resourceToUpdate = testDeployment(); + resourceToUpdate.getMetadata().setResourceVersion(null); + var updated = deploymentWithResourceVersion(3); + when(temporaryResourceCache.doneEventFilterModify(any())).thenReturn(Optional.empty()); + + var result = + informerEventSource.eventFilteringUpdateAndCacheResource( + resourceToUpdate, r -> updated, true); + + assertThat(result).isSameAs(updated); + verify(temporaryResourceCache, times(1)).startEventFilteringModify(any()); + verify(temporaryResourceCache, times(1)).doneEventFilterModify(any()); + verify(temporaryResourceCache, times(1)).putResource(updated); + verify(eventHandlerMock, never()).handleEvent(any()); + } + private PrimaryToSecondaryIndex injectIndexMock() throws Exception { @SuppressWarnings("unchecked") PrimaryToSecondaryIndex indexMock = mock(PrimaryToSecondaryIndex.class); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java index d05364fc44..16cec09eee 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java @@ -53,7 +53,9 @@ public UpdateControl reconcile( ChangeNamespaceTestCustomResource primary, Context context) { - context.resourceOperations().serverSideApply(configMap(primary)); + context + .resourceOperations() + .serverSideApply(configMap(primary), new ResourceOperations.Options(true)); if (primary.getStatus() == null) { primary.setStatus(new ChangeNamespaceTestCustomResourceStatus()); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/onrelistfilter/OnRelistFilterReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/onrelistfilter/OnRelistFilterReconciler.java index 13e8e72d74..d18ed0ef27 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/onrelistfilter/OnRelistFilterReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/onrelistfilter/OnRelistFilterReconciler.java @@ -34,6 +34,7 @@ import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.ResourceOperations; import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.EventSource; @@ -79,7 +80,10 @@ public UpdateControl reconcile( if (execution == 1) { var cm = prepareConfigMap(resource); switch (mode.get()) { - case NO_RELIST -> context.resourceOperations().serverSideApply(cm, configMapEventSource); + case NO_RELIST -> + context + .resourceOperations() + .serverSideApply(cm, configMapEventSource, new ResourceOperations.Options(true)); case RELIST_AROUND_UPDATE -> { configMapEventSource.simulateOnBeforeList(); var applied = context.resourceOperations().serverSideApply(cm, configMapEventSource); @@ -94,7 +98,9 @@ public UpdateControl reconcile( case RELIST_COMPLETES_BEFORE_UPDATE -> { configMapEventSource.simulateOnBeforeList(); configMapEventSource.simulateOnList(); - context.resourceOperations().serverSideApply(cm, configMapEventSource); + context + .resourceOperations() + .serverSideApply(cm, configMapEventSource, new ResourceOperations.Options(true)); } case RELIST_STARTS_DURING_UPDATE -> { // Drive the event-filtering update path manually so we can fire onBeforeList AFTER the @@ -114,7 +120,8 @@ public UpdateControl reconcile( .withFieldManager(fieldManager) .withPatchType(PatchType.SERVER_SIDE_APPLY) .build()); - }); + }, + true); // See RELIST_AROUND_UPDATE: wait for the own-write event to be buffered while the // re-list is still in progress, so it is tagged as part of the re-list and propagated. configMapEventSource.awaitWatchEventReceived(applied); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownsecondaryupdate/OwnSecondaryUpdateReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownsecondaryupdate/OwnSecondaryUpdateReconciler.java index f0ac28b955..b65d540ee2 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownsecondaryupdate/OwnSecondaryUpdateReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownsecondaryupdate/OwnSecondaryUpdateReconciler.java @@ -27,6 +27,7 @@ import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.ResourceOperations; import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.processing.event.source.EventSource; import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; @@ -50,7 +51,10 @@ public UpdateControl reconcile( // version actually advances. With the read-cache-after-write filter in place, none of the // resulting watch events should trigger a fresh reconciliation. for (int i = 1; i <= OWN_SSA_COUNT; i++) { - context.resourceOperations().serverSideApply(prepareCM(resource, i), configMapEventSource); + context + .resourceOperations() + .serverSideApply( + prepareCM(resource, i), configMapEventSource, new ResourceOperations.Options(true)); } return UpdateControl.noUpdate(); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchCustomResource.java new file mode 100644 index 0000000000..dff33b454d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchCustomResource.java @@ -0,0 +1,29 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch; + +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("scdsp") +public class SpecChangeDuringStatusPatchCustomResource + extends CustomResource + implements Namespaced {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchIT.java new file mode 100644 index 0000000000..93caa2fad1 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchIT.java @@ -0,0 +1,107 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch; + +import java.time.Duration; +import java.util.concurrent.TimeUnit; + +import org.junit.jupiter.api.RepeatedTest; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.client.dsl.base.PatchContext; +import io.fabric8.kubernetes.client.dsl.base.PatchType; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; + +import static io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch.SpecChangeDuringStatusPatchReconciler.STATUS_VALUE; +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +/** + * Reproduces a concurrent spec change happening while the controller patches its own status. When + * the reconciler patches the status it opens an event filtering window so it does not re-trigger + * itself. This test changes the spec on the cluster while that window is open and verifies that the + * spec change is still reconciled - it must not be silently absorbed together with the controller's + * own status update. + */ +class SpecChangeDuringStatusPatchIT { + + static final String RESOURCE_NAME = "test-resource"; + static final String SPEC_VALUE = "initial"; + public static final String UPDATED_SPEC_VALUE = "updated-val"; + + SpecChangeDuringStatusPatchReconciler reconciler = new SpecChangeDuringStatusPatchReconciler(); + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder().withReconciler(reconciler).build(); + + @RepeatedTest(10) + void specChangeDuringStatusPatchIsReconciled() throws InterruptedException { + var res = extension.create(testResource()); + var statusRes = testResource(); + statusRes.getMetadata().setNamespace(res.getMetadata().getNamespace()); + extension + .getKubernetesClient() + .resource(statusRes) + .status() + .patch( + new PatchContext.Builder() + .withForce(true) + .withFieldManager( + SpecChangeDuringStatusPatchReconciler.class.getSimpleName().toLowerCase()) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + + // wait until the reconciler is inside its own status patch, holding the filtering window open + assertThat(reconciler.statusPatchStartedLatch.await(30, TimeUnit.SECONDS)) + .as("reconciler should enter its own status patch operation") + .isTrue(); + + // change the spec on the cluster while the controller's status patch is still in flight + var current = extension.get(SpecChangeDuringStatusPatchCustomResource.class, RESOURCE_NAME); + current.getSpec().setValue(UPDATED_SPEC_VALUE); + extension.replace(current); + + // let the reconciler finish its own status patch + reconciler.specChangeDoneLatch.countDown(); + + // the spec change must be picked up by a fresh reconciliation and not lost with the own update + await() + .atMost(Duration.ofSeconds(5)) + .untilAsserted( + () -> { + assertThat(reconciler.numberOfExecutions.get()).isGreaterThanOrEqualTo(2); + assertThat(reconciler.lastObservedSpecValue.get()) + .as("a later reconciliation must observe the externally-applied spec change") + .isEqualTo(UPDATED_SPEC_VALUE); + }); + + // sanity check: the status the controller set is still present after the concurrent spec change + var updated = extension.get(SpecChangeDuringStatusPatchCustomResource.class, RESOURCE_NAME); + assertThat(updated.getSpec().getValue()).isEqualTo(UPDATED_SPEC_VALUE); + assertThat(updated.getStatus()).isNotNull(); + assertThat(updated.getStatus().getValue()).isEqualTo(STATUS_VALUE); + } + + SpecChangeDuringStatusPatchCustomResource testResource() { + var r = new SpecChangeDuringStatusPatchCustomResource(); + r.setMetadata(new ObjectMetaBuilder().withName(RESOURCE_NAME).build()); + r.setSpec(new SpecChangeDuringStatusPatchSpec().setValue(SPEC_VALUE)); + r.setStatus(new SpecChangeDuringStatusPatchStatus().setValue(STATUS_VALUE)); + return r; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchReconciler.java new file mode 100644 index 0000000000..33d809046b --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchReconciler.java @@ -0,0 +1,72 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; + +/** + * On the first reconciliation the reconciler patches its own status, but keeps the event filtering + * window (opened by {@link + * io.javaoperatorsdk.operator.api.reconciler.ResourceOperations#resourcePatch}) open until the test + * signals that it has changed the spec on the cluster. This reproduces the race where a spec change + * lands while the controller's own status patch is in flight: the spec change event must still + * propagate as a fresh reconciliation, it must not be absorbed as if it were our own status update. + */ +@ControllerConfiguration +public class SpecChangeDuringStatusPatchReconciler + implements Reconciler { + + static final String STATUS_VALUE = "reconciled"; + + final AtomicInteger numberOfExecutions = new AtomicInteger(); + final CountDownLatch statusPatchStartedLatch = new CountDownLatch(1); + final CountDownLatch specChangeDoneLatch = new CountDownLatch(1); + final AtomicReference lastObservedSpecValue = new AtomicReference<>(); + + @Override + public UpdateControl reconcile( + SpecChangeDuringStatusPatchCustomResource resource, + Context context) { + int execution = numberOfExecutions.incrementAndGet(); + lastObservedSpecValue.set(resource.getSpec().getValue()); + + if (execution == 1) { + resource.setStatus(new SpecChangeDuringStatusPatchStatus().setValue(STATUS_VALUE)); + resource.getMetadata().setResourceVersion(null); + // Patch our own status, but hold the filtering window open with a hook that lets the test + // change the spec on the cluster WHILE the status patch is still in flight. + statusPatchStartedLatch.countDown(); + try { + if (!specChangeDoneLatch.await(30, TimeUnit.SECONDS)) { + throw new IllegalStateException("timed out waiting for external spec change"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + return UpdateControl.patchStatus(resource); + } + return UpdateControl.noUpdate(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchSpec.java new file mode 100644 index 0000000000..64a6e7f0ef --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchSpec.java @@ -0,0 +1,30 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch; + +public class SpecChangeDuringStatusPatchSpec { + + private String value; + + public String getValue() { + return value; + } + + public SpecChangeDuringStatusPatchSpec setValue(String value) { + this.value = value; + return this; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchStatus.java new file mode 100644 index 0000000000..4cb857a4cc --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchStatus.java @@ -0,0 +1,30 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.baseapi.readcacheafterwrite.specchangeduringstatuspatch; + +public class SpecChangeDuringStatusPatchStatus { + + private String value; + + public String getValue() { + return value; + } + + public SpecChangeDuringStatusPatchStatus setValue(String value) { + this.value = value; + return this; + } +}