Skip to content

Extract interfaces for optional behavior of DependentResource #949

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.

Sign up for GitHub

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 5 commits into from
Feb 17, 2022
Merged

Extract interfaces for optional behavior of DependentResource #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
734577a
feat: extract EventSourceProvider interface to clean up common use case
metacosm Feb 16, 2022
fb1ad88
feat: extract DependentResourceConfigurator interface
metacosm Feb 16, 2022
cdb66c3
fix: provide configuration via interface on reconciler
metacosm Feb 16, 2022
c050d2f
revert: "fix: provide configuration via interface on reconciler"
metacosm Feb 17, 2022
07032b6
feat: change default configuration
metacosm Feb 17, 2022
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
5 changes: 3 additions & 2 deletions operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public <R extends HasMetadata> void register(Reconciler<R> reconciler,
* @param <R> the {@code HasMetadata} type associated with the reconciler
*/
public <R extends HasMetadata> void register(Reconciler<R> reconciler,
Consumer<ControllerConfigurationOverrider> configOverrider) {
Consumer<ControllerConfigurationOverrider<R>> configOverrider) {
final var controllerConfiguration = configurationService.getConfigurationFor(reconciler);
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 relied on the fact that the default ConfigurationService implementation will automatically create the configuration associated with the reconciler if it doesn't exist. However, in the general case, this would actually throw an NPE as this would return null: https://github.com/java-operator-sdk/java-operator-sdk/blob/323372a2e7b5b05f25a3af1162b420d5d95c19e5/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java#L54-L62

Copy link
Collaborator

Choose a reason for hiding this comment

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

The default is to add AnnotationControllerConfiguration, see:
http://github.com/java-operator-sdk/java-operator-sdk/blob/dbf6c2e001ff5bba701f231babc9affc8c7433a7/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationService.java#L33-L33

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the other changes, but have basically conceptual problems with this, using DependentResourceConfigurationProvider makes the whole Annotation based Dependent Resource definition kinda loosing it's meaning.
So from now in the background what will happen is just the DependentResource will be instantiated (with constaint of empty constructor present), and the the configuration that is explicitly implemented in the reconciler is passed to it.

I really liked that the configuration of the dependent resources is decoupled from the reconciler, since it makes then the reconciler much cleaner. Now rather I would then do just standalone DependentResources instead of this, when implementing an operator. Since we loose even that decoupling.

(And of course the reconcile and prepare event sources will be called explicitly in the background but that is again not a huge benefit and rather could be confusing / hard to understand for people who start with. So has negative sides too)

So I think we should either:

  1. Fix the configuraiton with using ControllerConfiguration mechanism
  2. Or just drop the whole annotation based DependentResource definitions (/managed dependent resources) since it is starts loosing it's benefit, and seems to be too hard and even confusing to implement / work with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The default is to add AnnotationControllerConfiguration, see: http://github.com/java-operator-sdk/java-operator-sdk/blob/dbf6c2e001ff5bba701f231babc9affc8c7433a7/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationService.java#L33-L33

checked, this works no on next branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

An alternative what a I see is to pass this on registration:
operator.register(controller, cofiguration, dependentResourceConfiguration) and
operator.register(controller, dependentResourceConfiguration)

So decouple it from controller configuration.

var configToOverride = ControllerConfigurationOverrider.override(controllerConfiguration);
configOverrider.accept(configToOverride);
Expand Down Expand Up @@ -195,7 +195,8 @@ public synchronized void stop() {
started = false;
}

public synchronized void add(Controller controller) {
@SuppressWarnings("unchecked")
synchronized void add(Controller controller) {
final var configuration = controller.getConfiguration();
final var resourceTypeName = ReconcilerUtils
.getResourceTypeNameWithVersion(configuration.getResourceClass());
Expand Down
5 changes: 3 additions & 2 deletions ...rc/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;

@SuppressWarnings({"rawtypes", "unchecked", "unused"})
public class ControllerConfigurationOverrider<R extends HasMetadata> {

private String finalizer;
Expand All @@ -21,7 +22,7 @@ public class ControllerConfigurationOverrider<R extends HasMetadata> {
private ResourceEventFilter<R> customResourcePredicate;
private final ControllerConfiguration<R> original;
private Duration reconciliationMaxInterval;
private List<DependentResourceSpec> dependentResourceSpecs;
private final List<DependentResourceSpec> dependentResourceSpecs;

private ControllerConfigurationOverrider(ControllerConfiguration<R> original) {
finalizer = original.getFinalizer();
Expand Down Expand Up @@ -89,7 +90,7 @@ public ControllerConfigurationOverrider<R> withReconciliationMaxInterval(
}

public void replaceDependentResourceConfig(
Class<? extends DependentResource<?, R, ?>> dependentResourceClass,
Class<? extends DependentResource<?, R>> dependentResourceClass,
Object dependentResourceConfig) {

var currentConfig =
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/io/javaoperatorsdk/operator/api/config/dependent/DependentResourceSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;

public class DependentResourceSpec<T extends DependentResource<?, ?, C>, C> {
public class DependentResourceSpec<T extends DependentResource<?, ?>, C> {

private final Class<T> dependentResourceClass;

Expand Down
2 changes: 1 addition & 1 deletion .../java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;

public abstract class AbstractDependentResource<R, P extends HasMetadata, C>
implements DependentResource<R, P, C> {
implements DependentResource<R, P> {

@Override
public void reconcile(P primary, Context context) {
Expand Down
9 changes: 1 addition & 8 deletions ...src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,11 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;

public interface DependentResource<R, P extends HasMetadata, C> {

Optional<EventSource> eventSource(EventSourceContext<P> context);

public interface DependentResource<R, P extends HasMetadata> {
void reconcile(P primary, Context context);

void delete(P primary, Context context);

Optional<R> getResource(P primaryResource);

default void configureWith(C config) {}
}
5 changes: 5 additions & 0 deletions ...a/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

public interface DependentResourceConfigurator<C> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

void configureWith(C config);
}
10 changes: 10 additions & 0 deletions ...c/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;

public interface EventSourceProvider<P extends HasMetadata> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1


EventSource eventSource(EventSourceContext<P> context);
}
21 changes: 14 additions & 7 deletions .../main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceConfigurator;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider;
import io.javaoperatorsdk.operator.api.reconciler.dependent.KubernetesClientAware;
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
Expand Down Expand Up @@ -49,11 +51,11 @@ public List<EventSource> prepareEventSources(EventSourceContext<P> context) {
dependentResources.stream()
.map(
drc -> {
final var dependentResource =
createAndConfigureFrom(drc, context.getClient());
dependentResource
.eventSource(context)
.ifPresent(es -> sources.add((EventSource) es));
final var dependentResource = createAndConfigureFrom(drc, context.getClient());
if (dependentResource instanceof EventSourceProvider) {
EventSourceProvider provider = (EventSourceProvider) dependentResource;
sources.add(provider.eventSource(context));
}
return dependentResource;
})
.collect(Collectors.toList());
Expand Down Expand Up @@ -87,11 +89,16 @@ private DependentResource createAndConfigureFrom(DependentResourceSpec dependent
DependentResource dependentResource =
(DependentResource) dependentResourceSpec.getDependentResourceClass()
.getConstructor().newInstance();

if (dependentResource instanceof KubernetesClientAware) {
((KubernetesClientAware) dependentResource).setKubernetesClient(client);
}
dependentResourceSpec.getDependentResourceConfiguration()
.ifPresent(dependentResource::configureWith);

if (dependentResource instanceof DependentResourceConfigurator) {
final var configurator = (DependentResourceConfigurator) dependentResource;
dependentResourceSpec.getDependentResourceConfiguration()
.ifPresent(configurator::configureWith);
}
return dependentResource;
} catch (InstantiationException | NoSuchMethodException | IllegalAccessException
| InvocationTargetException e) {
Expand Down
11 changes: 7 additions & 4 deletions ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.dependent.AbstractDependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceConfigurator;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider;
import io.javaoperatorsdk.operator.api.reconciler.dependent.KubernetesClientAware;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier;
Expand All @@ -24,7 +26,8 @@

public abstract class KubernetesDependentResource<R extends HasMetadata, P extends HasMetadata>
extends AbstractDependentResource<R, P, KubernetesDependentResourceConfig>
implements KubernetesClientAware {
implements KubernetesClientAware, EventSourceProvider<P>,
DependentResourceConfigurator<KubernetesDependentResourceConfig> {

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

Expand Down Expand Up @@ -61,7 +64,7 @@ private void configureWith(ConfigurationService configService, String labelSelec

/**
* Use to share informers between event more resources.
*
*
* @param configurationService get configs
* @param informerEventSource informer to use
* @param addOwnerReference to the created resource
Expand Down Expand Up @@ -108,15 +111,15 @@ protected R update(R actual, R target, P primary, Context context) {
}

@Override
public Optional<EventSource> eventSource(EventSourceContext<P> context) {
public EventSource eventSource(EventSourceContext<P> context) {
initResourceMatcherIfNotSet(context.getConfigurationService());
if (informerEventSource == null) {
configureWith(context.getConfigurationService(), null, null,
KubernetesDependent.ADD_OWNER_REFERENCE_DEFAULT);
log.warn("Using default configuration for " + resourceType().getSimpleName()
+ " KubernetesDependentResource, call configureWith to provide configuration");
}
return Optional.of(informerEventSource);
return informerEventSource;
}

public KubernetesDependentResource<R, P> setInformerEventSource(
Expand Down
2 changes: 1 addition & 1 deletion ...avaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public StandaloneDependentTestReconciler() {
@Override
public List<EventSource> prepareEventSources(
EventSourceContext<StandaloneDependentTestCustomResource> context) {
return List.of(configMapDependent.eventSource(context).get());
return List.of(configMapDependent.eventSource(context));
}

@Override
Expand Down
3 changes: 2 additions & 1 deletion ...rs/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ public static void main(String[] args) throws IOException {

MySQLSchemaReconciler schemaReconciler = new MySQLSchemaReconciler();

// override the default configuration
operator.register(schemaReconciler,
configOverrider -> configOverrider.replaceDependentResourceConfig(
SchemaDependentResource.class,
new ResourcePollerConfig(500, MySQLDbConfig.loadFromEnvironmentVars())));
new ResourcePollerConfig(300, MySQLDbConfig.loadFromEnvironmentVars())));
operator.installShutdownHook();
operator.start();

Expand Down
14 changes: 9 additions & 5 deletions ...ysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/SchemaDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,19 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.dependent.AbstractDependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceConfigurator;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
import io.javaoperatorsdk.operator.processing.event.source.polling.PerResourcePollingEventSource;
import io.javaoperatorsdk.operator.sample.schema.Schema;
import io.javaoperatorsdk.operator.sample.schema.SchemaService;

import static java.lang.String.format;

public class SchemaDependentResource extends
AbstractDependentResource<Schema, MySQLSchema, ResourcePollerConfig> {
public class SchemaDependentResource
extends AbstractDependentResource<Schema, MySQLSchema, ResourcePollerConfig>
implements EventSourceProvider<MySQLSchema>,
DependentResourceConfigurator<ResourcePollerConfig> {

private MySQLDbConfig dbConfig;
private int pollPeriod = 500;
Expand All @@ -28,13 +32,13 @@ public void configureWith(ResourcePollerConfig config) {
}

@Override
public Optional<EventSource> eventSource(EventSourceContext<MySQLSchema> context) {
public EventSource eventSource(EventSourceContext<MySQLSchema> context) {
if (dbConfig == null) {
dbConfig = MySQLDbConfig.loadFromEnvironmentVars();
}
return Optional.of(new PerResourcePollingEventSource<>(
return new PerResourcePollingEventSource<>(
new SchemaPollingResourceSupplier(dbConfig), context.getPrimaryCache(), pollPeriod,
Schema.class));
Schema.class);
}

@Override
Expand Down
9 changes: 4 additions & 5 deletions ...operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,10 @@ public WebPageReconciler(KubernetesClient kubernetesClient) {

@Override
public List<EventSource> prepareEventSources(EventSourceContext<WebPage> context) {
List<EventSource> eventSources = new ArrayList<>(3);
configMapDR.eventSource(context).ifPresent(eventSources::add);
deploymentDR.eventSource(context).ifPresent(eventSources::add);
serviceDR.eventSource(context).ifPresent(eventSources::add);
return eventSources;
return List.of(
configMapDR.eventSource(context),
deploymentDR.eventSource(context),
serviceDR.eventSource(context));
}

@Override
Expand Down