Merge pull request #313 from jmtd/revert-OPENJDK-559

jmtd · web-flow · commit 4776cfed8a79 · 2022-07-13T16:18:10.000+01:00
Revert "Merge pull request #285 from jmtd/OPENJDK-559-MaxRAMPercentage"
diff --git a/modules/jvm/api/module.yaml b/modules/jvm/api/module.yaml
@@ -14,22 +14,13 @@ envs:
   description: User specified Java options to be appended to generated options in JAVA_OPTS.
   example: "-Dsome.property=foo"
 - name: JAVA_MAX_MEM_RATIO
-  description:  Specify the maximum heap memory. Corresponds to the JVM argument `-XX:MaxRAMPercentage`. The default is `80.0` which means 80% of the available memory. You can disable this mechanism by setting the value to `0`.
-  example: "90.0"
+  description: Is used when no `-Xmx` option is given in **JAVA_OPTS**. This is used to calculate a default maximal heap memory based on a containers restriction. If used in a container without any memory constraints for the container then this option has no effect. If there is a memory constraint then `-Xmx` is set to a ratio of the container available memory as set here. The default is `50` which means 50% of the available memory is used as an upper boundary. You can skip this mechanism by setting this value to `0` in which case no `-Xmx` option is added.
 - name: JAVA_INITIAL_MEM_RATIO
-  description:
-    Specify the initial heap memory. Corresponds to the JVM argument
-    `-XX:InitialRAMPercentage`. By default this is not specified.
-    **This is deprecated and will be removed in a future release. Users should
-    specify `-XX:InitialRAMPercentage` directly in JAVA_OPTS instead.**
-  example: "25.0"
+  description: Is used when no `-Xms` option is given in **JAVA_OPTS**. This is used to calculate a default initial heap memory based on the maximum heap memory. If used in a container without any memory constraints for the container then this option has no effect. If there is a memory constraint then `-Xms` is set to a ratio of the `-Xmx` memory as set here. The default is `25` which means 25% of the `-Xmx` is used as the initial heap size. You can skip this mechanism by setting this value to `0` in which case no `-Xms` option is added.
+  example: "25"
 - name: JAVA_MAX_INITIAL_MEM
-  description:
-    This value is passed through to the `-Xms` Java option, setting both the
-    minimum and initial heap size. By default this is unset.
-    **This is deprecated and will be removed in a future release. Users should
-    specify `-Xms` directly in JAVA_OPTS instead.**
-  example: "4096m"
+  description: Is used when no `-Xms` option is given in **JAVA_OPTS**. This is used to calculate the maximum value of the initial heap memory. If used in a container without any memory constraints for the container then this option has no effect. If there is a memory constraint then `-Xms` is limited to the value set here. The default is 4096MB which means the calculated value of `-Xms` never will be greater than 4096MB. The value of this variable is expressed in MB.
+  example: "4096"
 - name: JAVA_DIAGNOSTICS
   description: Set this to get some diagnostics information to standard output when things are happening. **Note:** This option, if set to true, will set `-XX:+UnlockDiagnosticVMOptions`. **Disabled by default.**
   example: "true"
@@ -39,6 +30,9 @@ envs:
 - name: JAVA_DEBUG_PORT
   description: Port used for remote debugging. Defaults to *5005*.
   example: "8787"
+- name: CONTAINER_MAX_MEMORY
+  description: Memory limit given to the container.
+  example: "1024"
 - name: GC_MIN_HEAP_FREE_RATIO
   description: Minimum percentage of heap free after GC to avoid expansion.
   example: "20"
diff --git a/modules/jvm/bash/artifacts/opt/jboss/container/java/jvm/container-limits b/modules/jvm/bash/artifacts/opt/jboss/container/java/jvm/container-limits
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+# Detected container limits
+# If found these are exposed as the following environment variables:
+#
+# - CONTAINER_MAX_MEMORY
+# - CONTAINER_CORE_LIMIT
+#
+# This script is meant to be sourced.
+
+max_unbounded() {
+  cat /proc/meminfo | grep 'MemTotal:' | awk '{print $2*1024}'
+}
+
+container_memory() {
+  # High number which is the max limit unit which memory is supposed to be
+  # unbounded. 
+  local mem_file="/sys/fs/cgroup/memory/memory.limit_in_bytes"
+  local max_mem_unbounded="$(max_unbounded)"
+  if [ -r "${mem_file}" ]; then
+    local max_mem="$(cat ${mem_file})"
+    if [ ${max_mem} -lt ${max_mem_unbounded} ]; then
+      echo "${max_mem}"
+    fi
+  fi
+}
+
+min() {
+  printf "%s\n" "$@" | sort -g | head -n1
+}
+
+local max_mem="$(container_memory)"
+if [ x$max_mem != x ]; then
+  export CONTAINER_MAX_MEMORY="$max_mem"
+fi
diff --git a/modules/jvm/bash/artifacts/opt/jboss/container/java/jvm/java-default-options b/modules/jvm/bash/artifacts/opt/jboss/container/java/jvm/java-default-options
@@ -5,6 +5,33 @@
 #
 # Usage: JAVA_OPTS="$(java-default-options.sh)"
 
+# Env Vars respected:
+
+# JAVA_OPTIONS: Checked for already set options.  Deprecated, use JAVA_OPTS.
+# JAVA_OPTS: Checked for already set options
+# JAVA_INITIAL_MEM_RATIO: Ratio of maximum memory to use for initial heap size
+#                         (i.e. -Xms).  Defaults to 25 (i.e -Xms=-Xmx/4).
+# JAVA_MAX_INITIAL_MEM: The maximum value of the initial heap size, defaults to 4G.
+# JAVA_MAX_MEM_RATIO: Ratio use to calculate a default maximum Memory, in percent.
+#                     E.g. the default value "50" implies that 50% of the Memory
+#                     given to the container is used as the maximum heap memory with
+#                     '-Xmx'. It is a heuristic and should be better backed up with real
+#                     experiments and measurements.
+#                     For a good overviews what tuning options are available -->
+#                             https://youtu.be/Vt4G-pHXfs4
+#                             https://www.youtube.com/watch?v=w1rZOY5gbvk
+#                             https://vimeo.com/album/4133413/video/181900266
+# Also note that heap is only a small portion of the memory used by a JVM. There are lot
+# of other memory areas (metadata, thread, code cache, ...) which addes to the overall
+# size. There is no easy solution for this, 50% seems to be are reasonable compromise.
+# However, when your container gets killed because of an OOM, then you should tune
+# the absolute values
+#
+
+__DEFAULT_JAVA_MAX_MEM_RATIO=50
+__DEFAULT_JAVA_INITIAL_MEM_RATIO=25
+__DEFAULT_JAVA_MAX_INITIAL_MEM=4096
+
 # stubs for jvm specific overrides
 jvm_specific_options() {
   :
@@ -19,25 +46,63 @@ if [ -f "${JBOSS_CONTAINER_OPENJDK_JDK_MODULE}/jvm-options" ]; then
   source "${JBOSS_CONTAINER_OPENJDK_JDK_MODULE}/jvm-options"
 fi
 
+initialize_container_limits() {
+  # we can't run without limits
+  source ${JBOSS_CONTAINER_JAVA_JVM_MODULE}/container-limits
+}
+
 # Check for memory options and calculate a sane default if not given
 max_memory() {
-  # Check if explicitly disabled
+  # Check whether -Xmx is already given in JAVA_OPTIONS. Then we dont
+  # do anything here
+  # XXX: I think this should be removed.  If folks want to hard code max/min,
+  # then they can set the ratios to zero and set the options in JAVA_OPTS_APPEND.
+  if echo "${JAVA_OPTS:-${JAVA_OPTIONS}}" | grep -q -- "-Xmx"; then
+    return
+  fi
+
+  # Check if explicitely disabled
   if [ "x$JAVA_MAX_MEM_RATIO" = "x0" ]; then
     return
   fi
-  echo "-XX:MaxRAMPercentage=${JAVA_MAX_MEM_RATIO:-80.0}"
+
+  # Check for the 'real memory size' and caluclate mx from a ratio
+  # given (default is 50%)
+  if [ "x$CONTAINER_MAX_MEMORY" != x ]; then
+    local max_mem="${CONTAINER_MAX_MEMORY}"
+    local ratio=${JAVA_MAX_MEM_RATIO:-${__DEFAULT_JAVA_MAX_MEM_RATIO}}
+    local mx=$(echo "${max_mem} ${ratio} 1048576" | awk '{printf "%d\n" , ($1*$2)/(100*$3) + 0.5}')
+    echo "-Xmx${mx}m"
+  fi
 }
 
 # Check for memory options and calculate a sane default if not given
 initial_memory() {
-  # JAVA_MAX_INITIAL_MEM is deprecated and will be removed in a future release
-  if [ -n "$JAVA_MAX_INITIAL_MEM" ]; then
-    echo "-Xms$JAVA_MAX_INITIAL_MEM"
-  else
-    # there's no point setting this if we are passing -Xms, since -Xms
-    # overrides this
-    if [ -n "$JAVA_INITIAL_MEM_RATIO" ]; then
-      echo "-XX:InitialRAMPercentage=${JAVA_INITIAL_MEM_RATIO}"
+  # Check whether -Xms is already given in JAVA_OPTS. Then we dont
+  # do anything here
+  # XXX: I think this should be removed.  If folks want to hard code max/min,
+  # then they can set the ratios to zero and set the options in JAVA_OPTS_APPEND.
+  if echo "${JAVA_OPTS:-${JAVA_OPTIONS}}" | grep -q -- "-Xms"; then
+    return
+  fi
+
+  # Check if explicitly disabled
+  if [ "x$JAVA_INITIAL_MEM_RATIO" = "x0" ]; then
+    return
+  fi
+
+  # Check for the 'real memory size' and calculate ms from a ratio
+  # given (default is 25%)
+  if [ "x$CONTAINER_MAX_MEMORY" != x ]; then
+    local max_mem="${CONTAINER_MAX_MEMORY}"
+    local max_ratio=${JAVA_MAX_MEM_RATIO:-${__DEFAULT_JAVA_MAX_MEM_RATIO}}
+    local initial_ratio=${JAVA_INITIAL_MEM_RATIO:-${__DEFAULT_JAVA_INITIAL_MEM_RATIO}}
+    local ms=$(echo "${max_mem} ${max_ratio} ${initial_ratio} 1048576" | awk '{printf "%d\n" , ($1*(($2*$3)/10000))/$4 + 0.5}')
+    local max_initial_memory=${JAVA_MAX_INITIAL_MEM:-${__DEFAULT_JAVA_MAX_INITIAL_MEM}}
+    if [ "${ms}" -lt "${max_initial_memory}" ] ; then
+      echo "-Xms${ms}m"
+    else
+      echo "-Xms${max_initial_memory}m"
     fi
   fi
 }
@@ -92,5 +157,7 @@ error_handling() {
   echo "-XX:+ExitOnOutOfMemoryError"
 }
 
+initialize_container_limits > /dev/null
+
 ## Echo options, trimming trailing and multiple spaces
 echo "$(initial_memory) $(max_memory) $(gc_config) $(diagnostics) $(error_handling)" | awk '$1=$1'
diff --git a/modules/run/artifacts/opt/jboss/container/java/run/run-java.sh b/modules/run/artifacts/opt/jboss/container/java/run/run-java.sh
@@ -92,6 +92,11 @@ load_env() {
   fi
   export JAVA_APP_DIR
 
+  # Read in container limits and export the as environment variables
+  if [ -f "${JBOSS_CONTAINER_JAVA_JVM_MODULE}/container-limits" ]; then
+    source "${JBOSS_CONTAINER_JAVA_JVM_MODULE}/container-limits"
+  fi
+
   # JAVA_LIB_DIR defaults to JAVA_APP_DIR
   export JAVA_LIB_DIR="${JAVA_LIB_DIR:-${JAVA_APP_DIR}}"
   if [ -z "${JAVA_MAIN_CLASS}" ] && [ -z "${JAVA_APP_JAR}" ]; then
diff --git a/tests/features/java/gc.feature b/tests/features/java/gc.feature
@@ -1,64 +1,71 @@
 @openjdk
+@ubi8/openjdk-8
+@ubi8/openjdk-11
+@ubi8/openjdk-17
 @redhat-openjdk-18
-@ubi8
 Feature: Openshift OpenJDK GC tests
 
   Scenario: Check default GC configuration
-    Given container is started as uid 1000
-    Then container log should contain -XX:+UseParallelGC
-    And  container log should contain -XX:MinHeapFreeRatio=10
-    And  container log should contain -XX:MaxHeapFreeRatio=20
-    And  container log should contain -XX:GCTimeRatio=4
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
 
   Scenario: Check GC_MIN_HEAP_FREE_RATIO GC configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable                         | value  |
        | GC_MIN_HEAP_FREE_RATIO           | 5      |
-    Then container log should contain -XX:MinHeapFreeRatio=5
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=5 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=5 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
 
   Scenario: Check GC_MAX_HEAP_FREE_RATIO GC configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable                         | value  |
        | GC_MAX_HEAP_FREE_RATIO           | 50     |
-    Then container log should contain -XX:MaxHeapFreeRatio=50
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=50 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=50 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
 
   Scenario: Check GC_TIME_RATIO GC configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable                         | value  |
        | GC_TIME_RATIO                    | 5      |
-    Then container log should contain -XX:GCTimeRatio=5
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=5 -XX:AdaptiveSizePolicyWeight=90
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=5 -XX:AdaptiveSizePolicyWeight=90
 
   Scenario: Check GC_ADAPTIVE_SIZE_POLICY_WEIGHT GC configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable                         | value  |
        | GC_ADAPTIVE_SIZE_POLICY_WEIGHT   | 80     |
-    Then container log should contain -XX:AdaptiveSizePolicyWeight=80
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=80
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=80
 
   Scenario: Check GC_MAX_METASPACE_SIZE GC configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable                 | value  |
        | GC_MAX_METASPACE_SIZE    | 120    |
-    Then container log should contain -XX:MaxMetaspaceSize=120m
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MaxMetaspaceSize=120m
+    And container log should contain -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MaxMetaspaceSize=120m
 
   Scenario: Check GC_CONTAINER_OPTIONS configuration
-    Given container is started with env
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
        | variable             | value        |
        | GC_CONTAINER_OPTIONS | -XX:+UseG1GC |
-    Then container log should contain -XX:+UseG1GC
+    Then s2i build log should contain Using MAVEN_OPTS -XX:+UseG1GC
+    And container log should contain -XX:+UseG1GC
     And container log should not contain -XX:+UseParallelGC
 
   Scenario: Check GC_METASPACE_SIZE GC configuration
-    Given container is started with env
-       | variable             | value  |
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
+       | variable                 | value  |
        | GC_METASPACE_SIZE    | 120    |
-    Then container log should contain -XX:MetaspaceSize=120m
+    Then s2i build log should contain -XX:MetaspaceSize=120m
+    And container log should contain -XX:MetaspaceSize=120m
     And container log should not contain integer expression expected
 
   Scenario: Check GC_METASPACE_SIZE constrained by GC_MAX_METASPACE_SIZE GC configuration
-    Given container is started with env
-       | variable                 | value |
-       | GC_METASPACE_SIZE        | 120   |
+    Given s2i build https://github.com/jboss-openshift/openshift-quickstarts from undertow-servlet
+       | variable                 | value  |
+       | GC_METASPACE_SIZE    | 120    |
        | GC_MAX_METASPACE_SIZE    | 90    |
-    Then container log should contain -XX:MaxMetaspaceSize=90m
-    And  container log should contain -XX:MetaspaceSize=90m
+    Then s2i build log should contain -XX:MaxMetaspaceSize=90m -XX:MetaspaceSize=90m
+    And container log should contain -XX:MaxMetaspaceSize=90m -XX:MetaspaceSize=90m
 
diff --git a/tests/features/java/memory.feature b/tests/features/java/memory.feature
