From 040c2a4880bf8c6f9f105f9e12f7a237b83b93a1 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 2 Mar 2022 16:48:30 +0100 Subject: [PATCH 1/2] feat: matcher avoiding creating the desired state when possible Fixes #986 --- .../dependent/AbstractDependentResource.java | 14 +++-- .../api/reconciler/dependent/Matcher.java | 33 ++++++++++- .../api/reconciler/dependent/Updater.java | 16 ++++-- .../GenericKubernetesResourceMatcher.java | 41 ++++++++----- .../KubernetesDependentResource.java | 29 ++++++++-- .../GenericKubernetesResourceMatcherTest.java | 57 +++++++++++++------ .../dependent/SecretDependentResource.java | 18 +++--- 7 files changed, 152 insertions(+), 56 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java index 456e9339de..9e1dfbc261 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java @@ -36,19 +36,23 @@ public void reconcile(P primary, Context context) { final var updatable = isUpdatable(primary, context); if (creatable || updatable) { var maybeActual = getResource(primary); - var desired = desired(primary, context); if (maybeActual.isEmpty()) { if (creatable) { + var desired = desired(primary, context); log.debug("Creating dependent {} for primary {}", desired, primary); creator.create(desired, primary, context); } } else { final var actual = maybeActual.get(); - if (updatable && !updater.match(actual, desired, context)) { - log.debug("Updating dependent {} for primary {}", desired, primary); - updater.update(actual, desired, primary, context); + if (updatable) { + final var match = updater.match(actual, primary, context); + if (match.matched()) { + final var desired = match.computedDesired().orElse(desired(primary, context)); + log.debug("Updating dependent {} for primary {}", desired, primary); + updater.update(actual, desired, primary, context); + } } else { - log.debug("Update skipped for dependent {} as it matched the existing one", desired); + log.debug("Update skipped for dependent {} as it matched the existing one", actual); } } } else { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Matcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Matcher.java index ad82daf661..f0c5bd7fd9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Matcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Matcher.java @@ -1,7 +1,36 @@ package io.javaoperatorsdk.operator.api.reconciler.dependent; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; -public interface Matcher { - boolean match(R actualResource, R desiredResource, Context context); +public interface Matcher { + interface Result { + boolean matched(); + + default Optional computedDesired() { + return Optional.empty(); + } + + static Result nonComputed(boolean matched) { + return () -> matched; + } + + static Result computed(boolean matched, T computedDesired) { + return new Result<>() { + @Override + public boolean matched() { + return matched; + } + + @Override + public Optional computedDesired() { + return Optional.of(computedDesired); + } + }; + } + } + + Result match(R actualResource, P primary, Context context); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java index 6984f788f8..62199e1ede 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java @@ -1,18 +1,22 @@ package io.javaoperatorsdk.operator.api.reconciler.dependent; -import java.util.Objects; - import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher.Result; @SuppressWarnings("rawtypes") public interface Updater { - Updater NOOP = (actual, desired, primary, context) -> { + Updater NOOP = new Updater() { + @Override + public void update(Object actual, Object desired, HasMetadata primary, Context context) {} + + @Override + public Result match(Object actualResource, HasMetadata primary, Context context) { + return Result.nonComputed(true); + } }; void update(R actual, R desired, P primary, Context context); - default boolean match(R actualResource, R desiredResource, Context context) { - return Objects.equals(actualResource, desiredResource); - } + Result match(R actualResource, P primary, Context context); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java index 1c3c7d0822..25d7c098d5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java @@ -8,37 +8,52 @@ import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher; -public class GenericKubernetesResourceMatcher implements Matcher { +public class GenericKubernetesResourceMatcher + implements Matcher { - private GenericKubernetesResourceMatcher() {} + private final KubernetesDependentResource dependentResource; - @SuppressWarnings({"rawtypes", "unchecked"}) - public static Matcher matcherFor(Class resourceType) { + private GenericKubernetesResourceMatcher(KubernetesDependentResource dependentResource) { + this.dependentResource = dependentResource; + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + static Matcher matcherFor( + Class resourceType, KubernetesDependentResource dependentResource) { if (Secret.class.isAssignableFrom(resourceType)) { - return (actual, desired, context) -> ResourceComparators.compareSecretData((Secret) desired, - (Secret) actual); + return (actual, primary, context) -> { + final var desired = dependentResource.desired(primary, context); + return Result.computed( + ResourceComparators.compareSecretData((Secret) desired, (Secret) actual), desired); + }; } else if (ConfigMap.class.isAssignableFrom(resourceType)) { - return (actual, desired, context) -> ResourceComparators - .compareConfigMapData((ConfigMap) desired, (ConfigMap) actual); + return (actual, primary, context) -> { + final var desired = dependentResource.desired(primary, context); + return Result.computed( + ResourceComparators.compareConfigMapData((ConfigMap) desired, (ConfigMap) actual), + desired); + }; } else { - return new GenericKubernetesResourceMatcher(); + return new GenericKubernetesResourceMatcher(dependentResource); } } @Override - public boolean match(R actualResource, R desiredResource, Context context) { + public Result match(R actualResource, P primary, Context context) { final var objectMapper = context.getConfigurationService().getObjectMapper(); + final var desired = dependentResource.desired(primary, context); + // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 - var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desiredResource)); + var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); for (int i = 0; i < diffJsonPatch.size(); i++) { String operation = diffJsonPatch.get(i).get("op").asText(); if (!operation.equals("add")) { - return false; + return Result.computed(false, desired); } } - return true; + return Result.computed(true, desired); } } 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 8ce3560fdc..8bd534ab9a 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 @@ -21,7 +21,9 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider; import io.javaoperatorsdk.operator.api.reconciler.dependent.KubernetesClientAware; import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher.Result; import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceUpdatePreProcessor; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Updater; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier; import io.javaoperatorsdk.operator.processing.event.source.EventSource; @@ -39,14 +41,24 @@ public abstract class KubernetesDependentResource informerEventSource; private boolean addOwnerReference; - private final Matcher matcher; + private final Matcher matcher; private final ResourceUpdatePreProcessor processor; @SuppressWarnings("unchecked") public KubernetesDependentResource() { - init(this::create, this::update, this::delete); - matcher = this instanceof Matcher ? (Matcher) this - : GenericKubernetesResourceMatcher.matcherFor(resourceType()); + init(this::create, new Updater<>() { + @Override + public void update(R actual, R desired, P primary, Context context) { + KubernetesDependentResource.this.update(actual, desired, primary, context); + } + + @Override + public Result match(R actualResource, P primary, Context context) { + return KubernetesDependentResource.this.match(actualResource, primary, context); + } + }, this::delete); + matcher = this instanceof Matcher ? (Matcher) this + : GenericKubernetesResourceMatcher.matcherFor(resourceType(), this); processor = this instanceof ResourceUpdatePreProcessor ? (ResourceUpdatePreProcessor) this : GenericResourceUpdatePreProcessor.processorFor(resourceType()); @@ -117,8 +129,8 @@ public void update(R actual, R target, P primary, Context context) { } } - public boolean match(R actualResource, R desiredResource, Context context) { - return matcher.match(actualResource, desiredResource, context); + public Result match(R actualResource, P primary, Context context) { + return matcher.match(actualResource, primary, context); } public void delete(P primary, Context context) { @@ -173,4 +185,9 @@ public Optional getResource(P primaryResource) { public void setKubernetesClient(KubernetesClient kubernetesClient) { this.client = kubernetesClient; } + + @Override + protected R desired(P primary, Context context) { + return super.desired(primary, context); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java index 6302c20e6d..3ae424cdfb 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java @@ -1,8 +1,12 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.Optional; + import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -24,29 +28,40 @@ class GenericKubernetesResourceMatcherTest { @Test void checksIfDesiredValuesAreTheSame() { - var target1 = createDeployment(); - var desired1 = createDeployment(); - final var matcher = GenericKubernetesResourceMatcher.matcherFor(Deployment.class); - assertThat(matcher.match(target1, desired1, context)).isTrue(); - - var target2 = createDeployment(); - var desired2 = createDeployment(); - target2.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - assertThat(matcher.match(target2, desired2, context)) + var actual = createDeployment(); + final var desired = createDeployment(); + final var matcher = GenericKubernetesResourceMatcher.matcherFor(Deployment.class, + new KubernetesDependentResource<>() { + @Override + protected Deployment desired(HasMetadata primary, Context context) { + final var currentCase = Optional.ofNullable(primary) + .map(p -> p.getMetadata().getLabels().get("case")) + .orElse(null); + var d = desired; + if ("removed".equals(currentCase)) { + d = createDeployment(); + d.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); + } + return d; + } + }); + assertThat(matcher.match(actual, null, context).matched()).isTrue(); + assertThat(matcher.match(actual, null, context).computedDesired().isPresent()).isTrue(); + assertThat(matcher.match(actual, null, context).computedDesired().get()).isEqualTo(desired); + + actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); + assertThat(matcher.match(actual, null, context).matched()) .withFailMessage("Additive changes should be ok") .isTrue(); - var target3 = createDeployment(); - var desired3 = createDeployment(); - desired3.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - assertThat(matcher.match(target3, desired3, context)) + actual = createDeployment(); + assertThat(matcher.match(actual, createPrimary("removed"), context).matched()) .withFailMessage("Removed value should not be ok") .isFalse(); - var target4 = createDeployment(); - var desired4 = createDeployment(); - target4.getSpec().setReplicas(2); - assertThat(matcher.match(target4, desired4, context)) + actual = createDeployment(); + actual.getSpec().setReplicas(2); + assertThat(matcher.match(actual, null, context).matched()) .withFailMessage("Changed values are not ok") .isFalse(); } @@ -55,4 +70,12 @@ Deployment createDeployment() { return ReconcilerUtils.loadYaml( Deployment.class, GenericKubernetesResourceMatcherTest.class, "nginx-deployment.yaml"); } + + HasMetadata createPrimary(String caseName) { + return new DeploymentBuilder() + .editOrNewMetadata() + .addToLabels("case", caseName) + .endMetadata() + .build(); + } } diff --git a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SecretDependentResource.java b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SecretDependentResource.java index b118b07f0c..6c71252bb5 100644 --- a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SecretDependentResource.java +++ b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SecretDependentResource.java @@ -8,6 +8,7 @@ import io.fabric8.kubernetes.api.model.SecretBuilder; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.Creator; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher.Result; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier; @@ -32,7 +33,7 @@ protected Secret desired(MySQLSchema schema, Context context) { final var password = RandomStringUtils .randomAlphanumeric(16); // NOSONAR: we don't need cryptographically-strong randomness here final var name = schema.getMetadata().getName(); - final var secretName = String.format(SECRET_FORMAT, name); + final var secretName = getSecretName(name); final var userName = String.format(USERNAME_FORMAT, name); return new SecretBuilder() @@ -40,16 +41,19 @@ protected Secret desired(MySQLSchema schema, Context context) { .withName(secretName) .withNamespace(schema.getMetadata().getNamespace()) .endMetadata() - .addToData( - MYSQL_SECRET_USERNAME, encode(userName)) - .addToData( - MYSQL_SECRET_PASSWORD, encode(password)) + .addToData(MYSQL_SECRET_USERNAME, encode(userName)) + .addToData(MYSQL_SECRET_PASSWORD, encode(password)) .build(); } + private String getSecretName(String name) { + return String.format(SECRET_FORMAT, name); + } + @Override - public boolean match(Secret actual, Secret target, Context context) { - return ResourceID.fromResource(actual).equals(ResourceID.fromResource(target)); + public Result match(Secret actual, MySQLSchema primary, Context context) { + final var desiredSecretName = getSecretName(primary.getMetadata().getName()); + return Result.nonComputed(actual.getMetadata().getName().equals(desiredSecretName)); } @Override From dedc81491d17b94335aeb96eb950d6a86fb61f74 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 2 Mar 2022 17:41:02 +0100 Subject: [PATCH 2/2] fix: updater should be called when resources don't match :facepalm: --- .../api/reconciler/dependent/AbstractDependentResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java index 9e1dfbc261..8700af0805 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java @@ -46,7 +46,7 @@ public void reconcile(P primary, Context context) { final var actual = maybeActual.get(); if (updatable) { final var match = updater.match(actual, primary, context); - if (match.matched()) { + if (!match.matched()) { final var desired = match.computedDesired().orElse(desired(primary, context)); log.debug("Updating dependent {} for primary {}", desired, primary); updater.update(actual, desired, primary, context);