Implement OpenJDK 9 Image #129
Conversation
There was a problem hiding this comment.
Looks good overall.
I think the big thing we're missing here is an integration test that exercises a Java 9 app. It might be possible to write a structure test for the JDK 9 image that tries to compile a simple Java 9 class.
Also, maybe we can configure test-application to have a java9 profile, or maybe we just need a separate test app.
DEVELOPING.md
Outdated
| ``` | ||
| The resulting image is called openjdk | ||
| The resulting image(s) are called openjdk. |
There was a problem hiding this comment.
..with tags differentiating the version (openjdk:8 and openjdk:9).
DEVELOPING.md
Outdated
| $ mvn clean install | ||
|
|
||
| # only build the openjdk8 image | ||
| $ mvn clean install -am -pl openjdk8 |
There was a problem hiding this comment.
can we use non-abbreviated flags? For example, --also-make.
| - '--batch-mode' | ||
| - '-Ddocker.image.name=${_IMAGE}' | ||
| # only build the specified module | ||
| - '--projects=${_MODULE}' |
There was a problem hiding this comment.
Doesn't ${_MODULE} need to be a separate argument?
There was a problem hiding this comment.
It would appear not - the mvn executable is able to deal with arguments of the form "foo=bar".
scripts/local_integration_test.sh
Outdated
| echo "Building app container..." | ||
| docker build -t $APP_IMAGE . || gcloud docker -- build -t $APP_IMAGE . | ||
| docker build -t $APP_IMAGE . || docker -- build -t $APP_IMAGE . |
There was a problem hiding this comment.
I had gcloud docker so that you can pull project-private images from GCR to test.
There was a problem hiding this comment.
👍
FYI this means we'd need to use a maven/docker cloudbuild step, like we do in jetty, but that shouldn't be too difficult to set up.
There was a problem hiding this comment.
I guess one option is to add gcloud only if the Cloud SDK installation is detected.
| commandTests: | ||
| - name: 'correct java version is installed' | ||
| command: ['java', '-version'] | ||
| expectedError: ['openjdk version "${openjdk.version.major}-Debian"'] |
There was a problem hiding this comment.
This is interesting. They changed the -version output: https://blogs.oracle.com/java-platform-group/a-new-jdk-9-version-string-scheme and http://openjdk.java.net/jeps/223 -- we should follow this with our parsing logic so that it stays maintainable and we don't lose out on ensuring the minor version.
Based on above I'm confused by the -Debian part - where did that get introduced. It also might be part of the major version?
openjdk version "9-Debian"
OpenJDK Runtime Environment (build 9-Debian+0-9b181-2)
OpenJDK 64-Bit Server VM (build 9-Debian+0-9b181-2, mixed mode)
java 8:
Start command: java -showversion -Xms51137M -Xmx51137M -XX:+UseG1GC -XX:+ParallelRefProcEnabled -XX:+PrintCommandLineFlags -version
-XX:InitialHeapSize=53621030912 -XX:MaxHeapSize=53621030912 -XX:+ParallelRefProcEnabled -XX:+PrintCommandLineFlags -XX:+UseG1GC
openjdk version "1.8.0_131"
OpenJDK Runtime Environment (build 1.8.0_131-8u131-b11-1~bpo8+1-b11)
OpenJDK 64-Bit Server VM (build 25.131-b11, mixed mode)
There was a problem hiding this comment.
The -Debian suffix likely is there due to the fact that we're installing openjdk currently from the "unstable" debian package repository. I'm not sure if debian builds of openjdk9 will always have this suffix, but I'd hope at least that they will follow the new version scheme spec. I haven't seen clear info on this yet.
…ntime into openjdk-9 * 'openjdk-9' of github.com:GoogleCloudPlatform/openjdk-runtime: adding deployment token as a check before integration tests (#133)
|
@meltsufin @cassand @balopat I've addressed all remaining TODOs and I believe I've addressed all PR comments. Mind giving this another review? |
openjdk9/src/main/docker/Dockerfile
Outdated
| ENV GAE_IMAGE_LABEL ${docker.tag.long} | ||
|
|
||
| RUN \ | ||
| # add debian 'sid' repo in order to install openjdk-9 |
There was a problem hiding this comment.
typo? this is not the 'sid' anymore it's buster. Maybe a link https://wiki.debian.org/DebianBuster would be good too.
openjdk9/src/main/docker/Dockerfile
Outdated
| # jdk and its dependencies | ||
| ca-certificates-java=20170531'*' \ | ||
| openjdk-9-jdk-headless=${openjdk.version}'*' \ | ||
| libcups2=2.2.1-8~bpo8+1 \ |
There was a problem hiding this comment.
can we put in a comment above or here, why we need to downgrade libcups2?
I would even double check if we can remove it completely! (unless of course we want to explicitly support printing from cloud deployed apps or rather the JDK has some weird dependency on CUPS)
openjdk9/src/main/docker/Dockerfile
Outdated
| openjdk-9-jdk-headless=${openjdk.version}'*' \ | ||
| libcups2=2.2.1-8~bpo8+1 \ | ||
| # system utilities | ||
| procps \ |
There was a problem hiding this comment.
why do we need procps? jdk8 doesn't have it!
openjdk9/src/main/docker/Dockerfile
Outdated
| unzip \ | ||
| && apt-get clean \ | ||
| && rm /var/lib/apt/lists/*_* | ||
|
|
There was a problem hiding this comment.
btw jdk8 has wget, jdk9 doesn't. Why the discrepancy and why is it needed at all in jdk8? can we remove it? (maybe a separate issue)
There was a problem hiding this comment.
👍 You're right, it's not necessary. Created #135 to track removing it from openjdk:8.
| && rm /var/lib/apt/lists/*_* | ||
|
|
||
| # Add the cloud debugger libraries | ||
| ADD https://storage.googleapis.com/cloud-debugger/appengine-java/current/cdbg_java_agent.tar.gz /opt/cdbg/ |
There was a problem hiding this comment.
is this the same for jdk8 and 9? Btw, I can't find this URL anywhere documented, is this specifically built for GAE-java runtimes? The official docs from the debugger point to the GCE link: https://storage.googleapis.com/cloud-debugger/compute-java/debian-wheezy/cdbg_java_agent_gce.tar.gz
There was a problem hiding this comment.
I'd think this should be the same, but the fact that that URL references wheezy gives me pause. I'll ask around for the current best practice.
There was a problem hiding this comment.
Just wanted to circle back on this - this is indeed the correct path for the app engine cloud debugger binary. There are no public docs because it's installed automatically and transparently to the user.
| && rm /opt/cdbg/cdbg_java_agent.tar.gz \ | ||
| && chmod +x /docker-entrypoint.bash /shutdown/*.bash /setup-env.d/*.bash \ | ||
| && mkdir /var/log/app_engine \ | ||
| && chmod go+rwx /var/log/app_engine |
There was a problem hiding this comment.
all this is completely duplicated across jdk8 and 9 - I'm not sure we can get rid of this easily but it might worth to have another thought about some kind of templating setup.
There was a problem hiding this comment.
Fair point.
It looks like some of the openjdk images on dockerhub actually generate static dockerfiles, but then they commit them so it's completely transparent and easy to understand. Maybe that's something we can look into in the future.
There was a problem hiding this comment.
that is a cool approach, I created #136 we can continue to discuss it there.
|
👍 Thanks @balopat ! The |
There was a problem hiding this comment.
README.md + 2 more thoughts:
1.) Have you thought about compiling the test-application with Java 9? In the end it would bring out GCP library (in)compatibilities quicker...? On the other hand maven is not necessarily there yet to build Java9 apps.
2.) Philosophical question: Is the compilation test a structure test or an integration test? I'd think it's an integration test, but I can be convinced.
README.md
Outdated
|  | ||
|
|
||
|  | ||
|  | ||
|
|
||
| # Google Cloud Platform OpenJDK Docker Image | ||
|
|
There was a problem hiding this comment.
This image is mirrored at both `launcher.gcr.io/google/openjdk8` and `gcr.io/google-appengine/openjdk`.
I think we'll need to update this for openjdk9 as well I guess? How do we differentiate between 8 and 9 in the google-appengine version?
There was a problem hiding this comment.
Also, how does 8 vs 9 affect runtime:java?
There was a problem hiding this comment.
Yep, we should update this. I'm not sure what's involved in getting images added to launcher.gcr.io. @meltsufin Is this something we should look into?
There was a problem hiding this comment.
runtime: java is still correct. The user selects between openjdk8/9 using runtime_config.jdk (see below). Until we release the openjdk:9 image and add it to the runtime builder, jdk: openjdk9 won't be available. I'll update the readme to reflect the current state.
runtime_config:
jdk: openjdk8There was a problem hiding this comment.
As discussed, the structure test is a valid positioning for this - however, this case, please rename the test-app to java9-compilation-test or something similar.
|
if we add new JVM options which are specific for the openjdk-9 runtime (see for example #137) - where would they fit in this multi-module design? |
|
I'd think they should live in the src/main/docker dir within the module that they're specific to. Then the dockerfile would have access to them during the docker build. If we wanted to set additional JVM flags, we could add a script to the setup-env.d directory that modifies the args accordingly. |
|
Great, in my opinion, this is all solid work, great job! LGTM |
|
@meltsufin Do you mind giving this another review so it can be merged? Thanks! |
This PR adds an openjdk:9 image, which can be built alongside the existing openjdk:8 image.
Other notable changes:
openjdk-commonandopenjdk-test-commonto store common resources and tests between the two images.build.shscript to require a module (openjdk8 or openjdk9)to be specified, as we will be building/releasing only one image at a timestructure_test.shandlocal_integration_test.shinto the integration-test phase of the maven build. This simplifies both the maven and thecloudbuild.yamlconfigurationStill TODO:
local_integration_test.sh, docker client must be installed on the builder image: