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

Remove jtreg duplicated crypto call #5134

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

judovana
Copy link
Contributor

@judovana judovana commented Mar 12, 2024

Based on the experience (hopefully correct!) simplifying cryptotest runner
https://ci.adoptium.net/view/Test_grinder/job/Grinder/9093

@judovana judovana force-pushed the removeJtregDuplicatedCryptoCall branch from be0b4cd to 8ec9800 Compare March 12, 2024 13:43
@sophia-guo
Copy link
Contributor

From the grinder console output jtreg.jar are actually downloaded twice. One is downloaded by build.xml the other one is downloaded by run.sh. Can run.sh update to only download if no jtreg.jar exist?

@judovana
Copy link
Contributor Author

From the grinder console output jtreg.jar are actually downloaded twice. One is downloaded by build.xml the other one is downloaded by run.sh. Can run.sh update to only download if no jtreg.jar exist?

I had removed the build's download of jtreg. https://ci.adoptium.net/view/Test_grinder/job/Grinder/9150/

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

No, by removing this call and depending on the test material being added, in this case 'cryptotest', to fetch 'some unknown/untracked version of jtreg' we lose the ability to reproduce the exact same test material. We also set a bad precedent for other tests coming in the future that it is okay to pull in extra untracked dependencies and glut the test machines of anyone using AQAvit.

@judovana
Copy link
Contributor Author

judovana commented Mar 14, 2024

No, by removing this call and depending on the test material being added, in this case 'cryptotest', to fetch 'some unknown/untracked version of jtreg' we lose the ability to reproduce the exact same test material. We also set a bad precedent for other tests coming in the future that it is okay to pull in extra untracked dependencies and glut the test machines of anyone using AQAvit.

Sure!

Cuple of question on this topic.:

  • getJtreg ant target (which I foolhardily removed) is getting tarball and unpacking it? The path is (TEST_RESROOT)$(D)jtreg$(D) ?
    • what is the version of this jtreg? Can that be controlled? Is jdk-version dependent?
    • I'm intentionally pulling binary which contains openjdk/jtreg@8c105f2 which is not yet tagged as release. Can that be obtained?
    • the change is superimportant especially for ssl tests (and well for anything shell based) to have any reasonable output in traces
    • the change is unlikely to bubble to older releases.

Where I absolutely agree with what you wrote, few things I would like to understand:

  • foreign repo is cloned anyway, there can be anything. Eg until recently there was in tree copy of jtreg.
  • the repo (any repo) can, and is pulling other stuff. So why to forbid this exact dependence?
  • the owner of the suite, should - as they know best - ensure what version of depndencies to pull.
    • in case of jtreg, as generic runner this is futile sentence as jtreg.jar is pretty compatible, and have sense to be shared.
    • I had bad experience (maybe erroneous) with too new jtregs used on some ancient tests. That ended in having jdk8 frozen on older jtreg branch. Maybe that is no longer necessary, especially here, but the risk is there.

@smlambert
Copy link
Contributor

smlambert commented Mar 14, 2024

The jtreg that gets pulled and used certainly depends on the JDK version, and AQAvit will pull the 'right one' based on JDK_VERSION.

If we are assessing which foreign repos to include into AQAvit, one criteria would be that they integrate well into the system we have in place. My bad for not seeing that you were pulling your own copy of jtreg, that was a failure to review closely during my reviews.

When dovetailing into AQAvit, the point is to centralize functionality. If contributors still want to have the option to download a bespoke version of jtreg, then that can be controlled through an option passed to the test suite. Of the ~10 companies using the tests being added into AQAvit, the expectation is to know what is being downloaded onto their test machines and that the AQAvit project does some vetting/verification of those dependencies.

When does this openjdk/jtreg@8c105f2 land into an official tagged/versioned release of jtreg? If it is super-important, then it will be important that its into an official version, that we can then plan to pick up.

@judovana
Copy link
Contributor Author

@judovana
Copy link
Contributor Author

The jtreg that gets pulled and used certainly depends on the JDK version, and AQAvit will pull the 'right one' based on JDK_VERSION.

cool! There is also https://ci.adoptium.net/view/Dependencies/job/dependency_pipeline/lastSuccessfulBuild/artifact/jtreg/jtregtip.tar.gz ; any way to pull this?

If we are assessing which foreign repos to include into AQAvit, one criteria would be that they integrate well into the system we have in place. My bad for not seeing that you were pulling your own copy of jtreg, that was a failure to review closely during my reviews.

It is only on those ssl/crypto runner PRs. At it is impossibel to see without properly reading thre run.sh.
Also I guess I shold have highlight it, as in my own runner I'm setting JTREG_HOME for my runs, not wanting it to download anything. But then this was pushed away by my overrated love to in openjdk/jtreg@8c105f2

Kudos for @sophia-guo for noticing that!

When dovetailing into AQAvit, the point is to centralize functionality. If contributors still want to have the option to download a bespoke version of jtreg, then that can be controlled through an option passed to the test suite. Of the ~10 companies using the tests being added into AQAvit, the expectation is to know what is being downloaded onto their test machines and that the AQAvit project does some vetting/verification of those dependencies.

I agree for sure. Can it be subject of discussion for some very special cases and deps? but this is absolutely not that case.

When does this openjdk/jtreg@8c105f2 land into an official tagged/versioned release of jtreg? If it is super-important, then it will be important that its into an official version, that we can then plan to pick up.

No idea. Jtreg release is random AFAICT. Maybe around jdk24? And wq can be sure it will not reach the 6.* branch... Any possibility to build our fork in the dependency_pipeline ? To prevent misunderstanding - I do not recommend that, I do not insists on that, I do not want you to do that. Especially not on short run:) It is really just idea!

@smlambert
Copy link
Contributor

There is also https://ci.adoptium.net/view/Dependencies/job/dependency_pipeline/lastSuccessfulBuild/artifact/jtreg/jtregtip.tar.gz ; any way to pull this?

Yes, let's plan to add jtreg tip to the list of ones that can be used with AQAvit testing, in that way it satisfies both our needs.

@judovana
Copy link
Contributor Author

TYVM! Can this go in in meantime? (+5136 and maybe 5129 :)

@smlambert smlambert merged commit 10ac449 into adoptium:master Mar 15, 2024
2 checks passed
@pshipton
Copy link
Contributor

There is now a failure in OpenJ9 builds which I assume is caused by this change.
eclipse-openj9/openj9#19164

21:44:11  Test results: passed: 32
21:44:11  Report written to /Users/jenkins/workspace/Test_openjdk11_j9_extended.functional_aarch64_mac_Nightly_testList_0/aqa-tests/functional/security/Crypto/CryptoTest/test.1710553196/jdk/JTreport/html/report.html
21:44:11  Results written to /Users/jenkins/workspace/Test_openjdk11_j9_extended.functional_aarch64_mac_Nightly_testList_0/aqa-tests/functional/security/Crypto/CryptoTest/test.1710553196/jdk/JTwork
21:44:11  ~/workspace/Test_openjdk11_j9_extended.functional_aarch64_mac_Nightly_testList_0/aqa-tests/TKG/output_17105515401191/CryptoTests_0
21:44:11  readlink: illegal option -- f
21:44:11  usage: readlink [-n] [file ...]
21:44:11  Missing tests.log!
21:44:11  -----------------------------------
21:44:11  CryptoTests_0_FAILED

@pshipton
Copy link
Contributor

Failed differently on zlinux.
https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_extended.functional_s390x_linux_Nightly_testList_0/682

23:05:51  Error:  cryptotest/CryptoTest.java
23:05:51  Test results: passed: 31; error: 1
23:05:51  Report written to /home/jenkins/workspace/Test_openjdk17_j9_extended.functional_s390x_linux_Nightly_testList_0/aqa-tests/functional/security/Crypto/CryptoTest/test.1710557092/jdk/JTreport/html/report.html
23:05:51  Results written to /home/jenkins/workspace/Test_openjdk17_j9_extended.functional_s390x_linux_Nightly_testList_0/aqa-tests/functional/security/Crypto/CryptoTest/test.1710557092/jdk/JTwork
23:05:51  Error: Some tests failed or other problems occurred.
23:05:51  ~/workspace/Test_openjdk17_j9_extended.functional_s390x_linux_Nightly_testList_0/aqa-tests/TKG/output_17105480336205/CryptoTests_0
23:05:51  renamed '/home/jenkins/workspace/Test_openjdk17_j9_extended.functional_s390x_linux_Nightly_testList_0/aqa-tests/functional/security/Crypto/CryptoTest/test.1710557092.tar.gz' -> './test.1710557092.tar.gz'
23:05:51  -----------------------------------
23:05:51  CryptoTests_0_FAILED

@smlambert
Copy link
Contributor

There is now a failure in OpenJ9 builds which I assume is caused by this change.

Issue introduced by this change rh-openjdk/CryptoTest#63

using readlink -f which must not be present in all versions of readlink on all machines
https://www.gnu.org/software/coreutils/manual/html_node/readlink-invocation.html#readlink-invocation

@smlambert
Copy link
Contributor

Created #5151 with the plan to pin to specific versions of these test materials so we are not at the mercy of random changes.

@pshipton
Copy link
Contributor

Excluded the test on OpenJ9 amac, zlinux via #5153

sophiaxu0424 pushed a commit to sophiaxu0424/aqa-tests that referenced this pull request Mar 19, 2024
* Removed jtreg call by the upstream wrapper

which is doing moreover the same

* moved out of the tmpresults

* Removed build's download of jtreg as suite pulls its own

* Returned back to aqavit's jtregs withotu downloading custom one

Which will make
openjdk/jtreg@8c105f2
missing again, but that - a tleast for jdk bigger then 8, should be eg
from https://ci.adoptium.net/job/dependency_pipeline/lastSuccessfulBuild/artifact/jtreg/jtregtip.tar.gz
llxia added a commit to llxia/openjdk-tests that referenced this pull request Apr 18, 2024
- add CryptoTests_jtreg back (see adoptium#5134)
- CryptoTests_jtreg directly triggers the tests via jtreg and it runs against all vendors except redhat
- CryptoTests only runs for vendor=redhat
- add retry for git clone to mitigate network issue
- add check CryptoTest repo sha

Signed-off-by: Lan Xia <[email protected]>
llxia added a commit to llxia/openjdk-tests that referenced this pull request Apr 18, 2024
- add CryptoTests_jtreg back (base on the original change adoptium#5134)
- CryptoTests_jtreg directly triggers the tests via jtreg and it runs against all vendors except redhat
- CryptoTests only runs for vendor=redhat
- add retry for git clone to mitigate network issue
- add check CryptoTest repo sha

Signed-off-by: Lan Xia <[email protected]>
llxia added a commit to llxia/openjdk-tests that referenced this pull request Apr 18, 2024
- add CryptoTests_jtreg back (based on the original change adoptium#5134)
- CryptoTests_jtreg directly triggers the tests via jtreg and it runs against all vendors except redhat
- CryptoTests only runs for vendor=redhat
- re-enable CryptoTests_jtreg on aarch64_mac|x86-64_mac|s390x_linux
- add retry for git clone to mitigate network issue
- add check CryptoTest repo sha (sha will be captured in TAP file)

Signed-off-by: Lan Xia <[email protected]>
@llxia llxia mentioned this pull request Apr 18, 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.

5 participants