fix: ordered managed dependents (operator-framework#1120)

Co-authored-by: Chris Laprun <metacosm@gmail.com>

Author: csviri

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java

@@ -1,11 +1,7 @@
 package io.javaoperatorsdk.operator.api.config;
 
 import java.time.Duration;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 import java.util.function.Function;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -26,7 +22,7 @@ public class AnnotationControllerConfiguration<R extends HasMetadata>
 
   protected final Reconciler<R> reconciler;
   private final ControllerConfiguration annotation;
-  private Map<String, DependentResourceSpec<?, ?>> specs;
+  private List<DependentResourceSpec<?, ?>> specs;
 
   public AnnotationControllerConfiguration(Reconciler<R> reconciler) {
     this.reconciler = reconciler;
@@ -135,15 +131,15 @@ public static <T> T valueOrDefault(
 
   @SuppressWarnings({"rawtypes", "unchecked"})
   @Override
-  public Map<String, DependentResourceSpec<?, ?>> getDependentResources() {
+  public List<DependentResourceSpec<?, ?>> getDependentResources() {
     if (specs == null) {
       final var dependents =
           valueOrDefault(annotation, ControllerConfiguration::dependents, new Dependent[] {});
       if (dependents.length == 0) {
-        return Collections.emptyMap();
+        return Collections.emptyList();
       }
 
-      specs = new HashMap<>(dependents.length);
+      specs = new ArrayList<>(dependents.length);
       for (Dependent dependent : dependents) {
         Object config = null;
         final Class<? extends DependentResource> dependentType = dependent.type();
@@ -163,14 +159,15 @@ public static <T> T valueOrDefault(
         if (name.isBlank()) {
           name = DependentResource.defaultNameFor(dependentType);
         }
-        final DependentResourceSpec<?, ?> spec = specs.get(name);
-        if (spec != null) {
-          throw new IllegalArgumentException(
-              "A DependentResource named: " + name + " already exists: " + spec);
+        for (var spec : specs) {
+          if (spec.getName().equals(name)) {
+            throw new IllegalArgumentException(
+                "A DependentResource named: " + name + " already exists: " + spec);
+          }
         }
-        specs.put(name, new DependentResourceSpec(dependentType, config, name));
+        specs.add(new DependentResourceSpec(dependentType, config, name));
       }
     }
-    return Collections.unmodifiableMap(specs);
+    return Collections.unmodifiableList(specs);
   }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java

@@ -2,7 +2,7 @@
 
 import java.time.Duration;
 import java.util.Collections;
-import java.util.Map;
+import java.util.List;
 import java.util.Optional;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -46,8 +46,8 @@ default ResourceEventFilter<R> getEventFilter() {
     return ResourceEventFilters.passthrough();
   }
 
-  default Map<String, DependentResourceSpec<?, ?>> getDependentResources() {
-    return Collections.emptyMap();
+  default List<DependentResourceSpec<?, ?>> getDependentResources() {
+    return Collections.emptyList();
   }
 
   default Optional<Duration> reconciliationMaxInterval() {

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java

@@ -1,17 +1,17 @@
 package io.javaoperatorsdk.operator.api.config;
 
 import java.time.Duration;
-import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;
 
-@SuppressWarnings({"rawtypes", "unchecked", "unused"})
+@SuppressWarnings({"unused"})
 public class ControllerConfigurationOverrider<R extends HasMetadata> {
 
   private String finalizer;
@@ -22,7 +22,7 @@ public class ControllerConfigurationOverrider<R extends HasMetadata> {
   private ResourceEventFilter<R> customResourcePredicate;
   private final ControllerConfiguration<R> original;
   private Duration reconciliationMaxInterval;
-  private final Map<String, DependentResourceSpec<?, ?>> dependentResourceSpecs;
+  private final LinkedHashMap<String, DependentResourceSpec<?, ?>> namedDependentResourceSpecs;
 
   private ControllerConfigurationOverrider(ControllerConfiguration<R> original) {
     finalizer = original.getFinalizerName();
@@ -33,7 +33,9 @@ private ControllerConfigurationOverrider(ControllerConfiguration<R> original) {
     customResourcePredicate = original.getEventFilter();
     reconciliationMaxInterval = original.reconciliationMaxInterval().orElse(null);
     // make the original specs modifiable
-    dependentResourceSpecs = new HashMap<>(original.getDependentResources());
+    final var dependentResources = original.getDependentResources();
+    namedDependentResourceSpecs = new LinkedHashMap<>(dependentResources.size());
+    dependentResources.forEach(drs -> namedDependentResourceSpecs.put(drs.getName(), drs));
     this.original = original;
   }
 
@@ -92,14 +94,13 @@ public ControllerConfigurationOverrider<R> withReconciliationMaxInterval(
 
   public ControllerConfigurationOverrider<R> replacingNamedDependentResourceConfig(String name,
       Object dependentResourceConfig) {
-    final var currentConfig = dependentResourceSpecs.get(name);
-    if (currentConfig == null) {
+
+    var namedRDS = namedDependentResourceSpecs.get(name);
+    if (namedRDS == null) {
       throw new IllegalArgumentException("Cannot find a DependentResource named: " + name);
     }
-    dependentResourceSpecs.remove(name);
-    dependentResourceSpecs.put(name,
-        new DependentResourceSpec(currentConfig.getDependentResourceClass(),
-            dependentResourceConfig, name));
+    namedDependentResourceSpecs.put(name, new DependentResourceSpec<>(
+        namedRDS.getDependentResourceClass(), dependentResourceConfig, name));
     return this;
   }
 
@@ -116,7 +117,7 @@ public ControllerConfiguration<R> build() {
         customResourcePredicate,
         original.getResourceClass(),
         reconciliationMaxInterval,
-        dependentResourceSpecs);
+        namedDependentResourceSpecs.values().stream().collect(Collectors.toUnmodifiableList()));
   }
 
   public static <R extends HasMetadata> ControllerConfigurationOverrider<R> override(

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java

@@ -1,10 +1,7 @@
 package io.javaoperatorsdk.operator.api.config;
 
 import java.time.Duration;
-import java.util.Collections;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
@@ -21,7 +18,7 @@ public class DefaultControllerConfiguration<R extends HasMetadata>
   private final boolean generationAware;
   private final RetryConfiguration retryConfiguration;
   private final ResourceEventFilter<R> resourceEventFilter;
-  private final Map<String, DependentResourceSpec<?, ?>> dependents;
+  private final List<DependentResourceSpec<?, ?>> dependents;
   private final Duration reconciliationMaxInterval;
 
   // NOSONAR constructor is meant to provide all information
@@ -37,7 +34,7 @@ public DefaultControllerConfiguration(
       ResourceEventFilter<R> resourceEventFilter,
       Class<R> resourceClass,
       Duration reconciliationMaxInterval,
-      Map<String, DependentResourceSpec<?, ?>> dependents) {
+      List<DependentResourceSpec<?, ?>> dependents) {
     super(labelSelector, resourceClass, namespaces);
     this.associatedControllerClassName = associatedControllerClassName;
     this.name = name;
@@ -51,7 +48,7 @@ public DefaultControllerConfiguration(
             : retryConfiguration;
     this.resourceEventFilter = resourceEventFilter;
 
-    this.dependents = dependents != null ? dependents : Collections.emptyMap();
+    this.dependents = dependents != null ? dependents : Collections.emptyList();
   }
 
   @Override
@@ -90,7 +87,7 @@ public ResourceEventFilter<R> getEventFilter() {
   }
 
   @Override
-  public Map<String, DependentResourceSpec<?, ?>> getDependentResources() {
+  public List<DependentResourceSpec<?, ?>> getDependentResources() {
     return dependents;
   }
 

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java

@@ -34,9 +34,6 @@ public static void init() {
       log.debug("Initialized ExecutorServiceManager executor: {}, timeout: {}",
           configuration.getExecutorService().getClass(),
           configuration.getTerminationTimeoutSeconds());
-      log.debug("Initialized ExecutorServiceManager executor: {}, timeout: {}",
-          configuration.getExecutorService().getClass(),
-          configuration.getTerminationTimeoutSeconds());
     } else {
       log.debug("Already started, reusing already setup instance!");
     }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java

@@ -1,8 +1,7 @@
 package io.javaoperatorsdk.operator.processing;
 
 import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -54,7 +53,7 @@ public class Controller<P extends HasMetadata>
   private final ControllerConfiguration<P> configuration;
   private final KubernetesClient kubernetesClient;
   private final EventSourceManager<P> eventSourceManager;
-  private final Map<String, DependentResource> dependents;
+  private final LinkedHashMap<String, DependentResource> dependents;
   private final boolean contextInitializer;
   private final boolean hasDeleterDependents;
   private final boolean isCleaner;
@@ -77,18 +76,18 @@ public Controller(Reconciler<P> reconciler,
     final var specs = configuration.getDependentResources();
     final var size = specs.size();
     if (size == 0) {
-      dependents = Collections.emptyMap();
+      dependents = new LinkedHashMap<>();
     } else {
-      final var dependentsHolder = new HashMap<String, DependentResource>(size);
-      specs.forEach((name, drs) -> {
+      final Map<String, DependentResource> dependentsHolder = new LinkedHashMap<>(size);
+      specs.forEach(drs -> {
         final var dependent = createAndConfigureFrom(drs, kubernetesClient);
         // check if dependent implements Deleter to record that fact
         if (!hasDeleterHolder[0] && dependent instanceof Deleter) {
           hasDeleterHolder[0] = true;
         }
-        dependentsHolder.put(name, dependent);
+        dependentsHolder.put(drs.getName(), dependent);
       });
-      dependents = Collections.unmodifiableMap(dependentsHolder);
+      dependents = new LinkedHashMap<>(dependentsHolder);
     }
 
     hasDeleterDependents = hasDeleterHolder[0];
@@ -193,7 +192,8 @@ public UpdateControl<P> execute() throws Exception {
             dependents.forEach((name, dependent) -> {
               try {
                 final var reconcileResult = dependent.reconcile(resource, context);
-                context.managedDependentResourceContext().setReconcileResult(name, reconcileResult);
+                context.managedDependentResourceContext().setReconcileResult(name,
+                    reconcileResult);
                 log.info("Reconciled dependent '{}' -> {}", name, reconcileResult.getOperation());
               } catch (Exception e) {
                 final var message = e.getMessage();
@@ -377,9 +377,4 @@ public void stop() {
   public boolean useFinalizer() {
     return isCleaner || hasDeleterDependents;
   }
-
-  @SuppressWarnings("rawtypes")
-  public Map<String, DependentResource> getDependents() {
-    return dependents;
-  }
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java

@@ -25,13 +25,17 @@ void replaceNamedDependentResourceConfigShouldWork() {
     var dependents = configuration.getDependentResources();
     assertFalse(dependents.isEmpty());
     assertEquals(1, dependents.size());
+
     final var dependentResourceName = DependentResource.defaultNameFor(ReadOnlyDependent.class);
-    assertTrue(dependents.containsKey(dependentResourceName));
-    var dependentSpec = dependents.get(dependentResourceName);
+    assertTrue(dependents.stream().anyMatch(dr -> dr.getName().equals(dependentResourceName)));
+
+    var dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName))
+        .findFirst().get();
     assertEquals(ReadOnlyDependent.class, dependentSpec.getDependentResourceClass());
     var maybeConfig = dependentSpec.getDependentResourceConfiguration();
     assertTrue(maybeConfig.isPresent());
     assertTrue(maybeConfig.get() instanceof KubernetesDependentResourceConfig);
+
     var config = (KubernetesDependentResourceConfig) maybeConfig.orElseThrow();
     // check that the DependentResource inherits the controller's configuration if applicable
     assertEquals(1, config.namespaces().length);
@@ -47,7 +51,8 @@ void replaceNamedDependentResourceConfigShouldWork() {
             new KubernetesDependentResourceConfig(new String[] {overriddenNS}, labelSelector))
         .build();
     dependents = overridden.getDependentResources();
-    dependentSpec = dependents.get(dependentResourceName);
+    dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName))
+        .findFirst().get();
     config = (KubernetesDependentResourceConfig) dependentSpec.getDependentResourceConfiguration()
         .orElseThrow();
     assertEquals(1, config.namespaces().length);

operator-framework/src/test/java/io/javaoperatorsdk/operator/OrderedManagedDependentIT.java

@@ -0,0 +1,49 @@
+package io.javaoperatorsdk.operator;
+
+import java.time.Duration;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ObjectMeta;
+import io.javaoperatorsdk.operator.junit.OperatorExtension;
+import io.javaoperatorsdk.operator.sample.orderedmanageddependent.ConfigMapDependentResource1;
+import io.javaoperatorsdk.operator.sample.orderedmanageddependent.ConfigMapDependentResource2;
+import io.javaoperatorsdk.operator.sample.orderedmanageddependent.OrderedManagedDependentCustomResource;
+import io.javaoperatorsdk.operator.sample.orderedmanageddependent.OrderedManagedDependentTestReconciler;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class OrderedManagedDependentIT {
+
+  @RegisterExtension
+  OperatorExtension operator =
+      OperatorExtension.builder().withReconciler(new OrderedManagedDependentTestReconciler())
+          .build();
+
+  @Test
+  void managedDependentsAreReconciledInOrder() {
+    operator.create(OrderedManagedDependentCustomResource.class, createTestResource());
+
+    await().atMost(Duration.ofSeconds(5))
+        .until(() -> ((OrderedManagedDependentTestReconciler) operator.getFirstReconciler())
+            .getNumberOfExecutions() >= 1);
+    // todo change to more precise values when event filtering is fixed
+    // assertThat(OrderedManagedDependentTestReconciler.dependentExecution).hasSize(4);
+    assertThat(OrderedManagedDependentTestReconciler.dependentExecution.get(0))
+        .isEqualTo(ConfigMapDependentResource1.class);
+    assertThat(OrderedManagedDependentTestReconciler.dependentExecution.get(1))
+        .isEqualTo(ConfigMapDependentResource2.class);
+
+  }
+
+
+  private OrderedManagedDependentCustomResource createTestResource() {
+    OrderedManagedDependentCustomResource cr = new OrderedManagedDependentCustomResource();
+    cr.setMetadata(new ObjectMeta());
+    cr.getMetadata().setName("test");
+    return cr;
+  }
+
+}