Skip to content

feat: cache update on update control #2789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

feat: cache update on update control #2789

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41eb66f
improve: remove fabric8 daily build
csviri May 9, 2025
80495e1
revert daily build removal
csviri May 9, 2025
5d662b8
fix unit test
csviri May 9, 2025
2d6a2a1
cache option
csviri May 9, 2025
935637c
flag
csviri May 9, 2025
08e1b34
improvements
csviri May 14, 2025
c1775a9
Integration test
csviri May 14, 2025
bc2cf5b
fixes
csviri May 14, 2025
0d49575
docs
csviri May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions ...ework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,14 +457,35 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() {
* logic, and you want to further minimize the amount of work done / updates issued by the
* operator.
*
* @return if resource version should be parsed (as integer)
* @since 4.5.0
* @return if resource version should be parsed (as integer)
* @return if resourceVersion should be parsed (as integer)
*/
default boolean parseResourceVersionsForEventFilteringAndCaching() {
return false;
}

/**
* If you update the primary resource from the reconciler with or without {@link
* io.javaoperatorsdk.operator.api.reconciler.UpdateControl} it is not guaranteed that for the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really make sense. What's the point of referring to UpdateControl if that can happen whether we use it or not?

* next reconciliation will receive the most up-to-date resource. This is a consequence of
* Informers design in the Operator pattern.
*
* <p>The framework can be smart about it and make such guarantees, but this can be done
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole paragraph is less explicit and clear that what it could be, imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a look thx.

* absolutely reliably only if it bends some rules of the Kubernetes API contract. Thus, if we
* parse the resourceVersion of a primary resource as an integer and compare it with the version
* of the resource in the cache. Note that this is a gray area, since with default setup Etcd
* guarantees such monotonically increasing resource versions. But since it is still bending the
* rules by default, this feature is turned off, and work only if {@link
* #parseResourceVersionsForEventFilteringAndCaching} is also set to true.
*
* <p>See also {@link io.javaoperatorsdk.operator.api.reconciler.PrimaryUpdateAndCacheUtils}
*
* @return if the framework should cache the updated resource for next reconciliation
*/
default boolean cacheUpdatedResourcesViaUpdateControl() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to have a different option for this? More precisely, wouldn't it make sense to activate automatic caching whenever parsing of the resource version is activated? I mean the name of the parse method already implies that it is used for caching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, parsing resource version was done for now only in dependent resources.

I think this should be an opt-in feature since it does not strictly follows K8S API contract, so user should be aware. I would not turn it on by default for the low level API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if there are some issues user can still turn it off.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that adding options for corner cases makes things a lot more complex to understand, especially when the setting only applies when some other setting is enabled. To me, all these options should really be hidden under a single one where everything that can be done by assuming the resource versions grow monotonically should be done automatically when parsing is activated.

Also, if there are potential issues, then we probably shouldn't have the features to start with. Either we can implement it correctly or we shouldn't provide the feature. That's something I actually dislike with the SSA support: it's not working in all cases and makes things more complex to the point that people might not want to use it or actually revert to using optimistic-looking based updates. This isn't great.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the method name isn't correct as the setting impacts more than updates via UpdateControl, at least from what I can tell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something I actually dislike with the SSA support: it's not working in all cases and makes things more complex to the point that people might not want to use it or actually revert to using optimistic-looking based updates. This isn't great.

I don't see what SSA has to do with optimistic locking. OL can be turned on all update / patch types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if there are potential issues, then we probably shouldn't have the features to start with. Either we can implement it correctly or we shouldn't provide the feature.

It's not about potential issues, I don't see any, but it is good to have a failsafe. Note that this what we have is again bending a bit the Kuberentes API contract. It is safe with ETCD at the backed, might be different for a non etc etc. For what we provide the alternative explicit cache.

return false;
}

/**
* {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can
* either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for
Expand Down
13 changes: 13 additions & 0 deletions ...e/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class ConfigurationServiceOverrider {
private Boolean parseResourceVersions;
private Boolean useSSAToPatchPrimaryResource;
private Boolean cloneSecondaryResourcesWhenGettingFromCache;
private Boolean cacheUpdatedResourcesViaUpdateControl;

@SuppressWarnings("rawtypes")
private DependentResourceFactory dependentResourceFactory;
Expand Down Expand Up @@ -188,6 +189,11 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC
return this;
}

public ConfigurationServiceOverrider withCacheUpdatedResourcesViaUpdateControl(boolean value) {
this.cacheUpdatedResourcesViaUpdateControl = value;
return this;
}

public ConfigurationService build() {
return new BaseConfigurationService(original.getVersion(), cloner, client) {
@Override
Expand Down Expand Up @@ -328,6 +334,13 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() {
cloneSecondaryResourcesWhenGettingFromCache,
ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache);
}

@Override
public boolean cacheUpdatedResourcesViaUpdateControl() {
return overriddenValueOrDefault(
cacheUpdatedResourcesViaUpdateControl,
ConfigurationService::cacheUpdatedResourcesViaUpdateControl);
}
};
}
}
81 changes: 58 additions & 23 deletions .../src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import io.javaoperatorsdk.operator.api.reconciler.RetryInfo;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerEventSource;

import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.*;

Expand Down Expand Up @@ -60,7 +61,8 @@ public ReconciliationDispatcher(Controller<P> controller) {
new CustomResourceFacade<>(
controller.getCRClient(),
controller.getConfiguration(),
controller.getConfiguration().getConfigurationService().getResourceCloner()));
controller.getConfiguration().getConfigurationService().getResourceCloner(),
controller.getEventSourceManager().getControllerEventSource()));
}

public PostExecutionControl<P> handleExecution(ExecutionScope<P> executionScope) {
Expand Down Expand Up @@ -175,7 +177,7 @@ private PostExecutionControl<P> reconcileExecution(
}

if (updateControl.isPatchStatus()) {
customResourceFacade.patchStatus(toUpdate, originalResource);
updatedCustomResource = customResourceFacade.patchStatus(toUpdate, originalResource);
}
return createPostExecutionControl(updatedCustomResource, updateControl);
}
Expand Down Expand Up @@ -315,7 +317,7 @@ private P addFinalizerWithSSA(P originalResource) {
objectMeta.setNamespace(originalResource.getMetadata().getNamespace());
resource.setMetadata(objectMeta);
resource.addFinalizer(configuration().getFinalizerName());
return customResourceFacade.patchResourceWithSSA(resource);
return customResourceFacade.patchResourceWithSSA(resource, originalResource);
} catch (InstantiationException
| IllegalAccessException
| InvocationTargetException
Expand Down Expand Up @@ -414,15 +416,32 @@ static class CustomResourceFacade<R extends HasMetadata> {
private final boolean useSSA;
private final String fieldManager;
private final Cloner cloner;
private final boolean cacheUpdatedResources;

private final ControllerEventSource<R> controllerEventSource;

public CustomResourceFacade(
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation,
ControllerConfiguration<R> configuration,
Cloner cloner) {
Cloner cloner,
ControllerEventSource<R> controllerEventSource) {
this.resourceOperation = resourceOperation;
this.useSSA = configuration.getConfigurationService().useSSAToPatchPrimaryResource();
this.fieldManager = configuration.fieldManager();
this.cacheUpdatedResources =
configuration
.getConfigurationService()
.previousAnnotationForDependentResourcesEventFiltering()
&& configuration.getConfigurationService().cacheUpdatedResourcesViaUpdateControl();
this.cloner = cloner;
this.controllerEventSource = controllerEventSource;
}

private void cachePrimaryResource(R updatedResource, R previousVersionOfResource) {
if (cacheUpdatedResources) {
controllerEventSource.handleRecentResourceUpdate(
ResourceID.fromResource(updatedResource), updatedResource, previousVersionOfResource);
}
}

public R getResource(String namespace, String name) {
Expand All @@ -434,7 +453,9 @@ public R getResource(String namespace, String name) {
}

public R patchResourceWithoutSSA(R resource, R originalResource) {
return resource(originalResource).edit(r -> resource);
R updated = resource(originalResource).edit(r -> resource);
cachePrimaryResource(updated, originalResource);
return updated;
}

public R patchResource(R resource, R originalResource) {
Expand All @@ -444,32 +465,43 @@ public R patchResource(R resource, R originalResource) {
ResourceID.fromResource(resource),
resource.getMetadata().getResourceVersion());
}
R updated;
if (useSSA) {
return patchResourceWithSSA(resource);
return patchResourceWithSSA(resource, originalResource);
} else {
return resource(originalResource).edit(r -> resource);
updated = resource(originalResource).edit(r -> resource);
cachePrimaryResource(updated, originalResource);
return updated;
}
}

public R patchStatus(R resource, R originalResource) {
log.trace("Patching status for resource: {} with ssa: {}", resource, useSSA);
if (useSSA) {
R updated = null;
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());
updated =
res.subresource("status")
.patch(
new PatchContext.Builder()
.withFieldManager(fieldManager)
.withForce(true)
.withPatchType(PatchType.SERVER_SIDE_APPLY)
.build());
return updated;
} finally {
resource.getMetadata().setManagedFields(managedFields);
if (updated != null) {
cachePrimaryResource(updated, originalResource);
}
}
} else {
return editStatus(resource, originalResource);
R updated = editStatus(resource, originalResource);
cachePrimaryResource(updated, originalResource);
return updated;
}
}

Expand All @@ -490,14 +522,17 @@ private R editStatus(R resource, R originalResource) {
}
}

public R patchResourceWithSSA(R resource) {
return resource(resource)
.patch(
new PatchContext.Builder()
.withFieldManager(fieldManager)
.withForce(true)
.withPatchType(PatchType.SERVER_SIDE_APPLY)
.build());
public R patchResourceWithSSA(R resource, R originalResource) {
R updated =
resource(resource)
.patch(
new PatchContext.Builder()
.withFieldManager(fieldManager)
.withForce(true)
.withPatchType(PatchType.SERVER_SIDE_APPLY)
.build());
cachePrimaryResource(updated, originalResource);
return updated;
}

private Resource<R> resource(R resource) {
Expand Down
7 changes: 4 additions & 3 deletions .../test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ void addFinalizerOnNewResource() {
verify(reconciler, never()).reconcile(ArgumentMatchers.eq(testCustomResource), any());
verify(customResourceFacade, times(1))
.patchResourceWithSSA(
argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)));
argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)),
any());
}

@Test
Expand Down Expand Up @@ -357,13 +358,13 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() {
void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() {
removeFinalizers(testCustomResource);
reconciler.reconcile = (r, c) -> UpdateControl.noUpdate();
when(customResourceFacade.patchResourceWithSSA(any())).thenReturn(testCustomResource);
when(customResourceFacade.patchResourceWithSSA(any(), any())).thenReturn(testCustomResource);

var postExecControl =
reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, times(1))
.patchResourceWithSSA(argThat(a -> !a.getMetadata().getFinalizers().isEmpty()));
.patchResourceWithSSA(argThat(a -> !a.getMetadata().getFinalizers().isEmpty()), any());
assertThat(postExecControl.updateIsStatusPatch()).isFalse();
assertThat(postExecControl.getUpdatedCustomResource()).isPresent();
}
Expand Down
6 changes: 6 additions & 0 deletions ...avaoperatorsdk/operator/processing/event/source/controller/ControllerEventSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ public TestController(
reconciler,
new TestConfiguration(true, onAddFilter, onUpdateFilter, genericFilter),
MockKubernetesClient.client(TestCustomResource.class));

when(eventSourceManager.getControllerEventSource())
.thenReturn(mock(ControllerEventSource.class));
}

public TestController(boolean generationAware) {
Expand All @@ -176,6 +179,9 @@ public TestController(boolean generationAware) {

@Override
public EventSourceManager<TestCustomResource> getEventSourceManager() {
if (eventSourceManager == null) {
return super.getEventSourceManager();
}
return eventSourceManager;
}

Expand Down