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.

Sign up for GitHub

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

Jump to bottom

Added inital hook to allow both docker/podman and sudo/runas/"" #5460

Merged
merged 22 commits into from
Aug 16, 2024
Merged

Added inital hook to allow both docker/podman and sudo/runas/"" #5460

merged 22 commits into from
Aug 16, 2024

Conversation

judovana
Copy link
Contributor

This is initial shot on, moreover following lesson learned in https://github.com/adoptium/temurin-build/pull/3796/files and https://github.com/adoptium/temurin-build/pull/3869/files

Before proceeding with ore tuning, wdyt?

@judovana judovana marked this pull request as draft July 29, 2024 16:31
@judovana judovana force-pushed the pomanForExternal branch 5 times, most recently from 3f147e2 to 3d81b27 Compare July 30, 2024 14:12
@judovana judovana marked this pull request as ready for review July 30, 2024 16:19
@judovana
Copy link
Contributor Author

This looks ready for review. I do not claim it is feature completed, but seems to work. Especially the 0b4a57a troubles me

The cleanup of images loks broken (but afaik was also before):

podman rmi -f //11-hotspot-ubuntu-linux_x86-64--:
podman rmi -f /:

but only:

$ podman images
REPOSITORY                               TAG                         IMAGE ID      CREATED        SIZE
localhost/adoptopenjdk-lucene-solr-test  11-jdk-ubuntu-hotspot-full  37a5f93b6688  9 minutes ago  1.74 GB

around

@judovana
Copy link
Contributor Author

judovana commented Jul 30, 2024 •
edited
Loading

Looks like somebody fixed the 0b4a57a in meantime. Rebased. (original changes persisted in #5461)

karianna
karianna requested changes Jul 31, 2024
View reviewed changes
external/README.md Outdated Show resolved Hide resolved
external/README.md Outdated Show resolved Hide resolved
external/README.md Outdated Show resolved Hide resolved
external/README.md Outdated Show resolved Hide resolved
external/provider.sh Outdated Show resolved Hide resolved
external/provider.sh Outdated Show resolved Hide resolved
external/provider.sh Outdated Show resolved Hide resolved
@judovana judovana requested a review from karianna July 31, 2024 13:40
@judovana judovana mentioned this pull request Jul 31, 2024
Closed
@judovana
Copy link
Contributor Author

judovana commented Aug 1, 2024

Ok, the network issue was casued by jdk itself. (wrong jdk with symlik to cacerts). This PR is thus feature compelted and works fine

sophia-guo
sophia-guo reviewed Aug 9, 2024
View reviewed changes
external/README.md Outdated Show resolved Hide resolved
sophia-guo
sophia-guo reviewed Aug 9, 2024
View reviewed changes
external/jenkins/test.sh Outdated Show resolved Hide resolved
sophia-guo
sophia-guo reviewed Aug 9, 2024
View reviewed changes
external/scala/test.sh Outdated Show resolved Hide resolved
sophia-guo
sophia-guo reviewed Aug 9, 2024
View reviewed changes
external/external.sh Outdated
if [[ ${test} == 'external_custom' ]]; then
test="$(echo ${EXTERNAL_CUSTOM_REPO} | awk -F'/' '{print $NF}' | sed 's/.git//g')"
fi
if [ "${EXTERNAL_AQA_CLEAN}" == "false" ] ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the default of EXTERNAL_AQA_CLEAN? How would it be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the variable I introduced. If y oou have any better idea I would happily use it.
The makefiles are calling the clean at the end, and I it is very often undesired to lost the results. I have not found a better way how to propagate the "dont clean" to the makefiles (which are calling this script).

One other way led to several if statemnts around calling of the external.sh. The direct variable seemed better. I had also wrote it to the header and readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be changed as any other env. variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

For tkg story env variable has to be visible and set by build.xml. See other env variables in build.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not running in container. This is the part which is launching the contianer (just to ensure we are bothj in same page).

This variable is not set nowhere. It is up to user of build.xml to slightly modify the behaviour. You do not want to run the runs we clean off. You want to run clean on. However for debugging, it was of immense help, to disallow make _target to clean after itself pretty often.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand it's the part of launching the container. Take /lucene-solr for example, after the test run the image is cleaned by $(TEST_ROOT)$(D)external$(D)external.sh --clean --tag "${DOCKERIMAGE_TAG}" --dir lucene-solr. My question is how the variable EXTERNAL_AQA_CLEAN be set and visible to make _lucene_solr_nightly_smoketest? To support If EXTERNAL_AQA_CLEAN is false, then the image is not cleaned I think more changes are needed. Maybe you can separate that from this PR.

Copy link
Contributor Author

@judovana judovana Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

something like this:

EXTERNAL_AQA_CLEAN=false make _lucene_solr_nightly_smoketest
or
EXTERNAL_AQA_CLEAN=false  make _jacoco_test

works perfectly fine. Aka the final images are not clean after the run. Very good thing is, that they are cleared on next startup anyway.
If this is kept, I can then

podman run --privileged  -v /usr/lib/jvm/java-11-openjdk-11.0.19.0.7-1.fc37.portable.jdk.x86_64/:/opt/java/openjdk  --name jacoco-test --rm -i --entrypoint /bin/bash  adoptopenjdk-jacoco-test:11-jdk-ubuntu-hotspot-full
or
podman run --privileged  -v /usr/lib/jvm/java-11-openjdk-11.0.19.0.7-1.fc37.portable.jdk.x86_64/:/opt/java/openjdk  --name lucene-solr-test --rm -i --entrypoint /bin/bash  adoptopenjdk-lucene-solr-test:11-jdk-ubuntu-hotspot-full

Moreover out of the box, and immediately debug without any more interventions.

Thanx!

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. EXTERNAL_AQA_CLEAN=false make _lucene_solr_nightly_smoketest won't work for jenkins story without other changes. It will work locally and seems mainly needed for debug. Suggest update docs.

@judovana
Added inital hook to allow both docker/podman and sudo/runas/""
cfb048c
@judovana
Better subshell quoting
55bed93
@judovana
Added correct title to dockerfile
b800b8c
@judovana
being explicit on default temurin image origin
2e5fd08 
     [exec] INFO:  podman build  --no-cache -t adoptopenjdk-lucene-solr-test:11-jdk-ubuntu-hotspot-full -f /home/jvanek/git/jvmtest/external/lucene-solr/dockerfile/11/jdk/ubuntu/Dockerfile.hotspot.full /home/jvanek/git/jvmtest/external/
     [exec] #####################################################
     [exec] STEP 1/21: FROM eclipse-temurin:11-jdk
     [exec] Error: creating build container: short-name resolution enforced but cannot prompt without a TTY

Which needs to resolve. The docker.io/library
is already ised in external/external.sh:
     docker_image_name="docker.io/library/eclipse-temurin:${JDK_VERSION}-jdk"
@judovana
Fixed target name in README
0e3c9ab
@judovana
Runtime can now use podman too
8affe4f
@judovana
Added ability to not clean after run
dc254dc
@judovana
Honour JAVA_TOOLS_OPTIONS in external tests
ad185b5
@judovana
Copy link
Contributor Author

java_tool_options moved to separate PR - #5498 - Thanx for that!

@judovana judovana requested a review from sophia-guo August 12, 2024 13:21
sophia-guo
sophia-guo approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@sophia-guo sophia-guo left a comment

Choose a reason for hiding this comment

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

Based on the discussion EXTERNAL_AQA_CLEAN --> EXTERNAL_IMAGE_CLEAN might be better. But will leave to @judovana to make decision.

@judovana
Copy link
Contributor Author

Based on the discussion EXTERNAL_AQA_CLEAN --> EXTERNAL_IMAGE_CLEAN might be better. But will leave to @judovana to make decision.

Yes please, I will do the change. Do not merge yet.

@judovana
Copy link
Contributor Author

Renamed. Thanx!

smlambert
smlambert reviewed Aug 14, 2024
View reviewed changes
external/README.md Outdated Show resolved Hide resolved
external/README.md Outdated Show resolved Hide resolved
external/external.sh Outdated Show resolved Hide resolved
@judovana
Copy link
Contributor Author

Gosh fixed. Sorry.

@judovana
Copy link
Contributor Author

I didi it as whole to have one single commit.

@smlambert
Copy link
Contributor

I do not see any Grinder jobs using this branch. We should launch a few to verify this PR.

@judovana
Copy link
Contributor Author

Right you are! here you go: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10755/console that is my branch, on ubuntu.

failed https://ci.adoptium.net/view/Test_grinder/job/Grinder/10756/console is master, on rhel7 (faield also with my branch:( )

There is (in both!) podmanVersion : bash: podman: command not found Any idea waht it comes from? Grep is silent....

@judovana
Copy link
Contributor Author

WIll rerun grinder

@judovana
Copy link
Contributor Author

https://ci.adoptium.net/view/Test_grinder/job/Grinder/10760/console pAAs

@karianna karianna requested a review from smlambert August 15, 2024 22:37
@judovana
Copy link
Contributor Author

hi! I will be now away for a week, and @pmikova @jandrlik and @rcap are waiting for this change. Please merge if possible

@sophia-guo sophia-guo merged commit dcef912 into adoptium:master Aug 16, 2024
2 checks passed
@judovana judovana mentioned this pull request Nov 11, 2024
Open
@smlambert smlambert mentioned this pull request Nov 20, 2024
Merged
@judovana judovana mentioned this pull request Nov 21, 2024
Closed
@smlambert smlambert mentioned this pull request Nov 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophia-guo sophia-guo sophia-guo approved these changes

@karianna karianna karianna approved these changes

@smlambert smlambert Awaiting requested review from smlambert

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@judovana @smlambert @karianna @sophia-guo