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

Crypto test original target now allows to run kereberos tests #5248

Merged
merged 1 commit into from
May 10, 2024

Conversation

judovana
Copy link
Contributor

@judovana judovana commented Apr 19, 2024

if both
SKIP_AGENT_TESTS=false
AGENT_HOSTNAME=some.krb.enabled.agent
are provided, the 5 kerberos based tests will run too

part of #5246

@judovana
Copy link
Contributor Author

@llxia
Copy link
Contributor

llxia commented Apr 19, 2024

CryptoTests is limited to vendor=redhat. Please use Red Hat SDK in Grinder for testing.

@judovana
Copy link
Contributor Author

judovana commented Apr 19, 2024

It shuld not be. Both targets should be available to all vendors. The diffference requiring second target was in OS, not JDK.

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

Running identical tests twice within a build inefficiently consumes both machine and triage resources. Please be aware that we may execute these tests across various versions/platforms/impls multiple times daily. It's essential to strictly limit duplicate tests.

We have already discussed this in #5206 and #5240 (comment)

If your perspective differs, please feel free to have additional discussion in #5206

if both
  SKIP_AGENT_TESTS=false
  AGENT_HOSTNAME=some.krb.enabled.agent
are provided, the 5 kerberos based tests will run too
@judovana
Copy link
Contributor Author

@judovana
Copy link
Contributor Author

judovana commented Apr 19, 2024

Running identical tests twice within a build inefficiently consumes both machine and triage resources. Please be aware that we may execute these tests across various versions/platforms/impls multiple times daily. It's essential to strictly limit duplicate tests.

We have already discussed this in #5206 and #5240 (comment)

If your perspective differs, please feel free to have additional discussion in #5206

ok.

CryptoTests is limited to vendor=redhat. Please use Red Hat SDK in Grinder for testing.

What is the magical keyword and where to write that please?

@llxia
Copy link
Contributor

llxia commented Apr 19, 2024

What is the magical keyword and where to write that please?

There is no magical keyword. We just need to run CryptoTests with Red Hat SDK. It means setting the URL via CUSTOMIZED_SDK_URL in Grinder.

@judovana
Copy link
Contributor Author

What is the magical keyword and where to write that please?

There is no magical keyword. We just need to run CryptoTests with Red Hat SDK. It means setting the URL via CUSTOMIZED_SDK_URL in Grinder.

TYVm!

@judovana
Copy link
Contributor Author

Gosh. It seems to be biting our own tail - https://ci.adoptium.net/view/Test_grinder/job/Grinder/9721/console - the RH jdk is behind adoptium-marketplace login it seems.

@judovana
Copy link
Contributor Author

Hmm.. tis one https://ci.adoptium.net/view/Test_grinder/job/Grinder/9722/ was from own mirror. Any ideas?

@smlambert
Copy link
Contributor

smlambert commented Apr 19, 2024

SDK_RESOURCE=customized

but it won't matter, because #4374
(no other vendors package .xz, so we have not yet needed to support it)

@judovana
Copy link
Contributor Author

Ah. Sure. TY!

@judovana
Copy link
Contributor Author

I had repacked the file by gz but https://ci.adoptium.net/view/Test_grinder/job/Grinder/9744/console still fails. It seems tobe downloading, but there is no trace to failure. Also I had noticed possibly buggy behaviour: #5254

@judovana
Copy link
Contributor Author

Fixed ginder (thanx @smlambert !) https://ci.adoptium.net/view/Test_grinder/job/Grinder/9797/ is green

@judovana judovana requested a review from llxia April 25, 2024 12:23
@judovana
Copy link
Contributor Author

Thanx, can we mere now please?

@karianna
Copy link
Contributor

@llxia Will need to re-review first.

@llxia
Copy link
Contributor

llxia commented May 1, 2024

Where do we set SKIP_AGENT_TESTS and AGENT_HOSTNAME?

@judovana
Copy link
Contributor Author

judovana commented May 1, 2024

We don't, but user should be able to do so.

Eg I have agent created, so I will pass it on before launching aqavit, and crypto tests it will run also kerberos tests.

I'm working on ability to set up local kerberos server before starting the suite, and although i have setup of it done, it is still very distatn future.

@llxia
Copy link
Contributor

llxia commented May 1, 2024

The check is in https://github.com/rh-openjdk/CryptoTest/blob/master/run.sh#L124. Instead of adding if [ "x${SKIP_AGENT_TESTS}" = "x" ] ; then export SKIP_AGENT_TESTS=1 ; fi ; \ , I think we just need to remove the hardcoded line SKIP_AGENT_TESTS=1 in the original playlist <command>.

@judovana
Copy link
Contributor Author

judovana commented May 6, 2024

Thanx for eyball! I'm definitely ok to adjsut run.sh, but as yo wrote it it somehow do not fit.
I would actually like to rework all the SKIP_AGENT_TESTS/AGENT_HOSTNAME but it is so wide spread... But Maybe I need just bigger poke:)

The global idea should be:

  • the kerberos tests in CryptoTests are on by Default
    • but one must provide AGENT_HOSTNAME
    • the suite should yield if there is no host name and they are on
  • they can be easily disabled by SKIP_AGENT_TESTS set to "false"

Afaik it is how it is now

In aqa-tests

  • the kerberos tests shoudl be disabled by default, unless user enables them
    • if they are enabled, one must extrnaly provide AGENT_HOSTNAME
  • It should not reintroduce new variable, the SKIP_AGENT_TESTS shoudl be reused.

Which I hope this PR is doing.

So IIUC, dropping of SKIP_AGENT_TESTS=1 in original play-lists will evolve to fail in both cases.

@judovana
Copy link
Contributor Author

Ping please?

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

approve this is partial change for #5246.

@llxia llxia requested a review from smlambert May 10, 2024 19:11
@smlambert smlambert merged commit f76dfc8 into adoptium:master May 10, 2024
2 checks passed
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.

4 participants