Skip to content

fix(docker): expose Server discovery identity - #3322

Open
imbajin wants to merge 5 commits into
apache:masterfrom
hugegraph:fix/server-discovery-config
Open

imbajin wants to merge 5 commits into
apache:masterfrom
hugegraph:fix/server-discovery-config

Conversation

@imbajin

@imbajin imbajin commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

Purpose of the PR

Container and Helm deployments could not pass a Server service ID or graph space into rest-server.properties, even when callers supplied the legacy environment variables. Map the supported settings before startup and expose them in the chart. The Helm test also always queried DEFAULT-hugegraph; let operators select an existing graph binding while retaining the default.

Main Changes

  • Accept HG_SERVER_SERVICE_ID / HG_SERVER_GRAPHSPACE and migrate their legacy aliases; an effective nonempty new value takes precedence.
  • Add server.serviceId and server.graphSpace chart values, preserving existing DEFAULT behavior and avoiding duplicate identity variables.
  • Add optional server.testGraphAlias for an existing Gremlin graph binding. Keep the default and saved-values fallback, authentication and complete per-Pod checks; do not infer or create a graph from the assigned space.
  • Reject unsupported identity input before modifying configuration. Deployment service IDs accept printable ASCII, including spaces and backslashes; graph spaces follow the existing Server naming rules.
  • Document graph-space creation, authentication and the deployment input boundary. Core service naming and mounted properties retain their current behavior.

Verifying these changes

Shell and process checks, real HugeConfig readback controls, Helm tests, packaging, lint/render and schema CI checks passed. Formatting, clean compilation and independent final reviews passed. A real standalone Server with StandardAuthenticator and RocksDB confirmed that selecting an existing alias passes while missing aliases fail, with unchanged graph data. This verifies graph selection; stock PD/HStore startup, non-default discovery, authorization, isolation and Kubernetes deployment still require their own acceptance checks.

Documentation Status

Doc - Done: Docker and Helm guides in this PR. Paired website documentation: ASF #517. Coordinate both merges.

Implementation illustration

PR #3322 mechanism

Helm and container identity inputs are validated and mapped into Server properties before startup, with legacy-alias support and nonempty HG_SERVER_* values taking precedence. Helm DEFAULT values preserve the image configuration; non-default graph spaces must already exist. Configuration wiring only; live discovery, authentication and isolation still require topology validation.

- map service identity and graphspace environment inputs
- expose Helm settings with existing default behavior
- validate effective identity inputs before writing config
- document naming constraints and paired website guidance
- check effective graphspace names before writing config
- stabilize name validation with a scoped C locale
- cover new and legacy names plus locale boundaries
@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.74%. Comparing base (87239c5) to head (dd10fda).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3322      +/-   ##
============================================
+ Coverage     52.64%   54.74%   +2.10%     
+ Complexity     2369      369    -2000     
============================================
  Files           762      503     -259     
  Lines         67674    48402   -19272     
  Branches       9189     6963    -2226     
============================================
- Hits          35624    26500    -9124     
+ Misses        28492    19223    -9269     
+ Partials       3558     2679     -879     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The entrypoint mapping, its validation order and the chart wiring hold up, and the default render is unchanged; two small gaps remain in how the chart guards the new service identity values. Evidence: read the full diff at 83bb23d (10 files) with docker-entrypoint.sh, GraphManager (NAME_REGEX, loadServices, the graph-space load filter), HugeConfig, K8sDriver and HugeGremlinServer at that head; rendered the chart at that head with helm template (helm v4.2.4) for both comments; ran the service ID encoder from the entrypoint over values with spaces, backslashes, commas and ${...} and read them back through commons-configuration2 2.8.0 PropertiesConfiguration (the repo pins 2.10.1), all unchanged; docker / deployment-contracts and build-server / server_memory (Java 17), which run docker-entrypoint-test.sh, and helm / lint-and-render pass on this head; Analyze (java) and the three pd_store jobs were still pending. The entrypoint test was not rerun locally (it needs GNU sed), and nothing was run against a PD cluster.

Comment thread helm/hugegraph/templates/_helpers.tpl Outdated
Comment thread helm/hugegraph/templates/server-deployment.yaml Outdated

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The deployment change has a test coverage gap around the service ID value loaded by the Server. Evidence: The new entrypoint checks inspect the serialized properties file while the test stubs the Server startup path.

Comment thread hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh
- reserve legacy identity environment variables
- escape dollars before Kubernetes environment expansion
- verify current HugeConfig readback in Docker CI

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The entrypoint mapping, the validation and the chart wiring read correctly, and the earlier review points (legacy names in extraEnv, $(VAR) expansion, HugeConfig readback) are addressed at this head. One problem remains on the chart side: with a non-default server.graphSpace the chart's own helm test hook can no longer pass, because it queries DEFAULT-hugegraph on every Pod and a Server assigned to another graph space does not load graphs from DEFAULT. Evidence: static read of the full diff 6aa362a..91b00d2 plus GraphManager.java (filterLoadGraphByServiceGraphSpace, loadGraphsFromMeta, graph()), HugeConfig.java, K8sDriver.java and helm/hugegraph/templates/tests/test-connection.yaml at this head; all CI checks on this head pass, including docker / deployment-contracts and helm / lint-and-render. Nothing was run locally and the finding was not reproduced on a cluster.

Comment thread helm/hugegraph/templates/server-deployment.yaml
- merge the current Apache task and auth baseline
- retain identity and auth cases in the combined fixture
- compile both native readers with pinned dependencies
- expose an explicit graph alias with existing defaults
- retain authenticated per-Pod checks and retry limits
- validate alias input and cover missing graph failures
- explain target graph creation and binding requirements

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The entrypoint mapping, validation order, chart wiring and the new server.testGraphAlias hook read correctly, and the earlier review points are addressed at this head. One gap remains: the accepted service ID character set is wider than what the Server's service registry handles, so IDs containing / or - are accepted and then mis-keyed. Evidence: read the full diff 87239c5..dd10fda (13 files) with docker-entrypoint.sh, GraphManager (loadServices, clearGraphSpace, serviceName, NAME_REGEX), ServiceMetaManager, Service, HugeConfig and K8sDriver at that head; helm unittest on the chart at that head passed (27 suites, 209 tests); helm template accepted server.serviceId=a/b and analytics-prod. The entrypoint test was not run locally (it needs GNU sed), and the docker and helm CI jobs were still queued when this was written.

"serviceId": {
"type": "string",
"minLength": 1,
"pattern": "^[ -~]+$",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: the server.serviceId pattern accepts every printable ASCII character, but the Server's own service bookkeeping uses / and - as separators, so two ordinary-looking IDs are accepted here and then mishandled after startup.

Evidence (static read at dd10fda, not run on a cluster; helm template ci helm/hugegraph --set-string server.serviceId=a/b and ...=analytics-prod both render at this head):

  • /: GraphManager.loadServices stores the Server's own record through ServiceMetaManager.serviceConfKey, which joins .../GRAPHSPACE/<space>/SERVICE_CONF/<name> with /. serviceConfigs() reads the records back with key.split("/") and keeps only the last part. A Server started with team/analytics in team_a is written under .../SERVICE_CONF/team/analytics and is loaded by every later start, and by every other Server, as the service analytics. If team_a also has a real analytics service, both land on the same map key and one record replaces the other in the cache.
  • -: GraphManager.clearGraphSpace walks the cached <space>-<service> keys with key.split("-") and calls dropService(parts[0], parts[1]). For analytics-prod that is dropService("team_a", "analytics"): the record of analytics-prod is left in PD when the space is cleared or dropped, and a different service named analytics is the one looked up.

The entrypoint applies the same printable-ASCII check, and the new tests use hyphenated IDs (service-a, legacy-service), so this is the common case, not a corner one. The README text added here says printable ASCII round-trips unchanged, which holds for the properties file but not for the registry.

Requested change: reject / in server.serviceId (schema pattern and the entrypoint check, with a must_fail case), and for - either reject it the same way or make clearGraphSpace split with a limit of 2 (graph-space names cannot contain -). If the wider character set is intended, please state these two limits in the "Server service identity" sections of both READMEs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants