Skip to content

refactor(docker): expose server discovery identity - #3322

Merged
imbajin merged 15 commits into
apache:masterfrom
hugegraph:fix/server-discovery-config
Oct 15, 2026
Merged

imbajin merged 15 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

Docker and Helm deployments could keep a Server at DEFAULT when another service or graph space was intended. Service metadata could also truncate names containing / or -, and discovery queries used the wrong label for graph space. The image and chart now pass the selected configuration into native properties, retain complete service names and send the correct raw discovery labels.

Main Changes

  • Map HG_SERVER_SERVICE_ID/HG_SERVER_GRAPHSPACE and legacy SERVICE_ID/GRAPH_SPACE aliases, validate deployment inputs before config writes, and round-trip service-name spaces/backslashes through native properties.
  • Expose server.serviceId and server.graphSpace, and let the Helm hook query an explicitly selected existing graph alias.
  • Preserve raw service metadata names and use GRAPHSPACE in Server discovery queries.

Use a chart and Server image build from the same source version containing these mappings. Older release images without the feature do not support non-default settings. The Server's existing SERVICE_ID normalization (GraphManager.serviceId()) remains; query by raw graph space and service name. This PR adds no identifier-only collision guarantee or PD locking/Raft protocol.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests.
  • Need tests and can be verified as follows:

Local verification passed: Java 17 formatting and clean reactor compilation, Helm lint/unit/package checks, native Docker properties readback, and the focused service-metadata/query regressions.

Live non-default PD discovery, authentication and isolation remain deployment checks.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects (deployment settings and service metadata lookup)
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

In-repository Docker and Helm guides are updated. Paired website documentation: apache/hugegraph-doc#517.

Paired source PR: hugegraph#297.

Conceptual overview

Graphspace-scoped service discovery identity mapped from Docker and Helm to Server configuration

概念示意:将 Docker / Helm 的服务身份配置映射到 Server,以图空间限定服务发现查询。Conceptual diagram; not runtime test evidence.

@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 53.52%. Comparing base (92af2f8) to head (dcf93fd).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3322      +/-   ##
============================================
- Coverage     54.31%   53.52%   -0.79%     
+ Complexity     2250     2214      -36     
============================================
  Files           770      763       -7     
  Lines         67952    67768     -184     
  Branches       9213     9251      +38     
============================================
- Hits          36905    36274     -631     
- Misses        27470    27989     +519     
+ Partials       3577     3505      -72     
Flag Coverage Δ *Carryforward flag
hstore 35.67% <40.00%> (?)
pd 50.52% <ø> (?) Carriedforward from 284eade
server_memory 52.48% <100.00%> (?)
server_rocksdb 42.37% <0.00%> (?)
store 31.98% <ø> (?) Carriedforward from 284eade

*This pull request uses carry forward flags. Click here to find out more.

☔ 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

@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

@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.

Comment thread helm/hugegraph/values.schema.json

@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 alias schema permits graph names the Server rejects, so invalid values fail only after helm test starts. Evidence: GraphManager.NAME_REGEX is lowercase-only while the chart schema accepts uppercase.

Comment thread helm/hugegraph/values.schema.json
@imbajin imbajin changed the title fix(docker): expose Server discovery identity fix(docker): expose server discovery identity Oct 13, 2026
Pengzna
Pengzna previously approved these changes Oct 13, 2026
- 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
- reserve legacy identity environment variables
- escape dollars before Kubernetes environment expansion
- verify current HugeConfig readback in Docker CI
- 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
- preserve complete names when reloading service metadata
- keep the service suffix when clearing a graph space
- cover metadata write, reload, removal and space clearing
- document the case of existing Gremlin graph bindings
- retain service identity readback and original test fixtures
- preserve upstream authentication regression coverage
- keep consolidated Docker CI runner and contract classpath
@imbajin
imbajin force-pushed the fix/server-discovery-config branch from a2906f9 to 2241dfd Compare October 13, 2026 20:26

@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: yes. Summary: The new printable-ASCII inputs allow distinct service IDs that GraphManager.serviceId() normalizes to the same PD SERVICE_ID, so discovery queries can return endpoints from both deployments. Evidence: registration writes the normalized value to the SERVICE_ID label, and PD matches query labels by exact equality.

Comment thread hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh

@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. Score: 6/10 (10 - 2 Important already raised - 0.5 Minor = 7.5, capped at 6 for a blocking item). Summary: The entrypoint mapping, legacy migration, validation order, chart wiring, dollar escaping and the full-name metadata fixes read correctly, and their tests pass locally. The blocking item is the one already raised at this head: distinct accepted service IDs still collapse to the same PD SERVICE_ID label. Evidence: ran hugegraph-dist/docker/docker-entrypoint-test.sh (pass, without the HugeConfig readback classpath) and helm unittest for server_discovery_test.yaml and test_hook_gremlin_test.yaml (20/20 pass); rendered the chart with helm template for the serviceId, graphSpace and testGraphAlias cases; traced GraphManager.serviceId(), registerService() and DiscoveryMetaStore.labelMatch(), and PD KvService.scanWithPrefix() to confirm the new prefix substring sees full keys. All CI checks pass at 2241dfd.

Already raised and still open at this head:

  • Important (@imbajin, docker-entrypoint.sh:119): GraphManager.serviceId() lowercases and maps _ to -, so Foo/foo or analytics_prod/analytics-prod in one graph space register the same SERVICE_ID label, and PD label queries return both deployments.

Comment thread helm/hugegraph/templates/server-deployment.yaml
- compare raw service tuples before registration
- fence discovery writes with the current Raft term
- reject ambiguous service pools during discovery
- require compatible image identity mapping in Helm
- cover registry and pinned-image behavior
- integrate current build and runtime changes
- preserve discovery identity validation
- retain pinned-image compatibility checks
- reject older image identity setters before config writes
- preserve spaces through the native properties reader
- cover missing and incomplete encoder capabilities
- document pinned-image compatibility requirements
- integrate current Helm workflow improvements
- retain discovery registration validation
- preserve pinned-image identity protection
- capture the real service URL query sent to PD
- preserve graph space and raw service name filters
- verify registration type and returned endpoints
- merge current ASF master before scope reduction
- preserve submitted discovery history
- retain independent upstream fixes
- remove PD identity and Raft authority changes
- preserve native configuration and raw service-name fixes
- require matching feature-supporting chart and image builds
- remove image source-shape detection and related tests
- retain discovery configuration and graph alias settings
- preserve master wait-image and reserved-environment updates
- reconcile service identity guidance with the simplified guide

@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. Score: 9/10 (10 - 0.5 for my earlier Minor, still open - 0.5 Minor for the residual SERVICE_ID overlap from @imbajin). Summary: Since 2241dfd the PR drops the PD identity fencing and image check it briefly carried (no PD or Store file remains in the diff), sends GRAPHSPACE instead of a second REGISTER_TYPE in GraphManager.getServiceUrls() with a test that captures the real query, and documents both remaining limits. I found no new defect. Evidence: read the PR diff at the head and the interdiff from 2241dfd; GraphManagerServiceNamesTest 3/3 pass; helm unittest 36 suites / 305 tests and helm lint pass; docker-entrypoint-test.sh passes with the HugeConfig readback classpath; traced registerServiceToPd(), DiscoveryMetaStore.labelMatch() and PDHugeClientFactory.getURLs() in hugegraph-toolchain. Some advisory CI jobs are still pending, and nothing was run against a live PD cluster.

Earlier points:

  • @imbajin, docker-entrypoint.sh:119: the narrowing holds. The Server registers the raw GRAPHSPACE and SERVICE_NAME labels and the Java client and Hubble query exactly those, so Foo/foo and analytics_prod/analytics-prod stay distinct. No caller in this repo or in hugegraph-toolchain filters on SERVICE_ID alone. The normalisation in GraphManager.serviceId() predates this PR and both READMEs state the limit.
  • Correction to my review at 2241dfd: calling this blocking was too broad for the reason above. It is a documented residual limit, not a discovery failure on the supported path.
  • My earlier Minor, server-deployment.yaml:263: unchanged in code; both READMEs now require a matching image build. The default tag is latest, so default installs are not affected.

@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 and validation, the Helm wiring and $ escaping, the full-name service metadata reload, the single split in clearGraphSpace and the GRAPHSPACE label in getServiceUrls() all read correctly at this head, and I found no new code defect; one Minor documentation inaccuracy remains about where the SERVICE_ID normalization happens. Evidence: read the full diff 4b77b7b..284eade (17 files) in a scratch worktree, with GraphManager (serviceId(), loadServices(), registerServiceToPd(), registerK8StoPd(), filterLoadGraphByServiceGraphSpace()), ServiceMetaManager.serviceConfKey(), PdMetaDriver/EtcdMetaDriver.scanWithPrefix(), HugeConfig.get() (reads through getProperty, no interpolation), ServerOptions, K8sDriver (emits raw GRAPH_SPACE/SERVICE_ID) and PD DiscoveryMetaStore.labelMatch(). getServiceUrls() has no in-tree caller besides the new test. All latest-head CI jobs pass, including docker / docker-build (its deployment-contracts step runs docker-entrypoint-test.sh with the HugeConfig readback classpath) and helm / lint-and-render; codecov/project reports -0.78% against base 92af2f8 while codecov/patch passes. Nothing was run locally or against a live PD cluster.

Comment thread docker/README.md Outdated
- attribute SERVICE_ID normalization to GraphManager
- align Docker and Helm service identity guidance
- preserve existing discovery and collision contracts

@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: Score 9/10. The entrypoint mapping and validation of HG_SERVER_SERVICE_ID / HG_SERVER_GRAPHSPACE (and the legacy SERVICE_ID / GRAPH_SPACE names), the chart wiring with dollar escaping, the selectable helm test alias, the full-name service metadata reload, the single split in clearGraphSpace and the GRAPHSPACE label in getServiceUrls() all read correctly at this head, and the default DEFAULT/DEFAULT render is unchanged; I found no new defect. The two residual limits already raised in this thread (older images ignore the new variables, and GraphManager.serviceId() still normalizes SERVICE_ID) are documented in both READMEs and are not made worse here. Evidence: read the full diff 4b77b7b..dcf93fd (17 files) in a scratch worktree with GraphManager (serviceId(), loadServices(), registerServiceToPd(), registerK8StoPd(), clearGraphSpace(), filterLoadGraphByServiceGraphSpace()), ServiceMetaManager.serviceConfKey(), PdMetaDriver/EtcdMetaDriver.scanWithPrefix() and PD KvService.scanWithPrefix() (returns full keys, so the new prefix substring is exact), HugeConfig.get() and loadConfigFile() (Commons 2.10.1 properties, no list splitting or interpolation), HugeFactory.checkGraphName() and K8sDriver; sourced the entrypoint's encode_prop_value and set_prop_encoded (GNU sed) and read 30 service IDs back through commons-configuration2 2.10.1, including '=x', ':x', '#x', '!x', 'a,b', trailing and lone backslashes, '${HOME}', leading, trailing and doubled spaces, '\uZZZZ' and the sed metacharacters & ~ | / \1, all unchanged; helm unittest server_discovery_test.yaml and test_hook_gremlin_test.yaml (20/20 pass), helm lint, and helm template rejects serviceId=café, graphSpace=TeamA and testGraphAlias=team_a-demo-x while rendering 'a b$(X)' as 'a b$$(X)'. All latest-head checks pass except codecov/project (-0.79% against a base 11 commits behind; codecov/patch passes). The Java unit test and docker-entrypoint-test.sh were not rerun locally (CI ran both at this head), and nothing was run against a live PD cluster.

@imbajin imbajin changed the title fix(docker): expose server discovery identity refactor(docker): expose server discovery identity Oct 15, 2026
@imbajin
imbajin merged commit d90946d into apache:master Oct 15, 2026
18 of 19 checks passed
@imbajin
imbajin deleted the fix/server-discovery-config branch October 16, 2026 02:58
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.

3 participants