Skip to content

refactor: reduce redundancy by removing duplicate configuration recording #2035

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 1 commit into from
Aug 31, 2023
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,10 +1,7 @@
package io.javaoperatorsdk.operator.processing.event.source.controller;

import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -21,12 +18,8 @@
import io.javaoperatorsdk.operator.processing.event.source.informer.ManagedInformerEventSource;

import static io.javaoperatorsdk.operator.ReconcilerUtils.handleKubernetesClientException;
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName;
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID;
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion;
import static io.javaoperatorsdk.operator.processing.event.source.controller.InternalEventFilters.onUpdateFinalizerNeededAndApplied;
import static io.javaoperatorsdk.operator.processing.event.source.controller.InternalEventFilters.onUpdateGenerationAware;
import static io.javaoperatorsdk.operator.processing.event.source.controller.InternalEventFilters.onUpdateMarkedForDeletion;
import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.*;
import static io.javaoperatorsdk.operator.processing.event.source.controller.InternalEventFilters.*;

public class ControllerResourceEventSource<T extends HasMetadata>
extends ManagedInformerEventSource<T, T, ControllerConfiguration<T>>
Expand Down Expand Up @@ -137,9 +130,4 @@ public void setOnDeleteFilter(OnDeleteFilter<? super T> onDeleteFilter) {
throw new IllegalStateException(
"onDeleteFilter is not supported for controller resource event source");
}

@Override
public void addIndexers(Map<String, Function<T, List<String>>> indexers) {
manager().addIndexers(indexers);
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
package io.javaoperatorsdk.operator.processing.event.source.informer;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.*;
import java.util.function.Function;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -76,7 +72,6 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata>

private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class);

private final InformerConfiguration<R> configuration;
// always called from a synchronized method
private final EventRecorder<R> eventRecorder = new EventRecorder<>();
// we need direct control for the indexer to propagate the just update resource also to the index
Expand All @@ -91,8 +86,6 @@ public InformerEventSource(

public InformerEventSource(InformerConfiguration<R> configuration, KubernetesClient client) {
super(client.resources(configuration.getResourceClass()), configuration);
this.configuration = configuration;


// If there is a primary to secondary mapper there is no need for primary to secondary index.
primaryToSecondaryMapper = configuration.getPrimaryToSecondaryMapper();
Expand Down Expand Up @@ -193,7 +186,7 @@ private boolean temporaryCacheHasResourceWithSameVersionAs(R resource) {

private void propagateEvent(R object) {
var primaryResourceIdSet =
configuration.getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object);
configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object);
if (primaryResourceIdSet.isEmpty()) {
return;
}
Expand All @@ -217,8 +210,7 @@ public Set<R> getSecondaryResources(P primary) {
Set<ResourceID> secondaryIDs;
if (useSecondaryToPrimaryIndex()) {
var primaryResourceID = ResourceID.fromResource(primary);
secondaryIDs =
primaryToSecondaryIndex.getSecondaryResources(primaryResourceID);
secondaryIDs = primaryToSecondaryIndex.getSecondaryResources(primaryResourceID);
log.debug(
"Using PrimaryToSecondaryIndex to find secondary resources for primary: {}. Found secondary ids: {} ",
primaryResourceID, secondaryIDs);
Expand All @@ -232,8 +224,16 @@ public Set<R> getSecondaryResources(P primary) {
.collect(Collectors.toSet());
}

/**
* Returns the configuration object for the informer.
*
* @return the informer configuration object
*
* @deprecated Use {@link #configuration()} instead
*/
@Deprecated(forRemoval = true)
public InformerConfiguration<R> getConfiguration() {
return configuration;
return configuration();
}

@Override
Expand Down Expand Up @@ -334,7 +334,7 @@ public synchronized void cleanupOnCreateOrUpdateEventFiltering(ResourceID resour

@Override
public boolean allowsNamespaceChanges() {
return getConfiguration().followControllerNamespaceChanges();
return configuration().followControllerNamespaceChanges();
}


Expand Down Expand Up @@ -364,7 +364,7 @@ private boolean acceptedByDeleteFilters(R resource, boolean b) {
public void setConfigurationService(ConfigurationService configurationService) {
super.setConfigurationService(configurationService);

cache.addIndexers(indexerBuffer);
super.addIndexers(indexerBuffer);
indexerBuffer = new HashMap<>();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
package io.javaoperatorsdk.operator.processing.event.source.informer;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Function;
import java.util.function.Predicate;
Expand All @@ -23,7 +18,6 @@
import io.fabric8.kubernetes.client.informers.ResourceEventHandler;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.Cloner;
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
import io.javaoperatorsdk.operator.api.config.ResourceConfiguration;
import io.javaoperatorsdk.operator.health.InformerHealthIndicator;
Expand All @@ -40,19 +34,21 @@ public class InformerManager<T extends HasMetadata, C extends ResourceConfigurat
private static final Logger log = LoggerFactory.getLogger(InformerManager.class);

private final Map<String, InformerWrapper<T>> sources = new ConcurrentHashMap<>();
private Cloner cloner;
private final C configuration;
private final MixedOperation<T, KubernetesResourceList<T>, Resource<T>> client;
private final ResourceEventHandler<T> eventHandler;
private final Map<String, Function<T, List<String>>> indexers = new HashMap<>();
private final ConfigurationService configurationService;
private ConfigurationService configurationService;

public InformerManager(MixedOperation<T, KubernetesResourceList<T>, Resource<T>> client,
C configuration, ConfigurationService configurationService,
InformerManager(MixedOperation<T, KubernetesResourceList<T>, Resource<T>> client,
C configuration,
ResourceEventHandler<T> eventHandler) {
this.client = client;
this.configuration = configuration;
this.eventHandler = eventHandler;
}

void setConfigurationService(ConfigurationService configurationService) {
this.configurationService = configurationService;
}

Expand All @@ -74,7 +70,6 @@ private void initSources() {
if (!sources.isEmpty()) {
throw new IllegalStateException("Some sources already initialized.");
}
cloner = configurationService.getResourceCloner();
final var targetNamespaces = configuration.getEffectiveNamespaces(configurationService);
if (ResourceConfiguration.allNamespacesWatched(targetNamespaces)) {
var source = createEventSourceForNamespace(WATCH_ALL_NAMESPACES);
Expand Down Expand Up @@ -175,7 +170,7 @@ public Stream<T> list(String namespace, Predicate<T> predicate) {
public Optional<T> get(ResourceID resourceID) {
return getSource(resourceID.getNamespace().orElse(WATCH_ALL_NAMESPACES))
.flatMap(source -> source.get(resourceID))
.map(cloner::clone);
.map(r -> configurationService.getResourceCloner().clone(r));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,17 @@ public abstract class ManagedInformerEventSource<R extends HasMetadata, P extend
InformerWrappingEventSourceHealthIndicator<R>, Configurable<C> {

private static final Logger log = LoggerFactory.getLogger(ManagedInformerEventSource.class);
private final InformerManager<R, C> cache;

protected TemporaryResourceCache<R> temporaryResourceCache;
protected InformerManager<R, C> cache;
protected C configuration;
protected MixedOperation<R, KubernetesResourceList<R>, Resource<R>> client;

protected ManagedInformerEventSource(
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> client, C configuration) {
super(configuration.getResourceClass());
this.client = client;
temporaryResourceCache = new TemporaryResourceCache<>(this);
this.configuration = configuration;
this.cache = new InformerManager<>(client, configuration, this);
}

@Override
Expand Down Expand Up @@ -128,7 +127,9 @@ void setTemporalResourceCache(TemporaryResourceCache<R> temporaryResourceCache)
this.temporaryResourceCache = temporaryResourceCache;
}

public abstract void addIndexers(Map<String, Function<R, List<String>>> indexers);
public void addIndexers(Map<String, Function<R, List<String>>> indexers) {
cache.addIndexers(indexers);
}

public List<R> byIndex(String indexName, String indexKey) {
return manager().byIndex(indexName, indexKey);
Expand Down Expand Up @@ -156,7 +157,7 @@ public Status getStatus() {

@Override
public ResourceConfiguration<R> getInformerConfiguration() {
return configuration;
return configuration();
}

@Override
Expand All @@ -167,11 +168,11 @@ public C configuration() {
@Override
public String toString() {
return getClass().getSimpleName() + "{" +
"resourceClass: " + configuration.getResourceClass().getSimpleName() +
"resourceClass: " + configuration().getResourceClass().getSimpleName() +
"}";
}

public void setConfigurationService(ConfigurationService configurationService) {
cache = new InformerManager<>(client, configuration, configurationService, this);
cache.setConfigurationService(configurationService);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,8 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

@SuppressWarnings({"rawtypes", "unchecked"})
class EventSourceManagerTest {
Expand Down Expand Up @@ -166,7 +158,7 @@ void changesNamespacesOnControllerAndInformerEventSources() {
when(informerConfigurationMock.followControllerNamespaceChanges()).thenReturn(true);
InformerEventSource informerEventSource = mock(InformerEventSource.class);
when(informerEventSource.resourceType()).thenReturn(TestCustomResource.class);
when(informerEventSource.getConfiguration()).thenReturn(informerConfigurationMock);
when(informerEventSource.configuration()).thenReturn(informerConfigurationMock);
when(informerEventSource.allowsNamespaceChanges()).thenCallRealMethod();
manager.registerEventSource("ies", informerEventSource);

Expand Down