From 5e80d9843a94c605bb72bb495fc7a1d89e648f8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 14:11:42 +0200 Subject: [PATCH 1/9] feat: blacklist of problematic resources for previous version annotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 8 ++++++++ .../config/ConfigurationServiceOverrider.java | 15 +++++++++++++++ .../KubernetesDependentResource.java | 18 ++++++++++++++---- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 3ffc913c5e..8b0ee85f51 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -1,6 +1,7 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -13,6 +14,9 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Secret; +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.fabric8.kubernetes.api.model.apps.ReplicaSet; +import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.ConfigBuilder; import io.fabric8.kubernetes.client.CustomResource; @@ -448,6 +452,10 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { return true; } + default List> previousAnnotationUsageBlacklist() { + return List.of(Deployment.class, StatefulSet.class, ReplicaSet.class); + } + /** * If the event logic should parse the resourceVersion to determine the ordering of dependent * resource events. This is typically not needed. diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index f420be0fff..fa3c68c2e6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -1,6 +1,7 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -40,6 +41,7 @@ public class ConfigurationServiceOverrider { private Boolean parseResourceVersions; private Boolean useSSAToPatchPrimaryResource; private Boolean cloneSecondaryResourcesWhenGettingFromCache; + private List> previousAnnotationUsageBlacklist; @SuppressWarnings("rawtypes") private DependentResourceFactory dependentResourceFactory; @@ -188,6 +190,12 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC return this; } + public ConfigurationServiceOverrider previousAnnotationUsageBlacklist( + List> previousAnnotationUsageBlacklist) { + this.previousAnnotationUsageBlacklist = previousAnnotationUsageBlacklist; + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, client) { @Override @@ -328,6 +336,13 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { cloneSecondaryResourcesWhenGettingFromCache, ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache); } + + @Override + public List> previousAnnotationUsageBlacklist() { + return overriddenValueOrDefault( + previousAnnotationUsageBlacklist, + ConfigurationService::previousAnnotationUsageBlacklist); + } }; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index ea7edbc1a0..48963eb9ee 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -41,6 +41,7 @@ public abstract class KubernetesDependentResource kubernetesDependentResourceConfig; private volatile Boolean useSSA; + private volatile Boolean usePreviousAnnotationForEventFiltering; public KubernetesDependentResource(Class resourceType) { this(resourceType, null); @@ -163,10 +164,19 @@ protected boolean useSSA(Context

context) { } private boolean usePreviousAnnotation(Context

context) { - return context - .getControllerConfiguration() - .getConfigurationService() - .previousAnnotationForDependentResourcesEventFiltering(); + if (usePreviousAnnotationForEventFiltering == null) { + usePreviousAnnotationForEventFiltering = + context + .getControllerConfiguration() + .getConfigurationService() + .previousAnnotationForDependentResourcesEventFiltering() + && !context + .getControllerConfiguration() + .getConfigurationService() + .previousAnnotationUsageBlacklist() + .contains(this.resourceType()); + } + return usePreviousAnnotationForEventFiltering; } @Override From f7ba0670c6f8109af25e0885ec03503acee89e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 14:47:43 +0200 Subject: [PATCH 2/9] rename to blocklist, added javadocs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../api/config/ConfigurationService.java | 17 ++++++++++++++++- .../config/ConfigurationServiceOverrider.java | 12 ++++++------ .../kubernetes/KubernetesDependentResource.java | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 8b0ee85f51..4d3be7c7d9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -452,7 +452,22 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { return true; } - default List> previousAnnotationUsageBlacklist() { + /** + * For dependent resources framework can add an annotation to filter our events that are results + * of changes made by the framework. There are, however, few resources that do not follow the K8S + * API convention that changes in metadata do not increase the "metadata.generation". For these + * resources, the generation is increased by adding the annotation and their controller increases + * the observedGeneration in the status. This results in a new event, that if not handled + * correctly with the resource matcher yet again results in an update and a previous version + * annotation change, thus results in an infinite loop. + * + *

As a workaround, we automatically skip adding previous annotation for those well-known + * resources. Note that if you are sure that the matcher works (most of the cases does) for your + * case, you can remove the resource from the blocklist. + * + * @return blocklist of resource classes where the previous version annotation won't be used. + */ + default List> previousAnnotationUsageBlocklist() { return List.of(Deployment.class, StatefulSet.class, ReplicaSet.class); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index fa3c68c2e6..4430f616d9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -41,7 +41,7 @@ public class ConfigurationServiceOverrider { private Boolean parseResourceVersions; private Boolean useSSAToPatchPrimaryResource; private Boolean cloneSecondaryResourcesWhenGettingFromCache; - private List> previousAnnotationUsageBlacklist; + private List> previousAnnotationUsageBlocklist; @SuppressWarnings("rawtypes") private DependentResourceFactory dependentResourceFactory; @@ -190,9 +190,9 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC return this; } - public ConfigurationServiceOverrider previousAnnotationUsageBlacklist( + public ConfigurationServiceOverrider previousAnnotationUsageBlocklist( List> previousAnnotationUsageBlacklist) { - this.previousAnnotationUsageBlacklist = previousAnnotationUsageBlacklist; + this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist; return this; } @@ -338,10 +338,10 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { } @Override - public List> previousAnnotationUsageBlacklist() { + public List> previousAnnotationUsageBlocklist() { return overriddenValueOrDefault( - previousAnnotationUsageBlacklist, - ConfigurationService::previousAnnotationUsageBlacklist); + previousAnnotationUsageBlocklist, + ConfigurationService::previousAnnotationUsageBlocklist); } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index 48963eb9ee..db33990d45 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -173,7 +173,7 @@ private boolean usePreviousAnnotation(Context

context) { && !context .getControllerConfiguration() .getConfigurationService() - .previousAnnotationUsageBlacklist() + .previousAnnotationUsageBlocklist() .contains(this.resourceType()); } return usePreviousAnnotationForEventFiltering; From 3b147fee5c99832f5aba258c2830636e82f71172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 15:24:25 +0200 Subject: [PATCH 3/9] wip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/api/config/ConfigurationService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 4d3be7c7d9..8391bf9a7e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -14,8 +14,8 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Secret; +import io.fabric8.kubernetes.api.model.apps.DaemonSet; import io.fabric8.kubernetes.api.model.apps.Deployment; -import io.fabric8.kubernetes.api.model.apps.ReplicaSet; import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.ConfigBuilder; @@ -468,7 +468,7 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { * @return blocklist of resource classes where the previous version annotation won't be used. */ default List> previousAnnotationUsageBlocklist() { - return List.of(Deployment.class, StatefulSet.class, ReplicaSet.class); + return List.of(Deployment.class, StatefulSet.class, DaemonSet.class); } /** From 0aefb56a262dbe9c0523725f8a6cb4f6e6a3c482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 15:30:15 +0200 Subject: [PATCH 4/9] remove deamonSet since was not able to reproduce the behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/api/config/ConfigurationService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 8391bf9a7e..d2298d1fdf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -14,7 +14,6 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Secret; -import io.fabric8.kubernetes.api.model.apps.DaemonSet; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.client.Config; @@ -468,7 +467,7 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { * @return blocklist of resource classes where the previous version annotation won't be used. */ default List> previousAnnotationUsageBlocklist() { - return List.of(Deployment.class, StatefulSet.class, DaemonSet.class); + return List.of(Deployment.class, StatefulSet.class); } /** From 74c13ffc4765c4ffbfbc2ff6fac683ec16dc55dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 29 Apr 2025 14:59:11 +0200 Subject: [PATCH 5/9] Add integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- ...BasedGenericKubernetesResourceMatcher.java | 2 +- .../prevblocklist/DeploymentDependent.java | 95 +++++++++++++++++++ .../PrevAnnotationBlockCustomResource.java | 13 +++ .../PrevAnnotationBlockReconciler.java | 34 +++++++ .../PrevAnnotationBlockReconcilerIT.java | 50 ++++++++++ .../PrevAnnotationBlockSpec.java | 15 +++ 6 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 3c051acfb4..3fc5dbbee6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -179,7 +179,7 @@ private Optional checkIfFieldManagerExists(R actual, String } /** Correct for known issue with SSA */ - private void sanitizeState(R actual, R desired, Map actualMap) { + protected void sanitizeState(R actual, R desired, Map actualMap) { if (actual instanceof StatefulSet actualStatefulSet && desired instanceof StatefulSet desiredStatefulSet) { var actualSpec = actualStatefulSet.getSpec(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java new file mode 100644 index 0000000000..5cfb66f67e --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java @@ -0,0 +1,95 @@ +package io.javaoperatorsdk.operator.dependent.prevblocklist; + +import java.util.Map; + +import io.fabric8.kubernetes.api.model.ContainerBuilder; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.LabelSelectorBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.PodSpecBuilder; +import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; +import io.fabric8.kubernetes.api.model.Quantity; +import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder; +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; +import io.fabric8.kubernetes.api.model.apps.DeploymentSpecBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.SSABasedGenericKubernetesResourceMatcher; + +@KubernetesDependent +public class DeploymentDependent + extends CRUDKubernetesDependentResource { + + public static final String RESOURCE_NAME = "test1"; + + public DeploymentDependent() { + super(Deployment.class); + } + + @Override + protected Deployment desired( + PrevAnnotationBlockCustomResource primary, + Context context) { + + return new DeploymentBuilder() + .withMetadata( + new ObjectMetaBuilder() + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()) + .withSpec( + new DeploymentSpecBuilder() + .withReplicas(1) + .withSelector( + new LabelSelectorBuilder().withMatchLabels(Map.of("app", "nginx")).build()) + .withTemplate( + new PodTemplateSpecBuilder() + .withMetadata( + new ObjectMetaBuilder().withLabels(Map.of("app", "nginx")).build()) + .withSpec( + new PodSpecBuilder() + .withContainers( + new ContainerBuilder() + .withName("nginx") + .withImage("nginx:1.14.2") + .withResources( + new ResourceRequirementsBuilder() + .withLimits(Map.of("cpu", new Quantity("2000m"))) + .build()) + .build()) + .build()) + .build()) + .build()) + .build(); + } + + // for testing purposes replicating the matching logic but with the special matcher + @Override + public Result match( + Deployment actualResource, + Deployment desired, + PrevAnnotationBlockCustomResource primary, + Context context) { + final boolean matches; + addMetadata(true, actualResource, desired, primary, context); + if (useSSA(context)) { + matches = new SSAMatcherWithoutSanitization().matches(actualResource, desired, context); + } else { + matches = + GenericKubernetesResourceMatcher.match(desired, actualResource, false, false, context) + .matched(); + } + return Result.computed(matches, desired); + } + + // using this matcher, so we are able to reproduce issue with resource limits + static class SSAMatcherWithoutSanitization + extends SSABasedGenericKubernetesResourceMatcher { + + @Override + protected void sanitizeState(R actual, R desired, Map actualMap) {} + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java new file mode 100644 index 0000000000..7aa3194672 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java @@ -0,0 +1,13 @@ +package io.javaoperatorsdk.operator.dependent.prevblocklist; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@ShortNames("pabc") +public class PrevAnnotationBlockCustomResource extends CustomResource + implements Namespaced {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java new file mode 100644 index 0000000000..7f3dab61fe --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java @@ -0,0 +1,34 @@ +package io.javaoperatorsdk.operator.dependent.prevblocklist; + +import java.util.concurrent.atomic.AtomicInteger; + +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.Workflow; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; +import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; + +@Workflow(dependents = {@Dependent(type = DeploymentDependent.class)}) +@ControllerConfiguration() +public class PrevAnnotationBlockReconciler + implements Reconciler, TestExecutionInfoProvider { + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + + public PrevAnnotationBlockReconciler() {} + + @Override + public UpdateControl reconcile( + PrevAnnotationBlockCustomResource resource, + Context context) { + numberOfExecutions.getAndIncrement(); + + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java new file mode 100644 index 0000000000..137e2ba663 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java @@ -0,0 +1,50 @@ +package io.javaoperatorsdk.operator.dependent.prevblocklist; + +import java.time.Duration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +class PrevAnnotationBlockReconcilerIT { + + public static final String TEST_1 = "test1"; + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + // Removing resource from blocklist List would result in test failure + // .withConfigurationService( + // o -> o.previousAnnotationUsageBlocklist(Collections.emptyList())) + .withReconciler(PrevAnnotationBlockReconciler.class) + .build(); + + @Test + void doNotUsePrevAnnotationForDeploymentDependent() { + extension.create(testResource(TEST_1)); + + var reconciler = extension.getReconcilerOfType(PrevAnnotationBlockReconciler.class); + await() + .pollDelay(Duration.ofMillis(200)) + .untilAsserted( + () -> { + var deployment = extension.get(Deployment.class, TEST_1); + assertThat(deployment).isNotNull(); + assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0).isLessThan(10); + }); + } + + PrevAnnotationBlockCustomResource testResource(String name) { + var res = new PrevAnnotationBlockCustomResource(); + res.setMetadata(new ObjectMetaBuilder().withName(name).build()); + res.setSpec(new PrevAnnotationBlockSpec()); + res.getSpec().setValue("value"); + return res; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java new file mode 100644 index 0000000000..9d80e14bc1 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.dependent.prevblocklist; + +public class PrevAnnotationBlockSpec { + + private String value; + + public String getValue() { + return value; + } + + public PrevAnnotationBlockSpec setValue(String value) { + this.value = value; + return this; + } +} From 7a1902b61bfa2e0451b3c62b21a16705d870acd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 8 May 2025 09:43:04 +0200 Subject: [PATCH 6/9] explanation and using Set instead of list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/api/config/ConfigurationService.java | 9 ++++++--- .../api/config/ConfigurationServiceOverrider.java | 7 +++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index d2298d1fdf..bea0a53d68 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -1,7 +1,6 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; -import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -464,10 +463,14 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { * resources. Note that if you are sure that the matcher works (most of the cases does) for your * case, you can remove the resource from the blocklist. * + *

The consequence of adding a resource type to this list is that it will not use event + * filtering in dependent resources, so will process also events which are results from updates of + * the dependent. + * * @return blocklist of resource classes where the previous version annotation won't be used. */ - default List> previousAnnotationUsageBlocklist() { - return List.of(Deployment.class, StatefulSet.class); + default Set> previousAnnotationUsageBlocklist() { + return Set.of(Deployment.class, StatefulSet.class); } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 4430f616d9..bf61058b36 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -1,7 +1,6 @@ package io.javaoperatorsdk.operator.api.config; import java.time.Duration; -import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -41,7 +40,7 @@ public class ConfigurationServiceOverrider { private Boolean parseResourceVersions; private Boolean useSSAToPatchPrimaryResource; private Boolean cloneSecondaryResourcesWhenGettingFromCache; - private List> previousAnnotationUsageBlocklist; + private Set> previousAnnotationUsageBlocklist; @SuppressWarnings("rawtypes") private DependentResourceFactory dependentResourceFactory; @@ -191,7 +190,7 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC } public ConfigurationServiceOverrider previousAnnotationUsageBlocklist( - List> previousAnnotationUsageBlacklist) { + Set> previousAnnotationUsageBlacklist) { this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist; return this; } @@ -338,7 +337,7 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { } @Override - public List> previousAnnotationUsageBlocklist() { + public Set> previousAnnotationUsageBlocklist() { return overriddenValueOrDefault( previousAnnotationUsageBlocklist, ConfigurationService::previousAnnotationUsageBlocklist); From e31793c08afdac9b394bad0b30be02874db67ac8 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 15 May 2025 17:56:41 +0200 Subject: [PATCH 7/9] fix: improve javadoc Signed-off-by: Chris Laprun --- .../api/config/ConfigurationService.java | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index bea0a53d68..70e76a15f2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -444,30 +444,35 @@ default Set> defaultNonSSAResource() { * * @return if special annotation should be used for dependent resource to filter events * @since 4.5.0 - * @return if special annotation should be used for dependent resource to filter events */ default boolean previousAnnotationForDependentResourcesEventFiltering() { return true; } /** - * For dependent resources framework can add an annotation to filter our events that are results - * of changes made by the framework. There are, however, few resources that do not follow the K8S - * API convention that changes in metadata do not increase the "metadata.generation". For these - * resources, the generation is increased by adding the annotation and their controller increases - * the observedGeneration in the status. This results in a new event, that if not handled - * correctly with the resource matcher yet again results in an update and a previous version - * annotation change, thus results in an infinite loop. + * For dependent resources, the framework can add an annotation to filter out events resulting + * directly from the framework's operation. There are, however, some resources that do not follow + * the Kubernetes API conventions that changes in metadata should not increase the generation of + * the resource (as recorded in the {@code generation} field of the resource's {@code metadata}). + * For these resources, this convention is not respected and results in a new event for the + * framework to process. If that particular case is not handled correctly in the resource matcher, + * the framework will consider that the resource doesn't match the desired state and therefore + * triggers an update, which in turn, will re-add the annotation, thus starting the loop again, + * infinitely. * *

As a workaround, we automatically skip adding previous annotation for those well-known - * resources. Note that if you are sure that the matcher works (most of the cases does) for your - * case, you can remove the resource from the blocklist. + * resources. Note that if you are sure that the matcher works for your use case, and it should in + * most instances, you can remove the resource type from the blocklist. + * + *

The consequence of adding a resource type to the set is that the framework will not use + * event filtering to prevent events, initiated by changes made by the framework itself as a + * result of its processing of dependent resources, to trigger the associated reconciler again. * - *

The consequence of adding a resource type to this list is that it will not use event - * filtering in dependent resources, so will process also events which are results from updates of - * the dependent. + *

Note that this method only takes effect if annotating dependent resources to prevent + * dependent resources events from triggering the associated reconciler again is activated as + * controlled by {@link #previousAnnotationForDependentResourcesEventFiltering()} * - * @return blocklist of resource classes where the previous version annotation won't be used. + * @return a Set of resource classes where the previous version annotation won't be used. */ default Set> previousAnnotationUsageBlocklist() { return Set.of(Deployment.class, StatefulSet.class); From 19ce185af4fa58619c00e60e434a12efca40e9d1 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 15 May 2025 18:06:39 +0200 Subject: [PATCH 8/9] refactor: rename so that relation between methods is more explicit Signed-off-by: Chris Laprun --- .../operator/api/config/ConfigurationService.java | 3 ++- .../api/config/ConfigurationServiceOverrider.java | 11 ++++++----- .../kubernetes/KubernetesDependentResource.java | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 70e76a15f2..2f890aac25 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -474,7 +474,8 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { * * @return a Set of resource classes where the previous version annotation won't be used. */ - default Set> previousAnnotationUsageBlocklist() { + default Set> + previousAnnotationForDependentResourcesEventFilteringBlocklist() { return Set.of(Deployment.class, StatefulSet.class); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index bf61058b36..e487e3c6ee 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -189,9 +189,9 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC return this; } - public ConfigurationServiceOverrider previousAnnotationUsageBlocklist( - Set> previousAnnotationUsageBlacklist) { - this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist; + public ConfigurationServiceOverrider withPreviousAnnotationForDependentResourcesBlocklist( + Set> blocklist) { + this.previousAnnotationUsageBlocklist = blocklist; return this; } @@ -337,10 +337,11 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { } @Override - public Set> previousAnnotationUsageBlocklist() { + public Set> + previousAnnotationForDependentResourcesEventFilteringBlocklist() { return overriddenValueOrDefault( previousAnnotationUsageBlocklist, - ConfigurationService::previousAnnotationUsageBlocklist); + ConfigurationService::previousAnnotationForDependentResourcesEventFilteringBlocklist); } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index db33990d45..a638b15fdb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -173,7 +173,7 @@ private boolean usePreviousAnnotation(Context

context) { && !context .getControllerConfiguration() .getConfigurationService() - .previousAnnotationUsageBlocklist() + .previousAnnotationForDependentResourcesEventFilteringBlocklist() .contains(this.resourceType()); } return usePreviousAnnotationForEventFiltering; From 7639330f209d5030bf59c4a236c6b4b23845f20d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Fri, 16 May 2025 13:39:25 +0200 Subject: [PATCH 9/9] naming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../operator/api/config/ConfigurationService.java | 3 +-- .../operator/api/config/ConfigurationServiceOverrider.java | 4 ++-- .../dependent/kubernetes/KubernetesDependentResource.java | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 2f890aac25..18e74d29a9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -474,8 +474,7 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() { * * @return a Set of resource classes where the previous version annotation won't be used. */ - default Set> - previousAnnotationForDependentResourcesEventFilteringBlocklist() { + default Set> withPreviousAnnotationForDependentResourcesBlocklist() { return Set.of(Deployment.class, StatefulSet.class); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index e487e3c6ee..636c664f6b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -338,10 +338,10 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() { @Override public Set> - previousAnnotationForDependentResourcesEventFilteringBlocklist() { + withPreviousAnnotationForDependentResourcesBlocklist() { return overriddenValueOrDefault( previousAnnotationUsageBlocklist, - ConfigurationService::previousAnnotationForDependentResourcesEventFilteringBlocklist); + ConfigurationService::withPreviousAnnotationForDependentResourcesBlocklist); } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index a638b15fdb..03f4bcc812 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -173,7 +173,7 @@ private boolean usePreviousAnnotation(Context

context) { && !context .getControllerConfiguration() .getConfigurationService() - .previousAnnotationForDependentResourcesEventFilteringBlocklist() + .withPreviousAnnotationForDependentResourcesBlocklist() .contains(this.resourceType()); } return usePreviousAnnotationForEventFiltering;