Skip to content

Improvements for Quarkus extension #1043

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 10 commits into from
Mar 18, 2022
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
package io.javaoperatorsdk.operator.api.config;

import java.lang.reflect.InvocationTargetException;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.CustomResource;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.api.monitoring.Metrics;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand All @@ -19,6 +22,7 @@ public interface ConfigurationService {
ObjectMapper OBJECT_MAPPER = new ObjectMapper();

Cloner DEFAULT_CLONER = new Cloner() {
@SuppressWarnings("unchecked")
@Override
public HasMetadata clone(HasMetadata object) {
try {
Expand Down Expand Up @@ -126,4 +130,14 @@ default boolean closeClientOnStop() {
default ObjectMapper getObjectMapper() {
return OBJECT_MAPPER;
}

default <T extends DependentResource<?, ?>> T createFrom(DependentResourceSpec<T, ?> spec) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels a little weird, that a configuration service is responsible for instantiating a Dependent Resource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, would be better to add a factory if we want to go that way. Putting it directly on ConfigurationService was a shortcut.

try {
return spec.getDependentResourceClass().getConstructor().newInstance();
} catch (InstantiationException | NoSuchMethodException | IllegalAccessException
| InvocationTargetException e) {
throw new IllegalArgumentException("Cannot instantiate DependentResource "
+ spec.getDependentResourceClass().getCanonicalName(), e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ private RecentOperationCacheFiller<R> eventSourceAsRecentOperationCacheFiller()
return (RecentOperationCacheFiller<R>) ((EventSourceProvider<P>) this).getEventSource();
}

@SuppressWarnings("unchecked")
// this cannot be done in constructor since event source might be initialized later
protected boolean isFilteringEventSource() {
if (this instanceof EventSourceProvider) {
Expand All @@ -135,6 +136,7 @@ protected boolean isFilteringEventSource() {
}
}

@SuppressWarnings("unchecked")
// this cannot be done in constructor since event source might be initialized later
protected boolean isRecentOperationCacheFiller() {
if (this instanceof EventSourceProvider) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

public interface ResourceTypeAware<R> {

Class<R> resourceType();
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package io.javaoperatorsdk.operator.processing.dependent;

import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
Expand All @@ -10,6 +9,7 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.api.reconciler.Context;
Expand Down Expand Up @@ -85,25 +85,19 @@ private void initContextIfNeeded(P resource, Context context) {

private DependentResource createAndConfigureFrom(DependentResourceSpec dependentResourceSpec,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be move to controller ctor and passed to DependentResourceManager ctor ? It seems weird that dependent are created during prepareEventSources especially when it is not an EventSourceProvider and it feels like an inversion of control should better. This may avoid the call to ConfigurationServiceProvider deep in the stack.

Copy link
Collaborator Author

@metacosm metacosm Mar 17, 2022

Choose a reason for hiding this comment

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

Not sure I'm following. It makes sense to initialise the DependentResource implementations in prepareEventSources because DependentResources are more or less abstractions over EventSources. This also avoids iterating over the dependents several times (once to create them, another to initialize the event sources for the EventSourceProvider implementations).

That said, I'm open to alternatives.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I'm following. It makes sense to initialise the DependentResource implementations in prepareEventSources because DependentResources are more or less abstractions over EventSources. This also avoids iterating over the dependents several times (once to create them, another to initialize the event sources for the EventSourceProvider implementations).

That said, I'm open to alternatives.

Looping over a very small list at startup time doesn't worry me that much.

Looking at https://github.com/java-operator-sdk/java-operator-sdk/blob/4b3178f6546494faff048b9dcef1e00244586fe3/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconcilerDependentResources.java#L40-L42, the natural choice to instanciate the dependent resources in standalone mode is in the Reconciler ctor. I would expect that the framework is consistent and that the depedent are created with the nature of a dependent resource, they should have been be instantiated in the prepareEventSource too.

My POV is that DependentResourceManager is about managing DependentResource not DependentResourceSpec. The latter is a concern of the configuration of the Controller.
DependentManager receive a Controller instance in the ctor and has access to controller configuration and reconciler while all it needs is a list of DependentResource instance. The initContextIfNeeded could be moved to Controller to avoid the need of reconciler. BTW, there is a side effect of calling initContext from DependentResourceManager that make reconciler implementing ContextInitializer without dependent annotation works as it should. Not sure if ContextInitializer was introduce with depedents.
I think that when it will start dealing with dependent of dependents, the complexity of DependentResourceManager will raise and it will no more be much SRP compliant.

That being said, I checked the code and there is something fishy regarding valid lifetime of DependentResource instance and this will drive when and where it should be instanciated.

Operator::start will prepareEventSource and start the DependentResource event source. Then, Operator::stop will stop the event source. It means that if DependentResource implementing EventSourceProvider were Singleton, their event soure must be restartable. But k8s informer are not restartable, thus KubernetesDependentResource can not be a Singleton. Until now, this was not an issue because prepareEventSource called during start is actually creating new instance of DependentResource each time in createAndConfigureFrom. But it feels natural to have @ApplicationScoped/@Singleton dependent resource in Quarkus. It also told us that sample WebPageReconciler will fail if it is start, stop and start (in a test suite for example).
I think that it should be documented that EventSourceInitializer::prepareEventSource must return either restartable EventSource (should support start/stop/start/... sequence) or return transient instance.
Or cleaner imho, find a way to make KubernetesDependentResource event source restartable but this seems not that simple.

As Controller is LifecycleAware, it make sense to me that it take the ownership of DependentResource instances. For eg, if the DependentResourceSpec is not empty it can create them in the start method and then new DependentResourceManager(dependents). And if the DependentResourceSpec is empty, it can new a noop manager to optimize calls.

The last thing is that I don't know how it can ensure EventSource are restartable for DependentResource singleton. Should the Quarkus extension check and forbid @ApplicationScoped/@Singleton ?

sorry for the long story

Copy link
Collaborator

Choose a reason for hiding this comment

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

Operator::start will prepareEventSource and start the DependentResource event source. Then, Operator::stop will stop the event source. It means that if DependentResource implementing EventSourceProvider were Singleton, their event soure must be restartable.

I don't understand this, why would you restart the operator / event source, or in what scenario event source needs to be restartable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a lot to unpack there. I'm not sure we should worry about sources being restartable at this point. The main use case is still an operator that is started once when it is starting up and then stop when it's about to be shutdown. I don't think it makes sense to start/stop the operator multiple times during normal operations.

Regarding where DependentResource instances should be created, I'm going to look at the implications of moving that part to Controller but at this point, I'd rather leave things as-is so that we can release and get some user feedback (unless there's a blocking issue that we need to deal with now). As these changes seem to be related to the internal implementation, that's probably a change we can make in a later iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes sense to start/stop the operator multiple times during normal operations.

In normal operation, I totally agree. In test suite, this is exactly what does the JUnit Operator extension. It start/stop the same operator instance for each test. I just wanted to warn about that because sample operator are not representative of fully tested operators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Let's revisit if that becomes an issue.

KubernetesClient client) {
try {
DependentResource dependentResource =
(DependentResource) dependentResourceSpec.getDependentResourceClass()
.getConstructor().newInstance();
DependentResource dependentResource =
ConfigurationServiceProvider.instance().createFrom(dependentResourceSpec);

if (dependentResource instanceof KubernetesClientAware) {
((KubernetesClientAware) dependentResource).setKubernetesClient(client);
}
if (dependentResource instanceof KubernetesClientAware) {
((KubernetesClientAware) dependentResource).setKubernetesClient(client);
}

if (dependentResource instanceof DependentResourceConfigurator) {
final var configurator = (DependentResourceConfigurator) dependentResource;
dependentResourceSpec.getDependentResourceConfiguration()
.ifPresent(configurator::configureWith);
}
return dependentResource;
} catch (InstantiationException | NoSuchMethodException | IllegalAccessException
| InvocationTargetException e) {
throw new IllegalStateException(e);
if (dependentResource instanceof DependentResourceConfigurator) {
final var configurator = (DependentResourceConfigurator) dependentResource;
dependentResourceSpec.getDependentResourceConfiguration()
.ifPresent(configurator::configureWith);
}
return dependentResource;
}

public List<DependentResource> getDependents() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,22 @@
import java.util.Optional;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.Utils;
import io.javaoperatorsdk.operator.api.reconciler.dependent.AbstractDependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceTypeAware;
import io.javaoperatorsdk.operator.processing.event.ExternalResourceCachingEventSource;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;

public abstract class AbstractCachingDependentResource<R, P extends HasMetadata>
extends AbstractDependentResource<R, P> implements EventSourceProvider<P> {
extends AbstractDependentResource<R, P>
implements EventSourceProvider<P>, ResourceTypeAware<R> {

protected ExternalResourceCachingEventSource<R, P> eventSource;
private final Class<R> resourceType;

protected AbstractCachingDependentResource(Class<R> resourceType) {
this.resourceType = resourceType;
}

public Optional<R> fetchResource(P primaryResource) {
return eventSource.getAssociated(primaryResource);
Expand All @@ -23,8 +29,9 @@ public EventSource getEventSource() {
return eventSource;
}

protected Class<R> resourceType() {
return (Class<R>) Utils.getFirstTypeArgumentFromExtendedClass(getClass());
@Override
public Class<R> resourceType() {
return resourceType;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package io.javaoperatorsdk.operator.processing.dependent.external;

import io.fabric8.kubernetes.api.model.HasMetadata;

public abstract class AbstractPollingDependentResource<R, P extends HasMetadata>
extends AbstractCachingDependentResource<R, P> {

public static final int DEFAULT_POLLING_PERIOD = 5000;
private long pollingPeriod;

protected AbstractPollingDependentResource(Class<R> resourceType) {
this(resourceType, DEFAULT_POLLING_PERIOD);
}

public AbstractPollingDependentResource(Class<R> resourceType, long pollingPeriod) {
super(resourceType);
this.pollingPeriod = pollingPeriod;
}

public void setPollingPeriod(long pollingPeriod) {
this.pollingPeriod = pollingPeriod;
}

public long getPollingPeriod() {
return pollingPeriod;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,36 +5,21 @@
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
import io.javaoperatorsdk.operator.processing.event.source.polling.PerResourcePollingEventSource;

import static io.javaoperatorsdk.operator.processing.dependent.external.PollingDependentResource.DEFAULT_POLLING_PERIOD;

public abstract class PerResourcePollingDependentResource<R, P extends HasMetadata>
extends AbstractCachingDependentResource<R, P>
extends AbstractPollingDependentResource<R, P>
implements PerResourcePollingEventSource.ResourceFetcher<R, P> {

protected long pollingPeriod;

public PerResourcePollingDependentResource() {
this(DEFAULT_POLLING_PERIOD);
public PerResourcePollingDependentResource(Class<R> resourceType) {
super(resourceType);
}

public PerResourcePollingDependentResource(long pollingPeriod) {
this.pollingPeriod = pollingPeriod;
public PerResourcePollingDependentResource(Class<R> resourceType, long pollingPeriod) {
super(resourceType, pollingPeriod);
}

@Override
public EventSource initEventSource(EventSourceContext<P> context) {
eventSource = new PerResourcePollingEventSource<>(this, context.getPrimaryCache(),
pollingPeriod, resourceType());
getPollingPeriod(), resourceType());
return eventSource;
}

public PerResourcePollingDependentResource<R, P> setPollingPeriod(long pollingPeriod) {
this.pollingPeriod = pollingPeriod;
return this;
}

public long getPollingPeriod() {
return pollingPeriod;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,19 @@
import io.javaoperatorsdk.operator.processing.event.source.polling.PollingEventSource;

public abstract class PollingDependentResource<R, P extends HasMetadata>
extends AbstractCachingDependentResource<R, P> implements Supplier<Map<ResourceID, R>> {
extends AbstractPollingDependentResource<R, P> implements Supplier<Map<ResourceID, R>> {

public static final int DEFAULT_POLLING_PERIOD = 5000;
protected long pollingPeriod;

public PollingDependentResource() {
this(DEFAULT_POLLING_PERIOD);
public PollingDependentResource(Class<R> resourceType) {
super(resourceType);
}

public PollingDependentResource(long pollingPeriod) {
this.pollingPeriod = pollingPeriod;
public PollingDependentResource(Class<R> resourceType, long pollingPeriod) {
super(resourceType, pollingPeriod);
}

@Override
public EventSource initEventSource(EventSourceContext<P> context) {
eventSource = new PollingEventSource<>(this, pollingPeriod, resourceType());
eventSource = new PollingEventSource<>(this, getPollingPeriod(), resourceType());
return eventSource;
}

public PollingDependentResource<R, P> setPollingPeriod(long pollingPeriod) {
this.pollingPeriod = pollingPeriod;
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
* @param <P> Primary Resource
*/
public abstract class CrudKubernetesDependentResource<R extends HasMetadata, P extends HasMetadata>
extends
KubernetesDependentResource<R, P> implements Creator<R, P>, Updater<R, P>, Deleter<P> {
extends KubernetesDependentResource<R, P>
implements Creator<R, P>, Updater<R, P>, Deleter<P> {

public CrudKubernetesDependentResource(Class<R> resourceType) {
super(resourceType);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.javaoperatorsdk.operator.api.config.Utils;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
Expand All @@ -21,6 +20,7 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.KubernetesClientAware;
import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher;
import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher.Result;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceTypeAware;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceUpdatePreProcessor;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
Expand All @@ -31,7 +31,7 @@

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

private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class);
Expand All @@ -41,15 +41,17 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
private boolean addOwnerReference;
private final Matcher<R, P> matcher;
private final ResourceUpdatePreProcessor<R> processor;
private final Class<R> resourceType;

@SuppressWarnings("unchecked")
public KubernetesDependentResource() {
public KubernetesDependentResource(Class<R> resourceType) {
this.resourceType = resourceType;
matcher = this instanceof Matcher ? (Matcher<R, P>) this
: GenericKubernetesResourceMatcher.matcherFor(resourceType(), this);
: GenericKubernetesResourceMatcher.matcherFor(resourceType, this);

processor = this instanceof ResourceUpdatePreProcessor
? (ResourceUpdatePreProcessor<R>) this
: GenericResourceUpdatePreProcessor.processorFor(resourceType());
: GenericResourceUpdatePreProcessor.processorFor(resourceType);
}

@Override
Expand Down Expand Up @@ -140,9 +142,9 @@ public KubernetesDependentResource<R, P> setInformerEventSource(
return this;
}

@SuppressWarnings("unchecked")
protected Class<R> resourceType() {
return (Class<R>) Utils.getFirstTypeArgumentFromExtendedClass(getClass());
@Override
public Class<R> resourceType() {
return resourceType;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
package io.javaoperatorsdk.operator.api.config;

import java.util.Optional;

import org.junit.jupiter.api.Test;

import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

class UtilsTest {

Expand Down Expand Up @@ -75,12 +82,32 @@ void getsFirstTypeArgumentFromExtendedClass() {
assertThat(res).isEqualTo(Deployment.class);
}

public static class TestKubernetesDependentResource
extends KubernetesDependentResource<Deployment, TestCustomResource> {
@Test
void getsFirstTypeArgumentFromInterface() {
assertThat(Utils.getFirstTypeArgumentFromInterface(TestDependentResource.class))
.isEqualTo(Deployment.class);
}

public static class TestDependentResource
implements DependentResource<Deployment, TestCustomResource> {

@Override
protected Deployment desired(TestCustomResource primary, Context<TestCustomResource> context) {
public ReconcileResult<Deployment> reconcile(TestCustomResource primary,
Context<TestCustomResource> context) {
return null;
}

@Override
public Optional<Deployment> getResource(TestCustomResource primaryResource) {
return Optional.empty();
}
}

public static class TestKubernetesDependentResource
extends KubernetesDependentResource<Deployment, TestCustomResource> {

public TestKubernetesDependentResource() {
super(Deployment.class);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void checksIfDesiredValuesAreTheSame() {
var actual = createDeployment();
final var desired = createDeployment();
final var matcher = GenericKubernetesResourceMatcher.matcherFor(Deployment.class,
new KubernetesDependentResource<>() {
new KubernetesDependentResource<>(Deployment.class) {
@Override
protected Deployment desired(HasMetadata primary, Context context) {
final var currentCase = Optional.ofNullable(primary)
Expand Down
Loading