Extract controller configuration handling and provide Quarkus extension #238

metacosm · 2020-11-26T15:25:56Z

The idea is for Quarkus to be able to replace the default implementation by an implementation computed at build time.

s-soroosh · 2020-11-26T16:04:25Z

If I understand it correctly the intention is making the Quarkus native builds possible, right?
If so, I think we can support Quarkus without adding anything additional that is going to be exposed to the developer. otherwise would be great if you could elaborate its advantage.

metacosm · 2020-11-26T16:21:01Z

If I understand it correctly the intention is making the Quarkus native builds possible, right?

The intent is to be able to replace a lot (all) of what is currently done at runtime (e.g. annotation processing, reflection) by build time processing when running in a Quarkus environment.

If so, I think we can support Quarkus without adding anything additional that is going to be exposed to the developer. otherwise would be great if you could elaborate its advantage.

What do you mean by "exposed to the developer"? As things are, the intention is that things will keep working as they currently are when not using Quarkus but the configuration implementation will be swapped out by Quarkus.

s-soroosh · 2020-11-26T16:31:52Z

What do you mean by "exposed to the developer"? As things are, the intention is that things will keep working as they currently are when not using Quarkus but the configuration implementation will be swapped out by Quarkus.

The controller interface is having a new method called getConfiguration() as part of the interface.
I as a developer would rather to not expose to the stuff that is supposed to be handled by the framework and I should have nothing to deal with them.

I think we can achieve the same things just by giving a hint to the graalVM to keep the Reflection metadata in the image. By doing so we don't need to put the effort into having 2 different implementations.

metacosm · 2020-11-26T19:36:23Z

Note that this change is not Quarkus-specific: if we want to be able to externalize the configuration, we will need to provide different implementations of how the configuration is retrieved/computed. This is also helpful for Spring Boot or other frameworks for example, where each could provide their own way of retrieving the configuration.

If adding a getConfiguration method to the ResourceController interface is considered too intrusive, we could consider introducing an SPI that would need to be conformed to by implementation providers.

Another point that I think is worth mentioning: there's a lot more to Quarkus than native compilation. Its JVM mode also provides reduced memory size, faster startup, smaller binary size. I think it's worth making sure that we can support all these aspects and not simply focus on native compilation. In order to be able to support these other benefits, more work is needed, especially when things can be properly computed at build time instead of runtime. Registering things for reflection is quite costly even for native compilation.

kirek007 · 2020-11-27T12:42:21Z

I'm all in for replacing whole runtime logic with built time configuration.

metacosm · 2020-12-04T09:53:18Z

So things are getting interesting and I'm making some rather significant changes which I would like input on. The main idea behind these changes is to extract the parts that are done to make it easier to work with native compilation and isolate them in such a way that they can be used when needed but don't impact a future Quarkus extension. The parts in question are all the runtime configuration aspects along with the annotation processor and mapper parts as they would be not only be redundant with what Quarkus would be doing but actually be detrimental.

What do you think @adam-sandor @csviri @psycho-ir @kirek007?

s-soroosh · 2020-12-04T09:59:08Z

So things are getting interesting and I'm making some rather significant changes which I would like input on. The main idea behind these changes is to extract the parts that are done to make it easier to work with native compilation and isolate them in such a way that they can be used when needed but don't impact a future Quarkus extension. The parts in question are all the runtime configuration aspects along with the annotation processor and mapper parts as they would be not only be redundant with what Quarkus would be doing but actually be detrimental.

What do you think @adam-sandor @csviri @psycho-ir @kirek007?

Do you think it makes sense to write a one-pager doc about the change? I'm not sure about others but at least it would be very helpful for me and also can be added to the decision log or part of the SDK doc later on.

metacosm · 2020-12-04T13:31:53Z

@psycho-ir the changes are now made in this PR (though I still need to go through the tests to see if everything works as expected). The main idea as previously mentioned: there's a new runtime-configuration module that people are need to depend on to be able to use the current behavior with the annotation processing and the controller to CR class mapping. Basically, that module provides a default configuration service which extracts configuration from the Controller implementations at runtime or using annotation processing at built time.

Both approaches are discouraged when using Quarkus, which is why this PR makes that behavior optional so that other implementations such as one based on Quarkus, could implement a ConfigurationService that resolves the configuration some other way. This PR also adds a layer to be able to use external configuration of Controllers though it doesn't yet work completely as defined in #237.

The new approach is visible in the sample projects where projects now depend on the runtime-configuration module which they use by creating their Operator using the DefaultConfigurationService.instance() method. For Quarkus, the ConfigurationService is automatically injected. That's also the case for Spring Boot, though Spring Boot uses the current default configuration mechanism.

Of note, an api module was extracted from the operator-framework one so that both it and the runtime-configuration modules could depend on it without risking dependency cycles, though in the end, I'm not sure that was needed but I think it does make things a little cleaner.

metacosm · 2020-12-04T23:12:44Z

Quarkus extension is not finished yet but I'd like to get some feedback on the rest of the changes.

metacosm · 2020-12-17T14:07:19Z

Seems like the issue with the tests is the fact that the sample classes are currently in the operator-framework-core module which doesn't have the annotation processor, resulting in the classes not being registered to be later found.

The purpose is to be able to use a different implementation depending on the execution context. For example, a Quarkus-specific implementation would process the annotation at build time instead of doing so at runtime.

operator-framework module now also creates a jar out of its test classes for reuse purposes. This should probably be extracted more cleanly at some point.

This is the module that should now be imported by non-Quarkus apps

The issue is that since the sample code was living under the operator-framework-core module, which doesn't include the annotation processor, the controller implementations were not processed and thus not found by the runtime when needed. This should, hopefully, address the issue.

s-soroosh · 2020-12-17T17:23:22Z

...ator-framework/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java

@@ -18,6 +18,6 @@ public void returnsLatestOfEventType() {
        new EventList(
            Arrays.asList(mock(Event.class), new TimerEvent("2", null), event2, mock(Event.class)));

-    assertThat(eventList.getLatestOfType(TimerEvent.class).get()).isSameInstanceAs(event2);
+    Assertions.assertThat(eventList.getLatestOfType(TimerEvent.class).get()).isEqualTo(event2);


IMO the static import makes it cleaner

s-soroosh · 2020-12-17T17:37:15Z

...src/main/java/io/javaoperatorsdk/quarkus/extension/deployment/QuarkusExtensionProcessor.java

+                AnnotationValue::asBoolean,
+                () -> true),
+            valueOrDefault(
+                controllerAnnotation, "isClusterScoped", AnnotationValue::asBoolean, () -> false),


is isClusterScoped used anywhere?

It's used when the controller is registered with the Operator to know under which scope to register (namespace vs. cluster).

Or at least, it should be but, indeed, it doesn't appear to be used at the moment. I'd keep it, though, because that's an option that we'd want to enable for controllers. That said, I'd rather fix this issue (and possibly others) into subsequent PRs than keeping this one open any longer. It's a pain to maintain and a huge Damocles' sword over people's head :)

Can you point me to the code? Couldn't find it in the changeset.

Or at least, it should be but, indeed, it doesn't appear to be used at the moment. I'd keep it, though, because that's an option that we'd want to enable for controllers. That said, I'd rather fix this issue (and possibly others) into subsequent PRs than keeping this one open any longer. It's a pain to maintain and a huge Damocles' sword over people's head :)

As an alternative, it can be removed from the code and be addressed as part of the relevant issue.
IMHO it Would be good practice to develop a feature that has definition of done.

operator-framework/src/test/java/io/javaoperatorsdk/operator/IntegrationTestSupport.java

s-soroosh · 2020-12-17T17:54:30Z

...ntime/src/main/java/io/javaoperatorsdk/quarkus/extension/QuarkusControllerConfiguration.java

+    this.crClass = crClass;
+    this.doneableClassName = doneableClassName;
+    this.watchAllNamespaces = this.namespaces.contains(WATCH_ALL_NAMESPACES_MARKER);
+    this.retryConfiguration =


how can the user configure the retry in quarkus projects?

I need to add the configuration part, indeed. I will create an issue for it.

s-soroosh · 2020-12-17T18:08:41Z

README.md

-       Operator operator = new Operator(new DefaultKubernetesClient());
-       operator.registerController(new WebServerController());
+       Operator operator = new Operator(new DefaultKubernetesClient(),
+           DefaultConfigurationService.instance());


It's not really easy to understand what DefaultConfigurationService is :(.
I think there is a way to make better before the final release.

The problem is to be able to provide the configuration from the annotation processor in the absence of injection. What would you propose to make this simpler/clearer?
The same thing could apply to DefaultKubernetesClient for that matter… it shouldn't be exposed either, IMO.

ConfigurationProvider might be a better name?

From the user's point of view, there is only one ConfigurationService in the classpath belonging to the imported dependency, so would be best to hide it completely from the user by default.
I'm fine with the current approach and naming though, can become simpler in next iterations.

@psycho-ir the problem is that we cannot put it there by default because then it would be pulled in by the Quarkus implementation as well, which is exactly what I'm trying to avoid 😄

metacosm force-pushed the quarkus-support branch 2 times, most recently from be7b6d8 to 70f6bf7 Compare November 27, 2020 12:37

metacosm changed the title ~~Extract controller configuration handling into an interface~~ Extract controller configuration handling and provide Quarkus extension Dec 1, 2020

metacosm mentioned this pull request Dec 1, 2020

Quarkus provided dependency #239

Closed

metacosm force-pushed the quarkus-support branch from b63473e to 027134f Compare December 4, 2020 17:20

metacosm marked this pull request as ready for review December 4, 2020 23:11

metacosm self-assigned this Dec 8, 2020

metacosm force-pushed the quarkus-support branch 3 times, most recently from 1267463 to e200c2e Compare December 16, 2020 21:47

metacosm marked this pull request as draft December 16, 2020 21:48

metacosm force-pushed the quarkus-support branch from 058060b to ef8e1c4 Compare December 17, 2020 13:12

metacosm marked this pull request as ready for review December 17, 2020 13:12

metacosm requested review from s-soroosh, csviri and adam-sandor December 17, 2020 13:12

metacosm mentioned this pull request Dec 17, 2020

Define how controllers / operator should be externally configured #237

Closed

metacosm added 3 commits December 17, 2020 18:21

refactor: extract default finalizer name logic into own method

31fe80c

feat: Controllers can now be named for configuration purposes

07e4741

feat: encapsulate controllers' configuration behind an interface

89add61

The purpose is to be able to use a different implementation depending on the execution context. For example, a Quarkus-specific implementation would process the annotation at build time instead of doing so at runtime.

metacosm added 17 commits December 17, 2020 18:21

fix: wrong parent version

cfebbc7

feat: automatically register detected controllers

8efc8e2

chore: remove unneeded dependencies

eedf0d5

fix: wrong/duplicated imports

94f3bdd

fix: update versions in POM files

dbc5394

fix: restore to compiling state after rebase

c98f825

fix: move compile-fixtures to proper spot

8f1c8de

chore: move integration tests to runtime-configuration module

b3066f2

operator-framework module now also creates a jar out of its test classes for reuse purposes. This should probably be extracted more cleanly at some point.

refactor: fold api module back into operator-framework

465b53f

refactor: move classes back to their previous spot

85c466f

refactor: rename operator-framework module to operator-framework-core

495ff39

refactor: rename runtime-configuration to operator-framework

9c1c559

This is the module that should now be imported by non-Quarkus apps

refactor: rename SB starter to follow SB conventions

bebc154

refactor: rename quarkus extension for consistency

09e3446

chore: clean-up

0b1b421

fix: move CRD descriptors where they're needed

47cbc10