fnproject / fdk-java

Java API and runtime for fn.
Apache License 2.0
142 stars 53 forks source link

Init image for SubstrateVM. #156

Closed tzezula closed 6 years ago

tzezula commented 6 years ago

Added an init image for SubstrateVM compilation. The init --init-image fnproject/fn-java-native-init generates a template with docker runtime building a SubstrateVM image for Java Maven project.

shaunsmith commented 6 years ago

I demoed SVM support at the Hamburg JUG yesterday using this PR and it worked nicely! 😃 I've got a couple of comments:

  1. The name of the generated binary is fn which could lead to confusion as our cli is called fn. How about func which is the expected name in Go (func.go) and Node.js (func.js)?

  2. The init image generated Dockerfile hardwires the name of the target class and method but should be picking it up from the func.yaml file to align with the existing JDK behaviour.

tzezula commented 6 years ago

To @shaunsmith:

  1. The name of the generated binary... Yes, I will change the fn to func.

  2. The init image generated Dockerfile hardwires the name of the target class and method but should be picking it up from the func.yaml file to align with the existing JDK behaviour. As I understand the cli it's not possible in the docker type runtime. The cmd from the func.yaml is inserted by the the cli only for language helpers by writeTmpDockerfile in dockerBuild

shaunsmith commented 6 years ago

@tzezula You're right, my thinking is wrong here. But I see the class and method name also appears in the reflection.json. If the purpose of the init image is to create a scaffolding that a user can use to get started quickly and modify I think we should use the name of the function as the name of the class so it's close to what they are trying to get to. We don't do this now in the old --runtime boilerplate but it's something @delabassee has been asking for. The move to init-image give us a chance to improve our code generation. :) So if you take the function name and use it for the class name, put it in the Dockerfile, and in the reflection.json then at least we're a little closer to our end goal.

For Java, the obvious next step would be for the user to be able to specify the package they want the function class to be in. @mjg123 here's a good use case for supporting additional parameters to init-image!

tzezula commented 6 years ago

@shaunsmith I agree that it would be good to allow parametrized package and function name. Unfortunately it's not doable with current init-image. As far as I know the init-image just unpack the tar archive obtained from the container. There are no arguments sent to the init-image container, at least there is nothing about such a support in https://github.com/fnproject/docs/blob/master/cli/how-to/create-init-image.md. Am I right?

shaunsmith commented 6 years ago

The function name is passed into the init image through the FN_FUNCTION_NAME environment variable. An init-image that unpacks a tar is just the simplest example of how this can work. You're in a container and can run whatever code you want to generate the output. In the example the command that's run is CMD ["cat", "/init.tar"] but it could be anything. The next simplest thing after tar is probably a shell script that uses sed to substitute in the name of the function into your template files, builds a tar file, and then cats it to stdout.

tzezula commented 6 years ago

@shaunsmith Thanks! The FN_FUNCTION_NAME was the missing part. Fixed.

tzezula commented 6 years ago

I've updated the PR to resolve the comments.

  1. fn renamed to func
  2. Using ${FN_FUNCTION_NAME} when generating the template.
  3. Using the release.version for fdk.version
  4. Added rm -rf /var/lib/apt/lists/* to apt-get...

Questions: The artefacts for the version 1.0.64 from the release.version are not available in the https://dl.bintray.com/fnproject/fnproject/. Which versions are deployed into the maven repo?

zootalures commented 6 years ago

the version in release.version is the forthcoming release (next) not the previous version. we had a glitch with our release script which stopped 1.0.65-1.0.69 from being pushed, this is fixed.

zootalures commented 6 years ago

the release script (and on each branch) already does the following prior to each build:

# We need to replace the example dependency versions also
# (sed syntax for portability between MacOS and gnu)
find . -name pom.xml |
   xargs -n 1 sed -i.bak -e "s|<fdk\\.version>.*</fdk\\.version>|<fdk.version>${release_version}</fdk.version>|"
find . -name pom.xml.bak -delete

so any fdk.version properties referenced in pom files should be up-to-date

zootalures commented 6 years ago

I'm just testing this with the latest FDK version 1.0.70 and I see an error in run

the build works fine but then I see:

$ fn run 
Building image foofn:0.0.1 .
Exception in thread "main" com.oracle.svm.core.jdk.UnsupportedFeatureError: Unsupported method java.lang.Class.getAnnotationsByType(Class) is reachable: The declaring class of this element has been substituted, but this element is not present in the substitution class
        at java.lang.Throwable.<init>(Throwable.java:265)
        at java.lang.Error.<init>(Error.java:70)
        at com.oracle.svm.core.jdk.UnsupportedFeatureError.<init>(UnsupportedFeatureError.java:31)
        at com.oracle.svm.core.jdk.Target_com_oracle_svm_core_util_VMError.unsupportedFeature(VMErrorSubstitutions.java:109)
        at java.lang.Class.getAnnotationsByType(Class.java:3434)
        at com.fnproject.fn.runtime.EntryPoint.run(EntryPoint.java:81)
        at com.fnproject.fn.runtime.EntryPoint.main(EntryPoint.java:43)
        at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:163)

Is there something I can do to fix this (this code was added recently?)

tzezula commented 6 years ago

@zootalures I am just solving it. It was introduced by:

FnFeature[] features = method.getTargetClass().getAnnotationsByType(FnFeature.class);
zootalures commented 6 years ago

Doh - is that specific API not supported or is it a more general annotations issue?

tzezula commented 6 years ago

@zootalures It's a missing SubstrateVM substitution for Class.getAnnotationsByType(Class). I've already created a Pull Request to fix it and waiting for review. BTW: Should not be the FnFeature a repeatable annotation? Making it repeatable will not solve the problem. I just wonder why the getAnnotationsByType(Class) is used.

zootalures commented 6 years ago

Hmm good point - it probably should be repeatable - will take a look at that later, thanks! (we only have one FnFeature today, and that's Flow ;))

tzezula commented 6 years ago

I am now building the SubstrateVM from revision dca98069dc57c458b8bcb3392cf9e66316dd37b6 which is the fix of missing Class.getAnnotationsByType(Class). When there will be a release candidate containing the fix (1.0rc8) I will create a follow up pull request to build the SubstrateVM from the release candidate 1.0rc8.

zootalures commented 6 years ago

I’ve Merged the other change I think this can be merged after a rebase/test and we can iterate from there