-
Notifications
You must be signed in to change notification settings - Fork 218
feat: share InformerManager when configuration is equal #954
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,10 @@ | ||
package io.javaoperatorsdk.operator.processing.event.source.informer; | ||
|
||
import java.util.Objects; | ||
import java.util.Set; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.ConcurrentMap; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.fabric8.kubernetes.api.model.KubernetesResourceList; | ||
import io.fabric8.kubernetes.client.dsl.MixedOperation; | ||
|
@@ -13,15 +18,31 @@ public abstract class ManagedInformerEventSource<R extends HasMetadata, P extend | |
extends CachingEventSource<R, P> | ||
implements ResourceEventHandler<R> { | ||
|
||
@SuppressWarnings("rawtypes") | ||
private static final ConcurrentMap<ResourceConfigurationAsKey, InformerManager> managedInformers = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH, don't know what to think about this static context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be started and stopped multiple times. Don't like the facts that it is even shared between the controllers. It will be started and stopped multiple times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it would deserve a Integration test with multiple controllers and multiple informers each. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, you're the one who wanted to share informers between sources… :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls add integration test for this, like when 2 reconcilers added with 2 Informers, so there is only one informer in the background. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And checking that everything is propoerly triggered after resource updated. |
||
new ConcurrentHashMap<>(); | ||
|
||
protected ManagedInformerEventSource( | ||
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> client, C configuration) { | ||
super(configuration.getResourceClass()); | ||
initCache(configuration); | ||
manager().initSources(client, configuration, this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this multiple time init sources? for the manager? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ehh we need a quite good quality intergation tests here IMHO. This is quite risky to do this way. |
||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private void initCache(C configuration) { | ||
final var key = new ResourceConfigurationAsKey(configuration); | ||
var existing = managedInformers.get(key); | ||
if (existing == null) { | ||
existing = new InformerManager<>(); | ||
managedInformers.put(key, existing); | ||
} | ||
cache = existing; | ||
} | ||
|
||
@Override | ||
protected UpdatableCache<R> initCache() { | ||
return new InformerManager<>(); | ||
return null; // cache needs the configuration to be properly initialized | ||
} | ||
|
||
protected InformerManager<R, C> manager() { | ||
|
@@ -30,13 +51,63 @@ protected InformerManager<R, C> manager() { | |
|
||
@Override | ||
public void start() { | ||
manager().start(); | ||
super.start(); | ||
if (!isRunning()) { | ||
manager().start(); | ||
super.start(); | ||
} | ||
} | ||
|
||
@Override | ||
public void stop() { | ||
super.stop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about the thread safety but probably ok this way |
||
manager().stop(); | ||
if (isRunning()) { | ||
super.stop(); | ||
manager().stop(); | ||
} | ||
} | ||
|
||
@SuppressWarnings("rawtypes") | ||
private static class ResourceConfigurationAsKey { | ||
private final ResourceConfiguration configuration; | ||
|
||
private ResourceConfigurationAsKey(ResourceConfiguration configuration) { | ||
this.configuration = configuration; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
|
||
final var that = (ResourceConfigurationAsKey) o; | ||
if (configuration == that.configuration) { | ||
return true; | ||
} | ||
|
||
return Objects.equals(getLabelSelector(), that.getLabelSelector()) | ||
&& Objects.equals(getResourceClass(), that.getResourceClass()) | ||
&& Objects.equals(getNamespaces(), that.getNamespaces()); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(getLabelSelector(), getResourceClass(), getNamespaces()); | ||
} | ||
|
||
public String getLabelSelector() { | ||
return configuration.getLabelSelector(); | ||
} | ||
|
||
public Class getResourceClass() { | ||
return configuration.getResourceClass(); | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
public Set<String> getNamespaces() { | ||
return configuration.getNamespaces(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package io.javaoperatorsdk.operator.processing.event.source.informer; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.MockKubernetesClient; | ||
import io.javaoperatorsdk.operator.api.config.ConfigurationService; | ||
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
import static org.mockito.Mockito.mock; | ||
|
||
@SuppressWarnings({"rawtypes", "unchecked"}) | ||
public class ManagedInformerEventSourceTest { | ||
@Test | ||
void sourcesWithSameConfigurationShouldShareInformer() { | ||
final var namespaces = new String[] {"foo", "bar"}; | ||
final var config = InformerConfiguration.from(mock(ConfigurationService.class), | ||
HasMetadata.class) | ||
.withLabelSelector("label=value").withNamespaces(namespaces) | ||
.build(); | ||
|
||
final var informer1 = | ||
new InformerEventSource<>(config, MockKubernetesClient.client(HasMetadata.class)); | ||
final var informer2 = | ||
new InformerEventSource<>(config, MockKubernetesClient.client(HasMetadata.class)); | ||
|
||
final var manager = informer1.manager(); | ||
assertEquals(manager, informer2.manager()); | ||
assertEquals(namespaces.length, manager.numberOfSources()); | ||
} | ||
|
||
@Test | ||
void sourcesWithDifferentConfigurationsShouldNotShareInformer() { | ||
final var config1 = InformerConfiguration.from(mock(ConfigurationService.class), | ||
HasMetadata.class) | ||
.withLabelSelector("label=value").withNamespaces("foo", "bar") | ||
.build(); | ||
|
||
final var config2 = InformerConfiguration.from(config1) | ||
.withLabelSelector("label=otherValue").withNamespaces("baz") | ||
.build(); | ||
|
||
final var informer1 = | ||
new InformerEventSource<>(config1, MockKubernetesClient.client(HasMetadata.class)); | ||
final var informer2 = | ||
new InformerEventSource<>(config2, MockKubernetesClient.client(HasMetadata.class)); | ||
|
||
assertNotEquals(informer1.manager(), informer2.manager()); | ||
assertEquals(1, informer2.manager().numberOfSources()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just return an immutable map, this is not very useful outside of the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed just for tests, which is why it's package-protected.