Global image for external #5553

judovana · 2024-09-04T18:07:04Z

This PR is enabling any possible image to be fed to external tests.
User can set it via EXTERNAL_AQA_IMAGE variable. Eg EXTERNAL_AQA_IMAGE=my.local.repo/on/hdd/fedora:40. Any legal subset -eg just fedora is 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

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

judovana · 2024-09-04T18:07:32Z

@sophia-guo / @smlambert as we talked recently

sophia-guo · 2024-09-04T21:05:09Z

@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?

judovana · 2024-09-05T09:51:49Z

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.

judovana · 2024-09-05T12:21:16Z

#5555

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

judovana · 2024-09-05T15:09:36Z

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?

sophia-guo · 2024-09-06T15:36:16Z

external/dockerfile_functions.sh

            # 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}"


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?

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.

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.

sophia-guo · 2024-09-06T15:47:43Z

What is required to double check to make this pass through?

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.

judovana · 2024-09-06T16:16:27Z

What is required to double check to make this pass through?

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.

judovana · 2024-09-10T13:36:33Z

jacoco jdk21: fails coorectly (needs different jacaoco sources: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10846/console

15:36:05  [ERROR] Source option 7 is no longer supported. Use 8 or later.
15:36:05  [ERROR] Target option 7 is no longer supported. Use 8 or later.

(todo, fix??)
jacoco jdk17: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10842/tapResults/
jacoco jdk11: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10845/tapResults/
jacoco jdk8: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10844/tapResults/

judovana · 2024-09-10T14:08:31Z

jacoco jdk21: fails coorectly (needs different jacaoco sources: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10846/console
15:36:05  [ERROR] Source option 7 is no longer supported. Use 8 or later.
15:36:05  [ERROR] Target option 7 is no longer supported. Use 8 or later.
(todo, fix??)

#5575

judovana · 2024-09-10T15:14:33Z

tomcats on jdk11 (both)
https://ci.adoptium.net/view/Test_grinder/job/Grinder/10849/console
https://ci.adoptium.net/view/Test_grinder/job/Grinder/10850/console

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.

LongyuZhang

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
``

judovana · 2024-09-11T07:39:20Z

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?

judovana · 2024-09-12T15:30:27Z

@LongyuZhang I'm trying something like:

sh build_image.sh  jacoco 17 openj9 ubi jdk full linux_x86-64 default 0

and

sh build_image.sh  criu-functional 17 openj9 ubi jdk full linux_x86-64 default 0

And you seems to be right, it is ARG IMAGE=docker.io/library/eclipse-temurin:openj9-full and then of course the apt is called. Do you also see wrong ARG_IMAGE pelase?

judovana · 2024-09-28T05:40:30Z

The pacakges were moved to global scope. As you could have noticed, all - except your usecase - is passing.
Those two changes can not be separated - In the moment I'm want to support generic containers - thus at least fedora, rhel, centos and ubuntu, I need to specify packages for each of them, becasue the names and sets of packages are different. Hand in hand with it, I do not want to duplicate declarations of most common packages.

Hardcoded ubis are most terrible to adjust to the generic mages, because they were if-else-hardcoded to original code without much thinking. @LongyuZhang please provide reproducers to your case. I would really love to fix it. It seems it is the only failing one.

judovana · 2024-09-28T06:24:03Z

When I'm in maste the test you are pointing out dies:

export BUILD_LIST=external/criu-ubi-portable-checkpoint ; export EXTRA_DOCKER_ARGS="-v $TEST_JDK_HOME:/opt/java/openjdk" ;  export TEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk-11.0.24.0.8-2.portable.jdk.el.x86_64/
make compile
...
    [exec] DOCKERIMAGE_TAG nightly has been recognized.
     [exec] No credential available for container registry, will proceed without login...
     [exec] sudo podman pull ${env.DOCKER_REGISTRY_URL}/ubi8-with-criu/linux_x86-64-ubi8-criu:latest
     [exec] Error: parsing reference "${env.DOCKER_REGISTRY_URL}/ubi8-with-criu/linux_x86-64-ubi8-criu:latest": invalid reference format

When it is in this pr then it dies absolutely the same.

…eneric all `git wget perl make` install fie on all tried OSes

judovana · 2024-09-28T06:47:54Z

From that I'm guessing that the issue will be in function getMatchingPackagesKeys() { and/or function getImageOs() { , because I'm reading OS here and it seems the os is linux_x86-64-ubi8-criu. The packages declaration is (fedora|ubi|rhel|centos)_packages="git wget perl make" and if anything would go ok, the .*(fedora|ubi|rhel|centos).* matches linux_x86-64-ubi8-criu so something went wrong in this particular usecase, where hardocded UBIs are present. Msot likely the EXTERNAL_AQA_IMAGE, which is workaround for that if-ubi-if-critu unhappy blocks, was not set in time. But without reproducer or full verbose log, I can just guess

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 criu-portable-checkpoint or to improbable future when criu will be tested on system with different package names

You may try again. please?

judovana · 2024-09-28T07:20:09Z

the

 sh build_image.sh  criu-functional 17 openj9 ubi jdk full linux_x86-64 linux_x86-64-ubi8-criu  0

correctly, although I dont know if it is at least close to reperoducer

LongyuZhang · 2024-09-30T20:49:03Z

Re: #5553 (comment)
With the new update, the test passed (hyc_grinder 43691).

Since the CRIU tests have passed, I will leave this PR for @sophia-guo @smlambert to check on their side then. Thanks.

judovana · 2024-10-01T09:48:01Z

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!!

LongyuZhang · 2024-10-04T15:15:27Z

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.

judovana · 2024-10-07T14:11:36Z

@smlambert @sophia-guo hello! Anything more is needed?

smlambert

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).

external/README.md

smlambert · 2024-10-07T14:47:40Z

external/common.properties

@@ -0,0 +1,6 @@
+# those packages are included in all tests and all distributions


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.

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).

@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.

external/build_all.sh

external/external.sh

judovana · 2024-10-07T17:55:34Z

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).

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 if left, which distinguish apt and dnf systems. I would like to get rid also of it no longer run (another property in test.properties I guess). But this PR is already too big, so not (yet)added.
In addition, that support expected to pass each part of container URI as it separated aprts, which - considering number of those parts - was very uneasy to read, use or enhance.

Co-authored-by: Shelley Lambert <[email protected]>

judovana · 2024-10-31T10:41:53Z

decalred -> declared fixed. Any luxk with build_all.sh ?

judovana · 2024-10-31T11:01:21Z

fixed waht seemed to be fixable...
@smlambert Any news on "the hold" ? It keeps to be missing feature...
@sophia-guo You are ok with it as it is, or any more hints?

Thanx a lot both!

external/build_all.sh

external/common_functions.sh

karianna · 2024-11-05T21:11:08Z

external/dockerfile_functions.sh

@@ -212,6 +224,9 @@ print_ant_install() {
    local file=$1
    local ant_version=$2
    local os=$3
+    if isExternalImageEnabled ; then
+        os=$(getImageOs)


on line 512 this os variable is in Uppercase - which one should it be?

They have nothing in common. The OS was introduced 4 years ago, os was introduced 2 years ago. They both are local. I handle the same additional logic to both of them, as both share the value of current os. I can refactor to be both lowercase (As are other local variables). But I do not strong preffernece, nor do I want to add more complexity to this PR. Still Sharp eye you have. TY!

Both probably share that theirs authors are originally form Asia origin :)

karianna · 2024-11-05T21:12:08Z

external/external.sh

+   # ================= WARNING ================= WARNING ================= WARNING ================= #"
+else
+  echo \
+"   # Info Info  Info Info Info Info Info Info Info Info Info Info Info Info Info Info Info Info Info #


Will this whitespacing produce a consistent output?

I had aligned itto that "====" format Shelly suggested. They provide as consistent output as possible. Do you have any more exact concern with them? Like being to long or simiarly?

karianna · 2024-11-05T21:12:31Z

external/jacoco/test.properties

@@ -3,5 +3,5 @@ test_command="mvn clean verify"
 test_results="testResults"
 tag_version="v0.8.7"
 environment_variable="MODE=java"
-ubuntu_packages="git wget tar"
 maven_version="3.8.5"


Can we move to 3.9.9?

No way in this PR:( But otherwise I would love to.

external/provider.sh

Co-authored-by: Martijn Verburg <[email protected]>

judovana added 8 commits September 2, 2024 08:24

Added initial concept of configurable image for external

37d0c30

Added proper defautls to survive with -u

0ae0514

Added forgotten use of providers in cleaning of images for build_all

e415d58

Removed usage of variable in method name

5047445

to make it readable, find-able and more generic

excluded EXTERNAL_AQA_IMAGE from build_all.sh

3a1b749

Now honor EXTERNAL_AQA_CONTAINER_CLEAN also after external run

9f3f031

Now using the userset image (if set)

520656f

judovana marked this pull request as draft September 4, 2024 18:07

judovana added 2 commits September 5, 2024 17:03

Added set of warning for case the mounted jdk is missing or TEST_JAVA…

0a3f3db

… is not part of the mount commnad

judovana marked this pull request as ready for review September 5, 2024 15:08

karianna marked this pull request as draft September 6, 2024 07:13

sophia-guo reviewed Sep 6, 2024

View reviewed changes

judovana marked this pull request as ready for review September 9, 2024 09:46

judovana mentioned this pull request Sep 10, 2024

add logic for jdk-version specific suites #5575

Open

LongyuZhang reviewed Sep 10, 2024

View reviewed changes

Added example of to build_image.sh

b1e20d1

Changed decvlaration of pacakges in criu-ubi-portable-checkpoint to g…

172a1f5

…eneric all `git wget perl make` install fie on all tried OSes

Improved exit message for unknown os

0839a27

This was referenced Sep 28, 2024

Support external tests run without docker #2026

Open

remove external/build_all.sh script #5657

Open

karianna requested a review from LongyuZhang September 28, 2024 22:57

karianna requested a review from smlambert October 1, 2024 00:41

smlambert requested changes Oct 7, 2024

View reviewed changes

better readme for EXTERNAL_AQA_IMAGE

673b223

Co-authored-by: Shelley Lambert <[email protected]>

karianna requested a review from smlambert October 7, 2024 23:22

smlambert mentioned this pull request Oct 8, 2024

document and narrow usage of criu tests #5658

Open

judovana added 2 commits October 31, 2024 11:59

decalred -> declared

849d2bd

Removed the nasty warning warning... by nicer header

b7f25af

judovana force-pushed the globalImageForExternal branch from 1e631e5 to b7f25af Compare October 31, 2024 11:00

karianna requested changes Nov 5, 2024

View reviewed changes

judovana and others added 4 commits November 6, 2024 13:34

Update external/build_all.sh

14dd897

Co-authored-by: Martijn Verburg <[email protected]>

Update external/common_functions.sh

d8acc30

Co-authored-by: Martijn Verburg <[email protected]>

Added whitesapces between # and first char

a1db200

More === text ===.. isntead of text text text

e4b11c4

judovana requested a review from karianna November 6, 2024 15:40

		@@ -0,0 +1,6 @@
		# those packages are included in all tests and all distributions

Global image for external #5553

Are you sure you want to change the base?

Global image for external #5553

Conversation

judovana commented Sep 4, 2024 • edited by sophia-guo Loading

judovana commented Sep 4, 2024

sophia-guo commented Sep 4, 2024

judovana commented Sep 5, 2024

judovana commented Sep 5, 2024

judovana commented Sep 5, 2024

sophia-guo Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophia-guo commented Sep 6, 2024 • edited Loading

judovana commented Sep 6, 2024

judovana commented Sep 10, 2024

judovana commented Sep 10, 2024

judovana commented Sep 10, 2024

LongyuZhang left a comment

Choose a reason for hiding this comment

judovana commented Sep 11, 2024

judovana commented Sep 12, 2024 • edited Loading

judovana commented Sep 28, 2024 • edited Loading

judovana commented Sep 28, 2024

judovana commented Sep 28, 2024 • edited Loading

judovana commented Sep 28, 2024

LongyuZhang commented Sep 30, 2024

judovana commented Oct 1, 2024

LongyuZhang commented Oct 4, 2024

judovana commented Oct 7, 2024

smlambert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana commented Oct 7, 2024

judovana commented Oct 31, 2024

judovana commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana commented Sep 4, 2024 •

edited by sophia-guo

Loading

sophia-guo Sep 6, 2024 •

edited

Loading

sophia-guo commented Sep 6, 2024 •

edited

Loading

judovana commented Sep 12, 2024 •

edited

Loading

judovana commented Sep 28, 2024 •

edited

Loading

judovana commented Sep 28, 2024 •

edited

Loading