Unreported flaky test detected

If the 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.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

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

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

Thank you for this PR. What I'm reading here is that you're about to expose the Kubernetes constructs as is. When doing this, we'll offer a non-opinionated way to schedule Pods, which is IMHO ok as we don't seem to have an opinionated and agreed-upon solution.

Eventually, more coarse grained options might emerge as we learn about how this can be used in different environments. I'd say even then exposing this as a supported feature makes sense as we won't be able to foresee any possible scenario.

Even if the deployment uses an external Infinispan, there are still the embedded Infinispans for now, and therefore I'd advise the Operator not to take any caching configuration into account - at least for now. @pruivo is working an a PR which is planned for KC26 which would allow running Keycloak without its embedded distributed caches, but this is still some time in the future.

So for now, I'd say

• have a default node anti-affinity,
• ignore the caching options, and
• expose the scheduling as is.

cc: @ryanemerson

@ahus1 I'm fine with removing the defaults around spread constraints and moving that to a topic for the scaling guide.

I'm fine with removing the defaults around spread constraints and moving that to a topic for the scaling guide.

No, please add a default best-effort spread. I'm just against making the default settings dependent on the cache configuration.

No, please add a default best-effort spread. I'm just against making the default settings dependent on the cache configuration.

The default spread is just an affinity for the same zone. The pr has been updated with that.

Should just need some docs.

@vmuzikar WDYT?

Unreported flaky test detected

If the 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.oauth.TokenIntrospectionTest#testUnsupportedToken

Keycloak CI - Base IT (6)

java.lang.NullPointerException: Cannot invoke "com.fasterxml.jackson.databind.JsonNode.asBoolean()" because the return value of "com.fasterxml.jackson.databind.JsonNode.get(String)" is null
	at org.keycloak.testsuite.oauth.TokenIntrospectionTest.testUnsupportedToken(TokenIntrospectionTest.java:313)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
...

Report flaky test

@vmuzikar added the docs, should be fully ready for review now.

@ryanemerson @ahus1 @vmuzikar in the docs should we call out specifically recommending an affinity for the same zone as the database?

@shawkins

should we call out specifically recommending an affinity for the same zone as the database?

The good thing about pod affinity is that it is now (1) supported by the Operator and no longer part of the unsupported pod template and (2) we have a reasonable default.

Deploying Keycloak on the same region as the database seems to be a good idea, still I didn't tested this how much it actually impacts Keycloak. And if the database is running outside of Kubernetes, you would start coding the zone manually in the affinity rules, and not use the "try to run all pods in the same zone but I don't care which".

So I'd say it is probably good enough for now.

Once we work with this a bit more, I could see that we provide some predefined scheduling templates to hide the complexity and to promote some well-tested and well-understood scheduling methods. This would then be a follow-up PR.

+1 to Alexander's comment. I think that more advanced/opinionated setting examples should exist as part of the Keycloak HA guide or similar, where we explain in detail the trade-offs of different architectures and how they apply to specific use-cases.

@shawkins Just last minor thing about the example formatting, otherwise LGTM

Should be good now.

@mabartos I believe your comments has been resolved. I'm overriding your review request so we can merge. Once you're back, let me know if you have any further concerns, we can address them as a follow-up.