fix(docker): make the Server startup timeout configurable - #3187
Conversation
The entrypoint ran start-hugegraph.sh with a literal -t 120, so a Server that needed longer than 120 seconds to answer on its REST port was terminated by its own container, no matter how much startup budget the orchestrator's probe allowed. Read the timeout from HG_SERVER_STARTUP_TIMEOUT_S instead, keep 120 as the default, and reject values that are not positive whole numbers before init-store runs. Covered by docker-entrypoint-test.sh: default passthrough, an explicit override, and rejection of an invalid value. Documented in the Server docker README. Closes apache#3186
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3187 +/- ##
============================================
+ Coverage 37.77% 37.79% +0.01%
- Complexity 6560 6566 +6
============================================
Files 800 800
Lines 68960 68960
Branches 9166 9166
============================================
+ Hits 26052 26064 +12
+ Misses 39841 39829 -12
Partials 3067 3067 ☔ 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: The configurable timeout is wired correctly for normal values, but its accepted input range can overflow the startup deadline calculation. Evidence: Bash arithmetic reproduces a negative deadline for 9223372036854775807; all reported GitHub checks are successful.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The 120s default is preserved when the variable is unset and the guard runs ahead of every config write and init-store call, but :- also lets an explicitly empty value fall back to 120 silently, which the README section added here states does not happen. The new test asserts that a bad value does not start the server without pinning the ordering guarantee the code comment claims. Evidence: ran bash hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh at 5cad9dc under bash 5.3 (exit 0); drove the entrypoint with a stubbed bin/ for empty, whitespace, 0, +5 and 450, where empty alone exits 0 with -t 120; reran the suite against an entrypoint with the validation block relocated below init-store.sh, which also exits 0; healthcheck values read from hugegraph-server/Dockerfile:74, hugegraph-server/Dockerfile-hstore:76 and docker/docker-compose.yml:41-46.
Review follow-up on the configurable Server startup timeout. The guard accepted any positive integer, but wait_for_startup computes its deadline as $((now_s + timeout_s)). Near the 64-bit ceiling that sum wraps negative, the wait loop exits before its first probe, and the container reports a startup timeout immediately: the failure the variable exists to prevent. The accepted range is now 1 to 86400 seconds, bounded first by a five-digit pattern so the comparison itself cannot overflow. The default also used ':-', which treats an explicitly empty value as unset and silently restores 120. Compose writes exactly that whenever an interpolated host variable is missing, so a deployment that believed it had set 450 still died at 120, and the README said such a value would stop the container. Plain '-' keeps the default for an unset variable and rejects an empty one. Tests now cover the accepted upper bound, an unset variable, and seven rejected values including empty, whitespace, 86401 and INT64_MAX, and assert that a rejected value runs neither start-hugegraph nor init-store, which pins the ordering the guard's comment claims. The new assertions exit explicitly rather than relying on set -e with [[ ]], which bash 3.2 ignores. The README documents the range and that the container health check keeps its own budget, which does not move with this variable.
Brings the branch onto 3681148 so the coverage report compares against the current base rather than one commit behind it. No conflicts: master touched only Java sources, this branch only the Docker entrypoint, its test and the docker README.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The configurable timeout is not propagated through the provided Compose topologies, and the new shell tests can report success for incorrect values on Bash 3.2. Evidence: reproduced with Docker Compose config rendering and /bin/bash 3.2.57; all findings are in the changed Docker documentation/test paths.
…n its tests
Second review follow-up.
No Compose topology declared HG_SERVER_STARTUP_TIMEOUT_S, so a rendered
Server still took the entrypoint default and the container budget could not
be aligned with the healthcheck start period. All three topologies now pass
it through as ${HG_SERVER_STARTUP_TIMEOUT_S-120}: unset-only, so an absent
host value renders 120 while an explicitly empty one reaches the entrypoint
and is rejected there rather than silently becoming the default. The render
test asserts both the default and a host override on every Server service,
and renders with the variable stripped so the baseline cannot be coloured by
the developer's environment.
The entrypoint tests had three ways to pass while broken. They matched -t
with a substring against a flattened "$*", where -t 1200 satisfies an
assertion for 120; the stub now records one argument per line and the
assertion reads the exact value after -t. The positive cases were bare
[[ ]], which bash 3.2, still the /bin/bash of macOS, does not fail under
set -e; they now exit explicitly and report the value they saw. The unset
case only started a child shell, which inherits an exported value, so it now
runs under env -u.
Verified on bash 3.2.57 and 5.3.15: the suite passes on both, and on 3.2
each of the three regressions above is now reported instead of passing.
|
The One failure out of 814 tests: Attribution, for what it is worth: this branch changes three Compose files, Attempt 2 is running now. Happy to open a separate issue for the TTL timing if it is worth tracking. |
The Server image entrypoint reads HG_SERVER_STARTUP_TIMEOUT_S instead of the fixed 120 s wait on start-hugegraph.sh, bounded and rejecting an empty value, and the Compose files pass it through. Carried here so the helm-dev Server image can be tested with the timeout set from the chart's probe budget (apache#3186).
PD REST now checks the password against auth.secret-key and answers 401 on refusal (apache#3188). The PD image requires HG_PD_AUTH_SECRET_KEY, wait-storage.sh sends PD_AUTH_PASSWORD, and Hubble reads operations.pd.password. Two conflicts with the pull requests merged before it, both resolved as the union: the interceptor exclusion list keeps /v1/ready from apache#3185 alongside the /actuator/** widening from apache#3189, and test-compose.sh runs the startup timeout asserts from apache#3187 followed by the Hubble helper check from apache#3189. render_with_timeout from apache#3187 additionally passes HG_PD_AUTH_SECRET_KEY, which the Compose files require since apache#3189; without it the render step would fail on the two HStore topologies. The chart does not yet supply the PD secret, so PD Pods from an image built at this revision will not start under the chart until that wiring lands.
PD images from 1.8.0 (apache#3189) check the Basic-auth password of every management call against auth.secret-key and refuse to start without one. The chart now keeps that value in a kept release-pd-auth Secret, or in pd.auth.existingSecret, and hands it to the three readers: PD as HG_PD_AUTH_SECRET_KEY, the Server storage wait as PD_AUTH_PASSWORD, and Hubble as operations.pd.password written into its properties file by the existing wrapper. A checksum/pd-auth annotation on the three Pod templates rolls them when the Secret changes; the Server annotations block is now rendered unconditionally for it. Priority and lookup semantics mirror server.auth.token. Older images ignore the password, so the wiring is harmless on the images the draft currently tracks. The values schema requires one of existingSecret, value or autoGenerate and refuses newlines, carriage returns and backslashes in an inline value, since it lands in a Java properties file; the template guard repeats the first rule for values that bypass the schema. The three chart-managed variables join the reserved extraEnv lists. README: Chart Details bullet, four parameter rows, Disaster Recovery calls carry the secret, and the Limitations bullet separates the 1.7.0 behaviour from 1.8.0. NOTES prints how to read the secret. New suite pd_auth_secret_test.yaml, 9 tests; 58 in total. Lint on three presets; renders 16 objects by default and 19 with Hubble. Measured on a kind cluster with images built from master plus apache#3185, apache#3187 and apache#3189: the Secret is created, PD starts with the variable, the Server storage wait passes with the credential, and Hubble lists all nine nodes.
|
Tested on Kubernetes on 2026-09-05, with this branch merged into the hugegraph/hugegraph testing tree. Build under test. Tag
One note for whoever merges second: #3189 makes the Compose files require |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: nothing here should hold up the merge; the overflow, empty-value, argv-substring and Bash 3.2 threads are all genuinely closed at this head. One test gap worth fixing: the new Compose render assertions pass just as happily under :- as under -, so they cannot protect the empty-value semantics the entrypoint and README depend on. Four smaller notes follow. Evidence: at 0ba30a1, docker-entrypoint-test.sh and docker/test-compose.sh render both exit 0 locally (Docker Compose v5.1.2, GNU sed on PATH), and all 24 reported checks on this head are successful.
…ocument it Third review follow-up. The render assertions could not see the "-" against ":-" choice the rest of the change rests on: unset renders 120 and 450 renders 450 under both spellings, and the whole suite passed against a ":-" rewrite. Only an explicitly empty host value separates them, and that value has to reach the container for the entrypoint to reject it. Every topology is now rendered with an empty value and asserted to carry one. render_with_timeout no longer repeats compose_auth's environment. The two differ by one variable, so that is now an array the override rebinds in a subshell, and a variable added to compose_auth reaches both renders instead of only the baseline. The comment above the assertions claimed alignment with the healthcheck budget in both directions. The healthcheck values are literals giving every Server about 360 seconds, and the documented "up -d --wait" gives up there, so the knob aligns downward only and the comment now says so. The two Compose files that carried a bare unset-only default without the reason now carry the same note as the standalone file, and docker/README.md, the file Compose users read, documents the variable beside the other host knobs. Verified: the render suite fails a ":-" rewrite with the expected empty-value message, a variable added to compose_auth alone now reaches all three renders, and both suites pass on bash 3.2.57 and 5.3.15.
The example raised HG_SERVER_STARTUP_TIMEOUT_S to 450 while the Server health check in every Compose topology allows roughly 360 seconds, so on the slow host the example is written for the pasted command fails: Hubble gates on depends_on: condition: service_healthy, which a plain "up -d" honours, and Compose reports the Server dependency unhealthy at 360. Lower the example to 300 and say that the plain "up -d" path hits the same wall as "up -d --wait". Also strip HG_SERVER_STARTUP_TIMEOUT_S in compose_active, matching the render path. Without it a developer who exports the variable empty gets Server containers stopped by the new guard during "test-compose.sh smoke", with no sign that their shell caused it.
The Server docker README still showed 450 in its Compose example, while docker/README.md now shows 300. The Compose files give the Server health check roughly 360 seconds, and Hubble gates on it with depends_on: condition: service_healthy, so a 450 second startup budget makes docker compose up -d exit non-zero. Match the two READMEs at 300. The docker run example keeps 450. That path does not block on health, and the section already explains the 135 second image health check and how to raise it with --health-start-period.
|
@imbajin Ready for a review pass when you have time. Pushed Why those two commits. The
What I checked on the branch. The validation matrix against the real entrypoint: unset, empty,
Ordering holds: the guard is at One nit I left alone. The regex admits up to 99999 and a second check refuses anything over 86400, so the bounds live in two places. The error message is correct either way and collapsing them would churn the tests, so it did not seem worth it. Happy to do it if you would rather have one check. |
A whole top-level section for one environment variable was more room than the knob deserves, and it wrapped at about 70 characters where the rest of the file runs long. Section 7 is gone: the variable is now two sentences at the end of section 6, next to the health check it interacts with, and the budget arithmetic and the docker run example sit in a collapsed block for readers who need them. docker/README.md gets the same treatment. Net 26 lines lighter. The dead #7-server-startup-timeout anchor and the stale "section 7" comment in test-compose.sh are updated with it.
|
Fixed in Section 7 is gone. One environment variable did not need a top-level section, so it is now two sentences at the end of section 6, beside the health check it interacts with. The budget arithmetic and the Two things that would have broken with the section: |
apache#3187 landed on master and both branches append to the Compose render contract in docker/test-compose.sh. Kept both sides: the startup timeout render assertions from master, then hubble_password_helper_check.
Brings in the master merge already made on the hugegraph/hugegraph mirror, so the fork and the mirror share one history again. Master now carries apache#3187; it does not touch anything this branch changes.
2 participants
Purpose of the PR
docker-entrypoint.shrunsstart-hugegraph.shwith a literal-t 120, so a Server that needs longer than 120 seconds to answer on its REST port is ended by its own container. Under an orchestrator the startup budget belongs to the startup probe, and a probe cannot extend a process that terminates itself first: with the 450-second probe budget the Helm chart in #3132 configures, the process is gone at 120 seconds. Measured on 2026-09-01, 2 of 6 Server starts across two independent installs died this way and recovered only on container restart (log excerpts and the measurement setup are in the issue).Main Changes
docker-entrypoint.shreads the timeout fromHG_SERVER_STARTUP_TIMEOUT_S, defaulting to the current 120, so nothing changes for deployments that do not set it. Both Server images share this entrypoint, so one change covers the RocksDB and HStore images.wait_for_startup.docker-entrypoint-test.shnow records the arguments thestart-hugegraph.shstub receives and asserts the default-t 120, an explicit-t 450override, and rejection of2mbefore the server would have been started.hugegraph-server/hugegraph-dist/docker/README.md. The issue proposed the environment table indocker/README.md, but chore(docker): refactor docker-compose topologies with Hubble #3149 removed that table, so the Server docker README is the current home for entrypoint variables.Verifying these changes
bash hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh(already wired intoserver-ci.yml) covers the default, an override, and an invalid value.Does this PR potentially affect the following parts?
Documentation Status
Doc - Done