HUGEGRAPH-3099: Add RISC-V end-to-end smoke test script#3104
Conversation
Add a self-contained smoke test that builds the HugeGraph Server distribution on the host (x86-64/arm64), then runs it inside an emulated linux/riscv64 container via Docker QEMU to verify: - Architecture is riscv64 - RocksDB JNI open/put/get/close (libatomic linkage) - HugeGraph init + health check (GET /graphs -> 200) - REST schema CRUD (propertykey, vertexlabel, edgelabel, vertices, edge) - Gremlin query (g.V().count() == 2 on port 8182) - Restart + data persistence across container restart Output matches the expected format from the issue.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The smoke script has cross-platform blockers and several false-pass and resource-safety issues that need to be resolved before it can serve as a correctness gate. Evidence: static review of exact head df7eb35; bash -n passes, while targeted portability and behavior checks confirm the issues below.
| # Force the RocksDB backend for the smoke run. | ||
| CONF="$SERVER_DIR/conf/graphs/hugegraph.properties" | ||
| grep -qE '^[[:space:]]*backend[[:space:]]*=' "$CONF" \ | ||
| && sed -i 's/^[[:space:]]*backend[[:space:]]*=.*/backend=rocksdb/' "$CONF" \ |
There was a problem hiding this comment.
sed -i syntax is GNU-specific, but the script advertises Docker Desktop as a supported path. BSD sed on macOS requires an explicit backup suffix, so this command (and the same form on lines 224 and 241) exits under set -e before the image is built. Please use a portable edit helper or a temporary-file replacement and cover the macOS/Docker Desktop path.
There was a problem hiding this comment.
Fixed. Added a portable sedi() helper that uses temp-file-rename instead of sed -i. Covers the macOS/Docker Desktop path. The two sed -i calls inside the heredoc were left unchanged since they run inside the Ubuntu container where GNU sed is guaranteed.
|
|
||
| # Extract straight into the build context's server/ dir — no second copy | ||
| # (the extracted tree is ~1GB; copying it twice doubled the temp footprint). | ||
| BUILD_CTX=$(mktemp -d "hugegraph-riscv64-ctx.XXXXXX") |
There was a problem hiding this comment.
mktemp template is created in the caller's current directory rather than under the exported TMPDIR; the RocksDB temp directory on line 340 has the same issue. That defeats the stated safeguard for the roughly 1 GB extraction and can fill an unintended filesystem. Please include $TMPDIR in both templates (or use a portable explicit-directory option).
There was a problem hiding this comment.
Fixed. Both BUILD_CTX and ROCKS_BUILD now use ${TMPDIR:-/tmp}/ prefix, honoring the exported TMPDIR set at line 52. The ~1GB extract and RocksDB test class land on the same disk-backed filesystem.
| export TMPDIR | ||
|
|
||
| # Unique, traceable names so cleanup removes only our own resources. | ||
| STAMP=$(date +%Y%m%d%H%M%S) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed. Changed to date +%Y%m%d%H%M%S-$(head -c 4 /dev/urandom | od -An -tx1 | tr -d ' \n') — timestamp for traceability, 4 random hex bytes for collision resistance. Two runs in the same second now get unique resource names.
| # it (otherwise the build/run fails with 'exec format error'). binfmt handlers | ||
| # do NOT survive reboots, so a fresh checkout needs this — makes the smoke test | ||
| # self-contained on any x86 host. Pinned tag: :latest 403s on Docker Hub. | ||
| if ! ls /proc/sys/fs/binfmt_misc/qemu-riscv64 >/dev/null 2>&1; then |
There was a problem hiding this comment.
/proc, not the Docker engine that will execute the RISC-V container. Docker Desktop keeps binfmt inside its Linux VM, while a remote Docker context can have the opposite state, so this can unnecessarily run a privileged installer or skip a required one. Please probe linux/riscv64 execution through the selected Docker daemon and only install or instruct the user when that probe fails.
There was a problem hiding this comment.
Fixed. Replaced ls /proc/sys/fs/binfmt_misc/qemu-riscv64 with docker run --rm --platform linux/riscv64 ubuntu:24.04 uname -m. Probes the actual Docker daemon's capability instead of the host kernel.
| return 1 | ||
| fi | ||
| local code | ||
| code=$($DOCKER exec "$CONTAINER" \ |
There was a problem hiding this comment.
curl: elapsed advances only after the command returns. The POST, GET, and Gremlin requests likewise omit the 300-second limit mentioned later in the script, so a stalled accepted connection can hang until an outer CI timeout. Please add explicit connect/max timeouts to every request and derive readiness expiry from wall-clock time.
There was a problem hiding this comment.
Fixed. Added --connect-timeout 10 --max-time {60|300} to all 5 container curl calls. wait_health now uses $SECONDS for wall-clock tracking instead of a manual counter that didn't advance during curl execution.
| | tee "$ROCKS_BUILD/rocks-out.log" | ||
| JAVA_RC=${PIPESTATUS[0]} | ||
| set -e | ||
| grep -q "ROCKS_OK" "$ROCKS_BUILD/rocks-out.log" || { |
There was a problem hiding this comment.
JAVA_RC is captured but never required to be zero, so a JVM that prints ROCKS_OK and then fails during shutdown still reaches the PASS path. This is especially relevant for a native JNI correctness gate under QEMU. Please require both JAVA_RC == 0 and an exact success marker before reporting RocksDB success.
There was a problem hiding this comment.
Fixed. Added explicit [[ "$JAVA_RC" == "0" ]] check after the ROCKS_OK grep. A JVM that prints the success marker but fails during shutdown now correctly reports failure.
| $DOCKER restart "$CONTAINER" >/dev/null | ||
| wait_health || { fail "server not healthy after restart"; exit 1; } | ||
| container_alive || { fail "container died after restart health check"; exit 1; } | ||
| rest_get "http://localhost:8080/graphs/hugegraph/graph/vertices" |
There was a problem hiding this comment.
knows edge, so partial data loss can still produce a persistence PASS. Please assert Alice and Bob by their saved IDs/names and verify the exact edge after restart.
There was a problem hiding this comment.
Fixed. Now asserts both Alice and Bob by name using jq queries matched against the actual HugeVertexSerializer output format ({"properties":{"name":"alice"}}). Also verifies the knows edge exists with correct labels.
| pass "architecture: riscv64" | ||
| pass "RocksDB: open / put / get / close" | ||
| pass "HugeGraph: init / health / REST CRUD / Gremlin / restart / persistence" | ||
| pass "cleanup: container, volume, and image removed" |
There was a problem hiding this comment.
KEEP=1, and every Docker deletion failure is currently suppressed with || true; even KEEP=0 is treated as enabled because the check only tests non-emptiness. Please execute and verify normal cleanup before printing PASS, report KEEP as retained/SKIP, and surface any residual resources.
There was a problem hiding this comment.
Fixed. Moved pass into _cleanup() after teardown completes. Added ok flag to track Docker removal success. Changed KEEP check from -n (non-empty) to == "1" so KEEP=0 no longer skips cleanup.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3104 +/- ##
============================================
- Coverage 37.43% 36.24% -1.20%
Complexity 520 520
============================================
Files 808 808
Lines 69581 69581
Branches 9165 9165
============================================
- Hits 26048 25219 -829
- Misses 40750 41632 +882
+ Partials 2783 2730 -53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
correctness
- Replace GNU sed -i with portable sedi() helper (macOS compat)
- Use TMPDIR for mktemp to avoid filling RAM tmpfs
- Collision-resistant run ID (timestamp + random hex)
- Probe binfmt through Docker daemon, not host /proc
- Add curl --connect-timeout/--max-time to all requests
- Enforce JAVA_RC == 0 after RocksDB test
- Assert specific vertex names and edge in persistence check
- Fix cleanup PASS to run after teardown, track success
- Fix KEEP=0 treated as truthy
- Remove eval from safe_exec, use array splitting
|
Also, note that #3102 has already completed the same work in the native RSIC environment. Consider merging and normalizing it? |
could you explain on how to do that, I'm new to Open Source |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The smoke helper still has host-security and false-pass paths that should be fixed before relying on it. Evidence: static review of 1f76175; bash -n passed, and an EXIT-trap reproduction returned 0 after cleanup failure.
| if [[ "$ok" == "1" ]]; then | ||
| pass "cleanup: container, volume, and image removed" | ||
| else | ||
| fail "cleanup: some resources may remain (container=$CONTAINER vol=$VOL image=$TAG)" |
There was a problem hiding this comment.
ok=0; fail is an echo that returns successfully, and _exit_handler does not propagate _cleanup's status. An equivalent EXIT-trap reproduction prints the cleanup failure but still exits 0, so callers can record a passing disposable smoke run while resources remain. Please make _cleanup return nonzero and have _exit_handler convert a successful main run into failure when cleanup fails, while preserving an existing main failure code.
There was a problem hiding this comment.
Fixed. _cleanup now sets _CLEANUP_OK=1 on any Docker removal failure. _exit_handler checks both the main script's exit code and _CLEANUP_OK, exiting nonzero if either failed. Also added INT TERM to the trap so Ctrl+C during cleanup doesn't leave orphans.
| if ! $DOCKER run --rm --platform "$PLATFORM" "$BASE_IMAGE" uname -m 2>/dev/null \ | ||
| | grep -q riscv64; then | ||
| echo "==> Registering QEMU riscv64 emulation (binfmt)" | ||
| $DOCKER run --privileged --rm tonistiigi/binfmt:qemu-v8.1.5 --install riscv64 >/dev/null |
There was a problem hiding this comment.
--privileged, which gives that image host-level access to alter the Docker daemon's binfmt/kernel state. It also happens before required host tools such as the conditional mvn and the always-used Java compiler are validated, so a doomed run can make the persistent privileged change first. Please preflight all applicable host tools before this point and pin the privileged image to a verified immutable digest (preferably with signature verification).
There was a problem hiding this comment.
Fixed. Added host tool preflight (mvn, javac, docker buildx) before the binfmt install — a doomed run now fails early before any privileged operation. Added TODO comments for pinning both ubuntu:24.04 and tonistiigi/binfmtto SHA256 digests; I don't have the exact digests yet but the structure is ready for them.
| # 3. Start the server in the background; trap handles cleanup. | ||
| echo "==> Starting server container on ${PLATFORM}" | ||
| $DOCKER run -d --name "$CONTAINER" --platform "$PLATFORM" \ | ||
| -p 8080 --memory=4g -v "$VOL:/server" "$TAG" |
There was a problem hiding this comment.
-p 8080 publishes the unauthenticated test REST service on a random host port bound to all host interfaces for up to the one-hour wait, while every probe in this script uses docker exec and does not need a host mapping. Please remove this publish flag; if host access is intentionally required, bind it explicitly to 127.0.0.1.
There was a problem hiding this comment.
Fixed. Removed -p 8080 entirely. All probes use docker exec to reach localhost:8080 inside the container — no host port mapping needed.
| --connect-timeout 10 --max-time 300 \ | ||
| -w $'\n%{http_code}' \ | ||
| -H 'Accept: application/json' "$@") | ||
| REST_CODE=${rsp##*$'\n'} |
There was a problem hiding this comment.
rest_get parses REST_CODE, but no caller checks it; the CRUD and restart gates only assert JSON body shape. That allows a non-2xx response with a compatible body to pass even though the stated REST contract failed. Please make this helper require a successful Docker/curl exit and a 2xx status before any JSON assertion, and include the status and response body in failures.
There was a problem hiding this comment.
Fixed. rest_get now asserts REST_CODE == 2* and exits with the status code and truncated body on failure. This covers the CRUD and restart gates that previously only checked JSON shape.
| *.ai-prompt.md | ||
| WARP.md | ||
| .mcp.json | ||
| hugegraph-riscv64-ctx.*/ |
There was a problem hiding this comment.
🧹 These two unanchored patterns can hide matching directories anywhere in the repository, and they are redundant for the default path because both temporary directories are created beneath the already-ignored .riscv64-smoke-tmp/. Please keep only a root-anchored /.riscv64-smoke-tmp/ entry unless another concrete location must be ignored.
There was a problem hiding this comment.
Fixed. Removed hugegraph-riscv64-ctx.*/ and rocks-smoke.*/. Kept only /.riscv64-smoke-tmp/ (root-anchored). Both temp directories are created under that path, so the unanchored patterns were redundant.
we could start by understanding the content of PR itself? Of course, it would also be good if there was a virtual machine that could actually run the QEMU→RISCV environment. |
…P checks - Remove -p 8080 to avoid exposing unauthenticated REST on host network - Make _cleanup propagate failure via _CLEANUP_OK flag - Add INT TERM to trap for Ctrl+C cleanup safety - Check HTTP status in rest_get and Gremlin query - Add host tool preflight (mvn, javac) before privileged binfmt install - Add --cap-drop ALL --security-opt no-new-privileges to container - Validate vertex IDs before JSON interpolation - Fix .gitignore: remove unanchored patterns, keep /.riscv64-smoke-tmp/ - Remove dead _EXIT_CODE variable - Track BUILD_CTX/ROCKS_BUILD cleanup failures in ok flag - Propagate actual exit code from safe_exec - Remove redundant container_alive after wait_health - Dump diagnostics before KEEP=1 skip
ok this sounds good, which option should we take
and about the virtual machine, my smoke script is self contained, it detects if the Docker supports riscv64, installs QEMU binfmt if needed, builds a riscv64 image, and runs the full test under emulation. No separate VM required — Docker + binfmt handles everything. |
Summary
Adds a self-contained
run-riscv64-smoke.shscript that builds the HugeGraph Server distribution on the host (x86-64/arm64), then runs it inside an emulatedlinux/riscv64container via Docker QEMU to verify the RISC-V port works end-to-end.What it tests
The script covers every item in HUGEGRAPH-3099:
LD_PRELOAD=libatomic.so.1for RISC-Vinit-store.sh, pollsGET /graphsuntil 200GET /verticesandGET /edgesreturn datag.V().hasLabel("person").count() == 2via port 8182 with graph aliases bindingExpected output
Approach
Single self-contained script (no CI pipeline changes). Designed for contributors to run locally:
Key design decisions
docker-entrypoint.shsoinit-store.shruns before the server starts (skipping it caused silent failures)--memory=4gto prevent host OOM under QEMU emulationcurl --compressed— the server returns gzip by default regardless ofAcceptheaderCoexistence with PR #3102
This PR takes a different approach than #3102 — a single standalone script vs. a multi-file CI pipeline. The two are complementary: the script here is for local contributor validation, while #3102 integrates into CI. Happy to coordinate if a combined approach is preferred.