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

[OPENJDK-2735] Phase-3 BuildConfig template for multistage build for jlinked image #461

Closed
wants to merge 8 commits into from

Conversation

jhuttana
Copy link
Contributor

This is still WIP and an initial attempt to write a BuildConfig template.

@jhuttana jhuttana requested review from jmtd and Josh-Matsuoka March 14, 2024 19:49
@jmtd
Copy link
Member

jmtd commented Mar 15, 2024

This is looking good! I'll leave some comments.

dockerfile: |
#Stage-1: ubi9-jlinked-image is builder image + application + jlinked JVM
# install additional system dependencies (for ubi-micro) to /mnt/jrootfs
FROM ubi9-jlinked-image AS ubi9-jlinked-image # Replace this with appropriate base image
Copy link
Member

Choose a reason for hiding this comment

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

We wil want OpenShift to substitute FROM ubi9-jlinked-image with the correct imagestream name/URI for the specific instance of this template once deployed. To achieve that, change this line to FROM - AS ubi9-jlinked-image; and then we also need to add some more metadata elsewhere in the buildConfig, a dockerStrategy key underneath strategy:, which will describe the ImageStream to pull from. See https://docs.openshift.com/container-platform/4.15/cicd/builds/build-strategies.html#builds-strategy-docker-from-image_build-strategies

Copy link
Member

Choose a reason for hiding this comment

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

Aha, quoting that URI, we may have a problem: "If the Dockerfile uses multi-stage builds, the image in the last FROM instruction will be replaced.", so in this case, that would be the ubi-micro below, not what we want.

We might have to switch from a mutistage dockerfile to two separate, linked OpenShift buildConfigs instead.

@jmtd
Copy link
Member

jmtd commented Mar 19, 2024

I don't think switching to dockerfilePath will address the problem that OpenShift will substitute the last FROM instruction in the multi-stage build, rather than the first: in other words, whether the Dockerfile is embedded or not, OpenShift will (optionally) replace FROM registry.access.redhat.com/ubi9/ubi-micro, but not FROM ubi9-jlinked-image, which is the one we need substituted.

Two approaches that might work are moving the first stage work into Phase 1, or wrapping the whole thing up in an OpenShift template, and using the template parameters to substitute the FROM. We are going to be using OpenShift templates for other aspects, so it would be worth looking at that now.

spec:
source:
dockerfile: |
#Stage-1: ubi9-jlinked-image is builder image + application + jlinked JVM
Copy link
Member

Choose a reason for hiding this comment

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

error parsing multistage-buildconfig.yaml: error converting YAML to JSON: yaml: line 8: found a tab character where an indentation space is expected

metadata:
generation: 1
name: jlinked-image
namespace: default
Copy link
Member

Choose a reason for hiding this comment

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

remove this line

dockerfile: |
#Stage-1: ubi9-jlinked-image is builder image + application + jlinked JVM
# install additional system dependencies (for ubi-micro) to /mnt/jrootfs
FROM - AS ubi9-jlinked-image # Replace this with appropriate base image
Copy link
Member

Choose a reason for hiding this comment

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

since we cannot rely on OpeNShift to substitute "-" here, for dev purposes we could use FROM quay.io/jdowland/jlink:ubi9-jlinked-image as ubi9-jlinked-image

… for jlinked image

Signed-off-by: Jayashree Huttanagoudar <[email protected]>
… a new file multistage-buildconfig1.yaml with another strategy

Signed-off-by: Jayashree Huttanagoudar <[email protected]>
…stage-1 and stage-2

in the multistage Dockerfile. Also use proper BuildConfig objects to replace FROM, ENV and ARG in the Dockerfile.

Signed-off-by: Jayashree Huttanagoudar <[email protected]>
@jhuttana
Copy link
Contributor Author

jhuttana commented Mar 22, 2024

On my local crc setup I tried to create the template as follows:

$ oc create -f templates/multistage-buildconfig.yaml
template.template.openshift.io/jlinked-image-template created

$ oc get template
NAME                     DESCRIPTION                                                              PARAMETERS    OBJECTS
jlinked-image-template   Template to produce an imagestream and buildconfig for a Jlinked image   0 (all set)   3

Further I tried to process the template and end-up with some errors :)

$ oc process jlinked-image-template | oc create -f -
imagestream.image.openshift.io/jlinked-image created
Error from server (Invalid): error when creating "STDIN": BuildConfig.build.openshift.io "ubi9-jlinked-image-buildconfig" is invalid: spec.strategy: Invalid value: build.BuildStrategy{DockerStrategy:(*build.DockerBuildStrategy)(nil), SourceStrategy:(*build.SourceBuildStrategy)(nil), CustomStrategy:(*build.CustomBuildStrategy)(nil), JenkinsPipelineStrategy:(*build.JenkinsPipelineBuildStrategy)(nil)}: must provide a value for exactly one of sourceStrategy, customStrategy, dockerStrategy, or jenkinsPipelineStrategy
Error from server (Invalid): error when creating "STDIN": BuildConfig.build.openshift.io "lean-runtime-buildconfig" is invalid: spec.strategy: Invalid value: build.BuildStrategy{DockerStrategy:(*build.DockerBuildStrategy)(nil), SourceStrategy:(*build.SourceBuildStrategy)(nil), CustomStrategy:(*build.CustomBuildStrategy)(nil), JenkinsPipelineStrategy:(*build.JenkinsPipelineBuildStrategy)(nil)}: must provide a value for exactly one of sourceStrategy, customStrategy, dockerStrategy, or jenkinsPipelineStrategy

Looks like some wrong stuff is fed for build strategy !.
Which I need to figure out.

@jmtd
Copy link
Member

jmtd commented Mar 22, 2024

These are just indentation errors I think: It's saying the buildconfigs don't have .spec.strategy keys. At the moment the strategy keys are indented such that they're under spec.source.

dockerStrategy:
from:
kind: ImageStreamTag
name: quay.io/repository/jdowland/jlink:ubi9-jlinked-image
Copy link
Member

@jmtd jmtd Mar 22, 2024

Choose a reason for hiding this comment

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

This is a registry URI , but it needs to be an ImageStreamTag. The eventually-correct value for this will depend upon Josh's Phase 2 template. for now you could put jlinked-image which definitely exists (your template creates it) as a placeholder. I'd put a comment after # placeholder value

dockerfile: |
#Stage-1: ubi9-jlinked-image is builder image + application + jlinked JVM
# install additional system dependencies (for ubi-micro) to /mnt/jrootfs
FROM -
Copy link
Member

@jmtd jmtd Mar 22, 2024

Choose a reason for hiding this comment

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

until the dockerStategy.from.name below is the valid image stream, I'd put the test image URI here: FROM quay.io/repository/jdowland/jlink:ubi9-jlinked-image

imageChange:
from:
kind: ImageStreamTag
name: quay.io/repository/jdowland/jlink:ubi9-jlinked-image # ImageStreamTag to trigger the build (stage-1 in Dockerfile)
Copy link
Member

Choose a reason for hiding this comment

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

likewise this is not a valid ImageStreamTag. Put a placeholder value in until we know a real one further along development

dockerStrategy:
from:
kind: ImageStreamTag
name: registry.access.redhat.com/ubi9/ubi-micro
Copy link
Member

Choose a reason for hiding this comment

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

not a valid ImageStream tag. use a placeholder

source:
dockerfile: |
#Stage-2:copy application JAR and jlinked JRE to runtime image
FROM -
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't need to be parameterized I don't think. use FROM registry.access.redhat.com/ubi9/ubi-micro. (I wonder if we should actually define an ImageStream for ubi-micro or not.)

#Stage-2:copy application JAR and jlinked JRE to runtime image
FROM -

COPY --from=ubi9-jlinked-image /mnt/jrootfs/ /
Copy link
Member

Choose a reason for hiding this comment

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

these instructions will only work in a Dockerfile multi-stage build, since there's nothing labelled "ubi9-jlinked-image" in these instructions now

Copy link
Contributor Author

@jhuttana jhuttana Mar 22, 2024

Choose a reason for hiding this comment

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

If we replace FROM instruction with quay image directly as FROM quay.io/repository/jdowland/jlink:ubi9-jlinked-image then the output.to.name is ubi9-jlinked-image. My understanding is that this is made available by the first buildConfig that is ubi9-jlinked-image-buildconfig

Correct me if I am wrong

imageChange:
from:
kind: ImageStreamTag
name: registry.access.redhat.com/ubi9/ubi-micro # ImageStreamTag for the final output image (stage-2 in Dockerfile)
Copy link
Member

Choose a reason for hiding this comment

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

this isn't a valid ImageStreamTag. If we wan't to trigger a rebuild when ubi-micro is changed, we will need to define a ubi-micro ImageStream. Perhaps we should

--setopt=varsdir=/etc/dnf/vars \
grep gawk
RUN rm -rf /mnt/jrootfs/var/cache/* /mnt/jrootfs/var/lib/rpm /mnt/jrootfs/var/lib/dnf
strategy:
Copy link
Member

Choose a reason for hiding this comment

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

this key belongs under spec not spec.source


USER 185
CMD /opt/jboss/container/java/run/run-java.sh
strategy:
Copy link
Member

Choose a reason for hiding this comment

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

this key belongs under spec not spec.source

@jmtd
Copy link
Member

jmtd commented Mar 22, 2024

This is looking great. I'm not 100% sure that we should split the build into two buildConfigs here or not; it might be that we can use an openshift template parameter for the FROM in a dockerfile multi-stage build, if we could determine the correct value based on template parameters. (I'm not sure if we can or not!) But if we move ahead with two buildConfigs, as per the current status, we need to do something to fix up the COPY --from lines.

@jhuttana
Copy link
Contributor Author

This is looking great. I'm not 100% sure that we should split the build into two buildConfigs here or not; it might be that we can use an openshift template parameter for the FROM in a dockerfile multi-stage build, if we could determine the correct value based on template parameters. (I'm not sure if we can or not!) But if we move ahead with two buildConfigs, as per the current status, we need to do something to fix up the COPY --from lines.

Thanks for the review.
I will try to brainstorm and experiment few options and update my findings :)

Signed-off-by: Jayashree Huttanagoudar <[email protected]>
@jhuttana
Copy link
Contributor Author

With the recent commit able to create two separate BuildConfigs for stage-1 and stage-2 in our multi-stage Dockerfile

$ oc create -f templates/multistage-buildconfig.yaml
template.template.openshift.io/jlinked-image-template created

$ oc get templates
NAME                     DESCRIPTION                                                              PARAMETERS    OBJECTS
jlinked-image-template   Template to produce an imagestream and buildconfig for a Jlinked image   0 (all set)   3

$ oc process jlinked-image-template | oc create -f -
imagestream.image.openshift.io/jlinked-image created
buildconfig.build.openshift.io/ubi9-jlinked-image-buildconfig created
buildconfig.build.openshift.io/lean-runtime-buildconfig created

@jhuttana jhuttana changed the title [OPENJDK-2735] BuildConfig for multistage build to align with 3-phase build strategy for jlinked image [OPENJDK-2735] Phase-3 BuildConfig for multistage build for jlinked image Mar 25, 2024
@jhuttana jhuttana changed the title [OPENJDK-2735] Phase-3 BuildConfig for multistage build for jlinked image [OPENJDK-2735] Phase-3 BuildConfig template for multistage build for jlinked image Mar 25, 2024
@jmtd
Copy link
Member

jmtd commented Mar 26, 2024

Some notes on the current status:

The first buildConfig cannot be run because of a dependency issue

oc start-build ubi9-jlinked-image-buildconfig
The ImageStreamTag "jlinked-image:latest" is invalid: from: Error resolving ImageStreamTag jlinked-image:latest in namespace jlink: unable to find latest tagged image

The ImageStream exists, but there is no :latest tag, because no builds have run yet. The buildConfig outputs to that ImageStream, so we cannot also depend upon it. Removing dockerStrategy.from resolves that problem.

Next problem, builds from that build Config still fail, " Output image could not be resolved.". The output is described as ubi9-jlinked-image:latest which does not exist. The imageStream defined here is jlinked-image instead. Changing that and I can start builds

The builds fail because quay.io/repository/jdowland/jlink:ubi9-jlinked-image cannot be pulled. the correct name is quay.io/jdowland/jlink:ubi9-jlinked-image (no "repository"). Fixing that, and the build can fetch the image OK, and the build succeeds!

The next problem is, after the build succeeds, it updates the jlinked-image imagestream, which causes the trigger for the buildconfig to fire, so it builds again... a potentially infinite loop. The imageChange trigger for this buildConfig should be removed.

@jmtd
Copy link
Member

jmtd commented Mar 26, 2024

On to the second buildConfig, lean-runtime-buildconfig. This gets triggered by the imagestream jlinked-image getting updated (which the first buildConfig does), great.

This second buildConfig is defined to build from, and also write to, jlinked-image imageStream, which cannot work. We need a separate imagStream for each buildConfig to read or write to.

builds from the second buildConfig fail at the COPY --from=ubi9-jlinked-image instructions: it does not know what ubi9-jlinked-image is. I tried to change it to COPY --from=jlinked-image, matching the input imageStream, but that doesn't work either yet.

@jhuttana
Copy link
Contributor Author

With the recent changes FirstBuildConfig completed successfully.
The secondBuildConfig when I see on the web console as developer its state is still new but as Administrator -> Build -> Phase-3 -> Activity -> view evernts

Generated from buildconfig-controller
14 times in the last 13 minutes
error instantiating Build from BuildConfig phase-3/lean-runtime-buildconfig (0): Error resolving ImageStreamTag ubi9-jlinked-image:latest in namespace phase-3: unable to find latest tagged image

But the FirstBuildConfig has successully created ubi9-jlinked-image:latest

$ oc get is
NAME                 IMAGE REPOSITORY                                                                     TAGS     UPDATED
ubi9-jlinked-image   default-route-openshift-image-registry.apps-crc.testing/phase-3/ubi9-jlinked-image   latest   5 minutes ago

…to resolve the second BuildConfig indefinite pending status

Signed-off-by: Jayashree Huttanagoudar <[email protected]>
@jhuttana
Copy link
Contributor Author

I am not sure whether custom build strategy is useful to handle COPY instructions in the secondBuildConfig.
If yes, then we have to use script. Instead the better way would be to use strategy.dockerStrategy.dockerfilePath as used in #471

@jhuttana
Copy link
Contributor Author

jhuttana commented Apr 4, 2024

Closing this PR as we are focusing on #471

@jhuttana jhuttana closed this Apr 4, 2024
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