[RHPAM-4777] - Dynamic resources script is reading wrong container sy…

[spolti]

…s files on cgroupsv2

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

• Pull Request title is properly formatted: [CLOUD-XYA] Subject
• Pull Request contains link to the JIRA issue
• Pull Request contains description of the issue
• Pull Request does not include fixes for issues other than the main ticket
• Attached commits represent units of work and are properly formatted
• You have read and agreed to the Developer Certificate of Origin (DCO) (see CONTRIBUTING.md)
• Every commit contains Signed-off-by: Your Name <yourname@example.com> - use git commit -s

[jmtd]

Hey @spolti , thanks for tagging me for review. A first comment, we took a quite different approach in the openjdk images: since OpenJDK itself can now detect cgroups v1 or v2, we moved away from doing any kind of resource limit detection/setting in the wrapper scripts, and deleted container-limits script entirely. See e.g.
https://github.com/jboss-container-images/openjdk/pull/285/files: The JDK does it all automatically. We do change the OpenJDK default value for MaxRAMPercentage from 50% to 80%, but we do not set -Xms or -Xmx. Similarly we rely on the JVM auto-tuning for ParallelGCThreads, java.util.concurrent.ForkJoinPool.common.parallelism=, etc.

I see that this PR is very similar to apache/incubator-kie-kogito-images#1676, is that community/upstream?

[spolti]

Hi @jmtd, the auto tune feature is since Java 17, right?
If so, we still on Java 11 and we do keep that container limits in case GraalVM is used as well.

Yes, the second PR is Kogito upstream.

[jmtd]

Hi @jmtd, the auto tune feature is since Java 17, right?

Cgroups V2 support is in JDK8, 11 and 17. For auto-tuning, I guess it depends which variable we are talking about, but I'm fairly sure it's been in all of them for the ones above for some time.

If so, we still on Java 11 and we do keep that container limits in case GraalVM is used as well.

I don't really know anything about VM tuning for GraalVM so can't comment here!

[spolti]

Hi @jmtd, the auto tune feature is since Java 17, right?

Cgroups V2 support is in JDK8, 11 and 17. For auto-tuning, I guess it depends which variable we are talking about, but I'm fairly sure it's been in all of them for the ones above for some time.

That's nice, that can be a good addition, but I am afraid that, as RHPAM is on maintenance and I am leaving the RHPAM team, anyone will be able to migrate and test it. With that said, it could be better rely on this proposed changes.

I don't really know anything about VM tuning for GraalVM so can't comment here!

It is used by Kogito images (community only).

@radtriste @ricardozanini, thoughts on this? (could be a good addition for the future)

PS I don't know why I edited your comment instead replying it =/

[jmtd]

This path is only correct if the processes' cgroup is the root one. I.e., the result of awk -F: '{ print $3 }' /proc/$$/cgroup.

[jmtd]

It might be that this is always true in the contexts you care about, i.e., container with a java process as PID 1. It's not true on my desktop, for example, when testing the script logic:

$ cat /sys/fs/cgroup/memory.max
cat: /sys/fs/cgroup/memory.max: No such file or directory
$ awk -F: '{ print $3 }' /proc/$$/cgroup
/user.slice/user-1000.slice/session-300.scope
$ cat /sys/fs/cgroup/$(awk -F: '{ print $3 }' /proc/$$/cgroup)/memory.max
max

[spolti]

In containers, yes, I didn't spot any other different path, this should be enough.

[jmtd]

@spolti

That's nice, that can be a good addition, but I am afraid that, as RHPAM is on maintenance and I am leaving the RHPAM team

Good luck with your future projects! I think given you are leaving it makes a lot of sense to align this with community as closely as possible.

Since my product won't consume these changes, I haven't done an approval, but I wanted to run and test the code for you anyway, so I've done that and I think it's fine: my comment about the cgroup path is probably moot since I guess in the important container runtimes the process will be bound to the root cgroup and there won't be a heirarchy.

[luck3y]

@spolti Are these changes not going into main as well?

[spolti]

@spolti Are these changes not going into main as well?

Hi @luck3y, I can cherry-pick, but that depends if others wants this change on main.

[spolti]

@luck3y hi, are you ok if we merge this one?
And, would you like to have this cherry-picked to main?

[luck3y]

lgtm @spolti, I think we should at least cherry pick to main, otherwise it'll get lost.

[spolti]

ink we should at least cherry pick to main, otherwise it'll

@luck3y #424