Conversation
When a hugegraph/server container exited during startup, kubectl logs showed only "Starting HugeGraphServer failed". The cause stayed in logs/ inside the container, and Kubernetes dropped it on restart. - Set STDOUT_MODE=true in Dockerfile-hstore. apache#2980 did this for the standalone Dockerfile and missed this one. - In conf/log4j2.xml, also send WARN and above from the hadoop, zookeeper, sofa, netty and commons loggers to console. They were file-only. - In hugegraph-server.sh, always set -XX:+HeapDumpOnOutOfMemoryError, -XX:HeapDumpPath and -XX:ErrorFile under $LOGS. The heap-dump flags used to be dropped whenever JAVA_OPTIONS was set, and hs_err files went to the install directory. The file names carry the launch time because HotSpot will not overwrite an existing crash log or heap dump, and a restarted container often reuses the PID. The flags go before JAVA_OPTIONS and -j, so values given there still win; a flag already set in JAVA_TOOL_OPTIONS or JDK_JAVA_OPTIONS, which the JVM reads before the command line, is left out. - Say "container logs" rather than "docker logs" in the start failure message, since the same image runs on Kubernetes. - Cover the new flags and their overrides in test-java-security-properties.sh, and document the log directory and the Kubernetes mount in docker/README.md. This applies to every Server install, bare metal included: an OOM now writes a heap dump to logs/ even when JAVA_OPTIONS is set. Fixes apache#3256
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3258 +/- ##
============================================
+ Coverage 41.40% 41.57% +0.16%
+ Complexity 7337 7311 -26
============================================
Files 802 794 -8
Lines 69792 69127 -665
Branches 9312 9258 -54
============================================
- Hits 28897 28739 -158
+ Misses 37604 37110 -494
+ Partials 3291 3278 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: When telemetry is enabled, crash options supplied through JAVA_TOOL_OPTIONS can be discarded together with the launcher defaults. Evidence: static trace of the crash-option scan and the telemetry environment assignment.
With telemetry on, the launcher replaced JAVA_TOOL_OPTIONS with the OpenTelemetry -javaagent option. The crash-file defaults leave out any flag already set in JAVA_TOOL_OPTIONS, so an operator's -XX:ErrorFile there was dropped twice and HotSpot wrote hs_err to the working directory. Append the agent to the existing value instead, and cover the telemetry path in test-java-security-properties.sh with a stubbed agent checksum.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Holding approval until the two findings below are addressed in the published branch. I prepared fixes and regression checks locally, but cannot push to the PR's fork, so those changes are not part of this review's commit.
The local fix lets the JVM handle diagnostic-option precedence by prepending defaults to JAVA_TOOL_OPTIONS, and removes the temporary telemetry JAR through the test's EXIT cleanup. Focused checks reproduced the quoted heap-dump opt-out failure on this PR head and passed with the local fixes, covering quoted environment options, JDK argument files, telemetry preservation, and cleanup on failure. Shell syntax, whitespace, and editorconfig checks passed. A full Maven compile was attempted but failed in hugegraph-common under Maven's locally selected JDK 26; full-build validation remains outstanding.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Heap-dump filenames can collide when a container restarts within one second and reuses its PID, so consecutive failures cannot retain distinct dumps. Evidence: hugegraph-server.sh:138,154; OpenJDK 11 defaults the heap dump writer to overwrite=false.
Review follow-ups on the crash-file defaults: - Put the defaults at the front of JAVA_TOOL_OPTIONS instead of on the command line, and drop the shell scan of JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS. The JVM reads the operator's JAVA_TOOL_OPTIONS, JDK_JAVA_OPTIONS, the command line and _JAVA_OPTIONS after the defaults, so any of them overrides a default. Word splitting in the shell missed quoted options and @argfiles, and could treat flag-like text inside a property value as a flag. - Add a counter to the launch time in the file names when a heap dump or crash log with that name already exists in $LOGS. A restart in the same second with the same PID no longer targets an existing file. - Test the values a real JVM resolves, covering a quoted opt-out, an @argfile in JDK_JAVA_OPTIONS, flag text in a property value, operator paths in JAVA_OPTIONS, name collisions and telemetry. The EXIT cleanup now removes the telemetry jar and log fixtures, so a failing step does not leave them in the distribution.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The latest-head checks pass, but one test-fixture cleanup issue remains. Evidence: the new crash-file fixture uses fixed paths under the supplied distribution's logs/ and removes them unconditionally.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 10/10. Summary: At ac9d98b the heap dump and crash log defaults go first in JAVA_TOOL_OPTIONS, so the JVM's own precedence lets JAVA_TOOL_OPTIONS, JDK_JAVA_OPTIONS, JAVA_OPTIONS, -j and _JAVA_OPTIONS override them, file names get a counter when a name is already taken in logs/, and the telemetry agent is appended instead of replacing JAVA_TOOL_OPTIONS. The four earlier inline threads are addressed in this head and I found no new issues. Evidence: static review of the full diff and of hugegraph-server.sh, start-hugegraph.sh, docker-entrypoint.sh, Dockerfile-hstore and log4j2.xml at this head. CI at this head: 22 of 22 checks pass, and the build-server (rocksdb, 11) job ran the 'Run Java security properties tests' step, which starts a real JVM with the launcher's captured options, with conclusion success.
The collision check created two fixed crash-file names under the supplied distribution's logs/ and removed them afterwards, which would delete real files that already had those names. Record and remove only the files this run creates.
…iles Review follow-ups: - Docker cluster guide: no 1.7.0 image sets STDOUT_MODE, the standalone hugegraph/hugegraph:1.7.0 used on the page included. Describe stdout logging as current master behaviour for all images, and widen the version note to every 1.7.0 and older image, linking #2980 and #3258. - Server quickstart: the defaults now go first in JAVA_TOOL_OPTIONS (apache/hugegraph#3258), so list every source that overrides them, _JAVA_OPTIONS included, describe the counter added to a taken file name, and mention the "Picked up JAVA_TOOL_OPTIONS" startup line. - Chinese pages: wrap only at existing spaces, so a soft break no longer renders as a space between two Chinese characters.
|
The two red checks look unrelated to this change: |
3 participants
Purpose of the PR
hugegraph/server) keeps crash diagnostics only inside the container, so a restart on Kubernetes loses them #3256When a
hugegraph/server(HStore) container exits during startup,kubectl logs --previousshows onlyStarting HugeGraphServer failedand a pointer tologs/hugegraph-server.log. That file, anyhs_err_pid*.logand any heap dump live inside the container, and Kubernetes discards them when it restarts the pod. In #3203 the stack trace was only recovered by mounting a volume at/hugegraph-server/logsbefore the first boot.Main Changes
Dockerfile-hstoresetsSTDOUT_MODE=true. fix(docker): enable docker logs for pd/store/server containers #2980 added it to the standalone, PD and Store Dockerfiles and missed this one.conf/log4j2.xml: theorg.apache.hadoop,org.apache.zookeeper,com.alipay.sofa,io.nettyandorg.apache.commonsloggers areadditivity="false"with only thefileappender. Each gets<appender-ref ref="console" level="WARN"/>, so WARN and above reaches stdout and INFO stays in the file. Root andorg.apache.hugegraphalready log to console. Audit and slow-query logs are unchanged.hugegraph-server.sh:-XX:+HeapDumpOnOutOfMemoryErrorand-XX:HeapDumpPathused to sit inside theJAVA_OPTIONSdefault block, so settingJAVA_OPTIONSdropped them. Nothing set-XX:ErrorFile, sohs_errfiles went to the working directory (the install root). Both are now set on every start, pointing at$LOGS.java_pid<pid>_<launch time>.hprofandhs_err_pid%p_<launch time>.log, with a-1,-2, ... counter added when that name already exists in$LOGS. A restarted container often gets the same PID, sometimes within the same second, and HotSpot neither overwrites an existinghs_errfile (it falls back to the working directory) nor writes a heap dump over an existing one. The scriptexecs java, so$$is the JVM's PID.JAVA_TOOL_OPTIONS. The JVM reads the operator's ownJAVA_TOOL_OPTIONS,JDK_JAVA_OPTIONS(including@argfiles), the command line (JAVA_OPTIONS,-j) and_JAVA_OPTIONSafter that, so any of them overrides a default, and the JVM does its own parsing of quoted options. A side effect: the JVM printsPicked up JAVA_TOOL_OPTIONS: ...on stderr at startup.-y true), the OpenTelemetry-javaagentoption is appended toJAVA_TOOL_OPTIONSinstead of replacing it, so the defaults and the operator's flags there are kept.$LOGS, so they followLOGS_OVERRIDEif feat(dist): Make conf/logs/plugins/pid paths overridable and fix log4j2 config resolution #3253 lands. The two PRs touch nearby lines in this script.start-hugegraph.sh: inSTDOUT_MODEthe failure message now points at "container logs ('docker logs' or 'kubectl logs')", since the same image runs on Kubernetes.docker/README.md: new "Server logs and crash files" section covering what reaches stdout, the file names under/hugegraph-server/logs, mounting anemptyDiror PVC there on Kubernetes, heap-dump size, and turning dumps off with-XX:-HeapDumpOnOutOfMemoryErrorinJAVA_OPTS.Outside containers, a bare-metal install that sets
JAVA_OPTIONSnow also writes a heap dump tologs/on OOM. The dump can be as large as the heap.Not in this PR:
immediateFlush="false"on thefileappender (mentioned in the issue). WARN and above now also reach the console, which flushes each line.STDOUT_MODE=truethe launcher omits-Dhugegraph.bootstrap.error.log, so bootstrap fatal errors reach stdout only, not a mountedhugegraph-server.log.-XX:ErrorFile, which is left for a separate PR.Verifying these changes
test-java-security-properties.sh(run by server-ci) now starts a real JVM with the launcher's captured options and checks the heap dump and crash log values it resolves: the defaults, operator paths inJAVA_OPTIONS, a quoted opt-out and flag text inside a property value inJAVA_TOOL_OPTIONS, an@argfileinJDK_JAVA_OPTIONS, name collisions with a fixed clock, and telemetry. It passes with JDK 11 against a dist built from this branch and fails against master'shugegraph-server.sh. I did not build or run the Docker images.Does this PR potentially affect the following parts?
hugegraph-server.sh, and thehugegraph/serverimage now logs to stdoutDocumentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR:
docker/README.md(in this PR)