Skip to content
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

Added launcher script for runtime images. #247

Conversation

jhuttana
Copy link
Contributor

Could you please review the changes?
This fix is to address : https://issues.redhat.com/browse/OPENJDK-413 issue.

@jhuttana jhuttana requested a review from jmtd November 24, 2021 10:05
@jhuttana
Copy link
Contributor Author

As per my investigation the run-java.sh script holds good for runtime images also. I dont see anything which we can filter from this launcher script. Already we have filtered s2i related changes like maven, jolokia and prometheus(which in turn has maven related changes for build purpose) from runtime launch scripts. Other than maven(which has major role for application building) rest other content in the launcher script holds good for runtime images also.

- This required changes to module.yaml and run script also.

Signed-off-by: Jayashree Huttanagoudar <jhuttana@redhat.com>
@jmtd
Copy link
Member

jmtd commented Jan 6, 2022

If the existing run script is suitable for the runtime images, they should include that module (jboss.container.java.run) rather than copy the content to jboss.container.java.jre.run. However I am not (yet) convinced :) My concern is it's very easy for us to add features to the images and very, very hard to change or remove them afterwards. Also the intention of hte runtime images was to be as lean as possible so we should really scrutinise anything we add.

The JVM tuning parameters to the existing builder run script are out of date and need reviewing (see e.g. OPENJDK-78 and the related issues). I wouldn't want to copy the existing ones over to the runtime images if we are going to change or remove them anyway.

We definitely do not want configure_passwd running in the runtime images.

I think we need to list out all the features of the existing builder run script; the upstream fabric8 run-java script; any that have been specifically requested by e.g. the quarkus team for the runtime images; and compare those lists together, to figure out feature-by-feature what we want. Also, I'll tidy up the OPENJDK JIRAs to make the review work we need to do on the existing tuning parameters clearer, and possibly set those a blockers on this.

@jhuttana
Copy link
Contributor Author

jhuttana commented Jan 6, 2022

If the existing run script is suitable for the runtime images, they should include that module (jboss.container.java.run) rather than copy the content to jboss.container.java.jre.run. However I am not (yet) convinced :) My concern is it's very easy for us to add features to the images and very, very hard to change or remove them afterwards. Also the intention of hte runtime images was to be as lean as possible so we should really scrutinise anything we add.

The JVM tuning parameters to the existing builder run script are out of date and need reviewing (see e.g. OPENJDK-78 and the related issues). I wouldn't want to copy the existing ones over to the runtime images if we are going to change or remove them anyway.

This helps. I will also take a look.

We definitely do not want configure_passwd running in the runtime images.

I think we need to list out all the features of the existing builder run script; the upstream fabric8 run-java script; any that have been specifically requested by e.g. the quarkus team for the runtime images; and compare those lists together, to figure out feature-by-feature what we want. Also, I'll tidy up the OPENJDK JIRAs to make the review work we need to do on the existing tuning parameters clearer, and possibly set those a blockers on this.

Setting blockers will be a great help :)

Further,
The exiting run-java.sh script has below functions

$ grep -E '^[[:space:]]*([[:alnum:]_]+[[:space:]]*\(\)|function[[:space:]]+[[:alnum:]_]+)' ./modules/run/artifacts/opt/jboss/container/java/run/run-java.sh 
check_error() {
auto_detect_jar_file() {
get_jar_file() {
load_env() {
run_java_options() {
get_java_options() {
format_classpath() {
get_classpath() {
get_exec_args() {
function configure_passwd() {
startup() {

Except configure_passwd() , to me it looks like it covers all the features requested by quarkus team here: https://issues.redhat.com/browse/OPENJDK-413
Even some JVM tuning parameters are expected. In that case we can try to customize the JVM parameters specifically for runtime images.

@jhuttana
Copy link
Contributor Author

jhuttana commented Jan 8, 2022

I was going through https://issues.redhat.com/browse/OPENJDK-78 and in the issue description found point which :
"we should just start without JVM parameters and let us add some if we need, rather than trying to hack around to remove parameters."
This is in fact is good thing to let user decide which JVM parameters are suitable for their application. WDYT?

@jmtd
Copy link
Member

jmtd commented Jan 10, 2022

This is in fact is good thing to let user decide which JVM parameters are suitable for their application. WDYT?

The challenge is reaching that without breaking backwards compatibility for existing customers.

@jhuttana
Copy link
Contributor Author

Ok.

@maxandersen
Copy link

The challenge is reaching that without breaking backwards compatibility for existing customers.

maybe I'm missing something here but haven't backwards compatibility not already been broken by not having run-java.sh available ? :)

@jmtd
Copy link
Member

jmtd commented Jan 24, 2022

maybe I'm missing something here but haven't backwards compatibility not already been broken by not having run-java.sh available ? :)

These images have never shipped with a runtime script, so no. My comment RE backwards compatibility was in reply to the suggestion that we drop all the JVM tuning parameters by default for the existing run script - which is in the builder images.

@gsmet
Copy link

gsmet commented Mar 10, 2022

@jhuttana @jmtd any chance some progress could be made on this soon? We are eagerly waiting for this for the Quarkus containers.

Thanks!

@jmtd
Copy link
Member

jmtd commented Mar 10, 2022

@gsmet we have made some forward progress on this today. Sadly the last couple of weeks were interrupted by two CVEs and an OpenJDK regression.

@gsmet
Copy link

gsmet commented Mar 10, 2022

Cool, thanks for letting me know!

@jmtd
Copy link
Member

jmtd commented May 4, 2022

This was superseded by #277.

@jmtd jmtd closed this May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants