chore: add TODO breadcrumbs for the Helm chart's server workarounds - #3260
bitflicker64 wants to merge 1 commit 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.58%
Complexity 7311 7311
=========================================
Files 793 793
Lines 69106 69106
Branches 9258 9258
=========================================
+ Hits 28730 28736 +6
+ Misses 37098 37096 -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.
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, 13 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 Serverapplication-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