fix(test-utils): share one network namespace across cluster nodes#3325
fix(test-utils): share one network namespace across cluster nodes#3325GiHoon1123 wants to merge 3 commits into
Conversation
Cluster node containers spawned with --network host aren't reachable from the real host on macOS/Windows Docker Desktop (only from the hidden VM), so the test process can't connect to them at all (redis#2358). Switching to -p port publishing alone doesn't work either: a Redis Cluster node can only announce one address for itself, and no single address is valid for both the host (needs the published port) and sibling nodes (need the bridge-internal address). Instead, put every node in a cluster group into the same Docker network namespace via --network container:<id>. The first node spawned publishes every port the group will use; every other node joins its namespace instead of getting its own. All nodes announce --cluster-announce-ip 127.0.0.1 with their own announce-port/ announce-bus-port. Since every node shares one namespace, 127.0.0.1:<port> now means the same thing to the host and to every sibling node. --network container:<id> is a standard Docker Engine feature, so this needs no OS branching -- same code path on macOS and Linux. Fixes the pre-existing `any` type on the totalNodes() helper while touching this file (was blocking lint on the whole file).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit c0c27f7. Configure here.
The bus port is only ever dialed by sibling nodes over loopback inside the shared network namespace, never by the host, so it never needed a -p mapping. Publishing it anyway was also unsafe: port + CLUSTER_BUS_PORT_OFFSET can exceed 65535 for high ports, which makes Docker reject the owner's `docker run` outright on busy hosts where reservePorts() yields a port above 55535.
Removing the bus port's `-p` mapping (previous commit) didn't fully close the gap it described: `busPort = port + CLUSTER_BUS_PORT_OFFSET` was still computed and passed to `--cluster-announce-bus-port` unconditionally, so a client port above 55535 still produces an out-of-range bus port -- just handed to redis-server instead of Docker. portIterator is a single generator shared by every server/sentinel/ cluster spawn in this file and only counts up, so on a long enough run it can hand out ports past that ceiling. reservePorts() now takes an optional maxPort and skips any port above it; the cluster spawner passes 65535 - CLUSTER_BUS_PORT_OFFSET so the announced bus port always stays a valid TCP port.
|
Hi @GiHoon1123, thanks for the detailed investigation! Two findings from review, though:
Given that, our current inclination is to resolve #2358 with a short contributor-docs note about the toggle rather than rework the spawn path: it fixes all three topologies at once, carries no regression risk for Linux CI, and adds no test-infra code to maintain. |
2 participants
Description
Related to #2358 ("Running test suits only works on Linux hosts") -- specifically the cluster portion. Not marking this as "Fixes" since Sentinel test spawning still uses
--network hostand is presumably still broken on macOS (out of scope here, separate follow-up needed).Context
--network hostdoesn't expose container ports to the real host on macOS/Windows Docker Desktop (only to the hidden VM), so the host test process can't connect to spawned nodes at all.This exact fix was actually already suggested in this issue back in 2023:
That rebuttal is very close, but the precise failure mode is slightly different: with plain
-p, sibling containers can still reach each other fine over the default bridge network (that never needed publishing) -- the actual problem is that a Redis Cluster node can only announce one address for itself, and no single address is valid for both the host (needs the published port) and sibling nodes (need the bridge-internal address). Whichever one a node announces, the other audience can't use it.Approach
Instead of finding one address that satisfies both audiences, this puts every node in a cluster group into the same Docker network namespace via
--network container:<id>, so there's only one address space to worry about:65535 - 10000so the announced bus port always stays a valid TCP port.--cluster-announce-ip 127.0.0.1with their own--cluster-announce-port/--cluster-announce-bus-port.Since every node now shares one namespace,
127.0.0.1:<port>means the same thing to the host and to every sibling node.--network container:<id>is a standard Docker Engine feature (not Mac-specific), so this doesn't need any OS branching -- same code path for macOS and Linux.Verified locally (macOS)
mainfirst, before writing any fix.CLUSTER MEET, checkedCLUSTER NODESon both sides) before touching the real spawning code.packages/client/lib/cluster/index.spec.tscluster suite: 34 passing, including resharding, PubSub slot-migration, and topology tests -- exercised by the existing suite rather than new dedicated unit tests for the spawning code itself.docker rm -fon owner + dependents in random order cleans up correctly with no errors.FUNCTION LOAD, genericClienttests, "before all" timeouts) reproduce identically on unmodifiedmaintoo -- pre-existing local Docker resource-contention flakiness, not introduced by this change.Out of scope / known gaps
spawnRedisServer/sentinel path) is untouched and likely still broken on macOS.I'd especially welcome feedback on the approach itself (network namespace sharing) given the history on this issue -- happy to adjust if there's a reason this doesn't fit.
Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Changes only test-utils Docker orchestration but reorders cluster bring-up and networking; regressions could show up as CI flakiness or broken cluster suites on Linux as well as macOS.
Overview
Redis cluster test Docker spawning no longer relies on per-shard
--network hostcontainers. It now reserves all node ports up front, starts an owner container that publishes those client ports, and attaches every other node with--network container:<owner>so host and siblings both use127.0.0.1with--cluster-announce-*.Cluster bootstrap is flattened: all masters and replicas are created together, slots are assigned per master, nodes
CLUSTER MEET, the suite waits forcluster_state:okbeforeCLUSTER REPLICATE, then waits untilCLUSTER SLOTSreflects the full master+replica count. Bus ports stay inside the shared namespace (not published) and client ports are capped soport + 10000stays valid.totalNodesnow uses a properClusterSlotsReplytype instead ofany.Reviewed by Cursor Bugbot for commit ad2b5bc. Bugbot is set up for automated code reviews on this repo. Configure here.