Skip to content

Conversation

Josh-Matsuoka
Copy link
Contributor

@Josh-Matsuoka Josh-Matsuoka commented Jan 20, 2025

Addresses https://issues.redhat.com/browse/OPENJDK-2994

This adds some error checking to the s2i assemble script to make sure if any of the subcommands fail (mkdeps, mkstrippeddeps, etc) that the error is communicated and the build fails.

Requires #535 for manual testing with 21

To test:

Add the following to generate_deps or find an application that otherwise causes jdeps to fail:

--- a/modules/jlink/artifacts/opt/jboss/container/java/jlink/mkdeps.sh
+++ b/modules/jlink/artifacts/opt/jboss/container/java/jlink/mkdeps.sh
@@ -3,6 +3,8 @@ set -euo pipefail
 shopt -s globstar
 
 function generate_deps() {
+  echo "I'm going to exit now. Bye!"
+  exit 1
   # Create a temporary directory for a module path

Build the image

cekit --descriptor ubi9-openjdk-21.yaml build podman

Run an S2i build:

 s2i build --pull-policy if-not-present --context-dir=getting-started -e S2I_DELETE_SOURCE=false  -e LOGGING_SCRIPT_DEBUG=true -e S2I_ENABLE_JLINK=true -e QUARKUS_PACKAGE_TYPE=uber-jar https://github.com/quarkusio/quarkus-quickstarts.git localhost/ubi9/openjdk-21:latest jlink-appsrc

The build will hit the generate_deps failure and exit after that point.

@Josh-Matsuoka Josh-Matsuoka requested a review from jmtd January 20, 2025 23:00
Copy link
Member

@jmtd jmtd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right idea, but it's not quite working right yet. Given this example app

BASEIMG=ubi9/openjdk-21:1.18
APPSRC=https://github.com/jmtd/quarkus-quickstarts.git
CONTEXTDIR=getting-started
rev=3.9.2
OUTIMG=out-s2i-image

s2i build --pull-policy never --context-dir=$CONTEXTDIR -r=${rev} \
    -e S2I_ENABLE_JLINK=true \
    $APPSRC \
    $BASEIMG \
    $OUTIMG

The build fails, with the end of the log being

Working with:
/deployments/quarkus-run.jar
/deployments
jdeps failed: return code 2
Build completed successfully

Tracing the code, the logging before the last line is due to

The last line is printed by s2i itself, not our scripts.

So how come the exit 2 is not signalling failure to s2i?

@Josh-Matsuoka
Copy link
Contributor Author

I don't seem to be able to specify the revision, S2I isn't liking it:

error: pathspec '3.9.2' did not match any file(s) known to git

However running without it and ensuring that jdeps fails I can see similar strange behavior:

Invoking mkdeps

  • echo 'Invoking mkdeps'
  • generate_deps
  • exit 2
    THIS IS WORKING THIS IS WORKING
    % Total % Received % Xferd Average Speed Time Time Time Current
    Dload Upload Total Spent Left Speed
    100 138 100 138 0 0 520 0 --:--:-- --:--:-- --:--:-- 520
    100 454k 100 454k 0 0 1071k 0 --:--:-- --:--:-- --:--:-- 5538k
    Build completed successfully

I think the culprit is this script: https://github.com/jmtd/quarkus-quickstarts/blob/master/getting-started/.s2i/bin/assemble

The S2I scripts on the container side exit at the correct point when jdeps fails, then S2I runs the user defined assemble script in the application. This seems like an issue with how S2I prioritizes and orders the scripts and I'm not sure if it's something we have any control over from the builder image.

@jmtd
Copy link
Member

jmtd commented Jan 23, 2025

Hi @Josh-Matsuoka , that would be a really good explanation… unfortunately I pasted incorrect reproducer instructions. I have a local clone of quarkus-quickstarts with two remotes: quarkusio and jmtd. The tag 3.9.2 indeed doesn't exist in the jmtd remote -- but i wasn't using the master branch, I was using the tag from the quarkusio repository. So the following reproduces it, and sadly is not explained by a custom s2i assemble script:

BASEIMG=ubi9/openjdk-21:1.18
APPSRC=https://github.com/quarkusio/quarkus-quickstarts.git
CONTEXTDIR=getting-started
rev=3.9.2
OUTIMG=out-s2i-image

s2i build --pull-policy never --context-dir=$CONTEXTDIR -r=${rev} \
    -e S2I_ENABLE_JLINK=true \
    $APPSRC \
    $BASEIMG \
    $OUTIMG

@Josh-Matsuoka
Copy link
Contributor Author

The culprit here was this section:

    echo "jdeps failed: return code $?"
    exit $?

The return code was reported and the scripts exited but $? became 0 due to the echo so from S2I's point of view the build succeeded. The better route here is to remove this, the return code gets passsed back up to assemble where we can use log_error to report it properly.

This now works as expected:

Working with:
/deployments/quarkus-run.jar
/deployments
ERROR mkdeps failed, return code: 2
Build failed
ERROR: An error occurred: non-zero (13) exit code from localhost/openjdk-tech-preview/openjdk-21-jlink-rhel9:1.18

@Josh-Matsuoka Josh-Matsuoka requested a review from jmtd January 28, 2025 23:47
@jmtd jmtd merged commit 1281267 into rh-openjdk:jlink-dev Jan 29, 2025
0 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants