Skip to content

Commit cde1bf2

Browse files
csviricsviri
authored
fix: patch version handling (#2696)
1 parent 0b47463 commit cde1bf2

File tree

7 files changed

+74
-55
lines changed

7 files changed

+74
-55
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public DefaultContext<P> setRetryInfo(RetryInfo retryInfo) {
117117
return this;
118118
}
119119

120-
public P getPrimaryResource() {
121-
return primaryResource;
122-
}
120+
public P getPrimaryResource() {
121+
return primaryResource;
122+
}
123123
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

+33-23
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@ class ReconciliationDispatcher<P extends HasMetadata> {
5858

5959
public ReconciliationDispatcher(Controller<P> controller) {
6060
this(controller,
61-
new CustomResourceFacade<>(controller.getCRClient(), controller.getConfiguration()));
61+
new CustomResourceFacade<>(controller.getCRClient(), controller.getConfiguration(),
62+
controller.getConfiguration().getConfigurationService().getResourceCloner()));
6263
}
6364

6465
public PostExecutionControl<P> handleExecution(ExecutionScope<P> executionScope) {
@@ -364,14 +365,16 @@ static class CustomResourceFacade<R extends HasMetadata> {
364365
private final MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation;
365366
private final boolean useSSA;
366367
private final String fieldManager;
368+
private final Cloner cloner;
367369

368370
public CustomResourceFacade(
369-
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation,
370-
ControllerConfiguration<R> configuration) {
371+
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation,
372+
ControllerConfiguration<R> configuration, Cloner cloner) {
371373
this.resourceOperation = resourceOperation;
372374
this.useSSA =
373375
configuration.getConfigurationService().useSSAToPatchPrimaryResource();
374376
this.fieldManager = configuration.fieldManager();
377+
this.cloner = cloner;
375378
}
376379

377380
public R getResource(String namespace, String name) {
@@ -402,30 +405,37 @@ public R patchResource(R resource, R originalResource) {
402405

403406
public R patchStatus(R resource, R originalResource) {
404407
log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA);
408+
if (useSSA) {
409+
var managedFields = resource.getMetadata().getManagedFields();
410+
try {
411+
resource.getMetadata().setManagedFields(null);
412+
var res = resource(resource);
413+
return res.subresource("status").patch(new PatchContext.Builder()
414+
.withFieldManager(fieldManager)
415+
.withForce(true)
416+
.withPatchType(PatchType.SERVER_SIDE_APPLY)
417+
.build());
418+
} finally {
419+
resource.getMetadata().setManagedFields(managedFields);
420+
}
421+
} else {
422+
return editStatus(resource, originalResource);
423+
}
424+
}
425+
426+
private R editStatus(R resource, R originalResource) {
405427
String resourceVersion = resource.getMetadata().getResourceVersion();
406-
originalResource.getMetadata().setResourceVersion(null);
407-
resource.getMetadata().setResourceVersion(null);
428+
// the cached resource should not be changed in any circumstances
429+
// that can lead to all kinds of race conditions.
430+
R clonedOriginal = cloner.clone(originalResource);
408431
try {
409-
if (useSSA) {
410-
var managedFields = resource.getMetadata().getManagedFields();
411-
try {
412-
resource.getMetadata().setManagedFields(null);
413-
var res = resource(resource);
414-
return res.subresource("status").patch(new PatchContext.Builder()
415-
.withFieldManager(fieldManager)
416-
.withForce(true)
417-
.withPatchType(PatchType.SERVER_SIDE_APPLY)
418-
.build());
419-
} finally {
420-
resource.getMetadata().setManagedFields(managedFields);
421-
}
422-
} else {
423-
var res = resource(originalResource);
424-
return res.editStatus(r -> resource);
425-
}
432+
clonedOriginal.getMetadata().setResourceVersion(null);
433+
resource.getMetadata().setResourceVersion(null);
434+
var res = resource(clonedOriginal);
435+
return res.editStatus(r -> resource);
426436
} finally {
427437
// restore initial resource version
428-
originalResource.getMetadata().setResourceVersion(resourceVersion);
438+
clonedOriginal.getMetadata().setResourceVersion(resourceVersion);
429439
resource.getMetadata().setResourceVersion(resourceVersion);
430440
}
431441
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

+23-19
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
import java.util.function.BiFunction;
88
import java.util.function.Supplier;
99

10-
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
11-
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
1210
import org.junit.jupiter.api.BeforeEach;
1311
import org.junit.jupiter.api.Test;
1412
import org.mockito.ArgumentCaptor;
@@ -19,6 +17,7 @@
1917
import io.fabric8.kubernetes.api.model.ObjectMeta;
2018
import io.fabric8.kubernetes.client.CustomResource;
2119
import io.fabric8.kubernetes.client.KubernetesClientException;
20+
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
2221
import io.javaoperatorsdk.operator.MockKubernetesClient;
2322
import io.javaoperatorsdk.operator.OperatorException;
2423
import io.javaoperatorsdk.operator.TestUtils;
@@ -29,6 +28,7 @@
2928
import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration;
3029
import io.javaoperatorsdk.operator.api.reconciler.Cleaner;
3130
import io.javaoperatorsdk.operator.api.reconciler.Context;
31+
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
3232
import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
3333
import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusUpdateControl;
3434
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
@@ -70,8 +70,9 @@ void setup() {
7070
}
7171

7272
static void initConfigService(boolean useSSA) {
73-
initConfigService(useSSA,true);
73+
initConfigService(useSSA, true);
7474
}
75+
7576
static void initConfigService(boolean useSSA, boolean noCloning) {
7677
/*
7778
* We need this for mock reconcilers to properly generate the expected UpdateControl: without
@@ -82,18 +83,19 @@ static void initConfigService(boolean useSSA, boolean noCloning) {
8283
*/
8384
configurationService =
8485
ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(),
85-
overrider -> overrider.checkingCRDAndValidateLocalModel(false)
86+
overrider -> overrider.checkingCRDAndValidateLocalModel(false)
8687

8788
.withResourceCloner(new Cloner() {
8889
@Override
8990
public <R extends HasMetadata> R clone(R object) {
90-
if (noCloning) {
91-
return object;
92-
}else {
93-
return new KubernetesSerialization().clone(object);
91+
if (noCloning) {
92+
return object;
93+
} else {
94+
return new KubernetesSerialization().clone(object);
95+
}
9496
}
95-
}})
96-
.withUseSSAToPatchPrimaryResource(useSSA));
97+
})
98+
.withUseSSAToPatchPrimaryResource(useSSA));
9799
}
98100

99101
private <R extends HasMetadata> ReconciliationDispatcher<R> init(R customResource,
@@ -669,22 +671,24 @@ void reSchedulesFromErrorHandler() {
669671

670672
@Test
671673
void reconcilerContextUsesTheSameInstanceOfResourceAsParam() {
672-
initConfigService(false,false);
674+
initConfigService(false, false);
673675

674676
final ReconciliationDispatcher<TestCustomResource> dispatcher =
675-
init(testCustomResource, reconciler, null, customResourceFacade, true);
677+
init(testCustomResource, reconciler, null, customResourceFacade, true);
676678

677679
testCustomResource.addFinalizer(DEFAULT_FINALIZER);
678-
ArgumentCaptor<DefaultContext> contextArgumentCaptor = ArgumentCaptor.forClass(DefaultContext.class);
679-
ArgumentCaptor<TestCustomResource> customResourceCaptor = ArgumentCaptor.forClass(TestCustomResource.class);
680+
ArgumentCaptor<DefaultContext> contextArgumentCaptor =
681+
ArgumentCaptor.forClass(DefaultContext.class);
682+
ArgumentCaptor<TestCustomResource> customResourceCaptor =
683+
ArgumentCaptor.forClass(TestCustomResource.class);
680684

681685
dispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
682-
verify(reconciler, times(1))
683-
.reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture());
686+
verify(reconciler, times(1))
687+
.reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture());
684688

685-
assertThat(contextArgumentCaptor.getValue().getPrimaryResource())
686-
.isSameAs(customResourceCaptor.getValue())
687-
.isNotSameAs(testCustomResource);
689+
assertThat(contextArgumentCaptor.getValue().getPrimaryResource())
690+
.isSameAs(customResourceCaptor.getValue())
691+
.isNotSameAs(testCustomResource);
688692
}
689693

690694
private ObservedGenCustomResource createObservedGenCustomResource() {

‎operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java

+2-6
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,16 @@
88
import java.nio.file.Files;
99
import java.nio.file.Path;
1010
import java.time.Duration;
11-
import java.util.ArrayDeque;
1211
import java.util.ArrayList;
13-
import java.util.Deque;
1412
import java.util.HashMap;
1513
import java.util.HashSet;
16-
import java.util.Iterator;
17-
import java.util.LinkedList;
1814
import java.util.List;
1915
import java.util.Map;
2016
import java.util.Set;
2117
import java.util.function.Consumer;
2218
import java.util.function.Function;
2319
import java.util.stream.Stream;
2420

25-
import io.fabric8.kubernetes.client.dsl.NamespaceListVisitFromServerGetDeleteRecreateWaitApplicable;
2621
import org.junit.jupiter.api.extension.ExtensionContext;
2722
import org.slf4j.Logger;
2823
import org.slf4j.LoggerFactory;
@@ -49,7 +44,8 @@ public class LocallyRunOperatorExtension extends AbstractOperatorExtension {
4944
private static final Logger LOGGER = LoggerFactory.getLogger(LocallyRunOperatorExtension.class);
5045
private static final int CRD_DELETE_TIMEOUT = 1000;
5146
private static final Set<AppliedCRD> appliedCRDs = new HashSet<>();
52-
private static final boolean deleteCRDs = Boolean.parseBoolean(System.getProperty("testsuite.deleteCRDs", "true"));
47+
private static final boolean deleteCRDs =
48+
Boolean.parseBoolean(System.getProperty("testsuite.deleteCRDs", "true"));
5349

5450
private final Operator operator;
5551
private final List<ReconcilerSpec> reconcilers;

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java

+10-3
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,20 @@ public UpdateControl<ChangeNamespaceTestCustomResource> reconcile(
4545
.create();
4646
}
4747

48-
increaseNumberOfResourceExecutions(primary);
4948
if (primary.getStatus() == null) {
5049
primary.setStatus(new ChangeNamespaceTestCustomResourceStatus());
5150
}
51+
increaseNumberOfResourceExecutions(primary);
52+
53+
var statusPatchResource = new ChangeNamespaceTestCustomResource();
54+
statusPatchResource.setMetadata(new ObjectMetaBuilder()
55+
.withName(primary.getMetadata().getName())
56+
.withNamespace(primary.getMetadata().getNamespace())
57+
.build());
58+
statusPatchResource.setStatus(new ChangeNamespaceTestCustomResourceStatus());
5259
var statusUpdates = primary.getStatus().getNumberOfStatusUpdates();
53-
primary.getStatus().setNumberOfStatusUpdates(statusUpdates + 1);
54-
return UpdateControl.patchStatus(primary);
60+
statusPatchResource.getStatus().setNumberOfStatusUpdates(statusUpdates + 1);
61+
return UpdateControl.patchStatus(statusPatchResource);
5562
}
5663

5764
private void increaseNumberOfResourceExecutions(ChangeNamespaceTestCustomResource primary) {

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchLockingReconciler.java

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ public UpdateControl<StatusPatchLockingCustomResource> reconcile(
2121
throws InterruptedException {
2222
numberOfExecutions.addAndGet(1);
2323
Thread.sleep(WAIT_TIME);
24+
2425
if (resource.getStatus() == null) {
2526
resource.setStatus(new StatusPatchLockingCustomResourceStatus());
2627
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingIT.java ‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/statuspatchnonlocking/StatusPatchNotLockingForNonSSAIT.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,14 @@
1414
import static org.assertj.core.api.Assertions.assertThat;
1515
import static org.awaitility.Awaitility.await;
1616

17-
class StatusPatchNotLockingIT {
17+
class StatusPatchNotLockingForNonSSAIT {
1818

1919
public static final String TEST_RESOURCE_NAME = "test";
2020

2121
@RegisterExtension
2222
LocallyRunOperatorExtension operator =
2323
LocallyRunOperatorExtension.builder().withReconciler(StatusPatchLockingReconciler.class)
24+
.withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false))
2425
.build();
2526

2627
@Test

0 commit comments

Comments
 (0)