Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3401 +/- ##
============================================
+ Coverage 55.93% 57.28% +1.35%
- Complexity 2277 2835 +558
============================================
Files 774 776 +2
Lines 68534 68921 +387
Branches 9333 9802 +469
============================================
+ Hits 38334 39483 +1149
+ Misses 26634 25815 -819
- Partials 3566 3623 +57
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: A retained PD-managed configuration can override the selected Topling provider and prevent Store startup. The chart digest instructions and contract-test selection also need correction. Evidence: Exact-revision source tracing, a Spring property-source precedence reproduction and a CI selector reproduction; four image contract groups passed, but these checks do not exercise PD configuration loading.
| # selected runtime (including the Topling image's default) configures both sides. | ||
| ROCKSDB_JSON="" | ||
| if [[ -n "${TOPLINGDB_ROCKSDB_PROVIDER:-}" ]]; then | ||
| ROCKSDB_JSON="\"rocksdb\": { \"provider\": \"${ROCKSDB_PROVIDER}\" }," |
There was a problem hiding this comment.
rocksdb.provider=topling, but when the local file is absent or S:Timestamp is at least its mtime, PdConfigureListener.updatePdConfig() inserts the persisted PD configuration with sources.addFirst(). A retained application-pd.yml can contain the shipped provider: rocksdb, overriding this JSON. StoreNodeApplication then runs RocksDBRuntime.verify() after the listener and throws because the loaded JNI is Topling. A Spring 5.3.27 property-source probe confirms topling becomes rocksdb after adding the PD source. Keep explicit image/provider selection above that source and add a regression covering retained PD configuration followed by runtime verification; the current entrypoint test only checks generated JSON.
|
|
||
| A component tag such as `1.8.0` becomes `1.8.0-topling`; tags already ending in | ||
| `-topling` are kept. Custom repositories and `global.imageRegistry` work as usual. | ||
| An explicit `image.digest` remains exact and takes priority over the derived tag; |
There was a problem hiding this comment.
🧹 Minor: Document the actual digest restriction. These instructions say to supply the matching Topling image.digest, but templates/runtime-variant-validation.yaml unconditionally fails on any nonempty PD, Store or Server digest when global.runtimeVariant=topling. Following this guidance therefore prevents Helm rendering. Explain that these digests must currently be cleared in favor of matching version tags, and align docs/operations.md, or implement an explicit supported model for variant-specific digests.
| run: | | ||
| bash $TRAVIS_DIR/test-topling-selection.sh | ||
| bash hugegraph-store/hg-store-dist/src/assembly/travis/test-store-preloads.sh | ||
| bash docker/test-topling-images.sh |
There was a problem hiding this comment.
docker/test-topling-images.sh returns {'docker'}; no pd_store/PD job runs. A later script-only repair can pass CI without executing the changed tests. Add a PD entry for this path in CONTRACT_SUITES, or execute the test in the selected Docker job, and cover the script-only selection case. The workflow edit selects PD for this revision, so this is a future-change coverage gap rather than a claim that the current run skipped the test.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The variant selection, checksum-pinned JNI preparation, Helm tag and node-selector derivation and the new tests read correctly, but narrowing the standalone Server volume to rocksdb-data drops API-created graph definitions on the first image change, graphs created at runtime in the Topling standalone image fail the provider check, and a mounted graph config written by the Topling image blocks the documented return to the standard image. The existing review comment on helm/hugegraph/README.md line 135 about the image.digest guidance still applies at this head; it is not repeated here. Evidence: read the full diff against merge base 75ea5c7 plus the Server entrypoint, InitStore, GraphManager.createGraph/createGraphLocal, StandardHugeGraph.create, ConfigUtil.writeToFile, RocksDBOptions.PROVIDER, RocksDBStdSessions.openRocksDB, RocksDBRuntime.verify, preload-topling.sh, prepare-topling.sh, the Helm resolveValues helper and the Compose files. At this head helm unittest helm/hugegraph passes (40 suites, 327 tests) and bash docker/test-topling-images.sh passes; helm template --set global.runtimeVariant=topling --set store.image.digest=... fails as the existing README comment says. On Compose 5.1.4, replacing an image that declares VOLUME /app with one that declares VOLUME /app/data keeps the named /app/data volume but drops files under /app/conf. The only failing check, dependency-check, reports a spring-webmvc 5.3.27 advisory in hg-pd-service, which this PR does not touch.
| EXPOSE 8080 | ||
| VOLUME /hugegraph-server | ||
| # The default data and WAL share this directory. Keep lib/bin/conf image-owned. | ||
| VOLUME /hugegraph-server/rocksdb-data |
There was a problem hiding this comment.
Important: Graphs created through the REST API on the standalone image are lost on the first image change after this PR. With graphs.enable_dynamic_create_drop=true (the default), GraphManager.createGraph() in non-PD mode calls StandardHugeGraph.create(), which writes ./conf/graphs/<name>.properties through ConfigUtil.writeToFile(). That file is outside /hugegraph-server/rocksdb-data, and the supplied docker/docker-compose.yml mounts only that path. Before this change the image's VOLUME /hugegraph-server kept conf/ across a Compose recreate. Compose carries an old anonymous volume over only when the new image still declares that path, so after docker compose up with this image the definition is gone, the graph is no longer loaded, and its RocksDB files stay orphaned in the data volume. I reproduced the Compose side with Compose 5.1.4: replacing an image that declares VOLUME /app with one that declares VOLUME /app/data, with a named volume on /app/data, keeps /app/data but drops a file written under /app/conf. This hits the documented standard-to-Topling switch and a plain standard upgrade alike, and the new README text tells users of the supplied Compose files that their data is already mounted. Please give API-created graph definitions a persisted home (for example, point graphs at a directory under the data volume and seed the shipped hugegraph.properties there when it is missing), or at least state in the upgrade notes that conf/graphs/*.properties must be copied out of the old anonymous volume before the switch.
| for graph_conf in ./conf/graphs/*.properties; do | ||
| [[ -f "${graph_conf}" ]] || continue | ||
| if [[ "$(get_prop_encoded backend "${graph_conf}")" == rocksdb ]]; then | ||
| set_prop "rocksdb.provider" "${ROCKSDB_PROVIDER}" "${graph_conf}" |
There was a problem hiding this comment.
Important: In the Topling standalone image, creating a graph through the REST API fails unless the request sets rocksdb.provider=topling by hand. The image exports TOPLINGDB_ROCKSDB_PROVIDER=topling, so the launcher loads the Topling JNI classes, but this loop rewrites only the .properties files present at container start. A graph created later through POST /graphspaces/DEFAULT/graphs/{name} takes its config only from the request (GraphManager.createGraph() to createGraphLocal()), RocksDBOptions.PROVIDER defaults to rocksdb, and RocksDBStdSessions.openRocksDB() then calls RocksDBRuntime.verify("rocksdb"), which throws because org.rocksdb.SidePluginRepo comes from the same jar as org.rocksdb.RocksDB. That is the same mismatch this loop exists to prevent for the startup graphs, and the docs tell users the image sets the business provider for them. Please make runtime-created RocksDB graphs follow the selected provider (for example, have createGraphLocal() fill in rocksdb.provider from TOPLINGDB_ROCKSDB_PROVIDER when backend=rocksdb and the key is absent), and add a test that creates a graph with that variable set.
| # Apply the image's selected JNI provider to every embedded graph in the normal | ||
| # container graphs directory, including mounted additional graph definitions. | ||
| # Distributed graphs keep backend=hstore and do not select a local provider. | ||
| if [[ -n "${TOPLINGDB_ROCKSDB_PROVIDER:-}" ]]; then |
There was a problem hiding this comment.
Important: The documented return to the standard image fails for any graph config that is mounted into the container. With TOPLINGDB_ROCKSDB_PROVIDER=topling this loop writes rocksdb.provider=topling into every local graph file, mounted ones included, and the standard image leaves that value alone because the variable is unset there (the new case in docker-entrypoint-test.sh asserts that analytics.properties still says topling). The standard launcher loads stock RocksDB, so RocksDBRuntime.verify("topling") in RocksDBStdSessions.openRocksDB() stops the Server at startup. docs/storage-lifecycle.md says the unsuffixed image configures the launcher and provider together, and docker/README.md describes the return as the same stop/up sequence; both hold only while conf/graphs is image-owned. Please either have the standard image reset rocksdb.provider to rocksdb on the local graphs it rewrites, or document that a mounted graph config must be set back to rocksdb.provider=rocksdb before the return, and cover that case in the entrypoint test.
…erver334-comments-20261025
- Keep explicit image provider above retained PD configuration. - Default runtime-created local RocksDB graphs to that provider. - Route the image contract to PD CI and cover its selector. - Clarify Helm digests and manual graph-config upgrade safeguards.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The standard standalone image now rewrites rocksdb.provider in every backend=rocksdb graph file on each start, so a read-only graph config mount that works on the base image now stops the container at startup. The earlier provider-precedence, digest, CI selector and graph-definition threads look addressed at this head.
| for graph_conf in ./conf/graphs/*.properties; do | ||
| [[ -f "${graph_conf}" ]] || continue | ||
| if [[ "$(get_prop_encoded backend "${graph_conf}")" == rocksdb ]]; then | ||
| set_prop "rocksdb.provider" "${ROCKSDB_PROVIDER}" "${graph_conf}" |
There was a problem hiding this comment.
Important: The standard image sets TOPLINGDB_ROCKSDB_PROVIDER=rocksdb, so this loop writes every backend=rocksdb graph file on each start, and under set -e a read-only conf/graphs mount (for example a Kubernetes ConfigMap) stops a container that starts on the base image. Call set_prop only when the file's effective provider (absent means rocksdb) differs from ${ROCKSDB_PROVIDER}.
Leave matching provider files untouched, including the implicit RocksDB default. Report required provider updates clearly when graph configuration cannot be written. Cover read-only directory and file mounts in Docker CI and document switch constraints.
Retain data-only volumes, provider selection and graph seeding alongside numeric non-root runtime users. Finish build-time Topling preparation before selecting the runtime user, preserving access to build secrets and payload ownership. Keep both image-contract and non-root ownership checks.
Keep native capability and license failures visible before changing payload ownership. Exercise actual Docker RUN chains with preparer and ownership failures injected. Retain the numeric non-root user selected by the upstream image baseline.
Set explicit fixture read and directory traversal permissions for the image UID. Keep read-only bind enforcement while avoiding restrictive runner umask failures. Check fixture readability and cleanup before Docker startup under umask 077.
Give private non-secret test fixtures baseline access for the image UID. Ensure write rejection comes from the read-only bind rather than host inode permissions. Verify baseline read/write/traversal access under a restrictive runner umask.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The standalone provider synchronization skips valid CRLF graph configurations, so switching an explicitly configured RocksDB graph to the Topling image can fail at startup. Evidence: Ran the actual entrypoint with a writable CRLF fixture; it retained rocksdb.provider=rocksdb. Commons Configuration 2.10.1 reads that fixture as backend=rocksdb and provider=rocksdb; source tracing confirms the explicit provider bypasses the image fallback and reaches the runtime mismatch check.
| if [[ -n "${TOPLINGDB_ROCKSDB_PROVIDER:-}" ]]; then | ||
| for graph_conf in ./conf/graphs/*.properties; do | ||
| [[ -f "${graph_conf}" ]] || continue | ||
| if [[ "$(get_prop_encoded backend "${graph_conf}")" == rocksdb ]]; then |
There was a problem hiding this comment.
backend=rocksdb rocksdb.provider=rocksdb , get_prop_encoded retains the carriage return, so this comparison skips the file. Running the actual entrypoint with TOPLINGDB_ROCKSDB_PROVIDER=topling leaves rocksdb.provider=rocksdb unchanged, while Commons Configuration 2.10.1 reads both values normally. RocksDBStoreProvider.applyImageProvider() preserves that explicit provider, and RocksDBRuntime.verify() then rejects it against the Topling JNI. This breaks the documented image switch for a valid existing configuration. Trim the backend value's trailing whitespace/CR, as done for current_provider below, and add a CRLF regression with an explicit previous provider.
Before → After
Before: Deployment options do not expose Topling consistently, and mounting the entire installation directory can hide replacement image binaries.
After: Offer a checksum-pinned community JNI runtime in Docker, Compose, and Helm. Persist data and necessary state separately from binaries; document compatible-version runtime changes that retain storage and cluster identity.
Scope and validation
Linux amd64 with a Haswell-compatible CPU is required. Runtime replacement and live Helm upgrades are not qualified yet; a current review reports that PD-provided configuration can override the selected provider, and a known HStore cleanup error remains unresolved. This is not an unrestricted data migration or a performance claim.
Source proposal: 🔗 hugegraph/hugegraph#334.