Add clustering tests to new test framework #40283

mhajas · 2025-06-05T14:26:50Z

This is a replacement PR for #40127

I am sorry for confusion, I didn't realize I won't be able to push to that PR if I open it from Pedro's fork.

mhajas · 2025-06-05T14:29:05Z

This Pr addresses all comments from @stianst in #40127

stianst · 2025-06-06T06:15:58Z

tests/base/pom.xml

+    <profiles>
+        <profile>
+            <id>mixed-cluster</id>
+            <activation>
+                <activeByDefault>false</activeByDefault>
+            </activation>
+            <properties>
+                <kc.test.server>cluster</kc.test.server>
+            </properties>
+        </profile>
+    </profiles>
+


Guessing this is a left-over from when the tests where in tests/base and can be removed now?

Suggested change

<profiles>

<profile>

<id>mixed-cluster</id>

<activation>

<activeByDefault>false</activeByDefault>

</activation>

<properties>

<kc.test.server>cluster</kc.test.server>

</properties>

</profile>

</profiles>

If it's intentional and you want to be able to run the regular tests with a mixed cluster that would probably require the clustering to support a reverse-proxy, and you'd just run it with:

KC_TEST_SERVER=cluster mvn test

One of the main aims of the new testsuite is to don't have a ton of Maven profiles for things, but just use env variables instead.

This was not intentional. Pedro added it during POC and I missed this was there

stianst · 2025-06-06T06:16:36Z

tests/base/pom.xml

+        <dependency>
+            <groupId>org.keycloak.testframework</groupId>
+            <artifactId>keycloak-test-framework-clustering</artifactId>
+            <version>999.0.0-SNAPSHOT</version>
+        </dependency>


Did you intend to add this here?

Sorry, I completely missed this file during cleanup

stianst · 2025-06-06T06:22:52Z

...rk/core/src/main/java/org/keycloak/testframework/database/AbstractContainerTestDatabase.java


    protected JdbcDatabaseContainer<?> container;

    public AbstractContainerTestDatabase() {
        reuse = Config.getValueTypeConfig(TestDatabase.class, "reuse", false, Boolean.class);
+        internal = Config.getValueTypeConfig(TestDatabase.class, "internal", false, Boolean.class);


I'd do this automatically somehow rather than require configuring it. Maybe add getJdbcUrl(boolean remote) or something then update the KeycloakServer to fetch the right one depending on running in container or not.

Done. I used Config.getSelectedSupplier(KeycloakServer.class) and then used internal it when cluster is used. See https://github.com/keycloak/keycloak/pull/40283/files#diff-b2bf46ba37f5aa78f7d9a5943737f8dc80fda02931bf7714aa9846145d2920daR42-R52

I was thinking about adding it as a method on AbstractKeycloakServerSupplier so each KeycloakServer supplier can configure whether it runs in container or not. But I needed to add a new method to Registry that can provide Supplier instances for given class. It then looked something like this:

Supplier<KeycloakServer, ?> kcSupplier = instanceContext.getRegistry().findSupplierByType(KeycloakServer.class); TestDatabase database = instanceContext.getValue(); // If both KeycloakServer and TestDatabase run in container, we need to configure Keycloak with internal // url that is accessible within docker network if (database instanceof AbstractContainerTestDatabase containerDatabase && kcSupplier instanceof AbstractKeycloakServerSupplier abstractKcSupplier && abstractKcSupplier.runsInContainer() ) { return serverConfig.options(containerDatabase.serverConfig(true)); } return serverConfig.options(database.serverConfig());

Let me know what you prefer. To me it seems this second approach is complicating it too much.

stianst · 2025-06-06T06:27:03Z

...ork/clustering/src/main/java/org/keycloak/testframework/clustering/LoadBalancerSupplier.java

+        if (!(server instanceof ClusteredKeycloakServer)) {
+            throw new IllegalStateException("Load balancer can only be used with ClusteredKeycloakServer");
+        }
+
+        return new LoadBalancer((ClusteredKeycloakServer) server);


Nitpick, but I like this approach (disclaimer: suggestion written in GitHub UI, so may not compile):

Suggested change

if (!(server instanceof ClusteredKeycloakServer)) {

throw new IllegalStateException("Load balancer can only be used with ClusteredKeycloakServer");

}

return new LoadBalancer((ClusteredKeycloakServer) server);

if (server instanceof ClusteredKeycloakServer clusteredServer) {

return new LoadBalancer(clusteredServer);

} else {

throw new IllegalStateException("Load balancer can only be used with ClusteredKeycloakServer");

}

stianst · 2025-06-06T06:28:44Z

quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/DockerKeycloakDistribution.java

Haven't reviewed changes to this file, perhaps ping someone from the @keycloak/cloud-native team to take a look?

Yes I will make sure we have review from them before merging.

I'll take a look.

@Pepo48, there are some failures in CI related to changes in DockerKeycloakDistribution we will look into it.

mhajas · 2025-06-10T09:04:39Z

@Pepo48 I believe this PR is ready for review. The failed windows tests should not be related to this PR.

I would like to squash commits and make it ready for review. Do you prefer to review the changes now with commits history or I can do it now?

Pepo48 · 2025-06-11T14:28:02Z

@mhajas feel free to squash it first. Thanks!

Closes keycloak#39962 Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas · 2025-06-11T16:09:10Z

Thanks @Pepo48! I rebased, squashed and CI is now green.

Just to make sure, we don't need a review for the test-framework changes from you. We need a cloud-native eye on the DockerKeycloakDistribution changes

Pepo48

thanks @mhajas for the improvements. I found nothing that would block the PR in it's current stated.

I suggested somewhat cosmetic NPE checks to add, but I'll leave it up to your consideration.

During the review I spotted two unrelated issues to the changes in this PR (and indeed cosmetic) in the class:

on the line no. 226 I guess we can use logical OR instead of bitwise OR
on the line no. 276 there is a typo in the word "schecdule"

We can either fix it right away or address it separately in a follow up.

One way or another I'm approving the PR. Thanks again!

Pepo48 · 2025-06-12T15:34:58Z

quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/DockerKeycloakDistribution.java

@@ -292,4 +341,8 @@ public void clearEnv() {
        this.envVars.clear();
    }

+    public int getMappedPort(int port) {
+        return keycloakContainer.getMappedPort(port);


Does it make sense to add a null check? I can imagine a scenario, where KeycloakContainer could be null and subsequently getMappedPort could be called.

quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/DockerKeycloakDistribution.java

Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas · 2025-06-13T11:28:30Z

@Pepo48 Thank you for the review. I addressed your comments except the one in copyProvider method. Please approve if you agree with my reasoning.

Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas mentioned this pull request Jun 5, 2025

Add server type cluster to test-framework #40127

Closed

stianst reviewed Jun 6, 2025

View reviewed changes

keycloak-github-bot bot added the flaky-test label Jun 9, 2025

keycloak-github-bot bot mentioned this pull request Jun 9, 2025

Flaky test: org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode #39429

Closed

Add clustering tests to new test framework

8cc782a

Closes keycloak#39962 Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas force-pushed the fork/pruivo/t_mix_cluster branch from 4e34c08 to 8cc782a Compare June 11, 2025 14:35

mhajas marked this pull request as ready for review June 11, 2025 14:37

mhajas requested review from a team as code owners June 11, 2025 14:37

keycloak-github-bot bot added the team/cloud-native label Jun 11, 2025

Pepo48 previously approved these changes Jun 12, 2025

View reviewed changes

Address review comments

a268df5

Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas dismissed Pepo48’s stale review via a268df5 June 13, 2025 11:27

Fix wrongly built image

7ba0e53

Signed-off-by: Michal Hajas <mhajas@redhat.com>

Pepo48 approved these changes Jun 13, 2025

View reviewed changes

ahus1 enabled auto-merge (squash) June 13, 2025 18:25

ahus1 approved these changes Jun 13, 2025

View reviewed changes

ahus1 merged commit d2f4635 into keycloak:main Jun 13, 2025
80 checks passed

Add clustering tests to new test framework #40283

Add clustering tests to new test framework #40283

Uh oh!

Conversation

mhajas commented Jun 5, 2025

Uh oh!

mhajas commented Jun 5, 2025

Uh oh!

stianst Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stianst Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhajas commented Jun 10, 2025

Uh oh!

Pepo48 commented Jun 11, 2025

Uh oh!

mhajas commented Jun 11, 2025

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhajas commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

stianst Jun 6, 2025 •

edited

Loading

stianst Jun 6, 2025 •

edited

Loading