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

Fix the Keycloak memory issue on Podman setting JAVA_OPTS_APPEND #1389

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

jcarranzan
Copy link
Contributor

Summary

This PR addresses the issue quarkus-qe/quarkus-test-suite#2106
where Podman environments were unable to correctly enforce memory limits for Keycloak containers. The solution applies a conditional setting for environments using Podman by adding container.withEnv("JAVA_OPTS_APPEND", "-XX:MaxRAM=1g");, ensuring that the Java heap memory allocation respects the 1GB limit in these environments.

It won' be necessary this https://github.com/quarkus-qe/quarkus-test-suite/pull/2125/files in test suite.

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

@jcarranzan jcarranzan requested a review from rsvoboda October 30, 2024 11:50
Copy link
Contributor

@mabartos mabartos left a comment

Choose a reason for hiding this comment

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

@jcarranzan Thanks for the PR!

If it's verified the container limit set via the testcontainer's withMemory() does not work with Podman, but does with Docker, we can go this direction.

As a temporary solution, it works for me as well, but it'd be good to verify whether setting the limit really works with Docker and is not opinionated by available memory on the agent (Docker 16GB vs Podman 8GB).

@jcarranzan jcarranzan force-pushed the fix-podman-keycloak-memory branch from 6140ae7 to 7c151a7 Compare October 30, 2024 13:31
Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

I was expecting JAVA_OPTS_APPEND approach to be applied always, no matter if it's podman or docker. Can you elaborate why did you pick if-else approach?

@jcarranzan jcarranzan force-pushed the fix-podman-keycloak-memory branch from 93e0e92 to 281e7c6 Compare October 30, 2024 13:51
@jcarranzan jcarranzan force-pushed the fix-podman-keycloak-memory branch from 286db88 to 2cf1b7e Compare October 30, 2024 14:51
Copy link
Contributor

@mabartos mabartos left a comment

Choose a reason for hiding this comment

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

@jcarranzan LGTM. I hope the JAVA_OPTS_APPEND is not set somewhere else. Please, let me know if it works as expected.

@jcarranzan
Copy link
Contributor Author

jcarranzan commented Oct 31, 2024

JAVA_OPTS_APPEND

Yes @mabartos , it worked when I tested it on the Jenkins Podman job.
As I mentioned above in the description, once this MR is merged, we should just remove this property :
https://github.com/quarkus-qe/quarkus-test-suite/blob/27e6dbcb22319e740a3c065ee7d748053ea61f23/http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HttpAdvancedIT.java#L20

@jcarranzan jcarranzan requested a review from rsvoboda October 31, 2024 07:57
@rsvoboda rsvoboda merged commit e45d602 into quarkus-qe:main Oct 31, 2024
7 checks passed
@rsvoboda
Copy link
Member

@jcarranzan if this is intended for backport, please set appropriate label

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.

3 participants