bitflicker64 commented on code in PR #3258:
URL: https://github.com/apache/hugegraph/pull/3258#discussion_r4173234066
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -121,13 +121,43 @@ if [ "$JAVA_OPTIONS" = "" ]; then
echo "Failed to start HugeGraphServer, requires at least ${MIN_MEM}MB
free memory" >> "${OUTPUT}"
exit 1
fi
- JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m -XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=${LOGS} ${USER_OPTION}"
+ JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m ${USER_OPTION}"
# Rolling out detailed GC logs
#JAVA_OPTIONS="${JAVA_OPTIONS} -XX:+UseGCLogFileRotation
-XX:GCLogFileSize=10M -XX:NumberOfGCLogFiles=3 \
# -Xloggc:./logs/gc.log -XX:+PrintHeapAtGC
-XX:+PrintGCDetails -XX:+PrintGCDateStamps"
fi
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds.
They go
+# first so a value from JAVA_OPTIONS, -j or _JAVA_OPTIONS, all later on the
command
+# line, still wins. JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS are read before the
command
+# line, so a flag set there is left out here instead of being overridden.
+# A restarted container often reuses the JVM's PID, and HotSpot will not
overwrite an
+# existing crash log or heap dump, so the file names also carry the launch
time. The
+# script execs java below, so $$ is the JVM's PID.
+LAUNCH_STAMP=$(date +%Y%m%d-%H%M%S)
+CRASH_OPTIONS=""
+SET_HEAP_DUMP="true"
+SET_HEAP_DUMP_PATH="true"
+SET_ERROR_FILE="true"
+for OPTION in ${JAVA_TOOL_OPTIONS:-} ${JDK_JAVA_OPTIONS:-}; do
Review Comment:
**Preserve quoted and argument-file JVM overrides.** Shell word splitting
does not parse the JVM's quoting rules. For example,
`JAVA_TOOL_OPTIONS='"-XX:-HeapDumpOnOutOfMemoryError"'` is valid Java input,
but this loop sees the literal quotes, misses the opt-out, and adds
`-XX:+HeapDumpOnOutOfMemoryError` on the command line, which wins. Quoted
diagnostic paths and options loaded through `JDK_JAVA_OPTIONS=@file` have the
same problem. Conversely, flag-like text inside a quoted system-property value
can incorrectly suppress a default. This can unexpectedly enable large heap
dumps or send crash files somewhere other than the operator's persistent mount.
Prepend the defaults to the existing `JAVA_TOOL_OPTIONS` instead and leave
parsing/precedence to the JVM; a real-JVM regression check should cover these
cases.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-java-security-properties.sh:
##########
@@ -432,6 +442,85 @@ assert_argument "true" "$ENABLED_CAPTURE"
assert_argument "-Doperator.marker=preserved" "$ENABLED_CAPTURE"
assert_no_argument '^-D(networkaddress\.cache\.ttl|sun\.net\.inetaddr\.ttl)=' \
"$ENABLED_CAPTURE"
+assert_argument "-XX:+HeapDumpOnOutOfMemoryError" "$ENABLED_CAPTURE"
+# The names carry the launch time, since a restarted container often reuses the
+# PID and HotSpot will not overwrite an existing crash log or heap dump.
+LOGS_PATTERN=$(printf '%s' "${SERVER_ROOT}/logs" | sed
's/[][\.*^$+?(){}|]/\\&/g')
+assert_argument_matching \
+
"^-XX:HeapDumpPath=${LOGS_PATTERN}/java_pid[0-9]+_[0-9]{8}-[0-9]{6}\.hprof$" \
+ "$ENABLED_CAPTURE"
+assert_argument_matching \
+ "^-XX:ErrorFile=${LOGS_PATTERN}/hs_err_pid%p_[0-9]{8}-[0-9]{6}\.log$" \
+ "$ENABLED_CAPTURE"
+
+# Heap dumps and JVM crash logs must stay in logs/ when JAVA_OPTIONS replaces
+# the default heap options, and an operator's own paths must still win.
+CUSTOM_OPTIONS_CAPTURE="${TEMP_DIR}/custom-java-options.args"
+CAPTURE_FILE="$CUSTOM_OPTIONS_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JAVA_OPTIONS="-Xmx1g -XX:ErrorFile=/operator/hs_err.log \
+ -XX:HeapDumpPath=/operator/dumps" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_argument "-Xmx1g" "$CUSTOM_OPTIONS_CAPTURE"
+assert_argument "-XX:+HeapDumpOnOutOfMemoryError" "$CUSTOM_OPTIONS_CAPTURE"
+assert_argument_matching "^-XX:HeapDumpPath=${LOGS_PATTERN}/"
"$CUSTOM_OPTIONS_CAPTURE"
+LAST_ERROR_FILE_ARGUMENT=$(grep -E '^-XX:ErrorFile=' \
+ "$CUSTOM_OPTIONS_CAPTURE" | tail -n 1)
+if [[ "$LAST_ERROR_FILE_ARGUMENT" != "-XX:ErrorFile=/operator/hs_err.log" ]];
then
+ fail "operator -XX:ErrorFile was overridden by the launcher default"
+fi
+LAST_HEAP_DUMP_PATH_ARGUMENT=$(grep -E '^-XX:HeapDumpPath=' \
+ "$CUSTOM_OPTIONS_CAPTURE" | tail -n 1)
+if [[ "$LAST_HEAP_DUMP_PATH_ARGUMENT" != "-XX:HeapDumpPath=/operator/dumps"
]]; then
+ fail "operator -XX:HeapDumpPath was overridden by the launcher default"
+fi
+
+# JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS are read before the command line, so
+# the launcher must leave out a flag already set there rather than override it.
+TOOL_OPTIONS_CAPTURE="${TEMP_DIR}/tool-java-options.args"
+CAPTURE_FILE="$TOOL_OPTIONS_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JAVA_TOOL_OPTIONS="-XX:ErrorFile=/operator/hs_err.log" \
+ JDK_JAVA_OPTIONS="-XX:-HeapDumpOnOutOfMemoryError" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_no_argument '^-XX:ErrorFile=' "$TOOL_OPTIONS_CAPTURE"
+assert_no_argument '^-XX:[+-]HeapDumpOnOutOfMemoryError$'
"$TOOL_OPTIONS_CAPTURE"
+assert_argument_matching "^-XX:HeapDumpPath=${LOGS_PATTERN}/"
"$TOOL_OPTIONS_CAPTURE"
+
+# With telemetry on, the launcher adds its agent to JAVA_TOOL_OPTIONS. It must
+# keep the operator's flags there, or the -XX:ErrorFile left out above is lost.
+OT_EXPECTED_MD5=$(grep -E '^ *expected_md5=' "$SERVER_SCRIPT" | cut -d'"' -f2)
+MOCK_MD5_BIN="${TEMP_DIR}/mock-md5-bin"
+mkdir -p "$MOCK_MD5_BIN"
+printf '#!/bin/bash\necho "%s $1"\n' "$OT_EXPECTED_MD5" >
"${MOCK_MD5_BIN}/md5sum"
+chmod +x "${MOCK_MD5_BIN}/md5sum"
+OT_JAR_PATH="${SERVER_ROOT}/plugins/opentelemetry-javaagent.jar"
+OT_JAR_EXISTED="false"
+if [[ -e "$OT_JAR_PATH" ]]; then
+ OT_JAR_EXISTED="true"
+else
+ mkdir -p "${SERVER_ROOT}/plugins"
+ : > "$OT_JAR_PATH"
+fi
+TELEMETRY_CAPTURE="${TEMP_DIR}/telemetry.args"
+TELEMETRY_TOOL_OPTIONS="${TEMP_DIR}/telemetry-tool-options.txt"
+CAPTURE_FILE="$TELEMETRY_CAPTURE"
TOOL_OPTIONS_CAPTURE_FILE="$TELEMETRY_TOOL_OPTIONS" \
+ JAVA_HOME="$MOCK_JAVA_HOME" PATH="${MOCK_MD5_BIN}:${PATH}" \
+ JAVA_TOOL_OPTIONS="-XX:ErrorFile=/operator/hs_err.log" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true "" ""
true \
+ >/dev/null
+if [[ "$OT_JAR_EXISTED" == "false" ]]; then
Review Comment:
**Clean up the telemetry fixture on failure too.** With `set -e`, a nonzero
exit from the launcher above skips this removal and leaves the empty
`plugins/opentelemetry-javaagent.jar` in the supplied distribution. A later
real telemetry startup sees the file, skips downloading it, and fails its
checksum check. Add the conditional removal to the existing EXIT cleanup, while
preserving any JAR that existed before the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
