imbajin commented on code in PR #3258:
URL: https://github.com/apache/hugegraph/pull/3258#discussion_r4230467814
##########
docker/README.md:
##########
@@ -230,6 +230,36 @@ Standalone stores RocksDB data at
`/hugegraph-server/rocksdb-data`. The HStore t
`docker compose down` keeps named-volume data. `docker compose down -v`
intentionally deletes it.
+### Server logs and crash files
+
+This section describes images built from current master. Images from 1.7.0 and
earlier do not set `STDOUT_MODE`: `docker logs` shows only the entrypoint's
output, and the service logs stay in `logs/` inside the container.
+
+Both Server images (`hugegraph/hugegraph` and `hugegraph/server`) write the
HugeGraph log to container stdout. WARN and above from Hadoop, ZooKeeper, SOFA,
Netty and Commons also reaches stdout. INFO from Hadoop, Netty and Commons, the
audit log and the slow-query log stay in files only; ZooKeeper and SOFA log
nothing below WARN. Errors that `hugegraph-server.sh` reports before Java
starts, such as an unsupported JDK or too little free memory, and fatal
bootstrap errors go to stderr and, when `logs/` can be written, also to
`hugegraph-server.log`. Earlier entrypoint steps, such as `init-store.sh` in
the HStore image, report to stderr only. Either way, when a Server exits during
startup, `docker logs` or `kubectl logs --previous` shows why. Every start also
prints `Picked up JAVA_TOOL_OPTIONS: ...` with the crash-file defaults below.
Review Comment:
🧹 Blocking: no. Summary: The banner claim is too broad: the launcher adds
the crash-file defaults after JDK, memory, and GC preflight, so those failures
exit before Java can print `Picked up JAVA_TOOL_OPTIONS`. Please qualify this
as appearing when the JVM starts. Evidence: `hugegraph-server.sh` checks these
preflight conditions before exporting the diagnostic options and executing Java.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-java-security-properties.sh:
##########
@@ -559,6 +601,344 @@ 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"
+# Heap dump and crash log defaults go first in JAVA_TOOL_OPTIONS, so the JVM's
+# own precedence lets every operator source override them. Check the values a
+# real JVM settles on when started the way a captured launcher run would start
+# it: the JAVA_TOOL_OPTIONS, JDK_JAVA_OPTIONS and _JAVA_OPTIONS the launcher
+# passed on, and the crash-related -XX: arguments of its command line.
+effective_flag() {
+ local capture="$1"
+ local flag="$2"
+ local args=()
+ local line
+ local flags
+ while IFS= read -r line; do
+ if [[ "$line" =~
^-XX:([+-]HeapDumpOnOutOfMemoryError|HeapDumpPath=|ErrorFile=) ]]; then
+ args+=("$line")
+ fi
+ done < "$capture"
+ if ! flags=$(JAVA_TOOL_OPTIONS="$(cat "${capture}.tool-options")" \
+ JDK_JAVA_OPTIONS="$(cat "${capture}.jdk-java-options")" \
+ _JAVA_OPTIONS="$(cat "${capture}.underscore-java-options")" \
+ "$JAVA_BIN" ${args[@]+"${args[@]}"} -XX:+PrintFlagsFinal
-version \
+ 2>"${capture}.flags.err"); then
+ fail "JVM rejected the options captured in ${capture}: $(cat
"${capture}.flags.err")"
+ fi
+ # The value sits between "= " and the trailing "{origin}" columns and may
+ # contain spaces.
+ # A here-string, not a pipe: awk exits early, which would kill printf with
+ # SIGPIPE under pipefail.
+ awk -v flag="$flag" '$2 == flag {
+ sub(/^[^=]*= /, ""); sub(/ *(\{[^}]*\} *)+$/, ""); print; exit }' <<<
"$flags"
+}
+
+assert_effective_flag() {
+ local capture="$1"
+ local flag="$2"
+ local pattern="$3"
+ local value
+ value=$(effective_flag "$capture" "$flag")
+ [[ "$value" =~ $pattern ]] ||
+ fail "effective ${flag} is '${value}', expected to match ${pattern}"
+}
+
+LOGS_PATTERN=$(printf '%s' "${SERVER_ROOT}/logs" | sed
's/[][\.*^$+?(){}|]/\\&/g')
+# The names carry the host name, the launch time and, if needed, a counter,
+# since a restarted container often reuses the PID, pods may share one log
+# volume, HotSpot truncates an existing crash log (JDK 17+), and it will not
+# write a heap dump over an existing file.
+# HeapDumpPath names a directory per launch, so each JVM writes its own
+# java_pid<pid>.hprof there.
+NAME_ID_PATTERN="[A-Za-z0-9._-]+_[0-9]{8}-[0-9]{6}(-[0-9]+)?"
+HEAP_DUMP_PATTERN="^${LOGS_PATTERN}/heapdump_${NAME_ID_PATTERN}$"
+ERROR_FILE_PATTERN="^${LOGS_PATTERN}/hs_err_pid%p_${NAME_ID_PATTERN}\.log$"
+
+assert_no_argument
'^-XX:([+-]HeapDumpOnOutOfMemoryError|HeapDumpPath=|ErrorFile=)' \
+ "$ENABLED_CAPTURE"
+assert_effective_flag "$ENABLED_CAPTURE" HeapDumpOnOutOfMemoryError '^true$'
+assert_effective_flag "$ENABLED_CAPTURE" HeapDumpPath "$HEAP_DUMP_PATTERN"
+assert_effective_flag "$ENABLED_CAPTURE" ErrorFile "$ERROR_FILE_PATTERN"
+ENABLED_DUMP_DIR=$(effective_flag "$ENABLED_CAPTURE" HeapDumpPath)
+[[ -d "$ENABLED_DUMP_DIR" ]] ||
+ fail "launcher did not create the heap dump directory ${ENABLED_DUMP_DIR}"
+
+# Child JVMs the Server starts inherit JAVA_TOOL_OPTIONS. Two JVMs started with
+# the launcher's options, like the Server and a computer job, must each write
+# their own heap dump into the launch directory instead of competing for one.
+OOM_SOURCE="${TEMP_DIR}/OomCheck.java"
+cat > "$OOM_SOURCE" <<'JAVA'
+import java.util.ArrayList;
+import java.util.List;
+
+public class OomCheck {
+ public static void main(String[] args) {
+ List<long[]> hold = new ArrayList<>();
+ while (true) {
+ hold.add(new long[1 << 20]);
+ }
+ }
+}
+JAVA
+OOM_DUMP_DIR="$ENABLED_DUMP_DIR"
+for OOM_RUN in 1 2; do
+ JAVA_TOOL_OPTIONS="$(cat "${ENABLED_CAPTURE}.tool-options")" \
+ "$JAVA_BIN" -Xmx64m "$OOM_SOURCE" >"${TEMP_DIR}/oom-${OOM_RUN}.log"
2>&1 || true
+done
+OOM_DUMPS=$(find "$OOM_DUMP_DIR" -maxdepth 1 -name 'java_pid*.hprof' | wc -l)
+if [[ "$OOM_DUMPS" -ne 2 ]]; then
+ ls -l "$OOM_DUMP_DIR" >&2 || true
+ cat "${TEMP_DIR}"/oom-*.log >&2 || true
+ fail "expected a heap dump per JVM in ${OOM_DUMP_DIR}, found ${OOM_DUMPS}"
+fi
+rm -f "$OOM_DUMP_DIR"/java_pid*.hprof
+
+# JAVA_OPTIONS replaces the default heap options; the crash defaults stay, and
+# paths given there 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_effective_flag "$CUSTOM_OPTIONS_CAPTURE" HeapDumpOnOutOfMemoryError
'^true$'
+assert_effective_flag "$CUSTOM_OPTIONS_CAPTURE" HeapDumpPath
'^/operator/dumps$'
+assert_effective_flag "$CUSTOM_OPTIONS_CAPTURE" ErrorFile
'^/operator/hs_err\.log$'
+
+# The JVM, not the launcher, parses the operator's JAVA_TOOL_OPTIONS: a quoted
+# opt-out works, and flag-like text inside a property value changes nothing.
+QUOTED_TOOL_CAPTURE="${TEMP_DIR}/quoted-tool-options.args"
+CAPTURE_FILE="$QUOTED_TOOL_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JAVA_TOOL_OPTIONS='"-XX:-HeapDumpOnOutOfMemoryError"
"-Dmarker=-XX:ErrorFile=/nope"' \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_effective_flag "$QUOTED_TOOL_CAPTURE" HeapDumpOnOutOfMemoryError
'^false$'
+assert_effective_flag "$QUOTED_TOOL_CAPTURE" ErrorFile "$ERROR_FILE_PATTERN"
+
+# JDK_JAVA_OPTIONS, including an @argfile, also overrides the defaults.
+ARGFILE="${TEMP_DIR}/jdk-java-options.args"
+echo '-XX:ErrorFile=/operator/argfile-hs_err.log' > "$ARGFILE"
+ARGFILE_CAPTURE="${TEMP_DIR}/argfile.args"
+CAPTURE_FILE="$ARGFILE_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JDK_JAVA_OPTIONS="@${ARGFILE}" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_effective_flag "$ARGFILE_CAPTURE" ErrorFile
'^/operator/argfile-hs_err\.log$'
+assert_effective_flag "$ARGFILE_CAPTURE" HeapDumpPath "$HEAP_DUMP_PATTERN"
+
+# _JAVA_OPTIONS comes after the command line, and -j lands on the command line
+# when JAVA_OPTIONS is unset; both override the defaults.
+UNDERSCORE_CAPTURE="${TEMP_DIR}/underscore-java-options.args"
+CAPTURE_FILE="$UNDERSCORE_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ _JAVA_OPTIONS="-XX:HeapDumpPath=/operator/underscore" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true \
+ "-XX:ErrorFile=/operator/user-option-hs_err.log" >/dev/null
+
+assert_effective_flag "$UNDERSCORE_CAPTURE" HeapDumpPath
'^/operator/underscore$'
+assert_effective_flag "$UNDERSCORE_CAPTURE" ErrorFile
'^/operator/user-option-hs_err\.log$'
+
+# A launch never reuses a name already taken in logs/, even within the same
+# second: with a fixed clock and host name, the counter moves past an existing
+# heap dump directory, a crash log and a dangling symlink. The launcher never
+# removes dump directories: an empty one from an earlier launch may still be
the
+# target of a computer-job JVM that outlived its Server.
+MOCK_DATE_BIN="${TEMP_DIR}/mock-date-bin"
+mkdir -p "$MOCK_DATE_BIN"
+printf '#!/bin/bash\necho 20200101-000000\n' > "${MOCK_DATE_BIN}/date"
+chmod +x "${MOCK_DATE_BIN}/date"
+mkdir -p "${SERVER_ROOT}/logs"
+# Only fixtures this run creates are recorded for removal, so an existing file
+# with the same name in the supplied distribution is left alone.
+add_crash_fixture() {
+ local path="$1"
+ local kind="$2"
+ if [[ ! -e "$path" && ! -L "$path" ]]; then
+ CRASH_NAME_FIXTURES+=("$path")
+ case "$kind" in
+ file) : > "$path" ;;
+ empty-dir) mkdir "$path" ;;
+ dump-dir) mkdir "$path" && : > "${path}/java_pid1.hprof" ;;
+ dangling-link) ln -s "${TEMP_DIR}/missing-target" "$path" ;;
+ esac
+ fi
+}
+USED_DUMP_DIR="${SERVER_ROOT}/logs/heapdump_test-pod-a_20200101-000000"
+OLD_EMPTY_DUMP_DIR="${SERVER_ROOT}/logs/heapdump_test-pod-a_20191231-000000"
+OTHER_HOST_DUMP_DIR="${SERVER_ROOT}/logs/heapdump_test-pod-c_20191231-000000"
+add_crash_fixture "$USED_DUMP_DIR" dump-dir
+add_crash_fixture
"${SERVER_ROOT}/logs/hs_err_pid1_test-pod-a_20200101-000000-1.log" file
+add_crash_fixture "${SERVER_ROOT}/logs/heapdump_test-pod-a_20200101-000000-2"
dangling-link
+LAUNCHER_DUMP_DIRS+=("${SERVER_ROOT}/logs/heapdump_test-pod-a_20200101-000000-3"
Review Comment:
⚠️ Blocking: no. Summary: The test cleanup can remove an empty directory
that this invocation did not create. These fixed `LAUNCHER_DUMP_DIRS` entries
are passed to the EXIT cleanup unconditionally; if
`heapdump_test-pod-a_20200101-000000-3` remains after an interrupted run, the
retry treats it as occupied and still deletes it. Please track only paths
created by this test before registering them for removal. Evidence: this list
at lines 775-776 and cleanup at lines 242-245; `add_crash_fixture` guards
ownership but is not used for these entries.
--
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]
