Skip to content

Commit 8e6a18e

Browse files
csviricsviri
committed
feat: use ssa for status update by default from UpdateControl (#2278)
Signed-off-by: Attila Mészáros <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
1 parent c0d687b commit 8e6a18e

File tree

8 files changed

+240
-50
lines changed

8 files changed

+240
-50
lines changed

‎docs/documentation/v5-0-migration.md

+8-1
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,12 @@ permalink: /docs/v5-0-migration
1717
[`EventSourceUtils`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceUtils.java#L11-L11)
1818
now contains all the utility methods used for event sources naming that were previously defined in
1919
the `EventSourceInitializer` interface.
20-
3. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
20+
3. Patching status through `UpdateControl` like the `patchStatus` method now by default
21+
uses Server Side Apply instead of simple patch. To use the former approach, use the feature flag
22+
in [`ConfigurationService`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L400-L400)
23+
!!! IMPORTANT !!!
24+
Migration from a non-SSA based controller to SSA based controller can cause problems, due to known issues.
25+
See the following [integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L71-L82) where it is demonstrated.
26+
Also, the related part of a [workaround](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L110-L116).
27+
4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
2128
via the accordingly renamed `managedWorkflowAndDependentResourceContext` method.

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+10
Original file line numberDiff line numberDiff line change
@@ -401,4 +401,14 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
401401
return false;
402402
}
403403

404+
/**
405+
* {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus can either use
406+
* simple update or SSA for status subresource patching.
407+
*
408+
* @return true by default
409+
*/
410+
default boolean useSSAForResourceStatusPatch() {
411+
return true;
412+
}
413+
404414
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

+15-1
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public class ConfigurationServiceOverrider {
4040
private Set<Class<? extends HasMetadata>> defaultNonSSAResource;
4141
private Boolean previousAnnotationForDependentResources;
4242
private Boolean parseResourceVersions;
43+
private Boolean useSSAForResourceStatusPatch;
4344
private DependentResourceFactory dependentResourceFactory;
4445

4546
ConfigurationServiceOverrider(ConfigurationService original) {
@@ -174,12 +175,17 @@ public ConfigurationServiceOverrider withPreviousAnnotationForDependentResources
174175
return this;
175176
}
176177

177-
public ConfigurationServiceOverrider wihtParseResourceVersions(
178+
public ConfigurationServiceOverrider withParseResourceVersions(
178179
boolean value) {
179180
this.parseResourceVersions = value;
180181
return this;
181182
}
182183

184+
public ConfigurationServiceOverrider withUseSSAForResourceStatusPatch(boolean value) {
185+
this.useSSAForResourceStatusPatch = value;
186+
return this;
187+
}
188+
183189
public ConfigurationService build() {
184190
return new BaseConfigurationService(original.getVersion(), cloner, client) {
185191
@Override
@@ -320,6 +326,14 @@ public boolean parseResourceVersionsForEventFilteringAndCaching() {
320326
? parseResourceVersions
321327
: super.parseResourceVersionsForEventFilteringAndCaching();
322328
}
329+
330+
@Override
331+
public boolean useSSAForResourceStatusPatch() {
332+
return useSSAForResourceStatusPatch != null
333+
? useSSAForResourceStatusPatch
334+
: super.useSSAForResourceStatusPatch();
335+
336+
}
323337
};
324338
}
325339

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

+46-32
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
import io.fabric8.kubernetes.client.KubernetesClientException;
1313
import io.fabric8.kubernetes.client.dsl.MixedOperation;
1414
import io.fabric8.kubernetes.client.dsl.Resource;
15+
import io.fabric8.kubernetes.client.dsl.base.PatchContext;
16+
import io.fabric8.kubernetes.client.dsl.base.PatchType;
1517
import io.javaoperatorsdk.operator.OperatorException;
1618
import io.javaoperatorsdk.operator.api.ObservedGenerationAware;
1719
import io.javaoperatorsdk.operator.api.config.Cloner;
@@ -25,9 +27,7 @@
2527
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
2628
import io.javaoperatorsdk.operator.processing.Controller;
2729

28-
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName;
29-
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID;
30-
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion;
30+
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.*;
3131

3232
/**
3333
* Handles calls and results of a Reconciler and finalizer related logic
@@ -45,8 +45,7 @@ class ReconciliationDispatcher<P extends HasMetadata> {
4545
private final boolean retryConfigurationHasZeroAttempts;
4646
private final Cloner cloner;
4747

48-
ReconciliationDispatcher(Controller<P> controller,
49-
CustomResourceFacade<P> customResourceFacade) {
48+
ReconciliationDispatcher(Controller<P> controller, CustomResourceFacade<P> customResourceFacade) {
5049
this.controller = controller;
5150
this.customResourceFacade = customResourceFacade;
5251
this.cloner = controller.getConfiguration().getConfigurationService().getResourceCloner();
@@ -56,7 +55,8 @@ class ReconciliationDispatcher<P extends HasMetadata> {
5655
}
5756

5857
public ReconciliationDispatcher(Controller<P> controller) {
59-
this(controller, new CustomResourceFacade<>(controller.getCRClient()));
58+
this(controller,
59+
new CustomResourceFacade<>(controller.getCRClient(), controller.getConfiguration()));
6060
}
6161

6262
public PostExecutionControl<P> handleExecution(ExecutionScope<P> executionScope) {
@@ -135,33 +135,25 @@ private PostExecutionControl<P> reconcileExecution(ExecutionScope<P> executionSc
135135

136136
UpdateControl<P> updateControl = controller.reconcile(resourceForExecution, context);
137137
P updatedCustomResource = null;
138+
final var toUpdate =
139+
updateControl.isNoUpdate() ? originalResource : updateControl.getResource();
138140
if (updateControl.isUpdateResourceAndStatus()) {
139-
updatedCustomResource =
140-
updateCustomResource(updateControl.getResource());
141-
updateControl
142-
.getResource()
141+
updatedCustomResource = updateCustomResource(toUpdate);
142+
toUpdate
143143
.getMetadata()
144144
.setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion());
145-
updatedCustomResource =
146-
updateStatusGenerationAware(updateControl.getResource(), originalResource,
147-
updateControl.isPatchStatus());
148-
} else if (updateControl.isUpdateStatus()) {
149-
updatedCustomResource =
150-
updateStatusGenerationAware(updateControl.getResource(), originalResource,
151-
updateControl.isPatchStatus());
152145
} else if (updateControl.isUpdateResource()) {
146+
updatedCustomResource = updateCustomResource(toUpdate);
147+
}
148+
149+
// check if status also needs to be updated
150+
final var updateObservedGeneration = updateControl.isNoUpdate()
151+
? shouldUpdateObservedGenerationAutomatically(resourceForExecution)
152+
: shouldUpdateObservedGenerationAutomatically(updatedCustomResource);
153+
if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus()
154+
|| updateObservedGeneration) {
153155
updatedCustomResource =
154-
updateCustomResource(updateControl.getResource());
155-
if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
156-
updatedCustomResource =
157-
updateStatusGenerationAware(updateControl.getResource(), originalResource,
158-
updateControl.isPatchStatus());
159-
}
160-
} else if (updateControl.isNoUpdate()
161-
&& shouldUpdateObservedGenerationAutomatically(resourceForExecution)) {
162-
updatedCustomResource =
163-
updateStatusGenerationAware(originalResource, originalResource,
164-
updateControl.isPatchStatus());
156+
updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus());
165157
}
166158
return createPostExecutionControl(updatedCustomResource, updateControl);
167159
}
@@ -376,10 +368,16 @@ public P conflictRetryingUpdate(P resource, Function<P, Boolean> modificationFun
376368
static class CustomResourceFacade<R extends HasMetadata> {
377369

378370
private final MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation;
371+
private final boolean useSSAToUpdateStatus;
372+
private final String fieldManager;
379373

380374
public CustomResourceFacade(
381-
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation) {
375+
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation,
376+
ControllerConfiguration<R> configuration) {
382377
this.resourceOperation = resourceOperation;
378+
this.useSSAToUpdateStatus =
379+
configuration.getConfigurationService().useSSAForResourceStatusPatch();
380+
this.fieldManager = configuration.fieldManager();
383381
}
384382

385383
public R getResource(String namespace, String name) {
@@ -409,14 +407,30 @@ public R updateStatus(R resource) {
409407
}
410408

411409
public R patchStatus(R resource, R originalResource) {
412-
log.trace("Updating status for resource: {}", resource);
410+
log.trace("Patching status for resource: {} with ssa: {}", resource, useSSAToUpdateStatus);
413411
String resourceVersion = resource.getMetadata().getResourceVersion();
414412
// don't do optimistic locking on patch
415413
originalResource.getMetadata().setResourceVersion(null);
416414
resource.getMetadata().setResourceVersion(null);
417415
try {
418-
return resource(originalResource)
419-
.editStatus(r -> resource);
416+
if (useSSAToUpdateStatus) {
417+
var managedFields = resource.getMetadata().getManagedFields();
418+
try {
419+
420+
resource.getMetadata().setManagedFields(null);
421+
var res = resource(resource);
422+
return res.subresource("status").patch(new PatchContext.Builder()
423+
.withFieldManager(fieldManager)
424+
.withForce(true)
425+
.withPatchType(PatchType.SERVER_SIDE_APPLY)
426+
.build());
427+
} finally {
428+
resource.getMetadata().setManagedFields(managedFields);
429+
}
430+
} else {
431+
var res = resource(originalResource);
432+
return res.editStatus(r -> resource);
433+
}
420434
} finally {
421435
// restore initial resource version
422436
originalResource.getMetadata().setResourceVersion(resourceVersion);

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

+2-12
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,9 @@
4343
import static io.javaoperatorsdk.operator.TestUtils.markForDeletion;
4444
import static io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.MAX_UPDATE_RETRY;
4545
import static org.assertj.core.api.Assertions.assertThat;
46-
import static org.junit.jupiter.api.Assertions.assertEquals;
47-
import static org.junit.jupiter.api.Assertions.assertFalse;
48-
import static org.junit.jupiter.api.Assertions.assertTrue;
49-
import static org.junit.jupiter.api.Assertions.fail;
46+
import static org.junit.jupiter.api.Assertions.*;
5047
import static org.mockito.ArgumentMatchers.argThat;
51-
import static org.mockito.Mockito.any;
52-
import static org.mockito.Mockito.eq;
53-
import static org.mockito.Mockito.mock;
54-
import static org.mockito.Mockito.never;
55-
import static org.mockito.Mockito.spy;
56-
import static org.mockito.Mockito.times;
57-
import static org.mockito.Mockito.verify;
58-
import static org.mockito.Mockito.when;
48+
import static org.mockito.Mockito.*;
5949

6050
@SuppressWarnings({"unchecked", "rawtypes"})
6151
class ReconciliationDispatcherTest {

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

+3-1
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,12 @@ void valuesAreDeletedIfSetToNull() {
5858
assertThat(actual.getStatus().getMessage()).isEqualTo(MESSAGE);
5959
});
6060

61+
// resource needs to be read again to we don't replace the with wrong managed fields
62+
resource = operator.get(StatusPatchLockingCustomResource.class, TEST_RESOURCE_NAME);
6163
resource.getSpec().setMessageInStatus(false);
6264
operator.replace(resource);
6365

64-
await().untilAsserted(() -> {
66+
await().timeout(Duration.ofMinutes(3)).untilAsserted(() -> {
6567
var actual = operator.get(StatusPatchLockingCustomResource.class,
6668
TEST_RESOURCE_NAME);
6769
assertThat(actual.getStatus()).isNotNull();

0 commit comments

Comments
 (0)