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.

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
* 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
* 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() {
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
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);
}
};
}
}
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
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
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
Loading