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.

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

Merged
merged 6 commits into from
May 2, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43f87ea
feat: feature flag to clone secondary resource on access
csviri Apr 24, 2024
ada9f95
naming and docs improvements
csviri Apr 24, 2024
7eb8f50
changing to not clone resources by default
csviri Apr 25, 2024
a34cb4d
docs: improve javadoc
metacosm Apr 30, 2024
376e74a
fix: no need to set the cloning flag since false is now the default
metacosm Apr 30, 2024
bb66085
fix: enforce returning value from original config if not overridden
metacosm Apr 30, 2024
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
11 changes: 11 additions & 0 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 @@ -404,4 +404,15 @@ default boolean useSSAToPatchPrimaryResource() {
return true;
}

/**
* By default, secondary resources are cloned when returned from the cache.
* ({@link io.javaoperatorsdk.operator.api.reconciler.Context#getSecondaryResource(Class)}) This
* might have a performance impact, and if reconciler relies on SSA it is not needed to do. Since
* the goal of the cloning is to prevent modification on the object in the cache, that would no
* longer reflect the state on the server in that case.
*/
default boolean cloneSecondaryResourcesWhenGettingFromCache() {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't that be false by default since we're using SSA by default now, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was, thinking about that, and there are pros/cons to that. It quite risky when somebody is migrating to the new version. On the other hand yes, that would be more optimal to have by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The new version will have significant changes… actually, I'm thinking that the change to SSA will require significant rewrite of operators if you need to only send what changes. Right now, most operators send the whole changed resource, not simply what has changed.
For that matter, that's actually kind of what we're pushing towards with the desired state: people compute the whole resource and that's what gets used. It might not be a big deal because these resources' fields are probably managed by the JOSDK reconciler already but I'm wondering how prevalent that actually is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, most operators send the whole changed resource, not simply what has changed.

It's not only what changed, but the whole desired state with SSA.

Yes this might need some changes when there is no SSA used already, I mean the changes are made on an existing object. But this should be the way to do it. Seconday resource is read for comparison (matching) and when it is needed as an input for other resources.

I don't think it is such a big deal to migrate, but I can imagine some cases that might not be trivial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing to consider, though, is that people might not upgrade if they perceive the upgrade requires too many changes without bringing much benefit (i.e. if someone's operator already works correctly, they probably won't upgrade, especially if they need to change things a lot to do so).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, however, that is why we add feature flags.

}

}
14 changes: 14 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 previousAnnotationForDependentResources;
private Boolean parseResourceVersions;
private Boolean useSSAToPatchPrimaryResource;
private Boolean cloneSecondaryResourcesWhenGettingFromCache;
private DependentResourceFactory dependentResourceFactory;

ConfigurationServiceOverrider(ConfigurationService original) {
Expand Down Expand Up @@ -180,6 +181,12 @@ 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
Expand Down Expand Up @@ -323,6 +330,13 @@ public boolean useSSAToPatchPrimaryResource() {
: super.useSSAToPatchPrimaryResource();

}

@Override
public boolean cloneSecondaryResourcesWhenGettingFromCache() {
return cloneSecondaryResourcesWhenGettingFromCache != null
? cloneSecondaryResourcesWhenGettingFromCache
: super.cloneSecondaryResourcesWhenGettingFromCache();
}
};
}

Expand Down
1 change: 0 additions & 1 deletion .../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 @@ -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
4 changes: 3 additions & 1 deletion ...in/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java
View file
Open in desktop
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
9 changes: 4 additions & 5 deletions ...operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions ...perators/webpage/src/test/java/io/javaoperatorsdk/operator/sample/WebPageOperatorE2E.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@ public WebPageOperatorE2E() throws FileNotFoundException {}
AbstractOperatorExtension operator =
isLocal()
? LocallyRunOperatorExtension.builder()
.withConfigurationService(
o -> o.withCloneSecondaryResourcesWhenGettingFromCache(false))
.waitForNamespaceDeletion(false)
.withReconciler(new WebPageReconciler(client))
.build()
: ClusterDeployedOperatorExtension.builder()
.withConfigurationService(
o -> o.withCloneSecondaryResourcesWhenGettingFromCache(false))
.waitForNamespaceDeletion(false)
.withOperatorDeployment(client.load(new FileInputStream("k8s/operator.yaml")).items(),
resources -> {
Expand Down