Skip to content

fix(store): size the RocksDB memory budget from available memory - #3355

Open
byteayan wants to merge 14 commits into
apache:masterfrom
byteayan:fix/store-rocksdb-memory-budget
Open

byteayan wants to merge 14 commits into
apache:masterfrom
byteayan:fix/store-rocksdb-memory-budget

Conversation

@byteayan

@byteayan byteayan commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

Both shipped application-pd.yml files set rocksdb.total_memory_size: 32000000000, and the pd profile is always included, so the Runtime.maxMemory() fallback in AppConfig.init() never ran. RaftRocksdbOptions splits the value into the WriteBufferManager cache (66%) and the block cache. Both are native LRUCaches, so neither -Xmx nor the container limit bounds them, and a separate 1 GiB raft log cache sits on top. In the run in the issue all three Stores were OOM-killed at a 4 GiB limit once each held about 1 GB of RocksDB data.

Docker users had no clean way to change it. docker-entrypoint.sh builds SPRING_APPLICATION_JSON from five HG_STORE_* keys, and none of them is a rocksdb.* key. Bind-mounting a config file or passing -Drocksdb.total_memory_size=<bytes> in JAVA_OPTS both work, but neither is documented.

As the issue points out, dropping the line from the yml alone would hand RocksDB a budget equal to the heap, which is a second heap-sized allocation on top of the first.

Main Changes

  • New RocksdbMemoryBudget in hg-store-core/.../options. An unset, blank or 0 value means auto: half of (physical memory − max heap − MaxDirectMemorySize − 1.5 GiB reserve), clamped to 256 MiB to 32 GB. The reserve covers the raft log cache plus stacks, metaspace and other native memory. Physical memory comes from OperatingSystemMXBean.getTotalMemorySize(), which on JDK 17 reports the cgroup v1/v2 limit inside a container. Older JDK 17 updates may not see a limit set only on a parent cgroup, so the budget also reads the cgroup itself (v2: memory.max from the process's cgroup up to the mount root; v1: hierarchical_memory_limit) and uses the lower value. The membership path is rebased onto the mount's root from /proc/self/mountinfo, so a cgroupfs mounted from a subtree such as /kubepods is walked from the right directory. Every mount that contains the process's cgroup is checked and the lowest limit wins, so mount order doesn't matter. Inside a private cgroup namespace a limit above the container can't be seen, and the deployment guide says to pin the value there. MaxDirectMemorySize is counted as the JVM reports it, or as the heap size when unset, since that is what the JVM then allows. bin/start-hugegraph-store.sh now passes -XX:MaxDirectMemorySize equal to the heap it picks, unless -j sets -Xmx or the cap itself, in which case the JVM default (that heap) or the given cap applies. Either way the budget counts a cap the JVM enforces. If the Store couldn't locate its own cgroup in /proc/self/mountinfo (an inherited mount in a cgroup namespace), the limit at the visible mount root still counts and auto-sizing logs a warning to set the budget explicitly. If physical memory can't be read, the previous heap-size default applies.
    • The issue's setup (4 GiB limit, -Xmx1g, -XX:MaxDirectMemorySize=512m) gets 512 MiB, and an 8 GiB limit gets 2.5 GiB. I checked the 4 GiB case with resolve() inside eclipse-temurin:17-jre-jammy under a 4 GiB Docker limit: 536870912 bytes. With -Xmx2g and no direct-memory cap it lands on the 256 MiB floor. The 32 GB cap means no host gets a bigger budget than before. On bare metal the default launcher (2 GiB heap, 2 GiB direct cap) on a 16 GiB host gets 5.25 GiB. A raised heap should come with a smaller explicit cap, for example -j "-Xmx36g -XX:MaxDirectMemorySize=2g" (12.25 GiB on 64 GiB); without one, direct memory counts as a second heap and the floor applies. A large node that relied on the fixed 32 GB should pin the value after upgrading.
    • A positive value is used unchanged, with one exception: values under 1 MiB are refused. The write buffer manager gets 2/3 of the budget, so 1 would give a 0-byte manager, which RocksDB treats as no memtable limit at all. 4GB and other non-numeric values now fail at startup with "must be a number of bytes", where before they failed with a bare NumberFormatException. Nothing shipped (configs, compose files, Helm, CI) sets a value under 1 MiB.
    • Startup logs the chosen value and its inputs. It warns when the 256 MiB floor applies (heap plus direct memory already fill the limit), when an explicit value is under 256 MiB, and when an explicit value exceeds the memory left after the heap, direct memory and the reserve. That last case is what an upgraded install with an old mounted application-pd.yml will hit; the Store also warns when it sees exactly 32000000000, the old shipped value, and the deployment guide has an upgrade step (set it to 0, remove it, or set HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE).
  • AppConfig.init() uses it. While there I fixed the raft disruptor sizing for budgets under 1 GB: log(0) rounded to a buffer size of 0. It now gives 32. The formula moved to RocksdbMemoryBudget.disruptorBufferSize() so it has a test. The shipped application.yml pins disruptorBufferSize: 1024, so this only matters when that key is absent.
  • Both application-pd.yml files ship total_memory_size: 0, which also removes the [Bug] Store ships rocksdb.total_memory_size of 32 GB, ignores the container memory limit, and Docker users cannot override it #3254 TODO from chore: add TODO breadcrumbs for the Helm chart's server workarounds #3260. The test resources keep their explicit values.
  • docker-entrypoint.sh accepts HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE. Values must be digits only; anything else exits with code 2. It is added to SPRING_APPLICATION_JSON only when set, so an unset variable leaves the config file or auto-sizing in charge. When set it outranks both the config file and -Drocksdb.total_memory_size (checked against Spring Boot 2.5's property order).
  • helm/hugegraph/values-cluster.yaml: the store comment said container limits don't configure the RocksDB budget. It now says how the budget is derived (2.5 GiB for that preset) and how to pin it with store.extraEnv.
  • CI fixture, needed for this PR's HStore job: start-server.sh now writes usePD, schema.sync_deletion and the HBase timeouts idempotently. Since fix(api): allow anonymous cypher queries #3366, run-api-test.sh starts the same directory twice, and the duplicated usePD failed the second init-store with '[true, true]'. The new test-start-server.sh covers two starts on one directory.

In a small container the write buffer manager now flushes memtables much earlier than before, which means more small L0 files and lower write throughput in exchange for staying inside the limit. Both caps are soft, so RocksDB can still go over briefly under heavy writes. I have not measured either effect. Setting the value explicitly restores the old behaviour.

Verifying these changes

  • Need tests and can be verified as follows:
    • New RocksdbMemoryBudgetTest (23 tests, added to CoreSuiteTest, which the store-core-test profile runs). It covers:
      • explicit values kept unchanged, and values under 1 MiB refused;
      • unset, blank, 0 and 00 going to auto;
      • refusal of 4GB, -1, 1.5e9 and a value past Long.MAX_VALUE;
      • the 4 GiB and 8 GiB examples above, both clamp bounds, and the unknown-physical fallback;
      • the disruptor size for 512 MiB, 1 GB and 32 GB;
      • the derived write_buffer_size staying at or above RocksDB's 1 MiB minimum, checked through the same HugeConfig validation as startup;
      • unset direct memory counted as the heap, and bare-metal sizes with and without an explicit direct cap;
      • an inherited namespace mount (root /..) that can't be matched being flagged, with the visible root's limit still applied;
      • cgroup limits read from fake v1 and v2 trees: a limit set only on the pod, the container's own lower limit, the namespace-root view, v1 and v2 subtree mounts rebased through mountinfo (including an escaped mount point), several mounts in different orders, unlimited values, and an unreadable /proc/self/cgroup.
    • New test-store-docker-entrypoint.sh (9 cases), added to the store job in pd-store-ci.yml. It checks that the rocksdb key is absent when the variable is unset or empty, that bytes and 0 pass through, that app.data-path is still set, and that 4GB, -1, 1.5e9, 1 2 and 1"} are refused. It clears the variable from the caller's environment first, so "unset" really is unset.
    • mvn test -pl hugegraph-store/hg-store-test -P store-core-test on JDK 17: 128 tests, 0 failures. test-start-hugegraph-store.sh --gc-only (run in the store CI job) covers the launcher with a stub java: no -j gets the computed cap, a heap-only -j (lower or raised) leaves the cap to the JVM, and an explicit -j cap applies. python3 -m unittest discover -s .github/scripts -p 'test_*.py' passes. helm unittest helm/hugegraph: 294 tests pass. mvn editorconfig:check and git diff --check are clean.
    • Not tested: a full Store started in a memory-limited container, and the load from the issue.
    • test-store-docker-entrypoint.sh is mapped to the store suite in .github/scripts/ci-policy.py, so a PR that changes only that script still runs it; the selector test covers the mapping.

Does this PR potentially affect the following parts?

Documentation 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-store/docs/deployment-guide.md: the auto-sizing rule and its inputs, the 1 MiB minimum, the new env var and its precedence in the compose and Kubernetes examples, and the RocksDB block (total_memory_size: 0; the nonexistent block_cache_size key replaced with write_buffer_ratio).
  • hugegraph-store/README.md: the env var in the docker run section.
  • Paired docs(store): document the auto-sized RocksDB memory budget hugegraph-doc#535 (merge together with this PR): content/{en,cn}/docs/quickstart/hugegraph/hugegraph-hstore.md (the default, the auto-sizing rule and the env var) and content/{en,cn}/docs/guides/hugegraph-docker-cluster.md (the new HG_STORE_* row). The hstore page's "checked against Server master commit" note should move to this PR's merge commit once both land.

Conceptual overview

RocksDB memory budget derived from available memory with clamp bounds and a one MiB derived write-buffer floor

Concept illustration: derive the RocksDB budget from memory remaining after heap, direct memory and reserve, with clamp bounds and a 1 MiB derived write-buffer floor. Cache budgets remain soft.

The shipped application-pd.yml set rocksdb.total_memory_size to 32 GB.
RaftRocksdbOptions splits that into the write buffer manager's cache and
the block cache, both off-heap, so -Xmx and the container limit do not
bound them. A Store under a 4 GiB limit was OOM-killed once it held about
1 GB of data (apache#3254). The heap-size fallback in AppConfig never ran,
because the key was always set.

total_memory_size now ships as 0. Unset or 0 sizes the budget as half of
what is left after the heap, MaxDirectMemorySize (heap size when unset)
and a 1.5 GiB reserve for the raft log cache and other native memory,
clamped to 256 MiB..32 GB. Physical memory comes from the
OperatingSystemMXBean, which on JDK 17 reports the cgroup v1/v2 limit in
a container; older JDK 17 updates may not see a limit set only on a
parent cgroup. A positive value is used as given, but values under 1 MiB
are refused, since the write buffer manager's 2/3 share would round to
0 bytes and lift the memtable limit. A non-numeric value such as "4GB"
fails with a clear message instead of a bare NumberFormatException.
Startup warns when the floor applies, when an explicit value is under
256 MiB, or when it exceeds what is left after the heap, direct memory
and the reserve.

The Docker entrypoint, which builds SPRING_APPLICATION_JSON itself, had
no HG_STORE_* variable for any rocksdb.* key. It now accepts
HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE (bytes) and adds it only when set;
the value outranks the config file and -Drocksdb.total_memory_size.

AppConfig also stops computing a zero raft disruptor buffer for budgets
under 1 GB, where log(0) rounded to a size of 0; the formula moved to
RocksdbMemoryBudget.disruptorBufferSize() so it can be tested. The
shipped application.yml pins disruptorBufferSize, so this only applies
when that key is unset.

The comment in helm/hugegraph/values-cluster.yaml that said container
limits do not configure the RocksDB budget now describes the auto-sizing
and HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE.

Fixes apache#3254
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.56425% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.02%. Comparing base (92af2f8) to head (9e6dc50).
⚠️ Report is 28 commits behind head on master.

Files with missing lines Patch % Lines
...e/hugegraph/store/options/RocksdbMemoryBudget.java 81.56% 15 Missing and 18 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3355      +/-   ##
============================================
+ Coverage     54.31%   56.02%   +1.71%     
- Complexity     2250     6486    +4236     
============================================
  Files           770      775       +5     
  Lines         67952    68679     +727     
  Branches       9213     9631     +418     
============================================
+ Hits          36905    38477    +1572     
+ Misses        27470    26624     -846     
- Partials       3577     3578       +1     
Flag Coverage Δ
commons 96.37% <ø> (?)
hstore 35.82% <ø> (?)
pd 49.27% <ø> (?)
server_hbase 41.88% <ø> (?)
server_memory 54.39% <ø> (?)
server_rocksdb 42.35% <ø> (?)
store 32.60% <81.56%> (?)

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

With write_buffer_size unset or 0, AppConfig derives it as a thousandth
of the RocksDB budget. An auto-sized budget can now be under 1 GiB: a
4 GiB container with -Xmx1g and -XX:MaxDirectMemorySize=512m gets
512 MiB, which derived 536870 bytes. rocksdb.write_buffer_size requires
at least 1 MiB (RocksDBOptions.MEMTABLE_SIZE), so Store startup would
have rejected the config in BusinessHandlerImpl.initRocksdb().

RocksdbMemoryBudget.writeBufferSize() clamps the derived value to 1 MiB,
and a test runs it through the same OptionSpace/HugeConfig validation.
The shipped write_buffer_size of 32000000 never took this path.

@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: Score 7/10. The container case from #3254 is handled, and the earlier write_buffer_size thread is fixed by writeBufferSize() at 1927fbe. One blocking problem: auto-sizing counts an unset -XX:MaxDirectMemorySize as a second full heap, and the tarball launcher never sets it, so bare-metal installs with a large heap drop from the shipped 32000000000 bytes to the 256 MiB floor on upgrade. Evidence: the head's RocksdbMemoryBudget.java compiled standalone and resolve(null) run on a 16 GiB host with JDK 17.0.20 returned 268435456 for -Xmx8g and 3221225472 for -Xmx8g -XX:MaxDirectMemorySize=512m; start-hugegraph-store.sh, util.sh calc_xmx, the Store Dockerfile, RaftRocksdbOptions and the Helm store template were read at e691489. Only HugeGraph-Server CI and check-license are listed for this head, both queued, so the new Store tests were not seen passing in CI.

Auto-sizing counted an unset -XX:MaxDirectMemorySize as a second full
heap, because that is what the JVM allows. bin/start-hugegraph-store.sh
never sets the flag, so a bare-metal Store with a raised heap fell to
the 256 MiB floor on upgrade: -Xmx8g on a 16 GiB host, or -Xmx36g on a
64 GiB host, went from the old fixed 32000000000 bytes to 268435456.

The Store's direct buffers (gRPC/Netty) do not grow with the heap, so
an unset cap now counts as the heap size capped at 1 GiB. Those two
hosts get 2.75 GiB and 12.75 GiB. An explicit -XX:MaxDirectMemorySize
is still counted as given, so the container case from apache#3254 is
unchanged. The deployment guide now says what bare-metal hosts get.

@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 earlier threads are fixed at this head: the derived write_buffer_size is clamped to 1 MiB, and an unset -XX:MaxDirectMemorySize now counts as at most 1 GiB, so a large-heap bare-metal Store no longer drops to the 256 MiB floor. The auto budget fits the #3254 container setup, and an explicit value still overrides it. Evidence: read RocksdbMemoryBudget, AppConfig.init(), RaftRocksdbOptions, RocksDBOptions, the Store launcher and calc_xmx, the Dockerfile, the entrypoint and both Helm values files at 84e8e27; worked through the arithmetic for 4 GiB/-Xmx1g/512m (512 MiB), the Helm cluster preset (2.5 GiB) and 16 GiB/-Xmx8g (2.75 GiB); ran test-store-docker-entrypoint.sh locally (9 passed); CI at this head ran CoreSuiteTest with the new tests (52 tests, 0 failures) and the entrypoint step, and cluster, docker and helm jobs pass. Only codecov/project fails, because the base is 19 commits behind.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: Auto-sizing can still use host memory on supported older JDK 17 builds that miss parent-only cgroup limits. Evidence: exact-head source and cgroup behavior review; see inline comments for the mounted-config upgrade and Store test-selection gaps.

Comment thread hugegraph-store/docs/deployment-guide.md Outdated
Comment thread .github/workflows/pd-store-ci.yml
test-store-docker-entrypoint.sh runs in the store job of
pd-store-ci.yml, but the selector didn't map it, so a PR that changed
only that script selected server, docker and macOS paths and skipped
the test. Map it to {"store"} in CONTRACT_SUITES and extend the selector
test so it checks the store job's condition and step as well.
Older JDK 17 updates can miss a cgroup memory limit set only on a parent
cgroup and report host memory, so auto-sizing could still give RocksDB
most of host RAM inside a smaller pod or systemd slice. RocksdbMemoryBudget
now also reads the cgroup itself: on v2 it walks memory.max from the
process's cgroup up to the mount root, on v1 it reads
hierarchical_memory_limit, and it uses the lower of that and the JVM
value. A limit above a private cgroup namespace stays invisible, which
the deployment guide now says.

An existing deployment with a mounted or kept application-pd.yml still
has total_memory_size: 32000000000, which is explicit and bypasses
auto-sizing. The Store now warns when it sees that exact value, and the
deployment guide has an upgrade step to set it to 0, remove it, or set
HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE.
/proc/self/cgroup gives the membership path from the hierarchy root,
but the cgroupfs can be mounted from a subtree: mountinfo root
/kubepods at /sys/fs/cgroup puts the pod's memory.max at
/sys/fs/cgroup/pod1, not /sys/fs/cgroup/kubepods/pod1. The resolver
looked for the latter, fell back to the unlimited mount root and missed
the visible 4 GiB pod limit.

Read the cgroup2 mount, or the cgroup v1 mount carrying memory, from
/proc/self/mountinfo and strip its root from the membership path before
walking ancestors. Without a mountinfo entry the default /sys/fs/cgroup
layout is used as before.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Consider compatible readable cgroup mount views instead of returning the first match.

The original mount-root translation finding is fixed at this revision: independent Java 21 synthetic-file checks using the exact resolver, with only Lombok logging stubbed, now find the 4 GiB limit and produce the 512 MiB budget. Fourteen bounded checks pass, but three multi-mount discrepancies remain, confirmed by two independent reviews.

The resolver returns the first matching cgroup mount without considering later readable views. With membership /kubepods/pod1/ctr, an unrelated first mount or a leaf-only first mount followed by a full readable hierarchy can hide a visible 4 GiB ancestor. With a simulated 64 GiB host, 1 GiB heap and 512 MiB direct cap, the budget is then 32,000,000,000 bytes instead of 536,870,912. Reversing mount order succeeds; this reproduces for both v2 and v1.

Please consider compatible readable mount views and their ancestor limits, with order-invariance regressions. Selecting only the most-specific mount can still omit visible ancestors. Multiple mounts are a plausible layout: see the OpenJDK duplicate-mount fix and runc issue #2404.

Evidence boundary: synthetic file fixtures, not real mounts, full Store startup, performance or load measurement. A correct JVM container limit can mask the overbudget path; this is conditional, not a claim that every container is affected. No new numerical score is assigned.

mountinfo can list several mounts of the same controller, and the
resolver used the first one. An unrelated subtree or a leaf-only view
listed before the full hierarchy hid the pod's visible limit, so the
result depended on mount order.

Read every cgroup2 mount, or every v1 mount carrying memory, skip those
whose root doesn't contain the process's cgroup, walk each remaining
one and keep the lowest limit. With no matching mount the default
/sys/fs/cgroup layout is used as before.
@byteayan

Copy link
Copy Markdown
Contributor Author

@imbajin thanks, fixed in 535c2d0. The resolver now reads every cgroup2 mount, or every v1 mount carrying memory, from mountinfo instead of the first one. It skips mounts whose root doesn't contain the process's cgroup, walks each remaining one from its rebased path, and keeps the lowest limit. A leaf-only or unrelated mount listed first can no longer hide a fuller view listed later. New tests run three mount orders for v2 (unrelated, leaf-only and full views) and two for v1 (with a cpu-only mount that's ignored). Each finds the 4 GiB pod limit and, with a 64 GiB host, 1 GiB heap and 512 MiB direct cap, gives 512 MiB. Store core tests: 126 passed.

@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: 10/10.

Since 33e53ee, 535c2d0 reads every cgroup2 mount, or every v1 mount carrying memory, from mountinfo. It skips mounts whose root does not contain the process cgroup and keeps the lowest limit across the rest. I reran my earlier synthetic case against this head. A leaf-only cgroup2 mount listed before the full hierarchy now gives 4294967296 instead of -1, and the hybrid v2 plus v1 layout still resolves to the v1 limit. I also tried a private namespace with 0::/, a 0::/../sib path, a /.. mount root, the v1 9223372036854771712 sentinel, a nested child cgroup and an escaped mount point. Each gave the expected limit or fell back to the mount root.

Evidence: on a cgroup v2 host (JDK 17.0.20) I ran resolve() inside a transient slice with MemoryMax=4G and an unlimited service, using -Xmx1g -XX:MaxDirectMemorySize=512m. The JVM reported 16473444352, resolve(null) and resolve("0") returned 536870912, and resolve("32000000000") was kept. Locally at 535c2d0, RocksdbMemoryBudgetTest passed 21 tests and CoreSuiteTest passed 62 with 0 failures. CI at this head is still running.

Earlier threads:

  • @imbajin, cgroup mount first match: fixed in 535c2d0.
  • @imbajin, subtree mount root not rebased: fixed in 33e53ee, still holds.
  • @imbajin, entrypoint test not picked by CI policy: fixed in 3fb0da0.
  • @imbajin, parent-only limit missed by older JDK 17, and the old application-pd.yml keeping 32 GB: fixed and documented in 03a4d43, with a WARN on 32000000000.
  • @imbajin, derived write_buffer_size below 1 MiB: fixed in 1927fbe.
  • Mine, unset MaxDirectMemorySize counted as a full heap: fixed in 84e8e27, still holds.

Not covered: a full Store under write load in a memory-limited container.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rechecked the multi-mount finding against the updated source, reviewed 2026-10-16. The prior finding is fixed: all applicable mount views are considered, unrelated roots are excluded, and the minimum visible limit wins rather than the first mount. Two independent source reviews found no new blocker in this repair.

I independently replayed the three earlier counterexamples and added reordered v1/v2 mount and containment controls: 40 bounded Java 21 synthetic-file assertions passed. The executable copy retained the exact resolver/sizing code and replaced only Lombok logging with a no-op logger. This was not a full Store/container load test or a performance measurement.

Provisional assessment: 8.5/10, based on the earlier full review plus this verified repair, not a fresh full Store acceptance. Current-head CI and the pending paired documentation still need completion; the author's 126 Store tests remain author-reported rather than my own execution.

@byteayan

Copy link
Copy Markdown
Contributor Author

@imbajin thanks for the thorough rounds on this one. Everything you raised is now addressed at 535c2d0: the ancestor and subtree cgroup limits, the multi-mount order, the upgrade step for an old application-pd.yml, and the CONTRACT_SUITES mapping. All 19 checks are green on that commit, and the paired doc PR is apache/hugegraph-doc#535, linked above with Doc - Done. Could you take a final look when you have a moment?

byteayan added a commit to byteayan/hugegraph-doc that referenced this pull request Oct 16, 2026
apache/hugegraph#3355 fills an absent or 0 rocksdb.write_buffer_size
with total_memory_size / 1000, but never below the 1 MiB that
RocksDBOptions accepts. Any budget under about 1 GB, including the
256 MiB auto-sizing floor, therefore gets 1048576. Say so in the EN and
CN rows.
byteayan added a commit to byteayan/hugegraph-doc that referenced this pull request Oct 16, 2026
The Docker guide said an unset HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE sizes
the budget from the container memory limit, but the Compose files it
documents set no limit, so each Store sizes from the whole Docker host.
Say so, and tell readers to set a limit or pin the variable when several
Stores share a host. The HStore row now says the cgroup limit applies
when one is set, otherwise the host's memory.

The HStore page also said its tables were checked at a master commit
from before apache/hugegraph#3355; note that the three RocksDB and
disruptor rows describe that PR until its merge commit replaces it.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: Automatic sizing can miss the process cgroup limit in a supported cgroup-namespace mount layout and can reserve too little for uncapped direct memory. Either condition can leave the Store with a RocksDB budget above its available container memory. Evidence: exact-head source review of RocksdbMemoryBudget, the default Store launcher and gRPC configuration; see inline comments for the two cases.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updated independent cross-review: 8.0/10, pending focused handling of the namespace mapping case noted in #3355 (comment). I independently reproduced the missing-child-limit case using synthetic proc/mountinfo/cgroup files and the exact resolver source, compiled with only Lombok logging replaced by a no-op logger. Eight new bounded Java 21 assertions passed, and all 40 earlier controls still pass. An inherited mount rooted at /.. with process membership / falls back to the parent mount root and misses the limited child. If the JVM also reports host memory, auto-sizing can overestimate the budget; an accurate JVM-reported limit masks the failure. This is a conditional gap in the new protection, not a demonstrated production OOM or a claim that it regresses the previous fixed default. Please distinguish ambiguous mapping from an unlimited hierarchy and provide narrow safe handling or an explicit-budget diagnostic with a regression fixture. Normalizing /.. alone cannot recover the hidden child path.

The direct-memory finding #3355 (comment) also identifies a real accounting assumption: an independent Java 21 probe with -Xmx2g and unset MaxDirectMemorySize reported a 2 GiB allowance, while this code reserves 1 GiB. This heuristic is explicit in the source. No buffers or Store workload were allocated, so no direct-buffer workload failure or OOM was demonstrated. Counting the default heap-sized allowance is the conservative option; imposing a new runtime direct-memory cap would need separate validation.

Evidence boundary: two independent source reviews, synthetic resolver tests and a lightweight JVM allowance probe; no live namespace, JDK 17, full Store, OOM or performance acceptance. Current review source:

}
boolean matched = false;
for (String[] mount : v2 ? v2Mounts : v1Mounts) {
if (!contains(mount[0], parts[2])) {
// A mount of another subtree says nothing about this cgroup
continue;
}
matched = true;
Path point = Paths.get(mount[1]);
String relative = rebase(parts[2], mount[0]);
limit = lowest(limit, v2 ? cgroupV2Limit(point, relative) :
cgroupV1Limit(point, relative));
}
if (!matched) {
String relative = rebase(parts[2], "/");
limit = lowest(limit, v2 ? cgroupV2Limit(defaultRoot, relative) :
cgroupV1Limit(defaultRoot.resolve("memory"), relative));
}
. Prior 8.5 assessment is superseded by this new evidence on the same revision.

CI status at publication (2026-10-17 01:36 UTC): 19 exact-head checks passed, 2 were skipped, and 0 remain running/queued. Both Codecov checks passed. CI source: https://github.com/apache/hugegraph/actions/runs/38938886440. These reported CI results do not establish coverage of the synthetic namespace-mapping case and are separate from our bounded independent controls.

In a cgroup namespace /proc/self/cgroup says "/" while a cgroup2 mount
made outside the namespace has root "/..". No mount contains the
process's cgroup, and the child cgroup that is the namespace root can't
be located from inside, so its tighter limit is invisible. The resolver
used to fall back to /sys/fs/cgroup as if the layout were simply
unlimited.

Now it reads the limit at each such mount's own root, which still
bounds the process, and reports the mapping as unmatched. When
auto-sizing, the Store logs a warning saying a tighter limit may be
missed and the budget should be set explicitly. With no mountinfo entry
at all the default layout is used as before and nothing is flagged.
With -XX:MaxDirectMemorySize unset the JVM allows direct buffers up to
the heap size, but the budget counted at most 1 GiB. The Store accepts
gRPC messages up to 1000 MB, so direct-buffer-heavy traffic could use
more than was reserved while the RocksDB caches stayed sized.

bin/start-hugegraph-store.sh now passes -XX:MaxDirectMemorySize equal to
the heap it picks, unless -j sets one (a later flag wins), and the
budget counts whatever the JVM reports: the configured cap, or the heap
when unset. The launcher's default 2 GiB heap and cap give a 16 GiB host
about 5.25 GiB. A raised heap should come with a smaller explicit cap,
or direct memory counts as a second heap and the 256 MiB floor applies;
the deployment guide says so.
byteayan added a commit to byteayan/hugegraph-doc that referenced this pull request Oct 17, 2026
apache/hugegraph#3355 now budgets the JVM's real direct-memory
allowance: an unset -XX:MaxDirectMemorySize counts as the heap, and
bin/start-hugegraph-store.sh sets the flag to the heap it picks unless
-j passes one. Drop the "at most 1 GiB" from the EN and CN rows.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updated independent cross/adversarial review of the current revision: 8.5/10; waiting for current-head CI and a narrow launcher/documentation clarification.

The unmatched-cgroup flag, visible mount-root fallback, explicit-budget warning and regression fixtures satisfy the diagnostic alternative requested in our prior review. This clears that review hold; it does not mean the hidden child limit is recovered. An accurate JVM-reported bound still protects sizing, while an overreporting JVM plus a hidden tighter child remains a disclosed limitation. No new fail-fast requirement is being introduced.

The direct-memory accounting correction is sound: count the configured allowance, or the full heap-sized JVM allowance when unset. The previous 1 GiB heuristic is gone.

One new compatibility/documentation point: the launcher injects MaxDirectMemorySize from computed XMX before USER_OPTION. A heap-only -j override therefore leaves that generated direct cap unchanged. For example, computed 2 GiB followed by -j -Xmx512m still permits 2 GiB direct memory; raising heap alone retains the smaller computed cap. This changes the previous implicit heap-sized direct allowance, and the deployment guide's statement that raising heap without an explicit cap counts a second heap does not describe this launcher path. Tiny JVM argument-precedence probes confirm the allowance behavior; no workload failure was demonstrated.

The smallest compatibility-preserving option is to remove the newly generated direct-memory flag and keep the corrected full-heap accounting. If the fixed computed cap is intentional, document it explicitly and add lowered/raised heap-only -j regression cases alongside the explicit-cap override case. This is a narrow compatibility clarification, not evidence of a demonstrated OOM or a request for a broader resolver redesign.

Evidence: two independent source reviews, bounded Java 21 synthetic cgroup/accounting checks and lightweight JVM allowance probes without buffer allocations. These are not live namespace, JDK 17, Store workload, OOM, performance or same-RC acceptance tests. Current-head CI, checked at 2026-10-17 06:47 UTC, has 5 successful checks, 2 skipped checks and no failed checks. 11 checks remain running/queued. Older-head results are not inherited.

The launcher put -XX:MaxDirectMemorySize=<computed heap> before the -j
options, so a -j that only set -Xmx kept the cap from the heap the
launcher had computed: -j -Xmx512m still allowed 2 GiB of direct memory,
and a raised heap kept the smaller cap.

Leave the flag out when -j sets -Xmx or -XX:MaxDirectMemorySize. The
JVM then defaults the cap to the -j heap, or uses the given cap, and
RocksdbMemoryBudget counts whichever applies. test-start-hugegraph-store.sh
covers no -j, a lower and a raised heap-only -j, and an explicit cap.
@byteayan

Copy link
Copy Markdown
Contributor Author

@imbajin thanks, good catch on the heap-only -j. Fixed in 119f819: the launcher now adds -XX:MaxDirectMemorySize=<computed heap> only when -j sets neither -Xmx nor -XX:MaxDirectMemorySize. With a heap-only -j the JVM defaults the cap to that heap, and with an explicit cap the cap applies; the budget counts whichever the JVM reports. test-start-hugegraph-store.sh (already run in the store CI job) gains four cases: no -j gives the computed 1024m cap, -j -Xmx512m and -j -Xmx4g leave the cap out, and -j -XX:MaxDirectMemorySize=256m ends with 256m. Against the previous launcher both heap-only cases fail with the stale 1024m cap. The deployment guide now describes this path.

byteayan added a commit to byteayan/hugegraph-doc that referenced this pull request Oct 17, 2026
apache/hugegraph#3355 now leaves -XX:MaxDirectMemorySize out when -j
sets -Xmx or the cap itself, so a heap-only -j gets the JVM default.
Update the EN and CN rows.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up independent cross/adversarial review of the updated revision: 8.5/10; the standard heap-only -j compatibility concern from our previous review is addressed. Await current-head CI.

The launcher now omits its generated direct-memory cap when -j supplies -Xmx or an explicit direct cap; the documentation matches those ordinary invocation paths. Independent execution of the exact shipped --gc-only launcher fixture passed all 15 cases, including default, lowered heap, raised heap and explicit direct-cap cases. This uses stub Java for argument capture, not a real Store launch. Additional tiny JVM 21 probes confirmed the effective heap/direct allowance behavior without allocating buffers.

A non-blocking edge remains: the detection pattern expects a literal space before -Xmx, so tab/newline-separated options or the -XX:MaxHeapSize alias can retain the computed cap. Those cases were reproduced with lightweight argument probes; no Store workload failure was demonstrated. Optional simplification is still to omit the generated flag entirely and retain the corrected heap-sized accounting, or align detection with actual option tokenization. This does not reopen the ordinary -j fix or require a broader redesign.

Namespace diagnostic handling and direct-memory accounting are unchanged from the previous assessment, with the same disclosed evidence boundaries. Current-head CI, checked at 2026-10-17 07:00 UTC, has 11 successful checks, 2 skipped checks and no failed checks. 7 checks remain running/queued. Cancelled results from the previous revision are not treated as failures or inherited passes.

The check for a heap or cap in -j matched a literal " -Xmx", so options
separated by a tab or newline, or the -XX:MaxHeapSize alias, still got
the computed direct-memory cap. Split -j the way it reaches the JVM
(unquoted, on spaces, tabs and newlines) and match each option against
-Xmx*, -XX:MaxHeapSize=* and -XX:MaxDirectMemorySize=*. Two launcher
test cases cover the tab-separated and alias forms.
@byteayan

Copy link
Copy Markdown
Contributor Author

@imbajin thanks, closed that edge too in 643db96. The launcher now splits -j the way it reaches the JVM (unquoted, so on spaces, tabs and newlines) and matches each option against -Xmx*, -XX:MaxHeapSize=* and -XX:MaxDirectMemorySize=*, rather than looking for a literal " -Xmx". test-start-hugegraph-store.sh --gc-only gains a tab-separated -Xmx4g case and a -XX:MaxHeapSize=4g case; all 17 pass, and against 119f819 exactly those two fail with the computed 1024m cap.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Independent update for the current revision: 8.5/10 for the reviewed implementation; merge remains blocked on the current CI failure below.

The last launcher delta addresses the optional tab/newline and MaxHeapSize alias cases from our previous review. The exact shipped stub-launcher suite passes 17/17, both changed scripts pass bash -n, and lightweight JVM 21 probes confirm option precedence. These are bounded independent checks, not live Store tests.

The current HStore job fails during the second init-store invocation with Invalid value for key 'usePD': '[true, true]'. Anonymous Cypher tests passed 2/2 first. Independent attribution against the actual CI merge revision shows this is a shared test-fixture interaction exposed by merged Server PR #3366, not a demonstrated regression in this PR's RocksDB budgeting.

At the actual tested merge, run-api-test.sh starts the same directory twice, but start-server.sh appends usePD on each HStore start. The start script is identical in the PR head and base, and the two-start runner comes from the base.

A bounded independent twice-invoked exact-script fixture, with service commands stubbed, reproduced duplicate usePD, schema.sync_deletion, and the HBase timeout properties. Only the usePD Java failure is observed in CI; no Java failure is claimed for the other duplicate keys. Please make these appends idempotent, for example portable delete-before-append for each active key (escaping literal dots), and add a twice-start regression. The existing write_property helper alone neither inserts absent keys nor removes duplicates. A shared fixture fix should let current-head HStore CI reach the authenticated API phase; a blind rerun does not address the deterministic duplicate configuration.

No production OOM or runtime performance failure was observed. Current CI is separate from same-RC acceptance.

Remaining exact-head checks at publication (2026-10-17 07:38 UTC): 18 passed, 2 were skipped, and only HStore failed. No checks remain running or queued. Older-head results are not inherited.

run-api-test.sh now starts the same Server directory twice: once
anonymous, then with authentication. start-server.sh appended usePD,
schema.sync_deletion and the HBase timeouts with >> on every start, so
the second HStore start read usePD as '[true, true]' and init-store
failed with "Invalid value for key 'usePD'".

Add a set_property helper that deletes every existing line for the key
(dots escaped) before appending it once, and use it for those four keys.
test-start-server.sh runs the real script twice on one fixture with
init-store and the launcher stubbed, for hstore and hbase, and checks
each key appears exactly once. It runs in the Server rocksdb job and is
mapped in CONTRACT_SUITES.
@byteayan

Copy link
Copy Markdown
Contributor Author

@imbajin thanks for tracing that. Master is merged in (a5942ff), and 9e6dc50 makes the start-server.sh writes idempotent. A set_property helper deletes every existing line for the key, dots escaped, before appending it once. It now handles usePD, schema.sync_deletion and both HBase timeouts. The new test-start-server.sh runs the real script twice on one fixture, with init-store and the launcher stubbed, for hstore and hbase, and checks each key appears exactly once. All 7 checks pass, and against master's start-server.sh 5 of them fail on the duplicated keys. The test runs in the Server rocksdb job's startup-contracts step and is mapped in CONTRACT_SUITES, and the selector tests cover the mapping.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Independent repair review of the current revision: 8.5/10 overall; the duplicate-usePD HStore failure reported in our prior review is resolved on this revision. Current-head CI has completed successfully.

The startup fixture now replaces active usePD, schema.sync_deletion and HBase timeout assignments idempotently. Independent execution of the exact new twice-start fixture passes 7/7; running it against the old start script fails 5 checks, so it detects the original problem. All 20 CI-policy tests pass, and the new directly invoked fixture is executable and wired to the actual startup-contract step. Additional bounded duplicate/colon/lookalike/comment controls pass. These local checks use stub service commands, not a live Store. The master merge boundary was inspected separately; no unrelated imported master changes are claimed as newly reviewed.

Real current-head HStore CI now passes the formerly failing path: anonymous Cypher 2/2; second init-store completes; authenticated API suite 190 tests with 0 failures/errors and 53 skips; coverage and shutdown checks succeed. The log explicitly checks out the merge of this head into the tested base. Raft and TinkerPop phases were skipped, and this is not same-RC acceptance.

Latest workflow status at 2026-10-17 08:10 UTC: 20 checks passed, 1 were skipped, and no checks failed. The workflow completed successfully; no checks remain running or queued.

@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. At 9e6dc50 an unset or 0 rocksdb.total_memory_size is sized from the cgroup or physical limit minus heap, the direct memory the JVM actually allows and a 1.5 GiB reserve, and an explicit value of at least 1 MiB still wins. The earlier threads (derived write_buffer_size floor, parent-only cgroup limits, subtree and multi-mount rebasing, the inherited namespace mount, direct-memory accounting, the upgrade step for an old mounted application-pd.yml, CONTRACT_SUITES mapping, duplicate usePD) are handled at this head, and I found nothing new to raise. Remaining limits are disclosed in the PR description and the deployment guide: a limit set only above a private cgroup namespace stays invisible, a hidden child limit behind an inherited mount only gets a WARN, a raised heap without a direct cap counts direct memory as a second heap and can drop to the 256 MiB floor, and no full Store has been run under a container limit. Evidence: read the full exact-head diff plus RaftRocksdbOptions, RocksDBOptions, AppConfig.init(), start-hugegraph-store.sh, calc_xmx, the Store Dockerfile, the entrypoint and both Helm values files; compiled RocksdbMemoryBudget.java from this head standalone (Lombok logger swapped for a print stub) on OpenJDK 17.0.20.1 and got 536870912 for 4 GiB/1 GiB/512 MiB, 2684354560 for the 8 GiB Helm cluster preset, 5637144576 for the 16 GiB default launcher, a disruptor size of 32 for 512 MiB and the 1 MiB write-buffer floor; ran test-store-docker-entrypoint.sh (9 passed) and test-start-server.sh (7 passed) locally; every check on this head passed (riscv64 skipped), including pd_store / store, which runs CoreSuiteTest with the new RocksdbMemoryBudgetTest.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: no. Summary: One portability issue remains in the startup property helper on systems using stock BSD sed. Evidence: the exact substitution returns unbalanced brackets with the local Darwin system sed; the current-head CI run has no failed checks and skips riscv64.

local value=$3
local pattern

pattern=$(printf '%s' "$key" | sed 's/[][\.*^$/]/\\&/g')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Important: On Darwin systems using the stock BSD sed, the / delimiter in this substitution is parsed before the bracket expression closes, so the command returns unbalanced brackets. With set -e, the unconditional set_property "$CONF" "schema.sync_deletion" call exits before server startup. Please escape / or use another delimiter and cover the BSD sed case. Evidence: the exact expression exits 1 on the local Darwin system; the current-head macOS Actions job passed, so this is specific to sed implementations with this parsing behavior.

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.

[Bug] Store ships rocksdb.total_memory_size of 32 GB, ignores the container memory limit, and Docker users cannot override it

3 participants