From 312a094df9e7ed00d91dd0ab6a1a9e6f894f76f4 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 5 Oct 2022 15:06:10 +0200 Subject: [PATCH 1/5] feat: key based bulk resource creation --- .../reconciler/dependent/ReconcileResult.java | 9 +- .../dependent/AbstractDependentResource.java | 90 ++++++++++--------- .../dependent/BulkDependentResource.java | 20 +++-- .../KubernetesDependentResource.java | 15 ++-- ...ConfigMapDeleterBulkDependentResource.java | 59 +++++++----- .../ExternalBulkDependentResource.java | 88 +++++++++--------- 6 files changed, 150 insertions(+), 131 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/ReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/ReconcileResult.java index 468e14e8ea..66d982f01d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/ReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/ReconcileResult.java @@ -22,15 +22,14 @@ public static ReconcileResult noOperation(T resource) { return new ReconcileResult<>(resource, Operation.NONE); } - @SafeVarargs - public static ReconcileResult aggregatedResult(ReconcileResult... results) { + public static ReconcileResult aggregatedResult(List> results) { if (results == null) { throw new IllegalArgumentException("Should provide results to aggregate"); } - if (results.length == 1) { - return results[0]; + if (results.size() == 1) { + return results.get(0); } - final Map operations = new HashMap<>(results.length); + final Map operations = new HashMap<>(results.size()); for (ReconcileResult res : results) { res.getSingleResource().ifPresent(r -> operations.put(r, res.getSingleOperation())); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 201f08add6..b3906af9dc 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -1,6 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent; -import java.util.Optional; +import java.util.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -24,78 +24,84 @@ public abstract class AbstractDependentResource protected Creator creator; protected Updater updater; - protected BulkDependentResource bulkDependentResource; + @SuppressWarnings("rawtypes") + protected BulkDependentResource bulkDependentResource; private ResourceDiscriminator resourceDiscriminator; - private int currentCount; - @SuppressWarnings("unchecked") - public AbstractDependentResource() { + @SuppressWarnings({"unchecked", "rawtypes"}) + protected AbstractDependentResource() { creator = creatable ? (Creator) this : null; updater = updatable ? (Updater) this : null; - bulkDependentResource = bulk ? (BulkDependentResource) this : null; + bulkDependentResource = bulk ? (BulkDependentResource) this : null; } + @SuppressWarnings("unchecked") @Override public ReconcileResult reconcile(P primary, Context

context) { if (bulk) { - final var count = bulkDependentResource.count(primary, context); - deleteBulkResourcesIfRequired(count, primary, context); - @SuppressWarnings("unchecked") - final ReconcileResult[] results = new ReconcileResult[count]; - for (int i = 0; i < count; i++) { - results[i] = reconcileIndexAware(primary, i, context); + final var targetKeys = bulkDependentResource.targetKeys(primary, context); + Map actualResources = + bulkDependentResource.getSecondaryResources(primary, context); + + deleteBulkResourcesIfRequired(targetKeys, actualResources, primary, context); + final List> results = new ArrayList<>(targetKeys.size()); + + for (Object key : targetKeys) { + results.add(reconcileIndexAware(primary, actualResources.get(key), key, context)); } - currentCount = count; return ReconcileResult.aggregatedResult(results); } else { - return reconcileIndexAware(primary, 0, context); + var actualResource = getSecondaryResource(primary, context); + return reconcileIndexAware(primary, actualResource.orElse(null), null, context); } } - protected void deleteBulkResourcesIfRequired(int targetCount, P primary, Context

context) { - if (targetCount >= currentCount) { - return; - } - for (int i = targetCount; i < currentCount; i++) { - var resource = bulkDependentResource.getSecondaryResource(primary, i, context); - var index = i; - resource.ifPresent( - r -> bulkDependentResource.deleteBulkResourceWithIndex(primary, r, index, context)); - } + @SuppressWarnings("unchecked") + protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actualResources, + P primary, Context

context) { + actualResources.entrySet().forEach(entry -> { + if (!targetKeys.contains(entry.getKey())) { + bulkDependentResource.deleteBulkResource(primary, entry.getValue(), context); + } + }); + } + + protected void deleteBulkResourcesIfRequired(P primary, Context

context) { + var actualResources = bulkDependentResource.getSecondaryResources(primary, context); + deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context); } - protected ReconcileResult reconcileIndexAware(P primary, int i, Context

context) { - Optional maybeActual = bulk ? bulkDependentResource.getSecondaryResource(primary, i, context) - : getSecondaryResource(primary, context); + @SuppressWarnings("unchecked") + protected ReconcileResult reconcileIndexAware(P primary, R resource, Object key, + Context

context) { if (creatable || updatable) { - if (maybeActual.isEmpty()) { + if (resource == null) { if (creatable) { - var desired = desiredIndexAware(primary, i, context); + var desired = desiredIndexAware(primary, key, context); throwIfNull(desired, primary, "Desired"); logForOperation("Creating", primary, desired); var createdResource = handleCreate(desired, primary, context); return ReconcileResult.resourceCreated(createdResource); } } else { - final var actual = maybeActual.get(); if (updatable) { final Matcher.Result match; if (bulk) { - match = bulkDependentResource.match(actual, primary, i, context); + match = bulkDependentResource.match(resource, primary, key, context); } else { - match = updater.match(actual, primary, context); + match = updater.match(resource, primary, context); } if (!match.matched()) { final var desired = - match.computedDesired().orElse(desiredIndexAware(primary, i, context)); + match.computedDesired().orElse(desiredIndexAware(primary, key, context)); throwIfNull(desired, primary, "Desired"); logForOperation("Updating", primary, desired); - var updatedResource = handleUpdate(actual, desired, primary, context); + var updatedResource = handleUpdate(resource, desired, primary, context); return ReconcileResult.resourceUpdated(updatedResource); } } else { - log.debug("Update skipped for dependent {} as it matched the existing one", actual); + log.debug("Update skipped for dependent {} as it matched the existing one", resource); } } } else { @@ -103,11 +109,12 @@ protected ReconcileResult reconcileIndexAware(P primary, int i, Context

co "Dependent {} is read-only, implement Creator and/or Updater interfaces to modify it", getClass().getSimpleName()); } - return ReconcileResult.noOperation(maybeActual.orElse(null)); + return ReconcileResult.noOperation(resource); } - private R desiredIndexAware(P primary, int i, Context

context) { - return bulk ? desired(primary, i, context) : desired(primary, context); + private R desiredIndexAware(P primary, Object key, Context

context) { + return bulk ? (R) bulkDependentResource.desired(primary, key, context) + : desired(primary, context); } public Optional getSecondaryResource(P primary, Context

context) { @@ -172,13 +179,12 @@ protected R desired(P primary, Context

context) { "desired method must be implemented if this DependentResource can be created and/or updated"); } - protected R desired(P primary, int index, Context

context) { - throw new IllegalStateException("Must be implemented for bulk DependentResource creation"); - } + @SuppressWarnings("unchecked") public void delete(P primary, Context

context) { if (bulk) { - deleteBulkResourcesIfRequired(0, primary, context); + var actualResources = bulkDependentResource.getSecondaryResources(primary, context); + deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context); } else { handleDelete(primary, context); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java index 64a174e201..4894aa822d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java @@ -1,6 +1,7 @@ package io.javaoperatorsdk.operator.processing.dependent; -import java.util.Optional; +import java.util.Map; +import java.util.Set; import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -13,24 +14,27 @@ * {@link Creator} and {@link Deleter} interfaces out of the box. A concrete dependent resource can * implement additionally also {@link Updater}. */ -public interface BulkDependentResource extends Creator, Deleter

{ +public interface BulkDependentResource + extends Creator, Deleter

{ /** * @return number of resources to create */ - int count(P primary, Context

context); + Set targetKeys(P primary, Context

context); - R desired(P primary, int index, Context

context); + Map getSecondaryResources(P primary, Context

context); + R desired(P primary, T key, Context

context); + + // todo add back key? /** * Used to delete resource if the desired count is lower than the actual count of a resource. * * @param primary resource * @param resource actual resource from the cache for the index - * @param i index of the resource * @param context actual context */ - void deleteBulkResourceWithIndex(P primary, R resource, int i, Context

context); + void deleteBulkResource(P primary, R resource, Context

context); /** * Determines whether the specified secondary resource matches the desired state with target index @@ -45,8 +49,6 @@ public interface BulkDependentResource extends Creator * convenience methods ({@link Result#nonComputed(boolean)} and * {@link Result#computed(boolean, Object)}) */ - Result match(R actualResource, P primary, int index, Context

context); - - Optional getSecondaryResource(P primary, int index, Context

context); + Result match(R actualResource, P primary, T index, Context

context); } 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 1aaed6f12e..475610e87d 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 @@ -139,9 +139,9 @@ public Result match(R actualResource, P primary, Context

context) { return matcher.match(actualResource, primary, context); } - public Result match(R actualResource, P primary, int index, Context

context) { - final var desired = desired(primary, index, context); - return GenericKubernetesResourceMatcher.match(desired, actualResource, false); + public Result match(R actualResource, P primary, Object key, Context

context) { + final var desired = bulkDependentResource.desired(primary, key, context); + return GenericKubernetesResourceMatcher.match((R) desired, actualResource, false); } protected void handleDelete(P primary, Context

context) { @@ -149,8 +149,10 @@ protected void handleDelete(P primary, Context

context) { resource.ifPresent(r -> client.resource(r).delete()); } - public void deleteBulkResourceWithIndex(P primary, R resource, int i, Context

context) { + + public void deleteBulkResource(P primary, R resource, Context

context) { client.resource(resource).delete(); + } protected Resource prepare(R desired, P primary, String actionName) { @@ -229,11 +231,6 @@ protected R desired(P primary, Context

context) { return super.desired(primary, context); } - @Override - protected R desired(P primary, int index, Context

context) { - return super.desired(primary, index, context); - } - private void prepareEventFiltering(R desired, ResourceID resourceID) { ((InformerEventSource) eventSource().orElseThrow()) .prepareForCreateOrUpdateEventFiltering(resourceID, desired); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java index d8f85bb10a..8ef68f1aa3 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java @@ -1,8 +1,6 @@ package io.javaoperatorsdk.operator.sample.bulkdependent; -import java.util.Map; -import java.util.Optional; -import java.util.stream.Collectors; +import java.util.*; import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; @@ -10,6 +8,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.processing.dependent.BulkDependentResource; import io.javaoperatorsdk.operator.processing.dependent.Creator; +import io.javaoperatorsdk.operator.processing.dependent.Matcher; import io.javaoperatorsdk.operator.processing.dependent.Updater; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; @@ -22,48 +21,62 @@ public class ConfigMapDeleterBulkDependentResource implements Creator, Updater, Deleter, - BulkDependentResource { + BulkDependentResource { public static final String LABEL_KEY = "bulk"; public static final String LABEL_VALUE = "true"; public static final String ADDITIONAL_DATA_KEY = "additionalData"; + public static final String INDEX_DELIMITER = "-"; public ConfigMapDeleterBulkDependentResource() { super(ConfigMap.class); } @Override - public ConfigMap desired(BulkDependentTestCustomResource primary, - int index, Context context) { + public Set targetKeys(BulkDependentTestCustomResource primary, + Context context) { + var number = primary.getSpec().getNumberOfResources(); + Set res = new HashSet<>(); + for (int i = 0; i < number; i++) { + res.add(i); + } + return res; + } + + @Override + public ConfigMap desired(BulkDependentTestCustomResource primary, Integer key, + Context context) { ConfigMap configMap = new ConfigMap(); configMap.setMetadata(new ObjectMetaBuilder() - .withName(primary.getMetadata().getName() + "-" + index) + .withName(primary.getMetadata().getName() + INDEX_DELIMITER + key) .withNamespace(primary.getMetadata().getNamespace()) .withLabels(Map.of(LABEL_KEY, LABEL_VALUE)) .build()); configMap.setData( - Map.of("number", "" + index, ADDITIONAL_DATA_KEY, primary.getSpec().getAdditionalData())); + Map.of("number", "" + key, ADDITIONAL_DATA_KEY, primary.getSpec().getAdditionalData())); return configMap; } + // todo fix generics? @Override - public int count(BulkDependentTestCustomResource primary, - Context context) { - return primary.getSpec().getNumberOfResources(); + public Matcher.Result match(ConfigMap actualResource, + BulkDependentTestCustomResource primary, + Integer index, Context context) { + return super.match(actualResource, primary, index, context); } @Override - public Optional getSecondaryResource(BulkDependentTestCustomResource primary, - int index, Context context) { - var resources = context.getSecondaryResources(resourceType()).stream() - .filter(r -> r.getMetadata().getName().endsWith("-" + index)) - .collect(Collectors.toList()); - if (resources.isEmpty()) { - return Optional.empty(); - } else if (resources.size() > 1) { - throw new IllegalStateException("More than one resource found for index:" + index); - } else { - return Optional.of(resources.get(0)); - } + public Map getSecondaryResources(BulkDependentTestCustomResource primary, + Context context) { + var configMaps = context.getSecondaryResources(ConfigMap.class); + Map result = new HashMap<>(configMaps.size()); + configMaps.forEach(cm -> { + String name = cm.getMetadata().getName(); + if (name.startsWith(primary.getMetadata().getName())) { + String key = name.substring(name.lastIndexOf(INDEX_DELIMITER) + 1); + result.put(Integer.parseInt(key), cm); + } + }); + return result; } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java index ed486f6f34..30f7b108cf 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java @@ -1,10 +1,6 @@ package io.javaoperatorsdk.operator.sample.bulkdependent.external; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Optional; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -17,7 +13,7 @@ public class ExternalBulkDependentResource extends PollingDependentResource - implements BulkDependentResource, + implements BulkDependentResource, BulkUpdater { public static final String EXTERNAL_RESOURCE_NAME_DELIMITER = "#"; @@ -40,31 +36,6 @@ public Map> fetchResources() { return result; } - @Override - public void delete(BulkDependentTestCustomResource primary, - Context context) { - deleteBulkResourcesIfRequired(0, primary, context); - } - - @Override - public int count(BulkDependentTestCustomResource primary, - Context context) { - return primary.getSpec().getNumberOfResources(); - } - - @Override - public void deleteBulkResourceWithIndex(BulkDependentTestCustomResource primary, - ExternalResource resource, int i, Context context) { - externalServiceMock.delete(resource.getId()); - } - - @Override - public ExternalResource desired(BulkDependentTestCustomResource primary, int index, - Context context) { - return new ExternalResource(toExternalResourceId(primary, index), - primary.getSpec().getAdditionalData()); - } - @Override public ExternalResource create(ExternalResource desired, BulkDependentTestCustomResource primary, Context context) { @@ -77,14 +48,6 @@ public ExternalResource update(ExternalResource actual, ExternalResource desired return externalServiceMock.update(desired); } - @Override - public Matcher.Result match(ExternalResource actualResource, - BulkDependentTestCustomResource primary, - int index, Context context) { - var desired = desired(primary, index, context); - return Matcher.Result.computed(desired.equals(actualResource), desired); - } - private static String toExternalResourceId(BulkDependentTestCustomResource primary, int i) { return primary.getMetadata().getName() + EXTERNAL_RESOURCE_NAME_DELIMITER + primary.getMetadata().getNamespace() + @@ -97,10 +60,49 @@ private ResourceID toResourceID(ExternalResource externalResource) { } @Override - public Optional getSecondaryResource(BulkDependentTestCustomResource primary, - int index, Context context) { + public Set targetKeys(BulkDependentTestCustomResource primary, + Context context) { + var number = primary.getSpec().getNumberOfResources(); + Set res = new HashSet<>(); + for (int i = 0; i < number; i++) { + res.add(i); + } + return res; + } + + @Override + public Map getSecondaryResources( + BulkDependentTestCustomResource primary, + Context context) { return context.getSecondaryResources(resourceType()).stream() - .filter(r -> r.getId().endsWith(EXTERNAL_RESOURCE_NAME_DELIMITER + index)) - .collect(Collectors.toList()).stream().findFirst(); + .filter(r -> r.getId() + .startsWith(primary.getMetadata().getName() + EXTERNAL_RESOURCE_NAME_DELIMITER + + primary.getMetadata().getNamespace() + + EXTERNAL_RESOURCE_NAME_DELIMITER)) + .collect(Collectors.toMap( + r -> Integer.parseInt( + r.getId().substring(r.getId().lastIndexOf(EXTERNAL_RESOURCE_NAME_DELIMITER) + 1)), + r -> r)); + } + + @Override + public ExternalResource desired(BulkDependentTestCustomResource primary, Integer key, + Context context) { + return new ExternalResource(toExternalResourceId(primary, key), + primary.getSpec().getAdditionalData()); + } + + @Override + public void deleteBulkResource(BulkDependentTestCustomResource primary, ExternalResource resource, + Context context) { + externalServiceMock.delete(resource.getId()); + } + + @Override + public Matcher.Result match(ExternalResource actualResource, + BulkDependentTestCustomResource primary, Integer index, + Context context) { + var desired = desired(primary, index, context); + return Matcher.Result.computed(desired.equals(actualResource), desired); } } From 97e6d73e5020698fd946af40ee15580bf5fbbd7d Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 5 Oct 2022 15:40:33 +0200 Subject: [PATCH 2/5] improvements --- .../dependent/AbstractDependentResource.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index b3906af9dc..6c1f571df8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -57,21 +57,16 @@ public ReconcileResult reconcile(P primary, Context

context) { } } - @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "rawtypes"}) protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actualResources, P primary, Context

context) { - actualResources.entrySet().forEach(entry -> { - if (!targetKeys.contains(entry.getKey())) { - bulkDependentResource.deleteBulkResource(primary, entry.getValue(), context); + actualResources.forEach((key, value) -> { + if (!targetKeys.contains(key)) { + bulkDependentResource.deleteBulkResource(primary, value, context); } }); } - protected void deleteBulkResourcesIfRequired(P primary, Context

context) { - var actualResources = bulkDependentResource.getSecondaryResources(primary, context); - deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context); - } - @SuppressWarnings("unchecked") protected ReconcileResult reconcileIndexAware(P primary, R resource, Object key, Context

context) { From 00e88e4c511494262418f62a3458c527b6e3afea Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 5 Oct 2022 15:52:01 +0200 Subject: [PATCH 3/5] putting back keys to the api --- .../processing/dependent/AbstractDependentResource.java | 2 +- .../processing/dependent/BulkDependentResource.java | 6 ++++-- .../dependent/kubernetes/KubernetesDependentResource.java | 3 +-- .../ConfigMapDeleterBulkDependentResource.java | 8 ++++++++ .../external/ExternalBulkDependentResource.java | 1 + 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 6c1f571df8..5bf45431b7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -62,7 +62,7 @@ protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actu P primary, Context

context) { actualResources.forEach((key, value) -> { if (!targetKeys.contains(key)) { - bulkDependentResource.deleteBulkResource(primary, value, context); + bulkDependentResource.deleteBulkResource(primary, value, key, context); } }); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java index 4894aa822d..c50c9e9399 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java @@ -32,9 +32,10 @@ public interface BulkDependentResource * * @param primary resource * @param resource actual resource from the cache for the index + * @param key key of the resource * @param context actual context */ - void deleteBulkResource(P primary, R resource, Context

context); + void deleteBulkResource(P primary, R resource, T key, Context

context); /** * Determines whether the specified secondary resource matches the desired state with target index @@ -43,12 +44,13 @@ public interface BulkDependentResource * * @param actualResource the resource we want to determine whether it's matching the desired state * @param primary the primary resource from which the desired state is inferred + * @param key key of the resource * @param context the context in which the resource is being matched * @return a {@link Result} encapsulating whether the resource matched its desired state and this * associated state if it was computed as part of the matching process. Use the static * convenience methods ({@link Result#nonComputed(boolean)} and * {@link Result#computed(boolean, Object)}) */ - Result match(R actualResource, P primary, T index, Context

context); + Result match(R actualResource, P primary, T key, Context

context); } 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 475610e87d..7f366b1fba 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 @@ -150,9 +150,8 @@ protected void handleDelete(P primary, Context

context) { } - public void deleteBulkResource(P primary, R resource, Context

context) { + public void deleteBulkResource(P primary, R resource, Object key, Context

context) { client.resource(resource).delete(); - } protected Resource prepare(R desired, P primary, String actionName) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java index 8ef68f1aa3..af138fb922 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java @@ -57,6 +57,14 @@ public ConfigMap desired(BulkDependentTestCustomResource primary, Integer key, return configMap; } + // todo fix generics? + @Override + public void deleteBulkResource(BulkDependentTestCustomResource primary, ConfigMap resource, + Integer key, + Context context) { + super.deleteBulkResource(primary, resource, key, context); + } + // todo fix generics? @Override public Matcher.Result match(ConfigMap actualResource, diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java index 30f7b108cf..7f6acef2e8 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java @@ -94,6 +94,7 @@ public ExternalResource desired(BulkDependentTestCustomResource primary, Integer @Override public void deleteBulkResource(BulkDependentTestCustomResource primary, ExternalResource resource, + Integer key, Context context) { externalServiceMock.delete(resource.getId()); } From a532202ad527acf4a32601cd7585fc5f9ac624d7 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 5 Oct 2022 15:59:22 +0200 Subject: [PATCH 4/5] key is a string for bulk resources --- .../dependent/AbstractDependentResource.java | 14 ++++++------- .../dependent/BulkDependentResource.java | 13 ++++++------ .../KubernetesDependentResource.java | 4 ++-- ...ConfigMapDeleterBulkDependentResource.java | 20 +++++++++--------- .../ExternalBulkDependentResource.java | 21 +++++++++---------- 5 files changed, 35 insertions(+), 37 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 5bf45431b7..44bcdce85b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -25,7 +25,7 @@ public abstract class AbstractDependentResource protected Creator creator; protected Updater updater; @SuppressWarnings("rawtypes") - protected BulkDependentResource bulkDependentResource; + protected BulkDependentResource bulkDependentResource; private ResourceDiscriminator resourceDiscriminator; @SuppressWarnings({"unchecked", "rawtypes"}) @@ -36,18 +36,18 @@ protected AbstractDependentResource() { bulkDependentResource = bulk ? (BulkDependentResource) this : null; } - @SuppressWarnings("unchecked") + @Override public ReconcileResult reconcile(P primary, Context

context) { if (bulk) { final var targetKeys = bulkDependentResource.targetKeys(primary, context); - Map actualResources = + Map actualResources = bulkDependentResource.getSecondaryResources(primary, context); deleteBulkResourcesIfRequired(targetKeys, actualResources, primary, context); final List> results = new ArrayList<>(targetKeys.size()); - for (Object key : targetKeys) { + for (String key : targetKeys) { results.add(reconcileIndexAware(primary, actualResources.get(key), key, context)); } return ReconcileResult.aggregatedResult(results); @@ -58,7 +58,7 @@ public ReconcileResult reconcile(P primary, Context

context) { } @SuppressWarnings({"unchecked", "rawtypes"}) - protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actualResources, + protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actualResources, P primary, Context

context) { actualResources.forEach((key, value) -> { if (!targetKeys.contains(key)) { @@ -68,7 +68,7 @@ protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actu } @SuppressWarnings("unchecked") - protected ReconcileResult reconcileIndexAware(P primary, R resource, Object key, + protected ReconcileResult reconcileIndexAware(P primary, R resource, String key, Context

context) { if (creatable || updatable) { if (resource == null) { @@ -107,7 +107,7 @@ protected ReconcileResult reconcileIndexAware(P primary, R resource, Object k return ReconcileResult.noOperation(resource); } - private R desiredIndexAware(P primary, Object key, Context

context) { + private R desiredIndexAware(P primary, String key, Context

context) { return bulk ? (R) bulkDependentResource.desired(primary, key, context) : desired(primary, context); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java index c50c9e9399..1c7189293b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java @@ -14,19 +14,18 @@ * {@link Creator} and {@link Deleter} interfaces out of the box. A concrete dependent resource can * implement additionally also {@link Updater}. */ -public interface BulkDependentResource +public interface BulkDependentResource extends Creator, Deleter

{ /** * @return number of resources to create */ - Set targetKeys(P primary, Context

context); + Set targetKeys(P primary, Context

context); - Map getSecondaryResources(P primary, Context

context); + Map getSecondaryResources(P primary, Context

context); - R desired(P primary, T key, Context

context); + R desired(P primary, String key, Context

context); - // todo add back key? /** * Used to delete resource if the desired count is lower than the actual count of a resource. * @@ -35,7 +34,7 @@ public interface BulkDependentResource * @param key key of the resource * @param context actual context */ - void deleteBulkResource(P primary, R resource, T key, Context

context); + void deleteBulkResource(P primary, R resource, String key, Context

context); /** * Determines whether the specified secondary resource matches the desired state with target index @@ -51,6 +50,6 @@ public interface BulkDependentResource * convenience methods ({@link Result#nonComputed(boolean)} and * {@link Result#computed(boolean, Object)}) */ - Result match(R actualResource, P primary, T key, Context

context); + Result match(R actualResource, P primary, String key, Context

context); } 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 7f366b1fba..97ad7fa226 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 @@ -139,7 +139,7 @@ public Result match(R actualResource, P primary, Context

context) { return matcher.match(actualResource, primary, context); } - public Result match(R actualResource, P primary, Object key, Context

context) { + public Result match(R actualResource, P primary, String key, Context

context) { final var desired = bulkDependentResource.desired(primary, key, context); return GenericKubernetesResourceMatcher.match((R) desired, actualResource, false); } @@ -150,7 +150,7 @@ protected void handleDelete(P primary, Context

context) { } - public void deleteBulkResource(P primary, R resource, Object key, Context

context) { + public void deleteBulkResource(P primary, R resource, String key, Context

context) { client.resource(resource).delete(); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java index af138fb922..4c5deb29cb 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java @@ -21,7 +21,7 @@ public class ConfigMapDeleterBulkDependentResource implements Creator, Updater, Deleter, - BulkDependentResource { + BulkDependentResource { public static final String LABEL_KEY = "bulk"; public static final String LABEL_VALUE = "true"; @@ -33,18 +33,18 @@ public ConfigMapDeleterBulkDependentResource() { } @Override - public Set targetKeys(BulkDependentTestCustomResource primary, + public Set targetKeys(BulkDependentTestCustomResource primary, Context context) { var number = primary.getSpec().getNumberOfResources(); - Set res = new HashSet<>(); + Set res = new HashSet<>(); for (int i = 0; i < number; i++) { - res.add(i); + res.add(Integer.toString(i)); } return res; } @Override - public ConfigMap desired(BulkDependentTestCustomResource primary, Integer key, + public ConfigMap desired(BulkDependentTestCustomResource primary, String key, Context context) { ConfigMap configMap = new ConfigMap(); configMap.setMetadata(new ObjectMetaBuilder() @@ -60,7 +60,7 @@ public ConfigMap desired(BulkDependentTestCustomResource primary, Integer key, // todo fix generics? @Override public void deleteBulkResource(BulkDependentTestCustomResource primary, ConfigMap resource, - Integer key, + String key, Context context) { super.deleteBulkResource(primary, resource, key, context); } @@ -69,20 +69,20 @@ public void deleteBulkResource(BulkDependentTestCustomResource primary, ConfigMa @Override public Matcher.Result match(ConfigMap actualResource, BulkDependentTestCustomResource primary, - Integer index, Context context) { + String index, Context context) { return super.match(actualResource, primary, index, context); } @Override - public Map getSecondaryResources(BulkDependentTestCustomResource primary, + public Map getSecondaryResources(BulkDependentTestCustomResource primary, Context context) { var configMaps = context.getSecondaryResources(ConfigMap.class); - Map result = new HashMap<>(configMaps.size()); + Map result = new HashMap<>(configMaps.size()); configMaps.forEach(cm -> { String name = cm.getMetadata().getName(); if (name.startsWith(primary.getMetadata().getName())) { String key = name.substring(name.lastIndexOf(INDEX_DELIMITER) + 1); - result.put(Integer.parseInt(key), cm); + result.put(key, cm); } }); return result; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java index 7f6acef2e8..79b861d060 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java @@ -13,7 +13,7 @@ public class ExternalBulkDependentResource extends PollingDependentResource - implements BulkDependentResource, + implements BulkDependentResource, BulkUpdater { public static final String EXTERNAL_RESOURCE_NAME_DELIMITER = "#"; @@ -48,7 +48,7 @@ public ExternalResource update(ExternalResource actual, ExternalResource desired return externalServiceMock.update(desired); } - private static String toExternalResourceId(BulkDependentTestCustomResource primary, int i) { + private static String toExternalResourceId(BulkDependentTestCustomResource primary, String i) { return primary.getMetadata().getName() + EXTERNAL_RESOURCE_NAME_DELIMITER + primary.getMetadata().getNamespace() + EXTERNAL_RESOURCE_NAME_DELIMITER + i; @@ -60,18 +60,18 @@ private ResourceID toResourceID(ExternalResource externalResource) { } @Override - public Set targetKeys(BulkDependentTestCustomResource primary, + public Set targetKeys(BulkDependentTestCustomResource primary, Context context) { var number = primary.getSpec().getNumberOfResources(); - Set res = new HashSet<>(); + Set res = new HashSet<>(); for (int i = 0; i < number; i++) { - res.add(i); + res.add(Integer.toString(i)); } return res; } @Override - public Map getSecondaryResources( + public Map getSecondaryResources( BulkDependentTestCustomResource primary, Context context) { return context.getSecondaryResources(resourceType()).stream() @@ -80,13 +80,12 @@ public Map getSecondaryResources( primary.getMetadata().getNamespace() + EXTERNAL_RESOURCE_NAME_DELIMITER)) .collect(Collectors.toMap( - r -> Integer.parseInt( - r.getId().substring(r.getId().lastIndexOf(EXTERNAL_RESOURCE_NAME_DELIMITER) + 1)), + r -> r.getId().substring(r.getId().lastIndexOf(EXTERNAL_RESOURCE_NAME_DELIMITER) + 1), r -> r)); } @Override - public ExternalResource desired(BulkDependentTestCustomResource primary, Integer key, + public ExternalResource desired(BulkDependentTestCustomResource primary, String key, Context context) { return new ExternalResource(toExternalResourceId(primary, key), primary.getSpec().getAdditionalData()); @@ -94,14 +93,14 @@ public ExternalResource desired(BulkDependentTestCustomResource primary, Integer @Override public void deleteBulkResource(BulkDependentTestCustomResource primary, ExternalResource resource, - Integer key, + String key, Context context) { externalServiceMock.delete(resource.getId()); } @Override public Matcher.Result match(ExternalResource actualResource, - BulkDependentTestCustomResource primary, Integer index, + BulkDependentTestCustomResource primary, String index, Context context) { var desired = desired(primary, index, context); return Matcher.Result.computed(desired.equals(actualResource), desired); From 1304995ad1b653c5d643acd3fb6585445d62bd6c Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 5 Oct 2022 16:02:41 +0200 Subject: [PATCH 5/5] improvements --- .../dependent/AbstractDependentResource.java | 9 +++------ .../ConfigMapDeleterBulkDependentResource.java | 17 ----------------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 44bcdce85b..1786c3a568 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -24,7 +24,6 @@ public abstract class AbstractDependentResource protected Creator creator; protected Updater updater; - @SuppressWarnings("rawtypes") protected BulkDependentResource bulkDependentResource; private ResourceDiscriminator resourceDiscriminator; @@ -57,7 +56,7 @@ public ReconcileResult reconcile(P primary, Context

context) { } } - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings({"rawtypes"}) protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actualResources, P primary, Context

context) { actualResources.forEach((key, value) -> { @@ -67,7 +66,6 @@ protected void deleteBulkResourcesIfRequired(Set targetKeys, Map actu }); } - @SuppressWarnings("unchecked") protected ReconcileResult reconcileIndexAware(P primary, R resource, String key, Context

context) { if (creatable || updatable) { @@ -108,10 +106,11 @@ protected ReconcileResult reconcileIndexAware(P primary, R resource, String k } private R desiredIndexAware(P primary, String key, Context

context) { - return bulk ? (R) bulkDependentResource.desired(primary, key, context) + return bulk ? bulkDependentResource.desired(primary, key, context) : desired(primary, context); } + @Override public Optional getSecondaryResource(P primary, Context

context) { return resourceDiscriminator == null ? context.getSecondaryResource(resourceType()) : resourceDiscriminator.distinguish(resourceType(), primary, context); @@ -174,8 +173,6 @@ protected R desired(P primary, Context

context) { "desired method must be implemented if this DependentResource can be created and/or updated"); } - - @SuppressWarnings("unchecked") public void delete(P primary, Context

context) { if (bulk) { var actualResources = bulkDependentResource.getSecondaryResources(primary, context); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java index 4c5deb29cb..e41b38c97e 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java @@ -8,7 +8,6 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.processing.dependent.BulkDependentResource; import io.javaoperatorsdk.operator.processing.dependent.Creator; -import io.javaoperatorsdk.operator.processing.dependent.Matcher; import io.javaoperatorsdk.operator.processing.dependent.Updater; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; @@ -57,22 +56,6 @@ public ConfigMap desired(BulkDependentTestCustomResource primary, String key, return configMap; } - // todo fix generics? - @Override - public void deleteBulkResource(BulkDependentTestCustomResource primary, ConfigMap resource, - String key, - Context context) { - super.deleteBulkResource(primary, resource, key, context); - } - - // todo fix generics? - @Override - public Matcher.Result match(ConfigMap actualResource, - BulkDependentTestCustomResource primary, - String index, Context context) { - return super.match(actualResource, primary, index, context); - } - @Override public Map getSecondaryResources(BulkDependentTestCustomResource primary, Context context) {