ci: switch integration tests to rippleci/xrpld image (follows xrpl.js #3270)#291
ci: switch integration tests to rippleci/xrpld image (follows xrpl.js #3270)#291
Conversation
The rippled binary was renamed to xrpld upstream, and the rippleci/rippled image stopped receiving updates; integration tests across all open PRs failed because the container exited before becoming healthy. Mirrors xrpl.js PR #3270 (XRPLF/xrpl.js#3270): - Use rippleci/xrpld:develop with the --standalone entrypoint (no more bash -c "mkdir ... && rippled -a" wrapper; the xrpld image handles this internally). - Rename .ci-config/rippled.cfg to xrpld.cfg and update paths under /var/lib and /var/log from rippled/ to xrpld/. - Mount the config to /etc/opt/xrpld/ rather than /etc/opt/ripple/. - Replace the docker --health-cmd healthcheck (which shelled out to the renamed rippled CLI) with a direct JSON-RPC poll against localhost:5005, and always dump container logs on failure. Validated on PR #292 (closed after success): Integration Test green in 2m53s.
201ccf1 to
e84f597
Compare
| @@ -43,7 +43,7 @@ small | |||
|
|
|||
There was a problem hiding this comment.
Can you update the following files as well? Thanks!
There was a problem hiding this comment.
Both files were updated in 9196506 (before this review was submitted). CONTRIBUTING.md and docker-compose.yml now use rippleci/xrpld:develop, xrpld_standalone container name, /etc/opt/xrpld/ mount, and --standalone flag.
The rippled binary was renamed to xrpld upstream, and the CI image moved from rippleci/rippled to rippleci/xrpld (see xrpl.js PR #3270). Align the local developer docs and docker-compose service with the same image, mount path (/etc/opt/xrpld/), config filename (xrpld.cfg), and simplified --standalone entrypoint that the CI workflow uses.
| if: always() | ||
| run: docker stop rippled-service | ||
| run: | | ||
| docker logs xrpld-service || true |
There was a problem hiding this comment.
If there is an error while fetching the logs, developers should be informed.
| docker logs xrpld-service || true | |
| docker logs xrpld-service && docker stop xrpld-service |
Why do we need to return a catch-all true value?
There was a problem hiding this comment.
Fixed in 0080e17. Now chains docker logs xrpld-service && docker stop xrpld-service so a failure surfaces in the job result instead of being swallowed by || true.
| ${{ env.XRPLD_DOCKER_IMAGE }} --standalone | ||
|
|
||
| - name: Wait for rippled to be healthy | ||
| - name: Wait for xrpld to accept RPC |
There was a problem hiding this comment.
Is this job required?
-
This check is not comprehensive enough: Suppose one of my transactions is slated to fail due to a misconfigured config file bug, then this check does not help me. This job only checks if we can run RPC calls against the docker image.
-
The
docker logs <docker_id>already prints the logs of the rippled server. The logs should help us find the cause of any potential failures.
IMO the job titled Wait for rippled to be healthy can be completely removed. rippled starts pretty fast, I haven't experienced any delays in the other SDK CI pipelines. This will also simplify this Github workflow file.
There was a problem hiding this comment.
Good call. Dropped the dedicated wait step in 0080e17 — xrpld starts fast enough that the cargo-test step will fail on its own if RPC isn't ready, and that failure is more informative than a generic timeout.
| - `--entrypoint bash rippleci/rippled:develop` manually overrides the entrypoint (for the latest version of rippled on the `develop` branch). | ||
| - `-c 'mkdir -p /var/lib/rippled/db/ && rippled -a'` starts `rippled` in standalone mode, where ledgers only close on demand. | ||
| - `--name xrpld_standalone` is an instance name for clarity. | ||
| - `--volume $PWD/.ci-config/:/etc/opt/xrpld/` mounts `xrpld.cfg` so the node binds on `0.0.0.0` and is reachable from the host. It must be an absolute path, so we use `$PWD` instead of `./`. |
There was a problem hiding this comment.
| - `--volume $PWD/.ci-config/:/etc/opt/xrpld/` mounts `xrpld.cfg` so the node binds on `0.0.0.0` and is reachable from the host. It must be an absolute path, so we use `$PWD` instead of `./`. | |
| - `--volume $PWD/.ci-config/:/etc/opt/xrpld/`: This command bind-mounts the host-directory (left-side path) to the docker container (right-side path). `xrpld.cfg` is located in `$PWD/.ci-config/` directory. This command is intended to be run from the root of the `xrpl-rust` project. The second-half of the directory path specifies the absolute-path inside the docker container. The `xrpld` binary searches for the configuration file inside the path: `/etc/opt/xrpld/`. |
nit: The mount-bind path of the docker container implicitly makes it accessible from the localhost. Calling it out does not provide additional information to the reader.
There was a problem hiding this comment.
Applied in 0080e17 (adapted wording slightly to keep the $PWD note about absolute paths). Thanks for the clearer phrasing.
| command: ["--standalone"] | ||
| healthcheck: | ||
| test: ["CMD", "rippled", "server_info"] | ||
| test: |
There was a problem hiding this comment.
| test: | |
| test: ["CMD", "xrpld", "server_info"] |
nit: This achieves the same effect with minimal changelog.
There was a problem hiding this comment.
Done in 0080e17 — switched to ["CMD", "xrpld", "server_info"]. Simpler and drops the wget assumption on the image.
- Drop the explicit "Wait for xrpld to accept RPC" poll loop. xrpld starts fast and the subsequent cargo-test step fails fast on its own if the RPC port is not serving; the poll only obscured the underlying error with a generic timeout message. - Stop swallowing errors in the cleanup step. Chain docker logs and docker stop with && so a failure surfaces in the job result instead of silently returning success via || true. - Simplify docker-compose healthcheck to rely on the xrpld binary's own server_info subcommand (CMD form, no shell, no wget dep). - Clarify the --volume explanation in CONTRIBUTING to describe the bind-mount semantics without implying localhost reachability.
docker run uses --rm, so by the time the cleanup step executes either the test step has finished and the container has already exited and been removed, or xrpld crashed mid-test and was auto-removed. In both cases docker logs returns exit 1 with "No such container". The previous `docker logs && docker stop` chain would then short-circuit and report the cleanup step as the failing step, masking the real test-step failure in the job summary. Split logs from stop, warn (don't fail) when the container is absent for logs, and tolerate "already stopped" on docker stop. Real cargo test failures continue to surface on the Run integration tests step as before.
Summary
The
rippledbinary was renamed toxrpldupstream, and therippleci/rippledimage stopped receiving updates. Our integration tests across every open PR started failing because the publisheddevelopimage exited before becoming healthy (Connection refusedonlocalhost:5005, 0 passed / 41 failed).This PR mirrors the upstream fix in xrpl.js: XRPLF/xrpl.js#3270. Switching to
rippleci/xrpld:developis the actual root-cause fix rather than pinning an old digest of the deprecated image.Changes
.github/workflows/integration_test.yml:RIPPLED_DOCKER_IMAGE->XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop.docker runsimplified to${IMAGE} --standalone(thexrpldimage handlesmkdir+ launch internally; no morebash -c "mkdir -p /var/lib/rippled/db/ && rippled -a"wrapper)./etc/opt/ripple/to/etc/opt/xrpld/.rippled-service->xrpld-service.--health-cmd(which shelled out to the renamedrippledCLI and always failed) in favour of a direct JSON-RPC poll againsthttp://localhost:5005/..ci-config/rippled.cfg->.ci-config/xrpld.cfg:path=/var/lib/rippled/db/nudb->path=/var/lib/xrpld/db/nudb.[database_path] /var/lib/rippled/db->/var/lib/xrpld/db.[debug_logfile] /var/log/rippled/debug.log->/var/log/xrpld/debug.log.Verification
Validated on throwaway PR #292 (now closed): Integration Test green in 2m53s on this exact workflow. Unit tests, Build & Lint, Quality Check also pass.
Related follow-up
The 7 in-flight PRs (#130, #131, #151, #153, #156, #157, #158) currently carry a stopgap commit pinning
rippleci/rippled:developto a specific digest. After this PR merges tomain, those branches should:mainto pick up the xrpld switch, orTest plan
Credit
Approach lifted from @ckeshava's xrpl.js#3270.