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

Set memory limit for Keycloak container #1351

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

mabartos
Copy link
Contributor

@mabartos mabartos commented Oct 1, 2024

Closes #1350

Summary

(Summarize the problem solved by this PR, and how to verify it manually)

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Release (follows conventions described in the RELEASE.md)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

I'd propose increasing the memory limit (to 1GiB) for Keycloak instance running in a container for the test suite, as it might be more memory-consuming than the Keycloak dev service: quarkusio/quarkus#43601

Container limit is properly used in quarkus-test-suite project:

ID            NAME        CPU %       MEM USAGE / LIMIT  MEM %       NET IO        BLOCK IO           PIDS        CPU TIME      AVG CPU %
4521d014fec2  149223041   455.88%     548.9MB / 1.049GB  52.35%      1.5kB / 698B  185.4MB / 1.827MB  56          1m54.708381s  495.01%

The solution works as expected for my experiments.

It should close issue: quarkusio/quarkus#43630

But these changes should be probably backported to the quarkus-test-framework:1.5.x (or not sure what approach is used in such scenarios).

cc: @jcarranzan

@michalvavrik
Copy link
Member

@fedinskiy let's review unless you are busy; if so, pleaes request review from me again; thanks!

@michalvavrik michalvavrik requested review from fedinskiy and jcarranzan and removed request for michalvavrik October 1, 2024 15:25
@michalvavrik
Copy link
Member

Thank you @mabartos

Copy link
Contributor

@jcarranzan jcarranzan left a comment

Choose a reason for hiding this comment

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

So @mabartos with these modifications it would not be necessary to implement any changes related to JAVA_OPTS_KC_HEAP="-XX:MaxHeapFreeRatio=30 -XX:MaxRAMPercentage=65" as suggested here--> https://www.keycloak.org/server/containers#_specifying_different_memory_settings ?
Thanks.

@mabartos
Copy link
Contributor Author

mabartos commented Oct 2, 2024

@fedinskiy I added the test.

So @mabartos with these modifications it would not be necessary to implement any changes related to JAVA_OPTS_KC_HEAP="-XX:MaxHeapFreeRatio=30 -XX:MaxRAMPercentage=65" as suggested here

@jcarranzan it's not suggested. It should only emphasize the approach of setting heap settings via the env var. The default values are -XX:MaxHeapFreeRatio=70 -XX:MaxRAMPercentage=70 and they should be ok for most deployments.

@jcarranzan Does the documentation and the example seem confusing to you? I can improve the documentation a bit.

@jcarranzan
Copy link
Contributor

@fedinskiy I added the test.

So @mabartos with these modifications it would not be necessary to implement any changes related to JAVA_OPTS_KC_HEAP="-XX:MaxHeapFreeRatio=30 -XX:MaxRAMPercentage=65" as suggested here

@jcarranzan it's not suggested. It should only emphasize the approach of setting heap settings via the env var. The default values are -XX:MaxHeapFreeRatio=70 -XX:MaxRAMPercentage=70 and they should be ok for most deployments.

@jcarranzan Does the documentation and the example seem confusing to you? I can improve the documentation a bit.

Ok got it, it's not need modify docs, thank you @mabartos

@michalvavrik
Copy link
Member

run tests

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.

Set memory limit for Keycloak container
4 participants