diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java index 4c8b857d47..ce6f67176c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java @@ -117,7 +117,7 @@ public DefaultContext

setRetryInfo(RetryInfo retryInfo) { return this; } - public P getPrimaryResource() { - return primaryResource; - } + public P getPrimaryResource() { + return primaryResource; + } } 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 8c502d41ff..baa7c36121 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 @@ -58,7 +58,8 @@ class ReconciliationDispatcher

{ public ReconciliationDispatcher(Controller

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

handleExecution(ExecutionScope

executionScope) { @@ -364,14 +365,16 @@ static class CustomResourceFacade { private final MixedOperation, Resource> resourceOperation; private final boolean useSSA; private final String fieldManager; + private final Cloner cloner; public CustomResourceFacade( - MixedOperation, Resource> resourceOperation, - ControllerConfiguration configuration) { + MixedOperation, Resource> resourceOperation, + ControllerConfiguration configuration, Cloner cloner) { this.resourceOperation = resourceOperation; this.useSSA = configuration.getConfigurationService().useSSAToPatchPrimaryResource(); this.fieldManager = configuration.fieldManager(); + this.cloner = cloner; } public R getResource(String namespace, String name) { @@ -402,30 +405,37 @@ public R patchResource(R resource, R originalResource) { public R patchStatus(R resource, R originalResource) { log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA); + if (useSSA) { + var managedFields = resource.getMetadata().getManagedFields(); + try { + resource.getMetadata().setManagedFields(null); + var res = resource(resource); + return res.subresource("status").patch(new PatchContext.Builder() + .withFieldManager(fieldManager) + .withForce(true) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + } finally { + resource.getMetadata().setManagedFields(managedFields); + } + } else { + return editStatus(resource, originalResource); + } + } + + private R editStatus(R resource, R originalResource) { String resourceVersion = resource.getMetadata().getResourceVersion(); - originalResource.getMetadata().setResourceVersion(null); - resource.getMetadata().setResourceVersion(null); + // the cached resource should not be changed in any circumstances + // that can lead to all kinds of race conditions. + R clonedOriginal = cloner.clone(originalResource); try { - if (useSSA) { - var managedFields = resource.getMetadata().getManagedFields(); - try { - resource.getMetadata().setManagedFields(null); - var res = resource(resource); - return res.subresource("status").patch(new PatchContext.Builder() - .withFieldManager(fieldManager) - .withForce(true) - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .build()); - } finally { - resource.getMetadata().setManagedFields(managedFields); - } - } else { - var res = resource(originalResource); - return res.editStatus(r -> resource); - } + clonedOriginal.getMetadata().setResourceVersion(null); + resource.getMetadata().setResourceVersion(null); + var res = resource(clonedOriginal); + return res.editStatus(r -> resource); } finally { // restore initial resource version - originalResource.getMetadata().setResourceVersion(resourceVersion); + clonedOriginal.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 e5fe1c5882..f8f0c59845 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 @@ -7,8 +7,6 @@ import java.util.function.BiFunction; import java.util.function.Supplier; -import io.fabric8.kubernetes.client.utils.KubernetesSerialization; -import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -19,6 +17,7 @@ import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.client.KubernetesClientException; +import io.fabric8.kubernetes.client.utils.KubernetesSerialization; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.TestUtils; @@ -29,6 +28,7 @@ import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Cleaner; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.DefaultContext; import io.javaoperatorsdk.operator.api.reconciler.DeleteControl; import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusUpdateControl; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -70,8 +70,9 @@ void setup() { } static void initConfigService(boolean useSSA) { - initConfigService(useSSA,true); + initConfigService(useSSA, true); } + static void initConfigService(boolean useSSA, boolean noCloning) { /* * We need this for mock reconcilers to properly generate the expected UpdateControl: without @@ -82,18 +83,19 @@ static void initConfigService(boolean useSSA, boolean noCloning) { */ configurationService = ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(), - overrider -> overrider.checkingCRDAndValidateLocalModel(false) + overrider -> overrider.checkingCRDAndValidateLocalModel(false) .withResourceCloner(new Cloner() { @Override public R clone(R object) { - if (noCloning) { - return object; - }else { - return new KubernetesSerialization().clone(object); + if (noCloning) { + return object; + } else { + return new KubernetesSerialization().clone(object); + } } - }}) - .withUseSSAToPatchPrimaryResource(useSSA)); + }) + .withUseSSAToPatchPrimaryResource(useSSA)); } private ReconciliationDispatcher init(R customResource, @@ -669,22 +671,24 @@ void reSchedulesFromErrorHandler() { @Test void reconcilerContextUsesTheSameInstanceOfResourceAsParam() { - initConfigService(false,false); + initConfigService(false, false); final ReconciliationDispatcher dispatcher = - init(testCustomResource, reconciler, null, customResourceFacade, true); + init(testCustomResource, reconciler, null, customResourceFacade, true); testCustomResource.addFinalizer(DEFAULT_FINALIZER); - ArgumentCaptor contextArgumentCaptor = ArgumentCaptor.forClass(DefaultContext.class); - ArgumentCaptor customResourceCaptor = ArgumentCaptor.forClass(TestCustomResource.class); + ArgumentCaptor contextArgumentCaptor = + ArgumentCaptor.forClass(DefaultContext.class); + ArgumentCaptor customResourceCaptor = + ArgumentCaptor.forClass(TestCustomResource.class); dispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - verify(reconciler, times(1)) - .reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture()); + verify(reconciler, times(1)) + .reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture()); - assertThat(contextArgumentCaptor.getValue().getPrimaryResource()) - .isSameAs(customResourceCaptor.getValue()) - .isNotSameAs(testCustomResource); + assertThat(contextArgumentCaptor.getValue().getPrimaryResource()) + .isSameAs(customResourceCaptor.getValue()) + .isNotSameAs(testCustomResource); } private ObservedGenCustomResource createObservedGenCustomResource() { diff --git a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java index 45cd85b468..14bc096ff2 100644 --- a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java +++ b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java @@ -8,13 +8,9 @@ import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; -import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Deque; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -22,7 +18,6 @@ import java.util.function.Function; import java.util.stream.Stream; -import io.fabric8.kubernetes.client.dsl.NamespaceListVisitFromServerGetDeleteRecreateWaitApplicable; import org.junit.jupiter.api.extension.ExtensionContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,7 +44,8 @@ public class LocallyRunOperatorExtension extends AbstractOperatorExtension { private static final Logger LOGGER = LoggerFactory.getLogger(LocallyRunOperatorExtension.class); private static final int CRD_DELETE_TIMEOUT = 1000; private static final Set appliedCRDs = new HashSet<>(); - private static final boolean deleteCRDs = Boolean.parseBoolean(System.getProperty("testsuite.deleteCRDs", "true")); + private static final boolean deleteCRDs = + Boolean.parseBoolean(System.getProperty("testsuite.deleteCRDs", "true")); private final Operator operator; private final List reconcilers; 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 bac97514b6..fc14b0951f 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 @@ -45,13 +45,20 @@ public UpdateControl reconcile( .create(); } - increaseNumberOfResourceExecutions(primary); if (primary.getStatus() == null) { primary.setStatus(new ChangeNamespaceTestCustomResourceStatus()); } + increaseNumberOfResourceExecutions(primary); + + var statusPatchResource = new ChangeNamespaceTestCustomResource(); + statusPatchResource.setMetadata(new ObjectMetaBuilder() + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()); + statusPatchResource.setStatus(new ChangeNamespaceTestCustomResourceStatus()); var statusUpdates = primary.getStatus().getNumberOfStatusUpdates(); - primary.getStatus().setNumberOfStatusUpdates(statusUpdates + 1); - return UpdateControl.patchStatus(primary); + statusPatchResource.getStatus().setNumberOfStatusUpdates(statusUpdates + 1); + return UpdateControl.patchStatus(statusPatchResource); } private void increaseNumberOfResourceExecutions(ChangeNamespaceTestCustomResource primary) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchLockingReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchLockingReconciler.java index 9cafaa26ec..46f0e63331 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchLockingReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchLockingReconciler.java @@ -21,6 +21,7 @@ public UpdateControl reconcile( throws InterruptedException { numberOfExecutions.addAndGet(1); Thread.sleep(WAIT_TIME); + if (resource.getStatus() == null) { resource.setStatus(new StatusPatchLockingCustomResourceStatus()); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingForNonSSAIT.java similarity index 95% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingIT.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingForNonSSAIT.java index 24ce2d1047..2363a86392 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingForNonSSAIT.java @@ -14,13 +14,14 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; -class StatusPatchNotLockingIT { +class StatusPatchNotLockingForNonSSAIT { public static final String TEST_RESOURCE_NAME = "test"; @RegisterExtension LocallyRunOperatorExtension operator = LocallyRunOperatorExtension.builder().withReconciler(StatusPatchLockingReconciler.class) + .withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false)) .build(); @Test