imbajin commented on code in PR #3258:
URL: https://github.com/apache/hugegraph/pull/3258#discussion_r4224179614
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-java-security-properties.sh:
##########
@@ -559,6 +599,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')
Review Comment:
⚠️ If the distribution path passed to this test goes through a symlink,
`SERVER_ROOT` keeps the logical path (`cd ... && pwd` at line 22), while
`hugegraph-server.sh` resolves its own path with `cd -P` before setting `LOGS`.
The captured JVM options then contain the physical logs path but this pattern
expects the symlink path, so valid startup assertions fail. Please canonicalize
`SERVER_ROOT` with `pwd -P` when constructing this pattern. Evidence: test
lines 22/643 and the launcher’s `abs_path` at lines 18-29.
##########
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. Launcher errors that stop the Server before Java starts,
such as an unsupported JDK or too little free memory, and fatal bootstrap
errors go to stderr as well as `hugegraph-server.log`, so 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:
🧹 This claim does not hold for every HStore startup path.
`docker-entrypoint.sh` invokes `init-store.sh` before `start-hugegraph.sh`, and
`init-store.sh` calls `configure_riscv64_libatomic` without the `report_error`
callback; on a fresh image, that missing-library failure reaches stderr before
the Server launcher creates `logs/`, not `hugegraph-server.log`. Please limit
the file-log guarantee to errors reported by `hugegraph-server.sh`, or route
this earlier check through the same logger. Evidence: exact-head
`docker-entrypoint.sh` lines 222-255 and `init-store.sh` line 37.
##########
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. Launcher errors that stop the Server before Java starts,
such as an unsupported JDK or too little free memory, and fatal bootstrap
errors go to stderr as well as `hugegraph-server.log`, so 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.
+
+`/hugegraph-server/logs` holds `hugegraph-server.log`, JVM crash logs
(`hs_err_pid<pid>_<host>_<launch time>.log`) and out-of-memory heap dumps
(`heapdump_<host>_<launch time>/java_pid<pid>.hprof`), where `<launch time>` is
the container's local time as `YYYYMMDD-HHMMSS`. Each start gets its own
`heapdump_*` directory, so the Server and the JVMs it starts, such as computer
jobs, write separate dumps. The host name (on Kubernetes, the pod name, or
`spec.hostname` when that is set) and a `-1`, `-2`, ... counter keep names from
clashing; the counter also separates pods that share a host name. If the
directory cannot be created, for example on a full volume, the Server still
starts and dumps go to `logs/` itself as `java_pid<pid>.hprof`. That is best
effort: a full volume may have no room for the dump either, and a restarted
container that reuses a PID cannot write over an earlier dump with the same
name.
+
+Both images declare `VOLUME /hugegraph-server`, so `docker restart` and a
Compose recreate keep the files in that anonymous volume. `docker compose down`
leaves the volume behind but the next `up` does not attach it, so the files
become hard to reach and keep using disk until `docker compose down -v` deletes
them; mount a named volume or a bind mount at `/hugegraph-server/logs` to keep
them reachable. Kubernetes starts a new container on every restart and does not
turn the image's `VOLUME` into a pod volume, so mount an `emptyDir` or a
PersistentVolumeClaim at `/hugegraph-server/logs`. Give each pod its own
volume, or a per-pod directory on a shared PVC: crash files get distinct names,
but `hugegraph-server.log`, the audit log and the slow-query log have fixed
names, and pods sharing them would roll them over for each other. For a per-pod
directory, pass the pod name in through the Downward API and use it in
`subPathExpr` (the Helm chart sets neither):
+
+```yaml
+env:
+ - name: POD_NAME
+ valueFrom:
+ fieldRef:
+ fieldPath: metadata.name
+volumeMounts:
+ - name: server-logs
+ mountPath: /hugegraph-server/logs
+ subPathExpr: $(POD_NAME)
+```
+
+The Helm chart in [`helm/`](../helm/hugegraph) does not mount a logs volume
yet, so with it these files are lost when a container restarts.
+
+A heap dump can be as large as the JVM heap. Every start uses a new directory
and the JVMs a Server starts can dump too, so a Server that keeps running out
of memory fills the volume with heap-sized files. Size the volume for the dumps
you want to keep plus the logs, and keep it within any `ephemeral-storage`
limit or `emptyDir` `sizeLimit`: eviction deletes an `emptyDir` together with
the dump in it. Do not use an `emptyDir` with `medium: Memory`, which counts
against the memory limit. A liveness probe that restarts the pod while a large
dump is being written leaves a truncated file.
+
+The launcher never deletes dumps or `heapdump_*` directories. Each start
leaves a directory behind, empty unless something ran out of memory; the
launcher keeps it because a computer-job JVM can go on using it after its
Server exits. Move or delete old directories once no HugeGraph JVM from that
launch is running, but never the newest one of a running Server: without its
directory, the next dump is written as a file with that name and later dumps
from that launch fail.
+
+To turn heap dumps off for every JVM, set
`JAVA_TOOL_OPTIONS=-XX:-HeapDumpOnOutOfMemoryError`, which also keeps the
image's default `JAVA_OPTS`. Set it in the container's environment, not in the
host shell: `docker run -e JAVA_TOOL_OPTIONS=...`, an `environment:` entry on
the Server service in Compose (the shipped Compose files do not pass it
through), or `env` on the container in Kubernetes. Putting the flag in
`JAVA_OPTS` turns dumps off for the Server JVM only, because the JVMs it starts
do not see its command line, and setting `JAVA_OPTS` replaces the image default
(`-XX:+UseContainerSupport -XX:MaxRAMPercentage=50 ...`), so repeat those
flags. Either way, each start still creates its empty `heapdump_*` directory.
Review Comment:
🧹 The fallback above sets `HeapDumpPath` to `${LOGS}` when `mkdir
heapdump_*` fails, without creating a `heapdump_*` directory. The claim that
each start still creates its empty directory is therefore false on that
fallback path. Please qualify this as applying only when directory creation
succeeds; the paired EN/CN quickstart in `apache/hugegraph-doc#509` makes the
same unconditional claim. Evidence: `hugegraph-server.sh` lines 315-326.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -248,13 +257,84 @@ if [ "${OPEN_TELEMETRY}" == "true" ]; then
export OTEL_RESOURCE_ATTRIBUTES=service.name=server
fi
-if [[ "${STDOUT_MODE:-false}" != "true" ]]; then
- # Daemon stderr only reaches hugegraph-server-stdout.log, so a bootstrap
- # rejection of the effective DNS policy (a broken operator override
- # included) would be invisible in the log start-hugegraph.sh points
- # operators at. Let the bootstrap mirror its fatal errors there.
- JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
-fi
+# Let the bootstrap mirror its fatal errors (a rejected DNS policy, a broken
+# operator override included) into hugegraph-server.log. In daemon mode its
+# stderr only reaches hugegraph-server-stdout.log; in STDOUT_MODE it reaches
the
+# container log, which keeps a single earlier generation, so the file copy is
+# what survives a crash loop on a mounted logs volume.
+JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
+
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds. The
+# defaults come first in JAVA_TOOL_OPTIONS, ahead of the operator's own value.
The
+# JVM applies JAVA_TOOL_OPTIONS before JDK_JAVA_OPTIONS, the command line
+# (JAVA_OPTIONS and -j) and _JAVA_OPTIONS, and the last occurrence of a flag
wins,
+# so any of those overrides a default. The JVM does the parsing itself,
including
+# quoted options, and @argfiles in JDK_JAVA_OPTIONS or on the command line.
+# Child JVMs the Server starts (computer jobs, for example) inherit the
+# environment but not the command line, so they get these defaults too, and
every
+# path must stay unique per JVM. ErrorFile expands %p to each JVM's PID.
+# HeapDumpPath expands %p only from JDK 25, but on every version, when it
names an
+# existing directory, each JVM writes java_pid<its pid>.hprof inside it, so it
+# points at one directory per launch.
+# A restarted container often reuses the PID; HotSpot truncates an existing
crash
+# log (JDK 17+) and will not write a heap dump over an existing file. So the
names
+# carry the host name (the pod name on Kubernetes, so pods sharing one log
volume
+# do not collide), the launch time, and a counter. A plain mkdir claims each
name
+# atomically, so concurrent launches never share one. This runs after every
+# preflight check, so a launch that fails before Java starts leaves nothing.
+# Dump directories are never removed automatically: one stays empty until a JVM
+# using it runs out of memory, and that JVM may be a computer job that outlives
+# the Server, so the launcher cannot tell when a directory is safe to delete.
+crash_name_taken() {
+ local name="$1" file restore_failglob taken=1
+ [[ -e "${LOGS}/heapdump_${name}" || -L "${LOGS}/heapdump_${name}" ]] &&
return 0
+ restore_failglob=$(shopt -p failglob)
+ shopt -u failglob
+ for file in "${LOGS}"/hs_err_pid*_"${name}".log; do
+ if [[ -e ${file} || -L ${file} ]]; then
+ taken=0
+ break
+ fi
+ done
+ eval "${restore_failglob}"
+ return ${taken}
+}
+case "${LOGS}" in
+ *\"*|*%*)
+ # JAVA_TOOL_OPTIONS has no escape for a double quote, and ErrorFile
expands %.
+ report_error "WARN: ${LOGS} contains a double quote or %, so the heap
dump and crash log\
+ defaults are not set; set -XX:HeapDumpPath and -XX:ErrorFile yourself"
+ ;;
+ *)
+ LAUNCH_HOST=$(printf '%s' "${HOSTNAME:-localhost}" | LC_ALL=C tr -c
'A-Za-z0-9._-' '_')
+ LAUNCH_HOST="${LAUNCH_HOST:0:64}"
+ LAUNCH_STAMP="${LAUNCH_HOST}_$(date +%Y%m%d-%H%M%S)"
+ LAUNCH_ID="${LAUNCH_STAMP}"
+ LAUNCH_SUFFIX=0
+ HEAP_DUMP_PATH=""
+ while [[ -z ${HEAP_DUMP_PATH} ]]; do
+ if ! crash_name_taken "${LAUNCH_ID}" &&
+ mkdir "${LOGS}/heapdump_${LAUNCH_ID}" 2>/dev/null; then
+ HEAP_DUMP_PATH="${LOGS}/heapdump_${LAUNCH_ID}"
+ elif [[ ${LAUNCH_SUFFIX} -ge 1000 ]]; then
+ report_error "WARN: heap dump names for ${LAUNCH_STAMP} are
all taken; heap dumps go to ${LOGS}"
+ HEAP_DUMP_PATH="${LOGS}"
+ elif ! crash_name_taken "${LAUNCH_ID}"; then
+ # Not a name clash (a full disk, for example): keep starting,
and dump
+ # into $LOGS itself rather than make diagnostics a startup
requirement.
+ report_error "WARN: cannot create
${LOGS}/heapdump_${LAUNCH_ID}; heap dumps go to ${LOGS}"
+ HEAP_DUMP_PATH="${LOGS}"
+ else
+ LAUNCH_SUFFIX=$((LAUNCH_SUFFIX + 1))
+ LAUNCH_ID="${LAUNCH_STAMP}-${LAUNCH_SUFFIX}"
+ fi
+ done
+ CRASH_OPTIONS="-XX:+HeapDumpOnOutOfMemoryError"
+ CRASH_OPTIONS="${CRASH_OPTIONS} \"-XX:HeapDumpPath=${HEAP_DUMP_PATH}\""
Review Comment:
⚠️ The per-launch directory still names child dumps only by OS PID
(`java_pid<pid>.hprof`). If a computer-job JVM exits and its PID is reused
before this Server launch ends, the next child targets the existing dump path;
the launcher notes HotSpot will not overwrite an existing dump, so that OOM
loses its dump. Please handle this reuse case or qualify the per-JVM guarantee
in the docs. Evidence: inherited `JAVA_TOOL_OPTIONS` points child JVMs at this
same launch directory, and the comment above describes the
`java_pid<pid>.hprof` naming.
--
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]
