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 |
|
The Hardcoded ubis are most terrible to adjust to the generic mages, because they were |
|
When I'm in When it is in |
…eneric all `git wget perl make` install fie on all tried OSes
|
From that I'm guessing that the issue will be in Actually the tes which you are unhappy with, have all packages installable on all tried OSes. I can mititgate now the issue by redeclaring it, but it is jsut moving the problem to You may try again. please? |
|
the correctly, although I dont know if it is at least close to reperoducer |
|
Re: #5553 (comment) Since the CRIU tests have passed, I will leave this PR for @sophia-guo @smlambert to check on their side then. Thanks. |
|
Thanx! I'm happy reading from crystal ball was successful!-) I 'm still very curious how to reproduce it locally, because I'm afraid hiding it behind "generic_packages" the issu is hidden. As the criu tests are heavily bounded to single container, the real issue maynever popup again, but still:( @LongyuZhang if you please can share, how can i run that particular tast, I t would be highly appreciated. TYVM!! |
|
The reason we set this test differently is because our internal machines don't have access to packages needed by CRIU, so the infra team pre-built CRIU into the container. If you have access to packages to build CRIU in ubi, you can follow the CRIU instruction to build the ubi base image for this test. Thanks. |
|
@smlambert @sophia-guo hello! Anything more is needed? |
There was a problem hiding this comment.
I have started reviewing this PR, but have stopped for now and will put a hold on it while I look at the current functionality versus what we used to have. I believe we had this functionality of using other images/OSes 'previously' but must have 'regressed'. (related: #3363).
| @@ -0,0 +1,6 @@ | |||
| # those packages are included in all tests and all distributions | |||
There was a problem hiding this comment.
What is the point of this file? Seem it will never actually be used, as most external application tests vary so much, it does not have value. Drop it. It can be added in a later PR when and if it becomes needed.
There was a problem hiding this comment.
Sure. But before I do, please check: 5f88915#diff-3e2f7b162bce7417355d4fe0f18e6366b69e6abe5e73356f863274d3a4f9c04bR2 Those generic_packages="git wget unzip tar curl" are used in 99% of tests (see the rest of 5f88915 and search for -ubuntu_packages or -ubi_packages). I think such base really should be added. (note, this is not disagreeing with removal, just asking for brainstroming. I will remove it solemnly on this comment).
There was a problem hiding this comment.
@smlambert Even if practically unused, I would liek to keep this file. Is is extremly helpful when new distribution is tried. It simply allows you to declare its specific dependencies on one place, and then you can easily pinpoint it. And if this file remains unused for future, then they can be tuned up for individual suites alter.
Right you are, there was such a support, but it needed IFs for every distribution. I had made it as general as possible. There is only one real |
Co-authored-by: Shelley Lambert <[email protected]>
|
decalred -> declared fixed. Any luxk 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]>
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