Ignore JDBC Drivers artifacts by mabartos · Pull Request #22443 · keycloak/keycloak

mabartos · 2023-08-14T19:01:05Z

ghost · 2023-09-05T12:59:55Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.admin.concurrency.ConcurrentLoginTest#concurrentLoginSingleUser

Keycloak CI - New Store IT (hot-rod)

java.lang.RuntimeException: There were failures in threads. Failures count: 1
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:122)
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:63)
	at org.keycloak.testsuite.admin.concurrency.ConcurrentLoginTest.concurrentLoginSingleUser(ConcurrentLoginTest.java:145)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost · 2023-09-13T10:59:26Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithInvalidSignatureCRL

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

ghost

Unreported flaky test detected, please review

mabartos · 2023-09-13T10:59:57Z

@ahus1 @vmuzikar Could you please check it? Thanks

ahus1

Thank you for this change. Will this prevent some of the warning messages we've seen earlier when a DB driver is removed? What are the performance benefits of this change - there is always the cost to make it more difficult to maintain.

I could see the need for some users to change from an included DB driver to a self-provided DB driver. Would this change enable this, or is it unrelated?

While the list of drivers looks correct to me, I leave the final call if the added complexity is smaller than the benefits that we would expect here.

For anyone visiting this in the future, I'd like to see a at least a very short summary of the expected benefits.

mabartos · 2023-09-14T15:05:40Z

@ahus1 Thanks for your points. There are performance benefits, indeed. I'll describe it more tomorrow.

mabartos · 2023-09-15T14:33:40Z

This change brings quite a lot of benefits to us. First of all, the execution of build, static-init, and runtime build steps is done only for the specified DB. By this change, Quarkus is not aware of any other JDBC extension except the one for the chosen DB, so there's no need to execute these build steps, which could produce additional beans, service providers, configuration handling, agroal connection settings, and much more.

To summarize, we can prevent the execution of unused build steps, create additional resources, reduce memory allocations, GC executions, C1/C2 executions...

I've made a brief analysis of the performance (few executions followed by a comparison):

Startup

The significant improvement in startup is for AgroalProcessor as we saved ~80ms (cnt of CPU samples decreased by ~66%).
The overall improvement is ~100ms.

Heap

I've encountered a small peek during the startup
I assume objects provided during the build steps are destroyed after a few GC executions.
There's no peek anymore

Old

New

GC - GC executions reduced about ~20%.
Compiler - compilations reduced about ~13%

@ahus1

Will this prevent some of the warning messages we've seen earlier when a DB driver is removed?

Yes, and much more as the build steps for different JDBC extensions are not executed at all.

I could see the need for some users to change from an included DB driver to a self-provided DB driver. Would this change enable this, or is it unrelated?

I think it's unrelated to this PR as the logic behind using the custom JDBC drivers has not changed.

EDIT:
Specifying the JDBC Drivers JARs to be excluded has a huge impact on the memory allocations and utilized CPU samples. Should be included.

ahus1 · 2023-11-02T16:56:50Z

I abstain from voting here, I leave it to the CND team to decide.

mabartos · 2023-11-03T09:33:15Z

I just wanted to add my thought to the certain performance/maintainability controversion.

What's the worst that can happen? If some of the transitive dependencies (the JDBC driver JAR) change in the Quarkus extension and it's not reflected here, it'll just not be ignored and will be processed. So, we would end up in the same state as we're now in Keycloak main ;)

But there's a tiny probability it'll happen, as it'd be reflected in the Quarkus release notes, and it will not be changed so often anyway(if almost ever).

It just seems to me weird that if a user wants to use Postgres DB, the stuff related to Oracle, MS-SQL, MariaDB,... is processed. We should avoid such situations.

I'm a little bit surprised it unleashed such a broad conversation about it and took a considerable amount of effort to promote it :)

@vmuzikar @pedroigor I'd be glad for your input.

Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

ghost · 2023-11-20T15:52:53Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.account.AccountRestServiceTest#updateConsentForClientWithPut

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.account.AccountRestServiceTest.updateConsentForClientWithPut(AccountRestServiceTest.java:1377)
...

Report flaky test

org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClientWithPut

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError: 
type
Expected: is "GRANT_CONSENT"
     but: was "UPDATE_CONSENT"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
...

Report flaky test

org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClient

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError: 
type
Expected: is "GRANT_CONSENT"
     but: was "UPDATE_CONSENT"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
...

Report flaky test

ghost

Unreported flaky test detected, please review

vmuzikar

@mabartos Thanks for the elaborate analysis.

While the perf difference not that impressive from my perspective, I see relatively low risk in introducing this optimization with acceptable maintenance costs of it.

Of course it's not ideal as the final goal here is to have some more generic capability to exclude unwanted/unused bits from runtime and even the distribution based on selected features. But that's certnainly not something we can prioritize now.

I'm going to approve but I'd like to get a green tick from @pedroigor as well was involved in the discussion too, IIRC.

This reverts commit d0bfbde. Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>

Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

* Ignore JDBC Drivers artifacts (#22443) Closes #22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Fix obtaining removed artifacts from the configuration Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com>

Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com> Signed-off-by: ShefeeqPM <86718986+ShefeeqPM@users.noreply.github.com>

This reverts commit d0bfbde. Signed-off-by: Václav Muzikář <vmuzikar@redhat.com> Signed-off-by: ShefeeqPM <86718986+ShefeeqPM@users.noreply.github.com>

* Ignore JDBC Drivers artifacts (keycloak#22443) Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Fix obtaining removed artifacts from the configuration Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com> Signed-off-by: ShefeeqPM <86718986+ShefeeqPM@users.noreply.github.com>

ahus1 mentioned this pull request Aug 15, 2023

Remove Oracle Database JDBC driver from the Keycloak distribution #22453

Closed

ahus1 reviewed Aug 17, 2023

View reviewed changes

Comment thread quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/IgnoredArtifacts.java Outdated

vmuzikar mentioned this pull request Aug 23, 2023

Handle ignored artifacts separately #21440

Merged

mabartos self-assigned this Aug 24, 2023

mabartos force-pushed the ignoreJdbc branch 3 times, most recently from 72d4e17 to f22ac9f Compare September 5, 2023 11:28

ghost added the flaky-test label Sep 5, 2023

This was referenced Sep 5, 2023

Flaky test: org.keycloak.testsuite.forms.BrowserFlowTest#testAlternativeNonInteractiveExecutorInSubflow #22644

Closed

Flaky test: org.keycloak.testsuite.model.ConcurrentHashMapStorageTest#testStorageSeparation #22645

Closed

ghost reviewed Sep 5, 2023

View reviewed changes

mabartos force-pushed the ignoreJdbc branch 3 times, most recently from 7ded5c3 to eabf44d Compare September 13, 2023 09:38

ghost reviewed Sep 13, 2023

View reviewed changes

mabartos marked this pull request as ready for review September 13, 2023 10:59

mabartos requested review from a team as code owners September 13, 2023 10:59

ghost added team/cloud-native labels Sep 13, 2023

mabartos added the team/store label Sep 13, 2023

ahus1 reviewed Sep 13, 2023

View reviewed changes

vmuzikar requested review from Pepo48, shawkins and vmuzikar September 14, 2023 12:28

mabartos force-pushed the ignoreJdbc branch 2 times, most recently from 5130d1b to 45c2ced Compare November 20, 2023 10:30

This was referenced Nov 20, 2023

Flaky test: org.keycloak.testsuite.adapter.servlet.OfflineServletsAdapterTest#testServlet #21536

Closed

Flaky test: org.keycloak.testsuite.webauthn.account.WebAuthnTransportLocaleTest#multipleTransports #23884

Closed

Ignore JDBC Drivers artifacts

36b2da7

Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

mabartos force-pushed the ignoreJdbc branch from 45c2ced to 36b2da7 Compare November 20, 2023 14:38

ghost reviewed Nov 20, 2023

View reviewed changes

vmuzikar approved these changes Nov 22, 2023

View reviewed changes

pedroigor approved these changes Nov 22, 2023

View reviewed changes

shawkins approved these changes Nov 22, 2023

View reviewed changes

vmuzikar merged commit d0bfbde into keycloak:main Nov 22, 2023

vmuzikar mentioned this pull request Nov 22, 2023

Revert "Ignore JDBC Drivers artifacts" #24938

Merged

vmuzikar added a commit that referenced this pull request Nov 22, 2023

Revert "Ignore JDBC Drivers artifacts (#22443)"

9d37bad

This reverts commit d0bfbde. Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>

vmuzikar added a commit that referenced this pull request Nov 22, 2023

Revert "Ignore JDBC Drivers artifacts (#22443)" (#24938)

33dd830

This reverts commit d0bfbde. Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>

vmuzikar pushed a commit to mabartos/keycloak that referenced this pull request Dec 1, 2023

Ignore JDBC Drivers artifacts (keycloak#22443)

8eccff0

Closes keycloak#22442 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

Uh oh!

Conversation

mabartos commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Sep 5, 2023

Unreported flaky test detected

org.keycloak.testsuite.admin.concurrency.ConcurrentLoginTest#concurrentLoginSingleUser

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 13, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithInvalidSignatureCRL

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

mabartos commented Sep 13, 2023

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

mabartos commented Sep 14, 2023

Uh oh!

mabartos commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahus1 commented Nov 2, 2023

Uh oh!

mabartos commented Nov 3, 2023

Uh oh!

ghost commented Nov 20, 2023

Unreported flaky test detected

org.keycloak.testsuite.account.AccountRestServiceTest#updateConsentForClientWithPut

org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClientWithPut

org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClient

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mabartos commented Aug 14, 2023 •

edited

Loading

mabartos commented Sep 15, 2023 •

edited

Loading