Init image for SubstrateVM. by tzezula · Pull Request #156 · fnproject/fdk-java

tzezula · 2018-09-24T16:45:53Z

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.

zootalures · 2018-09-25T18:15:06Z

   cd runtime
   docker build -f Dockerfile-jdk9 -t fnproject/fn-java-fdk:jdk9-${BUILD_VERSION} .
 )
+


Any reason why we wouldn't build this on every release?

Also could you clarify what the deps are for the tools below (I see python 2.7)

The build of the fnproject/fn-java-native is very expensive.
It needs to build JVMCI enabled JDK and then the SubstrateVM. This is the reason I've made this optional.

The dependency on the python is only in the Docker container during the fnproject/fn-java-native build. The python is required for the mx build system.

zootalures · 2018-09-25T18:27:38Z

+WORKDIR /build
+
+RUN set -x \
+    && git clone https://github.com/graalvm/mx.git \


Is it preferable to use a released version here rather than master?

I know there is still a bit of build-unpredictability but I wonder if revving the graal version explicitly is the right thing (rather than on every release)

The mx is a build tool used to build the GraalVM.
It should be backward compatible, using latest should be fine.

zootalures · 2018-09-25T18:52:57Z

+    <modelVersion>4.0.0</modelVersion>
+    <properties>
+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+        <fdk.version>1.0.64</fdk.version>


Would prefer it if the init image generated either the current version it was built with (placeholder this to /release.version on init-image build) or (slightly less ideal but what the CLI does now) picked up the latest version from bintray and/or an env var.

Right.
I will fix it to use the /release.version.
Thanks for pointing me on it!

zootalures · 2018-09-25T18:57:50Z

+WORKDIR /function
+COPY --from=build /function/target/*.jar target/
+COPY --from=build /function/src/main/conf/reflection.json reflection.json
+RUN /usr/local/graalvm/bin/native-image \


The FDK now has a mandatory JNI dependency to handle the new HTTP-over/unix sockets proto - (http-stream) protocol (we are moving to this over other protocols going forward) - - (build from /runtime/src/main/c/) is there a way we can get this working in Graal ?

The UDS code is fairly vanilla but currently compiled against glibc - will this work by default or do we need to something to make this work better in graal?

Yes.
There is a support for the JNI in the SubstrateVM. I need to add JNIConfigurationFile similar to ReflectionConfigurationFile listing the native methods.

Regarding using the AF_UNIX socket library in the scratch image I need to try if we can still use the -static compilation or we need to copy some libraries from the build image.

Yep happy do that in a separate PR after this is merged -

zootalures · 2018-09-25T18:58:28Z

@@ -0,0 +1,2 @@
+runtime: docker
+format: http


FWIW - we are planning to deprecate http in favour of the unix socket proto over the next few weeks.

It is very hard to merge GraalVM SVM support if we every time run into "will be deprecated and replaced by XY in several weeks" I hope this planned change will not block the merge.

Understood - and appologies for inconvenience here - let's get back to how we support this after this is merged.

What is the format name which should be used for AF_UNIX sockets?
I will try to make it work.

http-stream

shaunsmith · 2018-09-26T04:48:32Z

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

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)?
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.

zootalures · 2018-09-26T14:55:37Z

+
+RUN set -x \
+    && apt-get -y update \
+    && apt-get -y install gcc g++ git make openjdk-8-doc openjdk-8-source python zlib1g-dev


is openjdk-8-doc and source needed here? -these are going to create huge layers - the source package pulls the X-libraries by virtue of pulling openjdk-8-jdk (vs the headless version which pulls a ton of deps)

Can you add a
&& rm -rf /var/lib/apt/lists/* to the end of this layer

I will try if the headless JDK version is enough to build the SubstrateVM.

I will add rm ...

Yeah - had a quick play and couldn't get it working - the issue is that the source package in debian seems to be fixed to the non-headless version

The dep footprint for this layer is not too bad for a dev image (119MB) and the Graal Layer is about 288MB

➜ docker history fnproject/fn-java-native IMAGE CREATED CREATED BY SIZE COMMENT 252228a80443 19 hours ago /bin/sh -c #(nop) WORKDIR /function 0B dc5e51662e83 19 hours ago /bin/sh -c #(nop) ENV GRAALVM_HOME=/usr/loc… 0B 36207c000987 19 hours ago /bin/sh -c #(nop) COPY dir:2cc8a2aa51ac7cfdf… 288MB 0bb10d5211e4 20 hours ago /bin/sh -c set -x && apt-get -y update … 119MB 8f4317c6c4fa 20 hours ago /bin/sh -c #(nop) LABEL maintainer=tomas.ze… 0B 44e19a16bde1 3 weeks ago /bin/sh -c #(nop) CMD ["bash"] 0B <missing> 3 weeks ago /bin/sh -c #(nop) ADD file:e6ca98733431f75e9… 55.3MB

Fwiw I’m not too bothered about the net size here it’s not great but pretty tolerable imho

The resulting build image is created below from the debian:stretch-slim and only the SubstrateVM is copied from it.

FROM debian:stretch-slim as final RUN set -x \ && apt-get -y update \ && apt-get -y install gcc zlib1g-dev COPY --from=build /build/graal/vm/latest_graalvm/graalvm /usr/local/graalvm

tzezula · 2018-09-27T12:28:05Z

To @shaunsmith:

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

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 · 2018-09-27T12:56:42Z

@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 · 2018-09-27T16:22:32Z

@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 · 2018-09-27T16:30:35Z

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 · 2018-10-01T10:31:40Z

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

tzezula · 2018-10-01T10:33:17Z

I've updated the PR to resolve the comments.

fn renamed to func
Using ${FN_FUNCTION_NAME} when generating the template.
Using the release.version for fdk.version
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 · 2018-10-01T13:09:51Z

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 · 2018-10-01T13:10:59Z

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 · 2018-10-01T13:35:02Z

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 · 2018-10-01T13:38:31Z

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

FnFeature[] features = method.getTargetClass().getAnnotationsByType(FnFeature.class);

zootalures · 2018-10-01T14:58:27Z

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

tzezula · 2018-10-01T16:21:28Z

@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 · 2018-10-01T16:25:04Z

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 ;))

…trateVM.

…e-image

tzezula · 2018-10-04T13:05:01Z

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 · 2018-10-04T21:13:42Z

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

zootalures

LGTM thanks!

Re Next steps I will CI the build and init images tomorrow and add some integration tests against the init image

Init image for SubstrateVM.

357d875

zootalures reviewed Sep 25, 2018

View reviewed changes

zootalures requested a review from mjg123 September 25, 2018 18:59

zootalures reviewed Sep 26, 2018

View reviewed changes

Fixed review comments.

6cade49

Merge branch 'master' into native-image

d0770b1

tzezula added 2 commits October 4, 2018 14:59

Fixed Unsupported method java.lang.Class.getAnnotationsByType in Subs…

af5599e

…trateVM.

Merge branch 'native-image' of github.com:tzezula/fdk-java into nativ…

7cd8dfe

…e-image

tzezula mentioned this pull request Oct 4, 2018

make FnFeature multi-valued remove use of getAnnotationsByType #160

Merged

zootalures approved these changes Oct 4, 2018

View reviewed changes

Merge branch 'master' into native-image

b48c0e5

zootalures merged commit 6a56da6 into fnproject:master Oct 4, 2018

Conversation

tzezula commented Sep 24, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaunsmith commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zootalures Sep 27, 2018 • 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!

tzezula commented Sep 27, 2018

Uh oh!

shaunsmith commented Sep 27, 2018

Uh oh!

tzezula commented Sep 27, 2018

Uh oh!

shaunsmith commented Sep 27, 2018

Uh oh!

tzezula commented Oct 1, 2018

Uh oh!

tzezula commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zootalures commented Oct 1, 2018

Uh oh!

zootalures commented Oct 1, 2018

Uh oh!

zootalures commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tzezula commented Oct 1, 2018

Uh oh!

zootalures commented Oct 1, 2018

Uh oh!

tzezula commented Oct 1, 2018

Uh oh!

zootalures commented Oct 1, 2018

Uh oh!

tzezula commented Oct 4, 2018

Uh oh!

zootalures commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zootalures left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zootalures Sep 27, 2018 •

edited

Loading

tzezula commented Oct 1, 2018 •

edited

Loading

zootalures commented Oct 1, 2018 •

edited

Loading

zootalures commented Oct 4, 2018 •

edited

Loading

zootalures left a comment •

edited

Loading