Improvements for Quarkus extension #1043
Conversation
DependentResources that implement this interface will get their resource type "injected" via a requirement to implement a single-arg public constructor.
This reverts commit 55a903a.
metacosm
changed the title
| @@ -85,25 +85,19 @@ private void initContextIfNeeded(P resource, Context context) { | |||
|
|
|||
| private DependentResource createAndConfigureFrom(DependentResourceSpec dependentResourceSpec, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not sure I'm following. It makes sense to initialise the
DependentResourceimplementations inprepareEventSourcesbecauseDependentResourcesare more or less abstractions overEventSources. This also avoids iterating over the dependents several times (once to create them, another to initialize the event sources for theEventSourceProviderimplementations).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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. Let's revisit if that becomes an issue.
| @@ -126,4 +130,14 @@ default boolean closeClientOnStop() { | |||
| default ObjectMapper getObjectMapper() { | |||
| return OBJECT_MAPPER; | |||
| } | |||
|
|
|||
| default <T extends DependentResource<?, ?>> T createFrom(DependentResourceSpec<T, ?> spec) { | |||
There was a problem hiding this comment.
This feels a little weird, that a configuration service is responsible for instantiating a Dependent Resource.
There was a problem hiding this comment.
True, would be better to add a factory if we want to go that way. Putting it directly on ConfigurationService was a shortcut.
|
Kudos, SonarCloud Quality Gate passed! |
DependentResourceinstantiation toConfigurationServiceso that it could be overridden by a different mechanism (in particular, CDI beans)DependentResourceimplementations that need it viaResourceTypeAwareinterface.