Native Image Friendly #231

s-soroosh · 2020-11-19T21:30:14Z

@kirek007 Here is the PoC.
You should be able to run the https://github.com/java-operator-sdk/basic-native-operator-example/tree/init with this version of the SDK with no need to define neither the donable class nor the custom resource class in @controller annotation.

closes #224

…r-test # Conflicts: # spring-boot-starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/TestController.java

kirek007 · 2020-11-20T09:28:47Z

@psycho-ir thanks! Change looks nice, I'll test it and merge if possible, even today :)

kirek007 · 2020-11-20T13:20:45Z

@psycho-ir I've tested and it works like a harm. I've added integration test to the example so we can be sure that application is starting and at least it's possible to create CRD and CR.
I've also notice that CustomResourceList class was need to used mixedoperations api so I've added generation of this class, but I'm not that familiar with this API and it would be great to hear what you, @adam-sandor and @csviri things about it.
I was not able to push to your repository so my changes are here: https://github.com/java-operator-sdk/java-operator-sdk/compare/annotation-processor-test

s-soroosh · 2020-11-20T13:35:19Z

Thanks for the feedback @kirek007, if we decide that we want to go in this direction I'll make the code simpler and test a few cases that I think fails with the current implementation.
This project can be used to make the code simpler
https://github.com/square/javapoet
and this one to test the compilation:
https://github.com/google/compile-testing

kirek007 · 2020-11-20T13:51:26Z

This code will be removed as soon as kubernetes-client v5 will reach stable release (There is already MR with implementation #229) , at least for Doneable class. I still don't know if we should generate List class or leave it to the developer. I will check that.

Personally I like that with this code we can start building native operators right now! I believe that it's worth investing to make it happen.

I'd like to hear from @adam-sandor and @csviri what they thinks about it.

kirek007 · 2020-11-20T13:52:21Z

This project can be used to make the code simpler
https://github.com/square/javapoet
and this one to test the compilation:
https://github.com/google/compile-testing

Yup, sounds reasonable to use them.

csviri · 2020-11-20T13:54:06Z

@kirek007 @psycho-ir We don't have to generate the list class, we can use the base class as it is now.

kirek007 · 2020-11-20T14:03:43Z

@kirek007 @psycho-ir We don't have to generate the list class, we can use the base class as it is now.

Makes sense because then it's up to developer if he want's to cast List results to custom object or implement List class to have clear API.

Ok, so this code will be removed when stable kubernetes-client 5.0 for quarkus will be available.
@psycho-ir you can ignore my changes then

s-soroosh · 2020-11-20T14:08:33Z

@kirek007 @csviri OK, so I'll continue to work on this PR if to address #224 and #166 if there is no objection.

metacosm · 2020-11-20T17:37:02Z

Note that we're planning more improvements on the Custom Resource handling in the k8s client. See fabric8io/kubernetes-client#2611, ideas are welcome.

…exists.

s-soroosh · 2020-11-22T00:00:21Z

The PR is ready for review.
I added 2 tests for an annotation processor to ensure it can handle cases the class implement more than 1 interfaces or there is an intermediary class.

operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java

metacosm · 2020-11-23T08:36:02Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java

-        } catch (CannotCompileException | NotFoundException e) {
-            throw new IllegalStateException(e);
+            final Class<T> customResourceClass = getCustomResourceClass(controller);
+            return (Class<? extends CustomResourceDoneable<T>>) Class.forName(customResourceClass.getCanonicalName() + "Doneable");


Will this work if the CR class is an inner class?

Yes it does work, haven't tested that with native-images yet though.
@kirek007 do you think we can have an integration test in example repository where has the CustomResource as inner class?

Sure, I'll add this test

After while, I believe that we should test it here, not in example. We would like to introduce native build as a main feature of the SDK.

Maybe we could add native build as a test step in next PR.
WDYT: @adam-sandor @csviri ?

After while, I believe that we should test it here, not in example. We would like to introduce native build as a main feature of the SDK.

True, would be great to have it as one of the examples in this repository, will be easier to find for the users and also can be used as regression tests for future changes in the framework.

kirek007 · 2020-11-24T15:03:07Z

Code looks good to me, If @csviri and @adam-sandor doesn't have anything to add I would suggest merging it.

csviri · 2020-11-20T13:53:10Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java

@@ -37,7 +36,8 @@ static boolean getGenerationEventProcessing(ResourceController controller) {
    }

    static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController<R> controller) {
-        return (Class<R>) getAnnotation(controller).customResourceClass();
+        final Class<R> type = Arrays.stream(controller.getClass().getGenericInterfaces()).filter(i -> i instanceof ParameterizedType).map(i -> (ParameterizedTypeImpl) i).findFirst().map(i -> (Class<R>) i.getActualTypeArguments()[0]).get();


In the future we plan to have a hierarchy of controllers, so extend from an abstract controller. Thus this should be aware of inheritance. I think its not trivial to cover all possible scenarios, that was the reason it was explicit in the annotation. Are you sure this will handle all the cases?
@psycho-ir

This is a very good point, the simplest solution would be searching for the interface we are interested in recursively, but that won't work with native-images as it's impossible to guarantee that reflection is enabled for the entire hierarchy.

I changed the annotation processor implementation to generate a file with a mapping of controller class names to custom resource names and used it here to get the class in one shot.

Changes are already committed, would be great if you could review it. Needs a little bit refactoring & cleanup though that I'll do tomorrow.

operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java

csviri · 2020-11-25T08:18:02Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/Controller.java


 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

 @Retention(RetentionPolicy.RUNTIME)
+@RegisterForReflection


Is this the only reason we have quarkus on classpath? Would be cleaner if we could do it without it.

So if somebody wan't to use quarkus, it can be added to the project. But if not its an unnecessary dependency on classpath.
However if there is no other way to do this, then we can let it here, since quarkus is a priority.

Right, good point.
We can create project that will act like "springboot-starter" for quarkus. It will take of usign supported quarkus version and configuration of reflections.

Yep, I'm planning on doing that with #228.

So my suggestion is to:

Remove quarkus from this PR

Because WIP: feat: make it possible to externally configure the operator #228 is getting quite big let's open new PR dedicated only for implementing quarkus starter
If @metacosm don't mind I can take care of this

That said, I'm not sure that native compilation will work properly without using RegisterForReflection… I also think we should merge #229 as soon as possible because that will simplify a lot of things by removing the need to deal with Doneable classes.

@kirek007 another aspect is also that #228 tries to extract the configuration parts from the Spring Boot starter so that it could be reused in a Quarkus extension instead of duplicating the code so I really think that it should get done sooner rather than later. That said, I'm not quite satisfied with how it currently is, in terms of configuration I mean.

I can remove the Quarkus dependency from here, the other way to achieve the same is generating a ReflectionConfigurationFile in compile time (just like what Quarkus is doing with the annotations). Can be done in the new module that will be added.

That would be great @psycho-ir

…annotation-processor-test

…rsdk"

metacosm · 2020-11-26T12:07:51Z

operator-framework/pom.xml

-            </plugin>
-        </plugins>
-    </build>
+    <profiles>


Shouldn't that be removed now that we only target Java 11+?

@psycho-ir, this should be removed, because we support only 11+ (it's already in master)

metacosm · 2020-11-26T13:05:32Z

Any idea of when this will be merged, @psycho-ir @csviri ?

kirek007

Please remove all quarkus dependencies and then I'll be able to test it and merge finally.

kirek007 · 2020-11-26T13:42:38Z

operator-framework/pom.xml

-            </plugin>
-        </plugins>
-    </build>
+    <profiles>


@psycho-ir, this should be removed, because we support only 11+ (it's already in master)

kirek007 · 2020-11-26T13:43:45Z

operator-framework/pom.xml

-            <groupId>org.javassist</groupId>
-            <artifactId>javassist</artifactId>
-            <version>3.27.0-GA</version>
+            <groupId>io.quarkus</groupId>


Please remove quarkus dependency, we'll add it in dedicated module

kirek007 · 2020-11-26T13:44:35Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/Controller.java


 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

 @Retention(RetentionPolicy.RUNTIME)
+@RegisterForReflection


This is related to quarkus dependency and should be removed

kirek007 · 2020-11-26T13:45:40Z

...work/src/main/java/io/javaoperatorsdk/operator/processing/ControllerAnnotationProcessor.java

+
+
+                final TypeSpec typeSpec = TypeSpec.classBuilder(doneableClassName)
+                        .addAnnotation(RegisterForReflection.class)


This annotation won't compile after removing quarkus-core

kirek007 · 2020-11-26T13:46:44Z

...tor-framework/src/test/resources/ControllerImplementedIntermediateAbstractClassExpected.java

+import io.fabric8.kubernetes.client.CustomResourceDoneable;
+import io.quarkus.runtime.annotations.RegisterForReflection;
+
+@RegisterForReflection


Won't compile after removing quarkus-core

kirek007 · 2020-11-26T13:48:13Z

Any idea of when this will be merged, @psycho-ir @csviri ?

As soon as all request changes will be resolved :)

s-soroosh · 2020-11-26T14:27:49Z

I removed quarks dependency, practically means this changeset wouldn't be adequate to support Quarkus native builds.
There will be a new project to support Quarkus builds just like the spring boot starter that javaoperatorsdk already has.

csviri · 2020-11-26T14:53:46Z

@psycho-ir You mean, there will be new maven module within this PR? Or you intend to do that a separate PR?

kirek007 · 2020-11-26T14:56:29Z

@psycho-ir You mean, there will be new maven module within this PR? Or you intend to do that a separate PR?

If you don't mind I would like to merge this one, and move quarkus work to separate branch

csviri

@psycho-ir You mean, there will be new maven module within this PR? Or you intend to do that a separate PR?

If you don't mind I would like to merge this one, and move quarkus work to separate branch

Sure I'm fine with that.

csviri · 2020-11-25T08:33:24Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/Controller.java


 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

 @Retention(RetentionPolicy.RUNTIME)
+@RegisterForReflection


So if somebody wan't to use quarkus, it can be added to the project. But if not its an unnecessary dependency on classpath.
However if there is no other way to do this, then we can let it here, since quarkus is a priority.

csviri · 2020-11-25T15:03:26Z

operator-framework/pom.xml

+        <profile>
+            <id>java8</id>
+            <activation>
+                <jdk>1.8</jdk>


Is this still valid? I guess this needs to be java 11

csviri · 2020-11-25T15:04:03Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/Controller.java


 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

 @Retention(RetentionPolicy.RUNTIME)
+@RegisterForReflection


That would be great @psycho-ir

s-soroosh · 2020-11-26T15:01:04Z

@psycho-ir You mean, there will be new maven module within this PR? Or you intend to do that a separate PR?

@csviri @kirek007 Sorry I wasn't clear, yes I meant a new module will be needed but not in the scope of this PR.
Feel free to merge the current changeset.

s-soroosh · 2020-11-26T15:05:07Z

@csviri

So if somebody wan't to use quarkus, it can be added to the project. But if not its an unnecessary dependency on classpath.
However if there is no other way to do this, then we can let it here, since quarkus is a priority.

Keeping the annotation for sure makes the integration with Quarkus way simpler.
Without it, I can think of adding another annotation processor that will make reflection-config.json files.

csviri

LGTM

Soroosh Sarabadani added 12 commits November 19, 2020 16:50

Add an annotation processor to generate Doneable classes

1e2efe0

Remove customResourceClass

0ffe8ef

Use reflection to get getCustomResourceDoneableClass

6f6260e

Add auto-service and quarkus runtime dependency

decfd9c

remove customResourceClass from controller classes

ede8325

fix tests

8aa10d1

Merge remote-tracking branch 'origin/master' into annotation-processo…

9380215

…r-test # Conflicts: # spring-boot-starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/TestController.java

resolve conflict

62fba5c

extract generateDoneableClass

41a10a2

Add tools.jar to boot classpath

14167cf

Add tools.jar to boot classpath only for java8

4103622

Add tools.jar to boot classpath only for java8

795c656

Soroosh Sarabadani added 3 commits November 20, 2020 15:54

Add javapoet

2697810

Generate the class with javapoet

7075ddf

Add compile-testing

ef3dd1a

Soroosh Sarabadani added 2 commits November 21, 2020 23:41

Test annotation processor works correctly with more one interface

d969187

Test annotation processor works correctly when an intermediary class …

bf2cb03

…exists.

s-soroosh changed the title ~~[WIP] Native Image Friendly~~ Native Image Friendly Nov 21, 2020

metacosm reviewed Nov 23, 2020

View reviewed changes

don't case to ParameterizedTypeImpl

7eaad18

Soroosh Sarabadani added 2 commits November 23, 2020 20:09

fix destination path

3121fce

Merge branch 'master' into annotation-processor-test

bb89c00

kirek007 added the in-review label Nov 24, 2020

kirek007 self-assigned this Nov 24, 2020

csviri approved these changes Nov 25, 2020

View reviewed changes

kirek007 approved these changes Nov 25, 2020

View reviewed changes

kirek007 mentioned this pull request Nov 25, 2020

Merging changes from #229 and #231 - Two Solutions for Quarkus/Native Compilation #236

Closed

Soroosh Sarabadani added 6 commits November 26, 2020 00:39

generate javaoperatorsdk-custom-resources file

0f110e1

load controllerToCustomResourceMappings map

9482857

Merge remote-tracking branch 'origin/annotation-processor-test' into …

e07d463

…annotation-processor-test

move the resource class metadata to sub directory called "javaoperato…

5446214

…rsdk"

remove unnecessary printlns

84a2b9c

Use url.open to read the resource streams

58334a6

metacosm reviewed Nov 26, 2020

View reviewed changes

remove java8 profile

93f150a

kirek007 suggested changes Nov 26, 2020

View reviewed changes

Soroosh Sarabadani added 2 commits November 26, 2020 14:57

remove dependency to quarkus

99a541d

extract custom resource mapping logic

e5909fa

csviri approved these changes Nov 26, 2020

View reviewed changes

Update the example in the Readme

5a0af91

csviri approved these changes Nov 26, 2020

View reviewed changes

csviri merged commit f0317c9 into operator-framework:master Nov 26, 2020



		final TypeSpec typeSpec = TypeSpec.classBuilder(doneableClassName)
		.addAnnotation(RegisterForReflection.class)

Native Image Friendly #231

Native Image Friendly #231

Uh oh!

Conversation

s-soroosh commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirek007 commented Nov 20, 2020

Uh oh!

kirek007 commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-soroosh commented Nov 20, 2020

Uh oh!

kirek007 commented Nov 20, 2020

Uh oh!

kirek007 commented Nov 20, 2020

Uh oh!

csviri commented Nov 20, 2020

Uh oh!

kirek007 commented Nov 20, 2020

Uh oh!

s-soroosh commented Nov 20, 2020

Uh oh!

metacosm commented Nov 20, 2020

Uh oh!

s-soroosh commented Nov 22, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirek007 commented Nov 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirek007 Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirek007 Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm commented Nov 26, 2020

Uh oh!

kirek007 left a comment

Choose a reason for hiding this comment

Uh oh!

kirek007 Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

s-soroosh commented Nov 19, 2020 •

edited

Loading

kirek007 commented Nov 20, 2020 •

edited

Loading

kirek007 Nov 25, 2020 •

edited

Loading

kirek007 Nov 26, 2020 •

edited

Loading

kirek007 Nov 26, 2020 •

edited

Loading