chore: add TODO breadcrumbs for the Helm chart's server workarounds - #3260
bitflicker64 wants to merge 2 commits into
Conversation
The Helm chart under review in apache#3218 carries guards and runbook steps that compensate for server-side behavior rather than Kubernetes concerns: ASCII Basic-auth decoding and the colon split, a startup graph load that never offers the graph to Gremlin Server, PD health that cannot see a raft error or an unopened KV store, a Store marked Up before its partition engines are restored, task routes that answer the same for a run and a no-op, a password cache invalidated on one replica only, a pinned RocksDB memory ceiling, and properties keys with no environment mapping. Each site gets a TODO stating what is wrong or missing, what the fix does, and which chart guard is removed afterwards, linking the tracking issue where one exists. Comments only; no behavior change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3260 +/- ##
============================================
+ Coverage 41.57% 41.59% +0.02%
- Complexity 7311 7318 +7
============================================
Files 793 794 +1
Lines 69106 69127 +21
Branches 9258 9258
============================================
+ Hits 28730 28753 +23
- Misses 37098 37100 +2
+ Partials 3278 3274 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Answers the live validation and review on the chart PR. helm test: keep the succeeded hook Pod so `helm test --logs` can read it; require one complete clean sweep per attempt instead of carrying earlier successes across retries; count one address family per Pod so a dual-stack headless Service does not count each Pod twice. The CI fixture gains the alternating-health and dual-stack counterexamples. Credentials: the admin password contract is printable ASCII with no backslash, colon or padding, enforced by the schema pattern and by the Server wrapper on an existingSecret, because the Server decodes Basic auth as ASCII and splits it on every colon. The Hubble PD-secret check and the Server check are byte-wise under the C locale, so the image locale cannot widen them. Rollout checksums: a chart-generated Secret contributes the digest of its value, generated once per render and memoised, so the install hashes the value it writes and the first no-change upgrade renders the same checksum and rolls nothing; that upgrade used to roll PD and Server together and start Servers against a PD member mid-restart. Inline values keep their digest, an operator-supplied existingSecret keeps its resourceVersion. Identity: validateValues reads the live StatefulSets and refuses a changed pd.ports.raft or store.ports.raft, a rename through nameOverride or fullnameOverride, and a selector-label change, on an initialized release. A NodePort or LoadBalancer Hubble Service needs hubble.service.allowInsecureExposure=true, like PD. README: settings-by-lifecycle table, rollback boundary with a post-rollback check, template-only GitOps section; NOTES.txt warns when generated credentials are in use and says to rerun helm test after an upgrade that rolls PD and Server. Server-side root causes carry a TODO at their site in apache#3260.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The TODOs identify real operational gaps, but several statements point to work already present or imply behavior the nearby code cannot provide. Evidence: exact-head source review, the companion chart source in PR #3218 and 22/22 latest-head checks passed.
| fi | ||
|
|
||
| # ── Map env → properties file ───────────────────────────────────────── | ||
| # TODO: map server.urls_to_pd, server.deploy_in_k8s and auth.admin_pa from the environment here. |
There was a problem hiding this comment.
🧹 Blocking: no. Summary: This TODO says auth.admin_pa still needs an environment mapping, but this script already maps PASSWORD to auth.admin_pa at lines 175–176. The chart wrapper is also not the only read-only filesystem blocker because this entrypoint continues editing configuration through set_prop. Please remove auth.admin_pa from the missing list and clarify the remaining writable-configuration requirement. Evidence: the existing PASSWORD branch and set_prop calls in this file.
| // offline or up, or in the initial activation list, go live automatically | ||
| // TODO: do not mark a re-registering Store Up before it has restored its partition engines | ||
| // (HgStoreEngine.restoreLocalPartitionEngine); report a restoring state, or expose | ||
| // restore-complete per shard group, so a rolling restart can wait on it. The Helm chart |
There was a problem hiding this comment.
🧹 Blocking: no. Summary: The relative helm/hugegraph path is absent from this PR's tree, so readers cannot follow the rollout procedure from this revision. Please link the companion chart source or apache/hugegraph#3218, and update the other new helm/hugegraph references too. Evidence: helm/hugegraph is added only in the separate, still-open chart PR.
| rocksdb: | ||
| # rocksdb total memory usage, force flush to disk when reaching this value | ||
| # TODO: make this overridable from the environment, or default it to 0 so AppConfig falls back | ||
| # to the JVM max heap: pinned at 32 GB, a Store in a container with a smaller memory limit is |
There was a problem hiding this comment.
🧹 Blocking: no. Summary: The configured 32 GB is an LRUCache capacity, not memory allocated up front, so a smaller container limit does not by itself guarantee an OOM kill. Please say cache growth can exceed the container limit and cause OOM under load. Evidence: RaftRocksdbOptions passes total_memory_size as the capacities of two LRUCache instances.
| public void onLeaderStop(final Status status) { | ||
| this.leaderTerm.set(-1); | ||
| // TODO: keep STATE_ERROR set by onError instead of overwriting it with STATE_FOLLOWER, so | ||
| // the probe view (and /v1/health, see StoreAPI.checkHealthy) can report a PD that stepped |
There was a problem hiding this comment.
🧹 Blocking: no. Summary: Preserving STATE_ERROR here affects the probe view used by readiness, but StoreAPI.checkHealthy still returns 200 without consulting Raft state. Please scope this TODO to /v1/ready and point to the separate StoreAPI health TODO. Evidence: checkHealthy returns an empty string and its Javadoc says it does not read the Raft state.
encode_prop_value escapes a space as a backslash and a space, which Commons Configuration keeps, so an admin password containing a space is stored with backslashes and the operator's value answers 401. The Helm chart refuses such passwords until the encoder round-trips them.
Admin password: the image entrypoint writes auth.admin_pa with each space escaped as a backslash and a space, and Commons Configuration keeps that backslash, so an accepted password containing a space was applied with the backslashes and the Secret value answered 401. The schema pattern and the Server wrapper now refuse any space, with a message naming the cause. A CI step runs the rendered wrapper against Secret values; the inner-space positive control becomes a must-fail. The image-side TODO at encode_prop_value is in apache#3260. NOTES: a template-only render now rolls the Pods on every sync (the checksums hash the generated value); the warning said the opposite. Server Service: a NodePort or LoadBalancer type publishes the plain HTTP API, Basic credentials and JWTs included, and was accepted silently. It now needs server.service.allowInsecureExposure=true, the acknowledgement PD and Hubble already use.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The TODO text matches the code at each site I checked, apart from the four points already raised on ff2b1d8, which are still open at a12513b. One breadcrumb is in the wrong class: the password-cache TODO sits in StandardAuthManager, but a Server on the shared PD catalog (hstore) runs StandardAuthManagerV2, so a fix made where the TODO points leaves the chart's rotation limitation in place. Evidence: static review of the full diff at a12513b; StandardHugeGraph picks StandardAuthManagerV2 when isHstore(); the backslash-space claim in the new docker-entrypoint.sh TODO checked with commons-configuration2 2.8.0 (a value written as a\ b reads back with the backslash kept); 22/22 latest-head checks passed.
| } | ||
|
|
||
| private void invalidatePasswordCache(Id id) { | ||
| // TODO: invalidate the password and token caches on every Server replica, not only on the |
There was a problem hiding this comment.
Important: This TODO describes the shared PD catalog case, but that deployment does not run this class.
StandardHugeGraph (around line 281 at this head) builds new StandardAuthManagerV2(this.params) when isHstore() and StandardAuthManager only otherwise. The Helm chart's Server uses the hstore backend, so its password and token caches live in StandardAuthManagerV2, which has its own pwdCache, tokenCache and a separate invalidatePasswordCache(Id) (around line 254). Someone who follows this breadcrumb and fixes StandardAuthManager leaves the chart's admin password rotation limitation unchanged, and nothing at the V2 site tells them about it.
Requested change: put this TODO on StandardAuthManagerV2.invalidatePasswordCache. If you also want to keep it here, reword it for the non-PD case (several Servers sharing one backend such as HBase) so it does not claim the PD catalog path.
2 participants
Purpose of the PR
The chart carries guards, probe derivations and runbook steps that compensate for server-side behavior rather than Kubernetes concerns. Each of those workarounds should disappear when the server behavior is fixed, and the only way a later change at the server site learns that is a comment at that site. This PR leaves one TODO per site, stating what is wrong or missing, what the proper fix does, and which chart guard is removed afterwards, linking the tracking issue where one exists.
Main Changes
Comments only, 14 sites, no behavior change:
AuthenticationFilter.java(Basic decode):answers 400, both accepted at account creation:latest2026-10-03AuthenticationFilter.java(FIXED_WHITE_API_SET)/readinessuntil #3221 landsserver.readinessPathdefaults to/versions,/readinessis opt-inGraphManager.loadGraphhelm test, documented Pod deletionStoreAPI.checkHealthy/v1/healthstays 200 inSTATE_ERRORand with an unopened KV store/v1/readyRaftStateMachine.onLeaderStopSTATE_ERRORwithSTATE_FOLLOWERStoreNodeService(registration)Upbefore its partition engines are restoredOnDeletewith a manual/v1/shardGroupsbarrierHgStoreEngine.restoreLocalPartitionEngineHgKVStoreImpl.initLOCKleaves PD running uninitialized; nothing restarts itTaskAPI.balanceLeadersStandardAuthManager.invalidatePasswordCacheServerOptions.ADMIN_PAdocker-entrypoint.sh(env mapping)server.urls_to_pd,server.deploy_in_k8s,auth.admin_pahave no environment mappingrest-server.propertiesin a wrapper and refusesreadOnlyRootFilesystemfor Serverdocker-entrypoint.sh(encode_prop_value):latest2026-10-03application-pd.yml(rocksdb.total_memory_size)Two Hubble-side workarounds (the Hubble properties parser and the missing environment mapping for
operations.pd.password) belong to hugegraph-toolchain and are not in this PR.Verifying these changes
Comments only. Lines stay within 100 characters.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need