-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Azul Platform Prime to java-buildpack #954
Conversation
ushaazul
commented
Jun 16, 2022
•
edited
Loading
edited
<change description here> Bug: n/a Reviewed-By: n/a Tests-Run: n/a modified: README.md modified: config/components.yml new file: config/zing_jre.yml new file: docs/jre-zing_jre.md new file: lib/java_buildpack/jre/zing_jre.rb
Excellent! Thanks for the submission. At first glance, it looks good. Two points of feedback.
Thanks |
Hi Daniel,
|
I totally understand about the CLA. That takes time with companies. When it goes green, I will review & we'll get this merged. Thanks again for the contribution! |
modified: docs/jre-zing_jre.md
Hi Daniel, Thanks |
Can we get an ETA on when this change will be merged? Thanks |
It may be a couple of weeks still. There are a number of issues we are trying to address in the next release. I am going to try and squeeze this into that release, but my plan is to review & merge this after the other items are done. |
Thank you so much. Really appreciate the effort. |
OK, so I got some time to test this. Some notes.
I don't think this matters though. If I'm understanding correctly, the intent is to have this JDK support disabled by default since this software requires licensing. That means users are to download the software directly from your site, host a local buildpack repo with the software and then use that. Is that correct?
The basics are there and seem to be working.
https://github.com/cloudfoundry/java-buildpack/blob/main/docs/jre-open_jdk_jre.md#jvmkill I don't think this is working correctly with the Zing JRE. It detects the OOME when it occurs, but it hangs in the process of trying to dump some final memory statistics about the process (useful for debugging). It seems to be hanging somewhere around here based on the logging output at the time it hangs. I'm not sure there's anything you can do with the JVM kill agent, but I don't know off hand why it's failing either, so it's hard to say. What I would probably suggest though is to just not include the JVM kill agent. That is happening automatically because you are basing your ZingJRE implementation off of OpenJDKLike, which installs the kill agent. You could simply remove that line and it'll skip installing the kill agent. You might instead just add some JVM flags to tell the JVM to exit on OOME instead, you can do that with this or that. Another thing installed by the OpenJDKLike implementation by default is the memory calculator. If that makes sense then you're all set. If it does not make sense for this JVM implementation, then you may want to consider removing that and adding JVM flags to tune the JVM's memory settings as well. You do need to tune JVM memory settings with care because...
I'm not sure how this particular JVM operates so I can't really tell you what's best for some of these design decisions, however, happy to provide more context or answer any additional questions you might have. Just let me know. |
Hi Daniel,
|
I tried to debug the hang that you were talking about in the earlier comment.
Tried it with Zing 22.06 jdk8 and jdk11. However when I tried to use rust/cargo. I see the following error
My rust file looks like this -
To ensure that we get this get this included in the next release |
I'm fine with that. See my previous note, there's info about how you'd go about skipping it. |
modified: open_jdk_like.rb
Removed installing java kill agent and memory calculator. |
Hi Daniel, jvmkill agent sources from I was able to printMemoryUsage stats with no issues. Tried it with Azul's Prime jdk8, 11, 15 and 17. Pls let me know in case if you can provide any additional details
|
@@ -38,10 +38,8 @@ def command | |||
# (see JavaBuildpack::Component::ModularComponent#sub_components) | |||
def sub_components(context) | |||
[ | |||
JvmkillAgent.new(sub_configuration_context(context, 'jvmkill_agent')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't remove this here as it'll break other things. Your class ZingJRE inherits from this class though, so what you need to do is to override this method, sub_components
in your class and supply a list that does not include JVM Kill agent.
As far as the killagent goes, that repo is in a weird state. A past developer on the project did a major rewrite on the main branch but that never actually got released. He's no longer working on the project and I don't know the state of that work. If you want to recompile, you need to checkout the latest tagged release & compile from that. That in itself has problems because some of the dependencies are very old, so it doesn't build as is with recent Rust versions. I'm working on updating that and getting something we can build again. I just haven't had time to finish that work yet. Just doing a second pass on testing with the kill agent. I'm using zing22.06.0.0-3-jdk8.0.332-linux_x64.tar.gz which I got from your trial downloads. It installs OK & and the app starts OK. I have a demo app that just adds new
For what it's worth, I'm getting this error also when the app starts up. It seems possibly related.
I'm certainly not trying to cast any blame here. It's very likely that there is something with the killagent, not handling a return value correct or handling the state of things properly, or possibly something specific to the environment on CF where the app runs. Given that you can support most of the functionality with JVM flags like Things that are remaining:
|
Thanks for testing it and your explanation.
item 1 second changes - lib//java_buildpack/jre/open_jdk_like.rb
For item2 - spec/java_buildpack/framework/java_opts_spec.rb
|
For item 1 - Yes, that looks right. For item 2 - You need to override
Using Again, you could try to add in heap dump options but that's probably better to do in a follow-up feature if someone requires it. To do that you would need to check for a bound volume service & set it up to write to that location. This is because you have to write the heap dump somewhere durable or it'll disappear when the app exits (file system in CF is ephemeral). |
modified: ../../../lib/java_buildpack/jre/zing_jre.rb
I have submitted changes as per your suggestion. Pls let me know if we need anything else. |
…E and some minor changes modified: docs/jre-zing_jre.md modified: lib/java_buildpack/jre/open_jdk_like.rb modified: lib/java_buildpack/jre/zing_jre.rb
…re.rb modified: lib/java_buildpack/jre/open_jdk_like.rb modified: lib/java_buildpack/jre/zing_jre.rb
I did some more testing and had a few minor things come up. I had to adjust how the command was being overridden, cause the way it was coded had resulted in the arguments being duplicated. I also cleaned up the docs & removed memory calc & jvm killagent. I'm going to merge your PR and follow after that with a small PR to polish things up. As far as timing, I should be able to get this out very soon. We do have to cut a patch release first with updates for the July 2022 quarterly updates. You'll see that here any time now, and then I'll merge this PR and #957, then I'll cut a new minor release with this functionality. That might be Friday, but more likely will be Monday. Thanks again for the PR! |
Thanks for the update and the ETA for the patch. |
Thanks again for the PR! Final update on timing. We have two more PRs to merge & are targeting the release on Friday. |