From d5a573744352af1e1ba53d65d264bbbdb9c8a448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 8 Aug 2023 09:34:00 +0200 Subject: [PATCH 01/11] feat: create resource only if not exists (#2001) --- .../kubernetes/KubernetesDependent.java | 9 ++- .../KubernetesDependentConverter.java | 12 +++- .../KubernetesDependentResource.java | 7 ++- .../KubernetesDependentResourceConfig.java | 17 +++++- ...eateOnlyIfNotExistingDependentWithSSA.java | 57 +++++++++++++++++++ .../ConfigMapDependentResource.java | 30 ++++++++++ ...xistingDependentWithSSACustomResource.java | 13 +++++ ...NotExistingDependentWithSSAReconciler.java | 32 +++++++++++ 8 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/CreateOnlyIfNotExistingDependentWithSSA.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/ConfigMapDependentResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSACustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSAReconciler.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java index 868ae30dbc..c3f7be408a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java @@ -7,7 +7,10 @@ import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator; -import io.javaoperatorsdk.operator.processing.event.source.filter.*; +import io.javaoperatorsdk.operator.processing.event.source.filter.GenericFilter; +import io.javaoperatorsdk.operator.processing.event.source.filter.OnAddFilter; +import io.javaoperatorsdk.operator.processing.event.source.filter.OnDeleteFilter; +import io.javaoperatorsdk.operator.processing.event.source.filter.OnUpdateFilter; import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_VALUE_SET; @@ -69,4 +72,8 @@ Class resourceDiscriminator() default ResourceDiscriminator.class; + /** + * Creates the resource only if did not exist before, this applies only if SSA is used. + */ + boolean createResourceOnlyIfNotExistingWithSSA() default KubernetesDependentResourceConfig.DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java index a9a60f8e0a..6ab07a9462 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java @@ -14,6 +14,8 @@ import io.javaoperatorsdk.operator.processing.event.source.filter.OnDeleteFilter; import io.javaoperatorsdk.operator.processing.event.source.filter.OnUpdateFilter; +import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig.DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA; + public class KubernetesDependentConverter implements ConfigurationConverter, KubernetesDependentResource> { @@ -25,6 +27,8 @@ public KubernetesDependentResourceConfig configFrom(KubernetesDependent confi var namespaces = parentConfiguration.getNamespaces(); var configuredNS = false; String labelSelector = null; + var createResourceOnlyIfNotExistingWithSSA = + DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA; OnAddFilter onAddFilter = null; OnUpdateFilter onUpdateFilter = null; OnDeleteFilter onDeleteFilter = null; @@ -39,9 +43,8 @@ public KubernetesDependentResourceConfig configFrom(KubernetesDependent confi final var fromAnnotation = configAnnotation.labelSelector(); labelSelector = Constants.NO_VALUE_SET.equals(fromAnnotation) ? null : fromAnnotation; - final var context = - Utils.contextFor(parentConfiguration, originatingClass, - configAnnotation.annotationType()); + final var context = Utils.contextFor(parentConfiguration, originatingClass, + configAnnotation.annotationType()); onAddFilter = Utils.instantiate(configAnnotation.onAddFilter(), OnAddFilter.class, context); onUpdateFilter = Utils.instantiate(configAnnotation.onUpdateFilter(), OnUpdateFilter.class, context); @@ -53,9 +56,12 @@ public KubernetesDependentResourceConfig configFrom(KubernetesDependent confi resourceDiscriminator = Utils.instantiate(configAnnotation.resourceDiscriminator(), ResourceDiscriminator.class, context); + createResourceOnlyIfNotExistingWithSSA = + configAnnotation.createResourceOnlyIfNotExistingWithSSA(); } return new KubernetesDependentResourceConfig(namespaces, labelSelector, configuredNS, + createResourceOnlyIfNotExistingWithSSA, resourceDiscriminator, onAddFilter, onUpdateFilter, onDeleteFilter, genericFilter); } } 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 20e83f02bf..8d32892e10 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 @@ -130,7 +130,12 @@ protected R handleUpdate(R actual, R desired, P primary, Context

context) { public R create(R target, P primary, Context

context) { if (useSSA(context)) { // setting resource version for SSA so only created if it doesn't exist already - target.getMetadata().setResourceVersion("1"); + var createIfNotExisting = kubernetesDependentResourceConfig == null + ? KubernetesDependentResourceConfig.DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA + : kubernetesDependentResourceConfig.createResourceOnlyIfNotExistingWithSSA(); + if (createIfNotExisting) { + target.getMetadata().setResourceVersion("1"); + } } final var resource = prepare(target, primary, "Creating"); return useSSA(context) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java index 4047b25a13..33f4f91d1f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java @@ -13,9 +13,12 @@ public class KubernetesDependentResourceConfig { + public static final boolean DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA = true; + private Set namespaces = Constants.SAME_AS_CONTROLLER_NAMESPACES_SET; private String labelSelector = NO_VALUE_SET; private boolean namespacesWereConfigured = false; + private boolean createResourceOnlyIfNotExistingWithSSA; private ResourceDiscriminator resourceDiscriminator; private OnAddFilter onAddFilter; @@ -28,14 +31,18 @@ public class KubernetesDependentResourceConfig { public KubernetesDependentResourceConfig() {} - public KubernetesDependentResourceConfig(Set namespaces, String labelSelector, - boolean configuredNS, ResourceDiscriminator resourceDiscriminator, + public KubernetesDependentResourceConfig(Set namespaces, + String labelSelector, + boolean configuredNS, + boolean createResourceOnlyIfNotExistingWithSSA, + ResourceDiscriminator resourceDiscriminator, OnAddFilter onAddFilter, OnUpdateFilter onUpdateFilter, OnDeleteFilter onDeleteFilter, GenericFilter genericFilter) { this.namespaces = namespaces; this.labelSelector = labelSelector; this.namespacesWereConfigured = configuredNS; + this.createResourceOnlyIfNotExistingWithSSA = createResourceOnlyIfNotExistingWithSSA; this.onAddFilter = onAddFilter; this.onUpdateFilter = onUpdateFilter; this.onDeleteFilter = onDeleteFilter; @@ -44,7 +51,8 @@ public KubernetesDependentResourceConfig(Set namespaces, String labelSel } public KubernetesDependentResourceConfig(Set namespaces, String labelSelector) { - this(namespaces, labelSelector, true, null, null, null, + this(namespaces, labelSelector, true, DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA, + null, null, null, null, null); } @@ -70,6 +78,9 @@ public OnAddFilter onAddFilter() { return onAddFilter; } + public boolean createResourceOnlyIfNotExistingWithSSA() { + return createResourceOnlyIfNotExistingWithSSA; + } public OnUpdateFilter onUpdateFilter() { return onUpdateFilter; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/CreateOnlyIfNotExistingDependentWithSSA.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CreateOnlyIfNotExistingDependentWithSSA.java new file mode 100644 index 0000000000..8a347f632e --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CreateOnlyIfNotExistingDependentWithSSA.java @@ -0,0 +1,57 @@ +package io.javaoperatorsdk.operator; + +import java.time.Duration; +import java.util.Map; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.createonlyifnotexistsdependentwithssa.CreateOnlyIfNotExistingDependentWithSSACustomResource; +import io.javaoperatorsdk.operator.sample.createonlyifnotexistsdependentwithssa.CreateOnlyIfNotExistingDependentWithSSAReconciler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +class CreateOnlyIfNotExistingDependentWithSSA { + + public static final String TEST_RESOURCE_NAME = "test1"; + public static final String KEY = "key"; + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(new CreateOnlyIfNotExistingDependentWithSSAReconciler()) + .build(); + + + @Test + void createsResourceOnlyIfNotExisting() { + var cm = new ConfigMapBuilder().withMetadata(new ObjectMetaBuilder() + .withName(TEST_RESOURCE_NAME) + .build()) + .withData(Map.of(KEY, "val")) + .build(); + + extension.create(cm); + extension.create(testResource()); + + await().pollDelay(Duration.ofMillis(200)).untilAsserted(() -> { + var currentCM = extension.get(ConfigMap.class, TEST_RESOURCE_NAME); + assertThat(currentCM.getData()).containsKey(KEY); + }); + } + + CreateOnlyIfNotExistingDependentWithSSACustomResource testResource() { + var res = new CreateOnlyIfNotExistingDependentWithSSACustomResource(); + res.setMetadata(new ObjectMetaBuilder() + .withName(TEST_RESOURCE_NAME) + .build()); + + return res; + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/ConfigMapDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/ConfigMapDependentResource.java new file mode 100644 index 0000000000..d6947c2834 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/ConfigMapDependentResource.java @@ -0,0 +1,30 @@ +package io.javaoperatorsdk.operator.sample.createonlyifnotexistsdependentwithssa; + +import java.util.Map; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; + +public class ConfigMapDependentResource extends + CRUDKubernetesDependentResource { + + public ConfigMapDependentResource() { + super(ConfigMap.class); + } + + @Override + protected ConfigMap desired(CreateOnlyIfNotExistingDependentWithSSACustomResource primary, + Context context) { + ConfigMap configMap = new ConfigMap(); + configMap.setMetadata(new ObjectMetaBuilder() + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()); + configMap.setData(Map.of("drkey", "v")); + return configMap; + } +} + + diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSACustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSACustomResource.java new file mode 100644 index 0000000000..23f06c365f --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSACustomResource.java @@ -0,0 +1,13 @@ +package io.javaoperatorsdk.operator.sample.createonlyifnotexistsdependentwithssa; + +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.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +public class CreateOnlyIfNotExistingDependentWithSSACustomResource + extends CustomResource + implements Namespaced { +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSAReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSAReconciler.java new file mode 100644 index 0000000000..884b5a859d --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createonlyifnotexistsdependentwithssa/CreateOnlyIfNotExistingDependentWithSSAReconciler.java @@ -0,0 +1,32 @@ +package io.javaoperatorsdk.operator.sample.createonlyifnotexistsdependentwithssa; + +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.dependent.Dependent; + +@ControllerConfiguration(dependents = { + @Dependent(type = ConfigMapDependentResource.class)}) +public class CreateOnlyIfNotExistingDependentWithSSAReconciler + implements Reconciler { + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + + @Override + public UpdateControl reconcile( + CreateOnlyIfNotExistingDependentWithSSACustomResource resource, + Context context) { + numberOfExecutions.addAndGet(1); + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } + + + +} From ffdcc10c5f2c684afc6cb8d812aefb2d24eeab64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 14 Aug 2023 12:07:21 +0200 Subject: [PATCH 02/11] feat: leader election callbacks (#2015) --- .../operator/LeaderElectionManager.java | 19 ++++-- .../config/LeaderElectionConfiguration.java | 20 ++++-- .../LeaderElectionConfigurationBuilder.java | 61 +++++++++++++++++++ 3 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfigurationBuilder.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index 1509d87f2a..316fdb4524 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -66,7 +66,7 @@ private void init(LeaderElectionConfiguration config) { config.getLeaseDuration(), config.getRenewDeadline(), config.getRetryPeriod(), - leaderCallbacks(), + leaderCallbacks(config), true, config.getLeaseName())) .build(); @@ -74,11 +74,20 @@ private void init(LeaderElectionConfiguration config) { - private LeaderCallbacks leaderCallbacks() { + private LeaderCallbacks leaderCallbacks(LeaderElectionConfiguration config) { return new LeaderCallbacks( - this::startLeading, - this::stopLeading, - leader -> log.info("New leader with identity: {}", leader)); + () -> { + config.getLeaderCallbacks().ifPresent(LeaderCallbacks::onStartLeading); + LeaderElectionManager.this.startLeading(); + }, + () -> { + config.getLeaderCallbacks().ifPresent(LeaderCallbacks::onStopLeading); + LeaderElectionManager.this.stopLeading(); + }, + leader -> { + config.getLeaderCallbacks().ifPresent(cb -> cb.onNewLeader(leader)); + log.info("New leader with identity: {}", leader); + }); } private void startLeading() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfiguration.java index 5a2c322657..0ab72ff165 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfiguration.java @@ -3,6 +3,8 @@ import java.time.Duration; import java.util.Optional; +import io.fabric8.kubernetes.client.extended.leaderelection.LeaderCallbacks; + public class LeaderElectionConfiguration { public static final Duration LEASE_DURATION_DEFAULT_VALUE = Duration.ofSeconds(15); @@ -17,13 +19,15 @@ public class LeaderElectionConfiguration { private final Duration renewDeadline; private final Duration retryPeriod; + private final LeaderCallbacks leaderCallbacks; + public LeaderElectionConfiguration(String leaseName, String leaseNamespace, String identity) { this( leaseName, leaseNamespace, LEASE_DURATION_DEFAULT_VALUE, RENEW_DEADLINE_DEFAULT_VALUE, - RETRY_PERIOD_DEFAULT_VALUE, identity); + RETRY_PERIOD_DEFAULT_VALUE, identity, null); } public LeaderElectionConfiguration(String leaseName, String leaseNamespace) { @@ -32,7 +36,7 @@ public LeaderElectionConfiguration(String leaseName, String leaseNamespace) { leaseNamespace, LEASE_DURATION_DEFAULT_VALUE, RENEW_DEADLINE_DEFAULT_VALUE, - RETRY_PERIOD_DEFAULT_VALUE, null); + RETRY_PERIOD_DEFAULT_VALUE, null, null); } public LeaderElectionConfiguration(String leaseName) { @@ -41,7 +45,7 @@ public LeaderElectionConfiguration(String leaseName) { null, LEASE_DURATION_DEFAULT_VALUE, RENEW_DEADLINE_DEFAULT_VALUE, - RETRY_PERIOD_DEFAULT_VALUE, null); + RETRY_PERIOD_DEFAULT_VALUE, null, null); } public LeaderElectionConfiguration( @@ -50,7 +54,7 @@ public LeaderElectionConfiguration( Duration leaseDuration, Duration renewDeadline, Duration retryPeriod) { - this(leaseName, leaseNamespace, leaseDuration, renewDeadline, retryPeriod, null); + this(leaseName, leaseNamespace, leaseDuration, renewDeadline, retryPeriod, null, null); } public LeaderElectionConfiguration( @@ -59,13 +63,15 @@ public LeaderElectionConfiguration( Duration leaseDuration, Duration renewDeadline, Duration retryPeriod, - String identity) { + String identity, + LeaderCallbacks leaderCallbacks) { this.leaseName = leaseName; this.leaseNamespace = leaseNamespace; this.leaseDuration = leaseDuration; this.renewDeadline = renewDeadline; this.retryPeriod = retryPeriod; this.identity = identity; + this.leaderCallbacks = leaderCallbacks; } public Optional getLeaseNamespace() { @@ -91,4 +97,8 @@ public Duration getRetryPeriod() { public Optional getIdentity() { return Optional.ofNullable(identity); } + + public Optional getLeaderCallbacks() { + return Optional.ofNullable(leaderCallbacks); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfigurationBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfigurationBuilder.java new file mode 100644 index 0000000000..4b21dd9d2d --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/LeaderElectionConfigurationBuilder.java @@ -0,0 +1,61 @@ +package io.javaoperatorsdk.operator.api.config; + +import java.time.Duration; + +import io.fabric8.kubernetes.client.extended.leaderelection.LeaderCallbacks; + +import static io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration.*; + +public final class LeaderElectionConfigurationBuilder { + + private String leaseName; + private String leaseNamespace; + private String identity; + private Duration leaseDuration = LEASE_DURATION_DEFAULT_VALUE; + private Duration renewDeadline = RENEW_DEADLINE_DEFAULT_VALUE; + private Duration retryPeriod = RETRY_PERIOD_DEFAULT_VALUE; + private LeaderCallbacks leaderCallbacks; + + private LeaderElectionConfigurationBuilder(String leaseName) { + this.leaseName = leaseName; + } + + public static LeaderElectionConfigurationBuilder aLeaderElectionConfiguration(String leaseName) { + return new LeaderElectionConfigurationBuilder(leaseName); + } + + public LeaderElectionConfigurationBuilder withLeaseNamespace(String leaseNamespace) { + this.leaseNamespace = leaseNamespace; + return this; + } + + public LeaderElectionConfigurationBuilder withIdentity(String identity) { + this.identity = identity; + return this; + } + + public LeaderElectionConfigurationBuilder withLeaseDuration(Duration leaseDuration) { + this.leaseDuration = leaseDuration; + return this; + } + + public LeaderElectionConfigurationBuilder withRenewDeadline(Duration renewDeadline) { + this.renewDeadline = renewDeadline; + return this; + } + + public LeaderElectionConfigurationBuilder withRetryPeriod(Duration retryPeriod) { + this.retryPeriod = retryPeriod; + return this; + } + + public LeaderElectionConfigurationBuilder withLeaderCallbacks(LeaderCallbacks leaderCallbacks) { + this.leaderCallbacks = leaderCallbacks; + return this; + } + + public LeaderElectionConfiguration build() { + return new LeaderElectionConfiguration(leaseName, leaseNamespace, leaseDuration, renewDeadline, + retryPeriod, identity, leaderCallbacks); + } +} From 5c458e56756430087481858af8d720ec5ed5cd00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 14 Aug 2023 14:17:29 +0200 Subject: [PATCH 03/11] discriminator improvements (#2013) --- .../api/reconciler/IndexDiscriminator.java | 50 +++++++++++++++++++ .../ResourceIDMatcherDiscriminator.java | 26 ++++++++-- .../IndexDiscriminator.java | 41 --------------- .../IndexDiscriminatorTestReconciler.java | 4 +- .../TestIndexDiscriminator.java | 14 ++++++ 5 files changed, 89 insertions(+), 46 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/IndexDiscriminator.java delete mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminator.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/TestIndexDiscriminator.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/IndexDiscriminator.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/IndexDiscriminator.java new file mode 100644 index 0000000000..7a27397b26 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/IndexDiscriminator.java @@ -0,0 +1,50 @@ +package io.javaoperatorsdk.operator.api.reconciler; + +import java.util.Optional; +import java.util.function.Function; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; + +/** + * Uses a custom index of {@link InformerEventSource} to access the target resource. The index needs + * to be explicitly created when the event source is defined. This approach improves the performance + * to access the resource. + */ +public class IndexDiscriminator + implements ResourceDiscriminator { + + private final String indexName; + private final String eventSourceName; + private final Function keyMapper; + + public IndexDiscriminator(String indexName, Function keyMapper) { + this(indexName, null, keyMapper); + } + + public IndexDiscriminator(String indexName, String eventSourceName, + Function keyMapper) { + this.indexName = indexName; + this.eventSourceName = eventSourceName; + this.keyMapper = keyMapper; + } + + @Override + public Optional distinguish(Class resource, + P primary, + Context

context) { + + InformerEventSource eventSource = + (InformerEventSource) context + .eventSourceRetriever() + .getResourceEventSourceFor(resource, eventSourceName); + var resources = eventSource.byIndex(indexName, keyMapper.apply(primary)); + if (resources.isEmpty()) { + return Optional.empty(); + } else if (resources.size() > 1) { + throw new IllegalStateException("More than one resource found"); + } else { + return Optional.of(resources.get(0)); + } + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceIDMatcherDiscriminator.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceIDMatcherDiscriminator.java index d23459e271..da773fc210 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceIDMatcherDiscriminator.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceIDMatcherDiscriminator.java @@ -5,21 +5,41 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.javaoperatorsdk.operator.processing.event.source.Cache; public class ResourceIDMatcherDiscriminator implements ResourceDiscriminator { + + private final String eventSourceName; private final Function mapper; public ResourceIDMatcherDiscriminator(Function mapper) { + this(null, mapper); + } + + public ResourceIDMatcherDiscriminator(String eventSourceName, Function mapper) { + this.eventSourceName = eventSourceName; this.mapper = mapper; } + @SuppressWarnings("unchecked") @Override public Optional distinguish(Class resource, P primary, Context

context) { var resourceID = mapper.apply(primary); - return context.getSecondaryResourcesAsStream(resource) - .filter(resourceID::isSameResource) - .findFirst(); + if (eventSourceName != null) { + return ((Cache) context.eventSourceRetriever().getResourceEventSourceFor(resource, + eventSourceName)) + .get(resourceID); + } else { + var eventSources = context.eventSourceRetriever().getResourceEventSourcesFor(resource); + if (eventSources.size() == 1) { + return ((Cache) eventSources.get(0)).get(resourceID); + } else { + return context.getSecondaryResourcesAsStream(resource) + .filter(resourceID::isSameResource) + .findFirst(); + } + } } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminator.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminator.java deleted file mode 100644 index eb6e193479..0000000000 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminator.java +++ /dev/null @@ -1,41 +0,0 @@ -package io.javaoperatorsdk.operator.sample.indexdiscriminator; - -import java.util.Optional; - -import io.fabric8.kubernetes.api.model.ConfigMap; -import io.javaoperatorsdk.operator.api.reconciler.Context; -import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator; -import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; - -import static io.javaoperatorsdk.operator.sample.indexdiscriminator.IndexDiscriminatorTestReconciler.configMapKeyFromPrimary; - -public class IndexDiscriminator - implements ResourceDiscriminator { - - private final String indexName; - private final String nameSuffix; - - public IndexDiscriminator(String indexName, String nameSuffix) { - this.indexName = indexName; - this.nameSuffix = nameSuffix; - } - - @Override - public Optional distinguish(Class resource, - IndexDiscriminatorTestCustomResource primary, - Context context) { - - InformerEventSource eventSource = - (InformerEventSource) context - .eventSourceRetriever() - .getResourceEventSourceFor(ConfigMap.class); - var resources = eventSource.byIndex(indexName, configMapKeyFromPrimary(primary, nameSuffix)); - if (resources.isEmpty()) { - return Optional.empty(); - } else if (resources.size() > 1) { - throw new IllegalStateException("more than one resource"); - } else { - return Optional.of(resources.get(0)); - } - } -} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminatorTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminatorTestReconciler.java index 0b0af2a1cc..b988c93491 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminatorTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/IndexDiscriminatorTestReconciler.java @@ -81,10 +81,10 @@ public Map prepareEventSources( firstDependentResourceConfigMap .setResourceDiscriminator( - new IndexDiscriminator(CONFIG_MAP_INDEX_1, FIRST_CONFIG_MAP_SUFFIX_1)); + new TestIndexDiscriminator(CONFIG_MAP_INDEX_1, FIRST_CONFIG_MAP_SUFFIX_1)); secondDependentResourceConfigMap .setResourceDiscriminator( - new IndexDiscriminator(CONFIG_MAP_INDEX_2, FIRST_CONFIG_MAP_SUFFIX_2)); + new TestIndexDiscriminator(CONFIG_MAP_INDEX_2, FIRST_CONFIG_MAP_SUFFIX_2)); return EventSourceInitializer.nameEventSources(eventSource); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/TestIndexDiscriminator.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/TestIndexDiscriminator.java new file mode 100644 index 0000000000..a56e44ced8 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/indexdiscriminator/TestIndexDiscriminator.java @@ -0,0 +1,14 @@ +package io.javaoperatorsdk.operator.sample.indexdiscriminator; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.javaoperatorsdk.operator.api.reconciler.IndexDiscriminator; + +import static io.javaoperatorsdk.operator.sample.indexdiscriminator.IndexDiscriminatorTestReconciler.configMapKeyFromPrimary; + +public class TestIndexDiscriminator + extends IndexDiscriminator { + + public TestIndexDiscriminator(String indexName, String nameSuffix) { + super(indexName, p -> configMapKeyFromPrimary(p, nameSuffix)); + } +} From ddeb01595fcd801d1cc9dd7677bff6cc1134a7bb Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Tue, 8 Aug 2023 12:30:52 -0400 Subject: [PATCH 04/11] changes to remove the recording mechanism uses an annotation and knowledge of the resourceVersion to track early events instead --- .../dependent/RecentOperationEventFilter.java | 11 -- .../KubernetesDependentResource.java | 42 ++---- ...BasedGenericKubernetesResourceMatcher.java | 15 ++ .../event/source/informer/EventRecorder.java | 72 ---------- .../source/informer/InformerEventSource.java | 132 ++++++------------ .../informer/TemporaryResourceCache.java | 4 +- .../source/informer/EventRecorderTest.java | 81 ----------- .../informer/InformerEventSourceTest.java | 109 --------------- ...CreateUpdateEventFilterTestReconciler.java | 45 ++---- 9 files changed, 85 insertions(+), 426 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/RecentOperationEventFilter.java delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorder.java delete mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorderTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/RecentOperationEventFilter.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/RecentOperationEventFilter.java deleted file mode 100644 index d48343e57c..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/RecentOperationEventFilter.java +++ /dev/null @@ -1,11 +0,0 @@ -package io.javaoperatorsdk.operator.api.reconciler.dependent; - -import io.javaoperatorsdk.operator.processing.event.ResourceID; - -public interface RecentOperationEventFilter extends RecentOperationCacheFiller { - - void prepareForCreateOrUpdateEventFiltering(ResourceID resourceID, R resource); - - void cleanupOnCreateOrUpdateEventFiltering(ResourceID resourceID); - -} 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 8d32892e10..2147b8c07b 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 @@ -37,6 +37,8 @@ public abstract class KubernetesDependentResource> { + public static String PREVIOUS_ANNOTATION_KEY = "javaoperatorsdk.io/previous"; + private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); protected KubernetesClient client; @@ -103,29 +105,6 @@ public void configureWith(InformerEventSource informerEventSource) { setEventSource(informerEventSource); } - - protected R handleCreate(R desired, P primary, Context

context) { - ResourceID resourceID = ResourceID.fromResource(desired); - try { - prepareEventFiltering(desired, resourceID); - return super.handleCreate(desired, primary, context); - } catch (RuntimeException e) { - cleanupAfterEventFiltering(resourceID); - throw e; - } - } - - protected R handleUpdate(R actual, R desired, P primary, Context

context) { - ResourceID resourceID = ResourceID.fromResource(desired); - try { - prepareEventFiltering(desired, resourceID); - return super.handleUpdate(actual, desired, primary, context); - } catch (RuntimeException e) { - cleanupAfterEventFiltering(resourceID); - throw e; - } - } - @SuppressWarnings("unused") public R create(R target, P primary, Context

context) { if (useSSA(context)) { @@ -137,6 +116,8 @@ public R create(R target, P primary, Context

context) { target.getMetadata().setResourceVersion("1"); } } + String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); + target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, id); final var resource = prepare(target, primary, "Creating"); return useSSA(context) ? resource @@ -152,6 +133,9 @@ public R update(R actual, R target, P primary, Context

context) { actual.getMetadata().getResourceVersion()); } R updatedResource; + String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); + target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, + id + "," + actual.getMetadata().getResourceVersion()); if (useSSA(context)) { updatedResource = prepare(target, primary, "Updating") .fieldManager(context.getControllerConfiguration().fieldManager()) @@ -165,6 +149,7 @@ public R update(R actual, R target, P primary, Context

context) { return updatedResource; } + @Override public Result match(R actualResource, P primary, Context

context) { final var desired = desired(primary, context); final boolean matches; @@ -197,6 +182,7 @@ private boolean useSSA(Context

context) { .ssaBasedCreateUpdateMatchForDependentResources(); } + @Override protected void handleDelete(P primary, R secondary, Context

context) { if (secondary != null) { client.resource(secondary).delete(); @@ -294,16 +280,6 @@ protected R desired(P primary, Context

context) { return super.desired(primary, context); } - private void prepareEventFiltering(R desired, ResourceID resourceID) { - ((InformerEventSource) eventSource().orElseThrow()) - .prepareForCreateOrUpdateEventFiltering(resourceID, desired); - } - - private void cleanupAfterEventFiltering(ResourceID resourceID) { - ((InformerEventSource) eventSource().orElseThrow()) - .cleanupOnCreateOrUpdateEventFiltering(resourceID); - } - @Override public Optional> configuration() { return Optional.ofNullable(kubernetesDependentResourceConfig); 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 f4718b45c3..84d841e64c 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 @@ -90,6 +90,8 @@ public boolean matches(R actual, R desired, Context context) { keepOnlyManagedFields(prunedActual, actualMap, managedFieldsEntry.getFieldsV1().getAdditionalProperties(), objectMapper); + removeSDKAnnotations(prunedActual); + removeIrrelevantValues(desiredMap); if (LoggingUtils.isNotSensitiveResource(desired)) { @@ -99,6 +101,19 @@ public boolean matches(R actual, R desired, Context context) { return prunedActual.equals(desiredMap); } + private void removeSDKAnnotations(HashMap prunedActual) { + Optional.ofNullable(((Map) prunedActual.get(METADATA_KEY))) + .ifPresent(m -> m.computeIfPresent("annotations", + (k, v) -> { + var annotations = (Map) v; + annotations.remove(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); + if (annotations.isEmpty()) { + return null; + } + return annotations; + })); + } + @SuppressWarnings("unchecked") private static void removeIrrelevantValues(Map desiredMap) { var metadata = (Map) desiredMap.get(METADATA_KEY); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorder.java deleted file mode 100644 index 5d23d870aa..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorder.java +++ /dev/null @@ -1,72 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event.source.informer; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.processing.event.ResourceID; - -public class EventRecorder { - - private final Map> resourceEvents = new HashMap<>(); - - public void startEventRecording(ResourceID resourceID) { - resourceEvents.putIfAbsent(resourceID, new ArrayList<>(5)); - } - - public boolean isRecordingFor(ResourceID resourceID) { - return resourceEvents.get(resourceID) != null; - } - - public void stopEventRecording(ResourceID resourceID) { - resourceEvents.remove(resourceID); - } - - public void recordEvent(R resource) { - resourceEvents.get(ResourceID.fromResource(resource)).add(resource); - } - - public boolean containsEventWithResourceVersion(ResourceID resourceID, - String resourceVersion) { - List events = resourceEvents.get(resourceID); - if (events == null) { - return false; - } - if (events.isEmpty()) { - return false; - } else { - return events.stream() - .anyMatch(e -> e.getMetadata().getResourceVersion().equals(resourceVersion)); - } - } - - public boolean containsEventWithVersionButItsNotLastOne( - ResourceID resourceID, String resourceVersion) { - List resources = resourceEvents.get(resourceID); - if (resources == null) { - throw new IllegalStateException( - "Null events list, this is probably a result of invalid usage of the " + - "InformerEventSource. Resource ID: " + resourceID); - } - if (resources.isEmpty()) { - throw new IllegalStateException("No events for resource id: " + resourceID); - } - return !resources - .get(resources.size() - 1) - .getMetadata() - .getResourceVersion() - .equals(resourceVersion); - } - - public R getLastEvent(ResourceID resourceID) { - List resources = resourceEvents.get(resourceID); - if (resources == null) { - throw new IllegalStateException( - "Null events list, this is probably a result of invalid usage of the " + - "InformerEventSource. Resource ID: " + resourceID); - } - return resources.get(resources.size() - 1); - } -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index 8cca524464..8c9946b924 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -5,6 +5,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.UUID; import java.util.function.Function; import java.util.stream.Collectors; @@ -18,7 +19,7 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; -import io.javaoperatorsdk.operator.api.reconciler.dependent.RecentOperationEventFilter; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -72,17 +73,16 @@ */ public class InformerEventSource extends ManagedInformerEventSource> - implements ResourceEventHandler, RecentOperationEventFilter { + implements ResourceEventHandler { private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class); private final InformerConfiguration configuration; - // always called from a synchronized method - private final EventRecorder eventRecorder = new EventRecorder<>(); // we need direct control for the indexer to propagate the just update resource also to the index private final PrimaryToSecondaryIndex primaryToSecondaryIndex; private final PrimaryToSecondaryMapper

primaryToSecondaryMapper; private Map>> indexerBuffer = new HashMap<>(); + private String id = UUID.randomUUID().toString(); public InformerEventSource( InformerConfiguration configuration, EventSourceContext

context) { @@ -110,6 +110,10 @@ public InformerEventSource(InformerConfiguration configuration, KubernetesCli genericFilter = configuration.genericFilter().orElse(null); } + public String getId() { + return id; + } + @Override public void onAdd(R newResource) { if (log.isDebugEnabled()) { @@ -154,12 +158,23 @@ public void onDelete(R resource, boolean b) { private synchronized void onAddOrUpdate(Operation operation, R newObject, R oldObject, Runnable superOnOp) { var resourceID = ResourceID.fromResource(newObject); - if (eventRecorder.isRecordingFor(resourceID)) { - log.debug("Recording event for: {}", resourceID); - eventRecorder.recordEvent(newObject); - return; + + String previous = newObject.getMetadata().getAnnotations() + .get(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); + boolean known = false; + if (previous != null) { + String[] parts = previous.split(","); + if (id.equals(parts[0])) { + if (oldObject == null && parts.length == 1) { + known = true; + } else if (oldObject != null && parts.length == 2 + && oldObject.getMetadata().getResourceVersion().equals(parts[1])) { + known = true; + } + } } - if (temporaryCacheHasResourceWithSameVersionAs(newObject)) { + if (temporaryCacheHasResourceWithSameVersionAs(newObject) + || (known && temporaryResourceCache.getResourceFromCache(resourceID).isEmpty())) { log.debug( "Skipping event propagation for {}, since was a result of a reconcile action. Resource ID: {}", operation, @@ -239,73 +254,31 @@ public InformerConfiguration getConfiguration() { @Override public synchronized void handleRecentResourceUpdate(ResourceID resourceID, R resource, R previousVersionOfResource) { - handleRecentCreateOrUpdate(Operation.UPDATE, resource, previousVersionOfResource, - () -> super.handleRecentResourceUpdate(resourceID, resource, previousVersionOfResource)); + handleRecentCreateOrUpdate(Operation.UPDATE, resource, previousVersionOfResource); } @Override public synchronized void handleRecentResourceCreate(ResourceID resourceID, R resource) { - handleRecentCreateOrUpdate(Operation.ADD, resource, null, - () -> super.handleRecentResourceCreate(resourceID, resource)); + handleRecentCreateOrUpdate(Operation.ADD, resource, null); } - private void handleRecentCreateOrUpdate(Operation operation, R resource, R oldResource, - Runnable runnable) { - primaryToSecondaryIndex.onAddOrUpdate(resource); - if (eventRecorder.isRecordingFor(ResourceID.fromResource(resource))) { - handleRecentResourceOperationAndStopEventRecording(operation, resource, oldResource); - } else { - runnable.run(); - } - } - - /** - * There can be the following cases: - *

    - *
  • 1. Did not receive the event yet for the target resource, then we need to put it to temp - * cache. Because event will arrive. Note that this not necessary mean that the even is not sent - * yet (we are in sync context). Also does not mean that there are no more events received after - * that. But during the event processing (onAdd, onUpdate) we make sure that the propagation just - * skipped for the right event.
    • - *
  • 2. Received the event about the operation already, it was the last. This means already is - * on cache of informer. So we have to do nothing. Since it was just recorded and not propagated. - *
    • - *
  • 3. Received the event but more events received since, so those were not propagated yet. So - * an event needs to be propagated to compensate.
    • - *
- * - * @param newResource just created or updated resource - */ - private void handleRecentResourceOperationAndStopEventRecording(Operation operation, - R newResource, R oldResource) { + private void handleRecentCreateOrUpdate(Operation operation, R newResource, R oldResource) { + primaryToSecondaryIndex.onAddOrUpdate(newResource); ResourceID resourceID = ResourceID.fromResource(newResource); - try { - if (!eventRecorder.containsEventWithResourceVersion( - resourceID, newResource.getMetadata().getResourceVersion())) { - log.debug( - "Did not found event in buffer with target version and resource id: {}", resourceID); - temporaryResourceCache.unconditionallyCacheResource(newResource); - } else { - // if the resource is not added to the temp cache, it is cleared, since - // the cache is cleared by subsequent events after updates, but if those did not receive - // the temp cache is still filled at this point with an old resource - log.debug("Cleaning temporary cache for resource id: {}", resourceID); - temporaryResourceCache.removeResourceFromCache(newResource); - if (eventRecorder.containsEventWithVersionButItsNotLastOne( - resourceID, newResource.getMetadata().getResourceVersion())) { - R lastEvent = eventRecorder.getLastEvent(resourceID); - - log.debug( - "Found events in event buffer but the target event is not last for id: {}. Propagating event.", - resourceID); - if (eventAcceptedByFilter(operation, newResource, oldResource)) { - propagateEvent(lastEvent); - } - } - } - } finally { - log.debug("Stopping event recording for: {}", resourceID); - eventRecorder.stopEventRecording(resourceID); + R cachedResource = get(resourceID).orElse(null); + if ((oldResource == null && cachedResource == null) + || (cachedResource != null && oldResource != null + && cachedResource.getMetadata().getResourceVersion() + .equals(oldResource.getMetadata().getResourceVersion()))) { + log.debug( + "Temporarily moving ahead to target version {} for resource id: {}", + newResource.getMetadata().getResourceVersion(), resourceID); + temporaryResourceCache.unconditionallyCacheResource(newResource); + } else if (temporaryResourceCache.removeResourceFromCache(newResource).isPresent()) { + // if the resource is not added to the temp cache, it is cleared, since + // the cache is cleared by subsequent events after updates, but if those did not receive + // the temp cache is still filled at this point with an old resource + log.debug("Cleaning temporary cache for resource id: {}", resourceID); } } @@ -313,25 +286,6 @@ private boolean useSecondaryToPrimaryIndex() { return this.primaryToSecondaryMapper == null; } - @Override - public synchronized void prepareForCreateOrUpdateEventFiltering(ResourceID resourceID, - R resource) { - log.debug("Starting event recording for: {}", resourceID); - eventRecorder.startEventRecording(resourceID); - } - - /** - * Mean to be called to clean up in case of an exception from the client. Usually in a catch - * block. - * - * @param resourceID to cleanup - */ - @Override - public synchronized void cleanupOnCreateOrUpdateEventFiltering(ResourceID resourceID) { - log.debug("Stopping event recording for: {}", resourceID); - eventRecorder.stopEventRecording(resourceID); - } - @Override public boolean allowsNamespaceChanges() { return getConfiguration().followControllerNamespaceChanges(); @@ -361,6 +315,7 @@ private boolean acceptedByDeleteFilters(R resource, boolean b) { // Since this event source instance is created by the user, the ConfigurationService is actually // injected after it is registered. Some of the subcomponents are initialized at that time here. + @Override public void setConfigurationService(ConfigurationService configurationService) { super.setConfigurationService(configurationService); @@ -368,6 +323,7 @@ public void setConfigurationService(ConfigurationService configurationService) { indexerBuffer = null; } + @Override public void addIndexers(Map>> indexers) { if (indexerBuffer == null) { throw new OperatorException("Cannot add indexers after InformerEventSource started."); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java index 8c3d56c008..65a063cff0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java @@ -41,8 +41,8 @@ public TemporaryResourceCache(ManagedInformerEventSource managedInforme this.managedInformerEventSource = managedInformerEventSource; } - public synchronized void removeResourceFromCache(T resource) { - cache.remove(ResourceID.fromResource(resource)); + public synchronized Optional removeResourceFromCache(T resource) { + return Optional.ofNullable(cache.remove(ResourceID.fromResource(resource))); } public synchronized void unconditionallyCacheResource(T newResource) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorderTest.java deleted file mode 100644 index 556ad089ff..0000000000 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventRecorderTest.java +++ /dev/null @@ -1,81 +0,0 @@ -package io.javaoperatorsdk.operator.processing.event.source.informer; - -import org.junit.jupiter.api.Test; - -import io.fabric8.kubernetes.api.model.ConfigMap; -import io.fabric8.kubernetes.api.model.ObjectMeta; -import io.javaoperatorsdk.operator.processing.event.ResourceID; - -import static org.assertj.core.api.Assertions.assertThat; - -class EventRecorderTest { - - public static final String RESOURCE_VERSION = "0"; - public static final String RESOURCE_VERSION1 = "1"; - - EventRecorder eventRecorder = new EventRecorder<>(); - - ConfigMap testConfigMap = testConfigMap(RESOURCE_VERSION); - ConfigMap testConfigMap2 = testConfigMap(RESOURCE_VERSION1); - - ResourceID id = ResourceID.fromResource(testConfigMap); - - @Test - void recordsEvents() { - - assertThat(eventRecorder.isRecordingFor(id)).isFalse(); - - eventRecorder.startEventRecording(id); - assertThat(eventRecorder.isRecordingFor(id)).isTrue(); - - eventRecorder.recordEvent(testConfigMap); - - eventRecorder.stopEventRecording(id); - assertThat(eventRecorder.isRecordingFor(id)).isFalse(); - } - - @Test - void getsLastRecorded() { - eventRecorder.startEventRecording(id); - - eventRecorder.recordEvent(testConfigMap); - eventRecorder.recordEvent(testConfigMap2); - - assertThat(eventRecorder.getLastEvent(id)).isEqualTo(testConfigMap2); - } - - @Test - void checksContainsWithResourceVersion() { - eventRecorder.startEventRecording(id); - - eventRecorder.recordEvent(testConfigMap); - eventRecorder.recordEvent(testConfigMap2); - - assertThat(eventRecorder.containsEventWithResourceVersion(id, RESOURCE_VERSION)).isTrue(); - assertThat(eventRecorder.containsEventWithResourceVersion(id, RESOURCE_VERSION1)).isTrue(); - assertThat(eventRecorder.containsEventWithResourceVersion(id, "xxx")).isFalse(); - } - - @Test - void checkLastItemVersion() { - eventRecorder.startEventRecording(id); - - eventRecorder.recordEvent(testConfigMap); - eventRecorder.recordEvent(testConfigMap2); - - assertThat(eventRecorder.containsEventWithVersionButItsNotLastOne(id, RESOURCE_VERSION)) - .isTrue(); - assertThat(eventRecorder.containsEventWithVersionButItsNotLastOne(id, RESOURCE_VERSION1)) - .isFalse(); - } - - ConfigMap testConfigMap(String resourceVersion) { - ConfigMap configMap = new ConfigMap(); - configMap.setMetadata(new ObjectMeta()); - configMap.getMetadata().setName("test"); - configMap.getMetadata().setResourceVersion(resourceVersion); - - return configMap; - } - -} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 69e26f0b35..336de37c68 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -91,115 +91,6 @@ void propagateEventAndRemoveResourceFromTempCacheIfResourceVersionMismatch() { verify(temporaryResourceCacheMock, times(1)).removeResourceFromCache(any()); } - @Test - void notPropagatesEventIfAfterUpdateReceivedJustTheRelatedEvent() { - var testDeployment = testDeployment(); - var prevTestDeployment = testDeployment(); - prevTestDeployment.getMetadata().setResourceVersion(PREV_RESOURCE_VERSION); - - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.onUpdate(prevTestDeployment, testDeployment); - informerEventSource.handleRecentResourceUpdate(ResourceID.fromResource(testDeployment), - testDeployment, prevTestDeployment); - - verify(eventHandlerMock, times(0)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(0)).unconditionallyCacheResource(any()); - } - - - @Test - void notPropagatesEventIfAfterCreateReceivedJustTheRelatedEvent() { - var testDeployment = testDeployment(); - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.onAdd(testDeployment); - informerEventSource.handleRecentResourceCreate(ResourceID.fromResource(testDeployment), - testDeployment); - - verify(eventHandlerMock, times(0)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(0)).unconditionallyCacheResource(any()); - } - - @Test - void propagatesEventIfNewEventReceivedAfterTheCurrentTargetEvent() { - var testDeployment = testDeployment(); - var prevTestDeployment = testDeployment(); - prevTestDeployment.getMetadata().setResourceVersion(PREV_RESOURCE_VERSION); - var nextTestDeployment = testDeployment(); - nextTestDeployment.getMetadata().setResourceVersion(NEXT_RESOURCE_VERSION); - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.onUpdate(prevTestDeployment, testDeployment); - informerEventSource.onUpdate(testDeployment, nextTestDeployment); - informerEventSource.handleRecentResourceUpdate(ResourceID.fromResource(testDeployment), - testDeployment, prevTestDeployment); - - verify(eventHandlerMock, times(1)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(0)).unconditionallyCacheResource(any()); - } - - @Test - void notPropagatesEventIfMoreReceivedButTheLastIsTheUpdated() { - var testDeployment = testDeployment(); - var prevTestDeployment = testDeployment(); - prevTestDeployment.getMetadata().setResourceVersion(PREV_RESOURCE_VERSION); - var prevPrevTestDeployment = testDeployment(); - prevPrevTestDeployment.getMetadata().setResourceVersion("-1"); - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.onUpdate(prevPrevTestDeployment, prevTestDeployment); - informerEventSource.onUpdate(prevTestDeployment, testDeployment); - informerEventSource.handleRecentResourceUpdate(ResourceID.fromResource(testDeployment), - testDeployment, prevTestDeployment); - - verify(eventHandlerMock, times(0)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(0)).unconditionallyCacheResource(any()); - } - - @Test - void putsResourceOnTempCacheIfNoEventRecorded() { - var testDeployment = testDeployment(); - var prevTestDeployment = testDeployment(); - prevTestDeployment.getMetadata().setResourceVersion(PREV_RESOURCE_VERSION); - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.handleRecentResourceUpdate(ResourceID.fromResource(testDeployment), - testDeployment, prevTestDeployment); - - verify(eventHandlerMock, times(0)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(1)).unconditionallyCacheResource(any()); - } - - @Test - void putsResourceOnTempCacheIfNoEventRecordedWithSameResourceVersion() { - var testDeployment = testDeployment(); - var prevTestDeployment = testDeployment(); - prevTestDeployment.getMetadata().setResourceVersion(PREV_RESOURCE_VERSION); - var prevPrevTestDeployment = testDeployment(); - prevPrevTestDeployment.getMetadata().setResourceVersion("-1"); - - informerEventSource - .prepareForCreateOrUpdateEventFiltering(ResourceID.fromResource(testDeployment), - testDeployment); - informerEventSource.onUpdate(prevPrevTestDeployment, prevTestDeployment); - informerEventSource.handleRecentResourceUpdate(ResourceID.fromResource(testDeployment), - testDeployment, prevTestDeployment); - - verify(eventHandlerMock, times(0)).handleEvent(any()); - verify(temporaryResourceCacheMock, times(1)).unconditionallyCacheResource(any()); - } - @Test void genericFilterForEvents() { informerEventSource.setGenericFilter(r -> false); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java index ef2ddfc4ec..bc82c487f6 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java @@ -46,41 +46,26 @@ public UpdateControl reconcile( if (configMap == null) { var configMapToCreate = createConfigMap(resource); final var resourceID = ResourceID.fromResource(configMapToCreate); - try { - informerEventSource.prepareForCreateOrUpdateEventFiltering(resourceID, configMapToCreate); - configMap = - client - .configMaps() - .inNamespace(resource.getMetadata().getNamespace()) - .resource(configMapToCreate) - .create(); - informerEventSource.handleRecentResourceCreate(resourceID, configMap); - } catch (RuntimeException e) { - informerEventSource - .cleanupOnCreateOrUpdateEventFiltering(resourceID); - throw e; - } + configMap = + client + .configMaps() + .inNamespace(resource.getMetadata().getNamespace()) + .resource(configMapToCreate) + .create(); + informerEventSource.handleRecentResourceCreate(resourceID, configMap); } else { ResourceID resourceID = ResourceID.fromResource(configMap); if (!Objects.equals( configMap.getData().get(CONFIG_MAP_TEST_DATA_KEY), resource.getSpec().getValue())) { configMap.getData().put(CONFIG_MAP_TEST_DATA_KEY, resource.getSpec().getValue()); - try { - informerEventSource - .prepareForCreateOrUpdateEventFiltering(resourceID, configMap); - var newConfigMap = - client - .configMaps() - .inNamespace(resource.getMetadata().getNamespace()) - .resource(configMap) - .replace(); - informerEventSource.handleRecentResourceUpdate(resourceID, - newConfigMap, configMap); - } catch (RuntimeException e) { - informerEventSource - .cleanupOnCreateOrUpdateEventFiltering(resourceID); - throw e; - } + var newConfigMap = + client + .configMaps() + .inNamespace(resource.getMetadata().getNamespace()) + .resource(configMap) + .replace(); + informerEventSource.handleRecentResourceUpdate(resourceID, + newConfigMap, configMap); } } return UpdateControl.noUpdate(); From 11c5a037c7eae77ab43388f82e6816e3edbabf54 Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Fri, 11 Aug 2023 09:25:27 -0400 Subject: [PATCH 05/11] consolidating the temporary cache updating logic and removing the locking change that will be handled via a separate pr --- .../KubernetesDependentResource.java | 2 +- .../source/informer/InformerEventSource.java | 18 +------- .../informer/ManagedInformerEventSource.java | 4 +- .../informer/TemporaryResourceCache.java | 45 +++++++++---------- .../informer/TemporaryResourceCacheTest.java | 4 +- 5 files changed, 29 insertions(+), 44 deletions(-) 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 2147b8c07b..c8d0568915 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 @@ -142,7 +142,7 @@ public R update(R actual, R target, P primary, Context

context) { .forceConflicts().serverSideApply(); } else { var updatedActual = updaterMatcher.updateResource(actual, target, context); - updatedResource = prepare(updatedActual, primary, "Updating").update(); + updatedResource = prepare(updatedActual, primary, "Updating").replace(); } log.debug("Resource version after update: {}", updatedResource.getMetadata().getResourceVersion()); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index 8c9946b924..7c5dbd83d5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -264,22 +264,8 @@ public synchronized void handleRecentResourceCreate(ResourceID resourceID, R res private void handleRecentCreateOrUpdate(Operation operation, R newResource, R oldResource) { primaryToSecondaryIndex.onAddOrUpdate(newResource); - ResourceID resourceID = ResourceID.fromResource(newResource); - R cachedResource = get(resourceID).orElse(null); - if ((oldResource == null && cachedResource == null) - || (cachedResource != null && oldResource != null - && cachedResource.getMetadata().getResourceVersion() - .equals(oldResource.getMetadata().getResourceVersion()))) { - log.debug( - "Temporarily moving ahead to target version {} for resource id: {}", - newResource.getMetadata().getResourceVersion(), resourceID); - temporaryResourceCache.unconditionallyCacheResource(newResource); - } else if (temporaryResourceCache.removeResourceFromCache(newResource).isPresent()) { - // if the resource is not added to the temp cache, it is cleared, since - // the cache is cleared by subsequent events after updates, but if those did not receive - // the temp cache is still filled at this point with an old resource - log.debug("Cleaning temporary cache for resource id: {}", resourceID); - } + temporaryResourceCache.putResource(newResource, Optional.ofNullable(oldResource) + .map(r -> r.getMetadata().getResourceVersion()).orElse(null)); } private boolean useSecondaryToPrimaryIndex() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 92a317096c..a7d3e5caa7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -91,7 +91,7 @@ public void stop() { @Override public void handleRecentResourceUpdate(ResourceID resourceID, R resource, R previousVersionOfResource) { - temporaryResourceCache.putUpdatedResource(resource, + temporaryResourceCache.putResource(resource, previousVersionOfResource.getMetadata().getResourceVersion()); } @@ -128,8 +128,10 @@ void setTemporalResourceCache(TemporaryResourceCache temporaryResourceCache) this.temporaryResourceCache = temporaryResourceCache; } + @Override public abstract void addIndexers(Map>> indexers); + @Override public List byIndex(String indexName, String indexKey) { return manager().byIndex(indexName, indexKey); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java index 65a063cff0..233b409f3f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java @@ -45,36 +45,33 @@ public synchronized Optional removeResourceFromCache(T resource) { return Optional.ofNullable(cache.remove(ResourceID.fromResource(resource))); } - public synchronized void unconditionallyCacheResource(T newResource) { - putToCache(newResource, null); - } - public synchronized void putAddedResource(T newResource) { - ResourceID resourceID = ResourceID.fromResource(newResource); - if (managedInformerEventSource.get(resourceID).isEmpty()) { - log.debug("Putting resource to cache with ID: {}", resourceID); - putToCache(newResource, resourceID); - } else { - log.debug("Won't put resource into cache found already informer cache: {}", resourceID); - } + putResource(newResource, null); } - public synchronized void putUpdatedResource(T newResource, String previousResourceVersion) { + /** + * put the item into the cache if the previousResourceVersion matches the current state. If not + * the currently cached item is removed. + * + * @param previousResourceVersion null indicates an add + */ + public synchronized void putResource(T newResource, String previousResourceVersion) { var resourceId = ResourceID.fromResource(newResource); - var informerCacheResource = managedInformerEventSource.get(resourceId); - if (informerCacheResource.isEmpty()) { - log.debug("No cached value present for resource: {}", newResource); - return; - } - // if this is not true that means the cache was already updated - if (informerCacheResource.get().getMetadata().getResourceVersion() - .equals(previousResourceVersion)) { - log.debug("Putting resource to temporal cache with id: {}", resourceId); + var cachedResource = getResourceFromCache(resourceId) + .orElse(managedInformerEventSource.get(resourceId).orElse(null)); + + if ((previousResourceVersion == null && cachedResource == null) + || (cachedResource != null && previousResourceVersion != null + && cachedResource.getMetadata().getResourceVersion() + .equals(previousResourceVersion))) { + log.debug( + "Temporarily moving ahead to target version {} for resource id: {}", + newResource.getMetadata().getResourceVersion(), resourceId); putToCache(newResource, resourceId); } else { - // if something is in cache it's surely obsolete now - log.debug("Trying to remove an obsolete resource from cache for id: {}", resourceId); - cache.remove(resourceId); + if (cache.remove(resourceId) != null) { + log.debug("Removed an obsolete resource from cache for id: {}", resourceId); + } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java index f848e26cec..4d5bdf0dfd 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java @@ -30,7 +30,7 @@ void updateAddsTheResourceIntoCacheIfTheInformerHasThePreviousResourceVersion() prevTestResource.getMetadata().setResourceVersion("0"); when(informerEventSource.get(any())).thenReturn(Optional.of(prevTestResource)); - temporaryResourceCache.putUpdatedResource(testResource, "0"); + temporaryResourceCache.putResource(testResource, "0"); var cached = temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)); assertThat(cached).isPresent(); @@ -43,7 +43,7 @@ void updateNotAddsTheResourceIntoCacheIfTheInformerHasOtherVersion() { informerCachedResource.getMetadata().setResourceVersion("x"); when(informerEventSource.get(any())).thenReturn(Optional.of(informerCachedResource)); - temporaryResourceCache.putUpdatedResource(testResource, "0"); + temporaryResourceCache.putResource(testResource, "0"); var cached = temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)); assertThat(cached).isNotPresent(); From 241b3a577914ba9633b7088b0e5f038dc61a05a0 Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Fri, 11 Aug 2023 10:10:11 -0400 Subject: [PATCH 06/11] adding informer event source test and refactoring/creating a couple of methods for readability --- .../KubernetesDependentResource.java | 13 +++-- .../source/informer/InformerEventSource.java | 53 ++++++++++--------- .../informer/InformerEventSourceTest.java | 33 ++++++++++++ 3 files changed, 69 insertions(+), 30 deletions(-) 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 c8d0568915..24ec56bf26 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 @@ -116,8 +116,7 @@ public R create(R target, P primary, Context

context) { target.getMetadata().setResourceVersion("1"); } } - String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); - target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, id); + addPreviousAnnotation(null, target); final var resource = prepare(target, primary, "Creating"); return useSSA(context) ? resource @@ -133,9 +132,7 @@ public R update(R actual, R target, P primary, Context

context) { actual.getMetadata().getResourceVersion()); } R updatedResource; - String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); - target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, - id + "," + actual.getMetadata().getResourceVersion()); + addPreviousAnnotation(actual.getMetadata().getResourceVersion(), target); if (useSSA(context)) { updatedResource = prepare(target, primary, "Updating") .fieldManager(context.getControllerConfiguration().fieldManager()) @@ -149,6 +146,12 @@ public R update(R actual, R target, P primary, Context

context) { return updatedResource; } + void addPreviousAnnotation(String resourceVersion, HasMetadata target) { + String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); + target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, + id + resourceVersion != null ? ("," + resourceVersion) : ""); + } + @Override public Result match(R actualResource, P primary, Context

context) { final var desired = desired(primary, context); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index 7c5dbd83d5..dcf817f1de 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -159,22 +159,7 @@ private synchronized void onAddOrUpdate(Operation operation, R newObject, R oldO Runnable superOnOp) { var resourceID = ResourceID.fromResource(newObject); - String previous = newObject.getMetadata().getAnnotations() - .get(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); - boolean known = false; - if (previous != null) { - String[] parts = previous.split(","); - if (id.equals(parts[0])) { - if (oldObject == null && parts.length == 1) { - known = true; - } else if (oldObject != null && parts.length == 2 - && oldObject.getMetadata().getResourceVersion().equals(parts[1])) { - known = true; - } - } - } - if (temporaryCacheHasResourceWithSameVersionAs(newObject) - || (known && temporaryResourceCache.getResourceFromCache(resourceID).isEmpty())) { + if (canSkipEvent(newObject, oldObject, resourceID)) { log.debug( "Skipping event propagation for {}, since was a result of a reconcile action. Resource ID: {}", operation, @@ -194,16 +179,34 @@ private synchronized void onAddOrUpdate(Operation operation, R newObject, R oldO } } - private boolean temporaryCacheHasResourceWithSameVersionAs(R resource) { - var resourceID = ResourceID.fromResource(resource); + private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) { var res = temporaryResourceCache.getResourceFromCache(resourceID); - return res.map(r -> { - boolean resVersionsEqual = r.getMetadata().getResourceVersion() - .equals(resource.getMetadata().getResourceVersion()); - log.debug("Resource found in temporal cache for id: {} resource versions equal: {}", - resourceID, resVersionsEqual); - return resVersionsEqual; - }).orElse(false); + if (res.isEmpty()) { + return isEventKnownFromAnnotation(newObject, oldObject); + } + boolean resVersionsEqual = newObject.getMetadata().getResourceVersion() + .equals(res.get().getMetadata().getResourceVersion()); + log.debug("Resource found in temporal cache for id: {} resource versions equal: {}", + resourceID, resVersionsEqual); + return resVersionsEqual; + } + + private boolean isEventKnownFromAnnotation(R newObject, R oldObject) { + String previous = newObject.getMetadata().getAnnotations() + .get(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); + boolean known = false; + if (previous != null) { + String[] parts = previous.split(","); + if (id.equals(parts[0])) { + if (oldObject == null && parts.length == 1) { + known = true; + } else if (oldObject != null && parts.length == 2 + && oldObject.getMetadata().getResourceVersion().equals(parts[1])) { + known = true; + } + } + } + return known; } private void propagateEvent(R object) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 336de37c68..22aa4341f7 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -8,6 +8,7 @@ import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.OperatorException; @@ -15,6 +16,7 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.InformerStoppedHandler; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper; @@ -77,6 +79,37 @@ void skipsEventPropagationIfResourceWithSameVersionInResourceCache() { verify(eventHandlerMock, never()).handleEvent(any()); } + @Test + void skipsAddEventPropagationViaAnnotation() { + informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, informerEventSource.getId()).endMetadata().build()); + + verify(eventHandlerMock, never()).handleEvent(any()); + } + + @Test + void skipsUpdateEventPropagationViaAnnotation() { + informerEventSource.onUpdate(testDeployment(), new DeploymentBuilder(testDeployment()).editMetadata() + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, informerEventSource.getId() + ",1").endMetadata().build()); + + verify(eventHandlerMock, never()).handleEvent(any()); + } + + @Test + void processEventPropagationWithoutAnnotation() { + informerEventSource.onUpdate(testDeployment(), testDeployment()); + + verify(eventHandlerMock, times(1)).handleEvent(any()); + } + + @Test + void processEventPropagationWithIncorrectAnnotation() { + informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, "invalid").endMetadata().build()); + + verify(eventHandlerMock, times(1)).handleEvent(any()); + } + @Test void propagateEventAndRemoveResourceFromTempCacheIfResourceVersionMismatch() { Deployment cachedDeployment = testDeployment(); From 91f760e820b03516e01015c5ad2bc3ee65beec1c Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Fri, 11 Aug 2023 12:51:03 -0400 Subject: [PATCH 07/11] updating the integration test also testing the exclusion in the ssa matching --- .../KubernetesDependentResource.java | 5 +- ...BasedGenericKubernetesResourceMatcher.java | 6 ++- .../informer/InformerEventSourceTest.java | 14 +++-- .../deployment-with-managed-fields.yaml | 5 ++ ...CreateUpdateEventFilterTestReconciler.java | 53 ++++++++++++------- 5 files changed, 56 insertions(+), 27 deletions(-) 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 24ec56bf26..e94f0408f3 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 @@ -147,9 +147,10 @@ public R update(R actual, R target, P primary, Context

context) { } void addPreviousAnnotation(String resourceVersion, HasMetadata target) { - String id = ((InformerEventSource) eventSource().orElseThrow()).getId(); + String id = + ((InformerEventSource) eventSource().orElseThrow()).getId(); target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, - id + resourceVersion != null ? ("," + resourceVersion) : ""); + id + Optional.ofNullable(resourceVersion).map(rv -> "," + rv).orElse("")); } @Override 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 84d841e64c..579f16f93f 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 @@ -40,6 +40,7 @@ // see also: https://kubernetes.slack.com/archives/C0123CNN8F3/p1686141087220719 public class SSABasedGenericKubernetesResourceMatcher { + private static final String ANNOTATIONS_KEY = "annotations"; @SuppressWarnings("rawtypes") private static final SSABasedGenericKubernetesResourceMatcher INSTANCE = new SSABasedGenericKubernetesResourceMatcher<>(); @@ -101,9 +102,10 @@ public boolean matches(R actual, R desired, Context context) { return prunedActual.equals(desiredMap); } - private void removeSDKAnnotations(HashMap prunedActual) { + @SuppressWarnings("unchecked") + private static void removeSDKAnnotations(HashMap prunedActual) { Optional.ofNullable(((Map) prunedActual.get(METADATA_KEY))) - .ifPresent(m -> m.computeIfPresent("annotations", + .ifPresent(m -> m.computeIfPresent(ANNOTATIONS_KEY, (k, v) -> { var annotations = (Map) v; annotations.remove(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 22aa4341f7..7c8c5f4467 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -82,15 +82,20 @@ void skipsEventPropagationIfResourceWithSameVersionInResourceCache() { @Test void skipsAddEventPropagationViaAnnotation() { informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, informerEventSource.getId()).endMetadata().build()); + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, + informerEventSource.getId()) + .endMetadata().build()); verify(eventHandlerMock, never()).handleEvent(any()); } @Test void skipsUpdateEventPropagationViaAnnotation() { - informerEventSource.onUpdate(testDeployment(), new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, informerEventSource.getId() + ",1").endMetadata().build()); + informerEventSource.onUpdate(testDeployment(), + new DeploymentBuilder(testDeployment()).editMetadata() + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, + informerEventSource.getId() + ",1") + .endMetadata().build()); verify(eventHandlerMock, never()).handleEvent(any()); } @@ -105,7 +110,8 @@ void processEventPropagationWithoutAnnotation() { @Test void processEventPropagationWithIncorrectAnnotation() { informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, "invalid").endMetadata().build()); + .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, "invalid") + .endMetadata().build()); verify(eventHandlerMock, times(1)).handleEvent(any()); } diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml index 6089fc7882..9e35473445 100644 --- a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml @@ -3,12 +3,17 @@ kind: Deployment metadata: annotations: deployment.kubernetes.io/revision: "1" + javaoperatorsdk.io/previous: "abc,400" creationTimestamp: "2023-06-01T08:43:47Z" generation: 1 managedFields: - apiVersion: apps/v1 fieldsType: FieldsV1 fieldsV1: + f:metadata: + f:annotations: + .: {} + f:javaoperatorsdk.io/previous: {} f:spec: f:progressDeadlineSeconds: {} f:replicas: {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java index bc82c487f6..6c38b2dd09 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java @@ -16,7 +16,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.junit.KubernetesClientAware; -import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.source.EventSource; import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; @@ -26,10 +26,35 @@ public class CreateUpdateEventFilterTestReconciler EventSourceInitializer, KubernetesClientAware { + private static final class DirectConfigMapDependentResource + extends + CRUDKubernetesDependentResource { + + private ConfigMap desired; + + private DirectConfigMapDependentResource(Class resourceType) { + super(resourceType); + } + + @Override + protected ConfigMap desired(CreateUpdateEventFilterTestCustomResource primary, + Context context) { + return desired; + } + + @Override + public void setEventSource( + io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource eventSource) { + super.setEventSource(eventSource); + } + } + public static final String CONFIG_MAP_TEST_DATA_KEY = "key"; private KubernetesClient client; private final AtomicInteger numberOfExecutions = new AtomicInteger(0); private InformerEventSource informerEventSource; + private DirectConfigMapDependentResource configMapDR = + new DirectConfigMapDependentResource(ConfigMap.class); @Override public UpdateControl reconcile( @@ -44,28 +69,14 @@ public UpdateControl reconcile( .withName(resource.getMetadata().getName()) .get(); if (configMap == null) { - var configMapToCreate = createConfigMap(resource); - final var resourceID = ResourceID.fromResource(configMapToCreate); - configMap = - client - .configMaps() - .inNamespace(resource.getMetadata().getNamespace()) - .resource(configMapToCreate) - .create(); - informerEventSource.handleRecentResourceCreate(resourceID, configMap); + configMapDR.desired = createConfigMap(resource); + configMapDR.reconcile(resource, context); } else { - ResourceID resourceID = ResourceID.fromResource(configMap); if (!Objects.equals( configMap.getData().get(CONFIG_MAP_TEST_DATA_KEY), resource.getSpec().getValue())) { configMap.getData().put(CONFIG_MAP_TEST_DATA_KEY, resource.getSpec().getValue()); - var newConfigMap = - client - .configMaps() - .inNamespace(resource.getMetadata().getNamespace()) - .resource(configMap) - .replace(); - informerEventSource.handleRecentResourceUpdate(resourceID, - newConfigMap, configMap); + configMapDR.desired = configMap; + configMapDR.reconcile(resource, context); } } return UpdateControl.noUpdate(); @@ -94,6 +105,10 @@ public Map prepareEventSources( .build(); informerEventSource = new InformerEventSource<>(informerConfiguration, client); + + this.configMapDR.setKubernetesClient(context.getClient()); + this.configMapDR.setEventSource(informerEventSource); + return EventSourceInitializer.nameEventSources(informerEventSource); } From 8dbca568f2ee716ad2fb78b3f16e81acf2d843d8 Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Mon, 14 Aug 2023 20:40:41 -0400 Subject: [PATCH 08/11] inverted the logic to the informereventsource moving where the annotation is handled for matching --- .../KubernetesDependentResource.java | 32 +++++++++++++------ ...BasedGenericKubernetesResourceMatcher.java | 17 ---------- .../source/informer/InformerEventSource.java | 24 +++++++++----- .../informer/InformerEventSourceTest.java | 14 ++------ .../deployment-with-managed-fields.yaml | 5 --- 5 files changed, 42 insertions(+), 50 deletions(-) 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 e94f0408f3..01f2a39d96 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 @@ -1,6 +1,7 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; -import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -37,8 +38,6 @@ public abstract class KubernetesDependentResource> { - public static String PREVIOUS_ANNOTATION_KEY = "javaoperatorsdk.io/previous"; - private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); protected KubernetesClient client; @@ -146,17 +145,16 @@ public R update(R actual, R target, P primary, Context

context) { return updatedResource; } - void addPreviousAnnotation(String resourceVersion, HasMetadata target) { - String id = - ((InformerEventSource) eventSource().orElseThrow()).getId(); - target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, - id + Optional.ofNullable(resourceVersion).map(rv -> "," + rv).orElse("")); + private void addPreviousAnnotation(String resourceVersion, HasMetadata target) { + ((InformerEventSource) eventSource().orElseThrow()) + .addPreviousAnnotation(resourceVersion, target); } @Override public Result match(R actualResource, P primary, Context

context) { final var desired = desired(primary, context); final boolean matches; + copySDKAnnotations(actualResource, desired); if (useSSA(context)) { addReferenceHandlingMetadata(desired, primary); matches = SSABasedGenericKubernetesResourceMatcher.getInstance() @@ -170,6 +168,7 @@ public Result match(R actualResource, P primary, Context

context) { @SuppressWarnings("unused") public Result match(R actualResource, R desired, P primary, Context

context) { if (useSSA(context)) { + copySDKAnnotations(actualResource, desired); addReferenceHandlingMetadata(desired, primary); var matches = SSABasedGenericKubernetesResourceMatcher.getInstance() .matches(actualResource, desired, context); @@ -181,6 +180,21 @@ public Result match(R actualResource, R desired, P primary, Context

contex } } + private void copySDKAnnotations(R actualResource, final R desired) { + String actual = actualResource.getMetadata().getAnnotations() + .get(InformerEventSource.PREVIOUS_ANNOTATION_KEY); + Map annotations = desired.getMetadata().getAnnotations(); + if (actual != null) { + if (annotations == null) { + annotations = new LinkedHashMap<>(); + desired.getMetadata().setAnnotations(annotations); + } + annotations.put(InformerEventSource.PREVIOUS_ANNOTATION_KEY, actual); + } else if (annotations != null) { + annotations.remove(InformerEventSource.PREVIOUS_ANNOTATION_KEY); + } + } + private boolean useSSA(Context

context) { return context.getControllerConfiguration().getConfigurationService() .ssaBasedCreateUpdateMatchForDependentResources(); @@ -254,7 +268,7 @@ private boolean useDefaultAnnotationsToIdentifyPrimary() { private void addDefaultSecondaryToPrimaryMapperAnnotations(R desired, P primary) { var annotations = desired.getMetadata().getAnnotations(); if (annotations == null) { - annotations = new HashMap<>(); + annotations = new LinkedHashMap<>(); desired.getMetadata().setAnnotations(annotations); } annotations.put(Mappers.DEFAULT_ANNOTATION_FOR_NAME, primary.getMetadata().getName()); 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 579f16f93f..f4718b45c3 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 @@ -40,7 +40,6 @@ // see also: https://kubernetes.slack.com/archives/C0123CNN8F3/p1686141087220719 public class SSABasedGenericKubernetesResourceMatcher { - private static final String ANNOTATIONS_KEY = "annotations"; @SuppressWarnings("rawtypes") private static final SSABasedGenericKubernetesResourceMatcher INSTANCE = new SSABasedGenericKubernetesResourceMatcher<>(); @@ -91,8 +90,6 @@ public boolean matches(R actual, R desired, Context context) { keepOnlyManagedFields(prunedActual, actualMap, managedFieldsEntry.getFieldsV1().getAdditionalProperties(), objectMapper); - removeSDKAnnotations(prunedActual); - removeIrrelevantValues(desiredMap); if (LoggingUtils.isNotSensitiveResource(desired)) { @@ -102,20 +99,6 @@ public boolean matches(R actual, R desired, Context context) { return prunedActual.equals(desiredMap); } - @SuppressWarnings("unchecked") - private static void removeSDKAnnotations(HashMap prunedActual) { - Optional.ofNullable(((Map) prunedActual.get(METADATA_KEY))) - .ifPresent(m -> m.computeIfPresent(ANNOTATIONS_KEY, - (k, v) -> { - var annotations = (Map) v; - annotations.remove(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); - if (annotations.isEmpty()) { - return null; - } - return annotations; - })); - } - @SuppressWarnings("unchecked") private static void removeIrrelevantValues(Map desiredMap) { var metadata = (Map) desiredMap.get(METADATA_KEY); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index dcf817f1de..0e5c4d0668 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -19,7 +19,6 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; -import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -75,6 +74,8 @@ public class InformerEventSource extends ManagedInformerEventSource> implements ResourceEventHandler { + public static String PREVIOUS_ANNOTATION_KEY = "javaoperatorsdk.io/previous"; + private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class); private final InformerConfiguration configuration; @@ -82,7 +83,7 @@ public class InformerEventSource private final PrimaryToSecondaryIndex primaryToSecondaryIndex; private final PrimaryToSecondaryMapper

primaryToSecondaryMapper; private Map>> indexerBuffer = new HashMap<>(); - private String id = UUID.randomUUID().toString(); + private final String id = UUID.randomUUID().toString(); public InformerEventSource( InformerConfiguration configuration, EventSourceContext

context) { @@ -110,10 +111,6 @@ public InformerEventSource(InformerConfiguration configuration, KubernetesCli genericFilter = configuration.genericFilter().orElse(null); } - public String getId() { - return id; - } - @Override public void onAdd(R newResource) { if (log.isDebugEnabled()) { @@ -192,8 +189,7 @@ private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) { } private boolean isEventKnownFromAnnotation(R newObject, R oldObject) { - String previous = newObject.getMetadata().getAnnotations() - .get(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY); + String previous = newObject.getMetadata().getAnnotations().get(PREVIOUS_ANNOTATION_KEY); boolean known = false; if (previous != null) { String[] parts = previous.split(","); @@ -320,4 +316,16 @@ public void addIndexers(Map>> indexers) { indexerBuffer.putAll(indexers); } + /** + * Add an annotation to the resource so that the subsequent will be omitted + * + * @param resourceVersion null if there is no prior version + * @param target mutable resource that will be returned + */ + public R addPreviousAnnotation(String resourceVersion, R target) { + target.getMetadata().getAnnotations().put(PREVIOUS_ANNOTATION_KEY, + id + Optional.ofNullable(resourceVersion).map(rv -> "," + rv).orElse("")); + return target; + } + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 7c8c5f4467..7acecc7099 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -16,7 +16,6 @@ import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.InformerStoppedHandler; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; -import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper; @@ -38,7 +37,6 @@ class InformerEventSourceTest { private static final String PREV_RESOURCE_VERSION = "0"; private static final String DEFAULT_RESOURCE_VERSION = "1"; - private static final String NEXT_RESOURCE_VERSION = "2"; private InformerEventSource informerEventSource; private final KubernetesClient clientMock = MockKubernetesClient.client(Deployment.class); @@ -81,10 +79,7 @@ void skipsEventPropagationIfResourceWithSameVersionInResourceCache() { @Test void skipsAddEventPropagationViaAnnotation() { - informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, - informerEventSource.getId()) - .endMetadata().build()); + informerEventSource.onAdd(informerEventSource.addPreviousAnnotation(null, testDeployment())); verify(eventHandlerMock, never()).handleEvent(any()); } @@ -92,10 +87,7 @@ void skipsAddEventPropagationViaAnnotation() { @Test void skipsUpdateEventPropagationViaAnnotation() { informerEventSource.onUpdate(testDeployment(), - new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, - informerEventSource.getId() + ",1") - .endMetadata().build()); + informerEventSource.addPreviousAnnotation("1", testDeployment())); verify(eventHandlerMock, never()).handleEvent(any()); } @@ -110,7 +102,7 @@ void processEventPropagationWithoutAnnotation() { @Test void processEventPropagationWithIncorrectAnnotation() { informerEventSource.onAdd(new DeploymentBuilder(testDeployment()).editMetadata() - .addToAnnotations(KubernetesDependentResource.PREVIOUS_ANNOTATION_KEY, "invalid") + .addToAnnotations(InformerEventSource.PREVIOUS_ANNOTATION_KEY, "invalid") .endMetadata().build()); verify(eventHandlerMock, times(1)).handleEvent(any()); diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml index 9e35473445..6089fc7882 100644 --- a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml @@ -3,17 +3,12 @@ kind: Deployment metadata: annotations: deployment.kubernetes.io/revision: "1" - javaoperatorsdk.io/previous: "abc,400" creationTimestamp: "2023-06-01T08:43:47Z" generation: 1 managedFields: - apiVersion: apps/v1 fieldsType: FieldsV1 fieldsV1: - f:metadata: - f:annotations: - .: {} - f:javaoperatorsdk.io/previous: {} f:spec: f:progressDeadlineSeconds: {} f:replicas: {} From 3f0647cf5b643d8700ac505594c1f7bf7bcf8160 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 15 Aug 2023 08:48:24 +0200 Subject: [PATCH 09/11] refactor: make field final --- .../event/source/informer/InformerEventSource.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java index 0e5c4d0668..1fb7d61b4e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java @@ -1,11 +1,6 @@ package io.javaoperatorsdk.operator.processing.event.source.informer; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.Set; -import java.util.UUID; +import java.util.*; import java.util.function.Function; import java.util.stream.Collectors; From 217936617f8ae540c205d522a3461a40a39611ab Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 15 Aug 2023 08:57:56 +0200 Subject: [PATCH 10/11] refactor: use proper type and simplify code --- .../api/reconciler/dependent/DependentResource.java | 2 +- .../AbstractEventSourceHolderDependentResource.java | 7 +++---- .../dependent/kubernetes/KubernetesDependentResource.java | 7 +++---- .../DependentPrimaryIndexerTestReconciler.java | 4 ++-- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java index affeb96bbf..8230dc4cf9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java @@ -44,7 +44,7 @@ public interface DependentResource { * @param eventSourceContext context of event source initialization * @return an optional event source */ - default Optional> eventSource( + default Optional> eventSource( EventSourceContext

eventSourceContext) { return Optional.empty(); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java index 972b61cd94..04aa5631cf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java @@ -34,7 +34,7 @@ protected AbstractEventSourceHolderDependentResource(Class resourceType) { this.resourceType = resourceType; } - public Optional> eventSource(EventSourceContext

context) { + public Optional eventSource(EventSourceContext

context) { // some sub-classes (e.g. KubernetesDependentResource) can have their event source created // before this method is called in the managed case, so only create the event source if it // hasn't already been set. @@ -67,9 +67,8 @@ public void resolveEventSource(EventSourceRetriever

eventSourceRetriever) { * @param context for event sources * @return event source instance */ - @SuppressWarnings("unchecked") public T initEventSource(EventSourceContext

context) { - return (T) eventSource(context).orElseThrow(); + return eventSource(context).orElseThrow(); } @Override @@ -96,7 +95,7 @@ protected void applyFilters() { this.eventSource.setGenericFilter(genericFilter); } - public Optional> eventSource() { + public Optional eventSource() { return Optional.ofNullable(eventSource); } 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 01f2a39d96..2a1103c51f 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 @@ -145,9 +145,8 @@ public R update(R actual, R target, P primary, Context

context) { return updatedResource; } - private void addPreviousAnnotation(String resourceVersion, HasMetadata target) { - ((InformerEventSource) eventSource().orElseThrow()) - .addPreviousAnnotation(resourceVersion, target); + private void addPreviousAnnotation(String resourceVersion, R target) { + eventSource().orElseThrow().addPreviousAnnotation(resourceVersion, target); } @Override @@ -258,7 +257,7 @@ protected InformerEventSource createEventSource(EventSourceContext

cont "Using default configuration for {} KubernetesDependentResource, call configureWith to provide configuration", resourceType().getSimpleName()); } - return (InformerEventSource) eventSource().orElseThrow(); + return eventSource().orElseThrow(); } private boolean useDefaultAnnotationsToIdentifyPrimary() { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/primaryindexer/DependentPrimaryIndexerTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/primaryindexer/DependentPrimaryIndexerTestReconciler.java index 6d79d4ee56..e123500c42 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/primaryindexer/DependentPrimaryIndexerTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/primaryindexer/DependentPrimaryIndexerTestReconciler.java @@ -12,8 +12,8 @@ import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.IndexerResourceCache; -import io.javaoperatorsdk.operator.processing.event.source.ResourceEventSource; import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper; +import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; @ControllerConfiguration(dependents = @Dependent( type = DependentPrimaryIndexerTestReconciler.ReadOnlyConfigMapDependent.class)) @@ -39,7 +39,7 @@ public Set toPrimaryResourceIDs(ConfigMap dependentResource) { } @Override - public Optional> eventSource( + public Optional> eventSource( EventSourceContext context) { cache = context.getPrimaryCache(); cache.addIndexer(CONFIG_MAP_RELATION_INDEXER, indexer); From 3396bd38c2743c5807b0041440877d66652ad142 Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Tue, 15 Aug 2023 09:19:18 -0400 Subject: [PATCH 11/11] consolidating how metadata is added to the desired also removing guards that the annotations can be null --- .../KubernetesDependentResource.java | 83 ++++++++----------- 1 file changed, 34 insertions(+), 49 deletions(-) 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 2a1103c51f..5fedd0899d 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 @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; -import java.util.LinkedHashMap; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -9,7 +8,6 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; -import io.fabric8.kubernetes.api.model.Namespaced; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.dsl.Resource; import io.javaoperatorsdk.operator.OperatorException; @@ -115,7 +113,7 @@ public R create(R target, P primary, Context

context) { target.getMetadata().setResourceVersion("1"); } } - addPreviousAnnotation(null, target); + addMetadata(false, null, target, primary); final var resource = prepare(target, primary, "Creating"); return useSSA(context) ? resource @@ -131,67 +129,64 @@ public R update(R actual, R target, P primary, Context

context) { actual.getMetadata().getResourceVersion()); } R updatedResource; - addPreviousAnnotation(actual.getMetadata().getResourceVersion(), target); + addMetadata(false, actual, target, primary); if (useSSA(context)) { updatedResource = prepare(target, primary, "Updating") .fieldManager(context.getControllerConfiguration().fieldManager()) .forceConflicts().serverSideApply(); } else { var updatedActual = updaterMatcher.updateResource(actual, target, context); - updatedResource = prepare(updatedActual, primary, "Updating").replace(); + updatedResource = prepare(updatedActual, primary, "Updating").update(); } log.debug("Resource version after update: {}", updatedResource.getMetadata().getResourceVersion()); return updatedResource; } - private void addPreviousAnnotation(String resourceVersion, R target) { - eventSource().orElseThrow().addPreviousAnnotation(resourceVersion, target); - } - @Override public Result match(R actualResource, P primary, Context

context) { final var desired = desired(primary, context); - final boolean matches; - copySDKAnnotations(actualResource, desired); - if (useSSA(context)) { - addReferenceHandlingMetadata(desired, primary); - matches = SSABasedGenericKubernetesResourceMatcher.getInstance() - .matches(actualResource, desired, context); - } else { - matches = updaterMatcher.matches(actualResource, desired, context); - } - return Result.computed(matches, desired); + return match(actualResource, desired, primary, updaterMatcher, context); } - @SuppressWarnings("unused") + @SuppressWarnings({"unused", "unchecked"}) public Result match(R actualResource, R desired, P primary, Context

context) { + return match(actualResource, desired, primary, + (ResourceUpdaterMatcher) GenericResourceUpdaterMatcher + .updaterMatcherFor(actualResource.getClass()), + context); + } + + public Result match(R actualResource, R desired, P primary, ResourceUpdaterMatcher matcher, + Context

context) { + final boolean matches; + addMetadata(true, actualResource, desired, primary); if (useSSA(context)) { - copySDKAnnotations(actualResource, desired); - addReferenceHandlingMetadata(desired, primary); - var matches = SSABasedGenericKubernetesResourceMatcher.getInstance() + matches = SSABasedGenericKubernetesResourceMatcher.getInstance() .matches(actualResource, desired, context); - return Result.computed(matches, desired); } else { - return GenericKubernetesResourceMatcher - .match(desired, actualResource, true, - false, false, context); + matches = matcher.matches(actualResource, desired, context); } + return Result.computed(matches, desired); } - private void copySDKAnnotations(R actualResource, final R desired) { - String actual = actualResource.getMetadata().getAnnotations() - .get(InformerEventSource.PREVIOUS_ANNOTATION_KEY); - Map annotations = desired.getMetadata().getAnnotations(); - if (actual != null) { - if (annotations == null) { - annotations = new LinkedHashMap<>(); - desired.getMetadata().setAnnotations(annotations); + protected void addMetadata(boolean forMatch, R actualResource, final R target, P primary) { + if (forMatch) { // keep the current + String actual = actualResource.getMetadata().getAnnotations() + .get(InformerEventSource.PREVIOUS_ANNOTATION_KEY); + Map annotations = target.getMetadata().getAnnotations(); + if (actual != null) { + annotations.put(InformerEventSource.PREVIOUS_ANNOTATION_KEY, actual); + } else { + annotations.remove(InformerEventSource.PREVIOUS_ANNOTATION_KEY); } - annotations.put(InformerEventSource.PREVIOUS_ANNOTATION_KEY, actual); - } else if (annotations != null) { - annotations.remove(InformerEventSource.PREVIOUS_ANNOTATION_KEY); + } else { // set a new one + eventSource().orElseThrow().addPreviousAnnotation( + Optional.ofNullable(actualResource).map(r -> r.getMetadata().getResourceVersion()) + .orElse(null), + target); } + addReferenceHandlingMetadata(target, primary); } private boolean useSSA(Context

context) { @@ -217,13 +212,7 @@ protected Resource prepare(R desired, P primary, String actionName) { desired.getClass(), ResourceID.fromResource(desired)); - addReferenceHandlingMetadata(desired, primary); - - if (desired instanceof Namespaced) { - return client.resource(desired).inNamespace(desired.getMetadata().getNamespace()); - } else { - return client.resource(desired); - } + return client.resource(desired); } protected void addReferenceHandlingMetadata(R desired, P primary) { @@ -266,10 +255,6 @@ private boolean useDefaultAnnotationsToIdentifyPrimary() { private void addDefaultSecondaryToPrimaryMapperAnnotations(R desired, P primary) { var annotations = desired.getMetadata().getAnnotations(); - if (annotations == null) { - annotations = new LinkedHashMap<>(); - desired.getMetadata().setAnnotations(annotations); - } annotations.put(Mappers.DEFAULT_ANNOTATION_FOR_NAME, primary.getMetadata().getName()); var primaryNamespaces = primary.getMetadata().getNamespace(); if (primaryNamespaces != null) {