Global image for external #5553
Conversation
to make it readable, find-able and more generic
instead of hardcoded ubuntu/ubi images a generic approach was set generic_packages are those which ahve same name on all supported iamges REGEX_packages cen set special packages per distribution eg ubuntu_packages will be installed on ubuntu only eg (fedora|ubi|rhel|centos)_packages will be iisntalled on RH systems This already supports versioned ones, so eg centos:10_packages will indeed restrict the packages to os centos 10 This introduced hackish way how to handle hardcoded criu-openj9-images in this multi-image environment
|
@sophia-guo / @smlambert as we talked recently |
|
@judovana thanks for the work on this PR. Those enhancements are nice. They are addressing two different issues would it be nice to separate the PR into two ( one for global image, one for common property)? Also as we talked recently to keep our workflow organized and ensure each change is properly tracked and reviewed, could you please open two issues and reference them in the respective PRs? |
|
The two parts can not go separtely in parallel. Maybe the properties part can go in first, but the geenric images depends on it. I would rather keep it togehther. They do not have sense one by one. (the btohering with first, if the second will not go in or if it will need major changes is not worthy) Sure, I will create an issue. Thanx a lot for reminder. |
It seems that default contianer was not searching directly in, as the linking on rest of the system led to it. Now all images I tried properly tests mounted java if it is there. if not, default search is still on and works fine for both default with mount, without mount or for other images with java on path without mount
… is not part of the mount commnad
|
Few external tests passes on few jdks on few images.... I will start to roll up some test matrix on this but tbh not sure how it is complete... Many external tests needs forking for newer jdks, also tuning up the depndencies will be fun.... What is required to double check to make this pass through? |
| # os is ubi, and test is criu | ||
| # temporarily all ubi based testing use internal base image | ||
| image_name="$DOCKER_REGISTRY_URL/$base_docker_registry_dir" | ||
| tag="latest" | ||
| EXTERNAL_AQA_IMAGE="${image_name}:${tag}" |
There was a problem hiding this comment.
If EXTERNAL_AQA_IMAGE needs to be set? If yes, should move to line 114 for both temurin and openj9 ? Looks like this is not needed?
There was a problem hiding this comment.
This is unhappy workaround for a corner case I was unable to manage. The only correct sollution to this is described in middle of initial description of issue #5555
The initial change should affect at least the second. To affect the build_all.sh on top of that, should be refactoring, which would remove all the for test in ${supported_tests} do for vm in ${supported_jvms} do for os in ${supported_os} do for package in ${supported_packages} do for build in ${supported_builds} do build_image.sh ${test} ${version} ${vm} ${os} ${package} ${build} to actually do all the looping through ${supported_jvms}, ${supported_os}, ${supported_packages}, ${supported_builds} first, to prepare set of (fully) qualified container IDs and then loop through them, via shortened for test in ${supported_tests} do build_image.sh ${test} ${image} I would probably like to addres it in different issue and different purpose as usage of build-all.sh is unclear to me.
This triple-if is changing to much to be served by the methods in provider.sh (without adding similar triple if there).
If the EXTERNAL_AQA_IMAGE is set in this combination, the exit is called. If not (and that is the only expected case as you need to set three parameters out of any defaults to actually overwrite them by this last-of-three ifs the setup of EXTERNAL_AQA_IMAGE is ensuring the values detected later (which are no longer consistent with the set ones because of those ifs) are correct.
There was a problem hiding this comment.
To construct the EXTERNAL_AQA_IMAGE artificially and globaly, is quite a good idea, and I was playing with it a lot. The reason why I have not included it, are non deterministic OSes. Ubuntu was hardcoded, to be there for jdk-xy-temurin/openj9, because there is no record of ubuntu in name. From my limited knowledge, this is the only case where it is actually missing. Eg redhat java images still have ubi in name, and so on.
If there will be more usage of EXTERNAL_AQA_IMAGE then current baseurl, path, name, os variables, then I guess such a crossroad table would be necessary, but for now I decided to not implement it.
Based on the changes, for temurin default image I would suggest to run a build with tomcat tests. The test job doesn't need to pass. It should be good as long as the generated Dockerfile looks good. Running locally it should be good with both default and EXTERNAL_AQA_IMAGE=my.local.repo/on/hdd/fedora:40. For openj9 external tests you may need @LongyuZhang help to run a few grinder to ensure it works for openj9. |
The default image(s) were not changed, and generated docekrfile remains identical!-) tat was moreover primary goal during this PR. The only chnage which is affecting the defaults is the search in /opt first, but it did not have anmy siode effect as far as I was able to check. I will try tomcat. |
|
jacoco jdk21: fails coorectly (needs different jacaoco sources: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10846/console (todo, fix??) |
|
|
tomcats on jdk11 (both) It was failing for a while because of some apt error, i started debug.. .and then it suudenly started to pass... (thats why two) Let me know if yuou need more grinders. |
There was a problem hiding this comment.
Tests for CRIU failed, looks like the container os and package install was not aligned after the change. It tries to use apt-get install inside ubi image (hyc_grinder_43087).
16:44:37 [exec] STEP 1/23: FROM sys-rt-docker-local/ubi8-with-criu/linux_x86-64-ubi8-criu:latest
16:44:37 [exec] STEP 2/23: ENV RESULT_COMMENT="IN CONTAINER(not-as-root/podman)"
16:44:37 [exec] --> fb4b96608433
16:44:37 [exec] STEP 3/23: ARG CRIU-UBI-PORTABLE-CHECKPOINT_TAG=
16:44:37 [exec] --> b2cd85d4a861
16:44:37 [exec] STEP 4/23: RUN apt-get update && apt-get install -qq -y --no-install-recommends software-properties-common && apt-get install -qq -y --no-install-recommends gnupg && add-apt-repository ppa:ubuntu-toolchain-r/test && apt-get update && apt-get install -y --no-install-recommends git wget unzip tar curl && rm -rf /var/lib/apt/lists/*
16:44:39 [exec] /bin/sh: apt-get: command not found
16:44:39 [exec] Error: building at STEP "RUN apt-get update && apt-get install -qq -y --no-install-recommends software-properties-common && apt-get install -qq -y --no-install-recommends gnupg && add-apt-repository ppa:ubuntu-toolchain-r/test && apt-get update && apt-get install -y --no-install-recommends git wget unzip tar curl && rm -rf /var/lib/apt/lists/*": while running runtime: exit status 127
16:44:39
16:44:39 BUILD FAILED
``
Thanx! Will elaborate! If I do not resolve this until your timezone, can you please provide mre details how to reproduce it locally? |
|
@LongyuZhang I'm trying something like: and And you seems to be right, it is |
|
decalred -> declared fixed. Any luck with build_all.sh ? |
judovana
force-pushed
the
globalImageForExternal
branch
from
1e631e5
to
b7f25af
Compare
|
fixed waht seemed to be fixable... Thanx a lot both! |
Co-authored-by: Martijn Verburg <[email protected]>
Co-authored-by: Martijn Verburg <[email protected]>
|
Anybody alive around? |
|
@judovana is it possible to run a grinder for the final check? All former grinders are obsolete. |
|
Sure thing. Thank you very much!!! |
Is green... Locally I tried also with EXTERNAL_AQA_IMAGE=fedora:41 and EXTERNAL_AQA_IMAGE=centos:stream9 passed. As part as #5575 I will be doing extensive 3D matrix of external_test/os/jdk Do yuou think EXTERNAL_AQA_IMAGE woould be worthy to be used in grinder? |
There is a strong desire to avoid bloating the already large set of Jenkins job parameters, which is why we have previously used EXTRA_DOCKER_ARGS to pass in alternate images. I am presuming with some modification we could still utilize the EXTRA_DOCKER_ARGS parameter to either check for --image or support other build args via parse_docker_args, and set EXTERNAL_AQA_IMAGE from that. |
|
Fair enough. I agree with bloating being undesired. Lets settle it down then for now, and if it works, then +1 for some |
gobalMatchingKeys -> globalMatchingKeys getFullTOpenJ9Image -> getFullOpenJ9Image
| if [ -z "${EXTRA_DOCKER_ARGS}" ] ; then | ||
| echo \ | ||
| " # ================= WARNING ================= WARNING ================= WARNING ================= # | ||
| # EXTRA_DOCKER_ARGS are not set. You will be testing java which is already in container and not TEST_JDK_HOME | ||
| # TEST_JDK_HOME is set to $TEST_JDK_HOME but will not be used. See test_base_functions.sh for order of search | ||
| # You should set your's TEST_JDK_HOME to mount to /opt/java/openjdk, eg: # | ||
| # export EXTRA_DOCKER_ARGS=\"-v \$TEST_JDK_HOME:/opt/java/openjdk\" # | ||
| # ================= WARNING ================= WARNING ================= WARNING ================= #" | ||
| else | ||
| echo \ | ||
| " # =================== Info =================== Info =================== Info =================== # | ||
| # EXTRA_DOCKER_ARGS set as \"$EXTRA_DOCKER_ARGS\" # | ||
| # =================== Info =================== Info =================== Info =================== #" | ||
| if echo "${EXTRA_DOCKER_ARGS}" | grep -q "$TEST_JDK_HOME" ; then | ||
| echo \ | ||
| " # =================== Info =================== Info =================== Info =================== # | ||
| # TEST_JDK_HOME of $TEST_JDK_HOME is used in EXTRA_DOCKER_ARGS # | ||
| # =================== Info =================== Info =================== Info =================== #" | ||
| else | ||
| echo \ | ||
| " # ================= WARNING ================= WARNING ================= WARNING ================= # | ||
| # TEST_JDK_HOME of $TEST_JDK_HOME is NOT used in EXTRA_DOCKER_ARGS # | ||
| # ================= WARNING ================= WARNING ================= WARNING ================= #" | ||
| fi | ||
| fi | ||
|
|
There was a problem hiding this comment.
For the record, not a great fan of this output. My original review said to change the previous incarnation to look more professional. Not going to block the PR on it, but think that this is excessively noisy and seems like its better suited for a debugging session than a production run.
|
@karianna - you've got a change request on this, when you feel comfortable that any final requests you have are addressed, please lift it. |
This PR is enabling any possible image to be fed to external tests.
User can set it via
EXTERNAL_AQA_IMAGEvariable. EgEXTERNAL_AQA_IMAGE=my.local.repo/on/hdd/fedora:40. Any legal subset -eg justfedorais allowed.The properties model was quite enhanced to work properly in multi distroenviroenment. This may be also good base for future localhost runs. I had tested all features on default ubuntu based temurin and on fedora:39. To tune it for centos/rhel will be quite a fun. But full tuning should not be part of this PR.
The build_all.sh is not affected by this change.
this is currently draft, becasue for some reasonnot yet found by me, the mounted jdk in fedora ... is not here. The default works. I wil continue to work on this, but any feedback appreciated.
Close #5555