Repository navigation
feat(docker): map server.urls_to_pd and server.deploy_in_k8s from env - #3352
Conversation
The Server entrypoint had no mapping for the two keys that decide which address a Server registers in PD, so the Helm chart rewrote rest-server.properties in a wrapper before running the entrypoint. HG_SERVER_URLS_TO_PD now sets server.urls_to_pd, with whitespace removed because the Server splits the list on bare commas. HG_SERVER_DEPLOY_IN_K8S sets server.deploy_in_k8s and accepts the same boolean spellings as HG_SERVER_INIT_STORE_ENABLED; it is checked before either conf file is written, so 0 or 1 stop the container instead of reaching the Server. Unset variables leave the conf files alone. The chart now reserves HG_SERVER_DEPLOY_IN_K8S, since an extraEnv value would otherwise override the flag its wrapper forces on. Rendering conf into a writable copy, the second half of the issue, is left for later: it depends on the start scripts taking a conf directory and on enable-auth.sh not keying off conf-bak/. Part of apache#3286
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Score 8/10. The two mappings are correct: the accepted server.deploy_in_k8s spellings match what TypedOption.parse accepts through commons-configuration 1.x PropertyConverter (no 0/1), the validation runs before any conf write, removing whitespace from server.urls_to_pd fixes the split(",") in GraphManager, and with the Helm wrapper the entrypoint writes the same bracketed URL the wrapper already wrote. One Minor point: in the invalid-flag test, the hugegraph.properties check cannot fail. Evidence: read the full diff at 36f2964 and the head docker-entrypoint.sh, server-deployment.yaml wrapper, HugeConfig, TypedOption, ServerOptions and GraphManager; the exact-head docker / docker-build job ran docker-entrypoint-test.sh and logged its PASS line, and helm / lint-and-render passed; ran the entrypoint suite on Linux with a mutant that validates after the backend and pd.peers writes, and it still passes (details inline).
By the time the invalid HG_SERVER_DEPLOY_IN_K8S loop runs, the graph conf already holds backend=hstore, so passing HG_SERVER_BACKEND=hstore there would rewrite the same bytes and the cmp could never fail. Pass rocksdb instead: a mapping that ran before the flag check now changes hugegraph.properties and fails the test.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Score 9/10. At 7a207d4 the only change since the earlier review at 36f2964 is the invalid-flag test now using HG_SERVER_BACKEND=rocksdb, so its hugegraph.properties cmp can fail, which closes the one Minor point raised there; the two mappings are correct and I found no new defect. Evidence: read the full diff against merge-base 4b77b7b (5 files) with the head docker-entrypoint.sh (the new check runs after the token-secret lookup, which only reads, and before the first set_prop), the Helm wrapper in server-deployment.yaml (it brackets an IPv6 POD_IP in the exported HG_SERVER_URLS_TO_PD before exec, so the entrypoint rewrites the same value), GraphManager (registers server.urls_to_pd only when server.deploy_in_k8s is true, under usePD), TypedOption (commons-configuration 1.x PropertyConverter, so 0 and 1 are not booleans) and HugeConfig (configuration2 with no list splitting, so a comma list reaches GraphManager's split(",") intact). Locally, with GNU sed and the head entrypoint cut before wait-storage: a wrapper-written IPv6 entry is left byte-identical, a spaced list with Yes is written as http://a:8080,http://b:8080 and yes, and HG_SERVER_DEPLOY_IN_K8S=1 exits with both conf files unchanged. CI run 38694462837 at this head passes: the docker-build log shows the three invalid-flag errors and the entrypoint suite PASS line, and helm lint-and-render passes; the other jobs are skipped by path filters. Nothing was run in a real Pod or Compose cluster.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: One input-validation defect was confirmed in the new deployment flag normalization. Evidence: t rue is stripped to true before the accepted-value check, so startup continues.
…urls-to-pd-env # Conflicts: # helm/hugegraph/templates/_helpers.tpl
The flag was normalized with tr -d '[:space:]', which removed every whitespace character, so a malformed value such as "t rue" became "true" and passed the allowlist. Trim only leading and trailing whitespace before lowercasing, and cover "t rue" and "tr ue" in the invalid-flag test.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Score 9/10. At f38914d both mappings are correct and the earlier Important point is fixed: HG_SERVER_DEPLOY_IN_K8S is now trimmed only at the edges, so t rue is rejected, and the check still runs before the first set_prop, so an invalid value leaves both conf files untouched. I found no new defect. Evidence: read the full diff against merge-base a0aae8e (5 files), the head docker-entrypoint.sh, the Server wrapper in server-deployment.yaml (it forces server.deploy_in_k8s=true, and the IPv6-bracketed HG_SERVER_URLS_TO_PD stays exported for the entrypoint), GraphManager (splits server.urls_to_pd on bare commas), and HugeConfig.prefixSchemeIfMissing (prefixes only the start of the value, which matches the README's one-prefix-per-URL note). Ran the trim and case logic locally in bash: ' \tTRUE\r\n' becomes true, while t rue and an embedded newline are rejected. CI run 38781299211 at this head passes: the docker-build log shows the five invalid-flag errors and the entrypoint suite PASS line, and helm unittest reports 306 passed. Nothing was run in a real Pod or Compose cluster.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The Docker mappings and Helm handoff are consistent with the Server configuration paths. Score: 8.8/10. Evidence: exact-head diff and current-head Docker and Helm checks passed; boolean parsing and URL registration paths were statically verified.
Purpose of the PR
The Server entrypoint maps a set of
HG_SERVER_*variables onto the conf files, but nothing mapped the two keys that decide which address a Server registers in PD:server.urls_to_pdandserver.deploy_in_k8s.GraphManagerregisters theurls_to_pdlist only whendeploy_in_k8sis true, andrestserver.urlotherwise. Without a mapping, the Helm chart rewritesrest-server.propertiesin a shell wrapper before it runs the entrypoint, and the TODO indocker-entrypoint.shasked for these mappings.Main Changes
HG_SERVER_URLS_TO_PDsetsserver.urls_to_pd. Whitespace is stripped first: the Server splits the list on bare commas, andhttp://a:8080, http://b:8080was written ashttp://a:8080,\ http://b:8080, which registered\ http://b:8080in PD without any error.HG_SERVER_DEPLOY_IN_K8Ssetsserver.deploy_in_k8s. It accepts the same case-insensitive spellings asHG_SERVER_INIT_STORE_ENABLED(the setHugeConfigaccepts, which excludes0and1). The check runs before the mapping block, so an invalid value stops the container with both conf files untouched.HG_SERVER_DEPLOY_IN_K8Samong the reservedserver.extraEnvnames. Its wrapper forces the flag on, and with this image anextraEnvvalue offalsewould turn it back off; an invalid one would crash-loop the Pod. The wrapper itself stays: it also holds the password checks,usePD, the custom-portrestserver.urland IPv6 bracketing, and it can only shrink once the chart's minimum image has these mappings. I reworded the TODO to say that.hugegraph-server/hugegraph-dist/docker/README.mddocuments both variables.Left for later: rendering the conf into a writable copy (the second half of #3286). It needs the start scripts to take a conf directory (#3253), and it needs
enable-auth.shto decide from the files rather than fromconf-bak/. Otherwise a freshly rendered conf would silently start without auth after the first boot.Overlap with #3322: it adds lines to the same mapping block and to
_helpers.tpl's reserved list.git merge-treemerges the entrypoint and its tests cleanly; the reserved-list line would need a one-line resolution.Verifying these changes
docker-entrypoint-test.shcovers both variables. Unset variables write noserver.*key. A list with surrounding and inner spaces is written without them, and a rerun with new values replaces each key once instead of appending. For1,0andenabledthe entrypoint exits non-zero and both conf files compare equal to their earlier copies.test-docker-entrypoint-process.sh, run inpython:3.12-slim-bookwormwithprocpsandcurl: passes.helm unittest helm/hugegraph(helm-unittest 3.21.0-1.0.3): 295 tests pass, including a new case that rejectsHG_SERVER_DEPLOY_IN_K8Sinserver.extraEnv.Does this PR potentially affect the following parts?
HG_SERVER_DEPLOY_IN_K8Sinserver.extraEnvDocumentation 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:
hugegraph-server/hugegraph-dist/docker/README.mdcontent/{en,cn}/docs/guides/hugegraph-docker-cluster.md,content/{en,cn}/docs/config/config-guide.mdConceptual overview
Concept illustration: map Docker environment variables to PD URL-list and K8s registration settings, with whitespace cleanup and boolean validation; Helm retains its wrapper.