Skip to content

feat: feature flag to not clone secondary resource on access #2364

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

Merged
merged 6 commits into from
May 2, 2024
Merged
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 @@ -20,6 +20,7 @@
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
import io.javaoperatorsdk.operator.api.monitoring.Metrics;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
Expand Down Expand Up @@ -396,12 +397,36 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
* either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for
* adding finalizers, managing observed generation, patching resources and status.
*
* @return {@code true} by default
* @return {@code true} if Server-Side Apply (SSA) should be used when patching the primary
* resources, {@code false} otherwise
* @since 5.0.0
* @see ConfigurationServiceOverrider#withUseSSAToPatchPrimaryResource(boolean)
*/
default boolean useSSAToPatchPrimaryResource() {
return true;
}

/**
* <p>
* Determines whether resources retrieved from caches such as via calls to
* {@link Context#getSecondaryResource(Class)} should be defensively cloned first.
* </p>
*
* <p>
* Defensive cloning to prevent problematic cache modifications (modifying the resource would
* otherwise modify the stored copy in the cache) was transparently done in previous JOSDK
* versions. This might have performance consequences and, with the more prevalent use of
* Server-Side Apply, where you should create a new copy of your resource with only modified
* fields, such modifications of these resources are less likely to occur.
* </p>
*
* @return {@code true} if resources should be defensively cloned before returning them from
* caches, {@code false} otherwise
*
* @since 5.0.0
*/
default boolean cloneSecondaryResourcesWhenGettingFromCache() {
return false;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.function.Consumer;
import java.util.function.Function;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -40,6 +41,7 @@ public class ConfigurationServiceOverrider {
private Boolean previousAnnotationForDependentResources;
private Boolean parseResourceVersions;
private Boolean useSSAToPatchPrimaryResource;
private Boolean cloneSecondaryResourcesWhenGettingFromCache;
private DependentResourceFactory dependentResourceFactory;

ConfigurationServiceOverrider(ConfigurationService original) {
Expand Down Expand Up @@ -180,29 +182,42 @@ public ConfigurationServiceOverrider withUseSSAToPatchPrimaryResource(boolean va
return this;
}

public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromCache(
boolean value) {
this.cloneSecondaryResourcesWhenGettingFromCache = value;
return this;
}

public ConfigurationService build() {
return new BaseConfigurationService(original.getVersion(), cloner, client) {
@Override
public Set<String> getKnownReconcilerNames() {
return original.getKnownReconcilerNames();
}

private <T> T overriddenValueOrDefault(T value,
Function<ConfigurationService, T> defaultValue) {
return value != null ? value : defaultValue.apply(original);
}

@Override
public boolean checkCRDAndValidateLocalModel() {
return checkCR != null ? checkCR : original.checkCRDAndValidateLocalModel();
return overriddenValueOrDefault(checkCR,
ConfigurationService::checkCRDAndValidateLocalModel);
}

@Override
@SuppressWarnings("rawtypes")
public DependentResourceFactory dependentResourceFactory() {
return dependentResourceFactory != null ? dependentResourceFactory
: DependentResourceFactory.DEFAULT;
return overriddenValueOrDefault(dependentResourceFactory,
ConfigurationService::dependentResourceFactory);
}

@Override
public int concurrentReconciliationThreads() {
return Utils.ensureValid(
concurrentReconciliationThreads != null ? concurrentReconciliationThreads
: original.concurrentReconciliationThreads(),
overriddenValueOrDefault(concurrentReconciliationThreads,
ConfigurationService::concurrentReconciliationThreads),
"maximum reconciliation threads",
minimumMaxValueFor(minConcurrentReconciliationThreads),
original.concurrentReconciliationThreads());
Expand All @@ -211,8 +226,8 @@ public int concurrentReconciliationThreads() {
@Override
public int concurrentWorkflowExecutorThreads() {
return Utils.ensureValid(
concurrentWorkflowExecutorThreads != null ? concurrentWorkflowExecutorThreads
: original.concurrentWorkflowExecutorThreads(),
overriddenValueOrDefault(concurrentWorkflowExecutorThreads,
ConfigurationService::concurrentWorkflowExecutorThreads),
"maximum workflow execution threads",
minimumMaxValueFor(minConcurrentWorkflowExecutorThreads),
original.concurrentWorkflowExecutorThreads());
Expand All @@ -224,8 +239,8 @@ public int concurrentWorkflowExecutorThreads() {
@Deprecated(forRemoval = true)
@Override
public int minConcurrentReconciliationThreads() {
return minConcurrentReconciliationThreads != null ? minConcurrentReconciliationThreads
: original.minConcurrentReconciliationThreads();
return overriddenValueOrDefault(minConcurrentReconciliationThreads,
ConfigurationService::minConcurrentReconciliationThreads);
}

/**
Expand All @@ -234,30 +249,29 @@ public int minConcurrentReconciliationThreads() {
@Override
@Deprecated(forRemoval = true)
public int minConcurrentWorkflowExecutorThreads() {
return minConcurrentWorkflowExecutorThreads != null ? minConcurrentWorkflowExecutorThreads
: original.minConcurrentWorkflowExecutorThreads();
return overriddenValueOrDefault(minConcurrentWorkflowExecutorThreads,
ConfigurationService::minConcurrentWorkflowExecutorThreads);
}

@Override
public Metrics getMetrics() {
return metrics != null ? metrics : original.getMetrics();
return overriddenValueOrDefault(metrics, ConfigurationService::getMetrics);
}

@Override
public boolean closeClientOnStop() {
return closeClientOnStop != null ? closeClientOnStop : original.closeClientOnStop();
return overriddenValueOrDefault(closeClientOnStop, ConfigurationService::closeClientOnStop);
}

@Override
public ExecutorService getExecutorService() {
return executorService != null ? executorService
: super.getExecutorService();
return overriddenValueOrDefault(executorService, ConfigurationService::getExecutorService);
}

@Override
public ExecutorService getWorkflowExecutorService() {
return workflowExecutorService != null ? workflowExecutorService
: super.getWorkflowExecutorService();
return overriddenValueOrDefault(workflowExecutorService,
ConfigurationService::getWorkflowExecutorService);
}

@Override
Expand All @@ -274,54 +288,55 @@ public Optional<InformerStoppedHandler> getInformerStoppedHandler() {

@Override
public boolean stopOnInformerErrorDuringStartup() {
return stopOnInformerErrorDuringStartup != null ? stopOnInformerErrorDuringStartup
: super.stopOnInformerErrorDuringStartup();
return overriddenValueOrDefault(stopOnInformerErrorDuringStartup,
ConfigurationService::stopOnInformerErrorDuringStartup);
}

@Override
public Duration cacheSyncTimeout() {
return cacheSyncTimeout != null ? cacheSyncTimeout : super.cacheSyncTimeout();
return overriddenValueOrDefault(cacheSyncTimeout, ConfigurationService::cacheSyncTimeout);
}

@Override
public ResourceClassResolver getResourceClassResolver() {
return resourceClassResolver != null ? resourceClassResolver
: super.getResourceClassResolver();
return overriddenValueOrDefault(resourceClassResolver,
ConfigurationService::getResourceClassResolver);
}

@Override
public boolean ssaBasedCreateUpdateMatchForDependentResources() {
return ssaBasedCreateUpdateMatchForDependentResources != null
? ssaBasedCreateUpdateMatchForDependentResources
: super.ssaBasedCreateUpdateMatchForDependentResources();
return overriddenValueOrDefault(ssaBasedCreateUpdateMatchForDependentResources,
ConfigurationService::ssaBasedCreateUpdateMatchForDependentResources);
}

@Override
public Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
return defaultNonSSAResource != null ? defaultNonSSAResource
: super.defaultNonSSAResource();
return overriddenValueOrDefault(defaultNonSSAResource,
ConfigurationService::defaultNonSSAResource);
}

@Override
public boolean previousAnnotationForDependentResourcesEventFiltering() {
return previousAnnotationForDependentResources != null
? previousAnnotationForDependentResources
: super.previousAnnotationForDependentResourcesEventFiltering();
return overriddenValueOrDefault(previousAnnotationForDependentResources,
ConfigurationService::previousAnnotationForDependentResourcesEventFiltering);
}

@Override
public boolean parseResourceVersionsForEventFilteringAndCaching() {
return parseResourceVersions != null
? parseResourceVersions
: super.parseResourceVersionsForEventFilteringAndCaching();
return overriddenValueOrDefault(parseResourceVersions,
ConfigurationService::parseResourceVersionsForEventFilteringAndCaching);
}

@Override
public boolean useSSAToPatchPrimaryResource() {
return useSSAToPatchPrimaryResource != null
? useSSAToPatchPrimaryResource
: super.useSSAToPatchPrimaryResource();
return overriddenValueOrDefault(useSSAToPatchPrimaryResource,
ConfigurationService::useSSAToPatchPrimaryResource);
}

@Override
public boolean cloneSecondaryResourcesWhenGettingFromCache() {
return overriddenValueOrDefault(cloneSecondaryResourcesWhenGettingFromCache,
ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache);
}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,6 @@ private P patchResource(P resource, P originalResource) {
getVersion(resource));
log.trace("Resource before update: {}", resource);

// todo unit test
final var finalizerName = configuration().getFinalizerName();
if (useSSA && controller.useFinalizer()) {
// addFinalizer already prevents adding an already present finalizer so no need to check
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ public Stream<T> list(String namespace, Predicate<T> predicate) {
public Optional<T> get(ResourceID resourceID) {
return getSource(resourceID.getNamespace().orElse(WATCH_ALL_NAMESPACES))
.flatMap(source -> source.get(resourceID))
.map(r -> configurationService.getResourceCloner().clone(r));
.map(r -> configurationService.cloneSecondaryResourcesWhenGettingFromCache()
? configurationService.getResourceCloner().clone(r)
: r);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.networking.v1.Ingress;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.Replaceable;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.*;
Expand Down Expand Up @@ -90,7 +89,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
desiredHtmlConfigMap.getMetadata().getName(),
ns);
kubernetesClient.configMaps().inNamespace(ns).resource(desiredHtmlConfigMap)
.createOr(Replaceable::update);
.serverSideApply();
}

var existingDeployment = context.getSecondaryResource(Deployment.class).orElse(null);
Expand All @@ -100,7 +99,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
desiredDeployment.getMetadata().getName(),
ns);
kubernetesClient.apps().deployments().inNamespace(ns).resource(desiredDeployment)
.createOr(Replaceable::update);
.serverSideApply();
}

var existingService = context.getSecondaryResource(Service.class).orElse(null);
Expand All @@ -110,14 +109,14 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
desiredDeployment.getMetadata().getName(),
ns);
kubernetesClient.services().inNamespace(ns).resource(desiredService)
.createOr(Replaceable::update);
.serverSideApply();
}

var existingIngress = context.getSecondaryResource(Ingress.class);
if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) {
var desiredIngress = makeDesiredIngress(webPage);
if (existingIngress.isEmpty() || !match(desiredIngress, existingIngress.get())) {
kubernetesClient.resource(desiredIngress).inNamespace(ns).createOr(Replaceable::update);
kubernetesClient.resource(desiredIngress).inNamespace(ns).serverSideApply();
}
} else
existingIngress.ifPresent(
Expand Down
Loading