feat(redis): add topology test stacks and configuration validation for redis/valkey

[nvanlaerebeke]

Summary

This is a supporting PR for Euro-Office/server#46.

It updates the DocumentServer to support and validate the native node-redis Sentinel configuration introduced by the server change and removal of the ioredis npm package.

Changes

• Added standalone Redis, Redis Sentinel, and Redis Cluster Compose topologies + support for Valkey.
• Added Makefile commands to start, stop, and purge topology test stacks.
• Added Redis/Valkey setup and verification documentation.
• Added checks for effective DocumentServer Redis configuration.
• Updated AGENTS.md with the new development workflow.

Validation

• I've run each topology against the feat: add Redis-backed editor data storage for high availability server#46 pull request and validated they work

Relationship to the server change

This PR provides the workspace-level orchestration, test environments, and documentation needed to run the Redis Sentinel configuration from Euro-Office/server#46.

Helm chart update

The Helm chart still used Redis configuration from the previous implementation, including the obsolete Sentinel connector settings.
These settings are incompatible with the native Redis configuration introduced in [Euro-Office/server#46].

To align the chart with the new implementation, I opened [Euro-Office/Kubernetes-Docs#11]

Dependency on server [Euro-Office/server#46]

As this PR depends on [Euro-Office/server#46], which provides the editorDataRedis backend. Until that PR is merged and a new latest-dev image is published, the Redis topology stacks must be tested with a local image built from a checkout containing the server #46 changes:

git -C server fetch https://github.com/Euro-Office/server.git pull/46/head:server-pr-46
git -C server checkout --detach server-pr-46

[juliusknorr]

Thanks, that seems like a pretty sane change in addition to the other pull request, a few minor comments left inside.

[nvanlaerebeke]

I addressed the issues, let me know if you need anything else.

Thanks for the swift review.

[MonaAghili]

I reviewed this together with server#46 (at 0c99287). Moving from iooptions to a native optionsSentinel block is what #46 needs. A few problems that haven't come up yet:

1. Node lists are now split on commas only, which breaks space-separated lists such as the Helm chart's.
IFS=',' read -ra replaced declare -a X=($VAR) for REDIS_SENTINEL_NODES and REDIS_CLUSTER_NODES (docker-entrypoint.sh#L45-L77), but Kubernetes-Docs joins both lists with spaces (join " "). Running the two parsing blocks with the chart's format:

• REDIS_SENTINEL_NODES="s1:26379 s2:26379 s3:26380" renders one root node, { "host": "s1", "port": 26380 }: the first host with the last port.
• REDIS_CLUSTER_NODES="n1:7000 n2:7001" renders rootNodes: [{ "url": "redis://n1:7000,n2:7001" }], which isn't a valid URL (ERR_INVALID_URL). The cluster never connects, and arm64 version #46 only calls listen() after a successful connect.

IFS=', ' read -ra accepts spaces, commas and ", " (I checked it with both blocks), so existing deployments keep working.

2. The test stacks never run the file this PR changes. Following up on @juliusknorr's note about the standalone entrypoint: after 0c99287 all three stacks use the latest-dev image, which runs the standalone entrypoint, and inject optionsSentinel / optionsCluster directly through NODE_CONFIG (docker-compose.redis-sentinel.yml#L10). The orchestrated entrypoint's new output is never exercised, which is how 1 slipped through. A stack or CI step that runs the orchestrated entrypoint with REDIS_SENTINEL_NODES / REDIS_CLUSTER_NODES would cover it.

3. The default make redis-up can't load the Redis backend. latest-dev is built from main, whose server submodule (f5142e9) has no editorDataRedis.js, and this branch still pins the older abfa294, which doesn't have it either. The overlays select editorDataStorage: editorDataRedis (docker-compose.redis-standalone.yml#L10), so DocService fails with MODULE_NOT_FOUND. The README says the default commands validate the topology, and the local-build instructions also need the server submodule pointed at #46.

4. There's no way to select the Redis backend on the orchestrated image. Nothing maps an environment variable to editorDataStorage / editorStatStorage (#363 adds EDITOR_DATA_STORAGE), so enabling it still needs a hand-written config override.

5. Minor: the "optionsSentinel: CONFIGURED" check only greps NODE_CONFIG (README.md#L336). A /healthcheck or [editorDataRedis] log check would show DocService actually connected.

As @moodyjmz noted on server#46, #363 (the companion to server#44) emits the iooptions shape for the same entrypoint lines, so only one of the two can land, and it has to match whichever server PR merges.

[nvanlaerebeke]

Thanks @MonaAghili for the review, I did forget about the helm chart, even though I'm using it myself on the 9.3.4 I've got deployed myself.

I addressed the points as follows:

1. Space-separated node lists

   Fixed in 1281087.

   The orchestrated entrypoint now accepts both comma and space-separated lists for both REDIS_SENTINEL_NODES and REDIS_CLUSTER_NODES.

2. Orchestrated entrypoint was not covered

   Addressed in cfd63e2.

   I added a dedicated GitHub Actions workflow and entrypoint test that runs the orchestrated entrypoint with space-separated Sentinel and Cluster node lists and verifies the generated configuration. The topology Compose stacks still use the standalone entrypoint intentionally, so both paths are now covered.

3. Redis backend unavailable with the default published image

   Addressed in 696269d.

   I had the #46 branch checked out locally so I could build the latest-dev manually and tested like that.

   Did not know I could update the README.md to specify PR's etc so I've added that, can be removed once the server PR is merged.

   Both the README and this PR description now document that the published latest-dev image does not contain the editorDataRedis until server #46 is merged and a new image is published. It also documents how to temporarily check out server PR #46 in the server submodule when building a local validation image.

   The topology overlays continue to select editorDataRedis through NODE_CONFIG, so they can be validated against a local image containing the server #46 backend.

4. No environment-variable selection on the orchestrated image

   Addressed in 10798ff.

   The orchestrated entrypoint now supports:

EDITOR_DATA_STORAGE=editorDataRedis
EDITOR_STAT_STORAGE=editorDataRedis

These values end up as for the local.json:

services.CoAuthoring.server.editorDataStorage
services.CoAuthoring.server.editorStatStorage

The values are validated before being assigned into NODE_CONFIG, and leaving them unset preserves the packaged editorDataMemory default.

This was adapted for server #46's native optionsSentinel/optionsCluster configuration rather than copying #363's legacy iooptions shape.

5. Configuration-only validation

   Addressed in 696269d.

   The README verification steps now include a DocService /healthcheck check in addition to inspecting NODE_CONFIG. That confirms that DocService starts successfully with the redis-backed configuration, rather than only confirming that the environment variable was present.

   The native Redis Sentinel configuration required by server #46 was introduced in 937b9ea, and the topology overlays were aligned with it in 0c99287.

[MonaAghili]

I re-reviewed against the PR head (10798ff) and the server#46 head (9050fa9), and ran the orchestrated entrypoint and the Sentinel and Cluster stacks locally. The common paths work:

• The overlays target the standalone entrypoint that latest-dev uses, with REDIS_SERVER_PASS / REDIS_SERVER_DB and NODE_CONFIG reaching docservice through supervisord.
• The orchestrated entrypoint produces valid JSON for space-, comma- and ", "-separated node lists.

Issues below, most important first.

1. Helm Sentinel deployments can't reach the new code path

The chart only emits REDIS_SENTINEL_NODES and REDIS_SENTINEL_GROUP_NAME when redisConnectorName is ioredis (documentserver.yaml#L28-L33), and values.yaml line 35 tells users to set ioredis for Sentinel. The entrypoint passes REDIS_CONNECTOR_NAME straight into redis.name (docker-entrypoint.sh line 155). server#46 throws Unsupported Redis connector for anything other than redis (redisConnection.js#L130).

So with the chart, Sentinel is either not configured (redis) or rejected (ioredis), and the space-separated fix only helps Cluster in practice. Either of these would close it:

• map ioredis to redis in the entrypoint, with a deprecation warning
• a companion Kubernetes-Docs change

2. The Sentinel stack doesn't fail over when the primary is stopped

After docker stop redis-primary, Docker's DNS stops resolving the name. Because of sentinel resolve-hostnames yes (docker-compose.redis-sentinel.yml lines 80-82), every Sentinel blocks for about 8s on Failed to resolve hostname 'redis-primary'. Each one then keeps re-entering TILT mode, where it takes no action. After 200s there was one +sdown, no +odown and no +switch-master, and the replica was still a replica. With docker pause redis-primary, failover completed in 32s.

The README says failover behaviour is the point of these stacks, so a stop-based test would look like a server#46 bug. Options:

• give redis-primary and redis-replica static IPs and monitor by IP
• document docker compose pause redis-primary or SENTINEL FAILOVER mymaster as the way to trigger a failover

sentinel down-after-milliseconds mymaster 5000 would also make failover tests quicker.

3. A second failover loses data

redis-primary has --requirepass but no --masterauth; only redis-replica has it. I failed over with pause, then unpaused redis-primary. It rejoined as a replica, logged MASTER aborted replication with an error: NOAUTH Authentication required. and stayed at master_link_status:down.

I then paused the new master. Sentinel still promoted redis-primary, and a key I wrote after the first failover was gone. Adding --masterauth redis-test-pass to redis-primary fixes it.

4. The entrypoint test cannot detect a broken NODE_CONFIG

docker-entrypoint.tests.sh greps the bash -x trace (lines 30-43). The expected strings already appear in that trace when the intermediate variables are assigned (REDIS_SENTINEL_NODES_ARRAY+=, EDITOR_STORAGE_CONFIG+=), before NODE_CONFIG is built. I tried two mutations of the entrypoint and the test printed PASS both times:

• removing '${EDITOR_STORAGE_CONFIG}', '${REDIS_CLUSTER}' and '${REDIS_SENTINEL_OPTIONS}' from the NODE_CONFIG template
• making NODE_CONFIG invalid JSON

The invalid-JSON case matters more than it looks. node-config 3.3.12 (the version server#46 pins) does not fail on a malformed NODE_CONFIG. It logs The $NODE_CONFIG environment variable is malformed JSON and continues with {}. So a broken template starts docservice on package defaults for DB, AMQP, JWT and Redis instead of failing.

The test also relies on the final exec failing. Inside the eo dev container, where the docservice binary exists, it would start DocService. Overriding exec lets the test check the real output. This passes on the current script and fails on both mutations:

env -i PATH="$PATH" COMPANY_NAME=euro-office JWT_ENABLED=false \
  EDITOR_DATA_STORAGE=editorDataRedis REDIS_SENTINEL_NODES='s1:26379 s2:26380' \
  bash -c 'exec() { printf "%s" "$NODE_CONFIG"; }; . "$0" docservice' "$ENTRYPOINT" \
| jq -e '.services.CoAuthoring.server.editorDataStorage == "editorDataRedis"
         and (.services.CoAuthoring.redis.optionsSentinel.sentinelRootNodes | length) == 2'

(The workflow hasn't run on this PR yet; it's waiting for approval.)

5. Node-list parsing has new silent failure modes

IFS=', ' read -ra (lines 46 and 65) handles spaces and commas. Compared with the previous declare -a X=($VAR), though:

• s1:26379<TAB>s2:26380 renders one node, { "host": "s1", "port": 26380 } (first host, last port)
• newline-separated lists keep only the first node, because read stops at the first newline
• a leading or doubled comma (,s1:26379, s1:26379,,s2:26380) produces an empty entry and invalid JSON, so the whole config is dropped (see 4)

This handles all of the above and is safe under set -e:

IFS=$', \t\n' read -r -d '' -a REDIS_SENTINEL_NODES_ALL <<< "$REDIS_SENTINEL_NODES" || true
# in the loop:
[[ -n "$node" ]] || continue

6. Sentinel + Cluster together is accepted and tested as valid, but server#46 rejects it

When both REDIS_SENTINEL_NODES and REDIS_CLUSTER_NODES are set, the entrypoint emits both optionsSentinel and optionsCluster (lines 163-164). server#46 throws Redis Cluster and Redis Sentinel options cannot be enabled together (redisConnection.js#L138). That happens during startup: editorData.connect() runs before listen() (DocsCoServer.js#L4316-L4322). The error is logged as editorData error, and DocService never starts listening.

The positive test case sets both (lines 16-17), so the suite treats an invalid configuration as correct. I'd suggest:

• fail fast in the entrypoint, as it already does for JWT_SECRET and EDITOR_*_STORAGE
• split the test into three cases: Sentinel only, Cluster only, and both set (expect rejection)

7. make redis-up TYPE=CLUSTER only works once

redis-cluster-init always runs redis-cli --cluster create (docker-compose.redis-cluster.yml lines 89-99). A second make redis-up TYPE=CLUSTER, or one after stop or a reboot, re-runs it against nodes that already form a cluster. It exits 1 with [ERR] Node redis-cluster-1:7000 is not empty, and because eo waits on service_completed_successfully, the stack doesn't come up. Skipping the create when cluster info already reports cluster_state:ok makes it re-runnable.

8. DocumentServer startup waits on a single Redis node

The standalone entrypoint waits for REDIS_SERVER_HOST:REDIS_SERVER_PORT with no timeout (build/scripts/standalone/entrypoint.sh line 168). The overlays set this to redis-primary:6379 for Sentinel and redis-cluster-1:7000 for Cluster, but server#46 doesn't connect through host/port in either mode. If eo starts or restarts while that node is down (the failover case), the container hangs on Waiting for redis-primary:6379... even though the topology is healthy. For Sentinel, pointing REDIS_SERVER_HOST/PORT at redis-sentinel-1:26379 avoids this.

9. The README checks don't show what docservice actually uses

The Sentinel checks are:

• optionsSentinel appears in printenv NODE_CONFIG
• /healthcheck returns true
• Sentinel has elected a master

All three also pass when docservice is running on editorDataMemory:

• the env var only shows what Compose injected
• editorDataMemory's healthCheck also returns true
• a malformed NODE_CONFIG is silently ignored (see 4)

The standalone check reads the injected NODE_CONFIG and local.json, and neither is the merged config docservice loads. The PR description calls these checks of the effective configuration, but they aren't. The MONITOR section is the only real proof. Making that the pass criterion would fix this, as would checking for keys under the Redis prefix (ds: by default) after opening a document.

10. Smaller items

• NODE_CONFIG is built by string interpolation, so other inputs also break it. These predate this PR, but because of the node-config behaviour in 4, each one silently drops the whole config:

  • a node without a port gives invalid JSON
  • a password containing " gives invalid JSON
  • a backslash in a password either breaks the JSON (\x) or silently changes the password (\b)
  • an IPv6 sentinel address gives valid JSON with the wrong host ("[fd00")

  Validating NODE_CONFIG before exec, or building it with a JSON tool, would remove this class of bug. Note that the orchestrated image has python3 but no jq.

• Merge order: server main has no editorDataRedis module. Until server#46 is merged and latest-dev is republished, every make redis-up target starts a docservice that cannot load its editor-data module. The README says so; it's worth holding the merge until then.

• REDIS_SENTINEL='[]' in the else branch (line 60) is never read.

• README lines 223-224 say the steps don't change the submodule pointer. After checking out server-pr-46, though, the superproject shows server (new commits), so git commit -a would commit the pointer. Worth a warning.

• The README's Sentinel MONITOR step hardcodes redis-primary, but after a failover the master is the former replica. Run SENTINEL get-master-addr-by-name mymaster first.

• README line 215 says the stack doesn't mount the checkout, but postgres bind-mounts ../server/schema/postgresql/createdb.sql (docker-compose.redis-base.yml line 39). The mount is redundant, since the standalone entrypoint's ensure_db_schema already applies the schema.

• The new workflow's checkout (lines 16-17) doesn't set persist-credentials: false. zizmor flags this as artipacked, and the other workflows in the repo set it.

Checked and fine:

• The pinned actions/checkout SHA is the real v7.0.1 and matches the other workflows.
• server#46 converts options.user / options.db to username / database.
• Dropping iooptions is safe because server main never read it.
• The new Makefile variables don't affect the main dev stack, and the ports and container names don't collide.
• Cluster creation with hostnames works, and the Sentinels discover each other with requirepass.

[renatoyamane]

Checked and fine:

Sorry, but I guess this information is useless.
Reviews should only inform things that need to be addressed and are blocking the PR

[MonaAghili]

Re-checked at 74937c0. Three new problems, the first of which blocks every orchestrated deployment, then one TLS point and the items from my last review that are still open.

New at 74937c0

1. Every orchestrated container now fails at startup: the image has no jq (reproduced)
The orchestrated entrypoint now calls jq unconditionally to build REDIS_OPTIONS (docker-entrypoint.sh#L61-L65), and the shared helper uses it for node parsing (redis-topology.sh#L10). The orchestrated image doesn't install jq: the ds-base package list has none (orchestrated.bake.Dockerfile#L18-L45), and this PR only adds the helper COPY. I ran the 74937c0 entrypoint inside ghcr.io/euro-office/cluster-docs:nightly with a plain configuration (no Sentinel or Cluster). In docservice, converter and adminpanel mode alike, it exits with 127 and line 65: jq: command not found. After dnf install jq in the same container, it produces valid config. The new workflow runs the test on ubuntu-latest, which has jq, so it can't catch this (orchestrated-entrypoint.yml#L14-L20).
Suggested fix: add jq to the ds-base dnf install list (or use python3, which the image already has), and run the entrypoint test inside the built image.

2. The server submodule now points at a server#46 commit that isn't merged anywhere
74937c0 moves server from abfa294 to c6a6b60a ("fix(server): harden Redis saved-state claim recovery"). That commit isn't on server main, and it dropped out of server#46 when that branch was rebased on Oct 3. Fetching it by SHA still works today, but merging this PR would point DocumentServer main at a server commit that was never merged. This is what the submodule warning in item 10 of my last review was about.
Suggested fix: reset server to the pointer on main and keep the README instructions for local testing.

3. CI hasn't run on this head
The branch conflicts with main in develop/README.md (changed on main on Oct 1), and GitHub doesn't start pull_request workflows while a PR has a merge conflict. So the image build, the entrypoint tests and zizmor haven't run on 74937c0. It needs a rebase first.

Redis TLS

Redis TLS can only be enabled by mounting a config file
The orchestrated entrypoint builds the Redis options from user, password and database only (docker-entrypoint.sh#L61-L65), and it replaces any NODE_CONFIG the operator sets: passing {"services":{"CoAuthoring":{"redis":{"options":{"socket":{"tls":true}}}}}} as NODE_CONFIG comes out as "options": {"db": "0"}. With a TLS-only Redis, the remaining option is mounting a local.json, for example through the chart's extraConf. I checked that a mounted local.json with socket.tls is merged with the entrypoint's config and connects to a TLS-only Redis. RabbitMQ already has AMQP_PROTO=amqps. A Redis equivalent would make TLS a normal setting, for example REDIS_SERVER_TLS=true setting socket.tls in options and optionsCluster.defaults, plus one for sentinelClientOptions.

Still open from my last review

• 1. Helm Sentinel: with the chart's Sentinel setting (REDIS_CONNECTOR_NAME=ioredis), the entrypoint still writes "name": "ioredis" (docker-entrypoint.sh#L143), which server#46 rejects with Unsupported Redis connector (connect.js#L44). Mapping ioredis to redis in the entrypoint would close it.
• 2. Sentinel failover on docker stop: sentinel resolve-hostnames yes is unchanged (docker-compose.redis-sentinel.yml#L87).
• 3. A second failover loses data: redis-primary still has --requirepass but no --masterauth (docker-compose.redis-sentinel.yml#L26-L46).
• 7. make redis-up TYPE=CLUSTER only works once: --cluster create still runs every time (docker-compose.redis-cluster.yml#L101).
• 8. Startup waits on a single Redis node: the new skip (entrypoint.sh#L173) only applies when REDIS_SENTINEL_* variables are set. The Sentinel overlay passes its topology through NODE_CONFIG and only sets TEST_REDIS_SENTINEL_*, so both overlays still wait on one node (redis-primary:6379 and redis-cluster-1:7000).
• 9, and the README items in 10: develop/README.md is unchanged, so the effective-config checks, the hardcoded redis-primary in the MONITOR step, and the postgres mount note still apply.
• 10. Workflow: the checkout in orchestrated-entrypoint.yml still doesn't set persist-credentials: false.

[nvanlaerebeke]

I didn't forget about this PR, I've fixed the jq that wasn't included in the base image and reverted the accidental commit for the server submodule reference.

I've also rebased on the latest version, there were a couple of conflicts that needed fixing.

In the mean time I'm testing the helm chart to make the required changes.
Previously I had a modified helm chart with a couple of changes to get it to deploy on my own registry, I've now created PR's for those changes as well, but those were separate issues not related to this PR.

Initially I'll test it with valkey standalone and cluster, those were very easy to deploy on my k3s instance, but the Valkey chart unfortunately doesn't yet support sentinel, think it's for their next chart release.
For sentinel I'll just run it locally for now.

[nvanlaerebeke]

Added the helm chart change in this PR:
Euro-Office/Kubernetes-Docs#11