Repository navigation
Implement load shedding - #24684
Implement load shedding#24684
Conversation
9f3d2e9 to
2f25f61
Compare
There was a problem hiding this comment.
Thank you @vmuzikar for this change. The parameter is still called http-max-concurrent-requests while it sets the quarkus.thread-pool.queue-size which is different: The original PR assumed that concurrent was by using a sempaphore "running plus queued", while the new implementation set the only the "queued". So I'd argue "concurrent" is misleading here.
So I suggest to rename this to http-max-queued-requests. We would also like to change the thread pool size as well, so we might need another parameter like http-max-running-requests. While this could be added in another PR, we should be at least sure about its name to avoid renaming the first one.
|
@ahus1 Thanks for the review! Renamed
I'd rather address this in a separate PR. If I understand correctly, the thread pool is adjusted automatically based on number of CPUs available. Not sure we really need to support overriding this unless there's a specific and common enough use case for it. |
The default thread pool size is per default 200 or even more depending on the number of CPUs. When running load tests, we found that this default regularly exceeds the capacity of the JGroup thread pool for Infinispan. For a three-pod Keycloak deployment, we actually recommend to reduce the number of Quarkus threads to 66. See https://www.keycloak.org/keycloak-benchmark/kubernetes-guide/latest/running/keycloak-deployment I still agree that this should be a separate PR. |
There was a problem hiding this comment.
Thank you for the changes. The docs and the CLI option looks now good to me.
|
@ahus1 Thanks, created a follow-up: #24704
@mabartos I don't consider a PR fully ready for review until tests are passing. Which currently can't be as we're missing the Quarkus upgrade. But it's up to discussion. ;) |
7ab65c5 to
48a3d57
Compare
|
This is now ready for review, Quarkus upgrade was merged. |
Closes keycloak#23340 Co-authored-by: Alexander Schwartz <aschwart@redhat.com> Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>
48a3d57 to
5e7d3fb
Compare
|
@ahus1 Do you want to give it another look? There should be no real changes, just a rebase with resolved conflicts. |
Unreported flaky test detectedIf 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#updateConsentForClientWithPutKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClientWithPutKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.account.AccountRestServiceTest#createConsentForClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) |
There was a problem hiding this comment.
Unreported flaky test detected, please review
There was a problem hiding this comment.
Thank you for the change. I tested it locally and it works as expected: Once the queue size is exceeded, Keycloak responds with a 503.
As we have a workaround for the liveness probe, this will still return 200 in this case. The readiness probe will fail if it needs to access the DB connection and can't determine the status of the database from the connection metrics. The overall health check fails as it is still blocking. All of this is expected.
Approving!
Closes #23340
Requires: #24639With the recent fixes in Quarkus, we can rely with load shedding on their implementation with high enough confidence.
This PR also includes a small smoke test to at least check the option is mapped properly. It does not aim to perform full verification of this feature as it's implemented in Quarkus.