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

Emphasize the need for setting container limit #28729

Closed
mabartos opened this issue Apr 15, 2024 · 4 comments · Fixed by #28808 or #28812
Closed

Emphasize the need for setting container limit #28729

mabartos opened this issue Apr 15, 2024 · 4 comments · Fixed by #28808 or #28812

Comments

@mabartos
Copy link
Contributor

mabartos commented Apr 15, 2024

Description

After providing changes[1] to memory settings for the container deployments in Keycloak 24, we've faced some issues related to increased memory utilization. The JVM Heap setting has changed and uses relative values, instead of static ones specified via -Xms, and -Xmx JVM options.

At this moment, the heap is defined via the -XX:InitialRAMPercentage, and -XX:MaxRAMPercentage JVM options with values -XX:InitialRAMPercentage=50, and -XX:MaxRAMPercentage=70. It states, that the initial heap size should be 50%(it's usually less - more comprehensive topic) of the total container memory, and the maximum heap as 70% of the total container memory.

When the limit for the containerized env is not set, it computes these values from the available memory for the container, which might be the total memory of the machine/node.

Even when we already have information in docs around the limit, it seems that users are not so aware of them. We should emphasize more that the limit setting for running Keycloak in the container is very recommended.

[1] #26661

For instance, issue:
#28671

@mabartos mabartos self-assigned this Apr 15, 2024
@mabartos mabartos added this to the 25.0.0 milestone Apr 16, 2024
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 16, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 16, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 16, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos mabartos linked a pull request Apr 16, 2024 that will close this issue
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 17, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 17, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@shawkins
Copy link
Contributor

Moving comments off of the pr, which wasn't the right place to raise them - #28812 (comment)

In several places we are referencing that the garbage collector returns heap memory very reluctantly, while this seems to be the case with our default vm settings this is general is not something that should be expected.

The other concerned raised was to allow users to rely upon the previous behavior if desired. The current docs suggest using the container limit instead - I agree that this is a better approach, but isn't as straight-forward to maintain backwards compatibility.

@mabartos
Copy link
Contributor Author

In several places we are referencing that the garbage collector returns heap memory very reluctantly, while this seems to be the case with our default vm settings this is general is not something that should be expected.

Yep, you're right. However, the statement about releasing memory reluctantly is also true with the currently supported setup - with the ParallelGC and the JVM memory settings. But it does not have to be necessarily bad. It might provide a better performance in reducing GC pauses, repeatedly shrinking heap space, etc.

However, we could not emphasize it's happening VERY reluctantly, in order to prevent some negative feedback around it.

It'd be great to do some experiments around the VM options in the containerized environment when we're dealing with different heap settings. We could find some balance in throughput X memory footprint and achieve better performance. But there will probably always be some drawbacks.

The other concerned raised was to allow users to rely upon the previous behavior if desired. The current docs suggest using the container limit instead - I agree that this is a better approach, but isn't as straight-forward to maintain backwards compatibility.

Ok, we can mention it as a brief paragraph that it's possible to use the old behavior, but rather recommending setting the limit.

@shawkins WDYT?

@vmuzikar
Copy link
Contributor

Just my two cents. Since we don't explicitly support tweaking GC settings, it's perfectly fine from my perspective to document that the JVM frees memory very reluctantly. It's true for our only supported setup. We could be a bit more explicit with the wording though and make it clear that this is concerning Keycloak specific configuration.

Ok, we can mention it as a brief paragraph that it's possible to use the old behavior, but rather recommending setting the limit.

-1 for this. Unless we want to support it, I would not document it. It's fine to make breaking changes in major releases.

@shawkins
Copy link
Contributor

It's true for our only supported setup

It's currently true with our openjdk 17 images, but it may not be with Java 21, etc. I just wanted to caution that we're making a general statement that may not hold up over time.

-1 for this. Unless we want to support it, I would not document it. It's fine to make breaking changes in major releases

Sounds good.

mabartos added a commit to mabartos/keycloak that referenced this issue Apr 17, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 18, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 18, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
ahus1 pushed a commit that referenced this issue Apr 18, 2024
Closes #28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
mabartos added a commit to mabartos/keycloak that referenced this issue Apr 18, 2024
Closes keycloak#28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
ahus1 pushed a commit that referenced this issue Apr 18, 2024
Closes #28729

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment