Skip to content

fix: ordered managed dependents #1120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use a map why not a list? as in ControllerConfiguration or DefaultControllerConfiguration: List<DependentResourceSpec<?, ?>>
DependentResourceSpec already contains the name. It's basically data duplication.

Would be also consistent with those 2 classes.

Copy link
Collaborator

@metacosm metacosm Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes it easier to check if the spec is already present, find it again, naturally ensures that there is no duplication, easier to replace as well. Basically, indexing the specs by their name.


private ControllerConfigurationOverrider(ControllerConfiguration<R> original) {
finalizer = original.getFinalizerName();
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -90,7 +87,7 @@ public ResourceEventFilter<R> getEventFilter() {
}

@Override
public Map<String, DependentResourceSpec<?, ?>> getDependentResources() {
public List<DependentResourceSpec<?, ?>> getDependentResources() {
return dependents;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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!");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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];
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -377,9 +377,4 @@ public void stop() {
public boolean useFinalizer() {
return isCleaner || hasDeleterDependents;
}

@SuppressWarnings("rawtypes")
public Map<String, DependentResource> getDependents() {
return dependents;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,17 @@ void replaceNamedDependentResourceConfigShouldWork() {
var dependents = configuration.getDependentResources();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test needs to be breaked down to multiple separate test cases. It's very hard to read this way.

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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}

}
Loading